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

[dbasehore .]

On Tue, Feb 6, 2018 at 8:23 AM, Marc Zyngier  wrote:
> On 05/02/18 21:33, dbasehore . wrote:
>> On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier  wrote:
>>> On 03/02/18 01:24, 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 | 101 
 +++
  1 file changed, 101 insertions(+)

 diff --git a/drivers/irqchip/irq-gic-v3-its.c 
 b/drivers/irqchip/irq-gic-v3-its.c
 index 06f025fd5726..e13515cdb68f 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)

 @@ -83,6 +85,15 @@ struct its_baser {
   u32 psz;
  };

 +/*
 + * Saved ITS state - this is where saved state for the ITS is stored
 + * when it's disabled during system suspend.
 + */
 +struct its_ctx {
 + u64 cbaser;
 + u32 ctlr;
 +};
>>>
>>> Nit: This is pretty small for the ITS context. Given that its_node is
>>> the context, you can safely expand this in the its_node structure.
>>
>> Sounds reasonable. I think I just have it this way because I used to
>> have more in here.
>>
>>>
 +
  struct its_device;

  /*
 @@ -101,6 +112,7 @@ struct its_node {
   struct its_collection   *collections;
   struct fwnode_handle*fwnode_handle;
   u64 (*get_msi_base)(struct its_device *its_dev);
 + struct its_ctx  its_ctx;
   struct list_headits_device_list;
   u64 flags;
   unsigned long   list_nr;
 @@ -3042,6 +3054,90 @@ 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) {
 + struct its_ctx *ctx;
 + void __iomem *base;
 +
 + if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
 + continue;
 +
 + ctx = >its_ctx;
 + base = its->base;
 + ctx->ctlr = readl_relaxed(base + GITS_CTLR);
 + err = its_force_quiescent(base);
 + if (err) {
 + pr_err("ITS failed to quiesce\n");
 + writel_relaxed(ctx->ctlr, base + GITS_CTLR);
 + goto err;
 + }
 +
 + ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
 + }
 +
 +err:
 + if (err) {
 + list_for_each_entry_continue_reverse(its, _nodes, entry) 
 {
 + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
 + struct its_ctx *ctx = >its_ctx;
 + void __iomem *base = its->base;
 +
 + writel_relaxed(ctx->ctlr, 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) {
 + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
 + struct its_ctx *ctx = >its_ctx;
 + void __iomem *base = its->base;
 + /*
 +  * Only the lower 32 bits matter here since the 
 upper 32
 +  * don't include any of the offset.
 +  */
 + u32 creader = readl_relaxed(base + GITS_CREADR);
>>>
>>> Accessor matching gits_write_cwriter and co?
>>>
 + int i;
 +
 + /*
 +  * Reset the write location to where the ITS is
 +  * currently at.
 +  

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

[dbasehore .]

On Tue, Feb 6, 2018 at 8:23 AM, Marc Zyngier  wrote:
> On 05/02/18 21:33, dbasehore . wrote:
>> On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier  wrote:
>>> On 03/02/18 01:24, 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 | 101 
 +++
  1 file changed, 101 insertions(+)

 diff --git a/drivers/irqchip/irq-gic-v3-its.c 
 b/drivers/irqchip/irq-gic-v3-its.c
 index 06f025fd5726..e13515cdb68f 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)

 @@ -83,6 +85,15 @@ struct its_baser {
   u32 psz;
  };

 +/*
 + * Saved ITS state - this is where saved state for the ITS is stored
 + * when it's disabled during system suspend.
 + */
 +struct its_ctx {
 + u64 cbaser;
 + u32 ctlr;
 +};
>>>
>>> Nit: This is pretty small for the ITS context. Given that its_node is
>>> the context, you can safely expand this in the its_node structure.
>>
>> Sounds reasonable. I think I just have it this way because I used to
>> have more in here.
>>
>>>
 +
  struct its_device;

  /*
 @@ -101,6 +112,7 @@ struct its_node {
   struct its_collection   *collections;
   struct fwnode_handle*fwnode_handle;
   u64 (*get_msi_base)(struct its_device *its_dev);
 + struct its_ctx  its_ctx;
   struct list_headits_device_list;
   u64 flags;
   unsigned long   list_nr;
 @@ -3042,6 +3054,90 @@ 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) {
 + struct its_ctx *ctx;
 + void __iomem *base;
 +
 + if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
 + continue;
 +
 + ctx = >its_ctx;
 + base = its->base;
 + ctx->ctlr = readl_relaxed(base + GITS_CTLR);
 + err = its_force_quiescent(base);
 + if (err) {
 + pr_err("ITS failed to quiesce\n");
 + writel_relaxed(ctx->ctlr, base + GITS_CTLR);
 + goto err;
 + }
 +
 + ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
 + }
 +
 +err:
 + if (err) {
 + list_for_each_entry_continue_reverse(its, _nodes, entry) 
 {
 + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
 + struct its_ctx *ctx = >its_ctx;
 + void __iomem *base = its->base;
 +
 + writel_relaxed(ctx->ctlr, 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) {
 + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
 + struct its_ctx *ctx = >its_ctx;
 + void __iomem *base = its->base;
 + /*
 +  * Only the lower 32 bits matter here since the 
 upper 32
 +  * don't include any of the offset.
 +  */
 + u32 creader = readl_relaxed(base + GITS_CREADR);
