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

[PATCH v8 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 v7:
    - Added new function "arm_smmu_v3_resource_size" in iort.c to get resource
      size.
    - Added new SMMU option "SHARED_IRQ" to enable errata #126 workaround.
    - Coding style issues fixed.
    - Suggested changes in arm_smmu_device_probe addressed.
    - Replaced ACPI_IORT_SMMU_CAVIUM_CN99XX macro with ACPI_IORT_SMMU_V3_CAVIUM_CN99XX
   
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                          |   14 +++-
 drivers/iommu/arm-smmu-v3.c                        |   93 ++++++++++++++++----
 4 files changed, 95 insertions(+), 20 deletions(-)

[PATCH v8 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 |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index c5fecf9..c166f3e 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -828,6 +828,18 @@ static int __init arm_smmu_v3_count_resources(struct acpi_iort_node *node)
 	return num_res;
 }
 
+static unsigned long arm_smmu_v3_resource_size(struct acpi_iort_smmu_v3 *smmu)
+{
+	/*
+	 * 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)
+		return SZ_64K;
+
+	return SZ_128K;
+}
+
 static void __init arm_smmu_v3_init_resources(struct resource *res,
 					      struct acpi_iort_node *node)
 {
@@ -838,7 +850,8 @@ static void __init arm_smmu_v3_init_resources(struct resource *res,
 	smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
 
 	res[num_res].start = smmu->base_address;
-	res[num_res].end = smmu->base_address + SZ_128K - 1;
+	res[num_res].end = smmu->base_address +
+				arm_smmu_v3_resource_size(smmu) - 1;
 	res[num_res].flags = IORESOURCE_MEM;
 
 	num_res++;
-- 
1.7.1

[PATCH v8 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                        |   68 ++++++++++++++-----
 3 files changed, 57 insertions(+), 18 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..6ecc48c 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..5de258f 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -597,6 +597,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 +664,20 @@ 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) &&
+	    (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY))
+		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 +1973,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 +2375,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 +2393,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 +2619,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)
 {
@@ -2617,6 +2639,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 +2676,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 (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)
+		return SZ_64K;
+	else
+		return SZ_128K;
+}
+
 static int arm_smmu_device_probe(struct platform_device *pdev)
 {
 	int irq, ret;
@@ -2668,9 +2700,20 @@ 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;
+	}
+
+	/* Set bypass mode according to firmware probing result */
+	bypass = !!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,17 +2740,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;
-
 	/* Probe the h/w */
 	ret = arm_smmu_device_hw_probe(smmu);
 	if (ret)
-- 
1.7.1

[PATCH v8 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.

SHARED_IRQ option is set as a errata workaround, which allows to share the irq
line by register single irq handler for all the interrupts.

Signed-off-by: Geetha sowjanya <gakula@caviumnetworks.com>
---
 .../devicetree/bindings/iommu/arm,smmu-v3.txt      |    5 ++
 drivers/iommu/arm-smmu-v3.c                        |   73 ++++++++++++++++----
 2 files changed, 64 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
index 6ecc48c..44b40e0 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
@@ -55,6 +55,11 @@ the PCIe specification.
 		      Set for Caviun ThunderX2 silicon that doesn't support
 		      SMMU page1 register space.
 
+- cavium,cn9900-broken-unique-irqline
+                    : Use single irq line for all the SMMUv3 interrupts.
+		      Set for Caviun ThunderX2 silicon that doesn't support
+		      MSI and also doesn't have unique irq lines for gerror,
+		      eventq and cmdq-sync.
 ** Example
 
         smmu@2b400000 {
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 2dea4a9..6c0c632 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -598,6 +598,7 @@ struct arm_smmu_device {
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
 #define ARM_SMMU_OPT_PAGE0_REGS_ONLY	(1 << 1)
+#define ARM_SMMU_OPT_SHARED_IRQ	(1 << 2)
 	u32				options;
 
 	struct arm_smmu_cmdq		cmdq;
@@ -665,6 +666,7 @@ 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"},
+	{ ARM_SMMU_OPT_SHARED_IRQ, "cavium,cn9900-broken-unique-irqline"},
 	{ 0, NULL},
 };
 
@@ -1313,6 +1315,21 @@ static irqreturn_t arm_smmu_gerror_handler(int irq, void *dev)
 	writel(gerror, smmu->base + ARM_SMMU_GERRORN);
 	return IRQ_HANDLED;
 }
+/* Shared irq handler*/
+static irqreturn_t arm_smmu_shared_irq_thread(int irq, void *dev)
+{
+	struct arm_smmu_device *smmu = dev;
+	irqreturn_t ret;
+
+	ret = arm_smmu_gerror_handler(irq, dev);
+	if (ret == IRQ_NONE) {
+		arm_smmu_evtq_thread(irq, dev);
+		arm_smmu_cmdq_sync_handler(irq, dev);
+		if (smmu->features & ARM_SMMU_FEAT_PRI)
+			arm_smmu_priq_thread(irq, dev);
+	}
+	return IRQ_HANDLED;
+}
 
 /* IO_PGTABLE API */
 static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
@@ -2230,18 +2247,9 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
 	devm_add_action(dev, arm_smmu_free_msis, dev);
 }
 
-static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
+static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
 {
-	int ret, irq;
-	u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
-
-	/* Disable IRQs first */
-	ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
-				      ARM_SMMU_IRQ_CTRLACK);
-	if (ret) {
-		dev_err(smmu->dev, "failed to disable irqs\n");
-		return ret;
-	}
+	int irq, ret;
 
 	arm_smmu_setup_msis(smmu);
 
@@ -2284,10 +2292,46 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 			if (ret < 0)
 				dev_warn(smmu->dev,
 					 "failed to enable priq irq\n");
-			else
-				irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
 		}
 	}
+}
+
+static void arm_smmu_setup_shared_irqs(struct arm_smmu_device *smmu)
+{
+	int ret, irq;
+
+	/* Single irq is used for all queues, request single interrupt lines */
+	irq = smmu->evtq.q.irq;
+	if (irq) {
+		ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
+					arm_smmu_shared_irq_thread,
+					IRQF_ONESHOT | IRQF_SHARED,
+					"arm-smmu-v3-shared_irq", smmu);
+		if (ret < 0)
+			dev_warn(smmu->dev, "failed to enable shared irq\n");
+	}
+}
+
+static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
+{
+	int ret;
+	u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
+
+	/* Disable IRQs first */
+	ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
+				      ARM_SMMU_IRQ_CTRLACK);
+	if (ret) {
+		dev_err(smmu->dev, "failed to disable irqs\n");
+		return ret;
+	}
+
+	if (smmu->options & ARM_SMMU_OPT_SHARED_IRQ)
+		arm_smmu_setup_shared_irqs(smmu);
+	else
+		arm_smmu_setup_unique_irqs(smmu);
+
+	if (smmu->features & ARM_SMMU_FEAT_PRI)
+		irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
 
 	/* Enable interrupt generation on the SMMU */
 	ret = arm_smmu_write_reg_sync(smmu, irqen_flags,
@@ -2622,7 +2666,8 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
 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;
+		smmu->options |= ARM_SMMU_OPT_PAGE0_REGS_ONLY
+				| ARM_SMMU_OPT_SHARED_IRQ;
 
 	dev_notice(smmu->dev, "option mask 0x%x\n", smmu->options);
 }
-- 
1.7.1

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

[Will Deacon]

On Tue, Jun 20, 2017 at 07:47:39PM +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.
> 
> SHARED_IRQ option is set as a errata workaround, which allows to share the irq
> line by register single irq handler for all the interrupts.
> 
> Signed-off-by: Geetha sowjanya <gakula@caviumnetworks.com>
> ---
>  .../devicetree/bindings/iommu/arm,smmu-v3.txt      |    5 ++
>  drivers/iommu/arm-smmu-v3.c                        |   73 ++++++++++++++++----
>  2 files changed, 64 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> index 6ecc48c..44b40e0 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> @@ -55,6 +55,11 @@ the PCIe specification.
>  		      Set for Caviun ThunderX2 silicon that doesn't support
>  		      SMMU page1 register space.
>  
> +- cavium,cn9900-broken-unique-irqline
> +                    : Use single irq line for all the SMMUv3 interrupts.
> +		      Set for Caviun ThunderX2 silicon that doesn't support
> +		      MSI and also doesn't have unique irq lines for gerror,
> +		      eventq and cmdq-sync.

I think we're better off just supporting a new (optional) named interrupt
as "combined", and then allowing that to be used instead of the others.

>  ** Example
>  
>          smmu@2b400000 {
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 2dea4a9..6c0c632 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -598,6 +598,7 @@ struct arm_smmu_device {
>  
>  #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
>  #define ARM_SMMU_OPT_PAGE0_REGS_ONLY	(1 << 1)
> +#define ARM_SMMU_OPT_SHARED_IRQ	(1 << 2)

Please call this COMBINED instead of SHARED (similarly elsewhere). That
said, not sure we need this.

>  	u32				options;
>  
>  	struct arm_smmu_cmdq		cmdq;
> @@ -665,6 +666,7 @@ 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"},
> +	{ ARM_SMMU_OPT_SHARED_IRQ, "cavium,cn9900-broken-unique-irqline"},
>  	{ 0, NULL},
>  };
>  
> @@ -1313,6 +1315,21 @@ static irqreturn_t arm_smmu_gerror_handler(int irq, void *dev)
>  	writel(gerror, smmu->base + ARM_SMMU_GERRORN);
>  	return IRQ_HANDLED;
>  }
> +/* Shared irq handler*/
> +static irqreturn_t arm_smmu_shared_irq_thread(int irq, void *dev)
> +{
> +	struct arm_smmu_device *smmu = dev;
> +	irqreturn_t ret;
> +
> +	ret = arm_smmu_gerror_handler(irq, dev);
> +	if (ret == IRQ_NONE) {
> +		arm_smmu_evtq_thread(irq, dev);
> +		arm_smmu_cmdq_sync_handler(irq, dev);
> +		if (smmu->features & ARM_SMMU_FEAT_PRI)
> +			arm_smmu_priq_thread(irq, dev);
> +	}
> +	return IRQ_HANDLED;
> +}

This isn't quite right, because you're now running low-level handlers (like
the gerror handler) in threaded context. You're better off registering a
low-level handler too (see below) which can kick gerror and cmdq_sync
before unconditionally returning IRQ_WAKE_THREAD.

>  
>  /* IO_PGTABLE API */
>  static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
> @@ -2230,18 +2247,9 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>  	devm_add_action(dev, arm_smmu_free_msis, dev);
>  }
>  
> -static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
> +static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
>  {
> -	int ret, irq;
> -	u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
> -
> -	/* Disable IRQs first */
> -	ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
> -				      ARM_SMMU_IRQ_CTRLACK);
> -	if (ret) {
> -		dev_err(smmu->dev, "failed to disable irqs\n");
> -		return ret;
> -	}
> +	int irq, ret;
>  
>  	arm_smmu_setup_msis(smmu);
>  
> @@ -2284,10 +2292,46 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>  			if (ret < 0)
>  				dev_warn(smmu->dev,
>  					 "failed to enable priq irq\n");
> -			else
> -				irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
>  		}
>  	}
> +}
> +
> +static void arm_smmu_setup_shared_irqs(struct arm_smmu_device *smmu)
> +{
> +	int ret, irq;
> +
> +	/* Single irq is used for all queues, request single interrupt lines */
> +	irq = smmu->evtq.q.irq;
> +	if (irq) {
> +		ret = devm_request_threaded_irq(smmu->dev, irq, NULL,

As above, stick your low-level handler in instead of NULL here.

> +					arm_smmu_shared_irq_thread,
> +					IRQF_ONESHOT | IRQF_SHARED,

Why do you need IRQF_SHARED here?

> +					"arm-smmu-v3-shared_irq", smmu);

Call this "combined" instead of shared, to avoid confusion with the IRQ
flags.

> +		if (ret < 0)
> +			dev_warn(smmu->dev, "failed to enable shared irq\n");
> +	}
> +}
> +
> +static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
> +{
> +	int ret;
> +	u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
> +
> +	/* Disable IRQs first */
> +	ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
> +				      ARM_SMMU_IRQ_CTRLACK);
> +	if (ret) {
> +		dev_err(smmu->dev, "failed to disable irqs\n");
> +		return ret;
> +	}
> +
> +	if (smmu->options & ARM_SMMU_OPT_SHARED_IRQ)
> +		arm_smmu_setup_shared_irqs(smmu);
> +	else
> +		arm_smmu_setup_unique_irqs(smmu);

I'd rather just have something like:

  irq = platform_get_irq_byname(pdev, "combined");

in the arm_smmu_device_probe function. If we find it's there, we use that
in preference to the other interrupts.

Will

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

[Will Deacon]

On Tue, Jun 20, 2017 at 07:47:37PM +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 |   15 ++++++++++++++-
>  1 files changed, 14 insertions(+), 1 deletions(-)

Looks fine to me, but I need Lorenzo's ack on this.

Will

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

[Will Deacon]

On Tue, Jun 20, 2017 at 07:47:38PM +0530, 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                        |   68 ++++++++++++++-----
>  3 files changed, 57 insertions(+), 18 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..6ecc48c 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

s/Caviun/Cavium/

Will

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

[Lorenzo Pieralisi]

On Tue, Jun 20, 2017 at 07:47:37PM +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 |   15 ++++++++++++++-
>  1 files changed, 14 insertions(+), 1 deletions(-)

Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index c5fecf9..c166f3e 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -828,6 +828,18 @@ static int __init arm_smmu_v3_count_resources(struct acpi_iort_node *node)
>  	return num_res;
>  }
>  
> +static unsigned long arm_smmu_v3_resource_size(struct acpi_iort_smmu_v3 *smmu)
> +{
> +	/*
> +	 * 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)
> +		return SZ_64K;
> +
> +	return SZ_128K;
> +}
> +
>  static void __init arm_smmu_v3_init_resources(struct resource *res,
>  					      struct acpi_iort_node *node)
>  {
> @@ -838,7 +850,8 @@ static void __init arm_smmu_v3_init_resources(struct resource *res,
>  	smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
>  
>  	res[num_res].start = smmu->base_address;
> -	res[num_res].end = smmu->base_address + SZ_128K - 1;
> +	res[num_res].end = smmu->base_address +
> +				arm_smmu_v3_resource_size(smmu) - 1;
>  	res[num_res].flags = IORESOURCE_MEM;
>  
>  	num_res++;
> -- 
> 1.7.1
> 

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

[Geetha Akula]

Hi Will,

On Tue, Jun 20, 2017 at 11:30 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jun 20, 2017 at 07:47:39PM +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.
>>
>> SHARED_IRQ option is set as a errata workaround, which allows to share the irq
>> line by register single irq handler for all the interrupts.
>>
>> Signed-off-by: Geetha sowjanya <gakula@caviumnetworks.com>
>> ---
>>  .../devicetree/bindings/iommu/arm,smmu-v3.txt      |    5 ++
>>  drivers/iommu/arm-smmu-v3.c                        |   73 ++++++++++++++++----
>>  2 files changed, 64 insertions(+), 14 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>> index 6ecc48c..44b40e0 100644
>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>> @@ -55,6 +55,11 @@ the PCIe specification.
>>                     Set for Caviun ThunderX2 silicon that doesn't support
>>                     SMMU page1 register space.
>>
>> +- cavium,cn9900-broken-unique-irqline
>> +                    : Use single irq line for all the SMMUv3 interrupts.
>> +                   Set for Caviun ThunderX2 silicon that doesn't support
>> +                   MSI and also doesn't have unique irq lines for gerror,
>> +                   eventq and cmdq-sync.
>
> I think we're better off just supporting a new (optional) named interrupt
> as "combined", and then allowing that to be used instead of the others.

Are you suggesting to have new name irq "combined" like gerror ?
If yes, then this won't be possible with apci. We need to update iort spec to
add new name irq.
>
>>  ** Example
>>
>>          smmu@2b400000 {
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 2dea4a9..6c0c632 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -598,6 +598,7 @@ struct arm_smmu_device {
>>
>>  #define ARM_SMMU_OPT_SKIP_PREFETCH   (1 << 0)
>>  #define ARM_SMMU_OPT_PAGE0_REGS_ONLY (1 << 1)
>> +#define ARM_SMMU_OPT_SHARED_IRQ      (1 << 2)
>
> Please call this COMBINED instead of SHARED (similarly elsewhere). That
> said, not sure we need this.
>
>>       u32                             options;
>>
>>       struct arm_smmu_cmdq            cmdq;
>> @@ -665,6 +666,7 @@ 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"},
>> +     { ARM_SMMU_OPT_SHARED_IRQ, "cavium,cn9900-broken-unique-irqline"},
>>       { 0, NULL},
>>  };
>>
>> @@ -1313,6 +1315,21 @@ static irqreturn_t arm_smmu_gerror_handler(int irq, void *dev)
>>       writel(gerror, smmu->base + ARM_SMMU_GERRORN);
>>       return IRQ_HANDLED;
>>  }
>> +/* Shared irq handler*/
>> +static irqreturn_t arm_smmu_shared_irq_thread(int irq, void *dev)
>> +{
>> +     struct arm_smmu_device *smmu = dev;
>> +     irqreturn_t ret;
>> +
>> +     ret = arm_smmu_gerror_handler(irq, dev);
>> +     if (ret == IRQ_NONE) {
>> +             arm_smmu_evtq_thread(irq, dev);
>> +             arm_smmu_cmdq_sync_handler(irq, dev);
>> +             if (smmu->features & ARM_SMMU_FEAT_PRI)
>> +                     arm_smmu_priq_thread(irq, dev);
>> +     }
>> +     return IRQ_HANDLED;
>> +}
>
> This isn't quite right, because you're now running low-level handlers (like
> the gerror handler) in threaded context. You're better off registering a
> low-level handler too (see below) which can kick gerror and cmdq_sync
> before unconditionally returning IRQ_WAKE_THREAD.

+static irqreturn_t arm_smmu_combined_irq_thread(int irq, void *dev)
+{
+       struct arm_smmu_device *smmu = dev;
+
+       arm_smmu_evtq_thread(irq, dev);
+       if (smmu->features & ARM_SMMU_FEAT_PRI)
+               arm_smmu_priq_thread(irq, dev);
+
+       return IRQ_HANDLED;
+}
+
+static irqreturn_t arm_smmu_combined_irq_handler(int irq, void *dev)
+{
+       irqreturn_t ret;
+
+       ret = arm_smmu_gerror_handler(irq, dev);
+       if (ret == IRQ_NONE) {
+               arm_smmu_cmdq_sync_handler(irq, dev);
+               return IRQ_WAKE_THREAD;
+       }
+       else
+               return ret;
+}

>
>>
>>  /* IO_PGTABLE API */
>>  static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
>> @@ -2230,18 +2247,9 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>>       devm_add_action(dev, arm_smmu_free_msis, dev);
>>  }
>>
>> -static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>> +static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
>>  {
>> -     int ret, irq;
>> -     u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
>> -
>> -     /* Disable IRQs first */
>> -     ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
>> -                                   ARM_SMMU_IRQ_CTRLACK);
>> -     if (ret) {
>> -             dev_err(smmu->dev, "failed to disable irqs\n");
>> -             return ret;
>> -     }
>> +     int irq, ret;
>>
>>       arm_smmu_setup_msis(smmu);
>>
>> @@ -2284,10 +2292,46 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>>                       if (ret < 0)
>>                               dev_warn(smmu->dev,
>>                                        "failed to enable priq irq\n");
>> -                     else
>> -                             irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
>>               }
>>       }
>> +}
>> +
>> +static void arm_smmu_setup_shared_irqs(struct arm_smmu_device *smmu)
>> +{
>> +     int ret, irq;
>> +
>> +     /* Single irq is used for all queues, request single interrupt lines */
>> +     irq = smmu->evtq.q.irq;
>> +     if (irq) {
>> +             ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
>
> As above, stick your low-level handler in instead of NULL here.
>
>> +                                     arm_smmu_shared_irq_thread,
>> +                                     IRQF_ONESHOT | IRQF_SHARED,
>
> Why do you need IRQF_SHARED here?


+devm_request_threaded_irq(smmu->dev, irq,
+                                       arm_smmu_combined_irq_handler,
+                                       arm_smmu_combined_irq_thread,
+                                       IRQF_SHARED,
+                                       "arm-smmu-v3-combined-irq", smmu);

On multi-node system, node1 SMMU's share irq lines with node0 SMMU's.

>
>> +                                     "arm-smmu-v3-shared_irq", smmu);
>
> Call this "combined" instead of shared, to avoid confusion with the IRQ
> flags.
>
>> +             if (ret < 0)
>> +                     dev_warn(smmu->dev, "failed to enable shared irq\n");
>> +     }
>> +}
>> +
>> +static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>> +{
>> +     int ret;
>> +     u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
>> +
>> +     /* Disable IRQs first */
>> +     ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
>> +                                   ARM_SMMU_IRQ_CTRLACK);
>> +     if (ret) {
>> +             dev_err(smmu->dev, "failed to disable irqs\n");
>> +             return ret;
>> +     }
>> +
>> +     if (smmu->options & ARM_SMMU_OPT_SHARED_IRQ)
>> +             arm_smmu_setup_shared_irqs(smmu);
>> +     else
>> +             arm_smmu_setup_unique_irqs(smmu);
>
> I'd rather just have something like:
>
>   irq = platform_get_irq_byname(pdev, "combined");
>
> in the arm_smmu_device_probe function. If we find it's there, we use that
> in preference to the other interrupts.
>
> Will

Thanks,
Geetha.

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

[Will Deacon]

Hi Geetha,

On Wed, Jun 21, 2017 at 12:09:45PM +0530, Geetha Akula wrote:
> On Tue, Jun 20, 2017 at 11:30 PM, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Jun 20, 2017 at 07:47:39PM +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.
> >>
> >> SHARED_IRQ option is set as a errata workaround, which allows to share the irq
> >> line by register single irq handler for all the interrupts.
> >>
> >> Signed-off-by: Geetha sowjanya <gakula@caviumnetworks.com>
> >> ---
> >>  .../devicetree/bindings/iommu/arm,smmu-v3.txt      |    5 ++
> >>  drivers/iommu/arm-smmu-v3.c                        |   73 ++++++++++++++++----
> >>  2 files changed, 64 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> >> index 6ecc48c..44b40e0 100644
> >> --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> >> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
> >> @@ -55,6 +55,11 @@ the PCIe specification.
> >>                     Set for Caviun ThunderX2 silicon that doesn't support
> >>                     SMMU page1 register space.
> >>
> >> +- cavium,cn9900-broken-unique-irqline
> >> +                    : Use single irq line for all the SMMUv3 interrupts.
> >> +                   Set for Caviun ThunderX2 silicon that doesn't support
> >> +                   MSI and also doesn't have unique irq lines for gerror,
> >> +                   eventq and cmdq-sync.
> >
> > I think we're better off just supporting a new (optional) named interrupt
> > as "combined", and then allowing that to be used instead of the others.
> 
> Are you suggesting to have new name irq "combined" like gerror ?
> If yes, then this won't be possible with apci. We need to update iort spec to
> add new name irq.

