[RFT PATCH 0/4] usb: dwc2: Turn on uframe sched everywhere

[RFT PATCH 0/4] usb: dwc2: Turn on uframe sched everywhere

[Douglas Anderson]

While looking at a dwc2 recently, I noticed that some platforms still
have the uframe scheduler off.  As far as I know, nothing good can
come out of having the uframe scheduler off.  Let's turn it on
everywhere.  Assuming this all works out then some time in the future
we can gut all the old code that handles the old non-uframe scheduler
and make everything simpler.

NOTE: I don't have access to any of the platforms that I'm changing
here, so I'm hoping to see a Tested-by from those on the list.


Douglas Anderson (4):
  usb: dwc2: Turn on uframe_sched on "bcm" platforms
  usb: dwc2: Turn on uframe_sched on "his" platforms
  usb: dwc2: Turn on uframe_sched on "amlogic" platforms
  usb: dwc2: Turn on uframe_sched on "stm32f4x9_fsotg" platforms

 drivers/usb/dwc2/params.c | 4 ----
 1 file changed, 4 deletions(-)

-- 
2.18.0.399.gad0ab374a1-goog

[RFT PATCH 1/4] usb: dwc2: Turn on uframe_sched on "bcm" platforms

[Douglas Anderson]

There's no reason to have the uframe scheduler off on dwc2.  Running
with uframe_sched = False is equivalent to saying "I don't want to run
the correct code, I want to run the old and incorrect code".

The uframe scheduler has been off on Broadcom since commit
58b179dcf28c ("staging: dwc2: disable uframe_sched on the bcm2835").
Since then there have been many many improvements, notably the commit
9f9f09b048f5 ("usb: dwc2: host: Totally redo the microframe
scheduler")

Presumably if everyone is good w/ the uframe_sched turned back on we
can kill all the old and crufty non-uframe sched code.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/usb/dwc2/params.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index af075d4da895..14b8742e62fa 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -47,7 +47,6 @@ static void dwc2_set_bcm_params(struct dwc2_hsotg *hsotg)
 	p->max_transfer_size = 65535;
 	p->max_packet_count = 511;
 	p->ahbcfg = 0x10;
-	p->uframe_sched = false;
 }
 
 static void dwc2_set_his_params(struct dwc2_hsotg *hsotg)
-- 
2.18.0.399.gad0ab374a1-goog

[RFT PATCH 2/4] usb: dwc2: Turn on uframe_sched on "his" platforms

[Douglas Anderson]

There's no reason to have the uframe scheduler off on dwc2.  Running
with uframe_sched = False is equivalent to saying "I don't want to run
the correct code, I want to run the old and incorrect code".

The uframe scheduler has been off on HiSilicon since commit
37dd9d65cc41 ("usb: dwc2: add support of hi6220").  Since then there
have been many many improvements, notably the commit 9f9f09b048f5
("usb: dwc2: host: Totally redo the microframe scheduler")

Presumably if everyone is good w/ the uframe_sched turned back on we
can kill all the old and crufty non-uframe sched code.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/usb/dwc2/params.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 14b8742e62fa..db97bb16ecc4 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -67,7 +67,6 @@ static void dwc2_set_his_params(struct dwc2_hsotg *hsotg)
 	p->reload_ctl = false;
 	p->ahbcfg = GAHBCFG_HBSTLEN_INCR16 <<
 		GAHBCFG_HBSTLEN_SHIFT;
-	p->uframe_sched = false;
 	p->change_speed_quirk = true;
 	p->power_down = false;
 }
-- 
2.18.0.399.gad0ab374a1-goog

Re: [RFT PATCH 2/4] usb: dwc2: Turn on uframe_sched on "his" platforms

[John Stultz]

On Fri, Jul 6, 2018 at 9:21 AM, Douglas Anderson <dianders@chromium.org> wrote:
> There's no reason to have the uframe scheduler off on dwc2.  Running
> with uframe_sched = False is equivalent to saying "I don't want to run
> the correct code, I want to run the old and incorrect code".
>
> The uframe scheduler has been off on HiSilicon since commit
> 37dd9d65cc41 ("usb: dwc2: add support of hi6220").  Since then there
> have been many many improvements, notably the commit 9f9f09b048f5
> ("usb: dwc2: host: Totally redo the microframe scheduler")
>
> Presumably if everyone is good w/ the uframe_sched turned back on we
> can kill all the old and crufty non-uframe sched code.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Not sure exactly how to exercise this, but in my initial testing with
usb on HiKey, this seems to work ok.

