Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1425109 > unrolled thread

RE: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB suspend

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB  suspend "Yang, Wenyou" <Wenyou.Yang@atmel.com> - 2016-06-17 15:50 +0200
    Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB  suspend Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-06-17 16:00 +0200
      RE: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB  suspend "Yang, Wenyou" <Wenyou.Yang@atmel.com> - 2016-06-20 05:20 +0200
        Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB  suspend Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-06-20 10:40 +0200
          Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB  suspend Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-06-20 11:00 +0200
            Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB  suspend Nicolas Ferre <nicolas.ferre@atmel.com> - 2016-06-20 11:30 +0200
          RE: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB  suspend "Yang, Wenyou" <Wenyou.Yang@atmel.com> - 2016-06-20 11:00 +0200

#1425109 — RE: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB suspend

Hi Alexandre,

> -----Original Message-----
> From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com]
> Sent: 2016年6月9日 4:38
> To: Rob Herring <robh@kernel.org>
> Cc: Yang, Wenyou <Wenyou.Yang@atmel.com>; Alan Stern
> <stern@rowland.harvard.edu>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Ferre, Nicolas <Nicolas.FERRE@atmel.com>;
> Pawel Moll <pawel.moll@arm.com>; Mark Brown <broonie@kernel.org>; Ian
> Campbell <ijc+devicetree@hellion.org.uk>; Kumar Gala <galak@codeaurora.org>;
> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-usb@vger.kernel.org
> Subject: Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB
> suspend
> 
> On 08/06/2016 at 15:26:51 -0500, Rob Herring wrote :
> > On Wed, Jun 08, 2016 at 12:15:10PM +0800, Wenyou Yang wrote:
> > > In order to the save power consumption, as a workaround, suspend
> > > forcibly the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI
> > > Interrupt Configuration Register in the SFRs while OHCI USB suspend.
> > >
> > > This suspend operation must be done before the USB clock is
> > > disabled, resume after the USB clock is enabled.
> > >
> > > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > > ---
> > >
> > > Changes in v3:
> > >  - Change the compatible description for more precise.
> > >
> > > Changes in v2:
> > >  - Add compatible to support forcibly suspend the ports.
> > >  - Add soc/at91/at91_sfr.h to accommodate the defines.
> > >  - Add error checking for .sfr_regmap.
> > >  - Remove unnecessary regmap_read() statement.
> > >
> > >  .../devicetree/bindings/usb/atmel-usb.txt          |  6 +-
> > >  drivers/usb/host/ohci-at91.c                       | 80 +++++++++++++++++++++-
> > >  include/soc/at91/at91_sfr.h                        | 29 ++++++++
> > >  3 files changed, 112 insertions(+), 3 deletions(-)  create mode
> > > 100644 include/soc/at91/at91_sfr.h
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > > b/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > > index 5883b73..888deaa 100644
> > > --- a/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > > +++ b/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > > @@ -3,8 +3,10 @@ Atmel SOC USB controllers  OHCI
> > >
> > >  Required properties:
> > > - - compatible: Should be "atmel,at91rm9200-ohci" for USB controllers
> > > -   used in host mode.
> > > + - compatible: Should be one of the following
> > > +	       "atmel,at91rm9200-ohci" for USB controllers used in host mode.
> > > +	       "atmel,sama5d2-ohci" for USB controllers used in host mode
> > > +	       on SAMA5D2 which can force to suspend.
> >
> > Guess I wasn't clear enough before. Drop "which can force to suspend".
> >
> 
> Well, my point is that we don't need a new compatible anyway.

Could you give some advice?.

> 
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux, Kernel and Android engineering http://free-electrons.com


Best Regards,
Wenyou Yang

[toc] | [next] | [standalone]

#1425113

