Hi Thomas, Sebastian,
On Tue, 30 Sep 2014 14:52:54 -0700
Thomas Gleixner <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>>> ---
> >>>> 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 <[email protected]> wrote:
> Hi Sebastian,
>
> On Wed, 8 Oct 2014 04:44:49 -0700
> Sebastian Hesselbarth <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > >>>> ---
> > >>>> 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 <[email protected]> wrote:
>
>> Hi Sebastian,
>>
>> On Wed, 8 Oct 2014 04:44:49 -0700
>> Sebastian Hesselbarth <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>>>> ---
>>>>>>> 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