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

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

Hi Roger,

> Hi,
> 
> On 12/04/16 14:19, Lukasz Majewski wrote:
> > Hi Tom, Mugunthan
> > 
> >> 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@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@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> 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!
> > 
> > If I can add my two cents.
> > 
> > 
> > I believe that it would be worth to add some explanation into at
> > least the commit message (like very short excerpt from respective
> > User Manual or at least chapter number for further reference).
> 
> The patch in [1] is about setting USB request buffer aligned to
> MaxPacketSize. In f_fastboot.c case we allocate request buffer like so
> 	req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE,
> EP_BUFFER_SIZE);
> 
> where EP_BUFFER_SIZE is 4096 which is an integral multiple of 512 as
> well as 64. So I'm not sure how [1] is related to the subject and if
> it will fix anything.
> 
> I think the problem is more about the length of the last OUT transfer
> packet. Some controllers might not like that to be not an integral
> multiple of wMaxPacketSize and we need to ensure that. 

My question was about the above sentence. I was wondering if there is
any errata or user manual entry explicitly specifying that.

> This is being
> done in f_mass_storage.c in set_bulk_out_req_length(). Doing that
> shouldn't affect other controllers.
> 
> So we need to really fix commit 9e4b510.
> 
> Another thing I noticed is that f_fastboot.c is not setting the right
> endpoint size for hight speed BULK_IN endpoint. I'll send out patches
> for that.

Those are now under review :-)

> 
> cheers,
> -roger
> 



-- 
Best regards,

Lukasz Majewski

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