On 17/06/2016 at 13:44:22 +0000, Yang, Wenyou wrote :
> Hi Alexandre,
> 
> > -----Original Message-----
> > From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com]
> > Sent: 2016年6月9日 4:38
> > To: Rob Herring <robh@kernel.org>
> > Cc: Yang, Wenyou <Wenyou.Yang@atmel.com>; Alan Stern
> > <stern@rowland.harvard.edu>; Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org>; Ferre, Nicolas <Nicolas.FERRE@atmel.com>;
> > Pawel Moll <pawel.moll@arm.com>; Mark Brown <broonie@kernel.org>; Ian
> > Campbell <ijc+devicetree@hellion.org.uk>; Kumar Gala <galak@codeaurora.org>;
> > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-usb@vger.kernel.org
> > Subject: Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB
> > suspend
> > 
> > On 08/06/2016 at 15:26:51 -0500, Rob Herring wrote :
> > > On Wed, Jun 08, 2016 at 12:15:10PM +0800, Wenyou Yang wrote:
> > > > In order to the save power consumption, as a workaround, suspend
> > > > forcibly the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI
> > > > Interrupt Configuration Register in the SFRs while OHCI USB suspend.
> > > >
> > > > This suspend operation must be done before the USB clock is
> > > > disabled, resume after the USB clock is enabled.
> > > >
> > > > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > > > ---
> > > >
> > > > Changes in v3:
> > > >  - Change the compatible description for more precise.
> > > >
> > > > Changes in v2:
> > > >  - Add compatible to support forcibly suspend the ports.
> > > >  - Add soc/at91/at91_sfr.h to accommodate the defines.
> > > >  - Add error checking for .sfr_regmap.
> > > >  - Remove unnecessary regmap_read() statement.
> > > >
> > > >  .../devicetree/bindings/usb/atmel-usb.txt          |  6 +-
> > > >  drivers/usb/host/ohci-at91.c                       | 80 +++++++++++++++++++++-
> > > >  include/soc/at91/at91_sfr.h                        | 29 ++++++++
> > > >  3 files changed, 112 insertions(+), 3 deletions(-)  create mode
> > > > 100644 include/soc/at91/at91_sfr.h
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > > > b/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > > > index 5883b73..888deaa 100644
> > > > --- a/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > > > +++ b/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > > > @@ -3,8 +3,10 @@ Atmel SOC USB controllers  OHCI
> > > >
> > > >  Required properties:
> > > > - - compatible: Should be "atmel,at91rm9200-ohci" for USB controllers
> > > > -   used in host mode.
> > > > + - compatible: Should be one of the following
> > > > +	       "atmel,at91rm9200-ohci" for USB controllers used in host mode.
> > > > +	       "atmel,sama5d2-ohci" for USB controllers used in host mode
> > > > +	       on SAMA5D2 which can force to suspend.
> > >
> > > Guess I wasn't clear enough before. Drop "which can force to suspend".
> > >
> > 
> > Well, my point is that we don't need a new compatible anyway.
> 
> Could you give some advice?.
> 

Sure, what I mean is that you can try to get the regmap for the SFR in
every case. Depending on whether you were able to get it, you can decide
to call ohci_at91_port_suspend/resume or not (just test for
sfr_regmap != NULL).

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1426160

Hi Aleandre,

