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

Re: [PATCH v4 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 the white port, 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.

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.

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.

This series is a second 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 (device_bind_driver() didn't work but
device_bind_driver_to_node() did), 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>

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

   Changes:

   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 v4 1/2] arm: 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 the white port, 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().


This patch registers usb2phy as a clock driver (device_bind_driver()
didn't work but device_bind_driver_to_node() did), 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).

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>

---
 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 62b8ba3a4a..97a1e11239 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

Re: [PATCH v4 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[2].

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.

Links: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/
       [2] 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>

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

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index cfbdc7d87e..766dde11a6 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,64 @@ 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;
+	int ret;
+
+	/* turn on 480m clk output if it is off */
+	if (!property_enabled(priv->reg_base, &phy_cfg->clkout_ctl)) {
+		ret = property_enable(priv->reg_base, &phy_cfg->clkout_ctl, true);
+		if (ret)
+			return ret;
+
+		/* 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 */
+	return property_enable(priv->reg_base, &phy_cfg->clkout_ctl, false);
+}
+
 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 +315,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 +334,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 +356,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 v4 2/2] arm: rk3399: usb2phy: phy-rockchip-inno-usb2.c: Implement operations for the 480MHz usb2phy clock in rk3399.

[Marek Vasut]

On 12/9/22 16:47, Xavier Drudis Ferran wrote:
> 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[2].
> 
> 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.

You might want to squash 1/2 and 2/2 together, since it's one change 
(add clock operations to a phy driver).

Since 1/2 works without any clock operations, who does enable these usb 
clock on this SoC ? Is there any driver or platform code for that ? If 
so, you can drop that platform code with this driver-side implementation 
in place (which is nice).

btw I am still hoping some of the rockchip people will have a look at 
this series.

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

[Xavier Drudis Ferran]

El Sun, Dec 11, 2022 at 06:20:41AM +0100, Marek Vasut deia:
> On 12/9/22 16:47, Xavier Drudis Ferran wrote:
> > 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[2].
> > 
> > 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.
> 
> You might want to squash 1/2 and 2/2 together, since it's one change (add
> clock operations to a phy driver).
>

I prefer to send them separate, but I don't mind if they are squashed
when applied. In my mind it is better if the second patch can be
reverted easily if it ever is found to give any trouble (or just
increase code size). But I'm not the most knowledgeable person in
this list by a huge margin, so if mantainers apply it squashed I'll be
happy with my 2 USB2 ports working.

In fact I don't mind if u-boot takes v4, v3, v2 or v1 of this, or any
other solution, I just want 2 USB2 ports in Rock Pi 4B to be usable in
u-boot.  As it is now only the USB3 ports work in u-boot, unless one
enables CONFIG_OCHI_GENERIC, and then the USB2 ports would work slowly.

> Since 1/2 works without any clock operations, who does enable these usb
> clock on this SoC ? Is there any driver or platform code for that ? If so,
> you can drop that platform code with this driver-side implementation in
> place (which is nice).
>

I'm sorry if I'm not being clear.

I can't drop that code, it needs to enable 2 clocks, but it is
enabling 3.  I can remove one clock from the list of enabled clocks (I
did so in v1) or I can leave it and ignore the error (I did so in
v2). But since that didn't seem to please the list, I tried v3 and v4.
Maybe I just added noise and should have waited further opinions.

ehci-generic.c bulk enables all clocks in the phandle list assigned to the
usb host (usb_host_0ehci).
The arch/arm/dts/rk3399.dtsi provides such a list.

But the last clock is the usb phy &u2phy0. The phy is otherwise
enabled, and the output clock associated with it seems not to serve
any purpose, since u-boot works when I remove &u2phy0 from the clock
phandle list (v1), and also when I leave it there but ignore the error
for the missing clock (v2). In master, v1 and v2 &u2phy0 is
UCLASS_PHY, but not UCLASS_CLK.

With version v3 the &u2phy0 is bound as UCLASS_CLK too, so the bulk
enable from ehci-generic succeeds, although it does nothing to the
hardware (the clock driver having no operations). This does not have
any bad effect as far as I can see. The simple fact that ehci-generic
doesn't fail makes the 2 USB2 ports usable in u-boot.

With v4 the &u2phy0 is bound as UCLASS_CLK, it is bulk enabled, and
that ends up calling an operation that does touch the hardware. This
still doesn't have any bad effect, but I can't tell that it works, because
it does the same as in v3 or v2 or v1.

