[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Steve Rae]

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;
 }
 
-- 
1.8.5

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Marek Vasut]

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


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Steve Rae]

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"
> > +     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;
> >  }
> >
> >
>
>
> --
> Best regards,
> Marek Vasut

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Lukasz Majewski]

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 "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;
> > >  }
> > >
> > >
> >
> >
> > --
> > Best regards,
> > Marek Vasut



-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

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

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Marek Vasut]

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

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Steve Rae]

No -- I do not believe that this issue is caused by different fastboot
(client) versions (the executable that runs on the host computer -
Linux, Windows, Mac, etc.)
I have personally attempted three (3) different versions, and the
results are consistent.

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

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[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


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Steve Rae]

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
>

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[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

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Lukasz Majewski]

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
> 



-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Lukasz Majewski]

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

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



-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Sam Protsenko]

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

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Steve Rae]

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
>

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Marek Vasut]

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

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Sam Protsenko]

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

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Steve Rae]

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 at linaro.org>
> > wrote:
> >>
> >> On Thu, Apr 7, 2016 at 10:36 AM, Lukasz Majewski <
> l.majewski at 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
> >
> >
>

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Sam Protsenko]

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/fastboot_protocol.txt
[2] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=36f84ffb45c75ff10442a9f8f2fd91dcf6c6f5dd

>
>>
>> This way you fix your boards (that you know need to be fixed) but keep
>> rest of boards intact. If some other boards need to be fixed too --
>> they will be fixed further by some folks who need that.
>>
>> > So, if you would send me a list of which defconfig files need to add
>> > this
>> > line, I'll update it....
>> > OR (I would prefer) you could submit a v3 which includes the boards that
>> > you
>> > know require this capability!
>> >
>>
>> I'm not gonna send this patch for you, sorry. I just don't need that,
>> and I'm not the author of original patch, so it's just not my concern.
>> I can't even test it for boards that actually broken.
>>
>> > Let me know,
>> > Thanks, Steve
>> >
>> >>
>> >>  Also
>> >> good thing to do after that is check options order in changed
>> >> defconfigs with "make savedefconfig" rule. Both your current changes
>> >> and appropriate lines in defconfigs should be committed as a single
>> >> patch, so that it doesn't break anything and "git bisect" may be used
>> >> to look for regressions. Also, really nice thing to do after all of
>> >> this, is to use "./tools/buildman/buildman" tool to check all ARM
>> >> boards for regressions after your patch (you should see that only your
>> >> boards were changed).
>> >
>> >
>> > yup -- I use buildman almost exclusively....
>> >
>> >>
>> >> Ideally, we should check it on all boards (or at least on all UDC
>> >> controllers supported in U-Boot) and figure out what is happening
>> >> exactly. But I'm totally fine with hack if it fixes real problem on
>> >> some platforms. I just ask you guys to not break anything else at the
>> >> same time (although it surely takes much more effort, but still).
>> >
>> >
>> > I'm confused (again) -- why are you asking: "you guys to not break
>> > anything
>> > else"...
>> > IT IS ALREADY BROKEN, it is broken right now, and has been broken since
>> > Feb
>> > 25 !
>> >
>> > Please fix this!
>> >
>>
>> ...So let's fix half of platforms and break the other half of
>> platforms altogether? It's not for me to decide, I'm not the
>> maintainer. But it just doesn't feel right to me.
>>
>> I understand your concern, and I can help you test your patches on my
>> boards any time and also run some debug patches to see the difference.
>> But I can't fix it for you. Also I'm not sure that your patch would be
>> merged in current shape (it's basically a hack). So if I were you I'd
>> try to figure out the root cause of this issue by comparing results of
>> some debug patches and tests, by running them on your boards (where
>> fastboot is broken) and on some boards where fastboot is working.
>> Maybe running wireshark in both cases can help to understand why it's
>> happening. From my POV it was a good assumption (made by someone
>> earlier) that possible reason is different UDC controllers (I have
>> DWC3 on my TI boards).
>>
>> >>
>> >> >> Thanks, Steve
>> >> >>
>> >> >> On Wed, Apr 6, 2016 at 4:01 AM, Marek Vasut <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
>> >
>> >
>
>

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Steve Rae]

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

