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

[Lukasz Majewski <l.majewski@samsung.com>]

Hi Marek,

> 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@denx.de
> > <mailto:marex@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@linaro.org
> >     <mailto: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
> >     <mailto: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? )
> >     > ( 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.

But the problem here is not connected with UMS/DFU/Thor which are on
the upper layer in the stack.

This is somewhat two fold:

1. Your host (libusb which one is using + fastboot implementation)
requires transfers which are padded to some (fixed?) value (not checking
what USB device descriptor is saying for example).

2. The UDC IP block silicon implementation is different for those two
companies.

Is Broadcomm using any other gadgets (DFU/UMS)? Is fastboot the only one
available (as it is done at bcm28155_ap.h) ?

To fix this problem I use debug from dwc2 UDC driver to see what
requests are commit IN and OUT (and when it hangs).

I also use Lauterbach to inspect memory state and registers.

However, I will not help you much since you are using different UDC IP
block and driver (musb vs dwc2_udc_otg).

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



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group