[PATCH v7 0/3] Cavium ThunderX2 SMMUv3 errata workarounds

[PATCH v7 0/3] Cavium ThunderX2 SMMUv3 errata workarounds

[Geetha sowjanya]

Cavium ThunderX2 SMMUv3 implementation has two Silicon Erratas.
1. Errata ID #74
   SMMU register alias Page 1 is not implemented
2. Errata ID #126
   SMMU doesnt support unique IRQ lines and also MSI for gerror,
   eventq and cmdq-sync

The following patchset does software workaround for these two erratas.

This series is based on patchset.
https://www.spinics.net/lists/arm-kernel/msg578443.html

Changes since v6:
   - Changed device tree compatible string to vendor specific.
   - Rebased on Robin's latest "Update SMMU models for IORT rev. C" v2 patch.
     https://www.spinics.net/lists/arm-kernel/msg582809.html

Changes since v5:
  - Rebased on Robin's "Update SMMU models for IORT rev. C" patch.
     https://www.spinics.net/lists/arm-kernel/msg580728.html
  - Replaced ACPI_IORT_SMMU_V3_CAVIUM_CN99XX macro with ACPI_IORT_SMMU_CAVIUM_CN99XX

Changes since v4:
 - Replaced all page1 offset macros ARM_SMMU_EVTQ/PRIQ_PROD/CONS with
    arm_smmu_page1_fixup(ARM_SMMU_EVTQ/PRIQ_PROD/CONS, smmu)

Changes since v3:
 - Merged patches 1, 2 and 4 of Version 3.
 - Modified the page1_offset_adjust() and get_irq_flags() implementation as
   suggested by Robin.

Changes since v2:
 - Updated "Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt" document with
   new SMMU option used to enable errata workaround.

Changes since v1:
 - Since the use of MIDR register is rejected and SMMU_IIDR is broken on this
   silicon, as suggested by Will Deacon modified the patches to use ThunderX2
   SMMUv3 IORT model number to enable errata workaround.

Geetha Sowjanya (1):
  iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126

Linu Cherian (2):
  ACPI/IORT: Fixup SMMUv3 resource size for Cavium ThunderX2 SMMUv3
    model
  iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2      erratum
    #74

 Documentation/arm64/silicon-errata.txt             |    2 +
 .../devicetree/bindings/iommu/arm,smmu-v3.txt      |    6 ++
 drivers/acpi/arm64/iort.c                          |   10 ++-
 drivers/iommu/arm-smmu-v3.c                        |   93 ++++++++++++++++----
 4 files changed, 91 insertions(+), 20 deletions(-)

[PATCH v7 1/3] ACPI/IORT: Fixup SMMUv3 resource size for Cavium ThunderX2 SMMUv3 model

[Geetha sowjanya]

From: Linu Cherian <linu.cherian@cavium.com>

