Re: [PATCH v1 7/8] arm64: dts: rockchip: add rk356x dwc3 usb3 nodes

[Michael Riesch <michael.riesch@wolfvision.net>]

Hi Peter,

(It should be noted that there was a slight mishap in communications
between the two of us resulting in two series with the same goal. Now
let's clean up the mess :-)

Thanks for your series. Seeing that it contains more patches than mine
it probably makes sense to use your series as basis. Please Cc: me in
future iterations of this patch series and consider my comments below.

On 2/25/22 15:54, Peter Geis wrote:
> Add the dwc3 device nodes to the rk356x device trees.
> The rk3566 has one usb2 capable dwc3 otg controller and one usb3 capable
> dwc3 host controller.
> The rk3568 has one usb3 capable dwc3 otg controller and one usb3 capable
> dwc3 host controller.
> 
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3566.dtsi | 12 +++++++
>  arch/arm64/boot/dts/rockchip/rk3568.dtsi |  9 +++++
>  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 45 +++++++++++++++++++++++-
>  3 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3566.dtsi b/arch/arm64/boot/dts/rockchip/rk3566.dtsi
> index 3839eef5e4f7..8e8b52f58f44 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3566.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3566.dtsi
> @@ -6,6 +6,10 @@ / {
>  	compatible = "rockchip,rk3566";
>  };
>  
> +&pipegrf {
> +	compatible = "rockchip,rk3566-pipe-grf", "syscon";
> +};
> +
>  &power {
>  	power-domain@RK3568_PD_PIPE {
>  		reg = <RK3568_PD_PIPE>;
> @@ -18,3 +22,11 @@ power-domain@RK3568_PD_PIPE {
>  		#power-domain-cells = <0>;
>  	};
>  };
> +
> +&usbdrd30 {

I would really love to have some alignment with the other USB controllers

usb_host{0,1}_{e,o}hci

here. I am aware that older SoCs and the SDK are using these names and
it might be painful to have different versions to maintain at the
moment, but can we please agree on

usb_host0_xhci
usb_host1_xhci

or something like that?

> +	phys = <&usb2phy0_otg>;
> +	phy-names = "usb2-phy";
> +	extcon = <&usb2phy0>;
> +	maximum-speed = "high-speed";
> +	snps,dis_u2_susphy_quirk;
> +};
> diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> index 5b0f528d6818..77c044cbaaad 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> @@ -99,6 +99,10 @@ opp-1992000000 {
>  	};
>  };
>  
> +&pipegrf {
> +	compatible = "rockchip,rk3568-pipe-grf", "syscon";
> +};
> +
>  &power {
>  	power-domain@RK3568_PD_PIPE {
>  		reg = <RK3568_PD_PIPE>;
> @@ -114,3 +118,8 @@ power-domain@RK3568_PD_PIPE {
>  		#power-domain-cells = <0>;
>  	};
>  };
> +
> +&usbdrd30 {
> +	phys = <&usb2phy0_otg>, <&combphy0 PHY_TYPE_USB3>;
> +	phy-names = "usb2-phy", "usb3-phy";
> +};
> diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> index 84d5d607e693..4fae5b3b326e 100644
> --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> @@ -230,6 +230,50 @@ scmi_shmem: sram@0 {
>  		};
>  	};
>  
> +	usbdrd30: usbdrd@fcc00000 {
> +		compatible = "rockchip,rk3568-dwc3", "snps,dwc3";
> +		reg = <0x0 0xfcc00000 0x0 0x400000>;
> +		interrupts = <GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru CLK_USB3OTG0_REF>, <&cru CLK_USB3OTG0_SUSPEND>,
> +			 <&cru ACLK_USB3OTG0>, <&cru PCLK_PIPE>;
> +		clock-names = "ref_clk", "suspend_clk",
> +			      "bus_clk", "grf_clk";

Please consider Johan's comments on my first series. In my tests
removing the PCLK_PIPE clock did not make any difference.

> +		dr_mode = "host";

Based on the description in the commit message above it should be "otg",
right? Boards are free to overrule this, of course.

> +		phy_type = "utmi_wide";
> +		power-domains = <&power RK3568_PD_PIPE>;
> +		resets = <&cru SRST_USB3OTG0>;
> +		reset-names = "usb3-otg";
> +		snps,dis_enblslpm_quirk;
> +		snps,dis-u2-freeclk-exists-quirk;
> +		snps,dis-del-phy-power-chg-quirk;
> +		snps,dis-tx-ipgap-linecheck-quirk;
> +		snps,xhci-trb-ent-quirk;

In my first version I had all those quirks as well, but are they
actually necessary? I decided to remove them all to have a fresh start
(also activating them did not seem to affect my test setup).

> +		status = "disabled";
> +	};
> +
> +	usbhost30: usbhost@fd000000 {

Please reconsider the this name as well.

> +		compatible = "rockchip,rk3568-dwc3", "snps,dwc3";
> +		reg = <0x0 0xfd000000 0x0 0x400000>;
> +		interrupts = <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru CLK_USB3OTG1_REF>, <&cru CLK_USB3OTG1_SUSPEND>,
> +			 <&cru ACLK_USB3OTG1>, <&cru PCLK_PIPE>;
> +		clock-names = "ref_clk", "suspend_clk",
> +			      "bus_clk", "grf_clk";
> +		dr_mode = "host";

Here "host" clearly makes sense, as this controller is not capable of otg.

> +		phys = <&usb2phy0_host>, <&combphy1 PHY_TYPE_USB3>;
> +		phy-names = "usb2-phy", "usb3-phy";
> +		phy_type = "utmi_wide";
> +		power-domains = <&power RK3568_PD_PIPE>;
> +		resets = <&cru SRST_USB3OTG1>;
> +		reset-names = "usb3-host";
> +		snps,dis_enblslpm_quirk;
> +		snps,dis-u2-freeclk-exists-quirk;
> +		snps,dis_u2_susphy_quirk;
> +		snps,dis-del-phy-power-chg-quirk;
> +		snps,dis-tx-ipgap-linecheck-quirk;

What was said about quirks above holds here as well (although one quirk
not documented in the bindings is missing here).

Best regards,
Michael

> +		status = "disabled";
> +	};
> +
>  	gic: interrupt-controller@fd400000 {
>  		compatible = "arm,gic-v3";
>  		reg = <0x0 0xfd400000 0 0x10000>, /* GICD */
> @@ -297,7 +341,6 @@ pmu_io_domains: io-domains {
>  	};
>  
>  	pipegrf: syscon@fdc50000 {
> -		compatible = "rockchip,rk3568-pipe-grf", "syscon";
>  		reg = <0x0 0xfdc50000 0x0 0x1000>;
>  	};
>