[PATCH v5 0/5] GICv3 Save and Restore

[PATCH v5 0/5] GICv3 Save and Restore

[Derek Basehore]

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.

Derek Basehore (4):
  cpu_pm: add syscore_suspend error handling
  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

 .../bindings/interrupt-controller/arm,gic-v3.txt   |   3 +
 drivers/irqchip/irq-gic-v3-its.c                   | 179 ++++++++++++++++-----
 include/linux/irqchip/arm-gic-v3.h                 |   1 +
 kernel/cpu_pm.c                                    |   3 +
 4 files changed, 148 insertions(+), 38 deletions(-)

-- 
2.16.0.rc1.238.g530d649a79-goog

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 1/4] cpu_pm: add syscore_suspend error handling

[Derek Basehore]

If cpu_cluster_pm_enter() fails, cpu_pm_exit() should be called. This
will put the CPU in the correct state to resume from the failure.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 kernel/cpu_pm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
index 67b02e138a47..03bcc0751a51 100644
--- a/kernel/cpu_pm.c
+++ b/kernel/cpu_pm.c
@@ -186,6 +186,9 @@ static int cpu_pm_suspend(void)
 		return ret;
 
 	ret = cpu_cluster_pm_enter();
+	if (ret)
+		cpu_pm_exit();
+
 	return ret;
 }
 
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH v5 1/4] cpu_pm: add syscore_suspend error handling

[Marc Zyngier]

On 07/02/18 01:41, Derek Basehore wrote:
> If cpu_cluster_pm_enter() fails, cpu_pm_exit() should be called. This
> will put the CPU in the correct state to resume from the failure.
> 
> Signed-off-by: Derek Basehore <dbasehore-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  kernel/cpu_pm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
> index 67b02e138a47..03bcc0751a51 100644
> --- a/kernel/cpu_pm.c
> +++ b/kernel/cpu_pm.c
> @@ -186,6 +186,9 @@ static int cpu_pm_suspend(void)
>  		return ret;
>  
>  	ret = cpu_cluster_pm_enter();
> +	if (ret)
> +		cpu_pm_exit();
> +
>  	return ret;
>  }
>  
> 

It is unclear to me why we need this patch as part of the ITS series. I
probably fixes something for you, but I don't see the connection with
the other patches.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/4] cpu_pm: add syscore_suspend error handling

[Brian Norris]

Hi Marc,

On Wed, Feb 07, 2018 at 08:57:27AM +0000, Marc Zyngier wrote:
> On 07/02/18 01:41, Derek Basehore wrote:
> > If cpu_cluster_pm_enter() fails, cpu_pm_exit() should be called. This
> > will put the CPU in the correct state to resume from the failure.
> > 
> > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > ---
> >  kernel/cpu_pm.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
> > index 67b02e138a47..03bcc0751a51 100644
> > --- a/kernel/cpu_pm.c
> > +++ b/kernel/cpu_pm.c
> > @@ -186,6 +186,9 @@ static int cpu_pm_suspend(void)
> >  		return ret;
> >  
> >  	ret = cpu_cluster_pm_enter();
> > +	if (ret)
> > +		cpu_pm_exit();
> > +
> >  	return ret;
> >  }
> >  
> > 
> 
> It is unclear to me why we need this patch as part of the ITS series. I
> probably fixes something for you, but I don't see the connection with
> the other patches.

Ths bug was noticed (by inspection) along with earlier versions of this
series, when Derek was still adding new cpu_pm callbacks, and new
failure modes within the existing callbacks. It's a proper fix to my
knowledge, but I believe it no longer has any particular relevance to
this series, since we're not really touching cpu_pm in this series any
more.

Brian

Re: [PATCH v5 1/4] cpu_pm: add syscore_suspend error handling

[Marc Zyngier]

On Wed, 07 Feb 2018 22:01:56 +0000,
Brian Norris wrote:

Hi Brian,

