[PATCH 0/6] iommu/arm-smmu: Misc changes

[PATCH 0/6] iommu/arm-smmu: Misc changes

[Andreas Herrmann]

Hi,

here a reworked set of patches. Hopefully all comments addressed.

I omitted the patch "ARM: dma-mapping: Always pass proper prot flags
to iommu_map()" as it is pushed by Marek to the dma-mapping fixes
branch.

I also omitted the device isolation patch (have to think about your
comment(s) and working on a bus notifier version) and ECX-2000 dts
change.


Regards,

Andreas

[PATCH 1/6] iommu/arm-smmu: Switch to subsys_initcall for driver registration

[Andreas Herrmann]

This should ensure that arm-smmu is initialized before other drivers
start handling devices that propably need smmu support.

Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 drivers/iommu/arm-smmu.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 181c9ba..548f5d1 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1981,7 +1981,7 @@ static void __exit arm_smmu_exit(void)
 	return platform_driver_unregister(&arm_smmu_driver);
 }
 
-module_init(arm_smmu_init);
+subsys_initcall(arm_smmu_init);
 module_exit(arm_smmu_exit);
 
 MODULE_DESCRIPTION("IOMMU API for ARM architected SMMU implementations");
-- 
1.7.9.5

[PATCH 2/6] iommu/arm-smmu: Refine check for proper size of mapped region

[Andreas Herrmann]

There is already a check to print a warning if the size of SMMU
address space (calculated from SMMU register values) is greater than
the size of the mapped memory region (e.g. passed via DT to the
driver).

Adapt this check to print also a warning in case the mapped region is
larger than the SMMU address space.

Such a mismatch could be intentional (to fix wrong register values).
If its not intentional (e.g. due to wrong DT information) this will
very likely cause a malfunction of the driver as SMMU_CB_BASE is
derived from the size of the mapped region. The warning helps to
identify the root cause in this case.

Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 drivers/iommu/arm-smmu.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 548f5d1..d19676c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1700,13 +1700,12 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 	id = readl_relaxed(gr0_base + ARM_SMMU_GR0_ID1);
 	smmu->pagesize = (id & ID1_PAGESIZE) ? SZ_64K : SZ_4K;
 
-	/* Check that we ioremapped enough */
+	/* Check for size mismatch of SMMU address space from mapped region */
 	size = 1 << (((id >> ID1_NUMPAGENDXB_SHIFT) & ID1_NUMPAGENDXB_MASK) + 1);
 	size *= (smmu->pagesize << 1);
-	if (smmu->size < size)
-		dev_warn(smmu->dev,
-			 "device is 0x%lx bytes but only mapped 0x%lx!\n",
-			 size, smmu->size);
+	if (smmu->size != size)
+		dev_warn(smmu->dev, "SMMU address space size (0x%lx) differs "
+			"from mapped region size (0x%lx)!\n", size, smmu->size);
 
 	smmu->num_s2_context_banks = (id >> ID1_NUMS2CB_SHIFT) &
 				      ID1_NUMS2CB_MASK;
-- 
1.7.9.5

[PATCH 3/6] iommu/arm-smmu: Check for num_context_irqs > 0 to avoid divide by zero exception

[Andreas Herrmann]

With the right (or wrong;-) definition of v1 SMMU node in DTB it is
possible to trigger a division by zero in arm_smmu_init_domain_context
(if number of context irqs is 0):

       if (smmu->version == 1) {
               root_cfg->irptndx = atomic_inc_return(&smmu->irptndx);
 =>            root_cfg->irptndx %= smmu->num_context_irqs;
       } else {

Avoid this by checking for num_context_irqs > 0 when probing
for SMMU devices.

Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 drivers/iommu/arm-smmu.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index d19676c..7d07561 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1803,12 +1803,11 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 			smmu->num_context_irqs++;
 	}
 
-	if (num_irqs < smmu->num_global_irqs) {
+	if (!smmu->num_context_irqs) {
 		dev_warn(dev, "found %d interrupts but expected at least %d\n",
-			 num_irqs, smmu->num_global_irqs);
-		smmu->num_global_irqs = num_irqs;
+			 num_irqs, smmu->num_global_irqs + 1);
+		return -ENODEV;
 	}
