| Message ID | 202013db5a47ecbac4a53c360ed1ca91ca663996.1685974993.git.xdrudis@tinet.cat |
|---|---|
| State | Accepted |
| Commit | e81512ac30c154c320b54036919cd3a6f4cc1516 |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2 | expand |
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:35 PM Xavier Drudis Ferran <xdrudis@tinet.cat> 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> > --- Thanks for fixing USB from the last couple of releases. Reviewed-by: Jagan Teki <jagan@amarulasolutions.com> Tested-by: Jagan Teki <jagan@amarulasolutions.com> # rk3399, rk3328, rv1126
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,
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> --- 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(-)