> Hi Marc,
> 
> On Wed, Feb 07, 2018 at 08:57:27AM +0000, Marc Zyngier wrote:
> > On 07/02/18 01:41, Derek Basehore wrote:
> > > If cpu_cluster_pm_enter() fails, cpu_pm_exit() should be called. This
> > > will put the CPU in the correct state to resume from the failure.
> > > 
> > > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > > ---
> > >  kernel/cpu_pm.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c
> > > index 67b02e138a47..03bcc0751a51 100644
> > > --- a/kernel/cpu_pm.c
> > > +++ b/kernel/cpu_pm.c
> > > @@ -186,6 +186,9 @@ static int cpu_pm_suspend(void)
> > >  		return ret;
> > >  
> > >  	ret = cpu_cluster_pm_enter();
> > > +	if (ret)
> > > +		cpu_pm_exit();
> > > +
> > >  	return ret;
> > >  }
> > >  
> > > 
> > 
> > It is unclear to me why we need this patch as part of the ITS series. I
> > probably fixes something for you, but I don't see the connection with
> > the other patches.
> 
> Ths bug was noticed (by inspection) along with earlier versions of this
> series, when Derek was still adding new cpu_pm callbacks, and new
> failure modes within the existing callbacks. It's a proper fix to my
> knowledge, but I believe it no longer has any particular relevance to
> this series, since we're not really touching cpu_pm in this series any
> more.

I don't doubt that this is a proper fix, but it has a better chance of
being noticed on its own, rather than buried together with a now
unrelated series.

I'd suggest that when you or Derek respin this series, you don't
include this patch, but instead post it on its own for review.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

[PATCH v5 2/4] irqchip/gic-v3-its: add ability to save/restore ITS state

[Derek Basehore]

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 <dbasehore@chromium.org>
---
 drivers/irqchip/irq-gic-v3-its.c | 99 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 06f025fd5726..5e63635e2a7b 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,96 @@ 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 failed to quiesce\n");
+			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;
+
+	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. There's not much we can
+		 * do if this fails.
+		 */
+		if (its_force_quiescent(base))
+			pr_err("ITS failed to quiesce on resume\n");
+
+		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 +3355,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;
@@ -3515,5 +3612,7 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
 		}
 	}
 
+	register_syscore_ops(&its_syscore_ops);
+
 	return 0;
 }
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH v5 2/4] irqchip/gic-v3-its: add ability to save/restore ITS state

[Marc Zyngier]

On 07/02/18 01:41, 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 <dbasehore@chromium.org>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 99 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 99 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 06f025fd5726..5e63635e2a7b 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,96 @@ 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 failed to quiesce\n");
> +			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;
> +
> +	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. There's not much we can
> +		 * do if this fails.
> +		 */
> +		if (its_force_quiescent(base))
> +			pr_err("ITS failed to quiesce on resume\n");

One thing we can do though is to abort the resume of this ITS, because
"When GITS_CTLR.Enabled == 1 or GITS_CTLR.Quiescent == 0, writing this
register is UNPREDICTABLE." (where "this register" is any of the
GITS_BASER<n>).

Printing the physical base address of the failing ITS can also help
people to debug their HW (my toy platform has 8 of them...).

> +
> +		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 +3355,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;
> @@ -3515,5 +3612,7 @@ int __init its_init(struct fwnode_handle *handle, struct rdists *rdists,
>  		}
>  	}
>  
> +	register_syscore_ops(&its_syscore_ops);
> +
>  	return 0;
>  }
> 

Otherwise starting to look OK.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v5 3/4] DT/arm,gic-v3-its: add reset-on-suspend property

[Derek Basehore]

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 <dbasehore@chromium.org>
---
 Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt | 3 +++
 1 file changed, 3 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..a470147d4f14 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
