diff mbox series

[v2,8/9] arm64: dts: qcom: ipq8074: fix Gen3 PCIe node

Message ID 20230113164449.906002-8-robimarko@gmail.com
State New
Headers
Series [v2,1/9] arm64: dts: qcom: ipq8074: fix Gen2 PCIe QMP PHY |

Commit Message

Robert Marko Jan. 13, 2023, 4:44 p.m. UTC 
  IPQ8074 comes in 2 silicon versions:
* v1 with 2x Gen2 PCIe ports and QMP PHY-s
* v2 with 1x Gen3 and 1x Gen2 PCIe ports and QMP PHY-s

v2 is the final and production version that is actually supported by the
kernel, however it looks like PCIe related nodes were added for the v1 SoC.

Finish the PCIe fixup by using the correct compatible, adding missing ATU
register space, declaring max-link-speed, use correct ranges, add missing
clocks and resets.

Fixes: 33057e1672fe ("ARM: dts: ipq8074: Add pcie nodes")
Signed-off-by: Robert Marko <robimarko@gmail.com>
---
 arch/arm64/boot/dts/qcom/ipq8074.dtsi | 30 +++++++++++++++------------
 1 file changed, 17 insertions(+), 13 deletions(-)

Comments

Arnd Bergmann Jan. 30, 2023, 5:11 p.m. UTC | #1 
On Fri, Jan 13, 2023, at 17:44, Robert Marko wrote:
> IPQ8074 comes in 2 silicon versions:
> * v1 with 2x Gen2 PCIe ports and QMP PHY-s
> * v2 with 1x Gen3 and 1x Gen2 PCIe ports and QMP PHY-s
>
> v2 is the final and production version that is actually supported by the
> kernel, however it looks like PCIe related nodes were added for the v1 SoC.
>
> Finish the PCIe fixup by using the correct compatible, adding missing ATU
> register space, declaring max-link-speed, use correct ranges, add missing
> clocks and resets.
>
> Fixes: 33057e1672fe ("ARM: dts: ipq8074: Add pcie nodes")
> Signed-off-by: Robert Marko <robimarko@gmail.com>

I was reading through the pull request today and saw this patch
along with the Gen2 one:


