fastboot: OUT transaction length must be aligned to wMaxPacketSize

commit 9e4b510 fastboot: OUT transaction length must be aligned to wMaxPacketSize breaks some boards...

Therefore add a conditional Kconfig to optionally enable this feature.

Signed-off-by Steve Rae <[email protected]>

---

Changes in v2:
- ammendment to the original patch

 drivers/usb/gadget/Kconfig      | 7 +++++++
 drivers/usb/gadget/f_fastboot.c | 4 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

drivers/usb/gadget/Kconfig +7 -0

index f4698f4..ab1c605 100644
@@ -33,3 +33,10 @@ menuconfig USB_GADGET
 	   a USB peripheral device.  Configure one hardware driver for your
 	   peripheral/device side bus controller, and a "gadget driver" for
 	   your peripheral protocol.
+
+config USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED
+	bool "fastboot download requires alignment with wMaxPacketSize"
+	help
+	   By default, the fastboot download OUT transactions are aligned
+	   to "ep->maxpacket". This option causes the fastboot download OUT
+	   transactions to be aligned with "wMaxPacketSize".

drivers/usb/gadget/f_fastboot.c +3 -1

index 2e87fee..130b5d0 100644
@@ -430,17 +430,19 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req)
 static unsigned int rx_bytes_expected(unsigned int maxpacket)
 {
 	int rx_remain = download_size - download_bytes;
-	int rem = 0;
+	int __maybe_unused rem = 0;
 	if (rx_remain < 0)
 		return 0;
 	if (rx_remain > EP_BUFFER_SIZE)
 		return EP_BUFFER_SIZE;
+#ifdef CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED
 	if (rx_remain < maxpacket) {
 		rx_remain = maxpacket;
 	} else if (rx_remain % maxpacket != 0) {
 		rem = rx_remain % maxpacket;
 		rx_remain = rx_remain + (maxpacket - rem);
 	}
+#endif
 	return rx_remain;
 }
 

Quoted 1 line · Steve Rae

commit 9e4b510 fastboot: OUT transaction length must be aligned to

Quoted 3 lines

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 ?

Quoted 10 lines

Signed-off-by Steve Rae <[email protected]>

---

Changes in v2:
- ammendment to the original patch

 drivers/usb/gadget/Kconfig      | 7 +++++++
 drivers/usb/gadget/f_fastboot.c | 4 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

drivers/usb/gadget/Kconfig menuconfig USB_GADGET · line 33

Quoted 6 lines

drivers/usb/gadget/Kconfig +0 -0

index f4698f4..ab1c605 100644
@@ -33,3 +33,10 @@ menuconfig USB_GADGET
         a USB peripheral device.  Configure one hardware driver for

drivers/usb/gadget/Kconfig menuconfig USB_GADGET · line 34

Quoted 1 line

drivers/usb/gadget/Kconfig +0 -0

         peripheral/device side bus controller, and a "gadget driver"

drivers/usb/gadget/Kconfig menuconfig USB_GADGET

Quoted 7 lines

drivers/usb/gadget/Kconfig +6 -0

         your peripheral protocol.
+
+config USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED
+     bool "fastboot download requires alignment with wMaxPacketSize"
+     help
+        By default, the fastboot download OUT transactions are aligned
+        to "ep->maxpacket". This option causes the fastboot download

Review

Quoted 2 lines

drivers/usb/gadget/Kconfig +1 -0

+        transactions to be aligned with "wMaxPacketSize".

(quoted diff) +0 -0