I'm mainly talking about the DT binding here, but I don't see why you
can't hack drivers/acpi/arm64/iort.c like you did for the other erratum and
have it register a single interrupt called "combined" based on the model
number.

> >> +                                     arm_smmu_shared_irq_thread,
> >> +                                     IRQF_ONESHOT | IRQF_SHARED,
> >
> > Why do you need IRQF_SHARED here?
> 
> 
> +devm_request_threaded_irq(smmu->dev, irq,
> +                                       arm_smmu_combined_irq_handler,
> +                                       arm_smmu_combined_irq_thread,
> +                                       IRQF_SHARED,
> +                                       "arm-smmu-v3-combined-irq", smmu);
> 
> On multi-node system, node1 SMMU's share irq lines with node0 SMMU's.

How does that work? Are these really MSIs under the hood? If so, why didn't
you just build them as... MSIs?

I sincerely hope that you never want to support paging of DMA memory on this
platform. It will run like a dog.

Will

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

[Marc Zyngier]

On 21/06/17 10:08, Will Deacon wrote:
> Hi Geetha,
> 
> On Wed, Jun 21, 2017 at 12:09:45PM +0530, Geetha Akula wrote:
>> On Tue, Jun 20, 2017 at 11:30 PM, Will Deacon <will.deacon@arm.com> wrote:
>>> On Tue, Jun 20, 2017 at 07:47:39PM +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.
>>>>
>>>> SHARED_IRQ option is set as a errata workaround, which allows to share the irq
>>>> line by register single irq handler for all the interrupts.
>>>>
>>>> Signed-off-by: Geetha sowjanya <gakula@caviumnetworks.com>
>>>> ---
>>>>  .../devicetree/bindings/iommu/arm,smmu-v3.txt      |    5 ++
>>>>  drivers/iommu/arm-smmu-v3.c                        |   73 ++++++++++++++++----
>>>>  2 files changed, 64 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>>>> index 6ecc48c..44b40e0 100644
>>>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>>>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>>>> @@ -55,6 +55,11 @@ the PCIe specification.
>>>>                     Set for Caviun ThunderX2 silicon that doesn't support
>>>>                     SMMU page1 register space.
>>>>
>>>> +- cavium,cn9900-broken-unique-irqline
>>>> +                    : Use single irq line for all the SMMUv3 interrupts.
>>>> +                   Set for Caviun ThunderX2 silicon that doesn't support
>>>> +                   MSI and also doesn't have unique irq lines for gerror,
>>>> +                   eventq and cmdq-sync.
>>>
>>> I think we're better off just supporting a new (optional) named interrupt
>>> as "combined", and then allowing that to be used instead of the others.
>>
>> Are you suggesting to have new name irq "combined" like gerror ?
>> If yes, then this won't be possible with apci. We need to update iort spec to
>> add new name irq.
> 
> I'm mainly talking about the DT binding here, but I don't see why you
> can't hack drivers/acpi/arm64/iort.c like you did for the other erratum and
> have it register a single interrupt called "combined" based on the model
> number.
> 
>>>> +                                     arm_smmu_shared_irq_thread,
>>>> +                                     IRQF_ONESHOT | IRQF_SHARED,
>>>
>>> Why do you need IRQF_SHARED here?
>>
>>
>> +devm_request_threaded_irq(smmu->dev, irq,
>> +                                       arm_smmu_combined_irq_handler,
>> +                                       arm_smmu_combined_irq_thread,
>> +                                       IRQF_SHARED,
>> +                                       "arm-smmu-v3-combined-irq", smmu);
>>
>> On multi-node system, node1 SMMU's share irq lines with node0 SMMU's.
> 
> How does that work? Are these really MSIs under the hood? If so, why didn't
> you just build them as... MSIs?

More specifically, I suspect that they are made out of message-signalled
SPIs, targeting the GIC distributor directly... That's the only way I
can imagine it has been built... If I'm right, we probably have the
firmware programming the same SPI number in both nodes.

But of course, that's pure speculation.

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Robert Richter]

On 20.06.17 19:47:39, 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.
> 
> SHARED_IRQ option is set as a errata workaround, which allows to share the irq
> line by register single irq handler for all the interrupts.

I found the entry in silicon-errata.txt missing, is that on purpose?

-Robert

> 
> Signed-off-by: Geetha sowjanya <gakula@caviumnetworks.com>
> ---
>  .../devicetree/bindings/iommu/arm,smmu-v3.txt      |    5 ++
>  drivers/iommu/arm-smmu-v3.c                        |   73 ++++++++++++++++----
>  2 files changed, 64 insertions(+), 14 deletions(-)