>>>
>>> Accessor matching gits_write_cwriter and co?
>>>
 + int i;
 +
 + /*
 +  * Reset the write location to where the ITS is
 +  * currently at.
 +  */
 + 

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

[Marc Zyngier]

On 05/02/18 21:33, dbasehore . wrote:
> On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier  wrote:
>> On 03/02/18 01:24, 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 | 101 
>>> +++
>>>  1 file changed, 101 insertions(+)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
>>> b/drivers/irqchip/irq-gic-v3-its.c
>>> index 06f025fd5726..e13515cdb68f 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)
>>>
>>> @@ -83,6 +85,15 @@ struct its_baser {
>>>   u32 psz;
>>>  };
>>>
>>> +/*
>>> + * Saved ITS state - this is where saved state for the ITS is stored
>>> + * when it's disabled during system suspend.
>>> + */
>>> +struct its_ctx {
>>> + u64 cbaser;
>>> + u32 ctlr;
>>> +};
>>
>> Nit: This is pretty small for the ITS context. Given that its_node is
>> the context, you can safely expand this in the its_node structure.
> 
> Sounds reasonable. I think I just have it this way because I used to
> have more in here.
> 
>>
>>> +
>>>  struct its_device;
>>>
>>>  /*
>>> @@ -101,6 +112,7 @@ struct its_node {
>>>   struct its_collection   *collections;
>>>   struct fwnode_handle*fwnode_handle;
>>>   u64 (*get_msi_base)(struct its_device *its_dev);
>>> + struct its_ctx  its_ctx;
>>>   struct list_headits_device_list;
>>>   u64 flags;
>>>   unsigned long   list_nr;
>>> @@ -3042,6 +3054,90 @@ 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) {
>>> + struct its_ctx *ctx;
>>> + void __iomem *base;
>>> +
>>> + if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>>> + continue;
>>> +
>>> + ctx = >its_ctx;
>>> + base = its->base;
>>> + ctx->ctlr = readl_relaxed(base + GITS_CTLR);
>>> + err = its_force_quiescent(base);
>>> + if (err) {
>>> + pr_err("ITS failed to quiesce\n");
>>> + writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>>> + goto err;
>>> + }
>>> +
>>> + ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
>>> + }
>>> +
>>> +err:
>>> + if (err) {
>>> + list_for_each_entry_continue_reverse(its, _nodes, entry) {
>>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>>> + struct its_ctx *ctx = >its_ctx;
>>> + void __iomem *base = its->base;
>>> +
>>> + writel_relaxed(ctx->ctlr, 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) {
>>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>>> + struct its_ctx *ctx = >its_ctx;
>>> + void __iomem *base = its->base;
>>> + /*
>>> +  * Only the lower 32 bits matter here since the upper 
>>> 32
>>> +  * don't include any of the offset.
>>> +  */
>>> + u32 creader = readl_relaxed(base + GITS_CREADR);
>>
>> Accessor matching gits_write_cwriter and co?
>>
>>> + int i;
>>> +
>>> + /*
>>> +  * Reset the write location to where the ITS is
>>> +  * currently at.
>>> +  */
>>> + gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
>>> + gits_write_cwriter(creader, base + GITS_CWRITER);
>>> + its->cmd_write = 

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

[Marc Zyngier]