-	smmu->num_context_irqs = num_irqs - smmu->num_global_irqs;
 
 	smmu->irqs = devm_kzalloc(dev, sizeof(*smmu->irqs) * num_irqs,
 				  GFP_KERNEL);
-- 
1.7.9.5

[PATCH 4/6] iommu/arm-smmu: Print context fault information

[Andreas Herrmann]

Print context fault information when the fault was not handled by
report_iommu_fault.

Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 drivers/iommu/arm-smmu.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 7d07561..579b6f8 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -590,6 +590,9 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
 		ret = IRQ_HANDLED;
 		resume = RESUME_RETRY;
 	} else {
+		dev_err_ratelimited(smmu->dev, "context fault: iova=0x%08lx, "
+				"fsynr=0x%x, cb=%d\n",
+				iova, fsynr, root_cfg->cbndx);
 		ret = IRQ_NONE;
 		resume = RESUME_TERMINATE;
 	}
-- 
1.7.9.5

[PATCH 5/6] iommu/arm-smmu: Clear global and context bank fault status registers

[Andreas Herrmann]

After reset these registers have unknown values.
This might cause problems when evaluating SMMU_GFSR and/or SMMU_CB_FSR
in handlers for combined interrupts.

Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 drivers/iommu/arm-smmu.c |   31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 579b6f8..b632bcd 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1562,9 +1562,13 @@ static struct iommu_ops arm_smmu_ops = {
 static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 {
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
-	void __iomem *sctlr_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB_SCTLR;
+	void __iomem *cb_base;
 	int i = 0;
-	u32 scr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
+	u32 reg;
+
+	/* clear global FSR */
+	reg = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSR);
+	writel(reg, gr0_base + ARM_SMMU_GR0_sGFSR);
 
 	/* Mark all SMRn as invalid and all S2CRn as bypass */
 	for (i = 0; i < smmu->num_mapping_groups; ++i) {
@@ -1572,33 +1576,38 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 		writel_relaxed(S2CR_TYPE_BYPASS, gr0_base + ARM_SMMU_GR0_S2CR(i));
 	}
 
-	/* Make sure all context banks are disabled */
-	for (i = 0; i < smmu->num_context_banks; ++i)
-		writel_relaxed(0, sctlr_base + ARM_SMMU_CB(smmu, i));
+	/* Make sure all context banks are disabled and clear CB_FSR  */
+	for (i = 0; i < smmu->num_context_banks; ++i) {
+		cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, i);
+		writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
+		writel_relaxed(FSR_FAULT, cb_base + ARM_SMMU_CB_FSR);
+	}
 
 	/* Invalidate the TLB, just in case */
 	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_STLBIALL);
 	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH);
 	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH);
 
+	reg = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
+
 	/* Enable fault reporting */
-	scr0 |= (sCR0_GFRE | sCR0_GFIE | sCR0_GCFGFRE | sCR0_GCFGFIE);
+	reg |= (sCR0_GFRE | sCR0_GFIE | sCR0_GCFGFRE | sCR0_GCFGFIE);
 
 	/* Disable TLB broadcasting. */
-	scr0 |= (sCR0_VMIDPNE | sCR0_PTM);
+	reg |= (sCR0_VMIDPNE | sCR0_PTM);
 
 	/* Enable client access, but bypass when no mapping is found */
-	scr0 &= ~(sCR0_CLIENTPD | sCR0_USFCFG);
+	reg &= ~(sCR0_CLIENTPD | sCR0_USFCFG);
 
 	/* Disable forced broadcasting */
-	scr0 &= ~sCR0_FB;
+	reg &= ~sCR0_FB;
 
 	/* Don't upgrade barriers */
-	scr0 &= ~(sCR0_BSU_MASK << sCR0_BSU_SHIFT);
+	reg &= ~(sCR0_BSU_MASK << sCR0_BSU_SHIFT);
 
 	/* Push the button */
 	arm_smmu_tlb_sync(smmu);
-	writel(scr0, gr0_base + ARM_SMMU_GR0_sCR0);
+	writel(reg, gr0_base + ARM_SMMU_GR0_sCR0);
 }
 
 static int arm_smmu_id_size_to_bits(int size)
-- 
1.7.9.5

[PATCH 6/6] iommu/arm-smmu: Support buggy implemenations where all config accesses are secure

[Andreas Herrmann]

In such a case we have to use secure aliases of some non-secure
registers.

This behaviour is controlled via a flag in smmu->bugs. It is set
based on DT information when probing an SMMU device.

Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
 drivers/iommu/arm-smmu.c |   33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index b632bcd..46f2b78 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -60,6 +60,15 @@
 #define ARM_SMMU_GR0(smmu)		((smmu)->base)
 #define ARM_SMMU_GR1(smmu)		((smmu)->base + (smmu)->pagesize)
 
+/*
+ * SMMU global address space with conditional offset to access secure aliases of
+ * non-secure registers (e.g. nsCR0: 0x400, nsGFSR: 0x448, nsGFSYNR0: 0x450)
+ */
+#define ARM_SMMU_GR0_NS(smmu)						\
+	((smmu)->base +							\
+		((smmu->bugs & ARM_SMMU_BUG_SECURE_CFG_ACCESS) ? 0x400 : 0))
+
+
 /* Page table bits */
 #define ARM_SMMU_PTE_PAGE		(((pteval_t)3) << 0)
 #define ARM_SMMU_PTE_CONT		(((pteval_t)1) << 52)
@@ -348,6 +357,8 @@ struct arm_smmu_device {
 #define ARM_SMMU_FEAT_TRANS_S2		(1 << 3)
 #define ARM_SMMU_FEAT_TRANS_NESTED	(1 << 4)
 	u32				features;
+#define ARM_SMMU_BUG_SECURE_CFG_ACCESS	(1 << 0)
+	u32				bugs;
 	int				version;
 
 	u32				num_context_banks;
@@ -611,16 +622,16 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
 {
 	u32 gfsr, gfsynr0, gfsynr1, gfsynr2;
 	struct arm_smmu_device *smmu = dev;
-	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
+	void __iomem *gr0_base = ARM_SMMU_GR0_NS(smmu);
 
 	gfsr = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSR);
-	if (!gfsr)
-		return IRQ_NONE;
-
 	gfsynr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSYNR0);
 	gfsynr1 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSYNR1);
 	gfsynr2 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSYNR2);
 
+	if (!gfsr)
+		return IRQ_NONE;
+
 	dev_err_ratelimited(smmu->dev,
 		"Unexpected global fault, this could be serious\n");
 	dev_err_ratelimited(smmu->dev,
@@ -1567,8 +1578,8 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	u32 reg;
 
 	/* clear global FSR */
-	reg = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSR);
-	writel(reg, gr0_base + ARM_SMMU_GR0_sGFSR);
+	reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sGFSR);
+	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sGFSR);
 
 	/* Mark all SMRn as invalid and all S2CRn as bypass */
 	for (i = 0; i < smmu->num_mapping_groups; ++i) {
@@ -1588,7 +1599,7 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH);
 	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH);
 
-	reg = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
+	reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
 
 	/* Enable fault reporting */
 	reg |= (sCR0_GFRE | sCR0_GFIE | sCR0_GCFGFRE | sCR0_GCFGFIE);
@@ -1607,7 +1618,7 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 
 	/* Push the button */
 	arm_smmu_tlb_sync(smmu);
-	writel(reg, gr0_base + ARM_SMMU_GR0_sCR0);
+	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
 }
 
 static int arm_smmu_id_size_to_bits(int size)
@@ -1881,6 +1892,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (of_property_read_bool(dev->of_node, "calxeda,smmu-secure-cfg-access"))
+		smmu->bugs |= ARM_SMMU_BUG_SECURE_CFG_ACCESS;
+
 	INIT_LIST_HEAD(&smmu->list);
 	spin_lock(&arm_smmu_devices_lock);
 	list_add(&smmu->list, &arm_smmu_devices);
@@ -1943,7 +1957,8 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
 		free_irq(smmu->irqs[i], smmu);
 
 	/* Turn the thing off */
-	writel(sCR0_CLIENTPD, ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_sCR0);
+	writel(sCR0_CLIENTPD,
+		ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH 6/6] iommu/arm-smmu: Support buggy implemenations where all config accesses are secure

[Rob Herring]

On 10/01/2013 07:39 AM, Andreas Herrmann wrote:
> In such a case we have to use secure aliases of some non-secure
> registers.
> 
> This behaviour is controlled via a flag in smmu->bugs. It is set
> based on DT information when probing an SMMU device.

Perhaps quirks would be a nicer word...

> 
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>  drivers/iommu/arm-smmu.c |   33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index b632bcd..46f2b78 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -60,6 +60,15 @@
>  #define ARM_SMMU_GR0(smmu)		((smmu)->base)
>  #define ARM_SMMU_GR1(smmu)		((smmu)->base + (smmu)->pagesize)
>  
> +/*
> + * SMMU global address space with conditional offset to access secure aliases of
> + * non-secure registers (e.g. nsCR0: 0x400, nsGFSR: 0x448, nsGFSYNR0: 0x450)
> + */
> +#define ARM_SMMU_GR0_NS(smmu)						\
> +	((smmu)->base +							\
> +		((smmu->bugs & ARM_SMMU_BUG_SECURE_CFG_ACCESS) ? 0x400 : 0))
> +
> +
>  /* Page table bits */
>  #define ARM_SMMU_PTE_PAGE		(((pteval_t)3) << 0)
>  #define ARM_SMMU_PTE_CONT		(((pteval_t)1) << 52)
> @@ -348,6 +357,8 @@ struct arm_smmu_device {
>  #define ARM_SMMU_FEAT_TRANS_S2		(1 << 3)
>  #define ARM_SMMU_FEAT_TRANS_NESTED	(1 << 4)
>  	u32				features;
> +#define ARM_SMMU_BUG_SECURE_CFG_ACCESS	(1 << 0)
> +	u32				bugs;
>  	int				version;
>  
>  	u32				num_context_banks;
> @@ -611,16 +622,16 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
>  {
>  	u32 gfsr, gfsynr0, gfsynr1, gfsynr2;
>  	struct arm_smmu_device *smmu = dev;
> -	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
> +	void __iomem *gr0_base = ARM_SMMU_GR0_NS(smmu);
>  
>  	gfsr = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSR);
> -	if (!gfsr)
> -		return IRQ_NONE;
> -
>  	gfsynr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSYNR0);
>  	gfsynr1 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSYNR1);
>  	gfsynr2 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSYNR2);
>  
> +	if (!gfsr)
> +		return IRQ_NONE;
> +
>  	dev_err_ratelimited(smmu->dev,
>  		"Unexpected global fault, this could be serious\n");
>  	dev_err_ratelimited(smmu->dev,
> @@ -1567,8 +1578,8 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>  	u32 reg;
>  
>  	/* clear global FSR */
> -	reg = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSR);
> -	writel(reg, gr0_base + ARM_SMMU_GR0_sGFSR);
> +	reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sGFSR);
> +	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sGFSR);
>  
>  	/* Mark all SMRn as invalid and all S2CRn as bypass */
>  	for (i = 0; i < smmu->num_mapping_groups; ++i) {
> @@ -1588,7 +1599,7 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>  	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH);
>  	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH);
>  
> -	reg = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
> +	reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
>  
>  	/* Enable fault reporting */
>  	reg |= (sCR0_GFRE | sCR0_GFIE | sCR0_GCFGFRE | sCR0_GCFGFIE);
> @@ -1607,7 +1618,7 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>  
>  	/* Push the button */
>  	arm_smmu_tlb_sync(smmu);
> -	writel(reg, gr0_base + ARM_SMMU_GR0_sCR0);
> +	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
>  }
>  
>  static int arm_smmu_id_size_to_bits(int size)
> @@ -1881,6 +1892,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	if (of_property_read_bool(dev->of_node, "calxeda,smmu-secure-cfg-access"))

This needs to be documented.

Rob

[PATCH 6/6] iommu/arm-smmu: Support buggy implemenations where all config accesses are secure

[Andreas Herrmann]

On Tue, Oct 01, 2013 at 09:28:51AM -0400, Rob Herring wrote:
> On 10/01/2013 07:39 AM, Andreas Herrmann wrote:
> > In such a case we have to use secure aliases of some non-secure
> > registers.
> > 
> > This behaviour is controlled via a flag in smmu->bugs. It is set
> > based on DT information when probing an SMMU device.
> 
> Perhaps quirks would be a nicer word...

In fact it might be better to rename this as "options".
For device isolation I had added a flag to indicate this in
smmu->features.
But it might be better to not mix hardware features with software
features or options.

So what do you (Will and Rob) think about a change like

 struct arm_smmu_device {
   ...

 #define ARM_SMMU_FEAT_COHERENT_WALK     (1 << 0)
 #define ARM_SMMU_FEAT_STREAM_MATCH      (1 << 1)
 #define ARM_SMMU_FEAT_TRANS_S1          (1 << 2)
 #define ARM_SMMU_FEAT_TRANS_S2          (1 << 3)
 #define ARM_SMMU_FEAT_TRANS_NESTED      (1 << 4)
         u32                             features;
+ #define ARM_SMMU_OPT_SECURE_CFG_ACCESS  (1 << 0)
+ #define ARM_SMMU_OPT_ISOLATE_DEVICES    (1 << 1)
+        u32                             options;
         int                             version;
   ...
 }

DT property "calxeda,smmu-secure-cfg-access" would set
ARM_SMMU_OPT_SECURE_CFG_ACCESS and
DT property "linux,arm-smmu-isolate-devices" would set
ARM_SMMU_OPT_ISOLATE_DEVICES.

(Maybe in future there will be more driver options. )

> > Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> > ---
> >  drivers/iommu/arm-smmu.c |   33 ++++++++++++++++++++++++---------
> >  1 file changed, 24 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index b632bcd..46f2b78 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -60,6 +60,15 @@
> >  #define ARM_SMMU_GR0(smmu)		((smmu)->base)
> >  #define ARM_SMMU_GR1(smmu)		((smmu)->base + (smmu)->pagesize)
> >  
> > +/*
> > + * SMMU global address space with conditional offset to access secure aliases of
> > + * non-secure registers (e.g. nsCR0: 0x400, nsGFSR: 0x448, nsGFSYNR0: 0x450)
> > + */
> > +#define ARM_SMMU_GR0_NS(smmu)						\
> > +	((smmu)->base +							\
> > +		((smmu->bugs & ARM_SMMU_BUG_SECURE_CFG_ACCESS) ? 0x400 : 0))
> > +
> > +
> >  /* Page table bits */
> >  #define ARM_SMMU_PTE_PAGE		(((pteval_t)3) << 0)
> >  #define ARM_SMMU_PTE_CONT		(((pteval_t)1) << 52)
> > @@ -348,6 +357,8 @@ struct arm_smmu_device {
> >  #define ARM_SMMU_FEAT_TRANS_S2		(1 << 3)
> >  #define ARM_SMMU_FEAT_TRANS_NESTED	(1 << 4)
> >  	u32				features;
> > +#define ARM_SMMU_BUG_SECURE_CFG_ACCESS	(1 << 0)
> > +	u32				bugs;
> >  	int				version;
> >  
> >  	u32				num_context_banks;
> > @@ -611,16 +622,16 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
> >  {
> >  	u32 gfsr, gfsynr0, gfsynr1, gfsynr2;
> >  	struct arm_smmu_device *smmu = dev;
> > -	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
> > +	void __iomem *gr0_base = ARM_SMMU_GR0_NS(smmu);
> >  
> >  	gfsr = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSR);
> > -	if (!gfsr)
> > -		return IRQ_NONE;
> > -
> >  	gfsynr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSYNR0);
> >  	gfsynr1 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSYNR1);
> >  	gfsynr2 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSYNR2);
> >  
> > +	if (!gfsr)
> > +		return IRQ_NONE;
> > +
> >  	dev_err_ratelimited(smmu->dev,
> >  		"Unexpected global fault, this could be serious\n");
> >  	dev_err_ratelimited(smmu->dev,
> > @@ -1567,8 +1578,8 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> >  	u32 reg;
> >  
> >  	/* clear global FSR */
> > -	reg = readl_relaxed(gr0_base + ARM_SMMU_GR0_sGFSR);
> > -	writel(reg, gr0_base + ARM_SMMU_GR0_sGFSR);
> > +	reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sGFSR);
> > +	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sGFSR);
> >  
> >  	/* Mark all SMRn as invalid and all S2CRn as bypass */
> >  	for (i = 0; i < smmu->num_mapping_groups; ++i) {
> > @@ -1588,7 +1599,7 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> >  	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH);
> >  	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH);
> >  
> > -	reg = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
> > +	reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> >  
> >  	/* Enable fault reporting */
> >  	reg |= (sCR0_GFRE | sCR0_GFIE | sCR0_GCFGFRE | sCR0_GCFGFIE);
> > @@ -1607,7 +1618,7 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> >  
> >  	/* Push the button */
> >  	arm_smmu_tlb_sync(smmu);
> > -	writel(reg, gr0_base + ARM_SMMU_GR0_sCR0);
> > +	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> >  }
> >  
> >  static int arm_smmu_id_size_to_bits(int size)
> > @@ -1881,6 +1892,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
> >  		}
> >  	}
> >  
> > +	if (of_property_read_bool(dev->of_node, "calxeda,smmu-secure-cfg-access"))
> 
> This needs to be documented.

Yes, will add it in a next version of the patch.

> Rob

Andreas

[PATCH 6/6] iommu/arm-smmu: Support buggy implemenations where all config accesses are secure

[Will Deacon]

On Tue, Oct 01, 2013 at 07:17:09PM +0100, Andreas Herrmann wrote:
> On Tue, Oct 01, 2013 at 09:28:51AM -0400, Rob Herring wrote:
> > On 10/01/2013 07:39 AM, Andreas Herrmann wrote:
> > > In such a case we have to use secure aliases of some non-secure
> > > registers.
> > > 
> > > This behaviour is controlled via a flag in smmu->bugs. It is set
> > > based on DT information when probing an SMMU device.
> > 
> > Perhaps quirks would be a nicer word...
> 
> In fact it might be better to rename this as "options".
> For device isolation I had added a flag to indicate this in
> smmu->features.
> But it might be better to not mix hardware features with software
> features or options.

Yes, I also didn't like the mixing of hardware and software features, but I
couldn't think of a good way to separate them out. Sounds like you might be
on to something.

> So what do you (Will and Rob) think about a change like
> 
>  struct arm_smmu_device {
>    ...
> 
>  #define ARM_SMMU_FEAT_COHERENT_WALK     (1 << 0)
>  #define ARM_SMMU_FEAT_STREAM_MATCH      (1 << 1)
>  #define ARM_SMMU_FEAT_TRANS_S1          (1 << 2)
>  #define ARM_SMMU_FEAT_TRANS_S2          (1 << 3)
>  #define ARM_SMMU_FEAT_TRANS_NESTED      (1 << 4)
>          u32                             features;
> + #define ARM_SMMU_OPT_SECURE_CFG_ACCESS  (1 << 0)
> + #define ARM_SMMU_OPT_ISOLATE_DEVICES    (1 << 1)
> +        u32                             options;
>          int                             version;

I'm ok with this change, but it gets you guys off the hook with your broken
integration!

>    ...
>  }
> 
> DT property "calxeda,smmu-secure-cfg-access" would set
> ARM_SMMU_OPT_SECURE_CFG_ACCESS and
> DT property "linux,arm-smmu-isolate-devices" would set
> ARM_SMMU_OPT_ISOLATE_DEVICES.
> 
> (Maybe in future there will be more driver options. )

Yeah, and you could probably go all out with macros to generate this stuff
based on compatible string and OPT #define too.

Will

[PATCH 0/6] iommu/arm-smmu: Misc changes

[Will Deacon]

On Tue, Oct 01, 2013 at 01:39:04PM +0100, Andreas Herrmann wrote:
> Hi,
> 
> here a reworked set of patches. Hopefully all comments addressed.

Cheers Andreas! I've taken the first 5. Whilst I like the cleanup you've
made to the final patch, I'll need an Ack from Rob until I can take anything
containing DT bindings.

Thanks again,

Will

[PATCH 0/6] iommu/arm-smmu: Misc changes

[Andreas Herrmann]

On Tue, Oct 01, 2013 at 11:50:09AM -0400, Will Deacon wrote:
> On Tue, Oct 01, 2013 at 01:39:04PM +0100, Andreas Herrmann wrote:
> > Hi,
> > 
> > here a reworked set of patches. Hopefully all comments addressed.
> 
> Cheers Andreas! I've taken the first 5.

Cool, thanks!

> Whilst I like the cleanup you've made to the final patch, I'll need
> an Ack from Rob until I can take anything containing DT bindings.

I'll send an update of the 5th patch.
(But first waiting whether you might have additional comments on my
last suggestion about introducing smmu->options).


Andreas