Tested-by: John Stultz <john.stultz@linaro.org>

thanks
-john

[RFT PATCH 3/4] usb: dwc2: Turn on uframe_sched on "amlogic" platforms

[Douglas Anderson]

There's no reason to have the uframe scheduler off on dwc2.  Running
with uframe_sched = False is equivalent to saying "I don't want to run
the correct code, I want to run the old and incorrect code".

The uframe scheduler has been off on Amlogic since commit f94310ac076e
("usb: dwc2: add support for Meson8b and GXBB SoCs").  While this was
after most of the recent improvements, notably the commit 9f9f09b048f5
("usb: dwc2: host: Totally redo the microframe scheduler"), presumably
the parameters were copied from another platform and the uframe
scheduler wasn't tried.

Presumably if everyone is good w/ the uframe_sched turned back on we
can kill all the old and crufty non-uframe sched code.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/usb/dwc2/params.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index db97bb16ecc4..93380f7c32b4 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -110,7 +110,6 @@ static void dwc2_set_amlogic_params(struct dwc2_hsotg *hsotg)
 	p->phy_type = DWC2_PHY_TYPE_PARAM_UTMI;
 	p->ahbcfg = GAHBCFG_HBSTLEN_INCR8 <<
 		GAHBCFG_HBSTLEN_SHIFT;
-	p->uframe_sched = false;
 }
 
 static void dwc2_set_amcc_params(struct dwc2_hsotg *hsotg)
-- 
2.18.0.399.gad0ab374a1-goog

[RFT PATCH 4/4] usb: dwc2: Turn on uframe_sched on "stm32f4x9_fsotg" platforms

[Douglas Anderson]

There's no reason to have the uframe scheduler off on dwc2.  Running
with uframe_sched = False is equivalent to saying "I don't want to run
the correct code, I want to run the old and incorrect code".

The uframe scheduler has been off on stm32f4x9_fsotg since commit
e35b135055e2 ("usb: dwc2: Add support for STM32F429/439/469 USB OTG
HS/FS in FS mode (internal PHY)").  That commit is pretty recent, so
it's unclear to me why the uframe scheduler was left off.  Hopefully
it's because someone copied it from other parameters and didn't think
to try it?

Presumably if everyone is good w/ the uframe_sched turned back on we
can kill all the old and crufty non-uframe sched code.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/usb/dwc2/params.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 93380f7c32b4..7be35bcde713 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -131,7 +131,6 @@ static void dwc2_set_stm32f4x9_fsotg_params(struct dwc2_hsotg *hsotg)
 	p->max_packet_count = 256;
 	p->phy_type = DWC2_PHY_TYPE_PARAM_FS;
 	p->i2c_enable = false;
-	p->uframe_sched = false;
 	p->activate_stm_fs_transceiver = true;
 }
 
-- 
2.18.0.399.gad0ab374a1-goog

Re: [RFT PATCH 0/4] usb: dwc2: Turn on uframe sched everywhere

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

Douglas Anderson <dianders@chromium.org> writes:

> While looking at a dwc2 recently, I noticed that some platforms still
> have the uframe scheduler off.  As far as I know, nothing good can
> come out of having the uframe scheduler off.  Let's turn it on
> everywhere.  Assuming this all works out then some time in the future
> we can gut all the old code that handles the old non-uframe scheduler
> and make everything simpler.
>
> NOTE: I don't have access to any of the platforms that I'm changing
> here, so I'm hoping to see a Tested-by from those on the list.
>
>
> Douglas Anderson (4):
>   usb: dwc2: Turn on uframe_sched on "bcm" platforms
>   usb: dwc2: Turn on uframe_sched on "his" platforms
>   usb: dwc2: Turn on uframe_sched on "amlogic" platforms
>   usb: dwc2: Turn on uframe_sched on "stm32f4x9_fsotg" platforms

care to resend without RFT prefix and with collected acks, tested-bys, etc?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [RFT PATCH 0/4] usb: dwc2: Turn on uframe sched everywhere

[Doug Anderson]

Hi,

