diff mbox series

[v3,7/7] arm64: dts: qcom: ipq9574: add PCIe2 nodes

Message ID 20240415182052.374494-8-mr.nuke.me@gmail.com
State Superseded
Headers show
Series ipq9574: Enable PCI-Express support | expand

Commit Message

Alex G. April 15, 2024, 6:20 p.m. UTC
On ipq9574, there are 4 PCIe controllers. Describe the pcie2 node, and
its PHY in devicetree.

Only pcie2 is described, because only hardware using that controller
was available for testing.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 arch/arm64/boot/dts/qcom/ipq9574.dtsi | 93 ++++++++++++++++++++++++++-
 1 file changed, 92 insertions(+), 1 deletion(-)

Comments

Manivannan Sadhasivam April 17, 2024, 7:34 a.m. UTC | #1
On Mon, Apr 15, 2024 at 01:20:52PM -0500, Alexandru Gagniuc wrote:
> On ipq9574, there are 4 PCIe controllers. Describe the pcie2 node, and
> its PHY in devicetree.
> 
> Only pcie2 is described, because only hardware using that controller
> was available for testing.
> 

You should describe all the instances in DT. Since the unused ones will be
'disabled', it won't affect anyone.

> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq9574.dtsi | 93 ++++++++++++++++++++++++++-
>  1 file changed, 92 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> index 7f2e5cbf3bbb..f075e2715300 100644
> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> @@ -300,7 +300,7 @@ gcc: clock-controller@1800000 {
>  				 <0>,
>  				 <0>,
>  				 <0>,
> -				 <0>,
> +				 <&pcie2_phy>,
>  				 <0>,
>  				 <0>;
>  			#clock-cells = <1>;
> @@ -745,6 +745,97 @@ frame@b128000 {
>  				status = "disabled";
>  			};
>  		};
> +
> +		pcie2_phy: phy@8c000 {
> +			compatible = "qcom,ipq9574-qmp-gen3x2-pcie-phy";
> +			reg = <0x0008c000 0x14f4>;
> +
> +			clocks = <&gcc GCC_PCIE2_AUX_CLK>,
> +				 <&gcc GCC_PCIE2_AHB_CLK>,
> +				 <&gcc GCC_PCIE2_PIPE_CLK>,
> +				 <&gcc GCC_ANOC_PCIE2_2LANE_M_CLK>,
> +				 <&gcc GCC_SNOC_PCIE2_2LANE_S_CLK>;
> +			clock-names = "aux",
> +				      "cfg_ahb",
> +				      "pipe",
> +				      "anoc",
> +				      "snoc";
> +
> +			clock-output-names = "pcie_phy2_pipe_clk";
> +			#clock-cells = <0>;
> +			#phy-cells = <0>;
> +
> +			resets = <&gcc GCC_PCIE2_PHY_BCR>,
> +				 <&gcc GCC_PCIE2PHY_PHY_BCR>;
> +			reset-names = "phy",
> +				      "common";
> +			status = "disabled";
> +		};
> +
> +		pcie2: pcie@20000000 {
> +			compatible = "qcom,pcie-ipq9574";
> +			reg = <0x20000000 0xf1d>,
> +			      <0x20000f20 0xa8>,
> +			      <0x20001000 0x1000>,
> +			      <0x00088000 0x4000>,
> +			      <0x20100000 0x1000>;
> +			reg-names = "dbi", "elbi", "atu", "parf", "config";
> +
> +			ranges = <0x81000000 0x0 0x20200000 0x20200000 0x0 0x00100000>,	/* I/O */

Please use below range:

<0x01000000 0x0 0x00000000 0x20200000 0x0 0x00100000>
<0x02000000 0x0 0x20300000 0x20300000 0x0 0x07d00000>

> +				 <0x82000000 0x0 0x20300000 0x20300000 0x0 0x07d00000>;	/* MEM */
> +
> +			device_type = "pci";
> +			linux,pci-domain = <3>;
> +			bus-range = <0x00 0xff>;
> +			num-lanes = <2>;
> +			max-link-speed = <3>;
> +			#address-cells = <3>;
> +			#size-cells = <2>;
> +
> +			phys = <&pcie2_phy>;
> +			phy-names = "pciephy";
> +
> +			interrupts = <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "msi";
> +
> +			#interrupt-cells = <1>;
> +			interrupt-map-mask = <0 0 0 0x7>;
> +			interrupt-map = <0 0 0 1 &intc 0 0 164
> +					 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
> +					<0 0 0 2 &intc 0 0 165
> +					 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
> +					<0 0 0 3 &intc 0 0 186
> +					 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> +					<0 0 0 4 &intc 0 0 187
> +					 IRQ_TYPE_LEVEL_HIGH>; /* int_d */

Use a single line for each INTX entry even if it exceeds 80 column width.

- Mani
Alex G. April 18, 2024, 3:33 p.m. UTC | #2
On 4/17/24 02:34, Manivannan Sadhasivam wrote:
> On Mon, Apr 15, 2024 at 01:20:52PM -0500, Alexandru Gagniuc wrote:
>> On ipq9574, there are 4 PCIe controllers. Describe the pcie2 node, and
>> its PHY in devicetree.
>>
>> Only pcie2 is described, because only hardware using that controller
>> was available for testing.
>>
> 
> You should describe all the instances in DT. Since the unused ones will be
> 'disabled', it won't affect anyone.

My concern with untested but "disabled" descriptions is that someone may 
think it's supported, try to enable it on their board, and run into 
issues. Theoretically, we could describe pcie3, as it uses the same 
gen3x2 phy.

The pcie0 and pcie1 use a gen3x1 phy, which I do not support in this 
series. I would have to leave the "phys" and "phy-names" for these 
nodes, leading to an incomplete description

Given this info, do you still wish that I add all other pcie nodes?

>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq9574.dtsi | 93 ++++++++++++++++++++++++++-
>>   1 file changed, 92 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> index 7f2e5cbf3bbb..f075e2715300 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>> @@ -300,7 +300,7 @@ gcc: clock-controller@1800000 {
>>   				 <0>,
>>   				 <0>,
>>   				 <0>,
>> -				 <0>,
>> +				 <&pcie2_phy>,
>>   				 <0>,
>>   				 <0>;
>>   			#clock-cells = <1>;
>> @@ -745,6 +745,97 @@ frame@b128000 {
>>   				status = "disabled";
>>   			};
>>   		};
>> +
>> +		pcie2_phy: phy@8c000 {
>> +			compatible = "qcom,ipq9574-qmp-gen3x2-pcie-phy";
>> +			reg = <0x0008c000 0x14f4>;
>> +
>> +			clocks = <&gcc GCC_PCIE2_AUX_CLK>,
>> +				 <&gcc GCC_PCIE2_AHB_CLK>,
>> +				 <&gcc GCC_PCIE2_PIPE_CLK>,
>> +				 <&gcc GCC_ANOC_PCIE2_2LANE_M_CLK>,
>> +				 <&gcc GCC_SNOC_PCIE2_2LANE_S_CLK>;
>> +			clock-names = "aux",
>> +				      "cfg_ahb",
>> +				      "pipe",
>> +				      "anoc",
>> +				      "snoc";
>> +
>> +			clock-output-names = "pcie_phy2_pipe_clk";
>> +			#clock-cells = <0>;
>> +			#phy-cells = <0>;
>> +
>> +			resets = <&gcc GCC_PCIE2_PHY_BCR>,
>> +				 <&gcc GCC_PCIE2PHY_PHY_BCR>;
>> +			reset-names = "phy",
>> +				      "common";
>> +			status = "disabled";
>> +		};
>> +
>> +		pcie2: pcie@20000000 {
>> +			compatible = "qcom,pcie-ipq9574";
>> +			reg = <0x20000000 0xf1d>,
>> +			      <0x20000f20 0xa8>,
>> +			      <0x20001000 0x1000>,
>> +			      <0x00088000 0x4000>,
>> +			      <0x20100000 0x1000>;
>> +			reg-names = "dbi", "elbi", "atu", "parf", "config";
>> +
>> +			ranges = <0x81000000 0x0 0x20200000 0x20200000 0x0 0x00100000>,	/* I/O */
> 
> Please use below range:
> 
> <0x01000000 0x0 0x00000000 0x20200000 0x0 0x00100000>
> <0x02000000 0x0 0x20300000 0x20300000 0x0 0x07d00000>
> 
Of course, thank you.

>> +				 <0x82000000 0x0 0x20300000 0x20300000 0x0 0x07d00000>;	/* MEM */
>> +
>> +			device_type = "pci";
>> +			linux,pci-domain = <3>;
>> +			bus-range = <0x00 0xff>;
>> +			num-lanes = <2>;
>> +			max-link-speed = <3>;
>> +			#address-cells = <3>;
>> +			#size-cells = <2>;
>> +
>> +			phys = <&pcie2_phy>;
>> +			phy-names = "pciephy";
>> +
>> +			interrupts = <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>;
>> +			interrupt-names = "msi";
>> +
>> +			#interrupt-cells = <1>;
>> +			interrupt-map-mask = <0 0 0 0x7>;
>> +			interrupt-map = <0 0 0 1 &intc 0 0 164
>> +					 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
>> +					<0 0 0 2 &intc 0 0 165
>> +					 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
>> +					<0 0 0 3 &intc 0 0 186
>> +					 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
>> +					<0 0 0 4 &intc 0 0 187
>> +					 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
> 
> Use a single line for each INTX entry even if it exceeds 80 column width.

Yes. Will do.

> - Mani
>
Manivannan Sadhasivam April 22, 2024, 7:10 a.m. UTC | #3
On Thu, Apr 18, 2024 at 10:33:23AM -0500, mr.nuke.me@gmail.com wrote:
> 
> 
> On 4/17/24 02:34, Manivannan Sadhasivam wrote:
> > On Mon, Apr 15, 2024 at 01:20:52PM -0500, Alexandru Gagniuc wrote:
> > > On ipq9574, there are 4 PCIe controllers. Describe the pcie2 node, and
> > > its PHY in devicetree.
> > > 
> > > Only pcie2 is described, because only hardware using that controller
> > > was available for testing.
> > > 
> > 
> > You should describe all the instances in DT. Since the unused ones will be
> > 'disabled', it won't affect anyone.
> 
> My concern with untested but "disabled" descriptions is that someone may
> think it's supported, try to enable it on their board, and run into issues.
> Theoretically, we could describe pcie3, as it uses the same gen3x2 phy.
> 

Okay.

> The pcie0 and pcie1 use a gen3x1 phy, which I do not support in this series.
> I would have to leave the "phys" and "phy-names" for these nodes, leading to
> an incomplete description
> 

Fine then. Please describe at least pcie3. Also add a TODO above the first pcie
node mentioning that someone need to populate others too.

- Mani

> Given this info, do you still wish that I add all other pcie nodes?
> 
> > > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> > > ---
> > >   arch/arm64/boot/dts/qcom/ipq9574.dtsi | 93 ++++++++++++++++++++++++++-
> > >   1 file changed, 92 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> > > index 7f2e5cbf3bbb..f075e2715300 100644
> > > --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> > > @@ -300,7 +300,7 @@ gcc: clock-controller@1800000 {
> > >   				 <0>,
> > >   				 <0>,
> > >   				 <0>,
> > > -				 <0>,
> > > +				 <&pcie2_phy>,
> > >   				 <0>,
> > >   				 <0>;
> > >   			#clock-cells = <1>;
> > > @@ -745,6 +745,97 @@ frame@b128000 {
> > >   				status = "disabled";
> > >   			};
> > >   		};
> > > +
> > > +		pcie2_phy: phy@8c000 {
> > > +			compatible = "qcom,ipq9574-qmp-gen3x2-pcie-phy";
> > > +			reg = <0x0008c000 0x14f4>;
> > > +
> > > +			clocks = <&gcc GCC_PCIE2_AUX_CLK>,
> > > +				 <&gcc GCC_PCIE2_AHB_CLK>,
> > > +				 <&gcc GCC_PCIE2_PIPE_CLK>,
> > > +				 <&gcc GCC_ANOC_PCIE2_2LANE_M_CLK>,
> > > +				 <&gcc GCC_SNOC_PCIE2_2LANE_S_CLK>;
> > > +			clock-names = "aux",
> > > +				      "cfg_ahb",
> > > +				      "pipe",
> > > +				      "anoc",
> > > +				      "snoc";
> > > +
> > > +			clock-output-names = "pcie_phy2_pipe_clk";
> > > +			#clock-cells = <0>;
> > > +			#phy-cells = <0>;
> > > +
> > > +			resets = <&gcc GCC_PCIE2_PHY_BCR>,
> > > +				 <&gcc GCC_PCIE2PHY_PHY_BCR>;
> > > +			reset-names = "phy",
> > > +				      "common";
> > > +			status = "disabled";
> > > +		};
> > > +
> > > +		pcie2: pcie@20000000 {
> > > +			compatible = "qcom,pcie-ipq9574";
> > > +			reg = <0x20000000 0xf1d>,
> > > +			      <0x20000f20 0xa8>,
> > > +			      <0x20001000 0x1000>,
> > > +			      <0x00088000 0x4000>,
> > > +			      <0x20100000 0x1000>;
> > > +			reg-names = "dbi", "elbi", "atu", "parf", "config";
> > > +
> > > +			ranges = <0x81000000 0x0 0x20200000 0x20200000 0x0 0x00100000>,	/* I/O */
> > 
> > Please use below range:
> > 
> > <0x01000000 0x0 0x00000000 0x20200000 0x0 0x00100000>
> > <0x02000000 0x0 0x20300000 0x20300000 0x0 0x07d00000>
> > 
> Of course, thank you.
> 
> > > +				 <0x82000000 0x0 0x20300000 0x20300000 0x0 0x07d00000>;	/* MEM */
> > > +
> > > +			device_type = "pci";
> > > +			linux,pci-domain = <3>;
> > > +			bus-range = <0x00 0xff>;
> > > +			num-lanes = <2>;
> > > +			max-link-speed = <3>;
> > > +			#address-cells = <3>;
> > > +			#size-cells = <2>;
> > > +
> > > +			phys = <&pcie2_phy>;
> > > +			phy-names = "pciephy";
> > > +
> > > +			interrupts = <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>;
> > > +			interrupt-names = "msi";
> > > +
> > > +			#interrupt-cells = <1>;
> > > +			interrupt-map-mask = <0 0 0 0x7>;
> > > +			interrupt-map = <0 0 0 1 &intc 0 0 164
> > > +					 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
> > > +					<0 0 0 2 &intc 0 0 165
> > > +					 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
> > > +					<0 0 0 3 &intc 0 0 186
> > > +					 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> > > +					<0 0 0 4 &intc 0 0 187
> > > +					 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
> > 
> > Use a single line for each INTX entry even if it exceeds 80 column width.
> 
> Yes. Will do.
> 
> > - Mani
> >
Konrad Dybcio April 22, 2024, 2:29 p.m. UTC | #4
[...]


>>>> +
>>>> +			#interrupt-cells = <1>;
>>>> +			interrupt-map-mask = <0 0 0 0x7>;
>>>> +			interrupt-map = <0 0 0 1 &intc 0 0 164
>>>> +					 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
>>>> +					<0 0 0 2 &intc 0 0 165
>>>> +					 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
>>>> +					<0 0 0 3 &intc 0 0 186
>>>> +					 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
>>>> +					<0 0 0 4 &intc 0 0 187
>>>> +					 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
>>>
>>> Use a single line for each INTX entry even if it exceeds 80 column width.
>>
>> Yes. Will do.

And drop the /* int_x */ comments

Konrad
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index 7f2e5cbf3bbb..f075e2715300 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -300,7 +300,7 @@  gcc: clock-controller@1800000 {
 				 <0>,
 				 <0>,
 				 <0>,
-				 <0>,
+				 <&pcie2_phy>,
 				 <0>,
 				 <0>;
 			#clock-cells = <1>;
@@ -745,6 +745,97 @@  frame@b128000 {
 				status = "disabled";
 			};
 		};
+
+		pcie2_phy: phy@8c000 {
+			compatible = "qcom,ipq9574-qmp-gen3x2-pcie-phy";
+			reg = <0x0008c000 0x14f4>;
+
+			clocks = <&gcc GCC_PCIE2_AUX_CLK>,
+				 <&gcc GCC_PCIE2_AHB_CLK>,
+				 <&gcc GCC_PCIE2_PIPE_CLK>,
+				 <&gcc GCC_ANOC_PCIE2_2LANE_M_CLK>,
+				 <&gcc GCC_SNOC_PCIE2_2LANE_S_CLK>;
+			clock-names = "aux",
+				      "cfg_ahb",
+				      "pipe",
+				      "anoc",
+				      "snoc";
+
+			clock-output-names = "pcie_phy2_pipe_clk";
+			#clock-cells = <0>;
+			#phy-cells = <0>;
+
+			resets = <&gcc GCC_PCIE2_PHY_BCR>,
+				 <&gcc GCC_PCIE2PHY_PHY_BCR>;
+			reset-names = "phy",
+				      "common";
+			status = "disabled";
+		};
+
+		pcie2: pcie@20000000 {
+			compatible = "qcom,pcie-ipq9574";
+			reg = <0x20000000 0xf1d>,
+			      <0x20000f20 0xa8>,
+			      <0x20001000 0x1000>,
+			      <0x00088000 0x4000>,
+			      <0x20100000 0x1000>;
+			reg-names = "dbi", "elbi", "atu", "parf", "config";
+
+			ranges = <0x81000000 0x0 0x20200000 0x20200000 0x0 0x00100000>,	/* I/O */
+				 <0x82000000 0x0 0x20300000 0x20300000 0x0 0x07d00000>;	/* MEM */
+
+			device_type = "pci";
+			linux,pci-domain = <3>;
+			bus-range = <0x00 0xff>;
+			num-lanes = <2>;
+			max-link-speed = <3>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			phys = <&pcie2_phy>;
+			phy-names = "pciephy";
+
+			interrupts = <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "msi";
+
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 0x7>;
+			interrupt-map = <0 0 0 1 &intc 0 0 164
+					 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
+					<0 0 0 2 &intc 0 0 165
+					 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
+					<0 0 0 3 &intc 0 0 186
+					 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
+					<0 0 0 4 &intc 0 0 187
+					 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+
+			clocks = <&gcc GCC_PCIE2_AXI_M_CLK>,
+				 <&gcc GCC_PCIE2_AXI_S_CLK>,
+				 <&gcc GCC_PCIE2_AXI_S_BRIDGE_CLK>,
+				 <&gcc GCC_PCIE2_RCHNG_CLK>;
+			clock-names = "axi_m",
+				      "axi_s",
+				      "axi_bridge",
+				      "rchng";
+
+			resets = <&gcc GCC_PCIE2_PIPE_ARES>,
+				 <&gcc GCC_PCIE2_AUX_ARES>,
+				 <&gcc GCC_PCIE2_CORE_STICKY_ARES>,
+				 <&gcc GCC_PCIE2_AXI_M_ARES>,
+				 <&gcc GCC_PCIE2_AXI_S_ARES>,
+				 <&gcc GCC_PCIE2_AXI_S_STICKY_ARES>,
+				 <&gcc GCC_PCIE2_AXI_M_STICKY_ARES>,
+				 <&gcc GCC_PCIE2_AHB_ARES>;
+			reset-names = "pipe",
+				      "aux",
+				      "sticky",
+				      "axi_m",
+				      "axi_s",
+				      "axi_s_sticky",
+				      "axi_m_sticky",
+				      "ahb";
+			status = "disabled";
+		};
 	};
 
 	thermal-zones {