diff mbox series

[v3,5/5] dts: usb: add StarFive JH7110 USB dts configuration.

Message ID 20230315104411.73614-6-minda.chen@starfivetech.com
State New
Headers
Series Add JH7110 USB and USB PHY driver support |

Commit Message

Minda Chen March 15, 2023, 10:44 a.m. UTC 
  USB Glue layer and Cadence USB subnode configuration,
also includes USB and PCIe phy dts configuration.

Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
---
 .../jh7110-starfive-visionfive-2.dtsi         |  7 +++
 arch/riscv/boot/dts/starfive/jh7110.dtsi      | 54 +++++++++++++++++++
 2 files changed, 61 insertions(+)

Comments

Peter Chen March 16, 2023, 2:43 a.m. UTC | #1 
On 23-03-15 18:44:11, Minda Chen wrote:
> USB Glue layer and Cadence USB subnode configuration,
> also includes USB and PCIe phy dts configuration.
> 
> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> ---
>  .../jh7110-starfive-visionfive-2.dtsi         |  7 +++
>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 54 +++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> index a132debb9b53..c64476aebc1a 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> @@ -236,3 +236,10 @@
>  	pinctrl-0 = <&uart0_pins>;
>  	status = "okay";
>  };
> +
> +&usb0 {
> +	status = "okay";
> +	usbdrd_cdns3: usb@0 {
> +		dr_mode = "peripheral";
> +	};
> +};
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index f70a4ed47eb4..17722fd1be62 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -362,6 +362,60 @@
>  			status = "disabled";
>  		};
>  
> +		usb0: usb@10100000 {
> +			compatible = "starfive,jh7110-usb";
> +			clocks = <&stgcrg JH7110_STGCLK_USB0_LPM>,
> +				 <&stgcrg JH7110_STGCLK_USB0_STB>,
> +				 <&stgcrg JH7110_STGCLK_USB0_APB>,
> +				 <&stgcrg JH7110_STGCLK_USB0_AXI>,
> +				 <&stgcrg JH7110_STGCLK_USB0_UTMI_APB>;
> +			clock-names = "lpm", "stb", "apb", "axi", "utmi_apb";
> +			resets = <&stgcrg JH7110_STGRST_USB0_PWRUP>,
> +				 <&stgcrg JH7110_STGRST_USB0_APB>,
> +				 <&stgcrg JH7110_STGRST_USB0_AXI>,
> +				 <&stgcrg JH7110_STGRST_USB0_UTMI_APB>;
> +			starfive,stg-syscon = <&stg_syscon 0x4 0xc4 0x148 0x1f4>;
> +			starfive,sys-syscon = <&sys_syscon 0x18>;
> +			status = "disabled";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges = <0x0 0x0 0x10100000 0x100000>;

Why it is four entry at ranges? Your address-cells and size-cells are
both 1, and your binding-doc is also three?

Peter
> +
> +			usbdrd_cdns3: usb@0 {
> +				compatible = "cdns,usb3";
> +				reg = <0x0 0x10000>,
> +				      <0x10000 0x10000>,
> +				      <0x20000 0x10000>;
> +				reg-names = "otg", "xhci", "dev";
> +				interrupts = <100>, <108>, <110>;
> +				interrupt-names = "host", "peripheral", "otg";
> +				phys = <&usbphy0>;
> +				phy-names = "cdns3,usb2-phy";
> +				maximum-speed = "super-speed";
> +			};
> +		};
> +
> +		usbphy0: phy@10200000 {
> +			compatible = "starfive,jh7110-usb-phy";
> +			reg = <0x0 0x10200000 0x0 0x10000>;
> +			clocks = <&syscrg JH7110_SYSCLK_USB_125M>,
> +				 <&stgcrg JH7110_STGCLK_USB0_APP_125>;
> +			clock-names = "125m", "app_125";
> +			#phy-cells = <0>;
> +		};
> +
> +		pciephy0: phy@10210000 {
> +			compatible = "starfive,jh7110-pcie-phy";
> +			reg = <0x0 0x10210000 0x0 0x10000>;
> +			#phy-cells = <0>;
> +		};
> +
> +		pciephy1: phy@10220000 {
> +			compatible = "starfive,jh7110-pcie-phy";
> +			reg = <0x0 0x10220000 0x0 0x10000>;
> +			#phy-cells = <0>;
> +		};
> +
>  		stgcrg: clock-controller@10230000 {
>  			compatible = "starfive,jh7110-stgcrg";
>  			reg = <0x0 0x10230000 0x0 0x10000>;
> -- 
> 2.17.1
>
Minda Chen March 16, 2023, 3:02 a.m. UTC | #2 
On 2023/3/16 10:43, Peter Chen wrote:
> On 23-03-15 18:44:11, Minda Chen wrote:
>> USB Glue layer and Cadence USB subnode configuration,
>> also includes USB and PCIe phy dts configuration.
>> 
>> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
>> ---
>>  .../jh7110-starfive-visionfive-2.dtsi         |  7 +++
>>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 54 +++++++++++++++++++
>>  2 files changed, 61 insertions(+)
>> 
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>> index a132debb9b53..c64476aebc1a 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>> @@ -236,3 +236,10 @@
>>  	pinctrl-0 = <&uart0_pins>;
>>  	status = "okay";
>>  };
>> +
>> +&usb0 {
>> +	status = "okay";
>> +	usbdrd_cdns3: usb@0 {
>> +		dr_mode = "peripheral";
>> +	};
>> +};
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> index f70a4ed47eb4..17722fd1be62 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> @@ -362,6 +362,60 @@
>>  			status = "disabled";
>>  		};
>>  
>> +		usb0: usb@10100000 {
>> +			compatible = "starfive,jh7110-usb";
>> +			clocks = <&stgcrg JH7110_STGCLK_USB0_LPM>,
>> +				 <&stgcrg JH7110_STGCLK_USB0_STB>,
>> +				 <&stgcrg JH7110_STGCLK_USB0_APB>,
>> +				 <&stgcrg JH7110_STGCLK_USB0_AXI>,
>> +				 <&stgcrg JH7110_STGCLK_USB0_UTMI_APB>;
>> +			clock-names = "lpm", "stb", "apb", "axi", "utmi_apb";
>> +			resets = <&stgcrg JH7110_STGRST_USB0_PWRUP>,
>> +				 <&stgcrg JH7110_STGRST_USB0_APB>,
>> +				 <&stgcrg JH7110_STGRST_USB0_AXI>,
>> +				 <&stgcrg JH7110_STGRST_USB0_UTMI_APB>;
>> +			starfive,stg-syscon = <&stg_syscon 0x4 0xc4 0x148 0x1f4>;
>> +			starfive,sys-syscon = <&sys_syscon 0x18>;
>> +			status = "disabled";
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			ranges = <0x0 0x0 0x10100000 0x100000>;
> 
> Why it is four entry at ranges? Your address-cells and size-cells are
> both 1, and your binding-doc is also three?
> 
> Peter
Because the parent soc node address-cells is 2. So the local address is 2 entry.
  	
	soc {
                compatible = "simple-bus";
                interrupt-parent = <&plic>;
                #address-cells = <2>;
                #size-cells = <2>;
                ranges;
                ...

So should I change the binding-doc ?
>> +
>> +			usbdrd_cdns3: usb@0 {
>> +				compatible = "cdns,usb3";
>> +				reg = <0x0 0x10000>,
>> +				      <0x10000 0x10000>,
>> +				      <0x20000 0x10000>;
>> +				reg-names = "otg", "xhci", "dev";
>> +				interrupts = <100>, <108>, <110>;
>> +				interrupt-names = "host", "peripheral", "otg";
>> +				phys = <&usbphy0>;
>> +				phy-names = "cdns3,usb2-phy";
>> +				maximum-speed = "super-speed";
>> +			};
>> +		};
>> +
>> +		usbphy0: phy@10200000 {
>> +			compatible = "starfive,jh7110-usb-phy";
>> +			reg = <0x0 0x10200000 0x0 0x10000>;
>> +			clocks = <&syscrg JH7110_SYSCLK_USB_125M>,
>> +				 <&stgcrg JH7110_STGCLK_USB0_APP_125>;
>> +			clock-names = "125m", "app_125";
>> +			#phy-cells = <0>;
>> +		};
>> +
>> +		pciephy0: phy@10210000 {
>> +			compatible = "starfive,jh7110-pcie-phy";
>> +			reg = <0x0 0x10210000 0x0 0x10000>;
>> +			#phy-cells = <0>;
>> +		};
>> +
>> +		pciephy1: phy@10220000 {
>> +			compatible = "starfive,jh7110-pcie-phy";
>> +			reg = <0x0 0x10220000 0x0 0x10000>;
>> +			#phy-cells = <0>;
>> +		};
>> +
>>  		stgcrg: clock-controller@10230000 {
>>  			compatible = "starfive,jh7110-stgcrg";
>>  			reg = <0x0 0x10230000 0x0 0x10000>;
>> -- 
>> 2.17.1
>> 
>
Krzysztof Kozlowski March 17, 2023, 8:44 a.m. UTC | #3 
On 15/03/2023 11:44, Minda Chen wrote:
> USB Glue layer and Cadence USB subnode configuration,
> also includes USB and PCIe phy dts configuration.
> 
> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

> ---
>  .../jh7110-starfive-visionfive-2.dtsi         |  7 +++
>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 54 +++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> index a132debb9b53..c64476aebc1a 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> @@ -236,3 +236,10 @@
>  	pinctrl-0 = <&uart0_pins>;
>  	status = "okay";
>  };
> +
> +&usb0 {
> +	status = "okay";
> +	usbdrd_cdns3: usb@0 {

You should rather override by phandle this as well.

> +		dr_mode = "peripheral";
> +	};
> +};
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index f70a4ed47eb4..17722fd1be62 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -362,6 +362,60 @@
>  			status = "disabled";
>  		};
>  
> +		usb0: usb@10100000 {
> +			compatible = "starfive,jh7110-usb";
> +			clocks = <&stgcrg JH7110_STGCLK_USB0_LPM>,
> +				 <&stgcrg JH7110_STGCLK_USB0_STB>,
> +				 <&stgcrg JH7110_STGCLK_USB0_APB>,
> +				 <&stgcrg JH7110_STGCLK_USB0_AXI>,
> +				 <&stgcrg JH7110_STGCLK_USB0_UTMI_APB>;
> +			clock-names = "lpm", "stb", "apb", "axi", "utmi_apb";
> +			resets = <&stgcrg JH7110_STGRST_USB0_PWRUP>,
> +				 <&stgcrg JH7110_STGRST_USB0_APB>,
> +				 <&stgcrg JH7110_STGRST_USB0_AXI>,
> +				 <&stgcrg JH7110_STGRST_USB0_UTMI_APB>;
> +			starfive,stg-syscon = <&stg_syscon 0x4 0xc4 0x148 0x1f4>;
> +			starfive,sys-syscon = <&sys_syscon 0x18>;
> +			status = "disabled";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges = <0x0 0x0 0x10100000 0x100000>;

reg and ranges should be second property. This also applies to your
binding example.


Best regards,
Krzysztof
Minda Chen March 17, 2023, 10:59 a.m. UTC | #4 
On 2023/3/17 16:44, Krzysztof Kozlowski wrote:
> On 15/03/2023 11:44, Minda Chen wrote:
>> USB Glue layer and Cadence USB subnode configuration,
>> also includes USB and PCIe phy dts configuration.
>> 
>> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> 
> Use subject prefixes matching the subsystem (which you can get for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching).
>ok
>> ---
>>  .../jh7110-starfive-visionfive-2.dtsi         |  7 +++
>>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 54 +++++++++++++++++++
>>  2 files changed, 61 insertions(+)
>> 
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>> index a132debb9b53..c64476aebc1a 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>> @@ -236,3 +236,10 @@
>>  	pinctrl-0 = <&uart0_pins>;
>>  	status = "okay";
>>  };
>> +
>> +&usb0 {
>> +	status = "okay";
>> +	usbdrd_cdns3: usb@0 {
> 
> You should rather override by phandle this as well.
> 
I will remove the wrapper node 'usb0'. 

Just like this :
+&usbdrd_cdns3 {
+       dr_mode = "peripheral";
+};


（similar configuration in arch/arm64/boot/dts/ti/k3-j721e-sk.dts, usb1 node)
   &usb1 {
        dr_mode = "host";
        ....
  
>> +		dr_mode = "peripheral";
>> +	};
>> +};
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> index f70a4ed47eb4..17722fd1be62 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> @@ -362,6 +362,60 @@
>>  			status = "disabled";
>>  		};
>>  
>> +		usb0: usb@10100000 {
>> +			compatible = "starfive,jh7110-usb";
>> +			clocks = <&stgcrg JH7110_STGCLK_USB0_LPM>,
>> +				 <&stgcrg JH7110_STGCLK_USB0_STB>,
>> +				 <&stgcrg JH7110_STGCLK_USB0_APB>,
>> +				 <&stgcrg JH7110_STGCLK_USB0_AXI>,
>> +				 <&stgcrg JH7110_STGCLK_USB0_UTMI_APB>;
>> +			clock-names = "lpm", "stb", "apb", "axi", "utmi_apb";
>> +			resets = <&stgcrg JH7110_STGRST_USB0_PWRUP>,
>> +				 <&stgcrg JH7110_STGRST_USB0_APB>,
>> +				 <&stgcrg JH7110_STGRST_USB0_AXI>,
>> +				 <&stgcrg JH7110_STGRST_USB0_UTMI_APB>;
>> +			starfive,stg-syscon = <&stg_syscon 0x4 0xc4 0x148 0x1f4>;
>> +			starfive,sys-syscon = <&sys_syscon 0x18>;
>> +			status = "disabled";
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			ranges = <0x0 0x0 0x10100000 0x100000>;
> 
> reg and ranges should be second property. This also applies to your
> binding example.
> 
> 
ok, thanks
> Best regards,
> Krzysztof
>
Rob Herring March 20, 2023, 3:34 p.m. UTC | #5 
On Wed, Mar 15, 2023 at 06:44:11PM +0800, Minda Chen wrote:
> USB Glue layer and Cadence USB subnode configuration,
> also includes USB and PCIe phy dts configuration.
> 
> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> ---
>  .../jh7110-starfive-visionfive-2.dtsi         |  7 +++
>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 54 +++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> index a132debb9b53..c64476aebc1a 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> @@ -236,3 +236,10 @@
>  	pinctrl-0 = <&uart0_pins>;
>  	status = "okay";
>  };
> +
> +&usb0 {
> +	status = "okay";
> +	usbdrd_cdns3: usb@0 {
> +		dr_mode = "peripheral";
> +	};
> +};
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index f70a4ed47eb4..17722fd1be62 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -362,6 +362,60 @@
>  			status = "disabled";
>  		};
>  
> +		usb0: usb@10100000 {
> +			compatible = "starfive,jh7110-usb";
> +			clocks = <&stgcrg JH7110_STGCLK_USB0_LPM>,
> +				 <&stgcrg JH7110_STGCLK_USB0_STB>,
> +				 <&stgcrg JH7110_STGCLK_USB0_APB>,
> +				 <&stgcrg JH7110_STGCLK_USB0_AXI>,
> +				 <&stgcrg JH7110_STGCLK_USB0_UTMI_APB>;
> +			clock-names = "lpm", "stb", "apb", "axi", "utmi_apb";
> +			resets = <&stgcrg JH7110_STGRST_USB0_PWRUP>,
> +				 <&stgcrg JH7110_STGRST_USB0_APB>,
> +				 <&stgcrg JH7110_STGRST_USB0_AXI>,
> +				 <&stgcrg JH7110_STGRST_USB0_UTMI_APB>;
> +			starfive,stg-syscon = <&stg_syscon 0x4 0xc4 0x148 0x1f4>;
> +			starfive,sys-syscon = <&sys_syscon 0x18>;
> +			status = "disabled";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges = <0x0 0x0 0x10100000 0x100000>;
> +
> +			usbdrd_cdns3: usb@0 {
> +				compatible = "cdns,usb3";

This pattern of USB wrapper and then a "generic" IP node is discouraged 
if it is just clocks, resets, power-domains, etc. IOW, unless there's an 
actual wrapper h/w block with its own registers, then don't do this 
split. Merge it all into a single node.

> +				reg = <0x0 0x10000>,
> +				      <0x10000 0x10000>,
> +				      <0x20000 0x10000>;
> +				reg-names = "otg", "xhci", "dev";
> +				interrupts = <100>, <108>, <110>;
> +				interrupt-names = "host", "peripheral", "otg";
> +				phys = <&usbphy0>;
> +				phy-names = "cdns3,usb2-phy";

No need for *-names when there is only 1 entry. Names are local to the 
device and only to distinguish entries, so 'usb2' would be sufficient 
here.

> +				maximum-speed = "super-speed";
> +			};
> +		};
> +
> +		usbphy0: phy@10200000 {
> +			compatible = "starfive,jh7110-usb-phy";
> +			reg = <0x0 0x10200000 0x0 0x10000>;
> +			clocks = <&syscrg JH7110_SYSCLK_USB_125M>,
> +				 <&stgcrg JH7110_STGCLK_USB0_APP_125>;
> +			clock-names = "125m", "app_125";
> +			#phy-cells = <0>;
> +		};
> +
> +		pciephy0: phy@10210000 {
> +			compatible = "starfive,jh7110-pcie-phy";
> +			reg = <0x0 0x10210000 0x0 0x10000>;
> +			#phy-cells = <0>;
> +		};
> +
> +		pciephy1: phy@10220000 {
> +			compatible = "starfive,jh7110-pcie-phy";
> +			reg = <0x0 0x10220000 0x0 0x10000>;
> +			#phy-cells = <0>;
> +		};
> +
>  		stgcrg: clock-controller@10230000 {
>  			compatible = "starfive,jh7110-stgcrg";
>  			reg = <0x0 0x10230000 0x0 0x10000>;
> -- 
> 2.17.1
>
Minda Chen March 21, 2023, 12:35 p.m. UTC | #6 
On 2023/3/20 23:34, Rob Herring wrote:
> On Wed, Mar 15, 2023 at 06:44:11PM +0800, Minda Chen wrote:
>> USB Glue layer and Cadence USB subnode configuration,
>> also includes USB and PCIe phy dts configuration.
>> 
>> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
>> ---
>>  .../jh7110-starfive-visionfive-2.dtsi         |  7 +++
>>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 54 +++++++++++++++++++
>>  2 files changed, 61 insertions(+)
>> 
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>> index a132debb9b53..c64476aebc1a 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>> @@ -236,3 +236,10 @@
>>  	pinctrl-0 = <&uart0_pins>;
>>  	status = "okay";
>>  };
>> +
>> +&usb0 {
>> +	status = "okay";
>> +	usbdrd_cdns3: usb@0 {
>> +		dr_mode = "peripheral";
>> +	};
>> +};
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> index f70a4ed47eb4..17722fd1be62 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> @@ -362,6 +362,60 @@
>>  			status = "disabled";
>>  		};
>>  
>> +		usb0: usb@10100000 {
>> +			compatible = "starfive,jh7110-usb";
>> +			clocks = <&stgcrg JH7110_STGCLK_USB0_LPM>,
>> +				 <&stgcrg JH7110_STGCLK_USB0_STB>,
>> +				 <&stgcrg JH7110_STGCLK_USB0_APB>,
>> +				 <&stgcrg JH7110_STGCLK_USB0_AXI>,
>> +				 <&stgcrg JH7110_STGCLK_USB0_UTMI_APB>;
>> +			clock-names = "lpm", "stb", "apb", "axi", "utmi_apb";
>> +			resets = <&stgcrg JH7110_STGRST_USB0_PWRUP>,
>> +				 <&stgcrg JH7110_STGRST_USB0_APB>,
>> +				 <&stgcrg JH7110_STGRST_USB0_AXI>,
>> +				 <&stgcrg JH7110_STGRST_USB0_UTMI_APB>;
>> +			starfive,stg-syscon = <&stg_syscon 0x4 0xc4 0x148 0x1f4>;
>> +			starfive,sys-syscon = <&sys_syscon 0x18>;
>> +			status = "disabled";
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			ranges = <0x0 0x0 0x10100000 0x100000>;
>> +
>> +			usbdrd_cdns3: usb@0 {
>> +				compatible = "cdns,usb3";
> 
> This pattern of USB wrapper and then a "generic" IP node is discouraged 
> if it is just clocks, resets, power-domains, etc. IOW, unless there's an 
> actual wrapper h/w block with its own registers, then don't do this 
> split. Merge it all into a single node.
> 
I am afraid it is difficult to merge in one single node. 

1.If cadence3 usb device is still the sub device. All the dts setting are in
StarFive node. This can not work.
StarFive driver code Using platform_device_add generate cadenc3 usb platform device. 
Even IO memory space setting can be passed to cadence3 USB, PHY setting can not be passed.
For the PHY driver using dts now. But in this case, Cadence3 USB no dts configure.

2. Just one USB Cadence platform device.
Maybe this can work. But Cadence USB driver code cdns3-plat.c required to changed.

Hi Peter Pawel and Roger
   There is a "platform_suspend" function pointer in "struct cdns3_platform_data"，
   Add "platform_init" and "platform_exit" for our JH7110 platform. Maybe it can work.
   Is it OK?   
>> +				reg = <0x0 0x10000>,
>> +				      <0x10000 0x10000>,
>> +				      <0x20000 0x10000>;
>> +				reg-names = "otg", "xhci", "dev";
>> +				interrupts = <100>, <108>, <110>;
>> +				interrupt-names = "host", "peripheral", "otg";
>> +				phys = <&usbphy0>;
>> +				phy-names = "cdns3,usb2-phy";
> 
> No need for *-names when there is only 1 entry. Names are local to the 
> device and only to distinguish entries, so 'usb2' would be sufficient 
> here.
> 
The PHY name 'cdns3,usb2-phy'  is defined in cadence3 usb driver code.
Cadence USB3 driver code using this name to get PHY instance.
And all the PHY ops used in Cadence3 USB sub device. 
>> +				maximum-speed = "super-speed";
>> +			};
>> +		};
>> +
>> +		usbphy0: phy@10200000 {
>> +			compatible = "starfive,jh7110-usb-phy";
>> +			reg = <0x0 0x10200000 0x0 0x10000>;
>> +			clocks = <&syscrg JH7110_SYSCLK_USB_125M>,
>> +				 <&stgcrg JH7110_STGCLK_USB0_APP_125>;
>> +			clock-names = "125m", "app_125";
>> +			#phy-cells = <0>;
>> +		};
>> +
>> +		pciephy0: phy@10210000 {
>> +			compatible = "starfive,jh7110-pcie-phy";
>> +			reg = <0x0 0x10210000 0x0 0x10000>;
>> +			#phy-cells = <0>;
>> +		};
>> +
>> +		pciephy1: phy@10220000 {
>> +			compatible = "starfive,jh7110-pcie-phy";
>> +			reg = <0x0 0x10220000 0x0 0x10000>;
>> +			#phy-cells = <0>;
>> +		};
>> +
>>  		stgcrg: clock-controller@10230000 {
>>  			compatible = "starfive,jh7110-stgcrg";
>>  			reg = <0x0 0x10230000 0x0 0x10000>;
>> -- 
>> 2.17.1
>>
Roger Quadros March 22, 2023, 8 a.m. UTC | #7 
Hi Minda,

On 21/03/2023 14:35, Minda Chen wrote:
> 
> 
> On 2023/3/20 23:34, Rob Herring wrote:
>> On Wed, Mar 15, 2023 at 06:44:11PM +0800, Minda Chen wrote:
>>> USB Glue layer and Cadence USB subnode configuration,
>>> also includes USB and PCIe phy dts configuration.
>>>
>>> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
>>> ---
>>>  .../jh7110-starfive-visionfive-2.dtsi         |  7 +++
>>>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 54 +++++++++++++++++++
>>>  2 files changed, 61 insertions(+)
>>>
>>> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>>> index a132debb9b53..c64476aebc1a 100644
>>> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>>> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>>> @@ -236,3 +236,10 @@
>>>  	pinctrl-0 = <&uart0_pins>;
>>>  	status = "okay";
>>>  };
>>> +
>>> +&usb0 {
>>> +	status = "okay";
>>> +	usbdrd_cdns3: usb@0 {
>>> +		dr_mode = "peripheral";
>>> +	};
>>> +};
>>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>> index f70a4ed47eb4..17722fd1be62 100644
>>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>> @@ -362,6 +362,60 @@
>>>  			status = "disabled";
>>>  		};
>>>  
>>> +		usb0: usb@10100000 {
>>> +			compatible = "starfive,jh7110-usb";
>>> +			clocks = <&stgcrg JH7110_STGCLK_USB0_LPM>,
>>> +				 <&stgcrg JH7110_STGCLK_USB0_STB>,
>>> +				 <&stgcrg JH7110_STGCLK_USB0_APB>,
>>> +				 <&stgcrg JH7110_STGCLK_USB0_AXI>,
>>> +				 <&stgcrg JH7110_STGCLK_USB0_UTMI_APB>;
>>> +			clock-names = "lpm", "stb", "apb", "axi", "utmi_apb";
>>> +			resets = <&stgcrg JH7110_STGRST_USB0_PWRUP>,
>>> +				 <&stgcrg JH7110_STGRST_USB0_APB>,
>>> +				 <&stgcrg JH7110_STGRST_USB0_AXI>,
>>> +				 <&stgcrg JH7110_STGRST_USB0_UTMI_APB>;
>>> +			starfive,stg-syscon = <&stg_syscon 0x4 0xc4 0x148 0x1f4>;
>>> +			starfive,sys-syscon = <&sys_syscon 0x18>;
>>> +			status = "disabled";
>>> +			#address-cells = <1>;
>>> +			#size-cells = <1>;
>>> +			ranges = <0x0 0x0 0x10100000 0x100000>;
>>> +
>>> +			usbdrd_cdns3: usb@0 {
>>> +				compatible = "cdns,usb3";
>>
>> This pattern of USB wrapper and then a "generic" IP node is discouraged 
>> if it is just clocks, resets, power-domains, etc. IOW, unless there's an 
>> actual wrapper h/w block with its own registers, then don't do this 
>> split. Merge it all into a single node.
>>
> I am afraid it is difficult to merge in one single node. 
> 
> 1.If cadence3 usb device is still the sub device. All the dts setting are in
> StarFive node. This can not work.
> StarFive driver code Using platform_device_add generate cadenc3 usb platform device. 
> Even IO memory space setting can be passed to cadence3 USB, PHY setting can not be passed.
> For the PHY driver using dts now. But in this case, Cadence3 USB no dts configure.
> 
> 2. Just one USB Cadence platform device.
> Maybe this can work. But Cadence USB driver code cdns3-plat.c required to changed.
> 
> Hi Peter Pawel and Roger
>    There is a "platform_suspend" function pointer in "struct cdns3_platform_data"，
>    Add "platform_init" and "platform_exit" for our JH7110 platform. Maybe it can work.
>    Is it OK?   

Once you move all the syscon register modifications from your wrapper driver
to your PHY driver, only clock and reset control are left in your wrapper driver.
This is generic enough to be done in the cdns3,usb driver itself so you don't need a
wrapper node.

Pawel, do you agree?
 
>>> +				reg = <0x0 0x10000>,
>>> +				      <0x10000 0x10000>,
>>> +				      <0x20000 0x10000>;
>>> +				reg-names = "otg", "xhci", "dev";
>>> +				interrupts = <100>, <108>, <110>;
>>> +				interrupt-names = "host", "peripheral", "otg";
>>> +				phys = <&usbphy0>;
>>> +				phy-names = "cdns3,usb2-phy";
>>
>> No need for *-names when there is only 1 entry. Names are local to the 
>> device and only to distinguish entries, so 'usb2' would be sufficient 
>> here.
>>
> The PHY name 'cdns3,usb2-phy'  is defined in cadence3 usb driver code.
> Cadence USB3 driver code using this name to get PHY instance.
> And all the PHY ops used in Cadence3 USB sub device. 
>>> +				maximum-speed = "super-speed";
>>> +			};
>>> +		};
>>> +
>>> +		usbphy0: phy@10200000 {
>>> +			compatible = "starfive,jh7110-usb-phy";
>>> +			reg = <0x0 0x10200000 0x0 0x10000>;
>>> +			clocks = <&syscrg JH7110_SYSCLK_USB_125M>,
>>> +				 <&stgcrg JH7110_STGCLK_USB0_APP_125>;
>>> +			clock-names = "125m", "app_125";
>>> +			#phy-cells = <0>;
>>> +		};
>>> +
>>> +		pciephy0: phy@10210000 {
>>> +			compatible = "starfive,jh7110-pcie-phy";
>>> +			reg = <0x0 0x10210000 0x0 0x10000>;
>>> +			#phy-cells = <0>;
>>> +		};
>>> +
>>> +		pciephy1: phy@10220000 {
>>> +			compatible = "starfive,jh7110-pcie-phy";
>>> +			reg = <0x0 0x10220000 0x0 0x10000>;
>>> +			#phy-cells = <0>;
>>> +		};
>>> +
>>>  		stgcrg: clock-controller@10230000 {
>>>  			compatible = "starfive,jh7110-stgcrg";
>>>  			reg = <0x0 0x10230000 0x0 0x10000>;
>>> -- 
>>> 2.17.1
>>>

cheers,
-roger
Minda Chen March 22, 2023, 10:50 a.m. UTC | #8 
On 2023/3/22 16:00, Roger Quadros wrote:
> Hi Minda,
> 
> On 21/03/2023 14:35, Minda Chen wrote:
>> 
>> 
>> On 2023/3/20 23:34, Rob Herring wrote:
>>> On Wed, Mar 15, 2023 at 06:44:11PM +0800, Minda Chen wrote:
>>>> USB Glue layer and Cadence USB subnode configuration,
>>>> also includes USB and PCIe phy dts configuration.
>>>>
>>>> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
>>>> ---
>>>>  .../jh7110-starfive-visionfive-2.dtsi         |  7 +++
>>>>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 54 +++++++++++++++++++
>>>>  2 files changed, 61 insertions(+)
>>>>
>>>> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>>>> index a132debb9b53..c64476aebc1a 100644
>>>> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>>>> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>>>> @@ -236,3 +236,10 @@
>>>>  	pinctrl-0 = <&uart0_pins>;
>>>>  	status = "okay";
>>>>  };
>>>> +
>>>> +&usb0 {
>>>> +	status = "okay";
>>>> +	usbdrd_cdns3: usb@0 {
>>>> +		dr_mode = "peripheral";
>>>> +	};
>>>> +};
>>>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>>> index f70a4ed47eb4..17722fd1be62 100644
>>>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>>> @@ -362,6 +362,60 @@
>>>>  			status = "disabled";
>>>>  		};
>>>>  
>>>> +		usb0: usb@10100000 {
>>>> +			compatible = "starfive,jh7110-usb";
>>>> +			clocks = <&stgcrg JH7110_STGCLK_USB0_LPM>,
>>>> +				 <&stgcrg JH7110_STGCLK_USB0_STB>,
>>>> +				 <&stgcrg JH7110_STGCLK_USB0_APB>,
>>>> +				 <&stgcrg JH7110_STGCLK_USB0_AXI>,
>>>> +				 <&stgcrg JH7110_STGCLK_USB0_UTMI_APB>;
>>>> +			clock-names = "lpm", "stb", "apb", "axi", "utmi_apb";
>>>> +			resets = <&stgcrg JH7110_STGRST_USB0_PWRUP>,
>>>> +				 <&stgcrg JH7110_STGRST_USB0_APB>,
>>>> +				 <&stgcrg JH7110_STGRST_USB0_AXI>,
>>>> +				 <&stgcrg JH7110_STGRST_USB0_UTMI_APB>;
>>>> +			starfive,stg-syscon = <&stg_syscon 0x4 0xc4 0x148 0x1f4>;
>>>> +			starfive,sys-syscon = <&sys_syscon 0x18>;
>>>> +			status = "disabled";
>>>> +			#address-cells = <1>;
>>>> +			#size-cells = <1>;
>>>> +			ranges = <0x0 0x0 0x10100000 0x100000>;
>>>> +
>>>> +			usbdrd_cdns3: usb@0 {
>>>> +				compatible = "cdns,usb3";
>>>
>>> This pattern of USB wrapper and then a "generic" IP node is discouraged 
>>> if it is just clocks, resets, power-domains, etc. IOW, unless there's an 
>>> actual wrapper h/w block with its own registers, then don't do this 
>>> split. Merge it all into a single node.
>>>
>> I am afraid it is difficult to merge in one single node. 
>> 
>> 1.If cadence3 usb device is still the sub device. All the dts setting are in
>> StarFive node. This can not work.
>> StarFive driver code Using platform_device_add generate cadenc3 usb platform device. 
>> Even IO memory space setting can be passed to cadence3 USB, PHY setting can not be passed.
>> For the PHY driver using dts now. But in this case, Cadence3 USB no dts configure.
>> 
>> 2. Just one USB Cadence platform device.
>> Maybe this can work. But Cadence USB driver code cdns3-plat.c required to changed.
>> 
>> Hi Peter Pawel and Roger
>>    There is a "platform_suspend" function pointer in "struct cdns3_platform_data"，
>>    Add "platform_init" and "platform_exit" for our JH7110 platform. Maybe it can work.
>>    Is it OK?   
> 
> Once you move all the syscon register modifications from your wrapper driver
> to your PHY driver, only clock and reset control are left in your wrapper driver.
> This is generic enough to be done in the cdns3,usb driver itself so you don't need a
> wrapper node.
> 
> Pawel, do you agree?
>  
move all the syscon codes to PHY driver is ok. I found dwc3/dwc3-of-simple.c is public codes
and contain just clock and reset control codes. I can change the residual codes to public codes.
But I found rockchip 3399 usb dts which is one of dwc3 simple platform still contain two nodes.
See Documentation/devicetree/bindings/usb/rockchip,rk3399-dwc3.yaml
>>>> +				reg = <0x0 0x10000>,
>>>> +				      <0x10000 0x10000>,
>>>> +				      <0x20000 0x10000>;
>>>> +				reg-names = "otg", "xhci", "dev";
>>>> +				interrupts = <100>, <108>, <110>;
>>>> +				interrupt-names = "host", "peripheral", "otg";
>>>> +				phys = <&usbphy0>;
>>>> +				phy-names = "cdns3,usb2-phy";
>>>
>>> No need for *-names when there is only 1 entry. Names are local to the 
>>> device and only to distinguish entries, so 'usb2' would be sufficient 
>>> here.
>>>
>> The PHY name 'cdns3,usb2-phy'  is defined in cadence3 usb driver code.
>> Cadence USB3 driver code using this name to get PHY instance.
>> And all the PHY ops used in Cadence3 USB sub device. 
>>>> +				maximum-speed = "super-speed";
>>>> +			};
>>>> +		};
>>>> +
>>>> +		usbphy0: phy@10200000 {
>>>> +			compatible = "starfive,jh7110-usb-phy";
>>>> +			reg = <0x0 0x10200000 0x0 0x10000>;
>>>> +			clocks = <&syscrg JH7110_SYSCLK_USB_125M>,
>>>> +				 <&stgcrg JH7110_STGCLK_USB0_APP_125>;
>>>> +			clock-names = "125m", "app_125";
>>>> +			#phy-cells = <0>;
>>>> +		};
>>>> +
>>>> +		pciephy0: phy@10210000 {
>>>> +			compatible = "starfive,jh7110-pcie-phy";
>>>> +			reg = <0x0 0x10210000 0x0 0x10000>;
>>>> +			#phy-cells = <0>;
>>>> +		};
>>>> +
>>>> +		pciephy1: phy@10220000 {
>>>> +			compatible = "starfive,jh7110-pcie-phy";
>>>> +			reg = <0x0 0x10220000 0x0 0x10000>;
>>>> +			#phy-cells = <0>;
>>>> +		};
>>>> +
>>>>  		stgcrg: clock-controller@10230000 {
>>>>  			compatible = "starfive,jh7110-stgcrg";
>>>>  			reg = <0x0 0x10230000 0x0 0x10000>;
>>>> -- 
>>>> 2.17.1
>>>>
> 
> cheers,
> -roger
diff mbox series

Patch

diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
index a132debb9b53..c64476aebc1a 100644
--- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
@@ -236,3 +236,10 @@ 
 	pinctrl-0 = <&uart0_pins>;
 	status = "okay";
 };
+
+&usb0 {
+	status = "okay";
+	usbdrd_cdns3: usb@0 {
+		dr_mode = "peripheral";
+	};
+};
diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index f70a4ed47eb4..17722fd1be62 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -362,6 +362,60 @@ 
 			status = "disabled";
 		};
 
+		usb0: usb@10100000 {
+			compatible = "starfive,jh7110-usb";
+			clocks = <&stgcrg JH7110_STGCLK_USB0_LPM>,
+				 <&stgcrg JH7110_STGCLK_USB0_STB>,
+				 <&stgcrg JH7110_STGCLK_USB0_APB>,
+				 <&stgcrg JH7110_STGCLK_USB0_AXI>,
+				 <&stgcrg JH7110_STGCLK_USB0_UTMI_APB>;
+			clock-names = "lpm", "stb", "apb", "axi", "utmi_apb";
+			resets = <&stgcrg JH7110_STGRST_USB0_PWRUP>,
+				 <&stgcrg JH7110_STGRST_USB0_APB>,
+				 <&stgcrg JH7110_STGRST_USB0_AXI>,
+				 <&stgcrg JH7110_STGRST_USB0_UTMI_APB>;
+			starfive,stg-syscon = <&stg_syscon 0x4 0xc4 0x148 0x1f4>;
+			starfive,sys-syscon = <&sys_syscon 0x18>;
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0x0 0x0 0x10100000 0x100000>;
+
+			usbdrd_cdns3: usb@0 {
+				compatible = "cdns,usb3";
+				reg = <0x0 0x10000>,
+				      <0x10000 0x10000>,
+				      <0x20000 0x10000>;
+				reg-names = "otg", "xhci", "dev";
+				interrupts = <100>, <108>, <110>;
+				interrupt-names = "host", "peripheral", "otg";
+				phys = <&usbphy0>;
+				phy-names = "cdns3,usb2-phy";
+				maximum-speed = "super-speed";
+			};
+		};
+
+		usbphy0: phy@10200000 {
+			compatible = "starfive,jh7110-usb-phy";
+			reg = <0x0 0x10200000 0x0 0x10000>;
+			clocks = <&syscrg JH7110_SYSCLK_USB_125M>,
+				 <&stgcrg JH7110_STGCLK_USB0_APP_125>;
+			clock-names = "125m", "app_125";
+			#phy-cells = <0>;
+		};
+
+		pciephy0: phy@10210000 {
+			compatible = "starfive,jh7110-pcie-phy";
+			reg = <0x0 0x10210000 0x0 0x10000>;
+			#phy-cells = <0>;
+		};
+
+		pciephy1: phy@10220000 {
+			compatible = "starfive,jh7110-pcie-phy";
+			reg = <0x0 0x10220000 0x0 0x10000>;
+			#phy-cells = <0>;
+		};
+
 		stgcrg: clock-controller@10230000 {
 			compatible = "starfive,jh7110-stgcrg";
 			reg = <0x0 0x10230000 0x0 0x10000>;