A lot of changes in v2. The distributor and redistributor saving and
restoring is left to the PSCI/firmware implementation after
discussions with ARM. This reduces the line changes by a lot and
removes now unneeded patches.
Patches are verified on an RK3399 platform with pending patches in the
ARM-Trusted-Firmware project.
Just a couple minor changes in v3 to formatting.
Fixed a false ITS wedged detection due to the cmd_write and creadr
offsets not matching up on reset in v4. Also minor formatting changes.
Got rid of additional device tree property with detecting if there are
collections stored in the ITS in v5. Made other minor changes.
v6: Fixed reinitialized collections to only happen when the collection
is stored in the ITS. Changed error handling to avoid undefined
behavior of the ITS.
v7: Fixed pr_errs to print out the physical rather than virtual base
address of the ITS and the error code. Updated the documentation.
Derek Basehore (3):
irqchip/gic-v3-its: add ability to save/restore ITS state
DT/arm,gic-v3-its: add reset-on-suspend property
irqchip/gic-v3-its: add ability to resend MAPC on resume
.../interrupt-controller/arm,gic-v3.txt | 5 +
drivers/irqchip/irq-gic-v3-its.c | 193 ++++++++++++++----
include/linux/irqchip/arm-gic-v3.h | 1 +
3 files changed, 161 insertions(+), 38 deletions(-)
--
2.16.2.395.g2e18187dfd-goog
This adds functionality to resend the MAPC command to an ITS node on
resume. If the ITS is powered down during suspend and the collections
are not backed by memory, the ITS will lose that state. This just sets
up the known state for the collections after the ITS is restored.
This is enabled via the reset-on-suspend flag in the DTS. It only runs
for collections stored in the ITS itself.
Signed-off-by: Derek Basehore <[email protected]>
Reviewed-by: Brian Norris <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 86 +++++++++++++++++-------------
include/linux/irqchip/arm-gic-v3.h | 1 +
2 files changed, 49 insertions(+), 38 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 926f76944a75..b8fc6930eb40 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1942,52 +1942,53 @@ static void its_cpu_init_lpis(void)
dsb(sy);
}
-static void its_cpu_init_collection(void)
+static void its_cpu_init_collection(struct its_node *its)
{
- struct its_node *its;
- int cpu;
-
- spin_lock(&its_lock);
- cpu = smp_processor_id();
-
- list_for_each_entry(its, &its_nodes, entry) {
- u64 target;
+ int cpu = smp_processor_id();
+ u64 target;
- /* avoid cross node collections and its mapping */
- if (its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_23144) {
- struct device_node *cpu_node;
+ /* avoid cross node collections and its mapping */
+ if (its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_23144) {
+ struct device_node *cpu_node;
- cpu_node = of_get_cpu_node(cpu, NULL);
- if (its->numa_node != NUMA_NO_NODE &&
- its->numa_node != of_node_to_nid(cpu_node))
- continue;
- }
+ cpu_node = of_get_cpu_node(cpu, NULL);
+ if (its->numa_node != NUMA_NO_NODE &&
+ its->numa_node != of_node_to_nid(cpu_node))
+ return;
+ }
+ /*
+ * We now have to bind each collection to its target
+ * redistributor.
+ */
+ if (gic_read_typer(its->base + GITS_TYPER) & GITS_TYPER_PTA) {
/*
- * We now have to bind each collection to its target
+ * This ITS wants the physical address of the
* redistributor.
*/
- if (gic_read_typer(its->base + GITS_TYPER) & GITS_TYPER_PTA) {
- /*
- * This ITS wants the physical address of the
- * redistributor.
- */
- target = gic_data_rdist()->phys_base;
- } else {
- /*
- * This ITS wants a linear CPU number.
- */
- target = gic_read_typer(gic_data_rdist_rd_base() + GICR_TYPER);
- target = GICR_TYPER_CPU_NUMBER(target) << 16;
- }
+ target = gic_data_rdist()->phys_base;
+ } else {
+ /* This ITS wants a linear CPU number. */
+ target = gic_read_typer(gic_data_rdist_rd_base() + GICR_TYPER);
+ target = GICR_TYPER_CPU_NUMBER(target) << 16;
+ }
- /* Perform collection mapping */
- its->collections[cpu].target_address = target;
- its->collections[cpu].col_id = cpu;
+ /* Perform collection mapping */
+ its->collections[cpu].target_address = target;
+ its->collections[cpu].col_id = cpu;
- its_send_mapc(its, &its->collections[cpu], 1);
- its_send_invall(its, &its->collections[cpu]);
- }
+ its_send_mapc(its, &its->collections[cpu], 1);
+ its_send_invall(its, &its->collections[cpu]);
+}
+
+static void its_cpu_init_collections(void)
+{
+ struct its_node *its;
+
+ spin_lock(&its_lock);
+
+ list_for_each_entry(its, &its_nodes, entry)
+ its_cpu_init_collection(its);
spin_unlock(&its_lock);
}
@@ -3135,6 +3136,15 @@ static void its_restore_enable(void)
its_write_baser(its, baser, baser->val);
}
writel_relaxed(its->ctlr_save, base + GITS_CTLR);
+
+ /*
+ * Reinit the collection if it's stored in the ITS. This is
+ * indicated by the col_id being less than the HWCOLLCNT.
+ * CID < HCC as specified in the GIC v3 Documentation.
+ */
+ if (its->collections[smp_processor_id()].col_id <
+ GITS_TYPER_HWCOLLCNT(gic_read_typer(base + GITS_TYPER)))
+ its_cpu_init_collection(its);
}
spin_unlock(&its_lock);
}
@@ -3401,7 +3411,7 @@ int its_cpu_init(void)
return -ENXIO;
}
its_cpu_init_lpis();
- its_cpu_init_collection();
+ its_cpu_init_collections();
}
return 0;
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index c00c4c33e432..c9c33b91a1f1 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -313,6 +313,7 @@
#define GITS_TYPER_DEVBITS(r) ((((r) >> GITS_TYPER_DEVBITS_SHIFT) & 0x1f) + 1)
#define GITS_TYPER_PTA (1UL << 19)
#define GITS_TYPER_HWCOLLCNT_SHIFT 24
+#define GITS_TYPER_HWCOLLCNT(r) (((r) >> GITS_TYPER_HWCOLLCNT_SHIFT) & 0xff)
#define GITS_TYPER_VMOVP (1ULL << 37)
#define GITS_IIDR_REV_SHIFT 12
--
2.16.2.395.g2e18187dfd-goog
This adds documentation for the new reset-on-suspend property. This
property enables saving and restoring the ITS for when it loses state
in system suspend.
Signed-off-by: Derek Basehore <[email protected]>
---
.../devicetree/bindings/interrupt-controller/arm,gic-v3.txt | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
index 0a57f2f4167d..dfd1b9b838cd 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
@@ -78,6 +78,11 @@ These nodes must have the following properties:
Optional:
- socionext,synquacer-pre-its: (u32, u32) tuple describing the untranslated
address and size of the pre-ITS window.
+- reset-on-suspend: Boolean property. Indicates that the ITS loses power
+ on suspend to memory (via PSCI_SYSTEM_SUSPEND). Registers and data within
+ the ITS will be the same as before probe (PSCI FW does not restore state).
+ Any state stored in external memory is maintained (such as collection
+ tables stored in external memory).
The main GIC node must contain the appropriate #address-cells,
#size-cells and ranges properties for the reg property of all ITS
--
2.16.2.395.g2e18187dfd-goog
Some platforms power off GIC logic in suspend, so we need to
save/restore state. The distributor and redistributor registers need
to be handled in platform code due to access permissions on those
registers, but the ITS registers can be restored in the kernel.
Signed-off-by: Derek Basehore <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 107 +++++++++++++++++++++++++++++++
1 file changed, 107 insertions(+)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 1d3056f53747..926f76944a75 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -33,6 +33,7 @@
#include <linux/of_platform.h>
#include <linux/percpu.h>
#include <linux/slab.h>
+#include <linux/syscore_ops.h>
#include <linux/irqchip.h>
#include <linux/irqchip/arm-gic-v3.h>
@@ -46,6 +47,7 @@
#define ITS_FLAGS_CMDQ_NEEDS_FLUSHING (1ULL << 0)
#define ITS_FLAGS_WORKAROUND_CAVIUM_22375 (1ULL << 1)
#define ITS_FLAGS_WORKAROUND_CAVIUM_23144 (1ULL << 2)
+#define ITS_FLAGS_SAVE_SUSPEND_STATE (1ULL << 3)
#define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING (1 << 0)
@@ -101,6 +103,8 @@ struct its_node {
struct its_collection *collections;
struct fwnode_handle *fwnode_handle;
u64 (*get_msi_base)(struct its_device *its_dev);
+ u64 cbaser_save;
+ u32 ctlr_save;
struct list_head its_device_list;
u64 flags;
unsigned long list_nr;
@@ -3042,6 +3046,104 @@ static void its_enable_quirks(struct its_node *its)
gic_enable_quirks(iidr, its_quirks, its);
}
+static int its_save_disable(void)
+{
+ struct its_node *its;
+ int err = 0;
+
+ spin_lock(&its_lock);
+ list_for_each_entry(its, &its_nodes, entry) {
+ void __iomem *base;
+
+ if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
+ continue;
+
+ base = its->base;
+ its->ctlr_save = readl_relaxed(base + GITS_CTLR);
+ err = its_force_quiescent(base);
+ if (err) {
+ pr_err("ITS@%pa: failed to quiesce: %d\n",
+ &its->phys_base, err);
+ writel_relaxed(its->ctlr_save, base + GITS_CTLR);
+ goto err;
+ }
+
+ its->cbaser_save = gits_read_cbaser(base + GITS_CBASER);
+ }
+
+err:
+ if (err) {
+ list_for_each_entry_continue_reverse(its, &its_nodes, entry) {
+ void __iomem *base;
+
+ if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
+ continue;
+
+ base = its->base;
+ writel_relaxed(its->ctlr_save, base + GITS_CTLR);
+ }
+ }
+ spin_unlock(&its_lock);
+
+ return err;
+}
+
+static void its_restore_enable(void)
+{
+ struct its_node *its;
+ int ret;
+
+ spin_lock(&its_lock);
+ list_for_each_entry(its, &its_nodes, entry) {
+ void __iomem *base;
+ int i;
+
+ if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
+ continue;
+
+ base = its->base;
+
+ /*
+ * Make sure that the ITS is disabled. If it fails to quiesce,
+ * don't restore it since writing to CBASER or BASER<n>
+ * registers is undefined according to the GIC v3 ITS
+ * Specification.
+ */
+ ret = its_force_quiescent(base);
+ if (ret) {
+ pr_err("ITS@%pa: failed to quiesce on resume: %d\n",
+ &its->phys_base, ret);
+ continue;
+ }
+
+ gits_write_cbaser(its->cbaser_save, base + GITS_CBASER);
+
+ /*
+ * Writing CBASER resets CREADR to 0, so make CWRITER and
+ * cmd_write line up with it.
+ */
+ its->cmd_write = its->cmd_base;
+ gits_write_cwriter(0, base + GITS_CWRITER);
+
+ /* Restore GITS_BASER from the value cache. */
+ for (i = 0; i < GITS_BASER_NR_REGS; i++) {
+ struct its_baser *baser = &its->tables[i];
+
+ if (!(baser->val & GITS_BASER_VALID))
+ continue;
+
+ its_write_baser(its, baser, baser->val);
+ }
+ writel_relaxed(its->ctlr_save, base + GITS_CTLR);
+ }
+ spin_unlock(&its_lock);
+}
+
+static struct syscore_ops its_syscore_ops = {
+ .suspend = its_save_disable,
+ .resume = its_restore_enable,
+};
+
static int its_init_domain(struct fwnode_handle *handle, struct its_node *its)
{
struct irq_domain *inner_domain;
@@ -3261,6 +3363,9 @@ static int __init its_probe_one(struct resource *res,
ctlr |= GITS_CTLR_ImDe;
writel_relaxed(ctlr, its->base + GITS_CTLR);
+ if (fwnode_property_present(handle, "reset-on-suspend"))
+ its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
+
err = its_init_domain(handle, its);
if (err)
goto out_free_tables;
@@ -3517,5 +3622,7 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
}
}
+ register_syscore_ops(&its_syscore_ops);
+
return 0;
}
--
2.16.2.395.g2e18187dfd-goog
On Wed, Feb 28, 2018 at 09:48:19PM -0800, Derek Basehore wrote:
> This adds documentation for the new reset-on-suspend property. This
> property enables saving and restoring the ITS for when it loses state
> in system suspend.
>
> Signed-off-by: Derek Basehore <[email protected]>
> ---
> .../devicetree/bindings/interrupt-controller/arm,gic-v3.txt | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> index 0a57f2f4167d..dfd1b9b838cd 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> @@ -78,6 +78,11 @@ These nodes must have the following properties:
> Optional:
> - socionext,synquacer-pre-its: (u32, u32) tuple describing the untranslated
> address and size of the pre-ITS window.
> +- reset-on-suspend: Boolean property. Indicates that the ITS loses power
> + on suspend to memory (via PSCI_SYSTEM_SUSPEND). Registers and data within
> + the ITS will be the same as before probe (PSCI FW does not restore state).
What does 'before probe' mean? Perhaps 'at reset'?
Is the state well-defined, or might there be some bits that differ from
boot-to-boot?
Otherwise, this generally looks ok.
> + Any state stored in external memory is maintained (such as collection
> + tables stored in external memory).
This a property of PSCI_SYSTEM_SUSPEND not corrupting memory state, so
this need not be stated here. This can go.
Thanks,
Mark.
On Wed, Feb 28, 2018 at 09:48:18PM -0800, Derek Basehore wrote:
> Some platforms power off GIC logic in suspend, so we need to
> save/restore state. The distributor and redistributor registers need
> to be handled in platform code due to access permissions on those
> registers, but the ITS registers can be restored in the kernel.
>
> Signed-off-by: Derek Basehore <[email protected]>
How much state do we have to save/restore?
Given we can apparently read all this state, couldn't we *always* save
the state, then upon resume detect if the state has been lost, restoring
it if so?
That way, we don't need a property in FW tables for DT or ACPI.
[...]
> @@ -3261,6 +3363,9 @@ static int __init its_probe_one(struct resource *res,
> ctlr |= GITS_CTLR_ImDe;
> writel_relaxed(ctlr, its->base + GITS_CTLR);
>
> + if (fwnode_property_present(handle, "reset-on-suspend"))
> + its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
Does this allow this property on an ACPI system?
If we need this on ACPI, we need a spec update to handle this properly,
and shouldn't use device properties like this.
Thanks,
Mark.
Hi Mark,
On 01/03/18 11:41, Mark Rutland wrote:
> On Wed, Feb 28, 2018 at 09:48:18PM -0800, Derek Basehore wrote:
>> Some platforms power off GIC logic in suspend, so we need to
>> save/restore state. The distributor and redistributor registers need
>> to be handled in platform code due to access permissions on those
>> registers, but the ITS registers can be restored in the kernel.
>>
>> Signed-off-by: Derek Basehore <[email protected]>
>
> How much state do we have to save/restore?
>
> Given we can apparently read all this state, couldn't we *always* save
> the state, then upon resume detect if the state has been lost, restoring
> it if so?
>
> That way, we don't need a property in FW tables for DT or ACPI.
That's a good point. I guess that we could just compare the saved
GITS_CTLR register and restore the full state only if the ITS comes back
as disabled.
I'm just a bit worried that it makes it an implicit convention between
kernel an FW, which could change in funny ways. Importantly, the PSCI
spec says states FW should restore *the whole state*. Obviously, it
cannot to that on HW that doesn't allow you to read out the state, hence
the DT flag that outlines the departure from the expected behaviour.
I'm happy to go either way, but then I have the feeling that we should
go back to quirking it on the actual implementation (GIC500 in this
case) if we're to from the property.
>
> [...]
>
>> @@ -3261,6 +3363,9 @@ static int __init its_probe_one(struct resource *res,
>> ctlr |= GITS_CTLR_ImDe;
>> writel_relaxed(ctlr, its->base + GITS_CTLR);
>>
>> + if (fwnode_property_present(handle, "reset-on-suspend"))
>> + its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
>
> Does this allow this property on an ACPI system?
>
> If we need this on ACPI, we need a spec update to handle this properly,
> and shouldn't use device properties like this.
Well spotted. I guess that dropping the property would fix that
altogether, assuming we feel that the above is safe enough.
Thoughts?
M.
--
Jazz is not dead. It just smells funny...
On Thu, Mar 1, 2018 at 4:29 AM, Marc Zyngier <[email protected]> wrote:
> Hi Mark,
>
> On 01/03/18 11:41, Mark Rutland wrote:
>> On Wed, Feb 28, 2018 at 09:48:18PM -0800, Derek Basehore wrote:
>>> Some platforms power off GIC logic in suspend, so we need to
>>> save/restore state. The distributor and redistributor registers need
>>> to be handled in platform code due to access permissions on those
>>> registers, but the ITS registers can be restored in the kernel.
>>>
>>> Signed-off-by: Derek Basehore <[email protected]>
>>
>> How much state do we have to save/restore?
>>
>> Given we can apparently read all this state, couldn't we *always* save
>> the state, then upon resume detect if the state has been lost, restoring
>> it if so?
>>
>> That way, we don't need a property in FW tables for DT or ACPI.
>
> That's a good point. I guess that we could just compare the saved
> GITS_CTLR register and restore the full state only if the ITS comes back
> as disabled.
>
> I'm just a bit worried that it makes it an implicit convention between
> kernel an FW, which could change in funny ways. Importantly, the PSCI
> spec says states FW should restore *the whole state*. Obviously, it
> cannot to that on HW that doesn't allow you to read out the state, hence
> the DT flag that outlines the departure from the expected behaviour.
>
> I'm happy to go either way, but then I have the feeling that we should
> go back to quirking it on the actual implementation (GIC500 in this
> case) if we're to from the property.
>
>>
>> [...]
>>
>>> @@ -3261,6 +3363,9 @@ static int __init its_probe_one(struct resource *res,
>>> ctlr |= GITS_CTLR_ImDe;
>>> writel_relaxed(ctlr, its->base + GITS_CTLR);
>>>
>>> + if (fwnode_property_present(handle, "reset-on-suspend"))
>>> + its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
>>
>> Does this allow this property on an ACPI system?
>>
>> If we need this on ACPI, we need a spec update to handle this properly,
>> and shouldn't use device properties like this.
>
> Well spotted. I guess that dropping the property would fix that
> altogether, assuming we feel that the above is safe enough.
>
> Thoughts?
I'm fine changing it to get rid of the devicetree property.
What's the reason for quirking the behavior though? It's not that much
code + data and nothing else relies on the state of the ITS getting
disabled across suspend/resume. Even if something did, we'd have to
resolve it with this feature anyways.
>
> M.
> --
> Jazz is not dead. It just smells funny...
On 02/03/18 02:08, dbasehore . wrote:
> On Thu, Mar 1, 2018 at 4:29 AM, Marc Zyngier <[email protected]> wrote:
>> Hi Mark,
>>
>> On 01/03/18 11:41, Mark Rutland wrote:
>>> On Wed, Feb 28, 2018 at 09:48:18PM -0800, Derek Basehore wrote:
>>>> Some platforms power off GIC logic in suspend, so we need to
>>>> save/restore state. The distributor and redistributor registers need
>>>> to be handled in platform code due to access permissions on those
>>>> registers, but the ITS registers can be restored in the kernel.
>>>>
>>>> Signed-off-by: Derek Basehore <[email protected]>
>>>
>>> How much state do we have to save/restore?
>>>
>>> Given we can apparently read all this state, couldn't we *always* save
>>> the state, then upon resume detect if the state has been lost, restoring
>>> it if so?
>>>
>>> That way, we don't need a property in FW tables for DT or ACPI.
>>
>> That's a good point. I guess that we could just compare the saved
>> GITS_CTLR register and restore the full state only if the ITS comes back
>> as disabled.
>>
>> I'm just a bit worried that it makes it an implicit convention between
>> kernel an FW, which could change in funny ways. Importantly, the PSCI
>> spec says states FW should restore *the whole state*. Obviously, it
>> cannot to that on HW that doesn't allow you to read out the state, hence
>> the DT flag that outlines the departure from the expected behaviour.
>>
>> I'm happy to go either way, but then I have the feeling that we should
>> go back to quirking it on the actual implementation (GIC500 in this
>> case) if we're to from the property.
>>
>>>
>>> [...]
>>>
>>>> @@ -3261,6 +3363,9 @@ static int __init its_probe_one(struct resource *res,
>>>> ctlr |= GITS_CTLR_ImDe;
>>>> writel_relaxed(ctlr, its->base + GITS_CTLR);
>>>>
>>>> + if (fwnode_property_present(handle, "reset-on-suspend"))
>>>> + its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
>>>
>>> Does this allow this property on an ACPI system?
>>>
>>> If we need this on ACPI, we need a spec update to handle this properly,
>>> and shouldn't use device properties like this.
>>
>> Well spotted. I guess that dropping the property would fix that
>> altogether, assuming we feel that the above is safe enough.
>>
>> Thoughts?
>
> I'm fine changing it to get rid of the devicetree property.
>
> What's the reason for quirking the behavior though? It's not that much
> code + data and nothing else relies on the state of the ITS getting
> disabled across suspend/resume. Even if something did, we'd have to
> resolve it with this feature anyways.
The reason we do this is to cope with GIC500 having the collection state
in registers instead of memory. If we didn't have this extraordinary
misfeature, FW could do a full save/restore of the ITS, and we wouldn't
have to do anything (which is what the driver currently expects).
A middle ground approach is to limit the feature to systems where
GITS_TYPER.HCC is non-zero instead of limiting it to GIC500. Pretty easy
to fix. This should have the same effect, as GIC500 is the only
implementation I'm aware of with HCC!=0.
Given that we're already at -rc5 and that I'd like to queue things for
4.17, I've made this change myself and queued patches 1 and 3 here[1].
Can you please have a look at let me know if that works for you?
Thanks,
M.
[1] git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
irq/irqchip-next
--
Jazz is not dead. It just smells funny...
On Wed, Mar 14, 2018 at 3:22 AM, Marc Zyngier <[email protected]> wrote:
> On 02/03/18 02:08, dbasehore . wrote:
>> On Thu, Mar 1, 2018 at 4:29 AM, Marc Zyngier <[email protected]> wrote:
>>> Hi Mark,
>>>
>>> On 01/03/18 11:41, Mark Rutland wrote:
>>>> On Wed, Feb 28, 2018 at 09:48:18PM -0800, Derek Basehore wrote:
>>>>> Some platforms power off GIC logic in suspend, so we need to
>>>>> save/restore state. The distributor and redistributor registers need
>>>>> to be handled in platform code due to access permissions on those
>>>>> registers, but the ITS registers can be restored in the kernel.
>>>>>
>>>>> Signed-off-by: Derek Basehore <[email protected]>
>>>>
>>>> How much state do we have to save/restore?
>>>>
>>>> Given we can apparently read all this state, couldn't we *always* save
>>>> the state, then upon resume detect if the state has been lost, restoring
>>>> it if so?
>>>>
>>>> That way, we don't need a property in FW tables for DT or ACPI.
>>>
>>> That's a good point. I guess that we could just compare the saved
>>> GITS_CTLR register and restore the full state only if the ITS comes back
>>> as disabled.
>>>
>>> I'm just a bit worried that it makes it an implicit convention between
>>> kernel an FW, which could change in funny ways. Importantly, the PSCI
>>> spec says states FW should restore *the whole state*. Obviously, it
>>> cannot to that on HW that doesn't allow you to read out the state, hence
>>> the DT flag that outlines the departure from the expected behaviour.
>>>
>>> I'm happy to go either way, but then I have the feeling that we should
>>> go back to quirking it on the actual implementation (GIC500 in this
>>> case) if we're to from the property.
>>>
>>>>
>>>> [...]
>>>>
>>>>> @@ -3261,6 +3363,9 @@ static int __init its_probe_one(struct resource *res,
>>>>> ctlr |= GITS_CTLR_ImDe;
>>>>> writel_relaxed(ctlr, its->base + GITS_CTLR);
>>>>>
>>>>> + if (fwnode_property_present(handle, "reset-on-suspend"))
>>>>> + its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
>>>>
>>>> Does this allow this property on an ACPI system?
>>>>
>>>> If we need this on ACPI, we need a spec update to handle this properly,
>>>> and shouldn't use device properties like this.
>>>
>>> Well spotted. I guess that dropping the property would fix that
>>> altogether, assuming we feel that the above is safe enough.
>>>
>>> Thoughts?
>>
>> I'm fine changing it to get rid of the devicetree property.
>>
>> What's the reason for quirking the behavior though? It's not that much
>> code + data and nothing else relies on the state of the ITS getting
>> disabled across suspend/resume. Even if something did, we'd have to
>> resolve it with this feature anyways.
>
> The reason we do this is to cope with GIC500 having the collection state
> in registers instead of memory. If we didn't have this extraordinary
> misfeature, FW could do a full save/restore of the ITS, and we wouldn't
> have to do anything (which is what the driver currently expects).
>
> A middle ground approach is to limit the feature to systems where
> GITS_TYPER.HCC is non-zero instead of limiting it to GIC500. Pretty easy
> to fix. This should have the same effect, as GIC500 is the only
> implementation I'm aware of with HCC!=0.
>
> Given that we're already at -rc5 and that I'd like to queue things for
> 4.17, I've made this change myself and queued patches 1 and 3 here[1].
>
> Can you please have a look at let me know if that works for you?
>
Assuming that your fine with only having the GIC500 implementations
that have HCC as non-zero getting ITS registers restored in the
kernel. As far as I can tell, this can happen in firmware for all
implementations. It's only the code to resend that MAPC on resume that
needs to be in the kernel.
> Thanks,
>
> M.
>
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
> irq/irqchip-next
> --
> Jazz is not dead. It just smells funny...
On Wed, Mar 14, 2018 at 2:44 PM, dbasehore . <[email protected]> wrote:
> On Wed, Mar 14, 2018 at 3:22 AM, Marc Zyngier <[email protected]> wrote:
>> On 02/03/18 02:08, dbasehore . wrote:
>>> On Thu, Mar 1, 2018 at 4:29 AM, Marc Zyngier <[email protected]> wrote:
>>>> Hi Mark,
>>>>
>>>> On 01/03/18 11:41, Mark Rutland wrote:
>>>>> On Wed, Feb 28, 2018 at 09:48:18PM -0800, Derek Basehore wrote:
>>>>>> Some platforms power off GIC logic in suspend, so we need to
>>>>>> save/restore state. The distributor and redistributor registers need
>>>>>> to be handled in platform code due to access permissions on those
>>>>>> registers, but the ITS registers can be restored in the kernel.
>>>>>>
>>>>>> Signed-off-by: Derek Basehore <[email protected]>
>>>>>
>>>>> How much state do we have to save/restore?
>>>>>
>>>>> Given we can apparently read all this state, couldn't we *always* save
>>>>> the state, then upon resume detect if the state has been lost, restoring
>>>>> it if so?
>>>>>
>>>>> That way, we don't need a property in FW tables for DT or ACPI.
>>>>
>>>> That's a good point. I guess that we could just compare the saved
>>>> GITS_CTLR register and restore the full state only if the ITS comes back
>>>> as disabled.
>>>>
>>>> I'm just a bit worried that it makes it an implicit convention between
>>>> kernel an FW, which could change in funny ways. Importantly, the PSCI
>>>> spec says states FW should restore *the whole state*. Obviously, it
>>>> cannot to that on HW that doesn't allow you to read out the state, hence
>>>> the DT flag that outlines the departure from the expected behaviour.
>>>>
>>>> I'm happy to go either way, but then I have the feeling that we should
>>>> go back to quirking it on the actual implementation (GIC500 in this
>>>> case) if we're to from the property.
>>>>
>>>>>
>>>>> [...]
>>>>>
>>>>>> @@ -3261,6 +3363,9 @@ static int __init its_probe_one(struct resource *res,
>>>>>> ctlr |= GITS_CTLR_ImDe;
>>>>>> writel_relaxed(ctlr, its->base + GITS_CTLR);
>>>>>>
>>>>>> + if (fwnode_property_present(handle, "reset-on-suspend"))
>>>>>> + its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
>>>>>
>>>>> Does this allow this property on an ACPI system?
>>>>>
>>>>> If we need this on ACPI, we need a spec update to handle this properly,
>>>>> and shouldn't use device properties like this.
>>>>
>>>> Well spotted. I guess that dropping the property would fix that
>>>> altogether, assuming we feel that the above is safe enough.
>>>>
>>>> Thoughts?
>>>
>>> I'm fine changing it to get rid of the devicetree property.
>>>
>>> What's the reason for quirking the behavior though? It's not that much
>>> code + data and nothing else relies on the state of the ITS getting
>>> disabled across suspend/resume. Even if something did, we'd have to
>>> resolve it with this feature anyways.
>>
>> The reason we do this is to cope with GIC500 having the collection state
>> in registers instead of memory. If we didn't have this extraordinary
>> misfeature, FW could do a full save/restore of the ITS, and we wouldn't
>> have to do anything (which is what the driver currently expects).
>>
>> A middle ground approach is to limit the feature to systems where
>> GITS_TYPER.HCC is non-zero instead of limiting it to GIC500. Pretty easy
>> to fix. This should have the same effect, as GIC500 is the only
>> implementation I'm aware of with HCC!=0.
>>
>> Given that we're already at -rc5 and that I'd like to queue things for
>> 4.17, I've made this change myself and queued patches 1 and 3 here[1].
>>
>> Can you please have a look at let me know if that works for you?
>>
>
> Assuming that your fine with only having the GIC500 implementations
> that have HCC as non-zero getting ITS registers restored in the
> kernel. As far as I can tell, this can happen in firmware for all
> implementations. It's only the code to resend that MAPC on resume that
> needs to be in the kernel.
Actually, I guess we can't tell if we need to resend MAPC on resume if
the firmware restores the state. Assuming it's a problem to just
resend MAPC all the time, only restoring the ITS registers in the
kernel when HCC != 0 makes sense.
>
>> Thanks,
>>
>> M.
>>
>> [1] git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
>> irq/irqchip-next
>> --
>> Jazz is not dead. It just smells funny...
On 14/03/18 21:44, dbasehore . wrote:
> On Wed, Mar 14, 2018 at 3:22 AM, Marc Zyngier <[email protected]> wrote:
>> On 02/03/18 02:08, dbasehore . wrote:
>>> On Thu, Mar 1, 2018 at 4:29 AM, Marc Zyngier <[email protected]> wrote:
>>>> Hi Mark,
>>>>
>>>> On 01/03/18 11:41, Mark Rutland wrote:
>>>>> On Wed, Feb 28, 2018 at 09:48:18PM -0800, Derek Basehore wrote:
>>>>>> Some platforms power off GIC logic in suspend, so we need to
>>>>>> save/restore state. The distributor and redistributor registers need
>>>>>> to be handled in platform code due to access permissions on those
>>>>>> registers, but the ITS registers can be restored in the kernel.
>>>>>>
>>>>>> Signed-off-by: Derek Basehore <[email protected]>
>>>>>
>>>>> How much state do we have to save/restore?
>>>>>
>>>>> Given we can apparently read all this state, couldn't we *always* save
>>>>> the state, then upon resume detect if the state has been lost, restoring
>>>>> it if so?
>>>>>
>>>>> That way, we don't need a property in FW tables for DT or ACPI.
>>>>
>>>> That's a good point. I guess that we could just compare the saved
>>>> GITS_CTLR register and restore the full state only if the ITS comes back
>>>> as disabled.
>>>>
>>>> I'm just a bit worried that it makes it an implicit convention between
>>>> kernel an FW, which could change in funny ways. Importantly, the PSCI
>>>> spec says states FW should restore *the whole state*. Obviously, it
>>>> cannot to that on HW that doesn't allow you to read out the state, hence
>>>> the DT flag that outlines the departure from the expected behaviour.
>>>>
>>>> I'm happy to go either way, but then I have the feeling that we should
>>>> go back to quirking it on the actual implementation (GIC500 in this
>>>> case) if we're to from the property.
>>>>
>>>>>
>>>>> [...]
>>>>>
>>>>>> @@ -3261,6 +3363,9 @@ static int __init its_probe_one(struct resource *res,
>>>>>> ctlr |= GITS_CTLR_ImDe;
>>>>>> writel_relaxed(ctlr, its->base + GITS_CTLR);
>>>>>>
>>>>>> + if (fwnode_property_present(handle, "reset-on-suspend"))
>>>>>> + its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
>>>>>
>>>>> Does this allow this property on an ACPI system?
>>>>>
>>>>> If we need this on ACPI, we need a spec update to handle this properly,
>>>>> and shouldn't use device properties like this.
>>>>
>>>> Well spotted. I guess that dropping the property would fix that
>>>> altogether, assuming we feel that the above is safe enough.
>>>>
>>>> Thoughts?
>>>
>>> I'm fine changing it to get rid of the devicetree property.
>>>
>>> What's the reason for quirking the behavior though? It's not that much
>>> code + data and nothing else relies on the state of the ITS getting
>>> disabled across suspend/resume. Even if something did, we'd have to
>>> resolve it with this feature anyways.
>>
>> The reason we do this is to cope with GIC500 having the collection state
>> in registers instead of memory. If we didn't have this extraordinary
>> misfeature, FW could do a full save/restore of the ITS, and we wouldn't
>> have to do anything (which is what the driver currently expects).
>>
>> A middle ground approach is to limit the feature to systems where
>> GITS_TYPER.HCC is non-zero instead of limiting it to GIC500. Pretty easy
>> to fix. This should have the same effect, as GIC500 is the only
>> implementation I'm aware of with HCC!=0.
>>
>> Given that we're already at -rc5 and that I'd like to queue things for
>> 4.17, I've made this change myself and queued patches 1 and 3 here[1].
>>
>> Can you please have a look at let me know if that works for you?
>>
>
> Assuming that your fine with only having the GIC500 implementations
> that have HCC as non-zero getting ITS registers restored in the
> kernel. As far as I can tell, this can happen in firmware for all
> implementations. It's only the code to resend that MAPC on resume that
> needs to be in the kernel.
We really do not want to be in the business of doing partial
save/restore, with "à la carte" implementations. That be unmaintainable.
If a FW implementation doesn't fully save/restore the ITS when HCC==0,
then it is buggy, and it should be fixed.
Thanks,
M.
--
Jazz is not dead. It just smells funny...