Cavium ThunderX2 implementation doesn't support second page in SMMU
register space. Hence, resource size is set as 64k for this model.

Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
---
 drivers/acpi/arm64/iort.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index c5fecf9..bba2b59 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -833,12 +833,20 @@ static void __init arm_smmu_v3_init_resources(struct resource *res,
 {
 	struct acpi_iort_smmu_v3 *smmu;
 	int num_res = 0;
+	unsigned long size = SZ_128K;
 
 	/* Retrieve SMMUv3 specific data */
 	smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
 
+	/*
+	 * Override the size, for Cavium ThunderX2 implementation
+	 * which doesn't support the page 1 SMMU register space.
+	 */
+	if (smmu->model == ACPI_IORT_SMMU_CAVIUM_CN99XX)
+		size = SZ_64K;
+
 	res[num_res].start = smmu->base_address;
-	res[num_res].end = smmu->base_address + SZ_128K - 1;
+	res[num_res].end = smmu->base_address + size - 1;
 	res[num_res].flags = IORESOURCE_MEM;
 
 	num_res++;
-- 
1.7.1

Re: [PATCH v7 1/3] ACPI/IORT: Fixup SMMUv3 resource size for Cavium ThunderX2 SMMUv3 model

[Lorenzo Pieralisi]

On Tue, May 30, 2017 at 05:33:39PM +0530, Geetha sowjanya wrote:
> From: Linu Cherian <linu.cherian@cavium.com>
> 
> Cavium ThunderX2 implementation doesn't support second page in SMMU
> register space. Hence, resource size is set as 64k for this model.
> 
> Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
> Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
> ---
>  drivers/acpi/arm64/iort.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index c5fecf9..bba2b59 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -833,12 +833,20 @@ static void __init arm_smmu_v3_init_resources(struct resource *res,
>  {
>  	struct acpi_iort_smmu_v3 *smmu;
>  	int num_res = 0;
> +	unsigned long size = SZ_128K;
>  
>  	/* Retrieve SMMUv3 specific data */
>  	smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
>  
> +	/*
> +	 * Override the size, for Cavium ThunderX2 implementation
> +	 * which doesn't support the page 1 SMMU register space.
> +	 */
> +	if (smmu->model == ACPI_IORT_SMMU_CAVIUM_CN99XX)
> +		size = SZ_64K;

Nit: add a function, say arm_smmu_v3_resource_size() with the logic
above that by default returns SZ_128K, I do not like this switch
in the middle of this function.

Lorenzo

> +
>  	res[num_res].start = smmu->base_address;
> -	res[num_res].end = smmu->base_address + SZ_128K - 1;
> +	res[num_res].end = smmu->base_address + size - 1;
>  	res[num_res].flags = IORESOURCE_MEM;
>  
>  	num_res++;
> -- 
> 1.7.1
> 

Re: [PATCH v7 1/3] ACPI/IORT: Fixup SMMUv3 resource size for Cavium ThunderX2 SMMUv3 model

[Geetha Akula]

On Thu, Jun 8, 2017 at 2:28 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Tue, May 30, 2017 at 05:33:39PM +0530, Geetha sowjanya wrote:
>> From: Linu Cherian <linu.cherian@cavium.com>
>>
>> Cavium ThunderX2 implementation doesn't support second page in SMMU
>> register space. Hence, resource size is set as 64k for this model.
>>
>> Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
>> Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
>> ---
>>  drivers/acpi/arm64/iort.c |   10 +++++++++-
>>  1 files changed, 9 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>> index c5fecf9..bba2b59 100644
>> --- a/drivers/acpi/arm64/iort.c
>> +++ b/drivers/acpi/arm64/iort.c
>> @@ -833,12 +833,20 @@ static void __init arm_smmu_v3_init_resources(struct resource *res,
>>  {
>>       struct acpi_iort_smmu_v3 *smmu;
>>       int num_res = 0;
>> +     unsigned long size = SZ_128K;
>>
>>       /* Retrieve SMMUv3 specific data */
>>       smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
>>
>> +     /*
>> +      * Override the size, for Cavium ThunderX2 implementation
>> +      * which doesn't support the page 1 SMMU register space.
>> +      */
>> +     if (smmu->model == ACPI_IORT_SMMU_CAVIUM_CN99XX)
>> +             size = SZ_64K;
>
> Nit: add a function, say arm_smmu_v3_resource_size() with the logic
> above that by default returns SZ_128K, I do not like this switch
> in the middle of this function.

I will resubmit the patch with suggested changes.


Thanks,
Geetha.
>
> Lorenzo
>
>> +
>>       res[num_res].start = smmu->base_address;
>> -     res[num_res].end = smmu->base_address + SZ_128K - 1;
>> +     res[num_res].end = smmu->base_address + size - 1;
>>       res[num_res].flags = IORESOURCE_MEM;
>>
>>       num_res++;
>> --
>> 1.7.1
>>

Re: [PATCH v7 1/3] ACPI/IORT: Fixup SMMUv3 resource size for Cavium ThunderX2 SMMUv3 model

[Robert Richter]

On 30.05.17 17:33:39, Geetha sowjanya wrote:
> From: Linu Cherian <linu.cherian@cavium.com>
> 
> Cavium ThunderX2 implementation doesn't support second page in SMMU
> register space. Hence, resource size is set as 64k for this model.
> 
> Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
> Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
> ---
>  drivers/acpi/arm64/iort.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index c5fecf9..bba2b59 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -833,12 +833,20 @@ static void __init arm_smmu_v3_init_resources(struct resource *res,
>  {
>  	struct acpi_iort_smmu_v3 *smmu;
>  	int num_res = 0;
> +	unsigned long size = SZ_128K;
>  
>  	/* Retrieve SMMUv3 specific data */
>  	smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
>  
> +	/*
> +	 * Override the size, for Cavium ThunderX2 implementation
> +	 * which doesn't support the page 1 SMMU register space.
> +	 */
> +	if (smmu->model == ACPI_IORT_SMMU_CAVIUM_CN99XX)

Geetha,

please resubmit the series since the macro changed to
ACPI_IORT_SMMU_V3_CAVIUM_CN99XX:

 https://github.com/acpica/acpica/commit/d00a4eb86e64bb4fa70f57ab5e5ca0a4ca2ad8ef#diff-a572aac2ccc26fe4a901616d7fdba859R1053

-Robert

> +		size = SZ_64K;
> +
>  	res[num_res].start = smmu->base_address;
> -	res[num_res].end = smmu->base_address + SZ_128K - 1;
> +	res[num_res].end = smmu->base_address + size - 1;
>  	res[num_res].flags = IORESOURCE_MEM;
>  
>  	num_res++;
> -- 
> 1.7.1
> 

Re: [PATCH v7 1/3] ACPI/IORT: Fixup SMMUv3 resource size for Cavium ThunderX2 SMMUv3 model

[Robert Richter]

On 20.06.17 10:19:43, Robert Richter wrote:
> On 30.05.17 17:33:39, Geetha sowjanya wrote:
> > From: Linu Cherian <linu.cherian@cavium.com>

> > +	/*
> > +	 * Override the size, for Cavium ThunderX2 implementation
> > +	 * which doesn't support the page 1 SMMU register space.
> > +	 */
> > +	if (smmu->model == ACPI_IORT_SMMU_CAVIUM_CN99XX)
> 
> Geetha,
> 
> please resubmit the series since the macro changed to
> ACPI_IORT_SMMU_V3_CAVIUM_CN99XX:
> 
>  https://github.com/acpica/acpica/commit/d00a4eb86e64bb4fa70f57ab5e5ca0a4ca2ad8ef#diff-a572aac2ccc26fe4a901616d7fdba859R1053

Rafael, Bob,

btw, I haven't seen

 https://github.com/acpica/acpica/commit/d00a4eb86e64

yet in linux-pm:linux-next. We would like to see this in 4.13 also. Is
it on the way already?

Thanks,

-Robert

Re: [PATCH v7 1/3] ACPI/IORT: Fixup SMMUv3 resource size for Cavium ThunderX2 SMMUv3 model

[Lorenzo Pieralisi]

On Tue, Jun 20, 2017 at 10:51:23AM +0200, Robert Richter wrote:
> On 20.06.17 10:19:43, Robert Richter wrote:
> > On 30.05.17 17:33:39, Geetha sowjanya wrote:
> > > From: Linu Cherian <linu.cherian@cavium.com>
> 
> > > +	/*
> > > +	 * Override the size, for Cavium ThunderX2 implementation
> > > +	 * which doesn't support the page 1 SMMU register space.
> > > +	 */
> > > +	if (smmu->model == ACPI_IORT_SMMU_CAVIUM_CN99XX)
> > 
> > Geetha,
> > 
> > please resubmit the series since the macro changed to
> > ACPI_IORT_SMMU_V3_CAVIUM_CN99XX:
> > 
> >  https://github.com/acpica/acpica/commit/d00a4eb86e64bb4fa70f57ab5e5ca0a4ca2ad8ef#diff-a572aac2ccc26fe4a901616d7fdba859R1053
> 
> Rafael, Bob,
> 
> btw, I haven't seen
> 
>  https://github.com/acpica/acpica/commit/d00a4eb86e64
> 
> yet in linux-pm:linux-next. We would like to see this in 4.13 also. Is
> it on the way already?

Yes it would be great to understand how we can make sure IORT and
related ACPICA changes can be kept in sync for this merge window, it
is -rc6 already so it is getting a bit late in the cycle.

Thanks,
Lorenzo

[PATCH v7 2/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #74

[Geetha sowjanya]

From: Linu Cherian <linu.cherian@cavium.com>

Cavium ThunderX2 SMMU implementation doesn't support page 1 register space
and PAGE0_REGS_ONLY option is enabled as an errata workaround.
This option when turned on, replaces all page 1 offsets used for
EVTQ_PROD/CONS, PRIQ_PROD/CONS register access with page 0 offsets.

SMMU resource size checks are now based on SMMU option PAGE0_REGS_ONLY,
since resource size can be either 64k/128k.
For this, arm_smmu_device_dt_probe/acpi_probe has been moved before
platform_get_resource call, so that SMMU options are set beforehand.

Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
---
 Documentation/arm64/silicon-errata.txt             |    1 +
 .../devicetree/bindings/iommu/arm,smmu-v3.txt      |    6 ++
 drivers/iommu/arm-smmu-v3.c                        |   64 +++++++++++++++-----
 3 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
index 10f2ddd..4693a32 100644
--- a/Documentation/arm64/silicon-errata.txt
+++ b/Documentation/arm64/silicon-errata.txt
@@ -62,6 +62,7 @@ stable kernels.
 | Cavium         | ThunderX GICv3  | #23154          | CAVIUM_ERRATUM_23154        |
 | Cavium         | ThunderX Core   | #27456          | CAVIUM_ERRATUM_27456        |
 | Cavium         | ThunderX SMMUv2 | #27704          | N/A                         |
+| Cavium         | ThunderX2 SMMUv3| #74             | N/A                         |
 |                |                 |                 |                             |
 | Freescale/NXP  | LS2080A/LS1043A | A-008585        | FSL_ERRATUM_A008585         |
 |                |                 |                 |                             |
diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
index be57550..607e270 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
@@ -49,6 +49,12 @@ the PCIe specification.
 - hisilicon,broken-prefetch-cmd
                     : Avoid sending CMD_PREFETCH_* commands to the SMMU.
 
+- cavium,cn9900-broken-page1-regspace
+                    : Replaces all page 1 offsets used for EVTQ_PROD/CONS,
+						PRIQ_PROD/CONS register access with page 0 offsets.
+						Set for Caviun ThunderX2 silicon that doesn't support
+						SMMU page1 register space.
+
 ** Example
 
         smmu@2b400000 {
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 380969a..4e80205 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -412,6 +412,9 @@
 #define MSI_IOVA_BASE			0x8000000
 #define MSI_IOVA_LENGTH			0x100000
 
+#define ARM_SMMU_PAGE0_REGS_ONLY(smmu)		\
+	((smmu)->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)
+
 static bool disable_bypass;
 module_param_named(disable_bypass, disable_bypass, bool, S_IRUGO);
 MODULE_PARM_DESC(disable_bypass,
@@ -597,6 +600,7 @@ struct arm_smmu_device {
 	u32				features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
+#define ARM_SMMU_OPT_PAGE0_REGS_ONLY    (1 << 1)
 	u32				options;
 
 	struct arm_smmu_cmdq		cmdq;
@@ -663,9 +667,19 @@ struct arm_smmu_option_prop {
 
 static struct arm_smmu_option_prop arm_smmu_options[] = {
 	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
+	{ ARM_SMMU_OPT_PAGE0_REGS_ONLY, "cavium,cn9900-broken-page1-regspace"},
 	{ 0, NULL},
 };
 
+static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset,
+						 struct arm_smmu_device *smmu)
+{
+	if (offset > SZ_64K && ARM_SMMU_PAGE0_REGS_ONLY(smmu))
+		offset -= SZ_64K;
+
+	return smmu->base + offset;
+}
+
 static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
 {
 	return container_of(dom, struct arm_smmu_domain, domain);
@@ -1961,8 +1975,8 @@ static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
 		return -ENOMEM;
 	}
 
-	q->prod_reg	= smmu->base + prod_off;
-	q->cons_reg	= smmu->base + cons_off;
+	q->prod_reg	= arm_smmu_page1_fixup(prod_off, smmu);
+	q->cons_reg	= arm_smmu_page1_fixup(cons_off, smmu);
 	q->ent_dwords	= dwords;
 
 	q->q_base  = Q_BASE_RWA;
@@ -2363,8 +2377,10 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
 
 	/* Event queue */
 	writeq_relaxed(smmu->evtq.q.q_base, smmu->base + ARM_SMMU_EVTQ_BASE);
-	writel_relaxed(smmu->evtq.q.prod, smmu->base + ARM_SMMU_EVTQ_PROD);
-	writel_relaxed(smmu->evtq.q.cons, smmu->base + ARM_SMMU_EVTQ_CONS);
+	writel_relaxed(smmu->evtq.q.prod,
+		       arm_smmu_page1_fixup(ARM_SMMU_EVTQ_PROD, smmu));
+	writel_relaxed(smmu->evtq.q.cons,
+		       arm_smmu_page1_fixup(ARM_SMMU_EVTQ_CONS, smmu));
 
 	enables |= CR0_EVTQEN;
 	ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
@@ -2379,9 +2395,9 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
 		writeq_relaxed(smmu->priq.q.q_base,
 			       smmu->base + ARM_SMMU_PRIQ_BASE);
 		writel_relaxed(smmu->priq.q.prod,
-			       smmu->base + ARM_SMMU_PRIQ_PROD);
+			       arm_smmu_page1_fixup(ARM_SMMU_PRIQ_PROD, smmu));
 		writel_relaxed(smmu->priq.q.cons,
-			       smmu->base + ARM_SMMU_PRIQ_CONS);
+			       arm_smmu_page1_fixup(ARM_SMMU_PRIQ_CONS, smmu));
 
 		enables |= CR0_PRIQEN;
 		ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
@@ -2605,6 +2621,14 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
 }
 
 #ifdef CONFIG_ACPI
+static void acpi_smmu_get_options(u32 model, struct arm_smmu_device *smmu)
+{
+	if (model == ACPI_IORT_SMMU_CAVIUM_CN99XX)
+		smmu->options |= ARM_SMMU_OPT_PAGE0_REGS_ONLY;
+
+	dev_notice(smmu->dev, "option mask 0x%x\n", smmu->options);
+}
+
 static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
 				      struct arm_smmu_device *smmu)
 {
@@ -2617,6 +2641,8 @@ static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
 	/* Retrieve SMMUv3 specific data */
 	iort_smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
 
+	acpi_smmu_get_options(iort_smmu->model, smmu);
+
 	if (iort_smmu->flags & ACPI_IORT_SMMU_V3_COHACC_OVERRIDE)
 		smmu->features |= ARM_SMMU_FEAT_COHERENCY;
 
@@ -2652,6 +2678,14 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
 	return ret;
 }
 
+static unsigned long arm_smmu_resource_size(struct arm_smmu_device *smmu)
+{
+	if (ARM_SMMU_PAGE0_REGS_ONLY(smmu))
+		return SZ_64K;
+	else
+		return SZ_128K;
+}
+
 static int arm_smmu_device_probe(struct platform_device *pdev)
 {
 	int irq, ret;
@@ -2668,9 +2702,17 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	}
 	smmu->dev = dev;
 
+	if (dev->of_node) {
+		ret = arm_smmu_device_dt_probe(pdev, smmu);
+	} else {
+		ret = arm_smmu_device_acpi_probe(pdev, smmu);
+		if (ret == -ENODEV)
+			return ret;
+	}
+
 	/* Base address */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (resource_size(res) + 1 < SZ_128K) {
+	if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
 		dev_err(dev, "MMIO region too small (%pr)\n", res);
 		return -EINVAL;
 	}
@@ -2697,14 +2739,6 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	if (irq > 0)
 		smmu->gerr_irq = irq;
 
-	if (dev->of_node) {
-		ret = arm_smmu_device_dt_probe(pdev, smmu);
-	} else {
-		ret = arm_smmu_device_acpi_probe(pdev, smmu);
-		if (ret == -ENODEV)
-			return ret;
-	}
-
 	/* Set bypass mode according to firmware probing result */
 	bypass = !!ret;
 
-- 
1.7.1

Re: [PATCH v7 2/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #74

[Robin Murphy]

On 30/05/17 13:03, Geetha sowjanya wrote:
> From: Linu Cherian <linu.cherian@cavium.com>
> 
> Cavium ThunderX2 SMMU implementation doesn't support page 1 register space
> and PAGE0_REGS_ONLY option is enabled as an errata workaround.
> This option when turned on, replaces all page 1 offsets used for
> EVTQ_PROD/CONS, PRIQ_PROD/CONS register access with page 0 offsets.
> 
> SMMU resource size checks are now based on SMMU option PAGE0_REGS_ONLY,
> since resource size can be either 64k/128k.
> For this, arm_smmu_device_dt_probe/acpi_probe has been moved before
> platform_get_resource call, so that SMMU options are set beforehand.
> 
> Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
> Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
> ---
>  Documentation/arm64/silicon-errata.txt             |    1 +
>  .../devicetree/bindings/iommu/arm,smmu-v3.txt      |    6 ++
>  drivers/iommu/arm-smmu-v3.c                        |   64 +++++++++++++++-----
>  3 files changed, 56 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
> index 10f2ddd..4693a32 100644
> --- a/Documentation/arm64/silicon-errata.txt
> +++ b/Documentation/arm64/silicon-errata.txt
> @@ -62,6 +62,7 @@ stable kernels.
>  | Cavium         | ThunderX GICv3  | #23154          | CAVIUM_ERRATUM_23154        |
>  | Cavium         | ThunderX Core   | #27456          | CAVIUM_ERRATUM_27456        |
>  | Cavium         | ThunderX SMMUv2 | #27704          | N/A                         |
> +| Cavium         | ThunderX2 SMMUv3| #74             | N/A                         |
>  |                |                 |                 |                             |
>  | Freescale/NXP  | LS2080A/LS1043A | A-008585        | FSL_ERRATUM_A008585         |
>  |                |                 |                 |                             |
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> index be57550..607e270 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> @@ -49,6 +49,12 @@ the PCIe specification.
>  - hisilicon,broken-prefetch-cmd
>                      : Avoid sending CMD_PREFETCH_* commands to the SMMU.
>  
> +- cavium,cn9900-broken-page1-regspace
> +                    : Replaces all page 1 offsets used for EVTQ_PROD/CONS,
> +						PRIQ_PROD/CONS register access with page 0 offsets.
> +						Set for Caviun ThunderX2 silicon that doesn't support
> +						SMMU page1 register space.

The indentation's a bit funky here - the rest of this file is actually
indented with spaces, but either way it's clear your editor isn't set to
8-space tabs ;)

> +
>  ** Example
>  
>          smmu@2b400000 {
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 380969a..4e80205 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -412,6 +412,9 @@
>  #define MSI_IOVA_BASE			0x8000000
>  #define MSI_IOVA_LENGTH			0x100000
>  
> +#define ARM_SMMU_PAGE0_REGS_ONLY(smmu)		\
> +	((smmu)->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)

At the two places we use this macro, frankly I think it would be clearer
to just reference smmu->options directly, as we currently do for
SKIP_PREFETCH. The abstraction also adds more lines than it saves...

> +
>  static bool disable_bypass;
>  module_param_named(disable_bypass, disable_bypass, bool, S_IRUGO);
>  MODULE_PARM_DESC(disable_bypass,
> @@ -597,6 +600,7 @@ struct arm_smmu_device {
>  	u32				features;
>  
>  #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
> +#define ARM_SMMU_OPT_PAGE0_REGS_ONLY    (1 << 1)

Whitespace again, although this time it's spaces where there should be a
tab.

>  	u32				options;
>  
>  	struct arm_smmu_cmdq		cmdq;
> @@ -663,9 +667,19 @@ struct arm_smmu_option_prop {
>  
>  static struct arm_smmu_option_prop arm_smmu_options[] = {
>  	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
> +	{ ARM_SMMU_OPT_PAGE0_REGS_ONLY, "cavium,cn9900-broken-page1-regspace"},
>  	{ 0, NULL},
>  };
>  
> +static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset,
> +						 struct arm_smmu_device *smmu)
> +{
> +	if (offset > SZ_64K && ARM_SMMU_PAGE0_REGS_ONLY(smmu))
> +		offset -= SZ_64K;
> +
> +	return smmu->base + offset;
> +}
> +
>  static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
>  {
>  	return container_of(dom, struct arm_smmu_domain, domain);
> @@ -1961,8 +1975,8 @@ static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
>  		return -ENOMEM;
>  	}
>  
> -	q->prod_reg	= smmu->base + prod_off;
> -	q->cons_reg	= smmu->base + cons_off;
> +	q->prod_reg	= arm_smmu_page1_fixup(prod_off, smmu);
> +	q->cons_reg	= arm_smmu_page1_fixup(cons_off, smmu);
>  	q->ent_dwords	= dwords;
>  
>  	q->q_base  = Q_BASE_RWA;
> @@ -2363,8 +2377,10 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
>  
>  	/* Event queue */
>  	writeq_relaxed(smmu->evtq.q.q_base, smmu->base + ARM_SMMU_EVTQ_BASE);
> -	writel_relaxed(smmu->evtq.q.prod, smmu->base + ARM_SMMU_EVTQ_PROD);
> -	writel_relaxed(smmu->evtq.q.cons, smmu->base + ARM_SMMU_EVTQ_CONS);
> +	writel_relaxed(smmu->evtq.q.prod,
> +		       arm_smmu_page1_fixup(ARM_SMMU_EVTQ_PROD, smmu));
> +	writel_relaxed(smmu->evtq.q.cons,
> +		       arm_smmu_page1_fixup(ARM_SMMU_EVTQ_CONS, smmu));
>  
>  	enables |= CR0_EVTQEN;
>  	ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
> @@ -2379,9 +2395,9 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
>  		writeq_relaxed(smmu->priq.q.q_base,
>  			       smmu->base + ARM_SMMU_PRIQ_BASE);
>  		writel_relaxed(smmu->priq.q.prod,
> -			       smmu->base + ARM_SMMU_PRIQ_PROD);
> +			       arm_smmu_page1_fixup(ARM_SMMU_PRIQ_PROD, smmu));
>  		writel_relaxed(smmu->priq.q.cons,
> -			       smmu->base + ARM_SMMU_PRIQ_CONS);
> +			       arm_smmu_page1_fixup(ARM_SMMU_PRIQ_CONS, smmu));
>  
>  		enables |= CR0_PRIQEN;
>  		ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
> @@ -2605,6 +2621,14 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
>  }
>  
>  #ifdef CONFIG_ACPI
> +static void acpi_smmu_get_options(u32 model, struct arm_smmu_device *smmu)
> +{
> +	if (model == ACPI_IORT_SMMU_CAVIUM_CN99XX)
> +		smmu->options |= ARM_SMMU_OPT_PAGE0_REGS_ONLY;
> +
> +	dev_notice(smmu->dev, "option mask 0x%x\n", smmu->options);
> +}
> +
>  static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
>  				      struct arm_smmu_device *smmu)
>  {
> @@ -2617,6 +2641,8 @@ static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
>  	/* Retrieve SMMUv3 specific data */
>  	iort_smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
>  
> +	acpi_smmu_get_options(iort_smmu->model, smmu);
> +
>  	if (iort_smmu->flags & ACPI_IORT_SMMU_V3_COHACC_OVERRIDE)
>  		smmu->features |= ARM_SMMU_FEAT_COHERENCY;
>  
> @@ -2652,6 +2678,14 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>  	return ret;
>  }
>  
> +static unsigned long arm_smmu_resource_size(struct arm_smmu_device *smmu)
> +{
> +	if (ARM_SMMU_PAGE0_REGS_ONLY(smmu))
> +		return SZ_64K;
> +	else
> +		return SZ_128K;
> +}
> +
>  static int arm_smmu_device_probe(struct platform_device *pdev)
>  {
>  	int irq, ret;
> @@ -2668,9 +2702,17 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>  	}
>  	smmu->dev = dev;
>  
> +	if (dev->of_node) {
> +		ret = arm_smmu_device_dt_probe(pdev, smmu);
> +	} else {
> +		ret = arm_smmu_device_acpi_probe(pdev, smmu);
> +		if (ret == -ENODEV)
> +			return ret;
> +	}
> +
>  	/* Base address */
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (resource_size(res) + 1 < SZ_128K) {
> +	if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
>  		dev_err(dev, "MMIO region too small (%pr)\n", res);
>  		return -EINVAL;
>  	}
> @@ -2697,14 +2739,6 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>  	if (irq > 0)
>  		smmu->gerr_irq = irq;
>  
> -	if (dev->of_node) {
> -		ret = arm_smmu_device_dt_probe(pdev, smmu);
> -	} else {
> -		ret = arm_smmu_device_acpi_probe(pdev, smmu);
> -		if (ret == -ENODEV)
> -			return ret;
> -	}
> -
>  	/* Set bypass mode according to firmware probing result */
>  	bypass = !!ret;

The bypass setting is directly related to the {dt,acpi}_probe logic, so
should move along with it. I can see that this ends up still working
because none of the interim code touches ret, but it does make things
unnecessarily hard to follow.

Robin.

Re: Fwd: [PATCH v7 2/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #74

[Jayachandran C]

On Fri, Jun 09, 2017 Robin Murphy wrote:
> 
> On 30/05/17 13:03, Geetha sowjanya wrote:
> > From: Linu Cherian <linu.cherian@cavium.com>
> >
> > Cavium ThunderX2 SMMU implementation doesn't support page 1 register space
> > and PAGE0_REGS_ONLY option is enabled as an errata workaround.
> > This option when turned on, replaces all page 1 offsets used for
> > EVTQ_PROD/CONS, PRIQ_PROD/CONS register access with page 0 offsets.
> >
> > SMMU resource size checks are now based on SMMU option PAGE0_REGS_ONLY,
> > since resource size can be either 64k/128k.
> > For this, arm_smmu_device_dt_probe/acpi_probe has been moved before
> > platform_get_resource call, so that SMMU options are set beforehand.
> >
> > Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
> > Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
> > ---
> >  Documentation/arm64/silicon-errata.txt             |    1 +
> >  .../devicetree/bindings/iommu/arm,smmu-v3.txt      |    6 ++
> >  drivers/iommu/arm-smmu-v3.c                        |   64 +++++++++++++++-----
> >  3 files changed, 56 insertions(+), 15 deletions(-)
> >
> > diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
> > index 10f2ddd..4693a32 100644
> > --- a/Documentation/arm64/silicon-errata.txt
> > +++ b/Documentation/arm64/silicon-errata.txt
> > @@ -62,6 +62,7 @@ stable kernels.
> >  | Cavium         | ThunderX GICv3  | #23154          | CAVIUM_ERRATUM_23154        |
> >  | Cavium         | ThunderX Core   | #27456          | CAVIUM_ERRATUM_27456        |
> >  | Cavium         | ThunderX SMMUv2 | #27704          | N/A                         |
> > +| Cavium         | ThunderX2 SMMUv3| #74             | N/A                         |
> >  |                |                 |                 |                             |
> >  | Freescale/NXP  | LS2080A/LS1043A | A-008585        | FSL_ERRATUM_A008585         |
> >  |                |                 |                 |                             |
> > diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> > index be57550..607e270 100644
> > --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> > +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> > @@ -49,6 +49,12 @@ the PCIe specification.
> >  - hisilicon,broken-prefetch-cmd
> >                      : Avoid sending CMD_PREFETCH_* commands to the SMMU.
> >
> > +- cavium,cn9900-broken-page1-regspace
> > +                    : Replaces all page 1 offsets used for EVTQ_PROD/CONS,
> > +                                             PRIQ_PROD/CONS register access with page 0 offsets.
> > +                                             Set for Caviun ThunderX2 silicon that doesn't support
> > +                                             SMMU page1 register space.
> 
> The indentation's a bit funky here - the rest of this file is actually
> indented with spaces, but either way it's clear your editor isn't set to
> 8-space tabs ;)
> 
> > +
> >  ** Example
> >
> >          smmu@2b400000 {
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > index 380969a..4e80205 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -412,6 +412,9 @@
> >  #define MSI_IOVA_BASE                        0x8000000
> >  #define MSI_IOVA_LENGTH                      0x100000
> >
> > +#define ARM_SMMU_PAGE0_REGS_ONLY(smmu)               \
> > +     ((smmu)->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)
> 
> At the two places we use this macro, frankly I think it would be clearer
> to just reference smmu->options directly, as we currently do for
> SKIP_PREFETCH. The abstraction also adds more lines than it saves...
> 
> > +
> >  static bool disable_bypass;
> >  module_param_named(disable_bypass, disable_bypass, bool, S_IRUGO);
> >  MODULE_PARM_DESC(disable_bypass,
> > @@ -597,6 +600,7 @@ struct arm_smmu_device {
> >       u32                             features;
> >
> >  #define ARM_SMMU_OPT_SKIP_PREFETCH   (1 << 0)
> > +#define ARM_SMMU_OPT_PAGE0_REGS_ONLY    (1 << 1)
> 
> Whitespace again, although this time it's spaces where there should be a
> tab.
> 
> >       u32                             options;
> >
> >       struct arm_smmu_cmdq            cmdq;
> > @@ -663,9 +667,19 @@ struct arm_smmu_option_prop {
> >
> >  static struct arm_smmu_option_prop arm_smmu_options[] = {
> >       { ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
> > +     { ARM_SMMU_OPT_PAGE0_REGS_ONLY, "cavium,cn9900-broken-page1-regspace"},
> >       { 0, NULL},
> >  };
> >
> > +static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset,
> > +                                              struct arm_smmu_device *smmu)
> > +{
> > +     if (offset > SZ_64K && ARM_SMMU_PAGE0_REGS_ONLY(smmu))
> > +             offset -= SZ_64K;
> > +
> > +     return smmu->base + offset;
> > +}
> > +
> >  static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
> >  {
> >       return container_of(dom, struct arm_smmu_domain, domain);
> > @@ -1961,8 +1975,8 @@ static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
> >               return -ENOMEM;
> >       }
> >
> > -     q->prod_reg     = smmu->base + prod_off;
> > -     q->cons_reg     = smmu->base + cons_off;
> > +     q->prod_reg     = arm_smmu_page1_fixup(prod_off, smmu);
> > +     q->cons_reg     = arm_smmu_page1_fixup(cons_off, smmu);
> >       q->ent_dwords   = dwords;
> >
> >       q->q_base  = Q_BASE_RWA;
> > @@ -2363,8 +2377,10 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
> >
> >       /* Event queue */
> >       writeq_relaxed(smmu->evtq.q.q_base, smmu->base + ARM_SMMU_EVTQ_BASE);
> > -     writel_relaxed(smmu->evtq.q.prod, smmu->base + ARM_SMMU_EVTQ_PROD);
> > -     writel_relaxed(smmu->evtq.q.cons, smmu->base + ARM_SMMU_EVTQ_CONS);
> > +     writel_relaxed(smmu->evtq.q.prod,
> > +                    arm_smmu_page1_fixup(ARM_SMMU_EVTQ_PROD, smmu));
> > +     writel_relaxed(smmu->evtq.q.cons,
> > +                    arm_smmu_page1_fixup(ARM_SMMU_EVTQ_CONS, smmu));

This sequence and the arm_smmu_page1_fixup() call is repeated in quite a few
places. I think this errata code is messy because the original driver does not
make the alias page usage explicit. 

A patch like the one below (no functional changes) would clean up the original
driver and make the errata change much simpler - any comments?

-- >8 --

Date: Tue, 30 May 2017 15:43:29 +0000
Subject: [PATCH] iommu: arm-smmu-v3: make alias page usage explicit

---
 drivers/iommu/arm-smmu-v3.c | 76 +++++++++++++++++++++++++++------------------
 1 file changed, 46 insertions(+), 30 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 380969a..11fdb4f 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -171,20 +171,19 @@
 #define STRTAB_BASE_CFG_FMT_LINEAR	(0 << STRTAB_BASE_CFG_FMT_SHIFT)
 #define STRTAB_BASE_CFG_FMT_2LVL	(1 << STRTAB_BASE_CFG_FMT_SHIFT)
 
+#define ARM_SMMU_Q_PROD(qbase)		((qbase) + 0x8)
+#define ARM_SMMU_Q_PROD_PAGE1(qbase)	((qbase) + 0x10008)
+#define ARM_SMMU_Q_CONS(qbase)		((qbase) + 0xc)
+#define ARM_SMMU_Q_CONS_PAGE1(qbase)	((qbase) + 0x1000c)
+
 #define ARM_SMMU_CMDQ_BASE		0x90
-#define ARM_SMMU_CMDQ_PROD		0x98
-#define ARM_SMMU_CMDQ_CONS		0x9c
 
 #define ARM_SMMU_EVTQ_BASE		0xa0
-#define ARM_SMMU_EVTQ_PROD		0x100a8
-#define ARM_SMMU_EVTQ_CONS		0x100ac
 #define ARM_SMMU_EVTQ_IRQ_CFG0		0xb0
 #define ARM_SMMU_EVTQ_IRQ_CFG1		0xb8
 #define ARM_SMMU_EVTQ_IRQ_CFG2		0xbc
 
 #define ARM_SMMU_PRIQ_BASE		0xc0
-#define ARM_SMMU_PRIQ_PROD		0x100c8
-#define ARM_SMMU_PRIQ_CONS		0x100cc
 #define ARM_SMMU_PRIQ_IRQ_CFG0		0xd0
 #define ARM_SMMU_PRIQ_IRQ_CFG1		0xd8
 #define ARM_SMMU_PRIQ_IRQ_CFG2		0xdc
@@ -1946,11 +1945,30 @@ static struct iommu_ops arm_smmu_ops = {
 };
 
 /* Probing and initialisation functions */
+static int arm_smmu_reset_one_queue(struct arm_smmu_device *smmu,
+				    struct arm_smmu_queue *q,
+				    unsigned long qoffset,
+				    int page_to_use)
+{
+	unsigned long prod, cons;
+
+	writeq_relaxed(q->q_base, smmu->base + qoffset);
+	if (page_to_use == 1) {
+		prod = ARM_SMMU_Q_PROD_PAGE1(qoffset);
+		cons = ARM_SMMU_Q_CONS_PAGE1(qoffset);
+	} else {
+		prod = ARM_SMMU_Q_PROD(qoffset);
+		cons = ARM_SMMU_Q_CONS(qoffset);
+	}
+	writeq_relaxed(q->prod, smmu->base + prod);
+	writeq_relaxed(q->cons, smmu->base + cons);
+}
+
 static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
 				   struct arm_smmu_queue *q,
-				   unsigned long prod_off,
-				   unsigned long cons_off,
-				   size_t dwords)
+				   unsigned long qoffset,
+				   size_t dwords,
+				   int page_to_use)
 {
 	size_t qsz = ((1 << q->max_n_shift) * dwords) << 3;
 
@@ -1961,8 +1979,13 @@ static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
 		return -ENOMEM;
 	}
 
-	q->prod_reg	= smmu->base + prod_off;
-	q->cons_reg	= smmu->base + cons_off;
+	if (page_to_use == 1) {
+		q->prod_reg	= smmu->base + ARM_SMMU_Q_PROD_PAGE1(qoffset);
+		q->cons_reg	= smmu->base + ARM_SMMU_Q_CONS_PAGE1(qoffset);
+	} else {
+		q->prod_reg	= smmu->base + ARM_SMMU_Q_PROD(qoffset);
+		q->cons_reg	= smmu->base + ARM_SMMU_Q_CONS(qoffset);
+	}
 	q->ent_dwords	= dwords;
 
 	q->q_base  = Q_BASE_RWA;
@@ -1980,14 +2003,14 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
 
 	/* cmdq */
 	spin_lock_init(&smmu->cmdq.lock);
-	ret = arm_smmu_init_one_queue(smmu, &smmu->cmdq.q, ARM_SMMU_CMDQ_PROD,
-				      ARM_SMMU_CMDQ_CONS, CMDQ_ENT_DWORDS);
+	ret = arm_smmu_init_one_queue(smmu, &smmu->cmdq.q, ARM_SMMU_CMDQ_BASE,
+				      CMDQ_ENT_DWORDS, 0);
 	if (ret)
 		return ret;
 
 	/* evtq */
-	ret = arm_smmu_init_one_queue(smmu, &smmu->evtq.q, ARM_SMMU_EVTQ_PROD,
-				      ARM_SMMU_EVTQ_CONS, EVTQ_ENT_DWORDS);
+	ret = arm_smmu_init_one_queue(smmu, &smmu->evtq.q, ARM_SMMU_EVTQ_BASE,
+				      EVTQ_ENT_DWORDS, USE_PAGE1);
 	if (ret)
 		return ret;
 
@@ -1995,8 +2018,8 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
 	if (!(smmu->features & ARM_SMMU_FEAT_PRI))
 		return 0;
 
-	return arm_smmu_init_one_queue(smmu, &smmu->priq.q, ARM_SMMU_PRIQ_PROD,
-				       ARM_SMMU_PRIQ_CONS, PRIQ_ENT_DWORDS);
+	return arm_smmu_init_one_queue(smmu, &smmu->priq.q, ARM_SMMU_PRIQ_BASE,
+				       PRIQ_ENT_DWORDS, 1);
 }
 
 static int arm_smmu_init_l1_strtab(struct arm_smmu_device *smmu)
@@ -2332,9 +2355,8 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
 		       smmu->base + ARM_SMMU_STRTAB_BASE_CFG);
 
 	/* Command queue */
-	writeq_relaxed(smmu->cmdq.q.q_base, smmu->base + ARM_SMMU_CMDQ_BASE);
-	writel_relaxed(smmu->cmdq.q.prod, smmu->base + ARM_SMMU_CMDQ_PROD);
-	writel_relaxed(smmu->cmdq.q.cons, smmu->base + ARM_SMMU_CMDQ_CONS);
+	arm_smmu_reset_one_queue(smmu, &smmu->evtq.q,
+				 ARM_SMMU_CMDQ_BASE, 0);
 
 	enables = CR0_CMDQEN;
 	ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
@@ -2362,9 +2384,8 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
 	arm_smmu_cmdq_issue_cmd(smmu, &cmd);
 
 	/* Event queue */
-	writeq_relaxed(smmu->evtq.q.q_base, smmu->base + ARM_SMMU_EVTQ_BASE);
-	writel_relaxed(smmu->evtq.q.prod, smmu->base + ARM_SMMU_EVTQ_PROD);
-	writel_relaxed(smmu->evtq.q.cons, smmu->base + ARM_SMMU_EVTQ_CONS);
+	arm_smmu_reset_one_queue(smmu, &smmu->evtq.q,
+				 ARM_SMMU_EVTQ_BASE, 1);
 
 	enables |= CR0_EVTQEN;
 	ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
@@ -2376,13 +2397,8 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
 
 	/* PRI queue */
 	if (smmu->features & ARM_SMMU_FEAT_PRI) {
-		writeq_relaxed(smmu->priq.q.q_base,
-			       smmu->base + ARM_SMMU_PRIQ_BASE);
-		writel_relaxed(smmu->priq.q.prod,
-			       smmu->base + ARM_SMMU_PRIQ_PROD);
-		writel_relaxed(smmu->priq.q.cons,
-			       smmu->base + ARM_SMMU_PRIQ_CONS);
-
+		arm_smmu_reset_one_queue(smmu, &smmu->priq.q,
+					 ARM_SMMU_PRIQ_BASE, 1);
 		enables |= CR0_PRIQEN;
 		ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
 					      ARM_SMMU_CR0ACK);
-- 
2.7.4

Re: Fwd: [PATCH v7 2/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #74

[Robin Murphy]

On 09/06/17 12:38, Jayachandran C wrote:
> On Fri, Jun 09, 2017 Robin Murphy wrote:
>>
>> On 30/05/17 13:03, Geetha sowjanya wrote:
>>> From: Linu Cherian <linu.cherian@cavium.com>
>>>
>>> Cavium ThunderX2 SMMU implementation doesn't support page 1 register space
>>> and PAGE0_REGS_ONLY option is enabled as an errata workaround.
>>> This option when turned on, replaces all page 1 offsets used for
>>> EVTQ_PROD/CONS, PRIQ_PROD/CONS register access with page 0 offsets.
>>>
>>> SMMU resource size checks are now based on SMMU option PAGE0_REGS_ONLY,
>>> since resource size can be either 64k/128k.
>>> For this, arm_smmu_device_dt_probe/acpi_probe has been moved before
>>> platform_get_resource call, so that SMMU options are set beforehand.
>>>
>>> Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
>>> Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
>>> ---
>>>  Documentation/arm64/silicon-errata.txt             |    1 +
>>>  .../devicetree/bindings/iommu/arm,smmu-v3.txt      |    6 ++
>>>  drivers/iommu/arm-smmu-v3.c                        |   64 +++++++++++++++-----
>>>  3 files changed, 56 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
>>> index 10f2ddd..4693a32 100644
>>> --- a/Documentation/arm64/silicon-errata.txt
>>> +++ b/Documentation/arm64/silicon-errata.txt
>>> @@ -62,6 +62,7 @@ stable kernels.
>>>  | Cavium         | ThunderX GICv3  | #23154          | CAVIUM_ERRATUM_23154        |
>>>  | Cavium         | ThunderX Core   | #27456          | CAVIUM_ERRATUM_27456        |
>>>  | Cavium         | ThunderX SMMUv2 | #27704          | N/A                         |
>>> +| Cavium         | ThunderX2 SMMUv3| #74             | N/A                         |
>>>  |                |                 |                 |                             |
>>>  | Freescale/NXP  | LS2080A/LS1043A | A-008585        | FSL_ERRATUM_A008585         |
>>>  |                |                 |                 |                             |
>>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>>> index be57550..607e270 100644
>>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>>> @@ -49,6 +49,12 @@ the PCIe specification.
>>>  - hisilicon,broken-prefetch-cmd
>>>                      : Avoid sending CMD_PREFETCH_* commands to the SMMU.
>>>
>>> +- cavium,cn9900-broken-page1-regspace
>>> +                    : Replaces all page 1 offsets used for EVTQ_PROD/CONS,
>>> +                                             PRIQ_PROD/CONS register access with page 0 offsets.
>>> +                                             Set for Caviun ThunderX2 silicon that doesn't support
>>> +                                             SMMU page1 register space.
>>
>> The indentation's a bit funky here - the rest of this file is actually
>> indented with spaces, but either way it's clear your editor isn't set to
>> 8-space tabs ;)
>>
>>> +
>>>  ** Example
>>>
>>>          smmu@2b400000 {
>>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>>> index 380969a..4e80205 100644
>>> --- a/drivers/iommu/arm-smmu-v3.c
>>> +++ b/drivers/iommu/arm-smmu-v3.c
>>> @@ -412,6 +412,9 @@
>>>  #define MSI_IOVA_BASE                        0x8000000
>>>  #define MSI_IOVA_LENGTH                      0x100000
>>>
>>> +#define ARM_SMMU_PAGE0_REGS_ONLY(smmu)               \
>>> +     ((smmu)->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)
>>
>> At the two places we use this macro, frankly I think it would be clearer
>> to just reference smmu->options directly, as we currently do for
>> SKIP_PREFETCH. The abstraction also adds more lines than it saves...
>>
>>> +
>>>  static bool disable_bypass;
>>>  module_param_named(disable_bypass, disable_bypass, bool, S_IRUGO);
>>>  MODULE_PARM_DESC(disable_bypass,
>>> @@ -597,6 +600,7 @@ struct arm_smmu_device {
>>>       u32                             features;
>>>
>>>  #define ARM_SMMU_OPT_SKIP_PREFETCH   (1 << 0)
>>> +#define ARM_SMMU_OPT_PAGE0_REGS_ONLY    (1 << 1)
>>
>> Whitespace again, although this time it's spaces where there should be a
>> tab.
>>
>>>       u32                             options;
>>>
>>>       struct arm_smmu_cmdq            cmdq;
>>> @@ -663,9 +667,19 @@ struct arm_smmu_option_prop {
>>>
>>>  static struct arm_smmu_option_prop arm_smmu_options[] = {
>>>       { ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
>>> +     { ARM_SMMU_OPT_PAGE0_REGS_ONLY, "cavium,cn9900-broken-page1-regspace"},
>>>       { 0, NULL},
>>>  };
>>>
>>> +static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset,
>>> +                                              struct arm_smmu_device *smmu)
>>> +{
>>> +     if (offset > SZ_64K && ARM_SMMU_PAGE0_REGS_ONLY(smmu))
>>> +             offset -= SZ_64K;
>>> +
>>> +     return smmu->base + offset;
>>> +}
>>> +
>>>  static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
>>>  {
>>>       return container_of(dom, struct arm_smmu_domain, domain);
>>> @@ -1961,8 +1975,8 @@ static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
>>>               return -ENOMEM;
>>>       }
>>>
>>> -     q->prod_reg     = smmu->base + prod_off;
>>> -     q->cons_reg     = smmu->base + cons_off;
>>> +     q->prod_reg     = arm_smmu_page1_fixup(prod_off, smmu);
>>> +     q->cons_reg     = arm_smmu_page1_fixup(cons_off, smmu);
>>>       q->ent_dwords   = dwords;
>>>
>>>       q->q_base  = Q_BASE_RWA;
>>> @@ -2363,8 +2377,10 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
>>>
>>>       /* Event queue */
>>>       writeq_relaxed(smmu->evtq.q.q_base, smmu->base + ARM_SMMU_EVTQ_BASE);
>>> -     writel_relaxed(smmu->evtq.q.prod, smmu->base + ARM_SMMU_EVTQ_PROD);
>>> -     writel_relaxed(smmu->evtq.q.cons, smmu->base + ARM_SMMU_EVTQ_CONS);
>>> +     writel_relaxed(smmu->evtq.q.prod,
>>> +                    arm_smmu_page1_fixup(ARM_SMMU_EVTQ_PROD, smmu));
>>> +     writel_relaxed(smmu->evtq.q.cons,
>>> +                    arm_smmu_page1_fixup(ARM_SMMU_EVTQ_CONS, smmu));
> 
> This sequence and the arm_smmu_page1_fixup() call is repeated in quite a few
> places. I think this errata code is messy because the original driver does not
> make the alias page usage explicit.

It *is* explicit - the architecture says the event queue and PRI queue
pointers exist only on page 1, and that is the offset we define for
them. The architecture also says "The equivalent Page 0 offsets of
registers that are defined on Page 1 are Reserved and ARM recommends
that they are not accessed. Access to these offsets is CONSTRAINED
UNPREDICTABLE..."

This workaround is a bodge dependent on a specific implementation always
having a specific CONSTRAINED UNPREDICTABLE behaviour, and I see no
point in trying to dress it up as anything else. Yes, it could be
considered a little bit messy, but messy is what you get when you step
outside the spec. The fixup is invoked a grand total of 6 times, over 3
locations, and there's no way of factoring it out further that doesn't
just add significantly more code and complexity than it would save.

Robin.

> A patch like the one below (no functional changes) would clean up the original
> driver and make the errata change much simpler - any comments?
> 
> -- >8 --
> 
> Date: Tue, 30 May 2017 15:43:29 +0000
> Subject: [PATCH] iommu: arm-smmu-v3: make alias page usage explicit
> 
> ---
>  drivers/iommu/arm-smmu-v3.c | 76 +++++++++++++++++++++++++++------------------
>  1 file changed, 46 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 380969a..11fdb4f 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -171,20 +171,19 @@
>  #define STRTAB_BASE_CFG_FMT_LINEAR	(0 << STRTAB_BASE_CFG_FMT_SHIFT)
>  #define STRTAB_BASE_CFG_FMT_2LVL	(1 << STRTAB_BASE_CFG_FMT_SHIFT)
>  
> +#define ARM_SMMU_Q_PROD(qbase)		((qbase) + 0x8)
> +#define ARM_SMMU_Q_PROD_PAGE1(qbase)	((qbase) + 0x10008)
> +#define ARM_SMMU_Q_CONS(qbase)		((qbase) + 0xc)
> +#define ARM_SMMU_Q_CONS_PAGE1(qbase)	((qbase) + 0x1000c)
> +
>  #define ARM_SMMU_CMDQ_BASE		0x90
> -#define ARM_SMMU_CMDQ_PROD		0x98
> -#define ARM_SMMU_CMDQ_CONS		0x9c
>  
>  #define ARM_SMMU_EVTQ_BASE		0xa0
> -#define ARM_SMMU_EVTQ_PROD		0x100a8
> -#define ARM_SMMU_EVTQ_CONS		0x100ac
>  #define ARM_SMMU_EVTQ_IRQ_CFG0		0xb0
>  #define ARM_SMMU_EVTQ_IRQ_CFG1		0xb8
>  #define ARM_SMMU_EVTQ_IRQ_CFG2		0xbc
>  
>  #define ARM_SMMU_PRIQ_BASE		0xc0
> -#define ARM_SMMU_PRIQ_PROD		0x100c8
> -#define ARM_SMMU_PRIQ_CONS		0x100cc
>  #define ARM_SMMU_PRIQ_IRQ_CFG0		0xd0
>  #define ARM_SMMU_PRIQ_IRQ_CFG1		0xd8
>  #define ARM_SMMU_PRIQ_IRQ_CFG2		0xdc
> @@ -1946,11 +1945,30 @@ static struct iommu_ops arm_smmu_ops = {
>  };
>  
>  /* Probing and initialisation functions */
> +static int arm_smmu_reset_one_queue(struct arm_smmu_device *smmu,
> +				    struct arm_smmu_queue *q,
> +				    unsigned long qoffset,
> +				    int page_to_use)
> +{
> +	unsigned long prod, cons;
> +
> +	writeq_relaxed(q->q_base, smmu->base + qoffset);
> +	if (page_to_use == 1) {
> +		prod = ARM_SMMU_Q_PROD_PAGE1(qoffset);
> +		cons = ARM_SMMU_Q_CONS_PAGE1(qoffset);
> +	} else {
> +		prod = ARM_SMMU_Q_PROD(qoffset);
> +		cons = ARM_SMMU_Q_CONS(qoffset);
> +	}
> +	writeq_relaxed(q->prod, smmu->base + prod);
> +	writeq_relaxed(q->cons, smmu->base + cons);
> +}
> +
>  static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
>  				   struct arm_smmu_queue *q,
> -				   unsigned long prod_off,
> -				   unsigned long cons_off,
> -				   size_t dwords)
> +				   unsigned long qoffset,
> +				   size_t dwords,
> +				   int page_to_use)
>  {
>  	size_t qsz = ((1 << q->max_n_shift) * dwords) << 3;
>  
> @@ -1961,8 +1979,13 @@ static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
>  		return -ENOMEM;
>  	}
>  
> -	q->prod_reg	= smmu->base + prod_off;
> -	q->cons_reg	= smmu->base + cons_off;
> +	if (page_to_use == 1) {
> +		q->prod_reg	= smmu->base + ARM_SMMU_Q_PROD_PAGE1(qoffset);
> +		q->cons_reg	= smmu->base + ARM_SMMU_Q_CONS_PAGE1(qoffset);
> +	} else {
> +		q->prod_reg	= smmu->base + ARM_SMMU_Q_PROD(qoffset);
> +		q->cons_reg	= smmu->base + ARM_SMMU_Q_CONS(qoffset);
> +	}
>  	q->ent_dwords	= dwords;
>  
>  	q->q_base  = Q_BASE_RWA;
> @@ -1980,14 +2003,14 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
>  
>  	/* cmdq */
>  	spin_lock_init(&smmu->cmdq.lock);
> -	ret = arm_smmu_init_one_queue(smmu, &smmu->cmdq.q, ARM_SMMU_CMDQ_PROD,
> -				      ARM_SMMU_CMDQ_CONS, CMDQ_ENT_DWORDS);
> +	ret = arm_smmu_init_one_queue(smmu, &smmu->cmdq.q, ARM_SMMU_CMDQ_BASE,
> +				      CMDQ_ENT_DWORDS, 0);
>  	if (ret)
>  		return ret;
>  
>  	/* evtq */
> -	ret = arm_smmu_init_one_queue(smmu, &smmu->evtq.q, ARM_SMMU_EVTQ_PROD,
> -				      ARM_SMMU_EVTQ_CONS, EVTQ_ENT_DWORDS);
> +	ret = arm_smmu_init_one_queue(smmu, &smmu->evtq.q, ARM_SMMU_EVTQ_BASE,
> +				      EVTQ_ENT_DWORDS, USE_PAGE1);
>  	if (ret)
>  		return ret;
>  
> @@ -1995,8 +2018,8 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
>  	if (!(smmu->features & ARM_SMMU_FEAT_PRI))
>  		return 0;
>  
> -	return arm_smmu_init_one_queue(smmu, &smmu->priq.q, ARM_SMMU_PRIQ_PROD,
> -				       ARM_SMMU_PRIQ_CONS, PRIQ_ENT_DWORDS);
> +	return arm_smmu_init_one_queue(smmu, &smmu->priq.q, ARM_SMMU_PRIQ_BASE,
> +				       PRIQ_ENT_DWORDS, 1);
>  }
>  
>  static int arm_smmu_init_l1_strtab(struct arm_smmu_device *smmu)
> @@ -2332,9 +2355,8 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
>  		       smmu->base + ARM_SMMU_STRTAB_BASE_CFG);
>  
>  	/* Command queue */
> -	writeq_relaxed(smmu->cmdq.q.q_base, smmu->base + ARM_SMMU_CMDQ_BASE);
> -	writel_relaxed(smmu->cmdq.q.prod, smmu->base + ARM_SMMU_CMDQ_PROD);
> -	writel_relaxed(smmu->cmdq.q.cons, smmu->base + ARM_SMMU_CMDQ_CONS);
> +	arm_smmu_reset_one_queue(smmu, &smmu->evtq.q,
> +				 ARM_SMMU_CMDQ_BASE, 0);
>  
>  	enables = CR0_CMDQEN;
>  	ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
> @@ -2362,9 +2384,8 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
>  	arm_smmu_cmdq_issue_cmd(smmu, &cmd);
>  
>  	/* Event queue */
> -	writeq_relaxed(smmu->evtq.q.q_base, smmu->base + ARM_SMMU_EVTQ_BASE);
> -	writel_relaxed(smmu->evtq.q.prod, smmu->base + ARM_SMMU_EVTQ_PROD);
> -	writel_relaxed(smmu->evtq.q.cons, smmu->base + ARM_SMMU_EVTQ_CONS);
> +	arm_smmu_reset_one_queue(smmu, &smmu->evtq.q,
> +				 ARM_SMMU_EVTQ_BASE, 1);
>  
>  	enables |= CR0_EVTQEN;
>  	ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
> @@ -2376,13 +2397,8 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
>  
>  	/* PRI queue */
>  	if (smmu->features & ARM_SMMU_FEAT_PRI) {
> -		writeq_relaxed(smmu->priq.q.q_base,
> -			       smmu->base + ARM_SMMU_PRIQ_BASE);
> -		writel_relaxed(smmu->priq.q.prod,
> -			       smmu->base + ARM_SMMU_PRIQ_PROD);
> -		writel_relaxed(smmu->priq.q.cons,
> -			       smmu->base + ARM_SMMU_PRIQ_CONS);
> -
> +		arm_smmu_reset_one_queue(smmu, &smmu->priq.q,
> +					 ARM_SMMU_PRIQ_BASE, 1);
>  		enables |= CR0_PRIQEN;
>  		ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
>  					      ARM_SMMU_CR0ACK);
> 

Re: Fwd: [PATCH v7 2/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #74

[Jayachandran C]

On Fri, Jun 09, 2017 at 04:43:07PM +0100, Robin Murphy wrote:
> On 09/06/17 12:38, Jayachandran C wrote:
> > On Fri, Jun 09, 2017 Robin Murphy wrote:
> >>
> >> On 30/05/17 13:03, Geetha sowjanya wrote:
> >>> From: Linu Cherian <linu.cherian@cavium.com>
> >>>
> >>> Cavium ThunderX2 SMMU implementation doesn't support page 1 register space
> >>> and PAGE0_REGS_ONLY option is enabled as an errata workaround.
> >>> This option when turned on, replaces all page 1 offsets used for
> >>> EVTQ_PROD/CONS, PRIQ_PROD/CONS register access with page 0 offsets.
> >>>
> >>> SMMU resource size checks are now based on SMMU option PAGE0_REGS_ONLY,
> >>> since resource size can be either 64k/128k.
> >>> For this, arm_smmu_device_dt_probe/acpi_probe has been moved before
> >>> platform_get_resource call, so that SMMU options are set beforehand.
> >>>
> >>> Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
> >>> Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
> >>> ---
> >>>  Documentation/arm64/silicon-errata.txt             |    1 +
> >>>  .../devicetree/bindings/iommu/arm,smmu-v3.txt      |    6 ++
> >>>  drivers/iommu/arm-smmu-v3.c                        |   64 +++++++++++++++-----
> >>>  3 files changed, 56 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
> >>> index 10f2ddd..4693a32 100644
> >>> --- a/Documentation/arm64/silicon-errata.txt
> >>> +++ b/Documentation/arm64/silicon-errata.txt
> >>> @@ -62,6 +62,7 @@ stable kernels.
> >>>  | Cavium         | ThunderX GICv3  | #23154          | CAVIUM_ERRATUM_23154        |
> >>>  | Cavium         | ThunderX Core   | #27456          | CAVIUM_ERRATUM_27456        |
> >>>  | Cavium         | ThunderX SMMUv2 | #27704          | N/A                         |
> >>> +| Cavium         | ThunderX2 SMMUv3| #74             | N/A                         |
> >>>  |                |                 |                 |                             |
> >>>  | Freescale/NXP  | LS2080A/LS1043A | A-008585        | FSL_ERRATUM_A008585         |
> >>>  |                |                 |                 |                             |
> >>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> >>> index be57550..607e270 100644
> >>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> >>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> >>> @@ -49,6 +49,12 @@ the PCIe specification.
> >>>  - hisilicon,broken-prefetch-cmd
> >>>                      : Avoid sending CMD_PREFETCH_* commands to the SMMU.
> >>>
> >>> +- cavium,cn9900-broken-page1-regspace
> >>> +                    : Replaces all page 1 offsets used for EVTQ_PROD/CONS,
> >>> +                                             PRIQ_PROD/CONS register access with page 0 offsets.
> >>> +                                             Set for Caviun ThunderX2 silicon that doesn't support
> >>> +                                             SMMU page1 register space.
> >>
> >> The indentation's a bit funky here - the rest of this file is actually
> >> indented with spaces, but either way it's clear your editor isn't set to
> >> 8-space tabs ;)
> >>
> >>> +
> >>>  ** Example
> >>>
> >>>          smmu@2b400000 {
> >>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> >>> index 380969a..4e80205 100644
> >>> --- a/drivers/iommu/arm-smmu-v3.c
> >>> +++ b/drivers/iommu/arm-smmu-v3.c
> >>> @@ -412,6 +412,9 @@
> >>>  #define MSI_IOVA_BASE                        0x8000000
> >>>  #define MSI_IOVA_LENGTH                      0x100000
> >>>
> >>> +#define ARM_SMMU_PAGE0_REGS_ONLY(smmu)               \
> >>> +     ((smmu)->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)
> >>
> >> At the two places we use this macro, frankly I think it would be clearer
> >> to just reference smmu->options directly, as we currently do for
> >> SKIP_PREFETCH. The abstraction also adds more lines than it saves...
> >>
> >>> +
> >>>  static bool disable_bypass;
> >>>  module_param_named(disable_bypass, disable_bypass, bool, S_IRUGO);
> >>>  MODULE_PARM_DESC(disable_bypass,
> >>> @@ -597,6 +600,7 @@ struct arm_smmu_device {
> >>>       u32                             features;
> >>>
> >>>  #define ARM_SMMU_OPT_SKIP_PREFETCH   (1 << 0)
> >>> +#define ARM_SMMU_OPT_PAGE0_REGS_ONLY    (1 << 1)
> >>
> >> Whitespace again, although this time it's spaces where there should be a
> >> tab.
> >>
> >>>       u32                             options;
> >>>
> >>>       struct arm_smmu_cmdq            cmdq;
> >>> @@ -663,9 +667,19 @@ struct arm_smmu_option_prop {
> >>>
> >>>  static struct arm_smmu_option_prop arm_smmu_options[] = {
> >>>       { ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
> >>> +     { ARM_SMMU_OPT_PAGE0_REGS_ONLY, "cavium,cn9900-broken-page1-regspace"},
> >>>       { 0, NULL},
> >>>  };
> >>>
> >>> +static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset,
> >>> +                                              struct arm_smmu_device *smmu)
> >>> +{
> >>> +     if (offset > SZ_64K && ARM_SMMU_PAGE0_REGS_ONLY(smmu))
> >>> +             offset -= SZ_64K;
> >>> +
> >>> +     return smmu->base + offset;
> >>> +}
> >>> +
> >>>  static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
> >>>  {
> >>>       return container_of(dom, struct arm_smmu_domain, domain);
> >>> @@ -1961,8 +1975,8 @@ static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
> >>>               return -ENOMEM;
> >>>       }
> >>>
> >>> -     q->prod_reg     = smmu->base + prod_off;
> >>> -     q->cons_reg     = smmu->base + cons_off;
> >>> +     q->prod_reg     = arm_smmu_page1_fixup(prod_off, smmu);
> >>> +     q->cons_reg     = arm_smmu_page1_fixup(cons_off, smmu);
> >>>       q->ent_dwords   = dwords;
> >>>
> >>>       q->q_base  = Q_BASE_RWA;
> >>> @@ -2363,8 +2377,10 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
> >>>
> >>>       /* Event queue */
> >>>       writeq_relaxed(smmu->evtq.q.q_base, smmu->base + ARM_SMMU_EVTQ_BASE);
> >>> -     writel_relaxed(smmu->evtq.q.prod, smmu->base + ARM_SMMU_EVTQ_PROD);
> >>> -     writel_relaxed(smmu->evtq.q.cons, smmu->base + ARM_SMMU_EVTQ_CONS);
> >>> +     writel_relaxed(smmu->evtq.q.prod,
> >>> +                    arm_smmu_page1_fixup(ARM_SMMU_EVTQ_PROD, smmu));
> >>> +     writel_relaxed(smmu->evtq.q.cons,
> >>> +                    arm_smmu_page1_fixup(ARM_SMMU_EVTQ_CONS, smmu));
> > 
> > This sequence and the arm_smmu_page1_fixup() call is repeated in quite a few
> > places. I think this errata code is messy because the original driver does not
> > make the alias page usage explicit.
> 
> It *is* explicit - the architecture says the event queue and PRI queue
> pointers exist only on page 1, and that is the offset we define for
> them. The architecture also says "The equivalent Page 0 offsets of
> registers that are defined on Page 1 are Reserved and ARM recommends
> that they are not accessed. Access to these offsets is CONSTRAINED
> UNPREDICTABLE..."
 
Ok. The patch makes the page used for producer/consumer queue registers
explicit fo cmdq (page 0) and the eventq/priq(page1). There is no suggestion
here to use page 0 address for eventq/priq.

> This workaround is a bodge dependent on a specific implementation always
> having a specific CONSTRAINED UNPREDICTABLE behaviour, and I see no
> point in trying to dress it up as anything else. Yes, it could be
> considered a little bit messy, but messy is what you get when you step
> outside the spec. The fixup is invoked a grand total of 6 times, over 3
> locations, and there's no way of factoring it out further that doesn't
> just add significantly more code and complexity than it would save.
 
With the changes below, the fixup are simpler and will be needed only in
2 places - and it gets the hardware init repeated 3 times in the driver
into a single place. That bikeshed will look real nice :)

JC.

> 
> > A patch like the one below (no functional changes) would clean up the original
> > driver and make the errata change much simpler - any comments?
> > 
> > -- >8 --
> > 
> > Date: Tue, 30 May 2017 15:43:29 +0000
> > Subject: [PATCH] iommu: arm-smmu-v3: make alias page usage explicit
> > 
> > ---
> >  drivers/iommu/arm-smmu-v3.c | 76 +++++++++++++++++++++++++++------------------
> >  1 file changed, 46 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > index 380969a..11fdb4f 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -171,20 +171,19 @@
> >  #define STRTAB_BASE_CFG_FMT_LINEAR	(0 << STRTAB_BASE_CFG_FMT_SHIFT)
> >  #define STRTAB_BASE_CFG_FMT_2LVL	(1 << STRTAB_BASE_CFG_FMT_SHIFT)
> >  
> > +#define ARM_SMMU_Q_PROD(qbase)		((qbase) + 0x8)
> > +#define ARM_SMMU_Q_PROD_PAGE1(qbase)	((qbase) + 0x10008)
> > +#define ARM_SMMU_Q_CONS(qbase)		((qbase) + 0xc)
> > +#define ARM_SMMU_Q_CONS_PAGE1(qbase)	((qbase) + 0x1000c)
> > +
> >  #define ARM_SMMU_CMDQ_BASE		0x90
> > -#define ARM_SMMU_CMDQ_PROD		0x98
> > -#define ARM_SMMU_CMDQ_CONS		0x9c
> >  
> >  #define ARM_SMMU_EVTQ_BASE		0xa0
> > -#define ARM_SMMU_EVTQ_PROD		0x100a8
> > -#define ARM_SMMU_EVTQ_CONS		0x100ac
> >  #define ARM_SMMU_EVTQ_IRQ_CFG0		0xb0
> >  #define ARM_SMMU_EVTQ_IRQ_CFG1		0xb8
> >  #define ARM_SMMU_EVTQ_IRQ_CFG2		0xbc
> >  
> >  #define ARM_SMMU_PRIQ_BASE		0xc0
> > -#define ARM_SMMU_PRIQ_PROD		0x100c8
> > -#define ARM_SMMU_PRIQ_CONS		0x100cc
> >  #define ARM_SMMU_PRIQ_IRQ_CFG0		0xd0
> >  #define ARM_SMMU_PRIQ_IRQ_CFG1		0xd8
> >  #define ARM_SMMU_PRIQ_IRQ_CFG2		0xdc
> > @@ -1946,11 +1945,30 @@ static struct iommu_ops arm_smmu_ops = {
> >  };
> >  
> >  /* Probing and initialisation functions */
> > +static int arm_smmu_reset_one_queue(struct arm_smmu_device *smmu,
> > +				    struct arm_smmu_queue *q,
> > +				    unsigned long qoffset,
> > +				    int page_to_use)
> > +{
> > +	unsigned long prod, cons;
> > +
> > +	writeq_relaxed(q->q_base, smmu->base + qoffset);
> > +	if (page_to_use == 1) {
> > +		prod = ARM_SMMU_Q_PROD_PAGE1(qoffset);
> > +		cons = ARM_SMMU_Q_CONS_PAGE1(qoffset);
> > +	} else {
> > +		prod = ARM_SMMU_Q_PROD(qoffset);
> > +		cons = ARM_SMMU_Q_CONS(qoffset);
> > +	}
> > +	writeq_relaxed(q->prod, smmu->base + prod);
> > +	writeq_relaxed(q->cons, smmu->base + cons);
> > +}
> > +
> >  static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
> >  				   struct arm_smmu_queue *q,
> > -				   unsigned long prod_off,
> > -				   unsigned long cons_off,
> > -				   size_t dwords)
> > +				   unsigned long qoffset,
> > +				   size_t dwords,
> > +				   int page_to_use)
> >  {
> >  	size_t qsz = ((1 << q->max_n_shift) * dwords) << 3;
> >  
> > @@ -1961,8 +1979,13 @@ static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
> >  		return -ENOMEM;
> >  	}
> >  
> > -	q->prod_reg	= smmu->base + prod_off;
> > -	q->cons_reg	= smmu->base + cons_off;
> > +	if (page_to_use == 1) {
> > +		q->prod_reg	= smmu->base + ARM_SMMU_Q_PROD_PAGE1(qoffset);
> > +		q->cons_reg	= smmu->base + ARM_SMMU_Q_CONS_PAGE1(qoffset);
> > +	} else {
> > +		q->prod_reg	= smmu->base + ARM_SMMU_Q_PROD(qoffset);
> > +		q->cons_reg	= smmu->base + ARM_SMMU_Q_CONS(qoffset);
> > +	}
> >  	q->ent_dwords	= dwords;
> >  
> >  	q->q_base  = Q_BASE_RWA;
> > @@ -1980,14 +2003,14 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
> >  
> >  	/* cmdq */
> >  	spin_lock_init(&smmu->cmdq.lock);
> > -	ret = arm_smmu_init_one_queue(smmu, &smmu->cmdq.q, ARM_SMMU_CMDQ_PROD,
> > -				      ARM_SMMU_CMDQ_CONS, CMDQ_ENT_DWORDS);
> > +	ret = arm_smmu_init_one_queue(smmu, &smmu->cmdq.q, ARM_SMMU_CMDQ_BASE,
> > +				      CMDQ_ENT_DWORDS, 0);
> >  	if (ret)
> >  		return ret;
> >  
> >  	/* evtq */
> > -	ret = arm_smmu_init_one_queue(smmu, &smmu->evtq.q, ARM_SMMU_EVTQ_PROD,
> > -				      ARM_SMMU_EVTQ_CONS, EVTQ_ENT_DWORDS);
> > +	ret = arm_smmu_init_one_queue(smmu, &smmu->evtq.q, ARM_SMMU_EVTQ_BASE,
> > +				      EVTQ_ENT_DWORDS, USE_PAGE1);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -1995,8 +2018,8 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
> >  	if (!(smmu->features & ARM_SMMU_FEAT_PRI))
> >  		return 0;
> >  
> > -	return arm_smmu_init_one_queue(smmu, &smmu->priq.q, ARM_SMMU_PRIQ_PROD,
> > -				       ARM_SMMU_PRIQ_CONS, PRIQ_ENT_DWORDS);
> > +	return arm_smmu_init_one_queue(smmu, &smmu->priq.q, ARM_SMMU_PRIQ_BASE,
> > +				       PRIQ_ENT_DWORDS, 1);
> >  }
> >  
> >  static int arm_smmu_init_l1_strtab(struct arm_smmu_device *smmu)
> > @@ -2332,9 +2355,8 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
> >  		       smmu->base + ARM_SMMU_STRTAB_BASE_CFG);
> >  
> >  	/* Command queue */
> > -	writeq_relaxed(smmu->cmdq.q.q_base, smmu->base + ARM_SMMU_CMDQ_BASE);
> > -	writel_relaxed(smmu->cmdq.q.prod, smmu->base + ARM_SMMU_CMDQ_PROD);
> > -	writel_relaxed(smmu->cmdq.q.cons, smmu->base + ARM_SMMU_CMDQ_CONS);
> > +	arm_smmu_reset_one_queue(smmu, &smmu->evtq.q,
> > +				 ARM_SMMU_CMDQ_BASE, 0);
> >  
> >  	enables = CR0_CMDQEN;
> >  	ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
> > @@ -2362,9 +2384,8 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
> >  	arm_smmu_cmdq_issue_cmd(smmu, &cmd);
> >  
> >  	/* Event queue */
> > -	writeq_relaxed(smmu->evtq.q.q_base, smmu->base + ARM_SMMU_EVTQ_BASE);
> > -	writel_relaxed(smmu->evtq.q.prod, smmu->base + ARM_SMMU_EVTQ_PROD);
> > -	writel_relaxed(smmu->evtq.q.cons, smmu->base + ARM_SMMU_EVTQ_CONS);
> > +	arm_smmu_reset_one_queue(smmu, &smmu->evtq.q,
> > +				 ARM_SMMU_EVTQ_BASE, 1);
> >  
> >  	enables |= CR0_EVTQEN;
> >  	ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
> > @@ -2376,13 +2397,8 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
> >  
> >  	/* PRI queue */
> >  	if (smmu->features & ARM_SMMU_FEAT_PRI) {
> > -		writeq_relaxed(smmu->priq.q.q_base,
> > -			       smmu->base + ARM_SMMU_PRIQ_BASE);
> > -		writel_relaxed(smmu->priq.q.prod,
> > -			       smmu->base + ARM_SMMU_PRIQ_PROD);
> > -		writel_relaxed(smmu->priq.q.cons,
> > -			       smmu->base + ARM_SMMU_PRIQ_CONS);
> > -
> > +		arm_smmu_reset_one_queue(smmu, &smmu->priq.q,
> > +					 ARM_SMMU_PRIQ_BASE, 1);
> >  		enables |= CR0_PRIQEN;
> >  		ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
> >  					      ARM_SMMU_CR0ACK);
> > 
> 

[PATCH v7 3/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126

[Geetha sowjanya]

From: Geetha Sowjanya <geethasowjanya.akula@cavium.com>

Cavium ThunderX2 SMMU doesn't support MSI and also doesn't have unique irq
lines for gerror, eventq and cmdq-sync.

This patch addresses the issue by checking if any interrupt sources are
using same irq number, then they are registered as shared irqs.

Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
---
 Documentation/arm64/silicon-errata.txt |    1 +
 drivers/iommu/arm-smmu-v3.c            |   29 +++++++++++++++++++++++++----
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
index 4693a32..42422f6 100644
--- a/Documentation/arm64/silicon-errata.txt
+++ b/Documentation/arm64/silicon-errata.txt
@@ -63,6 +63,7 @@ stable kernels.
 | Cavium         | ThunderX Core   | #27456          | CAVIUM_ERRATUM_27456        |
 | Cavium         | ThunderX SMMUv2 | #27704          | N/A                         |
 | Cavium         | ThunderX2 SMMUv3| #74             | N/A                         |
+| Cavium         | ThunderX2 SMMUv3| #126            | N/A                         |
 |                |                 |                 |                             |
 | Freescale/NXP  | LS2080A/LS1043A | A-008585        | FSL_ERRATUM_A008585         |
 |                |                 |                 |                             |
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 4e80205..d2db01f 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2232,6 +2232,25 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
 	devm_add_action(dev, arm_smmu_free_msis, dev);
 }
 
+static int get_irq_flags(struct arm_smmu_device *smmu, int irq)
+{
+	int match_count = 0;
+
+	if (irq == smmu->evtq.q.irq)
+		match_count++;
+	if (irq == smmu->cmdq.q.irq)
+		match_count++;
+	if (irq == smmu->gerr_irq)
+		match_count++;
+	if (irq == smmu->priq.q.irq)
+		match_count++;
+
+	if (match_count > 1)
+		return IRQF_SHARED | IRQF_ONESHOT;
+
+	return IRQF_ONESHOT;
+}
+
 static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 {
 	int ret, irq;
@@ -2252,7 +2271,7 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 	if (irq) {
 		ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
 						arm_smmu_evtq_thread,
-						IRQF_ONESHOT,
+						get_irq_flags(smmu, irq),
 						"arm-smmu-v3-evtq", smmu);
 		if (ret < 0)
 			dev_warn(smmu->dev, "failed to enable evtq irq\n");
@@ -2261,7 +2280,8 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 	irq = smmu->cmdq.q.irq;
 	if (irq) {
 		ret = devm_request_irq(smmu->dev, irq,
-				       arm_smmu_cmdq_sync_handler, 0,
+				       arm_smmu_cmdq_sync_handler,
+					   get_irq_flags(smmu, irq),
 				       "arm-smmu-v3-cmdq-sync", smmu);
 		if (ret < 0)
 			dev_warn(smmu->dev, "failed to enable cmdq-sync irq\n");
@@ -2270,7 +2290,8 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 	irq = smmu->gerr_irq;
 	if (irq) {
 		ret = devm_request_irq(smmu->dev, irq, arm_smmu_gerror_handler,
-				       0, "arm-smmu-v3-gerror", smmu);
+						get_irq_flags(smmu, irq),
+						"arm-smmu-v3-gerror", smmu);
 		if (ret < 0)
 			dev_warn(smmu->dev, "failed to enable gerror irq\n");
 	}
@@ -2280,7 +2301,7 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 		if (irq) {
 			ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
 							arm_smmu_priq_thread,
-							IRQF_ONESHOT,
+							get_irq_flags(smmu, irq),
 							"arm-smmu-v3-priq",
 							smmu);
 			if (ret < 0)
-- 
1.7.1

Re: [PATCH v7 3/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126

[John Garry]

On 30/05/2017 13:03, Geetha sowjanya wrote:
> From: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
>
> Cavium ThunderX2 SMMU doesn't support MSI and also doesn't have unique irq

separate irq lines

> lines for gerror, eventq and cmdq-sync.
>
> This patch addresses the issue by checking if any interrupt sources are
> using same irq number, then they are registered as shared irqs.
>
> Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
> ---
>  Documentation/arm64/silicon-errata.txt |    1 +
>  drivers/iommu/arm-smmu-v3.c            |   29 +++++++++++++++++++++++++----
>  2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
> index 4693a32..42422f6 100644
> --- a/Documentation/arm64/silicon-errata.txt
> +++ b/Documentation/arm64/silicon-errata.txt
> @@ -63,6 +63,7 @@ stable kernels.
>  | Cavium         | ThunderX Core   | #27456          | CAVIUM_ERRATUM_27456        |
>  | Cavium         | ThunderX SMMUv2 | #27704          | N/A                         |
>  | Cavium         | ThunderX2 SMMUv3| #74             | N/A                         |
> +| Cavium         | ThunderX2 SMMUv3| #126            | N/A                         |
>  |                |                 |                 |                             |
>  | Freescale/NXP  | LS2080A/LS1043A | A-008585        | FSL_ERRATUM_A008585         |
>  |                |                 |                 |                             |
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 4e80205..d2db01f 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2232,6 +2232,25 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>  	devm_add_action(dev, arm_smmu_free_msis, dev);
>  }
>
> +static int get_irq_flags(struct arm_smmu_device *smmu, int irq)
> +{
> +	int match_count = 0;
> +
> +	if (irq == smmu->evtq.q.irq)
> +		match_count++;
> +	if (irq == smmu->cmdq.q.irq)
> +		match_count++;
> +	if (irq == smmu->gerr_irq)
> +		match_count++;
> +	if (irq == smmu->priq.q.irq)
> +		match_count++;
> +
> +	if (match_count > 1)
> +		return IRQF_SHARED | IRQF_ONESHOT;
> +
> +	return IRQF_ONESHOT;
> +}
> +
>  static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>  {
>  	int ret, irq;
> @@ -2252,7 +2271,7 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>  	if (irq) {
>  		ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
>  						arm_smmu_evtq_thread,
> -						IRQF_ONESHOT,
> +						get_irq_flags(smmu, irq),
>  						"arm-smmu-v3-evtq", smmu);
>  		if (ret < 0)
>  			dev_warn(smmu->dev, "failed to enable evtq irq\n");
> @@ -2261,7 +2280,8 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>  	irq = smmu->cmdq.q.irq;
>  	if (irq) {
>  		ret = devm_request_irq(smmu->dev, irq,
> -				       arm_smmu_cmdq_sync_handler, 0,

If it is indeed ok to change this to having IRQF_ONESHOT set now 
(right?), then can you add a note to the commit message?

> +				       arm_smmu_cmdq_sync_handler,
> +					   get_irq_flags(smmu, irq),

indentation looks inconsistent

>  				       "arm-smmu-v3-cmdq-sync", smmu);
>  		if (ret < 0)
>  			dev_warn(smmu->dev, "failed to enable cmdq-sync irq\n");
> @@ -2270,7 +2290,8 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>  	irq = smmu->gerr_irq;
>  	if (irq) {
>  		ret = devm_request_irq(smmu->dev, irq, arm_smmu_gerror_handler,
> -				       0, "arm-smmu-v3-gerror", smmu);
> +						get_irq_flags(smmu, irq),
> +						"arm-smmu-v3-gerror", smmu);
>  		if (ret < 0)
>  			dev_warn(smmu->dev, "failed to enable gerror irq\n");
>  	}
> @@ -2280,7 +2301,7 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>  		if (irq) {
>  			ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
>  							arm_smmu_priq_thread,
> -							IRQF_ONESHOT,
> +							get_irq_flags(smmu, irq),
>  							"arm-smmu-v3-priq",
>  							smmu);
>  			if (ret < 0)
>

Re: [PATCH v7 3/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126

[Will Deacon]

Hi Geetha,

On Tue, May 30, 2017 at 05:33:41PM +0530, Geetha sowjanya wrote:
> From: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
> 
> Cavium ThunderX2 SMMU doesn't support MSI and also doesn't have unique irq
> lines for gerror, eventq and cmdq-sync.
> 
> This patch addresses the issue by checking if any interrupt sources are
> using same irq number, then they are registered as shared irqs.
> 
> Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
> ---
>  Documentation/arm64/silicon-errata.txt |    1 +
>  drivers/iommu/arm-smmu-v3.c            |   29 +++++++++++++++++++++++++----
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
> index 4693a32..42422f6 100644
> --- a/Documentation/arm64/silicon-errata.txt
> +++ b/Documentation/arm64/silicon-errata.txt
> @@ -63,6 +63,7 @@ stable kernels.
>  | Cavium         | ThunderX Core   | #27456          | CAVIUM_ERRATUM_27456        |
>  | Cavium         | ThunderX SMMUv2 | #27704          | N/A                         |
>  | Cavium         | ThunderX2 SMMUv3| #74             | N/A                         |
> +| Cavium         | ThunderX2 SMMUv3| #126            | N/A                         |
>  |                |                 |                 |                             |
>  | Freescale/NXP  | LS2080A/LS1043A | A-008585        | FSL_ERRATUM_A008585         |
>  |                |                 |                 |                             |
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 4e80205..d2db01f 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2232,6 +2232,25 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>  	devm_add_action(dev, arm_smmu_free_msis, dev);
>  }
>  
> +static int get_irq_flags(struct arm_smmu_device *smmu, int irq)
> +{
> +	int match_count = 0;
> +
> +	if (irq == smmu->evtq.q.irq)
> +		match_count++;
> +	if (irq == smmu->cmdq.q.irq)
> +		match_count++;
> +	if (irq == smmu->gerr_irq)
> +		match_count++;
> +	if (irq == smmu->priq.q.irq)
> +		match_count++;
> +
> +	if (match_count > 1)
> +		return IRQF_SHARED | IRQF_ONESHOT;
> +
> +	return IRQF_ONESHOT;
> +}

I really think this is the wrong way of solving the problem: using
IRQF_SHARED has implications elsewhere in the driver (for example, we must
then pass a unique dev_id otherwise freeing the IRQs won't work properly)
and I don't want to have to worry about these constraints just because of
this broken platform.

Please do what I suggested instead: register a single threaded interrupt
handler that acts as a multiplexer and manually calls the other routines.

Will

Re: [PATCH v7 0/3] Cavium ThunderX2 SMMUv3 errata workarounds

[Robert Richter]

On 30.05.17 17:33:38, Geetha sowjanya wrote:
> Cavium ThunderX2 SMMUv3 implementation has two Silicon Erratas.
> 1. Errata ID #74
>    SMMU register alias Page 1 is not implemented
> 2. Errata ID #126
>    SMMU doesnt support unique IRQ lines and also MSI for gerror,
>    eventq and cmdq-sync
> 
> The following patchset does software workaround for these two erratas.
> 
> This series is based on patchset.
> https://www.spinics.net/lists/arm-kernel/msg578443.html
> 
> Changes since v6:
>    - Changed device tree compatible string to vendor specific.
>    - Rebased on Robin's latest "Update SMMU models for IORT rev. C" v2 patch.
>      https://www.spinics.net/lists/arm-kernel/msg582809.html
> 
> Changes since v5:
>   - Rebased on Robin's "Update SMMU models for IORT rev. C" patch.
>      https://www.spinics.net/lists/arm-kernel/msg580728.html
>   - Replaced ACPI_IORT_SMMU_V3_CAVIUM_CN99XX macro with ACPI_IORT_SMMU_CAVIUM_CN99XX
> 
> Changes since v4:
>  - Replaced all page1 offset macros ARM_SMMU_EVTQ/PRIQ_PROD/CONS with
>     arm_smmu_page1_fixup(ARM_SMMU_EVTQ/PRIQ_PROD/CONS, smmu)
> 
> Changes since v3:
>  - Merged patches 1, 2 and 4 of Version 3.
>  - Modified the page1_offset_adjust() and get_irq_flags() implementation as
>    suggested by Robin.
> 
> Changes since v2:
>  - Updated "Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt" document with
>    new SMMU option used to enable errata workaround.
> 
> Changes since v1:
>  - Since the use of MIDR register is rejected and SMMU_IIDR is broken on this
>    silicon, as suggested by Will Deacon modified the patches to use ThunderX2
>    SMMUv3 IORT model number to enable errata workaround.
> 
> Geetha Sowjanya (1):
>   iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126
> 
> Linu Cherian (2):
>   ACPI/IORT: Fixup SMMUv3 resource size for Cavium ThunderX2 SMMUv3
>     model
>   iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2      erratum
>     #74
> 
>  Documentation/arm64/silicon-errata.txt             |    2 +
>  .../devicetree/bindings/iommu/arm,smmu-v3.txt      |    6 ++
>  drivers/acpi/arm64/iort.c                          |   10 ++-
>  drivers/iommu/arm-smmu-v3.c                        |   93 ++++++++++++++++----
>  4 files changed, 91 insertions(+), 20 deletions(-)

For the whole series:

Reviewed-by: Robert Richter <rrichter@cavium.com>

Re: [PATCH v7 0/3] Cavium ThunderX2 SMMUv3 errata workarounds

[Lorenzo Pieralisi]

On Tue, May 30, 2017 at 05:33:38PM +0530, Geetha sowjanya wrote:
> Cavium ThunderX2 SMMUv3 implementation has two Silicon Erratas.
> 1. Errata ID #74
>    SMMU register alias Page 1 is not implemented
> 2. Errata ID #126
>    SMMU doesnt support unique IRQ lines and also MSI for gerror,
>    eventq and cmdq-sync
> 
> The following patchset does software workaround for these two erratas.
> 
> This series is based on patchset.
> https://www.spinics.net/lists/arm-kernel/msg578443.html

Yes so it is not standalone. How are we going to merge these
ACPI IORT/ACPICA/SMMU patches - inclusive of:

[1] https://www.spinics.net/lists/arm-kernel/msg586458.html

Rafael, do ACPICA patches go upstream via the ACPI tree pull request ?

To remove dependency on ACPICA changes this series needs updating
anyway and for [1] above I think the only solution is for all the
patches to go via the ACPI tree (if ACPICA updates go upstream with it).

Thanks,
Lorenzo

> Changes since v6:
>    - Changed device tree compatible string to vendor specific.
>    - Rebased on Robin's latest "Update SMMU models for IORT rev. C" v2 patch.
>      https://www.spinics.net/lists/arm-kernel/msg582809.html
> 
> Changes since v5:
>   - Rebased on Robin's "Update SMMU models for IORT rev. C" patch.
>      https://www.spinics.net/lists/arm-kernel/msg580728.html
>   - Replaced ACPI_IORT_SMMU_V3_CAVIUM_CN99XX macro with ACPI_IORT_SMMU_CAVIUM_CN99XX
> 
> Changes since v4:
>  - Replaced all page1 offset macros ARM_SMMU_EVTQ/PRIQ_PROD/CONS with
>     arm_smmu_page1_fixup(ARM_SMMU_EVTQ/PRIQ_PROD/CONS, smmu)
> 
> Changes since v3:
>  - Merged patches 1, 2 and 4 of Version 3.
>  - Modified the page1_offset_adjust() and get_irq_flags() implementation as
>    suggested by Robin.
> 
> Changes since v2:
>  - Updated "Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt" document with
>    new SMMU option used to enable errata workaround.
> 
> Changes since v1:
>  - Since the use of MIDR register is rejected and SMMU_IIDR is broken on this
>    silicon, as suggested by Will Deacon modified the patches to use ThunderX2
>    SMMUv3 IORT model number to enable errata workaround.
> 
> Geetha Sowjanya (1):
>   iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126
> 
> Linu Cherian (2):
>   ACPI/IORT: Fixup SMMUv3 resource size for Cavium ThunderX2 SMMUv3
>     model
>   iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2      erratum
>     #74
> 
>  Documentation/arm64/silicon-errata.txt             |    2 +
>  .../devicetree/bindings/iommu/arm,smmu-v3.txt      |    6 ++
>  drivers/acpi/arm64/iort.c                          |   10 ++-
>  drivers/iommu/arm-smmu-v3.c                        |   93 ++++++++++++++++----
>  4 files changed, 91 insertions(+), 20 deletions(-)
> 

Re: [PATCH v7 0/3] Cavium ThunderX2 SMMUv3 errata workarounds

[Rafael J. Wysocki]

On Thu, Jun 8, 2017 at 6:32 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Tue, May 30, 2017 at 05:33:38PM +0530, Geetha sowjanya wrote:
>> Cavium ThunderX2 SMMUv3 implementation has two Silicon Erratas.
>> 1. Errata ID #74
>>    SMMU register alias Page 1 is not implemented
>> 2. Errata ID #126
>>    SMMU doesnt support unique IRQ lines and also MSI for gerror,
>>    eventq and cmdq-sync
>>
>> The following patchset does software workaround for these two erratas.
>>
>> This series is based on patchset.
>> https://www.spinics.net/lists/arm-kernel/msg578443.html
>
> Yes so it is not standalone. How are we going to merge these
> ACPI IORT/ACPICA/SMMU patches - inclusive of:
>
> [1] https://www.spinics.net/lists/arm-kernel/msg586458.html
>
> Rafael, do ACPICA patches go upstream via the ACPI tree pull request ?

Not as a rule.

> To remove dependency on ACPICA changes this series needs updating
> anyway and for [1] above I think the only solution is for all the
> patches to go via the ACPI tree (if ACPICA updates go upstream with it).

I think we may ask Lv to backport the header changes once they have
been merged into Linux.

Lv, would that work?

Thanks,
Rafael

Re: [PATCH v7 0/3] Cavium ThunderX2 SMMUv3 errata workarounds

[Robin Murphy]

On 08/06/17 18:13, Rafael J. Wysocki wrote:
> On Thu, Jun 8, 2017 at 6:32 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
>> On Tue, May 30, 2017 at 05:33:38PM +0530, Geetha sowjanya wrote:
>>> Cavium ThunderX2 SMMUv3 implementation has two Silicon Erratas.
>>> 1. Errata ID #74
>>>    SMMU register alias Page 1 is not implemented
>>> 2. Errata ID #126
>>>    SMMU doesnt support unique IRQ lines and also MSI for gerror,
>>>    eventq and cmdq-sync
>>>
>>> The following patchset does software workaround for these two erratas.
>>>
>>> This series is based on patchset.
>>> https://www.spinics.net/lists/arm-kernel/msg578443.html
>>
>> Yes so it is not standalone. How are we going to merge these
>> ACPI IORT/ACPICA/SMMU patches - inclusive of:
>>
>> [1] https://www.spinics.net/lists/arm-kernel/msg586458.html
>>
>> Rafael, do ACPICA patches go upstream via the ACPI tree pull request ?
> 
> Not as a rule.
> 
>> To remove dependency on ACPICA changes this series needs updating
>> anyway and for [1] above I think the only solution is for all the
>> patches to go via the ACPI tree (if ACPICA updates go upstream with it).
> 
> I think we may ask Lv to backport the header changes once they have
> been merged into Linux.
> 
> Lv, would that work?

FWIW, I have already sent a PR for the header patch for the new model
IDs to ACPICA upstream. I briefly considered the actual table update as
well, but didn't find time to comprehend the code changes that appeared
to be necessary for that.

Robin.

> 
> Thanks,
> Rafael
> 

Re: [PATCH v7 0/3] Cavium ThunderX2 SMMUv3 errata workarounds

[Lorenzo Pieralisi]

Hi Rafael, Lv,

On Thu, Jun 08, 2017 at 07:13:24PM +0200, Rafael J. Wysocki wrote:
> On Thu, Jun 8, 2017 at 6:32 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > On Tue, May 30, 2017 at 05:33:38PM +0530, Geetha sowjanya wrote:
> >> Cavium ThunderX2 SMMUv3 implementation has two Silicon Erratas.
> >> 1. Errata ID #74
> >>    SMMU register alias Page 1 is not implemented
> >> 2. Errata ID #126
> >>    SMMU doesnt support unique IRQ lines and also MSI for gerror,
> >>    eventq and cmdq-sync
> >>
> >> The following patchset does software workaround for these two erratas.
> >>
> >> This series is based on patchset.
> >> https://www.spinics.net/lists/arm-kernel/msg578443.html
> >
> > Yes so it is not standalone. How are we going to merge these
> > ACPI IORT/ACPICA/SMMU patches - inclusive of:
> >
> > [1] https://www.spinics.net/lists/arm-kernel/msg586458.html
> >
> > Rafael, do ACPICA patches go upstream via the ACPI tree pull request ?
> 
> Not as a rule.

So I take it as the can they go in as separate pull (standalone ACPICA
updates) ?

> > To remove dependency on ACPICA changes this series needs updating
> > anyway and for [1] above I think the only solution is for all the
> > patches to go via the ACPI tree (if ACPICA updates go upstream with it).
> 
> I think we may ask Lv to backport the header changes once they have
> been merged into Linux.
> 
> Lv, would that work?

I was asking to understand how to queue some patches for the upcoming
merge window that have an ACPICA dependency, how are we supposed to
handle that ? I would like to avoid cross tree dependencies, that's why
I asked about the ACPI pull request, so that IORT patches could go via
ACPI tree too this time along with ACPICA changes just trying to make
it simple.

Please let us know, thanks a lot.

Lorenzo