[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize
Steve Rae
steve.rae at broadcom.com
Wed Apr 6 22:45:35 CEST 2016
Thanks for the reply, Marek....
On Wed, Apr 6, 2016 at 12:53 PM, Marek Vasut <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....
>
> > 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! )
>
USB experts (Lukasz?): any ideas here....
>
> > 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 .....
>
> > 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.....
Thanks, Steve
> > 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
>
More information about the U-Boot
mailing list