On Thu, Apr 14, 2016 at 4:15 AM, Roger Quadros rogerq@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@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@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=0...
> > 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 = 1024This 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 = 488Why 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 = 4096Does 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