On Thu, Jul 26, 2018 at 3:59 AM, Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
> Douglas Anderson <dianders@chromium.org> writes:
>
>> While looking at a dwc2 recently, I noticed that some platforms still
>> have the uframe scheduler off.  As far as I know, nothing good can
>> come out of having the uframe scheduler off.  Let's turn it on
>> everywhere.  Assuming this all works out then some time in the future
>> we can gut all the old code that handles the old non-uframe scheduler
>> and make everything simpler.
>>
>> NOTE: I don't have access to any of the platforms that I'm changing
>> here, so I'm hoping to see a Tested-by from those on the list.
>>
>>
>> Douglas Anderson (4):
>>   usb: dwc2: Turn on uframe_sched on "bcm" platforms
>>   usb: dwc2: Turn on uframe_sched on "his" platforms
>>   usb: dwc2: Turn on uframe_sched on "amlogic" platforms
>>   usb: dwc2: Turn on uframe_sched on "stm32f4x9_fsotg" platforms
>
> care to resend without RFT prefix and with collected acks, tested-bys, etc?

Done with a prefix of 'REPOST' to hopefully make it obvious nothing
changed between the RFT and this version.  There were only two
test-by, but as mentioned in the updated cover letter:

Since there should be nothing platform specific about the uFrame
scheduler presumably this should mean we're good to go?  Specificaly
note that in at least one of the "Tested-by" reports this patch caused
a notable improvement.


BTW: I'm pleased to have you apply the patches, but I notice that
get_maintainer no longer flags you as the person to post dwc2 patches
to.  From linux-next:

$ ./scripts/get_maintainer.pl -f drivers/usb/dwc2
Minas Harutyunyan <hminas@synopsys.com> (maintainer:DESIGNWARE USB2
DRD IP DRIVER)
Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM)
linux-usb@vger.kernel.org (open list:DESIGNWARE USB2 DRD IP DRIVER)
linux-kernel@vger.kernel.org (open list)


-Doug

Re: [RFT PATCH 0/4] usb: dwc2: Turn on uframe sched everywhere

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2178 bytes --]


Hi,

Doug Anderson <dianders@chromium.org> writes:
> On Thu, Jul 26, 2018 at 3:59 AM, Felipe Balbi
> <felipe.balbi@linux.intel.com> wrote:
>> Douglas Anderson <dianders@chromium.org> writes:
>>
>>> While looking at a dwc2 recently, I noticed that some platforms still
>>> have the uframe scheduler off.  As far as I know, nothing good can
>>> come out of having the uframe scheduler off.  Let's turn it on
>>> everywhere.  Assuming this all works out then some time in the future
>>> we can gut all the old code that handles the old non-uframe scheduler
>>> and make everything simpler.
>>>
>>> NOTE: I don't have access to any of the platforms that I'm changing
>>> here, so I'm hoping to see a Tested-by from those on the list.
>>>
>>>
>>> Douglas Anderson (4):
>>>   usb: dwc2: Turn on uframe_sched on "bcm" platforms
>>>   usb: dwc2: Turn on uframe_sched on "his" platforms
>>>   usb: dwc2: Turn on uframe_sched on "amlogic" platforms
>>>   usb: dwc2: Turn on uframe_sched on "stm32f4x9_fsotg" platforms
>>
>> care to resend without RFT prefix and with collected acks, tested-bys, etc?
>
> Done with a prefix of 'REPOST' to hopefully make it obvious nothing
> changed between the RFT and this version.  There were only two
> test-by, but as mentioned in the updated cover letter:
>
> Since there should be nothing platform specific about the uFrame
> scheduler presumably this should mean we're good to go?  Specificaly
> note that in at least one of the "Tested-by" reports this patch caused
> a notable improvement.
>
>
> BTW: I'm pleased to have you apply the patches, but I notice that
> get_maintainer no longer flags you as the person to post dwc2 patches
> to.  From linux-next:
>
> $ ./scripts/get_maintainer.pl -f drivers/usb/dwc2
> Minas Harutyunyan <hminas@synopsys.com> (maintainer:DESIGNWARE USB2
> DRD IP DRIVER)
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM)
> linux-usb@vger.kernel.org (open list:DESIGNWARE USB2 DRD IP DRIVER)
> linux-kernel@vger.kernel.org (open list)

I need to add dwc2 to the gadget framework part of MAINTAINERS. I'll do
that at some point.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [RFT PATCH 0/4] usb: dwc2: Turn on uframe sched everywhere

