[PATCHv4 0/7] Unifying SMMU driver among Tegra SoCs

[PATCHv4 0/7] Unifying SMMU driver among Tegra SoCs

[Hiroshi Doyu]

Hi,

This series provides:

(1) Unified SMMU driver among Tegra SoCs
(2) Multiple Address Space support(MASID) in IOMMU(SMMMU)
(3) Tegra IOMMU'able devices, most of platform devices are IOMMU'able.

There's been some discussion[1] about device population order, and I
added a IOMMU hook in driver core:

  [PATCHv4 2/7] driver/core: Populate IOMMU'able devices in order

which is based on the following RFC patch:

  [RFC][PATCHv3+ 1/2] driver/core: Add of_iommu_attach()
  http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006914.html

Tested IOMMU functionality with T30 SD/MMC. Any further testing with
T114 and/or other devices would be really appreciated.

v4:
Add a hook in driver core to control device populatin order.
Introduced arm,smmu "mmu-master" binding instead of tegra own.
Removed DT patches from this series.

v3:
Updated based on Stephen Warren's feedback

v2:
Updated based on Thierry Reding's and Stephen Warren's feedback

v2: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/181888.html
v1: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/180267.html

Available in the git repository at:

  git://git-HoETi0wPbwRDw2glCA4ptUEOCMrvLtNR@public.gmane.org/user/hdoyu/linux.git smmu-upstreaming@20131111

Hiroshi Doyu (7):
  ARM: tegra: Create a DT header defining SWGROUP ID
  driver/core: Populate IOMMU'able devices in order
  iommu/tegra: smmu: Register IOMMU'able devices dynamically
  iommu/tegra: smmu: Calculate ASID register offset by ID
  iommu/tegra: smmu: Support "mmu-masters" binding
  iommu/tegra: smmu: Rename hwgrp -> swgroups
  iommu/tegra: smmu: Allow duplicate ASID wirte

 .../bindings/iommu/nvidia,tegra30-smmu.txt         |  28 +-
 drivers/base/dd.c                                  |   5 +
 drivers/iommu/Kconfig                              |   1 +
 drivers/iommu/of_iommu.c                           |  33 +++
 drivers/iommu/tegra-smmu.c                         | 301 ++++++++++++++-------
 include/dt-bindings/memory/tegra-swgroup.h         |  48 ++++
 include/linux/of_iommu.h                           |   7 +
 7 files changed, 318 insertions(+), 105 deletions(-)
 create mode 100644 include/dt-bindings/memory/tegra-swgroup.h

-- 
1.8.1.5

[PATCHv4 1/7] ARM: tegra: Create a DT header defining SWGROUP ID

[Hiroshi Doyu]

Create a header file to define the swgroup IDs used by the IOMMU(SMMU)
binding. "swgroup" is a group of H/W clients which a Tegra SoC
supports. This unique ID can be used to calculate MC_SMMU_<swgroup
name>_ASID_0 register offset and MC_<swgroup name>_HOTRESET_*_0
register bit. This will allow the same header to be used by both
device tree files, and drivers implementing this binding, which
guarantees that the two stay in sync. This also makes device trees
more readable by using names instead of magic numbers. For HOTRESET
bit shifting we need another conversion table, which will come later.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Update:
This is almost same as the previous v3. Just TEGRA_SWGROUP_MAX is
added.
  [PATCHv3 15/19] ARM: tegra: Create a DT header defining SWGROUP ID
---
 include/dt-bindings/memory/tegra-swgroup.h | 48 ++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 include/dt-bindings/memory/tegra-swgroup.h

diff --git a/include/dt-bindings/memory/tegra-swgroup.h b/include/dt-bindings/memory/tegra-swgroup.h
new file mode 100644
index 0000000..ef41166
--- /dev/null
+++ b/include/dt-bindings/memory/tegra-swgroup.h
@@ -0,0 +1,48 @@
+/*
+ * This header provides constants for binding nvidia,swgroup ID
+ */
+
+#ifndef _DT_BINDINGS_MEMORY_TEGRA_SWGROUP_H
+#define _DT_BINDINGS_MEMORY_TEGRA_SWGROUP_H
+
+#define TEGRA_SWGROUP_AFI	0	/* 0x238 */
+#define TEGRA_SWGROUP_AVPC	1	/* 0x23c */
+#define TEGRA_SWGROUP_DC	2	/* 0x240 */
+#define TEGRA_SWGROUP_DCB	3	/* 0x244 */
+#define TEGRA_SWGROUP_EPP	4	/* 0x248 */
+#define TEGRA_SWGROUP_G2	5	/* 0x24c */
+#define TEGRA_SWGROUP_HC	6	/* 0x250 */
+#define TEGRA_SWGROUP_HDA	7	/* 0x254 */
+#define TEGRA_SWGROUP_ISP	8	/* 0x258 */
+#define TEGRA_SWGROUP_ISP2	SWGROUP_ISP
+#define TEGRA_SWGROUP_DC14	9	/* 0x490 *//* Exceptional non-linear */
+#define TEGRA_SWGROUP_DC12	10	/* 0xa88 *//* Exceptional non-linear */
+#define TEGRA_SWGROUP_MPE	11	/* 0x264 */
+#define TEGRA_SWGROUP_MSENC	SWGROUP_MPE
+#define TEGRA_SWGROUP_NV	12	/* 0x268 */
+#define TEGRA_SWGROUP_NV2	13	/* 0x26c */
+#define TEGRA_SWGROUP_PPCS	14	/* 0x270 */
+#define TEGRA_SWGROUP_SATA2	15	/* 0x274 */
+#define TEGRA_SWGROUP_SATA	16	/* 0x278 */
+#define TEGRA_SWGROUP_VDE	17	/* 0x27c */
+#define TEGRA_SWGROUP_VI	18	/* 0x280 */
+#define TEGRA_SWGROUP_VIC	19	/* 0x284 */
+#define TEGRA_SWGROUP_XUSB_HOST	20	/* 0x288 */
+#define TEGRA_SWGROUP_XUSB_DEV	21	/* 0x28c */
+#define TEGRA_SWGROUP_A9AVP	22	/* 0x290 */
+#define TEGRA_SWGROUP_TSEC	23	/* 0x294 */
+#define TEGRA_SWGROUP_PPCS1	24	/* 0x298 */
+#define TEGRA_SWGROUP_SDMMC1A	25	/* 0xa94 *//* Linear shift again */
+#define TEGRA_SWGROUP_SDMMC2A	26	/* 0xa98 */
+#define TEGRA_SWGROUP_SDMMC3A	27	/* 0xa9c */
+#define TEGRA_SWGROUP_SDMMC4A	28	/* 0xaa0 */
+#define TEGRA_SWGROUP_ISP2B	29	/* 0xaa4 */
+#define TEGRA_SWGROUP_GPU	30	/* 0xaa8 */
+#define TEGRA_SWGROUP_GPUB	31	/* 0xaac */
+#define TEGRA_SWGROUP_PPCS2	32	/* 0xab0 */
+
+#define TEGRA_SWGROUP_MAX	64
+
+#define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)
+
+#endif /* _DT_BINDINGS_MEMORY_TEGRA_SWGROUP_H */
-- 
1.8.1.5

Re: [PATCHv4 1/7] ARM: tegra: Create a DT header defining SWGROUP ID

[Stephen Warren]

On 11/11/2013 01:31 AM, Hiroshi Doyu wrote:
> Create a header file to define the swgroup IDs used by the IOMMU(SMMU)
> binding. "swgroup" is a group of H/W clients which a Tegra SoC
> supports. This unique ID can be used to calculate MC_SMMU_<swgroup
> name>_ASID_0 register offset and MC_<swgroup name>_HOTRESET_*_0
> register bit. This will allow the same header to be used by both
> device tree files, and drivers implementing this binding, which
> guarantees that the two stay in sync. This also makes device trees
> more readable by using names instead of magic numbers. For HOTRESET
> bit shifting we need another conversion table, which will come later.

> diff --git a/include/dt-bindings/memory/tegra-swgroup.h b/include/dt-bindings/memory/tegra-swgroup.h

> +#define TEGRA_SWGROUP_PPCS2	32	/* 0xab0 */
> +
> +#define TEGRA_SWGROUP_MAX	64
> +
> +#define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)

If I put the following into a DT and compile it:

#define TEGRA_SWGROUP_PPCS2	32	/* 0xab0 */
#define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)
/ {
	test-prop = <(TEGRA_SWGROUP_BIT(PPCS2))>;
};

I get:

Error: arch/arm/boot/dts/tegra20.dtsi:11.28-29 integer value out of
range 0000000000000020 (32 bits)
FATAL ERROR: Syntax error parsing input tree

Is TEGRA_SWGROUP_BIT() not meant to be used in DT files? If it is, the
definition is broken. If it is not, it should be defined in the driver
not the header, since DT files have no use for it.