> @@ -871,9 +873,9 @@ pcie0: pci@20000000 {
>  			phy-names = "pciephy";
> 
>  			ranges = <0x81000000 0 0x20200000 0x20200000
> -				  0 0x100000   /* downstream I/O */
> +				  0 0x10000>, /* downstream I/O */

Fixing the length here seems fine, but the bus-side address
still looks wrong: 0x20200000 is way outside of the usual
port ranges from 0 to 0x10000 on the local bus.

> -				  0x82000000 0 0x20300000 0x20300000
> -				  0 0xd00000>; /* non-prefetchable memory */
> +				 <0x82000000 0 0x20220000 0x20220000
> +				  0 0xfde0000>; /* non-prefetchable memory */

I see the total size of the memory space is under 256MB. Are you
sure that there is no 64-bit BAR in addition to this?

I also see commit 7d1158c984d3 ("arm64: dts: qcom: sm8550: Add
PCIe PHYs and controllers nodes") introduce the same broken
I/O port range (oversized 1MB space wiht an identity map) for a
new SoC. This should probably be fixed as well, along with
reviewing the other ones.

Has the I/O space mapping on any of these actually been tested,
or just copied from one SoC to another? Very few devices actually
use I/O space, so it wouldn't be surprising if it never worked
in the first place.

       Arnd
Robert Marko Feb. 2, 2023, 9:16 a.m. UTC | #2 
On Mon, 30 Jan 2023 at 18:11, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Jan 13, 2023, at 17:44, Robert Marko wrote:
> > IPQ8074 comes in 2 silicon versions:
> > * v1 with 2x Gen2 PCIe ports and QMP PHY-s
> > * v2 with 1x Gen3 and 1x Gen2 PCIe ports and QMP PHY-s
> >
> > v2 is the final and production version that is actually supported by the
> > kernel, however it looks like PCIe related nodes were added for the v1 SoC.
> >
> > Finish the PCIe fixup by using the correct compatible, adding missing ATU
> > register space, declaring max-link-speed, use correct ranges, add missing
> > clocks and resets.
> >
> > Fixes: 33057e1672fe ("ARM: dts: ipq8074: Add pcie nodes")
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
>
> I was reading through the pull request today and saw this patch
> along with the Gen2 one:
>
>
> > @@ -871,9 +873,9 @@ pcie0: pci@20000000 {
> >                       phy-names = "pciephy";
> >
> >                       ranges = <0x81000000 0 0x20200000 0x20200000
> > -                               0 0x100000   /* downstream I/O */
> > +                               0 0x10000>, /* downstream I/O */
>
> Fixing the length here seems fine, but the bus-side address
> still looks wrong: 0x20200000 is way outside of the usual
> port ranges from 0 to 0x10000 on the local bus.
>
> > -                               0x82000000 0 0x20300000 0x20300000
> > -                               0 0xd00000>; /* non-prefetchable memory */
> > +                              <0x82000000 0 0x20220000 0x20220000
> > +                               0 0xfde0000>; /* non-prefetchable memory */
>
> I see the total size of the memory space is under 256MB. Are you
> sure that there is no 64-bit BAR in addition to this?
>
> I also see commit 7d1158c984d3 ("arm64: dts: qcom: sm8550: Add
> PCIe PHYs and controllers nodes") introduce the same broken
> I/O port range (oversized 1MB space wiht an identity map) for a
> new SoC. This should probably be fixed as well, along with
> reviewing the other ones.
>
> Has the I/O space mapping on any of these actually been tested,
> or just copied from one SoC to another? Very few devices actually
> use I/O space, so it wouldn't be surprising if it never worked
> in the first place.

Hi Arnd,
As pointed out in the commit description, the ranges property was copied
from the QCA-s downstream 5.4 kernel [1] as I dont have any documentation
on the SoC.

I have runtime tested this on Xiaomi AX3600 which has a QCA9889 card on the
Gen3 PCIe port, and on Xiaomi AX9000 which has QCA9889 on Gen2 port
and QCN9074 on the Gen3 port and they are working fine.

[1] https://git.codelinaro.org/clo/qsdk/oss/kernel/linux-ipq-5.4/-/blob/NHSS.QSDK.12.3.r2/arch/arm64/boot/dts/qcom/ipq8074.dtsi#L834

Regards,
Robert
>
>        Arnd
Arnd Bergmann Feb. 2, 2023, 9:42 a.m. UTC | #3 
On Thu, Feb 2, 2023, at 10:16, Robert Marko wrote:
> On Mon, 30 Jan 2023 at 18:11, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Fri, Jan 13, 2023, at 17:44, Robert Marko wrote:
>
> As pointed out in the commit description, the ranges property was copied
> from the QCA-s downstream 5.4 kernel [1] as I dont have any documentation
> on the SoC.
>
> I have runtime tested this on Xiaomi AX3600 which has a QCA9889 card on the
> Gen3 PCIe port, and on Xiaomi AX9000 which has QCA9889 on Gen2 port
> and QCN9074 on the Gen3 port and they are working fine.

Neither of those use I/O ports though, nor does any other sensible
device that was made in the past decade.

The compatible string tells me that this is a designware pcie block,
so I think driver actually sets up the mapping based on the ranges
property in DT in dw_pcie_iatu_detect() and dw_pcie_iatu_setup(),
rather than the mapping being determined by hardware or firmware
in advance.

Not sure about the general policy we have for this case, maybe the
pci controller or pci-dwc maintainers have an idea here. I would
think it's better to either have no I/O ranges in DT or have
sensible ones than ones that are clearly bogus, if the controller
is able to set up the ranges.

     Arnd
Manivannan Sadhasivam Feb. 28, 2023, 1:20 p.m. UTC | #4 
On Thu, Feb 02, 2023 at 10:42:15AM +0100, Arnd Bergmann wrote:
> On Thu, Feb 2, 2023, at 10:16, Robert Marko wrote:
> > On Mon, 30 Jan 2023 at 18:11, Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Fri, Jan 13, 2023, at 17:44, Robert Marko wrote:
> >
> > As pointed out in the commit description, the ranges property was copied
> > from the QCA-s downstream 5.4 kernel [1] as I dont have any documentation
> > on the SoC.
> >
> > I have runtime tested this on Xiaomi AX3600 which has a QCA9889 card on the
> > Gen3 PCIe port, and on Xiaomi AX9000 which has QCA9889 on Gen2 port
> > and QCN9074 on the Gen3 port and they are working fine.
> 
> Neither of those use I/O ports though, nor does any other sensible
> device that was made in the past decade.
> 
> The compatible string tells me that this is a designware pcie block,
> so I think driver actually sets up the mapping based on the ranges
> property in DT in dw_pcie_iatu_detect() and dw_pcie_iatu_setup(),
> rather than the mapping being determined by hardware or firmware
> in advance.
> 
> Not sure about the general policy we have for this case, maybe the
> pci controller or pci-dwc maintainers have an idea here. I would
> think it's better to either have no I/O ranges in DT or have
> sensible ones than ones that are clearly bogus, if the controller
> is able to set up the ranges.
> 

Just happen to see this thread and I agree that the I/O port range is indeeed
bogus. This is due to the fact that no one tested I/O range with a compatible
device.

I'm not sure about the PCI policy though but we should fix the mapping across
all SoCs. I will send out a series for that.

Thanks for spotting, Arnd!

- Mani

>      Arnd
Robert Marko March 1, 2023, 10:57 a.m. UTC | #5 
On Tue, 28 Feb 2023 at 14:20, Manivannan Sadhasivam <mani@kernel.org> wrote:
>
> On Thu, Feb 02, 2023 at 10:42:15AM +0100, Arnd Bergmann wrote:
> > On Thu, Feb 2, 2023, at 10:16, Robert Marko wrote:
> > > On Mon, 30 Jan 2023 at 18:11, Arnd Bergmann <arnd@arndb.de> wrote:
> > >> On Fri, Jan 13, 2023, at 17:44, Robert Marko wrote:
> > >
> > > As pointed out in the commit description, the ranges property was copied
> > > from the QCA-s downstream 5.4 kernel [1] as I dont have any documentation
> > > on the SoC.
> > >
> > > I have runtime tested this on Xiaomi AX3600 which has a QCA9889 card on the
> > > Gen3 PCIe port, and on Xiaomi AX9000 which has QCA9889 on Gen2 port
> > > and QCN9074 on the Gen3 port and they are working fine.
> >
> > Neither of those use I/O ports though, nor does any other sensible
> > device that was made in the past decade.
> >
> > The compatible string tells me that this is a designware pcie block,
> > so I think driver actually sets up the mapping based on the ranges
> > property in DT in dw_pcie_iatu_detect() and dw_pcie_iatu_setup(),
> > rather than the mapping being determined by hardware or firmware
> > in advance.
> >
> > Not sure about the general policy we have for this case, maybe the
> > pci controller or pci-dwc maintainers have an idea here. I would
> > think it's better to either have no I/O ranges in DT or have
> > sensible ones than ones that are clearly bogus, if the controller
> > is able to set up the ranges.
> >
>
> Just happen to see this thread and I agree that the I/O port range is indeeed
> bogus. This is due to the fact that no one tested I/O range with a compatible
> device.
>
> I'm not sure about the PCI policy though but we should fix the mapping across
> all SoCs. I will send out a series for that.

Thanks for sorting this out.

Regards,
Robert
>
> Thanks for spotting, Arnd!
>
> - Mani
>
> >      Arnd
>
> --
> மணிவண்ணன் சதாசிவம்
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/ipq8074.dtsi b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
index 5ef4383ab18b..74eecca4f9e3 100644
--- a/arch/arm64/boot/dts/qcom/ipq8074.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq8074.dtsi
@@ -854,16 +854,18 @@  IRQ_TYPE_LEVEL_HIGH>, /* int_c */
 		};
 
 		pcie0: pci@20000000 {
-			compatible = "qcom,pcie-ipq8074";
+			compatible = "qcom,pcie-ipq8074-gen3";
 			reg = <0x20000000 0xf1d>,
 			      <0x20000f20 0xa8>,
-			      <0x00080000 0x2000>,
+			      <0x20001000 0x1000>,
+			      <0x00080000 0x4000>,
 			      <0x20100000 0x1000>;
-			reg-names = "dbi", "elbi", "parf", "config";
+			reg-names = "dbi", "elbi", "atu", "parf", "config";
 			device_type = "pci";
 			linux,pci-domain = <0>;
 			bus-range = <0x00 0xff>;
 			num-lanes = <1>;
+			max-link-speed = <3>;
 			#address-cells = <3>;
 			#size-cells = <2>;
 
@@ -871,9 +873,9 @@  pcie0: pci@20000000 {
 			phy-names = "pciephy";
 
 			ranges = <0x81000000 0 0x20200000 0x20200000
-				  0 0x100000   /* downstream I/O */
-				  0x82000000 0 0x20300000 0x20300000
-				  0 0xd00000>; /* non-prefetchable memory */
+				  0 0x10000>, /* downstream I/O */
+				 <0x82000000 0 0x20220000 0x20220000
+				  0 0xfde0000>; /* non-prefetchable memory */
 
 			interrupts = <GIC_SPI 52 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-names = "msi";
@@ -891,28 +893,30 @@  IRQ_TYPE_LEVEL_HIGH>, /* int_c */
 			clocks = <&gcc GCC_SYS_NOC_PCIE0_AXI_CLK>,
 				 <&gcc GCC_PCIE0_AXI_M_CLK>,
 				 <&gcc GCC_PCIE0_AXI_S_CLK>,
-				 <&gcc GCC_PCIE0_AHB_CLK>,
-				 <&gcc GCC_PCIE0_AUX_CLK>;
-
+				 <&gcc GCC_PCIE0_AXI_S_BRIDGE_CLK>,
+				 <&gcc GCC_PCIE0_RCHNG_CLK>;
 			clock-names = "iface",
 				      "axi_m",
 				      "axi_s",
-				      "ahb",
-				      "aux";
+				      "axi_bridge",
+				      "rchng";
+
 			resets = <&gcc GCC_PCIE0_PIPE_ARES>,
 				 <&gcc GCC_PCIE0_SLEEP_ARES>,
 				 <&gcc GCC_PCIE0_CORE_STICKY_ARES>,
 				 <&gcc GCC_PCIE0_AXI_MASTER_ARES>,
 				 <&gcc GCC_PCIE0_AXI_SLAVE_ARES>,
 				 <&gcc GCC_PCIE0_AHB_ARES>,
-				 <&gcc GCC_PCIE0_AXI_MASTER_STICKY_ARES>;
+				 <&gcc GCC_PCIE0_AXI_MASTER_STICKY_ARES>,
+				 <&gcc GCC_PCIE0_AXI_SLAVE_STICKY_ARES>;
 			reset-names = "pipe",
 				      "sleep",
 				      "sticky",
 				      "axi_m",
 				      "axi_s",
 				      "ahb",
-				      "axi_m_sticky";
+				      "axi_m_sticky",
+				      "axi_s_sticky";
 			status = "disabled";
 		};
 	};