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

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

[Andreas Herrmann]

Hi,

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

Note that patch 7/7 (Introduce automatic stream-id-masking) is not
required to support SMMUs on Calxeda ECX-2000. In fact the algorithm
implemented won't help to determine a suitable smr-mask for 10
StreamIDs that we have for SATA.

In case of 2, 4, 8, ... (power-of-two) ... number of StreamIDs for a
master device it might be able (depending on further restriction for
the StreamIDs) to determine the right mask which then allows to use
only one SMR group for stream matching for this master instead of
using several SMR groups (one for each StreamID).

Changelog:
v2:
- Modified options handling
- Added patch to document new DT properties (that select driver options)
- Added patch to introduce automatic stream-id-masking
v1:
   http://marc.info/?l=linux-arm-kernel&m=138122450023564


Regards,
Andreas

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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index b632bcd..478c8ad 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,36 @@ 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;
+	} while (arm_smmu_options[++i].opt);
+}
+
+static const char *get_driver_option_property(u32 opt)
+{
+	int i = 0;
+	do {
+		if (arm_smmu_options[i].opt == opt)
+			return arm_smmu_options[i].prop;
+	} while (arm_smmu_options[++i].opt);
+	return NULL;
+}
+
 static struct arm_smmu_master *find_smmu_master(struct arm_smmu_device *smmu,
 						struct device_node *dev_node)
 {
@@ -1791,6 +1825,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

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

[Will Deacon]

On Wed, Oct 09, 2013 at 11:38:00PM +0100, Andreas Herrmann wrote:
> 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>
> ---

Looks good to me, thanks Andreas!

Will

[PATCH 2/7] 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 |   53 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 478c8ad..87239e8 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)
@@ -1983,6 +1984,56 @@ 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;
+	void __iomem *gr0_base;
+	u32 cr0;
+	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);
+		}
+
+		gr0_base = ARM_SMMU_GR0_NS(smmu);
+		cr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
+		cr0 |= sCR0_USFCFG;
+		writel(cr0, gr0_base + ARM_SMMU_GR0_sCR0);
+
+		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", },
@@ -2019,6 +2070,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/7] iommu/arm-smmu: Introduce bus notifier block

[Will Deacon]

On Wed, Oct 09, 2013 at 11:38:01PM +0100, Andreas Herrmann wrote:
>  drivers/iommu/arm-smmu.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 478c8ad..87239e8 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)
> @@ -1983,6 +1984,56 @@ 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;
> +	void __iomem *gr0_base;
> +	u32 cr0;
> +	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);

We need to find a better way of doing this; I'm really not happy hardcoding
that size limit and hoping for the best.

> +		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);
> +		}
> +
> +		gr0_base = ARM_SMMU_GR0_NS(smmu);
> +		cr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
> +		cr0 |= sCR0_USFCFG;
> +		writel(cr0, gr0_base + ARM_SMMU_GR0_sCR0);

Why do you need to enable this flag? If no mapping is found, that means *we*
screwed something up, so we should report that and not rely on the hardware
potentially raising a fault. I also prefer to allow passthrough for devices
that don't hit in the stream table, because it allows people to ignore the SMMU
in their DT if they want to.

Will

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

[Andreas Herrmann]

On Thu, Oct 10, 2013 at 12:12:17PM -0400, Will Deacon wrote:
> On Wed, Oct 09, 2013 at 11:38:01PM +0100, Andreas Herrmann wrote:
> >  drivers/iommu/arm-smmu.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> > 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 478c8ad..87239e8 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)
> > @@ -1983,6 +1984,56 @@ 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;
> > +	void __iomem *gr0_base;
> > +	u32 cr0;
> > +	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);
> 
> We need to find a better way of doing this; I'm really not happy hardcoding
> that size limit and hoping for the best.

Hmm, seems that mid-term we should try to enlarge this area by another
128M if a mapping fails due to this limit. I think Joerg's amd_iommu
driver does this.

(But at the same time I'd prefer to start with even a hardcoded value,
or to simply introduce some option to set this fixed limit per master
or per smmu.)

> > +		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);
> > +		}
> > +
> > +		gr0_base = ARM_SMMU_GR0_NS(smmu);
> > +		cr0 = readl_relaxed(gr0_base + ARM_SMMU_GR0_sCR0);
> > +		cr0 |= sCR0_USFCFG;
> > +		writel(cr0, gr0_base + ARM_SMMU_GR0_sCR0);
> 
> Why do you need to enable this flag? If no mapping is found, that means *we*
> screwed something up, so we should report that and not rely on the hardware
> potentially raising a fault. I also prefer to allow passthrough for devices
> that don't hit in the stream table, because it allows people to ignore the SMMU
> in their DT if they want to.

