From patchwork Wed Apr 13 01:55:14 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Steve Rae X-Patchwork-Id: 609843 X-Patchwork-Delegate: l.majewski@samsung.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from theia.denx.de (theia.denx.de [85.214.87.163]) by ozlabs.org (Postfix) with ESMTP id 3ql6ML1gh0z9sBX for ; Wed, 13 Apr 2016 11:55:26 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=broadcom.com header.i=@broadcom.com header.b=Sc0eViM6; dkim-atps=neutral Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 3C474A75D7; Wed, 13 Apr 2016 03:55:24 +0200 (CEST) Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id k44PbyyexUjm; Wed, 13 Apr 2016 03:55:23 +0200 (CEST) Received: from theia.denx.de (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 9A857A7514; Wed, 13 Apr 2016 03:55:23 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id F0244A7514 for ; Wed, 13 Apr 2016 03:55:19 +0200 (CEST) Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id hoTz05f3Yr5X for ; Wed, 13 Apr 2016 03:55:19 +0200 (CEST) X-policyd-weight: NOT_IN_SBL_XBL_SPAMHAUS=-1.5 NOT_IN_SPAMCOP=-1.5 NOT_IN_BL_NJABL=-1.5 (only DNSBL check requested) Received: from mail-lf0-f43.google.com (mail-lf0-f43.google.com [209.85.215.43]) by theia.denx.de (Postfix) with ESMTPS id 9489DA748A for ; Wed, 13 Apr 2016 03:55:15 +0200 (CEST) Received: by mail-lf0-f43.google.com with SMTP id g184so49798173lfb.3 for ; Tue, 12 Apr 2016 18:55:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-transfer-encoding; bh=sHw5rPkOKHJAk+cGEdZ+MtKgax1gKMdeWFl5Fakn6KU=; b=Sc0eViM6Q+qRykTtaOAnjagc3FJPGgCEMOEkS9IRZSpwBMXLfAyFlXRWGLuyh/Si3y +JeDEXoIYdF+qwevfri4tqhS09iqlSesaUG5uG3qwvRp/tOLzFyWYDhwev5ObAyU09xJ FwwTEF+bAgSCvEk2qdGJNU1fKUTbCmmJ1JoF4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-transfer-encoding; bh=sHw5rPkOKHJAk+cGEdZ+MtKgax1gKMdeWFl5Fakn6KU=; b=KSgaRAKiEF8L/WnXUq4E2TvHmjBd6f+yy/c5QuE7tZ8/31Vjwlr4JTMKh/ErBRs647 TfNjf+X1EF2XG1/Fj5aMnqxiUfjFSYQJKx74nnB18/wC6U5GDH96dj9tUDvbQhfJsZpm By6gKJDlE43IrO81ZyUlBcBn69f481teDv+cAKROUd63DA6rkBcIMdepxZyhxacMfx3T Jdxo4/h0HQMcpWtAKNsgxENCMXUJ5EKDHLfe5llMk1GQ4wfpX6KJbQZZUAnmQrnULQtT +rp8V2JUZnATZZe8qeF7tCHUcX/NnNNDVBcWaUNo4DD68XxzC/ymTDUUkbodXMIOTDCn csYw== X-Gm-Message-State: AOPr4FV+fJby5+WRSCdNYJDWB7HSQBOti8a7FWtM7KIQQWAOdjIqJWi28A9UGa/+2tRtYuYJIicWq3jdvRX4Bhgt MIME-Version: 1.0 X-Received: by 10.25.73.212 with SMTP id w203mr2459087lfa.22.1460512514916; Tue, 12 Apr 2016 18:55:14 -0700 (PDT) Received: by 10.25.214.81 with HTTP; Tue, 12 Apr 2016 18:55:14 -0700 (PDT) In-Reply-To: <570CFD42.5060009@ti.com> References: <1459881109-25259-1-git-send-email-srae@broadcom.com> <570436F8.5040507@denx.de> <5704EC8C.3000500@denx.de> <20160407093623.02b2ef6a@amdc2363> <5706A9AB.6000302@denx.de> <570B8BE0.6080506@ti.com> <20160411152228.GR13577@bill-the-cat> <20160412131948.3b280c95@amdc2363> <570CECA9.2020302@ti.com> <20160412153732.7cac01d1@amdc2363> <570CFD42.5060009@ti.com> Date: Tue, 12 Apr 2016 18:55:14 -0700 Message-ID: From: Steve Rae To: Roger Quadros Cc: Marek Vasut , Tom Rini , Steve Rae , U-Boot Mailing List Subject: Re: [U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.15 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" On Tue, Apr 12, 2016 at 6:50 AM, Roger Quadros 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 >>>>>>>> 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 >>>>>>>>>> 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 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: 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 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