drivers/usb/gadget/f_fastboot.c static void cb_getvar(struct usb_ep *ep, struct

Quoted 4 lines

index 2e87fee..130b5d0 100644

drivers/usb/gadget/f_fastboot.c +0 -0

@@ -430,17 +430,19 @@ static void cb_getvar(struct usb_ep *ep, struct

Quoted 11 lines

drivers/usb/gadget/f_fastboot.c +2 -1

 static unsigned int rx_bytes_expected(unsigned int maxpacket)
 {
      int rx_remain = download_size - download_bytes;
-     int rem = 0;
+     int __maybe_unused rem = 0;
      if (rx_remain < 0)
              return 0;
      if (rx_remain > EP_BUFFER_SIZE)
              return EP_BUFFER_SIZE;
+#ifdef CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED

drivers/usb/gadget/f_fastboot.c static void cb_getvar(struct usb_ep *ep, struct usb_request *req) · line 448

Quoted 10 lines

drivers/usb/gadget/f_fastboot.c +1 -0

      if (rx_remain < maxpacket) {
              rx_remain = maxpacket;
      } else if (rx_remain % maxpacket != 0) {
              rem = rx_remain % maxpacket;
              rx_remain = rx_remain + (maxpacket - rem);
      }
+#endif
      return rx_remain;
 }
 

--
Best regards,
Marek Vasut

Quoted 3 lines · Steve Rae

Quoted 1 line

commit 9e4b510 fastboot: OUT transaction length must be aligned to

wMaxPacketSize

Quoted 7 lines

Quoted 4 lines

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

Quoted 10 lines

Quoted 10 lines

Signed-off-by Steve Rae <[email protected]>

---

Changes in v2:
- ammendment to the original patch

 drivers/usb/gadget/Kconfig      | 7 +++++++
 drivers/usb/gadget/f_fastboot.c | 4 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

Quoted 7 lines

Quoted 7 lines

(quoted diff) +0 -0

b/drivers/usb/gadget/Kconfig index f4698f4..ab1c605 100644

drivers/usb/gadget/Kconfig +0 -0

@@ -33,3 +33,10 @@ menuconfig USB_GADGET
         a USB peripheral device.  Configure one hardware driver

for

your

Quoted 2 lines

Quoted 2 lines

peripheral/device side bus controller, and a "gadget
driver"

for

Quoted 10 lines

Quoted 10 lines

your peripheral protocol.

drivers/usb/gadget/Kconfig +3 -0

+
+config USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED
+     bool "fastboot download requires alignment with

wMaxPacketSize"

drivers/usb/gadget/Kconfig +2 -0

+     help
+        By default, the fastboot download OUT transactions are

aligned
+ to "ep->maxpacket". This option causes the fastboot
download

OUT

Quoted 2 lines

Review

Quoted 2 lines

drivers/usb/gadget/Kconfig +1 -0

+        transactions to be aligned with "wMaxPacketSize".

(quoted diff) +0 -0

b/drivers/usb/gadget/f_fastboot.c

Quoted 5 lines

Quoted 5 lines

index 2e87fee..130b5d0 100644

drivers/usb/gadget/f_fastboot.c +0 -0

@@ -430,17 +430,19 @@ static void cb_getvar(struct usb_ep *ep,

struct

usb_request *req)

Quoted 26 lines

Quoted 11 lines

static unsigned int rx_bytes_expected(unsigned int maxpacket)

 {
      int rx_remain = download_size - download_bytes;

drivers/usb/gadget/f_fastboot.c +1 -1

-     int rem = 0;
+     int __maybe_unused rem = 0;
      if (rx_remain < 0)
              return 0;
      if (rx_remain > EP_BUFFER_SIZE)
              return EP_BUFFER_SIZE;

drivers/usb/gadget/f_fastboot.c static void cb_getvar(struct usb_ep *ep, struct usb_request *req) · line 447

Quoted 10 lines

drivers/usb/gadget/f_fastboot.c +2 -0

+#ifdef CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED
      if (rx_remain < maxpacket) {
              rx_remain = maxpacket;
      } else if (rx_remain % maxpacket != 0) {
              rem = rx_remain % maxpacket;
              rx_remain = rx_remain + (maxpacket - rem);
      }
+#endif
      return rx_remain;
 }

--
Best regards,
Marek Vasut

Hi Steve, Marek, Sam

Quoted 13 lines · Marek Vasut

Quoted 3 lines · Steve Rae

Quoted 1 line

commit 9e4b510 fastboot: OUT transaction length must be aligned to

wMaxPacketSize

Quoted 7 lines

Quoted 4 lines

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 ?

Quoted 7 lines

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_ ...

Guys, please correct me if I'm wrong, but the problem _is_ be caused by supporting different fastboot protocol versions.

Unfortunately, there is no way to specify "version" in the protocol so different versions of fastboot application handle transmission differently.

I think that Sam and Steve were trying to test the code by using their fastboot apps, but without any conclusion. Am I right there? Has something changed?

I'm OK with Kconfig flag approach, if we don't have any reliable way to distinct protocol versions.

One fastboot user my try it with this option enabled or disabled.

What I would love to see is a proper entry into ./doc READMEs to clarify this issue (not necessarily with this patch series). It would help other users in the future.

Quoted 3 lines · On Apr 5, 2016 3:07 PM, "Marek Vasut" <[email protected] <mailto:[email protected]>>…

Quoted 1 line · Steve Rae

commit 9e4b510 fastboot: OUT transaction length must be aligned to

wMaxPacketSize

Quoted 6 lines

Quoted 3 lines

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

Quoted 22 lines · Steve Rae

Quoted 3 lines · On Apr 5, 2016 3:07 PM, "Marek Vasut" <[email protected] <mailto:[email protected]>>…

Quoted 1 line · Steve Rae

commit 9e4b510 fastboot: OUT transaction length must be aligned to

wMaxPacketSize

Quoted 6 lines

Quoted 3 lines

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_ ...

Quoted 2 lines

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

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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 <[email protected]> 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? )
( 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? 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....

Thanks, Steve

Quoted 35 lines · Marek Vasut

Quoted 22 lines · Steve Rae

Quoted 3 lines · On Apr 5, 2016 3:07 PM, "Marek Vasut" <[email protected] <mailto:[email protected]>>…

Quoted 1 line · Steve Rae

commit 9e4b510 fastboot: OUT transaction length must be aligned to

wMaxPacketSize

Quoted 6 lines

Quoted 3 lines

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_ ...

Quoted 2 lines

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

Quoted 5 lines · Steve Rae

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.

OK

Quoted 6 lines

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...

TI platforms use musb USB/OTG controller, could the issue them be specific to MUSB ?

Quoted 28 lines

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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 <[email protected]> has stated that this Feb 25 change is required to make "fastboot work on TI platforms".

Quoted 4 lines

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.

Quoted 8 lines

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? )
( 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! )

Quoted 6 lines

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)....

