2020-07-02 20:17:35

by Tomasz Nowicki

[permalink] [raw]
Subject: [PATCH v3 0/4] Add system mmu support for Armada-806

There were already two versions of series to support SMMU for AP806,
including workaround for accessing ARM SMMU 64bit registers.
First [1] by Hanna Hawa and second [2] by Gregory CLEMENT.
Since it got stuck this is yet another try. I incorporated the V2 comments,
mainly by moving workaround code to arm-smmu-impl.c as requested.

For the record, AP-806 can't access SMMU registers with 64bit width,
this patches split the readq/writeq into two 32bit accesses instead
and update DT bindings.

The series was successfully tested on a vanilla v5.8-rc3 kernel and
Intel e1000e PCIe NIC. The same for platform devices like SATA and USB.

[1]: https://lkml.org/lkml/2018/10/15/373
[2]: https://lkml.org/lkml/2019/7/11/426

Hanna Hawa (1):
iommu/arm-smmu: Workaround for Marvell Armada-AP806 SoC erratum
#582743

Marcin Wojtas (1):
arm64: dts: marvell: add SMMU support

Tomasz Nowicki (2):
iommu/arm-smmu: Add SMMU ID2 register fixup hook
dt-bindings: arm-smmu: add compatible string for Marvell Armada-AP806
SMMU-500

Documentation/arm64/silicon-errata.rst | 3 ++
.../devicetree/bindings/iommu/arm,smmu.yaml | 5 ++
arch/arm64/boot/dts/marvell/armada-8040.dtsi | 36 +++++++++++++
arch/arm64/boot/dts/marvell/armada-ap80x.dtsi | 17 ++++++
drivers/iommu/arm-smmu-impl.c | 52 +++++++++++++++++++
drivers/iommu/arm-smmu.c | 3 ++
drivers/iommu/arm-smmu.h | 1 +
7 files changed, 117 insertions(+)

--
2.17.1


2020-07-02 20:17:38

by Tomasz Nowicki

[permalink] [raw]
Subject: [PATCH v3 2/4] iommu/arm-smmu: Workaround for Marvell Armada-AP806 SoC erratum #582743

From: Hanna Hawa <[email protected]>

Due to erratum #582743, the Marvell Armada-AP806 can't access 64bit to
ARM SMMUv2 registers.

Provide implementation relevant hooks:
- split the writeq/readq to two accesses of writel/readl.
- mask the MMU_IDR2.PTFSv8 fields to not use AArch64 format (but
only AARCH32_L) since with AArch64 format 32 bits access is not supported.

Note that separate writes/reads to 2 is not problem regards to
atomicity, because the driver use the readq/writeq while initialize
the SMMU, report for SMMU fault, and use spinlock in one
case (iova_to_phys).

Signed-off-by: Hanna Hawa <[email protected]>
Signed-off-by: Gregory CLEMENT <[email protected]>
Signed-off-by: Tomasz Nowicki <[email protected]>
---
Documentation/arm64/silicon-errata.rst | 3 ++
drivers/iommu/arm-smmu-impl.c | 52 ++++++++++++++++++++++++++
2 files changed, 55 insertions(+)

diff --git a/Documentation/arm64/silicon-errata.rst b/Documentation/arm64/silicon-errata.rst
index 936cf2a59ca4..157214d3abe1 100644
--- a/Documentation/arm64/silicon-errata.rst
+++ b/Documentation/arm64/silicon-errata.rst
@@ -125,6 +125,9 @@ stable kernels.
| Cavium | ThunderX2 Core | #219 | CAVIUM_TX2_ERRATUM_219 |
+----------------+-----------------+-----------------+-----------------------------+
+----------------+-----------------+-----------------+-----------------------------+
+| Marvell | ARM-MMU-500 | #582743 | N/A |
++----------------+-----------------+-----------------+-----------------------------+
++----------------+-----------------+-----------------+-----------------------------+
| Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 |
+----------------+-----------------+-----------------+-----------------------------+
+----------------+-----------------+-----------------+-----------------------------+
diff --git a/drivers/iommu/arm-smmu-impl.c b/drivers/iommu/arm-smmu-impl.c
index c75b9d957b70..c1fc5e1b8193 100644
--- a/drivers/iommu/arm-smmu-impl.c
+++ b/drivers/iommu/arm-smmu-impl.c
@@ -147,6 +147,53 @@ static const struct arm_smmu_impl arm_mmu500_impl = {
.reset = arm_mmu500_reset,
};

+static u64 mrvl_mmu500_readq(struct arm_smmu_device *smmu, int page, int off)
+{
+ u64 val;
+
+ /*
+ * Marvell Armada-AP806 erratum #582743.
+ * Split all the readq to double readl
+ */
+ val = (u64)readl_relaxed(arm_smmu_page(smmu, page) + off + 4) << 32;
+ val |= readl_relaxed(arm_smmu_page(smmu, page) + off);
+
+ return val;
+}
+
+static void mrvl_mmu500_writeq(struct arm_smmu_device *smmu, int page, int off,
+ u64 val)
+{
+ /*
+ * Marvell Armada-AP806 erratum #582743.
+ * Split all the writeq to double writel
+ */
+ writel_relaxed(upper_32_bits(val), arm_smmu_page(smmu, page) + off + 4);
+ writel_relaxed(lower_32_bits(val), arm_smmu_page(smmu, page) + off);
+}
+
+static u32 mrvl_mmu500_cfg_id2_fixup(u32 id)
+{
+
+ /*
+ * Armada-AP806 erratum #582743.
+ * Hide the SMMU_IDR2.PTFSv8 fields to sidestep the AArch64
+ * formats altogether and allow using 32 bits access on the
+ * interconnect.
+ */
+ id &= ~(ARM_SMMU_ID2_PTFS_4K | ARM_SMMU_ID2_PTFS_16K |
+ ARM_SMMU_ID2_PTFS_64K);
+
+ return id;
+}
+
+static const struct arm_smmu_impl mrvl_mmu500_impl = {
+ .read_reg64 = mrvl_mmu500_readq,
+ .write_reg64 = mrvl_mmu500_writeq,
+ .cfg_id2_fixup = mrvl_mmu500_cfg_id2_fixup,
+ .reset = arm_mmu500_reset,
+};
+

struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu)
{
@@ -160,6 +207,11 @@ struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu)
*/
switch (smmu->model) {
case ARM_MMU500:
+ if (of_device_is_compatible(smmu->dev->of_node,
+ "marvell,ap806-smmu-500")) {
+ smmu->impl = &mrvl_mmu500_impl;
+ return smmu;
+ }
smmu->impl = &arm_mmu500_impl;
break;
case CAVIUM_SMMUV2:
--
2.17.1

2020-07-02 20:18:46

by Tomasz Nowicki

[permalink] [raw]
Subject: [PATCH v3 3/4] dt-bindings: arm-smmu: add compatible string for Marvell Armada-AP806 SMMU-500

Add specific compatible string for Marvell usage due to errata of
accessing 64bits registers of ARM SMMU, in AP806.

AP806 SoC uses the generic ARM-MMU500, and there's no specific
implementation of Marvell, this compatible is used for errata only.

Signed-off-by: Hanna Hawa <[email protected]>
Signed-off-by: Gregory CLEMENT <[email protected]>
Signed-off-by: Tomasz Nowicki <[email protected]>
---
Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
index d7ceb4c34423..7beca9c00b12 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
@@ -38,6 +38,11 @@ properties:
- qcom,sc7180-smmu-500
- qcom,sdm845-smmu-500
- const: arm,mmu-500
+ - description: Marvell SoCs implementing "arm,mmu-500"
+ items:
+ - enum:
+ - marvell,ap806-smmu-500
+ - const: arm,mmu-500
- items:
- const: arm,mmu-500
- const: arm,smmu-v2
--
2.17.1

2020-07-02 20:20:06

by Tomasz Nowicki

[permalink] [raw]
Subject: [PATCH v3 4/4] arm64: dts: marvell: add SMMU support

From: Marcin Wojtas <[email protected]>

Add IOMMU node for Marvell AP806 based SoCs together with platform
and PCI device Stream ID mapping.

Signed-off-by: Marcin Wojtas <[email protected]>
Signed-off-by: Tomasz Nowicki <[email protected]>
---
arch/arm64/boot/dts/marvell/armada-8040.dtsi | 36 +++++++++++++++++++
arch/arm64/boot/dts/marvell/armada-ap80x.dtsi | 17 +++++++++
2 files changed, 53 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-8040.dtsi b/arch/arm64/boot/dts/marvell/armada-8040.dtsi
index 7699b19224c2..25c1df709f72 100644
--- a/arch/arm64/boot/dts/marvell/armada-8040.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-8040.dtsi
@@ -23,3 +23,39 @@
&cp0_rtc {
status = "disabled";
};
+
+&cp0_usb3_0 {
+ iommus = <&smmu 0x440>;
+};
+
+&cp0_usb3_1 {
+ iommus = <&smmu 0x441>;
+};
+
+&cp0_sata0 {
+ iommus = <&smmu 0x444>;
+};
+
+&cp0_sdhci0 {
+ iommus = <&smmu 0x445>;
+};
+
+&cp1_sata0 {
+ iommus = <&smmu 0x454>;
+};
+
+&cp1_usb3_0 {
+ iommus = <&smmu 0x450>;
+};
+
+&cp1_usb3_1 {
+ iommus = <&smmu 0x451>;
+};
+
+&cp0_pcie0 {
+ iommu-map =
+ <0x0 &smmu 0x480 0x20>,
+ <0x100 &smmu 0x4a0 0x20>,
+ <0x200 &smmu 0x4c0 0x20>;
+ iommu-map-mask = <0x031f>;
+};
diff --git a/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi b/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
index 7f9b9a647717..ded8b8082d79 100644
--- a/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
@@ -56,6 +56,23 @@
compatible = "simple-bus";
ranges = <0x0 0x0 0xf0000000 0x1000000>;

+ smmu: iommu@5000000 {
+ compatible = "marvell,ap806-smmu-500", "arm,mmu-500";
+ reg = <0x100000 0x100000>;
+ dma-coherent;
+ #iommu-cells = <1>;
+ #global-interrupts = <1>;
+ interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
+ };
+
gic: interrupt-controller@210000 {
compatible = "arm,gic-400";
#interrupt-cells = <3>;
--
2.17.1

2020-07-02 20:21:12

by Tomasz Nowicki

[permalink] [raw]
Subject: [PATCH v3 1/4] iommu/arm-smmu: Add SMMU ID2 register fixup hook

We already have 'cfg_probe' hook which meant to override and apply
workarounds while probing ID registers. However, 'cfg_probe' is called
at the very end and therefore for some cases fixing up things becomes complex
or requires exporting of SMMU driver structures. Hence, seems it is better and
cleaner to do ID fixup right away. In preparation for adding Marvell
errata add an extra ID2 fixup hook.

Signed-off-by: Tomasz Nowicki <[email protected]>
---
drivers/iommu/arm-smmu.c | 3 +++
drivers/iommu/arm-smmu.h | 1 +
2 files changed, 4 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 243bc4cb2705..17c92e319754 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1857,6 +1857,9 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)

/* ID2 */
id = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_ID2);
+ if (smmu->impl && smmu->impl->cfg_id2_fixup)
+ id = smmu->impl->cfg_id2_fixup(id);
+
size = arm_smmu_id_size_to_bits(FIELD_GET(ARM_SMMU_ID2_IAS, id));
smmu->ipa_size = size;

diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
index d172c024be61..f4c8bd7d0b34 100644
--- a/drivers/iommu/arm-smmu.h
+++ b/drivers/iommu/arm-smmu.h
@@ -382,6 +382,7 @@ struct arm_smmu_impl {
void (*write_reg64)(struct arm_smmu_device *smmu, int page, int offset,
u64 val);
int (*cfg_probe)(struct arm_smmu_device *smmu);
+ u32 (*cfg_id2_fixup)(u32 id);
int (*reset)(struct arm_smmu_device *smmu);
int (*init_context)(struct arm_smmu_domain *smmu_domain);
void (*tlb_sync)(struct arm_smmu_device *smmu, int page, int sync,
--
2.17.1

2020-07-03 08:27:13

by Robin Murphy

[permalink] [raw]
Subject: Re: [PATCH v3 1/4] iommu/arm-smmu: Add SMMU ID2 register fixup hook

On 2020-07-02 21:16, Tomasz Nowicki wrote:
> We already have 'cfg_probe' hook which meant to override and apply
> workarounds while probing ID registers. However, 'cfg_probe' is called
> at the very end and therefore for some cases fixing up things becomes complex
> or requires exporting of SMMU driver structures. Hence, seems it is better and
> cleaner to do ID fixup right away. In preparation for adding Marvell
> errata add an extra ID2 fixup hook.

Hmm, the intent of ->cfg_probe was very much to give impl a chance to
adjust the detected features before we start consuming them, with this
exact case in mind. Since the Cavium quirk isn't actually doing that -
it just needs to run *anywhere* in the whole probe process - I'm under
no illusion that I put the hook in exactly the right place first time
around ;)

The diff below should be more on the mark...

Robin.

----->8-----
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 243bc4cb2705..884ffca5b1eb 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1891,6 +1891,9 @@ static int arm_smmu_device_cfg_probe(struct
arm_smmu_device *smmu)
smmu->features |= ARM_SMMU_FEAT_FMT_AARCH64_64K;
}

+ if (smmu->impl && smmu->impl->cfg_probe)
+ return smmu->impl->cfg_probe(smmu);
+
/* Now we've corralled the various formats, what'll it do? */
if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH32_S)
smmu->pgsize_bitmap |= SZ_4K | SZ_64K | SZ_1M | SZ_16M;
@@ -1909,7 +1912,6 @@ static int arm_smmu_device_cfg_probe(struct
arm_smmu_device *smmu)
dev_notice(smmu->dev, "\tSupported page sizes: 0x%08lx\n",
smmu->pgsize_bitmap);

-
if (smmu->features & ARM_SMMU_FEAT_TRANS_S1)
dev_notice(smmu->dev, "\tStage-1: %lu-bit VA -> %lu-bit IPA\n",
smmu->va_size, smmu->ipa_size);
@@ -1918,9 +1920,6 @@ static int arm_smmu_device_cfg_probe(struct
arm_smmu_device *smmu)
dev_notice(smmu->dev, "\tStage-2: %lu-bit IPA -> %lu-bit PA\n",
smmu->ipa_size, smmu->pa_size);

- if (smmu->impl && smmu->impl->cfg_probe)
- return smmu->impl->cfg_probe(smmu);
-
return 0;
}

2020-07-03 09:05:19

by Robin Murphy

[permalink] [raw]
Subject: Re: [PATCH v3 2/4] iommu/arm-smmu: Workaround for Marvell Armada-AP806 SoC erratum #582743

On 2020-07-02 21:16, Tomasz Nowicki wrote:
> From: Hanna Hawa <[email protected]>
>
> Due to erratum #582743, the Marvell Armada-AP806 can't access 64bit to
> ARM SMMUv2 registers.
>
> Provide implementation relevant hooks:
> - split the writeq/readq to two accesses of writel/readl.
> - mask the MMU_IDR2.PTFSv8 fields to not use AArch64 format (but
> only AARCH32_L) since with AArch64 format 32 bits access is not supported.
>
> Note that separate writes/reads to 2 is not problem regards to
> atomicity, because the driver use the readq/writeq while initialize
> the SMMU, report for SMMU fault, and use spinlock in one
> case (iova_to_phys).

The comment about the spinlock seems to be out of date, and TBH that
whole sentence is a bit unclear - how about something like:

"Note that most 64-bit registers like TTBRn can be accessed as two
32-bit halves without issue, and AArch32 format ensures that the
register writes which must be atomic (for TLBI etc.) need only be 32-bit."

> Signed-off-by: Hanna Hawa <[email protected]>
> Signed-off-by: Gregory CLEMENT <[email protected]>
> Signed-off-by: Tomasz Nowicki <[email protected]>
> ---
> Documentation/arm64/silicon-errata.rst | 3 ++
> drivers/iommu/arm-smmu-impl.c | 52 ++++++++++++++++++++++++++
> 2 files changed, 55 insertions(+)
>
> diff --git a/Documentation/arm64/silicon-errata.rst b/Documentation/arm64/silicon-errata.rst
> index 936cf2a59ca4..157214d3abe1 100644
> --- a/Documentation/arm64/silicon-errata.rst
> +++ b/Documentation/arm64/silicon-errata.rst
> @@ -125,6 +125,9 @@ stable kernels.
> | Cavium | ThunderX2 Core | #219 | CAVIUM_TX2_ERRATUM_219 |
> +----------------+-----------------+-----------------+-----------------------------+
> +----------------+-----------------+-----------------+-----------------------------+
> +| Marvell | ARM-MMU-500 | #582743 | N/A |
> ++----------------+-----------------+-----------------+-----------------------------+
> ++----------------+-----------------+-----------------+-----------------------------+
> | Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 |
> +----------------+-----------------+-----------------+-----------------------------+
> +----------------+-----------------+-----------------+-----------------------------+
> diff --git a/drivers/iommu/arm-smmu-impl.c b/drivers/iommu/arm-smmu-impl.c
> index c75b9d957b70..c1fc5e1b8193 100644
> --- a/drivers/iommu/arm-smmu-impl.c
> +++ b/drivers/iommu/arm-smmu-impl.c
> @@ -147,6 +147,53 @@ static const struct arm_smmu_impl arm_mmu500_impl = {
> .reset = arm_mmu500_reset,
> };
>
> +static u64 mrvl_mmu500_readq(struct arm_smmu_device *smmu, int page, int off)
> +{
> + u64 val;
> +
> + /*
> + * Marvell Armada-AP806 erratum #582743.
> + * Split all the readq to double readl
> + */
> + val = (u64)readl_relaxed(arm_smmu_page(smmu, page) + off + 4) << 32;
> + val |= readl_relaxed(arm_smmu_page(smmu, page) + off);

Even though io-64-nonatomic-hi-lo.h doesn't override readq() etc. for
64-bit builds, you can still use hi_lo_readq_relaxed() directly.

> +
> + return val;
> +}
> +
> +static void mrvl_mmu500_writeq(struct arm_smmu_device *smmu, int page, int off,
> + u64 val)
> +{
> + /*
> + * Marvell Armada-AP806 erratum #582743.
> + * Split all the writeq to double writel
> + */
> + writel_relaxed(upper_32_bits(val), arm_smmu_page(smmu, page) + off + 4);
> + writel_relaxed(lower_32_bits(val), arm_smmu_page(smmu, page) + off);

Similarly, hi_lo_writeq_relaxed().

> +}
> +
> +static u32 mrvl_mmu500_cfg_id2_fixup(u32 id)
> +{
> +
> + /*
> + * Armada-AP806 erratum #582743.
> + * Hide the SMMU_IDR2.PTFSv8 fields to sidestep the AArch64
> + * formats altogether and allow using 32 bits access on the
> + * interconnect.
> + */
> + id &= ~(ARM_SMMU_ID2_PTFS_4K | ARM_SMMU_ID2_PTFS_16K |
> + ARM_SMMU_ID2_PTFS_64K);
> +
> + return id;
> +}
> +
> +static const struct arm_smmu_impl mrvl_mmu500_impl = {
> + .read_reg64 = mrvl_mmu500_readq,
> + .write_reg64 = mrvl_mmu500_writeq,
> + .cfg_id2_fixup = mrvl_mmu500_cfg_id2_fixup,
> + .reset = arm_mmu500_reset,
> +};
> +
>
> struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu)
> {
> @@ -160,6 +207,11 @@ struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device *smmu)
> */
> switch (smmu->model) {
> case ARM_MMU500:
> + if (of_device_is_compatible(smmu->dev->of_node,

Nit: there's a local "np" variable now.

> + "marvell,ap806-smmu-500")) {
> + smmu->impl = &mrvl_mmu500_impl;
> + return smmu;
> + }

Please put this with the other integration checks below the switch
statement. Yes, it means we'll end up assigning smmu->impl twice for
this particular case, but that's the intended pattern.

Robin.

> smmu->impl = &arm_mmu500_impl;
> break;
> case CAVIUM_SMMUV2:
>

2020-07-03 09:06:46

by Robin Murphy

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] dt-bindings: arm-smmu: add compatible string for Marvell Armada-AP806 SMMU-500

On 2020-07-02 21:16, Tomasz Nowicki wrote:
> Add specific compatible string for Marvell usage due to errata of
> accessing 64bits registers of ARM SMMU, in AP806.
>
> AP806 SoC uses the generic ARM-MMU500, and there's no specific
> implementation of Marvell, this compatible is used for errata only.
>
> Signed-off-by: Hanna Hawa <[email protected]>
> Signed-off-by: Gregory CLEMENT <[email protected]>
> Signed-off-by: Tomasz Nowicki <[email protected]>
> ---
> Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> index d7ceb4c34423..7beca9c00b12 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> @@ -38,6 +38,11 @@ properties:
> - qcom,sc7180-smmu-500
> - qcom,sdm845-smmu-500
> - const: arm,mmu-500
> + - description: Marvell SoCs implementing "arm,mmu-500"
> + items:
> + - enum:
> + - marvell,ap806-smmu-500

Isn't a single-valued enum just a constant? :P

Robin.

> + - const: arm,mmu-500
> - items:
> - const: arm,mmu-500
> - const: arm,smmu-v2
>

2020-07-03 09:19:18

by Robin Murphy

[permalink] [raw]
Subject: Re: [PATCH v3 4/4] arm64: dts: marvell: add SMMU support

On 2020-07-02 21:16, Tomasz Nowicki wrote:
> From: Marcin Wojtas <[email protected]>
>
> Add IOMMU node for Marvell AP806 based SoCs together with platform
> and PCI device Stream ID mapping.
>
> Signed-off-by: Marcin Wojtas <[email protected]>
> Signed-off-by: Tomasz Nowicki <[email protected]>
> ---
> arch/arm64/boot/dts/marvell/armada-8040.dtsi | 36 +++++++++++++++++++
> arch/arm64/boot/dts/marvell/armada-ap80x.dtsi | 17 +++++++++
> 2 files changed, 53 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-8040.dtsi b/arch/arm64/boot/dts/marvell/armada-8040.dtsi
> index 7699b19224c2..25c1df709f72 100644
> --- a/arch/arm64/boot/dts/marvell/armada-8040.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-8040.dtsi
> @@ -23,3 +23,39 @@
> &cp0_rtc {
> status = "disabled";
> };
> +
> +&cp0_usb3_0 {
> + iommus = <&smmu 0x440>;
> +};
> +
> +&cp0_usb3_1 {
> + iommus = <&smmu 0x441>;
> +};
> +
> +&cp0_sata0 {
> + iommus = <&smmu 0x444>;
> +};
> +
> +&cp0_sdhci0 {
> + iommus = <&smmu 0x445>;
> +};
> +
> +&cp1_sata0 {
> + iommus = <&smmu 0x454>;
> +};
> +
> +&cp1_usb3_0 {
> + iommus = <&smmu 0x450>;
> +};
> +
> +&cp1_usb3_1 {
> + iommus = <&smmu 0x451>;
> +};
> +
> +&cp0_pcie0 {
> + iommu-map =
> + <0x0 &smmu 0x480 0x20>,
> + <0x100 &smmu 0x4a0 0x20>,
> + <0x200 &smmu 0x4c0 0x20>;
> + iommu-map-mask = <0x031f>;

Nice! I do like a good compressed mapping :D

> +};
> diff --git a/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi b/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
> index 7f9b9a647717..ded8b8082d79 100644
> --- a/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
> @@ -56,6 +56,23 @@
> compatible = "simple-bus";
> ranges = <0x0 0x0 0xf0000000 0x1000000>;
>
> + smmu: iommu@5000000 {
> + compatible = "marvell,ap806-smmu-500", "arm,mmu-500";
> + reg = <0x100000 0x100000>;
> + dma-coherent;
> + #iommu-cells = <1>;
> + #global-interrupts = <1>;
> + interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;

I'd recommend you have the node disabled by default here, then
explicitly enable it in armada-8040.dtsi where you add the Stream IDs.
Otherwise it will also end up enabled for 8020, 70x0, etc. where
disable_bypass will then catastrophically break everything.

Robin.

> + };
> +
> gic: interrupt-controller@210000 {
> compatible = "arm,gic-400";
> #interrupt-cells = <3>;
>

2020-07-03 09:22:01

by Tomasz Nowicki

[permalink] [raw]
Subject: Re: [PATCH v3 1/4] iommu/arm-smmu: Add SMMU ID2 register fixup hook

On 03.07.2020 10:24, Robin Murphy wrote:
> On 2020-07-02 21:16, Tomasz Nowicki wrote:
>> We already have 'cfg_probe' hook which meant to override and apply
>> workarounds while probing ID registers. However, 'cfg_probe' is called
>> at the very end and therefore for some cases fixing up things becomes
>> complex
>> or requires exporting of SMMU driver structures. Hence, seems it is
>> better and
>> cleaner to do ID fixup right away. In preparation for adding Marvell
>> errata add an extra ID2 fixup hook.
>
> Hmm, the intent of ->cfg_probe was very much to give impl a chance to
> adjust the detected features before we start consuming them, with this
> exact case in mind. Since the Cavium quirk isn't actually doing that -
> it just needs to run *anywhere* in the whole probe process - I'm under
> no illusion that I put the hook in exactly the right place first time
> around ;)
>
> The diff below should be more on the mark...
>
> Robin.
>
> ----->8-----
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 243bc4cb2705..884ffca5b1eb 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1891,6 +1891,9 @@ static int arm_smmu_device_cfg_probe(struct
> arm_smmu_device *smmu)
>              smmu->features |= ARM_SMMU_FEAT_FMT_AARCH64_64K;
>      }
>
> +    if (smmu->impl && smmu->impl->cfg_probe)
> +        return smmu->impl->cfg_probe(smmu);
> +
>      /* Now we've corralled the various formats, what'll it do? */
>      if (smmu->features & ARM_SMMU_FEAT_FMT_AARCH32_S)
>          smmu->pgsize_bitmap |= SZ_4K | SZ_64K | SZ_1M | SZ_16M;
> @@ -1909,7 +1912,6 @@ static int arm_smmu_device_cfg_probe(struct
> arm_smmu_device *smmu)
>      dev_notice(smmu->dev, "\tSupported page sizes: 0x%08lx\n",
>             smmu->pgsize_bitmap);
>
> -
>      if (smmu->features & ARM_SMMU_FEAT_TRANS_S1)
>          dev_notice(smmu->dev, "\tStage-1: %lu-bit VA -> %lu-bit IPA\n",
>                 smmu->va_size, smmu->ipa_size);
> @@ -1918,9 +1920,6 @@ static int arm_smmu_device_cfg_probe(struct
> arm_smmu_device *smmu)
>          dev_notice(smmu->dev, "\tStage-2: %lu-bit IPA -> %lu-bit PA\n",
>                 smmu->ipa_size, smmu->pa_size);
>
> -    if (smmu->impl && smmu->impl->cfg_probe)
> -        return smmu->impl->cfg_probe(smmu);
> -
>      return 0;
>  }
>

I was under impression that ->cfg_probe was meant for Cavium alike
errata (position independent). Then I will move ->cfg_probe as you
suggested. I prefer not to add yet another impl hook too :)

Thanks,
Tomasz

2020-07-03 09:28:09

by Tomasz Nowicki

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] dt-bindings: arm-smmu: add compatible string for Marvell Armada-AP806 SMMU-500

On 03.07.2020 11:05, Robin Murphy wrote:
> On 2020-07-02 21:16, Tomasz Nowicki wrote:
>> Add specific compatible string for Marvell usage due to errata of
>> accessing 64bits registers of ARM SMMU, in AP806.
>>
>> AP806 SoC uses the generic ARM-MMU500, and there's no specific
>> implementation of Marvell, this compatible is used for errata only.
>>
>> Signed-off-by: Hanna Hawa <[email protected]>
>> Signed-off-by: Gregory CLEMENT <[email protected]>
>> Signed-off-by: Tomasz Nowicki <[email protected]>
>> ---
>>   Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
>> b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
>> index d7ceb4c34423..7beca9c00b12 100644
>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
>> @@ -38,6 +38,11 @@ properties:
>>                 - qcom,sc7180-smmu-500
>>                 - qcom,sdm845-smmu-500
>>             - const: arm,mmu-500
>> +      - description: Marvell SoCs implementing "arm,mmu-500"
>> +        items:
>> +          - enum:
>> +              - marvell,ap806-smmu-500
>
> Isn't a single-valued enum just a constant? :P

That's how copy-paste engineering ends up :)

Thanks,
Tomasz

2020-07-03 09:34:35

by Tomasz Nowicki

[permalink] [raw]
Subject: Re: [PATCH v3 4/4] arm64: dts: marvell: add SMMU support

