[PATCH v5 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[PATCH v5 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

arch/arm/dts/rk3399.dtsi has a node

  usb_host0_ehci: usb@fe380000 {
       compatible = "generic-ehci";

with clocks:

       clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
                <&u2phy0>;

The first 2 refer to nodes with class UCLASS_CLK, but &u2phy0
has class UCLASS_PHY.

  u2phy0: usb2phy@e450 {
       compatible = "rockchip,rk3399-usb2phy";

Since clk_get_bulk() only looks for devices with UCLASS_CLK,
it fails with -ENODEV and then ehci_usb_probe() aborts.

The consequence is peripherals connected to a USB 2 port (e.g. in a
Rock Pi 4 one of the two ports nearer the edge) not being detected.
They're detected if CONFIG_USB_OHCI_GENERIC is selected in Kconfig,
because ohci_usb_probe() does not abort when one clk_get_by_index()
fails, but then they work in USB 1 mode,.

rk3399.dtsi comes from linux and the  u2phy0 was added[1] to the clock
list in:

    commit b5d1c57299734f5b54035ef2e61706b83041f20c
    Author: William wu <wulf@rock-chips.com>
    Date:   Wed Dec 21 18:41:05 2016 +0800

    arm64: dts: rockchip: add u2phy clock for ehci and ohci of rk3399

    We found that the suspend process was blocked when it run into
    ehci/ohci module due to clk-480m of usb2-phy was disabled.
    [...]

Suspend concerns don't apply to U-Boot, and the problem with U-Boot
failing to probe EHCI doesn't apply to linux, because in linux
rockchip_usb2phy_clk480m_register makes u2phy0 a proper clock provider
when called by rockchip_usb2phy_probe().

So I can think of a few alternative solutions:

1- Change ehci_usb_probe() to make it more similar to
   ohci_usb_probe(), and survive failure to get one clock. Looks a
   little harder, and I don't know whether it could break something if
   it ignored a clock that was important for something else than
   suspend. (tried in v2)

2- Change rk3399.dtsi effecttively reverting the linux commit
   b5d1c57299734f5b54035ef2e61706b83041f20c. This dealigns the .dtsi
   from linux and seems fragile at the next synchronisation.

3- Change the clock list in rk3399-u-boot.dtsi or somewhere else.
   This survives .dts* sync but may survive "too much" and miss some
   change from linux that we might want. (tried in v1)

4- Enable CONFIG_USB_OHCI_GENERIC and use the ports in USB 1 mode.
   This would need to be made for all boards using rk3399.  In a
   simple test reading one file from USB storage it gave 769.5 KiB/s
   instead of 20.5 MiB/s with solution 2.

5- Trying to replicate linux and have usb2phy somehow provide a clk,
   or have a separate clock device for usb2phy in addition to the phy
   device. (tried in v3 and this v5, v4 did it wrong)

This series is a third attempt to implement option 5 as Marek Vasut
requested in December 5th.  Options 1 and 3 didn't get through[2,3].

The first patch in the series (identical to v3) just registers usb2phy
as a clock driver, without any specific operations, so that
ehci-generic.c finds it and is happy. It worked in my tests on a Rock
Pi 4 B+ (rk3399).

Since Marek Vasut objected to an operationless driver[4], the second
patch adds enable and disable operations adapted from linux prepare
and unprepare operations (and round_rate(), which doesn't seem very
useful anyway since it's a fixed clock). Since there're no users of
this clock in u-boot, I can't see any difference in my tests with only
the first patch or both, so I can't be sure it really works if it's
ever needed, but it's hopefully more complete.

Links: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/
       [2] https://patchwork.ozlabs.org/project/uboot/patch/20220701185959.GC1700@begut/#2954536
       [3] https://patchwork.ozlabs.org/project/uboot/patch/Y44+ayJfUlI08ptM@localhost/#3016099
       [4] https://patchwork.ozlabs.org/project/uboot/patch/Y5IWpjYLB4aXMy9o@localhost/#3018135
      
Cc: Simon Glass <sjg@chromium.org>
Cc: Philipp Tomsich <philipp.tomsich@vrull.eu>
Cc: Kever Yang <kever.yang@rock-chips.com>
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Sean Anderson <seanga2@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Christoph Fritz <chf.fritz@googlemail.com>
Signed-off-by: Xavier Drudis Ferran <xdrudis@tinet.cat>
---

   Changes:

   v5: fixes a bug that Christoph Fritz discovered, consisting in the
       wrong eror code returned when enabling or disabling the clock
       because property_enable() returns an error code in linux but
       the modified register value in U-Boot. This caused the clk
       disable to abort before freeing the clock and it apparently
       left things bad enough to cause a hang or a reset.

   v4: move v3 to one patch in the series and add a second patch
       to add operations to enable disable the usb2phy 480Mhz clock.
       Also, honour clock-output-names for what is worth.

   v3: implement option 5 (bind usb2phy as a clk driver too) instead
       of option 1 (ehci-generic.c tolerates missing clocks).

   v2: implement option 1 (ehci-generic.c tolerates missing clocks)
      instead of option 3 (change dts node to remove the missing
      clock).

Re: [PATCH v5 1/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

arch/arm/dts/rk3399.dtsi has a node

  usb_host0_ehci: usb@fe380000 {
       compatible = "generic-ehci";

with clocks:

       clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>,
                <&u2phy0>;

The first 2 refer to nodes with class UCLASS_CLK, but &u2phy0
has class UCLASS_PHY.

  u2phy0: usb2phy@e450 {
       compatible = "rockchip,rk3399-usb2phy";

Since clk_get_bulk() only looks for devices with UCLASS_CLK,
it fails with -ENODEV and then ehci_usb_probe() aborts.

The consequence is peripherals connected to a USB 2 port (e.g. in a
Rock Pi 4 one of the two ports nearer the edge) not being detected.
They're detected if CONFIG_USB_OHCI_GENERIC is selected in Kconfig,
because ohci_usb_probe() does not abort when one clk_get_by_index()
fails, but then they work in USB 1 mode,.

rk3399.dtsi comes from linux and the  u2phy0 was added[1] to the clock
list in:


    commit b5d1c57299734f5b54035ef2e61706b83041f20c
    Author: William wu <wulf@rock-chips.com>
    Date:   Wed Dec 21 18:41:05 2016 +0800

    arm64: dts: rockchip: add u2phy clock for ehci and ohci of rk3399

    We found that the suspend process was blocked when it run into
    ehci/ohci module due to clk-480m of usb2-phy was disabled.
    [...]

Suspend concerns don't apply to U-Boot, and the problem with U-Boot
failing to probe EHCI doesn't apply to linux, because in linux
rockchip_usb2phy_clk480m_register makes u2phy0 a proper clock provider
when called by rockchip_usb2phy_probe().

So I can think of a few alternative solutions:

1- Change ehci_usb_probe() to make it more similar to
   ohci_usb_probe(), and survive failure to get one clock. Looks a
   little harder, and I don't know whether it could break something if
   it ignored a clock that was important for something else than
   suspend. (tried in v2)

2- Change rk3399.dtsi effecttively reverting the linux commit
   b5d1c57299734f5b54035ef2e61706b83041f20c. This dealigns the .dtsi
   from linux and seems fragile at the next synchronisation.

3- Change the clock list in rk3399-u-boot.dtsi or somewhere else.
   This survives .dts* sync but may survive "too much" and miss some
   change from linux that we might want. (tried in v1)

4- Enable CONFIG_USB_OHCI_GENERIC and use the ports in USB 1 mode.
   This would need to be made for all boards using rk3399.  In a
   simple test reading one file from USB storage it gave 769.5 KiB/s
   instead of 20.5 MiB/s with solution 2.

5- Trying to replicate linux and have usb2phy somehow provide a clk,
   or have a separate clock device for usb2phy in addition to the phy
   device. (tried in v3 and this v5, v4 did it wrong)

This series is a third attempt to implement option 5 as Marek Vasut
requested in December 5th.  Options 1 and 3 didn't get through[2,3].

The first patch in the series (identical to v3) just registers usb2phy
as a clock driver, without any specific operations, so that
ehci-generic.c finds it and is happy. It worked in my tests on a Rock
Pi 4 B+ (rk3399).

Since Marek Vasut objected to an operationless driver[4], the second
patch adds enable and disable operations adapted from linux prepare
and unprepare operations (and round_rate(), which doesn't seem very
useful anyway since it's a fixed clock). Since there're no users of
this clock in u-boot, I can't see any difference in my tests with only
the first patch or both, so I can't be sure it really works if it's
ever needed, but it's hopefully more complete.

Link: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/

Cc: Simon Glass <sjg@chromium.org>
Cc: Philipp Tomsich <philipp.tomsich@vrull.eu>
Cc: Kever Yang <kever.yang@rock-chips.com>
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Sean Anderson <seanga2@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Christoph Fritz <chf.fritz@googlemail.com>

Signed-off-by: Xavier Drudis Ferran <xdrudis@tinet.cat>
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 23 ++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index b32a498ea7..9769ada376 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -7,7 +7,7 @@
  */
 
 #include <common.h>
-#include <clk.h>
+#include <clk-uclass.h>
 #include <dm.h>
 #include <asm/global_data.h>
 #include <dm/device_compat.h>
@@ -168,6 +168,9 @@ static struct phy_ops rockchip_usb2phy_ops = {
 	.of_xlate = rockchip_usb2phy_of_xlate,
 };
 
+static struct clk_ops rockchip_usb2phy_clk_ops = {
+};
+
 static int rockchip_usb2phy_probe(struct udevice *dev)
 {
 	struct rockchip_usb2phy *priv = dev_get_priv(dev);
@@ -240,6 +243,18 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
 		}
 	}
 
+	if (!ret) {
+		node = dev_ofnode(dev);
+		name = ofnode_get_name(node);
+		dev_dbg(dev, "clk for node %s\n", name);
+		ret = device_bind_driver_to_node(dev, "rockchip_usb2phy_clock",
+						 name, node, &usb2phy_dev);
+		if (ret) {
+			dev_err(dev,
+				"'%s' cannot bind 'rockchip_usb2phy_clock'\n", name);
+		}
+	}
+
 	return ret;
 }
 
@@ -303,6 +318,12 @@ U_BOOT_DRIVER(rockchip_usb2phy_port) = {
 	.ops		= &rockchip_usb2phy_ops,
 };
 
+U_BOOT_DRIVER(rockchip_usb2phy_clock) = {
+	.name		= "rockchip_usb2phy_clock",
+	.id		= UCLASS_CLK,
+	.ops		= &rockchip_usb2phy_clk_ops,
+};
+
 U_BOOT_DRIVER(rockchip_usb2phy) = {
 	.name	= "rockchip_usb2phy",
 	.id	= UCLASS_PHY,
-- 
2.20.1

[PATCH 2/2] arm: rk3399: usb2phy: phy-rockchip-inno-usb2.c: Implement operations for the 480MHz usb2phy clock in rk3399.

[Xavier Drudis Ferran]

This clock has no users but appears in a phandle list used by
ehci-generic.c to bulk enable it. The phandle list comes from linux,
where it is needed for suspend/resume to work [1].

My tests give the same results with or without this patch, but Marek
Vasut found it weird to declare an empty clk_ops.

So I adapted the code from linux 6.1-rc8 so that it hopefully works
if it ever has some user. For now, without real users, it seems to
at least not give any errors.

Link: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/

Cc: Simon Glass <sjg@chromium.org>
Cc: Philipp Tomsich <philipp.tomsich@vrull.eu>
Cc: Kever Yang <kever.yang@rock-chips.com>
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Sean Anderson <seanga2@gmail.com>
Cc: Marek Vasut <marex@denx.de>

Signed-off-by: Xavier Drudis Ferran <xdrudis@tinet.cat>
---

 v5: ignores the return value from property_enable() which is not
     an error code in U-Boot (unlike in linux). This avoid a false
     failure of rockchip_usb2phy_clk_disable() that interfered with
     clock disable and appeared to cause hang or reset.
     
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 78 ++++++++++++++++++-
 1 file changed, 76 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 9769ada376..66e0e5aed5 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -55,6 +55,7 @@ struct rockchip_usb2phy_port_cfg {
 
 struct rockchip_usb2phy_cfg {
 	unsigned int reg;
+	struct usb2phy_reg	clkout_ctl;
 	const struct rockchip_usb2phy_port_cfg port_cfgs[USB2PHY_NUM_PORTS];
 };
 
@@ -76,6 +77,18 @@ static inline int property_enable(void *reg_base,
 	return writel(val, reg_base + reg->offset);
 }
 
+static inline bool property_enabled(void *reg_base,
+				    const struct usb2phy_reg *reg)
+{
+	unsigned int tmp, orig;
+	unsigned int mask = GENMASK(reg->bitend, reg->bitstart);
+
+	orig = readl(reg_base + reg->offset);
+
+	tmp = (orig & mask) >> reg->bitstart;
+	return tmp != reg->disable;
+}
+
 static const
 struct rockchip_usb2phy_port_cfg *us2phy_get_port(struct phy *phy)
 {
@@ -168,7 +181,63 @@ static struct phy_ops rockchip_usb2phy_ops = {
 	.of_xlate = rockchip_usb2phy_of_xlate,
 };
 
+/**
+ * round_rate() - Adjust a rate to the exact rate a clock can provide.
+ * @clk:	The clock to manipulate.
+ * @rate:	Desidered clock rate in Hz.
+ *
+ * Return: rounded rate in Hz, or -ve error code.
+ */
+ulong rockchip_usb2phy_clk_round_rate(struct clk *clk, ulong rate)
+{
+	return 480000000;
+}
+
+/**
+ * enable() - Enable a clock.
+ * @clk:	The clock to manipulate.
+ *
+ * Return: zero on success, or -ve error code.
+ */
+int rockchip_usb2phy_clk_enable(struct clk *clk)
+{
+	struct udevice *parent = dev_get_parent(clk->dev);
+	struct rockchip_usb2phy *priv = dev_get_priv(parent);
+	const struct rockchip_usb2phy_cfg *phy_cfg = priv->phy_cfg;
+
+	/* turn on 480m clk output if it is off */
+	if (!property_enabled(priv->reg_base, &phy_cfg->clkout_ctl)) {
+		property_enable(priv->reg_base, &phy_cfg->clkout_ctl, true);
+
+		/* waiting for the clk become stable */
+		usleep_range(1200, 1300);
+	}
+
+	return 0;
+}
+
+/**
+ * disable() - Disable a clock.
+ * @clk:	The clock to manipulate.
+ *
+ * Return: zero on success, or -ve error code.
+ */
+int rockchip_usb2phy_clk_disable(struct clk *clk)
+{
+	struct udevice *parent = dev_get_parent(clk->dev);
+	struct rockchip_usb2phy *priv = dev_get_priv(parent);
+	const struct rockchip_usb2phy_cfg *phy_cfg = priv->phy_cfg;
+
+	/* turn off 480m clk output */
+	property_enable(priv->reg_base, &phy_cfg->clkout_ctl, false);
+
+	return 0;
+}
+
 static struct clk_ops rockchip_usb2phy_clk_ops = {
+	.enable = rockchip_usb2phy_clk_enable,
+	.disable = rockchip_usb2phy_clk_disable,
+	.round_rate = rockchip_usb2phy_clk_round_rate
 };
 
 static int rockchip_usb2phy_probe(struct udevice *dev)
@@ -245,8 +314,11 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
 
 	if (!ret) {
 		node = dev_ofnode(dev);
-		name = ofnode_get_name(node);
-		dev_dbg(dev, "clk for node %s\n", name);
+		name = "clk_usbphy_480m";
+		dev_read_string_index(dev, "clock-output-names", 0, &name);
+
+		dev_dbg(dev, "clk %s for node %s\n", name, ofnode_get_name(node));
+
 		ret = device_bind_driver_to_node(dev, "rockchip_usb2phy_clock",
 						 name, node, &usb2phy_dev);
 		if (ret) {
@@ -261,6 +333,7 @@ static int rockchip_usb2phy_bind(struct udevice *dev)
 static const struct rockchip_usb2phy_cfg rk3399_usb2phy_cfgs[] = {
 	{
 		.reg		= 0xe450,
+		.clkout_ctl	= { 0xe450, 4, 4, 1, 0 },
 		.port_cfgs	= {
 			[USB2PHY_PORT_OTG] = {
 				.phy_sus	= { 0xe454, 1, 0, 2, 1 },
@@ -282,6 +355,7 @@ static const struct rockchip_usb2phy_cfg rk3399_usb2phy_cfgs[] = {
 	},
 	{
 		.reg		= 0xe460,
+		.clkout_ctl	= { 0xe460, 4, 4, 1, 0 },
 		.port_cfgs	= {
 			[USB2PHY_PORT_OTG] = {
 				.phy_sus        = { 0xe464, 1, 0, 2, 1 },
-- 
2.20.1

Re: [PATCH v5 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Christoph Fritz]

>    Changes:
> 
>    v5: fixes a bug that Christoph Fritz discovered, consisting in the
>        wrong eror code returned when enabling or disabling the clock
>        because property_enable() returns an error code in linux but
>        the modified register value in U-Boot. This caused the clk
>        disable to abort before freeing the clock and it apparently
>        left things bad enough to cause a hang or a reset.
> 

With your patches ontop of v2023.04-rc2, xhci works now fine on a
rk3399 board:

=> usb start
starting USB...
Bus usb@fe380000: USB EHCI 1.00
Bus usb@fe3c0000: USB EHCI 1.00
Bus usb@fe800000: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
Bus usb@fe900000: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
scanning bus usb@fe380000 for devices... 4 USB Device(s) found
scanning bus usb@fe3c0000 for devices... 1 USB Device(s) found
scanning bus usb@fe800000 for devices... 3 USB Device(s) found
scanning bus usb@fe900000 for devices... 1 USB Device(s) found
       scanning usb for storage devices... 0 Storage Device(s) found

Totally unrelated to your patches, stopping usb still crashes (with or
without your patch) but only when a USB-Ethernet-Dongle (+HUB) is
connected:

=> usb start
starting USB...
do_usb_start, 581
Bus usb@fe380000: USB EHCI 1.00
Bus usb@fe3c0000: USB EHCI 1.00
Bus usb@fe800000: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
Bus usb@fe900000: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
scanning bus usb@fe380000 for devices... 3 USB Device(s) found
scanning bus usb@fe3c0000 for devices... 1 USB Device(s) found
scanning bus usb@fe800000 for devices... 1 USB Device(s) found
scanning bus usb@fe900000 for devices... 1 USB Device(s) found
       scanning usb for storage devices... 0 Storage Device(s) found
=> usb tree
USB device tree:
  1  Hub (480 Mb/s, 0mA)
  |  u-boot EHCI Host Controller
  |
  +-2  Hub (480 Mb/s, 100mA)
    |   USB2.0 Hub
    |
    +-3  Human Interface (12 Mb/s, 400mA)
         ILITEK ILITEK-TP

  1  Hub (480 Mb/s, 0mA)
     u-boot EHCI Host Controller

  1  Hub (5 Gb/s, 0mA)
     U-Boot XHCI Host Controller

  1  Hub (5 Gb/s, 0mA)
     U-Boot XHCI Host Controller

=> usb stop
stopping USB..
=> usb start
starting USB...
Bus usb@fe380000: USB EHCI 1.00
Bus usb@fe3c0000: USB EHCI 1.00
Bus usb@fe800000: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
Bus usb@fe900000: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
scanning bus usb@fe380000 for devices... 4 USB Device(s) found
scanning bus usb@fe3c0000 for devices... 1 USB Device(s) found
scanning bus usb@fe800000 for devices... 3 USB Device(s) found
scanning bus usb@fe900000 for devices... 1 USB Device(s) found
       scanning usb for storage devices... 0 Storage Device(s) found
=> usb tree
USB device tree:
  1  Hub (480 Mb/s, 0mA)
  |  u-boot EHCI Host Controller
  |
  +-2  Hub (480 Mb/s, 100mA)
    |   USB2.0 Hub
    |
    +-3  Hub (480 Mb/s, 0mA)
    |    VIA Labs, Inc.          USB2.0 Hub
    |
    +-4  Human Interface (12 Mb/s, 400mA)
         ILITEK ILITEK-TP

  1  Hub (480 Mb/s, 0mA)
     u-boot EHCI Host Controller

  1  Hub (5 Gb/s, 0mA)
  |  U-Boot XHCI Host Controller
  |
  +-2  Hub (5 Gb/s, 0mA)
    |  VIA Labs, Inc.          USB3.0 Hub
    |
    +-3  Vendor specific (5 Gb/s, 36mA)
         Realtek USB 10/100/1000 LAN 00E04C68034E

  1  Hub (5 Gb/s, 0mA)
     U-Boot XHCI Host Controller

=> usb stop
stopping USB..
<hangs here forever>

I just quirked/masked the underlying issue by not doing usb_stop() at
all in drivers/usb/host/usb-uclass.c.

Re: [PATCH v5 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

El Fri, Mar 03, 2023 at 10:42:20AM +0100, Christoph Fritz deia:
> >    Changes:
> > 
> >    v5: fixes a bug that Christoph Fritz discovered, consisting in the
> >        wrong eror code returned when enabling or disabling the clock
> >        because property_enable() returns an error code in linux but
> >        the modified register value in U-Boot. This caused the clk
> >        disable to abort before freeing the clock and it apparently
> >        left things bad enough to cause a hang or a reset.
> > 
> 
> With your patches ontop of v2023.04-rc2, xhci works now fine on a
> rk3399 board:
>

Fine, thanks a lot for testing. I thought the problem was with EHCI.

> Totally unrelated to your patches, stopping usb still crashes (with or
> without your patch) but only when a USB-Ethernet-Dongle (+HUB) is
> connected:
>

Well, but with v4 you got a couple more messages about devices not
removed but children gone. With v5 those messages don't show any
more. So v5 fixes one of the bugs you were seeing, more bugs remain.

Unfortunately I don't think I have any usb-ethernet dongle here to test...

> => usb start
> starting USB...
> do_usb_start, 581
> Bus usb@fe380000: USB EHCI 1.00
> Bus usb@fe3c0000: USB EHCI 1.00
> Bus usb@fe800000: Register 2000140 NbrPorts 2
> Starting the controller
> USB XHCI 1.10
> Bus usb@fe900000: Register 2000140 NbrPorts 2
> Starting the controller
> USB XHCI 1.10
> scanning bus usb@fe380000 for devices... 3 USB Device(s) found
> scanning bus usb@fe3c0000 for devices... 1 USB Device(s) found
> scanning bus usb@fe800000 for devices... 1 USB Device(s) found
> scanning bus usb@fe900000 for devices... 1 USB Device(s) found
>        scanning usb for storage devices... 0 Storage Device(s) found
> => usb tree
> USB device tree:
>   1  Hub (480 Mb/s, 0mA)
>   |  u-boot EHCI Host Controller
>   |
>   +-2  Hub (480 Mb/s, 100mA)
>     |   USB2.0 Hub
>     |
>     +-3  Human Interface (12 Mb/s, 400mA)
>          ILITEK ILITEK-TP
> 
>   1  Hub (480 Mb/s, 0mA)
>      u-boot EHCI Host Controller
> 
>   1  Hub (5 Gb/s, 0mA)
>      U-Boot XHCI Host Controller
> 
>   1  Hub (5 Gb/s, 0mA)
>      U-Boot XHCI Host Controller
>

Ok, so this was with a keyboard or something connected to a USB 2 port
and nothing in the blue USB 3 port, right?

> => usb stop
> stopping USB..
> => usb start
> starting USB...
> Bus usb@fe380000: USB EHCI 1.00
> Bus usb@fe3c0000: USB EHCI 1.00
> Bus usb@fe800000: Register 2000140 NbrPorts 2
> Starting the controller
> USB XHCI 1.10
> Bus usb@fe900000: Register 2000140 NbrPorts 2
> Starting the controller
> USB XHCI 1.10
> scanning bus usb@fe380000 for devices... 4 USB Device(s) found
> scanning bus usb@fe3c0000 for devices... 1 USB Device(s) found
> scanning bus usb@fe800000 for devices... 3 USB Device(s) found
> scanning bus usb@fe900000 for devices... 1 USB Device(s) found
>        scanning usb for storage devices... 0 Storage Device(s) found
> => usb tree
> USB device tree:
>   1  Hub (480 Mb/s, 0mA)
>   |  u-boot EHCI Host Controller
>   |
>   +-2  Hub (480 Mb/s, 100mA)
>     |   USB2.0 Hub
>     |
>     +-3  Hub (480 Mb/s, 0mA)
>     |    VIA Labs, Inc.          USB2.0 Hub
>     |
>     +-4  Human Interface (12 Mb/s, 400mA)
>          ILITEK ILITEK-TP
> 
>   1  Hub (480 Mb/s, 0mA)
>      u-boot EHCI Host Controller
> 
>   1  Hub (5 Gb/s, 0mA)
>   |  U-Boot XHCI Host Controller
>   |
>   +-2  Hub (5 Gb/s, 0mA)
>     |  VIA Labs, Inc.          USB3.0 Hub
>     |
>     +-3  Vendor specific (5 Gb/s, 36mA)
>          Realtek USB 10/100/1000 LAN 00E04C68034E
> 
>   1  Hub (5 Gb/s, 0mA)
>      U-Boot XHCI Host Controller
>

And this was with the keyboard in USB2 and a USB3 VIA Labs hub
connected to the USB3 port and a Realtek ethernet-USB dongle attached
to that USB3 hub, right?


> => usb stop
> stopping USB..
> <hangs here forever>
> 
> I just quirked/masked the underlying issue by not doing usb_stop() at
> all in drivers/usb/host/usb-uclass.c.
> 

Does usb reset also hang ? If so the problem then must be that you are
left with no way to rescan for devices that were not connected at usb
start but get connected later ?

Did you say before it all worked if you unconfigured EHCI and worked
with XHCI only? (or OHCI + XHCI but no EHCI). Might it have to do with
the same hub hanging from the EHCI and XHCI controller? (but it doesn't
sound strange to me, the hub doesn't know what devices will be
connected to it...)

If you leave drivers/usb/host/usb-uclass.c as it was but drop some
#define DEBUG in device-remove.c does it give any hint ? 

I'll try to do some more tests with a USB3 hub and usb storage...

Thanks again for testing.

can't reproduce XHCI hang in Rock Pi 4

[Xavier Drudis Ferran]

I'm sorry but I haven't been able to reproduce your issue.


El Fri, Mar 03, 2023 at 11:26:46AM +0100, Xavier Drudis Ferran deia:
> El Fri, Mar 03, 2023 at 10:42:20AM +0100, Christoph Fritz deia:
> 
> Unfortunately I don't think I have any usb-ethernet dongle here to test...
> 
[...]
> > => usb stop
> > stopping USB..
> > => usb start
> > starting USB...
> > Bus usb@fe380000: USB EHCI 1.00
> > Bus usb@fe3c0000: USB EHCI 1.00
> > Bus usb@fe800000: Register 2000140 NbrPorts 2
> > Starting the controller
> > USB XHCI 1.10
> > Bus usb@fe900000: Register 2000140 NbrPorts 2
> > Starting the controller
> > USB XHCI 1.10
> > scanning bus usb@fe380000 for devices... 4 USB Device(s) found
> > scanning bus usb@fe3c0000 for devices... 1 USB Device(s) found
> > scanning bus usb@fe800000 for devices... 3 USB Device(s) found
> > scanning bus usb@fe900000 for devices... 1 USB Device(s) found
> >        scanning usb for storage devices... 0 Storage Device(s) found
> > => usb tree
> > USB device tree:
> >   1  Hub (480 Mb/s, 0mA)
> >   |  u-boot EHCI Host Controller
> >   |
> >   +-2  Hub (480 Mb/s, 100mA)
> >     |   USB2.0 Hub
> >     |
> >     +-3  Hub (480 Mb/s, 0mA)
> >     |    VIA Labs, Inc.          USB2.0 Hub
> >     |
> >     +-4  Human Interface (12 Mb/s, 400mA)
> >          ILITEK ILITEK-TP
> > 
> >   1  Hub (480 Mb/s, 0mA)
> >      u-boot EHCI Host Controller
> > 
> >   1  Hub (5 Gb/s, 0mA)
> >   |  U-Boot XHCI Host Controller
> >   |
> >   +-2  Hub (5 Gb/s, 0mA)
> >     |  VIA Labs, Inc.          USB3.0 Hub
> >     |
> >     +-3  Vendor specific (5 Gb/s, 36mA)
> >          Realtek USB 10/100/1000 LAN 00E04C68034E
> > 
> >   1  Hub (5 Gb/s, 0mA)
> >      U-Boot XHCI Host Controller
> >
> 

I tried with a USB3 hub connected to a usb3 port, a sata
adapter connected to the USB3 hub, and a SATA disk connected to the
SATA adapter, and I could read the disk.

The difference is that in my tree both the USB2 and the USB 3 hub
hang from the XHCI controller, not one from EHCI and one from XHCI.

This with my v5 patches on top of 4eb7c5030d3f3c70 (2023-02-19) and some
minor config changes that don't seem to matter.


=> usb start
starting USB...
Bus usb@fe380000: USB EHCI 1.00
Bus usb@fe3c0000: USB EHCI 1.00
Bus usb@fe800000: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
Bus usb@fe900000: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
scanning bus usb@fe380000 for devices... 1 USB Device(s) found
scanning bus usb@fe3c0000 for devices... 1 USB Device(s) found
scanning bus usb@fe800000 for devices... 4 USB Device(s) found
scanning bus usb@fe900000 for devices... 1 USB Device(s) found
       scanning usb for storage devices... 1 Storage Device(s) found
=> usb tree
USB device tree:
  1  Hub (480 Mb/s, 0mA)
     u-boot EHCI Host Controller 
   
  1  Hub (480 Mb/s, 0mA)
     u-boot EHCI Host Controller 
   
  1  Hub (5 Gb/s, 0mA)
  |  U-Boot XHCI Host Controller 
  |
  |\b+-2  Hub (5 Gb/s, 0mA)
  | |  GenesysLogic USB3.0 Hub 
  | |
  | |\b+-4  Mass Storage (5 Gb/s, 24mA)
  |      Prolific Technology Inc. ATAPI-6 Bridge Controller 0123456789000000005
  |    
  |\b+-3  Hub (480 Mb/s, 100mA)
       GenesysLogic USB2.0 Hub 
     
  1  Hub (5 Gb/s, 0mA)
     U-Boot XHCI Host Controller 
   
=> ls usb 0
<DIR>       4096 .
<DIR>       4096 ..
<DIR>      16384 lost+found
<DIR>       4096 var
<DIR>      12288 etc
[...]
=> usb stop
stopping USB..
=> usb start
starting USB...
Bus usb@fe380000: USB EHCI 1.00
Bus usb@fe3c0000: USB EHCI 1.00
Bus usb@fe800000: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
Bus usb@fe900000: Register 2000140 NbrPorts 2
Starting the controller
USB XHCI 1.10
scanning bus usb@fe380000 for devices... 1 USB Device(s) found
scanning bus usb@fe3c0000 for devices... 1 USB Device(s) found
scanning bus usb@fe800000 for devices... 4 USB Device(s) found
scanning bus usb@fe900000 for devices... 1 USB Device(s) found
       scanning usb for storage devices... 1 Storage Device(s) found
=> usb tree
USB device tree:
  1  Hub (480 Mb/s, 0mA)
     u-boot EHCI Host Controller 
   
  1  Hub (480 Mb/s, 0mA)
     u-boot EHCI Host Controller 
   
  1  Hub (5 Gb/s, 0mA)
  |  U-Boot XHCI Host Controller 
  |
  |\b+-2  Hub (480 Mb/s, 100mA)
  |    GenesysLogic USB2.0 Hub 
  |  
  |\b+-3  Hub (5 Gb/s, 0mA)
    |  GenesysLogic USB3.0 Hub 
    |
    |\b+-4  Mass Storage (5 Gb/s, 24mA)
         Prolific Technology Inc. ATAPI-6 Bridge Controller 0123456789000000005
       
  1  Hub (5 Gb/s, 0mA)
     U-Boot XHCI Host Controller 
   
=>

[no hang]

I don't know how to help, maybe just try to increase log verbosity or something...

Sorry

--
Xavier Drudis Ferran

Re: [PATCH v5 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Jagan Teki]

On Fri, Mar 3, 2023 at 3:57 PM Xavier Drudis Ferran <xdrudis@tinet.cat> wrote:
>
> El Fri, Mar 03, 2023 at 10:42:20AM +0100, Christoph Fritz deia:
> > >    Changes:
> > >
> > >    v5: fixes a bug that Christoph Fritz discovered, consisting in the
> > >        wrong eror code returned when enabling or disabling the clock
> > >        because property_enable() returns an error code in linux but
> > >        the modified register value in U-Boot. This caused the clk
> > >        disable to abort before freeing the clock and it apparently
> > >        left things bad enough to cause a hang or a reset.
> > >
> >
> > With your patches ontop of v2023.04-rc2, xhci works now fine on a
> > rk3399 board:
> >
>
> Fine, thanks a lot for testing. I thought the problem was with EHCI.
>
> > Totally unrelated to your patches, stopping usb still crashes (with or
> > without your patch) but only when a USB-Ethernet-Dongle (+HUB) is
> > connected:
> >
>
> Well, but with v4 you got a couple more messages about devices not
> removed but children gone. With v5 those messages don't show any
> more. So v5 fixes one of the bugs you were seeing, more bugs remain.
>
> Unfortunately I don't think I have any usb-ethernet dongle here to test...
>
> > => usb start
> > starting USB...
> > do_usb_start, 581
> > Bus usb@fe380000: USB EHCI 1.00
> > Bus usb@fe3c0000: USB EHCI 1.00
> > Bus usb@fe800000: Register 2000140 NbrPorts 2
> > Starting the controller
> > USB XHCI 1.10
> > Bus usb@fe900000: Register 2000140 NbrPorts 2
> > Starting the controller
> > USB XHCI 1.10
> > scanning bus usb@fe380000 for devices... 3 USB Device(s) found
> > scanning bus usb@fe3c0000 for devices... 1 USB Device(s) found
> > scanning bus usb@fe800000 for devices... 1 USB Device(s) found
> > scanning bus usb@fe900000 for devices... 1 USB Device(s) found
> >        scanning usb for storage devices... 0 Storage Device(s) found
> > => usb tree
> > USB device tree:
> >   1  Hub (480 Mb/s, 0mA)
> >   |  u-boot EHCI Host Controller
> >   |
> >   +-2  Hub (480 Mb/s, 100mA)
> >     |   USB2.0 Hub
> >     |
> >     +-3  Human Interface (12 Mb/s, 400mA)
> >          ILITEK ILITEK-TP
> >
> >   1  Hub (480 Mb/s, 0mA)
> >      u-boot EHCI Host Controller
> >
> >   1  Hub (5 Gb/s, 0mA)
> >      U-Boot XHCI Host Controller
> >
> >   1  Hub (5 Gb/s, 0mA)
> >      U-Boot XHCI Host Controller
> >
>
> Ok, so this was with a keyboard or something connected to a USB 2 port
> and nothing in the blue USB 3 port, right?
>
> > => usb stop
> > stopping USB..
> > => usb start
> > starting USB...
> > Bus usb@fe380000: USB EHCI 1.00
> > Bus usb@fe3c0000: USB EHCI 1.00
> > Bus usb@fe800000: Register 2000140 NbrPorts 2
> > Starting the controller
> > USB XHCI 1.10
> > Bus usb@fe900000: Register 2000140 NbrPorts 2
> > Starting the controller
> > USB XHCI 1.10
> > scanning bus usb@fe380000 for devices... 4 USB Device(s) found
> > scanning bus usb@fe3c0000 for devices... 1 USB Device(s) found
> > scanning bus usb@fe800000 for devices... 3 USB Device(s) found
> > scanning bus usb@fe900000 for devices... 1 USB Device(s) found
> >        scanning usb for storage devices... 0 Storage Device(s) found
> > => usb tree
> > USB device tree:
> >   1  Hub (480 Mb/s, 0mA)
> >   |  u-boot EHCI Host Controller
> >   |
> >   +-2  Hub (480 Mb/s, 100mA)
> >     |   USB2.0 Hub
> >     |
> >     +-3  Hub (480 Mb/s, 0mA)
> >     |    VIA Labs, Inc.          USB2.0 Hub
> >     |
> >     +-4  Human Interface (12 Mb/s, 400mA)
> >          ILITEK ILITEK-TP
> >
> >   1  Hub (480 Mb/s, 0mA)
> >      u-boot EHCI Host Controller
> >
> >   1  Hub (5 Gb/s, 0mA)
> >   |  U-Boot XHCI Host Controller
> >   |
> >   +-2  Hub (5 Gb/s, 0mA)
> >     |  VIA Labs, Inc.          USB3.0 Hub
> >     |
> >     +-3  Vendor specific (5 Gb/s, 36mA)
> >          Realtek USB 10/100/1000 LAN 00E04C68034E
> >
> >   1  Hub (5 Gb/s, 0mA)
> >      U-Boot XHCI Host Controller
> >
>
> And this was with the keyboard in USB2 and a USB3 VIA Labs hub
> connected to the USB3 port and a Realtek ethernet-USB dongle attached
> to that USB3 hub, right?
>
>
> > => usb stop
> > stopping USB..
> > <hangs here forever>
> >
> > I just quirked/masked the underlying issue by not doing usb_stop() at
> > all in drivers/usb/host/usb-uclass.c.
> >
>
> Does usb reset also hang ? If so the problem then must be that you are
> left with no way to rescan for devices that were not connected at usb
> start but get connected later ?
>
> Did you say before it all worked if you unconfigured EHCI and worked
> with XHCI only? (or OHCI + XHCI but no EHCI). Might it have to do with
> the same hub hanging from the EHCI and XHCI controller? (but it doesn't
> sound strange to me, the hub doesn't know what devices will be
> connected to it...)
>
> If you leave drivers/usb/host/usb-uclass.c as it was but drop some
> #define DEBUG in device-remove.c does it give any hint ?
>
> I'll try to do some more tests with a USB3 hub and usb storage...

Any news about the next revision of this patch? RK3399 has broken
since release due to this issue. This fix might make upcoming release
workable.

Please let us know.

Thanks,
Jagan.

Re: [SPAM] Re: [PATCH v5 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

El Fri, Jun 02, 2023 at 12:11:13PM +0530, Jagan Teki deia:
> 
> Any news about the next revision of this patch? RK3399 has broken
> since release due to this issue. This fix might make upcoming release
> workable.
> 
> Please let us know.
>

Sorry, I'm not sure you meant to send this to me ?

For my side I stopped working on it after I got no feedback for my last version:

https://lists.denx.de/pipermail/u-boot/2023-February/510672.html
https://lists.denx.de/pipermail/u-boot/2023-February/510676.html
https://lists.denx.de/pipermail/u-boot/2023-February/510678.html

I got no replies.

I asked whether I should resend.

https://lists.denx.de/pipermail/u-boot/2023-March/511621.html

> Btw, my v5 seems not to be correctly in patchwork, should I resend ?
> I guess I messed up subject lines ?

And I got no answer, so I thought there were still no interest and
gave up.

Now I see I answered to a question to Eugen only, not the list, so maybe
the list kept waiting for my answer.

Might be some misunderstanding.

Should I try again with the current next branch and send v6 ? 

Re: [SPAM] Re: [PATCH v5 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Jagan Teki]

On Fri, Jun 2, 2023 at 2:54 PM Xavier Drudis Ferran <xdrudis@tinet.cat> wrote:
>
> El Fri, Jun 02, 2023 at 12:11:13PM +0530, Jagan Teki deia:
> >
> > Any news about the next revision of this patch? RK3399 has broken
> > since release due to this issue. This fix might make upcoming release
> > workable.
> >
> > Please let us know.
> >
>
> Sorry, I'm not sure you meant to send this to me ?
>
> For my side I stopped working on it after I got no feedback for my last version:
>
> https://lists.denx.de/pipermail/u-boot/2023-February/510672.html
> https://lists.denx.de/pipermail/u-boot/2023-February/510676.html
> https://lists.denx.de/pipermail/u-boot/2023-February/510678.html
>
> I got no replies.
>
> I asked whether I should resend.
>
> https://lists.denx.de/pipermail/u-boot/2023-March/511621.html
>
> > Btw, my v5 seems not to be correctly in patchwork, should I resend ?
> > I guess I messed up subject lines ?
>
> And I got no answer, so I thought there were still no interest and
> gave up.
>
> Now I see I answered to a question to Eugen only, not the list, so maybe
> the list kept waiting for my answer.
>
> Might be some misunderstanding.
>
> Should I try again with the current next branch and send v6 ?

Please send.

Thanks,
Jagan.

Re: [SPAM] Re: [PATCH v5 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

El Fri, Jun 02, 2023 at 03:34:37PM +0530, Jagan Teki deia:
> On Fri, Jun 2, 2023 at 2:54 PM Xavier Drudis Ferran <xdrudis@tinet.cat> wrote:
> >
> > Should I try again with the current next branch and send v6 ?
> 
> Please send.
> 
> Thanks,
> Jagan.

I will try asap, thanks.

Re: [SPAM] Re: [PATCH v5 0/2] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2.

[Xavier Drudis Ferran]

El Sat, Jun 03, 2023 at 09:23:36AM +0200, Xavier Drudis Ferran deia:
> El Fri, Jun 02, 2023 at 03:34:37PM +0530, Jagan Teki deia:
> > On Fri, Jun 2, 2023 at 2:54 PM Xavier Drudis Ferran <xdrudis@tinet.cat> wrote:
> > >
> > > Should I try again with the current next branch and send v6 ?
> > 
> > Please send.
> > 
> > Thanks,
> > Jagan.
> 
> I will try asap, thanks.

Done

https://patchwork.ozlabs.org/project/uboot/list/?series=358005

Tests, etc. welcome.

Christoph Fritz issue with his USB-ethernet adapter on XHCI may still
be there, it looked unrelated and I can't test it.