Hi Marc,
On Fri, Mar 29, 2019 at 11:59 PM Robin Murphy <robin.murphy@xxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxx
> 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