@@ -78,6 +78,9 @@ 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 state is
+  reset on suspend. The state is then saved on suspend and restored on
+  resume.
 
 The main GIC node must contain the appropriate #address-cells,
 #size-cells and ranges properties for the reg property of all ITS
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH v5 3/4] DT/arm,gic-v3-its: add reset-on-suspend property

[Marc Zyngier]

On 07/02/18 01:41, 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 <dbasehore@chromium.org>
> ---
>  Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt | 3 +++
>  1 file changed, 3 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..a470147d4f14 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> @@ -78,6 +78,9 @@ 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 state is
> +  reset on suspend. The state is then saved on suspend and restored on
> +  resume.

By whom? It is important to be clear about the respective
responsibilities, as this forms a binding contract between firmware and OS.

Mark: Can you have a look at how to formulate this? I'm not sure we have
other instances of a non-architected behaviour involving FW
participation, aside from PSCI.

>  
>  The main GIC node must contain the appropriate #address-cells,
>  #size-cells and ranges properties for the reg property of all ITS
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v5 3/4] DT/arm,gic-v3-its: add reset-on-suspend property

[dbasehore .]

On Wed, Feb 7, 2018 at 1:21 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> On 07/02/18 01:41, 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 <dbasehore-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt | 3 +++
>>  1 file changed, 3 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..a470147d4f14 100644
>> --- a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
>> @@ -78,6 +78,9 @@ 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 state is
>> +  reset on suspend. The state is then saved on suspend and restored on
>> +  resume.
>
> By whom? It is important to be clear about the respective
> responsibilities, as this forms a binding contract between firmware and OS.
>
> Mark: Can you have a look at how to formulate this? I'm not sure we have
> other instances of a non-architected behaviour involving FW
> participation, aside from PSCI.

I'll wait for Mark's reply to reword this.

>
>>
>>  The main GIC node must contain the appropriate #address-cells,
>>  #size-cells and ranges properties for the reg property of all ITS
>>
>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 4/4] irqchip/gic-v3-its: add ability to resend MAPC on resume

[Derek Basehore]

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 for an ITS
that has a non-zero number of collections stored in it.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 drivers/irqchip/irq-gic-v3-its.c   | 80 ++++++++++++++++++++------------------
 include/linux/irqchip/arm-gic-v3.h |  1 +
 2 files changed, 43 insertions(+), 38 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 5e63635e2a7b..dd6cd6e68ed0 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);
 }
@@ -3127,6 +3128,9 @@ static void its_restore_enable(void)
 			its_write_baser(its, baser, baser->val);
 		}
 		writel_relaxed(its->ctlr_save, base + GITS_CTLR);
+
+		if (GITS_TYPER_HWCOLLCNT(gic_read_typer(base + GITS_TYPER)) > 0)
+			its_cpu_init_collection(its);
 	}
 	spin_unlock(&its_lock);
 }
@@ -3393,7 +3397,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.0.rc1.238.g530d649a79-goog

Re: [PATCH v5 4/4] irqchip/gic-v3-its: add ability to resend MAPC on resume

[Marc Zyngier]

On 07/02/18 01:41, Derek Basehore wrote:
> 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 for an ITS
> that has a non-zero number of collections stored in it.
> 
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  drivers/irqchip/irq-gic-v3-its.c   | 80 ++++++++++++++++++++------------------
>  include/linux/irqchip/arm-gic-v3.h |  1 +
>  2 files changed, 43 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 5e63635e2a7b..dd6cd6e68ed0 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);
>  }
> @@ -3127,6 +3128,9 @@ static void its_restore_enable(void)
>  			its_write_baser(its, baser, baser->val);
>  		}
>  		writel_relaxed(its->ctlr_save, base + GITS_CTLR);
> +
> +		if (GITS_TYPER_HWCOLLCNT(gic_read_typer(base + GITS_TYPER)) > 0)
> +			its_cpu_init_collection(its);

