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

[Marek Vasut]

On 04/06/2016 10:45 PM, Steve Rae wrote:
> Thanks for the reply, Marek....
> 
> On Wed, Apr 6, 2016 at 12:53 PM, Marek Vasut <marex at denx.de
> <mailto:marex at denx.de>> wrote:
> 
>     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 ?
> 
> 
> maybe -- I do not know....

Anyone with MUSB in Gadget mode who can test this ? I think some sunxi
had 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
>     <mailto: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
>     <mailto: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! )
> 
> 
> USB experts (Lukasz?): any ideas here.... 

I think Lukasz only uses UMS and Thor.

>     >
>     > 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 ?
> 
> 
> yes -- that is my impression .....

Bad.

>     > 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.
> 
> 
> OK -- I am fine with that....
>  
> 
>     > 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 ;-)
> 
> 
> Tom -- thanks in advance!
>  
> 
> 
>     > 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.
> 
> 
> OK -- I am happy to be patient for a while....  And that is also why I
> submitted the request to "revert" this change -- that email thread
> actually did spark a bit of a conversation; but then it seemed to die
> without any real resolution.....

I was not paying much attention to it as it's a gadget stuff and I am
not tracking gadget stuff that much. I will dive into it later.

Best regards,
Marek Vasut