Note: I mentioned this issue last time I reviewed this patch:-(

Re: [PATCHv4 1/7] ARM: tegra: Create a DT header defining SWGROUP ID

[Hiroshi Doyu]

On Tue, 12 Nov 2013 23:48:22 +0100
Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:

> On 11/11/2013 01:31 AM, Hiroshi Doyu wrote:
> > Create a header file to define the swgroup IDs used by the IOMMU(SMMU)
> > binding. "swgroup" is a group of H/W clients which a Tegra SoC
> > supports. This unique ID can be used to calculate MC_SMMU_<swgroup
> > name>_ASID_0 register offset and MC_<swgroup name>_HOTRESET_*_0
> > register bit. This will allow the same header to be used by both
> > device tree files, and drivers implementing this binding, which
> > guarantees that the two stay in sync. This also makes device trees
> > more readable by using names instead of magic numbers. For HOTRESET
> > bit shifting we need another conversion table, which will come later.
> 
> > diff --git a/include/dt-bindings/memory/tegra-swgroup.h b/include/dt-bindings/memory/tegra-swgroup.h
> 
> > +#define TEGRA_SWGROUP_PPCS2	32	/* 0xab0 */
> > +
> > +#define TEGRA_SWGROUP_MAX	64
> > +
> > +#define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)
> 
> If I put the following into a DT and compile it:
> 
> #define TEGRA_SWGROUP_PPCS2	32	/* 0xab0 */
> #define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)
> / {
> 	test-prop = <(TEGRA_SWGROUP_BIT(PPCS2))>;
> };
> 
> I get:
> 
> Error: arch/arm/boot/dts/tegra20.dtsi:11.28-29 integer value out of
> range 0000000000000020 (32 bits)
> FATAL ERROR: Syntax error parsing input tree
> 
> Is TEGRA_SWGROUP_BIT() not meant to be used in DT files? If it is, the
> definition is broken. If it is not, it should be defined in the driver
> not the header, since DT files have no use for it.

I'd like to use the macro in DT but what I want is 2 cells from 64 bit.
For the above example, I want the following 2 cell to be generated but I
haven't found any ways yet.

#define TEGRA_SWGROUP_PPCS2	32	/* 0xab0 */
#define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)
/ {
 	test-prop = <0x00000000 0x00000001>;
};

Re: [PATCHv4 1/7] ARM: tegra: Create a DT header defining SWGROUP ID

[Stephen Warren]

On 11/15/2013 03:29 AM, Hiroshi Doyu wrote:
> On Tue, 12 Nov 2013 23:48:22 +0100
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> 
>> On 11/11/2013 01:31 AM, Hiroshi Doyu wrote:
>>> Create a header file to define the swgroup IDs used by the IOMMU(SMMU)
>>> binding. "swgroup" is a group of H/W clients which a Tegra SoC
>>> supports. This unique ID can be used to calculate MC_SMMU_<swgroup
>>> name>_ASID_0 register offset and MC_<swgroup name>_HOTRESET_*_0
>>> register bit. This will allow the same header to be used by both
>>> device tree files, and drivers implementing this binding, which
>>> guarantees that the two stay in sync. This also makes device trees
>>> more readable by using names instead of magic numbers. For HOTRESET
>>> bit shifting we need another conversion table, which will come later.
>>
>>> diff --git a/include/dt-bindings/memory/tegra-swgroup.h b/include/dt-bindings/memory/tegra-swgroup.h
>>
>>> +#define TEGRA_SWGROUP_PPCS2	32	/* 0xab0 */
>>> +
>>> +#define TEGRA_SWGROUP_MAX	64
>>> +
>>> +#define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)
>>
>> If I put the following into a DT and compile it:
>>
>> #define TEGRA_SWGROUP_PPCS2	32	/* 0xab0 */
>> #define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)
>> / {
>> 	test-prop = <(TEGRA_SWGROUP_BIT(PPCS2))>;
>> };
>>
>> I get:
>>
>> Error: arch/arm/boot/dts/tegra20.dtsi:11.28-29 integer value out of
>> range 0000000000000020 (32 bits)
>> FATAL ERROR: Syntax error parsing input tree
>>
>> Is TEGRA_SWGROUP_BIT() not meant to be used in DT files? If it is, the
>> definition is broken. If it is not, it should be defined in the driver
>> not the header, since DT files have no use for it.
> 
> I'd like to use the macro in DT but what I want is 2 cells from 64 bit.
> For the above example, I want the following 2 cell to be generated but I
> haven't found any ways yet.
> 
> #define TEGRA_SWGROUP_PPCS2	32	/* 0xab0 */
> #define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)
> / {
>  	test-prop = <0x00000000 0x00000001>;
> };

I guess you'd need to do something like:

#define MSW_OF_U64(x) ((x) >> 32)
#define LSW_OF_U64(x) ((x) & 0xffffffff)

... and use those to construct the two cells explicitly.

Or, explicitly name TEGRA_SWGROUP_xxx so that it's obvious which go in
the MSW and which in the LSW, and then:

#define TEGRA_SWGROUP_BIT(x)	(1ULL << (TEGRA_SWGROUP_##x % 32))

It might also be possible to do:

#define TWO_U32_OF_U64(x) ((x) >> 32) ((x) & 0xffffffff)

... which expands to both cells at once, although that's verging on
hiding DT structure behind a macro, which isn't exceptionally great, but
might be acceptable in this limited case.

[PATCHv4 2/7] driver/core: Populate IOMMU'able devices in order

[Hiroshi Doyu]

An "IOMMU device" on the bus is poplulated first, "IOMMU'able devices"
are done later.

With CONFIG_OF_IOMMU, "#stream-id-cells" DT binding would be used to
identify whether a device is IOMMU'able or not. If a device is
IOMMU'able, we'll defer to populate that device till an iommu device
is populated. Once an iommu device is populated, "dev->bus->iommu_ops"
is set in the bus. Then, those defered IOMMU'able devices are
populated and configured as IOMMU'abled with help of the already
populated iommu device via iommu_ops->add_device().

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Update:
This is newly added, and the successor of the following RFC:
  [RFC][PATCHv3+ 1/2] driver/core: Add of_iommu_attach()
  http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006914.html
---
 drivers/base/dd.c        |  5 +++++
 drivers/iommu/of_iommu.c | 33 +++++++++++++++++++++++++++++++++
 include/linux/of_iommu.h |  7 +++++++
 3 files changed, 45 insertions(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 35fa368..6e892d4 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -25,6 +25,7 @@
 #include <linux/async.h>
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/devinfo.h>
+#include <linux/of_iommu.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -273,6 +274,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 
 	dev->driver = drv;
 
+	ret = of_iommu_attach(dev);
+	if (ret)
+		goto probe_failed;
+
 	/* If using pinctrl, bind pins now before probing */
 	ret = pinctrl_bind_pins(dev);
 	if (ret)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index ee249bc..335bf6a 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -20,6 +20,8 @@
 #include <linux/export.h>
 #include <linux/limits.h>
 #include <linux/of.h>
+#include <linux/device.h>
+#include <linux/iommu.h>
 
 /**
  * of_get_dma_window - Parse *dma-window property and returns 0 if found.
@@ -88,3 +90,34 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+static bool of_is_iommuable(struct device *dev)
+{
+	size_t bytes;
+	const __be32 *prop;
+	const char *propname = "#stream-id-cells";
+
+	prop = of_get_property(dev->of_node, propname, &bytes);
+	if (!prop || !bytes)
+		return false;
+
+	pr_debug("%s=%d %s\n", propname, bytes, dev_name(dev));
+	return true;
+}
+
+int of_iommu_attach(struct device *dev)
+{
+	struct iommu_ops *ops;
+
+	if (!of_is_iommuable(dev))
+		return 0;
+
+	ops = dev->bus->iommu_ops;
+	if (!ops)
+		return -EPROBE_DEFER;
+
+	if (ops->add_device)
+		return ops->add_device(dev);
+
+	return 0;
+}
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 51a560f..3457489 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -7,6 +7,8 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix,
 			     int index, unsigned long *busno, dma_addr_t *addr,
 			     size_t *size);
 
+extern int of_iommu_attach(struct device *dev);
+
 #else
 
 static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
@@ -16,6 +18,11 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
 	return -EINVAL;
 }
 
+static inline int of_iommu_attach(struct device *dev)
+{
+	return 0;
+}
+
 #endif	/* CONFIG_OF_IOMMU */
 
 #endif /* __OF_IOMMU_H */
-- 
1.8.1.5

Re: [PATCHv4 2/7] driver/core: Populate IOMMU'able devices in order

[Will Deacon]

On Mon, Nov 11, 2013 at 08:31:53AM +0000, Hiroshi Doyu wrote:
> An "IOMMU device" on the bus is poplulated first, "IOMMU'able devices"
> are done later.
> 
> With CONFIG_OF_IOMMU, "#stream-id-cells" DT binding would be used to
> identify whether a device is IOMMU'able or not. If a device is
> IOMMU'able, we'll defer to populate that device till an iommu device
> is populated. Once an iommu device is populated, "dev->bus->iommu_ops"
> is set in the bus. Then, those defered IOMMU'able devices are
> populated and configured as IOMMU'abled with help of the already
> populated iommu device via iommu_ops->add_device().
> 
> Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> Update:
> This is newly added, and the successor of the following RFC:
>   [RFC][PATCHv3+ 1/2] driver/core: Add of_iommu_attach()
>   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006914.html

This generally looks like the right idea to me, but it would be good to have
the input from a DT maintainer on the use of "#stream-id-cells" as an indicator
that a device is behind an IOMMU. (aside: I think "iommuable" is a horrible
word!).

What happens if you do the deferring at the bus level? E.g. defer all device
probes on a bus, until the IOMMU is probed for that bus. That might fit
better with future work, where we will almost certainly need to expose more
of the bus topology to Linux. Of course, I can see that falling down for
monolithic virtual buses like the platform bus.

Will

Re: [PATCHv4 2/7] driver/core: Populate IOMMU'able devices in order

[Stephen Warren]

On 11/11/2013 04:39 AM, Will Deacon wrote:
> On Mon, Nov 11, 2013 at 08:31:53AM +0000, Hiroshi Doyu wrote:
>> An "IOMMU device" on the bus is poplulated first, "IOMMU'able devices"
>> are done later.
>>
>> With CONFIG_OF_IOMMU, "#stream-id-cells" DT binding would be used to
>> identify whether a device is IOMMU'able or not. If a device is
>> IOMMU'able, we'll defer to populate that device till an iommu device
>> is populated. Once an iommu device is populated, "dev->bus->iommu_ops"
>> is set in the bus. Then, those defered IOMMU'able devices are
>> populated and configured as IOMMU'abled with help of the already
>> populated iommu device via iommu_ops->add_device().
>>
>> Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>> Update:
>> This is newly added, and the successor of the following RFC:
>>   [RFC][PATCHv3+ 1/2] driver/core: Add of_iommu_attach()
>>   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006914.html
> 
> This generally looks like the right idea to me, but it would be good to have
> the input from a DT maintainer on the use of "#stream-id-cells" as an indicator
> that a device is behind an IOMMU. (aside: I think "iommuable" is a horrible
> word!).
> 
> What happens if you do the deferring at the bus level? E.g. defer all device
> probes on a bus, until the IOMMU is probed for that bus. That might fit
> better with future work, where we will almost certainly need to expose more
> of the bus topology to Linux. Of course, I can see that falling down for
> monolithic virtual buses like the platform bus.

I don't think it's correct to think about "the IOMMU for the bus".

There could easily be multiple different IOMMU slave-sides attached to a
bus, and hence you'd need to defer probing until "all the IOMMs for the
bus" to be available.

Equally, I assume that dev->bus->iommu_ops rather assumes that bus
masters always master transactions onto the same bus that their control
registers are slaves upon. That also doesn't seem like a reasonable
assumption.

As such, I think an approach where each device waits for any IOMMUs that
affect it (wherever/whatever and however many they may be) is better
than one where we try to explicitly manage the probe order of devices
based on their slave registers' location.

Re: [PATCHv4 2/7] driver/core: Populate IOMMU'able devices in order

[Stephen Warren]

On 11/11/2013 01:31 AM, Hiroshi Doyu wrote:
> An "IOMMU device" on the bus is poplulated first, "IOMMU'able devices"
> are done later.
> 
> With CONFIG_OF_IOMMU, "#stream-id-cells" DT binding would be used to
> identify whether a device is IOMMU'able or not. If a device is
> IOMMU'able, we'll defer to populate that device till an iommu device
> is populated. Once an iommu device is populated, "dev->bus->iommu_ops"
> is set in the bus. Then, those defered IOMMU'able devices are
> populated and configured as IOMMU'abled with help of the already
> populated iommu device via iommu_ops->add_device().

This looks fairly neat and clean.

I'm still worried about using #stream-id-cells in DT nodes though. While
I do understand that the *Linux* device model currently only allows each
struct device to be affected by a single IOMMU, I worry that encoding
that same restriction into DT is a mistake. I'd far rather see a
property like:

SMMU:
    smmu: smmu@xxxxxx {
        #smmu-cells = <1>;
    }

Affected device:
    smmus = <&smmu 1>;
    (perhaps with smmu-names too)

That would allow the DT to represent basically arbitrary HW configurations.

The implementation of this patch would then be almost as trivial; you'd
just need to walk the smmus property to find each phandle in turn, just
like any other phandle+specifier property, and validate that the SMMU
driver was already probe()d for each.

Re: [PATCHv4 2/7] driver/core: Populate IOMMU'able devices in order

[Hiroshi Doyu]

On Wed, 13 Nov 2013 00:34:20 +0100
Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:

> On 11/11/2013 01:31 AM, Hiroshi Doyu wrote:
> > An "IOMMU device" on the bus is poplulated first, "IOMMU'able devices"
> > are done later.
> > 
> > With CONFIG_OF_IOMMU, "#stream-id-cells" DT binding would be used to
> > identify whether a device is IOMMU'able or not. If a device is
> > IOMMU'able, we'll defer to populate that device till an iommu device
> > is populated. Once an iommu device is populated, "dev->bus->iommu_ops"
> > is set in the bus. Then, those defered IOMMU'able devices are
> > populated and configured as IOMMU'abled with help of the already
> > populated iommu device via iommu_ops->add_device().
> 
> This looks fairly neat and clean.
> 
> I'm still worried about using #stream-id-cells in DT nodes though. While
> I do understand that the *Linux* device model currently only allows each
> struct device to be affected by a single IOMMU, I worry that encoding
> that same restriction into DT is a mistake. I'd far rather see a
> property like:
> 
> SMMU:
>     smmu: smmu@xxxxxx {
>         #smmu-cells = <1>;
>     }
> 
> Affected device:
>     smmus = <&smmu 1>;
>     (perhaps with smmu-names too)
> 
> That would allow the DT to represent basically arbitrary HW configurations.

True, and also can solve multi IOMMU problem as well.

> The implementation of this patch would then be almost as trivial; you'd
> just need to walk the smmus property to find each phandle in turn, just
> like any other phandle+specifier property, and validate that the SMMU
> driver was already probe()d for each.

This seems to be almost same as the previous v3 DT bindings, and if we
introduce 64 bitmap newly, this DT bindings would be something like
below:

   smmu: iommu@xxxxxx {
       #smmu-cells = <2>;
       ......
   };

   host1x {
           compatible = "nvidia,tegra30-host1x", "simple-bus";
           nvidia,memory-clients = <&smmu 0x??????? 0x???????>;
           ....
           gr3d {
                   compatible = "nvidia,tegra30-gr3d";
                   nvidia,memory-clients = <&smmu 0x??????? 0x???????>;
           }

If a device is attached to multiple IOMMU H/Ws,

           gr3d {
                   compatible = "nvidia,tegra30-gr3d";
                   nvidia,memory-clients = <&smmu 0x??????? 0x??????? &gart 0x???????>;
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Then, the problem is the binding "name" and its "scope". This original
patch works with "#stream-id-cells" in driver core because I assumed
that "#stream-id-cells" can indicate globally that a device can be an
IOMMU master.

We may be able to have some kind of callback function which checks
"#stream-id-cells" *in* SMMU driver, but at least this function needs to
be registered in the bus at very early stage, iommu_ops->is_iommu_master().
But this cannot be done with bus->iommu_ops since bus->iommu_ops is set
after IOMMU is populated. A kind of Chikin or the egg problem.

Having a global bindings which indicates a device's IOMMU
master'ability is quite convenient. For example, "iommu" and
"#iommu-cells" without refering any local data. Then the above
DT would be:

   smmu: iommu@xxxxxx {
       #iommu-cells = <2>;
       ^^^^^^^^^^^^^^^^^^
   };

   host1x {
           compatible = "nvidia,tegra30-host1x", "simple-bus";
           iommu = <&smmu 0x??????? 0x???????>;
	   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
           gr3d {
                   compatible = "nvidia,tegra30-gr3d";
                   iommu = <&smmu 0x??????? 0x???????>;
           }

What do you think to have a global IOMMU DT bindings?

Re: [PATCHv4 2/7] driver/core: Populate IOMMU'able devices in order

[Stephen Warren]

On 11/13/2013 12:23 AM, Hiroshi Doyu wrote:
> On Wed, 13 Nov 2013 00:34:20 +0100
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> 
>> On 11/11/2013 01:31 AM, Hiroshi Doyu wrote:
>>> An "IOMMU device" on the bus is poplulated first, "IOMMU'able devices"
>>> are done later.
>>>
>>> With CONFIG_OF_IOMMU, "#stream-id-cells" DT binding would be used to
>>> identify whether a device is IOMMU'able or not. If a device is
>>> IOMMU'able, we'll defer to populate that device till an iommu device
>>> is populated. Once an iommu device is populated, "dev->bus->iommu_ops"
>>> is set in the bus. Then, those defered IOMMU'able devices are
>>> populated and configured as IOMMU'abled with help of the already
>>> populated iommu device via iommu_ops->add_device().
>>
>> This looks fairly neat and clean.
>>
>> I'm still worried about using #stream-id-cells in DT nodes though. While
>> I do understand that the *Linux* device model currently only allows each
>> struct device to be affected by a single IOMMU, I worry that encoding
>> that same restriction into DT is a mistake. I'd far rather see a
>> property like:
>>
>> SMMU:
>>     smmu: smmu@xxxxxx {
>>         #smmu-cells = <1>;
>>     }
>>
>> Affected device:
>>     smmus = <&smmu 1>;
>>     (perhaps with smmu-names too)
>>
>> That would allow the DT to represent basically arbitrary HW configurations.
> 
> True, and also can solve multi IOMMU problem as well.
> 
>> The implementation of this patch would then be almost as trivial; you'd
>> just need to walk the smmus property to find each phandle in turn, just
>> like any other phandle+specifier property, and validate that the SMMU
>> driver was already probe()d for each.
> 
> This seems to be almost same as the previous v3 DT bindings, and if we
> introduce 64 bitmap newly, this DT bindings would be something like
> below:
> 
>    smmu: iommu@xxxxxx {
>        #smmu-cells = <2>;
>        ......
>    };
> 
>    host1x {
>            compatible = "nvidia,tegra30-host1x", "simple-bus";
>            nvidia,memory-clients = <&smmu 0x??????? 0x???????>;
>            ....
>            gr3d {
>                    compatible = "nvidia,tegra30-gr3d";
>                    nvidia,memory-clients = <&smmu 0x??????? 0x???????>;
>            }
> 
> If a device is attached to multiple IOMMU H/Ws,
> 
>            gr3d {
>                    compatible = "nvidia,tegra30-gr3d";
>                    nvidia,memory-clients = <&smmu 0x??????? 0x??????? &gart 0x???????>;
>                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Then, the problem is the binding "name" and its "scope". This original
> patch works with "#stream-id-cells" in driver core because I assumed
> that "#stream-id-cells" can indicate globally that a device can be an
> IOMMU master.

For example, the pinctrl bindings have the same issue, since they're
interpreted "globally", and by (code called from) the generic device
probing code.

We simply decided that the properties "pinctrl-names" and "pinctrl-n"
(n=0...) were globally defined by the pinctrl subsystem, and hence could
be parsed in any node.

We could do the same with "smmus" and "smmu-names" in this case.

> We may be able to have some kind of callback function which checks
> "#stream-id-cells" *in* SMMU driver, but at least this function needs to
> be registered in the bus at very early stage, iommu_ops->is_iommu_master().
> But this cannot be done with bus->iommu_ops since bus->iommu_ops is set
> after IOMMU is populated. A kind of Chikin or the egg problem.

I think this is simply the normal deferred probe.

When device X attempts to probe, the core SMMU code (called from the
core device probing code) iterates over the smmus property. If any of
the phandles listed there don't have a registered SMMU driver, then
defer probe of device X. Eventually, the SMMU driver will appear, and
the driver core will attempt to re-probe device X, and all the SMMUs
have devices probed already, and everything will work.

Re: [PATCHv4 2/7] driver/core: Populate IOMMU'able devices in order

[Will Deacon]

On Tue, Nov 12, 2013 at 11:34:20PM +0000, Stephen Warren wrote:
> On 11/11/2013 01:31 AM, Hiroshi Doyu wrote:
> > An "IOMMU device" on the bus is poplulated first, "IOMMU'able devices"
> > are done later.
> > 
> > With CONFIG_OF_IOMMU, "#stream-id-cells" DT binding would be used to
> > identify whether a device is IOMMU'able or not. If a device is
> > IOMMU'able, we'll defer to populate that device till an iommu device
> > is populated. Once an iommu device is populated, "dev->bus->iommu_ops"
> > is set in the bus. Then, those defered IOMMU'able devices are
> > populated and configured as IOMMU'abled with help of the already
> > populated iommu device via iommu_ops->add_device().
> 
> This looks fairly neat and clean.
> 
> I'm still worried about using #stream-id-cells in DT nodes though. While
> I do understand that the *Linux* device model currently only allows each
> struct device to be affected by a single IOMMU, I worry that encoding
> that same restriction into DT is a mistake. I'd far rather see a
> property like:
> 
> SMMU:
>     smmu: smmu@xxxxxx {
>         #smmu-cells = <1>;
>     }
> 
> Affected device:
>     smmus = <&smmu 1>;
>     (perhaps with smmu-names too)
> 
> That would allow the DT to represent basically arbitrary HW configurations.
> 
> The implementation of this patch would then be almost as trivial; you'd
> just need to walk the smmus property to find each phandle in turn, just
> like any other phandle+specifier property, and validate that the SMMU
> driver was already probe()d for each.

There are a few problems with that:

  1.) It assumes all devices sharing an SMMU have the same number of
      "smmu cells"

  2.) It moves SMMU-specific data out to the device, which makes it
      impossible to describe more complicated topologies where IDs can be
      remapped/remastered, potentially by multiple SMMUs and/or bus bridges.

When writing the binding for the ARM SMMU driver, I originally started with
something similar to what you're suggesting, but was later forced down a
different route when I realised what sort of systems were being built.

We will have similar problems for PCIe RIDs and GIC DIDs (I spoke about this
at the ARM mini-summit). They are not fixed across the system: they
originate from a device, but can change as they traverse the system
topology.

Will

Re: [PATCHv4 2/7] driver/core: Populate IOMMU'able devices in order

[Hiroshi Doyu]

Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote @ Wed, 13 Nov 2013 15:38:04 +0100:

> On Tue, Nov 12, 2013 at 11:34:20PM +0000, Stephen Warren wrote:
> > On 11/11/2013 01:31 AM, Hiroshi Doyu wrote:
> > > An "IOMMU device" on the bus is poplulated first, "IOMMU'able devices"
> > > are done later.
> > > 
> > > With CONFIG_OF_IOMMU, "#stream-id-cells" DT binding would be used to
> > > identify whether a device is IOMMU'able or not. If a device is
> > > IOMMU'able, we'll defer to populate that device till an iommu device
> > > is populated. Once an iommu device is populated, "dev->bus->iommu_ops"
> > > is set in the bus. Then, those defered IOMMU'able devices are
> > > populated and configured as IOMMU'abled with help of the already
> > > populated iommu device via iommu_ops->add_device().
> > 
> > This looks fairly neat and clean.
> > 
> > I'm still worried about using #stream-id-cells in DT nodes though. While
> > I do understand that the *Linux* device model currently only allows each
> > struct device to be affected by a single IOMMU, I worry that encoding
> > that same restriction into DT is a mistake. I'd far rather see a
> > property like:
> > 
> > SMMU:
> >     smmu: smmu@xxxxxx {
> >         #smmu-cells = <1>;
> >     }
> > 
> > Affected device:
> >     smmus = <&smmu 1>;
> >     (perhaps with smmu-names too)
> > 
> > That would allow the DT to represent basically arbitrary HW configurations.
> > 
> > The implementation of this patch would then be almost as trivial; you'd
> > just need to walk the smmus property to find each phandle in turn, just
> > like any other phandle+specifier property, and validate that the SMMU
> > driver was already probe()d for each.
> 
> There are a few problems with that:
> 
>   1.) It assumes all devices sharing an SMMU have the same number of
>       "smmu cells"

This can be solved with introducing the fixed size of bitmap. The size
of bitmap can be fixed even per SoC. In tegra we used 64(2 cells)
which I expect at most.

>   2.) It moves SMMU-specific data out to the device, which makes it
>       impossible to describe more complicated topologies where IDs can be
>       remapped/remastered, potentially by multiple SMMUs and/or bus bridges.
> 
> When writing the binding for the ARM SMMU driver, I originally started with
> something similar to what you're suggesting, but was later forced down a
> different route when I realised what sort of systems were being built.
> 
> We will have similar problems for PCIe RIDs and GIC DIDs (I spoke about this
> at the ARM mini-summit). They are not fixed across the system: they
> originate from a device, but can change as they traverse the system
> topology.

Is there any chance to overwrite SMMU driver specific params during
setting up topologies?

Re: [PATCHv4 2/7] driver/core: Populate IOMMU'able devices in order

[Will Deacon]

On Wed, Nov 13, 2013 at 04:06:10PM +0000, Hiroshi Doyu wrote:
> Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote @ Wed, 13 Nov 2013 15:38:04 +0100:
> 
> > On Tue, Nov 12, 2013 at 11:34:20PM +0000, Stephen Warren wrote:
> > > SMMU:
> > >     smmu: smmu@xxxxxx {
> > >         #smmu-cells = <1>;
> > >     }
> > > 
> > > Affected device:
> > >     smmus = <&smmu 1>;
> > >     (perhaps with smmu-names too)
> > > 
> > > That would allow the DT to represent basically arbitrary HW configurations.
> > > 
> > > The implementation of this patch would then be almost as trivial; you'd
> > > just need to walk the smmus property to find each phandle in turn, just
> > > like any other phandle+specifier property, and validate that the SMMU
> > > driver was already probe()d for each.
> > 
> > There are a few problems with that:
> > 
> >   1.) It assumes all devices sharing an SMMU have the same number of
> >       "smmu cells"
> 
> This can be solved with introducing the fixed size of bitmap. The size
> of bitmap can be fixed even per SoC. In tegra we used 64(2 cells)
> which I expect at most.

That really doesn't sound like a good idea where you have bridges (like a
PCIe host controller) which could have a significant chunk of StreamID
space. You'd also need to pad everything out with some dummy IDs for parsing
purposes. Yuck!

> >   2.) It moves SMMU-specific data out to the device, which makes it
> >       impossible to describe more complicated topologies where IDs can be
> >       remapped/remastered, potentially by multiple SMMUs and/or bus bridges.
> > 
> > When writing the binding for the ARM SMMU driver, I originally started with
> > something similar to what you're suggesting, but was later forced down a
> > different route when I realised what sort of systems were being built.
> > 
> > We will have similar problems for PCIe RIDs and GIC DIDs (I spoke about this
> > at the ARM mini-summit). They are not fixed across the system: they
> > originate from a device, but can change as they traverse the system
> > topology.
> 
> Is there any chance to overwrite SMMU driver specific params during
> setting up topologies?