OK, so, either way this is broken for some platforms and noone is interested enough to cooperate and fix this properly, yes ?

Quoted 5 lines

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

Quoted 1 line

Please advise!

Definitely not, I will not have a new macro added just to paper over some problem which noone bothered to research and fix properly, sorry.

Quoted 9 lines

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? 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....

I will pass this question onto Tom ;-)

Quoted 7 lines

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....

Kick the TI person into the backside until he comes up with a proper solution. Be annoying. Or, if that leads nowhere, I will just apply the revert and break it for TI and expect them to fix it proper.

btw. note that ELC is going on this week, so replies might be delayed.

Quoted 14 lines

Thanks, Steve

Quoted 11 lines · Marek Vasut

Quoted 10 lines · Steve Rae

Quoted 3 lines · On Apr 5, 2016 3:07 PM, "Marek Vasut" <[email protected] <mailto:[email protected]>>…

Quoted 1 line · Steve Rae

commit 9e4b510 fastboot: OUT transaction length must be aligned to

wMaxPacketSize

Quoted 3 lines

Quoted 3 lines

breaks some boards...

Therefore add a conditional Kconfig to optionally enable this

feature.

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 3 lines

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_ ...

Quoted 1 line

Quoted 1 line

Quoted 1 line

So I think you are asking the wrong person to drill down into this

issue....

Quoted 11 lines

Quoted 11 lines

Quoted 1 line

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,
Marek Vasut

Thanks for the reply, Marek....

On Wed, Apr 6, 2016 at 12:53 PM, Marek Vasut <[email protected] <mailto:[email protected]>> wrote:

On 04/06/2016 07:18 PM, Steve Rae wrote:
> 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.

OK

> 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...

TI platforms use musb USB/OTG controller, could the issue them be specific to MUSB ?

maybe -- I do not know....

Quoted 11 lines

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 <[email protected]

<mailto:[email protected]>>
> 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 <[email protected] <mailto:[email protected]>> 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? )
> ( 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! )

USB experts (Lukasz?): any ideas here....

Quoted 6 lines

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)....

OK, so, either way this is broken for some platforms and noone is interested enough to cooperate and fix this properly, yes ?

yes -- that is my impression .....

Quoted 5 lines

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!

Definitely not, I will not have a new macro added just to paper over some problem which noone bothered to research and fix properly, sorry.

OK -- I am fine with that....

Quoted 9 lines

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? 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....

I will pass this question onto Tom ;-)

Tom -- thanks in advance!

Quoted 6 lines

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....

Kick the TI person into the backside until he comes up with a proper solution. Be annoying. Or, if that leads nowhere, I will just apply the revert and break it for TI and expect them to fix it proper.

btw. note that ELC is going on this week, so replies might be delayed.

OK -- I am happy to be patient for a while.... And that is also why I submitted the request to "revert" this change -- that email thread actually did spark a bit of a conversation; but then it seemed to die without any real resolution.....

Quoted 22 lines

Thanks for the reply, Marek....

On Wed, Apr 6, 2016 at 12:53 PM, Marek Vasut <[email protected] <mailto:[email protected]>> wrote:

On 04/06/2016 07:18 PM, Steve Rae wrote:
> 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.

OK

> 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...

Quoted 5 lines

TI platforms use musb USB/OTG controller, could the issue them
be specific to MUSB ?

maybe -- I do not know....

Anyone with MUSB in Gadget mode who can test this ? I think some sunxi had MUSB.

Quoted 46 lines

Quoted 4 lines

HISTORY:

The U-Boot code, up to Feb 25, worked properly on my Broadcom boards -- this code contains:

Quoted 8 lines

               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
<[email protected]

<mailto:[email protected]>>
> 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 <[email protected] <mailto:[email protected]>> 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? )
> ( 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! )

Quoted 1 line

