diff mbox series

[v5,7/7] riscv: dts: starfive: Add USB dts configuration for JH7110

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

Commit Message

Minda Chen April 20, 2023, 11 a.m. UTC 
  Add USB wrapper layer and Cadence USB3 controller dts
configuration for StarFive JH7110 SoC and VisionFive2
Board.
USB controller connect to PHY, The PHY dts configuration
are also added.

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

Comments

Roger Quadros April 24, 2023, 2:53 p.m. UTC | #1 
On 20/04/2023 14:00, Minda Chen wrote:
> Add USB wrapper layer and Cadence USB3 controller dts
> configuration for StarFive JH7110 SoC and VisionFive2
> Board.
> USB controller connect to PHY, The PHY dts configuration
> are also added.
> 
> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> ---
>  .../jh7110-starfive-visionfive-2.dtsi         |  7 +++
>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 44 +++++++++++++++++++
>  2 files changed, 51 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 1155b97b593d..fa97ebfd93ad 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> @@ -221,3 +221,10 @@
>  	pinctrl-0 = <&uart0_pins>;
>  	status = "okay";
>  };
> +
> +&usb0 {
> +	phys = <&usbphy0>;
> +	phy-names = "usb2";
> +	dr_mode = "peripheral";
> +	status = "okay";
> +};
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index 29cd798b6732..eee395e19cdb 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -366,6 +366,50 @@
>  			status = "disabled";
>  		};
>  
> +		usb0: usb@10100000 {
> +			compatible = "starfive,jh7110-usb";
> +			reg = <0x0 0x10100000 0x0 0x10000>,
> +			      <0x0 0x10110000 0x0 0x10000>,
> +			      <0x0 0x10120000 0x0 0x10000>;
> +			reg-names = "otg", "xhci", "dev";
> +			interrupts = <100>, <108>, <110>;
> +			interrupt-names = "host", "peripheral", "otg";
> +			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>;
> +			reset-names = "pwrup", "apb", "axi", "utmi_apb";

All this can really be "cdns,usb3" node. The cdns,usb3 driver should
do reset and clocks init as it is generic.

> +			starfive,stg-syscon = <&stg_syscon 0x4>;
> +			status = "disabled";

Only the syscon handling looks starfive specific so only that handling
should be done in starfive USB driver.

This node should look like this

 
	starfive-usb@4 {
		compatible = "starfive,jh7110-usb";
		starfive,stg-syscon = <&stg_syscon 0x4>;

		usb0: usb@10100000 {
			compatible = "cdns,usb3";
			reg = <0x0 0x10100000 0x0 0x10000>,
			      <0x0 0x10110000 0x0 0x10000>,
			      <0x0 0x10120000 0x0 0x10000>;
			reg-names = "otg", "xhci", "dev";
			interrupts = <100>, <108>, <110>;
			interrupt-names = "host", "peripheral", "otg";
			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>;
			reset-names = "pwrup", "apb", "axi", "utmi_apb";
			starfive,stg-syscon = <&stg_syscon 0x4>;
			status = "disabled";
		};
	}

In starfife-usb driver you can use of_platform_default_populate()
to create the cdns,usb3 child for you.

> +		};
> +
> +		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_125m";
> +			#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>;

cheers,
-roger
Minda Chen April 26, 2023, 11:05 a.m. UTC | #2 
On 2023/4/24 22:53, Roger Quadros wrote:
> 
> 
> On 20/04/2023 14:00, Minda Chen wrote:
>> Add USB wrapper layer and Cadence USB3 controller dts
>> configuration for StarFive JH7110 SoC and VisionFive2
>> Board.
>> USB controller connect to PHY, The PHY dts configuration
>> are also added.
>> 
>> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
>> ---
>>  .../jh7110-starfive-visionfive-2.dtsi         |  7 +++
>>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 44 +++++++++++++++++++
>>  2 files changed, 51 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 1155b97b593d..fa97ebfd93ad 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>> @@ -221,3 +221,10 @@
>>  	pinctrl-0 = <&uart0_pins>;
>>  	status = "okay";
>>  };
>> +
>> +&usb0 {
>> +	phys = <&usbphy0>;
>> +	phy-names = "usb2";
>> +	dr_mode = "peripheral";
>> +	status = "okay";
>> +};
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> index 29cd798b6732..eee395e19cdb 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> @@ -366,6 +366,50 @@
>>  			status = "disabled";
>>  		};
>>  
>> +		usb0: usb@10100000 {
>> +			compatible = "starfive,jh7110-usb";
>> +			reg = <0x0 0x10100000 0x0 0x10000>,
>> +			      <0x0 0x10110000 0x0 0x10000>,
>> +			      <0x0 0x10120000 0x0 0x10000>;
>> +			reg-names = "otg", "xhci", "dev";
>> +			interrupts = <100>, <108>, <110>;
>> +			interrupt-names = "host", "peripheral", "otg";
>> +			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>;
>> +			reset-names = "pwrup", "apb", "axi", "utmi_apb";
> 
> All this can really be "cdns,usb3" node. The cdns,usb3 driver should
> do reset and clocks init as it is generic.
> 
But I can't find clock and reset init in Cadence codes while dwc usb3 can find. 
It looks only if clocks and reset generic init codes required to be added in  Cadence codes to support generic clock and reset init.
>> +			starfive,stg-syscon = <&stg_syscon 0x4>;
>> +			status = "disabled";
> 
> Only the syscon handling looks starfive specific so only that handling
> should be done in starfive USB driver.
> 
> This node should look like this
> 
>  
> 	starfive-usb@4 {
> 		compatible = "starfive,jh7110-usb";
> 		starfive,stg-syscon = <&stg_syscon 0x4>;
> 
> 		usb0: usb@10100000 {
> 			compatible = "cdns,usb3";
> 			reg = <0x0 0x10100000 0x0 0x10000>,
> 			      <0x0 0x10110000 0x0 0x10000>,
> 			      <0x0 0x10120000 0x0 0x10000>;
> 			reg-names = "otg", "xhci", "dev";
> 			interrupts = <100>, <108>, <110>;
> 			interrupt-names = "host", "peripheral", "otg";
> 			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>;
> 			reset-names = "pwrup", "apb", "axi", "utmi_apb";
> 			starfive,stg-syscon = <&stg_syscon 0x4>;
> 			status = "disabled";
> 		};
> 	}
>> In starfife-usb driver you can use of_platform_default_populate()
> to create the cdns,usb3 child for you.
> 
But actually the the syscon is not belong to USB. Below is Rob's previous comments. I am follow Rob's comments to change this.
 
  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.

Rob and Rogers
  Could you design whether merge the usb nodes？ 
dt-binding，USB codes are different in two case.
 
>> +		};
>> +
>> +		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_125m";
>> +			#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>;
> 
> cheers,
> -roger
Roger Quadros April 26, 2023, 12:46 p.m. UTC | #3 
Hi Minda,

On 26/04/2023 14:05, Minda Chen wrote:
> 
> 
> On 2023/4/24 22:53, Roger Quadros wrote:
>>
>>
>> On 20/04/2023 14:00, Minda Chen wrote:
>>> Add USB wrapper layer and Cadence USB3 controller dts
>>> configuration for StarFive JH7110 SoC and VisionFive2
>>> Board.
>>> USB controller connect to PHY, The PHY dts configuration
>>> are also added.
>>>
>>> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
>>> ---
>>>  .../jh7110-starfive-visionfive-2.dtsi         |  7 +++
>>>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 44 +++++++++++++++++++
>>>  2 files changed, 51 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 1155b97b593d..fa97ebfd93ad 100644
>>> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>>> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>>> @@ -221,3 +221,10 @@
>>>  	pinctrl-0 = <&uart0_pins>;
>>>  	status = "okay";
>>>  };
>>> +
>>> +&usb0 {
>>> +	phys = <&usbphy0>;
>>> +	phy-names = "usb2";
>>> +	dr_mode = "peripheral";
>>> +	status = "okay";
>>> +};
>>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>> index 29cd798b6732..eee395e19cdb 100644
>>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>> @@ -366,6 +366,50 @@
>>>  			status = "disabled";
>>>  		};
>>>  
>>> +		usb0: usb@10100000 {
>>> +			compatible = "starfive,jh7110-usb";
>>> +			reg = <0x0 0x10100000 0x0 0x10000>,
>>> +			      <0x0 0x10110000 0x0 0x10000>,
>>> +			      <0x0 0x10120000 0x0 0x10000>;
>>> +			reg-names = "otg", "xhci", "dev";
>>> +			interrupts = <100>, <108>, <110>;
>>> +			interrupt-names = "host", "peripheral", "otg";
>>> +			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>;
>>> +			reset-names = "pwrup", "apb", "axi", "utmi_apb";
>>
>> All this can really be "cdns,usb3" node. The cdns,usb3 driver should
>> do reset and clocks init as it is generic.
>>
> But I can't find clock and reset init in Cadence codes while dwc usb3 can find. 
> It looks only if clocks and reset generic init codes required to be added in  Cadence codes to support generic clock and reset init.
>>> +			starfive,stg-syscon = <&stg_syscon 0x4>;
>>> +			status = "disabled";
>>
>> Only the syscon handling looks starfive specific so only that handling
>> should be done in starfive USB driver.
>>
>> This node should look like this
>>
>>  
>> 	starfive-usb@4 {
>> 		compatible = "starfive,jh7110-usb";
>> 		starfive,stg-syscon = <&stg_syscon 0x4>;
>>
>> 		usb0: usb@10100000 {
>> 			compatible = "cdns,usb3";
>> 			reg = <0x0 0x10100000 0x0 0x10000>,
>> 			      <0x0 0x10110000 0x0 0x10000>,
>> 			      <0x0 0x10120000 0x0 0x10000>;
>> 			reg-names = "otg", "xhci", "dev";
>> 			interrupts = <100>, <108>, <110>;
>> 			interrupt-names = "host", "peripheral", "otg";
>> 			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>;
>> 			reset-names = "pwrup", "apb", "axi", "utmi_apb";
>> 			starfive,stg-syscon = <&stg_syscon 0x4>;
>> 			status = "disabled";
>> 		};
>> 	}
>>> In starfife-usb driver you can use of_platform_default_populate()
>> to create the cdns,usb3 child for you.
>>
> But actually the the syscon is not belong to USB. Below is Rob's previous comments. I am follow Rob's comments to change this.

Managing these syscon registers cannot be done in cdns,usb3 driver.
So you definitely need a wrapper driver for that.

>  
>   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.
> 
> Rob and Rogers
>   Could you design whether merge the usb nodes？ 
> dt-binding，USB codes are different in two case.
>  

There should ideally be only one USB node and that should use "cdns,usb3" compatible.
Clocks, resets and power-domain handling should be done in cdns,usb3 driver.

But since you also need to manage some syscon registers "cdsn,usb3" driver is not
sufficient for you.

I will leave the DT-binding question for this case to Rob.

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 1155b97b593d..fa97ebfd93ad 100644
--- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
@@ -221,3 +221,10 @@ 
 	pinctrl-0 = <&uart0_pins>;
 	status = "okay";
 };
+
+&usb0 {
+	phys = <&usbphy0>;
+	phy-names = "usb2";
+	dr_mode = "peripheral";
+	status = "okay";
+};
diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index 29cd798b6732..eee395e19cdb 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -366,6 +366,50 @@ 
 			status = "disabled";
 		};
 
+		usb0: usb@10100000 {
+			compatible = "starfive,jh7110-usb";
+			reg = <0x0 0x10100000 0x0 0x10000>,
+			      <0x0 0x10110000 0x0 0x10000>,
+			      <0x0 0x10120000 0x0 0x10000>;
+			reg-names = "otg", "xhci", "dev";
+			interrupts = <100>, <108>, <110>;
+			interrupt-names = "host", "peripheral", "otg";
+			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>;
+			reset-names = "pwrup", "apb", "axi", "utmi_apb";
+			starfive,stg-syscon = <&stg_syscon 0x4>;
+			status = "disabled";
+		};
+
+		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_125m";
+			#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>;