On Thu, Apr 7, 2016 at 4:11 PM, Sam Protsenko <semen.protse...@linaro.org> wrote:
> On Fri, Apr 8, 2016 at 12:39 AM, Steve Rae <steve....@broadcom.com> wrote: > > > > > > On Thu, Apr 7, 2016 at 2:16 PM, Sam Protsenko < > semen.protse...@linaro.org> > > wrote: > >> > >> On Thu, Apr 7, 2016 at 8:07 PM, Steve Rae <steve....@broadcom.com> > wrote: > >> > Hi Sam, > >> > > >> > On Thu, Apr 7, 2016 at 9:46 AM, Sam Protsenko > >> > <semen.protse...@linaro.org> > >> > wrote: > >> >> > >> >> On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski > >> >> <l.majew...@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.ka...@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.protse...@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. > >> > > >> > > >> > I'm confused... > >> > You are saying that it is OK to checkin a change that fixes TI boards > >> > (Feb > >> > 25), even though it breaks Broadcom boards; > >> > but if _this_ change "breaks anything" then it is NOT OK ????? > >> > ( I politely disagree.... ) > >> > PS - therefore - what is the right way? (..."this is I believe not > right > >> > way > >> > to do things"...) > >> > > >> > >> Look, it's current state of things. Some stuff is broken, I admit > >> that. But you can't just break something while fixing another stuff. > >> It's not even about "your" boards or "my" boards. It's just not right, > >> I thought it's pretty obvious. So what is correct way to do in that > >> case? I believe it's fix only boards you know for sure are broken, but > >> keep old fastboot behaviour for the rest of boards. Not only TI, but > >> all boards except yours. So that after buildman run you can see that > >> only your boards were changed, something like that. > > > > > > I cannot agree with the assumptions that you are making -- their is no > > evidence that "all boards except <mine>" where broken prior to the Feb 25 > > patch.... > > > >> > >> >> > >> >> 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. > >> > > >> > > >> > So -- here is why I cannot complete this task: > >> > I have absolutely no idea which boards actually _require_ this > >> > capability, > >> > therefore, I have no idea which defconfigs I would need to update! > >> > > >> > >> As I see it: > >> > >> - look into include/configs/*.h > >> - find all headers that use fastboot capability > >> - find corresponding TARGET_ for each header > >> - find all defconfigs for each TARGET_ > >> - your defconfigs should disable alignment > >> - rest of defconfigs should enable alignment (default behavior) > > > > > > Why should the default behavior be "align with wMaxPacketSize"? > > It's not the point. The default behavior can be any of those two, I > basically meant "previous" or "another" behavior. > > > I would argue that the default behavior should be "align with > > "ep->maxpacket" size"! > > For the sake of argument, we don't know for sure which behavior is > correct, because it's not documented. > > > U-Boot history shows that "align with "ep->maxpacket" size" was the > original > > code; > > then a patch was added to change it to "align with wMaxPacketSize" -- > > however, there is NO EXPLANATION given, other than the commit message: > > > > fastboot: OUT transaction length must be aligned to wMaxPacketSize > > > > OUT transactions must be aligned to wMaxPacketSize for each transfer, > > or else transfer will not complete successfully. This patch modifies > > rx_bytes_expected to return a transfer length that is aligned to > > wMaxPacketSize. > > > > Note that the value of wMaxPacketSize and ep->maxpacket may not be > > the same value, and it is the value of wMaxPacketSize that should be > > used for alignment. wMaxPacketSize is passed depending on the speed > of > > connection. > > > > The only actual documentation for fastboot protocol I found is [1], > and I don't see any mention of alignment there at all. So it wouldn't > surprise me if that patch was done just out of of empiric > observations. Which doesn't make it any less significant. > > Please review [2], as it can be related. Especially this part of commit > message: > > USB 2.0 specification, section 8.5.3.2 says: > > " 8.5.3.2 Variable-length Data Stage A control pipe may have a > variable-length data phase in which the host requests more data than > is contained in the specified data structure. When all of the data > structure is returned to the host, the function should indicate that > the Data stage is ended by returning a packet that is shorter than the > MaxPacketSize for the pipe. If the data structure is an exact multiple > of wMaxPacketSize for the pipe, the function will return a zero-length > packet to indicate the end of the Data stage. " > > I'm too sleepy right now to interpret information and make any > conclusions, but maybe we should check how the end of Data Stage is > implemented for different UDC controllers in U-Boot. TI boards use > DWC3 controller. Which is yours? > DWC2 > > [1] > https://android.googlesource.com/platform/system/core/+/master/fastboot/fastboot_protocol.txt > [2] > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=36f84ffb45c75ff10442a9f8f2fd91dcf6c6f5dd > > > > >> > >> This way you fix your boards (that you know need to be fixed) but keep > >> rest of boards intact. If some other boards need to be fixed too -- > >> they will be fixed further by some folks who need that. > >> > >> > So, if you would send me a list of which defconfig files need to add > >> > this > >> > line, I'll update it.... > >> > OR (I would prefer) you could submit a v3 which includes the boards > that > >> > you > >> > know require this capability! > >> > > >> > >> I'm not gonna send this patch for you, sorry. I just don't need that, > >> and I'm not the author of original patch, so it's just not my concern. > >> I can't even test it for boards that actually broken. > >> > >> > Let me know, > >> > Thanks, Steve > >> > > >> >> > >> >> 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). > >> > > >> > > >> > yup -- I use buildman almost exclusively.... > >> > > >> >> > >> >> 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'm confused (again) -- why are you asking: "you guys to not break > >> > anything > >> > else"... > >> > IT IS ALREADY BROKEN, it is broken right now, and has been broken > since > >> > Feb > >> > 25 ! > >> > > >> > Please fix this! > >> > > >> > >> ...So let's fix half of platforms and break the other half of > >> platforms altogether? It's not for me to decide, I'm not the > >> maintainer. But it just doesn't feel right to me. > >> > >> I understand your concern, and I can help you test your patches on my > >> boards any time and also run some debug patches to see the difference. > >> But I can't fix it for you. Also I'm not sure that your patch would be > >> merged in current shape (it's basically a hack). So if I were you I'd > >> try to figure out the root cause of this issue by comparing results of > >> some debug patches and tests, by running them on your boards (where > >> fastboot is broken) and on some boards where fastboot is working. > >> Maybe running wireshark in both cases can help to understand why it's > >> happening. From my POV it was a good assumption (made by someone > >> earlier) that possible reason is different UDC controllers (I have > >> DWC3 on my TI boards). > >> > >> >> > >> >> >> Thanks, Steve > >> >> >> > >> >> >> On Wed, Apr 6, 2016 at 4:01 AM, Marek Vasut <ma...@denx.de> > wrote: > >> >> >> > On 04/06/2016 07:35 AM, Steve Rae wrote: > >> >> >> >> > >> >> >> >> On Apr 5, 2016 3:07 PM, "Marek Vasut" <ma...@denx.de > >> >> >> >> <mailto:ma...@denx.de>> wrote: > >> >> >> >>> > >> >> >> >>> On 04/05/2016 08:31 PM, Steve Rae wrote: > >> >> >> >>> > commit 9e4b510 fastboot: OUT transaction length must be > >> >> >> >>> > aligned > >> >> >> >>> > to > >> >> >> >> wMaxPacketSize > >> >> >> >>> > breaks some boards... > >> >> >> >>> > > >> >> >> >>> > Therefore add a conditional Kconfig to optionally enable > this > >> >> >> >>> > feature. > >> >> >> >>> > >> >> >> >>> Did you drill into it to figure out why this is needed ? > >> >> >> >>> > >> >> >> >> > >> >> >> >> Marek, > >> >> >> >> Let me clarify.... > >> >> >> >> All my boards work with the original code (before the commit > >> >> >> >> which > >> >> >> >> aligned the size to the wMaxPacketSize).... Since that commit, > >> >> >> >> all my boards are broken. > >> >> >> >> And you will notice in this patch, that none of my boards > define > >> >> >> >> this CONFIG_ ... > >> >> >> >> > >> >> >> >> So I think you are asking the wrong person to drill down into > >> >> >> >> this > >> >> >> >> issue.... Sorry, Steve > >> >> >> > > >> >> >> > Well who else can I ask ? You're our only hope at fixing this > >> >> >> > proper. > >> >> >> > > >> >> >> > Anyway, see my other reply, maybe we should just add an arg to > >> >> >> > fastboot command to select one more of operation or the other > and > >> >> >> > default to the one which works. > >> >> >> > > >> >> >> > -- > >> >> >> > Best regards, > >> >> >> > Marek Vasut > >> >> >> > >> >> > > >> >> > > >> >> > > >> >> > -- > >> >> > Best regards, > >> >> > > >> >> > Lukasz Majewski > >> >> > > >> >> > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group > >> > > >> > > > > > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot