Patchwork [U-Boot,v3,6/7] USB: usb-hub: Add a weak function for resetting devices

login
register
mail settings
Submitter Dan Murphy
Date July 17, 2013, 8:16 p.m.
Message ID <1374092167-27645-7-git-send-email-dmurphy@ti.com>
Download mbox | patch
Permalink /patch/259751/
State Awaiting Upstream
Delegated to: Marek Vasut
Headers show

Comments

Dan Murphy - July 17, 2013, 8:16 p.m.
Add a __weak function that can be overridden to reset devices
attached to an ehci devices after the FEAT_POWER has been submitted

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
v3 - Modified location of the weak function - http://patchwork.ozlabs.org/patch/258229/

 common/usb_hub.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)
Marek Vasut - July 18, 2013, 4:30 a.m.
Dear Dan Murphy,

> Add a __weak function that can be overridden to reset devices
> attached to an ehci devices after the FEAT_POWER has been submitted
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> v3 - Modified location of the weak function -
> http://patchwork.ozlabs.org/patch/258229/
> 
>  common/usb_hub.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 774ba63..c655b75 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -62,6 +62,10 @@
>  static struct usb_hub_device hub_dev[USB_MAX_HUB];
>  static int usb_hub_index;
> 
> +__weak void usb_hub_reset_devices(int port)
> +{
> +	return;
> +}
> 
>  static int usb_get_hub_descriptor(struct usb_device *dev, void *data, int
> size) {
> @@ -444,6 +448,14 @@ static int usb_hub_configure(struct usb_device *dev)
>  	      "" : "no ");
>  	usb_hub_power_on(hub);
> 
> +	/*
> +	 * Reset any devices that may be in a bad state when applying
> +	 * the power.  This is a __weak function.  Resettig of the devices
> +	 * should occur in the board file of the device.
> +	 */
> +	for (i = 0; i < dev->maxchild; i++)
> +		usb_hub_reset_devices(i + 1);
> +
>  	for (i = 0; i < dev->maxchild; i++) {
>  		ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
>  		unsigned short portstatus, portchange;

We hae like 10 reset hooks in the usb code, this means something just isn't 
right. The device doesn't enumerate, right ? Have you tried waiting a little 
after releasing the reset and before starting the EHCI ?

Best regards,
Marek Vasut
Dan Murphy - July 24, 2013, 7:55 p.m.
Marek
On 07/17/2013 11:30 PM, Marek Vasut wrote:
> Dear Dan Murphy,
>
>> Add a __weak function that can be overridden to reset devices
>> attached to an ehci devices after the FEAT_POWER has been submitted
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>> v3 - Modified location of the weak function -
>> http://patchwork.ozlabs.org/patch/258229/
>>
>>  common/usb_hub.c |   12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>> index 774ba63..c655b75 100644
>> --- a/common/usb_hub.c
>> +++ b/common/usb_hub.c
>> @@ -62,6 +62,10 @@
>>  static struct usb_hub_device hub_dev[USB_MAX_HUB];
>>  static int usb_hub_index;
>>
>> +__weak void usb_hub_reset_devices(int port)
>> +{
>> +	return;
>> +}
>>
>>  static int usb_get_hub_descriptor(struct usb_device *dev, void *data, int
>> size) {
>> @@ -444,6 +448,14 @@ static int usb_hub_configure(struct usb_device *dev)
>>  	      "" : "no ");
>>  	usb_hub_power_on(hub);
>>
>> +	/*
>> +	 * Reset any devices that may be in a bad state when applying
>> +	 * the power.  This is a __weak function.  Resettig of the devices
>> +	 * should occur in the board file of the device.
>> +	 */
>> +	for (i = 0; i < dev->maxchild; i++)
>> +		usb_hub_reset_devices(i + 1);
>> +
>>  	for (i = 0; i < dev->maxchild; i++) {
>>  		ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
>>  		unsigned short portstatus, portchange;
> We hae like 10 reset hooks in the usb code, this means something just isn't 
> right. The device doesn't enumerate, right ? Have you tried waiting a little 
> after releasing the reset and before starting the EHCI ?
>
> Best regards,
> Marek Vasut
Sorry for the delay in the response I have been looking heavily into this.

I see we have many reset hooks but they all seem to mean something for different components.

The one I am introducing is for the device itself and made it weak as there are devices that work properly like the 3503.

What I am finding is that I have to apply port power and then hit the device's reset line to get the device to enumerate.
Or I can hold the 9730 device in reset throughout the full USB init sequence and upon releasing it after port power the device is detected.

There is no opportunity to release the reset while the USB detection state machine is running

I have contacted the vendor to see if there are any known anomalies with this part if there is then I can document this in the commit.

Dan
Marek Vasut - July 25, 2013, 5:32 a.m.
Dear Dan Murphy,

> Marek
> 
> On 07/17/2013 11:30 PM, Marek Vasut wrote:
> > Dear Dan Murphy,
> > 
> >> Add a __weak function that can be overridden to reset devices
> >> attached to an ehci devices after the FEAT_POWER has been submitted
> >> 
> >> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> >> ---
> >> v3 - Modified location of the weak function -
> >> http://patchwork.ozlabs.org/patch/258229/
> >> 
> >>  common/usb_hub.c |   12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >> 
> >> diff --git a/common/usb_hub.c b/common/usb_hub.c
> >> index 774ba63..c655b75 100644
> >> --- a/common/usb_hub.c
> >> +++ b/common/usb_hub.c
> >> @@ -62,6 +62,10 @@
> >> 
> >>  static struct usb_hub_device hub_dev[USB_MAX_HUB];
> >>  static int usb_hub_index;
> >> 
> >> +__weak void usb_hub_reset_devices(int port)
> >> +{
> >> +	return;
> >> +}
> >> 
> >>  static int usb_get_hub_descriptor(struct usb_device *dev, void *data,
> >>  int
> >> 
> >> size) {
> >> @@ -444,6 +448,14 @@ static int usb_hub_configure(struct usb_device
> >> *dev)
> >> 
> >>  	      "" : "no ");
> >>  	
> >>  	usb_hub_power_on(hub);
> >> 
> >> +	/*
> >> +	 * Reset any devices that may be in a bad state when applying
> >> +	 * the power.  This is a __weak function.  Resettig of the devices
> >> +	 * should occur in the board file of the device.
> >> +	 */
> >> +	for (i = 0; i < dev->maxchild; i++)
> >> +		usb_hub_reset_devices(i + 1);
> >> +
> >> 
> >>  	for (i = 0; i < dev->maxchild; i++) {
> >>  	
> >>  		ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
> >>  		unsigned short portstatus, portchange;
> > 
> > We hae like 10 reset hooks in the usb code, this means something just
> > isn't right. The device doesn't enumerate, right ? Have you tried
> > waiting a little after releasing the reset and before starting the EHCI
> > ?
> > 
> > Best regards,
> > Marek Vasut
> 
> Sorry for the delay in the response I have been looking heavily into this.
> 
> I see we have many reset hooks but they all seem to mean something for
> different components.
> 
> The one I am introducing is for the device itself and made it weak as there
> are devices that work properly like the 3503.
> 
> What I am finding is that I have to apply port power and then hit the
> device's reset line to get the device to enumerate. Or I can hold the 9730
> device in reset throughout the full USB init sequence and upon releasing
> it after port power the device is detected.
> 
> There is no opportunity to release the reset while the USB detection state
> machine is running
> 
> I have contacted the vendor to see if there are any known anomalies with
> this part if there is then I can document this in the commit.

Sigh, ok, that's unfortunate. I keep wondering how come it works in Linux. But 
anyway, if the manufacturer won't come up with anything, we will apply this 
stuff.

Best regards,
Marek Vasut
Roger Quadros - July 25, 2013, 9:22 a.m.
Hi Marek,

On 07/25/2013 08:32 AM, Marek Vasut wrote:
> Dear Dan Murphy,
> 
>> Marek
>>
>> On 07/17/2013 11:30 PM, Marek Vasut wrote:
>>> Dear Dan Murphy,
>>>
>>>> Add a __weak function that can be overridden to reset devices
>>>> attached to an ehci devices after the FEAT_POWER has been submitted
>>>>
>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>> ---
>>>> v3 - Modified location of the weak function -
>>>> http://patchwork.ozlabs.org/patch/258229/
>>>>
>>>>  common/usb_hub.c |   12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>>>> index 774ba63..c655b75 100644
>>>> --- a/common/usb_hub.c
>>>> +++ b/common/usb_hub.c
>>>> @@ -62,6 +62,10 @@
>>>>
>>>>  static struct usb_hub_device hub_dev[USB_MAX_HUB];
>>>>  static int usb_hub_index;
>>>>
>>>> +__weak void usb_hub_reset_devices(int port)
>>>> +{
>>>> +	return;
>>>> +}
>>>>
>>>>  static int usb_get_hub_descriptor(struct usb_device *dev, void *data,
>>>>  int
>>>>
>>>> size) {
>>>> @@ -444,6 +448,14 @@ static int usb_hub_configure(struct usb_device
>>>> *dev)
>>>>
>>>>  	      "" : "no ");
>>>>  	
>>>>  	usb_hub_power_on(hub);
>>>>
>>>> +	/*
>>>> +	 * Reset any devices that may be in a bad state when applying
>>>> +	 * the power.  This is a __weak function.  Resettig of the devices
>>>> +	 * should occur in the board file of the device.
>>>> +	 */
>>>> +	for (i = 0; i < dev->maxchild; i++)
>>>> +		usb_hub_reset_devices(i + 1);
>>>> +
>>>>
>>>>  	for (i = 0; i < dev->maxchild; i++) {
>>>>  	
>>>>  		ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
>>>>  		unsigned short portstatus, portchange;
>>>
>>> We hae like 10 reset hooks in the usb code, this means something just
>>> isn't right. The device doesn't enumerate, right ? Have you tried
>>> waiting a little after releasing the reset and before starting the EHCI
>>> ?
>>>
>>> Best regards,
>>> Marek Vasut
>>
>> Sorry for the delay in the response I have been looking heavily into this.
>>
>> I see we have many reset hooks but they all seem to mean something for
>> different components.
>>
>> The one I am introducing is for the device itself and made it weak as there
>> are devices that work properly like the 3503.
>>
>> What I am finding is that I have to apply port power and then hit the
>> device's reset line to get the device to enumerate. Or I can hold the 9730
>> device in reset throughout the full USB init sequence and upon releasing
>> it after port power the device is detected.
>>
>> There is no opportunity to release the reset while the USB detection state
>> machine is running
>>
>> I have contacted the vendor to see if there are any known anomalies with
>> this part if there is then I can document this in the commit.
> 
> Sigh, ok, that's unfortunate. I keep wondering how come it works in Linux. But 
> anyway, if the manufacturer won't come up with anything, we will apply this 
> stuff.
> 

On Linux, we don't toggle Port power of the root hub once it has been enabled (i.e. EHCI started)
and RESET line to 9730 is released.

Dan, mentioned to me that on u-boot we toggle the Port power of the root hub each
time a USB session is started. This seems to be causing problems with the 9730 as it
seems to need an explicit reset after the Port power bit is toggled, in order to be detected.

cheers,
-roger

Patch

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 774ba63..c655b75 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -62,6 +62,10 @@ 
 static struct usb_hub_device hub_dev[USB_MAX_HUB];
 static int usb_hub_index;
 
+__weak void usb_hub_reset_devices(int port)
+{
+	return;
+}
 
 static int usb_get_hub_descriptor(struct usb_device *dev, void *data, int size)
 {
@@ -444,6 +448,14 @@  static int usb_hub_configure(struct usb_device *dev)
 	      "" : "no ");
 	usb_hub_power_on(hub);
 
+	/*
+	 * Reset any devices that may be in a bad state when applying
+	 * the power.  This is a __weak function.  Resettig of the devices
+	 * should occur in the board file of the device.
+	 */
+	for (i = 0; i < dev->maxchild; i++)
+		usb_hub_reset_devices(i + 1);
+
 	for (i = 0; i < dev->maxchild; i++) {
 		ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1);
 		unsigned short portstatus, portchange;