Not sure I understand what you're getting at here. Could you elaborate
please?

Will

Re: [PATCHv4 2/7] driver/core: Populate IOMMU'able devices in order

[Stephen Warren]

On 11/13/2013 10:31 AM, Will Deacon wrote:
> On Wed, Nov 13, 2013 at 04:06:10PM +0000, Hiroshi Doyu wrote:
>> Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote @ Wed, 13 Nov 2013 15:38:04 +0100:
>>
>>> On Tue, Nov 12, 2013 at 11:34:20PM +0000, Stephen Warren wrote:
>>>> SMMU:
>>>>     smmu: smmu@xxxxxx {
>>>>         #smmu-cells = <1>;
>>>>     }
>>>>
>>>> Affected device:
>>>>     smmus = <&smmu 1>;
>>>>     (perhaps with smmu-names too)
>>>>
>>>> That would allow the DT to represent basically arbitrary HW configurations.
>>>>
>>>> The implementation of this patch would then be almost as trivial; you'd
>>>> just need to walk the smmus property to find each phandle in turn, just
>>>> like any other phandle+specifier property, and validate that the SMMU
>>>> driver was already probe()d for each.
>>>
>>> There are a few problems with that:
>>>
>>>   1.) It assumes all devices sharing an SMMU have the same number of
>>>       "smmu cells"
>>
>> This can be solved with introducing the fixed size of bitmap. The size
>> of bitmap can be fixed even per SoC. In tegra we used 64(2 cells)
>> which I expect at most.
> 
> That really doesn't sound like a good idea where you have bridges (like a
> PCIe host controller) which could have a significant chunk of StreamID
> space. You'd also need to pad everything out with some dummy IDs for parsing
> purposes. Yuck!

Can't you solve this by having the SMMU include a stream ID mapping
table. In the case of stream IDs having a large "address" space, then
just define the mapping table syntax to allow easy specification of
ranges using start/length, start/end, or value/mask pairs. Similar to
ranges or PCI's interrupt-map{,-mask}.

Re: [PATCHv4 2/7] driver/core: Populate IOMMU'able devices in order

[Will Deacon]