> -----Original Message-----
> From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com]
> Sent: 2016年6月17日 21:55
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>
> Cc: Rob Herring <robh@kernel.org>; Alan Stern <stern@rowland.harvard.edu>;
> Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ferre, Nicolas
> <Nicolas.FERRE@atmel.com>; Pawel Moll <pawel.moll@arm.com>; Mark Brown
> <broonie@kernel.org>; Ian Campbell <ijc+devicetree@hellion.org.uk>; Kumar
> Gala <galak@codeaurora.org>; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> usb@vger.kernel.org
> Subject: Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB
> suspend
> 
> On 17/06/2016 at 13:44:22 +0000, Yang, Wenyou wrote :
> > Hi Alexandre,
> >
> > > -----Original Message-----
> > > From: Alexandre Belloni
> > > [mailto:alexandre.belloni@free-electrons.com]
> > > Sent: 2016年6月9日 4:38
> > > To: Rob Herring <robh@kernel.org>
> > > Cc: Yang, Wenyou <Wenyou.Yang@atmel.com>; Alan Stern
> > > <stern@rowland.harvard.edu>; Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org>; Ferre, Nicolas
> > > <Nicolas.FERRE@atmel.com>; Pawel Moll <pawel.moll@arm.com>; Mark
> > > Brown <broonie@kernel.org>; Ian Campbell
> > > <ijc+devicetree@hellion.org.uk>; Kumar Gala <galak@codeaurora.org>;
> > > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > > kernel@lists.infradead.org; linux-usb@vger.kernel.org
> > > Subject: Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports
> > > while USB suspend
> > >
> > > On 08/06/2016 at 15:26:51 -0500, Rob Herring wrote :
> > > > On Wed, Jun 08, 2016 at 12:15:10PM +0800, Wenyou Yang wrote:
> > > > > In order to the save power consumption, as a workaround, suspend
> > > > > forcibly the USB PORTA/B/C via set the SUSPEND_A/B/C bits of
> > > > > OHCI Interrupt Configuration Register in the SFRs while OHCI USB
> suspend.
> > > > >
> > > > > This suspend operation must be done before the USB clock is
> > > > > disabled, resume after the USB clock is enabled.
> > > > >
> > > > > Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> > > > > ---
> > > > >
> > > > > Changes in v3:
> > > > >  - Change the compatible description for more precise.
> > > > >
> > > > > Changes in v2:
> > > > >  - Add compatible to support forcibly suspend the ports.
> > > > >  - Add soc/at91/at91_sfr.h to accommodate the defines.
> > > > >  - Add error checking for .sfr_regmap.
> > > > >  - Remove unnecessary regmap_read() statement.
> > > > >
> > > > >  .../devicetree/bindings/usb/atmel-usb.txt          |  6 +-
> > > > >  drivers/usb/host/ohci-at91.c                       | 80
> +++++++++++++++++++++-
> > > > >  include/soc/at91/at91_sfr.h                        | 29 ++++++++
> > > > >  3 files changed, 112 insertions(+), 3 deletions(-)  create mode
> > > > > 100644 include/soc/at91/at91_sfr.h
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > > > > b/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > > > > index 5883b73..888deaa 100644
> > > > > --- a/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > > > > +++ b/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > > > > @@ -3,8 +3,10 @@ Atmel SOC USB controllers  OHCI
> > > > >
> > > > >  Required properties:
> > > > > - - compatible: Should be "atmel,at91rm9200-ohci" for USB controllers
> > > > > -   used in host mode.
> > > > > + - compatible: Should be one of the following
> > > > > +	       "atmel,at91rm9200-ohci" for USB controllers used in host
> mode.
> > > > > +	       "atmel,sama5d2-ohci" for USB controllers used in host mode
> > > > > +	       on SAMA5D2 which can force to suspend.
> > > >
> > > > Guess I wasn't clear enough before. Drop "which can force to suspend".
> > > >
> > >
> > > Well, my point is that we don't need a new compatible anyway.
> >
> > Could you give some advice?.
> >
> 
> Sure, what I mean is that you can try to get the regmap for the SFR in every case.
> Depending on whether you were able to get it, you can decide to call
> ohci_at91_port_suspend/resume or not (just test for sfr_regmap != NULL).

I don't think so. The SFR includes a lot of miscellaneous functions, more than this one.

> 
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux, Kernel and Android engineering http://free-electrons.com


Best Regards,
Wenyou Yang

[toc] | [prev] | [next] | [standalone]

#1426332

On 20/06/2016 at 03:16:35 +0000, Yang, Wenyou wrote :
> > Sure, what I mean is that you can try to get the regmap for the SFR in every case.
> > Depending on whether you were able to get it, you can decide to call
> > ohci_at91_port_suspend/resume or not (just test for sfr_regmap != NULL).
> 
> I don't think so. The SFR includes a lot of miscellaneous functions, more than this one.
> 

