subject:"RE\: \[PATCH\] usb\: ohci\-at91\: Suspend the ports while USB suspending"

RE: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

2016-08-03 Thread Yang, Wenyou

Hi Alan,

From: Alan Stern [st...@rowland.harvard.edu]
Sent: Friday, May 13, 2016 2:11
To: Yang, Wenyou
Cc: Greg Kroah-Hartman; Ferre, Nicolas; linux-...@vger.kernel.org; 
linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org
Subject: Re: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

On Thu, 12 May 2016, Wenyou Yang wrote:

> In order to get lower consumption, as a workaround, suspend
> the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI Interrupt
> Configuration Register while OHCI USB suspending.

What does this mean?  What does suspending a port do?  Is it the same
as a normal USB port suspend?

If it is the same, why doesn't the USB_PORT_FEAT_SUSPEND subcase of the
SetPortFeature case in ohci_hub_control() already take care of this?

Yes, you are right. 

We can use  the USB_PORT_FEAT_SUSPEND subcase of the
SetPortFeature case to take care this.

I will send a new patch, please help review it. Thanks a lot.

> This suspend operation must be done before stopping the USB clock,
> resume after the USB clock enabled.
>
> Signed-off-by: Wenyou Yang <wenyou.y...@atmel.com>
> ---

