| Message ID | 82ab78ee-4a38-4eee-f064-272b6f964f17@free.fr |
|---|---|
| State | Not Applicable |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCIe and AR8151 on APQ8098/MSM8998 | expand |
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.
Hi Marc, On Fri, Mar 29, 2019 at 11:59 PM Robin Murphy <robin.murphy@arm.com> 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@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu
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.
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>;
ANOC1 SMMU filters PCIe accesses. Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> --- arch/arm64/boot/dts/qcom/msm8998.dtsi | 15 +++++++++++++++ 1 file changed, 15 insertions(+)