[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize
Steve Rae
steve.rae at broadcom.com
Fri Apr 15 21:56:17 CEST 2016
On Thu, Apr 14, 2016 at 4:15 AM, Roger Quadros <rogerq at ti.com> wrote:
> Steve,
>
> On 13/04/16 04:55, Steve Rae wrote:
> > On Tue, Apr 12, 2016 at 6:50 AM, Roger Quadros <rogerq at ti.com> wrote:
> >> Lukasz,
> >>
> >> On 12/04/16 16:37, Lukasz Majewski wrote:
> >>> 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 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!
> >>>>>
> >>>>> 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.
> >>
> >> It is not an errata but stated in the dwc3 user manual like so
> >>
> >> section 8.2.3.3 Buffer Size Rules and Zero-Length Packets
> >>
> >> For OUT endpoints, the following rules apply:
> >> ■ The BUFSIZ field must be ≥ 1 byte.
> >> ■ The total size of a Buffer Descriptor must be a multiple of
> MaxPacketSize
> >> ■ A received zero-length packet still requires a MaxPacketSize buffer.
> Therefore, if the expected
> >> amount of data to be received is a multiple of MaxPacketSize, software
> should add MaxPacketSize
> >> bytes to the buffer to sink a possible zero-length packet at the end of
> the transfer.
> >>
> >>>
> >>>> 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.
> >
> > Yes -- this is the one that causes my stalling issue:
> > I'll copy some debug output from another email thread:
> >
> > Lukasz:
> > As per your suggestion, I turned on the following:
> > diff --git a/drivers/usb/gadget/dwc2_udc_otg.c
> > b/drivers/usb/gadget/dwc2_udc_otg.c
> > index 5d53440..763c6d9 100644
> > --- a/drivers/usb/gadget/dwc2_udc_otg.c
> > +++ b/drivers/usb/gadget/dwc2_udc_otg.c
> > @@ -40,11 +40,11 @@
> >
> > #define OTG_DMA_MODE 1
> >
> > -#define DEBUG_SETUP 0
> > -#define DEBUG_EP0 0
> > -#define DEBUG_ISR 0
> > -#define DEBUG_OUT_EP 0
> > -#define DEBUG_IN_EP 0
> > +#define DEBUG_SETUP 1
> > +#define DEBUG_EP0 1
> > +#define DEBUG_ISR 1
> > +#define DEBUG_OUT_EP 1
> > +#define DEBUG_IN_EP 1
> >
> > and captured the logs of the "last transactions..." (the "-" is with
> > the Feb 25 Patch removed, the "+" is with the Feb 25 Patch
> > applied....)
> >
> > *** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP),
> > GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003
> > *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000
> > EP2-OUT : DOEPINT = 0x2011
> > complete_rx: RX DMA done : ep = 2, rx bytes = 4096/4096, is_short =
> > 0, DOEPTSIZ = 0x0, remained bytes = 4096
> > complete_rx: Next Rx request start...
> > setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ =
> > 0x401000, DOEPCTL = 0x80098200
> > buf = 0xffb84f80, pktcnt = 8, xfersize = 4096
> >
> > *** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP),
> > GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003
> > *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000
> > EP2-OUT : DOEPINT = 0x2011
> > complete_rx: RX DMA done : ep = 2, rx bytes = 4096/4096, is_short =
> > 0, DOEPTSIZ = 0x0, remained bytes = 4096
> > complete_rx: Next Rx request start...
> > -setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ =
> > 0x100218, DOEPCTL = 0x80098200
> > - buf = 0xffb84f80, pktcnt = 2, xfersize = 536
> > +setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ =
> > 0x100400, DOEPCTL = 0x80098200
> > + buf = 0xffb84f80, pktcnt = 2, xfersize = 1024
> >
>
> This part looks fine as we're rounding up 536 to 1024 for 512 byte
> alignment.
> > *** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP),
> > GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003
> > *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000
> > EP2-OUT : DOEPINT = 0x2011
> > -complete_rx: RX DMA done : ep = 2, rx bytes = 536/536, is_short = 0,
> > DOEPTSIZ = 0x0, remained bytes = 536
>
the "rx bytes = 536/536" (in the working code)...
so maybe the driver "knows" that there are no more bytes to come ?!?!?!?
> > -dwc2_queue: ep_is_in, DWC2_UDC_OTG_GINTSTS=0x14008028
> > -setdma_tx:EP1 TX DMA start : DIEPDMA0 = 0xffb85fc0,DIEPTSIZ0 =
> > 0x80004, DIEPCTL0 = 0x80498040
> > - buf = 0xffb85fc0, pktcnt = 1, xfersize = 4
> > +complete_rx: RX DMA done : ep = 2, rx bytes = 536/1024, is_short = 0,
> > DOEPTSIZ = 0x1e8, remained bytes = 536
>
> Here it says we completed the 536 bytes last transfer right?
>
the "rx bytes = 536/1024" (in the NON-working code)...
so maybe the driver "thinks" that there are still 1024-536=488 bytes to
come ?!?!?!?
>
> > +setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb85198,DOEPTSIZ =
> > 0x801e8, DOEPCTL = 0x80098200
> > + buf = 0xffb85198, pktcnt = 1, xfersize = 488
>
> Why is this additional 488 bytes being queued? This is the real issue
> we need to debug.
>
so maybe because we told the driver to "expect" 1024 bytes, when if fact we
only "expect" 536 ?!?!?!?
I really don't know, and have not spent any time inside the driver code --
because it was always working properly......
>
> cheers,
> -roger
>
> >
> > +++++++ hangs here...
> > -downloading of 258584 bytes finished
> > -complete_rx: Next Rx request start...
> > -setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ =
> > 0x401000, DOEPCTL = 0x80098200
> > - buf = 0xffb84f80, pktcnt = 8, xfersize = 4096
> >
> > Does this help explain anything ?!?!?!
> >
> > Thanks, Steve
> >
> >
> >
> >>>>
> >>>> 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.
> >
> > I am fine with these patches -- Thanks Steve
> >
> >>>
> >>> Those are now under review :-)
> >>>
> >> Thanks :)
> >>
> >> cheers,
> >> -roger
>
More information about the U-Boot
mailing list