All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH] usb: gadget: ci_udc: fix suspend/resume of USB Mass Storage
@ 2016-08-22 19:38 John Tobias
  2016-08-23 22:49 ` Fabio Estevam
  0 siblings, 1 reply; 9+ messages in thread
From: John Tobias @ 2016-08-22 19:38 UTC (permalink / raw)
  To: u-boot

When the host machine went to suspend mode and then wake it up, it send
a USB_REQ_GET_STATUS. The existing case condition below, never
become true and it causes the host machine to reset the connection. Once,
it reset you will see an error "Disk Not Ejected Properly".

case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS):

By applying this patch, the device could respond to the USB_REQ_GET_STATUS
command correctly.
---
 drivers/usb/gadget/ci_udc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
index d36bcf6..fdec613 100644
--- a/drivers/usb/gadget/ci_udc.c
+++ b/drivers/usb/gadget/ci_udc.c
@@ -703,8 +703,8 @@ static void handle_setup(void)
 	list_del_init(&ci_req->queue);
 	ci_ep->req_primed = false;
 
-	switch (SETUP(r.bRequestType, r.bRequest)) {
-	case SETUP(USB_RECIP_ENDPOINT, USB_REQ_CLEAR_FEATURE):
+	switch (r.bRequest) {
+	case USB_REQ_CLEAR_FEATURE:
 		_num = r.wIndex & 15;
 		_in = !!(r.wIndex & 0x80);
 
@@ -729,7 +729,7 @@ static void handle_setup(void)
 		}
 		return;
 
-	case SETUP(USB_RECIP_DEVICE, USB_REQ_SET_ADDRESS):
+	case USB_REQ_SET_ADDRESS:
 		/*
 		 * write address delayed (will take effect
 		 * after the next IN txn)
@@ -739,7 +739,7 @@ static void handle_setup(void)
 		usb_ep_queue(controller.gadget.ep0, req, 0);
 		return;
 
-	case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS):
+	case USB_REQ_GET_STATUS:
 		req->length = 2;
 		buf = (char *)req->buf;
 		buf[0] = 1 << USB_DEVICE_SELF_POWERED;
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] usb: gadget: ci_udc: fix suspend/resume of USB Mass Storage
  2016-08-22 19:38 [U-Boot] [PATCH] usb: gadget: ci_udc: fix suspend/resume of USB Mass Storage John Tobias
@ 2016-08-23 22:49 ` Fabio Estevam
  2016-08-24  0:18   ` Marek Vasut
  0 siblings, 1 reply; 9+ messages in thread
From: Fabio Estevam @ 2016-08-23 22:49 UTC (permalink / raw)
  To: u-boot

Adding Marek on Cc

On Mon, Aug 22, 2016 at 4:38 PM, John Tobias <john.tobias.ph@gmail.com> wrote:
> When the host machine went to suspend mode and then wake it up, it send
> a USB_REQ_GET_STATUS. The existing case condition below, never
> become true and it causes the host machine to reset the connection. Once,
> it reset you will see an error "Disk Not Ejected Properly".
>
> case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS):
>
> By applying this patch, the device could respond to the USB_REQ_GET_STATUS
> command correctly.
> ---
>  drivers/usb/gadget/ci_udc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
> index d36bcf6..fdec613 100644
> --- a/drivers/usb/gadget/ci_udc.c
> +++ b/drivers/usb/gadget/ci_udc.c
> @@ -703,8 +703,8 @@ static void handle_setup(void)
>         list_del_init(&ci_req->queue);
>         ci_ep->req_primed = false;
>
> -       switch (SETUP(r.bRequestType, r.bRequest)) {
> -       case SETUP(USB_RECIP_ENDPOINT, USB_REQ_CLEAR_FEATURE):
> +       switch (r.bRequest) {
> +       case USB_REQ_CLEAR_FEATURE:
>                 _num = r.wIndex & 15;
>                 _in = !!(r.wIndex & 0x80);
>
> @@ -729,7 +729,7 @@ static void handle_setup(void)
>                 }
>                 return;
>
> -       case SETUP(USB_RECIP_DEVICE, USB_REQ_SET_ADDRESS):
> +       case USB_REQ_SET_ADDRESS:
>                 /*
>                  * write address delayed (will take effect
>                  * after the next IN txn)
> @@ -739,7 +739,7 @@ static void handle_setup(void)
>                 usb_ep_queue(controller.gadget.ep0, req, 0);
>                 return;
>
> -       case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS):
> +       case USB_REQ_GET_STATUS:
>                 req->length = 2;
>                 buf = (char *)req->buf;
>                 buf[0] = 1 << USB_DEVICE_SELF_POWERED;
> --
> 2.9.3
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] usb: gadget: ci_udc: fix suspend/resume of USB Mass Storage
  2016-08-23 22:49 ` Fabio Estevam