You are right that's not strictly required for device isolation.

(but it helped to learn about relevant stream-ids that were not
covered by by my DTS ;-)


Andreas

[PATCH 3/7] iommu/arm-smmu: Introduce stream ID masking

[Andreas Herrmann]

Ie. use a mask based on smr_mask_bits to map all stream IDs of an SMMU
to one context.

This behaviour is controlled per SMMU node with DT property
"arm,smmu-mask-stream-ids" and is only allowed if just a single master
is attached to an SMMU. If the option is specified, all stream-ids
that are provided in DT for the single master have no relevance.

This is useful/convenient if a master has more than 8 stream-ids or if
not all stream-ids are known for a master device.

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

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 87239e8..f877d02 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_MASK_STREAM_IDS		(1 << 1)
 
 /* Maximum number of stream IDs assigned to a single device */
 #define MAX_MASTER_STREAMIDS		8
@@ -362,6 +363,7 @@ struct arm_smmu_device {
 
 	u32				num_mapping_groups;
 	DECLARE_BITMAP(smr_map, ARM_SMMU_MAX_SMRS);
+	u32				smr_mask_bits;
 
 	unsigned long			input_size;
 	unsigned long			s1_output_size;
@@ -373,6 +375,7 @@ struct arm_smmu_device {
 
 	struct list_head		list;
 	struct rb_root			masters;
+	u32				num_masters;
 };
 
 struct arm_smmu_cfg {
@@ -410,6 +413,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_MASK_STREAM_IDS, "arm,smmu-mask-stream-ids" },
 	{ 0, NULL},
 };
 
@@ -475,6 +479,7 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
 
 	rb_link_node(&master->node, parent, new);
 	rb_insert_color(&master->node, &smmu->masters);
+	smmu->num_masters++;
 	return 0;
 }
 
@@ -507,8 +512,20 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 	master->of_node		= masterspec->np;
 	master->num_streamids	= masterspec->args_count;
 
-	for (i = 0; i < master->num_streamids; ++i)
-		master->streamids[i] = masterspec->args[i];
+	if (smmu->options & ARM_SMMU_OPT_MASK_STREAM_IDS) {
+		if (smmu->num_masters) {
+			dev_err(dev, "%s not supported with multiple masters\n",
+				get_driver_option_property(
+					ARM_SMMU_OPT_MASK_STREAM_IDS));
+			return -EINVAL;
+		}
+		/* set fixed streamid (0) that will be used for masking */
+		master->num_streamids = 1;
+		master->streamids[0] = 0;
+	} else {
+		for (i = 0; i < master->num_streamids; ++i)
+			master->streamids[i] = masterspec->args[i];
+	}
 
 	return insert_smmu_master(smmu, master);
 }
@@ -1050,17 +1067,20 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
 			goto err_free_smrs;
 		}
 
-		smrs[i] = (struct arm_smmu_smr) {
-			.idx	= idx,
-			.mask	= 0, /* We don't currently share SMRs */
-			.id	= master->streamids[i],
-		};
+		smrs[i].idx = idx;
+		smrs[i].id = master->streamids[i];
+
+		if (smmu->options & ARM_SMMU_OPT_MASK_STREAM_IDS)
+			smrs[i].mask = smmu->smr_mask_bits;
+		else
+			smrs[i].mask = 0;
 	}
 
 	/* It worked! Now, poke the actual hardware */
 	for (i = 0; i < master->num_streamids; ++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));
 	}
 
@@ -1713,7 +1733,7 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 	}
 
 	if (id & ID0_SMS) {
-		u32 smr, sid, mask;
+		u32 smr, sid;
 
 		smmu->features |= ARM_SMMU_FEAT_STREAM_MATCH;
 		smmu->num_mapping_groups = (id >> ID0_NUMSMRG_SHIFT) &
@@ -1729,18 +1749,18 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 		writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(0));
 		smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(0));
 
-		mask = (smr >> SMR_MASK_SHIFT) & SMR_MASK_MASK;
+		smmu->smr_mask_bits = (smr >> SMR_MASK_SHIFT) & SMR_MASK_MASK;
 		sid = (smr >> SMR_ID_SHIFT) & SMR_ID_MASK;
-		if ((mask & sid) != sid) {
+		if ((smmu->smr_mask_bits & sid) != sid) {
 			dev_err(smmu->dev,
 				"SMR mask bits (0x%x) insufficient for ID field (0x%x)\n",
-				mask, sid);
+				smmu->smr_mask_bits, sid);
 			return -ENODEV;
 		}
 
 		dev_notice(smmu->dev,
 			   "\tstream matching with %u register groups, mask 0x%x",
-			   smmu->num_mapping_groups, mask);
+			   smmu->num_mapping_groups, smmu->smr_mask_bits);
 	}
 
 	/* ID1 */
-- 
1.7.9.5

[PATCH 4/7] 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 f877d02..91316a8 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -51,6 +51,7 @@
 /* Driver options */
 #define ARM_SMMU_OPT_ISOLATE_DEVICES		(1 << 0)
 #define ARM_SMMU_OPT_MASK_STREAM_IDS		(1 << 1)
+#define ARM_SMMU_OPT_SECURE_CONFIG_ACCESS	(1 << 2)
 
 /* Maximum number of stream IDs assigned to a single device */
 #define MAX_MASTER_STREAMIDS		8
@@ -65,6 +66,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)
@@ -414,6 +424,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_MASK_STREAM_IDS, "arm,smmu-mask-stream-ids" },
+	{ ARM_SMMU_OPT_SECURE_CONFIG_ACCESS, "arm,smmu-secure-config-access" },
 	{ 0, NULL},
 };
 
@@ -663,16 +674,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,
@@ -1622,8 +1633,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) {
@@ -1643,7 +1654,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);
@@ -1662,7 +1673,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)
@@ -2000,7 +2011,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 4/7] iommu/arm-smmu: Support buggy implementations where all config accesses are secure

[Will Deacon]

On Wed, Oct 09, 2013 at 11:38:03PM +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.
> 
> 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 f877d02..91316a8 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -51,6 +51,7 @@
>  /* Driver options */
>  #define ARM_SMMU_OPT_ISOLATE_DEVICES		(1 << 0)
>  #define ARM_SMMU_OPT_MASK_STREAM_IDS		(1 << 1)
> +#define ARM_SMMU_OPT_SECURE_CONFIG_ACCESS	(1 << 2)
>  
>  /* Maximum number of stream IDs assigned to a single device */
>  #define MAX_MASTER_STREAMIDS		8
> @@ -65,6 +66,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))

You have a slight issue with the ordering of your patches, as this macro is
used by the stream masking patch, but this patch is based on top of the
latter.

Can you introduce this patch first please? I think it's ok for merging, but
I still have reservations with the other one.

Will

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

[Andreas Herrmann]

On Thu, Oct 10, 2013 at 11:47:27AM -0400, Will Deacon wrote:
> On Wed, Oct 09, 2013 at 11:38:03PM +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.
> > 
> > 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 f877d02..91316a8 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -51,6 +51,7 @@
> >  /* Driver options */
> >  #define ARM_SMMU_OPT_ISOLATE_DEVICES		(1 << 0)
> >  #define ARM_SMMU_OPT_MASK_STREAM_IDS		(1 << 1)
> > +#define ARM_SMMU_OPT_SECURE_CONFIG_ACCESS	(1 << 2)
> >  
> >  /* Maximum number of stream IDs assigned to a single device */
> >  #define MAX_MASTER_STREAMIDS		8
> > @@ -65,6 +66,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))
> 
> You have a slight issue with the ordering of your patches, as this macro is
> used by the stream masking patch, but this patch is based on top of the
> latter.
> 
> Can you introduce this patch first please? I think it's ok for merging, but
> I still have reservations with the other one.

Oops, I've done some shuffling to update the first patch in the
series. Obviously I mixed something up during this.

I'll fix this asap.


Andreas

[PATCH 5/7] 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    |   45 +++++++++++++++++++++++++++++++++++--
 arch/arm/boot/dts/ecx-common.dtsi |    9 +++++---
 2 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/ecx-2000.dts b/arch/arm/boot/dts/ecx-2000.dts
index 139b40c..270f052 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,46 @@
 			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>;
+			#global-interrupts = <1>;
+			interrupts = <0 114 4 0 114 4>;
+			arm,smmu-secure-config-access;
+			arm,smmu-isolate-devices;
+			arm,smmu-mask-stream-ids;
+		};
+	};
+
 };
 
 /include/ "ecx-common.dtsi"
