[PATCH v3 0/6] iommu/arm-smmu: Misc modifications to support SMMUs on Calxeda ECX-2000

[PATCH v3 0/6] iommu/arm-smmu: Misc modifications to support SMMUs on Calxeda ECX-2000

[Andreas Herrmann]

Hi,

Here is v3 of arm-smmu changes to support SMMUs on Calxeda ECX-2000.

Esp. I've reworked the automatic StreamID masking. With this I try to
keep the number of used SMRs as low as possible. Of course it still
depends on some kind of reasonable ordering of StreamIDs for a master.

The general problem to determine the least number of mask/id pairs to
use for a given number of StreamIDs is not easy to solve.  But I think
the solution provided with patch 4 plus considering that we have some
influence how the StreamIDs are ordered (eg. when provided in DT) is
sufficient for kernel usage.

Here an example of SMR-usage for 3 SMMUs, each with one master device
and having StreamIDs 0-9, 0-1 and 0-1:

 arm-smmu 920180000.smmu: SMR0: 0x80070000
 arm-smmu 920180000.smmu: SMR1: 0x80010008
 arm-smmu 920180000.smmu: S2CR0: 0x0
 arm-smmu 920180000.smmu: S2CR1: 0x0
 arm-smmu 920000000.smmu: SMR0: 0x80010000
 arm-smmu 920000000.smmu: S2CR0: 0x0
 arm-smmu 920080000.smmu: SMR0: 0x80010000
 arm-smmu 920080000.smmu: S2CR0: 0x0


Changelog:
v3:
- Reworked automatic StreamID masking (I think the previous posted
  version was buggy anyway). It repeatedly tries to determine mask/id
  pairs for stream matching.
- Removed the patch that introduced support to mask all StreamIDs
  of a master device.
- Removed setting of flag sCR0_USFCFG in arm_smmu_device_notifier.
v2:
  http://marc.info/?l=linux-arm-kernel&m=138135834704855
v1:
   http://marc.info/?l=linux-arm-kernel&m=138122450023564


Regards,
Andreas

[PATCH 1/6] iommu/arm-smmu: Introduce driver option handling

[Andreas Herrmann]

Introduce handling of driver options. Options are set based on DT
information when probing an SMMU device. The first option introduced
is "arm,smmu-isolate-devices". (It will be used in the bus notifier
block.)

Signed-off-by: Andreas Herrmann <andreas.herrmann-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
---
 drivers/iommu/arm-smmu.c |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index b632bcd..0ad204e 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -47,6 +47,9 @@
 
 #include <asm/pgalloc.h>
 
+/* Driver options */
+#define ARM_SMMU_OPT_ISOLATE_DEVICES		(1 << 0)
+
 /* Maximum number of stream IDs assigned to a single device */
 #define MAX_MASTER_STREAMIDS		8
 