USB experts (Lukasz?): any ideas here....

I think Lukasz only uses UMS and Thor.

Quoted 8 lines

Quoted 7 lines

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)....

Quoted 5 lines

OK, so, either way this is broken for some platforms and noone
is interested enough to cooperate and fix this properly, yes ?

yes -- that is my impression .....

Bad.

Quoted 46 lines

Quoted 6 lines

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!

Definitely not, I will not have a new macro added just to paper
over some problem which noone bothered to research and fix properly, sorry.

OK -- I am fine with that....

Quoted 10 lines

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? 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....

I will pass this question onto Tom ;-)

Tom -- thanks in advance!

Quoted 7 lines

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....

Kick the TI person into the backside until he comes up with a
proper solution. Be annoying. Or, if that leads nowhere, I will just apply the revert and break it for TI and expect them to fix it proper.

Quoted 8 lines

btw. note that ELC is going on this week, so replies might be
delayed.

OK -- I am happy to be patient for a while.... And that is also why I submitted the request to "revert" this change -- that email thread actually did spark a bit of a conversation; but then it seemed to die without any real resolution.....

I was not paying much attention to it as it's a gadget stuff and I am not tracking gadget stuff that much. I will dive into it later.

Best regards,
Marek Vasut

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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 <[email protected]> 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? )