On 05/02/18 21:33, dbasehore . wrote:
> On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier  wrote:
>> On 03/02/18 01:24, 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 | 101 
>>> +++
>>>  1 file changed, 101 insertions(+)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
>>> b/drivers/irqchip/irq-gic-v3-its.c
>>> index 06f025fd5726..e13515cdb68f 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)
>>>
>>> @@ -83,6 +85,15 @@ struct its_baser {
>>>   u32 psz;
>>>  };
>>>
>>> +/*
>>> + * Saved ITS state - this is where saved state for the ITS is stored
>>> + * when it's disabled during system suspend.
>>> + */
>>> +struct its_ctx {
>>> + u64 cbaser;
>>> + u32 ctlr;
>>> +};
>>
>> Nit: This is pretty small for the ITS context. Given that its_node is
>> the context, you can safely expand this in the its_node structure.
> 
> Sounds reasonable. I think I just have it this way because I used to
> have more in here.
> 
>>
>>> +
>>>  struct its_device;
>>>
>>>  /*
>>> @@ -101,6 +112,7 @@ struct its_node {
>>>   struct its_collection   *collections;
>>>   struct fwnode_handle*fwnode_handle;
>>>   u64 (*get_msi_base)(struct its_device *its_dev);
>>> + struct its_ctx  its_ctx;
>>>   struct list_headits_device_list;
>>>   u64 flags;
>>>   unsigned long   list_nr;
>>> @@ -3042,6 +3054,90 @@ 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) {
>>> + struct its_ctx *ctx;
>>> + void __iomem *base;
>>> +
>>> + if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>>> + continue;
>>> +
>>> + ctx = >its_ctx;
>>> + base = its->base;
>>> + ctx->ctlr = readl_relaxed(base + GITS_CTLR);
>>> + err = its_force_quiescent(base);
>>> + if (err) {
>>> + pr_err("ITS failed to quiesce\n");
>>> + writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>>> + goto err;
>>> + }
>>> +
>>> + ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
>>> + }
>>> +
>>> +err:
>>> + if (err) {
>>> + list_for_each_entry_continue_reverse(its, _nodes, entry) {
>>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>>> + struct its_ctx *ctx = >its_ctx;
>>> + void __iomem *base = its->base;
>>> +
>>> + writel_relaxed(ctx->ctlr, 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) {
>>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>>> + struct its_ctx *ctx = >its_ctx;
>>> + void __iomem *base = its->base;
>>> + /*
>>> +  * Only the lower 32 bits matter here since the upper 
>>> 32
>>> +  * don't include any of the offset.
>>> +  */
>>> + u32 creader = readl_relaxed(base + GITS_CREADR);
>>
>> Accessor matching gits_write_cwriter and co?
>>
>>> + int i;
>>> +
>>> + /*
>>> +  * Reset the write location to where the ITS is
>>> +  * currently at.
>>> +  */
>>> + gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
>>> + gits_write_cwriter(creader, base + GITS_CWRITER);
>>> + its->cmd_write = >cmd_base[
>>> + 

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

[dbasehore .]

On Mon, Feb 5, 2018 at 5:01 PM, dbasehore .  wrote:
> On Mon, Feb 5, 2018 at 4:33 PM, dbasehore .  wrote:
>> On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier  wrote:
>>> On 03/02/18 01:24, 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 | 101 
 +++
  1 file changed, 101 insertions(+)

 diff --git a/drivers/irqchip/irq-gic-v3-its.c 
 b/drivers/irqchip/irq-gic-v3-its.c
 index 06f025fd5726..e13515cdb68f 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)

 @@ -83,6 +85,15 @@ struct its_baser {
   u32 psz;
  };

 +/*
 + * Saved ITS state - this is where saved state for the ITS is stored
 + * when it's disabled during system suspend.
 + */
 +struct its_ctx {
 + u64 cbaser;
 + u32 ctlr;
 +};
>>>
>>> Nit: This is pretty small for the ITS context. Given that its_node is
>>> the context, you can safely expand this in the its_node structure.
>>>
 +
  struct its_device;

  /*
 @@ -101,6 +112,7 @@ struct its_node {
   struct its_collection   *collections;
   struct fwnode_handle*fwnode_handle;
   u64 (*get_msi_base)(struct its_device *its_dev);
 + struct its_ctx  its_ctx;
   struct list_headits_device_list;
   u64 flags;
   unsigned long   list_nr;
 @@ -3042,6 +3054,90 @@ 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) {
 + struct its_ctx *ctx;
 + void __iomem *base;
 +
 + if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
 + continue;
 +
 + ctx = >its_ctx;
 + base = its->base;
 + ctx->ctlr = readl_relaxed(base + GITS_CTLR);
 + err = its_force_quiescent(base);
 + if (err) {
 + pr_err("ITS failed to quiesce\n");
 + writel_relaxed(ctx->ctlr, base + GITS_CTLR);
 + goto err;
 + }
 +
 + ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
 + }
 +
 +err:
 + if (err) {
 + list_for_each_entry_continue_reverse(its, _nodes, entry) 
 {
 + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
 + struct its_ctx *ctx = >its_ctx;
 + void __iomem *base = its->base;
 +
 + writel_relaxed(ctx->ctlr, 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) {
 + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
 + struct its_ctx *ctx = >its_ctx;
 + void __iomem *base = its->base;
 + /*
 +  * Only the lower 32 bits matter here since the 
 upper 32
 +  * don't include any of the offset.
 +  */
 + u32 creader = readl_relaxed(base + GITS_CREADR);
>>>
>>> Accessor matching gits_write_cwriter and co?
>>>
 + int i;
 +
 + /*
 +  * Reset the write location to where the ITS is
 +  * currently at.
 +  */
 + 

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

[dbasehore .]

On Mon, Feb 5, 2018 at 5:01 PM, dbasehore .  wrote:
> On Mon, Feb 5, 2018 at 4:33 PM, dbasehore .  wrote:
>> On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier  wrote:
>>> On 03/02/18 01:24, 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 | 101 
 +++
  1 file changed, 101 insertions(+)

 diff --git a/drivers/irqchip/irq-gic-v3-its.c 
 b/drivers/irqchip/irq-gic-v3-its.c
 index 06f025fd5726..e13515cdb68f 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)

 @@ -83,6 +85,15 @@ struct its_baser {
   u32 psz;
  };

 +/*
 + * Saved ITS state - this is where saved state for the ITS is stored
 + * when it's disabled during system suspend.
 + */
 +struct its_ctx {
 + u64 cbaser;
 + u32 ctlr;
 +};
