| Message ID | Y5IWpjYLB4aXMy9o@localhost |
|---|---|
| State | Superseded |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | [v3] arm: dts: rockchip: rk3399: usb: ehci: Fix EHCI probe in rk3399 to access peripherals by USB 2. | expand |
On 12/8/22 17:53, Xavier Drudis Ferran wrote: [...] > +++ 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 = { > +}; Is this empty structure needed ? Why ? Either it shouldn't be here, or it should implement some callbacks, like clock enable/disable ? Otherwise looks much better, thanks !
El Thu, Dec 08, 2022 at 09:12:08PM +0100, Marek Vasut deia: > On 12/8/22 17:53, Xavier Drudis Ferran wrote: > > [...] > > > +++ 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 = { > > +}; > > Is this empty structure needed ? Why ? > > Either it shouldn't be here, or it should implement some callbacks, like > clock enable/disable ? > I tried without it but it gave me a runtime error. I think I have the log somewhere if you want to see it. It looked like a null pointer dereference at first sight. I just added it and it got fixed. I didn't research what the failing functions were trying to do. Or is this a common case and there's some null_clk_ops() function or macro magic or something somewhere ? The thing is nobody is using this clk in u-boot. It's just its phandle in a clock phandle list that ehci-generic.c happens to use in bulk. So the default implementation seems to be enough to allocate, enable and release it in bulk with its clk set. Or at least to call the functions without error. As I have left it, it might not work if ever someone wants to use it. But should I try to implement it so that it is usable ? How should I test it ? Shouldn't we wait until someone has some real use for it ? Linux spent sometime without it in the phandle list until William wu discovered it was needed for suspend/resume, so u-boot may never need it. Or should I add a comment in the code ? > Otherwise looks much better, thanks ! Thanks.
On 12/8/22 22:00, Xavier Drudis Ferran wrote: > El Thu, Dec 08, 2022 at 09:12:08PM +0100, Marek Vasut deia: >> On 12/8/22 17:53, Xavier Drudis Ferran wrote: >> >> [...] >> >>> +++ 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 = { >>> +}; >> >> Is this empty structure needed ? Why ? >> >> Either it shouldn't be here, or it should implement some callbacks, like >> clock enable/disable ? >> > > I tried without it but it gave me a runtime error. I think I have the > log somewhere if you want to see it. It looked like a null pointer > dereference at first sight. I just added it and it got fixed. I > didn't research what the failing functions were trying to do. > > Or is this a common case and there's some null_clk_ops() function or > macro magic or something somewhere ? It seems Linux kernel drivers/phy/rockchip/phy-rockchip-inno-usb2.c does implement clock operations for this 480 MHz clock . > The thing is nobody is using this clk in u-boot. It's just its phandle > in a clock phandle list that ehci-generic.c happens to use in bulk. So > the default implementation seems to be enough to allocate, enable and > release it in bulk with its clk set. Or at least to call the functions > without error. > > As I have left it, it might not work if ever someone wants to use it. > But should I try to implement it so that it is usable ? How should I > test it ? Shouldn't we wait until someone has some real use for it ? > Linux spent sometime without it in the phandle list until William wu > discovered it was needed for suspend/resume, so u-boot may never need > it. > > Or should I add a comment in the code ? I would say, model the clock tree the same way Linux does model it. I know little about this hardware though.
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,
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 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/#2954536 [3] https://patchwork.ozlabs.org/project/uboot/patch/Y44+ayJfUlI08ptM@localhost/#3016099 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: 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). --- drivers/usb/host/ehci-generic.c | 59 ++++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) --- drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)