[U-Boot] [PATCH v2] 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 srae@broadcom.com ---
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(-)
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index f4698f4..ab1c605 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -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". diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 2e87fee..130b5d0 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -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; }
On 04/05/2016 08:31 PM, Steve Rae wrote:
commit 9e4b510 fastboot: OUT transaction length must be aligned to wMaxPacketSize breaks some boards...
Therefore add a conditional Kconfig to optionally enable this feature.
Did you drill into it to figure out why this is needed ?
Signed-off-by: Steve Rae srae@broadcom.com
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(-)
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index f4698f4..ab1c605 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -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 alignedto "ep->maxpacket". This option causes the fastboot download OUTtransactions to be aligned with "wMaxPacketSize".diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 2e87fee..130b5d0 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -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; }
On Apr 5, 2016 3:07 PM, "Marek Vasut" marex@denx.de wrote:
On 04/05/2016 08:31 PM, Steve Rae wrote:
commit 9e4b510 fastboot: OUT transaction length must be aligned to
wMaxPacketSize
breaks some boards...
Therefore add a conditional Kconfig to optionally enable this feature.
Did you drill into it to figure out why this is needed ?
Marek, Let me clarify.... All my boards work with the original code (before the commit which aligned the size to the wMaxPacketSize).... Since that commit, all my boards are broken. And you will notice in this patch, that none of my boards define this CONFIG_ ...
So I think you are asking the wrong person to drill down into this issue.... Sorry, Steve
Signed-off-by: Steve Rae srae@broadcom.com
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(-)
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index f4698f4..ab1c605 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -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"helpBy default, the fastboot download OUT transactions are alignedto "ep->maxpacket". This option causes the fastboot download
OUT
transactions to be aligned with "wMaxPacketSize".diff --git a/drivers/usb/gadget/f_fastboot.c
b/drivers/usb/gadget/f_fastboot.c
index 2e87fee..130b5d0 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -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; }
-- Best regards, Marek Vasut
Hi Steve, Marek, Sam
On Apr 5, 2016 3:07 PM, "Marek Vasut" marex@denx.de wrote:
On 04/05/2016 08:31 PM, Steve Rae wrote:
commit 9e4b510 fastboot: OUT transaction length must be aligned to
wMaxPacketSize
breaks some boards...
Therefore add a conditional Kconfig to optionally enable this feature.
Did you drill into it to figure out why this is needed ?
Marek, Let me clarify.... All my boards work with the original code (before the commit which aligned the size to the wMaxPacketSize).... Since that commit, all my boards are broken. And you will notice in this patch, that none of my boards define this CONFIG_ ...
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.
So I think you are asking the wrong person to drill down into this issue.... Sorry, Steve
Signed-off-by: Steve Rae srae@broadcom.com
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(-)
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index f4698f4..ab1c605 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -33,3 +33,10 @@ menuconfig USB_GADGET a USB peripheral device. Configure one hardware driver for
your
peripheral/device side bus controller, and a "gadgetdriver"
for
your peripheral protocol.
+config USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED
bool "fastboot download requires alignment withwMaxPacketSize"
helpBy default, the fastboot download OUT transactions arealigned
to "ep->maxpacket". This option causes the fastbootdownload
OUT
transactions to be aligned with "wMaxPacketSize".diff --git a/drivers/usb/gadget/f_fastboot.c
b/drivers/usb/gadget/f_fastboot.c
index 2e87fee..130b5d0 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -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; }
-- Best regards, Marek Vasut
On 04/06/2016 09:09 AM, Lukasz Majewski wrote:
Hi Steve, Marek, Sam
On Apr 5, 2016 3:07 PM, "Marek Vasut" marex@denx.de wrote:
On 04/05/2016 08:31 PM, Steve Rae wrote:
commit 9e4b510 fastboot: OUT transaction length must be aligned to
wMaxPacketSize
breaks some boards...
Therefore add a conditional Kconfig to optionally enable this feature.
Did you drill into it to figure out why this is needed ?
Marek, Let me clarify.... All my boards work with the original code (before the commit which aligned the size to the wMaxPacketSize).... Since that commit, all my boards are broken. And you will notice in this patch, that none of my boards define this CONFIG_ ...
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.
Hm, if there are two protocol versions and the protocol is such a braindead design that it doesn't send version in it's datagrams, then instead of selecting one protocol version via Kconfig option at compile time, we should add an optional arg for the fastboot command to select which host you're talking to at run-time ?
On 04/06/2016 07:35 AM, Steve Rae wrote:
On Apr 5, 2016 3:07 PM, "Marek Vasut" <marex@denx.de mailto:marex@denx.de> wrote:
On 04/05/2016 08:31 PM, Steve Rae wrote:
commit 9e4b510 fastboot: OUT transaction length must be aligned to
wMaxPacketSize
breaks some boards...
Therefore add a conditional Kconfig to optionally enable this feature.
Did you drill into it to figure out why this is needed ?
Marek, Let me clarify.... All my boards work with the original code (before the commit which aligned the size to the wMaxPacketSize).... Since that commit, all my boards are broken. And you will notice in this patch, that none of my boards define this CONFIG_ ...
So I think you are asking the wrong person to drill down into this issue.... Sorry, Steve
Well who else can I ask ? You're our only hope at fixing this proper.
Anyway, see my other reply, maybe we should just add an arg to fastboot command to select one more of operation or the other and default to the one which works.
No -- I do not believe that this issue is caused by different fastboot (client) versions (the executable that runs on the host computer - Linux, Windows, Mac, etc.) I have personally attempted three (3) different versions, and the results are consistent.
And no I don't think that I "am the only hope at fixing this proper" -- as you will see below, this" issue" seems to be unique to the "TI platforms" (... nobody else has stated they have an issue either way -- but I don't think many use this feature ....) So maybe someone with "TI platforms" could investigate this more thoroughly...
HISTORY:
The U-Boot code, up to Feb 25, worked properly on my Broadcom boards -- this code contains: req->length = rx_bytes_expected(); if (req->length < ep->maxpacket) req->length = ep->maxpacket; which aligned the remaining "rx_bytes_expected" to be aligned to the "ep->maxpacket" size.
On Feb 25, there was a patch applied from dileep.katta@linaro.org which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards: + if (rx_remain < maxpacket) { + rx_remain = maxpacket; + } else if (rx_remain % maxpacket != 0) { + rem = rx_remain % maxpacket; + rx_remain = rx_remain + (maxpacket - rem); + }
After attempting to unsuccessfully contact Dileep, I requested that this patch be reverted -- because it broke my boards! (see the other email thread).
Sam Protsenko semen.protsenko@linaro.org has stated that this Feb 25 change is required to make "fastboot work on TI platforms".
Thus, - Broadcom boards require alignment to "ep->maxpacket" size - TI platforms require alignment to "wMaxPacketSize" size And we seem to be at a stale-mate. Unfortunately, I do not know enough about the USB internals to understand why this change breaks the Broadcom boards; or why it _is_ required on the TI platforms.... ( Is there any debugging that can be turned on to validate what is happening at the lower levels? ) ( 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
On Wed, Apr 6, 2016 at 4:01 AM, Marek Vasut marex@denx.de wrote:
On 04/06/2016 07:35 AM, Steve Rae wrote:
On Apr 5, 2016 3:07 PM, "Marek Vasut" <marex@denx.de mailto:marex@denx.de> wrote:
On 04/05/2016 08:31 PM, Steve Rae wrote:
commit 9e4b510 fastboot: OUT transaction length must be aligned to
wMaxPacketSize
breaks some boards...
Therefore add a conditional Kconfig to optionally enable this feature.
Did you drill into it to figure out why this is needed ?
Marek, Let me clarify.... All my boards work with the original code (before the commit which aligned the size to the wMaxPacketSize).... Since that commit, all my boards are broken. And you will notice in this patch, that none of my boards define this CONFIG_ ...
So I think you are asking the wrong person to drill down into this issue.... Sorry, Steve
Well who else can I ask ? You're our only hope at fixing this proper.
Anyway, see my other reply, maybe we should just add an arg to fastboot command to select one more of operation or the other and default to the one which works.
-- Best regards, Marek Vasut
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 ?
HISTORY:
The U-Boot code, up to Feb 25, worked properly on my Broadcom boards -- this code contains: req->length = rx_bytes_expected(); if (req->length < ep->maxpacket) req->length = ep->maxpacket; which aligned the remaining "rx_bytes_expected" to be aligned to the "ep->maxpacket" size.
On Feb 25, there was a patch applied from dileep.katta@linaro.org which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:
if (rx_remain < maxpacket) {rx_remain = maxpacket;} else if (rx_remain % maxpacket != 0) {rem = rx_remain % maxpacket;rx_remain = rx_remain + (maxpacket - rem);}After attempting to unsuccessfully contact Dileep, I requested that this patch be reverted -- because it broke my boards! (see the other email thread).
Sam Protsenko semen.protsenko@linaro.org has stated that this Feb 25 change is required to make "fastboot work on TI platforms".
Thus,
- Broadcom boards require alignment to "ep->maxpacket" size
- TI platforms require alignment to "wMaxPacketSize" size
And we seem to be at a stale-mate. Unfortunately, I do not know enough about the USB internals to understand why this change breaks the Broadcom boards; or why it _is_ required on the TI platforms.... ( Is there any debugging that can be turned on to validate what is happening at the lower levels? ) ( 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)....
OK, so, either way this is broken for some platforms and noone is interested enough to cooperate and fix this properly, yes ?
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.
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 ;-)
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.
Thanks, Steve
On Wed, Apr 6, 2016 at 4:01 AM, Marek Vasut marex@denx.de wrote:
On 04/06/2016 07:35 AM, Steve Rae wrote:
On Apr 5, 2016 3:07 PM, "Marek Vasut" <marex@denx.de mailto:marex@denx.de> wrote:
On 04/05/2016 08:31 PM, Steve Rae wrote:
commit 9e4b510 fastboot: OUT transaction length must be aligned to
wMaxPacketSize
breaks some boards...
Therefore add a conditional Kconfig to optionally enable this feature.
Did you drill into it to figure out why this is needed ?
Marek, Let me clarify.... All my boards work with the original code (before the commit which aligned the size to the wMaxPacketSize).... Since that commit, all my boards are broken. And you will notice in this patch, that none of my boards define this CONFIG_ ...
So I think you are asking the wrong person to drill down into this issue.... Sorry, Steve
Well who else can I ask ? You're our only hope at fixing this proper.
Anyway, see my other reply, maybe we should just add an arg to fastboot command to select one more of operation or the other and default to the one which works.
-- Best regards, Marek Vasut
Thanks for the reply, Marek....
On Wed, Apr 6, 2016 at 12:53 PM, Marek Vasut marex@denx.de 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....
HISTORY:
The U-Boot code, up to Feb 25, worked properly on my Broadcom boards -- this code contains: req->length = rx_bytes_expected(); if (req->length < ep->maxpacket) req->length = ep->maxpacket; which aligned the remaining "rx_bytes_expected" to be aligned to the "ep->maxpacket" size.
On Feb 25, there was a patch applied from dileep.katta@linaro.org which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:
if (rx_remain < maxpacket) {rx_remain = maxpacket;} else if (rx_remain % maxpacket != 0) {rem = rx_remain % maxpacket;rx_remain = rx_remain + (maxpacket - rem);}After attempting to unsuccessfully contact Dileep, I requested that this patch be reverted -- because it broke my boards! (see the other email thread).
Sam Protsenko semen.protsenko@linaro.org has stated that this Feb 25 change is required to make "fastboot work on TI platforms".
Thus,
- Broadcom boards require alignment to "ep->maxpacket" size
- TI platforms require alignment to "wMaxPacketSize" size
And we seem to be at a stale-mate. Unfortunately, I do not know enough about the USB internals to understand why this change breaks the Broadcom boards; or why it _is_ required on the TI platforms.... ( Is there any debugging that can be turned on to validate what is happening at the lower levels? ) ( 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....
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 .....
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....
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!
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.....
Thanks, Steve
Thanks, Steve
On Wed, Apr 6, 2016 at 4:01 AM, Marek Vasut marex@denx.de wrote:
On 04/06/2016 07:35 AM, Steve Rae wrote:
On Apr 5, 2016 3:07 PM, "Marek Vasut" <marex@denx.de mailto:marex@denx.de> wrote:
On 04/05/2016 08:31 PM, Steve Rae wrote:
commit 9e4b510 fastboot: OUT transaction length must be aligned to
wMaxPacketSize
breaks some boards...
Therefore add a conditional Kconfig to optionally enable this
feature.
Did you drill into it to figure out why this is needed ?
Marek, Let me clarify.... All my boards work with the original code (before the commit which aligned the size to the wMaxPacketSize).... Since that commit, all my boards are broken. And you will notice in this patch, that none of my boards define this CONFIG_ ...
So I think you are asking the wrong person to drill down into this
issue....
Sorry, Steve
Well who else can I ask ? You're our only hope at fixing this proper.
Anyway, see my other reply, maybe we should just add an arg to fastboot command to select one more of operation or the other and default to the one which works.
-- Best regards, Marek Vasut
-- Best regards, Marek Vasut
On 04/06/2016 10:45 PM, Steve Rae wrote:
Thanks for the reply, Marek....
On Wed, Apr 6, 2016 at 12:53 PM, Marek Vasut <marex@denx.de mailto:marex@denx.de> 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....
Anyone with MUSB in Gadget mode who can test this ? I think some sunxi had MUSB.
> HISTORY: > > The U-Boot code, up to Feb 25, worked properly on my Broadcom boards > -- this code contains: > req->length = rx_bytes_expected(); > if (req->length < ep->maxpacket) > req->length = ep->maxpacket; > which aligned the remaining "rx_bytes_expected" to be aligned to the > "ep->maxpacket" size. > > On Feb 25, there was a patch applied from <dileep.katta@linaro.org <mailto:dileep.katta@linaro.org>> > which forces the remaining "rx_bytes_expected" to be aligned to the > "wMaxPacketSize" size -- this patch broke all Broadcom boards: > + if (rx_remain < maxpacket) { > + rx_remain = maxpacket; > + } else if (rx_remain % maxpacket != 0) { > + rem = rx_remain % maxpacket; > + rx_remain = rx_remain + (maxpacket - rem); > + } > > After attempting to unsuccessfully contact Dileep, I requested that > this patch be reverted -- because it broke my boards! (see the other > email thread). > > Sam Protsenko <semen.protsenko@linaro.org <mailto:semen.protsenko@linaro.org>> has stated that this Feb 25 > change is required to make "fastboot work on TI platforms". > > Thus, > - Broadcom boards require alignment to "ep->maxpacket" size > - TI platforms require alignment to "wMaxPacketSize" size > And we seem to be at a stale-mate. > Unfortunately, I do not know enough about the USB internals to > understand why this change breaks the Broadcom boards; or why it _is_ > required on the TI platforms.... > ( Is there any debugging that can be turned on to validate what is > happening at the lower levels? ) > ( 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....
I think Lukasz only uses UMS and Thor.
> > 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 .....
Bad.
> 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....
> 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!
> 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.....
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
Hi Marek,
On 04/06/2016 10:45 PM, Steve Rae wrote:
Thanks for the reply, Marek....
On Wed, Apr 6, 2016 at 12:53 PM, Marek Vasut <marex@denx.de mailto:marex@denx.de> 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 thembe specific to MUSB ?
maybe -- I do not know....
Anyone with MUSB in Gadget mode who can test this ? I think some sunxi had MUSB.
> HISTORY: > > The U-Boot code, up to Feb 25, worked properly on my Broadcom > boards -- this code contains: > req->length = rx_bytes_expected(); > if (req->length < ep->maxpacket) > req->length = ep->maxpacket; > which aligned the remaining "rx_bytes_expected" to be aligned > to the "ep->maxpacket" size. > > On Feb 25, there was a patch applied from > <dileep.katta@linaro.org <mailto:dileep.katta@linaro.org>> > which forces the remaining "rx_bytes_expected" to be aligned > to the "wMaxPacketSize" size -- this patch broke all Broadcom > boards: > + if (rx_remain < maxpacket) { > + rx_remain = maxpacket; > + } else if (rx_remain % maxpacket != 0) { > + rem = rx_remain % maxpacket; > + rx_remain = rx_remain + (maxpacket - rem); > + } > > After attempting to unsuccessfully contact Dileep, I > requested that this patch be reverted -- because it broke my > boards! (see the other email thread). > > Sam Protsenko <semen.protsenko@linaro.org <mailto:semen.protsenko@linaro.org>> has stated that this Feb 25 > change is required to make "fastboot work on TI platforms". > > Thus, > - Broadcom boards require alignment to "ep->maxpacket" size > - TI platforms require alignment to "wMaxPacketSize" size > And we seem to be at a stale-mate. > Unfortunately, I do not know enough about the USB internals to > understand why this change breaks the Broadcom boards; or why > it _is_ required on the TI platforms.... > ( Is there any debugging that can be turned on to validate > what is happening at the lower levels? ) > ( 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....
I think Lukasz only uses UMS and Thor.
But the problem here is not connected with UMS/DFU/Thor which are on the upper layer in the stack.
This is somewhat two fold:
1. Your host (libusb which one is using + fastboot implementation) requires transfers which are padded to some (fixed?) value (not checking what USB device descriptor is saying for example).
2. The UDC IP block silicon implementation is different for those two companies.
Is Broadcomm using any other gadgets (DFU/UMS)? Is fastboot the only one available (as it is done at bcm28155_ap.h) ?
To fix this problem I use debug from dwc2 UDC driver to see what requests are commit IN and OUT (and when it hangs).
I also use Lauterbach to inspect memory state and registers.
However, I will not help you much since you are using different UDC IP block and driver (musb vs dwc2_udc_otg).
> > 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 nooneis interested enough to cooperate and fix this properly, yes ?
yes -- that is my impression .....
Bad.
> 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 paperover some problem which noone bothered to research and fix properly, sorry.
OK -- I am fine with that....
> 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!
> 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 aproper 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 bedelayed.
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
Hi Steve,
No -- I do not believe that this issue is caused by different fastboot (client) versions (the executable that runs on the host computer - Linux, Windows, Mac, etc.) I have personally attempted three (3) different versions, and the results are consistent.
And no I don't think that I "am the only hope at fixing this proper" -- as you will see below, this" issue" seems to be unique to the "TI platforms" (... nobody else has stated they have an issue either way -- but I don't think many use this feature ....) So maybe someone with "TI platforms" could investigate this more thoroughly...
HISTORY:
The U-Boot code, up to Feb 25, worked properly on my Broadcom boards -- this code contains: req->length = rx_bytes_expected(); if (req->length < ep->maxpacket) req->length = ep->maxpacket; which aligned the remaining "rx_bytes_expected" to be aligned to the "ep->maxpacket" size.
On Feb 25, there was a patch applied from dileep.katta@linaro.org which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:
if (rx_remain < maxpacket) {rx_remain = maxpacket;} else if (rx_remain % maxpacket != 0) {rem = rx_remain % maxpacket;rx_remain = rx_remain + (maxpacket - rem);}After attempting to unsuccessfully contact Dileep, I requested that this patch be reverted -- because it broke my boards! (see the other email thread).
Sam Protsenko semen.protsenko@linaro.org has stated that this Feb 25 change is required to make "fastboot work on TI platforms".
Thus,
- Broadcom boards require alignment to "ep->maxpacket" size
- TI platforms require alignment to "wMaxPacketSize" size
And we seem to be at a stale-mate. Unfortunately, I do not know enough about the USB internals to understand why this change breaks the Broadcom boards; or why it _is_ required on the TI platforms.... ( Is there any debugging that can be turned on to validate what is happening at the lower levels? )
I can only speak about DWC2 (from Synopsis) embedded at Samsung boards. There are low level debugging registers (documented, but not supposed to be used at normal operation), which give you some impression regarding very low level events.
DWC2 at Samsung is using those to work properly since we had some problems with dwc2 IP blocks implementation on early Samsung platforms :-). This approach works in u-boot up till now.
Another option is to use JTAG debugger (like Lauterbach) to inspect state of this IP block.
( Can anyone explain why "wMaxPacketSize" size would be required? -- my limited understanding of endpoints makes me think that "ep->maxpacket" size is actually the correct value! )
I asked Sam to submit a patch which conditionally applied the alignment to "wMaxPacketSize" size change -- he stated that he was too busy right now -- so I submitted this patch on his behalf (although he still needs to add the Kconfig for the TI platforms in order to make his boards work)....
I suppose I could also propose a patch where the condition _removes_ this feature (and define it on the Broadcom boards) -- do we generally like "negated" conditionals? +#ifndef CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE Please advise!
Further, how does the U-Boot community respond to a change which breaks something which is already working? Doesn't the "author" of that change bear any responsibility on assisting to get "their" change working properly with "all" the existing boards?
As we know the author of this change is not working at Linaro anymore.
I'm getting the impression that "because the current code works for me", that I am not getting any assistance in resolving this issue -- which is why I suggested "reverting" this change back to the original code; that way, it would (politely?) force someone interested in "TI platforms" to step up and look into this....
Sorry for asking so many questions in one email -- but I'd appreciate answers.... ( I also apologize in advance for the "attitude" which is leaking into this email... ) Please tell me what I can do! I had working boards; now they are all broken -- and I don't how how to get them working again....
If you don't have enough time (and HW) for investigate the issue, I think that Kconfig option with documentation entry is the way to go.
I hope that Sam don't have any objections with such approach.
Thanks, Steve
On Wed, Apr 6, 2016 at 4:01 AM, Marek Vasut marex@denx.de wrote:
On 04/06/2016 07:35 AM, Steve Rae wrote:
On Apr 5, 2016 3:07 PM, "Marek Vasut" <marex@denx.de mailto:marex@denx.de> wrote:
On 04/05/2016 08:31 PM, Steve Rae wrote:
commit 9e4b510 fastboot: OUT transaction length must be aligned to
wMaxPacketSize
breaks some boards...
Therefore add a conditional Kconfig to optionally enable this feature.
Did you drill into it to figure out why this is needed ?
Marek, Let me clarify.... All my boards work with the original code (before the commit which aligned the size to the wMaxPacketSize).... Since that commit, all my boards are broken. And you will notice in this patch, that none of my boards define this CONFIG_ ...
So I think you are asking the wrong person to drill down into this issue.... Sorry, Steve
Well who else can I ask ? You're our only hope at fixing this proper.
Anyway, see my other reply, maybe we should just add an arg to fastboot command to select one more of operation or the other and default to the one which works.
-- Best regards, Marek Vasut
On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski l.majewski@samsung.com wrote:
Hi Steve,
No -- I do not believe that this issue is caused by different fastboot (client) versions (the executable that runs on the host computer - Linux, Windows, Mac, etc.) I have personally attempted three (3) different versions, and the results are consistent.
And no I don't think that I "am the only hope at fixing this proper" -- as you will see below, this" issue" seems to be unique to the "TI platforms" (... nobody else has stated they have an issue either way -- but I don't think many use this feature ....) So maybe someone with "TI platforms" could investigate this more thoroughly...
HISTORY:
The U-Boot code, up to Feb 25, worked properly on my Broadcom boards -- this code contains: req->length = rx_bytes_expected(); if (req->length < ep->maxpacket) req->length = ep->maxpacket; which aligned the remaining "rx_bytes_expected" to be aligned to the "ep->maxpacket" size.
On Feb 25, there was a patch applied from dileep.katta@linaro.org which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:
if (rx_remain < maxpacket) {rx_remain = maxpacket;} else if (rx_remain % maxpacket != 0) {rem = rx_remain % maxpacket;rx_remain = rx_remain + (maxpacket - rem);}After attempting to unsuccessfully contact Dileep, I requested that this patch be reverted -- because it broke my boards! (see the other email thread).
Sam Protsenko semen.protsenko@linaro.org has stated that this Feb 25 change is required to make "fastboot work on TI platforms".
Thus,
- Broadcom boards require alignment to "ep->maxpacket" size
- TI platforms require alignment to "wMaxPacketSize" size
And we seem to be at a stale-mate. Unfortunately, I do not know enough about the USB internals to understand why this change breaks the Broadcom boards; or why it _is_ required on the TI platforms.... ( Is there any debugging that can be turned on to validate what is happening at the lower levels? )
I can only speak about DWC2 (from Synopsis) embedded at Samsung boards. There are low level debugging registers (documented, but not supposed to be used at normal operation), which give you some impression regarding very low level events.
DWC2 at Samsung is using those to work properly since we had some problems with dwc2 IP blocks implementation on early Samsung platforms :-). This approach works in u-boot up till now.
Another option is to use JTAG debugger (like Lauterbach) to inspect state of this IP block.
( Can anyone explain why "wMaxPacketSize" size would be required? -- my limited understanding of endpoints makes me think that "ep->maxpacket" size is actually the correct value! )
I asked Sam to submit a patch which conditionally applied the alignment to "wMaxPacketSize" size change -- he stated that he was too busy right now -- so I submitted this patch on his behalf (although he still needs to add the Kconfig for the TI platforms in order to make his boards work)....
I suppose I could also propose a patch where the condition _removes_ this feature (and define it on the Broadcom boards) -- do we generally like "negated" conditionals? +#ifndef CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE Please advise!
Further, how does the U-Boot community respond to a change which breaks something which is already working? Doesn't the "author" of that change bear any responsibility on assisting to get "their" change working properly with "all" the existing boards?
As we know the author of this change is not working at Linaro anymore.
I'm getting the impression that "because the current code works for me", that I am not getting any assistance in resolving this issue -- which is why I suggested "reverting" this change back to the original code; that way, it would (politely?) force someone interested in "TI platforms" to step up and look into this....
Sorry for asking so many questions in one email -- but I'd appreciate answers.... ( I also apologize in advance for the "attitude" which is leaking into this email... ) Please tell me what I can do! I had working boards; now they are all broken -- and I don't how how to get them working again....
If you don't have enough time (and HW) for investigate the issue, I think that Kconfig option with documentation entry is the way to go.
I hope that Sam don't have any objections with such approach.
If this commit doesn't break any platform -- I'm ok with that. If it breaks anything (TI boards particularly) -- I'd ask to revert it at once, as this is I believe not right way to do things.
So Steve, please add CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED option to all required defconfigs (except yours), so that your patch only fixes your platforms, but doesn't break any other platform at the same time. Also good thing to do after that is check options order in changed defconfigs with "make savedefconfig" rule. Both your current changes and appropriate lines in defconfigs should be committed as a single patch, so that it doesn't break anything and "git bisect" may be used to look for regressions. Also, really nice thing to do after all of this, is to use "./tools/buildman/buildman" tool to check all ARM boards for regressions after your patch (you should see that only your boards were changed).
Ideally, we should check it on all boards (or at least on all UDC controllers supported in U-Boot) and figure out what is happening exactly. But I'm totally fine with hack if it fixes real problem on some platforms. I just ask you guys to not break anything else at the same time (although it surely takes much more effort, but still).
Thanks, Steve
On Wed, Apr 6, 2016 at 4:01 AM, Marek Vasut marex@denx.de wrote:
On 04/06/2016 07:35 AM, Steve Rae wrote:
On Apr 5, 2016 3:07 PM, "Marek Vasut" <marex@denx.de mailto:marex@denx.de> wrote:
On 04/05/2016 08:31 PM, Steve Rae wrote:
commit 9e4b510 fastboot: OUT transaction length must be aligned to
wMaxPacketSize
breaks some boards...
Therefore add a conditional Kconfig to optionally enable this feature.
Did you drill into it to figure out why this is needed ?
Marek, Let me clarify.... All my boards work with the original code (before the commit which aligned the size to the wMaxPacketSize).... Since that commit, all my boards are broken. And you will notice in this patch, that none of my boards define this CONFIG_ ...
So I think you are asking the wrong person to drill down into this issue.... Sorry, Steve
Well who else can I ask ? You're our only hope at fixing this proper.
Anyway, see my other reply, maybe we should just add an arg to fastboot command to select one more of operation or the other and default to the one which works.
-- Best regards, Marek Vasut
-- Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
Hi Sam,
On Thu, Apr 7, 2016 at 9:46 AM, Sam Protsenko semen.protsenko@linaro.org wrote:
On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski l.majewski@samsung.com wrote:
Hi Steve,
No -- I do not believe that this issue is caused by different fastboot (client) versions (the executable that runs on the host computer - Linux, Windows, Mac, etc.) I have personally attempted three (3) different versions, and the results are consistent.
And no I don't think that I "am the only hope at fixing this proper" -- as you will see below, this" issue" seems to be unique to the "TI platforms" (... nobody else has stated they have an issue either way -- but I don't think many use this feature ....) So maybe someone with "TI platforms" could investigate this more thoroughly...
HISTORY:
The U-Boot code, up to Feb 25, worked properly on my Broadcom boards -- this code contains: req->length = rx_bytes_expected(); if (req->length < ep->maxpacket) req->length = ep->maxpacket; which aligned the remaining "rx_bytes_expected" to be aligned to the "ep->maxpacket" size.
On Feb 25, there was a patch applied from dileep.katta@linaro.org which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:
if (rx_remain < maxpacket) {rx_remain = maxpacket;} else if (rx_remain % maxpacket != 0) {rem = rx_remain % maxpacket;rx_remain = rx_remain + (maxpacket - rem);}After attempting to unsuccessfully contact Dileep, I requested that this patch be reverted -- because it broke my boards! (see the other email thread).
Sam Protsenko semen.protsenko@linaro.org has stated that this Feb 25 change is required to make "fastboot work on TI platforms".
Thus,
- Broadcom boards require alignment to "ep->maxpacket" size
- TI platforms require alignment to "wMaxPacketSize" size
And we seem to be at a stale-mate. Unfortunately, I do not know enough about the USB internals to understand why this change breaks the Broadcom boards; or why it _is_ required on the TI platforms.... ( Is there any debugging that can be turned on to validate what is happening at the lower levels? )
I can only speak about DWC2 (from Synopsis) embedded at Samsung boards. There are low level debugging registers (documented, but not supposed to be used at normal operation), which give you some impression regarding very low level events.
DWC2 at Samsung is using those to work properly since we had some problems with dwc2 IP blocks implementation on early Samsung platforms :-). This approach works in u-boot up till now.
Another option is to use JTAG debugger (like Lauterbach) to inspect state of this IP block.
( Can anyone explain why "wMaxPacketSize" size would be required? -- my limited understanding of endpoints makes me think that "ep->maxpacket" size is actually the correct value! )
I asked Sam to submit a patch which conditionally applied the alignment to "wMaxPacketSize" size change -- he stated that he was too busy right now -- so I submitted this patch on his behalf (although he still needs to add the Kconfig for the TI platforms in order to make his boards work)....
I suppose I could also propose a patch where the condition _removes_ this feature (and define it on the Broadcom boards) -- do we generally like "negated" conditionals? +#ifndef
CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE
Please advise!
Further, how does the U-Boot community respond to a change which breaks something which is already working? Doesn't the "author" of that change bear any responsibility on assisting to get "their" change working properly with "all" the existing boards?
As we know the author of this change is not working at Linaro anymore.
I'm getting the impression that "because the current code works for me", that I am not getting any assistance in resolving this issue -- which is why I suggested "reverting" this change back to the original code; that way, it would (politely?) force someone interested in "TI platforms" to step up and look into this....
Sorry for asking so many questions in one email -- but I'd appreciate answers.... ( I also apologize in advance for the "attitude" which is leaking into this email... ) Please tell me what I can do! I had working boards; now they are all broken -- and I don't how how to get them working again....
If you don't have enough time (and HW) for investigate the issue, I think that Kconfig option with documentation entry is the way to go.
I hope that Sam don't have any objections with such approach.
If this commit doesn't break any platform -- I'm ok with that. If it breaks anything (TI boards particularly) -- I'd ask to revert it at once, as this is I believe not right way to do things.
I'm confused... You are saying that it is OK to checkin a change that fixes TI boards (Feb 25), even though it breaks Broadcom boards; but if _this_ change "breaks anything" then it is NOT OK ????? ( I politely disagree.... ) PS - therefore - what is the right way? (..."this is I believe not right way to do things"...)
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
Also good thing to do after that is check options order in changed defconfigs with "make savedefconfig" rule. Both your current changes and appropriate lines in defconfigs should be committed as a single patch, so that it doesn't break anything and "git bisect" may be used to look for regressions. Also, really nice thing to do after all of this, is to use "./tools/buildman/buildman" tool to check all ARM boards for regressions after your patch (you should see that only your boards were changed).
yup -- I use buildman almost exclusively....
Ideally, we should check it on all boards (or at least on all UDC controllers supported in U-Boot) and figure out what is happening exactly. But I'm totally fine with hack if it fixes real problem on some platforms. I just ask you guys to not break anything else at the same time (although it surely takes much more effort, but still).
I'm confused (again) -- why are you asking: "you guys to not break anything else"... IT IS ALREADY BROKEN, it is broken right now, and has been broken since Feb 25 !
Please fix this!
Thanks, Steve
On Wed, Apr 6, 2016 at 4:01 AM, Marek Vasut marex@denx.de wrote:
On 04/06/2016 07:35 AM, Steve Rae wrote:
On Apr 5, 2016 3:07 PM, "Marek Vasut" <marex@denx.de mailto:marex@denx.de> wrote:
On 04/05/2016 08:31 PM, Steve Rae wrote: > commit 9e4b510 fastboot: OUT transaction length must be aligned > to
wMaxPacketSize
> breaks some boards... > > Therefore add a conditional Kconfig to optionally enable this > feature.
Did you drill into it to figure out why this is needed ?
Marek, Let me clarify.... All my boards work with the original code (before the commit which aligned the size to the wMaxPacketSize).... Since that commit, all my boards are broken. And you will notice in this patch, that none of my boards define this CONFIG_ ...
So I think you are asking the wrong person to drill down into this issue.... Sorry, Steve
Well who else can I ask ? You're our only hope at fixing this proper.
Anyway, see my other reply, maybe we should just add an arg to fastboot command to select one more of operation or the other and default to the one which works.
-- Best regards, Marek Vasut
-- Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
On Thu, Apr 7, 2016 at 8:07 PM, Steve Rae steve.rae@broadcom.com wrote:
Hi Sam,
On Thu, Apr 7, 2016 at 9:46 AM, Sam Protsenko semen.protsenko@linaro.org wrote:
On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski l.majewski@samsung.com wrote:
Hi Steve,
No -- I do not believe that this issue is caused by different fastboot (client) versions (the executable that runs on the host computer - Linux, Windows, Mac, etc.) I have personally attempted three (3) different versions, and the results are consistent.
And no I don't think that I "am the only hope at fixing this proper" -- as you will see below, this" issue" seems to be unique to the "TI platforms" (... nobody else has stated they have an issue either way -- but I don't think many use this feature ....) So maybe someone with "TI platforms" could investigate this more thoroughly...
HISTORY:
The U-Boot code, up to Feb 25, worked properly on my Broadcom boards -- this code contains: req->length = rx_bytes_expected(); if (req->length < ep->maxpacket) req->length = ep->maxpacket; which aligned the remaining "rx_bytes_expected" to be aligned to the "ep->maxpacket" size.
On Feb 25, there was a patch applied from dileep.katta@linaro.org which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:
if (rx_remain < maxpacket) {rx_remain = maxpacket;} else if (rx_remain % maxpacket != 0) {rem = rx_remain % maxpacket;rx_remain = rx_remain + (maxpacket - rem);}After attempting to unsuccessfully contact Dileep, I requested that this patch be reverted -- because it broke my boards! (see the other email thread).
Sam Protsenko semen.protsenko@linaro.org has stated that this Feb 25 change is required to make "fastboot work on TI platforms".
Thus,
- Broadcom boards require alignment to "ep->maxpacket" size
- TI platforms require alignment to "wMaxPacketSize" size
And we seem to be at a stale-mate. Unfortunately, I do not know enough about the USB internals to understand why this change breaks the Broadcom boards; or why it _is_ required on the TI platforms.... ( Is there any debugging that can be turned on to validate what is happening at the lower levels? )
I can only speak about DWC2 (from Synopsis) embedded at Samsung boards. There are low level debugging registers (documented, but not supposed to be used at normal operation), which give you some impression regarding very low level events.
DWC2 at Samsung is using those to work properly since we had some problems with dwc2 IP blocks implementation on early Samsung platforms :-). This approach works in u-boot up till now.
Another option is to use JTAG debugger (like Lauterbach) to inspect state of this IP block.
( Can anyone explain why "wMaxPacketSize" size would be required? -- my limited understanding of endpoints makes me think that "ep->maxpacket" size is actually the correct value! )
I asked Sam to submit a patch which conditionally applied the alignment to "wMaxPacketSize" size change -- he stated that he was too busy right now -- so I submitted this patch on his behalf (although he still needs to add the Kconfig for the TI platforms in order to make his boards work)....
I suppose I could also propose a patch where the condition _removes_ this feature (and define it on the Broadcom boards) -- do we generally like "negated" conditionals? +#ifndef
CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE Please advise!
Further, how does the U-Boot community respond to a change which breaks something which is already working? Doesn't the "author" of that change bear any responsibility on assisting to get "their" change working properly with "all" the existing boards?
As we know the author of this change is not working at Linaro anymore.
I'm getting the impression that "because the current code works for me", that I am not getting any assistance in resolving this issue -- which is why I suggested "reverting" this change back to the original code; that way, it would (politely?) force someone interested in "TI platforms" to step up and look into this....
Sorry for asking so many questions in one email -- but I'd appreciate answers.... ( I also apologize in advance for the "attitude" which is leaking into this email... ) Please tell me what I can do! I had working boards; now they are all broken -- and I don't how how to get them working again....
If you don't have enough time (and HW) for investigate the issue, I think that Kconfig option with documentation entry is the way to go.
I hope that Sam don't have any objections with such approach.
If this commit doesn't break any platform -- I'm ok with that. If it breaks anything (TI boards particularly) -- I'd ask to revert it at once, as this is I believe not right way to do things.
I'm confused... You are saying that it is OK to checkin a change that fixes TI boards (Feb 25), even though it breaks Broadcom boards; but if _this_ change "breaks anything" then it is NOT OK ????? ( I politely disagree.... ) PS - therefore - what is the right way? (..."this is I believe not right way to do things"...)
Look, it's current state of things. Some stuff is broken, I admit that. But you can't just break something while fixing another stuff. It's not even about "your" boards or "my" boards. It's just not right, I thought it's pretty obvious. So what is correct way to do in that case? I believe it's fix only boards you know for sure are broken, but keep old fastboot behaviour for the rest of boards. Not only TI, but all boards except yours. So that after buildman run you can see that only your boards were changed, something like that.
So Steve, please add CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED option to all required defconfigs (except yours), so that your patch only fixes your platforms, but doesn't break any other platform at the same time.
So -- here is why I cannot complete this task: I have absolutely no idea which boards actually _require_ this capability, therefore, I have no idea which defconfigs I would need to update!
As I see it:
- look into include/configs/*.h - find all headers that use fastboot capability - find corresponding TARGET_ for each header - find all defconfigs for each TARGET_ - your defconfigs should disable alignment - rest of defconfigs should enable alignment (default behavior)
This way you fix your boards (that you know need to be fixed) but keep rest of boards intact. If some other boards need to be fixed too -- they will be fixed further by some folks who need that.
So, if you would send me a list of which defconfig files need to add this line, I'll update it.... OR (I would prefer) you could submit a v3 which includes the boards that you know require this capability!
I'm not gonna send this patch for you, sorry. I just don't need that, and I'm not the author of original patch, so it's just not my concern. I can't even test it for boards that actually broken.
Let me know, Thanks, Steve
Also good thing to do after that is check options order in changed defconfigs with "make savedefconfig" rule. Both your current changes and appropriate lines in defconfigs should be committed as a single patch, so that it doesn't break anything and "git bisect" may be used to look for regressions. Also, really nice thing to do after all of this, is to use "./tools/buildman/buildman" tool to check all ARM boards for regressions after your patch (you should see that only your boards were changed).
yup -- I use buildman almost exclusively....
Ideally, we should check it on all boards (or at least on all UDC controllers supported in U-Boot) and figure out what is happening exactly. But I'm totally fine with hack if it fixes real problem on some platforms. I just ask you guys to not break anything else at the same time (although it surely takes much more effort, but still).
I'm confused (again) -- why are you asking: "you guys to not break anything else"... IT IS ALREADY BROKEN, it is broken right now, and has been broken since Feb 25 !
Please fix this!
...So let's fix half of platforms and break the other half of platforms altogether? It's not for me to decide, I'm not the maintainer. But it just doesn't feel right to me.
I understand your concern, and I can help you test your patches on my boards any time and also run some debug patches to see the difference. But I can't fix it for you. Also I'm not sure that your patch would be merged in current shape (it's basically a hack). So if I were you I'd try to figure out the root cause of this issue by comparing results of some debug patches and tests, by running them on your boards (where fastboot is broken) and on some boards where fastboot is working. Maybe running wireshark in both cases can help to understand why it's happening. From my POV it was a good assumption (made by someone earlier) that possible reason is different UDC controllers (I have DWC3 on my TI boards).
Thanks, Steve
On Wed, Apr 6, 2016 at 4:01 AM, Marek Vasut marex@denx.de wrote:
On 04/06/2016 07:35 AM, Steve Rae wrote:
On Apr 5, 2016 3:07 PM, "Marek Vasut" <marex@denx.de mailto:marex@denx.de> wrote: > > On 04/05/2016 08:31 PM, Steve Rae wrote: > > commit 9e4b510 fastboot: OUT transaction length must be aligned > > to wMaxPacketSize > > breaks some boards... > > > > Therefore add a conditional Kconfig to optionally enable this > > feature. > > Did you drill into it to figure out why this is needed ? >
Marek, Let me clarify.... All my boards work with the original code (before the commit which aligned the size to the wMaxPacketSize).... Since that commit, all my boards are broken. And you will notice in this patch, that none of my boards define this CONFIG_ ...
So I think you are asking the wrong person to drill down into this issue.... Sorry, Steve
Well who else can I ask ? You're our only hope at fixing this proper.
Anyway, see my other reply, maybe we should just add an arg to fastboot command to select one more of operation or the other and default to the one which works.
-- Best regards, Marek Vasut
-- Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
On Thu, Apr 7, 2016 at 2:16 PM, Sam Protsenko semen.protsenko@linaro.org wrote:
On Thu, Apr 7, 2016 at 8:07 PM, Steve Rae steve.rae@broadcom.com wrote:
Hi Sam,
On Thu, Apr 7, 2016 at 9:46 AM, Sam Protsenko <
semen.protsenko@linaro.org>
wrote:
On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski <
l.majewski@samsung.com>
wrote:
Hi Steve,
No -- I do not believe that this issue is caused by different
fastboot
(client) versions (the executable that runs on the host computer - Linux, Windows, Mac, etc.) I have personally attempted three (3) different versions, and the results are consistent.
And no I don't think that I "am the only hope at fixing this proper" -- as you will see below, this" issue" seems to be unique to the "TI platforms" (... nobody
else
has stated they have an issue either way -- but I don't think many
use
this feature ....) So maybe someone with "TI platforms" could investigate this more thoroughly...
HISTORY:
The U-Boot code, up to Feb 25, worked properly on my Broadcom boards -- this code contains: req->length = rx_bytes_expected(); if (req->length < ep->maxpacket) req->length = ep->maxpacket; which aligned the remaining "rx_bytes_expected" to be aligned to the "ep->maxpacket" size.
On Feb 25, there was a patch applied from dileep.katta@linaro.org which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:
if (rx_remain < maxpacket) {rx_remain = maxpacket;} else if (rx_remain % maxpacket != 0) {rem = rx_remain % maxpacket;rx_remain = rx_remain + (maxpacket - rem);}After attempting to unsuccessfully contact Dileep, I requested that this patch be reverted -- because it broke my boards! (see the other email thread).
Sam Protsenko semen.protsenko@linaro.org has stated that this Feb
25
change is required to make "fastboot work on TI platforms".
Thus,
- Broadcom boards require alignment to "ep->maxpacket" size
- TI platforms require alignment to "wMaxPacketSize" size
And we seem to be at a stale-mate. Unfortunately, I do not know enough about the USB internals to understand why this change breaks the Broadcom boards; or why it _is_ required on the TI platforms.... ( Is there any debugging that can be turned on to validate what is happening at the lower levels? )
I can only speak about DWC2 (from Synopsis) embedded at Samsung
boards.
There are low level debugging registers (documented, but not supposed to be used at normal operation), which give you some impression regarding very low level events.
DWC2 at Samsung is using those to work properly since we had some problems with dwc2 IP blocks implementation on early Samsung platforms :-). This approach works in u-boot up till now.
Another option is to use JTAG debugger (like Lauterbach) to inspect state of this IP block.
( Can anyone explain why "wMaxPacketSize" size would be required? -- my limited understanding of endpoints makes me think that "ep->maxpacket" size is actually the correct value! )
I asked Sam to submit a patch which conditionally applied the alignment to "wMaxPacketSize" size change -- he stated that he was
too
busy right now -- so I submitted this patch on his behalf (although
he
still needs to add the Kconfig for the TI platforms in order to make his boards work)....
I suppose I could also propose a patch where the condition _removes_ this feature (and define it on the Broadcom boards) -- do we generally like "negated" conditionals? +#ifndef
CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE
Please advise!
Further, how does the U-Boot community respond to a change which breaks something which is already working? Doesn't the "author" of that change bear any responsibility on assisting to get "their"
change
working properly with "all" the existing boards?
As we know the author of this change is not working at Linaro anymore.
I'm getting the impression that "because the current code works for me", that I am
not
getting any assistance in resolving this issue -- which is why I suggested "reverting" this change back to the original code; that
way,
it would (politely?) force someone interested in "TI platforms" to step up and look into this....
Sorry for asking so many questions in one email -- but I'd appreciate answers.... ( I also apologize in advance for the "attitude" which is leaking
into
this email... ) Please tell me what I can do! I had working boards; now they are all broken -- and I don't how how to get them working again....
If you don't have enough time (and HW) for investigate the issue, I think that Kconfig option with documentation entry is the way to go.
I hope that Sam don't have any objections with such approach.
If this commit doesn't break any platform -- I'm ok with that. If it breaks anything (TI boards particularly) -- I'd ask to revert it at once, as this is I believe not right way to do things.
I'm confused... You are saying that it is OK to checkin a change that fixes TI boards
(Feb
25), even though it breaks Broadcom boards; but if _this_ change "breaks anything" then it is NOT OK ????? ( I politely disagree.... ) PS - therefore - what is the right way? (..."this is I believe not right
way
to do things"...)
Look, it's current state of things. Some stuff is broken, I admit that. But you can't just break something while fixing another stuff. It's not even about "your" boards or "my" boards. It's just not right, I thought it's pretty obvious. So what is correct way to do in that case? I believe it's fix only boards you know for sure are broken, but keep old fastboot behaviour for the rest of boards. Not only TI, but all boards except yours. So that after buildman run you can see that only your boards were changed, something like that.
I cannot agree with the assumptions that you are making -- their is no evidence that "all boards except <mine>" where broken prior to the Feb 25 patch....
So Steve, please add CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED option to all required defconfigs (except yours), so that your patch only fixes your platforms, but doesn't break any other platform at the same time.
So -- here is why I cannot complete this task: I have absolutely no idea which boards actually _require_ this
capability,
therefore, I have no idea which defconfigs I would need to update!
As I see it:
- look into include/configs/*.h
- find all headers that use fastboot capability
- find corresponding TARGET_ for each header
- find all defconfigs for each TARGET_
- your defconfigs should disable alignment
- rest of defconfigs should enable alignment (default behavior)
Why should the default behavior be "align with wMaxPacketSize"? 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.
This way you fix your boards (that you know need to be fixed) but keep rest of boards intact. If some other boards need to be fixed too -- they will be fixed further by some folks who need that.
So, if you would send me a list of which defconfig files need to add this line, I'll update it.... OR (I would prefer) you could submit a v3 which includes the boards that
you
know require this capability!
I'm not gonna send this patch for you, sorry. I just don't need that, and I'm not the author of original patch, so it's just not my concern. I can't even test it for boards that actually broken.
Let me know, Thanks, Steve
Also good thing to do after that is check options order in changed defconfigs with "make savedefconfig" rule. Both your current changes and appropriate lines in defconfigs should be committed as a single patch, so that it doesn't break anything and "git bisect" may be used to look for regressions. Also, really nice thing to do after all of this, is to use "./tools/buildman/buildman" tool to check all ARM boards for regressions after your patch (you should see that only your boards were changed).
yup -- I use buildman almost exclusively....
Ideally, we should check it on all boards (or at least on all UDC controllers supported in U-Boot) and figure out what is happening exactly. But I'm totally fine with hack if it fixes real problem on some platforms. I just ask you guys to not break anything else at the same time (although it surely takes much more effort, but still).
I'm confused (again) -- why are you asking: "you guys to not break
anything
else"... IT IS ALREADY BROKEN, it is broken right now, and has been broken since
Feb
25 !
Please fix this!
...So let's fix half of platforms and break the other half of platforms altogether? It's not for me to decide, I'm not the maintainer. But it just doesn't feel right to me.
I understand your concern, and I can help you test your patches on my boards any time and also run some debug patches to see the difference. But I can't fix it for you. Also I'm not sure that your patch would be merged in current shape (it's basically a hack). So if I were you I'd try to figure out the root cause of this issue by comparing results of some debug patches and tests, by running them on your boards (where fastboot is broken) and on some boards where fastboot is working. Maybe running wireshark in both cases can help to understand why it's happening. From my POV it was a good assumption (made by someone earlier) that possible reason is different UDC controllers (I have DWC3 on my TI boards).
Thanks, Steve
On Wed, Apr 6, 2016 at 4:01 AM, Marek Vasut marex@denx.de wrote:
On 04/06/2016 07:35 AM, Steve Rae wrote: > > On Apr 5, 2016 3:07 PM, "Marek Vasut" <marex@denx.de > mailto:marex@denx.de> wrote: >> >> On 04/05/2016 08:31 PM, Steve Rae wrote: >> > commit 9e4b510 fastboot: OUT transaction length must be aligned >> > to > wMaxPacketSize >> > breaks some boards... >> > >> > Therefore add a conditional Kconfig to optionally enable this >> > feature. >> >> Did you drill into it to figure out why this is needed ? >> > > Marek, > Let me clarify.... > All my boards work with the original code (before the commit which > aligned the size to the wMaxPacketSize).... Since that commit, > all my boards are broken. > And you will notice in this patch, that none of my boards define > this CONFIG_ ... > > So I think you are asking the wrong person to drill down into this > issue.... Sorry, Steve
Well who else can I ask ? You're our only hope at fixing this proper.
Anyway, see my other reply, maybe we should just add an arg to fastboot command to select one more of operation or the other and default to the one which works.
-- Best regards, Marek Vasut
-- Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
On Fri, Apr 8, 2016 at 12:39 AM, Steve Rae steve.rae@broadcom.com wrote:
On Thu, Apr 7, 2016 at 2:16 PM, Sam Protsenko semen.protsenko@linaro.org wrote:
On Thu, Apr 7, 2016 at 8:07 PM, Steve Rae steve.rae@broadcom.com wrote:
Hi Sam,
On Thu, Apr 7, 2016 at 9:46 AM, Sam Protsenko semen.protsenko@linaro.org wrote:
On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski l.majewski@samsung.com wrote:
Hi Steve,
No -- I do not believe that this issue is caused by different fastboot (client) versions (the executable that runs on the host computer - Linux, Windows, Mac, etc.) I have personally attempted three (3) different versions, and the results are consistent.
And no I don't think that I "am the only hope at fixing this proper" -- as you will see below, this" issue" seems to be unique to the "TI platforms" (... nobody else has stated they have an issue either way -- but I don't think many use this feature ....) So maybe someone with "TI platforms" could investigate this more thoroughly...
HISTORY:
The U-Boot code, up to Feb 25, worked properly on my Broadcom boards -- this code contains: req->length = rx_bytes_expected(); if (req->length < ep->maxpacket) req->length = ep->maxpacket; which aligned the remaining "rx_bytes_expected" to be aligned to the "ep->maxpacket" size.
On Feb 25, there was a patch applied from dileep.katta@linaro.org which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:
if (rx_remain < maxpacket) {rx_remain = maxpacket;} else if (rx_remain % maxpacket != 0) {rem = rx_remain % maxpacket;rx_remain = rx_remain + (maxpacket - rem);}After attempting to unsuccessfully contact Dileep, I requested that this patch be reverted -- because it broke my boards! (see the other email thread).
Sam Protsenko semen.protsenko@linaro.org has stated that this Feb 25 change is required to make "fastboot work on TI platforms".
Thus,
- Broadcom boards require alignment to "ep->maxpacket" size
- TI platforms require alignment to "wMaxPacketSize" size
And we seem to be at a stale-mate. Unfortunately, I do not know enough about the USB internals to understand why this change breaks the Broadcom boards; or why it _is_ required on the TI platforms.... ( Is there any debugging that can be turned on to validate what is happening at the lower levels? )
I can only speak about DWC2 (from Synopsis) embedded at Samsung boards. There are low level debugging registers (documented, but not supposed to be used at normal operation), which give you some impression regarding very low level events.
DWC2 at Samsung is using those to work properly since we had some problems with dwc2 IP blocks implementation on early Samsung platforms :-). This approach works in u-boot up till now.
Another option is to use JTAG debugger (like Lauterbach) to inspect state of this IP block.
( Can anyone explain why "wMaxPacketSize" size would be required? -- my limited understanding of endpoints makes me think that "ep->maxpacket" size is actually the correct value! )
I asked Sam to submit a patch which conditionally applied the alignment to "wMaxPacketSize" size change -- he stated that he was too busy right now -- so I submitted this patch on his behalf (although he still needs to add the Kconfig for the TI platforms in order to make his boards work)....
I suppose I could also propose a patch where the condition _removes_ this feature (and define it on the Broadcom boards) -- do we generally like "negated" conditionals? +#ifndef
CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE Please advise!
Further, how does the U-Boot community respond to a change which breaks something which is already working? Doesn't the "author" of that change bear any responsibility on assisting to get "their" change working properly with "all" the existing boards?
As we know the author of this change is not working at Linaro anymore.
I'm getting the impression that "because the current code works for me", that I am not getting any assistance in resolving this issue -- which is why I suggested "reverting" this change back to the original code; that way, it would (politely?) force someone interested in "TI platforms" to step up and look into this....
Sorry for asking so many questions in one email -- but I'd appreciate answers.... ( I also apologize in advance for the "attitude" which is leaking into this email... ) Please tell me what I can do! I had working boards; now they are all broken -- and I don't how how to get them working again....
If you don't have enough time (and HW) for investigate the issue, I think that Kconfig option with documentation entry is the way to go.
I hope that Sam don't have any objections with such approach.
If this commit doesn't break any platform -- I'm ok with that. If it breaks anything (TI boards particularly) -- I'd ask to revert it at once, as this is I believe not right way to do things.
I'm confused... You are saying that it is OK to checkin a change that fixes TI boards (Feb 25), even though it breaks Broadcom boards; but if _this_ change "breaks anything" then it is NOT OK ????? ( I politely disagree.... ) PS - therefore - what is the right way? (..."this is I believe not right way to do things"...)
Look, it's current state of things. Some stuff is broken, I admit that. But you can't just break something while fixing another stuff. It's not even about "your" boards or "my" boards. It's just not right, I thought it's pretty obvious. So what is correct way to do in that case? I believe it's fix only boards you know for sure are broken, but keep old fastboot behaviour for the rest of boards. Not only TI, but all boards except yours. So that after buildman run you can see that only your boards were changed, something like that.
I cannot agree with the assumptions that you are making -- their is no evidence that "all boards except <mine>" where broken prior to the Feb 25 patch....
So Steve, please add CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED option to all required defconfigs (except yours), so that your patch only fixes your platforms, but doesn't break any other platform at the same time.
So -- here is why I cannot complete this task: I have absolutely no idea which boards actually _require_ this capability, therefore, I have no idea which defconfigs I would need to update!
As I see it:
- look into include/configs/*.h
- find all headers that use fastboot capability
- find corresponding TARGET_ for each header
- find all defconfigs for each TARGET_
- your defconfigs should disable alignment
- rest of defconfigs should enable alignment (default behavior)
Why should the default behavior be "align with wMaxPacketSize"?
It's not the point. The default behavior can be any of those two, I basically meant "previous" or "another" behavior.
I would argue that the default behavior should be "align with "ep->maxpacket" size"!
For the sake of argument, we don't know for sure which behavior is correct, because it's not documented.
U-Boot history shows that "align with "ep->maxpacket" size" was the original code; then a patch was added to change it to "align with wMaxPacketSize" -- however, there is NO EXPLANATION given, other than the commit message:
fastboot: OUT transaction length must be aligned to wMaxPacketSize OUT transactions must be aligned to wMaxPacketSize for each transfer, or else transfer will not complete successfully. This patch modifies rx_bytes_expected to return a transfer length that is aligned to wMaxPacketSize. Note that the value of wMaxPacketSize and ep->maxpacket may not be the same value, and it is the value of wMaxPacketSize that should be used for alignment. wMaxPacketSize is passed depending on the speed of connection.
The only actual documentation for fastboot protocol I found is [1], and I don't see any mention of alignment there at all. So it wouldn't surprise me if that patch was done just out of of empiric observations. Which doesn't make it any less significant.
Please review [2], as it can be related. Especially this part of commit message:
USB 2.0 specification, section 8.5.3.2 says:
" 8.5.3.2 Variable-length Data Stage A control pipe may have a variable-length data phase in which the host requests more data than is contained in the specified data structure. When all of the data structure is returned to the host, the function should indicate that the Data stage is ended by returning a packet that is shorter than the MaxPacketSize for the pipe. If the data structure is an exact multiple of wMaxPacketSize for the pipe, the function will return a zero-length packet to indicate the end of the Data stage. "
I'm too sleepy right now to interpret information and make any conclusions, but maybe we should check how the end of Data Stage is implemented for different UDC controllers in U-Boot. TI boards use DWC3 controller. Which is yours?
[1] https://android.googlesource.com/platform/system/core/+/master/fastboot/fast... [2] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3...
This way you fix your boards (that you know need to be fixed) but keep rest of boards intact. If some other boards need to be fixed too -- they will be fixed further by some folks who need that.
So, if you would send me a list of which defconfig files need to add this line, I'll update it.... OR (I would prefer) you could submit a v3 which includes the boards that you know require this capability!
I'm not gonna send this patch for you, sorry. I just don't need that, and I'm not the author of original patch, so it's just not my concern. I can't even test it for boards that actually broken.
Let me know, Thanks, Steve
Also good thing to do after that is check options order in changed defconfigs with "make savedefconfig" rule. Both your current changes and appropriate lines in defconfigs should be committed as a single patch, so that it doesn't break anything and "git bisect" may be used to look for regressions. Also, really nice thing to do after all of this, is to use "./tools/buildman/buildman" tool to check all ARM boards for regressions after your patch (you should see that only your boards were changed).
yup -- I use buildman almost exclusively....
Ideally, we should check it on all boards (or at least on all UDC controllers supported in U-Boot) and figure out what is happening exactly. But I'm totally fine with hack if it fixes real problem on some platforms. I just ask you guys to not break anything else at the same time (although it surely takes much more effort, but still).
I'm confused (again) -- why are you asking: "you guys to not break anything else"... IT IS ALREADY BROKEN, it is broken right now, and has been broken since Feb 25 !
Please fix this!
...So let's fix half of platforms and break the other half of platforms altogether? It's not for me to decide, I'm not the maintainer. But it just doesn't feel right to me.
I understand your concern, and I can help you test your patches on my boards any time and also run some debug patches to see the difference. But I can't fix it for you. Also I'm not sure that your patch would be merged in current shape (it's basically a hack). So if I were you I'd try to figure out the root cause of this issue by comparing results of some debug patches and tests, by running them on your boards (where fastboot is broken) and on some boards where fastboot is working. Maybe running wireshark in both cases can help to understand why it's happening. From my POV it was a good assumption (made by someone earlier) that possible reason is different UDC controllers (I have DWC3 on my TI boards).
Thanks, Steve
On Wed, Apr 6, 2016 at 4:01 AM, Marek Vasut marex@denx.de wrote: > On 04/06/2016 07:35 AM, Steve Rae wrote: >> >> On Apr 5, 2016 3:07 PM, "Marek Vasut" <marex@denx.de >> mailto:marex@denx.de> wrote: >>> >>> On 04/05/2016 08:31 PM, Steve Rae wrote: >>> > commit 9e4b510 fastboot: OUT transaction length must be >>> > aligned >>> > to >> wMaxPacketSize >>> > breaks some boards... >>> > >>> > Therefore add a conditional Kconfig to optionally enable this >>> > feature. >>> >>> Did you drill into it to figure out why this is needed ? >>> >> >> Marek, >> Let me clarify.... >> All my boards work with the original code (before the commit >> which >> aligned the size to the wMaxPacketSize).... Since that commit, >> all my boards are broken. >> And you will notice in this patch, that none of my boards define >> this CONFIG_ ... >> >> So I think you are asking the wrong person to drill down into >> this >> issue.... Sorry, Steve > > Well who else can I ask ? You're our only hope at fixing this > proper. > > Anyway, see my other reply, maybe we should just add an arg to > fastboot command to select one more of operation or the other and > default to the one which works. > > -- > Best regards, > Marek Vasut
-- Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
On Thu, Apr 7, 2016 at 4:11 PM, Sam Protsenko semen.protsenko@linaro.org wrote:
On Fri, Apr 8, 2016 at 12:39 AM, Steve Rae steve.rae@broadcom.com wrote:
On Thu, Apr 7, 2016 at 2:16 PM, Sam Protsenko <
semen.protsenko@linaro.org>
wrote:
On Thu, Apr 7, 2016 at 8:07 PM, Steve Rae steve.rae@broadcom.com
wrote:
Hi Sam,
On Thu, Apr 7, 2016 at 9:46 AM, Sam Protsenko semen.protsenko@linaro.org wrote:
On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski l.majewski@samsung.com wrote:
Hi Steve,
> No -- I do not believe that this issue is caused by different > fastboot > (client) versions (the executable that runs on the host computer - > Linux, Windows, Mac, etc.) > I have personally attempted three (3) different versions, and the > results are consistent. > > And no I don't think that I "am the only hope at fixing this
proper"
> -- as you will see below, > this" issue" seems to be unique to the "TI platforms" (... nobody > else > has stated they have an issue either way -- but I don't think many > use > this feature ....) > So maybe someone with "TI platforms" could investigate this more > thoroughly... > > HISTORY: > > The U-Boot code, up to Feb 25, worked properly on my Broadcom
boards
> -- this code contains: > req->length = rx_bytes_expected(); > if (req->length < ep->maxpacket) > req->length = ep->maxpacket; > which aligned the remaining "rx_bytes_expected" to be aligned to
the
> "ep->maxpacket" size. > > On Feb 25, there was a patch applied from <
dileep.katta@linaro.org>
> which forces the remaining "rx_bytes_expected" to be aligned to
the
> "wMaxPacketSize" size -- this patch broke all Broadcom boards: > + if (rx_remain < maxpacket) { > + rx_remain = maxpacket; > + } else if (rx_remain % maxpacket != 0) { > + rem = rx_remain % maxpacket; > + rx_remain = rx_remain + (maxpacket - rem); > + } > > After attempting to unsuccessfully contact Dileep, I requested
that
> this patch be reverted -- because it broke my boards! (see the
other
> email thread). > > Sam Protsenko semen.protsenko@linaro.org has stated that this
Feb
> 25 > change is required to make "fastboot work on TI platforms". > > Thus, > - Broadcom boards require alignment to "ep->maxpacket" size > - TI platforms require alignment to "wMaxPacketSize" size > And we seem to be at a stale-mate. > Unfortunately, I do not know enough about the USB internals to > understand why this change breaks the Broadcom boards; or why it > _is_ > required on the TI platforms.... > ( Is there any debugging that can be turned on to validate what is > happening at the lower levels? )
I can only speak about DWC2 (from Synopsis) embedded at Samsung boards. There are low level debugging registers (documented, but not
supposed
to be used at normal operation), which give you some impression regarding very low level events.
DWC2 at Samsung is using those to work properly since we had some problems with dwc2 IP blocks implementation on early Samsung platforms :-). This approach works in u-boot up till now.
Another option is to use JTAG debugger (like Lauterbach) to inspect state of this IP block.
> ( Can anyone explain why "wMaxPacketSize" size would be required?
--
> my limited understanding of endpoints makes me think that > "ep->maxpacket" size is actually the correct value! ) > > I asked Sam to submit a patch which conditionally applied the > alignment to "wMaxPacketSize" size change -- he stated that he was > too > busy right now -- so I submitted this patch on his behalf
(although
> he > still needs to add the Kconfig for the TI platforms in order to
make
> his boards work).... > > I suppose I could also propose a patch where the condition
_removes_
> this feature (and define it on the Broadcom boards) -- do we > generally like "negated" conditionals? > +#ifndef > > >
CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE
> Please advise! > > Further, how does the U-Boot community respond to a change which > breaks something which is already working? Doesn't the "author" of > that change bear any responsibility on assisting to get "their" > change > working properly with "all" the existing boards?
As we know the author of this change is not working at Linaro anymore.
> I'm getting the > impression that "because the current code works for me", that I am > not > getting any assistance in resolving this issue -- which is why I > suggested "reverting" this change back to the original code; that > way, > it would (politely?) force someone interested in "TI platforms" to > step up and look into this.... > > Sorry for asking so many questions in one email -- but I'd > appreciate > answers.... > ( I also apologize in advance for the "attitude" which is leaking > into > this email... ) > Please tell me what I can do! I had working boards; now they are
all
> broken -- and I don't how how to get them working again....
If you don't have enough time (and HW) for investigate the issue, I think that Kconfig option with documentation entry is the way to
go.
I hope that Sam don't have any objections with such approach.
If this commit doesn't break any platform -- I'm ok with that. If it breaks anything (TI boards particularly) -- I'd ask to revert it at once, as this is I believe not right way to do things.
I'm confused... You are saying that it is OK to checkin a change that fixes TI boards (Feb 25), even though it breaks Broadcom boards; but if _this_ change "breaks anything" then it is NOT OK ????? ( I politely disagree.... ) PS - therefore - what is the right way? (..."this is I believe not
right
way to do things"...)
Look, it's current state of things. Some stuff is broken, I admit that. But you can't just break something while fixing another stuff. It's not even about "your" boards or "my" boards. It's just not right, I thought it's pretty obvious. So what is correct way to do in that case? I believe it's fix only boards you know for sure are broken, but keep old fastboot behaviour for the rest of boards. Not only TI, but all boards except yours. So that after buildman run you can see that only your boards were changed, something like that.
I cannot agree with the assumptions that you are making -- their is no evidence that "all boards except <mine>" where broken prior to the Feb 25 patch....
So Steve, please add CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED option to all required defconfigs (except yours), so that your patch only fixes
your
platforms, but doesn't break any other platform at the same time.
So -- here is why I cannot complete this task: I have absolutely no idea which boards actually _require_ this capability, therefore, I have no idea which defconfigs I would need to update!
As I see it:
- look into include/configs/*.h
- find all headers that use fastboot capability
- find corresponding TARGET_ for each header
- find all defconfigs for each TARGET_
- your defconfigs should disable alignment
- rest of defconfigs should enable alignment (default behavior)
Why should the default behavior be "align with wMaxPacketSize"?
It's not the point. The default behavior can be any of those two, I basically meant "previous" or "another" behavior.
I would argue that the default behavior should be "align with "ep->maxpacket" size"!
For the sake of argument, we don't know for sure which behavior is correct, because it's not documented.
U-Boot history shows that "align with "ep->maxpacket" size" was the
original
code; then a patch was added to change it to "align with wMaxPacketSize" -- however, there is NO EXPLANATION given, other than the commit message:
fastboot: OUT transaction length must be aligned to wMaxPacketSize OUT transactions must be aligned to wMaxPacketSize for each transfer, or else transfer will not complete successfully. This patch modifies rx_bytes_expected to return a transfer length that is aligned to wMaxPacketSize. Note that the value of wMaxPacketSize and ep->maxpacket may not be the same value, and it is the value of wMaxPacketSize that should be used for alignment. wMaxPacketSize is passed depending on the speedof
connection.The only actual documentation for fastboot protocol I found is [1], and I don't see any mention of alignment there at all. So it wouldn't surprise me if that patch was done just out of of empiric observations. Which doesn't make it any less significant.
Please review [2], as it can be related. Especially this part of commit message:
USB 2.0 specification, section 8.5.3.2 says:
" 8.5.3.2 Variable-length Data Stage A control pipe may have a variable-length data phase in which the host requests more data than is contained in the specified data structure. When all of the data structure is returned to the host, the function should indicate that the Data stage is ended by returning a packet that is shorter than the MaxPacketSize for the pipe. If the data structure is an exact multiple of wMaxPacketSize for the pipe, the function will return a zero-length packet to indicate the end of the Data stage. "
I'm too sleepy right now to interpret information and make any conclusions, but maybe we should check how the end of Data Stage is implemented for different UDC controllers in U-Boot. TI boards use DWC3 controller. Which is yours?
DWC2
[1] https://android.googlesource.com/platform/system/core/+/master/fastboot/fast... [2] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3...
This way you fix your boards (that you know need to be fixed) but keep rest of boards intact. If some other boards need to be fixed too -- they will be fixed further by some folks who need that.
So, if you would send me a list of which defconfig files need to add this line, I'll update it.... OR (I would prefer) you could submit a v3 which includes the boards
that
you know require this capability!
I'm not gonna send this patch for you, sorry. I just don't need that, and I'm not the author of original patch, so it's just not my concern. I can't even test it for boards that actually broken.
Let me know, Thanks, Steve
Also good thing to do after that is check options order in changed defconfigs with "make savedefconfig" rule. Both your current changes and appropriate lines in defconfigs should be committed as a single patch, so that it doesn't break anything and "git bisect" may be used to look for regressions. Also, really nice thing to do after all of this, is to use "./tools/buildman/buildman" tool to check all ARM boards for regressions after your patch (you should see that only
your
boards were changed).
yup -- I use buildman almost exclusively....
Ideally, we should check it on all boards (or at least on all UDC controllers supported in U-Boot) and figure out what is happening exactly. But I'm totally fine with hack if it fixes real problem on some platforms. I just ask you guys to not break anything else at the same time (although it surely takes much more effort, but still).
I'm confused (again) -- why are you asking: "you guys to not break anything else"... IT IS ALREADY BROKEN, it is broken right now, and has been broken
since
Feb 25 !
Please fix this!
...So let's fix half of platforms and break the other half of platforms altogether? It's not for me to decide, I'm not the maintainer. But it just doesn't feel right to me.
I understand your concern, and I can help you test your patches on my boards any time and also run some debug patches to see the difference. But I can't fix it for you. Also I'm not sure that your patch would be merged in current shape (it's basically a hack). So if I were you I'd try to figure out the root cause of this issue by comparing results of some debug patches and tests, by running them on your boards (where fastboot is broken) and on some boards where fastboot is working. Maybe running wireshark in both cases can help to understand why it's happening. From my POV it was a good assumption (made by someone earlier) that possible reason is different UDC controllers (I have DWC3 on my TI boards).
> Thanks, Steve > > On Wed, Apr 6, 2016 at 4:01 AM, Marek Vasut marex@denx.de
wrote:
> > On 04/06/2016 07:35 AM, Steve Rae wrote: > >> > >> On Apr 5, 2016 3:07 PM, "Marek Vasut" <marex@denx.de > >> mailto:marex@denx.de> wrote: > >>> > >>> On 04/05/2016 08:31 PM, Steve Rae wrote: > >>> > commit 9e4b510 fastboot: OUT transaction length must be > >>> > aligned > >>> > to > >> wMaxPacketSize > >>> > breaks some boards... > >>> > > >>> > Therefore add a conditional Kconfig to optionally enable
this
> >>> > feature. > >>> > >>> Did you drill into it to figure out why this is needed ? > >>> > >> > >> Marek, > >> Let me clarify.... > >> All my boards work with the original code (before the commit > >> which > >> aligned the size to the wMaxPacketSize).... Since that commit, > >> all my boards are broken. > >> And you will notice in this patch, that none of my boards
define
> >> this CONFIG_ ... > >> > >> So I think you are asking the wrong person to drill down into > >> this > >> issue.... Sorry, Steve > > > > Well who else can I ask ? You're our only hope at fixing this > > proper. > > > > Anyway, see my other reply, maybe we should just add an arg to > > fastboot command to select one more of operation or the other
and
> > default to the one which works. > > > > -- > > Best regards, > > Marek Vasut >
-- Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
On Fri, Apr 08, 2016 at 02:11:48AM +0300, Sam Protsenko wrote: [snip]
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]
[1] https://android.googlesource.com/platform/system/core/+/master/fastboot/fast...
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?
Tom
-----Original Message----- From: U-Boot [mailto:u-boot-bounces@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]
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]
[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?
The maxpacket size is 512 bytes. As per TI+DWC3 specification, for device mode the TRB buffer size must be multiple of maximum endpoint packet size. For usb data reception (including short transfer), the request buffer length shall be multiple of endpoint maxpacket size.
Regards Ravi
On 04/07/2016 06:46 PM, Sam Protsenko wrote:
On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski l.majewski@samsung.com wrote:
Hi Steve,
No -- I do not believe that this issue is caused by different fastboot (client) versions (the executable that runs on the host computer - Linux, Windows, Mac, etc.) I have personally attempted three (3) different versions, and the results are consistent.
And no I don't think that I "am the only hope at fixing this proper" -- as you will see below, this" issue" seems to be unique to the "TI platforms" (... nobody else has stated they have an issue either way -- but I don't think many use this feature ....) So maybe someone with "TI platforms" could investigate this more thoroughly...
HISTORY:
The U-Boot code, up to Feb 25, worked properly on my Broadcom boards -- this code contains: req->length = rx_bytes_expected(); if (req->length < ep->maxpacket) req->length = ep->maxpacket; which aligned the remaining "rx_bytes_expected" to be aligned to the "ep->maxpacket" size.
On Feb 25, there was a patch applied from dileep.katta@linaro.org which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:
if (rx_remain < maxpacket) {rx_remain = maxpacket;} else if (rx_remain % maxpacket != 0) {rem = rx_remain % maxpacket;rx_remain = rx_remain + (maxpacket - rem);}After attempting to unsuccessfully contact Dileep, I requested that this patch be reverted -- because it broke my boards! (see the other email thread).
Sam Protsenko semen.protsenko@linaro.org has stated that this Feb 25 change is required to make "fastboot work on TI platforms".
Thus,
- Broadcom boards require alignment to "ep->maxpacket" size
- TI platforms require alignment to "wMaxPacketSize" size
And we seem to be at a stale-mate. Unfortunately, I do not know enough about the USB internals to understand why this change breaks the Broadcom boards; or why it _is_ required on the TI platforms.... ( Is there any debugging that can be turned on to validate what is happening at the lower levels? )
I can only speak about DWC2 (from Synopsis) embedded at Samsung boards. There are low level debugging registers (documented, but not supposed to be used at normal operation), which give you some impression regarding very low level events.
DWC2 at Samsung is using those to work properly since we had some problems with dwc2 IP blocks implementation on early Samsung platforms :-). This approach works in u-boot up till now.
Another option is to use JTAG debugger (like Lauterbach) to inspect state of this IP block.
( Can anyone explain why "wMaxPacketSize" size would be required? -- my limited understanding of endpoints makes me think that "ep->maxpacket" size is actually the correct value! )
I asked Sam to submit a patch which conditionally applied the alignment to "wMaxPacketSize" size change -- he stated that he was too busy right now -- so I submitted this patch on his behalf (although he still needs to add the Kconfig for the TI platforms in order to make his boards work)....
I suppose I could also propose a patch where the condition _removes_ this feature (and define it on the Broadcom boards) -- do we generally like "negated" conditionals? +#ifndef CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE Please advise!
Further, how does the U-Boot community respond to a change which breaks something which is already working? Doesn't the "author" of that change bear any responsibility on assisting to get "their" change working properly with "all" the existing boards?
As we know the author of this change is not working at Linaro anymore.
I'm getting the impression that "because the current code works for me", that I am not getting any assistance in resolving this issue -- which is why I suggested "reverting" this change back to the original code; that way, it would (politely?) force someone interested in "TI platforms" to step up and look into this....
Sorry for asking so many questions in one email -- but I'd appreciate answers.... ( I also apologize in advance for the "attitude" which is leaking into this email... ) Please tell me what I can do! I had working boards; now they are all broken -- and I don't how how to get them working again....
If you don't have enough time (and HW) for investigate the issue, I think that Kconfig option with documentation entry is the way to go.
I hope that Sam don't have any objections with such approach.
If this commit doesn't break any platform -- I'm ok with that. If it breaks anything (TI boards particularly) -- I'd ask to revert it at once, as this is I believe not right way to do things.
So Steve, please add CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED option to all required defconfigs (except yours), so that your patch only fixes your platforms, but doesn't break any other platform at the same time. Also good thing to do after that is check options order in changed defconfigs with "make savedefconfig" rule. Both your current changes and appropriate lines in defconfigs should be committed as a single patch, so that it doesn't break anything and "git bisect" may be used to look for regressions. Also, really nice thing to do after all of this, is to use "./tools/buildman/buildman" tool to check all ARM boards for regressions after your patch (you should see that only your boards were changed).
Ideally, we should check it on all boards (or at least on all UDC controllers supported in U-Boot) and figure out what is happening exactly. But I'm totally fine with hack if it fixes real problem on some platforms. I just ask you guys to not break anything else at the same time (although it surely takes much more effort, but still).
I am totally not fine with hack, so please fix it such that both platforms work without added config option. Thanks
Best regards, Marek Vasut
On Friday 08 April 2016 12:10 AM, Marek Vasut wrote:
On 04/07/2016 06:46 PM, Sam Protsenko wrote:
On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski l.majewski@samsung.com wrote:
Hi Steve,
No -- I do not believe that this issue is caused by different fastboot (client) versions (the executable that runs on the host computer - Linux, Windows, Mac, etc.) I have personally attempted three (3) different versions, and the results are consistent.
And no I don't think that I "am the only hope at fixing this proper" -- as you will see below, this" issue" seems to be unique to the "TI platforms" (... nobody else has stated they have an issue either way -- but I don't think many use this feature ....) So maybe someone with "TI platforms" could investigate this more thoroughly...
HISTORY:
The U-Boot code, up to Feb 25, worked properly on my Broadcom boards -- this code contains: req->length = rx_bytes_expected(); if (req->length < ep->maxpacket) req->length = ep->maxpacket; which aligned the remaining "rx_bytes_expected" to be aligned to the "ep->maxpacket" size.
On Feb 25, there was a patch applied from dileep.katta@linaro.org which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:
if (rx_remain < maxpacket) {rx_remain = maxpacket;} else if (rx_remain % maxpacket != 0) {rem = rx_remain % maxpacket;rx_remain = rx_remain + (maxpacket - rem);}After attempting to unsuccessfully contact Dileep, I requested that this patch be reverted -- because it broke my boards! (see the other email thread).
Sam Protsenko semen.protsenko@linaro.org has stated that this Feb 25 change is required to make "fastboot work on TI platforms".
Thus,
- Broadcom boards require alignment to "ep->maxpacket" size
- TI platforms require alignment to "wMaxPacketSize" size
And we seem to be at a stale-mate. Unfortunately, I do not know enough about the USB internals to understand why this change breaks the Broadcom boards; or why it _is_ required on the TI platforms.... ( Is there any debugging that can be turned on to validate what is happening at the lower levels? )
I can only speak about DWC2 (from Synopsis) embedded at Samsung boards. There are low level debugging registers (documented, but not supposed to be used at normal operation), which give you some impression regarding very low level events.
DWC2 at Samsung is using those to work properly since we had some problems with dwc2 IP blocks implementation on early Samsung platforms :-). This approach works in u-boot up till now.
Another option is to use JTAG debugger (like Lauterbach) to inspect state of this IP block.
( Can anyone explain why "wMaxPacketSize" size would be required? -- my limited understanding of endpoints makes me think that "ep->maxpacket" size is actually the correct value! )
I asked Sam to submit a patch which conditionally applied the alignment to "wMaxPacketSize" size change -- he stated that he was too busy right now -- so I submitted this patch on his behalf (although he still needs to add the Kconfig for the TI platforms in order to make his boards work)....
I suppose I could also propose a patch where the condition _removes_ this feature (and define it on the Broadcom boards) -- do we generally like "negated" conditionals? +#ifndef CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE Please advise!
Further, how does the U-Boot community respond to a change which breaks something which is already working? Doesn't the "author" of that change bear any responsibility on assisting to get "their" change working properly with "all" the existing boards?
As we know the author of this change is not working at Linaro anymore.
I'm getting the impression that "because the current code works for me", that I am not getting any assistance in resolving this issue -- which is why I suggested "reverting" this change back to the original code; that way, it would (politely?) force someone interested in "TI platforms" to step up and look into this....
Sorry for asking so many questions in one email -- but I'd appreciate answers.... ( I also apologize in advance for the "attitude" which is leaking into this email... ) Please tell me what I can do! I had working boards; now they are all broken -- and I don't how how to get them working again....
If you don't have enough time (and HW) for investigate the issue, I think that Kconfig option with documentation entry is the way to go.
I hope that Sam don't have any objections with such approach.
If this commit doesn't break any platform -- I'm ok with that. If it breaks anything (TI boards particularly) -- I'd ask to revert it at once, as this is I believe not right way to do things.
So Steve, please add CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED option to all required defconfigs (except yours), so that your patch only fixes your platforms, but doesn't break any other platform at the same time. Also good thing to do after that is check options order in changed defconfigs with "make savedefconfig" rule. Both your current changes and appropriate lines in defconfigs should be committed as a single patch, so that it doesn't break anything and "git bisect" may be used to look for regressions. Also, really nice thing to do after all of this, is to use "./tools/buildman/buildman" tool to check all ARM boards for regressions after your patch (you should see that only your boards were changed).
Ideally, we should check it on all boards (or at least on all UDC controllers supported in U-Boot) and figure out what is happening exactly. But I'm totally fine with hack if it fixes real problem on some platforms. I just ask you guys to not break anything else at the same time (although it surely takes much more effort, but still).
I am totally not fine with hack, so please fix it such that both platforms work without added config option. Thanks
The issue is already solved in Kernel with the patch [1]. May we can take a similar approach and fix the issue without having config options.
[1]: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0...
Regards Mugunthan V N
On Mon, Apr 11, 2016 at 05:04:56PM +0530, Mugunthan V N wrote:
On Friday 08 April 2016 12:10 AM, Marek Vasut wrote:
On 04/07/2016 06:46 PM, Sam Protsenko wrote:
On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski l.majewski@samsung.com wrote:
Hi Steve,
No -- I do not believe that this issue is caused by different fastboot (client) versions (the executable that runs on the host computer - Linux, Windows, Mac, etc.) I have personally attempted three (3) different versions, and the results are consistent.
And no I don't think that I "am the only hope at fixing this proper" -- as you will see below, this" issue" seems to be unique to the "TI platforms" (... nobody else has stated they have an issue either way -- but I don't think many use this feature ....) So maybe someone with "TI platforms" could investigate this more thoroughly...
HISTORY:
The U-Boot code, up to Feb 25, worked properly on my Broadcom boards -- this code contains: req->length = rx_bytes_expected(); if (req->length < ep->maxpacket) req->length = ep->maxpacket; which aligned the remaining "rx_bytes_expected" to be aligned to the "ep->maxpacket" size.
On Feb 25, there was a patch applied from dileep.katta@linaro.org which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:
if (rx_remain < maxpacket) {rx_remain = maxpacket;} else if (rx_remain % maxpacket != 0) {rem = rx_remain % maxpacket;rx_remain = rx_remain + (maxpacket - rem);}After attempting to unsuccessfully contact Dileep, I requested that this patch be reverted -- because it broke my boards! (see the other email thread).
Sam Protsenko semen.protsenko@linaro.org has stated that this Feb 25 change is required to make "fastboot work on TI platforms".
Thus,
- Broadcom boards require alignment to "ep->maxpacket" size
- TI platforms require alignment to "wMaxPacketSize" size
And we seem to be at a stale-mate. Unfortunately, I do not know enough about the USB internals to understand why this change breaks the Broadcom boards; or why it _is_ required on the TI platforms.... ( Is there any debugging that can be turned on to validate what is happening at the lower levels? )
I can only speak about DWC2 (from Synopsis) embedded at Samsung boards. There are low level debugging registers (documented, but not supposed to be used at normal operation), which give you some impression regarding very low level events.
DWC2 at Samsung is using those to work properly since we had some problems with dwc2 IP blocks implementation on early Samsung platforms :-). This approach works in u-boot up till now.
Another option is to use JTAG debugger (like Lauterbach) to inspect state of this IP block.
( Can anyone explain why "wMaxPacketSize" size would be required? -- my limited understanding of endpoints makes me think that "ep->maxpacket" size is actually the correct value! )
I asked Sam to submit a patch which conditionally applied the alignment to "wMaxPacketSize" size change -- he stated that he was too busy right now -- so I submitted this patch on his behalf (although he still needs to add the Kconfig for the TI platforms in order to make his boards work)....
I suppose I could also propose a patch where the condition _removes_ this feature (and define it on the Broadcom boards) -- do we generally like "negated" conditionals? +#ifndef CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE Please advise!
Further, how does the U-Boot community respond to a change which breaks something which is already working? Doesn't the "author" of that change bear any responsibility on assisting to get "their" change working properly with "all" the existing boards?
As we know the author of this change is not working at Linaro anymore.
I'm getting the impression that "because the current code works for me", that I am not getting any assistance in resolving this issue -- which is why I suggested "reverting" this change back to the original code; that way, it would (politely?) force someone interested in "TI platforms" to step up and look into this....
Sorry for asking so many questions in one email -- but I'd appreciate answers.... ( I also apologize in advance for the "attitude" which is leaking into this email... ) Please tell me what I can do! I had working boards; now they are all broken -- and I don't how how to get them working again....
If you don't have enough time (and HW) for investigate the issue, I think that Kconfig option with documentation entry is the way to go.
I hope that Sam don't have any objections with such approach.
If this commit doesn't break any platform -- I'm ok with that. If it breaks anything (TI boards particularly) -- I'd ask to revert it at once, as this is I believe not right way to do things.
So Steve, please add CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED option to all required defconfigs (except yours), so that your patch only fixes your platforms, but doesn't break any other platform at the same time. Also good thing to do after that is check options order in changed defconfigs with "make savedefconfig" rule. Both your current changes and appropriate lines in defconfigs should be committed as a single patch, so that it doesn't break anything and "git bisect" may be used to look for regressions. Also, really nice thing to do after all of this, is to use "./tools/buildman/buildman" tool to check all ARM boards for regressions after your patch (you should see that only your boards were changed).
Ideally, we should check it on all boards (or at least on all UDC controllers supported in U-Boot) and figure out what is happening exactly. But I'm totally fine with hack if it fixes real problem on some platforms. I just ask you guys to not break anything else at the same time (although it surely takes much more effort, but still).
I am totally not fine with hack, so please fix it such that both platforms work without added config option. Thanks
The issue is already solved in Kernel with the patch [1]. May we can take a similar approach and fix the issue without having config options.
This seems reasonable. Can you do this, along with a follow-up patch that sets it for DWC3? Thanks!
Hi Tom, Mugunthan
On Mon, Apr 11, 2016 at 05:04:56PM +0530, Mugunthan V N wrote:
On Friday 08 April 2016 12:10 AM, Marek Vasut wrote:
On 04/07/2016 06:46 PM, Sam Protsenko wrote:
On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski l.majewski@samsung.com wrote:
Hi Steve,
No -- I do not believe that this issue is caused by different fastboot (client) versions (the executable that runs on the host computer - Linux, Windows, Mac, etc.) I have personally attempted three (3) different versions, and the results are consistent.
And no I don't think that I "am the only hope at fixing this proper" -- as you will see below, this" issue" seems to be unique to the "TI platforms" (... nobody else has stated they have an issue either way -- but I don't think many use this feature ....) So maybe someone with "TI platforms" could investigate this more thoroughly...
HISTORY:
The U-Boot code, up to Feb 25, worked properly on my Broadcom boards -- this code contains: req->length = rx_bytes_expected(); if (req->length < ep->maxpacket) req->length = ep->maxpacket; which aligned the remaining "rx_bytes_expected" to be aligned to the "ep->maxpacket" size.
On Feb 25, there was a patch applied from dileep.katta@linaro.org which forces the remaining "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size -- this patch broke all Broadcom boards:
if (rx_remain < maxpacket) {rx_remain = maxpacket;} else if (rx_remain % maxpacket != 0) {rem = rx_remain % maxpacket;rx_remain = rx_remain + (maxpacket - rem);}After attempting to unsuccessfully contact Dileep, I requested that this patch be reverted -- because it broke my boards! (see the other email thread).
Sam Protsenko semen.protsenko@linaro.org has stated that this Feb 25 change is required to make "fastboot work on TI platforms".
Thus,
- Broadcom boards require alignment to "ep->maxpacket" size
- TI platforms require alignment to "wMaxPacketSize" size
And we seem to be at a stale-mate. Unfortunately, I do not know enough about the USB internals to understand why this change breaks the Broadcom boards; or why it _is_ required on the TI platforms.... ( Is there any debugging that can be turned on to validate what is happening at the lower levels? )
I can only speak about DWC2 (from Synopsis) embedded at Samsung boards. There are low level debugging registers (documented, but not supposed to be used at normal operation), which give you some impression regarding very low level events.
DWC2 at Samsung is using those to work properly since we had some problems with dwc2 IP blocks implementation on early Samsung platforms :-). This approach works in u-boot up till now.
Another option is to use JTAG debugger (like Lauterbach) to inspect state of this IP block.
( Can anyone explain why "wMaxPacketSize" size would be required? -- my limited understanding of endpoints makes me think that "ep->maxpacket" size is actually the correct value! )
I asked Sam to submit a patch which conditionally applied the alignment to "wMaxPacketSize" size change -- he stated that he was too busy right now -- so I submitted this patch on his behalf (although he still needs to add the Kconfig for the TI platforms in order to make his boards work)....
I suppose I could also propose a patch where the condition _removes_ this feature (and define it on the Broadcom boards) -- do we generally like "negated" conditionals? +#ifndef CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE Please advise!
Further, how does the U-Boot community respond to a change which breaks something which is already working? Doesn't the "author" of that change bear any responsibility on assisting to get "their" change working properly with "all" the existing boards?
As we know the author of this change is not working at Linaro anymore.
I'm getting the impression that "because the current code works for me", that I am not getting any assistance in resolving this issue -- which is why I suggested "reverting" this change back to the original code; that way, it would (politely?) force someone interested in "TI platforms" to step up and look into this....
Sorry for asking so many questions in one email -- but I'd appreciate answers.... ( I also apologize in advance for the "attitude" which is leaking into this email... ) Please tell me what I can do! I had working boards; now they are all broken -- and I don't how how to get them working again....
If you don't have enough time (and HW) for investigate the issue, I think that Kconfig option with documentation entry is the way to go.
I hope that Sam don't have any objections with such approach.
If this commit doesn't break any platform -- I'm ok with that. If it breaks anything (TI boards particularly) -- I'd ask to revert it at once, as this is I believe not right way to do things.
So Steve, please add CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED option to all required defconfigs (except yours), so that your patch only fixes your platforms, but doesn't break any other platform at the same time. Also good thing to do after that is check options order in changed defconfigs with "make savedefconfig" rule. Both your current changes and appropriate lines in defconfigs should be committed as a single patch, so that it doesn't break anything and "git bisect" may be used to look for regressions. Also, really nice thing to do after all of this, is to use "./tools/buildman/buildman" tool to check all ARM boards for regressions after your patch (you should see that only your boards were changed).
Ideally, we should check it on all boards (or at least on all UDC controllers supported in U-Boot) and figure out what is happening exactly. But I'm totally fine with hack if it fixes real problem on some platforms. I just ask you guys to not break anything else at the same time (although it surely takes much more effort, but still).
I am totally not fine with hack, so please fix it such that both platforms work without added config option. Thanks
The issue is already solved in Kernel with the patch [1]. May we can take a similar approach and fix the issue without having config options.
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,
On 12/04/16 14:19, Lukasz Majewski wrote:
Hi Tom, Mugunthan
On Mon, Apr 11, 2016 at 05:04:56PM +0530, Mugunthan V N wrote:
On Friday 08 April 2016 12:10 AM, Marek Vasut wrote:
On 04/07/2016 06:46 PM, Sam Protsenko wrote:
On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski l.majewski@samsung.com wrote:
Hi Steve,
> No -- I do not believe that this issue is caused by different > fastboot (client) versions (the executable that runs on the > host computer - Linux, Windows, Mac, etc.) > I have personally attempted three (3) different versions, and > the results are consistent. > > And no I don't think that I "am the only hope at fixing this > proper" -- as you will see below, > this" issue" seems to be unique to the "TI platforms" (... > nobody else has stated they have an issue either way -- but I > don't think many use this feature ....) > So maybe someone with "TI platforms" could investigate this > more thoroughly... > > HISTORY: > > The U-Boot code, up to Feb 25, worked properly on my Broadcom > boards -- this code contains: > req->length = rx_bytes_expected(); > if (req->length < ep->maxpacket) > req->length = ep->maxpacket; > which aligned the remaining "rx_bytes_expected" to be aligned > to the "ep->maxpacket" size. > > On Feb 25, there was a patch applied from > dileep.katta@linaro.org which forces the remaining > "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size > -- this patch broke all Broadcom boards: > + if (rx_remain < maxpacket) { > + rx_remain = maxpacket; > + } else if (rx_remain % maxpacket != 0) { > + rem = rx_remain % maxpacket; > + rx_remain = rx_remain + (maxpacket - rem); > + } > > After attempting to unsuccessfully contact Dileep, I requested > that this patch be reverted -- because it broke my boards! > (see the other email thread). > > Sam Protsenko semen.protsenko@linaro.org has stated that > this Feb 25 change is required to make "fastboot work on TI > platforms". > > Thus, > - Broadcom boards require alignment to "ep->maxpacket" size > - TI platforms require alignment to "wMaxPacketSize" size > And we seem to be at a stale-mate. > Unfortunately, I do not know enough about the USB internals to > understand why this change breaks the Broadcom boards; or why > it _is_ required on the TI platforms.... > ( Is there any debugging that can be turned on to validate > what is happening at the lower levels? )
I can only speak about DWC2 (from Synopsis) embedded at Samsung boards. There are low level debugging registers (documented, but not supposed to be used at normal operation), which give you some impression regarding very low level events.
DWC2 at Samsung is using those to work properly since we had some problems with dwc2 IP blocks implementation on early Samsung platforms :-). This approach works in u-boot up till now.
Another option is to use JTAG debugger (like Lauterbach) to inspect state of this IP block.
> ( Can anyone explain why "wMaxPacketSize" size would be > required? -- my limited understanding of endpoints makes me > think that "ep->maxpacket" size is actually the correct value! > ) > > I asked Sam to submit a patch which conditionally applied the > alignment to "wMaxPacketSize" size change -- he stated that he > was too busy right now -- so I submitted this patch on his > behalf (although he still needs to add the Kconfig for the TI > platforms in order to make his boards work).... > > I suppose I could also propose a patch where the condition > _removes_ this feature (and define it on the Broadcom boards) > -- do we generally like "negated" conditionals? > +#ifndef > CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE > Please advise! > > Further, how does the U-Boot community respond to a change > which breaks something which is already working? Doesn't the > "author" of that change bear any responsibility on assisting > to get "their" change working properly with "all" the existing > boards?
As we know the author of this change is not working at Linaro anymore.
> I'm getting the > impression that "because the current code works for me", that > I am not getting any assistance in resolving this issue -- > which is why I suggested "reverting" this change back to the > original code; that way, it would (politely?) force someone > interested in "TI platforms" to step up and look into this.... > > Sorry for asking so many questions in one email -- but I'd > appreciate answers.... > ( I also apologize in advance for the "attitude" which is > leaking into this email... ) > Please tell me what I can do! I had working boards; now they > are all broken -- and I don't how how to get them working > again....
If you don't have enough time (and HW) for investigate the issue, I think that Kconfig option with documentation entry is the way to go.
I hope that Sam don't have any objections with such approach.
If this commit doesn't break any platform -- I'm ok with that. If it breaks anything (TI boards particularly) -- I'd ask to revert it at once, as this is I believe not right way to do things.
So Steve, please add CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED option to all required defconfigs (except yours), so that your patch only fixes your platforms, but doesn't break any other platform at the same time. Also good thing to do after that is check options order in changed defconfigs with "make savedefconfig" rule. Both your current changes and appropriate lines in defconfigs should be committed as a single patch, so that it doesn't break anything and "git bisect" may be used to look for regressions. Also, really nice thing to do after all of this, is to use "./tools/buildman/buildman" tool to check all ARM boards for regressions after your patch (you should see that only your boards were changed).
Ideally, we should check it on all boards (or at least on all UDC controllers supported in U-Boot) and figure out what is happening exactly. But I'm totally fine with hack if it fixes real problem on some platforms. I just ask you guys to not break anything else at the same time (although it surely takes much more effort, but still).
I am totally not fine with hack, so please fix it such that both platforms work without added config option. Thanks
The issue is already solved in Kernel with the patch [1]. May we can take a similar approach and fix the issue without having config options.
This seems reasonable. Can you do this, along with a follow-up patch that sets it for DWC3? Thanks!
If I can add my two cents.
I believe that it would be worth to add some explanation into at least the commit message (like very short excerpt from respective User Manual or at least chapter number for further reference).
The patch in [1] is about setting USB request buffer aligned to MaxPacketSize. In f_fastboot.c case we allocate request buffer like so req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE, EP_BUFFER_SIZE);
where EP_BUFFER_SIZE is 4096 which is an integral multiple of 512 as well as 64. So I'm not sure how [1] is related to the subject and if it will fix anything.
I think the problem is more about the length of the last OUT transfer packet. Some controllers might not like that to be not an integral multiple of wMaxPacketSize and we need to ensure that. 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,
Hi,
On 12/04/16 14:19, Lukasz Majewski wrote:
Hi Tom, Mugunthan
On Mon, Apr 11, 2016 at 05:04:56PM +0530, Mugunthan V N wrote:
On Friday 08 April 2016 12:10 AM, Marek Vasut wrote:
On 04/07/2016 06:46 PM, Sam Protsenko wrote:
On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski l.majewski@samsung.com wrote: > Hi Steve, > >> No -- I do not believe that this issue is caused by different >> fastboot (client) versions (the executable that runs on the >> host computer - Linux, Windows, Mac, etc.) >> I have personally attempted three (3) different versions, and >> the results are consistent. >> >> And no I don't think that I "am the only hope at fixing this >> proper" -- as you will see below, >> this" issue" seems to be unique to the "TI platforms" (... >> nobody else has stated they have an issue either way -- but I >> don't think many use this feature ....) >> So maybe someone with "TI platforms" could investigate this >> more thoroughly... >> >> HISTORY: >> >> The U-Boot code, up to Feb 25, worked properly on my Broadcom >> boards -- this code contains: >> req->length = rx_bytes_expected(); >> if (req->length < ep->maxpacket) >> req->length = ep->maxpacket; >> which aligned the remaining "rx_bytes_expected" to be aligned >> to the "ep->maxpacket" size. >> >> On Feb 25, there was a patch applied from >> dileep.katta@linaro.org which forces the remaining >> "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size >> -- this patch broke all Broadcom boards: >> + if (rx_remain < maxpacket) { >> + rx_remain = maxpacket; >> + } else if (rx_remain % maxpacket != 0) { >> + rem = rx_remain % maxpacket; >> + rx_remain = rx_remain + (maxpacket - rem); >> + } >> >> After attempting to unsuccessfully contact Dileep, I requested >> that this patch be reverted -- because it broke my boards! >> (see the other email thread). >> >> Sam Protsenko semen.protsenko@linaro.org has stated that >> this Feb 25 change is required to make "fastboot work on TI >> platforms". >> >> Thus, >> - Broadcom boards require alignment to "ep->maxpacket" size >> - TI platforms require alignment to "wMaxPacketSize" size >> And we seem to be at a stale-mate. >> Unfortunately, I do not know enough about the USB internals to >> understand why this change breaks the Broadcom boards; or why >> it _is_ required on the TI platforms.... >> ( Is there any debugging that can be turned on to validate >> what is happening at the lower levels? ) > > I can only speak about DWC2 (from Synopsis) embedded at Samsung > boards. There are low level debugging registers (documented, > but not supposed to be used at normal operation), which give > you some impression regarding very low level events. > > DWC2 at Samsung is using those to work properly since we had > some problems with dwc2 IP blocks implementation on early > Samsung platforms :-). This approach works in u-boot up till > now. > > Another option is to use JTAG debugger (like Lauterbach) to > inspect state of this IP block. > >> ( Can anyone explain why "wMaxPacketSize" size would be >> required? -- my limited understanding of endpoints makes me >> think that "ep->maxpacket" size is actually the correct value! >> ) >> >> I asked Sam to submit a patch which conditionally applied the >> alignment to "wMaxPacketSize" size change -- he stated that he >> was too busy right now -- so I submitted this patch on his >> behalf (although he still needs to add the Kconfig for the TI >> platforms in order to make his boards work).... >> >> I suppose I could also propose a patch where the condition >> _removes_ this feature (and define it on the Broadcom boards) >> -- do we generally like "negated" conditionals? >> +#ifndef >> CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE >> Please advise! >> >> Further, how does the U-Boot community respond to a change >> which breaks something which is already working? Doesn't the >> "author" of that change bear any responsibility on assisting >> to get "their" change working properly with "all" the existing >> boards? > > As we know the author of this change is not working at Linaro > anymore. > >> I'm getting the >> impression that "because the current code works for me", that >> I am not getting any assistance in resolving this issue -- >> which is why I suggested "reverting" this change back to the >> original code; that way, it would (politely?) force someone >> interested in "TI platforms" to step up and look into this.... >> >> Sorry for asking so many questions in one email -- but I'd >> appreciate answers.... >> ( I also apologize in advance for the "attitude" which is >> leaking into this email... ) >> Please tell me what I can do! I had working boards; now they >> are all broken -- and I don't how how to get them working >> again.... > > If you don't have enough time (and HW) for investigate the > issue, I think that Kconfig option with documentation entry is > the way to go. > > I hope that Sam don't have any objections with such approach. >
If this commit doesn't break any platform -- I'm ok with that. If it breaks anything (TI boards particularly) -- I'd ask to revert it at once, as this is I believe not right way to do things.
So Steve, please add CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED option to all required defconfigs (except yours), so that your patch only fixes your platforms, but doesn't break any other platform at the same time. Also good thing to do after that is check options order in changed defconfigs with "make savedefconfig" rule. Both your current changes and appropriate lines in defconfigs should be committed as a single patch, so that it doesn't break anything and "git bisect" may be used to look for regressions. Also, really nice thing to do after all of this, is to use "./tools/buildman/buildman" tool to check all ARM boards for regressions after your patch (you should see that only your boards were changed).
Ideally, we should check it on all boards (or at least on all UDC controllers supported in U-Boot) and figure out what is happening exactly. But I'm totally fine with hack if it fixes real problem on some platforms. I just ask you guys to not break anything else at the same time (although it surely takes much more effort, but still).
I am totally not fine with hack, so please fix it such that both platforms work without added config option. Thanks
The issue is already solved in Kernel with the patch [1]. May we can take a similar approach and fix the issue without having config options.
This seems reasonable. Can you do this, along with a follow-up patch that sets it for DWC3? Thanks!
If I can add my two cents.
I believe that it would be worth to add some explanation into at least the commit message (like very short excerpt from respective User Manual or at least chapter number for further reference).
The patch in [1] is about setting USB request buffer aligned to MaxPacketSize. In f_fastboot.c case we allocate request buffer like so req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE, EP_BUFFER_SIZE);
where EP_BUFFER_SIZE is 4096 which is an integral multiple of 512 as well as 64. So I'm not sure how [1] is related to the subject and if it will fix anything.
I think the problem is more about the length of the last OUT transfer packet. Some controllers might not like that to be not an integral multiple of wMaxPacketSize and we need to ensure that.
My question was about the above sentence. I was wondering if there is any errata or user manual entry explicitly specifying that.
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.
Those are now under review :-)
cheers, -roger
Lukasz,
On 12/04/16 16:37, Lukasz Majewski wrote:
Hi Roger,
Hi,
On 12/04/16 14:19, Lukasz Majewski wrote:
Hi Tom, Mugunthan
On Mon, Apr 11, 2016 at 05:04:56PM +0530, Mugunthan V N wrote:
On Friday 08 April 2016 12:10 AM, Marek Vasut wrote:
On 04/07/2016 06:46 PM, Sam Protsenko wrote: > On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski > l.majewski@samsung.com wrote: >> Hi Steve, >> >>> No -- I do not believe that this issue is caused by different >>> fastboot (client) versions (the executable that runs on the >>> host computer - Linux, Windows, Mac, etc.) >>> I have personally attempted three (3) different versions, and >>> the results are consistent. >>> >>> And no I don't think that I "am the only hope at fixing this >>> proper" -- as you will see below, >>> this" issue" seems to be unique to the "TI platforms" (... >>> nobody else has stated they have an issue either way -- but I >>> don't think many use this feature ....) >>> So maybe someone with "TI platforms" could investigate this >>> more thoroughly... >>> >>> HISTORY: >>> >>> The U-Boot code, up to Feb 25, worked properly on my Broadcom >>> boards -- this code contains: >>> req->length = rx_bytes_expected(); >>> if (req->length < ep->maxpacket) >>> req->length = ep->maxpacket; >>> which aligned the remaining "rx_bytes_expected" to be aligned >>> to the "ep->maxpacket" size. >>> >>> On Feb 25, there was a patch applied from >>> dileep.katta@linaro.org which forces the remaining >>> "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size >>> -- this patch broke all Broadcom boards: >>> + if (rx_remain < maxpacket) { >>> + rx_remain = maxpacket; >>> + } else if (rx_remain % maxpacket != 0) { >>> + rem = rx_remain % maxpacket; >>> + rx_remain = rx_remain + (maxpacket - rem); >>> + } >>> >>> After attempting to unsuccessfully contact Dileep, I requested >>> that this patch be reverted -- because it broke my boards! >>> (see the other email thread). >>> >>> Sam Protsenko semen.protsenko@linaro.org has stated that >>> this Feb 25 change is required to make "fastboot work on TI >>> platforms". >>> >>> Thus, >>> - Broadcom boards require alignment to "ep->maxpacket" size >>> - TI platforms require alignment to "wMaxPacketSize" size >>> And we seem to be at a stale-mate. >>> Unfortunately, I do not know enough about the USB internals to >>> understand why this change breaks the Broadcom boards; or why >>> it _is_ required on the TI platforms.... >>> ( Is there any debugging that can be turned on to validate >>> what is happening at the lower levels? ) >> >> I can only speak about DWC2 (from Synopsis) embedded at Samsung >> boards. There are low level debugging registers (documented, >> but not supposed to be used at normal operation), which give >> you some impression regarding very low level events. >> >> DWC2 at Samsung is using those to work properly since we had >> some problems with dwc2 IP blocks implementation on early >> Samsung platforms :-). This approach works in u-boot up till >> now. >> >> Another option is to use JTAG debugger (like Lauterbach) to >> inspect state of this IP block. >> >>> ( Can anyone explain why "wMaxPacketSize" size would be >>> required? -- my limited understanding of endpoints makes me >>> think that "ep->maxpacket" size is actually the correct value! >>> ) >>> >>> I asked Sam to submit a patch which conditionally applied the >>> alignment to "wMaxPacketSize" size change -- he stated that he >>> was too busy right now -- so I submitted this patch on his >>> behalf (although he still needs to add the Kconfig for the TI >>> platforms in order to make his boards work).... >>> >>> I suppose I could also propose a patch where the condition >>> _removes_ this feature (and define it on the Broadcom boards) >>> -- do we generally like "negated" conditionals? >>> +#ifndef >>> CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE >>> Please advise! >>> >>> Further, how does the U-Boot community respond to a change >>> which breaks something which is already working? Doesn't the >>> "author" of that change bear any responsibility on assisting >>> to get "their" change working properly with "all" the existing >>> boards? >> >> As we know the author of this change is not working at Linaro >> anymore. >> >>> I'm getting the >>> impression that "because the current code works for me", that >>> I am not getting any assistance in resolving this issue -- >>> which is why I suggested "reverting" this change back to the >>> original code; that way, it would (politely?) force someone >>> interested in "TI platforms" to step up and look into this.... >>> >>> Sorry for asking so many questions in one email -- but I'd >>> appreciate answers.... >>> ( I also apologize in advance for the "attitude" which is >>> leaking into this email... ) >>> Please tell me what I can do! I had working boards; now they >>> are all broken -- and I don't how how to get them working >>> again.... >> >> If you don't have enough time (and HW) for investigate the >> issue, I think that Kconfig option with documentation entry is >> the way to go. >> >> I hope that Sam don't have any objections with such approach. >> > > If this commit doesn't break any platform -- I'm ok with that. > If it breaks anything (TI boards particularly) -- I'd ask to > revert it at once, as this is I believe not right way to do > things. > > So Steve, please add > CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED option to > all required defconfigs (except yours), so that your patch only > fixes your platforms, but doesn't break any other platform at > the same time. Also good thing to do after that is check options > order in changed defconfigs with "make savedefconfig" rule. Both > your current changes and appropriate lines in defconfigs should > be committed as a single patch, so that it doesn't break > anything and "git bisect" may be used to look for regressions. > Also, really nice thing to do after all of this, is to use > "./tools/buildman/buildman" tool to check all ARM boards for > regressions after your patch (you should see that only your > boards were changed). > > Ideally, we should check it on all boards (or at least on all > UDC controllers supported in U-Boot) and figure out what is > happening exactly. But I'm totally fine with hack if it fixes > real problem on some platforms. I just ask you guys to not > break anything else at the same time (although it surely takes > much more effort, but still).
I am totally not fine with hack, so please fix it such that both platforms work without added config option. Thanks
The issue is already solved in Kernel with the patch [1]. May we can take a similar approach and fix the issue without having config options.
This seems reasonable. Can you do this, along with a follow-up patch that sets it for DWC3? Thanks!
If I can add my two cents.
I believe that it would be worth to add some explanation into at least the commit message (like very short excerpt from respective User Manual or at least chapter number for further reference).
The patch in [1] is about setting USB request buffer aligned to MaxPacketSize. In f_fastboot.c case we allocate request buffer like so req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE, EP_BUFFER_SIZE);
where EP_BUFFER_SIZE is 4096 which is an integral multiple of 512 as well as 64. So I'm not sure how [1] is related to the subject and if it will fix anything.
I think the problem is more about the length of the last OUT transfer packet. Some controllers might not like that to be not an integral multiple of wMaxPacketSize and we need to ensure that.
My question was about the above sentence. I was wondering if there is any errata or user manual entry explicitly specifying that.
It is not an errata but stated in the dwc3 user manual like so
section 8.2.3.3 Buffer Size Rules and Zero-Length Packets
For OUT endpoints, the following rules apply: ■ The BUFSIZ field must be ≥ 1 byte. ■ The total size of a Buffer Descriptor must be a multiple of MaxPacketSize ■ A received zero-length packet still requires a MaxPacketSize buffer. Therefore, if the expected amount of data to be received is a multiple of MaxPacketSize, software should add MaxPacketSize bytes to the buffer to sink a possible zero-length packet at the end of the transfer.
This is being done in f_mass_storage.c in set_bulk_out_req_length(). Doing that shouldn't affect other controllers.
So we need to really fix commit 9e4b510.
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 :-)
Thanks :)
cheers, -roger
On Tue, Apr 12, 2016 at 6:50 AM, Roger Quadros rogerq@ti.com wrote:
Lukasz,
On 12/04/16 16:37, Lukasz Majewski wrote:
Hi Roger,
Hi,
On 12/04/16 14:19, Lukasz Majewski wrote:
Hi Tom, Mugunthan
On Mon, Apr 11, 2016 at 05:04:56PM +0530, Mugunthan V N wrote:
On Friday 08 April 2016 12:10 AM, Marek Vasut wrote: > On 04/07/2016 06:46 PM, Sam Protsenko wrote: >> On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski >> l.majewski@samsung.com wrote: >>> Hi Steve, >>> >>>> No -- I do not believe that this issue is caused by different >>>> fastboot (client) versions (the executable that runs on the >>>> host computer - Linux, Windows, Mac, etc.) >>>> I have personally attempted three (3) different versions, and >>>> the results are consistent. >>>> >>>> And no I don't think that I "am the only hope at fixing this >>>> proper" -- as you will see below, >>>> this" issue" seems to be unique to the "TI platforms" (... >>>> nobody else has stated they have an issue either way -- but I >>>> don't think many use this feature ....) >>>> So maybe someone with "TI platforms" could investigate this >>>> more thoroughly... >>>> >>>> HISTORY: >>>> >>>> The U-Boot code, up to Feb 25, worked properly on my Broadcom >>>> boards -- this code contains: >>>> req->length = rx_bytes_expected(); >>>> if (req->length < ep->maxpacket) >>>> req->length = ep->maxpacket; >>>> which aligned the remaining "rx_bytes_expected" to be aligned >>>> to the "ep->maxpacket" size. >>>> >>>> On Feb 25, there was a patch applied from >>>> dileep.katta@linaro.org which forces the remaining >>>> "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size >>>> -- this patch broke all Broadcom boards: >>>> + if (rx_remain < maxpacket) { >>>> + rx_remain = maxpacket; >>>> + } else if (rx_remain % maxpacket != 0) { >>>> + rem = rx_remain % maxpacket; >>>> + rx_remain = rx_remain + (maxpacket - rem); >>>> + } >>>> >>>> After attempting to unsuccessfully contact Dileep, I requested >>>> that this patch be reverted -- because it broke my boards! >>>> (see the other email thread). >>>> >>>> Sam Protsenko semen.protsenko@linaro.org has stated that >>>> this Feb 25 change is required to make "fastboot work on TI >>>> platforms". >>>> >>>> Thus, >>>> - Broadcom boards require alignment to "ep->maxpacket" size >>>> - TI platforms require alignment to "wMaxPacketSize" size >>>> And we seem to be at a stale-mate. >>>> Unfortunately, I do not know enough about the USB internals to >>>> understand why this change breaks the Broadcom boards; or why >>>> it _is_ required on the TI platforms.... >>>> ( Is there any debugging that can be turned on to validate >>>> what is happening at the lower levels? ) >>> >>> I can only speak about DWC2 (from Synopsis) embedded at Samsung >>> boards. There are low level debugging registers (documented, >>> but not supposed to be used at normal operation), which give >>> you some impression regarding very low level events. >>> >>> DWC2 at Samsung is using those to work properly since we had >>> some problems with dwc2 IP blocks implementation on early >>> Samsung platforms :-). This approach works in u-boot up till >>> now. >>> >>> Another option is to use JTAG debugger (like Lauterbach) to >>> inspect state of this IP block. >>> >>>> ( Can anyone explain why "wMaxPacketSize" size would be >>>> required? -- my limited understanding of endpoints makes me >>>> think that "ep->maxpacket" size is actually the correct value! >>>> ) >>>> >>>> I asked Sam to submit a patch which conditionally applied the >>>> alignment to "wMaxPacketSize" size change -- he stated that he >>>> was too busy right now -- so I submitted this patch on his >>>> behalf (although he still needs to add the Kconfig for the TI >>>> platforms in order to make his boards work).... >>>> >>>> I suppose I could also propose a patch where the condition >>>> _removes_ this feature (and define it on the Broadcom boards) >>>> -- do we generally like "negated" conditionals? >>>> +#ifndef >>>> CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE >>>> Please advise! >>>> >>>> Further, how does the U-Boot community respond to a change >>>> which breaks something which is already working? Doesn't the >>>> "author" of that change bear any responsibility on assisting >>>> to get "their" change working properly with "all" the existing >>>> boards? >>> >>> As we know the author of this change is not working at Linaro >>> anymore. >>> >>>> I'm getting the >>>> impression that "because the current code works for me", that >>>> I am not getting any assistance in resolving this issue -- >>>> which is why I suggested "reverting" this change back to the >>>> original code; that way, it would (politely?) force someone >>>> interested in "TI platforms" to step up and look into this.... >>>> >>>> Sorry for asking so many questions in one email -- but I'd >>>> appreciate answers.... >>>> ( I also apologize in advance for the "attitude" which is >>>> leaking into this email... ) >>>> Please tell me what I can do! I had working boards; now they >>>> are all broken -- and I don't how how to get them working >>>> again.... >>> >>> If you don't have enough time (and HW) for investigate the >>> issue, I think that Kconfig option with documentation entry is >>> the way to go. >>> >>> I hope that Sam don't have any objections with such approach. >>> >> >> If this commit doesn't break any platform -- I'm ok with that. >> If it breaks anything (TI boards particularly) -- I'd ask to >> revert it at once, as this is I believe not right way to do >> things. >> >> So Steve, please add >> CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED option to >> all required defconfigs (except yours), so that your patch only >> fixes your platforms, but doesn't break any other platform at >> the same time. Also good thing to do after that is check options >> order in changed defconfigs with "make savedefconfig" rule. Both >> your current changes and appropriate lines in defconfigs should >> be committed as a single patch, so that it doesn't break >> anything and "git bisect" may be used to look for regressions. >> Also, really nice thing to do after all of this, is to use >> "./tools/buildman/buildman" tool to check all ARM boards for >> regressions after your patch (you should see that only your >> boards were changed). >> >> Ideally, we should check it on all boards (or at least on all >> UDC controllers supported in U-Boot) and figure out what is >> happening exactly. But I'm totally fine with hack if it fixes >> real problem on some platforms. I just ask you guys to not >> break anything else at the same time (although it surely takes >> much more effort, but still). > > I am totally not fine with hack, so please fix it such that both > platforms work without added config option. Thanks >
The issue is already solved in Kernel with the patch [1]. May we can take a similar approach and fix the issue without having config options.
This seems reasonable. Can you do this, along with a follow-up patch that sets it for DWC3? Thanks!
If I can add my two cents.
I believe that it would be worth to add some explanation into at least the commit message (like very short excerpt from respective User Manual or at least chapter number for further reference).
The patch in [1] is about setting USB request buffer aligned to MaxPacketSize. In f_fastboot.c case we allocate request buffer like so req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE, EP_BUFFER_SIZE);
where EP_BUFFER_SIZE is 4096 which is an integral multiple of 512 as well as 64. So I'm not sure how [1] is related to the subject and if it will fix anything.
I think the problem is more about the length of the last OUT transfer packet. Some controllers might not like that to be not an integral multiple of wMaxPacketSize and we need to ensure that.
My question was about the above sentence. I was wondering if there is any errata or user manual entry explicitly specifying that.
It is not an errata but stated in the dwc3 user manual like so
section 8.2.3.3 Buffer Size Rules and Zero-Length Packets
For OUT endpoints, the following rules apply: ■ The BUFSIZ field must be ≥ 1 byte. ■ The total size of a Buffer Descriptor must be a multiple of MaxPacketSize ■ A received zero-length packet still requires a MaxPacketSize buffer. Therefore, if the expected amount of data to be received is a multiple of MaxPacketSize, software should add MaxPacketSize bytes to the buffer to sink a possible zero-length packet at the end of the transfer.
This is being done in f_mass_storage.c in set_bulk_out_req_length(). Doing that shouldn't affect other controllers.
So we need to really fix commit 9e4b510.
Yes -- this is the one that causes my stalling issue: I'll copy some debug output from another email thread:
Lukasz: As per your suggestion, I turned on the following: diff --git a/drivers/usb/gadget/dwc2_udc_otg.c b/drivers/usb/gadget/dwc2_udc_otg.c index 5d53440..763c6d9 100644 --- a/drivers/usb/gadget/dwc2_udc_otg.c +++ b/drivers/usb/gadget/dwc2_udc_otg.c @@ -40,11 +40,11 @@
#define OTG_DMA_MODE 1
-#define DEBUG_SETUP 0 -#define DEBUG_EP0 0 -#define DEBUG_ISR 0 -#define DEBUG_OUT_EP 0 -#define DEBUG_IN_EP 0 +#define DEBUG_SETUP 1 +#define DEBUG_EP0 1 +#define DEBUG_ISR 1 +#define DEBUG_OUT_EP 1 +#define DEBUG_IN_EP 1
and captured the logs of the "last transactions..." (the "-" is with the Feb 25 Patch removed, the "+" is with the Feb 25 Patch applied....)
*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000 EP2-OUT : DOEPINT = 0x2011 complete_rx: RX DMA done : ep = 2, rx bytes = 4096/4096, is_short = 0, DOEPTSIZ = 0x0, remained bytes = 4096 complete_rx: Next Rx request start... setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x401000, DOEPCTL = 0x80098200 buf = 0xffb84f80, pktcnt = 8, xfersize = 4096
*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000 EP2-OUT : DOEPINT = 0x2011 complete_rx: RX DMA done : ep = 2, rx bytes = 4096/4096, is_short = 0, DOEPTSIZ = 0x0, remained bytes = 4096 complete_rx: Next Rx request start... -setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x100218, DOEPCTL = 0x80098200 - buf = 0xffb84f80, pktcnt = 2, xfersize = 536 +setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x100400, DOEPCTL = 0x80098200 + buf = 0xffb84f80, pktcnt = 2, xfersize = 1024
*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000 EP2-OUT : DOEPINT = 0x2011 -complete_rx: RX DMA done : ep = 2, rx bytes = 536/536, is_short = 0, DOEPTSIZ = 0x0, remained bytes = 536 -dwc2_queue: ep_is_in, DWC2_UDC_OTG_GINTSTS=0x14008028 -setdma_tx:EP1 TX DMA start : DIEPDMA0 = 0xffb85fc0,DIEPTSIZ0 = 0x80004, DIEPCTL0 = 0x80498040 - buf = 0xffb85fc0, pktcnt = 1, xfersize = 4 +complete_rx: RX DMA done : ep = 2, rx bytes = 536/1024, is_short = 0, DOEPTSIZ = 0x1e8, remained bytes = 536 +setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb85198,DOEPTSIZ = 0x801e8, DOEPCTL = 0x80098200 + buf = 0xffb85198, pktcnt = 1, xfersize = 488
+++++++ hangs here... -downloading of 258584 bytes finished -complete_rx: Next Rx request start... -setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x401000, DOEPCTL = 0x80098200 - buf = 0xffb84f80, pktcnt = 8, xfersize = 4096
Does this help explain anything ?!?!?!
Thanks, Steve
Another thing I noticed is that f_fastboot.c is not setting the right endpoint size for hight speed BULK_IN endpoint. I'll send out patches for that.
I am fine with these patches -- Thanks Steve
Those are now under review :-)
Thanks :)
cheers, -roger
Steve,
On 13/04/16 04:55, Steve Rae wrote:
On Tue, Apr 12, 2016 at 6:50 AM, Roger Quadros rogerq@ti.com wrote:
Lukasz,
On 12/04/16 16:37, Lukasz Majewski wrote:
Hi Roger,
Hi,
On 12/04/16 14:19, Lukasz Majewski wrote:
Hi Tom, Mugunthan
On Mon, Apr 11, 2016 at 05:04:56PM +0530, Mugunthan V N wrote: > On Friday 08 April 2016 12:10 AM, Marek Vasut wrote: >> On 04/07/2016 06:46 PM, Sam Protsenko wrote: >>> On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski >>> l.majewski@samsung.com wrote: >>>> Hi Steve, >>>> >>>>> No -- I do not believe that this issue is caused by different >>>>> fastboot (client) versions (the executable that runs on the >>>>> host computer - Linux, Windows, Mac, etc.) >>>>> I have personally attempted three (3) different versions, and >>>>> the results are consistent. >>>>> >>>>> And no I don't think that I "am the only hope at fixing this >>>>> proper" -- as you will see below, >>>>> this" issue" seems to be unique to the "TI platforms" (... >>>>> nobody else has stated they have an issue either way -- but I >>>>> don't think many use this feature ....) >>>>> So maybe someone with "TI platforms" could investigate this >>>>> more thoroughly... >>>>> >>>>> HISTORY: >>>>> >>>>> The U-Boot code, up to Feb 25, worked properly on my Broadcom >>>>> boards -- this code contains: >>>>> req->length = rx_bytes_expected(); >>>>> if (req->length < ep->maxpacket) >>>>> req->length = ep->maxpacket; >>>>> which aligned the remaining "rx_bytes_expected" to be aligned >>>>> to the "ep->maxpacket" size. >>>>> >>>>> On Feb 25, there was a patch applied from >>>>> dileep.katta@linaro.org which forces the remaining >>>>> "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size >>>>> -- this patch broke all Broadcom boards: >>>>> + if (rx_remain < maxpacket) { >>>>> + rx_remain = maxpacket; >>>>> + } else if (rx_remain % maxpacket != 0) { >>>>> + rem = rx_remain % maxpacket; >>>>> + rx_remain = rx_remain + (maxpacket - rem); >>>>> + } >>>>> >>>>> After attempting to unsuccessfully contact Dileep, I requested >>>>> that this patch be reverted -- because it broke my boards! >>>>> (see the other email thread). >>>>> >>>>> Sam Protsenko semen.protsenko@linaro.org has stated that >>>>> this Feb 25 change is required to make "fastboot work on TI >>>>> platforms". >>>>> >>>>> Thus, >>>>> - Broadcom boards require alignment to "ep->maxpacket" size >>>>> - TI platforms require alignment to "wMaxPacketSize" size >>>>> And we seem to be at a stale-mate. >>>>> Unfortunately, I do not know enough about the USB internals to >>>>> understand why this change breaks the Broadcom boards; or why >>>>> it _is_ required on the TI platforms.... >>>>> ( Is there any debugging that can be turned on to validate >>>>> what is happening at the lower levels? ) >>>> >>>> I can only speak about DWC2 (from Synopsis) embedded at Samsung >>>> boards. There are low level debugging registers (documented, >>>> but not supposed to be used at normal operation), which give >>>> you some impression regarding very low level events. >>>> >>>> DWC2 at Samsung is using those to work properly since we had >>>> some problems with dwc2 IP blocks implementation on early >>>> Samsung platforms :-). This approach works in u-boot up till >>>> now. >>>> >>>> Another option is to use JTAG debugger (like Lauterbach) to >>>> inspect state of this IP block. >>>> >>>>> ( Can anyone explain why "wMaxPacketSize" size would be >>>>> required? -- my limited understanding of endpoints makes me >>>>> think that "ep->maxpacket" size is actually the correct value! >>>>> ) >>>>> >>>>> I asked Sam to submit a patch which conditionally applied the >>>>> alignment to "wMaxPacketSize" size change -- he stated that he >>>>> was too busy right now -- so I submitted this patch on his >>>>> behalf (although he still needs to add the Kconfig for the TI >>>>> platforms in order to make his boards work).... >>>>> >>>>> I suppose I could also propose a patch where the condition >>>>> _removes_ this feature (and define it on the Broadcom boards) >>>>> -- do we generally like "negated" conditionals? >>>>> +#ifndef >>>>> CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE >>>>> Please advise! >>>>> >>>>> Further, how does the U-Boot community respond to a change >>>>> which breaks something which is already working? Doesn't the >>>>> "author" of that change bear any responsibility on assisting >>>>> to get "their" change working properly with "all" the existing >>>>> boards? >>>> >>>> As we know the author of this change is not working at Linaro >>>> anymore. >>>> >>>>> I'm getting the >>>>> impression that "because the current code works for me", that >>>>> I am not getting any assistance in resolving this issue -- >>>>> which is why I suggested "reverting" this change back to the >>>>> original code; that way, it would (politely?) force someone >>>>> interested in "TI platforms" to step up and look into this.... >>>>> >>>>> Sorry for asking so many questions in one email -- but I'd >>>>> appreciate answers.... >>>>> ( I also apologize in advance for the "attitude" which is >>>>> leaking into this email... ) >>>>> Please tell me what I can do! I had working boards; now they >>>>> are all broken -- and I don't how how to get them working >>>>> again.... >>>> >>>> If you don't have enough time (and HW) for investigate the >>>> issue, I think that Kconfig option with documentation entry is >>>> the way to go. >>>> >>>> I hope that Sam don't have any objections with such approach. >>>> >>> >>> If this commit doesn't break any platform -- I'm ok with that. >>> If it breaks anything (TI boards particularly) -- I'd ask to >>> revert it at once, as this is I believe not right way to do >>> things. >>> >>> So Steve, please add >>> CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED option to >>> all required defconfigs (except yours), so that your patch only >>> fixes your platforms, but doesn't break any other platform at >>> the same time. Also good thing to do after that is check options >>> order in changed defconfigs with "make savedefconfig" rule. Both >>> your current changes and appropriate lines in defconfigs should >>> be committed as a single patch, so that it doesn't break >>> anything and "git bisect" may be used to look for regressions. >>> Also, really nice thing to do after all of this, is to use >>> "./tools/buildman/buildman" tool to check all ARM boards for >>> regressions after your patch (you should see that only your >>> boards were changed). >>> >>> Ideally, we should check it on all boards (or at least on all >>> UDC controllers supported in U-Boot) and figure out what is >>> happening exactly. But I'm totally fine with hack if it fixes >>> real problem on some platforms. I just ask you guys to not >>> break anything else at the same time (although it surely takes >>> much more effort, but still). >> >> I am totally not fine with hack, so please fix it such that both >> platforms work without added config option. Thanks >> > > The issue is already solved in Kernel with the patch [1]. May we > can take a similar approach and fix the issue without having > config options. > > [1]: > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0...
This seems reasonable. Can you do this, along with a follow-up patch that sets it for DWC3? Thanks!
If I can add my two cents.
I believe that it would be worth to add some explanation into at least the commit message (like very short excerpt from respective User Manual or at least chapter number for further reference).
The patch in [1] is about setting USB request buffer aligned to MaxPacketSize. In f_fastboot.c case we allocate request buffer like so req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE, EP_BUFFER_SIZE);
where EP_BUFFER_SIZE is 4096 which is an integral multiple of 512 as well as 64. So I'm not sure how [1] is related to the subject and if it will fix anything.
I think the problem is more about the length of the last OUT transfer packet. Some controllers might not like that to be not an integral multiple of wMaxPacketSize and we need to ensure that.
My question was about the above sentence. I was wondering if there is any errata or user manual entry explicitly specifying that.
It is not an errata but stated in the dwc3 user manual like so
section 8.2.3.3 Buffer Size Rules and Zero-Length Packets
For OUT endpoints, the following rules apply: ■ The BUFSIZ field must be ≥ 1 byte. ■ The total size of a Buffer Descriptor must be a multiple of MaxPacketSize ■ A received zero-length packet still requires a MaxPacketSize buffer. Therefore, if the expected amount of data to be received is a multiple of MaxPacketSize, software should add MaxPacketSize bytes to the buffer to sink a possible zero-length packet at the end of the transfer.
This is being done in f_mass_storage.c in set_bulk_out_req_length(). Doing that shouldn't affect other controllers.
So we need to really fix commit 9e4b510.
Yes -- this is the one that causes my stalling issue: I'll copy some debug output from another email thread:
Lukasz: As per your suggestion, I turned on the following: diff --git a/drivers/usb/gadget/dwc2_udc_otg.c b/drivers/usb/gadget/dwc2_udc_otg.c index 5d53440..763c6d9 100644 --- a/drivers/usb/gadget/dwc2_udc_otg.c +++ b/drivers/usb/gadget/dwc2_udc_otg.c @@ -40,11 +40,11 @@
#define OTG_DMA_MODE 1
-#define DEBUG_SETUP 0 -#define DEBUG_EP0 0 -#define DEBUG_ISR 0 -#define DEBUG_OUT_EP 0 -#define DEBUG_IN_EP 0 +#define DEBUG_SETUP 1 +#define DEBUG_EP0 1 +#define DEBUG_ISR 1 +#define DEBUG_OUT_EP 1 +#define DEBUG_IN_EP 1
and captured the logs of the "last transactions..." (the "-" is with the Feb 25 Patch removed, the "+" is with the Feb 25 Patch applied....)
*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000 EP2-OUT : DOEPINT = 0x2011 complete_rx: RX DMA done : ep = 2, rx bytes = 4096/4096, is_short = 0, DOEPTSIZ = 0x0, remained bytes = 4096 complete_rx: Next Rx request start... setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x401000, DOEPCTL = 0x80098200 buf = 0xffb84f80, pktcnt = 8, xfersize = 4096
*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000 EP2-OUT : DOEPINT = 0x2011 complete_rx: RX DMA done : ep = 2, rx bytes = 4096/4096, is_short = 0, DOEPTSIZ = 0x0, remained bytes = 4096 complete_rx: Next Rx request start... -setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x100218, DOEPCTL = 0x80098200
buf = 0xffb84f80, pktcnt = 2, xfersize = 536+setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x100400, DOEPCTL = 0x80098200
buf = 0xffb84f80, pktcnt = 2, xfersize = 1024
This part looks fine as we're rounding up 536 to 1024 for 512 byte alignment.
*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000 EP2-OUT : DOEPINT = 0x2011 -complete_rx: RX DMA done : ep = 2, rx bytes = 536/536, is_short = 0, DOEPTSIZ = 0x0, remained bytes = 536 -dwc2_queue: ep_is_in, DWC2_UDC_OTG_GINTSTS=0x14008028 -setdma_tx:EP1 TX DMA start : DIEPDMA0 = 0xffb85fc0,DIEPTSIZ0 = 0x80004, DIEPCTL0 = 0x80498040
buf = 0xffb85fc0, pktcnt = 1, xfersize = 4+complete_rx: RX DMA done : ep = 2, rx bytes = 536/1024, is_short = 0, DOEPTSIZ = 0x1e8, remained bytes = 536
Here it says we completed the 536 bytes last transfer right?
+setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb85198,DOEPTSIZ = 0x801e8, DOEPCTL = 0x80098200
buf = 0xffb85198, pktcnt = 1, xfersize = 488
Why is this additional 488 bytes being queued? This is the real issue we need to debug.
cheers, -roger
+++++++ hangs here... -downloading of 258584 bytes finished -complete_rx: Next Rx request start... -setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x401000, DOEPCTL = 0x80098200
buf = 0xffb84f80, pktcnt = 8, xfersize = 4096Does this help explain anything ?!?!?!
Thanks, Steve
Another thing I noticed is that f_fastboot.c is not setting the right endpoint size for hight speed BULK_IN endpoint. I'll send out patches for that.
I am fine with these patches -- Thanks Steve
Those are now under review :-)
Thanks :)
cheers, -roger
On Thu, Apr 14, 2016 at 4:15 AM, Roger Quadros rogerq@ti.com wrote:
Steve,
On 13/04/16 04:55, Steve Rae wrote:
On Tue, Apr 12, 2016 at 6:50 AM, Roger Quadros rogerq@ti.com wrote:
Lukasz,
On 12/04/16 16:37, Lukasz Majewski wrote:
Hi Roger,
Hi,
On 12/04/16 14:19, Lukasz Majewski wrote:
Hi Tom, Mugunthan
> On Mon, Apr 11, 2016 at 05:04:56PM +0530, Mugunthan V N wrote: >> On Friday 08 April 2016 12:10 AM, Marek Vasut wrote: >>> On 04/07/2016 06:46 PM, Sam Protsenko wrote: >>>> On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski >>>> l.majewski@samsung.com wrote: >>>>> Hi Steve, >>>>> >>>>>> No -- I do not believe that this issue is caused by different >>>>>> fastboot (client) versions (the executable that runs on the >>>>>> host computer - Linux, Windows, Mac, etc.) >>>>>> I have personally attempted three (3) different versions, and >>>>>> the results are consistent. >>>>>> >>>>>> And no I don't think that I "am the only hope at fixing this >>>>>> proper" -- as you will see below, >>>>>> this" issue" seems to be unique to the "TI platforms" (... >>>>>> nobody else has stated they have an issue either way -- but I >>>>>> don't think many use this feature ....) >>>>>> So maybe someone with "TI platforms" could investigate this >>>>>> more thoroughly... >>>>>> >>>>>> HISTORY: >>>>>> >>>>>> The U-Boot code, up to Feb 25, worked properly on my Broadcom >>>>>> boards -- this code contains: >>>>>> req->length = rx_bytes_expected(); >>>>>> if (req->length < ep->maxpacket) >>>>>> req->length = ep->maxpacket; >>>>>> which aligned the remaining "rx_bytes_expected" to be aligned >>>>>> to the "ep->maxpacket" size. >>>>>> >>>>>> On Feb 25, there was a patch applied from >>>>>> dileep.katta@linaro.org which forces the remaining >>>>>> "rx_bytes_expected" to be aligned to the "wMaxPacketSize" size >>>>>> -- this patch broke all Broadcom boards: >>>>>> + if (rx_remain < maxpacket) { >>>>>> + rx_remain = maxpacket; >>>>>> + } else if (rx_remain % maxpacket != 0) { >>>>>> + rem = rx_remain % maxpacket; >>>>>> + rx_remain = rx_remain + (maxpacket - rem); >>>>>> + } >>>>>> >>>>>> After attempting to unsuccessfully contact Dileep, I requested >>>>>> that this patch be reverted -- because it broke my boards! >>>>>> (see the other email thread). >>>>>> >>>>>> Sam Protsenko semen.protsenko@linaro.org has stated that >>>>>> this Feb 25 change is required to make "fastboot work on TI >>>>>> platforms". >>>>>> >>>>>> Thus, >>>>>> - Broadcom boards require alignment to "ep->maxpacket" size >>>>>> - TI platforms require alignment to "wMaxPacketSize" size >>>>>> And we seem to be at a stale-mate. >>>>>> Unfortunately, I do not know enough about the USB internals to >>>>>> understand why this change breaks the Broadcom boards; or why >>>>>> it _is_ required on the TI platforms.... >>>>>> ( Is there any debugging that can be turned on to validate >>>>>> what is happening at the lower levels? ) >>>>> >>>>> I can only speak about DWC2 (from Synopsis) embedded at Samsung >>>>> boards. There are low level debugging registers (documented, >>>>> but not supposed to be used at normal operation), which give >>>>> you some impression regarding very low level events. >>>>> >>>>> DWC2 at Samsung is using those to work properly since we had >>>>> some problems with dwc2 IP blocks implementation on early >>>>> Samsung platforms :-). This approach works in u-boot up till >>>>> now. >>>>> >>>>> Another option is to use JTAG debugger (like Lauterbach) to >>>>> inspect state of this IP block. >>>>> >>>>>> ( Can anyone explain why "wMaxPacketSize" size would be >>>>>> required? -- my limited understanding of endpoints makes me >>>>>> think that "ep->maxpacket" size is actually the correct value! >>>>>> ) >>>>>> >>>>>> I asked Sam to submit a patch which conditionally applied the >>>>>> alignment to "wMaxPacketSize" size change -- he stated that he >>>>>> was too busy right now -- so I submitted this patch on his >>>>>> behalf (although he still needs to add the Kconfig for the TI >>>>>> platforms in order to make his boards work).... >>>>>> >>>>>> I suppose I could also propose a patch where the condition >>>>>> _removes_ this feature (and define it on the Broadcom boards) >>>>>> -- do we generally like "negated" conditionals? >>>>>> +#ifndef >>>>>>
CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_DISABLE_ALIGNMENT_WITH_WMAXPACKETSIZE
>>>>>> Please advise! >>>>>> >>>>>> Further, how does the U-Boot community respond to a change >>>>>> which breaks something which is already working? Doesn't the >>>>>> "author" of that change bear any responsibility on assisting >>>>>> to get "their" change working properly with "all" the existing >>>>>> boards? >>>>> >>>>> As we know the author of this change is not working at Linaro >>>>> anymore. >>>>> >>>>>> I'm getting the >>>>>> impression that "because the current code works for me", that >>>>>> I am not getting any assistance in resolving this issue -- >>>>>> which is why I suggested "reverting" this change back to the >>>>>> original code; that way, it would (politely?) force someone >>>>>> interested in "TI platforms" to step up and look into this.... >>>>>> >>>>>> Sorry for asking so many questions in one email -- but I'd >>>>>> appreciate answers.... >>>>>> ( I also apologize in advance for the "attitude" which is >>>>>> leaking into this email... ) >>>>>> Please tell me what I can do! I had working boards; now they >>>>>> are all broken -- and I don't how how to get them working >>>>>> again.... >>>>> >>>>> If you don't have enough time (and HW) for investigate the >>>>> issue, I think that Kconfig option with documentation entry is >>>>> the way to go. >>>>> >>>>> I hope that Sam don't have any objections with such approach. >>>>> >>>> >>>> If this commit doesn't break any platform -- I'm ok with that. >>>> If it breaks anything (TI boards particularly) -- I'd ask to >>>> revert it at once, as this is I believe not right way to do >>>> things. >>>> >>>> So Steve, please add >>>> CONFIG_USB_GADGET_FASTBOOT_DOWNLOAD_ALIGNMENT_REQUIRED option to >>>> all required defconfigs (except yours), so that your patch only >>>> fixes your platforms, but doesn't break any other platform at >>>> the same time. Also good thing to do after that is check options >>>> order in changed defconfigs with "make savedefconfig" rule. Both >>>> your current changes and appropriate lines in defconfigs should >>>> be committed as a single patch, so that it doesn't break >>>> anything and "git bisect" may be used to look for regressions. >>>> Also, really nice thing to do after all of this, is to use >>>> "./tools/buildman/buildman" tool to check all ARM boards for >>>> regressions after your patch (you should see that only your >>>> boards were changed). >>>> >>>> Ideally, we should check it on all boards (or at least on all >>>> UDC controllers supported in U-Boot) and figure out what is >>>> happening exactly. But I'm totally fine with hack if it fixes >>>> real problem on some platforms. I just ask you guys to not >>>> break anything else at the same time (although it surely takes >>>> much more effort, but still). >>> >>> I am totally not fine with hack, so please fix it such that both >>> platforms work without added config option. Thanks >>> >> >> The issue is already solved in Kernel with the patch [1]. May we >> can take a similar approach and fix the issue without having >> config options. >> >> [1]: >>
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0...
> > This seems reasonable. Can you do this, along with a follow-up > patch that sets it for DWC3? Thanks!
If I can add my two cents.
I believe that it would be worth to add some explanation into at least the commit message (like very short excerpt from respective User Manual or at least chapter number for further reference).
The patch in [1] is about setting USB request buffer aligned to MaxPacketSize. In f_fastboot.c case we allocate request buffer like so req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE, EP_BUFFER_SIZE);
where EP_BUFFER_SIZE is 4096 which is an integral multiple of 512 as well as 64. So I'm not sure how [1] is related to the subject and if it will fix anything.
I think the problem is more about the length of the last OUT transfer packet. Some controllers might not like that to be not an integral multiple of wMaxPacketSize and we need to ensure that.
My question was about the above sentence. I was wondering if there is any errata or user manual entry explicitly specifying that.
It is not an errata but stated in the dwc3 user manual like so
section 8.2.3.3 Buffer Size Rules and Zero-Length Packets
For OUT endpoints, the following rules apply: ■ The BUFSIZ field must be ≥ 1 byte. ■ The total size of a Buffer Descriptor must be a multiple of
MaxPacketSize
■ A received zero-length packet still requires a MaxPacketSize buffer.
Therefore, if the expected
amount of data to be received is a multiple of MaxPacketSize, software
should add MaxPacketSize
bytes to the buffer to sink a possible zero-length packet at the end of
the transfer.
This is being done in f_mass_storage.c in set_bulk_out_req_length(). Doing that shouldn't affect other controllers.
So we need to really fix commit 9e4b510.
Yes -- this is the one that causes my stalling issue: I'll copy some debug output from another email thread:
Lukasz: As per your suggestion, I turned on the following: diff --git a/drivers/usb/gadget/dwc2_udc_otg.c b/drivers/usb/gadget/dwc2_udc_otg.c index 5d53440..763c6d9 100644 --- a/drivers/usb/gadget/dwc2_udc_otg.c +++ b/drivers/usb/gadget/dwc2_udc_otg.c @@ -40,11 +40,11 @@
#define OTG_DMA_MODE 1
-#define DEBUG_SETUP 0 -#define DEBUG_EP0 0 -#define DEBUG_ISR 0 -#define DEBUG_OUT_EP 0 -#define DEBUG_IN_EP 0 +#define DEBUG_SETUP 1 +#define DEBUG_EP0 1 +#define DEBUG_ISR 1 +#define DEBUG_OUT_EP 1 +#define DEBUG_IN_EP 1
and captured the logs of the "last transactions..." (the "-" is with the Feb 25 Patch removed, the "+" is with the Feb 25 Patch applied....)
*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000 EP2-OUT : DOEPINT = 0x2011 complete_rx: RX DMA done : ep = 2, rx bytes = 4096/4096, is_short = 0, DOEPTSIZ = 0x0, remained bytes = 4096 complete_rx: Next Rx request start... setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x401000, DOEPCTL = 0x80098200 buf = 0xffb84f80, pktcnt = 8, xfersize = 4096
*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000 EP2-OUT : DOEPINT = 0x2011 complete_rx: RX DMA done : ep = 2, rx bytes = 4096/4096, is_short = 0, DOEPTSIZ = 0x0, remained bytes = 4096 complete_rx: Next Rx request start... -setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x100218, DOEPCTL = 0x80098200
buf = 0xffb84f80, pktcnt = 2, xfersize = 536+setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x100400, DOEPCTL = 0x80098200
buf = 0xffb84f80, pktcnt = 2, xfersize = 1024This part looks fine as we're rounding up 536 to 1024 for 512 byte alignment.
*** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000 EP2-OUT : DOEPINT = 0x2011 -complete_rx: RX DMA done : ep = 2, rx bytes = 536/536, is_short = 0, DOEPTSIZ = 0x0, remained bytes = 536
the "rx bytes = 536/536" (in the working code)... so maybe the driver "knows" that there are no more bytes to come ?!?!?!?
-dwc2_queue: ep_is_in, DWC2_UDC_OTG_GINTSTS=0x14008028 -setdma_tx:EP1 TX DMA start : DIEPDMA0 = 0xffb85fc0,DIEPTSIZ0 = 0x80004, DIEPCTL0 = 0x80498040
buf = 0xffb85fc0, pktcnt = 1, xfersize = 4+complete_rx: RX DMA done : ep = 2, rx bytes = 536/1024, is_short = 0, DOEPTSIZ = 0x1e8, remained bytes = 536
Here it says we completed the 536 bytes last transfer right?
the "rx bytes = 536/1024" (in the NON-working code)... so maybe the driver "thinks" that there are still 1024-536=488 bytes to come ?!?!?!?
+setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb85198,DOEPTSIZ = 0x801e8, DOEPCTL = 0x80098200
buf = 0xffb85198, pktcnt = 1, xfersize = 488Why is this additional 488 bytes being queued? This is the real issue we need to debug.
so maybe because we told the driver to "expect" 1024 bytes, when if fact we only "expect" 536 ?!?!?!? I really don't know, and have not spent any time inside the driver code -- because it was always working properly......
cheers, -roger
+++++++ hangs here... -downloading of 258584 bytes finished -complete_rx: Next Rx request start... -setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x401000, DOEPCTL = 0x80098200
buf = 0xffb84f80, pktcnt = 8, xfersize = 4096Does this help explain anything ?!?!?!
Thanks, Steve
Another thing I noticed is that f_fastboot.c is not setting the right endpoint size for hight speed BULK_IN endpoint. I'll send out patches for that.
I am fine with these patches -- Thanks Steve
Those are now under review :-)
Thanks :)
cheers, -roger
participants (9)
-
B, Ravi -
Lukasz Majewski -
Marek Vasut -
Mugunthan V N -
Roger Quadros -
Sam Protsenko -
Steve Rae -
Steve Rae -
Tom Rini