DWC2


>
> [1]
> https://android.googlesource.com/platform/system/core/+/master/fastboot/fastboot_protocol.txt
> [2]
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=36f84ffb45c75ff10442a9f8f2fd91dcf6c6f5dd
>
> >
> >>
> >> This way you fix your boards (that you know need to be fixed) but keep
> >> rest of boards intact. If some other boards need to be fixed too --
> >> they will be fixed further by some folks who need that.
> >>
> >> > So, if you would send me a list of which defconfig files need to add
> >> > this
> >> > line, I'll update it....
> >> > OR (I would prefer) you could submit a v3 which includes the boards
> that
> >> > you
> >> > know require this capability!
> >> >
> >>
> >> I'm not gonna send this patch for you, sorry. I just don't need that,
> >> and I'm not the author of original patch, so it's just not my concern.
> >> I can't even test it for boards that actually broken.
> >>
> >> > Let me know,
> >> > Thanks, Steve
> >> >
> >> >>
> >> >>  Also
> >> >> good thing to do after that is check options order in changed
> >> >> defconfigs with "make savedefconfig" rule. Both your current changes
> >> >> and appropriate lines in defconfigs should be committed as a single
> >> >> patch, so that it doesn't break anything and "git bisect" may be used
> >> >> to look for regressions. Also, really nice thing to do after all of
> >> >> this, is to use "./tools/buildman/buildman" tool to check all ARM
> >> >> boards for regressions after your patch (you should see that only
> your
> >> >> boards were changed).
> >> >
> >> >
> >> > yup -- I use buildman almost exclusively....
> >> >
> >> >>
> >> >> Ideally, we should check it on all boards (or at least on all UDC
> >> >> controllers supported in U-Boot) and figure out what is happening
> >> >> exactly. But I'm totally fine with hack if it fixes real problem on
> >> >> some platforms. I just ask you guys to not break anything else at the
> >> >> same time (although it surely takes much more effort, but still).
> >> >
> >> >
> >> > I'm confused (again) -- why are you asking: "you guys to not break
> >> > anything
> >> > else"...
> >> > IT IS ALREADY BROKEN, it is broken right now, and has been broken
> since
> >> > Feb
> >> > 25 !
> >> >
> >> > Please fix this!
> >> >
> >>
> >> ...So let's fix half of platforms and break the other half of
> >> platforms altogether? It's not for me to decide, I'm not the
> >> maintainer. But it just doesn't feel right to me.
> >>
> >> I understand your concern, and I can help you test your patches on my
> >> boards any time and also run some debug patches to see the difference.
> >> But I can't fix it for you. Also I'm not sure that your patch would be
> >> merged in current shape (it's basically a hack). So if I were you I'd
> >> try to figure out the root cause of this issue by comparing results of
> >> some debug patches and tests, by running them on your boards (where
> >> fastboot is broken) and on some boards where fastboot is working.
> >> Maybe running wireshark in both cases can help to understand why it's
> >> happening. From my POV it was a good assumption (made by someone
> >> earlier) that possible reason is different UDC controllers (I have
> >> DWC3 on my TI boards).
> >>
> >> >>
> >> >> >> Thanks, Steve
> >> >> >>
> >> >> >> On Wed, Apr 6, 2016 at 4:01 AM, Marek Vasut <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
> >> >
> >> >
> >
> >
>

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Tom Rini]

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160408/da1480e6/attachment.sig>

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Mugunthan V N]

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=0b2d2bbade59ab2067f326d6dbc2628bee227fd5

Regards
Mugunthan V N

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[B, Ravi]

Tom

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