>>>
>>> Nit: This is pretty small for the ITS context. Given that its_node is
>>> the context, you can safely expand this in the its_node structure.
>>>
 +
  struct its_device;

  /*
 @@ -101,6 +112,7 @@ struct its_node {
   struct its_collection   *collections;
   struct fwnode_handle*fwnode_handle;
   u64 (*get_msi_base)(struct its_device *its_dev);
 + struct its_ctx  its_ctx;
   struct list_headits_device_list;
   u64 flags;
   unsigned long   list_nr;
 @@ -3042,6 +3054,90 @@ 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) {
 + struct its_ctx *ctx;
 + void __iomem *base;
 +
 + if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
 + continue;
 +
 + ctx = >its_ctx;
 + base = its->base;
 + ctx->ctlr = readl_relaxed(base + GITS_CTLR);
 + err = its_force_quiescent(base);
 + if (err) {
 + pr_err("ITS failed to quiesce\n");
 + writel_relaxed(ctx->ctlr, base + GITS_CTLR);
 + goto err;
 + }
 +
 + ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
 + }
 +
 +err:
 + if (err) {
 + list_for_each_entry_continue_reverse(its, _nodes, entry) 
 {
 + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
 + struct its_ctx *ctx = >its_ctx;
 + void __iomem *base = its->base;
 +
 + writel_relaxed(ctx->ctlr, 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) {
 + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
 + struct its_ctx *ctx = >its_ctx;
 + void __iomem *base = its->base;
 + /*
 +  * Only the lower 32 bits matter here since the 
 upper 32
 +  * don't include any of the offset.
 +  */
 + u32 creader = readl_relaxed(base + GITS_CREADR);
>>>
>>> Accessor matching gits_write_cwriter and co?
>>>
 + int i;
 +
 + /*
 +  * Reset the write location to where the ITS is
 +  * currently at.
 +  */
 + gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
 + gits_write_cwriter(creader, 

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

[dbasehore .]

On Mon, Feb 5, 2018 at 4:33 PM, dbasehore .  wrote:
> On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier  wrote:
>> On 03/02/18 01:24, 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 | 101 
>>> +++
>>>  1 file changed, 101 insertions(+)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
>>> b/drivers/irqchip/irq-gic-v3-its.c
>>> index 06f025fd5726..e13515cdb68f 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)
>>>
>>> @@ -83,6 +85,15 @@ struct its_baser {
>>>   u32 psz;
>>>  };
>>>
>>> +/*
>>> + * Saved ITS state - this is where saved state for the ITS is stored
>>> + * when it's disabled during system suspend.
>>> + */
>>> +struct its_ctx {
>>> + u64 cbaser;
>>> + u32 ctlr;
>>> +};
>>
>> Nit: This is pretty small for the ITS context. Given that its_node is
>> the context, you can safely expand this in the its_node structure.
>>
>>> +
>>>  struct its_device;
>>>
>>>  /*
>>> @@ -101,6 +112,7 @@ struct its_node {
>>>   struct its_collection   *collections;
>>>   struct fwnode_handle*fwnode_handle;
>>>   u64 (*get_msi_base)(struct its_device *its_dev);
>>> + struct its_ctx  its_ctx;
>>>   struct list_headits_device_list;
>>>   u64 flags;
>>>   unsigned long   list_nr;
>>> @@ -3042,6 +3054,90 @@ 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) {
>>> + struct its_ctx *ctx;
>>> + void __iomem *base;
>>> +
>>> + if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>>> + continue;
>>> +
>>> + ctx = >its_ctx;
>>> + base = its->base;
>>> + ctx->ctlr = readl_relaxed(base + GITS_CTLR);
>>> + err = its_force_quiescent(base);
>>> + if (err) {
>>> + pr_err("ITS failed to quiesce\n");
>>> + writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>>> + goto err;
>>> + }
>>> +
>>> + ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
>>> + }
>>> +
>>> +err:
>>> + if (err) {
>>> + list_for_each_entry_continue_reverse(its, _nodes, entry) {
>>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>>> + struct its_ctx *ctx = >its_ctx;
>>> + void __iomem *base = its->base;
>>> +
>>> + writel_relaxed(ctx->ctlr, 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) {
>>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>>> + struct its_ctx *ctx = >its_ctx;
>>> + void __iomem *base = its->base;
>>> + /*
>>> +  * Only the lower 32 bits matter here since the upper 
>>> 32
>>> +  * don't include any of the offset.
>>> +  */
>>> + u32 creader = readl_relaxed(base + GITS_CREADR);
>>
>> Accessor matching gits_write_cwriter and co?
>>
>>> + int i;
>>> +
>>> + /*
>>> +  * Reset the write location to where the ITS is
>>> +  * currently at.
>>> +  */
>>> + gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
>>> + gits_write_cwriter(creader, base + GITS_CWRITER);
>>> + its->cmd_write = >cmd_base[
>>> + creader / 

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

[dbasehore .]

On Mon, Feb 5, 2018 at 4:33 PM, dbasehore .  wrote:
> On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier  wrote:
>> On 03/02/18 01:24, 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 | 101 
>>> +++
>>>  1 file changed, 101 insertions(+)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
>>> b/drivers/irqchip/irq-gic-v3-its.c
>>> index 06f025fd5726..e13515cdb68f 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)
>>>
>>> @@ -83,6 +85,15 @@ struct its_baser {
>>>   u32 psz;
>>>  };
>>>
>>> +/*
>>> + * Saved ITS state - this is where saved state for the ITS is stored
>>> + * when it's disabled during system suspend.
>>> + */
>>> +struct its_ctx {
>>> + u64 cbaser;
>>> + u32 ctlr;
>>> +};
>>
>> Nit: This is pretty small for the ITS context. Given that its_node is
>> the context, you can safely expand this in the its_node structure.
>>
>>> +
>>>  struct its_device;
>>>
>>>  /*
>>> @@ -101,6 +112,7 @@ struct its_node {
>>>   struct its_collection   *collections;
>>>   struct fwnode_handle*fwnode_handle;
>>>   u64 (*get_msi_base)(struct its_device *its_dev);
>>> + struct its_ctx  its_ctx;
>>>   struct list_headits_device_list;
>>>   u64 flags;
>>>   unsigned long   list_nr;
>>> @@ -3042,6 +3054,90 @@ 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) {
>>> + struct its_ctx *ctx;
>>> + void __iomem *base;
>>> +
>>> + if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>>> + continue;
>>> +
>>> + ctx = >its_ctx;
>>> + base = its->base;
>>> + ctx->ctlr = readl_relaxed(base + GITS_CTLR);
>>> + err = its_force_quiescent(base);
>>> + if (err) {
>>> + pr_err("ITS failed to quiesce\n");
>>> + writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>>> + goto err;
>>> + }
>>> +
>>> + ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
>>> + }
>>> +
>>> +err:
>>> + if (err) {
>>> + list_for_each_entry_continue_reverse(its, _nodes, entry) {
>>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>>> + struct its_ctx *ctx = >its_ctx;
>>> + void __iomem *base = its->base;
>>> +
>>> + writel_relaxed(ctx->ctlr, 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) {
>>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>>> + struct its_ctx *ctx = >its_ctx;
>>> + void __iomem *base = its->base;
>>> + /*
>>> +  * Only the lower 32 bits matter here since the upper 
>>> 32
>>> +  * don't include any of the offset.
>>> +  */
>>> + u32 creader = readl_relaxed(base + GITS_CREADR);
>>
>> Accessor matching gits_write_cwriter and co?
>>
>>> + int i;
>>> +
>>> + /*
>>> +  * Reset the write location to where the ITS is
>>> +  * currently at.
>>> +  */
>>> + gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
>>> + gits_write_cwriter(creader, base + GITS_CWRITER);
>>> + its->cmd_write = >cmd_base[
>>> + creader / sizeof(struct its_cmd_block)];
>>
>> Nit: please do not split lines like 

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