On 03.07.2020 11:16, Robin Murphy wrote:
> On 2020-07-02 21:16, Tomasz Nowicki wrote:
>> From: Marcin Wojtas <[email protected]>
>>
>> Add IOMMU node for Marvell AP806 based SoCs together with platform
>> and PCI device Stream ID mapping.
>>
>> Signed-off-by: Marcin Wojtas <[email protected]>
>> Signed-off-by: Tomasz Nowicki <[email protected]>
>> ---
>>   arch/arm64/boot/dts/marvell/armada-8040.dtsi  | 36 +++++++++++++++++++
>>   arch/arm64/boot/dts/marvell/armada-ap80x.dtsi | 17 +++++++++
>>   2 files changed, 53 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/marvell/armada-8040.dtsi
>> b/arch/arm64/boot/dts/marvell/armada-8040.dtsi
>> index 7699b19224c2..25c1df709f72 100644
>> --- a/arch/arm64/boot/dts/marvell/armada-8040.dtsi
>> +++ b/arch/arm64/boot/dts/marvell/armada-8040.dtsi
>> @@ -23,3 +23,39 @@
>>   &cp0_rtc {
>>       status = "disabled";
>>   };
>> +
>> +&cp0_usb3_0 {
>> +    iommus = <&smmu 0x440>;
>> +};
>> +
>> +&cp0_usb3_1 {
>> +    iommus = <&smmu 0x441>;
>> +};
>> +
>> +&cp0_sata0 {
>> +    iommus = <&smmu 0x444>;
>> +};
>> +
>> +&cp0_sdhci0 {
>> +    iommus = <&smmu 0x445>;
>> +};
>> +
>> +&cp1_sata0 {
>> +    iommus = <&smmu 0x454>;
>> +};
>> +
>> +&cp1_usb3_0 {
>> +    iommus = <&smmu 0x450>;
>> +};
>> +
>> +&cp1_usb3_1 {
>> +    iommus = <&smmu 0x451>;
>> +};
>> +
>> +&cp0_pcie0 {
>> +    iommu-map =
>> +        <0x0   &smmu 0x480 0x20>,
>> +        <0x100 &smmu 0x4a0 0x20>,
>> +        <0x200 &smmu 0x4c0 0x20>;
>> +    iommu-map-mask = <0x031f>;
>
> Nice! I do like a good compressed mapping :D
>
>> +};
>> diff --git a/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
>> b/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
>> index 7f9b9a647717..ded8b8082d79 100644
>> --- a/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
>> +++ b/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
>> @@ -56,6 +56,23 @@
>>               compatible = "simple-bus";
>>               ranges = <0x0 0x0 0xf0000000 0x1000000>;
>> +            smmu: iommu@5000000 {
>> +                compatible = "marvell,ap806-smmu-500", "arm,mmu-500";
>> +                reg = <0x100000 0x100000>;
>> +                dma-coherent;
>> +                #iommu-cells = <1>;
>> +                #global-interrupts = <1>;
>> +                interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
>
> I'd recommend you have the node disabled by default here, then
> explicitly enable it in armada-8040.dtsi where you add the Stream IDs.
> Otherwise it will also end up enabled for 8020, 70x0, etc. where
> disable_bypass will then catastrophically break everything.
>

Good point! I will fix this.

Thanks,
Tomasz

2020-07-03 10:39:16

by Marcin Wojtas

[permalink] [raw]
Subject: Re: [PATCH v3 4/4] arm64: dts: marvell: add SMMU support

Hi Tomasz,


pt., 3 lip 2020 o 11:33 Tomasz Nowicki <[email protected]> napisał(a):
>
> On 03.07.2020 11:16, Robin Murphy wrote:
> > On 2020-07-02 21:16, Tomasz Nowicki wrote:
> >> From: Marcin Wojtas <[email protected]>
> >>
> >> Add IOMMU node for Marvell AP806 based SoCs together with platform
> >> and PCI device Stream ID mapping.
> >>
> >> Signed-off-by: Marcin Wojtas <[email protected]>
> >> Signed-off-by: Tomasz Nowicki <[email protected]>
> >> ---
> >> arch/arm64/boot/dts/marvell/armada-8040.dtsi | 36 +++++++++++++++++++
> >> arch/arm64/boot/dts/marvell/armada-ap80x.dtsi | 17 +++++++++
> >> 2 files changed, 53 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/marvell/armada-8040.dtsi
> >> b/arch/arm64/boot/dts/marvell/armada-8040.dtsi
> >> index 7699b19224c2..25c1df709f72 100644
> >> --- a/arch/arm64/boot/dts/marvell/armada-8040.dtsi
> >> +++ b/arch/arm64/boot/dts/marvell/armada-8040.dtsi
> >> @@ -23,3 +23,39 @@
> >> &cp0_rtc {
> >> status = "disabled";
> >> };
> >> +
> >> +&cp0_usb3_0 {
> >> + iommus = <&smmu 0x440>;
> >> +};
> >> +
> >> +&cp0_usb3_1 {
> >> + iommus = <&smmu 0x441>;
> >> +};
> >> +
> >> +&cp0_sata0 {
> >> + iommus = <&smmu 0x444>;
> >> +};
> >> +
> >> +&cp0_sdhci0 {
> >> + iommus = <&smmu 0x445>;
> >> +};
> >> +
> >> +&cp1_sata0 {
> >> + iommus = <&smmu 0x454>;
> >> +};
> >> +
> >> +&cp1_usb3_0 {
> >> + iommus = <&smmu 0x450>;
> >> +};
> >> +
> >> +&cp1_usb3_1 {
> >> + iommus = <&smmu 0x451>;
> >> +};
> >> +
> >> +&cp0_pcie0 {
> >> + iommu-map =
> >> + <0x0 &smmu 0x480 0x20>,
> >> + <0x100 &smmu 0x4a0 0x20>,
> >> + <0x200 &smmu 0x4c0 0x20>;
> >> + iommu-map-mask = <0x031f>;
> >
> > Nice! I do like a good compressed mapping :D
> >
> >> +};
> >> diff --git a/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
> >> b/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
> >> index 7f9b9a647717..ded8b8082d79 100644
> >> --- a/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
> >> +++ b/arch/arm64/boot/dts/marvell/armada-ap80x.dtsi
> >> @@ -56,6 +56,23 @@
> >> compatible = "simple-bus";
> >> ranges = <0x0 0x0 0xf0000000 0x1000000>;
> >> + smmu: iommu@5000000 {
> >> + compatible = "marvell,ap806-smmu-500", "arm,mmu-500";
> >> + reg = <0x100000 0x100000>;
> >> + dma-coherent;
> >> + #iommu-cells = <1>;
> >> + #global-interrupts = <1>;
> >> + interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> >> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> >> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> >> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> >> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> >> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> >> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> >> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> >> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
> >
> > I'd recommend you have the node disabled by default here, then
> > explicitly enable it in armada-8040.dtsi where you add the Stream IDs.
> > Otherwise it will also end up enabled for 8020, 70x0, etc. where
> > disable_bypass will then catastrophically break everything.
> >
>
> Good point! I will fix this.
>

In addition to above, I think it is worth defining the stream ID's for
Armada 7040 and CN913x SoCs.

Best regards,
Marcin

2020-07-03 11:27:11

by Tomasz Nowicki

[permalink] [raw]
Subject: Re: [PATCH v3 2/4] iommu/arm-smmu: Workaround for Marvell Armada-AP806 SoC erratum #582743



On 03.07.2020 11:03, Robin Murphy wrote:
> On 2020-07-02 21:16, Tomasz Nowicki wrote:
>> From: Hanna Hawa <[email protected]>
>>
>> Due to erratum #582743, the Marvell Armada-AP806 can't access 64bit to
>> ARM SMMUv2 registers.
>>
>> Provide implementation relevant hooks:
>> - split the writeq/readq to two accesses of writel/readl.
>> - mask the MMU_IDR2.PTFSv8 fields to not use AArch64 format (but
>> only AARCH32_L) since with AArch64 format 32 bits access is not
>> supported.
>>
>> Note that separate writes/reads to 2 is not problem regards to
>> atomicity, because the driver use the readq/writeq while initialize
>> the SMMU, report for SMMU fault, and use spinlock in one
>> case (iova_to_phys).
>
> The comment about the spinlock seems to be out of date, and TBH that
> whole sentence is a bit unclear - how about something like:
>
> "Note that most 64-bit registers like TTBRn can be accessed as two
> 32-bit halves without issue, and AArch32 format ensures that the
> register writes which must be atomic (for TLBI etc.) need only be 32-bit."
>
>> Signed-off-by: Hanna Hawa <[email protected]>
>> Signed-off-by: Gregory CLEMENT <[email protected]>
>> Signed-off-by: Tomasz Nowicki <[email protected]>
>> ---
>>   Documentation/arm64/silicon-errata.rst |  3 ++
>>   drivers/iommu/arm-smmu-impl.c          | 52 ++++++++++++++++++++++++++
>>   2 files changed, 55 insertions(+)
>>
>> diff --git a/Documentation/arm64/silicon-errata.rst
>> b/Documentation/arm64/silicon-errata.rst
>> index 936cf2a59ca4..157214d3abe1 100644
>> --- a/Documentation/arm64/silicon-errata.rst
>> +++ b/Documentation/arm64/silicon-errata.rst
>> @@ -125,6 +125,9 @@ stable kernels.
>>   | Cavium         | ThunderX2 Core  | #219            |
>> CAVIUM_TX2_ERRATUM_219      |
>>
>> +----------------+-----------------+-----------------+-----------------------------+
>>
>>
>> +----------------+-----------------+-----------------+-----------------------------+
>>
>> +| Marvell        | ARM-MMU-500     | #582743         |
>> N/A                         |
>> ++----------------+-----------------+-----------------+-----------------------------+
>>
>> ++----------------+-----------------+-----------------+-----------------------------+
>>
>>   | Freescale/NXP  | LS2080A/LS1043A | A-008585        |
>> FSL_ERRATUM_A008585         |
>>
>> +----------------+-----------------+-----------------+-----------------------------+
>>
>>
>> +----------------+-----------------+-----------------+-----------------------------+
>>
>> diff --git a/drivers/iommu/arm-smmu-impl.c
>> b/drivers/iommu/arm-smmu-impl.c
>> index c75b9d957b70..c1fc5e1b8193 100644
>> --- a/drivers/iommu/arm-smmu-impl.c
>> +++ b/drivers/iommu/arm-smmu-impl.c
>> @@ -147,6 +147,53 @@ static const struct arm_smmu_impl arm_mmu500_impl
>> = {
>>       .reset = arm_mmu500_reset,
>>   };
>> +static u64 mrvl_mmu500_readq(struct arm_smmu_device *smmu, int page,
>> int off)
>> +{
>> +    u64 val;
>> +
>> +    /*
>> +     * Marvell Armada-AP806 erratum #582743.
>> +     * Split all the readq to double readl
>> +     */
>> +    val = (u64)readl_relaxed(arm_smmu_page(smmu, page) + off + 4) << 32;
>> +    val |= readl_relaxed(arm_smmu_page(smmu, page) + off);
>
> Even though io-64-nonatomic-hi-lo.h doesn't override readq() etc. for
> 64-bit builds, you can still use hi_lo_readq_relaxed() directly.
>
>> +
>> +    return val;
>> +}
>> +
>> +static void mrvl_mmu500_writeq(struct arm_smmu_device *smmu, int
>> page, int off,
>> +                   u64 val)
>> +{
>> +    /*
>> +     * Marvell Armada-AP806 erratum #582743.
>> +     * Split all the writeq to double writel
>> +     */
>> +    writel_relaxed(upper_32_bits(val), arm_smmu_page(smmu, page) +
>> off + 4);
>> +    writel_relaxed(lower_32_bits(val), arm_smmu_page(smmu, page) + off);
>
> Similarly, hi_lo_writeq_relaxed().
>
>> +}
>> +
>> +static u32 mrvl_mmu500_cfg_id2_fixup(u32 id)
>> +{
>> +
>> +    /*
>> +     * Armada-AP806 erratum #582743.
>> +     * Hide the SMMU_IDR2.PTFSv8 fields to sidestep the AArch64
>> +     * formats altogether and allow using 32 bits access on the
>> +     * interconnect.
>> +     */
>> +    id &= ~(ARM_SMMU_ID2_PTFS_4K | ARM_SMMU_ID2_PTFS_16K |
>> +        ARM_SMMU_ID2_PTFS_64K);
>> +
>> +    return id;
>> +}
>> +
>> +static const struct arm_smmu_impl mrvl_mmu500_impl = {
>> +    .read_reg64 = mrvl_mmu500_readq,
>> +    .write_reg64 = mrvl_mmu500_writeq,
>> +    .cfg_id2_fixup = mrvl_mmu500_cfg_id2_fixup,
>> +    .reset = arm_mmu500_reset,
>> +};
>> +
>>   struct arm_smmu_device *arm_smmu_impl_init(struct arm_smmu_device
>> *smmu)
>>   {
>> @@ -160,6 +207,11 @@ struct arm_smmu_device *arm_smmu_impl_init(struct
>> arm_smmu_device *smmu)
>>        */
>>       switch (smmu->model) {
>>       case ARM_MMU500:
>> +        if (of_device_is_compatible(smmu->dev->of_node,
>
> Nit: there's a local "np" variable now.
>
>> +                        "marvell,ap806-smmu-500")) {
>> +            smmu->impl = &mrvl_mmu500_impl;
>> +            return smmu;
>> +        }
>
> Please put this with the other integration checks below the switch
> statement. Yes, it means we'll end up assigning smmu->impl twice for
> this particular case, but that's the intended pattern.
>

Thanks, all above comments do make sense and will be fixed in next spin.

Thanks,
Tomasz

2020-07-13 21:37:07

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH v3 3/4] dt-bindings: arm-smmu: add compatible string for Marvell Armada-AP806 SMMU-500

On Fri, Jul 03, 2020 at 11:26:32AM +0200, Tomasz Nowicki wrote:
> On 03.07.2020 11:05, Robin Murphy wrote:
> > On 2020-07-02 21:16, Tomasz Nowicki wrote:
> > > Add specific compatible string for Marvell usage due to errata of
> > > accessing 64bits registers of ARM SMMU, in AP806.
> > >
> > > AP806 SoC uses the generic ARM-MMU500, and there's no specific
> > > implementation of Marvell, this compatible is used for errata only.
> > >
> > > Signed-off-by: Hanna Hawa <[email protected]>
> > > Signed-off-by: Gregory CLEMENT <[email protected]>
> > > Signed-off-by: Tomasz Nowicki <[email protected]>
> > > ---
> > > ? Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 5 +++++
> > > ? 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > > b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > > index d7ceb4c34423..7beca9c00b12 100644
> > > --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
> > > @@ -38,6 +38,11 @@ properties:
> > > ??????????????? - qcom,sc7180-smmu-500
> > > ??????????????? - qcom,sdm845-smmu-500
> > > ??????????? - const: arm,mmu-500
> > > +????? - description: Marvell SoCs implementing "arm,mmu-500"
> > > +??????? items:
> > > +????????? - enum:
> > > +????????????? - marvell,ap806-smmu-500
> >
> > Isn't a single-valued enum just a constant? :P
>
> That's how copy-paste engineering ends up :)

It's fine like this if you expect more SoCs to be added.

Either way,

Reviewed-by: Rob Herring <[email protected]>

2020-07-14 08:21:01

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH v3 0/4] Add system mmu support for Armada-806

Hi Tomasz,

On Thu, Jul 02, 2020 at 10:16:29PM +0200, Tomasz Nowicki wrote:
> There were already two versions of series to support SMMU for AP806,
> including workaround for accessing ARM SMMU 64bit registers.
> First [1] by Hanna Hawa and second [2] by Gregory CLEMENT.
> Since it got stuck this is yet another try. I incorporated the V2 comments,
> mainly by moving workaround code to arm-smmu-impl.c as requested.
>
> For the record, AP-806 can't access SMMU registers with 64bit width,
> this patches split the readq/writeq into two 32bit accesses instead
> and update DT bindings.
>
> The series was successfully tested on a vanilla v5.8-rc3 kernel and
> Intel e1000e PCIe NIC. The same for platform devices like SATA and USB.
>
> [1]: https://lkml.org/lkml/2018/10/15/373
> [2]: https://lkml.org/lkml/2019/7/11/426

Do you have a v4 of this series? It looks like there were a few comments
left to address, but with that I can pick it up for 5.9.

Cheers,

Will

2020-07-14 10:27:16

by Tomasz Nowicki

[permalink] [raw]
Subject: Re: [PATCH v3 0/4] Add system mmu support for Armada-806

Hi Will,

On 14.07.2020 10:19, Will Deacon wrote:
> Hi Tomasz,
>
> On Thu, Jul 02, 2020 at 10:16:29PM +0200, Tomasz Nowicki wrote:
>> There were already two versions of series to support SMMU for AP806,
>> including workaround for accessing ARM SMMU 64bit registers.
>> First [1] by Hanna Hawa and second [2] by Gregory CLEMENT.
>> Since it got stuck this is yet another try. I incorporated the V2 comments,
>> mainly by moving workaround code to arm-smmu-impl.c as requested.
>>
>> For the record, AP-806 can't access SMMU registers with 64bit width,
>> this patches split the readq/writeq into two 32bit accesses instead
>> and update DT bindings.
>>
>> The series was successfully tested on a vanilla v5.8-rc3 kernel and
>> Intel e1000e PCIe NIC. The same for platform devices like SATA and USB.
>>
>> [1]: https://lkml.org/lkml/2018/10/15/373
>> [2]: https://lkml.org/lkml/2019/7/11/426
>
> Do you have a v4 of this series? It looks like there were a few comments
> left to address, but with that I can pick it up for 5.9.

Yes, I will send it out today.

Thanks,
Tomasz