> On Fri, Apr 08, 2016 at 02:11:48AM +0300, Sam Protsenko wrote:
> [snip]
> > 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 

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Tom Rini]

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=0b2d2bbade59ab2067f326d6dbc2628bee227fd5

This seems reasonable.  Can you do this, along with a follow-up patch
that sets it for DWC3?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160411/2a4031a8/attachment.sig>

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Lukasz Majewski]

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=0b2d2bbade59ab2067f326d6dbc2628bee227fd5
> 
> This seems reasonable.  Can you do this, along with a follow-up patch
> that sets it for DWC3?  Thanks!

If I can add my two cents.


I believe that it would be worth to add some explanation into at least
the commit message (like very short excerpt from respective User Manual
or at least chapter number for further reference).

> 



-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Roger Quadros]

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=0b2d2bbade59ab2067f326d6dbc2628bee227fd5
>>
>> This seems reasonable.  Can you do this, along with a follow-up patch
>> that sets it for DWC3?  Thanks!
> 
> If I can add my two cents.
> 
> 
> I believe that it would be worth to add some explanation into at least
> the commit message (like very short excerpt from respective User Manual
> or at least chapter number for further reference).

The patch in [1] is about setting USB request buffer aligned to MaxPacketSize.
In f_fastboot.c case we allocate request buffer like so
	req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE, EP_BUFFER_SIZE);

where EP_BUFFER_SIZE is 4096 which is an integral multiple of 512 as well as 64.
So I'm not sure how [1] is related to the subject and if it will fix anything.

I think the problem is more about the length of the last OUT transfer packet.
Some controllers might not like that to be not an integral multiple of wMaxPacketSize
and we need to ensure that. 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

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Lukasz Majewski]

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=0b2d2bbade59ab2067f326d6dbc2628bee227fd5
> >>
> >> This seems reasonable.  Can you do this, along with a follow-up
> >> patch that sets it for DWC3?  Thanks!
> > 
> > If I can add my two cents.
> > 
> > 
> > I believe that it would be worth to add some explanation into at
> > least the commit message (like very short excerpt from respective
> > User Manual or at least chapter number for further reference).
> 
> The patch in [1] is about setting USB request buffer aligned to
> MaxPacketSize. In f_fastboot.c case we allocate request buffer like so
> 	req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE,
> EP_BUFFER_SIZE);
> 
> where EP_BUFFER_SIZE is 4096 which is an integral multiple of 512 as
> well as 64. So I'm not sure how [1] is related to the subject and if
> it will fix anything.
> 
> I think the problem is more about the length of the last OUT transfer
> packet. Some controllers might not like that to be not an integral
> multiple of wMaxPacketSize and we need to ensure that. 

My question was about the above sentence. I was wondering if there is
any errata or user manual entry explicitly specifying that.

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



-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Roger Quadros]

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=0b2d2bbade59ab2067f326d6dbc2628bee227fd5
>>>>
>>>> This seems reasonable.  Can you do this, along with a follow-up
>>>> patch that sets it for DWC3?  Thanks!
>>>
>>> If I can add my two cents.
>>>
>>>
>>> I believe that it would be worth to add some explanation into at
>>> least the commit message (like very short excerpt from respective
>>> User Manual or at least chapter number for further reference).
>>
>> The patch in [1] is about setting USB request buffer aligned to
>> MaxPacketSize. In f_fastboot.c case we allocate request buffer like so
>> 	req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE,
>> EP_BUFFER_SIZE);
>>
>> where EP_BUFFER_SIZE is 4096 which is an integral multiple of 512 as
>> well as 64. So I'm not sure how [1] is related to the subject and if
>> it will fix anything.
>>
>> I think the problem is more about the length of the last OUT transfer
>> packet. Some controllers might not like that to be not an integral
>> multiple of wMaxPacketSize and we need to ensure that. 
> 
> My question was about the above sentence. I was wondering if there is
> any errata or user manual entry explicitly specifying that.

It is not an errata but stated in the dwc3 user manual like so