I know but this is irrelevant to this discussion. If you need to use the
SFR from another driver you will simply get it from that other driver.

I that case, you will try to get "atmel,sama5d2-sfr". It is only present
on sama5d2 so you have enough information to know whether or not you can
use it to suspend/resume.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1426346

On 20/06/2016 at 08:46:02 +0000, Yang, Wenyou wrote :
> Hi Alexandre & Nicolas,
> 
> > -----Original Message-----
> > From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com]
> > Sent: 2016年6月20日 16:04
> > To: Yang, Wenyou <Wenyou.Yang@atmel.com>
> > Cc: Rob Herring <robh@kernel.org>; Alan Stern <stern@rowland.harvard.edu>;
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ferre, Nicolas
> > <Nicolas.FERRE@atmel.com>; Pawel Moll <pawel.moll@arm.com>; Mark Brown
> > <broonie@kernel.org>; Ian Campbell <ijc+devicetree@hellion.org.uk>; Kumar
> > Gala <galak@codeaurora.org>; linux-kernel@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> > usb@vger.kernel.org
> > Subject: Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB
> > suspend
> > 
> > On 20/06/2016 at 03:16:35 +0000, Yang, Wenyou wrote :
> > > > Sure, what I mean is that you can try to get the regmap for the SFR in every
> > case.
> > > > Depending on whether you were able to get it, you can decide to call
> > > > ohci_at91_port_suspend/resume or not (just test for sfr_regmap != NULL).
> > >
> > > I don't think so. The SFR includes a lot of miscellaneous functions, more than
> > this one.
> > >
> > 
> > I know but this is irrelevant to this discussion. If you need to use the SFR from
> > another driver you will simply get it from that other driver.
> > 
> > I that case, you will try to get "atmel,sama5d2-sfr". It is only present on sama5d2
> > so you have enough information to know whether or not you can use it to
> > suspend/resume.
> 
> I understand what your meaning :).
> Use "atmel,sama5d2-sfr" compatible to distinguish whether forcibly suspend USB port via SFR or not.
> 
> I am not sure if it is a better solution.
> 

It is definitively superior. There is only one lookup in the device tree
instead of two because whatever happens, you will have to get the SFR
regmap and you don't need to add a compatible string for an IP that
didn't change.

> Nicolas, could you give your opinion?
> 
> > 
> > --
> > Alexandre Belloni, Free Electrons
> > Embedded Linux, Kernel and Android engineering http://free-electrons.com
> 
> 
> Best Regards,
> Wenyou Yang

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1426374

Le 20/06/2016 10:52, Alexandre Belloni a écrit :
> On 20/06/2016 at 08:46:02 +0000, Yang, Wenyou wrote :
>> Hi Alexandre & Nicolas,
>>
>>> -----Original Message-----
>>> From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com]
>>> Sent: 2016年6月20日 16:04
>>> To: Yang, Wenyou <Wenyou.Yang@atmel.com>
>>> Cc: Rob Herring <robh@kernel.org>; Alan Stern <stern@rowland.harvard.edu>;
>>> Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ferre, Nicolas
>>> <Nicolas.FERRE@atmel.com>; Pawel Moll <pawel.moll@arm.com>; Mark Brown
>>> <broonie@kernel.org>; Ian Campbell <ijc+devicetree@hellion.org.uk>; Kumar
>>> Gala <galak@codeaurora.org>; linux-kernel@vger.kernel.org;
>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>>> usb@vger.kernel.org
>>> Subject: Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB
>>> suspend
>>>
>>> On 20/06/2016 at 03:16:35 +0000, Yang, Wenyou wrote :
>>>>> Sure, what I mean is that you can try to get the regmap for the SFR in every
>>> case.
>>>>> Depending on whether you were able to get it, you can decide to call
>>>>> ohci_at91_port_suspend/resume or not (just test for sfr_regmap != NULL).
>>>>
>>>> I don't think so. The SFR includes a lot of miscellaneous functions, more than
>>> this one.
>>>>
>>>
>>> I know but this is irrelevant to this discussion. If you need to use the SFR from
>>> another driver you will simply get it from that other driver.
>>>
>>> I that case, you will try to get "atmel,sama5d2-sfr". It is only present on sama5d2
>>> so you have enough information to know whether or not you can use it to
>>> suspend/resume.
>>
>> I understand what your meaning :).
>> Use "atmel,sama5d2-sfr" compatible to distinguish whether forcibly suspend USB port via SFR or not.
>>
>> I am not sure if it is a better solution.
>>
> 
> It is definitively superior. There is only one lookup in the device tree
> instead of two because whatever happens, you will have to get the SFR
> regmap and you don't need to add a compatible string for an IP that
> didn't change.
> 
>> Nicolas, could you give your opinion?