@@ -348,6 +351,7 @@ struct arm_smmu_device {
 #define ARM_SMMU_FEAT_TRANS_S2		(1 << 3)
 #define ARM_SMMU_FEAT_TRANS_NESTED	(1 << 4)
 	u32				features;
+	u32				options;
 	int				version;
 
 	u32				num_context_banks;
@@ -398,6 +402,29 @@ struct arm_smmu_domain {
 static DEFINE_SPINLOCK(arm_smmu_devices_lock);
 static LIST_HEAD(arm_smmu_devices);
 
+struct arm_smmu_option_prop {
+	u32 opt;
+	const char *prop;
+};
+
+static struct arm_smmu_option_prop arm_smmu_options [] = {
+	{ ARM_SMMU_OPT_ISOLATE_DEVICES, "arm,smmu-isolate-devices" },
+	{ 0, NULL},
+};
+
+static void check_driver_options(struct arm_smmu_device *smmu)
+{
+	int i = 0;
+	do {
+		if (of_property_read_bool(smmu->dev->of_node,
+						arm_smmu_options[i].prop)) {
+			smmu->options |= arm_smmu_options[i].opt;
+			dev_dbg(smmu->dev, "option %s\n",
+				arm_smmu_options[i].prop);
+		}
+	} while (arm_smmu_options[++i].opt);
+}
+
 static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu,
 						struct device_node *dev_node)
 {
@@ -1791,6 +1818,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	}
 	smmu->dev = dev;
 
+	check_driver_options(smmu);
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(dev, "missing base address/size\n");
-- 
1.7.9.5

[PATCH 2/6] iommu/arm-smmu: Introduce bus notifier block

[Andreas Herrmann]

At the moment just handle BUS_NOTIFY_BIND_DRIVER to conditionally
isolate all master devices for an SMMU.

Depending on DT information each device is put into its own protection
domain (if possible).  For configuration with one or just a few
masters per SMMU that is easy to achieve.

In case of many devices per SMMU (e.g. MMU-500 with it's distributed
translation support) isolation of each device might not be possible --
depending on number of available SMR groups and/or context banks.

Default is that device isolation is contolled per SMMU with SMMU node
property "arm,smmu-isolate-devices" in a DT. If this property is set
for an SMMU node, device isolation is performed.

W/o device isolation the driver detects SMMUs but no translation is
configured (transactions just bypass translation process).

Note that for device isolation dma_base and size are fixed as 0 and
SZ_128M at the moment.

Signed-off-by: Andreas Herrmann <andreas.herrmann-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
---
 drivers/iommu/arm-smmu.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 0ad204e..3ba17ac 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -46,6 +46,7 @@
 #include <linux/amba/bus.h>
 
 #include <asm/pgalloc.h>
+#include <asm/dma-iommu.h>
 
 /* Driver options */
 #define ARM_SMMU_OPT_ISOLATE_DEVICES		(1 << 0)
@@ -1976,6 +1977,48 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int arm_smmu_device_notifier(struct notifier_block *nb,
+				unsigned long action, void *data)
+{
+	struct device *dev = data;
+	struct dma_iommu_mapping *mapping;
+	struct arm_smmu_device *smmu;
+	int ret;
+
+	switch (action) {
+	case BUS_NOTIFY_BIND_DRIVER:
+
+		arm_smmu_add_device(dev);
+		smmu = dev->archdata.iommu;
+		if (!smmu || !(smmu->options & ARM_SMMU_OPT_ISOLATE_DEVICES))
+			break;
+
+		mapping = arm_iommu_create_mapping(&platform_bus_type,
+						0, SZ_128M, 0);
+		if (IS_ERR(mapping)) {
+			ret = PTR_ERR(mapping);
+			dev_info(dev, "arm_iommu_create_mapping failed\n");
+			break;
+		}
+
+		ret = arm_iommu_attach_device(dev, mapping);
+		if (ret < 0) {
+			dev_info(dev, "arm_iommu_attach_device failed\n");
+			arm_iommu_release_mapping(mapping);
+		}
+
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static struct notifier_block device_nb = {
+	.notifier_call = arm_smmu_device_notifier,
+};
+
 #ifdef CONFIG_OF
 static struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "arm,smmu-v1", },
@@ -2012,6 +2055,8 @@ static int __init arm_smmu_init(void)
 	if (!iommu_present(&amba_bustype))
 		bus_set_iommu(&amba_bustype, &arm_smmu_ops);
 
+	bus_register_notifier(&platform_bus_type, &device_nb);
+
 	return 0;
 }
 
-- 
1.7.9.5

Re: [PATCH 2/6] iommu/arm-smmu: Introduce bus notifier block

[Will Deacon]

On Fri, Oct 18, 2013 at 09:13:11PM +0100, Andreas Herrmann wrote:
> At the moment just handle BUS_NOTIFY_BIND_DRIVER to conditionally
> isolate all master devices for an SMMU.
> 
> Depending on DT information each device is put into its own protection
> domain (if possible).  For configuration with one or just a few
> masters per SMMU that is easy to achieve.
> 
> In case of many devices per SMMU (e.g. MMU-500 with it's distributed
> translation support) isolation of each device might not be possible --
> depending on number of available SMR groups and/or context banks.
> 
> Default is that device isolation is contolled per SMMU with SMMU node
> property "arm,smmu-isolate-devices" in a DT. If this property is set
> for an SMMU node, device isolation is performed.
> 
> W/o device isolation the driver detects SMMUs but no translation is
> configured (transactions just bypass translation process).
> 
> Note that for device isolation dma_base and size are fixed as 0 and
> SZ_128M at the moment.

I still think we need to solve the base/size issue before these can be
merged (the option parsing code in patch 1 looks fine).

Maybe we could it all on demand by registering a fault handler with an
empty IOMMU domain?

Will

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

[Andreas Herrmann]

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

This handling is switched on by DT property
"arm,smmu-secure-config-access" for an SMMU node.

Signed-off-by: Andreas Herrmann <andreas.herrmann-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
---
 drivers/iommu/arm-smmu.c |   30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 3ba17ac..a65a559 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -50,6 +50,7 @@
 
 /* Driver options */
 #define ARM_SMMU_OPT_ISOLATE_DEVICES		(1 << 0)
+#define ARM_SMMU_OPT_SECURE_CONFIG_ACCESS	(1 << 1)
 
 /* Maximum number of stream IDs assigned to a single device */
 #define MAX_MASTER_STREAMIDS		8
@@ -64,6 +65,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->options & ARM_SMMU_OPT_SECURE_CONFIG_ACCESS)	\
+			? 0x400 : 0))
+
 /* Page table bits */
 #define ARM_SMMU_PTE_PAGE		(((pteval_t)3) << 0)
 #define ARM_SMMU_PTE_CONT		(((pteval_t)1) << 52)
@@ -410,6 +420,7 @@ struct arm_smmu_option_prop {
 
 static struct arm_smmu_option_prop arm_smmu_options [] = {
 	{ ARM_SMMU_OPT_ISOLATE_DEVICES, "arm,smmu-isolate-devices" },
+	{ ARM_SMMU_OPT_SECURE_CONFIG_ACCESS, "arm,smmu-secure-config-access" },
 	{ 0, NULL},
 };
 
@@ -639,16 +650,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,
@@ -1595,8 +1606,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) {
@@ -1616,7 +1627,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);
@@ -1635,7 +1646,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)
@@ -1973,7 +1984,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

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

[Will Deacon]

On Fri, Oct 18, 2013 at 09:13:12PM +0100, Andreas Herrmann wrote:
> In such a case we have to use secure aliases of some non-secure
> registers.
> 
> This handling is switched on by DT property
> "arm,smmu-secure-config-access" for an SMMU node.

This looks fine, but won't apply without the device isolation changes.

Will

[PATCH 4/6] iommu/arm-smmu: Introduce automatic stream-id-masking

[Andreas Herrmann]

Try to determine a mask that can be used for all StreamIDs of a master
device. This allows to use just one SMR group instead of
number-of-streamids SMR groups for a master device.

Signed-off-by: Andreas Herrmann <andreas.herrmann-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
---
 drivers/iommu/arm-smmu.c |  121 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 105 insertions(+), 16 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index a65a559..5f585fc 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -28,6 +28,7 @@
  *	- Context fault reporting
  */
 
+#define DEBUG
 #define pr_fmt(fmt) "arm-smmu: " fmt
 
 #include <linux/delay.h>
@@ -42,6 +43,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/bitops.h>
 
 #include <linux/amba/bus.h>
 
@@ -338,8 +340,9 @@ struct arm_smmu_master {
 	 * SMMU chain.
 	 */
 	struct rb_node			node;
-	int				num_streamids;
+	u32				num_streamids;
 	u16				streamids[MAX_MASTER_STREAMIDS];
+	int				num_used_smrs;
 
 	/*
 	 * We only need to allocate these on the root SMMU, as we
@@ -1025,10 +1028,90 @@ static void arm_smmu_domain_destroy(struct iommu_domain *domain)
 	kfree(smmu_domain);
 }
 
+static int determine_smr_mask(struct arm_smmu_master *master,
+			struct arm_smmu_smr *smr, int start, int nr)
+{
+	u16 i, zero_bits_mask, one_bits_mask, const_mask;
+
+	BUG_ON(!is_power_of_2(nr));
+
+	if (nr == 1) {
+		/* no mask, use streamid to match and be done with it */
+		smr->mask = 0;
+		smr->id = master->streamids[start];
+		return 0;
+	}
+
+	zero_bits_mask = 0;
+	one_bits_mask = 0xffff;
+	for (i = start; i < start + nr; i++) {
+		zero_bits_mask |= master->streamids[i];   /* const 0 bits */
+		one_bits_mask &= master->streamids[i]; /* const 1 bits */
+	}
+	zero_bits_mask = ~zero_bits_mask;
+
+	/* bits having constant values (either 0 or 1) */
+	const_mask = zero_bits_mask | one_bits_mask;
+
+	i = hweight16(~const_mask);
+	if ((1 << i) == nr) {
+		smr->mask = ~const_mask;
+		smr->id = one_bits_mask;
+	} else {
+		/* no usable mask for this set of streamids */
+		return 1;
+	}
+
+	return 0;
+}
+
+static int determine_smr_mapping(struct arm_smmu_master *master,
+				struct arm_smmu_smr *smrs, int max_smrs)
+{
+	int nr_sid, nr, i, bit, start;
+
+	start = nr = 0;
+	nr_sid = master->num_streamids;
+	do {
+		/*
+		 * largest power-of-2 number of streamids for which to
+		 * determine a usable mask/id pair for stream matching
+		 */
+		bit = fls(nr_sid);
+		if (!bit)
+			return 0;
+
+		/*
+		 * iterate over power-of-2 numbers to determine
+		 * largest possible mask/id pair for stream matching
+		 * of next <nr> streamids
+		 */
+		for (i = bit - 1; i >= 0; i--) {
+			nr = 1 << i;
+			if(!determine_smr_mask(master,
+						&smrs[master->num_used_smrs],
+						start, nr))
+				break;
+		}
+
+		nr_sid -= nr;
+		start += nr;
+		master->num_used_smrs++;
+	} while (master->num_used_smrs <= max_smrs);
+
+	if (nr_sid) {
+		/* not enough mapping groups available */
+		master->num_used_smrs = 0;
+		return -ENOSPC;
+	}
+
+	return 0;
+}
+
 static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
 					  struct arm_smmu_master *master)
 {
-	int i;
+	int i, max_smrs, ret;
 	struct arm_smmu_smr *smrs;
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
 
@@ -1038,42 +1121,45 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
 	if (master->smrs)
 		return -EEXIST;
 
-	smrs = kmalloc(sizeof(*smrs) * master->num_streamids, GFP_KERNEL);
+	max_smrs = min(smmu->num_mapping_groups, master->num_streamids);
+	smrs = kmalloc(sizeof(*smrs) * max_smrs, GFP_KERNEL);
 	if (!smrs) {
 		dev_err(smmu->dev, "failed to allocate %d SMRs for master %s\n",
-			master->num_streamids, master->of_node->name);
+			max_smrs, master->of_node->name);
 		return -ENOMEM;
 	}
 
+	ret = determine_smr_mapping(master, smrs, max_smrs);
+	if (ret)
+		goto err_free_smrs;
+
 	/* Allocate the SMRs on the root SMMU */
-	for (i = 0; i < master->num_streamids; ++i) {
+	for (i = 0; i < master->num_used_smrs; ++i) {
 		int idx = __arm_smmu_alloc_bitmap(smmu->smr_map, 0,
 						  smmu->num_mapping_groups);
 		if (IS_ERR_VALUE(idx)) {
 			dev_err(smmu->dev, "failed to allocate free SMR\n");
-			goto err_free_smrs;
+			goto err_free_bitmap;
 		}
-
-		smrs[i] = (struct arm_smmu_smr) {
-			.idx	= idx,
-			.mask	= 0, /* We don't currently share SMRs */
-			.id	= master->streamids[i],
-		};
+		smrs[i].idx = idx;
 	}
 
 	/* It worked! Now, poke the actual hardware */
-	for (i = 0; i < master->num_streamids; ++i) {
+	for (i = 0; i < master->num_used_smrs; ++i) {
 		u32 reg = SMR_VALID | smrs[i].id << SMR_ID_SHIFT |
 			  smrs[i].mask << SMR_MASK_SHIFT;
+		dev_dbg(smmu->dev, "SMR%d: 0x%x\n", smrs[i].idx, reg);
 		writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_SMR(smrs[i].idx));
 	}
 
 	master->smrs = smrs;
 	return 0;
 
-err_free_smrs:
+err_free_bitmap:
 	while (--i >= 0)
 		__arm_smmu_free_bitmap(smmu->smr_map, smrs[i].idx);
+	master->num_used_smrs = 0;
+err_free_smrs:
 	kfree(smrs);
 	return -ENOSPC;
 }
