On 2023/6/5 23:05, 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 effectively 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 patch tries to implement option 5 as Marek Vasut requested in December 5th. Options 1 and 3 didn't get through [2][3].

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

Link: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/ [2] https://patchwork.ozlabs.org/project/uboot/patch/20220701185959.GC1700@begut... [3] https://patchwork.ozlabs.org/project/uboot/patch/Y44+ayJfUlI08ptM@localhost/

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 Cc: Jagan Teki jagan@amarulasolutions.com

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

Reviewed-by: Kever Yang kever.yang@rock-chips.com

Thanks, - Kever

---

V7: improve error handling. Call device_chld_unbind() on error. Remove unnecessary if.

v6: just retested over current next branch and some corrections to message and headers (no changes to code).

---

drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 33 +++++++++++++++++-- 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c index 55e1dbcfef..732d37201d 100644 --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c @@ -7,10 +7,11 @@ */

#include <common.h> -#include <clk.h> +#include <clk-uclass.h> #include <dm.h> #include <asm/global_data.h> #include <dm/device_compat.h> +#include <dm/device-internal.h> #include <dm/lists.h> #include <generic-phy.h> #include <reset.h> @@ -168,6 +169,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);

@@ -234,7 +238,8 @@ static int rockchip_usb2phy_bind(struct udevice *dev) dev_for_each_subnode(node, dev) { if (!ofnode_valid(node)) { dev_info(dev, "subnode %s not found\n", dev->name);

• 	return -ENXIO;
  

• 	ret = -ENXIO;
  

• 	goto bind_fail;
  

  }

  name = ofnode_get_name(node);

@@ -245,10 +250,26 @@ static int rockchip_usb2phy_bind(struct udevice *dev) if (ret) { dev_err(dev, "'%s' cannot bind 'rockchip_usb2phy_port'\n", name);

• 	return ret;
  

• 	goto bind_fail;
  

  } }

• 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);
  

• goto bind_fail;
  

• }

• return 0;

+bind_fail:

• device_chld_unbind(dev, NULL);
• return ret; }

@@ -366,6 +387,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,

On Mon, Jun 5, 2023 at 8:37 PM Xavier Drudis Ferran xdrudis@tinet.cat wrote:

This clock doesn't seem needed 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 use, it seems to at least not give any errors when called.

Link: [1] https://lkml.kernel.org/lkml/1731551.Q6cHK6n5ZM@phil/T/ [2] https://patchwork.ozlabs.org/project/uboot/patch/Y5IWpjYLB4aXMy9o@localhost/

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 Cc: Jagan Teki jagan@amarulasolutions.com

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

v7: add clkout_ctl values for rk3568 (from linux). UNTESTED (I don't have the hardware).

v6: just retested over current next branch and some corrections to message and headers (no changes to code).

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 | 80 ++++++++++++++++++- 1 file changed, 78 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c index 732d37201d..be5f79490c 100644 --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c @@ -56,6 +56,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];
  

};

@@ -77,6 +78,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) { @@ -169,7 +182,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.
• */

I forgot to comment, this last time. I feel these explicit comments wouldn't be required as clk-uclass clearly documented.

+ulong rockchip_usb2phy_clk_round_rate(struct clk *clk, ulong rate)

static

+{

•   return 480000000;
  

+}

+/**

• • enable() - Enable a clock.
• • @clk: The clock to manipulate.
• • Return: zero on success, or -ve error code.
• */

ditto.

+int rockchip_usb2phy_clk_enable(struct clk *clk)

ditto.

+{

•   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.
• */

ditto.

+int rockchip_usb2phy_clk_disable(struct clk *clk)

ditto.

Thanks, Jagan.