( 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?

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....

Thanks, Steve

Quoted 38 lines · Marek Vasut

Quoted 24 lines · Steve Rae

Quoted 4 lines · On Apr 5, 2016 3:07 PM, "Marek Vasut" <[email protected] <mailto:[email protected]>>…

Quoted 2 lines · Steve Rae

commit 9e4b510 fastboot: OUT transaction length must be aligned to

wMaxPacketSize

Quoted 7 lines

Quoted 4 lines

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_ ...

Quoted 2 lines

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

Hi Steve,

Quoted 41 lines

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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 <[email protected]> has stated that this Feb 25 change is required to make "fastboot work on TI platforms".

Quoted 9 lines

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.

Quoted 17 lines

( 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!

Quoted 4 lines

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.

Quoted 7 lines

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....

Quoted 6 lines

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.

Quoted 42 lines

Thanks, Steve

Quoted 38 lines · Marek Vasut

Quoted 24 lines · Steve Rae

Quoted 4 lines · On Apr 5, 2016 3:07 PM, "Marek Vasut" <[email protected] <mailto:[email protected]>>…

Quoted 2 lines · Steve Rae

commit 9e4b510 fastboot: OUT transaction length must be aligned to

wMaxPacketSize

Quoted 7 lines

Quoted 4 lines

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_ ...

Quoted 2 lines

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

Quoted 58 lines · Lukasz Majewski

Hi Steve,

Quoted 41 lines

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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 <[email protected]> has stated that this Feb 25 change is required to make "fastboot work on TI platforms".

Quoted 9 lines

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.

Quoted 22 lines

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.

Quoted 15 lines

( 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

Quoted 26 lines

Quoted 6 lines

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.

Quoted 7 lines

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....

Quoted 6 lines

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.

Quoted 2 lines

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).

Quoted 51 lines

Quoted 42 lines

Thanks, Steve

Quoted 38 lines · Marek Vasut

Quoted 24 lines · Steve Rae

Quoted 4 lines · On Apr 5, 2016 3:07 PM, "Marek Vasut" <[email protected] <mailto:[email protected]>>…

Quoted 2 lines · Steve Rae

commit 9e4b510 fastboot: OUT transaction length must be aligned to

wMaxPacketSize

Quoted 7 lines

Quoted 4 lines

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_ ...

Quoted 2 lines

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

Hi Sam,

Quoted 110 lines · Sam Protsenko

Quoted 107 lines · Lukasz Majewski

Hi Steve,

Quoted 41 lines

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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 <[email protected]> has stated that this Feb 25 change is required to make "fastboot work on TI platforms".

Quoted 9 lines

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.

Quoted 15 lines

( 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

Quoted 7 lines

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.

Quoted 7 lines

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....

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

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"...)

Quoted 5 lines

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!

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!

Let me know,
Thanks, Steve

Quoted 10 lines

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....

Quoted 6 lines

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!

Quoted 52 lines

Quoted 52 lines

Quoted 43 lines

Thanks, Steve

Quoted 38 lines · Marek Vasut

Quoted 24 lines · Steve Rae

Quoted 4 lines · On Apr 5, 2016 3:07 PM, "Marek Vasut" <[email protected] <mailto:[email protected]>>…

Quoted 2 lines · Steve Rae

commit 9e4b510 fastboot: OUT transaction length must be aligned to

wMaxPacketSize

Quoted 7 lines

Quoted 4 lines

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_ ...

Quoted 2 lines

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

Quoted 3 lines · Steve Rae

Hi Sam,

On Thu, Apr 7, 2016 at 9:46 AM, Sam Protsenko <

semen.protsenko at linaro.org>

Quoted 3 lines

Quoted 2 lines · wrote:

On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski <

l.majewski at samsung.com>

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines · wrote:

Hi Steve,

Quoted 1 line

No -- I do not believe that this issue is caused by different

fastboot

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

(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

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

has stated they have an issue either way -- but I don't think many

use

Quoted 24 lines

Quoted 24 lines

Quoted 24 lines

Quoted 24 lines

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       if (rx_remain < maxpacket) {
+               rx_remain = maxpacket;
+       } else if (rx_remain % maxpacket != 0) {
+               rem = rx_remain % maxpacket;
+               rx_remain = rx_remain + (maxpacket - rem);
+       }

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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 <[email protected]> has stated that this Feb

25

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

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? )

Quoted 1 line

Quoted 1 line

Quoted 1 line

I can only speak about DWC2 (from Synopsis) embedded at Samsung

boards.

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

( 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

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

busy right now -- so I submitted this patch on his behalf (although

he

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

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

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

working properly with "all" the existing boards?

As we know the author of this change is not working at Linaro anymore.

Quoted 2 lines

I'm getting the
impression that "because the current code works for me", that I am

not

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

getting any assistance in resolving this issue -- which is why I suggested "reverting" this change back to the original code; that

way,

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

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

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 3 lines

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

Quoted 4 lines

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

Quoted 2 lines

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.

Quoted 9 lines

Quoted 5 lines

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,

Quoted 2 lines

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)

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.

Quoted 3 lines

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

Quoted 2 lines

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.

Quoted 18 lines

Let me know,
Thanks, Steve

Quoted 10 lines

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....

Quoted 1 line

Quoted 8 lines

Quoted 5 lines

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

Quoted 2 lines

else"...
IT IS ALREADY BROKEN, it is broken right now, and has been broken since

Feb

Quoted 4 lines

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).

Quoted 54 lines

Quoted 52 lines

Quoted 52 lines

Quoted 43 lines

Thanks, Steve

Quoted 38 lines · Marek Vasut

Quoted 24 lines · Steve Rae

Quoted 4 lines · On Apr 5, 2016 3:07 PM, "Marek Vasut" <[email protected] <mailto:[email protected]>>…

Quoted 2 lines · Steve Rae

commit 9e4b510 fastboot: OUT transaction length must be aligned to

wMaxPacketSize

Quoted 7 lines

Quoted 4 lines

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_ ...

Quoted 2 lines

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

Quoted 152 lines · Sam Protsenko

Quoted 139 lines · Steve Rae

Hi Sam,

Quoted 126 lines · Sam Protsenko

Quoted 122 lines · Lukasz Majewski

Hi Steve,

Quoted 45 lines

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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 <[email protected]> has stated that this Feb 25
change is required to make "fastboot work on TI platforms".

Quoted 10 lines

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.

Quoted 18 lines

( 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

Quoted 8 lines

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.

Quoted 9 lines

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....

Quoted 8 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 10 lines

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"...)

Quoted 8 lines

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....

Quoted 14 lines

Quoted 8 lines

Quoted 6 lines

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.

Quoted 5 lines

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!

Quoted 8 lines

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"?

I would argue that the default behavior should be "align with "ep->maxpacket" size"!

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.

Quoted 126 lines

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.

Quoted 7 lines

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.

Quoted 25 lines

Let me know,
Thanks, Steve

Quoted 10 lines

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....

Quoted 6 lines

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).

Quoted 9 lines

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).

Quoted 57 lines

Quoted 55 lines

Quoted 55 lines

Quoted 46 lines

Thanks, Steve

Quoted 41 lines · Marek Vasut

Quoted 26 lines · Steve Rae

Quoted 5 lines · On Apr 5, 2016 3:07 PM, "Marek Vasut" <[email protected] <mailto:[email protected]>>…

Quoted 3 lines · Steve Rae

commit 9e4b510 fastboot: OUT transaction length must be aligned
to

wMaxPacketSize

Quoted 7 lines

Quoted 4 lines

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_ ...

Quoted 3 lines

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

Quoted 3 lines · Steve Rae

On Thu, Apr 7, 2016 at 2:16 PM, Sam Protsenko <

semen.protsenko at linaro.org>

Quoted 3 lines

Quoted 2 lines · wrote:

On Thu, Apr 7, 2016 at 8:07 PM, Steve Rae <[email protected]>

Quoted 11 lines · wrote:

Quoted 11 lines

Quoted 11 lines

Hi Sam,

Quoted 6 lines · Sam Protsenko

Quoted 2 lines · Lukasz Majewski

Hi Steve,

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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"

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

-- 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...

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

HISTORY:

The U-Boot code, up to Feb 25, worked properly on my Broadcom

boards

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

-- 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

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

"ep->maxpacket" size.

On Feb 25, there was a patch applied from <

dileep.katta at linaro.org>

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

which forces the remaining "rx_bytes_expected" to be aligned to

the

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

"wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

this patch be reverted -- because it broke my boards! (see the

other

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

email thread).

Sam Protsenko <[email protected]> has stated that this

Feb

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

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? )

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

I can only speak about DWC2 (from Synopsis) embedded at Samsung boards.
There are low level debugging registers (documented, but not

supposed

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

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.

Quoted 1 line

( Can anyone explain why "wMaxPacketSize" size would be required?

--

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

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

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

he
still needs to add the Kconfig for the TI platforms in order to

make

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

his boards work)....

I suppose I could also propose a patch where the condition

_removes_

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

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

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 7 lines

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.

Quoted 9 lines

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....

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

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

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 7 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

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

Quoted 14 lines

Quoted 4 lines

Quoted 3 lines

way
to do things"...)

Quoted 8 lines

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.

Quoted 9 lines

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....

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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

Quoted 19 lines

Quoted 9 lines

Quoted 8 lines

Quoted 1 line

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!

Quoted 8 lines

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)

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 1 line

U-Boot history shows that "align with "ep->maxpacket" size" was the

original

Quoted 6 lines

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

Quoted 8 lines

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

Quoted 2 lines

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?

[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

Quoted 10 lines

Quoted 10 lines

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.

Quoted 4 lines

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

Quoted 12 lines

Quoted 12 lines

Quoted 3 lines

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.

Quoted 4 lines

Let me know,
Thanks, Steve

Quoted 1 line

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 1 line

boards were changed).

yup -- I use buildman almost exclusively....

Quoted 6 lines

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).

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 26 lines