diff --git a/arch/arm/boot/dts/ecx-common.dtsi b/arch/arm/boot/dts/ecx-common.dtsi
index e8559b7..50e401e 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 = <0>;
 		};
 
 		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 {
-- 
1.7.9.5

[PATCH 6/7] 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         |   23 ++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index e34c6cd..6415a88 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -48,6 +48,28 @@ 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-mask-stream-ids : Enable mapping of all StreamIDs to one
+                             context for this SMMU. It is only allowed
+                             if there is one single master. It saves
+                             the need to know and specify all
+                             StreamIDs in cases where just one master
+                             is attached to an SMMU. If StreamIDs are
+                             specified in the mmu-masters property
+                             those are ignored. (The referred device
+                             node must still have a "#stream-id-cells"
+                             property. But it can be set to 0.)
+
+- 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 +89,5 @@ Example:
                  */
                 mmu-masters = <&dma0 0xd01d 0xd01e>,
                               <&dma1 0xd11c>;
+                arm,smmu-isolate-devices;
         };
-- 
1.7.9.5

[PATCH 7/7] 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 |   79 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 63 insertions(+), 16 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 91316a8..4e8ceab 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>
@@ -341,6 +342,9 @@ struct arm_smmu_master {
 	struct rb_node			node;
 	int				num_streamids;
 	u16				streamids[MAX_MASTER_STREAMIDS];
+	int				num_smrs;
+	u16				smr_mask;
+	u16				smr_id;
 
 	/*
 	 * We only need to allocate these on the root SMMU, as we
@@ -530,14 +534,11 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 					ARM_SMMU_OPT_MASK_STREAM_IDS));
 			return -EINVAL;
 		}
-		/* set fixed streamid (0) that will be used for masking */
-		master->num_streamids = 1;
-		master->streamids[0] = 0;
-	} else {
-		for (i = 0; i < master->num_streamids; ++i)
-			master->streamids[i] = masterspec->args[i];
 	}
 
+	for (i = 0; i < master->num_streamids; ++i)
+		master->streamids[i] = masterspec->args[i];
+
 	return insert_smmu_master(smmu, master);
 }
 
@@ -1049,6 +1050,41 @@ static void arm_smmu_domain_destroy(struct iommu_domain *domain)
 	kfree(smmu_domain);
 }
 
+/*
+ * no duplicates streamids please
+ */
+static void determine_smr_mapping(struct arm_smmu_device *smmu,
+				struct arm_smmu_master *master)
+{
+	int nr_sid;
+	u16 i, v1, v2, const_mask;
+
+	if (smmu->options & ARM_SMMU_OPT_MASK_STREAM_IDS) {
+		master->smr_mask = smmu->smr_mask_bits;
+		master->smr_id = 0;
+		return;
+	}
+
+	nr_sid = master->num_streamids;
+	if (!is_power_of_2(nr_sid))
+		return;
+
+	v1 = 0;
+	v2 = -1;
+	for (i = 0; i < nr_sid; i++) {
+		v1 |= master->streamids[i];	/* for const 0 bits */
+		v2 &= ~(master->streamids[i]);	/* const 1 bits */
+	}
+	const_mask = (~v1) | v2;	/* const bits (either 0 or 1) */
+
+	v1 = hweight16(~const_mask);
+	if ((1 << v1) == nr_sid) {
+		/* if smr_mask is set, only 1 SMR group is used smr[0] = 0 */
+		master->smr_mask = ~const_mask;
+		master->smr_id = v1 & const_mask;
+	}
+}
+
 static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
 					  struct arm_smmu_master *master)
 {
@@ -1062,15 +1098,22 @@ 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);
+	determine_smr_mapping(smmu, master);
+
+	if (master->smr_mask)
+		master->num_smrs = 1;
+	else
+		master->num_smrs = master->num_streamids;
+
+	smrs = kmalloc(sizeof(*smrs) * master->num_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);
+			master->num_smrs, master->of_node->name);
 		return -ENOMEM;
 	}
 
 	/* Allocate the SMRs on the root SMMU */