[Stefan Wahren]

Hi Doug,

since Stephen isn't BCM2835 maintainer anymore, please keep me in the loop.

I'm not sure how to test this. Is there some kind of test scenario 
(don't have a USB analyzer)?

Best regards
Stefan

Re: [RFT PATCH 0/4] usb: dwc2: Turn on uframe sched everywhere

[Doug Anderson]

Hi,

On Tue, Jul 10, 2018 at 6:57 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> Hi Doug,
>
> since Stephen isn't BCM2835 maintainer anymore, please keep me in the loop.

Sorry for missing you.  :(  I wonder if you should add yourself to the
MAINTAINERS file as a reviewer for dwc2 so you can get CCed always?


> I'm not sure how to test this. Is there some kind of test scenario (don't
> have a USB analyzer)?

I guess it'll depend on what USB devices you have sitting around.  The
more USB devices you have plugged in then the more likely the dumb
scheduler will get things wrong and a device won't like you.  You
should also see significant compatibility improvements when you have
low/full speed devices plugged into high speed USB hubs.

IIRC when testing I did things like:
- In general, try to have lots of things plugged in.
- Plug many different keyboards / mice in.  See if keys are dropped /
mouse is jerky.
- Try USB audio (ideally full speed, not high speed).  How is the audio?
- If you're brave, try a USB webcam.  How is the video?


If you're trying a USB webcam w/ dwc2, maybe add in patches mentioned
in <https://bugs.chromium.org/p/chromium/issues/detail?id=820961>


-Doug

Re: [RFT PATCH 0/4] usb: dwc2: Turn on uframe sched everywhere

[Stefan Wahren]

Hi Doug,

[add Felipe and Grigor]

> Doug Anderson <dianders@chromium.org> hat am 10. Juli 2018 um 17:21 geschrieben:
> 
> 
> Hi,
> 
> On Tue, Jul 10, 2018 at 6:57 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> > Hi Doug,
> >
> > since Stephen isn't BCM2835 maintainer anymore, please keep me in the loop.
> 
> Sorry for missing you.  :(  I wonder if you should add yourself to the
> MAINTAINERS file as a reviewer for dwc2 so you can get CCed always?

i don't think this is necessary and my USB knowledge is to little. I've setup a jenkins to catch regressions from linux-next. This is currently sufficient.

> 
> 
> > I'm not sure how to test this. Is there some kind of test scenario (don't
> > have a USB analyzer)?
> 
> I guess it'll depend on what USB devices you have sitting around.  The
> more USB devices you have plugged in then the more likely the dumb
> scheduler will get things wrong and a device won't like you.  You
> should also see significant compatibility improvements when you have
> low/full speed devices plugged into high speed USB hubs.
> 
> IIRC when testing I did things like:
> - In general, try to have lots of things plugged in.
> - Plug many different keyboards / mice in.  See if keys are dropped /
> mouse is jerky.
> - Try USB audio (ideally full speed, not high speed).  How is the audio?
> - If you're brave, try a USB webcam.  How is the video?

Thanks for the hints. The BCM2835 only have 8 EP, so my test setup isn't big:

1x Rasberry Pi 2 (with onboard ethernet SMSC95xx via USB)
1x keyboard connected to the Pi directly
1x external powered 7 port HUB (1x mice and 1x USB serial adapter)

/:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=dwc2/1p, 480M
    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/5p, 480M
        |__ Port 1: Dev 3, If 0, Class=Vendor Specific Class, Driver=smsc95xx, 480M
        |__ Port 2: Dev 4, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
        |__ Port 2: Dev 4, If 1, Class=Human Interface Device, Driver=usbhid, 1.5M
        |__ Port 4: Dev 5, If 0, Class=Hub, Driver=hub/4p, 480M
            |__ Port 1: Dev 6, If 0, Class=Hub, Driver=hub/4p, 480M
                |__ Port 1: Dev 7, If 0, Class=Human Interface Device, Driver=usbhid, 1.5M
                |__ Port 4: Dev 8, If 0, Class=Vendor Specific Class, Driver=, 12M

But even in this simple scenario, your patch #1 is an improvement.

Without the hub connected and without your patch
or with the hub (and the 2 devices) connected and with your patch the SMSC95xx works properly:

[   45.668254] smsc95xx 1-1.1:1.0 eth0: hardware isn't capable of remote wakeup
[   47.146506] smsc95xx 1-1.1:1.0 eth0: link up, 100Mbps, full-duplex, lpa 0xCDE1

But with the hub (and the 2 devices) connected and without your patch the SMSC95xx driver has timeouts and in the result the RPi won't get an IP from my router:

[   20.408622] smsc95xx 1-1.1:1.0 eth0: hardware isn't capable of remote wakeup
[   20.807821] Adding 102396k swap on /var/swap.  Priority:-2 extents:1 across:102396k SS
[   26.401346] smsc95xx 1-1.1:1.0 eth0: Failed to read reg index 0x00000114: -110
[   26.401360] smsc95xx 1-1.1:1.0 eth0: Error reading MII_ACCESS
[   26.401371] smsc95xx 1-1.1:1.0 eth0: MII is busy in smsc95xx_mdio_read
[   26.401387] smsc95xx 1-1.1:1.0 eth0: Failed to read MII_BMSR
[   31.441429] smsc95xx 1-1.1:1.0 eth0: Failed to read reg index 0x00000114: -110
[   31.441454] smsc95xx 1-1.1:1.0 eth0: Error reading MII_ACCESS
[   31.441465] smsc95xx 1-1.1:1.0 eth0: MII is busy in smsc95xx_mdio_read
[   31.441481] smsc95xx 1-1.1:1.0 eth0: link reset failed (-110) usbnet usb-3f980000.usb-1.1, smsc95xx USB 2.0 Ethernet
[   36.481359] smsc95xx 1-1.1:1.0 eth0: Failed to read reg index 0x00000114: -110
[   36.481372] smsc95xx 1-1.1:1.0 eth0: Error reading MII_ACCESS
[   36.481383] smsc95xx 1-1.1:1.0 eth0: MII is busy in smsc95xx_mdio_read
[   36.481402] smsc95xx 1-1.1:1.0 eth0: link reset failed (-110) usbnet usb-3f980000.usb-1.1, smsc95xx USB 2.0 Ethernet
[   41.521345] smsc95xx 1-1.1:1.0 eth0: Failed to read reg index 0x00000114: -110
[   41.521358] smsc95xx 1-1.1:1.0 eth0: Error reading MII_ACCESS
[   41.521369] smsc95xx 1-1.1:1.0 eth0: MII is busy in smsc95xx_mdio_read
[   41.521387] smsc95xx 1-1.1:1.0 eth0: link reset failed (-110) usbnet usb-3f980000.usb-1.1, smsc95xx USB 2.0 Ethernet
[   46.561349] smsc95xx 1-1.1:1.0 eth0: Failed to read reg index 0x00000114: -110
[   46.561363] smsc95xx 1-1.1:1.0 eth0: Error reading MII_ACCESS
[   46.561374] smsc95xx 1-1.1:1.0 eth0: MII is busy in smsc95xx_mdio_read
[   46.561393] smsc95xx 1-1.1:1.0 eth0: link reset failed (-110) usbnet usb-3f980000.usb-1.1, smsc95xx USB 2.0 Ethernet
[   59.441375] smsc95xx 1-1.1:1.0 eth0: Failed to read reg index 0x00000118: -110
[   59.441389] smsc95xx 1-1.1:1.0 eth0: Error reading MII_DATA
[   59.441405] smsc95xx 1-1.1:1.0 eth0: link reset failed (-110) usbnet usb-3f980000.usb-1.1, smsc95xx USB 2.0 Ethernet
[   64.545268] smsc95xx 1-1.1:1.0 eth0: Failed to read reg index 0x00000114: -110
[   64.545285] smsc95xx 1-1.1:1.0 eth0: Error reading MII_ACCESS
[   64.545297] smsc95xx 1-1.1:1.0 eth0: MII is busy in smsc95xx_mdio_read
[   64.545324] smsc95xx 1-1.1:1.0 eth0: link reset failed (-110) usbnet usb-3f980000.usb-1.1, smsc95xx USB 2.0 Ethernet

So you can have my

Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

for the bcm patch.

> 
> 
> If you're trying a USB webcam w/ dwc2, maybe add in patches mentioned
> in <https://bugs.chromium.org/p/chromium/issues/detail?id=820961>

Interesting

Best regards
Stefan

> 
> 
> -Doug