[PATCH 0/3] irqchip: dw-apb-ictl: IRQ_GC_MASK_CACHE_PER_TYPE and PM support

[PATCH 0/3] irqchip: dw-apb-ictl: IRQ_GC_MASK_CACHE_PER_TYPE and PM support

[Jisheng Zhang]

These patches try to improve dw-apb-ictl irqchip driver a bit.

The first patch does a bit clean up work -- unify the register access usage.

The two dw-apb-ictl's irq_chip_type instances have separate mask registers, so the second patch enables IRQ_GC_MASK_CACHE_PER_TYPE.

The last patch adds suspend/resume support to the driver.

Tested on Marvell BG2Q-DMP board.

Jisheng Zhang (3):
  irqchip: dw-apb-ictl: always use use {readl|writel}_relaxed
  irqchip: dw-apb-ictl: enable IRQ_GC_MASK_CACHE_PER_TYPE
  irqchip: dw-apb-ictl: add PM support

 drivers/irqchip/irq-dw-apb-ictl.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

-- 
2.1.0

[PATCH 1/3] irqchip: dw-apb-ictl: always use use {readl|writel}_relaxed

[Jisheng Zhang]

relaxed version and non-relaxed version are mixed, this patch always
use the relaxed version to unify the memory access usage.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/irqchip/irq-dw-apb-ictl.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-dw-apb-ictl.c b/drivers/irqchip/irq-dw-apb-ictl.c
index 31e231e..fcc3385 100644
--- a/drivers/irqchip/irq-dw-apb-ictl.c
+++ b/drivers/irqchip/irq-dw-apb-ictl.c
@@ -94,16 +94,16 @@ static int __init dw_apb_ictl_init(struct device_node *np,
 	 */
 
 	/* mask and enable all interrupts */
-	writel(~0, iobase + APB_INT_MASK_L);
-	writel(~0, iobase + APB_INT_MASK_H);
-	writel(~0, iobase + APB_INT_ENABLE_L);
-	writel(~0, iobase + APB_INT_ENABLE_H);
+	writel_relaxed(~0, iobase + APB_INT_MASK_L);
+	writel_relaxed(~0, iobase + APB_INT_MASK_H);
+	writel_relaxed(~0, iobase + APB_INT_ENABLE_L);
+	writel_relaxed(~0, iobase + APB_INT_ENABLE_H);
 
-	reg = readl(iobase + APB_INT_ENABLE_H);
+	reg = readl_relaxed(iobase + APB_INT_ENABLE_H);
 	if (reg)
 		nrirqs = 32 + fls(reg);
 	else
-		nrirqs = fls(readl(iobase + APB_INT_ENABLE_L));
+		nrirqs = fls(readl_relaxed(iobase + APB_INT_ENABLE_L));
 
 	domain = irq_domain_add_linear(np, nrirqs,
 				       &irq_generic_chip_ops, NULL);
-- 
2.1.0

[PATCH 2/3] irqchip: dw-apb-ictl: enable IRQ_GC_MASK_CACHE_PER_TYPE

[Jisheng Zhang]

The irq_chip_type instances have separate mask registers, so we need to
enable IRQ_GC_MASK_CACHE_PER_TYPE to actually handle separate mask registers.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/irqchip/irq-dw-apb-ictl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/irqchip/irq-dw-apb-ictl.c b/drivers/irqchip/irq-dw-apb-ictl.c
index fcc3385..c136b67 100644
--- a/drivers/irqchip/irq-dw-apb-ictl.c
+++ b/drivers/irqchip/irq-dw-apb-ictl.c
@@ -115,6 +115,7 @@ static int __init dw_apb_ictl_init(struct device_node *np,
 
 	ret = irq_alloc_domain_generic_chips(domain, 32, (nrirqs > 32) ? 2 : 1,
 					     np->name, handle_level_irq, clr, 0,
+					     IRQ_GC_MASK_CACHE_PER_TYPE |
 					     IRQ_GC_INIT_MASK_CACHE);
 	if (ret) {
 		pr_err("%s: unable to alloc irq domain gc\n", np->full_name);
-- 
2.1.0

[PATCH 3/3] irqchip: dw-apb-ictl: add PM support

[Jisheng Zhang]

This patch adds in support for S2R for dw-apb-ictl irqchip driver.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/irqchip/irq-dw-apb-ictl.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/irqchip/irq-dw-apb-ictl.c b/drivers/irqchip/irq-dw-apb-ictl.c
index c136b67..53bb732 100644
--- a/drivers/irqchip/irq-dw-apb-ictl.c
+++ b/drivers/irqchip/irq-dw-apb-ictl.c
@@ -50,6 +50,21 @@ static void dw_apb_ictl_handler(unsigned int irq, struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+#ifdef CONFIG_PM
+static void dw_apb_ictl_resume(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+
+	irq_gc_lock(gc);
+	writel_relaxed(~0, gc->reg_base + ct->regs.enable);
+	writel_relaxed(*ct->mask_cache, gc->reg_base + ct->regs.mask);
+	irq_gc_unlock(gc);
+}
+#else
+#define dw_apb_ictl_resume	NULL
+#endif /* CONFIG_PM */
+
 static int __init dw_apb_ictl_init(struct device_node *np,
 				   struct device_node *parent)
 {
@@ -127,13 +142,17 @@ static int __init dw_apb_ictl_init(struct device_node *np,
 	gc->reg_base = iobase;
 
 	gc->chip_types[0].regs.mask = APB_INT_MASK_L;
+	gc->chip_types[0].regs.enable = APB_INT_ENABLE_L;
 	gc->chip_types[0].chip.irq_mask = irq_gc_mask_set_bit;
 	gc->chip_types[0].chip.irq_unmask = irq_gc_mask_clr_bit;
+	gc->chip_types[0].chip.irq_resume = dw_apb_ictl_resume;
 
 	if (nrirqs > 32) {
 		gc->chip_types[1].regs.mask = APB_INT_MASK_H;
+		gc->chip_types[1].regs.enable = APB_INT_ENABLE_H;
 		gc->chip_types[1].chip.irq_mask = irq_gc_mask_set_bit;
 		gc->chip_types[1].chip.irq_unmask = irq_gc_mask_clr_bit;
+		gc->chip_types[1].chip.irq_resume = dw_apb_ictl_resume;
 	}
 
 	irq_set_handler_data(irq, gc);
-- 
2.1.0

Re: [PATCH 1/3] irqchip: dw-apb-ictl: always use use {readl|writel}_relaxed

[Sebastian Hesselbarth]

On 09/23/2014 08:34 AM, Jisheng Zhang wrote:
> relaxed version and non-relaxed version are mixed, this patch always
> use the relaxed version to unify the memory access usage.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>

While I don't agree with the above reason (unification) for this
change, I agree with the patch because it _can_ be changed, so

Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

> ---
>   drivers/irqchip/irq-dw-apb-ictl.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/irqchip/irq-dw-apb-ictl.c b/drivers/irqchip/irq-dw-apb-ictl.c
> index 31e231e..fcc3385 100644
> --- a/drivers/irqchip/irq-dw-apb-ictl.c
> +++ b/drivers/irqchip/irq-dw-apb-ictl.c
> @@ -94,16 +94,16 @@ static int __init dw_apb_ictl_init(struct device_node *np,
>   	 */
>
>   	/* mask and enable all interrupts */
> -	writel(~0, iobase + APB_INT_MASK_L);
> -	writel(~0, iobase + APB_INT_MASK_H);
> -	writel(~0, iobase + APB_INT_ENABLE_L);
> -	writel(~0, iobase + APB_INT_ENABLE_H);
> +	writel_relaxed(~0, iobase + APB_INT_MASK_L);
> +	writel_relaxed(~0, iobase + APB_INT_MASK_H);
> +	writel_relaxed(~0, iobase + APB_INT_ENABLE_L);
> +	writel_relaxed(~0, iobase + APB_INT_ENABLE_H);
>
> -	reg = readl(iobase + APB_INT_ENABLE_H);
> +	reg = readl_relaxed(iobase + APB_INT_ENABLE_H);
>   	if (reg)
>   		nrirqs = 32 + fls(reg);
>   	else
> -		nrirqs = fls(readl(iobase + APB_INT_ENABLE_L));
> +		nrirqs = fls(readl_relaxed(iobase + APB_INT_ENABLE_L));
>
>   	domain = irq_domain_add_linear(np, nrirqs,
>   				       &irq_generic_chip_ops, NULL);
>

Re: [PATCH 2/3] irqchip: dw-apb-ictl: enable IRQ_GC_MASK_CACHE_PER_TYPE

[Sebastian Hesselbarth]

On 09/23/2014 08:34 AM, Jisheng Zhang wrote:
> The irq_chip_type instances have separate mask registers, so we need to
> enable IRQ_GC_MASK_CACHE_PER_TYPE to actually handle separate mask registers.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>

Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

> ---
>   drivers/irqchip/irq-dw-apb-ictl.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/irqchip/irq-dw-apb-ictl.c b/drivers/irqchip/irq-dw-apb-ictl.c
> index fcc3385..c136b67 100644
> --- a/drivers/irqchip/irq-dw-apb-ictl.c
> +++ b/drivers/irqchip/irq-dw-apb-ictl.c
> @@ -115,6 +115,7 @@ static int __init dw_apb_ictl_init(struct device_node *np,
>
>   	ret = irq_alloc_domain_generic_chips(domain, 32, (nrirqs > 32) ? 2 : 1,
>   					     np->name, handle_level_irq, clr, 0,
> +					     IRQ_GC_MASK_CACHE_PER_TYPE |
>   					     IRQ_GC_INIT_MASK_CACHE);
>   	if (ret) {
>   		pr_err("%s: unable to alloc irq domain gc\n", np->full_name);
>

Re: [PATCH 3/3] irqchip: dw-apb-ictl: add PM support

[Sebastian Hesselbarth]

On 09/23/2014 08:35 AM, Jisheng Zhang wrote:
> This patch adds in support for S2R for dw-apb-ictl irqchip driver.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>   drivers/irqchip/irq-dw-apb-ictl.c | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
>
> diff --git a/drivers/irqchip/irq-dw-apb-ictl.c b/drivers/irqchip/irq-dw-apb-ictl.c
> index c136b67..53bb732 100644
> --- a/drivers/irqchip/irq-dw-apb-ictl.c
> +++ b/drivers/irqchip/irq-dw-apb-ictl.c
> @@ -50,6 +50,21 @@ static void dw_apb_ictl_handler(unsigned int irq, struct irq_desc *desc)
>   	chained_irq_exit(chip, desc);
>   }
>
> +#ifdef CONFIG_PM
> +static void dw_apb_ictl_resume(struct irq_data *d)
> +{
> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> +
> +	irq_gc_lock(gc);
> +	writel_relaxed(~0, gc->reg_base + ct->regs.enable);
> +	writel_relaxed(*ct->mask_cache, gc->reg_base + ct->regs.mask);
> +	irq_gc_unlock(gc);
> +}

I agree with the overall change, but may this also be suited for a
generic irq_chip helper instead of being a driver specific one?

Maybe Thomas or Jason can comment on this.

Also, now that you are using writel_relaxed, I understand that both
writes above can happen in any order? Are there any implication we
have to consider, i.e. do we require any of the registers above to
be written first?

Sebastian

> +#else
> +#define dw_apb_ictl_resume	NULL
> +#endif /* CONFIG_PM */
> +
>   static int __init dw_apb_ictl_init(struct device_node *np,
>   				   struct device_node *parent)
>   {
> @@ -127,13 +142,17 @@ static int __init dw_apb_ictl_init(struct device_node *np,
>   	gc->reg_base = iobase;
>
>   	gc->chip_types[0].regs.mask = APB_INT_MASK_L;
> +	gc->chip_types[0].regs.enable = APB_INT_ENABLE_L;
>   	gc->chip_types[0].chip.irq_mask = irq_gc_mask_set_bit;
>   	gc->chip_types[0].chip.irq_unmask = irq_gc_mask_clr_bit;
> +	gc->chip_types[0].chip.irq_resume = dw_apb_ictl_resume;
>
>   	if (nrirqs > 32) {
>   		gc->chip_types[1].regs.mask = APB_INT_MASK_H;
> +		gc->chip_types[1].regs.enable = APB_INT_ENABLE_H;
>   		gc->chip_types[1].chip.irq_mask = irq_gc_mask_set_bit;
>   		gc->chip_types[1].chip.irq_unmask = irq_gc_mask_clr_bit;
> +		gc->chip_types[1].chip.irq_resume = dw_apb_ictl_resume;
>   	}
>
>   	irq_set_handler_data(irq, gc);
>

Re: [PATCH 1/3] irqchip: dw-apb-ictl: always use use {readl|writel}_relaxed

[Thomas Gleixner]

On Tue, 30 Sep 2014, Sebastian Hesselbarth wrote:

> On 09/23/2014 08:34 AM, Jisheng Zhang wrote:
> > relaxed version and non-relaxed version are mixed, this patch always
> > use the relaxed version to unify the memory access usage.
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> 
> While I don't agree with the above reason (unification) for this

Then we want a proper changelog for it, really.

Thanks,

	tglx

Re: [PATCH 3/3] irqchip: dw-apb-ictl: add PM support

[Thomas Gleixner]

On Tue, 30 Sep 2014, Sebastian Hesselbarth wrote:
> On 09/23/2014 08:35 AM, Jisheng Zhang wrote:
> > This patch adds in support for S2R for dw-apb-ictl irqchip driver.
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >   drivers/irqchip/irq-dw-apb-ictl.c | 19 +++++++++++++++++++
> >   1 file changed, 19 insertions(+)
> > 
> > diff --git a/drivers/irqchip/irq-dw-apb-ictl.c
> > b/drivers/irqchip/irq-dw-apb-ictl.c
> > index c136b67..53bb732 100644
> > --- a/drivers/irqchip/irq-dw-apb-ictl.c
> > +++ b/drivers/irqchip/irq-dw-apb-ictl.c
> > @@ -50,6 +50,21 @@ static void dw_apb_ictl_handler(unsigned int irq,
> > struct irq_desc *desc)
> >   	chained_irq_exit(chip, desc);
> >   }
> > 
> > +#ifdef CONFIG_PM
> > +static void dw_apb_ictl_resume(struct irq_data *d)
> > +{
> > +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> > +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> > +
> > +	irq_gc_lock(gc);
> > +	writel_relaxed(~0, gc->reg_base + ct->regs.enable);
> > +	writel_relaxed(*ct->mask_cache, gc->reg_base + ct->regs.mask);
> > +	irq_gc_unlock(gc);
> > +}
> 
> I agree with the overall change, but may this also be suited for a
> generic irq_chip helper instead of being a driver specific one?
> 
> Maybe Thomas or Jason can comment on this.

If we have enough similar resume callbacks, yes. 
 
> Also, now that you are using writel_relaxed, I understand that both
> writes above can happen in any order? Are there any implication we
> have to consider, i.e. do we require any of the registers above to
> be written first?

Was about to ask that as well :)

Thanks,

	tglx

Re: [PATCH 3/3] irqchip: dw-apb-ictl: add PM support

[Jisheng Zhang]

Hi Thomas, Sebastian,

On Tue, 30 Sep 2014 14:52:54 -0700
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Tue, 30 Sep 2014, Sebastian Hesselbarth wrote:
> > On 09/23/2014 08:35 AM, Jisheng Zhang wrote:
> > > This patch adds in support for S2R for dw-apb-ictl irqchip driver.
> > > 
> > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > > ---
> > >   drivers/irqchip/irq-dw-apb-ictl.c | 19 +++++++++++++++++++
> > >   1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/drivers/irqchip/irq-dw-apb-ictl.c
> > > b/drivers/irqchip/irq-dw-apb-ictl.c
> > > index c136b67..53bb732 100644
> > > --- a/drivers/irqchip/irq-dw-apb-ictl.c
> > > +++ b/drivers/irqchip/irq-dw-apb-ictl.c
> > > @@ -50,6 +50,21 @@ static void dw_apb_ictl_handler(unsigned int irq,
> > > struct irq_desc *desc)
> > >   	chained_irq_exit(chip, desc);
> > >   }
> > > 
> > > +#ifdef CONFIG_PM
> > > +static void dw_apb_ictl_resume(struct irq_data *d)
> > > +{
> > > +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> > > +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> > > +
> > > +	irq_gc_lock(gc);
> > > +	writel_relaxed(~0, gc->reg_base + ct->regs.enable);
> > > +	writel_relaxed(*ct->mask_cache, gc->reg_base + ct->regs.mask);
> > > +	irq_gc_unlock(gc);
> > > +}
> > 
> > I agree with the overall change, but may this also be suited for a
> > generic irq_chip helper instead of being a driver specific one?
> > 
> > Maybe Thomas or Jason can comment on this.
> 
> If we have enough similar resume callbacks, yes. 
>  
> > Also, now that you are using writel_relaxed, I understand that both
> > writes above can happen in any order? Are there any implication we
> > have to consider, i.e. do we require any of the registers above to
> > be written first?

The registers sits at device type memory, the writes should happen in the same
order as before.

Thanks for reviewing,
Jisheng

> 
> Was about to ask that as well :)
> 
> Thanks,
> 
> 	tglx

Re: [PATCH 3/3] irqchip: dw-apb-ictl: add PM support

[Sebastian Hesselbarth]

On 10/08/2014 01:31 PM, Jisheng Zhang wrote:
> Hi Thomas, Sebastian,
>
> On Tue, 30 Sep 2014 14:52:54 -0700
> Thomas Gleixner <tglx@linutronix.de> wrote:
>
>> On Tue, 30 Sep 2014, Sebastian Hesselbarth wrote:
>>> On 09/23/2014 08:35 AM, Jisheng Zhang wrote:
>>>> This patch adds in support for S2R for dw-apb-ictl irqchip driver.
>>>>
>>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>>>> ---
>>>>    drivers/irqchip/irq-dw-apb-ictl.c | 19 +++++++++++++++++++
>>>>    1 file changed, 19 insertions(+)
>>>>
>>>> diff --git a/drivers/irqchip/irq-dw-apb-ictl.c
>>>> b/drivers/irqchip/irq-dw-apb-ictl.c
>>>> index c136b67..53bb732 100644
>>>> --- a/drivers/irqchip/irq-dw-apb-ictl.c
>>>> +++ b/drivers/irqchip/irq-dw-apb-ictl.c
>>>> @@ -50,6 +50,21 @@ static void dw_apb_ictl_handler(unsigned int irq,
>>>> struct irq_desc *desc)
>>>>    	chained_irq_exit(chip, desc);
>>>>    }
>>>>
>>>> +#ifdef CONFIG_PM
>>>> +static void dw_apb_ictl_resume(struct irq_data *d)
>>>> +{
>>>> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>>>> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
>>>> +
>>>> +	irq_gc_lock(gc);
>>>> +	writel_relaxed(~0, gc->reg_base + ct->regs.enable);
>>>> +	writel_relaxed(*ct->mask_cache, gc->reg_base + ct->regs.mask);
>>>> +	irq_gc_unlock(gc);
>>>> +}
>>>
>>> I agree with the overall change, but may this also be suited for a
>>> generic irq_chip helper instead of being a driver specific one?
>>>
>>> Maybe Thomas or Jason can comment on this.
>>
>> If we have enough similar resume callbacks, yes.
>>
>>> Also, now that you are using writel_relaxed, I understand that both
>>> writes above can happen in any order? Are there any implication we
>>> have to consider, i.e. do we require any of the registers above to
>>> be written first?
>
> The registers sits at device type memory, the writes should happen in the same
> order as before.

Jisheng,

it is not about the location of the register but, as far as I
understand, when using {readl,writel}_relaxed the compiler is
free to reorder the calls. So, if there is a strict order we
want to ensure, we have to use non-relaxed {readl,writel}.

The performance penalty of non-relaxed calls can be ignored anyway
as it is done only once after resume.

Sebastian

Re: [PATCH 3/3] irqchip: dw-apb-ictl: add PM support

[Jisheng Zhang]

Hi Sebastian,

On Wed, 8 Oct 2014 04:44:49 -0700
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:

> On 10/08/2014 01:31 PM, Jisheng Zhang wrote:
> > Hi Thomas, Sebastian,
> >
> > On Tue, 30 Sep 2014 14:52:54 -0700
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> >> On Tue, 30 Sep 2014, Sebastian Hesselbarth wrote:
> >>> On 09/23/2014 08:35 AM, Jisheng Zhang wrote:
> >>>> This patch adds in support for S2R for dw-apb-ictl irqchip driver.
> >>>>
> >>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >>>> ---
> >>>>    drivers/irqchip/irq-dw-apb-ictl.c | 19 +++++++++++++++++++
> >>>>    1 file changed, 19 insertions(+)
> >>>>
> >>>> diff --git a/drivers/irqchip/irq-dw-apb-ictl.c
> >>>> b/drivers/irqchip/irq-dw-apb-ictl.c
> >>>> index c136b67..53bb732 100644
> >>>> --- a/drivers/irqchip/irq-dw-apb-ictl.c
> >>>> +++ b/drivers/irqchip/irq-dw-apb-ictl.c
> >>>> @@ -50,6 +50,21 @@ static void dw_apb_ictl_handler(unsigned int irq,
> >>>> struct irq_desc *desc)
> >>>>    	chained_irq_exit(chip, desc);
> >>>>    }
> >>>>
> >>>> +#ifdef CONFIG_PM
> >>>> +static void dw_apb_ictl_resume(struct irq_data *d)
> >>>> +{
> >>>> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> >>>> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> >>>> +
> >>>> +	irq_gc_lock(gc);
> >>>> +	writel_relaxed(~0, gc->reg_base + ct->regs.enable);
> >>>> +	writel_relaxed(*ct->mask_cache, gc->reg_base + ct->regs.mask);
> >>>> +	irq_gc_unlock(gc);
> >>>> +}
> >>>
> >>> I agree with the overall change, but may this also be suited for a
> >>> generic irq_chip helper instead of being a driver specific one?
> >>>
> >>> Maybe Thomas or Jason can comment on this.
> >>
> >> If we have enough similar resume callbacks, yes.
> >>
> >>> Also, now that you are using writel_relaxed, I understand that both
> >>> writes above can happen in any order? Are there any implication we
> >>> have to consider, i.e. do we require any of the registers above to
> >>> be written first?
> >
> > The registers sits at device type memory, the writes should happen in the
> > same order as before.
> 
> Jisheng,
> 
> it is not about the location of the register but, as far as I
> understand, when using {readl,writel}_relaxed the compiler is
> free to reorder the calls. So, if there is a strict order we

The "volatile" in readl/writel relaxed implementations should prevent the
compiler to do reorder. Or I misunderstand something?

Thanks,
Jisheng

> want to ensure, we have to use non-relaxed {readl,writel}.
> 
> The performance penalty of non-relaxed calls can be ignored anyway
> as it is done only once after resume.
> 
> Sebastian

Re: [PATCH 3/3] irqchip: dw-apb-ictl: add PM support

[Jisheng Zhang]

Hi Sebastian,

On Wed, 8 Oct 2014 04:50:53 -0700
Jisheng Zhang <jszhang@marvell.com> wrote:

> Hi Sebastian,
> 
> On Wed, 8 Oct 2014 04:44:49 -0700
> Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:
> 
> > On 10/08/2014 01:31 PM, Jisheng Zhang wrote:
> > > Hi Thomas, Sebastian,
> > >
> > > On Tue, 30 Sep 2014 14:52:54 -0700
> > > Thomas Gleixner <tglx@linutronix.de> wrote:
> > >
> > >> On Tue, 30 Sep 2014, Sebastian Hesselbarth wrote:
> > >>> On 09/23/2014 08:35 AM, Jisheng Zhang wrote:
> > >>>> This patch adds in support for S2R for dw-apb-ictl irqchip driver.
> > >>>>
> > >>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > >>>> ---
> > >>>>    drivers/irqchip/irq-dw-apb-ictl.c | 19 +++++++++++++++++++
> > >>>>    1 file changed, 19 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/irqchip/irq-dw-apb-ictl.c
> > >>>> b/drivers/irqchip/irq-dw-apb-ictl.c
> > >>>> index c136b67..53bb732 100644
> > >>>> --- a/drivers/irqchip/irq-dw-apb-ictl.c
> > >>>> +++ b/drivers/irqchip/irq-dw-apb-ictl.c
> > >>>> @@ -50,6 +50,21 @@ static void dw_apb_ictl_handler(unsigned int irq,
> > >>>> struct irq_desc *desc)
> > >>>>    	chained_irq_exit(chip, desc);
> > >>>>    }
> > >>>>
> > >>>> +#ifdef CONFIG_PM
> > >>>> +static void dw_apb_ictl_resume(struct irq_data *d)
> > >>>> +{
> > >>>> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
> > >>>> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
> > >>>> +
> > >>>> +	irq_gc_lock(gc);
> > >>>> +	writel_relaxed(~0, gc->reg_base + ct->regs.enable);
> > >>>> +	writel_relaxed(*ct->mask_cache, gc->reg_base +
> > >>>> ct->regs.mask);
> > >>>> +	irq_gc_unlock(gc);
> > >>>> +}
> > >>>
> > >>> I agree with the overall change, but may this also be suited for a
> > >>> generic irq_chip helper instead of being a driver specific one?
> > >>>
> > >>> Maybe Thomas or Jason can comment on this.
> > >>
> > >> If we have enough similar resume callbacks, yes.
> > >>
> > >>> Also, now that you are using writel_relaxed, I understand that both
> > >>> writes above can happen in any order? Are there any implication we
> > >>> have to consider, i.e. do we require any of the registers above to
> > >>> be written first?
> > >
> > > The registers sits at device type memory, the writes should happen in
> > > the same order as before.
> > 
> > Jisheng,
> > 
> > it is not about the location of the register but, as far as I
> > understand, when using {readl,writel}_relaxed the compiler is
> > free to reorder the calls. So, if there is a strict order we
> 
> The "volatile" in readl/writel relaxed implementations should prevent the
> compiler to do reorder. Or I misunderstand something?

My understanding is that the relaxed version imply compiler barriers.
I'm not sure I understand the real/writel relaxed implementations correctly. But
one obvious example which shows the relaxed version won't have the compiler
reorder issue is drivers/irqchip/irq-gic.c, all the configurations must be done
before enable the GIC which is done by "writel_relaxed(1, cpu_base + GIC_CPU_CTRL);"
However, we didn't see any explicit compiler barriers.

Thanks,
Jisheng


> 
> Thanks,
> Jisheng
> 
> > want to ensure, we have to use non-relaxed {readl,writel}.
> > 
> > The performance penalty of non-relaxed calls can be ignored anyway
> > as it is done only once after resume.
> > 
> > Sebastian
> 

Re: [PATCH 3/3] irqchip: dw-apb-ictl: add PM support

[Sebastian Hesselbarth]

On 10/08/2014 01:58 PM, Jisheng Zhang wrote:
> Hi Sebastian,
>
> On Wed, 8 Oct 2014 04:50:53 -0700
> Jisheng Zhang <jszhang@marvell.com> wrote:
>
>> Hi Sebastian,
>>
>> On Wed, 8 Oct 2014 04:44:49 -0700
>> Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:
>>
>>> On 10/08/2014 01:31 PM, Jisheng Zhang wrote:
>>>> Hi Thomas, Sebastian,
>>>>
>>>> On Tue, 30 Sep 2014 14:52:54 -0700
>>>> Thomas Gleixner <tglx@linutronix.de> wrote:
>>>>
>>>>> On Tue, 30 Sep 2014, Sebastian Hesselbarth wrote:
>>>>>> On 09/23/2014 08:35 AM, Jisheng Zhang wrote:
>>>>>>> This patch adds in support for S2R for dw-apb-ictl irqchip driver.
>>>>>>>
>>>>>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>>>>>>> ---
>>>>>>>     drivers/irqchip/irq-dw-apb-ictl.c | 19 +++++++++++++++++++
>>>>>>>     1 file changed, 19 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/irqchip/irq-dw-apb-ictl.c
>>>>>>> b/drivers/irqchip/irq-dw-apb-ictl.c
>>>>>>> index c136b67..53bb732 100644
>>>>>>> --- a/drivers/irqchip/irq-dw-apb-ictl.c
>>>>>>> +++ b/drivers/irqchip/irq-dw-apb-ictl.c
>>>>>>> @@ -50,6 +50,21 @@ static void dw_apb_ictl_handler(unsigned int irq,
>>>>>>> struct irq_desc *desc)
>>>>>>>     	chained_irq_exit(chip, desc);
>>>>>>>     }
>>>>>>>
>>>>>>> +#ifdef CONFIG_PM
>>>>>>> +static void dw_apb_ictl_resume(struct irq_data *d)
>>>>>>> +{
>>>>>>> +	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>>>>>>> +	struct irq_chip_type *ct = irq_data_get_chip_type(d);
>>>>>>> +
>>>>>>> +	irq_gc_lock(gc);
>>>>>>> +	writel_relaxed(~0, gc->reg_base + ct->regs.enable);
>>>>>>> +	writel_relaxed(*ct->mask_cache, gc->reg_base +
>>>>>>> ct->regs.mask);
>>>>>>> +	irq_gc_unlock(gc);
>>>>>>> +}
>>>>>>
>>>>>> I agree with the overall change, but may this also be suited for a
>>>>>> generic irq_chip helper instead of being a driver specific one?
>>>>>>
>>>>>> Maybe Thomas or Jason can comment on this.
>>>>>
>>>>> If we have enough similar resume callbacks, yes.
>>>>>
>>>>>> Also, now that you are using writel_relaxed, I understand that both
>>>>>> writes above can happen in any order? Are there any implication we
>>>>>> have to consider, i.e. do we require any of the registers above to
>>>>>> be written first?
>>>>
>>>> The registers sits at device type memory, the writes should happen in
>>>> the same order as before.
>>>
>>> it is not about the location of the register but, as far as I
>>> understand, when using {readl,writel}_relaxed the compiler is
>>> free to reorder the calls. So, if there is a strict order we
>>
>> The "volatile" in readl/writel relaxed implementations should prevent the
>> compiler to do reorder. Or I misunderstand something?
>
> My understanding is that the relaxed version imply compiler barriers.
> I'm not sure I understand the real/writel relaxed implementations correctly. But
> one obvious example which shows the relaxed version won't have the compiler
> reorder issue is drivers/irqchip/irq-gic.c, all the configurations must be done
> before enable the GIC which is done by "writel_relaxed(1, cpu_base + GIC_CPU_CTRL);"
> However, we didn't see any explicit compiler barriers.

Yup, I just checked the discussion here:
http://comments.gmane.org/gmane.linux.ports.arm.kernel/117626

You are right, write order is ensured.

Sebastian

Re: [PATCH 0/3] irqchip: dw-apb-ictl: IRQ_GC_MASK_CACHE_PER_TYPE and PM support

[Jisheng Zhang]

Dear Thomas, Jason,

Is there any potential issue with this patch serials I need to resolve?

Thanks in advance,
Jisheng

On Mon, 22 Sep 2014 23:34:57 -0700
Jisheng Zhang <jszhang@marvell.com> wrote:

> These patches try to improve dw-apb-ictl irqchip driver a bit.
> 
> The first patch does a bit clean up work -- unify the register access usage.
> 
> The two dw-apb-ictl's irq_chip_type instances have separate mask registers,
> so the second patch enables IRQ_GC_MASK_CACHE_PER_TYPE.
> 
> The last patch adds suspend/resume support to the driver.
> 
> Tested on Marvell BG2Q-DMP board.
> 
> Jisheng Zhang (3):
>   irqchip: dw-apb-ictl: always use use {readl|writel}_relaxed
>   irqchip: dw-apb-ictl: enable IRQ_GC_MASK_CACHE_PER_TYPE
>   irqchip: dw-apb-ictl: add PM support
> 
>  drivers/irqchip/irq-dw-apb-ictl.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 

Re: [PATCH 0/3] irqchip: dw-apb-ictl: IRQ_GC_MASK_CACHE_PER_TYPE and PM support

[Jason Cooper]

On Tue, Nov 11, 2014 at 01:59:47PM +0800, Jisheng Zhang wrote:
> Dear Thomas, Jason,
> 
> Is there any potential issue with this patch serials I need to resolve?

Please send a v2 with Sebastian's Acks.  Also, please update the commit
log on #1 as requested.  For #3, Please add a summary of the discussion
to its commit log.  eg: "We can used relaxed variants here because..."

thx,

Jason.