On Wed, Nov 13, 2013 at 05:53:36PM +0000, Stephen Warren wrote:
> On 11/13/2013 10:31 AM, Will Deacon wrote:
> > On Wed, Nov 13, 2013 at 04:06:10PM +0000, Hiroshi Doyu wrote:
> >> This can be solved with introducing the fixed size of bitmap. The size
> >> of bitmap can be fixed even per SoC. In tegra we used 64(2 cells)
> >> which I expect at most.
> > 
> > That really doesn't sound like a good idea where you have bridges (like a
> > PCIe host controller) which could have a significant chunk of StreamID
> > space. You'd also need to pad everything out with some dummy IDs for parsing
> > purposes. Yuck!
> 
> Can't you solve this by having the SMMU include a stream ID mapping
> table. In the case of stream IDs having a large "address" space, then
> just define the mapping table syntax to allow easy specification of
> ranges using start/length, start/end, or value/mask pairs. Similar to
> ranges or PCI's interrupt-map{,-mask}.

That may work for windows, but the mapping from input stream IDs to output
IDs can be arbitrary (I *really* hope people stick to windows for requester
IDs though).

Will

Re: [PATCHv4 2/7] driver/core: Populate IOMMU'able devices in order

[Stephen Warren]

On 11/13/2013 07:38 AM, Will Deacon wrote:
> On Tue, Nov 12, 2013 at 11:34:20PM +0000, Stephen Warren wrote:
>> On 11/11/2013 01:31 AM, Hiroshi Doyu wrote:
>>> An "IOMMU device" on the bus is poplulated first, "IOMMU'able devices"
>>> are done later.
>>>
>>> With CONFIG_OF_IOMMU, "#stream-id-cells" DT binding would be used to
>>> identify whether a device is IOMMU'able or not. If a device is
>>> IOMMU'able, we'll defer to populate that device till an iommu device
>>> is populated. Once an iommu device is populated, "dev->bus->iommu_ops"
>>> is set in the bus. Then, those defered IOMMU'able devices are
>>> populated and configured as IOMMU'abled with help of the already
>>> populated iommu device via iommu_ops->add_device().
>>
>> This looks fairly neat and clean.
>>
>> I'm still worried about using #stream-id-cells in DT nodes though. While
>> I do understand that the *Linux* device model currently only allows each
>> struct device to be affected by a single IOMMU, I worry that encoding
>> that same restriction into DT is a mistake. I'd far rather see a
>> property like:
>>
>> SMMU:
>>     smmu: smmu@xxxxxx {
>>         #smmu-cells = <1>;
>>     }
>>
>> Affected device:
>>     smmus = <&smmu 1>;
>>     (perhaps with smmu-names too)
>>
>> That would allow the DT to represent basically arbitrary HW configurations.
>>
>> The implementation of this patch would then be almost as trivial; you'd
>> just need to walk the smmus property to find each phandle in turn, just
>> like any other phandle+specifier property, and validate that the SMMU
>> driver was already probe()d for each.
> 
> There are a few problems with that:
> 
>   1.) It assumes all devices sharing an SMMU have the same number of
>       "smmu cells"

The SMMU HW defines how many cells are required to represent the client
stream IDs. So, this isn't a problem.

Are you thinking of cases where an SMMU client issues transactions with
multiple different stream IDs? That is supported by having multiple
entries in the smmus property.

(Assuming that #smmu-cells in the SMMU is <1>)

HW that issues with 1 stream ID:

	smmus = <&smmu 123>;

HW that issues with 2 stream IDs:

	smmus = <&smmu 123 &smmu 345>;

>   2.) It moves SMMU-specific data out to the device, which makes it

Yes, but I think it's really SMMU-client-specific data not
SMMU-specific. The SMMU doesn't dictate the stream IDs that "clients"
include with their transactions; the "client" HW dictates that.

>       impossible to describe more complicated topologies where IDs can be
>       remapped/remastered, potentially by multiple SMMUs and/or bus bridges.

I don't think so.

The SMMU "clients" can indicate what stream IDs they use to issue
transactions.

The SMMU DT node or driver itself can still include a table that
describes how it re-writes those stream IDs when forwarding transactions
to the next step. I don't believe there's any requirement that the SMMU
list all its clients and this mapping table in the same property. So,
the SMMU could easily have:

(entries are this SMMU's #stream-id-cells, assumed to be 1 here, then
the next SMMU's #stream-id-cells, assumed to be 2 here)

	smmu-stream-id-translations =
		<123 1 987>,
		<345 0 765>;

> When writing the binding for the ARM SMMU driver, I originally started with
> something similar to what you're suggesting, but was later forced down a
> different route when I realised what sort of systems were being built.
> 
> We will have similar problems for PCIe RIDs and GIC DIDs (I spoke about this
> at the ARM mini-summit). They are not fixed across the system: they
> originate from a device, but can change as they traverse the system
> topology.

[PATCHv4 3/7] iommu/tegra: smmu: Register IOMMU'able devices dynamically

[Hiroshi Doyu]

platform_devices are registered as IOMMU'able dynamically via
add_device() and remove_device().

Tegra SMMU can have multiple address spaces(AS). IOMMU'able devices
can belong to one of them. Multiple IOVA maps are created at boot-up,
which can be attached to devices later. We reserve 2 of them for
static assignment, AS[0] for system default, AS[1] for AHB clusters as
protected domain from others, where there are many traditional
pheripheral devices like USB, SD/MMC. They should be isolated from
some smart devices like host1x for system robustness. Even if smart
devices behaves wrongly, the traditional devices(SD/MMC, USB) wouldn't
be affected, and the system could continue most likely. DMA API(ARM)
needs ARM_DMA_USE_IOMMU to be enabled.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Update:
Combined the following from v3. This makes more sense what they do.
  [PATCHv3 06/19] iommu/tegra: smmu: Select ARM_DMA_USE_IOMMU in Kconfig
  [PATCHv3 07/19] iommu/tegra: smmu: Create default IOVA maps
  [PATCHv3 08/19] iommu/tegra: smmu: Register platform_device to IOMMU dynamically
  [PATCHv3 19/19] iommu/tegra: smmu: Support Multiple ASID
---
 drivers/iommu/Kconfig      |  1 +
 drivers/iommu/tegra-smmu.c | 68 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index c880eba..d1bc65d 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -170,6 +170,7 @@ config TEGRA_IOMMU_SMMU
 	bool "Tegra SMMU IOMMU Support"
 	depends on ARCH_TEGRA && TEGRA_AHB
 	select IOMMU_API
+	select ARM_DMA_USE_IOMMU
 	help
 	  Enables support for remapping discontiguous physical memory
 	  shared with the operating system into contiguous I/O virtual
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 37dd862..6968c11 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -39,6 +39,7 @@
 
 #include <asm/page.h>
 #include <asm/cacheflush.h>
+#include <asm/dma-iommu.h>
 
 enum smmu_hwgrp {
 	HWGRP_AFI,
@@ -319,6 +320,8 @@ struct smmu_device {
 
 	struct device_node *ahb;
 
+	struct dma_iommu_mapping **map;
+
 	int		num_as;
 	struct smmu_as	as[0];		/* Run-time allocated array */
 };
@@ -947,6 +950,47 @@ static void smmu_iommu_domain_destroy(struct iommu_domain *domain)
 	dev_dbg(smmu->dev, "smmu_as@%p\n", as);
 }
 
+/*
+ * ASID[0] for the system default
+ * ASID[1] for PPCS("AHB bus children"), which has SDMMC
+ * ASID[2][3].. open for drivers, first come, first served.
+ */
+enum {
+	SYSTEM_DEFAULT,
+	SYSTEM_PROTECTED,
+};
+
+static int smmu_iommu_add_device(struct device *dev)
+{
+	int err = -EPROBE_DEFER;
+	u64 swgroups;
+	struct dma_iommu_mapping *map = NULL;
+
+	swgroups = smmu_of_get_memory_client(dev);
+	switch (swgroups) {
+	case TEGRA_SWGROUP_BIT(PPCS):
+		map = smmu_handle->map[SYSTEM_PROTECTED];
+		break;
+	default:
+		map = smmu_handle->map[SYSTEM_DEFAULT];
+		break;
+	}
+
+	if (map)
+		err = arm_iommu_attach_device(dev, map);
+
+	pr_debug("swgroups=%016llx map=%p err=%d %s\n",
+		 swgroups, map, err, dev_name(dev));
+
+	return err;
+}
+
+static void smmu_iommu_remove_device(struct device *dev)
+{
+	dev_dbg(dev, "Detaching from map %p\n", to_dma_iommu_mapping(dev));
+	arm_iommu_detach_device(dev);
+}
+
 static struct iommu_ops smmu_iommu_ops = {
 	.domain_init	= smmu_iommu_domain_init,
 	.domain_destroy	= smmu_iommu_domain_destroy,
@@ -956,6 +1000,8 @@ static struct iommu_ops smmu_iommu_ops = {
 	.unmap		= smmu_iommu_unmap,
 	.iova_to_phys	= smmu_iommu_iova_to_phys,
 	.domain_has_cap	= smmu_iommu_domain_has_cap,
+	.add_device	= smmu_iommu_add_device,
+	.remove_device	= smmu_iommu_remove_device,
 	.pgsize_bitmap	= SMMU_IOMMU_PGSIZES,
 };
 
@@ -1144,6 +1190,23 @@ static int tegra_smmu_resume(struct device *dev)
 	return err;
 }
 
+static void tegra_smmu_create_default_map(struct smmu_device *smmu)
+{
+	int i;
+
+	for (i = 0; i < smmu->num_as; i++) {
+		dma_addr_t base = smmu->iovmm_base;
+		size_t size = smmu->page_count << PAGE_SHIFT;
+
+		smmu->map[i] = arm_iommu_create_mapping(&platform_bus_type,
+							base, size, 0);
+		if (IS_ERR(smmu->map[i]))
+			dev_err(smmu->dev,
+				"Couldn't create: asid=%d map=%p %pa-%pa\n",
+				i, smmu->map[i], &base, &base + size - 1);
+	}
+}
+
 static int tegra_smmu_probe(struct platform_device *pdev)
 {
 	struct smmu_device *smmu;
@@ -1160,13 +1223,15 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	if (of_property_read_u32(dev->of_node, "nvidia,#asids", &asids))
 		return -ENODEV;
 
-	bytes = sizeof(*smmu) + asids * sizeof(*smmu->as);
+	bytes = sizeof(*smmu) + asids * (sizeof(*smmu->as) +
+					 sizeof(struct dma_iommu_mapping *));
 	smmu = devm_kzalloc(dev, bytes, GFP_KERNEL);
 	if (!smmu) {
 		dev_err(dev, "failed to allocate smmu_device\n");
 		return -ENOMEM;
 	}
 
+	smmu->map = (struct dma_iommu_mapping **)(smmu->as + asids);
 	smmu->nregs = pdev->num_resources;
 	smmu->regs = devm_kzalloc(dev, 2 * smmu->nregs * sizeof(*smmu->regs),
 				  GFP_KERNEL);
@@ -1236,6 +1301,7 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	smmu_debugfs_create(smmu);
 	smmu_handle = smmu;
 	bus_set_iommu(&platform_bus_type, &smmu_iommu_ops);
+	tegra_smmu_create_default_map(smmu);
 	return 0;
 }
 
-- 
1.8.1.5

Re: [PATCHv4 3/7] iommu/tegra: smmu: Register IOMMU'able devices dynamically

[Stephen Warren]

On 11/11/2013 01:31 AM, Hiroshi Doyu wrote:
> platform_devices are registered as IOMMU'able dynamically via
> add_device() and remove_device().
> 
> Tegra SMMU can have multiple address spaces(AS). IOMMU'able devices
> can belong to one of them. Multiple IOVA maps are created at boot-up,
> which can be attached to devices later. We reserve 2 of them for
> static assignment, AS[0] for system default, AS[1] for AHB clusters as
> protected domain from others, where there are many traditional
> pheripheral devices like USB, SD/MMC. They should be isolated from
> some smart devices like host1x for system robustness. Even if smart
> devices behaves wrongly, the traditional devices(SD/MMC, USB) wouldn't
> be affected, and the system could continue most likely. DMA API(ARM)
> needs ARM_DMA_USE_IOMMU to be enabled.

> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c

> +enum {
> +	SYSTEM_DEFAULT,
> +	SYSTEM_PROTECTED,
> +};
> +
...
> +static int smmu_iommu_add_device(struct device *dev)
...
> +	switch (swgroups) {
> +	case TEGRA_SWGROUP_BIT(PPCS):
> +		map = smmu_handle->map[SYSTEM_PROTECTED];
> +		break;
> +	default:
> +		map = smmu_handle->map[SYSTEM_DEFAULT];
> +		break;
> +	}

As I think I mentioned before, this hard-codes some valid ASIDs.
However, the number of valid ASIDs is configured dynamically from DT, in
tegra_smmu_probe(), via:

static int tegra_smmu_probe(struct platform_device *pdev)
...
        int i, asids, err = 0;
...
        if (of_property_read_u32(dev->of_node, "nvidia,#asids", &asids))
                return -ENODEV;
...
        smmu->num_as = asids;

Shouldn't tegra_smmu_probe() validate that asids >= SYSTEM_PROTECTED, or
similar?

Re: [PATCHv4 3/7] iommu/tegra: smmu: Register IOMMU'able devices dynamically

[Stephen Warren]

On 11/11/2013 01:31 AM, Hiroshi Doyu wrote:
> platform_devices are registered as IOMMU'able dynamically via
> add_device() and remove_device().
> 
> Tegra SMMU can have multiple address spaces(AS). IOMMU'able devices
> can belong to one of them. Multiple IOVA maps are created at boot-up,
> which can be attached to devices later. We reserve 2 of them for
> static assignment, AS[0] for system default, AS[1] for AHB clusters as
> protected domain from others, where there are many traditional
> pheripheral devices like USB, SD/MMC. They should be isolated from
> some smart devices like host1x for system robustness. Even if smart
> devices behaves wrongly, the traditional devices(SD/MMC, USB) wouldn't
> be affected, and the system could continue most likely. DMA API(ARM)
> needs ARM_DMA_USE_IOMMU to be enabled.

> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c

> +static int smmu_iommu_add_device(struct device *dev)
> +{
> +	int err = -EPROBE_DEFER;
> +	u64 swgroups;
> +	struct dma_iommu_mapping *map = NULL;
> +
> +	swgroups = smmu_of_get_memory_client(dev);

BTW, that function doesn't seem to exist in this patch series nor
next-20131107. This call is removed in patch 5/7, but this does make the
series non-bisectable.

[PATCHv4 4/7] iommu/tegra: smmu: Calculate ASID register offset by ID

[Hiroshi Doyu]

ASID register offset is caclulated by SWGROUP ID so that we can get
rid of old SoC specific MACROs. This ID conversion is needed for the
unified SMMU driver over Tegra SoCs. We use dt-bindings MACRO instead
of SoC dependent MACROs. The formula is:

  MC_SMMU_<swgroup name>_ASID_0 = MC_SMMU_AFI_ASID_0 + ID * 4;

Now SWGROUP ID is the global HardWare Accelerator(HWA) identifier
among all Tegra SoC except Tegra2.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Update:
Combined the following patches from v3:
[PATCHv3 09/19] iommu/tegra: smmu: Calculate ASID register offset by ID
[PATCHv3 16/19] iommu/tegra: smmu: Use dt-bindings MACRO
---
 drivers/iommu/tegra-smmu.c | 62 +++-------------------------------------------
 1 file changed, 3 insertions(+), 59 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6968c11..67252e1 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -41,45 +41,7 @@
 #include <asm/cacheflush.h>
 #include <asm/dma-iommu.h>
 
-enum smmu_hwgrp {
-	HWGRP_AFI,
-	HWGRP_AVPC,
-	HWGRP_DC,
-	HWGRP_DCB,
-	HWGRP_EPP,
-	HWGRP_G2,
-	HWGRP_HC,
-	HWGRP_HDA,
-	HWGRP_ISP,
-	HWGRP_MPE,
-	HWGRP_NV,
-	HWGRP_NV2,
-	HWGRP_PPCS,
-	HWGRP_SATA,
-	HWGRP_VDE,
-	HWGRP_VI,
-
-	HWGRP_COUNT,
-
-	HWGRP_END = ~0,
-};
-
-#define HWG_AFI		(1 << HWGRP_AFI)
-#define HWG_AVPC	(1 << HWGRP_AVPC)
-#define HWG_DC		(1 << HWGRP_DC)
-#define HWG_DCB		(1 << HWGRP_DCB)
-#define HWG_EPP		(1 << HWGRP_EPP)
-#define HWG_G2		(1 << HWGRP_G2)
-#define HWG_HC		(1 << HWGRP_HC)
-#define HWG_HDA		(1 << HWGRP_HDA)
-#define HWG_ISP		(1 << HWGRP_ISP)
-#define HWG_MPE		(1 << HWGRP_MPE)
-#define HWG_NV		(1 << HWGRP_NV)
-#define HWG_NV2		(1 << HWGRP_NV2)
-#define HWG_PPCS	(1 << HWGRP_PPCS)
-#define HWG_SATA	(1 << HWGRP_SATA)
-#define HWG_VDE		(1 << HWGRP_VDE)
-#define HWG_VI		(1 << HWGRP_VI)
+#include <dt-bindings/memory/tegra-swgroup.h>
 
 /* bitmap of the page sizes currently supported */
 #define SMMU_IOMMU_PGSIZES	(SZ_4K)
@@ -238,25 +200,7 @@ enum {
 
 #define HWGRP_INIT(client) [HWGRP_##client] = SMMU_##client##_ASID
 
-static const u32 smmu_hwgrp_asid_reg[] = {
-	HWGRP_INIT(AFI),
-	HWGRP_INIT(AVPC),
-	HWGRP_INIT(DC),
-	HWGRP_INIT(DCB),
-	HWGRP_INIT(EPP),
-	HWGRP_INIT(G2),
-	HWGRP_INIT(HC),
-	HWGRP_INIT(HDA),
-	HWGRP_INIT(ISP),
-	HWGRP_INIT(MPE),
-	HWGRP_INIT(NV),
-	HWGRP_INIT(NV2),
-	HWGRP_INIT(PPCS),
-	HWGRP_INIT(SATA),
-	HWGRP_INIT(VDE),
-	HWGRP_INIT(VI),
-};
-#define HWGRP_ASID_REG(x) (smmu_hwgrp_asid_reg[x])
+#define HWGRP_ASID_REG(x) ((x) * sizeof(u32) + SMMU_AFI_ASID)
 
 /*
  * Per client for address space
@@ -826,7 +770,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 	 * Reserve "page zero" for AVP vectors using a common dummy
 	 * page.
 	 */
-	if (map & HWG_AVPC) {
+	if (map & TEGRA_SWGROUP_BIT(AVPC)) {
 		struct page *page;
 
 		page = as->smmu->avp_vector_page;
-- 
1.8.1.5

Re: [PATCHv4 4/7] iommu/tegra: smmu: Calculate ASID register offset by ID

[Stephen Warren]

On 11/11/2013 01:31 AM, Hiroshi Doyu wrote:
> ASID register offset is caclulated by SWGROUP ID so that we can get
> rid of old SoC specific MACROs. This ID conversion is needed for the
> unified SMMU driver over Tegra SoCs. We use dt-bindings MACRO instead
> of SoC dependent MACROs. The formula is:
> 
>   MC_SMMU_<swgroup name>_ASID_0 = MC_SMMU_AFI_ASID_0 + ID * 4;
> 
> Now SWGROUP ID is the global HardWare Accelerator(HWA) identifier
> among all Tegra SoC except Tegra2.

> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c

I would suggest deleting the following too, since they are presumably
specific to a single SoC:

> #define SMMU_AFI_ASID   0x238   /* PCIE */
> #define SMMU_AVPC_ASID  0x23c   /* AVP */
> #define SMMU_DC_ASID    0x240   /* Display controller */
> #define SMMU_DCB_ASID   0x244   /* Display controller B */
> #define SMMU_EPP_ASID   0x248   /* Encoder pre-processor */
> #define SMMU_G2_ASID    0x24c   /* 2D engine */
> #define SMMU_HC_ASID    0x250   /* Host1x */
> #define SMMU_HDA_ASID   0x254   /* High-def audio */
> #define SMMU_ISP_ASID   0x258   /* Image signal processor */
> #define SMMU_MPE_ASID   0x264   /* MPEG encoder */
> #define SMMU_NV_ASID    0x268   /* (3D) */
> #define SMMU_NV2_ASID   0x26c   /* (3D) */
> #define SMMU_PPCS_ASID  0x270   /* AHB */
> #define SMMU_SATA_ASID  0x278   /* SATA */
> #define SMMU_VDE_ASID   0x27c   /* Video decoder */
> #define SMMU_VI_ASID    0x280   /* Video input */

> +#define HWGRP_ASID_REG(x) ((x) * sizeof(u32) + SMMU_AFI_ASID)

Also, I would suggest renaming SMMU_AFI_ASID to something more like
SMMU_ASID_0 or SMMU_ASID_BASE so as not to call it the AFI register,
since presumably the whole point of this patch is that there's no
guarantee that register is the AFI ASId in all SoCs?

[PATCHv4 5/7] iommu/tegra: smmu: Support "mmu-masters" binding

[Hiroshi Doyu]

Follow arm,smmu's "mmu-masters" binding.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Update:
Newly added for v4. In v3, I used "nvidia,swgroups" and
"nvidia,memory-clients" bindings.
---
 .../bindings/iommu/nvidia,tegra30-smmu.txt         |  28 ++++-
 drivers/iommu/tegra-smmu.c                         | 138 +++++++++++++++++----
 2 files changed, 141 insertions(+), 25 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
index 89fb543..51884e9 100644
--- a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
@@ -8,9 +8,16 @@ Required properties:
 - nvidia,#asids : # of ASIDs
 - dma-window : IOVA start address and length.
 - nvidia,ahb : phandle to the ahb bus connected to SMMU.
+- mmu-masters   : A list of phandles to device nodes representing bus
+                  masters for which the SMMU can provide a translation
+                  and their corresponding StreamIDs (see example below).
+                  Each device node linked from this list must have a
+                  "#stream-id-cells" property, indicating the number of
+                  StreamIDs(swgroup ID) associated with it, which is defined
+		  in "include/dt-bindings/memory/tegra-swgroup.h".
 
 Example:
-	smmu {
+	iommu {
 		compatible = "nvidia,tegra30-smmu";
 		reg = <0x7000f010 0x02c
 		       0x7000f1f0 0x010
@@ -18,4 +25,23 @@ Example:
 		nvidia,#asids = <4>;		/* # of ASIDs */
 		dma-window = <0 0x40000000>;	/* IOVA start & length */
 		nvidia,ahb = <&ahb>;
+
+		mmu-masters = <&host1x TEGRA_SWGROUP_HC>,
+			      <&mpe TEGRA_SWGROUP_MPE>,
+			      <&vi TEGRA_SWGROUP_VI>,
+			      <&epp TEGRA_SWGROUP_EPP>,
+			      <&isp TEGRA_SWGROUP_ISP>,
+			      <&gr2d TEGRA_SWGROUP_G2>,
+			      <&gr3d TEGRA_SWGROUP_NV TEGRA_SWGROUP_NV2>,
+			      <&dc TEGRA_SWGROUP_DC>,
+			      <&dcb TEGRA_SWGROUP_DCB>,
+			      <&uarta TEGRA_SWGROUP_PPCS>,
+			      <&uartb TEGRA_SWGROUP_PPCS>,
+			      <&uartc TEGRA_SWGROUP_PPCS>,
+			      <&uartd TEGRA_SWGROUP_PPCS>,
+			      <&uarte TEGRA_SWGROUP_PPCS>,
+			      <&sdhci0 TEGRA_SWGROUP_PPCS>,
+			      <&sdhci1 TEGRA_SWGROUP_PPCS>,
+			      <&sdhci2 TEGRA_SWGROUP_PPCS>,
+			      <&sdhci3 TEGRA_SWGROUP_PPCS>;
 	};
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 67252e1..ab198ce 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -206,10 +206,12 @@ enum {
  * Per client for address space
  */
 struct smmu_client {
+	struct device_node	*of_node;
+	struct rb_node		node;
 	struct device		*dev;
 	struct list_head	list;
 	struct smmu_as		*as;
-	u32			hwgrp;
+	u64			hwgrp;
 };
 
 /*
@@ -249,6 +251,7 @@ struct smmu_device {
 	spinlock_t	lock;
 	char		*name;
 	struct device	*dev;
+	struct rb_root	clients;
 	struct page *avp_vector_page;	/* dummy page shared by all AS's */
 
 	/*
@@ -326,23 +329,22 @@ static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs)
  */
 #define FLUSH_SMMU_REGS(smmu)	smmu_read(smmu, SMMU_CONFIG)
 
-#define smmu_client_hwgrp(c) (u32)((c)->dev->platform_data)
-
 static int __smmu_client_set_hwgrp(struct smmu_client *c,
-				   unsigned long map, int on)
+				   u64 map, int on)
 {
 	int i;
 	struct smmu_as *as = c->as;
 	u32 val, offs, mask = SMMU_ASID_ENABLE(as->asid);
 	struct smmu_device *smmu = as->smmu;
+	unsigned long *bitmap = (unsigned long *)&map;
 
 	WARN_ON(!on && map);
 	if (on && !map)
 		return -EINVAL;
 	if (!on)
-		map = smmu_client_hwgrp(c);
+		map = c->hwgrp;
 
-	for_each_set_bit(i, &map, HWGRP_COUNT) {
+	for_each_set_bit(i, bitmap, TEGRA_SWGROUP_MAX) {
 		offs = HWGRP_ASID_REG(i);
 		val = smmu_read(smmu, offs);
 		if (on) {
@@ -360,7 +362,7 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,
 	return 0;
 
 err_hw_busy:
-	for_each_set_bit(i, &map, HWGRP_COUNT) {
+	for_each_set_bit(i, bitmap, TEGRA_SWGROUP_MAX) {
 		offs = HWGRP_ASID_REG(i);
 		val = smmu_read(smmu, offs);
 		val &= ~mask;
@@ -732,27 +734,26 @@ static int smmu_iommu_domain_has_cap(struct iommu_domain *domain,
 	return 0;
 }
 
+static struct smmu_client *find_smmu_client(struct smmu_device *smmu,
+					    struct device_node *dev_node);
+
 static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 				 struct device *dev)
 {
 	struct smmu_as *as = domain->priv;
 	struct smmu_device *smmu = as->smmu;
 	struct smmu_client *client, *c;
-	u32 map;
 	int err;
 
-	client = devm_kzalloc(smmu->dev, sizeof(*c), GFP_KERNEL);
+	client = find_smmu_client(smmu, dev->of_node);
 	if (!client)
-		return -ENOMEM;
-	client->dev = dev;
-	client->as = as;
-	map = (unsigned long)dev->platform_data;
-	if (!map)
 		return -EINVAL;
 
-	err = smmu_client_enable_hwgrp(client, map);
+	client->dev = dev;
+	client->as = as;
+	err = smmu_client_enable_hwgrp(client, client->hwgrp);
 	if (err)
-		goto err_hwgrp;
+		return -EINVAL;
 
 	spin_lock(&as->client_lock);
 	list_for_each_entry(c, &as->client, list) {
@@ -770,7 +771,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 	 * Reserve "page zero" for AVP vectors using a common dummy
 	 * page.
 	 */
-	if (map & TEGRA_SWGROUP_BIT(AVPC)) {
+	if (client->hwgrp & TEGRA_SWGROUP_BIT(AVPC)) {
 		struct page *page;
 
 		page = as->smmu->avp_vector_page;
@@ -785,8 +786,6 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 err_client:
 	smmu_client_disable_hwgrp(client);
 	spin_unlock(&as->client_lock);
-err_hwgrp:
-	devm_kfree(smmu->dev, client);
 	return err;
 }
 
@@ -803,7 +802,6 @@ static void smmu_iommu_detach_dev(struct iommu_domain *domain,
 		if (c->dev == dev) {
 			smmu_client_disable_hwgrp(c);
 			list_del(&c->list);
-			devm_kfree(smmu->dev, c);
 			c->as = NULL;
 			dev_dbg(smmu->dev,
 				"%s is detached\n", dev_name(c->dev));
@@ -907,11 +905,14 @@ enum {
 static int smmu_iommu_add_device(struct device *dev)
 {
 	int err = -EPROBE_DEFER;
-	u64 swgroups;
 	struct dma_iommu_mapping *map = NULL;
+	struct smmu_client *client;
+
+	client = find_smmu_client(smmu_handle, dev->of_node);
+	if (!client)
+		return -EINVAL;
 
-	swgroups = smmu_of_get_memory_client(dev);
-	switch (swgroups) {
+	switch (client->hwgrp) {
 	case TEGRA_SWGROUP_BIT(PPCS):
 		map = smmu_handle->map[SYSTEM_PROTECTED];
 		break;
@@ -924,7 +925,7 @@ static int smmu_iommu_add_device(struct device *dev)
 		err = arm_iommu_attach_device(dev, map);
 
 	pr_debug("swgroups=%016llx map=%p err=%d %s\n",
-		 swgroups, map, err, dev_name(dev));
+		 client->hwgrp, map, err, dev_name(dev));
 
 	return err;
 }
@@ -1151,6 +1152,77 @@ static void tegra_smmu_create_default_map(struct smmu_device *smmu)
 	}
 }
 
+static struct smmu_client *find_smmu_client(struct smmu_device *smmu,
+					    struct device_node *dev_node)
+{
+	struct rb_node *node = smmu->clients.rb_node;
+
+	while (node) {
+		struct smmu_client *client;
+
+		client = container_of(node, struct smmu_client, node);
+		if (dev_node < client->of_node)
+			node = node->rb_left;
+		else if (dev_node > client->of_node)
+			node = node->rb_right;
+		else
+			return client;
+	}
+
+	return NULL;
+}
+
+static int insert_smmu_client(struct smmu_device *smmu,
+			      struct smmu_client *client)
+{
+	struct rb_node **new, *parent;
+
+	new = &smmu->clients.rb_node;
+	parent = NULL;
+	while (*new) {
+		struct smmu_client *this;
+		this = container_of(*new, struct smmu_client, node);
+
+		parent = *new;
+		if (client->of_node < this->of_node)
+			new = &((*new)->rb_left);
+		else if (client->of_node > this->of_node)
+			new = &((*new)->rb_right);
+		else
+			return -EEXIST;
+	}
+
+	rb_link_node(&client->node, parent, new);
+	rb_insert_color(&client->node, &smmu->clients);
+	return 0;
+}
+
+static int register_smmu_client(struct smmu_device *smmu,
+				struct device *dev,
+				struct of_phandle_args *args)
+{
+	struct smmu_client *client;
+	int i;
+
+	client = find_smmu_client(smmu, args->np);
+	if (client) {
+		dev_err(dev,
+			"rejecting multiple registrations for client device %s\n",
+			args->np->full_name);
+		return -EBUSY;
+	}
+
+	client = devm_kzalloc(dev, sizeof(*client), GFP_KERNEL);
+	if (!client)
+		return -ENOMEM;
+
+	client->of_node = args->np;
+	for (i = 0; i < args->args_count; i++)
+		client->hwgrp |= 1ULL << args->args[i];
+
+	return insert_smmu_client(smmu, client);
+}
+
 static int tegra_smmu_probe(struct platform_device *pdev)
 {
 	struct smmu_device *smmu;
@@ -1158,6 +1230,7 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	int i, asids, err = 0;
 	dma_addr_t uninitialized_var(base);
 	size_t bytes, uninitialized_var(size);
+	struct of_phandle_args args;
 
 	if (smmu_handle)
 		return -EIO;
@@ -1238,6 +1311,23 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 		return err;
 	platform_set_drvdata(pdev, smmu);
 
+	i = 0;
+	smmu->clients = RB_ROOT;
+	while (true) {
+		err = of_parse_phandle_with_args(dev->of_node, "mmu-masters",
+						 "#stream-id-cells", i, &args);
+		if (err)
+			break;
+
+		err = register_smmu_client(smmu, dev, &args);
+		if (err) {
+			dev_err(dev, "failed to add client %s\n",
+				args.np->full_name);
+		}
+
+		i++;
+	}
+
 	smmu->avp_vector_page = alloc_page(GFP_KERNEL);
 	if (!smmu->avp_vector_page)
 		return -ENOMEM;
-- 
1.8.1.5

Re: [PATCHv4 5/7] iommu/tegra: smmu: Support "mmu-masters" binding

[Will Deacon]

Hi Hiroshi,

On Mon, Nov 11, 2013 at 08:31:56AM +0000, Hiroshi Doyu wrote:
> Follow arm,smmu's "mmu-masters" binding.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> Update:
> Newly added for v4. In v3, I used "nvidia,swgroups" and
> "nvidia,memory-clients" bindings.

[...]

> @@ -1151,6 +1152,77 @@ static void tegra_smmu_create_default_map(struct smmu_device *smmu)
>  	}
>  }
>  
> +static struct smmu_client *find_smmu_client(struct smmu_device *smmu,
> +					    struct device_node *dev_node)
> +{
> +	struct rb_node *node = smmu->clients.rb_node;
> +
> +	while (node) {
> +		struct smmu_client *client;
> +
> +		client = container_of(node, struct smmu_client, node);
> +		if (dev_node < client->of_node)
> +			node = node->rb_left;
> +		else if (dev_node > client->of_node)
> +			node = node->rb_right;
> +		else
> +			return client;
> +	}
> +
> +	return NULL;
> +}
> +
> +static int insert_smmu_client(struct smmu_device *smmu,
> +			      struct smmu_client *client)
> +{
> +	struct rb_node **new, *parent;
> +
> +	new = &smmu->clients.rb_node;
> +	parent = NULL;
> +	while (*new) {
> +		struct smmu_client *this;
> +		this = container_of(*new, struct smmu_client, node);
> +
> +		parent = *new;
> +		if (client->of_node < this->of_node)
> +			new = &((*new)->rb_left);
> +		else if (client->of_node > this->of_node)
> +			new = &((*new)->rb_right);
> +		else
> +			return -EEXIST;
> +	}
> +
> +	rb_link_node(&client->node, parent, new);
> +	rb_insert_color(&client->node, &smmu->clients);
> +	return 0;
> +}
> +
> +static int register_smmu_client(struct smmu_device *smmu,
> +				struct device *dev,
> +				struct of_phandle_args *args)
> +{
> +	struct smmu_client *client;
> +	int i;
> +
> +	client = find_smmu_client(smmu, args->np);
> +	if (client) {
> +		dev_err(dev,
> +			"rejecting multiple registrations for client device %s\n",
> +			args->np->full_name);
> +		return -EBUSY;
> +	}
> +
> +	client = devm_kzalloc(dev, sizeof(*client), GFP_KERNEL);
> +	if (!client)
> +		return -ENOMEM;
> +
> +	client->of_node = args->np;
> +	for (i = 0; i < args->args_count; i++)
> +		client->hwgrp |= 1ULL << args->args[i];
> +
> +	return insert_smmu_client(smmu, client);
> +}

This code looks familiar ;)

If this approach makes it past the DT guys, I wonder if there's any scope
for factoring out some of the arm-smmu code so that basic client
parsing/searching/registration can be made into a library?

Will
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 5/7] iommu/tegra: smmu: Support "mmu-masters" binding

[Hiroshi Doyu]

Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote @ Mon, 11 Nov 2013 12:35:10 +0100:

> This code looks familiar ;)

;)
 
> If this approach makes it past the DT guys, I wonder if there's any scope
> for factoring out some of the arm-smmu code so that basic client
> parsing/searching/registration can be made into a library?

That's what I thought too. I just didn't proceed further till I get
some feedback on this design first. As you suggested, some of arm,smmu
part can be enough generic so that some other of IOMMU drivers can
make use of that as a library.

Re: [PATCHv4 5/7] iommu/tegra: smmu: Support "mmu-masters" binding

[Stephen Warren]

On 11/11/2013 01:31 AM, Hiroshi Doyu wrote:
> Follow arm,smmu's "mmu-masters" binding.

> diff --git a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt

> +- mmu-masters   : A list of phandles to device nodes representing bus
> +                  masters for which the SMMU can provide a translation
> +                  and their corresponding StreamIDs (see example below).
> +                  Each device node linked from this list must have a
> +                  "#stream-id-cells" property, indicating the number of
> +                  StreamIDs(swgroup ID) associated with it, which is defined
> +		  in "include/dt-bindings/memory/tegra-swgroup.h".

Some of those lines are indented with TABs, others with spaces.

> +		mmu-masters = <&host1x TEGRA_SWGROUP_HC>,
> +			      <&mpe TEGRA_SWGROUP_MPE>,
> +			      <&vi TEGRA_SWGROUP_VI>,
> +			      <&epp TEGRA_SWGROUP_EPP>,
> +			      <&isp TEGRA_SWGROUP_ISP>,
> +			      <&gr2d TEGRA_SWGROUP_G2>,
> +			      <&gr3d TEGRA_SWGROUP_NV TEGRA_SWGROUP_NV2>,

So right now, the driver is statically assigning clients to a couple of
specific ASIDs. What if we want to configure that mapping from DT; does
that make sense? Instead of mmu-masters being a list of <phandle
streamid*>, should it be <phandle ASID streamid*> or <phandle (streamid
ASID)*>?




> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c

>  struct smmu_client {
...
> -	u32			hwgrp;
> +	u64			hwgrp;

I think that's used later with for_each_set_bit() etc. Should it be
declared as an explicit bitmap object, or at least an unsigned long to
directly match the bitmap APIs?

Related, what if someone bumps <dt-bindings/memory/tegra-swgroup.h>'s
TEGRA_SWGROUP_MAX to 96 without changing the code?

>  static int smmu_iommu_attach_dev(struct iommu_domain *domain,
>  				 struct device *dev)
...
> -	client = devm_kzalloc(smmu->dev, sizeof(*c), GFP_KERNEL);
> +	client = find_smmu_client(smmu, dev->of_node);
>  	if (!client)
... (deletions of replaced code)
>  		return -EINVAL;

-ENODEV cursorily sounds better? Same in smmu_iommu_add_device().

> @@ -1238,6 +1311,23 @@ static int tegra_smmu_probe(struct platform_device *pdev)

> +	i = 0;
> +	smmu->clients = RB_ROOT;
> +	while (true) {
> +		err = of_parse_phandle_with_args(dev->of_node, "mmu-masters",
> +						 "#stream-id-cells", i, &args);
> +		if (err)
> +			break;

An iterator macro similar to of_property_for_each_u32/string() might be
nicer, which could replace all that with:

of_property_for_each_phandle_with_args(dev->of_node, "mmu-masters",
					"#stream-id-cells") {

Re: [PATCHv4 5/7] iommu/tegra: smmu: Support "mmu-masters" binding

[Hiroshi Doyu]

On Wed, 13 Nov 2013 01:17:22 +0100
Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:

> > +		mmu-masters = <&host1x TEGRA_SWGROUP_HC>,
> > +			      <&mpe TEGRA_SWGROUP_MPE>,
> > +			      <&vi TEGRA_SWGROUP_VI>,
> > +			      <&epp TEGRA_SWGROUP_EPP>,
> > +			      <&isp TEGRA_SWGROUP_ISP>,
> > +			      <&gr2d TEGRA_SWGROUP_G2>,
> > +			      <&gr3d TEGRA_SWGROUP_NV TEGRA_SWGROUP_NV2>,
> 
> So right now, the driver is statically assigning clients to a couple of
> specific ASIDs. What if we want to configure that mapping from DT; does
> that make sense? Instead of mmu-masters being a list of <phandle
> streamid*>, should it be <phandle ASID streamid*> or <phandle (streamid
> ASID)*>?

That's possible.

Here, swgroup ID == stream ID, and a device is statically bind to a
specific swgroup ID(hard coded). ASID is dynamically assigned to
swgroup(devices). So assigning ASID belongs to a policy, but we can
consider this assigning as board specifc policy since it's hard to
change after kernel boots up in general. So assigning ASID in a board
DT makes sense. The format would be:

  <phandle "swgroup ID" "asid">,

ex:
  <&host1x TEGRA_SWGROUP_HC 0>,

The above depends on the discussion of the standard IOMMU bindings,
but the number of argument can be set by each IOMMU driver.

If we take the the other way,

 smmu: iommu@xxxxxx {
       #iommu-cells = <3>;
       ^^^^^^^^^^^^^^^^^^
   };

   host1x {
           compatible = "nvidia,tegra30-host1x", "simple-bus";
           iommu = <&smmu 0x??????? 0x??????? "asid">;
	   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^#######
           gr3d {
                   compatible = "nvidia,tegra30-gr3d";
                   iommu = <&smmu 0x??????? 0x???????>;
           }

I think that this "asid" part can be set 0 in tegra??.dtsi and the
actual value can be overwritten in tegra??-<boardname>.dts file.

Re: [PATCHv4 5/7] iommu/tegra: smmu: Support "mmu-masters" binding

[Stephen Warren]

On 11/13/2013 12:45 AM, Hiroshi Doyu wrote:
> On Wed, 13 Nov 2013 01:17:22 +0100
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> 
>>> +		mmu-masters = <&host1x TEGRA_SWGROUP_HC>,
>>> +			      <&mpe TEGRA_SWGROUP_MPE>,
>>> +			      <&vi TEGRA_SWGROUP_VI>,
>>> +			      <&epp TEGRA_SWGROUP_EPP>,
>>> +			      <&isp TEGRA_SWGROUP_ISP>,
>>> +			      <&gr2d TEGRA_SWGROUP_G2>,
>>> +			      <&gr3d TEGRA_SWGROUP_NV TEGRA_SWGROUP_NV2>,
>>
>> So right now, the driver is statically assigning clients to a couple of
>> specific ASIDs. What if we want to configure that mapping from DT; does
>> that make sense? Instead of mmu-masters being a list of <phandle
>> streamid*>, should it be <phandle ASID streamid*> or <phandle (streamid
>> ASID)*>?
> 
> That's possible.
> 
> Here, swgroup ID == stream ID, and a device is statically bind to a
> specific swgroup ID(hard coded). ASID is dynamically assigned to
> swgroup(devices). So assigning ASID belongs to a policy, but we can
> consider this assigning as board specifc policy since it's hard to
> change after kernel boots up in general. So assigning ASID in a board
> DT makes sense. The format would be:
> 
>   <phandle "swgroup ID" "asid">,
> 
> ex:
>   <&host1x TEGRA_SWGROUP_HC 0>,
> 
> The above depends on the discussion of the standard IOMMU bindings,
> but the number of argument can be set by each IOMMU driver.
> 
> If we take the the other way,
> 
>  smmu: iommu@xxxxxx {
>        #iommu-cells = <3>;
>        ^^^^^^^^^^^^^^^^^^
>    };
> 
>    host1x {
>            compatible = "nvidia,tegra30-host1x", "simple-bus";
>            iommu = <&smmu 0x??????? 0x??????? "asid">;
> 	   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^#######
>            gr3d {
>                    compatible = "nvidia,tegra30-gr3d";
>                    iommu = <&smmu 0x??????? 0x???????>;
>            }
> 
> I think that this "asid" part can be set 0 in tegra??.dtsi and the
> actual value can be overwritten in tegra??-<boardname>.dts file.

The one issue here is that we can only override entire properties, so
it's not possible for a board file to *just* replace the ASID, it'd have
to duplicate the entire property, just to change the one value.

Is the ASID mapping really likely to be board-specific though? To my
naive thinking, it seems that the chip design (e.g. number of
peripherals, number of available ASIDs) would tend to imply the
device->ASID mapping, since it would have been considered as part of
chip design. Hence, wouldn't soc.dtsi typically specify the expected
ASID mapping, and boards rarely if ever override it?

If the ASID mapping really is likely to vary per board, perhaps it makes
sense to put it into a separate property somehow so it's easier to override?

Re: [PATCHv4 5/7] iommu/tegra: smmu: Support "mmu-masters" binding

[Hiroshi Doyu]

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 13 Nov 2013 18:58:23 +0100:

> >  smmu: iommu@xxxxxx {
> >        #iommu-cells = <3>;
> >        ^^^^^^^^^^^^^^^^^^
> >    };
> > 
> >    host1x {
> >            compatible = "nvidia,tegra30-host1x", "simple-bus";
> >            iommu = <&smmu 0x??????? 0x??????? "asid">;
> > 	   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^#######
> >            gr3d {
> >                    compatible = "nvidia,tegra30-gr3d";
> >                    iommu = <&smmu 0x??????? 0x???????>;
> >            }
> > 
> > I think that this "asid" part can be set 0 in tegra??.dtsi and the
> > actual value can be overwritten in tegra??-<boardname>.dts file.
> 
> The one issue here is that we can only override entire properties, so
> it's not possible for a board file to *just* replace the ASID, it'd have
> to duplicate the entire property, just to change the one value.
> 
> Is the ASID mapping really likely to be board-specific though? To my
> naive thinking, it seems that the chip design (e.g. number of
> peripherals, number of available ASIDs) would tend to imply the
> device->ASID mapping, since it would have been considered as part of
> chip design. Hence, wouldn't soc.dtsi typically specify the expected
> ASID mapping, and boards rarely if ever override it?
> 
> If the ASID mapping really is likely to vary per board, perhaps it makes
> sense to put it into a separate property somehow so it's easier to override?

  Older Tegra like T30: swgroups > asid(==4)
  Newer Tegra         : swgroups < asid

At newer Tegra, each swgroup should have one asid. In older Tegra,
there aren't many options how to assign ASID since # of AS is 4, and
we know which one should be isolated/protected first. Even we can fix
this assignment for a SoC, not for a board.

Also I think that a newer tegra won't care this mapping since all will
have an own AS. So maybe the binding between "swgroup" and "asid"
should be another one, since this info is only needed for the older
Tegra. For example:

  smmu: iommu@xxxxxx {
        #iommu-cells = <2>;

	/*
	 * This as <-> swgroups mapping is only needed for older tegra.
	 */
	as-swgroups = <0x??????? 0x???????,  /* AS[0]: assigned swgroups bits */
		       0x??????? 0x???????,  /* AS[1]: assigned swgroups bits */
		       0x??????? 0x???????,  /* AS[2]: assigned swgroups bits */
		       0x??????? 0x???????>; /* AS[3]: assigned swgroups bits */
    };
 
    host1x {
            compatible = "nvidia,tegra30-host1x", "simple-bus";
            iommus = <&smmu 0x??????? 0x???????>;
            gr3d {
                    compatible = "nvidia,tegra30-gr3d";
                    iommus = <&smmu 0x??????? 0x???????>;
            }
   };

Then we would push this mapping info into DT only for older
Tegra. Newer Tegra won't care where each swgroup will get its own.

Re: [PATCHv4 5/7] iommu/tegra: smmu: Support "mmu-masters" binding

[Stephen Warren]

On 11/13/2013 11:41 PM, Hiroshi Doyu wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 13 Nov 2013 18:58:23 +0100:
> 
>>>  smmu: iommu@xxxxxx {
>>>        #iommu-cells = <3>;
>>>        ^^^^^^^^^^^^^^^^^^
>>>    };
>>>
>>>    host1x {
>>>            compatible = "nvidia,tegra30-host1x", "simple-bus";
>>>            iommu = <&smmu 0x??????? 0x??????? "asid">;
>>> 	   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^#######
>>>            gr3d {
>>>                    compatible = "nvidia,tegra30-gr3d";
>>>                    iommu = <&smmu 0x??????? 0x???????>;
>>>            }
>>>
>>> I think that this "asid" part can be set 0 in tegra??.dtsi and the
>>> actual value can be overwritten in tegra??-<boardname>.dts file.
>>
>> The one issue here is that we can only override entire properties, so
>> it's not possible for a board file to *just* replace the ASID, it'd have
>> to duplicate the entire property, just to change the one value.
>>
>> Is the ASID mapping really likely to be board-specific though? To my
>> naive thinking, it seems that the chip design (e.g. number of
>> peripherals, number of available ASIDs) would tend to imply the
>> device->ASID mapping, since it would have been considered as part of
>> chip design. Hence, wouldn't soc.dtsi typically specify the expected
>> ASID mapping, and boards rarely if ever override it?
>>
>> If the ASID mapping really is likely to vary per board, perhaps it makes
>> sense to put it into a separate property somehow so it's easier to override?
> 
>   Older Tegra like T30: swgroups > asid(==4)
>   Newer Tegra         : swgroups < asid

In that case, I'd vote for hard-coding the mapping in the driver in all
cases. For older Tegra, we'll have to hard-code some static mapping just
like you've already done in the driver. For newer Tegra, we would just
assign a new AS for each swgroup as you say. If we ever need to tweak
this, we can invent a new DT property to affect the default. That makes
the DT content quite a bit simpler for now:-)

Re: [PATCHv4 5/7] iommu/tegra: smmu: Support "mmu-masters" binding

[Kumar Gala]

On Nov 11, 2013, at 2:31 AM, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:

> Follow arm,smmu's "mmu-masters" binding.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> Update:
> Newly added for v4. In v3, I used "nvidia,swgroups" and
> "nvidia,memory-clients" bindings.
> ---
> .../bindings/iommu/nvidia,tegra30-smmu.txt         |  28 ++++-
> drivers/iommu/tegra-smmu.c                         | 138 +++++++++++++++++----
> 2 files changed, 141 insertions(+), 25 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
> index 89fb543..51884e9 100644
> --- a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
> +++ b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
> @@ -8,9 +8,16 @@ Required properties:
> - nvidia,#asids : # of ASIDs
> - dma-window : IOVA start address and length.
> - nvidia,ahb : phandle to the ahb bus connected to SMMU.
> +- mmu-masters   : A list of phandles to device nodes representing bus
> +                  masters for which the SMMU can provide a translation
> +                  and their corresponding StreamIDs (see example below).
> +                  Each device node linked from this list must have a
> +                  "#stream-id-cells" property, indicating the number of
> +                  StreamIDs(swgroup ID) associated with it, which is defined
> +		  in "include/dt-bindings/memory/tegra-swgroup.h".
> 
> Example:
> -	smmu {
> +	iommu {
> 		compatible = "nvidia,tegra30-smmu";
> 		reg = <0x7000f010 0x02c
> 		       0x7000f1f0 0x010
> @@ -18,4 +25,23 @@ Example:
> 		nvidia,#asids = <4>;		/* # of ASIDs */
> 		dma-window = <0 0x40000000>;	/* IOVA start & length */
> 		nvidia,ahb = <&ahb>;
> +
> +		mmu-masters = <&host1x TEGRA_SWGROUP_HC>,
> +			      <&mpe TEGRA_SWGROUP_MPE>,
> +			      <&vi TEGRA_SWGROUP_VI>,
> +			      <&epp TEGRA_SWGROUP_EPP>,
> +			      <&isp TEGRA_SWGROUP_ISP>,
> +			      <&gr2d TEGRA_SWGROUP_G2>,
> +			      <&gr3d TEGRA_SWGROUP_NV TEGRA_SWGROUP_NV2>,
> +			      <&dc TEGRA_SWGROUP_DC>,
> +			      <&dcb TEGRA_SWGROUP_DCB>,
> +			      <&uarta TEGRA_SWGROUP_PPCS>,
> +			      <&uartb TEGRA_SWGROUP_PPCS>,
> +			      <&uartc TEGRA_SWGROUP_PPCS>,
> +			      <&uartd TEGRA_SWGROUP_PPCS>,
> +			      <&uarte TEGRA_SWGROUP_PPCS>,
> +			      <&sdhci0 TEGRA_SWGROUP_PPCS>,
> +			      <&sdhci1 TEGRA_SWGROUP_PPCS>,
> +			      <&sdhci2 TEGRA_SWGROUP_PPCS>,
> +			      <&sdhci3 TEGRA_SWGROUP_PPCS>;
> 	};

At first glance this seems backward from all other cases we have in which we usually have the device have a property that points back, like interrupt-parent.

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

[PATCHv4 6/7] iommu/tegra: smmu: Rename hwgrp -> swgroups

[Hiroshi Doyu]

Use the correct term for SWGROUP related variables and macros.

The term "swgroup" is the collection of "memory client". A "memory
client" usually represents a HardWare Accelerator(HWA) like
GPU. Sometimes a strut device can belong to multiple "swgroup" so that
"swgroup's'" is used here. This "swgroups" is the term used in Tegra
TRM. Rename along with TRM.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Update:
New for v4
---
 drivers/iommu/tegra-smmu.c | 44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index ab198ce..904c36a 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -193,14 +193,12 @@ enum {
 
 #define NUM_SMMU_REG_BANKS	3
 
-#define smmu_client_enable_hwgrp(c, m)	smmu_client_set_hwgrp(c, m, 1)
-#define smmu_client_disable_hwgrp(c)	smmu_client_set_hwgrp(c, 0, 0)
-#define __smmu_client_enable_hwgrp(c, m) __smmu_client_set_hwgrp(c, m, 1)
-#define __smmu_client_disable_hwgrp(c)	__smmu_client_set_hwgrp(c, 0, 0)
+#define smmu_client_enable_swgroups(c, m) smmu_client_set_swgroups(c, m, 1)
+#define smmu_client_disable_swgroups(c)	smmu_client_set_swgroups(c, 0, 0)
+#define __smmu_client_enable_swgroups(c, m) __smmu_client_set_swgroups(c, m, 1)
+#define __smmu_client_disable_swgroups(c) __smmu_client_set_swgroups(c, 0, 0)
 
-#define HWGRP_INIT(client) [HWGRP_##client] = SMMU_##client##_ASID
-
-#define HWGRP_ASID_REG(x) ((x) * sizeof(u32) + SMMU_AFI_ASID)
+#define SWGROUP_ASID_REG(x) ((x) * sizeof(u32) + SMMU_AFI_ASID)
 
 /*
  * Per client for address space
@@ -211,7 +209,7 @@ struct smmu_client {
 	struct device		*dev;
 	struct list_head	list;
 	struct smmu_as		*as;
-	u64			hwgrp;
+	u64			swgroups;
 };
 
 /*
@@ -329,7 +327,7 @@ static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs)
  */
 #define FLUSH_SMMU_REGS(smmu)	smmu_read(smmu, SMMU_CONFIG)
 
-static int __smmu_client_set_hwgrp(struct smmu_client *c,
+static int __smmu_client_set_swgroups(struct smmu_client *c,
 				   u64 map, int on)
 {
 	int i;
@@ -342,10 +340,10 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,
 	if (on && !map)
 		return -EINVAL;
 	if (!on)
-		map = c->hwgrp;
+		map = c->swgroups;
 
 	for_each_set_bit(i, bitmap, TEGRA_SWGROUP_MAX) {
-		offs = HWGRP_ASID_REG(i);
+		offs = SWGROUP_ASID_REG(i);
 		val = smmu_read(smmu, offs);
 		if (on) {
 			if (WARN_ON(val & mask))
@@ -358,12 +356,12 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,
 		smmu_write(smmu, val, offs);
 	}
 	FLUSH_SMMU_REGS(smmu);
-	c->hwgrp = map;
+	c->swgroups = map;
 	return 0;
 
 err_hw_busy:
 	for_each_set_bit(i, bitmap, TEGRA_SWGROUP_MAX) {
-		offs = HWGRP_ASID_REG(i);
+		offs = SWGROUP_ASID_REG(i);
 		val = smmu_read(smmu, offs);
 		val &= ~mask;
 		smmu_write(smmu, val, offs);
@@ -371,7 +369,7 @@ err_hw_busy:
 	return -EBUSY;
 }
 
-static int smmu_client_set_hwgrp(struct smmu_client *c, u32 map, int on)
+static int smmu_client_set_swgroups(struct smmu_client *c, u32 map, int on)
 {
 	u32 val;
 	unsigned long flags;
@@ -379,7 +377,7 @@ static int smmu_client_set_hwgrp(struct smmu_client *c, u32 map, int on)
 	struct smmu_device *smmu = as->smmu;
 
 	spin_lock_irqsave(&smmu->lock, flags);
-	val = __smmu_client_set_hwgrp(c, map, on);
+	val = __smmu_client_set_swgroups(c, map, on);
 	spin_unlock_irqrestore(&smmu->lock, flags);
 	return val;
 }
@@ -419,7 +417,7 @@ static int smmu_setup_regs(struct smmu_device *smmu)
 		smmu_write(smmu, val, SMMU_PTB_DATA);
 
 		list_for_each_entry(c, &as->client, list)
-			__smmu_client_set_hwgrp(c, c->hwgrp, 1);
+			__smmu_client_set_swgroups(c, c->swgroups, 1);
 	}
 
 	smmu_write(smmu, smmu->translation_enable_0, SMMU_TRANSLATION_ENABLE_0);
@@ -751,7 +749,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 
 	client->dev = dev;
 	client->as = as;
-	err = smmu_client_enable_hwgrp(client, client->hwgrp);
+	err = smmu_client_enable_swgroups(client, client->swgroups);
 	if (err)
 		return -EINVAL;
 
@@ -771,7 +769,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 	 * Reserve "page zero" for AVP vectors using a common dummy
 	 * page.
 	 */
-	if (client->hwgrp & TEGRA_SWGROUP_BIT(AVPC)) {
+	if (client->swgroups & TEGRA_SWGROUP_BIT(AVPC)) {
 		struct page *page;
 
 		page = as->smmu->avp_vector_page;
@@ -784,7 +782,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 	return 0;
 
 err_client:
-	smmu_client_disable_hwgrp(client);
+	smmu_client_disable_swgroups(client);
 	spin_unlock(&as->client_lock);
 	return err;
 }
@@ -800,7 +798,7 @@ static void smmu_iommu_detach_dev(struct iommu_domain *domain,
 
 	list_for_each_entry(c, &as->client, list) {
 		if (c->dev == dev) {
-			smmu_client_disable_hwgrp(c);
+			smmu_client_disable_swgroups(c);
 			list_del(&c->list);
 			c->as = NULL;
 			dev_dbg(smmu->dev,
@@ -912,7 +910,7 @@ static int smmu_iommu_add_device(struct device *dev)
 	if (!client)
 		return -EINVAL;
 
-	switch (client->hwgrp) {
+	switch (client->swgroups) {
 	case TEGRA_SWGROUP_BIT(PPCS):
 		map = smmu_handle->map[SYSTEM_PROTECTED];
 		break;
@@ -925,7 +923,7 @@ static int smmu_iommu_add_device(struct device *dev)
 		err = arm_iommu_attach_device(dev, map);
 
 	pr_debug("swgroups=%016llx map=%p err=%d %s\n",
-		 client->hwgrp, map, err, dev_name(dev));
+		 client->swgroups, map, err, dev_name(dev));
 
 	return err;
 }
@@ -1218,7 +1216,7 @@ static int register_smmu_client(struct smmu_device *smmu,
 
 	client->of_node = args->np;
 	for (i = 0; i < args->args_count; i++)
-		client->hwgrp |= 1ULL << args->args[i];
+		client->swgroups |= 1ULL << args->args[i];
 
 	return insert_smmu_client(smmu, client);
 }
-- 
1.8.1.5

[PATCHv4 7/7] iommu/tegra: smmu: Allow duplicate ASID wirte

[Hiroshi Doyu]

The device, which belongs to the same ASID, can try to enable the same
ASID as the other swgroup devices. This should be allowed but just
skip the actual register write. If the write value is different, it
will return -EINVAL.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Update:
This was the part of v3, which isn't used any more.
  [PATCHv3 10/19] iommu/tegra: smmu: Get "nvidia,swgroups" from DT
---
 drivers/iommu/tegra-smmu.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 904c36a..70f974c 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -346,8 +346,11 @@ static int __smmu_client_set_swgroups(struct smmu_client *c,
 		offs = SWGROUP_ASID_REG(i);
 		val = smmu_read(smmu, offs);
 		if (on) {
-			if (WARN_ON(val & mask))
-				goto err_hw_busy;
+			if (val) {
+				if (WARN_ON(val != mask))
+					return -EINVAL;
+				goto skip;
+			}
 			val |= mask;
 		} else {
 			WARN_ON((val & mask) == mask);
@@ -357,16 +360,8 @@ static int __smmu_client_set_swgroups(struct smmu_client *c,
 	}
 	FLUSH_SMMU_REGS(smmu);
 	c->swgroups = map;
+skip:
 	return 0;
-
-err_hw_busy:
-	for_each_set_bit(i, bitmap, TEGRA_SWGROUP_MAX) {
-		offs = SWGROUP_ASID_REG(i);
-		val = smmu_read(smmu, offs);
-		val &= ~mask;
-		smmu_write(smmu, val, offs);
-	}
-	return -EBUSY;
 }
 
 static int smmu_client_set_swgroups(struct smmu_client *c, u32 map, int on)
-- 
1.8.1.5

Re: [PATCHv4 7/7] iommu/tegra: smmu: Allow duplicate ASID wirte

[Stephen Warren]

On 11/11/2013 01:31 AM, Hiroshi Doyu wrote:
> The device, which belongs to the same ASID, can try to enable the same
> ASID as the other swgroup devices. This should be allowed but just
> skip the actual register write. If the write value is different, it
> will return -EINVAL.

That's pretty confusing. I'm not at all sure I understand what it means.

I /think/ you're saying that multiple devices can exist whose swgroup
values are the same, and when setting up those devices, the ASID
register for that swgroup gets written multiple times, once per device.
This change allows that, assuming that all the different devices attempt
to select the same ASID for that swgroup. If so,

Giving some specific examples of devices with the same swgroup value
would be useful. Along with sternly talking to the HW designers and
telling them to just give everything unique IDs:-(

> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c

> @@ -346,8 +346,11 @@ static int __smmu_client_set_swgroups(struct smmu_client *c,
>  		offs = SWGROUP_ASID_REG(i);
>  		val = smmu_read(smmu, offs);
>  		if (on) {
> -			if (WARN_ON(val & mask))
> -				goto err_hw_busy;
> +			if (val) {

Why only check this if (val)? Surely the check is valid if (val == 0)
too; you don't want to go writing 0 into the ASID register if it's
already configured to point at something else?

Or, is "val" a bitmask of ASIDs, not an integer representing the ASID?

> +				if (WARN_ON(val != mask))
> +					return -EINVAL;
> +				goto skip;
> +			}
>  			val |= mask;

... I guess this would imply it's a bitmask.

But then that begs the question: if (on) means that ASID mapping is
being enabled, then isn't that invalid if (!mask)?

Re: [PATCHv4 0/7] Unifying SMMU driver among Tegra SoCs

[Stephen Warren]

On 11/11/2013 01:31 AM, Hiroshi Doyu wrote:
> Hi,
> 
> This series provides:
> 
> (1) Unified SMMU driver among Tegra SoCs
> (2) Multiple Address Space support(MASID) in IOMMU(SMMMU)
> (3) Tegra IOMMU'able devices, most of platform devices are IOMMU'able.
> 
> There's been some discussion[1] about device population order, and I
> added a IOMMU hook in driver core:
> 
>   [PATCHv4 2/7] driver/core: Populate IOMMU'able devices in order
> 
> which is based on the following RFC patch:
> 
>   [RFC][PATCHv3+ 1/2] driver/core: Add of_iommu_attach()
>   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006914.html

I assume that "based on" means "is a new version of", so I don't need to
look at the "PATCHv3+" patch, since this series replaces it?

Re: [PATCHv4 0/7] Unifying SMMU driver among Tegra SoCs

[Hiroshi Doyu]

On Tue, 12 Nov 2013 23:40:21 +0100
Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:

> On 11/11/2013 01:31 AM, Hiroshi Doyu wrote:
> > Hi,
> > 
> > This series provides:
> > 
> > (1) Unified SMMU driver among Tegra SoCs
> > (2) Multiple Address Space support(MASID) in IOMMU(SMMMU)
> > (3) Tegra IOMMU'able devices, most of platform devices are IOMMU'able.
> > 
> > There's been some discussion[1] about device population order, and I
> > added a IOMMU hook in driver core:
> > 
> >   [PATCHv4 2/7] driver/core: Populate IOMMU'able devices in order
> > 
> > which is based on the following RFC patch:
> > 
> >   [RFC][PATCHv3+ 1/2] driver/core: Add of_iommu_attach()
> >   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006914.html
> 
> I assume that "based on" means "is a new version of", so I don't need to
> look at the "PATCHv3+" patch, since this series replaces it?

Right.