> @@ -132,6 +135,17 @@ static void at91_stop_hc(struct platform_device *pdev)
>
>  /*-*/
>
> +struct regmap *at91_dt_syscon_sfr(void)
> +{
> + struct regmap *regmap;
> +
> + regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
> + if (IS_ERR(regmap))
> + return NULL;

If you get an error, the regmap pointer is set to NULL...

> @@ -197,6 +211,8 @@ static int usb_hcd_at91_probe(const struct hc_driver 
> *driver,
>   goto err;
>   }
>
> + ohci_at91->sfr_regmap = at91_dt_syscon_sfr();

With no other error checking...

> +
>   board = hcd->self.controller->platform_data;
>   ohci = hcd_to_ohci(hcd);
>   ohci->num_ports = board->ports;

> +static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable)
> +{
> + u32 regval;
> + int ret;
> +
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + ret = regmap_read(regmap, SFR_OHCIICR, );

And now what happens if regmap is NULL?  Hint: It won't be pretty...

Alan Stern


Best Regards,
Wenyou Yang

RE: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

2016-08-03 Thread Yang, Wenyou

Hi Alan,

From: Alan Stern [st...@rowland.harvard.edu]
Sent: Friday, May 13, 2016 2:11
To: Yang, Wenyou
Cc: Greg Kroah-Hartman; Ferre, Nicolas; linux-...@vger.kernel.org; 
linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org
Subject: Re: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

On Thu, 12 May 2016, Wenyou Yang wrote:

> In order to get lower consumption, as a workaround, suspend
> the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI Interrupt
> Configuration Register while OHCI USB suspending.

What does this mean?  What does suspending a port do?  Is it the same
as a normal USB port suspend?

If it is the same, why doesn't the USB_PORT_FEAT_SUSPEND subcase of the
SetPortFeature case in ohci_hub_control() already take care of this?

Yes, you are right. 

We can use  the USB_PORT_FEAT_SUSPEND subcase of the
SetPortFeature case to take care this.

I will send a new patch, please help review it. Thanks a lot.

> This suspend operation must be done before stopping the USB clock,
> resume after the USB clock enabled.
>
> Signed-off-by: Wenyou Yang 
> ---

> @@ -132,6 +135,17 @@ static void at91_stop_hc(struct platform_device *pdev)
>
>  /*-*/
>
> +struct regmap *at91_dt_syscon_sfr(void)
> +{
> + struct regmap *regmap;
> +
> + regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
> + if (IS_ERR(regmap))
> + return NULL;

If you get an error, the regmap pointer is set to NULL...

> @@ -197,6 +211,8 @@ static int usb_hcd_at91_probe(const struct hc_driver 
> *driver,
>   goto err;
>   }
>
> + ohci_at91->sfr_regmap = at91_dt_syscon_sfr();

With no other error checking...

> +
>   board = hcd->self.controller->platform_data;
>   ohci = hcd_to_ohci(hcd);
>   ohci->num_ports = board->ports;

> +static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable)
> +{
> + u32 regval;
> + int ret;
> +
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + ret = regmap_read(regmap, SFR_OHCIICR, );

And now what happens if regmap is NULL?  Hint: It won't be pretty...

Alan Stern


Best Regards,
Wenyou Yang

RE: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

2016-06-23 Thread Yang, Wenyou

Hi Alan,

Sorry for late answer.

> -Original Message-
> From: Alan Stern [mailto:st...@rowland.harvard.edu]
> Sent: 2016年5月13日 2:11
> To: Yang, Wenyou <wenyou.y...@atmel.com>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>; Ferre, Nicolas
> <nicolas.fe...@atmel.com>; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org
> Subject: Re: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending
> 
> On Thu, 12 May 2016, Wenyou Yang wrote:
> 
> > In order to get lower consumption, as a workaround, suspend the USB
> > PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI Interrupt
> > Configuration Register while OHCI USB suspending.
> 
> What does this mean?  What does suspending a port do?  Is it the same as a
> normal USB port suspend?

The usb controller from Synopsis does not managed correctly the suspend mode 
for the EHCI. 
There is no way to have the VDDUTMII (USB device and host UTMI interface) 
suspend without any device connected to it. 

That's why we added this specific control to fix this issue. Namely, by setting 
some bits of one of the special function registers to fix this issue outside 
the usb controller. 

And the suspend mode works in OHCI mode.

It is not same as a normal USB port suspend.

> 
> If it is the same, why doesn't the USB_PORT_FEAT_SUSPEND subcase of the
> SetPortFeature case in ohci_hub_control() already take care of this?
> 
> > This suspend operation must be done before stopping the USB clock,
> > resume after the USB clock enabled.
> >
> > Signed-off-by: Wenyou Yang <wenyou.y...@atmel.com>
> > ---
> 
> > @@ -132,6 +135,17 @@ static void at91_stop_hc(struct platform_device
> > *pdev)
> >
> >
> > /*
> > -*/
> >
> > +struct regmap *at91_dt_syscon_sfr(void) {
> > +   struct regmap *regmap;
> > +
> > +   regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
> > +   if (IS_ERR(regmap))
> > +   return NULL;
> 
> If you get an error, the regmap pointer is set to NULL...
> 
> > @@ -197,6 +211,8 @@ static int usb_hcd_at91_probe(const struct hc_driver
> *driver,
> > goto err;
> > }
> >
> > +   ohci_at91->sfr_regmap = at91_dt_syscon_sfr();
> 
> With no other error checking...
> 
> > +
> > board = hcd->self.controller->platform_data;
> > ohci = hcd_to_ohci(hcd);
> > ohci->num_ports = board->ports;
> 
> > +static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable) {
> > +   u32 regval;
> > +   int ret;
> > +
> > +   if (IS_ERR(regmap))
> > +   return PTR_ERR(regmap);
> > +
> > +   ret = regmap_read(regmap, SFR_OHCIICR, );
> 
> And now what happens if regmap is NULL?  Hint: It won't be pretty...
> 
> Alan Stern


Best Regards,
Wenyou Yang

RE: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

2016-06-23 Thread Yang, Wenyou

Hi Alan,

Sorry for late answer.

> -Original Message-
> From: Alan Stern [mailto:st...@rowland.harvard.edu]
> Sent: 2016年5月13日 2:11
> To: Yang, Wenyou 
> Cc: Greg Kroah-Hartman ; Ferre, Nicolas
> ; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org
> Subject: Re: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending
> 
> On Thu, 12 May 2016, Wenyou Yang wrote:
> 
> > In order to get lower consumption, as a workaround, suspend the USB
> > PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI Interrupt
> > Configuration Register while OHCI USB suspending.
> 
> What does this mean?  What does suspending a port do?  Is it the same as a
> normal USB port suspend?

The usb controller from Synopsis does not managed correctly the suspend mode 
for the EHCI. 
There is no way to have the VDDUTMII (USB device and host UTMI interface) 
suspend without any device connected to it. 

That's why we added this specific control to fix this issue. Namely, by setting 
some bits of one of the special function registers to fix this issue outside 
the usb controller. 

And the suspend mode works in OHCI mode.

It is not same as a normal USB port suspend.

> 
> If it is the same, why doesn't the USB_PORT_FEAT_SUSPEND subcase of the
> SetPortFeature case in ohci_hub_control() already take care of this?
> 
> > This suspend operation must be done before stopping the USB clock,
> > resume after the USB clock enabled.
> >
> > Signed-off-by: Wenyou Yang 
> > ---
> 
> > @@ -132,6 +135,17 @@ static void at91_stop_hc(struct platform_device
> > *pdev)
> >
> >
> > /*
> > -*/
> >
> > +struct regmap *at91_dt_syscon_sfr(void) {
> > +   struct regmap *regmap;
> > +
> > +   regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
> > +   if (IS_ERR(regmap))
> > +   return NULL;
> 
> If you get an error, the regmap pointer is set to NULL...
> 
> > @@ -197,6 +211,8 @@ static int usb_hcd_at91_probe(const struct hc_driver
> *driver,
> > goto err;
> > }
> >
> > +   ohci_at91->sfr_regmap = at91_dt_syscon_sfr();
> 
> With no other error checking...
> 
> > +
> > board = hcd->self.controller->platform_data;
> > ohci = hcd_to_ohci(hcd);
> > ohci->num_ports = board->ports;
> 
> > +static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable) {
> > +   u32 regval;
> > +   int ret;
> > +
> > +   if (IS_ERR(regmap))
> > +   return PTR_ERR(regmap);
> > +
> > +   ret = regmap_read(regmap, SFR_OHCIICR, );
> 
> And now what happens if regmap is NULL?  Hint: It won't be pretty...
> 
> Alan Stern


Best Regards,
Wenyou Yang

RE: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

2016-05-12 Thread Yang, Wenyou

Hi Alan,

> -Original Message-
> From: Alan Stern [mailto:st...@rowland.harvard.edu]
> Sent: 2016年5月13日 2:11
> To: Yang, Wenyou <wenyou.y...@atmel.com>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>; Ferre, Nicolas
> <nicolas.fe...@atmel.com>; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org
> Subject: Re: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending
> 
> On Thu, 12 May 2016, Wenyou Yang wrote:
> 
> > In order to get lower consumption, as a workaround, suspend the USB
> > PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI Interrupt
> > Configuration Register while OHCI USB suspending.
> 
> What does this mean?  What does suspending a port do?  

It is a IP workaround for SAMA5D2 USB. By setting corresponding bits of a 
specific register to get lower consumption.  

> Is it the same as a normal USB port suspend?

No, it is not same.

> 
> If it is the same, why doesn't the USB_PORT_FEAT_SUSPEND subcase of the
> SetPortFeature case in ohci_hub_control() already take care of this?
> 
> > This suspend operation must be done before stopping the USB clock,
> > resume after the USB clock enabled.
> >
> > Signed-off-by: Wenyou Yang <wenyou.y...@atmel.com>
> > ---
> 
> > @@ -132,6 +135,17 @@ static void at91_stop_hc(struct platform_device
> > *pdev)
> >
> >
> > /*
> > -*/
> >
> > +struct regmap *at91_dt_syscon_sfr(void) {
> > +   struct regmap *regmap;
> > +
> > +   regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
> > +   if (IS_ERR(regmap))
> > +   return NULL;
> 
> If you get an error, the regmap pointer is set to NULL...
> 
> > @@ -197,6 +211,8 @@ static int usb_hcd_at91_probe(const struct hc_driver
> *driver,
> > goto err;
> > }
> >
> > +   ohci_at91->sfr_regmap = at91_dt_syscon_sfr();
> 
> With no other error checking...

Add error checking in next version.

> 
> > +
> > board = hcd->self.controller->platform_data;
> > ohci = hcd_to_ohci(hcd);
> > ohci->num_ports = board->ports;
> 
> > +static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable) {
> > +   u32 regval;
> > +   int ret;
> > +
> > +   if (IS_ERR(regmap))
> > +   return PTR_ERR(regmap);
> > +
> > +   ret = regmap_read(regmap, SFR_OHCIICR, );
> 
> And now what happens if regmap is NULL?  Hint: It won't be pretty...

Yes, it is not pretty. Will rework in next version.

> 
> Alan Stern


Best Regards,
Wenyou Yang

RE: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

2016-05-12 Thread Yang, Wenyou

Hi Alan,

> -Original Message-
> From: Alan Stern [mailto:st...@rowland.harvard.edu]
> Sent: 2016年5月13日 2:11
> To: Yang, Wenyou 
> Cc: Greg Kroah-Hartman ; Ferre, Nicolas
> ; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org
> Subject: Re: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending
> 
> On Thu, 12 May 2016, Wenyou Yang wrote:
> 
> > In order to get lower consumption, as a workaround, suspend the USB
> > PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI Interrupt
> > Configuration Register while OHCI USB suspending.
> 
> What does this mean?  What does suspending a port do?  

It is a IP workaround for SAMA5D2 USB. By setting corresponding bits of a 
specific register to get lower consumption.  

> Is it the same as a normal USB port suspend?

No, it is not same.

> 
> If it is the same, why doesn't the USB_PORT_FEAT_SUSPEND subcase of the
> SetPortFeature case in ohci_hub_control() already take care of this?
> 
> > This suspend operation must be done before stopping the USB clock,
> > resume after the USB clock enabled.
> >
> > Signed-off-by: Wenyou Yang 
> > ---
> 
> > @@ -132,6 +135,17 @@ static void at91_stop_hc(struct platform_device
> > *pdev)
> >
> >
> > /*
> > -*/
> >
> > +struct regmap *at91_dt_syscon_sfr(void) {
> > +   struct regmap *regmap;
> > +
> > +   regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
> > +   if (IS_ERR(regmap))
> > +   return NULL;
> 
> If you get an error, the regmap pointer is set to NULL...
> 
> > @@ -197,6 +211,8 @@ static int usb_hcd_at91_probe(const struct hc_driver
> *driver,
> > goto err;
> > }
> >
> > +   ohci_at91->sfr_regmap = at91_dt_syscon_sfr();
> 
> With no other error checking...

Add error checking in next version.

> 
> > +
> > board = hcd->self.controller->platform_data;
> > ohci = hcd_to_ohci(hcd);
> > ohci->num_ports = board->ports;
> 
> > +static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable) {
> > +   u32 regval;
> > +   int ret;
> > +
> > +   if (IS_ERR(regmap))
> > +   return PTR_ERR(regmap);
> > +
> > +   ret = regmap_read(regmap, SFR_OHCIICR, );
> 
> And now what happens if regmap is NULL?  Hint: It won't be pretty...

Yes, it is not pretty. Will rework in next version.

> 
> Alan Stern


Best Regards,
Wenyou Yang

Re: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

2016-05-12 Thread Alan Stern

On Thu, 12 May 2016, Wenyou Yang wrote:

> In order to get lower consumption, as a workaround, suspend
> the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI Interrupt
> Configuration Register while OHCI USB suspending.

What does this mean?  What does suspending a port do?  Is it the same 
as a normal USB port suspend?

If it is the same, why doesn't the USB_PORT_FEAT_SUSPEND subcase of the 
SetPortFeature case in ohci_hub_control() already take care of this?

> This suspend operation must be done before stopping the USB clock,
> resume after the USB clock enabled.
> 
> Signed-off-by: Wenyou Yang 
> ---

> @@ -132,6 +135,17 @@ static void at91_stop_hc(struct platform_device *pdev)
>  
>  /*-*/
>  
> +struct regmap *at91_dt_syscon_sfr(void)
> +{
> + struct regmap *regmap;
> +
> + regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
> + if (IS_ERR(regmap))
> + return NULL;

If you get an error, the regmap pointer is set to NULL...

> @@ -197,6 +211,8 @@ static int usb_hcd_at91_probe(const struct hc_driver 
> *driver,
>   goto err;
>   }
>  
> + ohci_at91->sfr_regmap = at91_dt_syscon_sfr();

With no other error checking...

> +
>   board = hcd->self.controller->platform_data;
>   ohci = hcd_to_ohci(hcd);
>   ohci->num_ports = board->ports;

> +static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable)
> +{
> + u32 regval;
> + int ret;
> +
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + ret = regmap_read(regmap, SFR_OHCIICR, );

And now what happens if regmap is NULL?  Hint: It won't be pretty...

Alan Stern

Re: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

2016-05-12 Thread Alan Stern

On Thu, 12 May 2016, Wenyou Yang wrote:

> In order to get lower consumption, as a workaround, suspend
> the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI Interrupt
> Configuration Register while OHCI USB suspending.

What does this mean?  What does suspending a port do?  Is it the same 
as a normal USB port suspend?

If it is the same, why doesn't the USB_PORT_FEAT_SUSPEND subcase of the 
SetPortFeature case in ohci_hub_control() already take care of this?

> This suspend operation must be done before stopping the USB clock,
> resume after the USB clock enabled.
> 
> Signed-off-by: Wenyou Yang 
> ---

> @@ -132,6 +135,17 @@ static void at91_stop_hc(struct platform_device *pdev)
>  
>  /*-*/
>  
> +struct regmap *at91_dt_syscon_sfr(void)
> +{
> + struct regmap *regmap;
> +
> + regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
> + if (IS_ERR(regmap))
> + return NULL;

If you get an error, the regmap pointer is set to NULL...

> @@ -197,6 +211,8 @@ static int usb_hcd_at91_probe(const struct hc_driver 
> *driver,
>   goto err;
>   }
>  
> + ohci_at91->sfr_regmap = at91_dt_syscon_sfr();

With no other error checking...

> +
>   board = hcd->self.controller->platform_data;
>   ohci = hcd_to_ohci(hcd);
>   ohci->num_ports = board->ports;

> +static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable)
> +{
> + u32 regval;
> + int ret;
> +
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + ret = regmap_read(regmap, SFR_OHCIICR, );

And now what happens if regmap is NULL?  Hint: It won't be pretty...

Alan Stern

RE: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

2016-05-11 Thread Yang, Wenyou



> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2016年5月12日 11:53
> To: Yang, Wenyou <wenyou.y...@atmel.com>
> Cc: Alan Stern <st...@rowland.harvard.edu>; Greg Kroah-Hartman
> <gre...@linuxfoundation.org>; Ferre, Nicolas <nicolas.fe...@atmel.com>;
> linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org
> Subject: Re: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending
> 
> Hi,
> 
> On 12/05/2016 at 09:05:34 +0800, Wenyou Yang wrote :
> > In order to get lower consumption, as a workaround, suspend the USB
> > PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI Interrupt
> > Configuration Register while OHCI USB suspending.
> >
> 
> Why is that a workaround?

Because if these bits is not set as this patch, there is 5mA current left
at VDDOSC rail during suspend. If apply this patch, this current will disappear.

So, I think it is a workaround.

> 
> > This suspend operation must be done before stopping the USB clock,
> > resume after the USB clock enabled.
> >
> > Signed-off-by: Wenyou Yang <wenyou.y...@atmel.com>
> > ---
> >
> >  drivers/usb/host/ohci-at91.c | 63
> > 
> >  1 file changed, 63 insertions(+)
> >
> > diff --git a/drivers/usb/host/ohci-at91.c
> > b/drivers/usb/host/ohci-at91.c index d177372..ce898e0 100644
> > --- a/drivers/usb/host/ohci-at91.c
> > +++ b/drivers/usb/host/ohci-at91.c
> > @@ -21,6 +21,8 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> >  #include 
> >  #include 
> >
> > @@ -51,6 +53,7 @@ struct ohci_at91_priv {
> > struct clk *hclk;
> > bool clocked;
> > bool wakeup;/* Saved wake-up state for resume */
> > +   struct regmap *sfr_regmap;
> >  };
> >  /* interface and function clocks; sometimes also an AHB clock */
> >
> > @@ -132,6 +135,17 @@ static void at91_stop_hc(struct platform_device
> > *pdev)
> >
> >
> > /*
> > -*/
> >
> > +struct regmap *at91_dt_syscon_sfr(void) {
> > +   struct regmap *regmap;
> > +
> > +   regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
> > +   if (IS_ERR(regmap))
> > +   return NULL;
> > +
> > +   return regmap;
> > +}
> > +
> >  static void usb_hcd_at91_remove (struct usb_hcd *, struct
> > platform_device *);
> >
> >  /* configure so an HC device and id are always provided */ @@ -197,6
> > +211,8 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver,
> > goto err;
> > }
> >
> > +   ohci_at91->sfr_regmap = at91_dt_syscon_sfr();
> > +
> > board = hcd->self.controller->platform_data;
> > ohci = hcd_to_ohci(hcd);
> > ohci->num_ports = board->ports;
> > @@ -581,6 +597,49 @@ static int ohci_hcd_at91_drv_remove(struct
> platform_device *pdev)
> > return 0;
> >  }
> >
> > +#define SFR_OHCIICR0x10
> > +#define SFR_OHCIICR_SUSPEND_A  BIT(8)
> > +#define SFR_OHCIICR_SUSPEND_B  BIT(9)
> > +#define SFR_OHCIICR_SUSPEND_C  BIT(10)
> > +
> > +#define SFR_OHCIICR_USB_SUSPEND(SFR_OHCIICR_SUSPEND_A | \
> > +SFR_OHCIICR_SUSPEND_B | \
> > +SFR_OHCIICR_SUSPEND_C)
> > +
> 
> Those defines should go in a header file either in include/soc/at91 or in
> include/linux/mfd
> 
> > +static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable) {
> > +   u32 regval;
> > +   int ret;
> > +
> > +   if (IS_ERR(regmap))
> > +   return PTR_ERR(regmap);
> > +
> > +   ret = regmap_read(regmap, SFR_OHCIICR, );
> > +   if (ret)
> > +   return ret;
> > +
> > +   if (enable)
> > +   regval &= ~SFR_OHCIICR_USB_SUSPEND;
> > +   else
> > +   regval |= SFR_OHCIICR_USB_SUSPEND;
> > +
> > +   regmap_write(regmap, SFR_OHCIICR, regval);
> > +
> > +   regmap_read(regmap, SFR_OHCIICR, );
> > +
> > +   return 0;
> > +}
> > +
> > +static int ohci_at91_port_suspend(struct regmap *regmap) {
> > +   return ohci_at91_port_ctrl(regmap, false); }
> > +
> > +static int ohci_at91_port_resume(struct regmap *regmap) {
> > +   return ohci_at91_port_ctrl(regmap, true); }
> > +
> >  static int __maybe_unused
> >  ohci_hcd_at91_drv_suspend(struct device *dev)  { @@ -618,6 +677,8 @@
> > ohci_hcd_at91_drv_suspend(struct device *dev)
> > ohci_writel(ohci, ohci->hc_control, >regs->control);
> > ohci->rh_state = OHCI_RH_HALTED;
> >
> > +   ohci_at91_port_suspend(ohci_at91->sfr_regmap);
> > +
> 
> The main issue I see here is that you will call that function for all SoCs 
> and it will
> always fail unless it is running on a sama5d2. Maybe we could be smarter about
> that.

Can we use another compatible, such as "atmel,sama5d2-ohci" to differentiate 
it? 


Best Regards,
Wenyou Yang

RE: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

2016-05-11 Thread Yang, Wenyou



> -Original Message-
> From: Alexandre Belloni [mailto:alexandre.bell...@free-electrons.com]
> Sent: 2016年5月12日 11:53
> To: Yang, Wenyou 
> Cc: Alan Stern ; Greg Kroah-Hartman
> ; Ferre, Nicolas ;
> linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org
> Subject: Re: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending
> 
> Hi,
> 
> On 12/05/2016 at 09:05:34 +0800, Wenyou Yang wrote :
> > In order to get lower consumption, as a workaround, suspend the USB
> > PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI Interrupt
> > Configuration Register while OHCI USB suspending.
> >
> 
> Why is that a workaround?

Because if these bits is not set as this patch, there is 5mA current left
at VDDOSC rail during suspend. If apply this patch, this current will disappear.

So, I think it is a workaround.

> 
> > This suspend operation must be done before stopping the USB clock,
> > resume after the USB clock enabled.
> >
> > Signed-off-by: Wenyou Yang 
> > ---
> >
> >  drivers/usb/host/ohci-at91.c | 63
> > 
> >  1 file changed, 63 insertions(+)
> >
> > diff --git a/drivers/usb/host/ohci-at91.c
> > b/drivers/usb/host/ohci-at91.c index d177372..ce898e0 100644
> > --- a/drivers/usb/host/ohci-at91.c
> > +++ b/drivers/usb/host/ohci-at91.c
> > @@ -21,6 +21,8 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> >  #include 
> >  #include 
> >
> > @@ -51,6 +53,7 @@ struct ohci_at91_priv {
> > struct clk *hclk;
> > bool clocked;
> > bool wakeup;/* Saved wake-up state for resume */
> > +   struct regmap *sfr_regmap;
> >  };
> >  /* interface and function clocks; sometimes also an AHB clock */
> >
> > @@ -132,6 +135,17 @@ static void at91_stop_hc(struct platform_device
> > *pdev)
> >
> >
> > /*
> > -*/
> >
> > +struct regmap *at91_dt_syscon_sfr(void) {
> > +   struct regmap *regmap;
> > +
> > +   regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
> > +   if (IS_ERR(regmap))
> > +   return NULL;
> > +
> > +   return regmap;
> > +}
> > +
> >  static void usb_hcd_at91_remove (struct usb_hcd *, struct
> > platform_device *);
> >
> >  /* configure so an HC device and id are always provided */ @@ -197,6
> > +211,8 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver,
> > goto err;
> > }
> >
> > +   ohci_at91->sfr_regmap = at91_dt_syscon_sfr();
> > +
> > board = hcd->self.controller->platform_data;
> > ohci = hcd_to_ohci(hcd);
> > ohci->num_ports = board->ports;
> > @@ -581,6 +597,49 @@ static int ohci_hcd_at91_drv_remove(struct
> platform_device *pdev)
> > return 0;
> >  }
> >
> > +#define SFR_OHCIICR0x10
> > +#define SFR_OHCIICR_SUSPEND_A  BIT(8)
> > +#define SFR_OHCIICR_SUSPEND_B  BIT(9)
> > +#define SFR_OHCIICR_SUSPEND_C  BIT(10)
> > +
> > +#define SFR_OHCIICR_USB_SUSPEND(SFR_OHCIICR_SUSPEND_A | \
> > +SFR_OHCIICR_SUSPEND_B | \
> > +SFR_OHCIICR_SUSPEND_C)
> > +
> 
> Those defines should go in a header file either in include/soc/at91 or in
> include/linux/mfd
> 
> > +static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable) {
> > +   u32 regval;
> > +   int ret;
> > +
> > +   if (IS_ERR(regmap))
> > +   return PTR_ERR(regmap);
> > +
> > +   ret = regmap_read(regmap, SFR_OHCIICR, );
> > +   if (ret)
> > +   return ret;
> > +
> > +   if (enable)
> > +   regval &= ~SFR_OHCIICR_USB_SUSPEND;
> > +   else
> > +   regval |= SFR_OHCIICR_USB_SUSPEND;
> > +
> > +   regmap_write(regmap, SFR_OHCIICR, regval);
> > +
> > +   regmap_read(regmap, SFR_OHCIICR, );
> > +
> > +   return 0;
> > +}
> > +
> > +static int ohci_at91_port_suspend(struct regmap *regmap) {
> > +   return ohci_at91_port_ctrl(regmap, false); }
> > +
> > +static int ohci_at91_port_resume(struct regmap *regmap) {
> > +   return ohci_at91_port_ctrl(regmap, true); }
> > +
> >  static int __maybe_unused
> >  ohci_hcd_at91_drv_suspend(struct device *dev)  { @@ -618,6 +677,8 @@
> > ohci_hcd_at91_drv_suspend(struct device *dev)
> > ohci_writel(ohci, ohci->hc_control, >regs->control);
> > ohci->rh_state = OHCI_RH_HALTED;
> >
> > +   ohci_at91_port_suspend(ohci_at91->sfr_regmap);
> > +
> 
> The main issue I see here is that you will call that function for all SoCs 
> and it will
> always fail unless it is running on a sama5d2. Maybe we could be smarter about
> that.

Can we use another compatible, such as "atmel,sama5d2-ohci" to differentiate 
it? 


Best Regards,
Wenyou Yang

Re: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

2016-05-11 Thread Alexandre Belloni

Hi,

On 12/05/2016 at 09:05:34 +0800, Wenyou Yang wrote :
> In order to get lower consumption, as a workaround, suspend
> the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI Interrupt
> Configuration Register while OHCI USB suspending.
> 

Why is that a workaround?

> This suspend operation must be done before stopping the USB clock,
> resume after the USB clock enabled.
> 
> Signed-off-by: Wenyou Yang 
> ---
> 
>  drivers/usb/host/ohci-at91.c | 63 
> 
>  1 file changed, 63 insertions(+)
> 
> diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
> index d177372..ce898e0 100644
> --- a/drivers/usb/host/ohci-at91.c
> +++ b/drivers/usb/host/ohci-at91.c
> @@ -21,6 +21,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  
> @@ -51,6 +53,7 @@ struct ohci_at91_priv {
>   struct clk *hclk;
>   bool clocked;
>   bool wakeup;/* Saved wake-up state for resume */
> + struct regmap *sfr_regmap;
>  };
>  /* interface and function clocks; sometimes also an AHB clock */
>  
> @@ -132,6 +135,17 @@ static void at91_stop_hc(struct platform_device *pdev)
>  
>  /*-*/
>  
> +struct regmap *at91_dt_syscon_sfr(void)
> +{
> + struct regmap *regmap;
> +
> + regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
> + if (IS_ERR(regmap))
> + return NULL;
> +
> + return regmap;
> +}
> +
>  static void usb_hcd_at91_remove (struct usb_hcd *, struct platform_device *);
>  
>  /* configure so an HC device and id are always provided */
> @@ -197,6 +211,8 @@ static int usb_hcd_at91_probe(const struct hc_driver 
> *driver,
>   goto err;
>   }
>  
> + ohci_at91->sfr_regmap = at91_dt_syscon_sfr();
> +
>   board = hcd->self.controller->platform_data;
>   ohci = hcd_to_ohci(hcd);
>   ohci->num_ports = board->ports;
> @@ -581,6 +597,49 @@ static int ohci_hcd_at91_drv_remove(struct 
> platform_device *pdev)
>   return 0;
>  }
>  
> +#define SFR_OHCIICR  0x10
> +#define SFR_OHCIICR_SUSPEND_ABIT(8)
> +#define SFR_OHCIICR_SUSPEND_BBIT(9)
> +#define SFR_OHCIICR_SUSPEND_CBIT(10)
> +
> +#define SFR_OHCIICR_USB_SUSPEND  (SFR_OHCIICR_SUSPEND_A | \
> +  SFR_OHCIICR_SUSPEND_B | \
> +  SFR_OHCIICR_SUSPEND_C)
> +