This isn't correct. Think of a system where half the collections are in
HW, and the other half memory based (nothing in the spec forbids this).
You must evaluate the CID of each collection and replay the MAPC *only*
if it falls into the range [0..HCC-1]. The memory-based collections are
already mapped, and remapping an already mapped collection requires
extra care (see MAPC and the UNPREDICTABLE behaviour when V=1), so don't
go there.

>  	}
>  	spin_unlock(&its_lock);
>  }
> @@ -3393,7 +3397,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
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v5 4/4] irqchip/gic-v3-its: add ability to resend MAPC on resume

[Brian Norris]

Hi Marc,

I'm really not an expert on this, so take my observations with a large
grain of salt:

On Wed, Feb 07, 2018 at 08:46:42AM +0000, Marc Zyngier wrote:
> On 07/02/18 01:41, Derek Basehore wrote:
> > 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 for an ITS
> > that has a non-zero number of collections stored in it.
> > 
> > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > ---
> >  drivers/irqchip/irq-gic-v3-its.c   | 80 ++++++++++++++++++++------------------
> >  include/linux/irqchip/arm-gic-v3.h |  1 +
> >  2 files changed, 43 insertions(+), 38 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> > index 5e63635e2a7b..dd6cd6e68ed0 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)

...

> > @@ -3127,6 +3128,9 @@ static void its_restore_enable(void)
> >  			its_write_baser(its, baser, baser->val);
> >  		}
> >  		writel_relaxed(its->ctlr_save, base + GITS_CTLR);
> > +
> > +		if (GITS_TYPER_HWCOLLCNT(gic_read_typer(base + GITS_TYPER)) > 0)
> > +			its_cpu_init_collection(its);
> 
> This isn't correct. Think of a system where half the collections are in
> HW, and the other half memory based (nothing in the spec forbids this).
> You must evaluate the CID of each collection and replay the MAPC *only*
> if it falls into the range [0..HCC-1]. The memory-based collections are
> already mapped, and remapping an already mapped collection requires
> extra care (see MAPC and the UNPREDICTABLE behaviour when V=1), so don't
> go there.

