[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Roger Quadros <rogerq@ti.com>]

Lukasz,

On 12/04/16 16:37, Lukasz Majewski wrote:
> Hi Roger,
> 
>> Hi,
>>
>> On 12/04/16 14:19, Lukasz Majewski wrote:
>>> Hi Tom, Mugunthan
>>>
>>>> On Mon, Apr 11, 2016 at 05:04:56PM +0530, Mugunthan V N wrote:
>>>>> On Friday 08 April 2016 12:10 AM, Marek Vasut wrote:
>>>>>> On 04/07/2016 06:46 PM, Sam Protsenko wrote:
>>>>>>> On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski
>>>>>>> <l.majewski@samsung.com> wrote:
>>>>>>>> Hi Steve,
>>>>>>>>
>>>>>>>>> No -- I do not believe that this issue is caused by different
>>>>>>>>> fastboot (client) versions (the executable that runs on the
>>>>>>>>> host computer - Linux, Windows, Mac, etc.)
>>>>>>>>> I have personally attempted three (3) different versions, and
>>>>>>>>> the results are consistent.
>>>>>>>>>
>>>>>>>>> And no I don't think that I "am the only hope at fixing this
>>>>>>>>> proper" -- as you will see below,
>>>>>>>>> this" issue" seems to be unique to the "TI platforms" (...
>>>>>>>>> nobody else has stated they have an issue either way -- but I
>>>>>>>>> don't think many use this feature ....)
>>>>>>>>> So maybe someone with "TI platforms" could investigate this
>>>>>>>>> more thoroughly...
>>>>>>>>>
>>>>>>>>> HISTORY:
>>>>>>>>>
>>>>>>>>> The U-Boot code, up to Feb 25, worked properly on my Broadcom
>>>>>>>>> boards -- this code contains:
>>>>>>>>>                req->length = rx_bytes_expected();
>>>>>>>>>                 if (req->length < ep->maxpacket)
>>>>>>>>>                         req->length = ep->maxpacket;
>>>>>>>>> which aligned the remaining "rx_bytes_expected" to be aligned
>>>>>>>>> to the "ep->maxpacket" size.
>>>>>>>>>
>>>>>>>>> On Feb 25, there was a patch applied from
>>>>>>>>> <dileep.katta@linaro.org> which forces the remaining
>>>>>>>>> "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size
>>>>>>>>> -- this patch broke all Broadcom boards:
>>>>>>>>> +       if (rx_remain < maxpacket) {
>>>>>>>>> +               rx_remain = maxpacket;
>>>>>>>>> +       } else if (rx_remain % maxpacket != 0) {
>>>>>>>>> +               rem = rx_remain % maxpacket;
>>>>>>>>> +               rx_remain = rx_remain + (maxpacket - rem);
>>>>>>>>> +       }
>>>>>>>>>
>>>>>>>>> After attempting to unsuccessfully contact Dileep, I requested
>>>>>>>>> that this patch be reverted -- because it broke my boards!
>>>>>>>>> (see the other email thread).
>>>>>>>>>
>>>>>>>>> Sam Protsenko <semen.protsenko@linaro.org> has stated that
>>>>>>>>> this Feb 25 change is required to make "fastboot work on TI
>>>>>>>>> platforms".
>>>>>>>>>
>>>>>>>>> Thus,
>>>>>>>>> - Broadcom boards require alignment to "ep->maxpacket" size
>>>>>>>>> - TI platforms require alignment to "wMaxPacketSize" size
>>>>>>>>> And we seem to be at a stale-mate.
>>>>>>>>> Unfortunately, I do not know enough about the USB internals to
>>>>>>>>> understand why this change breaks the Broadcom boards; or why
>>>>>>>>> it _is_ required on the TI platforms....
>>>>>>>>> ( Is there any debugging that can be turned on to validate
>>>>>>>>> what is happening at the lower levels? )
>>>>>>>>
>>>>>>>> I can only speak about DWC2 (from Synopsis) embedded at Samsung
>>>>>>>> boards. There are low level debugging registers (documented,
>>>>>>>> but not supposed to be used at normal operation), which give
>>>>>>>> you some impression regarding very low level events.
>>>>>>>>
>>>>>>>> DWC2 at Samsung is using those to work properly since we had
>>>>>>>> some problems with dwc2 IP blocks implementation on early
>>>>>>>> Samsung platforms :-). This approach works in u-boot up till
>>>>>>>> now.
>>>>>>>>
>>>>>>>> Another option is to use JTAG debugger (like Lauterbach) to
>>>>>>>> inspect state of this IP block.
>>>>>>>>
>>>>>>>>> ( Can anyone explain why "wMaxPacketSize" size would be
>>>>>>>>> required? -- my limited understanding of endpoints makes me
>>>>>>>>> think that "ep->maxpacket" size is actually the correct value!
>>>>>>>>> )
>>>>>>>>>
>>>>>>>>> I asked Sam to submit a patch which conditionally applied the
>>>>>>>>> alignment to "wMaxPacketSize" size change -- he stated that he
>>>>>>>>> was too busy right now -- so I submitted this patch on his
>>>>>>>>> behalf (although he still needs to add the Kconfig for the TI
>>>>>>>>> platforms in order to make his boards work)....
>>>>>>>>>
>>>>>>>>> I suppose I could also propose a patch where the condition
>>>>>>>>> _removes_ this feature (and define it on the Broadcom boards)
>>>>>>>>> -- do we generally like "negated" conditionals?
>>>>>>>>> +#ifndef
>>>>>>>>> CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE
>>>>>>>>> Please advise!
>>>>>>>>>
>>>>>>>>> Further, how does the U-Boot community respond to a change
>>>>>>>>> which breaks something which is already working? Doesn't the
>>>>>>>>> "author" of that change bear any responsibility on assisting
>>>>>>>>> to get "their" change working properly with "all" the existing
>>>>>>>>> boards?
>>>>>>>>
>>>>>>>> As we know the author of this change is not working at Linaro
>>>>>>>> anymore.
>>>>>>>>
>>>>>>>>> I'm getting the
>>>>>>>>> impression that "because the current code works for me", that
>>>>>>>>> I am not getting any assistance in resolving this issue --
>>>>>>>>> which is why I suggested "reverting" this change back to the
>>>>>>>>> original code; that way, it would (politely?) force someone
>>>>>>>>> interested in "TI platforms" to step up and look into this....
>>>>>>>>>
>>>>>>>>> Sorry for asking so many questions in one email -- but I'd
>>>>>>>>> appreciate answers....
>>>>>>>>> ( I also apologize in advance for the "attitude" which is
>>>>>>>>> leaking into this email... )
>>>>>>>>> Please tell me what I can do! I had working boards; now they
>>>>>>>>> are all broken -- and I don't how how to get them working
>>>>>>>>> again....
>>>>>>>>
>>>>>>>> If you don't have enough time (and HW) for investigate the
>>>>>>>> issue, I think that Kconfig option with documentation entry is
>>>>>>>> the way to go.
>>>>>>>>
>>>>>>>> I hope that Sam don't have any objections with such approach.
>>>>>>>>
>>>>>>>
>>>>>>> If this commit doesn't break any platform -- I'm ok with that.
>>>>>>> If it breaks anything (TI boards particularly) -- I'd ask to
>>>>>>> revert it at once, as this is I believe not right way to do
>>>>>>> things.
>>>>>>>
>>>>>>> So Steve, please add
>>>>>>> CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED option to
>>>>>>> all required defconfigs (except yours), so that your patch only
>>>>>>> fixes your platforms, but doesn't break any other platform at
>>>>>>> the same time. Also good thing to do after that is check options
>>>>>>> order in changed defconfigs with "make savedefconfig" rule. Both
>>>>>>> your current changes and appropriate lines in defconfigs should
>>>>>>> be committed as a single patch, so that it doesn't break
>>>>>>> anything and "git bisect" may be used to look for regressions.
>>>>>>> Also, really nice thing to do after all of this, is to use
>>>>>>> "./tools/buildman/buildman" tool to check all ARM boards for
>>>>>>> regressions after your patch (you should see that only your
>>>>>>> boards were changed).
>>>>>>>
>>>>>>> Ideally, we should check it on all boards (or at least on all
>>>>>>> UDC controllers supported in U-Boot) and figure out what is
>>>>>>> happening exactly. But I'm totally fine with hack if it fixes
>>>>>>> real problem on some platforms. I just ask you guys to not
>>>>>>> break anything else at the same time (although it surely takes
>>>>>>> much more effort, but still).
>>>>>>
>>>>>> I am totally not fine with hack, so please fix it such that both
>>>>>> platforms work without added config option. Thanks
>>>>>>
>>>>>
>>>>> The issue is already solved in Kernel with the patch [1]. May we
>>>>> can take a similar approach and fix the issue without having
>>>>> config options.
>>>>>
>>>>> [1]:
>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0b2d2bbade59ab2067f326d6dbc2628bee227fd5
>>>>
>>>> This seems reasonable.  Can you do this, along with a follow-up
>>>> patch that sets it for DWC3?  Thanks!
>>>
>>> If I can add my two cents.
>>>
>>>
>>> I believe that it would be worth to add some explanation into at
>>> least the commit message (like very short excerpt from respective
>>> User Manual or at least chapter number for further reference).
>>
>> The patch in [1] is about setting USB request buffer aligned to
>> MaxPacketSize. In f_fastboot.c case we allocate request buffer like so
>> 	req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE,
>> EP_BUFFER_SIZE);
>>
>> where EP_BUFFER_SIZE is 4096 which is an integral multiple of 512 as
>> well as 64. So I'm not sure how [1] is related to the subject and if
>> it will fix anything.
>>
>> I think the problem is more about the length of the last OUT transfer
>> packet. Some controllers might not like that to be not an integral
>> multiple of wMaxPacketSize and we need to ensure that. 
> 
> My question was about the above sentence. I was wondering if there is
> any errata or user manual entry explicitly specifying that.

It is not an errata but stated in the dwc3 user manual like so

section 8.2.3.3	Buffer Size Rules and Zero-Length Packets

For OUT endpoints, the following rules apply:
? The BUFSIZ field must be ? 1 byte.
? The total size of a Buffer Descriptor must be a multiple of MaxPacketSize
? A received zero-length packet still requires a MaxPacketSize buffer. Therefore, if the expected
amount of data to be received is a multiple of MaxPacketSize, software should add MaxPacketSize
bytes to the buffer to sink a possible zero-length packet at the end of the transfer.

> 
>> This is being
>> done in f_mass_storage.c in set_bulk_out_req_length(). Doing that
>> shouldn't affect other controllers.
>>
>> So we need to really fix commit 9e4b510.
>>
>> Another thing I noticed is that f_fastboot.c is not setting the right
>> endpoint size for hight speed BULK_IN endpoint. I'll send out patches
>> for that.
> 
> Those are now under review :-)
> 
Thanks :)

cheers,
-roger