| Message ID | 20240415182052.374494-8-mr.nuke.me@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ipq9574: Enable PCI-Express support | expand |
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
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 >
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 > >
[...] >>>> + >>>> + #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 --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 {
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(-)