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

[Mugunthan V N]

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 at 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 at 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 at 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

Regards
Mugunthan V N