| Message ID | 20230113164449.906002-8-robimarko@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3e83a9c41ab0244a45a4a2800b9adb8de0d15f82 |
| Headers | show |
| Series | [v2,1/9] arm64: dts: qcom: ipq8074: fix Gen2 PCIe QMP PHY | expand |
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
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
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
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
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 --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"; }; };
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(-)