So when I say there are no users, I don't mean the code isn't called.
It is, but the behaviour I can see is the same when it is and when it
isn't, so whatever the enabled clock does, it does not seem that
u-boot is using it.

Why is it in the phandle list then ? Because it apparently helps linux
resume or suspend. When the output clock is not enabled, something
seems to block linux resume. But I'm not testing linux resume, and it
doesn't matter, because in linux the output clock is enabled anyway,
because &u2phy0 provides a clk, and is in the clock phandle list of
usb_host0_ehci).

Since u-boot and linux share rk3399.dtsi the &u2phy0 is in the phandle list
in u-boot too, but since in master it isn't UCLASS_CLK, ehci-generic fails
and USB2 is not available in that port. 

Maybe there is some use for the clock in u-boot that I'm not seeing because
I don't test good enough or I don't know enough about USB, or lack USB testing
equipment, or something. 

> btw I am still hoping some of the rockchip people will have a look at this
> series.

Yes, I'm looking forward to it too. So far Kever Yang reacted in late August
to v1 by proposing v2 (as far as I understand).

Thanks for listening to my walls of text.

--
Xavi

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

[Marek Vasut]

On 12/9/22 16:39, Xavier Drudis Ferran wrote:
> 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 the white port, 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.
> 
> 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.
> 
> 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.
> 
> This series is a second 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 (device_bind_driver() didn't work but
> device_bind_driver_to_node() did), 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 Philipp, Quentin

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

[Christoph Fritz]

Hello Xavier

> The first patch in the series (identical to v3) just registers usb2phy
> as a clock driver (device_bind_driver() didn't work but
> device_bind_driver_to_node() did), 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.
> 

I have tested both of your patches on an rk3399:

without patches applied:

  | starting USB...
  | Bus usb@fe380000: ehci_generic usb@fe380000: Failed to get clocks (ret=-19)
  | Port not available.

with patches applied:

  | starting USB...
  | Bus usb@fe380000: USB EHCI 1.00
  | Bus usb@fe3c0000: USB EHCI 1.00
  | Bus usb@fe800000: Register 2000140 NbrPorts 2


'usb stop' still makes u-boot hang, but with your patches applied
following output gets printed before:

  | => usb stop
  | stopping USB..
  | device_remove: Device 'usb@fe380000' failed to remove, but children are gone
  | device_remove: Device 'usb@fe3c0000' failed to remove, but children are gone
  <u-boot hangs>

Without CONFIG_USB_EHCI_HCD 'usb stop' works just fine.

Thanks
  -- Christoph

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

[Xavier Drudis Ferran]

El Sun, Feb 19, 2023 at 08:48:57PM +0100, Christoph Fritz deia:
> 
> I have tested both of your patches on an rk3399:
> 
> without patches applied:
> 
>   | starting USB...
>   | Bus usb@fe380000: ehci_generic usb@fe380000: Failed to get clocks (ret=-19)
>   | Port not available.
> 
> with patches applied:
> 
>   | starting USB...
>   | Bus usb@fe380000: USB EHCI 1.00
>   | Bus usb@fe3c0000: USB EHCI 1.00
>   | Bus usb@fe800000: Register 2000140 NbrPorts 2
> 
> 
> 'usb stop' still makes u-boot hang, but with your patches applied
> following output gets printed before:
> 
>   | => usb stop
>   | stopping USB..
>   | device_remove: Device 'usb@fe380000' failed to remove, but children are gone
>   | device_remove: Device 'usb@fe3c0000' failed to remove, but children are gone
>   <u-boot hangs>
> 
> Without CONFIG_USB_EHCI_HCD 'usb stop' works just fine.
> 
> Thanks
>   -- Christoph
> 

Thank you a lot for testing and reporting.

I think I found the cause. property_enable() returns an error code in
linux but just the value written to the register in U-Boot. So I'll be
sending an v5 with a change to ignore the return from
property_enable() when enabling and disabling the clock, just as other
calls in the file do.

Alternatively one could use v1, v2 or v3 of the patch. But since they
weren't accepted I'll try an v5.

I wellcome help testing or ideas on how to test whether an output
clock is correctly enabled or disabled when everything seems to work
fine already without enabling it, but it needs to be enabled simply
because of a dts coming from linux where it seems to have effects on
suspend/resume.

Thanks,

--
Xavier Drudis Ferran