* [PATCH 0/3] irqchip: dw-apb-ictl: IRQ_GC_MASK_CACHE_PER_TYPE and PM support
@ 2014-09-23 6:34 Jisheng Zhang
2014-09-23 6:34 ` [PATCH 1/3] irqchip: dw-apb-ictl: always use use {readl|writel}_relaxed Jisheng Zhang
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Jisheng Zhang @ 2014-09-23 6:34 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/3] irqchip: dw-apb-ictl: always use use {readl|writel}_relaxed 2014-09-23 6:34 [PATCH 0/3] irqchip: dw-apb-ictl: IRQ_GC_MASK_CACHE_PER_TYPE and PM support Jisheng Zhang @ 2014-09-23 6:34 ` Jisheng Zhang 2014-09-30 12:25 ` Sebastian Hesselbarth 2014-09-23 6:34 ` [PATCH 2/3] irqchip: dw-apb-ictl: enable IRQ_GC_MASK_CACHE_PER_TYPE Jisheng Zhang ` (2 subsequent siblings) 3 siblings, 1 reply; 16+ messages in thread From: Jisheng Zhang @ 2014-09-23 6:34 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 1/3] irqchip: dw-apb-ictl: always use use {readl|writel}_relaxed 2014-09-23 6:34 ` [PATCH 1/3] irqchip: dw-apb-ictl: always use use {readl|writel}_relaxed Jisheng Zhang @ 2014-09-30 12:25 ` Sebastian Hesselbarth 2014-09-30 21:50 ` Thomas Gleixner 0 siblings, 1 reply; 16+ messages in thread From: Sebastian Hesselbarth @ 2014-09-30 12:25 UTC (permalink / raw) To: linux-arm-kernel 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); > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] irqchip: dw-apb-ictl: always use use {readl|writel}_relaxed 2014-09-30 12:25 ` Sebastian Hesselbarth @ 2014-09-30 21:50 ` Thomas Gleixner 0 siblings, 0 replies; 16+ messages in thread From: Thomas Gleixner @ 2014-09-30 21:50 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] irqchip: dw-apb-ictl: enable IRQ_GC_MASK_CACHE_PER_TYPE 2014-09-23 6:34 [PATCH 0/3] irqchip: dw-apb-ictl: IRQ_GC_MASK_CACHE_PER_TYPE and PM support Jisheng Zhang 2014-09-23 6:34 ` [PATCH 1/3] irqchip: dw-apb-ictl: always use use {readl|writel}_relaxed Jisheng Zhang @ 2014-09-23 6:34 ` Jisheng Zhang 2014-09-30 12:28 ` Sebastian Hesselbarth 2014-09-23 6:35 ` [PATCH 3/3] irqchip: dw-apb-ictl: add PM support Jisheng Zhang 2014-11-11 5:59 ` [PATCH 0/3] irqchip: dw-apb-ictl: IRQ_GC_MASK_CACHE_PER_TYPE and " Jisheng Zhang 3 siblings, 1 reply; 16+ messages in thread From: Jisheng Zhang @ 2014-09-23 6:34 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] irqchip: dw-apb-ictl: enable IRQ_GC_MASK_CACHE_PER_TYPE 2014-09-23 6:34 ` [PATCH 2/3] irqchip: dw-apb-ictl: enable IRQ_GC_MASK_CACHE_PER_TYPE Jisheng Zhang @ 2014-09-30 12:28 ` Sebastian Hesselbarth 0 siblings, 0 replies; 16+ messages in thread From: Sebastian Hesselbarth @ 2014-09-30 12:28 UTC (permalink / raw) To: linux-arm-kernel 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); > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] irqchip: dw-apb-ictl: add PM support 2014-09-23 6:34 [PATCH 0/3] irqchip: dw-apb-ictl: IRQ_GC_MASK_CACHE_PER_TYPE and PM support Jisheng Zhang 2014-09-23 6:34 ` [PATCH 1/3] irqchip: dw-apb-ictl: always use use {readl|writel}_relaxed Jisheng Zhang 2014-09-23 6:34 ` [PATCH 2/3] irqchip: dw-apb-ictl: enable IRQ_GC_MASK_CACHE_PER_TYPE Jisheng Zhang @ 2014-09-23 6:35 ` Jisheng Zhang 2014-09-30 12:33 ` Sebastian Hesselbarth 2014-11-11 5:59 ` [PATCH 0/3] irqchip: dw-apb-ictl: IRQ_GC_MASK_CACHE_PER_TYPE and " Jisheng Zhang 3 siblings, 1 reply; 16+ messages in thread From: Jisheng Zhang @ 2014-09-23 6:35 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] irqchip: dw-apb-ictl: add PM support 2014-09-23 6:35 ` [PATCH 3/3] irqchip: dw-apb-ictl: add PM support Jisheng Zhang @ 2014-09-30 12:33 ` Sebastian Hesselbarth 2014-09-30 21:52 ` Thomas Gleixner 0 siblings, 1 reply; 16+ messages in thread From: Sebastian Hesselbarth @ 2014-09-30 12:33 UTC (permalink / raw) To: linux-arm-kernel 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); > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] irqchip: dw-apb-ictl: add PM support 2014-09-30 12:33 ` Sebastian Hesselbarth @ 2014-09-30 21:52 ` Thomas Gleixner 2014-10-08 11:31 ` Jisheng Zhang 0 siblings, 1 reply; 16+ messages in thread From: Thomas Gleixner @ 2014-09-30 21:52 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] irqchip: dw-apb-ictl: add PM support 2014-09-30 21:52 ` Thomas Gleixner @ 2014-10-08 11:31 ` Jisheng Zhang 2014-10-08 11:44 ` Sebastian Hesselbarth 0 siblings, 1 reply; 16+ messages in thread From: Jisheng Zhang @ 2014-10-08 11:31 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] irqchip: dw-apb-ictl: add PM support 2014-10-08 11:31 ` Jisheng Zhang @ 2014-10-08 11:44 ` Sebastian Hesselbarth 2014-10-08 11:50 ` Jisheng Zhang 0 siblings, 1 reply; 16+ messages in thread From: Sebastian Hesselbarth @ 2014-10-08 11:44 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] irqchip: dw-apb-ictl: add PM support 2014-10-08 11:44 ` Sebastian Hesselbarth @ 2014-10-08 11:50 ` Jisheng Zhang 2014-10-08 11:58 ` Jisheng Zhang 0 siblings, 1 reply; 16+ messages in thread From: Jisheng Zhang @ 2014-10-08 11:50 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] irqchip: dw-apb-ictl: add PM support 2014-10-08 11:50 ` Jisheng Zhang @ 2014-10-08 11:58 ` Jisheng Zhang 2014-10-08 18:19 ` Sebastian Hesselbarth 0 siblings, 1 reply; 16+ messages in thread From: Jisheng Zhang @ 2014-10-08 11:58 UTC (permalink / raw) To: linux-arm-kernel 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] irqchip: dw-apb-ictl: add PM support 2014-10-08 11:58 ` Jisheng Zhang @ 2014-10-08 18:19 ` Sebastian Hesselbarth 0 siblings, 0 replies; 16+ messages in thread From: Sebastian Hesselbarth @ 2014-10-08 18:19 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 0/3] irqchip: dw-apb-ictl: IRQ_GC_MASK_CACHE_PER_TYPE and PM support 2014-09-23 6:34 [PATCH 0/3] irqchip: dw-apb-ictl: IRQ_GC_MASK_CACHE_PER_TYPE and PM support Jisheng Zhang ` (2 preceding siblings ...) 2014-09-23 6:35 ` [PATCH 3/3] irqchip: dw-apb-ictl: add PM support Jisheng Zhang @ 2014-11-11 5:59 ` Jisheng Zhang 2014-11-11 13:45 ` Jason Cooper 3 siblings, 1 reply; 16+ messages in thread From: Jisheng Zhang @ 2014-11-11 5:59 UTC (permalink / raw) To: linux-arm-kernel 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(-) > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 0/3] irqchip: dw-apb-ictl: IRQ_GC_MASK_CACHE_PER_TYPE and PM support 2014-11-11 5:59 ` [PATCH 0/3] irqchip: dw-apb-ictl: IRQ_GC_MASK_CACHE_PER_TYPE and " Jisheng Zhang @ 2014-11-11 13:45 ` Jason Cooper 0 siblings, 0 replies; 16+ messages in thread From: Jason Cooper @ 2014-11-11 13:45 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2014-11-11 13:45 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-23 6:34 [PATCH 0/3] irqchip: dw-apb-ictl: IRQ_GC_MASK_CACHE_PER_TYPE and PM support Jisheng Zhang
2014-09-23 6:34 ` [PATCH 1/3] irqchip: dw-apb-ictl: always use use {readl|writel}_relaxed Jisheng Zhang
2014-09-30 12:25 ` Sebastian Hesselbarth
2014-09-30 21:50 ` Thomas Gleixner
2014-09-23 6:34 ` [PATCH 2/3] irqchip: dw-apb-ictl: enable IRQ_GC_MASK_CACHE_PER_TYPE Jisheng Zhang
2014-09-30 12:28 ` Sebastian Hesselbarth
2014-09-23 6:35 ` [PATCH 3/3] irqchip: dw-apb-ictl: add PM support Jisheng Zhang
2014-09-30 12:33 ` Sebastian Hesselbarth
2014-09-30 21:52 ` Thomas Gleixner
2014-10-08 11:31 ` Jisheng Zhang
2014-10-08 11:44 ` Sebastian Hesselbarth
2014-10-08 11:50 ` Jisheng Zhang
2014-10-08 11:58 ` Jisheng Zhang
2014-10-08 18:19 ` Sebastian Hesselbarth
2014-11-11 5:59 ` [PATCH 0/3] irqchip: dw-apb-ictl: IRQ_GC_MASK_CACHE_PER_TYPE and " Jisheng Zhang
2014-11-11 13:45 ` Jason Cooper
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).