Quoted 26 lines

Quoted 5 lines

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).

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Thanks, Steve

On Wed, Apr 6, 2016 at 4:01 AM, Marek Vasut <[email protected]>

Quoted 5 lines · wrote:

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines · Steve Rae

Quoted 4 lines

Quoted 1 line · On Apr 5, 2016 3:07 PM, "Marek Vasut" <[email protected] <mailto:[email protected]>>…

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines · Steve Rae

Quoted 3 lines

commit 9e4b510 fastboot: OUT transaction length must be aligned
to

wMaxPacketSize

Quoted 3 lines

Quoted 3 lines

breaks some boards...

Therefore add a conditional Kconfig to optionally enable

this

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

feature.

Did you drill into it to figure out why this is needed ?

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

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

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 5 lines

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

Quoted 19 lines

Quoted 17 lines

Quoted 17 lines

Quoted 15 lines

Quoted 15 lines

Quoted 6 lines

Quoted 5 lines

default to the one which works.

--
Best regards,
Marek Vasut

--
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

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.

[1] https://android.googlesource.com/platform/system/core/+/master/fastboot/fastboot_protocol.txt

-----Original Message-----
From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Tom Rini
Sent: Saturday, April 09, 2016 1:14 AM
To: Sam Protsenko
Cc: Marek Vasut; Steve Rae; U-Boot Mailing List
Subject: Re: [U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

On Fri, Apr 08, 2016 at 02:11:48AM +0300, Sam Protsenko wrote:
[snip]

Quoted 4 lines

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.

[snip]

Quoted 3 lines

[1]
https://android.googlesource.com/platform/system/core/+/master/fastboo
t/fastboot_protocol.txt

So, the protocol says "Max packet size must be 64 bytes for full-speed,
512 bytes for high-speed and 1024 bytes for Super Speed USB.". What are wMaxPacketSize and ep->maxpacket in these cases, both the TI+DWC3 ones, Broadcom+DWC2 and if someone can grab one, TI+MUSB. And then, what
exactly does everyone failure logs look like, perhaps with some annotations where it fails?

Quoted 103 lines · Lukasz Majewski

Hi Steve,

Quoted 41 lines

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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 <[email protected]> has stated that this Feb 25 change is required to make "fastboot work on TI platforms".

Quoted 9 lines

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.

Quoted 17 lines

( 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!

Quoted 4 lines

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.

Quoted 7 lines

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....

Quoted 6 lines

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....

Quoted 5 lines

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).

Quoted 127 lines · Sam Protsenko

Quoted 103 lines · Lukasz Majewski

Hi Steve,

Quoted 41 lines

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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 <[email protected]> has stated that this Feb 25 change is required to make "fastboot work on TI platforms".

Quoted 9 lines

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.

Quoted 17 lines

( 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!

Quoted 4 lines

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.

Quoted 7 lines

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....

Quoted 6 lines

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....

Quoted 5 lines

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).

Quoted 5 lines

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

Quoted 128 lines · Marek Vasut

Quoted 127 lines · Sam Protsenko

Quoted 103 lines · Lukasz Majewski

Hi Steve,

Quoted 41 lines

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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 <[email protected]> has stated that this Feb 25 change is required to make "fastboot work on TI platforms".

Quoted 9 lines

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.

Quoted 17 lines

( 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!

Quoted 4 lines

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.

Quoted 7 lines

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....

Quoted 6 lines

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....

Quoted 5 lines

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).

Quoted 9 lines

Quoted 5 lines

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

Quoted 155 lines · Marek Vasut

Quoted 146 lines · Sam Protsenko

Quoted 138 lines

Quoted 110 lines · Lukasz Majewski

Hi Steve,

Quoted 43 lines

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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 <[email protected]> has stated that this Feb 25 change is required to make "fastboot work on TI platforms".

Quoted 9 lines

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.

Quoted 18 lines

( 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!

Quoted 5 lines

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.

Quoted 7 lines

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....

Quoted 7 lines

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....

Quoted 6 lines

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).

Quoted 6 lines

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).

