Hello everyone, After a lot of poking, I am finally able to use the AR8151 ethernet on the APQ8098 board. The magic bits are the iommu-map prop and the PCIE20_PARF_BDF_TRANSLATE_N setup. The WIP thread is archived here: https://marc.info/?t=155059539200004&r=1&w=2 Marc Gonzalez (3): PCI: qcom: Setup PCIE20_PARF_BDF_TRANSLATE_N arm64: dts: qcom: msm8998: Add PCIe SMMU node arm64: dts: qcom: msm8998: Add PCIe PHY and RC nodes arch/arm64/boot/dts/qcom/msm8998.dtsi | 93 ++++++++++++++++++++++++++ drivers/pci/controller/dwc/pcie-qcom.c | 4 ++ 2 files changed, 97 insertions(+) -- 2.17.1
Initialize PCIE20_PARF_BDF_TRANSLATE_N for ops_2_3_2. Signed-off-by: Marc Gonzalez <marc.w.gonzalez-GANU6spQydw@public.gmane.org> --- drivers/pci/controller/dwc/pcie-qcom.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index 0ed235d560e3..5e5522a427b8 100644 --- a/drivers/pci/controller/dwc/pcie-qcom.c +++ b/drivers/pci/controller/dwc/pcie-qcom.c @@ -54,6 +54,7 @@ #define PCIE20_PARF_LTSSM 0x1B0 #define PCIE20_PARF_SID_OFFSET 0x234 #define PCIE20_PARF_BDF_TRANSLATE_CFG 0x24C +#define PCIE20_PARF_BDF_TRANSLATE_N 0x250 #define PCIE20_ELBI_SYS_CTRL 0x04 #define PCIE20_ELBI_SYS_CTRL_LT_ENABLE BIT(0) @@ -602,6 +603,9 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie) val |= BIT(31); writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2); + val = PCI_DEVID(1, 0); + writel(val, pcie->parf + PCIE20_PARF_BDF_TRANSLATE_N); + return 0; err_slave_clk: -- 2.17.1
ANOC1 SMMU filters PCIe accesses. Signed-off-by: Marc Gonzalez <marc.w.gonzalez-GANU6spQydw@public.gmane.org> --- arch/arm64/boot/dts/qcom/msm8998.dtsi | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi index f9a922fdae75..5a1c0961b281 100644 --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi @@ -606,6 +606,21 @@ #thermal-sensor-cells = <1>; }; + anoc1_smmu: arm,smmu@1680000 { + compatible = "qcom,msm8996-smmu-v2", "qcom,smmu-v2"; + reg = <0x01680000 0x10000>; + #iommu-cells = <1>; + + #global-interrupts = <0>; + interrupts = + <GIC_SPI 364 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 365 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 366 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 367 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 368 IRQ_TYPE_EDGE_RISING>, + <GIC_SPI 369 IRQ_TYPE_EDGE_RISING>; + }; + tcsr_mutex_regs: syscon@1f40000 { compatible = "syscon"; reg = <0x1f40000 0x20000>; -- 2.17.1
Add MSM8998 PCIe QMP PHY and PCIe root complex DT nodes. Signed-off-by: Marc Gonzalez <marc.w.gonzalez-GANU6spQydw@public.gmane.org> --- arch/arm64/boot/dts/qcom/msm8998.dtsi | 78 +++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi index 5a1c0961b281..9f979a51f679 100644 --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi @@ -621,6 +621,84 @@ <GIC_SPI 369 IRQ_TYPE_EDGE_RISING>; }; + pcie0: pci@1c00000 { + compatible = "qcom,pcie-msm8996"; + reg-names = "parf", "dbi", "elbi", "config"; + reg = <0x01c00000 0x2000>, + <0x1b000000 0xf1d>, + <0x1b000f20 0xa8>, + <0x1b100000 0x100000>; + device_type = "pci"; + linux,pci-domain = <0>; + bus-range = <0x00 0xff>; + #address-cells = <3>; + #size-cells = <2>; + power-domains = <&gcc PCIE_0_GDSC>; + + num-lanes = <1>; + phy-names = "pciephy"; + phys = <&pciephy>; + + ranges = + /*** downstream I/O ***/ + <0x01000000 0x0 0x1b200000 0x1b200000 0x0 0x100000>, + /*** non-prefetchable memory ***/ + <0x02000000 0x0 0x1b300000 0x1b300000 0x0 0xd00000>; + + #interrupt-cells = <1>; + interrupt-names = "msi"; + interrupts = <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>; + interrupt-map-mask = <0 0 0 0x7>; + interrupt-map = + <0 0 0 1 &intc 0 135 IRQ_TYPE_LEVEL_HIGH>, /* int_a */ + <0 0 0 2 &intc 0 136 IRQ_TYPE_LEVEL_HIGH>, /* int_b */ + <0 0 0 3 &intc 0 138 IRQ_TYPE_LEVEL_HIGH>, /* int_c */ + <0 0 0 4 &intc 0 139 IRQ_TYPE_LEVEL_HIGH>; /* int_d */ + + clock-names = "pipe", "bus_master", "bus_slave", "cfg", "aux"; + clocks = + <&gcc GCC_PCIE_0_PIPE_CLK>, + <&gcc GCC_PCIE_0_MSTR_AXI_CLK>, + <&gcc GCC_PCIE_0_SLV_AXI_CLK>, + <&gcc GCC_PCIE_0_CFG_AHB_CLK>, + <&gcc GCC_PCIE_0_AUX_CLK>; + + iommu-map = <0x100 &anoc1_smmu 0x1480 1>; + + /* PCIe Fundamental Reset */ + perst-gpios = <&tlmm 35 GPIO_ACTIVE_LOW>; + }; + + phy@1c06000 { + compatible = "qcom,msm8998-qmp-pcie-phy"; + reg = <0x01c06000 0x18c>; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + clock-names = "aux", "cfg_ahb", "ref"; + clocks = + <&gcc GCC_PCIE_PHY_AUX_CLK>, + <&gcc GCC_PCIE_0_CFG_AHB_CLK>, + <&gcc GCC_PCIE_CLKREF_CLK>; + + reset-names = "phy", "common"; + resets = <&gcc GCC_PCIE_0_PHY_BCR>, <&gcc GCC_PCIE_PHY_BCR>; + + vdda-phy-supply = <&vreg_l1a_0p875>; + vdda-pll-supply = <&vreg_l2a_1p2>; + + pciephy: lane@1c06800 { + reg = <0x01c06200 0x128>, <0x01c06400 0x1fc>, <0x01c06800 0x20c>; + #phy-cells = <0>; + + clock-names = "pipe0"; + clocks = <&gcc GCC_PCIE_0_PIPE_CLK>; + clock-output-names = "pcie_0_pipe_clk_src"; + #clock-cells = <0>; + }; + }; + tcsr_mutex_regs: syscon@1f40000 { compatible = "syscon"; reg = <0x1f40000 0x20000>; -- 2.17.1
On 28/03/2019 18:05, Marc Gonzalez wrote: > ANOC1 SMMU filters PCIe accesses. I'm not sure this description is entirely accurate... ANOC likely stands for "Application Network-On-Chip" > arch/arm64/boot/dts/qcom/msm8998.dtsi | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi > index f9a922fdae75..5a1c0961b281 100644 > --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi > @@ -606,6 +606,21 @@ > #thermal-sensor-cells = <1>; > }; > > + anoc1_smmu: arm,smmu@1680000 { > + compatible = "qcom,msm8996-smmu-v2", "qcom,smmu-v2"; Maybe I should instead use "qcom,msm8998-smmu-v2", "qcom,smmu-v2"; and define "qcom,msm8998-smmu-v2" in Documentation/devicetree/bindings/iommu/arm,smmu.txt ? (Would the Documentation change need to be in a separate patch?) > + reg = <0x01680000 0x10000>; > + #iommu-cells = <1>; I'm not sure about this property. IIRC, Robin said <0> is not valid, but I don't have any iommus prop, only iommu-map. > + > + #global-interrupts = <0>; > + interrupts = > + <GIC_SPI 364 IRQ_TYPE_EDGE_RISING>, > + <GIC_SPI 365 IRQ_TYPE_EDGE_RISING>, > + <GIC_SPI 366 IRQ_TYPE_EDGE_RISING>, > + <GIC_SPI 367 IRQ_TYPE_EDGE_RISING>, > + <GIC_SPI 368 IRQ_TYPE_EDGE_RISING>, > + <GIC_SPI 369 IRQ_TYPE_EDGE_RISING>; > + }; > + The rest of the node looks fairly straight-forward. DT code was adapted from: https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm-arm-smmu-8998.dtsi?h=LE.UM.1.3.r3.25#n18 I left out the so-called implementation-defined init: https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm-arm-smmu-impl-defs-8998.dtsi?h=LE.UM.1.3.r3.25#n123 Should I try to merge this part in mainline? (I don't have any documentation for it though.) Regards.
On 29/03/2019 10:51, Marc Gonzalez wrote: > On 28/03/2019 18:05, Marc Gonzalez wrote: > >> ANOC1 SMMU filters PCIe accesses. > > I'm not sure this description is entirely accurate... > > ANOC likely stands for "Application Network-On-Chip" > > >> arch/arm64/boot/dts/qcom/msm8998.dtsi | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi >> index f9a922fdae75..5a1c0961b281 100644 >> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi >> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi >> @@ -606,6 +606,21 @@ >> #thermal-sensor-cells = <1>; >> }; >> >> + anoc1_smmu: arm,smmu@1680000 { >> + compatible = "qcom,msm8996-smmu-v2", "qcom,smmu-v2"; > > Maybe I should instead use "qcom,msm8998-smmu-v2", "qcom,smmu-v2"; > and define "qcom,msm8998-smmu-v2" in > Documentation/devicetree/bindings/iommu/arm,smmu.txt ? Yes please. > (Would the Documentation change need to be in a separate patch?) That's generally preferred, yes. > >> + reg = <0x01680000 0x10000>; >> + #iommu-cells = <1>; > > I'm not sure about this property. IIRC, Robin said <0> is not valid, > but I don't have any iommus prop, only iommu-map. You have to join the dots between the various bindings a little, but the iommu-base part of each iommu-map entry is an IOMMU specifier, and IOMMU specifiers are #iommu-cells long. To cut a long story short, 1 is definitely appropriate, because arm-smmu's definition of a 2-cell specifier wouldn't make much sense in the iommu-map context (and the current code for parsing iommu-map actually just assumes 1 anyway). >> + >> + #global-interrupts = <0>; >> + interrupts = >> + <GIC_SPI 364 IRQ_TYPE_EDGE_RISING>, >> + <GIC_SPI 365 IRQ_TYPE_EDGE_RISING>, >> + <GIC_SPI 366 IRQ_TYPE_EDGE_RISING>, >> + <GIC_SPI 367 IRQ_TYPE_EDGE_RISING>, >> + <GIC_SPI 368 IRQ_TYPE_EDGE_RISING>, >> + <GIC_SPI 369 IRQ_TYPE_EDGE_RISING>; >> + }; >> + > > The rest of the node looks fairly straight-forward. You should probably have some "bus" and "iface" clocks too, per the requirement of "qcom,smmu-v2". Maybe Vivek might know what's relevant for MSM8998? > > DT code was adapted from: > > https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm-arm-smmu-8998.dtsi?h=LE.UM.1.3.r3.25#n18 > > I left out the so-called implementation-defined init: > > https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm-arm-smmu-impl-defs-8998.dtsi?h=LE.UM.1.3.r3.25#n123 > > Should I try to merge this part in mainline? > (I don't have any documentation for it though.) "pls no :(" I'm not sure what route the path takes from "DT describes the platform" to get to "DT lists opaque register addresses and magic data to write into them", but I imagine it might involve getting hit in the head along the way. Robin.
On 3/29/2019 12:29 PM, Robin Murphy wrote: > On 29/03/2019 10:51, Marc Gonzalez wrote: >> On 28/03/2019 18:05, Marc Gonzalez wrote: >>> + >>> + #global-interrupts = <0>; >>> + interrupts = >>> + <GIC_SPI 364 IRQ_TYPE_EDGE_RISING>, >>> + <GIC_SPI 365 IRQ_TYPE_EDGE_RISING>, >>> + <GIC_SPI 366 IRQ_TYPE_EDGE_RISING>, >>> + <GIC_SPI 367 IRQ_TYPE_EDGE_RISING>, >>> + <GIC_SPI 368 IRQ_TYPE_EDGE_RISING>, >>> + <GIC_SPI 369 IRQ_TYPE_EDGE_RISING>; >>> + }; >>> + >> >> The rest of the node looks fairly straight-forward. > > You should probably have some "bus" and "iface" clocks too, per the > requirement of "qcom,smmu-v2". Maybe Vivek might know what's relevant > for MSM8998? > The clocks that power the SMMU are not under the control of Linux, but rather the RPM subsystem. The interface to them is the interconnect framework, which does not yet support 8998 per my understanding. The clocks will always be on, but perhaps not at the best rate for performance until the interconnect is brought in. -- Jeffrey Hugo Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Hi Marc, On Fri, Mar 29, 2019 at 11:59 PM Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote: > > On 29/03/2019 10:51, Marc Gonzalez wrote: > > On 28/03/2019 18:05, Marc Gonzalez wrote: > > > >> ANOC1 SMMU filters PCIe accesses. > > > > I'm not sure this description is entirely accurate... > > > > ANOC likely stands for "Application Network-On-Chip" How about simply saying - "Add device node for ANOC1 SMMU" for commit title. Commit text - ANOC1 smmu services a list of peripherals - USB, UFS, BLSP2, and PCIe. Add the device node for this smmu. > > > > > >> arch/arm64/boot/dts/qcom/msm8998.dtsi | 15 +++++++++++++++ > >> 1 file changed, 15 insertions(+) > >> > >> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi > >> index f9a922fdae75..5a1c0961b281 100644 > >> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi > >> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi > >> @@ -606,6 +606,21 @@ > >> #thermal-sensor-cells = <1>; > >> }; > >> > >> + anoc1_smmu: arm,smmu@1680000 { > >> + compatible = "qcom,msm8996-smmu-v2", "qcom,smmu-v2"; > > > > Maybe I should instead use "qcom,msm8998-smmu-v2", "qcom,smmu-v2"; > > and define "qcom,msm8998-smmu-v2" in > > Documentation/devicetree/bindings/iommu/arm,smmu.txt ? > > Yes please. > > > (Would the Documentation change need to be in a separate patch?) > > That's generally preferred, yes. > > > > >> + reg = <0x01680000 0x10000>; > >> + #iommu-cells = <1>; > > > > I'm not sure about this property. IIRC, Robin said <0> is not valid, > > but I don't have any iommus prop, only iommu-map. > > You have to join the dots between the various bindings a little, but the > iommu-base part of each iommu-map entry is an IOMMU specifier, and IOMMU > specifiers are #iommu-cells long. > > To cut a long story short, 1 is definitely appropriate, because > arm-smmu's definition of a 2-cell specifier wouldn't make much sense in > the iommu-map context (and the current code for parsing iommu-map > actually just assumes 1 anyway). Besides this, Looking at the SID mappings for ANOC1 smmu, devices such as USB, and UFS can very well enable iommu support, and thus iommu-cells = 1 seems like the correct thingy. > > >> + > >> + #global-interrupts = <0>; > >> + interrupts = > >> + <GIC_SPI 364 IRQ_TYPE_EDGE_RISING>, > >> + <GIC_SPI 365 IRQ_TYPE_EDGE_RISING>, > >> + <GIC_SPI 366 IRQ_TYPE_EDGE_RISING>, > >> + <GIC_SPI 367 IRQ_TYPE_EDGE_RISING>, > >> + <GIC_SPI 368 IRQ_TYPE_EDGE_RISING>, > >> + <GIC_SPI 369 IRQ_TYPE_EDGE_RISING>; > >> + }; > >> + > > > > The rest of the node looks fairly straight-forward. > > You should probably have some "bus" and "iface" clocks too, per the > requirement of "qcom,smmu-v2". Maybe Vivek might know what's relevant > for MSM8998? As Jeffrey rightly mentioned, these clocks are not under the control of Linux. So, we won't need to add clocks to this SMMU. Thanks Vivek > > > > > DT code was adapted from: > > > > https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm-arm-smmu-8998.dtsi?h=LE.UM.1.3.r3.25#n18 > > > > I left out the so-called implementation-defined init: > > > > https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm-arm-smmu-impl-defs-8998.dtsi?h=LE.UM.1.3.r3.25#n123 > > > > Should I try to merge this part in mainline? > > (I don't have any documentation for it though.) > > "pls no :(" > > I'm not sure what route the path takes from "DT describes the platform" > to get to "DT lists opaque register addresses and magic data to write > into them", but I imagine it might involve getting hit in the head along > the way. > > Robin. > _______________________________________________ > iommu mailing list > iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi Marc,
Thank you for the work on that!
On 3/28/19 7:01 PM, Marc Gonzalez wrote:
> Initialize PCIE20_PARF_BDF_TRANSLATE_N for ops_2_3_2.
>
> Signed-off-by: Marc Gonzalez <marc.w.gonzalez-GANU6spQydw@public.gmane.org>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 0ed235d560e3..5e5522a427b8 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -54,6 +54,7 @@
> #define PCIE20_PARF_LTSSM 0x1B0
> #define PCIE20_PARF_SID_OFFSET 0x234
> #define PCIE20_PARF_BDF_TRANSLATE_CFG 0x24C
> +#define PCIE20_PARF_BDF_TRANSLATE_N 0x250
>
> #define PCIE20_ELBI_SYS_CTRL 0x04
> #define PCIE20_ELBI_SYS_CTRL_LT_ENABLE BIT(0)
> @@ -602,6 +603,9 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
> val |= BIT(31);
> writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
>
> + val = PCI_DEVID(1, 0);
> + writel(val, pcie->parf + PCIE20_PARF_BDF_TRANSLATE_N);
I wonder, shouldn't this register manipulation happen in common code
e.g. pcie-designware-host.c and of course the manipulation should be
conditional because not all platforms might have iommu.
Also, could we have more generic iommu-map description like the one for
sdm845 at [1] ?
--
regards,
Stan
On 02/04/2019 10:08, Stanimir Varbanov wrote: > On 3/28/19 7:01 PM, Marc Gonzalez wrote: > >> Initialize PCIE20_PARF_BDF_TRANSLATE_N for ops_2_3_2. >> >> Signed-off-by: Marc Gonzalez <marc.w.gonzalez-GANU6spQydw@public.gmane.org> >> --- >> drivers/pci/controller/dwc/pcie-qcom.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c >> index 0ed235d560e3..5e5522a427b8 100644 >> --- a/drivers/pci/controller/dwc/pcie-qcom.c >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c >> @@ -54,6 +54,7 @@ >> #define PCIE20_PARF_LTSSM 0x1B0 >> #define PCIE20_PARF_SID_OFFSET 0x234 >> #define PCIE20_PARF_BDF_TRANSLATE_CFG 0x24C >> +#define PCIE20_PARF_BDF_TRANSLATE_N 0x250 >> >> #define PCIE20_ELBI_SYS_CTRL 0x04 >> #define PCIE20_ELBI_SYS_CTRL_LT_ENABLE BIT(0) >> @@ -602,6 +603,9 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie) >> val |= BIT(31); >> writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2); >> >> + val = PCI_DEVID(1, 0); >> + writel(val, pcie->parf + PCIE20_PARF_BDF_TRANSLATE_N); > > I wonder, shouldn't this register manipulation happen in common code > e.g. pcie-designware-host.c I don't think so. PARF (PCIe Auxiliary Register File) appears to be a vendor-specific area. Perhaps someone with access to the Designware documentation could check. (Though it is possible that several vendors have implemented exactly the same PARF layout.) > and of course the manipulation should be conditional because > not all platforms might have iommu. My patch tweaks PCIE20_PARF_BDF_TRANSLATE_N in qcom_pcie_init_2_3_2() which means 8996 and 8998 only (and possibly sdm660) $ git grep smmu-exist vendor -- arch/arm/boot/dts/qcom vendor:arch/arm/boot/dts/qcom/msm8996.dtsi: qcom,smmu-exist; vendor:arch/arm/boot/dts/qcom/msm8996.dtsi: qcom,smmu-exist; vendor:arch/arm/boot/dts/qcom/msm8996.dtsi: qcom,smmu-exist; vendor:arch/arm/boot/dts/qcom/msm8998-interposer-sdm660.dtsi: qcom,smmu-exist; vendor:arch/arm/boot/dts/qcom/msm8998.dtsi: qcom,smmu-exist; https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/drivers/pci/host/pci-msm.c?h=LE.UM.1.3.r3.25#n3880 > Also, could we have more generic iommu-map description like the one for > sdm845 at [1] ? I assume you meant https://patchwork.kernel.org/patch/10831293/ I don't think that such a map is quite right for msm8998 -- and even for sdm845 :-p There is only a single PCIe master, other than the root complex: # lspci 00:00.0 PCI bridge: Qualcomm Device 0105 01:00.0 Ethernet controller: Qualcomm Atheros AR8151 v2.0 Gigabit Ethernet (rev c0) Hence iommu-map = <0x100 &anoc1_smmu 0x1480 1>; i.e. map stream-id 0x1480 to device-id 01:00.0 We could pick a different stream-id for 0x100 but I don't see any advantage (??) in doing so... If a SoC designer took the msm8998 and added a PCIe bridge to support multiple PCIe EPs, then they would override the iommu-map in their board-specific DTS. I do have a nagging feeling that this patch is not perfect for 3 reasons: 1) Did msm8996 PCIe work without tweaking PCIE20_PARF_BDF_TRANSLATE_N? If so, why? What is the difference with msm8998 PCIe? "MSM8998 does not have static SID configuration for PCIe." What about MSM8996? 2) OF: /soc/pci@1c00000: no iommu-map translation for rid 0x0 on (null) The kernel warns that the root complex has no asssigned stream-id. I'm not sure that's actually an issue... 3) I just blindly write PCI_DEVID(1, 0) to PCIE20_PARF_BDF_TRANSLATE_N To be really generic, I would have to walk the iommu-map and do for_each_iommu_map_entry offset = PCIE20_PARF_BDF_TRANSLATE_N + (out_base - 0x1480) * 4; writel(rid_base, pcie->parf + offset); Perhaps do this in of_pci_iommu_init() ? Maybe have to tweak of_map_rid() or duplicate some code... On first approximation, maybe I can just do the blind write, but only for 8998. That way, I don't change anything for the other platforms. Comments? Regards.
On 30/03/2019 14:18, Vivek Gautam wrote:
>> You should probably have some "bus" and "iface" clocks too, per the
>> requirement of "qcom,smmu-v2". Maybe Vivek might know what's relevant
>> for MSM8998?
>
> As Jeffrey rightly mentioned, these clocks are not under the control of Linux.
> So, we won't need to add clocks to this SMMU.
OK, in that case the "clock-names" part of binding doc probably wants
refining to reflect which implementations do actually require clocks.
Robin.
On 4/2/2019 7:24 PM, Robin Murphy wrote: > On 30/03/2019 14:18, Vivek Gautam wrote: >>> You should probably have some "bus" and "iface" clocks too, per the >>> requirement of "qcom,smmu-v2". Maybe Vivek might know what's relevant >>> for MSM8998? >> >> As Jeffrey rightly mentioned, these clocks are not under the control >> of Linux. >> So, we won't need to add clocks to this SMMU. > > OK, in that case the "clock-names" part of binding doc probably wants > refining to reflect which implementations do actually require clocks. Certainly. Marc, do you want to push a patch for the same? Or, let me know I can prepare one. Thanks Vivek > > Robin.
On 28/03/2019 18:06, Marc Gonzalez wrote: > Add MSM8998 PCIe QMP PHY and PCIe root complex DT nodes. I suppose I should add a reference to the downstream DTS: https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998.dtsi?h=LE.UM.1.3.r3.25#n2537 > diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi > index 5a1c0961b281..9f979a51f679 100644 > --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi > @@ -621,6 +621,84 @@ > <GIC_SPI 369 IRQ_TYPE_EDGE_RISING>; > }; > > + pcie0: pci@1c00000 { > + compatible = "qcom,pcie-msm8996"; Should probably be "qcom,pcie-msm8998"; in case we need to fudge something in 8998 and not in 8996. Apart from this minor issue, I think it's good to go for 5.2 (I'll respin without the SMMU patch) I just need Kishon to take the PHY series, and Stephen to take the clock hack, and we're rolling. Oh and it would be useful to have someone review the PCIE20_PARF_BDF_TRANSLATE_N patch. Regards.
Hi Marc, Few comments inline. On 3/28/19 7:06 PM, Marc Gonzalez wrote: > Add MSM8998 PCIe QMP PHY and PCIe root complex DT nodes. > > Signed-off-by: Marc Gonzalez <marc.w.gonzalez-GANU6spQydw@public.gmane.org> > --- > arch/arm64/boot/dts/qcom/msm8998.dtsi | 78 +++++++++++++++++++++++++++ > 1 file changed, 78 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi > index 5a1c0961b281..9f979a51f679 100644 > --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi > @@ -621,6 +621,84 @@ > <GIC_SPI 369 IRQ_TYPE_EDGE_RISING>; > }; > > + pcie0: pci@1c00000 { > + compatible = "qcom,pcie-msm8996"; > + reg-names = "parf", "dbi", "elbi", "config"; > + reg = <0x01c00000 0x2000>, > + <0x1b000000 0xf1d>, > + <0x1b000f20 0xa8>, > + <0x1b100000 0x100000>; could you please populate reg property and after that reg-names property. > + device_type = "pci"; > + linux,pci-domain = <0>; > + bus-range = <0x00 0xff>; > + #address-cells = <3>; > + #size-cells = <2>; > + power-domains = <&gcc PCIE_0_GDSC>; > + > + num-lanes = <1>; > + phy-names = "pciephy"; > + phys = <&pciephy>; > + > + ranges = > + /*** downstream I/O ***/ could you make this a standard kernel comment or completely drop it > + <0x01000000 0x0 0x1b200000 0x1b200000 0x0 0x100000>, > + /*** non-prefetchable memory ***/ ditto > + <0x02000000 0x0 0x1b300000 0x1b300000 0x0 0xd00000>; > + > + #interrupt-cells = <1>; > + interrupt-names = "msi"; > + interrupts = <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-map-mask = <0 0 0 0x7>; > + interrupt-map = > + <0 0 0 1 &intc 0 135 IRQ_TYPE_LEVEL_HIGH>, /* int_a */ move this to a line upper > + <0 0 0 2 &intc 0 136 IRQ_TYPE_LEVEL_HIGH>, /* int_b */ > + <0 0 0 3 &intc 0 138 IRQ_TYPE_LEVEL_HIGH>, /* int_c */ > + <0 0 0 4 &intc 0 139 IRQ_TYPE_LEVEL_HIGH>; /* int_d */ > + > + clock-names = "pipe", "bus_master", "bus_slave", "cfg", "aux"; > + clocks = > + <&gcc GCC_PCIE_0_PIPE_CLK>, move this to a line upper > + <&gcc GCC_PCIE_0_MSTR_AXI_CLK>, > + <&gcc GCC_PCIE_0_SLV_AXI_CLK>, > + <&gcc GCC_PCIE_0_CFG_AHB_CLK>, > + <&gcc GCC_PCIE_0_AUX_CLK>; please swap the order clocks and clock-names > + > + iommu-map = <0x100 &anoc1_smmu 0x1480 1>; iommu-map-mask? It is optional but I had to ask :) > + > + /* PCIe Fundamental Reset */ this comment is useless :) please drop it > + perst-gpios = <&tlmm 35 GPIO_ACTIVE_LOW>; > + }; > + > + phy@1c06000 { > + compatible = "qcom,msm8998-qmp-pcie-phy"; > + reg = <0x01c06000 0x18c>; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + clock-names = "aux", "cfg_ahb", "ref"; > + clocks = > + <&gcc GCC_PCIE_PHY_AUX_CLK>, > + <&gcc GCC_PCIE_0_CFG_AHB_CLK>, > + <&gcc GCC_PCIE_CLKREF_CLK>;\ please, swap the order of clocks and clock-names, and move first clock a line upper. Also delete '\' symbol at the end of last line. > + > + reset-names = "phy", "common"; > + resets = <&gcc GCC_PCIE_0_PHY_BCR>, <&gcc GCC_PCIE_PHY_BCR>; resets prop and after that reset-names, please. > + > + vdda-phy-supply = <&vreg_l1a_0p875>; > + vdda-pll-supply = <&vreg_l2a_1p2>; > + > + pciephy: lane@1c06800 { > + reg = <0x01c06200 0x128>, <0x01c06400 0x1fc>, <0x01c06800 0x20c>; > + #phy-cells = <0>; > + > + clock-names = "pipe0"; > + clocks = <&gcc GCC_PCIE_0_PIPE_CLK>; please, swap clocks and clock-names > + clock-output-names = "pcie_0_pipe_clk_src"; > + #clock-cells = <0>; > + }; > + }; > + > tcsr_mutex_regs: syscon@1f40000 { > compatible = "syscon"; > reg = <0x1f40000 0x20000>; > -- regards, Stan
Hi Marc, Few comments inline. On 3/28/19 7:06 PM, Marc Gonzalez wrote: > Add MSM8998 PCIe QMP PHY and PCIe root complex DT nodes. > > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> > --- > arch/arm64/boot/dts/qcom/msm8998.dtsi | 78 +++++++++++++++++++++++++++ > 1 file changed, 78 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi > index 5a1c0961b281..9f979a51f679 100644 > --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi > @@ -621,6 +621,84 @@ > <GIC_SPI 369 IRQ_TYPE_EDGE_RISING>; > }; > > + pcie0: pci@1c00000 { > + compatible = "qcom,pcie-msm8996"; > + reg-names = "parf", "dbi", "elbi", "config"; > + reg = <0x01c00000 0x2000>, > + <0x1b000000 0xf1d>, > + <0x1b000f20 0xa8>, > + <0x1b100000 0x100000>; could you please populate reg property and after that reg-names property. > + device_type = "pci"; > + linux,pci-domain = <0>; > + bus-range = <0x00 0xff>; > + #address-cells = <3>; > + #size-cells = <2>; > + power-domains = <&gcc PCIE_0_GDSC>; > + > + num-lanes = <1>; > + phy-names = "pciephy"; > + phys = <&pciephy>; > + > + ranges = > + /*** downstream I/O ***/ could you make this a standard kernel comment or completely drop it > + <0x01000000 0x0 0x1b200000 0x1b200000 0x0 0x100000>, > + /*** non-prefetchable memory ***/ ditto > + <0x02000000 0x0 0x1b300000 0x1b300000 0x0 0xd00000>; > + > + #interrupt-cells = <1>; > + interrupt-names = "msi"; > + interrupts = <GIC_SPI 405 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-map-mask = <0 0 0 0x7>; > + interrupt-map = > + <0 0 0 1 &intc 0 135 IRQ_TYPE_LEVEL_HIGH>, /* int_a */ move this to a line upper > + <0 0 0 2 &intc 0 136 IRQ_TYPE_LEVEL_HIGH>, /* int_b */ > + <0 0 0 3 &intc 0 138 IRQ_TYPE_LEVEL_HIGH>, /* int_c */ > + <0 0 0 4 &intc 0 139 IRQ_TYPE_LEVEL_HIGH>; /* int_d */ > + > + clock-names = "pipe", "bus_master", "bus_slave", "cfg", "aux"; > + clocks = > + <&gcc GCC_PCIE_0_PIPE_CLK>, move this to a line upper > + <&gcc GCC_PCIE_0_MSTR_AXI_CLK>, > + <&gcc GCC_PCIE_0_SLV_AXI_CLK>, > + <&gcc GCC_PCIE_0_CFG_AHB_CLK>, > + <&gcc GCC_PCIE_0_AUX_CLK>; please swap the order clocks and clock-names > + > + iommu-map = <0x100 &anoc1_smmu 0x1480 1>; iommu-map-mask? It is optional but I had to ask :) > + > + /* PCIe Fundamental Reset */ this comment is useless :) please drop it > + perst-gpios = <&tlmm 35 GPIO_ACTIVE_LOW>; > + }; > + > + phy@1c06000 { > + compatible = "qcom,msm8998-qmp-pcie-phy"; > + reg = <0x01c06000 0x18c>; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + clock-names = "aux", "cfg_ahb", "ref"; > + clocks = > + <&gcc GCC_PCIE_PHY_AUX_CLK>, > + <&gcc GCC_PCIE_0_CFG_AHB_CLK>, > + <&gcc GCC_PCIE_CLKREF_CLK>;\ please, swap the order of clocks and clock-names, and move first clock a line upper. Also delete '\' symbol at the end of last line. > + > + reset-names = "phy", "common"; > + resets = <&gcc GCC_PCIE_0_PHY_BCR>, <&gcc GCC_PCIE_PHY_BCR>; resets prop and after that reset-names, please. > + > + vdda-phy-supply = <&vreg_l1a_0p875>; > + vdda-pll-supply = <&vreg_l2a_1p2>; > + > + pciephy: lane@1c06800 { > + reg = <0x01c06200 0x128>, <0x01c06400 0x1fc>, <0x01c06800 0x20c>; > + #phy-cells = <0>; > + > + clock-names = "pipe0"; > + clocks = <&gcc GCC_PCIE_0_PIPE_CLK>; please, swap clocks and clock-names > + clock-output-names = "pcie_0_pipe_clk_src"; > + #clock-cells = <0>; > + }; > + }; > + > tcsr_mutex_regs: syscon@1f40000 { > compatible = "syscon"; > reg = <0x1f40000 0x20000>; > -- regards, Stan _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
On 10/04/2019 17:32, Stanimir Varbanov wrote: > Few comments inline. I'll send v3. Changes: - Move all X-names props *after* corresponding X(s) prop - Drop comments >> + iommu-map = <0x100 &anoc1_smmu 0x1480 1>; > > iommu-map-mask? It is optional but I had to ask :) The only RID in the system is 0x100. # lspci 00:00.0 PCI bridge: Qualcomm Device 0105 01:00.0 Ethernet controller: Qualcomm Atheros AR8151 v2.0 Gigabit Ethernet (rev c0) Since we just want to map 0x100, we don't need an iommu-map-mask. >> + /* PCIe Fundamental Reset */ > > this comment is useless :) please drop it IMO, "perst" is a poor name. Can you guess what it stands for? Regards.
On 10/04/2019 17:32, Stanimir Varbanov wrote: > Few comments inline. I'll send v3. Changes: - Move all X-names props *after* corresponding X(s) prop - Drop comments >> + iommu-map = <0x100 &anoc1_smmu 0x1480 1>; > > iommu-map-mask? It is optional but I had to ask :) The only RID in the system is 0x100. # lspci 00:00.0 PCI bridge: Qualcomm Device 0105 01:00.0 Ethernet controller: Qualcomm Atheros AR8151 v2.0 Gigabit Ethernet (rev c0) Since we just want to map 0x100, we don't need an iommu-map-mask. >> + /* PCIe Fundamental Reset */ > > this comment is useless :) please drop it IMO, "perst" is a poor name. Can you guess what it stands for? Regards. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Hi Marc, On 4/11/19 11:44 AM, Marc Gonzalez wrote: > On 10/04/2019 17:32, Stanimir Varbanov wrote: > >> Few comments inline. > > I'll send v3. > > Changes: > - Move all X-names props *after* corresponding X(s) prop > - Drop comments > > >>> + iommu-map = <0x100 &anoc1_smmu 0x1480 1>; >> >> iommu-map-mask? It is optional but I had to ask :) > > The only RID in the system is 0x100. > > # lspci > 00:00.0 PCI bridge: Qualcomm Device 0105 > 01:00.0 Ethernet controller: Qualcomm Atheros AR8151 v2.0 Gigabit Ethernet (rev c0) > > Since we just want to map 0x100, we don't need an iommu-map-mask. Do you see warnings during boot about missing property? > > >>> + /* PCIe Fundamental Reset */ >> >> this comment is useless :) please drop it > > IMO, "perst" is a poor name. Can you guess what it stands for? The name is got from PCIE base specification. See 6.6.1. Conventional Reset from "PCI EXPRESS BASE SPECIFICATION, REV. 3.0" -- regards, Stan
Hi Marc, On 4/11/19 11:44 AM, Marc Gonzalez wrote: > On 10/04/2019 17:32, Stanimir Varbanov wrote: > >> Few comments inline. > > I'll send v3. > > Changes: > - Move all X-names props *after* corresponding X(s) prop > - Drop comments > > >>> + iommu-map = <0x100 &anoc1_smmu 0x1480 1>; >> >> iommu-map-mask? It is optional but I had to ask :) > > The only RID in the system is 0x100. > > # lspci > 00:00.0 PCI bridge: Qualcomm Device 0105 > 01:00.0 Ethernet controller: Qualcomm Atheros AR8151 v2.0 Gigabit Ethernet (rev c0) > > Since we just want to map 0x100, we don't need an iommu-map-mask. Do you see warnings during boot about missing property? > > >>> + /* PCIe Fundamental Reset */ >> >> this comment is useless :) please drop it > > IMO, "perst" is a poor name. Can you guess what it stands for? The name is got from PCIE base specification. See 6.6.1. Conventional Reset from "PCI EXPRESS BASE SPECIFICATION, REV. 3.0" -- regards, Stan _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
On 11/04/2019 11:09, Stanimir Varbanov wrote: > On 4/11/19 11:44 AM, Marc Gonzalez wrote: > >> Since we just want to map 0x100, we don't need an iommu-map-mask. > > Do you see warnings during boot about missing property? Absent iommu-map-mask property is expected. No warning: https://elixir.bootlin.com/linux/latest/source/drivers/of/base.c#L2245 /* The default is to select all bits. */ map_mask = 0xffffffff; /* * Can be overridden by "{iommu,msi}-map-mask" property. * If of_property_read_u32() fails, the default is used. */ if (map_mask_name) of_property_read_u32(np, map_mask_name, &map_mask); >>>> + /* PCIe Fundamental Reset */ >>> >>> this comment is useless :) please drop it >> >> IMO, "perst" is a poor name. Can you guess what it stands for? > > The name is got from PCIE base specification. See 6.6.1. > Conventional Reset from "PCI EXPRESS BASE SPECIFICATION, REV. 3.0" Indeed... Well, "perst" still sucks :-p Regards.
On 11/04/2019 11:09, Stanimir Varbanov wrote: > On 4/11/19 11:44 AM, Marc Gonzalez wrote: > >> Since we just want to map 0x100, we don't need an iommu-map-mask. > > Do you see warnings during boot about missing property? Absent iommu-map-mask property is expected. No warning: https://elixir.bootlin.com/linux/latest/source/drivers/of/base.c#L2245 /* The default is to select all bits. */ map_mask = 0xffffffff; /* * Can be overridden by "{iommu,msi}-map-mask" property. * If of_property_read_u32() fails, the default is used. */ if (map_mask_name) of_property_read_u32(np, map_mask_name, &map_mask); >>>> + /* PCIe Fundamental Reset */ >>> >>> this comment is useless :) please drop it >> >> IMO, "perst" is a poor name. Can you guess what it stands for? > > The name is got from PCIE base specification. See 6.6.1. > Conventional Reset from "PCI EXPRESS BASE SPECIFICATION, REV. 3.0" Indeed... Well, "perst" still sucks :-p Regards. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
On Thu, Mar 28, 2019 at 05:59:48PM +0100, Marc Gonzalez wrote: > Hello everyone, > > After a lot of poking, I am finally able to use the AR8151 ethernet on the APQ8098 board. > The magic bits are the iommu-map prop and the PCIE20_PARF_BDF_TRANSLATE_N setup. > > The WIP thread is archived here: > https://marc.info/?t=155059539200004&r=1&w=2 > > > Marc Gonzalez (3): > PCI: qcom: Setup PCIE20_PARF_BDF_TRANSLATE_N > arm64: dts: qcom: msm8998: Add PCIe SMMU node > arm64: dts: qcom: msm8998: Add PCIe PHY and RC nodes > > arch/arm64/boot/dts/qcom/msm8998.dtsi | 93 ++++++++++++++++++++++++++ > drivers/pci/controller/dwc/pcie-qcom.c | 4 ++ > 2 files changed, 97 insertions(+) Marc, what's the plan with this series ? Please let me know so that I can handle it correctly in the PCI patch queue, I am not sure by reading comments it has evolved much since posting. Thanks, Lorenzo _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[ Trimming recipients list ] On 11/06/2019 17:55, Lorenzo Pieralisi wrote: > On Thu, Mar 28, 2019 at 05:59:48PM +0100, Marc Gonzalez wrote: > >> After a lot of poking, I am finally able to use the AR8151 ethernet on the APQ8098 board. >> The magic bits are the iommu-map prop and the PCIE20_PARF_BDF_TRANSLATE_N setup. >> >> The WIP thread is archived here: >> https://marc.info/?t=155059539200004&r=1&w=2 >> >> >> Marc Gonzalez (3): >> PCI: qcom: Setup PCIE20_PARF_BDF_TRANSLATE_N >> arm64: dts: qcom: msm8998: Add PCIe SMMU node >> arm64: dts: qcom: msm8998: Add PCIe PHY and RC nodes >> >> arch/arm64/boot/dts/qcom/msm8998.dtsi | 93 ++++++++++++++++++++++++++ >> drivers/pci/controller/dwc/pcie-qcom.c | 4 ++ >> 2 files changed, 97 insertions(+) > > Marc, > > what's the plan with this series ? Please let me know so that > I can handle it correctly in the PCI patch queue, I am not > sure by reading comments it has evolved much since posting. Hello Lorenzo, You can ignore/drop this series, it has been superseded; FWIW, the latest patches no longer touch drivers/pci The hold-up was finding an acceptable work-around for the $&#! bug in qcom's SMMU emulation code. I hope Joerg will push it ASAP into linux-next so it can receive some testing before 5.3-rc1. The current incarnation of the PCIe PHY and RC nodes is: https://patchwork.kernel.org/patch/10895341/ Regards. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu