diff mbox series

[v13,08/10] arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280

Message ID 20231007154806.605-9-quic_kriskura@quicinc.com
State New
Headers show
Series Add multiport support for DWC3 controllers | expand

Commit Message

Krishna Kurapati Oct. 7, 2023, 3:48 p.m. UTC
Add USB and DWC3 node for tertiary port of SC8280 along with multiport
IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride
platforms.

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 84 ++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

Comments

Krishna Kurapati Oct. 18, 2023, 11:57 a.m. UTC | #1
On 10/12/2023 10:32 PM, Krishna Kurapati PSSNV wrote:
> 
> 
> On 10/12/2023 10:10 PM, Konrad Dybcio wrote:
>>
>>
>> On 10/7/23 17:48, Krishna Kurapati wrote:
>>> Add USB and DWC3 node for tertiary port of SC8280 along with multiport
>>> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride
>>> platforms.
>>>
>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>> ---
>> [...]
>>
>>> +
>>> +            interconnects = <&aggre1_noc MASTER_USB3_MP 0 &mc_virt 
>>> SLAVE_EBI1 0>,
>>> +                    <&gem_noc MASTER_APPSS_PROC 0 &config_noc 
>>> SLAVE_USB3_MP 0>;
>> Please use QCOM_ICC_TAG_ALWAYS from 
>> include/dt-bindings/interconnect/qcom,icc.h (like in sa8775p)
>>
>> With that I think it's good to go :)
>>
> Hi Konrad. Thanks for the review.
> 
> I see that the tags are used fr spi/i2c but not usb. So to maintain 
> uniformity, wanted to keep the same here.
> 

Hi Konrad,

  Even in sa8775p.dtsi, the interconnect nodes have 0 & 1 instead of 
macros. So wouldn't it be disturbing the uniformity if we use ICC_TAG's 
here. Let me know your thoughts on this.

Regards,
Krishna,
Johan Hovold Oct. 23, 2023, 4:09 p.m. UTC | #2
On Sat, Oct 07, 2023 at 09:18:04PM +0530, Krishna Kurapati wrote:
> Add USB and DWC3 node for tertiary port of SC8280 along with multiport
> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride
> platforms.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 84 ++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index cad59af7ccef..5f64f75b07db 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -3330,6 +3330,90 @@ system-cache-controller@9200000 {
>  			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
>  		};
>  
> +		usb_2: usb@a4f8800 {
> +			compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3";

So you went with a dedicated compatible even though you are now
inferring the number of ports from the interrupts property.

Should we drop that compatible again or is there any other reason to
keep a separate one?

> +			interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>,
> +					      <&pdc 126 IRQ_TYPE_EDGE_RISING>,
> +					      <&pdc 129 IRQ_TYPE_EDGE_RISING>,
> +					      <&pdc 128 IRQ_TYPE_EDGE_RISING>,
> +					      <&pdc 131 IRQ_TYPE_EDGE_RISING>,
> +					      <&pdc 130 IRQ_TYPE_EDGE_RISING>,
> +					      <&pdc 133 IRQ_TYPE_EDGE_RISING>,
> +					      <&pdc 132 IRQ_TYPE_EDGE_RISING>,
> +					      <&pdc 16 IRQ_TYPE_LEVEL_HIGH>,
> +					      <&pdc 17 IRQ_TYPE_LEVEL_HIGH>,
> +					      <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
> +					      <&intc GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>,
> +					      <&intc GIC_SPI 857 IRQ_TYPE_LEVEL_HIGH>,
> +					      <&intc GIC_SPI 856 IRQ_TYPE_LEVEL_HIGH>;
> +
> +			interrupt-names = "dp_hs_phy_1", "dm_hs_phy_1",
> +					  "dp_hs_phy_2", "dm_hs_phy_2",
> +					  "dp_hs_phy_3", "dm_hs_phy_3",
> +					  "dp_hs_phy_4", "dm_hs_phy_4",
> +					  "ss_phy_1", "ss_phy_2",
> +					  "pwr_event_1",
> +					  "pwr_event_2",
> +					  "pwr_event_3",
> +					  "pwr_event_4";

The interrupt order does not match the binding, where the power event
interrupts come first.

And we probably also want the hs_phy_irqs here after fixing the
incomplete binding.

> +			usb_2_dwc3: usb@a400000 {
> +				compatible = "snps,dwc3";
> +				reg = <0 0x0a400000 0 0xcd00>;
> +				interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;

I'd also like to know what that second dwc3 interrupt is for and whether
it should be defined here as well.

> +				iommus = <&apps_smmu 0x800 0x0>;
> +				phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
> +				       <&usb_2_hsphy1>, <&usb_2_qmpphy1>,
> +				       <&usb_2_hsphy2>,
> +				       <&usb_2_hsphy3>;
> +				phy-names = "usb2-port0", "usb3-port0",
> +					    "usb2-port1", "usb3-port1",
> +					    "usb2-port2",
> +					    "usb2-port3";
> +
> +				/*
> +				 * Multiport controllers are host only contollers, so

spelling again...

> +				 * the dr_mode can be defaulted to host irrespective of
> +				 * the platform.
> +				 */

I know someone asked you to add a comment, but I think you should drop
it again because it makes little sense in its current form.

This particular controller is always going to be host only so just set
dr_mode here. No one is going to be overriding that.

Any comment would need to be about this particular platform and not make
claims about future controllers.

> +				dr_mode = "host";
> +			};
> +		};
> +

Johan
Krzysztof Kozlowski Oct. 23, 2023, 5:16 p.m. UTC | #3
On 23/10/2023 18:09, Johan Hovold wrote:
> On Sat, Oct 07, 2023 at 09:18:04PM +0530, Krishna Kurapati wrote:
>> Add USB and DWC3 node for tertiary port of SC8280 along with multiport
>> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride
>> platforms.
>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 84 ++++++++++++++++++++++++++
>>  1 file changed, 84 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> index cad59af7ccef..5f64f75b07db 100644
>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> @@ -3330,6 +3330,90 @@ system-cache-controller@9200000 {
>>  			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
>>  		};
>>  
>> +		usb_2: usb@a4f8800 {
>> +			compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3";
> 
> So you went with a dedicated compatible even though you are now
> inferring the number of ports from the interrupts property.
> 
> Should we drop that compatible again or is there any other reason to
> keep a separate one?

Please keep the dedicated compatible even if currently it is not used.
https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42

Best regards,
Krzysztof
Krishna Kurapati Oct. 23, 2023, 5:34 p.m. UTC | #4
On 10/23/2023 9:39 PM, Johan Hovold wrote:
> On Sat, Oct 07, 2023 at 09:18:04PM +0530, Krishna Kurapati wrote:
>> Add USB and DWC3 node for tertiary port of SC8280 along with multiport
>> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride
>> platforms.
>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 84 ++++++++++++++++++++++++++
>>   1 file changed, 84 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> index cad59af7ccef..5f64f75b07db 100644
>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> @@ -3330,6 +3330,90 @@ system-cache-controller@9200000 {
>>   			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
>>   		};
>>   
>> +		usb_2: usb@a4f8800 {
>> +			compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3";
> 
> So you went with a dedicated compatible even though you are now
> inferring the number of ports from the interrupts property.
> 
> Should we drop that compatible again or is there any other reason to
> keep a separate one?
>  >> +			interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>,
>> +					      <&pdc 126 IRQ_TYPE_EDGE_RISING>,
>> +					      <&pdc 129 IRQ_TYPE_EDGE_RISING>,
>> +					      <&pdc 128 IRQ_TYPE_EDGE_RISING>,
>> +					      <&pdc 131 IRQ_TYPE_EDGE_RISING>,
>> +					      <&pdc 130 IRQ_TYPE_EDGE_RISING>,
>> +					      <&pdc 133 IRQ_TYPE_EDGE_RISING>,
>> +					      <&pdc 132 IRQ_TYPE_EDGE_RISING>,
>> +					      <&pdc 16 IRQ_TYPE_LEVEL_HIGH>,
>> +					      <&pdc 17 IRQ_TYPE_LEVEL_HIGH>,
>> +					      <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
>> +					      <&intc GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>,
>> +					      <&intc GIC_SPI 857 IRQ_TYPE_LEVEL_HIGH>,
>> +					      <&intc GIC_SPI 856 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> +			interrupt-names = "dp_hs_phy_1", "dm_hs_phy_1",
>> +					  "dp_hs_phy_2", "dm_hs_phy_2",
>> +					  "dp_hs_phy_3", "dm_hs_phy_3",
>> +					  "dp_hs_phy_4", "dm_hs_phy_4",
>> +					  "ss_phy_1", "ss_phy_2",
>> +					  "pwr_event_1",
>> +					  "pwr_event_2",
>> +					  "pwr_event_3",
>> +					  "pwr_event_4";
> 
> The interrupt order does not match the binding, where the power event
> interrupts come first.
> 
> And we probably also want the hs_phy_irqs here after fixing the
> incomplete binding.

You want to update the driver code for this as well ? I have no problem 
in adding it in DT and binding but not in driver.

> 
>> +			usb_2_dwc3: usb@a400000 {
>> +				compatible = "snps,dwc3";
>> +				reg = <0 0x0a400000 0 0xcd00>;
>> +				interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
> 
> I'd also like to know what that second dwc3 interrupt is for and whether
> it should be defined here as well.

Second interrupts is for HW acceleration I believe for which support is 
not there currently.

Regards,
Krishna,
Johan Hovold Oct. 24, 2023, 7:13 a.m. UTC | #5
On Mon, Oct 23, 2023 at 11:04:38PM +0530, Krishna Kurapati PSSNV wrote:
> On 10/23/2023 9:39 PM, Johan Hovold wrote:
> > On Sat, Oct 07, 2023 at 09:18:04PM +0530, Krishna Kurapati wrote:

> > The interrupt order does not match the binding, where the power event
> > interrupts come first.
> > 
> > And we probably also want the hs_phy_irqs here after fixing the
> > incomplete binding.
> 
> You want to update the driver code for this as well ? I have no problem 
> in adding it in DT and binding but not in driver.

Possibly both.
 
> >> +			usb_2_dwc3: usb@a400000 {
> >> +				compatible = "snps,dwc3";
> >> +				reg = <0 0x0a400000 0 0xcd00>;
> >> +				interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
> > 
> > I'd also like to know what that second dwc3 interrupt is for and whether
> > it should be defined here as well.
> 
> Second interrupts is for HW acceleration I believe for which support is 
> not there currently.

But the interrupt is there. And devicetree describes the hardware, not
the implementation, as I've pointed out before.

Johan
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index cad59af7ccef..5f64f75b07db 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -3330,6 +3330,90 @@  system-cache-controller@9200000 {
 			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
+		usb_2: usb@a4f8800 {
+			compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3";
+			reg = <0 0x0a4f8800 0 0x400>;
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges;
+
+			clocks = <&gcc GCC_CFG_NOC_USB3_MP_AXI_CLK>,
+				 <&gcc GCC_USB30_MP_MASTER_CLK>,
+				 <&gcc GCC_AGGRE_USB3_MP_AXI_CLK>,
+				 <&gcc GCC_USB30_MP_SLEEP_CLK>,
+				 <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>,
+				 <&gcc GCC_AGGRE_USB_NOC_AXI_CLK>,
+				 <&gcc GCC_AGGRE_USB_NOC_NORTH_AXI_CLK>,
+				 <&gcc GCC_AGGRE_USB_NOC_SOUTH_AXI_CLK>,
+				 <&gcc GCC_SYS_NOC_USB_AXI_CLK>;
+			clock-names = "cfg_noc", "core", "iface", "sleep", "mock_utmi",
+				      "noc_aggr", "noc_aggr_north", "noc_aggr_south", "noc_sys";
+
+			assigned-clocks = <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>,
+					  <&gcc GCC_USB30_MP_MASTER_CLK>;
+			assigned-clock-rates = <19200000>, <200000000>;
+
+			interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>,
+					      <&pdc 126 IRQ_TYPE_EDGE_RISING>,
+					      <&pdc 129 IRQ_TYPE_EDGE_RISING>,
+					      <&pdc 128 IRQ_TYPE_EDGE_RISING>,
+					      <&pdc 131 IRQ_TYPE_EDGE_RISING>,
+					      <&pdc 130 IRQ_TYPE_EDGE_RISING>,
+					      <&pdc 133 IRQ_TYPE_EDGE_RISING>,
+					      <&pdc 132 IRQ_TYPE_EDGE_RISING>,
+					      <&pdc 16 IRQ_TYPE_LEVEL_HIGH>,
+					      <&pdc 17 IRQ_TYPE_LEVEL_HIGH>,
+					      <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
+					      <&intc GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>,
+					      <&intc GIC_SPI 857 IRQ_TYPE_LEVEL_HIGH>,
+					      <&intc GIC_SPI 856 IRQ_TYPE_LEVEL_HIGH>;
+
+			interrupt-names = "dp_hs_phy_1", "dm_hs_phy_1",
+					  "dp_hs_phy_2", "dm_hs_phy_2",
+					  "dp_hs_phy_3", "dm_hs_phy_3",
+					  "dp_hs_phy_4", "dm_hs_phy_4",
+					  "ss_phy_1", "ss_phy_2",
+					  "pwr_event_1",
+					  "pwr_event_2",
+					  "pwr_event_3",
+					  "pwr_event_4";
+
+			power-domains = <&gcc USB30_MP_GDSC>;
+			required-opps = <&rpmhpd_opp_nom>;
+
+			resets = <&gcc GCC_USB30_MP_BCR>;
+
+			interconnects = <&aggre1_noc MASTER_USB3_MP 0 &mc_virt SLAVE_EBI1 0>,
+					<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_USB3_MP 0>;
+			interconnect-names = "usb-ddr", "apps-usb";
+
+			wakeup-source;
+
+			status = "disabled";
+
+			usb_2_dwc3: usb@a400000 {
+				compatible = "snps,dwc3";
+				reg = <0 0x0a400000 0 0xcd00>;
+				interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
+				iommus = <&apps_smmu 0x800 0x0>;
+				phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
+				       <&usb_2_hsphy1>, <&usb_2_qmpphy1>,
+				       <&usb_2_hsphy2>,
+				       <&usb_2_hsphy3>;
+				phy-names = "usb2-port0", "usb3-port0",
+					    "usb2-port1", "usb3-port1",
+					    "usb2-port2",
+					    "usb2-port3";
+
+				/*
+				 * Multiport controllers are host only contollers, so
+				 * the dr_mode can be defaulted to host irrespective of
+				 * the platform.
+				 */
+				dr_mode = "host";
+			};
+		};
+
 		usb_0: usb@a6f8800 {
 			compatible = "qcom,sc8280xp-dwc3", "qcom,dwc3";
 			reg = <0 0x0a6f8800 0 0x400>;