@@ -1112,7 +1198,7 @@ static void arm_smmu_bypass_stream_mapping(struct arm_smmu_device *smmu,
 static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 				      struct arm_smmu_master *master)
 {
-	int i, ret;
+	int i, ret, num_s2crs;
 	struct arm_smmu_device *parent, *smmu = smmu_domain->root_cfg.smmu;
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
 
@@ -1136,11 +1222,14 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 	}
 
 	/* Now we're at the root, time to point at our context bank */
-	for (i = 0; i < master->num_streamids; ++i) {
+	num_s2crs = master->num_used_smrs ? master->num_used_smrs :
+		master->num_streamids;
+	for (i = 0; i < num_s2crs; ++i) {
 		u32 idx, s2cr;
 		idx = master->smrs ? master->smrs[i].idx : master->streamids[i];
 		s2cr = (S2CR_TYPE_TRANS << S2CR_TYPE_SHIFT) |
 		       (smmu_domain->root_cfg.cbndx << S2CR_CBNDX_SHIFT);
+		dev_dbg(smmu->dev, "S2CR%d: 0x%x\n", idx, s2cr);
 		writel_relaxed(s2cr, gr0_base + ARM_SMMU_GR0_S2CR(idx));
 	}
 
-- 
1.7.9.5

Re: [PATCH 4/6] iommu/arm-smmu: Introduce automatic stream-id-masking

[Will Deacon]

Hi Andreas,

Cheers for the new patch, some comments/questions in line...

On Fri, Oct 18, 2013 at 09:13:13PM +0100, Andreas Herrmann wrote:
> Try to determine a mask that can be used for all StreamIDs of a master
> device. This allows to use just one SMR group instead of
> number-of-streamids SMR groups for a master device.
> 
> Signed-off-by: Andreas Herrmann <andreas.herrmann-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c |  121 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 105 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index a65a559..5f585fc 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -28,6 +28,7 @@
>   *	- Context fault reporting
>   */
>  
> +#define DEBUG

You can drop this change from the patch.

>  #define pr_fmt(fmt) "arm-smmu: " fmt
>  
>  #include <linux/delay.h>
> @@ -42,6 +43,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> +#include <linux/bitops.h>
>  
>  #include <linux/amba/bus.h>
>  
> @@ -338,8 +340,9 @@ struct arm_smmu_master {
>  	 * SMMU chain.
>  	 */
>  	struct rb_node			node;
> -	int				num_streamids;
> +	u32				num_streamids;
>  	u16				streamids[MAX_MASTER_STREAMIDS];
> +	int				num_used_smrs;
>  
>  	/*
>  	 * We only need to allocate these on the root SMMU, as we
> @@ -1025,10 +1028,90 @@ static void arm_smmu_domain_destroy(struct iommu_domain *domain)
>  	kfree(smmu_domain);
>  }
>  
> +static int determine_smr_mask(struct arm_smmu_master *master,
> +			struct arm_smmu_smr *smr, int start, int nr)
> +{
> +	u16 i, zero_bits_mask, one_bits_mask, const_mask;
> +
> +	BUG_ON(!is_power_of_2(nr));

If you change the parameters to take a bit number instead of a value, then
you don't need the BUG_ON by construction.

> +	if (nr == 1) {
> +		/* no mask, use streamid to match and be done with it */
> +		smr->mask = 0;
> +		smr->id = master->streamids[start];
> +		return 0;
> +	}
> +
> +	zero_bits_mask = 0;
> +	one_bits_mask = 0xffff;
> +	for (i = start; i < start + nr; i++) {
> +		zero_bits_mask |= master->streamids[i];   /* const 0 bits */
> +		one_bits_mask &= master->streamids[i]; /* const 1 bits */
> +	}
> +	zero_bits_mask = ~zero_bits_mask;
> +
> +	/* bits having constant values (either 0 or 1) */
> +	const_mask = zero_bits_mask | one_bits_mask;
> +
> +	i = hweight16(~const_mask);
> +	if ((1 << i) == nr) {
> +		smr->mask = ~const_mask;
> +		smr->id = one_bits_mask;

An extra thing to think about is the number of implemented bits in the SMR
registers (I believe this is implemented defined). We currently do a basic
sanity check in arm_smmu_device_cfg_probe, where we check that the mask and
id fields are sufficient for each other, but we should also check this
against the real ids and masks once they are known.

> +	} else {
> +		/* no usable mask for this set of streamids */
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int determine_smr_mapping(struct arm_smmu_master *master,
> +				struct arm_smmu_smr *smrs, int max_smrs)
> +{
> +	int nr_sid, nr, i, bit, start;
> +
> +	start = nr = 0;
> +	nr_sid = master->num_streamids;
> +	do {
> +		/*
> +		 * largest power-of-2 number of streamids for which to
> +		 * determine a usable mask/id pair for stream matching
> +		 */
> +		bit = fls(nr_sid);
> +		if (!bit)
> +			return 0;
> +
> +		/*
> +		 * iterate over power-of-2 numbers to determine
> +		 * largest possible mask/id pair for stream matching
> +		 * of next <nr> streamids
> +		 */
> +		for (i = bit - 1; i >= 0; i--) {
> +			nr = 1 << i;
> +			if(!determine_smr_mask(master,
> +						&smrs[master->num_used_smrs],
> +						start, nr))

Does this rely on the stream IDs being sorted? We're not actually trying all
combinations of IDs, so it seems like changing the order of IDs in the
device tree will potentially change the number of smrs you end up using.

> +				break;
> +		}
> +
> +		nr_sid -= nr;
> +		start += nr;
> +		master->num_used_smrs++;

Does the manipulation of num_used_smrs here mean that we should only call
this function exactly once? Would it be worth checking that num_used_smrs is
zero when we enter this function?

Also, in v1, we discussed about checking for duplicate stream ids in
register_smmu_master. Did you look into that?

> +	} while (master->num_used_smrs <= max_smrs);
> +
> +	if (nr_sid) {
> +		/* not enough mapping groups available */
> +		master->num_used_smrs = 0;
> +		return -ENOSPC;
> +	}
> +
> +	return 0;
> +}
> +
>  static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
>  					  struct arm_smmu_master *master)
>  {
> -	int i;
> +	int i, max_smrs, ret;
>  	struct arm_smmu_smr *smrs;
>  	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
>  
> @@ -1038,42 +1121,45 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
>  	if (master->smrs)
>  		return -EEXIST;
>  
> -	smrs = kmalloc(sizeof(*smrs) * master->num_streamids, GFP_KERNEL);
> +	max_smrs = min(smmu->num_mapping_groups, master->num_streamids);
> +	smrs = kmalloc(sizeof(*smrs) * max_smrs, GFP_KERNEL);
>  	if (!smrs) {
>  		dev_err(smmu->dev, "failed to allocate %d SMRs for master %s\n",
> -			master->num_streamids, master->of_node->name);
> +			max_smrs, master->of_node->name);
>  		return -ENOMEM;
>  	}
>  
> +	ret = determine_smr_mapping(master, smrs, max_smrs);
> +	if (ret)
> +		goto err_free_smrs;
> +
>  	/* Allocate the SMRs on the root SMMU */
> -	for (i = 0; i < master->num_streamids; ++i) {
> +	for (i = 0; i < master->num_used_smrs; ++i) {
>  		int idx = __arm_smmu_alloc_bitmap(smmu->smr_map, 0,
>  						  smmu->num_mapping_groups);
>  		if (IS_ERR_VALUE(idx)) {
>  			dev_err(smmu->dev, "failed to allocate free SMR\n");
> -			goto err_free_smrs;
> +			goto err_free_bitmap;
>  		}
> -
> -		smrs[i] = (struct arm_smmu_smr) {
> -			.idx	= idx,
> -			.mask	= 0, /* We don't currently share SMRs */
> -			.id	= master->streamids[i],
> -		};
> +		smrs[i].idx = idx;
>  	}
>  
>  	/* It worked! Now, poke the actual hardware */
> -	for (i = 0; i < master->num_streamids; ++i) {
> +	for (i = 0; i < master->num_used_smrs; ++i) {
>  		u32 reg = SMR_VALID | smrs[i].id << SMR_ID_SHIFT |
>  			  smrs[i].mask << SMR_MASK_SHIFT;
> +		dev_dbg(smmu->dev, "SMR%d: 0x%x\n", smrs[i].idx, reg);
>  		writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_SMR(smrs[i].idx));
>  	}
>  
>  	master->smrs = smrs;
>  	return 0;
>  
> -err_free_smrs:
> +err_free_bitmap:
>  	while (--i >= 0)
>  		__arm_smmu_free_bitmap(smmu->smr_map, smrs[i].idx);
> +	master->num_used_smrs = 0;
> +err_free_smrs:
>  	kfree(smrs);
>  	return -ENOSPC;
>  }
> @@ -1112,7 +1198,7 @@ static void arm_smmu_bypass_stream_mapping(struct arm_smmu_device *smmu,
>  static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>  				      struct arm_smmu_master *master)
>  {
> -	int i, ret;
> +	int i, ret, num_s2crs;
>  	struct arm_smmu_device *parent, *smmu = smmu_domain->root_cfg.smmu;
>  	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
>  
> @@ -1136,11 +1222,14 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>  	}
>  
>  	/* Now we're at the root, time to point at our context bank */
> -	for (i = 0; i < master->num_streamids; ++i) {
> +	num_s2crs = master->num_used_smrs ? master->num_used_smrs :
> +		master->num_streamids;

I know we already changed it once, but checks like this are ugly, so perhaps
renaming num_user_smrs (again!) to something like num_s2crs, then just
making sure that it is set to num_streamids if we're not using stream
matching?

Will

[PATCH 5/6] ARM: dts: Add nodes for SMMUs on Calxeda ECX-2000

[Andreas Herrmann]

Signed-off-by: Andreas Herrmann <andreas.herrmann-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/boot/dts/ecx-2000.dts    |   44 +++++++++++++++++++++++++++++++++++--
 arch/arm/boot/dts/ecx-common.dtsi |    9 +++++---
 drivers/iommu/arm-smmu.c          |    2 +-
 include/linux/of.h                |    2 +-
 4 files changed, 50 insertions(+), 7 deletions(-)

diff --git a/arch/arm/boot/dts/ecx-2000.dts b/arch/arm/boot/dts/ecx-2000.dts
index 139b40c..e979e8e 100644
--- a/arch/arm/boot/dts/ecx-2000.dts
+++ b/arch/arm/boot/dts/ecx-2000.dts
@@ -76,10 +76,11 @@
 	};
 
 	soc {
-		ranges = <0x00000000 0x00000000 0x00000000 0xffffffff>;
+		ranges = <0x0 0x0 0x0 0xffffffff>;
 
 		timer {
-			compatible = "arm,cortex-a15-timer", "arm,armv7-timer"; 			interrupts = <1 13 0xf08>,
+			compatible = "arm,cortex-a15-timer", "arm,armv7-timer";
+			interrupts = <1 13 0xf08>,
 				<1 14 0xf08>,
 				<1 11 0xf08>,
 				<1 10 0xf08>;
@@ -103,6 +104,45 @@
 			interrupts = <0 76 4  0 75 4  0 74 4  0 73 4>;
 		};
 	};
+
+	soc@920000000 {
+		ranges = <0x9 0x20000000 0x9 0x20000000 0x290000>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+		compatible = "simple-bus";
+		interrupt-parent = <&intc>;
+
+		smmu_mac0: smmu@920000000 {
+			compatible = "arm,mmu-400";
+			reg = <0x9 0x20000000 0x10000>;
+			#global-interrupts = <1>;
+			interrupts = <0 106 4 0 106 4>;
+			mmu-masters = <&mac0 0 1>;
+			arm,smmu-secure-config-access;
+			arm,smmu-isolate-devices;
+		};
+
+		smmu_mac1: smmu@920080000 {
+			compatible = "arm,mmu-400";
+			reg = <0x9 0x20080000 0x10000>;
+			#global-interrupts = <1>;
+			interrupts = <0 108 4 0 108 4>;
+			mmu-masters = <&mac1 0 1>;
+			arm,smmu-secure-config-access;
+			arm,smmu-isolate-devices;
+		};
+
+		smmu_sata: smmu@920180000 {
+			compatible = "arm,mmu-400";
+			reg = <0x00000009 0x20180000 0x10000>;
+			mmu-masters = <&sata 0 1 2 3 4 5 6 7 8 9>;
+			#global-interrupts = <1>;
+			interrupts = <0 114 4 0 114 4>;
+			arm,smmu-secure-config-access;
+			arm,smmu-isolate-devices;
+		};
+	};
+
 };
 
 /include/ "ecx-common.dtsi"
