On Thu, Apr 7, 2016 at 4:11 PM, Sam Protsenko semen.protsenko@linaro.org wrote:
On Fri, Apr 8, 2016 at 12:39 AM, Steve Rae steve.rae@broadcom.com wrote:
On Thu, Apr 7, 2016 at 2:16 PM, Sam Protsenko <
semen.protsenko@linaro.org>
wrote:
On Thu, Apr 7, 2016 at 8:07 PM, Steve Rae steve.rae@broadcom.com
wrote:
Hi Sam,
On Thu, Apr 7, 2016 at 9:46 AM, Sam Protsenko semen.protsenko@linaro.org 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.
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 speedof
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/fast... [2] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3...
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 marex@denx.de
wrote:
> > On 04/06/2016 07:35 AM, Steve Rae wrote: > >> > >> On Apr 5, 2016 3:07 PM, "Marek Vasut" <marex@denx.de > >> mailto:marex@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