[PATCH v3 0/7] Cavium ThunderX2 SMMUv3 errata workarounds

[PATCH v3 0/7] Cavium ThunderX2 SMMUv3 errata workarounds

[Geetha sowjanya]

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

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 from 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.

Changes from v2:
 Updated "Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt" document with 
 new SMMU option used to enable errata workaround.
 
Geetha Sowjanya (1):
  iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126

Linu Cherian (6):
  iommu/arm-smmu-v3: Introduce smmu option PAGE0_REGS_ONLY for ThunderX2
    errata#74.
  iommu/arm-smmu-v3: Do resource size checks based on SMMU option
    PAGE0_REGS_ONLY
  ACPICA: IORT: Add Cavium ThunderX2 SMMUv3 model definition.
  iommu/arm-smmu-v3: For ACPI based device probing, set PAGE0_REGS_ONLY
    option     for ThunderX2 SMMUv3 implementations.
  ACPI/IORT: Fixup SMMUv3 resource size for Cavium ThunderX2 SMMUv3
    model
  arm64: Documentation: Add Cavium ThunderX2 SMMUv3 erratas

 Documentation/arm64/silicon-errata.txt |   2 +
 drivers/acpi/arm64/iort.c              |  10 ++-
 drivers/iommu/arm-smmu-v3.c            | 122 ++++++++++++++++++++++++++-------
 include/acpi/actbl2.h                  |   2 +
 4 files changed, 110 insertions(+), 26 deletions(-)

-- 
1.8.3.1

[PATCH v3 1/7] iommu/arm-smmu-v3: Introduce SMMU option PAGE0_REGS_ONLY for ThunderX2 errata #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 will be 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.

Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
---
 .../devicetree/bindings/iommu/arm,smmu-v3.txt      |  6 +++
 drivers/iommu/arm-smmu-v3.c                        | 44 ++++++++++++++++------
 2 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
index be57550..e6da62b 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-cn99xx,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..107b4a6 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -176,15 +176,15 @@
 #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_PROD(smmu)	(page1_offset_adjust(0x100a8, smmu))
+#define ARM_SMMU_EVTQ_CONS(smmu)	(page1_offset_adjust(0x100ac, smmu))
 #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_PROD(smmu)	(page1_offset_adjust(0x100c8, smmu))
+#define ARM_SMMU_PRIQ_CONS(smmu)	(page1_offset_adjust(0x100cc, smmu))
 #define ARM_SMMU_PRIQ_IRQ_CFG0		0xd0
 #define ARM_SMMU_PRIQ_IRQ_CFG1		0xd8
 #define ARM_SMMU_PRIQ_IRQ_CFG2		0xdc
@@ -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-cn99xx,broken-page1-regspace"},
 	{ 0, NULL},
 };
 
+static inline unsigned long page1_offset_adjust(
+	unsigned long off, struct arm_smmu_device *smmu)
+{
+	if (!ARM_SMMU_PAGE0_REGS_ONLY(smmu))
+		return off;
+	else
+		return (off - SZ_64K);
+}
+
 static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
 {
 	return container_of(dom, struct arm_smmu_domain, domain);
@@ -1986,8 +2000,10 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
 		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_PROD(smmu),
+				      ARM_SMMU_EVTQ_CONS(smmu),
+				      EVTQ_ENT_DWORDS);
 	if (ret)
 		return ret;
 
@@ -1995,8 +2011,10 @@ 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_PROD(smmu),
+				       ARM_SMMU_PRIQ_CONS(smmu),
+				       PRIQ_ENT_DWORDS);
 }
 
 static int arm_smmu_init_l1_strtab(struct arm_smmu_device *smmu)
@@ -2363,8 +2381,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, smmu->base +
+		       ARM_SMMU_EVTQ_PROD(smmu));
+	writel_relaxed(smmu->evtq.q.cons, smmu->base +
+		       ARM_SMMU_EVTQ_CONS(smmu));
 
 	enables |= CR0_EVTQEN;
 	ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
@@ -2379,9 +2399,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);
+			       smmu->base + ARM_SMMU_PRIQ_PROD(smmu));
 		writel_relaxed(smmu->priq.q.cons,
-			       smmu->base + ARM_SMMU_PRIQ_CONS);
+			       smmu->base + ARM_SMMU_PRIQ_CONS(smmu));
 
 		enables |= CR0_PRIQEN;
 		ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
-- 
1.8.3.1

Re: [PATCH v3 1/7] iommu/arm-smmu-v3: Introduce SMMU option PAGE0_REGS_ONLY for ThunderX2 errata #74

[Robert Richter]

On 05.05.17 17:38:05, 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 will be 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.
> 
> Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
> Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
> ---
>  .../devicetree/bindings/iommu/arm,smmu-v3.txt      |  6 +++
>  drivers/iommu/arm-smmu-v3.c                        | 44 ++++++++++++++++------
>  2 files changed, 38 insertions(+), 12 deletions(-)

> @@ -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)

Why hide the check behind this macro? Maybe make
ARM_SMMU_OPT_PAGE0_REGS_ONLY shorter a bit instead?

-Robert

Re: [PATCH v3 1/7] iommu/arm-smmu-v3: Introduce SMMU option PAGE0_REGS_ONLY for ThunderX2 errata #74

[Robert Richter]

On 05.05.17 17:38:05, 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 will be 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.
> 
> Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
> Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
> ---
>  .../devicetree/bindings/iommu/arm,smmu-v3.txt      |  6 +++
>  drivers/iommu/arm-smmu-v3.c                        | 44 ++++++++++++++++------
>  2 files changed, 38 insertions(+), 12 deletions(-)

> @@ -1995,8 +2011,10 @@ 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_PROD(smmu),
> +				       ARM_SMMU_PRIQ_CONS(smmu),
> +				       PRIQ_ENT_DWORDS);

I would also suggest Robin's idea from the v1 review here. This works
if we rework arm_smmu_init_one_queue() to pass addresses instead of
offsets.

This would make these widespread offset calculations obsolete.

-Robert

Re: [PATCH v3 1/7] iommu/arm-smmu-v3: Introduce SMMU option PAGE0_REGS_ONLY for ThunderX2 errata #74

[Linu Cherian]

On Sat May 06, 2017 at 01:03:28AM +0200, Robert Richter wrote:
> On 05.05.17 17:38:05, 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 will be 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.
> > 
> > Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
> > Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
> > ---
> >  .../devicetree/bindings/iommu/arm,smmu-v3.txt      |  6 +++
> >  drivers/iommu/arm-smmu-v3.c                        | 44 ++++++++++++++++------
> >  2 files changed, 38 insertions(+), 12 deletions(-)
> 
> > @@ -1995,8 +2011,10 @@ 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_PROD(smmu),
> > +				       ARM_SMMU_PRIQ_CONS(smmu),
> > +				       PRIQ_ENT_DWORDS);
> 
> I would also suggest Robin's idea from the v1 review here. This works
> if we rework arm_smmu_init_one_queue() to pass addresses instead of
> offsets.
> 
> This would make these widespread offset calculations obsolete.
>

Have pasted here the relevant changes for doing fixups on smmu base instead
of offset to get feedback. 

This actually results in more lines of changes. If you think the below
approach is still better, will post a V4 of this series with this change.


+static inline unsigned long arm_smmu_page1_base(
+	struct arm_smmu_device *smmu)
+{
+	if (ARM_SMMU_PAGE0_REGS_ONLY(smmu))
+		return smmu->base;
+	else
+		return smmu->base + SZ_64K;
+}
+

@@ -1948,8 +1962,8 @@ static void arm_smmu_put_resv_regions(struct device *dev,
 /* Probing and initialisation functions */
 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,
+				   unsigned long prod_addr,
+				   unsigned long cons_addr,
 				   size_t dwords)
 {
 	size_t qsz = ((1 << q->max_n_shift) * dwords) << 3;
@@ -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	= prod_addr;
+	q->cons_reg	= cons_addr;
 	q->ent_dwords	= dwords;
 
 	q->q_base  = Q_BASE_RWA;
@@ -1977,17 +1991,25 @@ static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
 static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
 {
 	int ret;
+	unsigned long page1_base, page0_base;
+
+	page0_base = smmu->base;
+	page1_base = arm_smmu_page1_base(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, 
+				      page0_base + ARM_SMMU_CMDQ_PROD,
+				      page0_base + ARM_SMMU_CMDQ_CONS, 
+				      CMDQ_ENT_DWORDS);
 	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,
+				      page1_base + ARM_SMMU_EVTQ_PROD,
+				      page1_base + ARM_SMMU_EVTQ_CONS,
+				      EVTQ_ENT_DWORDS);
 	if (ret)
 		return ret;
 
@@ -1995,8 +2017,10 @@ 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,
+				       page1_base + ARM_SMMU_PRIQ_PROD,
+				       page1_base + ARM_SMMU_PRIQ_CONS,
+				       PRIQ_ENT_DWORDS);
 }



@@ -2301,8 +2349,11 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
 {
 	int ret;
 	u32 reg, enables;
+	unsigned long page1_base;
 	struct arm_smmu_cmdq_ent cmd;
 
+	page1_base = arm_smmu_page1_base(smmu);
+
 	/* Clear CR0 and sync (disables SMMU and queue processing) */
 	reg = readl_relaxed(smmu->base + ARM_SMMU_CR0);
 	if (reg & CR0_SMMUEN)
@@ -2363,8 +2414,8 @@ 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, page1_base + ARM_SMMU_EVTQ_PROD);
+	writel_relaxed(smmu->evtq.q.cons, page1_base + ARM_SMMU_EVTQ_CONS);
 
 	enables |= CR0_EVTQEN;
 	ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
@@ -2379,9 +2430,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);
+			       page1_base + ARM_SMMU_PRIQ_PROD);
 		writel_relaxed(smmu->priq.q.cons,
-			       smmu->base + ARM_SMMU_PRIQ_CONS);
+			       page1_base + ARM_SMMU_PRIQ_CONS);
 
 		enables |= CR0_PRIQEN;
 		ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,



Thanks.
-- 
Linu cherian

Re: [PATCH v3 1/7] iommu/arm-smmu-v3: Introduce SMMU option PAGE0_REGS_ONLY for ThunderX2 errata #74

[Robert Richter]

On 08.05.17 14:47:39, Linu Cherian wrote:
> Have pasted here the relevant changes for doing fixups on smmu base instead
> of offset to get feedback. 

To me this looks better than the ARM_SMMU_EVTQ_*() macros. It still
needs some more shaping (e.g. maybe remove page1_base var and call
arm_smmu_page1_base() directly).

But let's see what others say first.

Thanks,

-Robert

> 
> This actually results in more lines of changes. If you think the below
> approach is still better, will post a V4 of this series with this change.
> 
> 
> +static inline unsigned long arm_smmu_page1_base(
> +	struct arm_smmu_device *smmu)
> +{
> +	if (ARM_SMMU_PAGE0_REGS_ONLY(smmu))
> +		return smmu->base;
> +	else
> +		return smmu->base + SZ_64K;
> +}
> +
> 
> @@ -1948,8 +1962,8 @@ static void arm_smmu_put_resv_regions(struct device *dev,
>  /* Probing and initialisation functions */
>  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,
> +				   unsigned long prod_addr,
> +				   unsigned long cons_addr,
>  				   size_t dwords)
>  {
>  	size_t qsz = ((1 << q->max_n_shift) * dwords) << 3;
> @@ -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	= prod_addr;
> +	q->cons_reg	= cons_addr;
>  	q->ent_dwords	= dwords;
>  
>  	q->q_base  = Q_BASE_RWA;
> @@ -1977,17 +1991,25 @@ static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
>  static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
>  {
>  	int ret;
> +	unsigned long page1_base, page0_base;
> +
> +	page0_base = smmu->base;
> +	page1_base = arm_smmu_page1_base(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, 
> +				      page0_base + ARM_SMMU_CMDQ_PROD,
> +				      page0_base + ARM_SMMU_CMDQ_CONS, 
> +				      CMDQ_ENT_DWORDS);
>  	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,
> +				      page1_base + ARM_SMMU_EVTQ_PROD,
> +				      page1_base + ARM_SMMU_EVTQ_CONS,
> +				      EVTQ_ENT_DWORDS);
>  	if (ret)
>  		return ret;
>  
> @@ -1995,8 +2017,10 @@ 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,
> +				       page1_base + ARM_SMMU_PRIQ_PROD,
> +				       page1_base + ARM_SMMU_PRIQ_CONS,
> +				       PRIQ_ENT_DWORDS);
>  }
> 
> 
> 
> @@ -2301,8 +2349,11 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
>  {
>  	int ret;
>  	u32 reg, enables;
> +	unsigned long page1_base;
>  	struct arm_smmu_cmdq_ent cmd;
>  
> +	page1_base = arm_smmu_page1_base(smmu);
> +
>  	/* Clear CR0 and sync (disables SMMU and queue processing) */
>  	reg = readl_relaxed(smmu->base + ARM_SMMU_CR0);
>  	if (reg & CR0_SMMUEN)
> @@ -2363,8 +2414,8 @@ 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, page1_base + ARM_SMMU_EVTQ_PROD);
> +	writel_relaxed(smmu->evtq.q.cons, page1_base + ARM_SMMU_EVTQ_CONS);
>  
>  	enables |= CR0_EVTQEN;
>  	ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
> @@ -2379,9 +2430,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_qBASE);
>  		writel_relaxed(smmu->priq.q.prod,
> -			       smmu->base + ARM_SMMU_PRIQ_PROD);
> +			       page1_base + ARM_SMMU_PRIQ_PROD);
>  		writel_relaxed(smmu->priq.q.cons,
> -			       smmu->base + ARM_SMMU_PRIQ_CONS);
> +			       page1_base + ARM_SMMU_PRIQ_CONS);
>  
>  		enables |= CR0_PRIQEN;
>  		ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
> 
> 
> 
> Thanks.
> -- 
> Linu cherian

Re: [PATCH v3 1/7] iommu/arm-smmu-v3: Introduce SMMU option PAGE0_REGS_ONLY for ThunderX2 errata #74

[Robin Murphy]

On 08/05/17 10:17, Linu Cherian wrote:
> On Sat May 06, 2017 at 01:03:28AM +0200, Robert Richter wrote:
>> On 05.05.17 17:38:05, 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 will be 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.
>>>
>>> Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
>>> Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
>>> ---
>>>  .../devicetree/bindings/iommu/arm,smmu-v3.txt      |  6 +++
>>>  drivers/iommu/arm-smmu-v3.c                        | 44 ++++++++++++++++------
>>>  2 files changed, 38 insertions(+), 12 deletions(-)
>>
>>> @@ -1995,8 +2011,10 @@ 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_PROD(smmu),
>>> +				       ARM_SMMU_PRIQ_CONS(smmu),
>>> +				       PRIQ_ENT_DWORDS);
>>
>> I would also suggest Robin's idea from the v1 review here. This works
>> if we rework arm_smmu_init_one_queue() to pass addresses instead of
>> offsets.
>>
>> This would make these widespread offset calculations obsolete.
>>
> 
> Have pasted here the relevant changes for doing fixups on smmu base instead
> of offset to get feedback. 
> 
> This actually results in more lines of changes. If you think the below
> approach is still better, will post a V4 of this series with this change.

Why not just do this?:

static inline unsigned long page1_offset_adjust(
	unsigned long off, struct arm_smmu_device *smmu)
{
	if (off > SZ_64K && ARM_SMMU_PAGE0_REGS_ONLY(smmu))
		return (off - SZ_64K);

	return off;
}

AFAICS that should be the least disruptive way to go about it.

Robin.

> 
> 
> +static inline unsigned long arm_smmu_page1_base(
> +	struct arm_smmu_device *smmu)
> +{
> +	if (ARM_SMMU_PAGE0_REGS_ONLY(smmu))
> +		return smmu->base;
> +	else
> +		return smmu->base + SZ_64K;
> +}
> +
> 
> @@ -1948,8 +1962,8 @@ static void arm_smmu_put_resv_regions(struct device *dev,
>  /* Probing and initialisation functions */
>  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,
> +				   unsigned long prod_addr,
> +				   unsigned long cons_addr,
>  				   size_t dwords)
>  {
>  	size_t qsz = ((1 << q->max_n_shift) * dwords) << 3;
> @@ -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	= prod_addr;
> +	q->cons_reg	= cons_addr;
>  	q->ent_dwords	= dwords;
>  
>  	q->q_base  = Q_BASE_RWA;
> @@ -1977,17 +1991,25 @@ static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
>  static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
>  {
>  	int ret;
> +	unsigned long page1_base, page0_base;
> +
> +	page0_base = smmu->base;
> +	page1_base = arm_smmu_page1_base(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, 
> +				      page0_base + ARM_SMMU_CMDQ_PROD,
> +				      page0_base + ARM_SMMU_CMDQ_CONS, 
> +				      CMDQ_ENT_DWORDS);
>  	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,
> +				      page1_base + ARM_SMMU_EVTQ_PROD,
> +				      page1_base + ARM_SMMU_EVTQ_CONS,
> +				      EVTQ_ENT_DWORDS);
>  	if (ret)
>  		return ret;
>  
> @@ -1995,8 +2017,10 @@ 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,
> +				       page1_base + ARM_SMMU_PRIQ_PROD,
> +				       page1_base + ARM_SMMU_PRIQ_CONS,
> +				       PRIQ_ENT_DWORDS);
>  }
> 
> 
> 
> @@ -2301,8 +2349,11 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
>  {
>  	int ret;
>  	u32 reg, enables;
> +	unsigned long page1_base;
>  	struct arm_smmu_cmdq_ent cmd;
>  
> +	page1_base = arm_smmu_page1_base(smmu);
> +
>  	/* Clear CR0 and sync (disables SMMU and queue processing) */
>  	reg = readl_relaxed(smmu->base + ARM_SMMU_CR0);
>  	if (reg & CR0_SMMUEN)
> @@ -2363,8 +2414,8 @@ 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, page1_base + ARM_SMMU_EVTQ_PROD);
> +	writel_relaxed(smmu->evtq.q.cons, page1_base + ARM_SMMU_EVTQ_CONS);
>  
>  	enables |= CR0_EVTQEN;
>  	ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
> @@ -2379,9 +2430,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);
> +			       page1_base + ARM_SMMU_PRIQ_PROD);
>  		writel_relaxed(smmu->priq.q.cons,
> -			       smmu->base + ARM_SMMU_PRIQ_CONS);
> +			       page1_base + ARM_SMMU_PRIQ_CONS);
>  
>  		enables |= CR0_PRIQEN;
>  		ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
> 
> 
> 
> Thanks.
> 

Re: [PATCH v3 1/7] iommu/arm-smmu-v3: Introduce SMMU option PAGE0_REGS_ONLY for ThunderX2 errata #74

[Robert Richter]

On 08.05.17 10:59:46, Robin Murphy wrote:
> On 08/05/17 10:17, Linu Cherian wrote:
> > This actually results in more lines of changes. If you think the below
> > approach is still better, will post a V4 of this series with this change.
> 
> Why not just do this?:
> 
> static inline unsigned long page1_offset_adjust(
> 	unsigned long off, struct arm_smmu_device *smmu)
> {
> 	if (off > SZ_64K && ARM_SMMU_PAGE0_REGS_ONLY(smmu))
> 		return (off - SZ_64K);
> 
> 	return off;
> }
> 
> AFAICS that should be the least disruptive way to go about it.

Yeah, let's go with this.

Thanks Robin,

-Robert

[PATCH v3 2/7] iommu/arm-smmu-v3: Do resource size checks based on SMMU

[Geetha sowjanya]

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

With implementations supporting only page 0 register space,
resource size can be 64k as well and hence perform size checks
based on SMMU option PAGE0_REGS_ONLY.

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>
---
 drivers/iommu/arm-smmu-v3.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 107b4a6..f027676 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2672,6 +2672,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;
@@ -2688,9 +2696,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;
 	}
@@ -2717,14 +2733,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.8.3.1

Re: [PATCH v3 2/7] iommu/arm-smmu-v3: Do resource size checks based on SMMU

[Robert Richter]

On 05.05.17 17:38:06, Geetha sowjanya wrote:
> From: Linu Cherian <linu.cherian@cavium.com>
> 
> With implementations supporting only page 0 register space,
> resource size can be 64k as well and hence perform size checks
> based on SMMU option PAGE0_REGS_ONLY.
> 
> 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>
> ---
>  drivers/iommu/arm-smmu-v3.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 107b4a6..f027676 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2672,6 +2672,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;
> +}
> +

I think this can be dropped. See below.

>  static int arm_smmu_device_probe(struct platform_device *pdev)
>  {
>  	int irq, ret;
> @@ -2688,9 +2696,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;
>  	}

Why not just do the follwoing here:

 	/* Base address */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
 		dev_err(dev, "MMIO region too small (%pr)\n", res);
 		return -EINVAL;
 	}
 	ioaddr = res->start;

+	/*
+	 * Override the size, for Cavium ThunderX2 implementation
+	 * which doesn't support the page 1 SMMU register space.
+	 */
+	if (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)
+		res->end = res->size + SZ_64K -1;
+
 	smmu->base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(smmu->base))
 		return PTR_ERR(smmu->base);

Since we can drop patch #5 then, the fix would be isolated to this
file only. And we can use smmu->options as the onle check for this.

-Robert

> @@ -2717,14 +2733,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.8.3.1
> 

Re: [PATCH v3 2/7] iommu/arm-smmu-v3: Do resource size checks based on SMMU

[Linu Cherian]

On Sat May 06, 2017 at 12:18:44AM +0200, Robert Richter wrote:
> On 05.05.17 17:38:06, Geetha sowjanya wrote:
> > From: Linu Cherian <linu.cherian@cavium.com>
> > 
> > With implementations supporting only page 0 register space,
> > resource size can be 64k as well and hence perform size checks
> > based on SMMU option PAGE0_REGS_ONLY.
> > 
> > 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>
> > ---
> >  drivers/iommu/arm-smmu-v3.c | 26 +++++++++++++++++---------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > index 107b4a6..f027676 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -2672,6 +2672,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;
> > +}
> > +
> 
> I think this can be dropped. See below.
> 
> >  static int arm_smmu_device_probe(struct platform_device *pdev)
> >  {
> >  	int irq, ret;
> > @@ -2688,9 +2696,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;
> >  	}
> 
> Why not just do the follwoing here:
> 
>  	/* Base address */
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
>  		dev_err(dev, "MMIO region too small (%pr)\n", res);
>  		return -EINVAL;
>  	}
>  	ioaddr = res->start;
> 
> +	/*
> +	 * Override the size, for Cavium ThunderX2 implementation
> +	 * which doesn't support the page 1 SMMU register space.
> +	 */
> +	if (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)
> +		res->end = res->size + SZ_64K -1;
> +
>  	smmu->base = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(smmu->base))
>  		return PTR_ERR(smmu->base);


This might not work, since platform_device_add is being called from
iort.c before the res->end gets fixed up here. 


-- 
Linu cherian

Re: [PATCH v3 2/7] iommu/arm-smmu-v3: Do resource size checks based on SMMU

[Robert Richter]

On 08.05.17 15:14:37, Linu Cherian wrote:
> On Sat May 06, 2017 at 12:18:44AM +0200, Robert Richter wrote:
> > On 05.05.17 17:38:06, Geetha sowjanya wrote:
> > > From: Linu Cherian <linu.cherian@cavium.com>
> > > 
> > > With implementations supporting only page 0 register space,
> > > resource size can be 64k as well and hence perform size checks
> > > based on SMMU option PAGE0_REGS_ONLY.
> > > 
> > > 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>
> > > ---
> > >  drivers/iommu/arm-smmu-v3.c | 26 +++++++++++++++++---------
> > >  1 file changed, 17 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > > index 107b4a6..f027676 100644
> > > --- a/drivers/iommu/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > @@ -2672,6 +2672,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;
> > > +}
> > > +
> > 
> > I think this can be dropped. See below.
> > 
> > >  static int arm_smmu_device_probe(struct platform_device *pdev)
> > >  {
> > >  	int irq, ret;
> > > @@ -2688,9 +2696,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;
> > >  	}
> > 
> > Why not just do the follwoing here:
> > 
> >  	/* Base address */
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
> >  		dev_err(dev, "MMIO region too small (%pr)\n", res);
> >  		return -EINVAL;
> >  	}
> >  	ioaddr = res->start;
> > 
> > +	/*
> > +	 * Override the size, for Cavium ThunderX2 implementation
> > +	 * which doesn't support the page 1 SMMU register space.
> > +	 */
> > +	if (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)
> > +		res->end = res->size + SZ_64K -1;
> > +
> >  	smmu->base = devm_ioremap_resource(dev, res);
> >  	if (IS_ERR(smmu->base))
> >  		return PTR_ERR(smmu->base);
> 
> 
> This might not work, since platform_device_add is being called from
> iort.c before the res->end gets fixed up here. 

It should. You added it with 128k and you get it back with
platform_get_resource(), but before ioremap you shrink the size to
64k.

-Robert

Re: [PATCH v3 2/7] iommu/arm-smmu-v3: Do resource size checks based on SMMU

[Linu Cherian]

On Mon May 08, 2017 at 12:09:32PM +0200, Robert Richter wrote:
> On 08.05.17 15:14:37, Linu Cherian wrote:
> > On Sat May 06, 2017 at 12:18:44AM +0200, Robert Richter wrote:
> > > On 05.05.17 17:38:06, Geetha sowjanya wrote:
> > > > From: Linu Cherian <linu.cherian@cavium.com>
> > > > 
> > > > With implementations supporting only page 0 register space,
> > > > resource size can be 64k as well and hence perform size checks
> > > > based on SMMU option PAGE0_REGS_ONLY.
> > > > 
> > > > 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>
> > > > ---
> > > >  drivers/iommu/arm-smmu-v3.c | 26 +++++++++++++++++---------
> > > >  1 file changed, 17 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > > > index 107b4a6..f027676 100644
> > > > --- a/drivers/iommu/arm-smmu-v3.c
> > > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > > @@ -2672,6 +2672,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;
> > > > +}
> > > > +
> > > 
> > > I think this can be dropped. See below.
> > > 
> > > >  static int arm_smmu_device_probe(struct platform_device *pdev)
> > > >  {
> > > >  	int irq, ret;
> > > > @@ -2688,9 +2696,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;
> > > >  	}
> > > 
> > > Why not just do the follwoing here:
> > > 
> > >  	/* Base address */
> > >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >  	if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
> > >  		dev_err(dev, "MMIO region too small (%pr)\n", res);
> > >  		return -EINVAL;
> > >  	}
> > >  	ioaddr = res->start;
> > > 
> > > +	/*
> > > +	 * Override the size, for Cavium ThunderX2 implementation
> > > +	 * which doesn't support the page 1 SMMU register space.
> > > +	 */
> > > +	if (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)
> > > +		res->end = res->size + SZ_64K -1;
> > > +
> > >  	smmu->base = devm_ioremap_resource(dev, res);
> > >  	if (IS_ERR(smmu->base))
> > >  		return PTR_ERR(smmu->base);
> > 
> > 
> > This might not work, since platform_device_add is being called from
> > iort.c before the res->end gets fixed up here. 
> 
> It should. You added it with 128k and you get it back with
> platform_get_resource(), but before ioremap you shrink the size to
> 64k.
> 

The smmu devices are located at 64k offsets and not at 128k
offsets and hence this would be result in resource conflict during
platform_add_device ?

Code snippet from platform_add_device:

        for (i = 0; i < pdev->num_resources; i++) {
 	        struct resource *p, *r = &pdev->resource[i];

		if (r->name == NULL)
	                r->name = dev_name(&pdev->dev);

		p = r->parent;
                if (!p) {
                        if (resource_type(r) == IORESOURCE_MEM)
                                p = &iomem_resource;
                        else if (resource_type(r) == IORESOURCE_IO)
                                p = &ioport_resource;
                }

                if (p && insert_resource(p, r)) {
                        dev_err(&pdev->dev, "failed to claim resource %d: %pR\n", i, r);
                        ret = -EBUSY;
                        goto failed;
                }
        }







-- 
Linu cherian

Re: [PATCH v3 2/7] iommu/arm-smmu-v3: Do resource size checks based on SMMU

[Robert Richter]

On 08.05.17 16:20:49, Linu Cherian wrote:
> 
> On Mon May 08, 2017 at 12:09:32PM +0200, Robert Richter wrote:
> > On 08.05.17 15:14:37, Linu Cherian wrote:
> > > On Sat May 06, 2017 at 12:18:44AM +0200, Robert Richter wrote:
> > > > On 05.05.17 17:38:06, Geetha sowjanya wrote:
> > > > > From: Linu Cherian <linu.cherian@cavium.com>
> > > > > 
> > > > > With implementations supporting only page 0 register space,
> > > > > resource size can be 64k as well and hence perform size checks
> > > > > based on SMMU option PAGE0_REGS_ONLY.
> > > > > 
> > > > > 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>
> > > > > ---
> > > > >  drivers/iommu/arm-smmu-v3.c | 26 +++++++++++++++++---------
> > > > >  1 file changed, 17 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > > > > index 107b4a6..f027676 100644
> > > > > --- a/drivers/iommu/arm-smmu-v3.c
> > > > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > > > @@ -2672,6 +2672,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;
> > > > > +}
> > > > > +
> > > > 
> > > > I think this can be dropped. See below.
> > > > 
> > > > >  static int arm_smmu_device_probe(struct platform_device *pdev)
> > > > >  {
> > > > >  	int irq, ret;
> > > > > @@ -2688,9 +2696,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;
> > > > >  	}
> > > > 
> > > > Why not just do the follwoing here:
> > > > 
> > > >  	/* Base address */
> > > >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > >  	if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
> > > >  		dev_err(dev, "MMIO region too small (%pr)\n", res);
> > > >  		return -EINVAL;
> > > >  	}
> > > >  	ioaddr = res->start;
> > > > 
> > > > +	/*
> > > > +	 * Override the size, for Cavium ThunderX2 implementation
> > > > +	 * which doesn't support the page 1 SMMU register space.
> > > > +	 */
> > > > +	if (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)
> > > > +		res->end = res->size + SZ_64K -1;
> > > > +
> > > >  	smmu->base = devm_ioremap_resource(dev, res);
> > > >  	if (IS_ERR(smmu->base))
> > > >  		return PTR_ERR(smmu->base);
> > > 
> > > 
> > > This might not work, since platform_device_add is being called from
> > > iort.c before the res->end gets fixed up here. 
> > 
> > It should. You added it with 128k and you get it back with
> > platform_get_resource(), but before ioremap you shrink the size to
> > 64k.
> > 
> 
> The smmu devices are located at 64k offsets and not at 128k
> offsets and hence this would be result in resource conflict during
> platform_add_device ?

Right, we have overlapping io spaces then. So we need to change also
iort.c for the fix.

-Robert

Re: [PATCH v3 2/7] iommu/arm-smmu-v3: Do resource size checks based on SMMU

[Geetha Akula]

On Mon, May 8, 2017 at 3:39 PM, Robert Richter
<robert.richter@cavium.com> wrote:
> On 08.05.17 15:14:37, Linu Cherian wrote:
>> On Sat May 06, 2017 at 12:18:44AM +0200, Robert Richter wrote:
>> > On 05.05.17 17:38:06, Geetha sowjanya wrote:
>> > > From: Linu Cherian <linu.cherian@cavium.com>
>> > >
>> > > With implementations supporting only page 0 register space,
>> > > resource size can be 64k as well and hence perform size checks
>> > > based on SMMU option PAGE0_REGS_ONLY.
>> > >
>> > > 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>
>> > > ---
>> > >  drivers/iommu/arm-smmu-v3.c | 26 +++++++++++++++++---------
>> > >  1 file changed, 17 insertions(+), 9 deletions(-)
>> > >
>> > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> > > index 107b4a6..f027676 100644
>> > > --- a/drivers/iommu/arm-smmu-v3.c
>> > > +++ b/drivers/iommu/arm-smmu-v3.c
>> > > @@ -2672,6 +2672,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;
>> > > +}
>> > > +
>> >
>> > I think this can be dropped. See below.
>> >
>> > >  static int arm_smmu_device_probe(struct platform_device *pdev)
>> > >  {
>> > >   int irq, ret;
>> > > @@ -2688,9 +2696,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;
>> > >   }
>> >
>> > Why not just do the follwoing here:
>> >
>> >     /* Base address */
>> >     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> >     if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
>> >             dev_err(dev, "MMIO region too small (%pr)\n", res);
>> >             return -EINVAL;
>> >     }
>> >     ioaddr = res->start;
>> >
>> > +   /*
>> > +    * Override the size, for Cavium ThunderX2 implementation
>> > +    * which doesn't support the page 1 SMMU register space.
>> > +    */
>> > +   if (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)
>> > +           res->end = res->size + SZ_64K -1;
>> > +
>> >     smmu->base = devm_ioremap_resource(dev, res);
>> >     if (IS_ERR(smmu->base))
>> >             return PTR_ERR(smmu->base);
>>
>>
>> This might not work, since platform_device_add is being called from
>> iort.c before the res->end gets fixed up here.
>
> It should. You added it with 128k and you get it back with
> platform_get_resource(), but before ioremap you shrink the size to
> 64k.
>
> -Robert

Hi Robert,

Linu is right. You are missing the sequence of event. If we skip the
changes in iort file, smmu initialization in acpi fails.


[    3.721647] ACPI: IORT: iort_add_smmu_platform_device
[    3.726826] platform arm-smmu-v3.1.auto: failed to claim resource
0: [mem 0x402310000-0x40232ffff]
[    3.736052] arm-smmu-v3 arm-smmu-v3.0.auto: option mask 0x1
[    3.741753] arm_smmu_device_probe


Thank you,
Geetha.

[PATCH v3 3/7] ACPICA: IORT: Add Cavium ThunderX2 SMMUv3 model definition.

[Geetha sowjanya]

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

Add SMMUv3 model definition for ThunderX2.

Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
---
 include/acpi/actbl2.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index faa9f2c..76a6f5d 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -779,6 +779,8 @@ struct acpi_iort_smmu {
 #define ACPI_IORT_SMMU_CORELINK_MMU400  0x00000002	/* ARM Corelink MMU-400 */
 #define ACPI_IORT_SMMU_CORELINK_MMU500  0x00000003	/* ARM Corelink MMU-500 */
 
+#define ACPI_IORT_SMMU_V3_CAVIUM_CN99XX 0x00000002 /* Cavium ThunderX2 SMMUv3 */
+
 /* Masks for Flags field above */
 
 #define ACPI_IORT_SMMU_DVM_SUPPORTED    (1)
-- 
1.8.3.1

Re: [PATCH v3 3/7] ACPICA: IORT: Add Cavium ThunderX2 SMMUv3 model definition.

[Hanjun Guo]

On 2017/5/5 20:08, Geetha sowjanya wrote:
> From: Linu Cherian <linu.cherian@cavium.com>
>
> Add SMMUv3 model definition for ThunderX2.
>
> Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
> Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
> ---
>  include/acpi/actbl2.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
> index faa9f2c..76a6f5d 100644
> --- a/include/acpi/actbl2.h
> +++ b/include/acpi/actbl2.h
> @@ -779,6 +779,8 @@ struct acpi_iort_smmu {
>  #define ACPI_IORT_SMMU_CORELINK_MMU400  0x00000002	/* ARM Corelink MMU-400 */
>  #define ACPI_IORT_SMMU_CORELINK_MMU500  0x00000003	/* ARM Corelink MMU-500 */
>
> +#define ACPI_IORT_SMMU_V3_CAVIUM_CN99XX 0x00000002 /* Cavium ThunderX2 SMMUv3 */

There are some other model numbers in the unreleased spec,
I think we need to wait for the updated IORT spec to
be released.

Thanks
Hanjun

Re: [PATCH v3 3/7] ACPICA: IORT: Add Cavium ThunderX2 SMMUv3 model definition.

[David Daney]

On 05/05/2017 06:53 AM, Hanjun Guo wrote:
> On 2017/5/5 20:08, Geetha sowjanya wrote:
>> From: Linu Cherian <linu.cherian@cavium.com>
>>
>> Add SMMUv3 model definition for ThunderX2.
>>
>> Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
>> Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
>> ---
>>  include/acpi/actbl2.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
>> index faa9f2c..76a6f5d 100644
>> --- a/include/acpi/actbl2.h
>> +++ b/include/acpi/actbl2.h
>> @@ -779,6 +779,8 @@ struct acpi_iort_smmu {
>>  #define ACPI_IORT_SMMU_CORELINK_MMU400  0x00000002    /* ARM Corelink 
>> MMU-400 */
>>  #define ACPI_IORT_SMMU_CORELINK_MMU500  0x00000003    /* ARM Corelink 
>> MMU-500 */
>>
>> +#define ACPI_IORT_SMMU_V3_CAVIUM_CN99XX 0x00000002 /* Cavium 
>> ThunderX2 SMMUv3 */
> 
> There are some other model numbers in the unreleased spec,
> I think we need to wait for the updated IORT spec to
> be released.
> 

... or if we are fairly confident that the identifier will not need to 
change, we can merge this as is and establish a de facto specification 
that the Real IORT specification will then be forced to follow.

Is there anything other than bureaucratic inertia holding up the real 
specification?


David.

Re: [PATCH v3 3/7] ACPICA: IORT: Add Cavium ThunderX2 SMMUv3 model definition.

[Will Deacon]

On Fri, May 05, 2017 at 07:56:17AM -0700, David Daney wrote:
> On 05/05/2017 06:53 AM, Hanjun Guo wrote:
> >On 2017/5/5 20:08, Geetha sowjanya wrote:
> >>From: Linu Cherian <linu.cherian@cavium.com>
> >>
> >>Add SMMUv3 model definition for ThunderX2.
> >>
> >>Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
> >>Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
> >>---
> >> include/acpi/actbl2.h | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >>diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
> >>index faa9f2c..76a6f5d 100644
> >>--- a/include/acpi/actbl2.h
> >>+++ b/include/acpi/actbl2.h
> >>@@ -779,6 +779,8 @@ struct acpi_iort_smmu {
> >> #define ACPI_IORT_SMMU_CORELINK_MMU400  0x00000002    /* ARM Corelink
> >>MMU-400 */
> >> #define ACPI_IORT_SMMU_CORELINK_MMU500  0x00000003    /* ARM Corelink
> >>MMU-500 */
> >>
> >>+#define ACPI_IORT_SMMU_V3_CAVIUM_CN99XX 0x00000002 /* Cavium ThunderX2
> >>SMMUv3 */
> >
> >There are some other model numbers in the unreleased spec,
> >I think we need to wait for the updated IORT spec to
> >be released.
> >
> 
> ... or if we are fairly confident that the identifier will not need to
> change, we can merge this as is and establish a de facto specification that
> the Real IORT specification will then be forced to follow.
> 
> Is there anything other than bureaucratic inertia holding up the real
> specification?

My understanding is that IORT is going to be published imminently (i.e.
before the next kernel release), so it makes sense to wait rather than fork
the spec.

Will

Re: [PATCH v3 3/7] ACPICA: IORT: Add Cavium ThunderX2 SMMUv3 model definition.

[Jon Masters]

On 05/05/2017 10:58 AM, Will Deacon wrote:
> On Fri, May 05, 2017 at 07:56:17AM -0700, David Daney wrote:
>> On 05/05/2017 06:53 AM, Hanjun Guo wrote:
>>> On 2017/5/5 20:08, Geetha sowjanya wrote:
>>>> From: Linu Cherian <linu.cherian@cavium.com>

>>>> +#define ACPI_IORT_SMMU_V3_CAVIUM_CN99XX 0x00000002 /* Cavium ThunderX2
>>>> SMMUv3 */
>>>
>>> There are some other model numbers in the unreleased spec,
>>> I think we need to wait for the updated IORT spec to
>>> be released.

Indeed. I've synced with the author on this and he's got it in hand.

>> ... or if we are fairly confident that the identifier will not need to
>> change, we can merge this as is and establish a de facto specification that
>> the Real IORT specification will then be forced to follow.

Can't do that - this always causes trouble ;) But if there's any delay
I'll ask that the IDs at least be listed somewhere public or something.

>> Is there anything other than bureaucratic inertia holding up the real
>> specification?
> 
> My understanding is that IORT is going to be published imminently (i.e.
> before the next kernel release), so it makes sense to wait rather than fork
> the spec.

Let's track this and get the updated patches posted next week once the
new ID drops. Meanwhile, I suggest reviewing them as-is for other
issues. I'm tracking this for internal purposes and require this to be
upstream asap so I'll be sitting on this thread for updates ;)

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

[PATCH v3 4/7] iommu/arm-smmu-v3: For ACPI based device probing, set PAGE0_REGS_ONLY option for ThunderX2 SMMUv3 implementation.

[Geetha sowjanya]

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

Enable PAGE0_REGS_ONLY option for Cavium ThunderX2 SMMUv3 model.

Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
---
 drivers/iommu/arm-smmu-v3.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index f027676..8f7d8ad 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2625,6 +2625,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_V3_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)
 {
@@ -2637,6 +2645,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;
 
-- 
1.8.3.1

[PATCH v3 5/7] 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 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index c5fecf9..23c5350 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_V3_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.8.3.1

Re: [PATCH v3 5/7] ACPI/IORT: Fixup SMMUv3 resource size for Cavium ThunderX2 SMMUv3 model

[Robert Richter]

On 05.05.17 17:38:09, 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 file changed, 9 insertions(+), 1 deletion(-)

The whole patch can be dropped. See my comment in #2.

-Robert

> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index c5fecf9..23c5350 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_V3_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.8.3.1
> 

[PATCH v3 6/7] 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>
---
 drivers/iommu/arm-smmu-v3.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 016b702..46428e7 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2236,10 +2236,30 @@ 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 0;
+}
+
 static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 {
 	int ret, irq;
 	u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
+	u32 irqflags = 0;
 
 	/* Disable IRQs first */
 	ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
@@ -2254,9 +2274,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 	/* Request interrupt lines */
 	irq = smmu->evtq.q.irq;
 	if (irq) {
+		irqflags = get_irq_flags(smmu, irq);
 		ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
 						arm_smmu_evtq_thread,
-						IRQF_ONESHOT,
+						IRQF_ONESHOT | irqflags,
 						"arm-smmu-v3-evtq", smmu);
 		if (ret < 0)
 			dev_warn(smmu->dev, "failed to enable evtq irq\n");
@@ -2264,8 +2285,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 
 	irq = smmu->cmdq.q.irq;
 	if (irq) {
+		irqflags = get_irq_flags(smmu, irq);
 		ret = devm_request_irq(smmu->dev, irq,
-				       arm_smmu_cmdq_sync_handler, 0,
+				       arm_smmu_cmdq_sync_handler, irqflags,
 				       "arm-smmu-v3-cmdq-sync", smmu);
 		if (ret < 0)
 			dev_warn(smmu->dev, "failed to enable cmdq-sync irq\n");
@@ -2273,8 +2295,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 
 	irq = smmu->gerr_irq;
 	if (irq) {
+		irqflags = get_irq_flags(smmu, irq);
 		ret = devm_request_irq(smmu->dev, irq, arm_smmu_gerror_handler,
-				       0, "arm-smmu-v3-gerror", smmu);
+				       irqflags, "arm-smmu-v3-gerror", smmu);
 		if (ret < 0)
 			dev_warn(smmu->dev, "failed to enable gerror irq\n");
 	}
@@ -2282,9 +2305,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 	if (smmu->features & ARM_SMMU_FEAT_PRI) {
 		irq = smmu->priq.q.irq;
 		if (irq) {
+			irqflags = get_irq_flags(smmu, irq);
 			ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
 							arm_smmu_priq_thread,
-							IRQF_ONESHOT,
+							IRQF_ONESHOT | irqflags,
 							"arm-smmu-v3-priq",
 							smmu);
 			if (ret < 0)
-- 
1.8.3.1

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

[Robin Murphy]

On 05/05/17 13:08, 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>
> ---
>  drivers/iommu/arm-smmu-v3.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 016b702..46428e7 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2236,10 +2236,30 @@ 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 0;

I'd say just have this return IRQF_ONESHOT in the non-shared case...

> +}
> +
>  static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>  {
>  	int ret, irq;
>  	u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
> +	u32 irqflags = 0;
>  
>  	/* Disable IRQs first */
>  	ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
> @@ -2254,9 +2274,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>  	/* Request interrupt lines */
>  	irq = smmu->evtq.q.irq;
>  	if (irq) {
> +		irqflags = get_irq_flags(smmu, irq);
>  		ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
>  						arm_smmu_evtq_thread,
> -						IRQF_ONESHOT,
> +						IRQF_ONESHOT | irqflags,

...and pass get_irq_flags(smmu, irq) directly as the argument here.

The local variable and intermediate logic only seem to add unnecessary
complexity, given that the two cases we actually end up with are:

IRQF_ONESHOT | 0

vs.

IRQF_ONESHOT | IRQF_SHARED | IRQF_ONESHOT

neither of which looks particularly sensible.

Robin.

>  						"arm-smmu-v3-evtq", smmu);
>  		if (ret < 0)
>  			dev_warn(smmu->dev, "failed to enable evtq irq\n");
> @@ -2264,8 +2285,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>  
>  	irq = smmu->cmdq.q.irq;
>  	if (irq) {
> +		irqflags = get_irq_flags(smmu, irq);
>  		ret = devm_request_irq(smmu->dev, irq,
> -				       arm_smmu_cmdq_sync_handler, 0,
> +				       arm_smmu_cmdq_sync_handler, irqflags,
>  				       "arm-smmu-v3-cmdq-sync", smmu);
>  		if (ret < 0)
>  			dev_warn(smmu->dev, "failed to enable cmdq-sync irq\n");
> @@ -2273,8 +2295,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>  
>  	irq = smmu->gerr_irq;
>  	if (irq) {
> +		irqflags = get_irq_flags(smmu, irq);
>  		ret = devm_request_irq(smmu->dev, irq, arm_smmu_gerror_handler,
> -				       0, "arm-smmu-v3-gerror", smmu);
> +				       irqflags, "arm-smmu-v3-gerror", smmu);
>  		if (ret < 0)
>  			dev_warn(smmu->dev, "failed to enable gerror irq\n");
>  	}
> @@ -2282,9 +2305,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>  	if (smmu->features & ARM_SMMU_FEAT_PRI) {
>  		irq = smmu->priq.q.irq;
>  		if (irq) {
> +			irqflags = get_irq_flags(smmu, irq);
>  			ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
>  							arm_smmu_priq_thread,
> -							IRQF_ONESHOT,
> +							IRQF_ONESHOT | irqflags,
>  							"arm-smmu-v3-priq",
>  							smmu);
>  			if (ret < 0)
> 

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

[Geetha Akula]

On Mon, May 8, 2017 at 4:51 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 05/05/17 13:08, 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>
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 32 ++++++++++++++++++++++++++++----
>>  1 file changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 016b702..46428e7 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -2236,10 +2236,30 @@ 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 0;
>
> I'd say just have this return IRQF_ONESHOT in the non-shared case...
>
>> +}
>> +
>>  static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>>  {
>>       int ret, irq;
>>       u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
>> +     u32 irqflags = 0;
>>
>>       /* Disable IRQs first */
>>       ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
>> @@ -2254,9 +2274,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>>       /* Request interrupt lines */
>>       irq = smmu->evtq.q.irq;
>>       if (irq) {
>> +             irqflags = get_irq_flags(smmu, irq);
>>               ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
>>                                               arm_smmu_evtq_thread,
>> -                                             IRQF_ONESHOT,
>> +                                             IRQF_ONESHOT | irqflags,
>
> ...and pass get_irq_flags(smmu, irq) directly as the argument here.
>
> The local variable and intermediate logic only seem to add unnecessary
> complexity, given that the two cases we actually end up with are:
>
> IRQF_ONESHOT | 0
>
> vs.
>
> IRQF_ONESHOT | IRQF_SHARED | IRQF_ONESHOT
>
> neither of which looks particularly sensible.
>
> Robin.
>
I will resubmit the patch with suggested changes.

Thank you,
Geetha.



>>                                               "arm-smmu-v3-evtq", smmu);
>>               if (ret < 0)
>>                       dev_warn(smmu->dev, "failed to enable evtq irq\n");
>> @@ -2264,8 +2285,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>>
>>       irq = smmu->cmdq.q.irq;
>>       if (irq) {
>> +             irqflags = get_irq_flags(smmu, irq);
>>               ret = devm_request_irq(smmu->dev, irq,
>> -                                    arm_smmu_cmdq_sync_handler, 0,
>> +                                    arm_smmu_cmdq_sync_handler, irqflags,
>>                                      "arm-smmu-v3-cmdq-sync", smmu);
>>               if (ret < 0)
>>                       dev_warn(smmu->dev, "failed to enable cmdq-sync irq\n");
>> @@ -2273,8 +2295,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>>
>>       irq = smmu->gerr_irq;
>>       if (irq) {
>> +             irqflags = get_irq_flags(smmu, irq);
>>               ret = devm_request_irq(smmu->dev, irq, arm_smmu_gerror_handler,
>> -                                    0, "arm-smmu-v3-gerror", smmu);
>> +                                    irqflags, "arm-smmu-v3-gerror", smmu);
>>               if (ret < 0)
>>                       dev_warn(smmu->dev, "failed to enable gerror irq\n");
>>       }
>> @@ -2282,9 +2305,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>>       if (smmu->features & ARM_SMMU_FEAT_PRI) {
>>               irq = smmu->priq.q.irq;
>>               if (irq) {
>> +                     irqflags = get_irq_flags(smmu, irq);
>>                       ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
>>                                                       arm_smmu_priq_thread,
>> -                                                     IRQF_ONESHOT,
>> +                                                     IRQF_ONESHOT | irqflags,
>>                                                       "arm-smmu-v3-priq",
>>                                                       smmu);
>>                       if (ret < 0)
>>
>

[PATCH v3 7/7] arm64: Documentation: Add Cavium ThunderX2 SMMUv3 erratas

[Geetha sowjanya]

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

Add Cavium ThunderX2 SMMUv3 erratas to the errata list.

Signed-off-by: Linu Cherian <linu.cherian@cavium.com>
Signed-off-by: Geetha Sowjanya <geethasowjanya.akula@cavium.com>
---
 Documentation/arm64/silicon-errata.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
index 10f2ddd..42422f6 100644
--- a/Documentation/arm64/silicon-errata.txt
+++ b/Documentation/arm64/silicon-errata.txt
@@ -62,6 +62,8 @@ 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                         |
+| Cavium         | ThunderX2 SMMUv3| #126            | N/A                         |
 |                |                 |                 |                             |
 | Freescale/NXP  | LS2080A/LS1043A | A-008585        | FSL_ERRATUM_A008585         |
 |                |                 |                 |                             |
-- 
1.8.3.1

Re: [PATCH v3 0/7] Cavium ThunderX2 SMMUv3 errata workarounds

[Robert Richter]

On 05.05.17 17:38:04, Geetha sowjanya wrote:
> From: Linu Cherian <linu.cherian@cavium.com>
> 
> 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 from 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.
> 
> Changes from v2:
>  Updated "Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt" document with 
>  new SMMU option used to enable errata workaround.
>  
> Geetha Sowjanya (1):
>   iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126
> 
> Linu Cherian (6):
>   iommu/arm-smmu-v3: Introduce smmu option PAGE0_REGS_ONLY for ThunderX2
>     errata#74.
>   iommu/arm-smmu-v3: Do resource size checks based on SMMU option
>     PAGE0_REGS_ONLY
>   ACPICA: IORT: Add Cavium ThunderX2 SMMUv3 model definition.
>   iommu/arm-smmu-v3: For ACPI based device probing, set PAGE0_REGS_ONLY
>     option     for ThunderX2 SMMUv3 implementations.
>   ACPI/IORT: Fixup SMMUv3 resource size for Cavium ThunderX2 SMMUv3
>     model
>   arm64: Documentation: Add Cavium ThunderX2 SMMUv3 erratas

This split into patches does not look reasonable to me. 1 patch only
for each workaround should be sufficient.

-Robert

> 
>  Documentation/arm64/silicon-errata.txt |   2 +
>  drivers/acpi/arm64/iort.c              |  10 ++-
>  drivers/iommu/arm-smmu-v3.c            | 122 ++++++++++++++++++++++++++-------
>  include/acpi/actbl2.h                  |   2 +
>  4 files changed, 110 insertions(+), 26 deletions(-)
> 
> -- 
> 1.8.3.1
> 

Re: [PATCH v3 0/7] Cavium ThunderX2 SMMUv3 errata workarounds

[Linu Cherian]

On Sat May 06, 2017 at 12:22:50AM +0200, Robert Richter wrote:
> On 05.05.17 17:38:04, Geetha sowjanya wrote:
> > From: Linu Cherian <linu.cherian@cavium.com>
> > 
> > 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 from 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.
> > 
> > Changes from v2:
> >  Updated "Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt" document with 
> >  new SMMU option used to enable errata workaround.
> >  
> > Geetha Sowjanya (1):
> >   iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126
> > 
> > Linu Cherian (6):
> >   iommu/arm-smmu-v3: Introduce smmu option PAGE0_REGS_ONLY for ThunderX2
> >     errata#74.
> >   iommu/arm-smmu-v3: Do resource size checks based on SMMU option
> >     PAGE0_REGS_ONLY
> >   ACPICA: IORT: Add Cavium ThunderX2 SMMUv3 model definition.
> >   iommu/arm-smmu-v3: For ACPI based device probing, set PAGE0_REGS_ONLY
> >     option     for ThunderX2 SMMUv3 implementations.
> >   ACPI/IORT: Fixup SMMUv3 resource size for Cavium ThunderX2 SMMUv3
> >     model
> >   arm64: Documentation: Add Cavium ThunderX2 SMMUv3 erratas
> 
> This split into patches does not look reasonable to me. 1 patch only
> for each workaround should be sufficient.
> 

* Should we not atleast keep the changes in drivers/acpi/iort.c and
 include/acpi/actbl2.h seperate, since they are outside smmuv3 driver ? 

* Probably i can merge the below patches, 
  1. iommu/arm-smmu-v3: Introduce smmu option PAGE0_REGS_ONLY for ThunderX2 
    errata#74.
  2. iommu/arm-smmu-v3: Do resource size checks based on SMMU option
     PAGE0_REGS_O

Is that fine ?

Thanks.
-- 
Linu cherian

Re: [PATCH v3 0/7] Cavium ThunderX2 SMMUv3 errata workarounds

[Robert Richter]

On 08.05.17 20:45:36, Linu Cherian wrote:
> On Sat May 06, 2017 at 12:22:50AM +0200, Robert Richter wrote:
> > On 05.05.17 17:38:04, Geetha sowjanya wrote:
> > > From: Linu Cherian <linu.cherian@cavium.com>
> > > 
> > > 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 from 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.
> > > 
> > > Changes from v2:
> > >  Updated "Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt" document with 
> > >  new SMMU option used to enable errata workaround.
> > >  
> > > Geetha Sowjanya (1):
> > >   iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126
> > > 
> > > Linu Cherian (6):
> > >   iommu/arm-smmu-v3: Introduce smmu option PAGE0_REGS_ONLY for ThunderX2
> > >     errata#74.
> > >   iommu/arm-smmu-v3: Do resource size checks based on SMMU option
> > >     PAGE0_REGS_ONLY
> > >   ACPICA: IORT: Add Cavium ThunderX2 SMMUv3 model definition.
> > >   iommu/arm-smmu-v3: For ACPI based device probing, set PAGE0_REGS_ONLY
> > >     option     for ThunderX2 SMMUv3 implementations.
> > >   ACPI/IORT: Fixup SMMUv3 resource size for Cavium ThunderX2 SMMUv3
> > >     model
> > >   arm64: Documentation: Add Cavium ThunderX2 SMMUv3 erratas
> > 
> > This split into patches does not look reasonable to me. 1 patch only
> > for each workaround should be sufficient.
> > 
> 
> * Should we not atleast keep the changes in drivers/acpi/iort.c and
>  include/acpi/actbl2.h seperate, since they are outside smmuv3 driver ? 

IMO both patches depend each other. You can't shrink the io space
first without adjusting the offset when accessing it. At least the
order should be changed. But since it is one workaround I would prefer
also one patch.

-Robert