[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize
Roger Quadros
rogerq at ti.com
Thu Apr 14 13:15:06 CEST 2016
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
> -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?
> +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.
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