Those defines should go in a header file either in include/soc/at91 or
in include/linux/mfd

> +static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable)
> +{
> + u32 regval;
> + int ret;
> +
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + ret = regmap_read(regmap, SFR_OHCIICR, );
> + if (ret)
> + return ret;
> +
> + if (enable)
> + regval &= ~SFR_OHCIICR_USB_SUSPEND;
> + else
> + regval |= SFR_OHCIICR_USB_SUSPEND;
> +
> + regmap_write(regmap, SFR_OHCIICR, regval);
> +
> + regmap_read(regmap, SFR_OHCIICR, );
> +
> + return 0;
> +}
> +
> +static int ohci_at91_port_suspend(struct regmap *regmap)
> +{
> + return ohci_at91_port_ctrl(regmap, false);
> +}
> +
> +static int ohci_at91_port_resume(struct regmap *regmap)
> +{
> + return ohci_at91_port_ctrl(regmap, true);
> +}
> +
>  static int __maybe_unused
>  ohci_hcd_at91_drv_suspend(struct device *dev)
>  {
> @@ -618,6 +677,8 @@ ohci_hcd_at91_drv_suspend(struct device *dev)
>   ohci_writel(ohci, ohci->hc_control, >regs->control);
>   ohci->rh_state = OHCI_RH_HALTED;
>  
> + ohci_at91_port_suspend(ohci_at91->sfr_regmap);
> +

The main issue I see here is that you will call that function for all
SoCs and it will always fail unless it is running on a sama5d2. Maybe we
could be smarter about that.


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

Re: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

2016-05-11 Thread Alexandre Belloni

Hi,

On 12/05/2016 at 09:05:34 +0800, Wenyou Yang wrote :
> In order to get lower consumption, as a workaround, suspend
> the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI Interrupt
> Configuration Register while OHCI USB suspending.
> 

Why is that a workaround?

> This suspend operation must be done before stopping the USB clock,
> resume after the USB clock enabled.
> 
> Signed-off-by: Wenyou Yang 
> ---
> 
>  drivers/usb/host/ohci-at91.c | 63 
> 
>  1 file changed, 63 insertions(+)
> 
> diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
> index d177372..ce898e0 100644
> --- a/drivers/usb/host/ohci-at91.c
> +++ b/drivers/usb/host/ohci-at91.c
> @@ -21,6 +21,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  
> @@ -51,6 +53,7 @@ struct ohci_at91_priv {
>   struct clk *hclk;
>   bool clocked;
>   bool wakeup;/* Saved wake-up state for resume */
> + struct regmap *sfr_regmap;
>  };
>  /* interface and function clocks; sometimes also an AHB clock */
>  
> @@ -132,6 +135,17 @@ static void at91_stop_hc(struct platform_device *pdev)
>  
>  /*-*/
>  
> +struct regmap *at91_dt_syscon_sfr(void)
> +{
> + struct regmap *regmap;
> +
> + regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
> + if (IS_ERR(regmap))
> + return NULL;
> +
> + return regmap;
> +}
> +
>  static void usb_hcd_at91_remove (struct usb_hcd *, struct platform_device *);
>  
>  /* configure so an HC device and id are always provided */
> @@ -197,6 +211,8 @@ static int usb_hcd_at91_probe(const struct hc_driver 
> *driver,
>   goto err;
>   }
>  
> + ohci_at91->sfr_regmap = at91_dt_syscon_sfr();
> +
>   board = hcd->self.controller->platform_data;
>   ohci = hcd_to_ohci(hcd);
>   ohci->num_ports = board->ports;
> @@ -581,6 +597,49 @@ static int ohci_hcd_at91_drv_remove(struct 
> platform_device *pdev)
>   return 0;
>  }
>  
> +#define SFR_OHCIICR  0x10
> +#define SFR_OHCIICR_SUSPEND_ABIT(8)
> +#define SFR_OHCIICR_SUSPEND_BBIT(9)
> +#define SFR_OHCIICR_SUSPEND_CBIT(10)
> +
> +#define SFR_OHCIICR_USB_SUSPEND  (SFR_OHCIICR_SUSPEND_A | \
> +  SFR_OHCIICR_SUSPEND_B | \
> +  SFR_OHCIICR_SUSPEND_C)
> +

