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

Mon Apr 11 17:22:28 CEST 2016

On Mon, Apr 11, 2016 at 05:04:56PM +0530, Mugunthan V N wrote:
> On Friday 08 April 2016 12:10 AM, Marek Vasut wrote:
> > On 04/07/2016 06:46 PM, Sam Protsenko 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.
> >>
> >> 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. 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).
> >>
> >> 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 am totally not fine with hack, so please fix it such that both
> > platforms work without added config option. Thanks
> > 
> 
> The issue is already solved in Kernel with the patch [1]. May we can
> take a similar approach and fix the issue without having config options.
> 
> [1]:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0b2d2bbade59ab2067f326d6dbc2628bee227fd5

This seems reasonable.  Can you do this, along with a follow-up patch
that sets it for DWC3?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160411/2a4031a8/attachment.sig>