diff --git a/arch/arm/boot/dts/ecx-common.dtsi b/arch/arm/boot/dts/ecx-common.dtsi
index e8559b7..961dc5b 100644
--- a/arch/arm/boot/dts/ecx-common.dtsi
+++ b/arch/arm/boot/dts/ecx-common.dtsi
@@ -25,7 +25,7 @@
 		compatible = "simple-bus";
 		interrupt-parent = <&intc>;
 
-		sata@ffe08000 {
+		sata: sata@ffe08000 {
 			compatible = "calxeda,hb-ahci";
 			reg = <0xffe08000 0x10000>;
 			interrupts = <0 83 4>;
@@ -35,6 +35,7 @@
 					     &combophy0 3>;
 			calxeda,sgpio-gpio =<&gpioh 5 1 &gpioh 6 1 &gpioh 7 1>;
 			calxeda,led-order = <4 0 1 2 3>;
+			#stream-id-cells = <10>;
 		};
 
 		sdhci@ffe0e000 {
@@ -208,18 +209,20 @@
 			clock-names = "apb_pclk";
 		};
 
-		ethernet@fff50000 {
+		mac0: ethernet@fff50000 {
 			compatible = "calxeda,hb-xgmac";
 			reg = <0xfff50000 0x1000>;
 			interrupts = <0 77 4  0 78 4  0 79 4>;
 			dma-coherent;
+			#stream-id-cells = <2>;
 		};
 
-		ethernet@fff51000 {
+		mac1: ethernet@fff51000 {
 			compatible = "calxeda,hb-xgmac";
 			reg = <0xfff51000 0x1000>;
 			interrupts = <0 80 4  0 81 4  0 82 4>;
 			dma-coherent;
+			#stream-id-cells = <2>;
 		};
 
 		combophy0: combo-phy@fff58000 {
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 5f585fc..9fc34d1 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -55,7 +55,7 @@
 #define ARM_SMMU_OPT_SECURE_CONFIG_ACCESS	(1 << 1)
 
 /* Maximum number of stream IDs assigned to a single device */
-#define MAX_MASTER_STREAMIDS		8
+#define MAX_MASTER_STREAMIDS		10
 
 /* Maximum number of context banks per SMMU */
 #define ARM_SMMU_MAX_CBS		128
diff --git a/include/linux/of.h b/include/linux/of.h
index f95aee3..47f4857 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -67,7 +67,7 @@ struct device_node {
 #endif
 };
 
-#define MAX_PHANDLE_ARGS 8
+#define MAX_PHANDLE_ARGS 10
 struct of_phandle_args {
 	struct device_node *np;
 	int args_count;
-- 
1.7.9.5

Re: [PATCH 5/6] ARM: dts: Add nodes for SMMUs on Calxeda ECX-2000

[Will Deacon]

On Fri, Oct 18, 2013 at 09:13:14PM +0100, Andreas Herrmann wrote:
> Signed-off-by: Andreas Herrmann <andreas.herrmann-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> ---
>  arch/arm/boot/dts/ecx-2000.dts    |   44 +++++++++++++++++++++++++++++++++++--
>  arch/arm/boot/dts/ecx-common.dtsi |    9 +++++---
>  drivers/iommu/arm-smmu.c          |    2 +-
>  include/linux/of.h                |    2 +-
>  4 files changed, 50 insertions(+), 7 deletions(-)

[...]

> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 5f585fc..9fc34d1 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -55,7 +55,7 @@
>  #define ARM_SMMU_OPT_SECURE_CONFIG_ACCESS	(1 << 1)
>  
>  /* Maximum number of stream IDs assigned to a single device */
> -#define MAX_MASTER_STREAMIDS		8
> +#define MAX_MASTER_STREAMIDS		10

Maybe we should define this to be MAX_PHANDLE_ARGS instead, since we're
really bound by the DT parsing code rather than anything else.

Will

>  /* Maximum number of context banks per SMMU */
>  #define ARM_SMMU_MAX_CBS		128
> diff --git a/include/linux/of.h b/include/linux/of.h
> index f95aee3..47f4857 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -67,7 +67,7 @@ struct device_node {
>  #endif
>  };
>  
> -#define MAX_PHANDLE_ARGS 8
> +#define MAX_PHANDLE_ARGS 10
>  struct of_phandle_args {
>  	struct device_node *np;
>  	int args_count;

This should be a separate patch.

Will

Re: [PATCH 5/6] ARM: dts: Add nodes for SMMUs on Calxeda ECX-2000

[Andreas Herrmann]

On Wed, Oct 30, 2013 at 09:15:21PM -0400, Will Deacon wrote:
> On Fri, Oct 18, 2013 at 09:13:14PM +0100, Andreas Herrmann wrote:
> > Signed-off-by: Andreas Herrmann <andreas.herrmann-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> > ---
> >  arch/arm/boot/dts/ecx-2000.dts    |   44 +++++++++++++++++++++++++++++++++++--
> >  arch/arm/boot/dts/ecx-common.dtsi |    9 +++++---
> >  drivers/iommu/arm-smmu.c          |    2 +-
> >  include/linux/of.h                |    2 +-
> >  4 files changed, 50 insertions(+), 7 deletions(-)
> 
> [...]
> 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 5f585fc..9fc34d1 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -55,7 +55,7 @@
> >  #define ARM_SMMU_OPT_SECURE_CONFIG_ACCESS	(1 << 1)
> >  
> >  /* Maximum number of stream IDs assigned to a single device */
> > -#define MAX_MASTER_STREAMIDS		8
> > +#define MAX_MASTER_STREAMIDS		10
> 
> Maybe we should define this to be MAX_PHANDLE_ARGS instead, since we're
> really bound by the DT parsing code rather than anything else.

Agreed.

> 
> >  /* Maximum number of context banks per SMMU */
> >  #define ARM_SMMU_MAX_CBS		128
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index f95aee3..47f4857 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -67,7 +67,7 @@ struct device_node {
> >  #endif
> >  };
> >  
> > -#define MAX_PHANDLE_ARGS 8
> > +#define MAX_PHANDLE_ARGS 10
> >  struct of_phandle_args {
> >  	struct device_node *np;
> >  	int args_count;
> 
> This should be a separate patch.

Ok.

Andreas

[PATCH 6/6] documentation/iommu: Update description of ARM System MMU binding

[Andreas Herrmann]

This patch adds descriptions fore new properties of device tree
binding for the ARM SMMU architecture. These properties control
arm-smmu driver options.

Cc: Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Andreas Herrmann <andreas.herrmann-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
---
 .../devicetree/bindings/iommu/arm,smmu.txt         |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index e34c6cd..de88cf9 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -48,6 +48,17 @@ conditions.
                   from the mmu-masters towards memory) node for this
                   SMMU.
 
+- arm,smmu-isolate-devices : Enable device isolation for all masters
+                             of this SMMU. Ie. each master will be
+                             attached to its own iommu domain.
+
+- arm,smmu-secure-config-access : Enable proper handling of buggy
+                                  implementations that always use
+                                  secure access to SMMU configuration
+                                  registers. In this case non-secure
+                                  aliases of secure registers have to
+                                  be used during SMMU configuration.
+
 Example:
 
         smmu {
@@ -67,4 +78,5 @@ Example:
                  */
                 mmu-masters = <&dma0 0xd01d 0xd01e>,
                               <&dma1 0xd11c>;
+                arm,smmu-isolate-devices;
         };
-- 
1.7.9.5

Re: [PATCH 6/6] documentation/iommu: Update description of ARM System MMU binding

[Will Deacon]

On Fri, Oct 18, 2013 at 09:13:15PM +0100, Andreas Herrmann wrote:
> This patch adds descriptions fore new properties of device tree
> binding for the ARM SMMU architecture. These properties control
> arm-smmu driver options.
> 
> Cc: Rob Herring <robherring2@gmail.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
>  .../devicetree/bindings/iommu/arm,smmu.txt         |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> index e34c6cd..de88cf9 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> @@ -48,6 +48,17 @@ conditions.
>                    from the mmu-masters towards memory) node for this
>                    SMMU.
>  
> +- arm,smmu-isolate-devices : Enable device isolation for all masters
> +                             of this SMMU. Ie. each master will be
> +                             attached to its own iommu domain.
> +
> +- arm,smmu-secure-config-access : Enable proper handling of buggy
> +                                  implementations that always use
> +                                  secure access to SMMU configuration
> +                                  registers. In this case non-secure
> +                                  aliases of secure registers have to
> +                                  be used during SMMU configuration.

Why are you using the "arm" vendor prefix for the secure config access
stuff? Wouldn't it make more sense to use "calxeda", just in case somebody
else finds a different way to wire things up in this regard?

Will

Re: [PATCH 6/6] documentation/iommu: Update description of ARM System MMU binding

[Rob Herring]

On Wed, Oct 30, 2013 at 8:17 PM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> On Fri, Oct 18, 2013 at 09:13:15PM +0100, Andreas Herrmann wrote:
>> This patch adds descriptions fore new properties of device tree
>> binding for the ARM SMMU architecture. These properties control
>> arm-smmu driver options.
>>
>> Cc: Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
>> Signed-off-by: Andreas Herrmann <andreas.herrmann-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>> ---
>>  .../devicetree/bindings/iommu/arm,smmu.txt         |   12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
>> index e34c6cd..de88cf9 100644
>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
>> @@ -48,6 +48,17 @@ conditions.
>>                    from the mmu-masters towards memory) node for this
>>                    SMMU.
>>
>> +- arm,smmu-isolate-devices : Enable device isolation for all masters
>> +                             of this SMMU. Ie. each master will be
>> +                             attached to its own iommu domain.
>> +
>> +- arm,smmu-secure-config-access : Enable proper handling of buggy
>> +                                  implementations that always use
>> +                                  secure access to SMMU configuration
>> +                                  registers. In this case non-secure
>> +                                  aliases of secure registers have to
>> +                                  be used during SMMU configuration.
>
> Why are you using the "arm" vendor prefix for the secure config access
> stuff? Wouldn't it make more sense to use "calxeda", just in case somebody
> else finds a different way to wire things up in this regard?

I think that the property prefix should match the compatible vendor
prefix. You could then argue that the compatible string itself should
be prefixed with "calxeda". In that case, this property would not be
needed at all as you could just key off the compatible string to
determine this characteristic. Of the options, my preference would be
just to leave things as is.

Rob

Re: [PATCH 6/6] documentation/iommu: Update description of ARM System MMU binding

[Andreas Herrmann]

On Thu, Oct 31, 2013 at 02:45:55AM -0400, Rob Herring wrote:
> On Wed, Oct 30, 2013 at 8:17 PM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> > On Fri, Oct 18, 2013 at 09:13:15PM +0100, Andreas Herrmann wrote:
> >> This patch adds descriptions fore new properties of device tree
> >> binding for the ARM SMMU architecture. These properties control
> >> arm-smmu driver options.
> >>
> >> Cc: Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> >> Signed-off-by: Andreas Herrmann <andreas.herrmann-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> >> ---
> >>  .../devicetree/bindings/iommu/arm,smmu.txt         |   12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> >> index e34c6cd..de88cf9 100644
> >> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> >> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> >> @@ -48,6 +48,17 @@ conditions.
> >>                    from the mmu-masters towards memory) node for this
> >>                    SMMU.
> >>
> >> +- arm,smmu-isolate-devices : Enable device isolation for all masters
> >> +                             of this SMMU. Ie. each master will be
> >> +                             attached to its own iommu domain.
> >> +
> >> +- arm,smmu-secure-config-access : Enable proper handling of buggy
> >> +                                  implementations that always use
> >> +                                  secure access to SMMU configuration
> >> +                                  registers. In this case non-secure
> >> +                                  aliases of secure registers have to
> >> +                                  be used during SMMU configuration.
> >
> > Why are you using the "arm" vendor prefix for the secure config access
> > stuff? Wouldn't it make more sense to use "calxeda", just in case somebody
> > else finds a different way to wire things up in this regard?

I think in an early version I've used calxeda prefix but later thought
it has to match the arm prefix. I'm also fine with
calxeda,smmu-secure-config-access. But in case someone else screws
this up in a similar way and needs the same driver behaviour it's odd
if XYZ has to use the calxeda prefix instead of the arm prefix for
this option.

> I think that the property prefix should match the compatible vendor
> prefix. You could then argue that the compatible string itself should
> be prefixed with "calxeda". In that case, this property would not be
> needed at all as you could just key off the compatible string to
> determine this characteristic. Of the options, my preference would be
> just to leave things as is.

I think Will's main point is that Calxeda has a bug in the wiring and
that this is not ARM's fault. Renaming the prefix will kind of
emphasize this. In case we keep the arm prefix, how about modifying
the description for the option to state that currently only Calxeda
ECX-2000 screwed up the wiring?


Andreas

Re: [PATCH 6/6] documentation/iommu: Update description of ARM System MMU binding

[Will Deacon]

On Thu, Oct 31, 2013 at 09:16:57AM +0000, Andreas Herrmann wrote:
> On Thu, Oct 31, 2013 at 02:45:55AM -0400, Rob Herring wrote:
> > On Wed, Oct 30, 2013 at 8:17 PM, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> > > Why are you using the "arm" vendor prefix for the secure config access
> > > stuff? Wouldn't it make more sense to use "calxeda", just in case somebody
> > > else finds a different way to wire things up in this regard?
> 
> I think in an early version I've used calxeda prefix but later thought
> it has to match the arm prefix. I'm also fine with
> calxeda,smmu-secure-config-access. But in case someone else screws
> this up in a similar way and needs the same driver behaviour it's odd
> if XYZ has to use the calxeda prefix instead of the arm prefix for
> this option.
> 
> > I think that the property prefix should match the compatible vendor
> > prefix. You could then argue that the compatible string itself should
> > be prefixed with "calxeda". In that case, this property would not be
> > needed at all as you could just key off the compatible string to
> > determine this characteristic. Of the options, my preference would be
> > just to leave things as is.
> 
> I think Will's main point is that Calxeda has a bug in the wiring and
> that this is not ARM's fault. Renaming the prefix will kind of
> emphasize this. In case we keep the arm prefix, how about modifying
> the description for the option to state that currently only Calxeda
> ECX-2000 screwed up the wiring?

There's also the potential for another SoC vendor to screw up the wiring in
a different way,so having the soc vendor as a prefix makes it easy to
distinguish the different cases (as opposed to a list of
arm,random-screw-up-N properties).

Will