Re: [PATCH v6 1/3] irqchip/gic-v3-its: add ability to save/restore ITS state

[Brian Norris]

Hi Derek,

On Wed, Feb 07, 2018 at 06:36:46PM -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 
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 103 
> +++
>  1 file changed, 103 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index 06f025fd5726..35ad48f51dfa 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -33,6 +33,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -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_headits_device_list;
>   u64 flags;
>   unsigned long   list_nr;
> @@ -3042,6 +3046,100 @@ 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(_lock);
> + list_for_each_entry(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");

Isn't this a case where it was suggested to print the PA? Also, printing
the error code never hurts. Like:

pr_err("ITS@%pa: failed to quiesce: %d\n",
   >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, _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(_lock);
> +
> + return err;
> +}
> +
> +static void its_restore_enable(void)
> +{
> + struct its_node *its;
> +
> + spin_lock(_lock);
> + list_for_each_entry(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
> +  * registers is undefined according to the GIC v3 ITS
> +  * Specification.
> +  */
> + if (its_force_quiescent(base)) {
> + pr_err("ITS(%p): failed to quiesce on resume\n", base);

I think Marc was requesting the PA, not the VA. The VA isn't really very
useful. Also, might as well log with similar style to the existing logs
(ITS@xxx), and log the error code:

pr_err("ITS@%pa: failed to quiesce on resume: %d\n",
   >phys_base, ret);

Brian

> + 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 = >tables[i];
> +
> + if (!(baser->val & GITS_BASER_VALID))
> + 

Re: [PATCH v6 1/3] irqchip/gic-v3-its: add ability to save/restore ITS state

[Brian Norris]

Hi Derek,

On Wed, Feb 07, 2018 at 06:36:46PM -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 
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 103 
> +++
>  1 file changed, 103 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index 06f025fd5726..35ad48f51dfa 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -33,6 +33,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -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_headits_device_list;
>   u64 flags;
>   unsigned long   list_nr;
> @@ -3042,6 +3046,100 @@ 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(_lock);
> + list_for_each_entry(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");

Isn't this a case where it was suggested to print the PA? Also, printing
the error code never hurts. Like:

pr_err("ITS@%pa: failed to quiesce: %d\n",
   >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, _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(_lock);
> +
> + return err;
> +}
> +
> +static void its_restore_enable(void)
> +{
> + struct its_node *its;
> +
> + spin_lock(_lock);
> + list_for_each_entry(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
> +  * registers is undefined according to the GIC v3 ITS
> +  * Specification.
> +  */
> + if (its_force_quiescent(base)) {
> + pr_err("ITS(%p): failed to quiesce on resume\n", base);

I think Marc was requesting the PA, not the VA. The VA isn't really very
useful. Also, might as well log with similar style to the existing logs
(ITS@xxx), and log the error code:

pr_err("ITS@%pa: failed to quiesce on resume: %d\n",
   >phys_base, ret);

Brian

> + 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 = >tables[i];
> +
> + if (!(baser->val & GITS_BASER_VALID))
> + continue;
> +
> +  

[PATCH v6 1/3] 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 
---
 drivers/irqchip/irq-gic-v3-its.c | 103 +++
 1 file changed, 103 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 06f025fd5726..35ad48f51dfa 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -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_headits_device_list;
u64 flags;
unsigned long   list_nr;
@@ -3042,6 +3046,100 @@ 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(_lock);
+   list_for_each_entry(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, _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(_lock);
+
+   return err;
+}
+
+static void its_restore_enable(void)
+{
+   struct its_node *its;
+
+   spin_lock(_lock);
+   list_for_each_entry(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
+* registers is undefined according to the GIC v3 ITS
+* Specification.
+*/
+   if (its_force_quiescent(base)) {
+   pr_err("ITS(%p): failed to quiesce on resume\n", base);
+   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 = >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(_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 +3359,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 |= 

[PATCH v6 1/3] 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 
---
 drivers/irqchip/irq-gic-v3-its.c | 103 +++
 1 file changed, 103 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 06f025fd5726..35ad48f51dfa 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -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_headits_device_list;
u64 flags;
unsigned long   list_nr;
@@ -3042,6 +3046,100 @@ 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(_lock);
+   list_for_each_entry(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, _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(_lock);
+
+   return err;
+}
+
+static void its_restore_enable(void)
+{
+   struct its_node *its;
+
+   spin_lock(_lock);
+   list_for_each_entry(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
+* registers is undefined according to the GIC v3 ITS
+* Specification.
+*/
+   if (its_force_quiescent(base)) {
+   pr_err("ITS(%p): failed to quiesce on resume\n", base);
+   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 = >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(_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 +3359,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;
+