Quoted 3 lines

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.

Quoted 2 lines

[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!

Hi Tom, Mugunthan

Quoted 156 lines · Mugunthan V N

Quoted 155 lines · Marek Vasut

Quoted 146 lines · Sam Protsenko

Quoted 138 lines

Quoted 110 lines · Lukasz Majewski

Hi Steve,

Quoted 43 lines

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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 <[email protected]> has stated that this Feb 25 change is required to make "fastboot work on TI platforms".

Quoted 9 lines

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.

Quoted 18 lines

( 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!

Quoted 5 lines

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.

Quoted 7 lines

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....

Quoted 7 lines

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....

Quoted 6 lines

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).

Quoted 6 lines

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).

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines

[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).

Hi,

Quoted 164 lines · Lukasz Majewski

Hi Tom, Mugunthan

Quoted 156 lines · Mugunthan V N

Quoted 155 lines · Marek Vasut

Quoted 146 lines · Sam Protsenko

Quoted 138 lines

Quoted 110 lines · Lukasz Majewski

Hi Steve,

Quoted 43 lines

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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 <[email protected]> has stated that this Feb 25 change is required to make "fastboot work on TI platforms".

Quoted 9 lines

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.

Quoted 18 lines

( 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!

Quoted 5 lines

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.

Quoted 7 lines

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....

Quoted 7 lines

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....

Quoted 6 lines

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).

Quoted 6 lines

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).

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines

[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!

Quoted 6 lines

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.

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.

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.

cheers,
-roger

Hi Roger,

Quoted 183 lines

Hi,

Quoted 164 lines · Lukasz Majewski

Hi Tom, Mugunthan

Quoted 156 lines · Mugunthan V N

Quoted 155 lines · Marek Vasut

Quoted 146 lines · Sam Protsenko

Quoted 138 lines

Quoted 110 lines · Lukasz Majewski

Hi Steve,

Quoted 43 lines

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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 <[email protected]> has stated that this Feb 25 change is required to make "fastboot work on TI platforms".

Quoted 9 lines

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.

Quoted 18 lines

( 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!

Quoted 5 lines

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.

Quoted 7 lines

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....

Quoted 7 lines

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....

Quoted 6 lines

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).

Quoted 6 lines

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).

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines

[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!

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

This is being
done in f_mass_storage.c in set_bulk_out_req_length(). Doing that shouldn't affect other controllers.

Quoted 5 lines

So we need to really fix commit 9e4b510.

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.

Those are now under review :-)

Lukasz,

Quoted 185 lines · Lukasz Majewski

Hi Roger,

Quoted 183 lines

Hi,

Quoted 164 lines · Lukasz Majewski

Hi Tom, Mugunthan

Quoted 156 lines · Mugunthan V N

Quoted 155 lines · Marek Vasut

Quoted 146 lines · Sam Protsenko

Quoted 138 lines

Quoted 110 lines · Lukasz Majewski

Hi Steve,

Quoted 43 lines

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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 <[email protected]> has stated that this Feb 25 change is required to make "fastboot work on TI platforms".

Quoted 9 lines

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.

Quoted 18 lines

( 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!

Quoted 5 lines

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.

Quoted 7 lines

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....

Quoted 7 lines

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....

Quoted 6 lines

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).

Quoted 6 lines

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).

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines

[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!

Quoted 6 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 4 lines

Quoted 4 lines

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.

Quoted 3 lines

Those are now under review :-)

Thanks :)

cheers,
-roger

Quoted 197 lines · Roger Quadros

Lukasz,

Quoted 185 lines · Lukasz Majewski

Hi Roger,

Quoted 183 lines

