[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize
Steve Rae
steve.rae at broadcom.com
Thu Apr 7 19:07:06 CEST 2016
Hi Sam,
On Thu, Apr 7, 2016 at 9:46 AM, Sam Protsenko <semen.protsenko at linaro.org>
wrote:
> On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski <l.majewski at 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 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? )
> >
> > 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.
>
I'm confused...
You are saying that it is OK to checkin a change that fixes TI boards (Feb
25), even though it breaks Broadcom boards;
but if _this_ change "breaks anything" then it is NOT OK ?????
( I politely disagree.... )
PS - therefore - what is the right way? (..."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.
So -- here is why I cannot complete this task:
I have absolutely no idea which boards actually _require_ this capability,
therefore, I have no idea which defconfigs I would need to update!
So, if you would send me a list of which defconfig files need to add this
line, I'll update it....
OR (I would prefer) you could submit a v3 which includes the boards that
you know require this capability!
Let me know,
Thanks, Steve
> 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).
>
yup -- I use buildman almost exclusively....
> 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'm confused (again) -- why are you asking: "you guys to not break anything
else"...
IT IS ALREADY BROKEN, it is broken right now, and has been broken since Feb
25 !
Please fix this!
> >> 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,
> >
> > Lukasz Majewski
> >
> > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
>
More information about the U-Boot
mailing list