[dbasehore .]

On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier  wrote:
> On 03/02/18 01:24, 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 | 101 
>> +++
>>  1 file changed, 101 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index 06f025fd5726..e13515cdb68f 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)
>>
>> @@ -83,6 +85,15 @@ struct its_baser {
>>   u32 psz;
>>  };
>>
>> +/*
>> + * Saved ITS state - this is where saved state for the ITS is stored
>> + * when it's disabled during system suspend.
>> + */
>> +struct its_ctx {
>> + u64 cbaser;
>> + u32 ctlr;
>> +};
>
> Nit: This is pretty small for the ITS context. Given that its_node is
> the context, you can safely expand this in the its_node structure.
>
>> +
>>  struct its_device;
>>
>>  /*
>> @@ -101,6 +112,7 @@ struct its_node {
>>   struct its_collection   *collections;
>>   struct fwnode_handle*fwnode_handle;
>>   u64 (*get_msi_base)(struct its_device *its_dev);
>> + struct its_ctx  its_ctx;
>>   struct list_headits_device_list;
>>   u64 flags;
>>   unsigned long   list_nr;
>> @@ -3042,6 +3054,90 @@ 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) {
>> + struct its_ctx *ctx;
>> + void __iomem *base;
>> +
>> + if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>> + continue;
>> +
>> + ctx = >its_ctx;
>> + base = its->base;
>> + ctx->ctlr = readl_relaxed(base + GITS_CTLR);
>> + err = its_force_quiescent(base);
>> + if (err) {
>> + pr_err("ITS failed to quiesce\n");
>> + writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>> + goto err;
>> + }
>> +
>> + ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
>> + }
>> +
>> +err:
>> + if (err) {
>> + list_for_each_entry_continue_reverse(its, _nodes, entry) {
>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>> + struct its_ctx *ctx = >its_ctx;
>> + void __iomem *base = its->base;
>> +
>> + writel_relaxed(ctx->ctlr, 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) {
>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>> + struct its_ctx *ctx = >its_ctx;
>> + void __iomem *base = its->base;
>> + /*
>> +  * Only the lower 32 bits matter here since the upper 
>> 32
>> +  * don't include any of the offset.
>> +  */
>> + u32 creader = readl_relaxed(base + GITS_CREADR);
>
> Accessor matching gits_write_cwriter and co?
>
>> + int i;
>> +
>> + /*
>> +  * Reset the write location to where the ITS is
>> +  * currently at.
>> +  */
>> + gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
>> + gits_write_cwriter(creader, base + GITS_CWRITER);
>> + its->cmd_write = >cmd_base[
>> + creader / sizeof(struct its_cmd_block)];
>
> Nit: please do not split lines like this, this is unreadable. We both
> have screens that are wide enough for this to fit on a single line.
>
> More importantly: Why isn't it sufficient 

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

