| Message ID | 1411454100-6814-4-git-send-email-jszhang@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); >
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
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
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
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
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 >
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
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);
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(+)