-	for (i = 0; i < master->num_streamids; ++i) {
+	for (i = 0; i < master->num_smrs; ++i) {
 		int idx = __arm_smmu_alloc_bitmap(smmu->smr_map, 0,
 						  smmu->num_mapping_groups);
 		if (IS_ERR_VALUE(idx)) {
@@ -1079,16 +1122,18 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
 		}
 
 		smrs[i].idx = idx;
-		smrs[i].id = master->streamids[i];
 
-		if (smmu->options & ARM_SMMU_OPT_MASK_STREAM_IDS)
-			smrs[i].mask = smmu->smr_mask_bits;
-		else
+		if (master->smr_mask) {
+			smrs[i].mask = master->smr_mask;
+			smrs[i].id = master->smr_id;
+		} else {
 			smrs[i].mask = 0;
+			smrs[i].id = master->streamids[i];
+		}
 	}
 
 	/* It worked! Now, poke the actual hardware */
-	for (i = 0; i < master->num_streamids; ++i) {
+	for (i = 0; i < master->num_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);
@@ -1139,7 +1184,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);
 
@@ -1163,11 +1208,13 @@ 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_smrs ? master->num_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 7/7] iommu/arm-smmu: Introduce automatic stream-id-masking

[Will Deacon]

On Wed, Oct 09, 2013 at 11:38:06PM +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 |   79 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 63 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 91316a8..4e8ceab 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>
> @@ -341,6 +342,9 @@ struct arm_smmu_master {
>  	struct rb_node			node;
>  	int				num_streamids;
>  	u16				streamids[MAX_MASTER_STREAMIDS];
> +	int				num_smrs;

This is easy to confuse with smmu->num_mapping_groups, but is actually the
number of SMRs in use by this master, right? Maybe tweaking the name
(num_used_smrs?) would make this clearer.

> +	u16				smr_mask;
> +	u16				smr_id;
>  
>  	/*
>  	 * We only need to allocate these on the root SMMU, as we
> @@ -530,14 +534,11 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
>  					ARM_SMMU_OPT_MASK_STREAM_IDS));
>  			return -EINVAL;
>  		}
> -		/* set fixed streamid (0) that will be used for masking */
> -		master->num_streamids = 1;
> -		master->streamids[0] = 0;
> -	} else {
> -		for (i = 0; i < master->num_streamids; ++i)
> -			master->streamids[i] = masterspec->args[i];
>  	}
>  
> +	for (i = 0; i < master->num_streamids; ++i)
> +		master->streamids[i] = masterspec->args[i];
> +
>  	return insert_smmu_master(smmu, master);
>  }
>  
> @@ -1049,6 +1050,41 @@ static void arm_smmu_domain_destroy(struct iommu_domain *domain)
>  	kfree(smmu_domain);
>  }
>  
> +/*
> + * no duplicates streamids please
> + */

We could probably check for that actually in register_smmu_master.

> +static void determine_smr_mapping(struct arm_smmu_device *smmu,
> +				struct arm_smmu_master *master)
> +{
> +	int nr_sid;
> +	u16 i, v1, v2, const_mask;

The bitwise stuff later on could use some more meaningful identifiers
(although the comments do help).

> +
> +	if (smmu->options & ARM_SMMU_OPT_MASK_STREAM_IDS) {
> +		master->smr_mask = smmu->smr_mask_bits;
> +		master->smr_id = 0;
> +		return;
> +	}
> +
> +	nr_sid = master->num_streamids;
> +	if (!is_power_of_2(nr_sid))
> +		return;

As I mentioned before, we could do better than this if we forced the DT to
contain complete topological information. Then we could round up to the next
power of two and check that we didn't accidentally include another device.
What is your opinion on this?

> +	v1 = 0;
> +	v2 = -1;

I'd rather this was written as 0xffff;

> +	for (i = 0; i < nr_sid; i++) {
> +		v1 |= master->streamids[i];	/* for const 0 bits */
> +		v2 &= ~(master->streamids[i]);	/* const 1 bits */
> +	}
> +	const_mask = (~v1) | v2;	/* const bits (either 0 or 1) */
> +
> +	v1 = hweight16(~const_mask);
> +	if ((1 << v1) == nr_sid) {
> +		/* if smr_mask is set, only 1 SMR group is used smr[0] = 0 */
> +		master->smr_mask = ~const_mask;
> +		master->smr_id = v1 & const_mask;
> +	}

Hehe, this is cool, nice one! I originally thought you could just xor stuff,
but that ends up being slightly nasty because it all has to be done pairwise.

> +}
> +
>  static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
>  					  struct arm_smmu_master *master)
>  {
> @@ -1062,15 +1098,22 @@ 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);
> +	determine_smr_mapping(smmu, master);
> +
> +	if (master->smr_mask)
> +		master->num_smrs = 1;

So the next challenge would be to allocate one SMR using your power-of-2
trick, then mop up what's left with individual SMR entries.

Will

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

[Andreas Herrmann]

On Thu, Oct 10, 2013 at 12:05:39PM -0400, Will Deacon wrote:
> On Wed, Oct 09, 2013 at 11:38:06PM +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 |   79 ++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 63 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index 91316a8..4e8ceab 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>
> > @@ -341,6 +342,9 @@ struct arm_smmu_master {
> >  	struct rb_node			node;
> >  	int				num_streamids;
> >  	u16				streamids[MAX_MASTER_STREAMIDS];
> > +	int				num_smrs;
> 
> This is easy to confuse with smmu->num_mapping_groups, but is actually the
> number of SMRs in use by this master, right? Maybe tweaking the name
> (num_used_smrs?) would make this clearer.
> 
> > +	u16				smr_mask;
> > +	u16				smr_id;
> >  
> >  	/*
> >  	 * We only need to allocate these on the root SMMU, as we
> > @@ -530,14 +534,11 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
> >  					ARM_SMMU_OPT_MASK_STREAM_IDS));
> >  			return -EINVAL;
> >  		}
> > -		/* set fixed streamid (0) that will be used for masking */
> > -		master->num_streamids = 1;
> > -		master->streamids[0] = 0;
> > -	} else {
> > -		for (i = 0; i < master->num_streamids; ++i)
> > -			master->streamids[i] = masterspec->args[i];
> >  	}
> >  
> > +	for (i = 0; i < master->num_streamids; ++i)
> > +		master->streamids[i] = masterspec->args[i];
> > +
> >  	return insert_smmu_master(smmu, master);
> >  }
> >  
> > @@ -1049,6 +1050,41 @@ static void arm_smmu_domain_destroy(struct iommu_domain *domain)
> >  	kfree(smmu_domain);
> >  }
> >  
> > +/*
> > + * no duplicates streamids please
> > + */
> 
> We could probably check for that actually in register_smmu_master.
> 
> > +static void determine_smr_mapping(struct arm_smmu_device *smmu,
> > +				struct arm_smmu_master *master)
> > +{
> > +	int nr_sid;
> > +	u16 i, v1, v2, const_mask;
> 
> The bitwise stuff later on could use some more meaningful identifiers
> (although the comments do help).
> 
> > +
> > +	if (smmu->options & ARM_SMMU_OPT_MASK_STREAM_IDS) {
> > +		master->smr_mask = smmu->smr_mask_bits;
> > +		master->smr_id = 0;
> > +		return;
> > +	}
> > +
> > +	nr_sid = master->num_streamids;
> > +	if (!is_power_of_2(nr_sid))
> > +		return;
> 
> As I mentioned before, we could do better than this if we forced the DT to
> contain complete topological information. Then we could round up to the next
> power of two and check that we didn't accidentally include another device.
> What is your opinion on this?
> 
> > +	v1 = 0;
> > +	v2 = -1;
> 
> I'd rather this was written as 0xffff;
> 
> > +	for (i = 0; i < nr_sid; i++) {
> > +		v1 |= master->streamids[i];	/* for const 0 bits */
> > +		v2 &= ~(master->streamids[i]);	/* const 1 bits */
> > +	}
> > +	const_mask = (~v1) | v2;	/* const bits (either 0 or 1) */
> > +
> > +	v1 = hweight16(~const_mask);
> > +	if ((1 << v1) == nr_sid) {
> > +		/* if smr_mask is set, only 1 SMR group is used smr[0] = 0 */
> > +		master->smr_mask = ~const_mask;
> > +		master->smr_id = v1 & const_mask;
> > +	}
> 
> Hehe, this is cool, nice one! I originally thought you could just xor stuff,
> but that ends up being slightly nasty because it all has to be done pairwise.
> 
> > +}
> > +
> >  static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
> >  					  struct arm_smmu_master *master)
> >  {
> > @@ -1062,15 +1098,22 @@ 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);
> > +	determine_smr_mapping(smmu, master);
> > +
> > +	if (master->smr_mask)
> > +		master->num_smrs = 1;
> 
> So the next challenge would be to allocate one SMR using your power-of-2
> trick, then mop up what's left with individual SMR entries.

BTW, the coding style of this patch might be nasty. I had cut it out
of some later version of my prototype driver and adapted it to your
driver.

So you don't generally object and it seems you'd accept this patch (if
minor things fixed) and I should scrub the other mask-stream-id patch.

Sigh ... so I need to figure out how to do the mop-up.


Andreas

PS: Even w/o the "mop-up" I could get my sata-smmu combination get working
    with this patch as it is.
    (I'd just need to add some fake streamids to get to a power-of-2. And
    yes, while I am writing this I am pretty sure you'd hate this ;-)