section 8.2.3.3	Buffer Size Rules and Zero-Length Packets

For OUT endpoints, the following rules apply:
? The BUFSIZ field must be ? 1 byte.
? The total size of a Buffer Descriptor must be a multiple of MaxPacketSize
? A received zero-length packet still requires a MaxPacketSize buffer. Therefore, if the expected
amount of data to be received is a multiple of MaxPacketSize, software should add MaxPacketSize
bytes to the buffer to sink a possible zero-length packet at the end of the transfer.

> 
>> This is being
>> done in f_mass_storage.c in set_bulk_out_req_length(). Doing that
>> shouldn't affect other controllers.
>>
>> So we need to really fix commit 9e4b510.
>>
>> 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

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Steve Rae]

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=0b2d2bbade59ab2067f326d6dbc2628bee227fd5
>>>>>
>>>>> This seems reasonable.  Can you do this, along with a follow-up
>>>>> patch that sets it for DWC3?  Thanks!
>>>>
>>>> If I can add my two cents.
>>>>
>>>>
>>>> I believe that it would be worth to add some explanation into at
>>>> least the commit message (like very short excerpt from respective
>>>> User Manual or at least chapter number for further reference).
>>>
>>> The patch in [1] is about setting USB request buffer aligned to
>>> MaxPacketSize. In f_fastboot.c case we allocate request buffer like so
>>>      req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE,
>>> EP_BUFFER_SIZE);
>>>
>>> where EP_BUFFER_SIZE is 4096 which is an integral multiple of 512 as
>>> well as 64. So I'm not sure how [1] is related to the subject and if
>>> it will fix anything.
>>>
>>> I think the problem is more about the length of the last OUT transfer
>>> packet. Some controllers might not like that to be not an integral
>>> multiple of wMaxPacketSize and we need to ensure that.
>>
>> My question was about the above sentence. I was wondering if there is
>> any errata or user manual entry explicitly specifying that.
>
> It is not an errata but stated in the dwc3 user manual like so
>
> section 8.2.3.3 Buffer Size Rules and Zero-Length Packets
>
> For OUT endpoints, the following rules apply:
> ? The BUFSIZ field must be ? 1 byte.
> ? The total size of a Buffer Descriptor must be a multiple of MaxPacketSize
> ? A received zero-length packet still requires a MaxPacketSize buffer. Therefore, if the expected
> amount of data to be received is a multiple of MaxPacketSize, software should add MaxPacketSize
> bytes to the buffer to sink a possible zero-length packet at the end of the transfer.
>
>>
>>> This is being
>>> done in f_mass_storage.c in set_bulk_out_req_length(). Doing that
>>> shouldn't affect other controllers.
>>>
>>> So we need to really fix commit 9e4b510.

Yes -- this is the one that causes my stalling issue:
I'll copy some debug output from another email thread:

Lukasz:
As per your suggestion, I turned on the following:
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

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Roger Quadros]

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=0b2d2bbade59ab2067f326d6dbc2628bee227fd5
>>>>>>
>>>>>> This seems reasonable.  Can you do this, along with a follow-up
>>>>>> patch that sets it for DWC3?  Thanks!
>>>>>
>>>>> If I can add my two cents.
>>>>>
>>>>>
>>>>> I believe that it would be worth to add some explanation into at
>>>>> least the commit message (like very short excerpt from respective
>>>>> User Manual or at least chapter number for further reference).
>>>>
>>>> The patch in [1] is about setting USB request buffer aligned to
>>>> MaxPacketSize. In f_fastboot.c case we allocate request buffer like so
>>>>      req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE,
>>>> EP_BUFFER_SIZE);
>>>>
>>>> where EP_BUFFER_SIZE is 4096 which is an integral multiple of 512 as
>>>> well as 64. So I'm not sure how [1] is related to the subject and if
>>>> it will fix anything.
>>>>
>>>> I think the problem is more about the length of the last OUT transfer
>>>> packet. Some controllers might not like that to be not an integral
>>>> multiple of wMaxPacketSize and we need to ensure that.
>>>
>>> My question was about the above sentence. I was wondering if there is
>>> any errata or user manual entry explicitly specifying that.
>>
>> It is not an errata but stated in the dwc3 user manual like so
>>
>> section 8.2.3.3 Buffer Size Rules and Zero-Length Packets
>>
>> For OUT endpoints, the following rules apply:
>> ? The BUFSIZ field must be ? 1 byte.
>> ? The total size of a Buffer Descriptor must be a multiple of MaxPacketSize
>> ? A received zero-length packet still requires a MaxPacketSize buffer. Therefore, if the expected
>> amount of data to be received is a multiple of MaxPacketSize, software should add MaxPacketSize
>> bytes to the buffer to sink a possible zero-length packet at the end of the transfer.
>>
>>>
>>>> This is being
>>>> done in f_mass_storage.c in set_bulk_out_req_length(). Doing that
>>>> shouldn't affect other controllers.
>>>>
>>>> So we need to really fix commit 9e4b510.
> 
> Yes -- this is the one that causes my stalling issue:
> I'll copy some debug output from another email thread:
> 
> Lukasz:
> As per your suggestion, I turned on the following:
> 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 = 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

[U-Boot] [PATCH v2] fastboot: OUT transaction length must be aligned to wMaxPacketSize

[Steve Rae]

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=0b2d2bbade59ab2067f326d6dbc2628bee227fd5
> >>>>>>
> >>>>>> This seems reasonable.  Can you do this, along with a follow-up
> >>>>>> patch that sets it for DWC3?  Thanks!
> >>>>>
> >>>>> If I can add my two cents.
> >>>>>
> >>>>>
> >>>>> I believe that it would be worth to add some explanation into at
> >>>>> least the commit message (like very short excerpt from respective
> >>>>> User Manual or at least chapter number for further reference).
> >>>>
> >>>> The patch in [1] is about setting USB request buffer aligned to
> >>>> MaxPacketSize. In f_fastboot.c case we allocate request buffer like so
> >>>>      req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE,
> >>>> EP_BUFFER_SIZE);
> >>>>
> >>>> where EP_BUFFER_SIZE is 4096 which is an integral multiple of 512 as
> >>>> well as 64. So I'm not sure how [1] is related to the subject and if
> >>>> it will fix anything.
> >>>>
> >>>> I think the problem is more about the length of the last OUT transfer
> >>>> packet. Some controllers might not like that to be not an integral
> >>>> multiple of wMaxPacketSize and we need to ensure that.
> >>>
> >>> My question was about the above sentence. I was wondering if there is
> >>> any errata or user manual entry explicitly specifying that.
> >>
> >> It is not an errata but stated in the dwc3 user manual like so
> >>
> >> section 8.2.3.3 Buffer Size Rules and Zero-Length Packets
> >>
> >> For OUT endpoints, the following rules apply:
> >> ? The BUFSIZ field must be ? 1 byte.
> >> ? The total size of a Buffer Descriptor must be a multiple of
> MaxPacketSize
> >> ? A received zero-length packet still requires a MaxPacketSize buffer.
> Therefore, if the expected
> >> amount of data to be received is a multiple of MaxPacketSize, software
> should add MaxPacketSize
> >> bytes to the buffer to sink a possible zero-length packet at the end of
> the transfer.
> >>
> >>>
> >>>> This is being
> >>>> done in f_mass_storage.c in set_bulk_out_req_length(). Doing that
> >>>> shouldn't affect other controllers.
> >>>>
> >>>> So we need to really fix commit 9e4b510.
> >
> > Yes -- this is the one that causes my stalling issue:
> > I'll copy some debug output from another email thread:
> >
> > Lukasz:
> > As per your suggestion, I turned on the following:
> > 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
>

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 = 488
>
> Why 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 = 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
>