IIUC, this is only run on CPU0 (it's in syscore resume), so implicitly,
CID is 0. Thus, the current condition is already doing what you ask:

	HCC > 0 == CID

which is equivalent to:

	HCC - 1 >= CID

Or should we really double check what CPU we're running on?

Brian

Re: [PATCH v5 4/4] irqchip/gic-v3-its: add ability to resend MAPC on resume

[dbasehore .]

On Wed, Feb 7, 2018 at 3:22 PM, Brian Norris <briannorris@chromium.org> wrote:
> Hi Marc,
>
> I'm really not an expert on this, so take my observations with a large
> grain of salt:
>
> On Wed, Feb 07, 2018 at 08:46:42AM +0000, Marc Zyngier wrote:
>> On 07/02/18 01:41, Derek Basehore wrote:
>> > 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 for an ITS
>> > that has a non-zero number of collections stored in it.
>> >
>> > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
>> > ---
>> >  drivers/irqchip/irq-gic-v3-its.c   | 80 ++++++++++++++++++++------------------
>> >  include/linux/irqchip/arm-gic-v3.h |  1 +
>> >  2 files changed, 43 insertions(+), 38 deletions(-)
>> >
>> > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> > index 5e63635e2a7b..dd6cd6e68ed0 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)
>
> ...
>
>> > @@ -3127,6 +3128,9 @@ static void its_restore_enable(void)
>> >                     its_write_baser(its, baser, baser->val);
>> >             }
>> >             writel_relaxed(its->ctlr_save, base + GITS_CTLR);
>> > +
>> > +           if (GITS_TYPER_HWCOLLCNT(gic_read_typer(base + GITS_TYPER)) > 0)
>> > +                   its_cpu_init_collection(its);
>>
>> This isn't correct. Think of a system where half the collections are in
>> HW, and the other half memory based (nothing in the spec forbids this).
>> You must evaluate the CID of each collection and replay the MAPC *only*
>> if it falls into the range [0..HCC-1]. The memory-based collections are
>> already mapped, and remapping an already mapped collection requires
>> extra care (see MAPC and the UNPREDICTABLE behaviour when V=1), so don't
>> go there.
>
> IIUC, this is only run on CPU0 (it's in syscore resume), so implicitly,
> CID is 0. Thus, the current condition is already doing what you ask:
>
>         HCC > 0 == CID
>
> which is equivalent to:
>
>         HCC - 1 >= CID
>
> Or should we really double check what CPU we're running on?

There seems to be the edge case where you hotplug CPU 0 before
suspending. In that case, I believe you're on the lowest number CPU
left?

It seems that all of the CPUs that are disabled have the ITS
reinitialized from scratch via enable_nonboot_cpus(). This code runs
on only the CPU that firmware resumes with. If that CPU isn't CPU 0
for whatever reason, we need to make sure that it's processor ID is
less than HCC.

>
> Brian

Re: [PATCH v5 4/4] irqchip/gic-v3-its: add ability to resend MAPC on resume

[Marc Zyngier]

On 08/02/18 00:00, dbasehore . wrote:
> On Wed, Feb 7, 2018 at 3:22 PM, Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>> Hi Marc,
>>
>> I'm really not an expert on this, so take my observations with a large
>> grain of salt:
>>
>> On Wed, Feb 07, 2018 at 08:46:42AM +0000, Marc Zyngier wrote:
>>> On 07/02/18 01:41, Derek Basehore wrote:
>>>> 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 for an ITS
>>>> that has a non-zero number of collections stored in it.
>>>>
>>>> Signed-off-by: Derek Basehore <dbasehore-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>>> ---
>>>>  drivers/irqchip/irq-gic-v3-its.c   | 80 ++++++++++++++++++++------------------
>>>>  include/linux/irqchip/arm-gic-v3.h |  1 +
>>>>  2 files changed, 43 insertions(+), 38 deletions(-)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>>>> index 5e63635e2a7b..dd6cd6e68ed0 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)
>>
>> ...
>>
>>>> @@ -3127,6 +3128,9 @@ static void its_restore_enable(void)
>>>>                     its_write_baser(its, baser, baser->val);
>>>>             }
>>>>             writel_relaxed(its->ctlr_save, base + GITS_CTLR);
>>>> +
>>>> +           if (GITS_TYPER_HWCOLLCNT(gic_read_typer(base + GITS_TYPER)) > 0)
>>>> +                   its_cpu_init_collection(its);
>>>
>>> This isn't correct. Think of a system where half the collections are in
>>> HW, and the other half memory based (nothing in the spec forbids this).
>>> You must evaluate the CID of each collection and replay the MAPC *only*
>>> if it falls into the range [0..HCC-1]. The memory-based collections are
>>> already mapped, and remapping an already mapped collection requires
>>> extra care (see MAPC and the UNPREDICTABLE behaviour when V=1), so don't
>>> go there.
>>
>> IIUC, this is only run on CPU0 (it's in syscore resume), so implicitly,
>> CID is 0. Thus, the current condition is already doing what you ask:
>>
>>         HCC > 0 == CID
>>
>> which is equivalent to:
>>
>>         HCC - 1 >= CID
>>
>> Or should we really double check what CPU we're running on?
> 
> There seems to be the edge case where you hotplug CPU 0 before
> suspending. In that case, I believe you're on the lowest number CPU
> left?

I don't think the core code makes any guarantee in that respect. This is
probably what happens in practice, but I wouldn't bet anything on this
being set in stone.

> It seems that all of the CPUs that are disabled have the ITS
> reinitialized from scratch via enable_nonboot_cpus(). This code runs
> on only the CPU that firmware resumes with. If that CPU isn't CPU 0
> for whatever reason, we need to make sure that it's processor ID is
> less than HCC.

Exactly, thanks for putting it better than I initially did.

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html