[dbasehore .]

On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier  wrote:
> On 03/02/18 01:24, 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 | 101 
>> +++
>>  1 file changed, 101 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index 06f025fd5726..e13515cdb68f 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)
>>
>> @@ -83,6 +85,15 @@ struct its_baser {
>>   u32 psz;
>>  };
>>
>> +/*
>> + * Saved ITS state - this is where saved state for the ITS is stored
>> + * when it's disabled during system suspend.
>> + */
>> +struct its_ctx {
>> + u64 cbaser;
>> + u32 ctlr;
>> +};
>
> Nit: This is pretty small for the ITS context. Given that its_node is
> the context, you can safely expand this in the its_node structure.
>
>> +
>>  struct its_device;
>>
>>  /*
>> @@ -101,6 +112,7 @@ struct its_node {
>>   struct its_collection   *collections;
>>   struct fwnode_handle*fwnode_handle;
>>   u64 (*get_msi_base)(struct its_device *its_dev);
>> + struct its_ctx  its_ctx;
>>   struct list_headits_device_list;
>>   u64 flags;
>>   unsigned long   list_nr;
>> @@ -3042,6 +3054,90 @@ 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) {
>> + struct its_ctx *ctx;
>> + void __iomem *base;
>> +
>> + if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>> + continue;
>> +
>> + ctx = >its_ctx;
>> + base = its->base;
>> + ctx->ctlr = readl_relaxed(base + GITS_CTLR);
>> + err = its_force_quiescent(base);
>> + if (err) {
>> + pr_err("ITS failed to quiesce\n");
>> + writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>> + goto err;
>> + }
>> +
>> + ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
>> + }
>> +
>> +err:
>> + if (err) {
>> + list_for_each_entry_continue_reverse(its, _nodes, entry) {
>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>> + struct its_ctx *ctx = >its_ctx;
>> + void __iomem *base = its->base;
>> +
>> + writel_relaxed(ctx->ctlr, 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) {
>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>> + struct its_ctx *ctx = >its_ctx;
>> + void __iomem *base = its->base;
>> + /*
>> +  * Only the lower 32 bits matter here since the upper 
>> 32
>> +  * don't include any of the offset.
>> +  */
>> + u32 creader = readl_relaxed(base + GITS_CREADR);
>
> Accessor matching gits_write_cwriter and co?
>
>> + int i;
>> +
>> + /*
>> +  * Reset the write location to where the ITS is
>> +  * currently at.
>> +  */
>> + gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
>> + gits_write_cwriter(creader, base + GITS_CWRITER);
>> + its->cmd_write = >cmd_base[
>> + creader / sizeof(struct its_cmd_block)];
>
> Nit: please do not split lines like this, this is unreadable. We both
> have screens that are wide enough for this to fit on a single line.
>
> More importantly: Why isn't it sufficient to reset both CREADR and
> CWRITER to zero? 

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

[dbasehore .]

On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier  wrote:
> On 03/02/18 01:24, 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 | 101 
>> +++
>>  1 file changed, 101 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index 06f025fd5726..e13515cdb68f 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)
>>
>> @@ -83,6 +85,15 @@ struct its_baser {
>>   u32 psz;
>>  };
>>
>> +/*
>> + * Saved ITS state - this is where saved state for the ITS is stored
>> + * when it's disabled during system suspend.
>> + */
>> +struct its_ctx {
>> + u64 cbaser;
>> + u32 ctlr;
>> +};
>
> Nit: This is pretty small for the ITS context. Given that its_node is
> the context, you can safely expand this in the its_node structure.

Sounds reasonable. I think I just have it this way because I used to
have more in here.

