[U-Boot] [PATCH] usb: gadget: ci_udc: fix suspend/resume of USB Mass Storage

[John Tobias]

Hi Marek,

On Tue, Aug 23, 2016 at 8:29 PM, Marek Vasut <marex at 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 at 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 at 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