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

[PATCH] 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. I just can't seem to imagine how to achieve this with the
   U-Boot driver model, maybe because of my limited familiarity with
   it.

This patch is option 3. Looked like the simplest and most performant.
But it might not be what's wanted, so comments wellcome.

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>
---
 arch/arm/dts/rk3399-u-boot.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi
index 716b9a433a..63c7b10405 100644
--- a/arch/arm/dts/rk3399-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-u-boot.dtsi
@@ -147,3 +147,11 @@
 &vopl {
 	u-boot,dm-pre-reloc;
 };
+
+&usb_host0_ehci {
+	clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>;
+};
+
+&usb_host1_ehci {
+	clocks = <&cru HCLK_HOST1>, <&cru HCLK_HOST1_ARB>;
+};
-- 
2.20.1

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

[Xavier Drudis Ferran]

I just checked this still works in next. No v2 needed (until feedback,
because I've heard about nodes providing two devices and maybe you
prefer that, so far I'd still see it too complex).

El Fri, Jul 01, 2022 at 08:59:59PM +0200, Xavier Drudis Ferran deia:
> 
> 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. I just can't seem to imagine how to achieve this with the
>    U-Boot driver model, maybe because of my limited familiarity with
>    it.
> 
> This patch is option 3. Looked like the simplest and most performant.
> But it might not be what's wanted, so comments wellcome.
> 
> 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>
> ---
>  arch/arm/dts/rk3399-u-boot.dtsi | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi
> index 716b9a433a..63c7b10405 100644
> --- a/arch/arm/dts/rk3399-u-boot.dtsi
> +++ b/arch/arm/dts/rk3399-u-boot.dtsi
> @@ -147,3 +147,11 @@
>  &vopl {
>  	u-boot,dm-pre-reloc;
>  };
> +
> +&usb_host0_ehci {
> +	clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>;
> +};
> +
> +&usb_host1_ehci {
> +	clocks = <&cru HCLK_HOST1>, <&cru HCLK_HOST1_ARB>;
> +};
> -- 
> 2.20.1
> 

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

[Kever Yang]

Hi Xavier,

On 2022/7/2 02:59, 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. I just can't seem to imagine how to achieve this with the
>     U-Boot driver model, maybe because of my limited familiarity with
>     it.
>
> This patch is option 3. Looked like the simplest and most performant.
> But it might not be what's wanted, so comments wellcome.
The idea for dtsi now is to sync the kernel and U-Boot dts, and Linux 
distribution will
use the dts from U-Boot. If we change the dts property in -u-boot.dts, 
it will also pass
to kernel, which make kernel function not available. So we would like to 
sync the driver
for kernel and u-boot to use the same dts nodes.

So we can try option 1, output warning instead of error return for clock 
get fail, since the clock
driver for kernel and u-boot are totally different.


Thanks,
- Kever
>
> 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>
> ---
>   arch/arm/dts/rk3399-u-boot.dtsi | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi
> index 716b9a433a..63c7b10405 100644
> --- a/arch/arm/dts/rk3399-u-boot.dtsi
> +++ b/arch/arm/dts/rk3399-u-boot.dtsi
> @@ -147,3 +147,11 @@
>   &vopl {
>   	u-boot,dm-pre-reloc;
>   };
> +
> +&usb_host0_ehci {
> +	clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>;
> +};
> +
> +&usb_host1_ehci {
> +	clocks = <&cru HCLK_HOST1>, <&cru HCLK_HOST1_ARB>;
> +};

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

[Xavier Drudis Ferran]

El Sat, Aug 27, 2022 at 11:20:17AM +0800, Kever Yang deia:
> The idea for dtsi now is to sync the kernel and U-Boot dts, and Linux
> distribution will
> use the dts from U-Boot. If we change the dts property in -u-boot.dts, it
> will also pass
> to kernel, which make kernel function not available. So we would like to
> sync the driver
> for kernel and u-boot to use the same dts nodes.
>

That's news to me. I was thinking all the time that the reason for having 
some dtsi? files with -u-boot suffix was that these were exclusive for u-boot
and the others were shared with linux. If now the content of .*-u-boot.dtsi? 
gets passed to linux too, I no longer know why we separate the files.
Is this for Rockchip or ARM64 or a general rule for all of U-Boot? 
Should I (or someone) try to change documentation or is that already done?
I thought I read something different few months ago ? Maybe I misunderstood...

> So we can try option 1, output warning instead of error return for clock get
> fail, since the clock
> driver for kernel and u-boot are totally different.
> 

Ok. I hope I find time to try that some day. 

Thanks for looking at the patch.