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

[Marek Vasut]

On 04/06/2016 07:18 PM, Steve Rae wrote:
> 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.

OK

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

TI platforms use musb USB/OTG controller, could the issue them be
specific to MUSB ?

> 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? )
> ( 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)....

OK, so, either way this is broken for some platforms and noone is
interested enough to cooperate and fix this properly, yes ?

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

Definitely not, I will not have a new macro added just to paper over
some problem which noone bothered to research and fix properly, sorry.

> 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? 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....

I will pass this question onto Tom ;-)

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

Kick the TI person into the backside until he comes up with a proper
solution. Be annoying. Or, if that leads nowhere, I will just apply
the revert and break it for TI and expect them to fix it proper.

btw. note that ELC is going on this week, so replies might be delayed.

> Thanks, Steve
> 
> On Wed, Apr 6, 2016 at 4:01 AM, Marek Vasut <marex at denx.de> wrote:
>> On 04/06/2016 07:35 AM, Steve Rae wrote:
>>>
>>> On Apr 5, 2016 3:07 PM, "Marek Vasut" <marex at denx.de
>>> <mailto:marex at denx.de>> wrote:
>>>>
>>>> On 04/05/2016 08:31 PM, Steve Rae wrote:
>>>>> commit 9e4b510 fastboot: OUT transaction length must be aligned to
>>> wMaxPacketSize
>>>>> breaks some boards...
>>>>>
>>>>> Therefore add a conditional Kconfig to optionally enable this feature.
>>>>
>>>> Did you drill into it to figure out why this is needed ?
>>>>
>>>
>>> Marek,
>>> Let me clarify....
>>> All my boards work with the original code (before the commit which
>>> aligned  the size to the wMaxPacketSize).... Since that commit, all my
>>> boards are broken.
>>> And you will notice in this patch, that none of my boards define this
>>> CONFIG_ ...
>>>
>>> So I think you are asking the wrong person to drill down into this issue....
>>> Sorry, Steve
>>
>> Well who else can I ask ? You're our only hope at fixing this proper.
>>
>> Anyway, see my other reply, maybe we should just add an arg to fastboot
>> command to select one more of operation or the other and default to the
>> one which works.
>>
>> --
>> Best regards,
>> Marek Vasut


-- 
Best regards,
Marek Vasut