* [PATCH 1/6] iommu/arm-smmu: Introduce driver option handling
2013-10-18 20:13 [PATCH v3 0/6] iommu/arm-smmu: Misc modifications to support SMMUs on Calxeda ECX-2000 Andreas Herrmann
@ 2013-10-18 20:13 ` Andreas Herrmann
2013-10-18 20:13 ` [PATCH 2/6] iommu/arm-smmu: Introduce bus notifier block Andreas Herrmann
` (4 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Andreas Herrmann @ 2013-10-18 20:13 UTC (permalink / raw)
To: linux-arm-kernel
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@calxeda.com>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 2/6] iommu/arm-smmu: Introduce bus notifier block
2013-10-18 20:13 [PATCH v3 0/6] iommu/arm-smmu: Misc modifications to support SMMUs on Calxeda ECX-2000 Andreas Herrmann
2013-10-18 20:13 ` [PATCH 1/6] iommu/arm-smmu: Introduce driver option handling Andreas Herrmann
@ 2013-10-18 20:13 ` Andreas Herrmann
2013-10-31 0:46 ` Will Deacon
2013-10-18 20:13 ` [PATCH 3/6] iommu/arm-smmu: Support buggy implementations where all config accesses are secure Andreas Herrmann
` (3 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Andreas Herrmann @ 2013-10-18 20:13 UTC (permalink / raw)
To: linux-arm-kernel
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@calxeda.com>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 2/6] iommu/arm-smmu: Introduce bus notifier block
2013-10-18 20:13 ` [PATCH 2/6] iommu/arm-smmu: Introduce bus notifier block Andreas Herrmann
@ 2013-10-31 0:46 ` Will Deacon
0 siblings, 0 replies; 17+ messages in thread
From: Will Deacon @ 2013-10-31 0:46 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/6] iommu/arm-smmu: Support buggy implementations where all config accesses are secure
2013-10-18 20:13 [PATCH v3 0/6] iommu/arm-smmu: Misc modifications to support SMMUs on Calxeda ECX-2000 Andreas Herrmann
2013-10-18 20:13 ` [PATCH 1/6] iommu/arm-smmu: Introduce driver option handling Andreas Herrmann
2013-10-18 20:13 ` [PATCH 2/6] iommu/arm-smmu: Introduce bus notifier block Andreas Herrmann
@ 2013-10-18 20:13 ` Andreas Herrmann
2013-10-31 0:48 ` Will Deacon
2013-10-18 20:13 ` [PATCH 4/6] iommu/arm-smmu: Introduce automatic stream-id-masking Andreas Herrmann
` (2 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Andreas Herrmann @ 2013-10-18 20:13 UTC (permalink / raw)
To: linux-arm-kernel
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@calxeda.com>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 4/6] iommu/arm-smmu: Introduce automatic stream-id-masking
2013-10-18 20:13 [PATCH v3 0/6] iommu/arm-smmu: Misc modifications to support SMMUs on Calxeda ECX-2000 Andreas Herrmann
` (2 preceding siblings ...)
2013-10-18 20:13 ` [PATCH 3/6] iommu/arm-smmu: Support buggy implementations where all config accesses are secure Andreas Herrmann
@ 2013-10-18 20:13 ` Andreas Herrmann
2013-10-31 17:55 ` Will Deacon
2013-10-18 20:13 ` [PATCH 5/6] ARM: dts: Add nodes for SMMUs on Calxeda ECX-2000 Andreas Herrmann
2013-10-18 20:13 ` [PATCH 6/6] documentation/iommu: Update description of ARM System MMU binding Andreas Herrmann
5 siblings, 1 reply; 17+ messages in thread
From: Andreas Herrmann @ 2013-10-18 20:13 UTC (permalink / raw)
To: linux-arm-kernel
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@calxeda.com>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 4/6] iommu/arm-smmu: Introduce automatic stream-id-masking
2013-10-18 20:13 ` [PATCH 4/6] iommu/arm-smmu: Introduce automatic stream-id-masking Andreas Herrmann
@ 2013-10-31 17:55 ` Will Deacon
0 siblings, 0 replies; 17+ messages in thread
From: Will Deacon @ 2013-10-31 17:55 UTC (permalink / raw)
To: linux-arm-kernel
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@calxeda.com>
> ---
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 5/6] ARM: dts: Add nodes for SMMUs on Calxeda ECX-2000
2013-10-18 20:13 [PATCH v3 0/6] iommu/arm-smmu: Misc modifications to support SMMUs on Calxeda ECX-2000 Andreas Herrmann
` (3 preceding siblings ...)
2013-10-18 20:13 ` [PATCH 4/6] iommu/arm-smmu: Introduce automatic stream-id-masking Andreas Herrmann
@ 2013-10-18 20:13 ` Andreas Herrmann
2013-10-31 1:15 ` Will Deacon
2013-10-18 20:13 ` [PATCH 6/6] documentation/iommu: Update description of ARM System MMU binding Andreas Herrmann
5 siblings, 1 reply; 17+ messages in thread
From: Andreas Herrmann @ 2013-10-18 20:13 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
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 at 920000000 {
+ ranges = <0x9 0x20000000 0x9 0x20000000 0x290000>;
+ #address-cells = <2>;
+ #size-cells = <1>;
+ compatible = "simple-bus";
+ interrupt-parent = <&intc>;
+
+ smmu_mac0: smmu at 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 at 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 at 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 at ffe08000 {
+ sata: sata at 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 at ffe0e000 {
@@ -208,18 +209,20 @@
clock-names = "apb_pclk";
};
- ethernet at fff50000 {
+ mac0: ethernet at 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 at fff51000 {
+ mac1: ethernet at 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 at 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 5/6] ARM: dts: Add nodes for SMMUs on Calxeda ECX-2000
2013-10-18 20:13 ` [PATCH 5/6] ARM: dts: Add nodes for SMMUs on Calxeda ECX-2000 Andreas Herrmann
@ 2013-10-31 1:15 ` Will Deacon
2013-10-31 8:58 ` Andreas Herrmann
0 siblings, 1 reply; 17+ messages in thread
From: Will Deacon @ 2013-10-31 1:15 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Oct 18, 2013 at 09:13:14PM +0100, Andreas Herrmann wrote:
> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
> ---
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 5/6] ARM: dts: Add nodes for SMMUs on Calxeda ECX-2000
2013-10-31 1:15 ` Will Deacon
@ 2013-10-31 8:58 ` Andreas Herrmann
0 siblings, 0 replies; 17+ messages in thread
From: Andreas Herrmann @ 2013-10-31 8:58 UTC (permalink / raw)
To: linux-arm-kernel
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@calxeda.com>
> > ---
> > 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 6/6] documentation/iommu: Update description of ARM System MMU binding
2013-10-18 20:13 [PATCH v3 0/6] iommu/arm-smmu: Misc modifications to support SMMUs on Calxeda ECX-2000 Andreas Herrmann
` (4 preceding siblings ...)
2013-10-18 20:13 ` [PATCH 5/6] ARM: dts: Add nodes for SMMUs on Calxeda ECX-2000 Andreas Herrmann
@ 2013-10-18 20:13 ` Andreas Herrmann
2013-10-31 1:17 ` Will Deacon
5 siblings, 1 reply; 17+ messages in thread
From: Andreas Herrmann @ 2013-10-18 20:13 UTC (permalink / raw)
To: linux-arm-kernel
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.
+
Example:
smmu {
@@ -67,4 +78,5 @@ Example:
*/
mmu-masters = <&dma0 0xd01d 0xd01e>,
<&dma1 0xd11c>;
+ arm,smmu-isolate-devices;
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 6/6] documentation/iommu: Update description of ARM System MMU binding
2013-10-18 20:13 ` [PATCH 6/6] documentation/iommu: Update description of ARM System MMU binding Andreas Herrmann
@ 2013-10-31 1:17 ` Will Deacon
2013-10-31 6:45 ` Rob Herring
0 siblings, 1 reply; 17+ messages in thread
From: Will Deacon @ 2013-10-31 1:17 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 6/6] documentation/iommu: Update description of ARM System MMU binding
2013-10-31 1:17 ` Will Deacon
@ 2013-10-31 6:45 ` Rob Herring
2013-10-31 9:16 ` Andreas Herrmann
0 siblings, 1 reply; 17+ messages in thread
From: Rob Herring @ 2013-10-31 6:45 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Oct 30, 2013 at 8:17 PM, Will Deacon <will.deacon@arm.com> 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@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?
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 6/6] documentation/iommu: Update description of ARM System MMU binding
2013-10-31 6:45 ` Rob Herring
@ 2013-10-31 9:16 ` Andreas Herrmann
2013-10-31 16:02 ` Will Deacon
0 siblings, 1 reply; 17+ messages in thread
From: Andreas Herrmann @ 2013-10-31 9:16 UTC (permalink / raw)
To: linux-arm-kernel
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@arm.com> 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@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?
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 6/6] documentation/iommu: Update description of ARM System MMU binding
2013-10-31 9:16 ` Andreas Herrmann
@ 2013-10-31 16:02 ` Will Deacon
0 siblings, 0 replies; 17+ messages in thread
From: Will Deacon @ 2013-10-31 16:02 UTC (permalink / raw)
To: linux-arm-kernel
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@arm.com> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4/6] iommu/arm-smmu: Introduce automatic stream-id-masking
2014-01-30 18:18 [PATCH 0/6] iommu/arm-smmu: Misc modifications to support SMMUs on Calxeda ECX-2000 Andreas Herrmann
@ 2014-01-30 18:18 ` Andreas Herrmann
0 siblings, 0 replies; 17+ messages in thread
From: Andreas Herrmann @ 2014-01-30 18:18 UTC (permalink / raw)
To: linux-arm-kernel
Try to determine mask/id values that match several stream IDs of a
master device when doing Stream ID matching. Thus the number of used
SMR groups that are required to map all stream IDs of a master device
to a context should be less than the number of SMR groups used so far
(currently one SMR group is used for one stream ID).
Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
---
drivers/iommu/arm-smmu.c | 172 ++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 157 insertions(+), 15 deletions(-)
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 4a32fde..31d9458 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -42,6 +42,7 @@
#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
+#include <linux/bitops.h>
#include <linux/amba/bus.h>
@@ -336,8 +337,9 @@ struct arm_smmu_master {
* SMMU chain.
*/
struct rb_node node;
- int num_streamids;
+ u32 num_streamids;
u16 streamids[MAX_MASTER_STREAMIDS];
+ int num_s2crs;
/*
* We only need to allocate these on the root SMMU, as we
@@ -382,6 +384,9 @@ struct arm_smmu_device {
u32 num_context_irqs;
unsigned int *irqs;
+ u32 smr_mask_mask;
+ u32 smr_id_mask;
+
struct list_head list;
struct rb_root masters;
};
@@ -1033,10 +1038,140 @@ static void arm_smmu_domain_destroy(struct iommu_domain *domain)
kfree(smmu_domain);
}
+/*
+ * For a given set N of 2**order different stream IDs (no duplicates
+ * please!) we determine values mask and id such that
+ *
+ * (1) (x & mask) == id
+ *
+ * for each stream ID x from the given set N.
+ *
+ * If the number of bits that are set in mask equals n, then there
+ * exist 2**n different values y for which
+ *
+ * (2) (y & mask) == id
+ *
+ * Thus if n equals order we know that for the calculated mask and id
+ * values there are exactly 2**order == 2**n stream IDs for which (1)
+ * is true. And we finally can use mask and id to configure an SMR to
+ * match all stream IDs in the set N.
+ */
+static int determine_smr_mask(struct arm_smmu_device *smmu,
+ struct arm_smmu_master *master,
+ struct arm_smmu_smr *smr, int start, int order)
+{
+ u16 i, zero_bits_mask, one_bits_mask, const_mask;
+ int nr;
+
+ nr = 1 << order;
+
+ 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 (i == order) {
+ /*
+ * We have found a mask/id pair that matches exactly
+ * nr = 2**order stream IDs which we used for its
+ * calculation.
+ */
+ smr->mask = ~const_mask;
+ smr->id = one_bits_mask;
+ } else {
+ /*
+ * No usable mask/id pair for this set of streamids.
+ * If i > order then mask/id would match more than nr
+ * streamids.
+ * If i < order then mask/id would match less than nr
+ * streamids. (In this case we potentially have used
+ * some duplicate streamids for the calculation.)
+ */
+ return 1;
+ }
+
+ if (((smr->mask & smmu->smr_mask_mask) != smr->mask) ||
+ ((smr->id & smmu->smr_id_mask) != smr->id))
+ /* insufficient number of mask/id bits */
+ return 1;
+
+ return 0;
+}
+
+static int determine_smr_mapping(struct arm_smmu_device *smmu,
+ struct arm_smmu_master *master,
+ struct arm_smmu_smr *smrs, int max_smrs)
+{
+ int nr_sid, nr, i, bit, start;
+
+ /*
+ * This function is called only once -- when a master is added
+ * to a domain. If master->num_s2crs != 0 then this master
+ * was already added to a domain.
+ */
+ if (master->num_s2crs)
+ return -EINVAL;
+
+ 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 < 0)
+ return 0;
+
+ /*
+ * iterate over power-of-2 numbers to determine
+ * largest possible mask/id pair for stream matching
+ * of next 2**i streamids
+ */
+ for (i = bit; i >= 0; i--) {
+ if (!determine_smr_mask(smmu, master,
+ &smrs[master->num_s2crs],
+ start, i))
+ break;
+ }
+
+ if (i < 0)
+ goto out;
+
+ nr = 1 << i;
+ nr_sid -= nr;
+ start += nr;
+ master->num_s2crs++;
+ } while (master->num_s2crs <= max_smrs);
+
+out:
+ if (nr_sid) {
+ /* not enough mapping groups available */
+ master->num_s2crs = 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);
@@ -1046,31 +1181,31 @@ 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(smmu, 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_s2crs; ++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_s2crs; ++i) {
u32 reg = SMR_VALID | smrs[i].id << SMR_ID_SHIFT |
smrs[i].mask << SMR_MASK_SHIFT;
writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_SMR(smrs[i].idx));
@@ -1079,9 +1214,11 @@ static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
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_s2crs = 0;
+err_free_smrs:
kfree(smrs);
return -ENOSPC;
}
@@ -1144,11 +1281,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) {
+ if (!master->num_s2crs)
+ master->num_s2crs = master->num_streamids;
+ for (i = 0; i < master->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));
}
@@ -1758,6 +1898,8 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
mask, sid);
return -ENODEV;
}
+ smmu->smr_mask_mask = mask;
+ smmu->smr_id_mask = sid;
dev_notice(smmu->dev,
"\tstream matching with %u register groups, mask 0x%x",
--
1.7.9.5
^ permalink raw reply related [flat|nested] 17+ messages in thread