I'll paraphrase Alexandre but this is what I understood:

Having the information in one place and not having to managed the
synchronization with 2 potential sources of information is clearly an
advantage of Alexandre's solution.

If the next SoC has the same workaround/feature, we will anyway have a
different SFR string to cling to...
So it won't change much and we won't have the confusion of having the
same sama5d2 compatible string on the OHCI side (same behavior) and
different compatible string on the SFR side (probably a new SFR for a
new SoC...).

If the next SoC doesn't have this workaround/feature... well, it's
simple, we don't look for the SFR, we don't use the bits, and we come
back to the situation that we've always experienced ; with the same
compatibility sting for OHCI as the IP never actually changed...

In conclusion: try Alexandre's solution and we'll certainly find that
it's actually simpler.

Bonus point: it voids the discussion on the OHCI compatible string
descriptions!

Bye,

>>> Alexandre Belloni, Free Electrons
>>> Embedded Linux, Kernel and Android engineering http://free-electrons.com
>>
>>
>> Best Regards,
>> Wenyou Yang
> 


-- 
Nicolas Ferre

[toc] | [prev] | [next] | [standalone]

#1426347

Hi Alexandre & Nicolas,

> -----Original Message-----
> From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com]
> Sent: 2016年6月20日 16:04
> To: Yang, Wenyou <Wenyou.Yang@atmel.com>
> Cc: Rob Herring <robh@kernel.org>; Alan Stern <stern@rowland.harvard.edu>;
> Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Ferre, Nicolas
> <Nicolas.FERRE@atmel.com>; Pawel Moll <pawel.moll@arm.com>; Mark Brown
> <broonie@kernel.org>; Ian Campbell <ijc+devicetree@hellion.org.uk>; Kumar
> Gala <galak@codeaurora.org>; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> usb@vger.kernel.org
> Subject: Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB
> suspend
> 
> On 20/06/2016 at 03:16:35 +0000, Yang, Wenyou wrote :
> > > Sure, what I mean is that you can try to get the regmap for the SFR in every
> case.
> > > Depending on whether you were able to get it, you can decide to call
> > > ohci_at91_port_suspend/resume or not (just test for sfr_regmap != NULL).
> >
> > I don't think so. The SFR includes a lot of miscellaneous functions, more than
> this one.
> >
> 
> I know but this is irrelevant to this discussion. If you need to use the SFR from
> another driver you will simply get it from that other driver.
> 
> I that case, you will try to get "atmel,sama5d2-sfr". It is only present on sama5d2
> so you have enough information to know whether or not you can use it to
> suspend/resume.

I understand what your meaning :).
Use "atmel,sama5d2-sfr" compatible to distinguish whether forcibly suspend USB port via SFR or not.

I am not sure if it is a better solution.

Nicolas, could you give your opinion?

> 
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux, Kernel and Android engineering http://free-electrons.com


Best Regards,
Wenyou Yang

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web