| Message ID | CAM7GXo=H2od_KE0NAQ7yXpbAj4PTvsVwjnG7O_k5PNAfdL4ccA@mail.gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Łukasz Majewski |
| Headers | show |
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=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
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=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 >
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