Those defines should go in a header file either in include/soc/at91 or
in include/linux/mfd

> +static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable)
> +{
> + u32 regval;
> + int ret;
> +
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + ret = regmap_read(regmap, SFR_OHCIICR, );
> + if (ret)
> + return ret;
> +
> + if (enable)
> + regval &= ~SFR_OHCIICR_USB_SUSPEND;
> + else
> + regval |= SFR_OHCIICR_USB_SUSPEND;
> +
> + regmap_write(regmap, SFR_OHCIICR, regval);
> +
> + regmap_read(regmap, SFR_OHCIICR, );
> +
> + return 0;
> +}
> +
> +static int ohci_at91_port_suspend(struct regmap *regmap)
> +{
> + return ohci_at91_port_ctrl(regmap, false);
> +}
> +
> +static int ohci_at91_port_resume(struct regmap *regmap)
> +{
> + return ohci_at91_port_ctrl(regmap, true);
> +}
> +
>  static int __maybe_unused
>  ohci_hcd_at91_drv_suspend(struct device *dev)
>  {
> @@ -618,6 +677,8 @@ ohci_hcd_at91_drv_suspend(struct device *dev)
>   ohci_writel(ohci, ohci->hc_control, >regs->control);
>   ohci->rh_state = OHCI_RH_HALTED;
>  
> + ohci_at91_port_suspend(ohci_at91->sfr_regmap);
> +

The main issue I see here is that you will call that function for all
SoCs and it will always fail unless it is running on a sama5d2. Maybe we
could be smarter about that.


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

RE: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

RE: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

RE: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

RE: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

RE: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

RE: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

Re: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

Re: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

RE: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

RE: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

Re: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

Re: [PATCH] usb: ohci-at91: Suspend the ports while USB suspending

12 matches

Site Navigation

Mail list logo

Footer information