Hi,

Quoted 164 lines · Lukasz Majewski

Hi Tom, Mugunthan

Quoted 156 lines · Mugunthan V N

Quoted 155 lines · Marek Vasut

Quoted 146 lines · Sam Protsenko

Quoted 138 lines

Quoted 110 lines · Lukasz Majewski

Hi Steve,

Quoted 43 lines

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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 <[email protected]> has stated that this Feb 25 change is required to make "fastboot work on TI platforms".

Quoted 9 lines

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.

Quoted 18 lines

( 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!

Quoted 5 lines

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.

Quoted 7 lines

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....

Quoted 7 lines

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....

Quoted 6 lines

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).

Quoted 6 lines

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).

Quoted 3 lines

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.

Quoted 5 lines

Quoted 2 lines

[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!

Quoted 6 lines

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.

Quoted 6 lines

Quoted 3 lines

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

Quoted 15 lines

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.

Quoted 6 lines

Quoted 6 lines

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:

drivers/usb/gadget/dwc2_udc_otg.c +5 -5

index 5d53440..763c6d9 100644
@@ -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

drivers/usb/gadget/dwc2_udc_otg.c +1 -1

-        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

drivers/usb/gadget/dwc2_udc_otg.c +0 -2

-dwc2_queue: ep_is_in, DWC2_UDC_OTG_GINTSTS=0x14008028
-setdma_tx:EP1 TX DMA start : DIEPDMA0 = 0xffb85fc0,DIEPTSIZ0 =

0x80004, DIEPCTL0 = 0x80498040

drivers/usb/gadget/dwc2_udc_otg.c +1 -1

-        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...

drivers/usb/gadget/dwc2_udc_otg.c +0 -3

-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

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 7 lines

Quoted 3 lines

Those are now under review :-)

Thanks :)

cheers,
-roger

Steve,

Quoted 95 lines · Steve Rae

Quoted 94 lines · Roger Quadros

Lukasz,

Quoted 91 lines · Lukasz Majewski

Hi Roger,

Quoted 89 lines

Hi,

Quoted 86 lines · Lukasz Majewski

Hi Tom, Mugunthan

Quoted 84 lines · Mugunthan V N

Quoted 83 lines · Marek Vasut

Quoted 82 lines · Sam Protsenko

Quoted 81 lines

Quoted 79 lines · Lukasz Majewski

Hi Steve,

Quoted 43 lines

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 <[email protected]> which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:

(quoted diff) +6 -0

+       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 <[email protected]> has stated that this Feb 25 change is required to make "fastboot work on TI platforms".

Quoted 9 lines

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.

Quoted 11 lines

( 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)....

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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

Quoted 63 lines

Quoted 63 lines

Quoted 63 lines

Quoted 63 lines

Quoted 63 lines

Quoted 63 lines

Quoted 63 lines

Quoted 62 lines

Quoted 52 lines

Quoted 26 lines

Quoted 7 lines

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.

Quoted 7 lines

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....

Quoted 7 lines

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....

Quoted 6 lines

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).

Quoted 6 lines

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

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

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

Quoted 27 lines

Quoted 27 lines

Quoted 26 lines

Quoted 16 lines

Quoted 10 lines

Quoted 3 lines

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);

Quoted 7 lines

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.

Quoted 7 lines

Quoted 7 lines

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

Quoted 1 line

Quoted 1 line

? A received zero-length packet still requires a MaxPacketSize buffer.

Therefore, if the expected

Quoted 1 line

Quoted 1 line

amount of data to be received is a multiple of MaxPacketSize, software

should add MaxPacketSize

Quoted 1 line

Quoted 1 line

bytes to the buffer to sink a possible zero-length packet at the end of

the transfer.

Quoted 57 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

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:

drivers/usb/gadget/dwc2_udc_otg.c +5 -5

index 5d53440..763c6d9 100644
@@ -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

Quoted 5 lines

drivers/usb/gadget/dwc2_udc_otg.c +1 -1

-        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.

Quoted 6 lines

*** 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

Quoted 6 lines

drivers/usb/gadget/dwc2_udc_otg.c +0 -2

-dwc2_queue: ep_is_in, DWC2_UDC_OTG_GINTSTS=0x14008028
-setdma_tx:EP1 TX DMA start : DIEPDMA0 = 0xffb85fc0,DIEPTSIZ0 =

0x80004, DIEPCTL0 = 0x80498040

drivers/usb/gadget/dwc2_udc_otg.c +1 -1

-        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?

Quoted 4 lines

+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

Quoted 28 lines

+++++++ hangs here...

drivers/usb/gadget/dwc2_udc_otg.c +0 -3

-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

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 7 lines

Quoted 3 lines

Those are now under review :-)

Thanks :)

cheers,
-roger