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

[Marek Vasut <marex@denx.de>]

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

Best regards,
Marek Vasut