@ 2016-08-24  0:18   ` Marek Vasut
  2016-08-24  1:30     ` John Tobias
  0 siblings, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2016-08-24  0:18 UTC (permalink / raw)
  To: u-boot

On 08/24/2016 12:49 AM, Fabio Estevam wrote:
> Adding Marek on Cc

Thanks

> On Mon, Aug 22, 2016 at 4:38 PM, John Tobias <john.tobias.ph@gmail.com> wrote:
>> When the host machine went to suspend mode and then wake it up, it send
>> a USB_REQ_GET_STATUS.

Can you expand on this ? It is not clear what the conditions to
replicate this are. Please do so in detail.

>> The existing case condition below, never
>> become true and it causes the host machine to reset the connection. Once,
>> it reset you will see an error "Disk Not Ejected Properly".
>>
>> case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS):
>>
>> By applying this patch, the device could respond to the USB_REQ_GET_STATUS
>> command correctly.
>> ---
>>  drivers/usb/gadget/ci_udc.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
>> index d36bcf6..fdec613 100644
>> --- a/drivers/usb/gadget/ci_udc.c
>> +++ b/drivers/usb/gadget/ci_udc.c
>> @@ -703,8 +703,8 @@ static void handle_setup(void)
>>         list_del_init(&ci_req->queue);
>>         ci_ep->req_primed = false;
>>
>> -       switch (SETUP(r.bRequestType, r.bRequest)) {
>> -       case SETUP(USB_RECIP_ENDPOINT, USB_REQ_CLEAR_FEATURE):
>> +       switch (r.bRequest) {
>> +       case USB_REQ_CLEAR_FEATURE:
>>                 _num = r.wIndex & 15;
>>                 _in = !!(r.wIndex & 0x80);
>>
>> @@ -729,7 +729,7 @@ static void handle_setup(void)
>>                 }
>>                 return;
>>
>> -       case SETUP(USB_RECIP_DEVICE, USB_REQ_SET_ADDRESS):
>> +       case USB_REQ_SET_ADDRESS:
>>                 /*
>>                  * write address delayed (will take effect
>>                  * after the next IN txn)
>> @@ -739,7 +739,7 @@ static void handle_setup(void)
>>                 usb_ep_queue(controller.gadget.ep0, req, 0);
>>                 return;
>>
>> -       case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS):
>> +       case USB_REQ_GET_STATUS:
>>                 req->length = 2;
>>                 buf = (char *)req->buf;
>>                 buf[0] = 1 << USB_DEVICE_SELF_POWERED;
>> --
>> 2.9.3
>>


-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] usb: gadget: ci_udc: fix suspend/resume of USB Mass Storage
  2016-08-24  0:18   ` Marek Vasut
@ 2016-08-24  1:30     ` John Tobias
  2016-08-24  3:29       ` Marek Vasut
  0 siblings, 1 reply; 9+ messages in thread
From: John Tobias @ 2016-08-24  1:30 UTC (permalink / raw)
  To: u-boot

Hi Marek,

On Tue, Aug 23, 2016 at 5:18 PM, Marek Vasut <marex@denx.de> wrote:
> On 08/24/2016 12:49 AM, Fabio Estevam wrote:
>> Adding Marek on Cc
>
> Thanks
>
>> On Mon, Aug 22, 2016 at 4:38 PM, John Tobias <john.tobias.ph@gmail.com> wrote:
>>> When the host machine went to suspend mode and then wake it up, it send
>>> a USB_REQ_GET_STATUS.
>
> Can you expand on this ? It is not clear what the conditions to
> replicate this are. Please do so in detail.


For MacOS Yosemite:

Connect your board into your host machine and run UMS. Once, the icon
pop-up on your host machine (MacOS), put it in sleep mode (by clicking
sleep in the menu under apple icon on the upper left corner of the
screen). Then, once, you're sure that the host machine is in sleep
mode, just wake it up.

    1.0 If you load the uboot without the said patch, you should see a
message "Disk Not Ejected Properly", the icon disappeared and the
drive is no longer available.
    1.1 If you load the uboot with the said patch, you should be able
to access the drive after the host woke-up.

For Windows (I've only tested in Windows 7):
Similar procedures, after the icon pop-up on the screen, put the host
machine in sleep mode (by clicking sleep under "Shut down" menu). The
behavior should similar to 1.0 and 1.1.


Btw, if you are testing it in Windows, connect your device directly
into the USB port of your host machine, not in the HUB.


>
>>> The existing case condition below, never
>>> become true and it causes the host machine to reset the connection. Once,
>>> it reset you will see an error "Disk Not Ejected Properly".
>>>
>>> case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS):
>>>
>>> By applying this patch, the device could respond to the USB_REQ_GET_STATUS
>>> command correctly.
>>> ---
>>>  drivers/usb/gadget/ci_udc.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
>>> index d36bcf6..fdec613 100644
>>> --- a/drivers/usb/gadget/ci_udc.c
>>> +++ b/drivers/usb/gadget/ci_udc.c
>>> @@ -703,8 +703,8 @@ static void handle_setup(void)
>>>         list_del_init(&ci_req->queue);
>>>         ci_ep->req_primed = false;
>>>
>>> -       switch (SETUP(r.bRequestType, r.bRequest)) {
>>> -       case SETUP(USB_RECIP_ENDPOINT, USB_REQ_CLEAR_FEATURE):
>>> +       switch (r.bRequest) {
>>> +       case USB_REQ_CLEAR_FEATURE:
>>>                 _num = r.wIndex & 15;
>>>                 _in = !!(r.wIndex & 0x80);
>>>
>>> @@ -729,7 +729,7 @@ static void handle_setup(void)
>>>                 }
>>>                 return;
>>>
>>> -       case SETUP(USB_RECIP_DEVICE, USB_REQ_SET_ADDRESS):
>>> +       case USB_REQ_SET_ADDRESS:
>>>                 /*
>>>                  * write address delayed (will take effect
>>>                  * after the next IN txn)
>>> @@ -739,7 +739,7 @@ static void handle_setup(void)
>>>                 usb_ep_queue(controller.gadget.ep0, req, 0);
>>>                 return;
>>>
>>> -       case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS):
>>> +       case USB_REQ_GET_STATUS:
>>>                 req->length = 2;
>>>                 buf = (char *)req->buf;
>>>                 buf[0] = 1 << USB_DEVICE_SELF_POWERED;
>>> --
>>> 2.9.3
>>>
>
>
> --
> Best regards,
> Marek Vasut

Regards,

John Tobias

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] usb: gadget: ci_udc: fix suspend/resume of USB Mass Storage
  2016-08-24  1:30     ` John Tobias
@ 2016-08-24  3:29       ` Marek Vasut
  2016-08-24  5:25         ` John Tobias
  0 siblings, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2016-08-24  3:29 UTC (permalink / raw)
  To: u-boot

On 08/24/2016 03:30 AM, John Tobias wrote:
> Hi Marek,

Hi,

> On Tue, Aug 23, 2016 at 5:18 PM, Marek Vasut <marex@denx.de> wrote:
>> On 08/24/2016 12:49 AM, Fabio Estevam wrote:
>>> Adding Marek on Cc
>>
>> Thanks
>>
>>> On Mon, Aug 22, 2016 at 4:38 PM, John Tobias <john.tobias.ph@gmail.com> wrote:
>>>> When the host machine went to suspend mode and then wake it up, it send
>>>> a USB_REQ_GET_STATUS.
>>
>> Can you expand on this ? It is not clear what the conditions to
>> replicate this are. Please do so in detail.
> 
> 
> For MacOS Yosemite:
> 
> Connect your board into your host machine and run UMS. Once, the icon
> pop-up on your host machine (MacOS), put it in sleep mode (by clicking
> sleep in the menu under apple icon on the upper left corner of the
> screen). Then, once, you're sure that the host machine is in sleep
> mode, just wake it up.
> 
>     1.0 If you load the uboot without the said patch, you should see a
> message "Disk Not Ejected Properly", the icon disappeared and the
> drive is no longer available.
>     1.1 If you load the uboot with the said patch, you should be able
> to access the drive after the host woke-up.
> 
> For Windows (I've only tested in Windows 7):
> Similar procedures, after the icon pop-up on the screen, put the host
> machine in sleep mode (by clicking sleep under "Shut down" menu). The
> behavior should similar to 1.0 and 1.1.

OK, I see, now it's clearer . This really should be in the commit message.

> Btw, if you are testing it in Windows, connect your device directly
> into the USB port of your host machine, not in the HUB.

I think I'll have to trust you on all this.

Do you know which usb request arrives which throws the code off ?
I don't think it's correct to remove the bRequestType checking altogether.

Thanks

>>>> The existing case condition below, never
>>>> become true and it causes the host machine to reset the connection. Once,
>>>> it reset you will see an error "Disk Not Ejected Properly".
>>>>
>>>> case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS):
>>>>
>>>> By applying this patch, the device could respond to the USB_REQ_GET_STATUS
>>>> command correctly.
>>>> ---
>>>>  drivers/usb/gadget/ci_udc.c | 8 ++++----
>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
>>>> index d36bcf6..fdec613 100644
>>>> --- a/drivers/usb/gadget/ci_udc.c
>>>> +++ b/drivers/usb/gadget/ci_udc.c
>>>> @@ -703,8 +703,8 @@ static void handle_setup(void)
>>>>         list_del_init(&ci_req->queue);
>>>>         ci_ep->req_primed = false;
>>>>
>>>> -       switch (SETUP(r.bRequestType, r.bRequest)) {
>>>> -       case SETUP(USB_RECIP_ENDPOINT, USB_REQ_CLEAR_FEATURE):
>>>> +       switch (r.bRequest) {
>>>> +       case USB_REQ_CLEAR_FEATURE:
>>>>                 _num = r.wIndex & 15;
>>>>                 _in = !!(r.wIndex & 0x80);
>>>>
>>>> @@ -729,7 +729,7 @@ static void handle_setup(void)
>>>>                 }
>>>>                 return;
>>>>
>>>> -       case SETUP(USB_RECIP_DEVICE, USB_REQ_SET_ADDRESS):
>>>> +       case USB_REQ_SET_ADDRESS:
>>>>                 /*
>>>>                  * write address delayed (will take effect
>>>>                  * after the next IN txn)
>>>> @@ -739,7 +739,7 @@ static void handle_setup(void)
>>>>                 usb_ep_queue(controller.gadget.ep0, req, 0);
>>>>                 return;
>>>>
>>>> -       case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS):
>>>> +       case USB_REQ_GET_STATUS:
>>>>                 req->length = 2;
>>>>                 buf = (char *)req->buf;
>>>>                 buf[0] = 1 << USB_DEVICE_SELF_POWERED;
>>>> --
>>>> 2.9.3
>>>>
>>
>>
>> --
>> Best regards,
>> Marek Vasut
> 
> Regards,
> 
> John Tobias
> 


-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] usb: gadget: ci_udc: fix suspend/resume of USB Mass Storage
  2016-08-24  3:29       ` Marek Vasut
@ 2016-08-24  5:25         ` John Tobias
  2016-08-24 15:17           ` Marek Vasut
  2016-09-08 11:05           ` Fabio Estevam
  0 siblings, 2 replies; 9+ messages in thread
From: John Tobias @ 2016-08-24  5:25 UTC (permalink / raw)
  To: u-boot

Hi Marek,

On Tue, Aug 23, 2016 at 8:29 PM, Marek Vasut <marex@denx.de> wrote:
> On 08/24/2016 03:30 AM, John Tobias wrote:
>> Hi Marek,
>
> Hi,
>
>> On Tue, Aug 23, 2016 at 5:18 PM, Marek Vasut <marex@denx.de> wrote:
>>> On 08/24/2016 12:49 AM, Fabio Estevam wrote:
>>>> Adding Marek on Cc
>>>
>>> Thanks
>>>
>>>> On Mon, Aug 22, 2016 at 4:38 PM, John Tobias <john.tobias.ph@gmail.com> wrote:
>>>>> When the host machine went to suspend mode and then wake it up, it send
>>>>> a USB_REQ_GET_STATUS.
>>>
>>> Can you expand on this ? It is not clear what the conditions to
>>> replicate this are. Please do so in detail.
>>
>>
>> For MacOS Yosemite:
>>
>> Connect your board into your host machine and run UMS. Once, the icon
>> pop-up on your host machine (MacOS), put it in sleep mode (by clicking
>> sleep in the menu under apple icon on the upper left corner of the
>> screen). Then, once, you're sure that the host machine is in sleep
>> mode, just wake it up.
>>
>>     1.0 If you load the uboot without the said patch, you should see a
>> message "Disk Not Ejected Properly", the icon disappeared and the
>> drive is no longer available.
>>     1.1 If you load the uboot with the said patch, you should be able
>> to access the drive after the host woke-up.
>>
>> For Windows (I've only tested in Windows 7):
>> Similar procedures, after the icon pop-up on the screen, put the host
>> machine in sleep mode (by clicking sleep under "Shut down" menu). The
>> behavior should similar to 1.0 and 1.1.
>
> OK, I see, now it's clearer . This really should be in the commit message.

I'll resend v2 to include it.

>
>> Btw, if you are testing it in Windows, connect your device directly
>> into the USB port of your host machine, not in the HUB.
>
> I think I'll have to trust you on all this.
>
> Do you know which usb request arrives which throws the code off ?

USB_REQ_GET_STATUS.

I tried to following combinations below but, the return
SETUP(r.bRequestType, r.bRequest) doesn't match at all.

SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS)
SETUP(USB_RECIP_DEVICE, USB_REQ_GET_STATUS)
SETUP(USB_DIR_IN, USB_REQ_GET_STATUS)

> I don't think it's correct to remove the bRequestType checking altogether.

Looking at the fsl_udc_core.c
http://lxr.free-electrons.com/source/drivers/usb/gadget/udc/fsl_udc_core.c#L1405
looks identical.

>
> Thanks
>
>>>>> The existing case condition below, never
>>>>> become true and it causes the host machine to reset the connection. Once,
>>>>> it reset you will see an error "Disk Not Ejected Properly".
>>>>>
>>>>> case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS):
>>>>>
>>>>> By applying this patch, the device could respond to the USB_REQ_GET_STATUS
>>>>> command correctly.
>>>>> ---
>>>>>  drivers/usb/gadget/ci_udc.c | 8 ++++----
>>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
>>>>> index d36bcf6..fdec613 100644
>>>>> --- a/drivers/usb/gadget/ci_udc.c
>>>>> +++ b/drivers/usb/gadget/ci_udc.c
>>>>> @@ -703,8 +703,8 @@ static void handle_setup(void)
>>>>>         list_del_init(&ci_req->queue);
>>>>>         ci_ep->req_primed = false;
>>>>>
>>>>> -       switch (SETUP(r.bRequestType, r.bRequest)) {
>>>>> -       case SETUP(USB_RECIP_ENDPOINT, USB_REQ_CLEAR_FEATURE):
>>>>> +       switch (r.bRequest) {
>>>>> +       case USB_REQ_CLEAR_FEATURE:
>>>>>                 _num = r.wIndex & 15;
>>>>>                 _in = !!(r.wIndex & 0x80);
>>>>>
>>>>> @@ -729,7 +729,7 @@ static void handle_setup(void)
>>>>>                 }
>>>>>                 return;
>>>>>
>>>>> -       case SETUP(USB_RECIP_DEVICE, USB_REQ_SET_ADDRESS):
>>>>> +       case USB_REQ_SET_ADDRESS:
>>>>>                 /*
>>>>>                  * write address delayed (will take effect
>>>>>                  * after the next IN txn)
>>>>> @@ -739,7 +739,7 @@ static void handle_setup(void)
>>>>>                 usb_ep_queue(controller.gadget.ep0, req, 0);
>>>>>                 return;
>>>>>
>>>>> -       case SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS):
>>>>> +       case USB_REQ_GET_STATUS:
>>>>>                 req->length = 2;
>>>>>                 buf = (char *)req->buf;
>>>>>                 buf[0] = 1 << USB_DEVICE_SELF_POWERED;
>>>>> --
>>>>> 2.9.3
>>>>>
>>>
>>>
>>> --
>>> Best regards,
>>> Marek Vasut
>>
>> Regards,
>>
>> John Tobias
>>
>
>
> --
> Best regards,
> Marek Vasut

Regards,

John Tobias

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] usb: gadget: ci_udc: fix suspend/resume of USB Mass Storage
  2016-08-24  5:25         ` John Tobias
@ 2016-08-24 15:17           ` Marek Vasut
  2016-09-08 11:05           ` Fabio Estevam
  1 sibling, 0 replies; 9+ messages in thread
From: Marek Vasut @ 2016-08-24 15:17 UTC (permalink / raw)
  To: u-boot

On 08/24/2016 07:25 AM, John Tobias wrote:
> Hi Marek,

Hi!

> On Tue, Aug 23, 2016 at 8:29 PM, Marek Vasut <marex@denx.de> wrote:
>> On 08/24/2016 03:30 AM, John Tobias wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> On Tue, Aug 23, 2016 at 5:18 PM, Marek Vasut <marex@denx.de> wrote:
>>>> On 08/24/2016 12:49 AM, Fabio Estevam wrote:
>>>>> Adding Marek on Cc
>>>>
>>>> Thanks
>>>>
>>>>> On Mon, Aug 22, 2016 at 4:38 PM, John Tobias <john.tobias.ph@gmail.com> wrote:
>>>>>> When the host machine went to suspend mode and then wake it up, it send
>>>>>> a USB_REQ_GET_STATUS.
>>>>
>>>> Can you expand on this ? It is not clear what the conditions to
>>>> replicate this are. Please do so in detail.
>>>
>>>
>>> For MacOS Yosemite:
>>>
>>> Connect your board into your host machine and run UMS. Once, the icon
>>> pop-up on your host machine (MacOS), put it in sleep mode (by clicking
>>> sleep in the menu under apple icon on the upper left corner of the
>>> screen). Then, once, you're sure that the host machine is in sleep
>>> mode, just wake it up.
>>>
>>>     1.0 If you load the uboot without the said patch, you should see a
>>> message "Disk Not Ejected Properly", the icon disappeared and the
>>> drive is no longer available.
>>>     1.1 If you load the uboot with the said patch, you should be able
>>> to access the drive after the host woke-up.
>>>
>>> For Windows (I've only tested in Windows 7):
>>> Similar procedures, after the icon pop-up on the screen, put the host
>>> machine in sleep mode (by clicking sleep under "Shut down" menu). The
>>> behavior should similar to 1.0 and 1.1.
>>
>> OK, I see, now it's clearer . This really should be in the commit message.
> 
> I'll resend v2 to include it.

Thanks, but please hold on a bit before we finish this discussion.

>>> Btw, if you are testing it in Windows, connect your device directly
>>> into the USB port of your host machine, not in the HUB.
>>
>> I think I'll have to trust you on all this.
>>
>> Do you know which usb request arrives which throws the code off ?
> 
> USB_REQ_GET_STATUS.
> 
> I tried to following combinations below but, the return
> SETUP(r.bRequestType, r.bRequest) doesn't match at all.
> 
> SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS)
> SETUP(USB_RECIP_DEVICE, USB_REQ_GET_STATUS)
> SETUP(USB_DIR_IN, USB_REQ_GET_STATUS)

So you get bRequest = USB_REQ_GET_STATUS request with bRequestType set
to what exactly to trigger this bug ?

>> I don't think it's correct to remove the bRequestType checking altogether.
> 
> Looking at the fsl_udc_core.c
> http://lxr.free-electrons.com/source/drivers/usb/gadget/udc/fsl_udc_core.c#L1405
> looks identical.

That doesn't imply it's correct .

[...]

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] usb: gadget: ci_udc: fix suspend/resume of USB Mass Storage
  2016-08-24  5:25         ` John Tobias
  2016-08-24 15:17           ` Marek Vasut
@ 2016-09-08 11:05           ` Fabio Estevam
  2016-09-08 14:42             ` John Tobias
  1 sibling, 1 reply; 9+ messages in thread
From: Fabio Estevam @ 2016-09-08 11:05 UTC (permalink / raw)
  To: u-boot

Hi John,

On Wed, Aug 24, 2016 at 2:25 AM, John Tobias <john.tobias.ph@gmail.com> wrote:

> I tried to following combinations below but, the return
> SETUP(r.bRequestType, r.bRequest) doesn't match at all.
>
> SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS)
> SETUP(USB_RECIP_DEVICE, USB_REQ_GET_STATUS)
> SETUP(USB_DIR_IN, USB_REQ_GET_STATUS)
>
>> I don't think it's correct to remove the bRequestType checking altogether.
>
> Looking at the fsl_udc_core.c
> http://lxr.free-electrons.com/source/drivers/usb/gadget/udc/fsl_udc_core.c#L1405
> looks identical.

This is the old udc driver for the non-dt imx.

Currently we use drivers/usb/chipidea/udc.c, which also seems to match
what you do in your patch.

Regards,

Fabio Estevam

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [U-Boot] [PATCH] usb: gadget: ci_udc: fix suspend/resume of USB Mass Storage
  2016-09-08 11:05           ` Fabio Estevam
@ 2016-09-08 14:42             ` John Tobias
  0 siblings, 0 replies; 9+ messages in thread
From: John Tobias @ 2016-09-08 14:42 UTC (permalink / raw)
  To: u-boot

Hi Fabio,

On Thu, Sep 8, 2016 at 4:05 AM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi John,
>
> On Wed, Aug 24, 2016 at 2:25 AM, John Tobias <john.tobias.ph@gmail.com> wrote:
>
>> I tried to following combinations below but, the return
>> SETUP(r.bRequestType, r.bRequest) doesn't match at all.
>>
>> SETUP(USB_DIR_IN | USB_RECIP_DEVICE, USB_REQ_GET_STATUS)
>> SETUP(USB_RECIP_DEVICE, USB_REQ_GET_STATUS)
>> SETUP(USB_DIR_IN, USB_REQ_GET_STATUS)
>>
>>> I don't think it's correct to remove the bRequestType checking altogether.
>>
>> Looking at the fsl_udc_core.c
>> http://lxr.free-electrons.com/source/drivers/usb/gadget/udc/fsl_udc_core.c#L1405
>> looks identical.
>
> This is the old udc driver for the non-dt imx.
>
> Currently we use drivers/usb/chipidea/udc.c, which also seems to match
> what you do in your patch.
>
> Regards,
>
> Fabio Estevam

Thanks for the info and clarification.

Btw, any updates with this patch?.

Regards,

John

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-09-08 14:42 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-22 19:38 [U-Boot] [PATCH] usb: gadget: ci_udc: fix suspend/resume of USB Mass Storage John Tobias
2016-08-23 22:49 ` Fabio Estevam
2016-08-24  0:18   ` Marek Vasut
2016-08-24  1:30     ` John Tobias
2016-08-24  3:29       ` Marek Vasut
2016-08-24  5:25         ` John Tobias
2016-08-24 15:17           ` Marek Vasut
2016-09-08 11:05           ` Fabio Estevam
2016-09-08 14:42             ` John Tobias

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.