diff mbox

[RFC,v2,10/13] usb: hub: Power sequence the ports on activation

Message ID 1462451666-17945-11-git-send-email-k.kozlowski@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Krzysztof Kozlowski May 5, 2016, 12:34 p.m. UTC
The autodetection of attached USB device might not work on certain
boards where the power is delivered externally. These devices also might
require a hard reset. Use pwrseq for that in USB hub.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/usb/core/hub.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Alan Stern May 5, 2016, 2:09 p.m. UTC | #1
On Thu, 5 May 2016, Krzysztof Kozlowski wrote:

> The autodetection of attached USB device might not work on certain
> boards where the power is delivered externally. These devices also might
> require a hard reset. Use pwrseq for that in USB hub.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  drivers/usb/core/hub.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 1c82fcc448f5..0fddaacc62bf 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -26,6 +26,7 @@
>  #include <linux/mutex.h>
>  #include <linux/random.h>
>  #include <linux/pm_qos.h>
> +#include <linux/pwrseq.h>
>  
>  #include <asm/uaccess.h>
>  #include <asm/byteorder.h>
> @@ -1661,7 +1662,17 @@ static int hub_configure(struct usb_hub *hub,
>  
>  	usb_hub_adjust_deviceremovable(hdev, hub->descriptor);
>  
> +	/* FIXME: When do the pre-power-on? */

It's hard to answer this without knowing what pre-power-on involves.

Why not create a pwrseq_power_on() routine that does pre_power_on 
followed by post_power_on?

> +	/*
> +	for (i = 0; i < maxchild; i++)
> +		pwrseq_pre_power_on(hub->ports[i]->pwrseq);
> +	*/
> +
> +	for (i = 0; i < maxchild; i++)
> +		pwrseq_post_power_on(hub->ports[i]->pwrseq);

This is patch 10/13.  hub->ports[i]->pwrseq doesn't get added until 
11/13.  Obviously you never tried compiling each patch in the series.

> +
>  	hub_activate(hub, HUB_INIT);
> +

Unnecessary blank line added.

>  	return 0;
>  
>  fail:

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski May 5, 2016, 5:37 p.m. UTC | #2
On Thu, May 05, 2016 at 10:09:47AM -0400, Alan Stern wrote:
> On Thu, 5 May 2016, Krzysztof Kozlowski wrote:
> 
> > The autodetection of attached USB device might not work on certain
> > boards where the power is delivered externally. These devices also might
> > require a hard reset. Use pwrseq for that in USB hub.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > ---
> >  drivers/usb/core/hub.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 1c82fcc448f5..0fddaacc62bf 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/mutex.h>
> >  #include <linux/random.h>
> >  #include <linux/pm_qos.h>
> > +#include <linux/pwrseq.h>
> >  
> >  #include <asm/uaccess.h>
> >  #include <asm/byteorder.h>
> > @@ -1661,7 +1662,17 @@ static int hub_configure(struct usb_hub *hub,
> >  
> >  	usb_hub_adjust_deviceremovable(hdev, hub->descriptor);
> >  
> > +	/* FIXME: When do the pre-power-on? */
> 
> It's hard to answer this without knowing what pre-power-on involves.

In my particular case, I want to achieve a full reset through regulator (off
and on) because the bootloader left it in initialized state. I assume
that if bootloader did not configure the device, the reset won't be
harmful.

In a MMC case, this pre-power-on on is setting the 'reset' GPIO (thus
triggering the reset) and post-power-on is clearing the 'reset'.

> Why not create a pwrseq_power_on() routine that does pre_power_on 
> followed by post_power_on?

For my purpose it seems sensible.

> 
> > +	/*
> > +	for (i = 0; i < maxchild; i++)
> > +		pwrseq_pre_power_on(hub->ports[i]->pwrseq);
> > +	*/
> > +
> > +	for (i = 0; i < maxchild; i++)
> > +		pwrseq_post_power_on(hub->ports[i]->pwrseq);
> 
> This is patch 10/13.  hub->ports[i]->pwrseq doesn't get added until 
> 11/13.  Obviously you never tried compiling each patch in the series.

Ahh yes, I forgot to reorder them. Thanks for spotting this.

> 
> > +
> >  	hub_activate(hub, HUB_INIT);
> > +
> 
> Unnecessary blank line added.

Thanks for feedback,
Krzysztof

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Javier Martinez Canillas May 5, 2016, 7:56 p.m. UTC | #3
Hello Krzysztof,

On 05/05/2016 08:34 AM, Krzysztof Kozlowski wrote:
> The autodetection of attached USB device might not work on certain
> boards where the power is delivered externally. These devices also might
> require a hard reset. Use pwrseq for that in USB hub.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---

Patch looks good to me. So after fixing the bisectability issue
pointed out by Alan Stern:

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
diff mbox

Patch

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 1c82fcc448f5..0fddaacc62bf 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -26,6 +26,7 @@ 
 #include <linux/mutex.h>
 #include <linux/random.h>
 #include <linux/pm_qos.h>
+#include <linux/pwrseq.h>
 
 #include <asm/uaccess.h>
 #include <asm/byteorder.h>
@@ -1661,7 +1662,17 @@  static int hub_configure(struct usb_hub *hub,
 
 	usb_hub_adjust_deviceremovable(hdev, hub->descriptor);
 
+	/* FIXME: When do the pre-power-on? */
+	/*
+	for (i = 0; i < maxchild; i++)
+		pwrseq_pre_power_on(hub->ports[i]->pwrseq);
+	*/
+
+	for (i = 0; i < maxchild; i++)
+		pwrseq_post_power_on(hub->ports[i]->pwrseq);
+
 	hub_activate(hub, HUB_INIT);
+
 	return 0;
 
 fail: