[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize
Steve Rae
steve.rae at broadcom.com
Wed Apr 13 03:55:14 CEST 2016
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
*** 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
+setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb85198,DOEPTSIZ =
0x801e8, DOEPCTL = 0x80098200
+ buf = 0xffb85198, pktcnt = 1, xfersize = 488
+++++++ 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