Re: [PATCH v1 2/3] arm64: dts: qcom: msm8998: Add PCIe SMMU node
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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 2/3] arm64: dts: qcom: msm8998: Add PCIe SMMU node
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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 2/3] arm64: dts: qcom: msm8998: Add PCIe SMMU node
Hi Marc,
On Fri, Mar 29, 2019 at 11:59 PM Robin Murphy 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@168 {
> >> +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 = <0x0168 0x1>;
> >> +#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 =
> >> +,
> >> +,
> >> +,
> >> +,
> >> +,
> >> +;
> >> +};
> >> +
> >
> > 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
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 2/3] arm64: dts: qcom: msm8998: Add PCIe SMMU node
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 = + , + , + , + , + , + ; + }; + 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
Re: [PATCH v1 2/3] arm64: dts: qcom: msm8998: Add PCIe SMMU node
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@168 {
+ 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 = <0x0168 0x1>;
+ #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 =
+ ,
+ ,
+ ,
+ ,
+ ,
+ ;
+ };
+
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.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v1 2/3] arm64: dts: qcom: msm8998: Add PCIe SMMU node
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@168 {
> + 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 = <0x0168 0x1>;
> + #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 =
> + ,
> + ,
> + ,
> + ,
> + ,
> + ;
> + };
> +
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.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v1 2/3] arm64: dts: qcom: msm8998: Add PCIe SMMU node
ANOC1 SMMU filters PCIe accesses.
Signed-off-by: Marc Gonzalez
---
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@168 {
+ compatible = "qcom,msm8996-smmu-v2", "qcom,smmu-v2";
+ reg = <0x0168 0x1>;
+ #iommu-cells = <1>;
+
+ #global-interrupts = <0>;
+ interrupts =
+ ,
+ ,
+ ,
+ ,
+ ,
+ ;
+ };
+
tcsr_mutex_regs: syscon@1f4 {
compatible = "syscon";
reg = <0x1f4 0x2>;
--
2.17.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