>
>> +
>>  struct its_device;
>>
>>  /*
>> @@ -101,6 +112,7 @@ struct its_node {
>>   struct its_collection   *collections;
>>   struct fwnode_handle*fwnode_handle;
>>   u64 (*get_msi_base)(struct its_device *its_dev);
>> + struct its_ctx  its_ctx;
>>   struct list_headits_device_list;
>>   u64 flags;
>>   unsigned long   list_nr;
>> @@ -3042,6 +3054,90 @@ 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) {
>> + struct its_ctx *ctx;
>> + void __iomem *base;
>> +
>> + if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>> + continue;
>> +
>> + ctx = >its_ctx;
>> + base = its->base;
>> + ctx->ctlr = readl_relaxed(base + GITS_CTLR);
>> + err = its_force_quiescent(base);
>> + if (err) {
>> + pr_err("ITS failed to quiesce\n");
>> + writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>> + goto err;
>> + }
>> +
>> + ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
>> + }
>> +
>> +err:
>> + if (err) {
>> + list_for_each_entry_continue_reverse(its, _nodes, entry) {
>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>> + struct its_ctx *ctx = >its_ctx;
>> + void __iomem *base = its->base;
>> +
>> + writel_relaxed(ctx->ctlr, 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) {
>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>> + struct its_ctx *ctx = >its_ctx;
>> + void __iomem *base = its->base;
>> + /*
>> +  * Only the lower 32 bits matter here since the upper 
>> 32
>> +  * don't include any of the offset.
>> +  */
>> + u32 creader = readl_relaxed(base + GITS_CREADR);
>
> Accessor matching gits_write_cwriter and co?
>
>> + int i;
>> +
>> + /*
>> +  * Reset the write location to where the ITS is
>> +  * currently at.
>> +  */
>> + gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
>> + gits_write_cwriter(creader, base + GITS_CWRITER);
>> + its->cmd_write = >cmd_base[
>> + creader / sizeof(struct its_cmd_block)];
>
> Nit: please do not split lines like this, this is unreadable. We both
> have screens that are 

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

[dbasehore .]

On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier  wrote:
> On 03/02/18 01:24, 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 | 101 
>> +++
>>  1 file changed, 101 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index 06f025fd5726..e13515cdb68f 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)
>>
>> @@ -83,6 +85,15 @@ struct its_baser {
>>   u32 psz;
>>  };
>>
>> +/*
>> + * Saved ITS state - this is where saved state for the ITS is stored
>> + * when it's disabled during system suspend.
>> + */
>> +struct its_ctx {
>> + u64 cbaser;
>> + u32 ctlr;
>> +};
>
> Nit: This is pretty small for the ITS context. Given that its_node is
> the context, you can safely expand this in the its_node structure.

Sounds reasonable. I think I just have it this way because I used to
have more in here.

>
>> +
>>  struct its_device;
>>
>>  /*
>> @@ -101,6 +112,7 @@ struct its_node {
>>   struct its_collection   *collections;
>>   struct fwnode_handle*fwnode_handle;
>>   u64 (*get_msi_base)(struct its_device *its_dev);
>> + struct its_ctx  its_ctx;
>>   struct list_headits_device_list;
>>   u64 flags;
>>   unsigned long   list_nr;
>> @@ -3042,6 +3054,90 @@ 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) {
>> + struct its_ctx *ctx;
>> + void __iomem *base;
>> +
>> + if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>> + continue;
>> +
>> + ctx = >its_ctx;
>> + base = its->base;
>> + ctx->ctlr = readl_relaxed(base + GITS_CTLR);
>> + err = its_force_quiescent(base);
>> + if (err) {
>> + pr_err("ITS failed to quiesce\n");
>> + writel_relaxed(ctx->ctlr, base + GITS_CTLR);
>> + goto err;
>> + }
>> +
>> + ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
>> + }
>> +
>> +err:
>> + if (err) {
>> + list_for_each_entry_continue_reverse(its, _nodes, entry) {
>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>> + struct its_ctx *ctx = >its_ctx;
>> + void __iomem *base = its->base;
>> +
>> + writel_relaxed(ctx->ctlr, 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) {
>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
>> + struct its_ctx *ctx = >its_ctx;
>> + void __iomem *base = its->base;
>> + /*
>> +  * Only the lower 32 bits matter here since the upper 
>> 32
>> +  * don't include any of the offset.
>> +  */
>> + u32 creader = readl_relaxed(base + GITS_CREADR);
>
> Accessor matching gits_write_cwriter and co?
>
>> + int i;
>> +
>> + /*
>> +  * Reset the write location to where the ITS is
>> +  * currently at.
>> +  */
>> + gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
>> + gits_write_cwriter(creader, base + GITS_CWRITER);
>> + its->cmd_write = >cmd_base[
>> + creader / sizeof(struct its_cmd_block)];
>
> Nit: please do not split lines like this, this is unreadable. We both
> have screens that are wide enough for this to fit on a single line.
>

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

[Marc Zyngier]

On 03/02/18 01:24, 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 | 101 
> +++
>  1 file changed, 101 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index 06f025fd5726..e13515cdb68f 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)
>  
> @@ -83,6 +85,15 @@ struct its_baser {
>   u32 psz;
>  };
>  
> +/*
> + * Saved ITS state - this is where saved state for the ITS is stored
> + * when it's disabled during system suspend.
> + */
> +struct its_ctx {
> + u64 cbaser;
> + u32 ctlr;
> +};

Nit: This is pretty small for the ITS context. Given that its_node is
the context, you can safely expand this in the its_node structure.

> +
>  struct its_device;
>  
>  /*
> @@ -101,6 +112,7 @@ struct its_node {
>   struct its_collection   *collections;
>   struct fwnode_handle*fwnode_handle;
>   u64 (*get_msi_base)(struct its_device *its_dev);
> + struct its_ctx  its_ctx;
>   struct list_headits_device_list;
>   u64 flags;
>   unsigned long   list_nr;
> @@ -3042,6 +3054,90 @@ 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) {
> + struct its_ctx *ctx;
> + void __iomem *base;
> +
> + if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
> + continue;
> +
> + ctx = >its_ctx;
> + base = its->base;
> + ctx->ctlr = readl_relaxed(base + GITS_CTLR);
> + err = its_force_quiescent(base);
> + if (err) {
> + pr_err("ITS failed to quiesce\n");
> + writel_relaxed(ctx->ctlr, base + GITS_CTLR);
> + goto err;
> + }
> +
> + ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
> + }
> +
> +err:
> + if (err) {
> + list_for_each_entry_continue_reverse(its, _nodes, entry) {
> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
> + struct its_ctx *ctx = >its_ctx;
> + void __iomem *base = its->base;
> +
> + writel_relaxed(ctx->ctlr, 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) {
> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
> + struct its_ctx *ctx = >its_ctx;
> + void __iomem *base = its->base;
> + /*
> +  * Only the lower 32 bits matter here since the upper 32
> +  * don't include any of the offset.
> +  */
> + u32 creader = readl_relaxed(base + GITS_CREADR);

Accessor matching gits_write_cwriter and co?

> + int i;
> +
> + /*
> +  * Reset the write location to where the ITS is
> +  * currently at.
> +  */
> + gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
> + gits_write_cwriter(creader, base + GITS_CWRITER);
> + its->cmd_write = >cmd_base[
> + creader / sizeof(struct its_cmd_block)];

Nit: please do not split lines like this, this is unreadable. We both
have screens that are wide enough for this to fit on a single line.

More importantly: Why isn't it sufficient to reset both CREADR and
CWRITER to zero? Is there any case where you can suspend whilst having
anything in flight?

> + /* Restore GITS_BASER from the value cache. */
> + 

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

[Marc Zyngier]

On 03/02/18 01:24, 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 | 101 
> +++
>  1 file changed, 101 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index 06f025fd5726..e13515cdb68f 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)
>  
> @@ -83,6 +85,15 @@ struct its_baser {
>   u32 psz;
>  };
>  
> +/*
> + * Saved ITS state - this is where saved state for the ITS is stored
> + * when it's disabled during system suspend.
> + */
> +struct its_ctx {
> + u64 cbaser;
> + u32 ctlr;
> +};

Nit: This is pretty small for the ITS context. Given that its_node is
the context, you can safely expand this in the its_node structure.

> +
>  struct its_device;
>  
>  /*
> @@ -101,6 +112,7 @@ struct its_node {
>   struct its_collection   *collections;
>   struct fwnode_handle*fwnode_handle;
>   u64 (*get_msi_base)(struct its_device *its_dev);
> + struct its_ctx  its_ctx;
>   struct list_headits_device_list;
>   u64 flags;
>   unsigned long   list_nr;
> @@ -3042,6 +3054,90 @@ 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) {
> + struct its_ctx *ctx;
> + void __iomem *base;
> +
> + if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
> + continue;
> +
> + ctx = >its_ctx;
> + base = its->base;
> + ctx->ctlr = readl_relaxed(base + GITS_CTLR);
> + err = its_force_quiescent(base);
> + if (err) {
> + pr_err("ITS failed to quiesce\n");
> + writel_relaxed(ctx->ctlr, base + GITS_CTLR);
> + goto err;
> + }
> +
> + ctx->cbaser = gits_read_cbaser(base + GITS_CBASER);
> + }
> +
> +err:
> + if (err) {
> + list_for_each_entry_continue_reverse(its, _nodes, entry) {
> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
> + struct its_ctx *ctx = >its_ctx;
> + void __iomem *base = its->base;
> +
> + writel_relaxed(ctx->ctlr, 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) {
> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) {
> + struct its_ctx *ctx = >its_ctx;
> + void __iomem *base = its->base;
> + /*
> +  * Only the lower 32 bits matter here since the upper 32
> +  * don't include any of the offset.
> +  */
> + u32 creader = readl_relaxed(base + GITS_CREADR);

Accessor matching gits_write_cwriter and co?

> + int i;
> +
> + /*
> +  * Reset the write location to where the ITS is
> +  * currently at.
> +  */
> + gits_write_cbaser(ctx->cbaser, base + GITS_CBASER);
> + gits_write_cwriter(creader, base + GITS_CWRITER);
> + its->cmd_write = >cmd_base[
> + creader / sizeof(struct its_cmd_block)];

Nit: please do not split lines like this, this is unreadable. We both
have screens that are wide enough for this to fit on a single line.

More importantly: Why isn't it sufficient to reset both CREADR and
CWRITER to zero? Is there any case where you can suspend whilst having
anything in flight?

> + /* Restore GITS_BASER from the value cache. */
> + for (i = 0; i <