[PATCHv5 0/9] Unifying Tegra IOMMU(SMMU) driver among Tegra SoCs

[PATCHv5 0/9] Unifying Tegra IOMMU(SMMU) driver among Tegra SoCs

[Hiroshi Doyu]

Hi,

This series provide:

(1) Unified IOMMU(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 for
the solution I implemented an IOMMU hook in driver core:

  [PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

which is based on:
  http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006933.html

The main problem here is,

IOMMU devices on the bus need to be poplulated first, then iommu
master devices are done later.

With CONFIG_OF_IOMMU, "iommus=" DT binding would be used to identify
whether a device can be an iommu msater or not. If a device can, 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 master devices are populated and
configured for IOMMU with help of the already populated iommu device
via iommu_ops->add_device(). Multiple IOMMUs can be listed on this
"iommus" binding so that a device can have multiple IOMMUs attached.

Currenly this "iommus=" binding is used as the global binding.

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.
  http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006931.html

v3:
Updated based on Stephen Warren's feedback
  http://lists.linuxfoundation.org/pipermail/iommu/2013-October/006724.html

v2:
Updated based on Thierry Reding's and Stephen Warren's feedback
  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 at nv-tegra.nvidia.com/user/hdoyu/linux.git smmu-upstreaming at 20131119


Hiroshi Doyu (9):
  of: introduce of_property_for_earch_phandle_with_args()
  driver/core: populate devices in order for IOMMUs
  ARM: tegra: create a DT header defining SWGROUP ID
  iommu/tegra: smmu: register device to iommu dynamically
  iommu/tegra: smmu: calculate ASID register offset by ID
  iommu/tegra: smmu: get swgroups from DT "iommus="
  iommu/tegra: smmu: allow duplicate ASID wirte
  iommu/tegra: smmu: Rename hwgrp -> swgroups
  [FOR TEST] ARM: dt: tegra30: add "iommus" binding

 .../bindings/iommu/nvidia,tegra30-smmu.txt         |  17 +-
 arch/arm/boot/dts/tegra30.dtsi                     |  23 +-
 drivers/base/dd.c                                  |   5 +
 drivers/iommu/Kconfig                              |   1 +
 drivers/iommu/of_iommu.c                           |  22 ++
 drivers/iommu/tegra-smmu.c                         | 334 +++++++++++++--------
 include/dt-bindings/memory/tegra-swgroup.h         |  50 +++
 include/linux/of.h                                 |   3 +
 include/linux/of_iommu.h                           |   7 +
 9 files changed, 336 insertions(+), 126 deletions(-)
 create mode 100644 include/dt-bindings/memory/tegra-swgroup.h

[1] https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/thread.html#36542
-- 
1.8.1.5

[PATCHv5 1/9] of: introduce of_property_for_earch_phandle_with_args()

[Hiroshi Doyu]

The following pattern of code is tempting:

  for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++)

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
v5:
New patch for v5.
---
 include/linux/of.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/of.h b/include/linux/of.h
index 276c546..131fef5 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -613,6 +613,9 @@ static inline int of_property_read_u32(const struct device_node *np,
 		s;						\
 		s = of_prop_next_string(prop, s))
 
+#define of_property_for_each_phandle_with_args(np, list, cells, i, args) \
+	for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++)
+
 #if defined(CONFIG_PROC_FS) && defined(CONFIG_PROC_DEVICETREE)
 extern void proc_device_tree_add_node(struct device_node *, struct proc_dir_entry *);
 extern void proc_device_tree_add_prop(struct proc_dir_entry *pde, struct property *prop);
-- 
1.8.1.5

[PATCHv5 1/9] of: introduce of_property_for_earch_phandle_with_args()

[Grant Likely]

On Tue, 19 Nov 2013 11:33:05 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> The following pattern of code is tempting:
> 
>   for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++)
> 
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>

That's a very minimal commit message. Can you elaborate please.

> ---
> v5:
> New patch for v5.
> ---
>  include/linux/of.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 276c546..131fef5 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -613,6 +613,9 @@ static inline int of_property_read_u32(const struct device_node *np,
>  		s;						\
>  		s = of_prop_next_string(prop, s))
>  
> +#define of_property_for_each_phandle_with_args(np, list, cells, i, args) \
> +	for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++)
> +

That works, but pretty darn inefficient. We need an iterator version of
of_parse_phandle_with_args() to loop over all the entries.

>  #if defined(CONFIG_PROC_FS) && defined(CONFIG_PROC_DEVICETREE)
>  extern void proc_device_tree_add_node(struct device_node *, struct proc_dir_entry *);
>  extern void proc_device_tree_add_prop(struct proc_dir_entry *pde, struct property *prop);
> -- 
> 1.8.1.5
> 

[PATCHv5 1/9] of: introduce of_property_for_earch_phandle_with_args()

[Hiroshi Doyu]

On Thu, 21 Nov 2013 13:43:28 +0100
Grant Likely <grant.likely@linaro.org> wrote:

> On Tue, 19 Nov 2013 11:33:05 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> > The following pattern of code is tempting:
> > 
> >   for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++)
> > 
> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> 
> That's a very minimal commit message. Can you elaborate please.

The above can be:

"
  The following pattern of code is tempting to add a new member for
  of_property_for_each_*() family as an idiom.
  
    for (i = 0;
        !of_parse_phandle_with_args(np, list, cells, i, args); i++)
                  <do something with "args">;
"

Actual usage is here:

        int i;
        struct of_phandle_args args;

        of_property_for_each_phandle_with_args(dev->of_node, "iommus",
                                               "#iommu-cells", i, &args) {
                pr_debug("%s(i=%d) %s\n", __func__, i, dev_name(dev));

                if (!of_find_iommu_by_node(args.np))
                        return -EPROBE_DEFER;

Is this acceptable?

[PATCHv5 1/9] of: introduce of_property_for_earch_phandle_with_args()

[Grant Likely]

On Thu, 21 Nov 2013 15:12:18 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> On Thu, 21 Nov 2013 13:43:28 +0100
> Grant Likely <grant.likely@linaro.org> wrote:
> 
> > On Tue, 19 Nov 2013 11:33:05 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> > > The following pattern of code is tempting:
> > > 
> > >   for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++)
> > > 
> > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > 
> > That's a very minimal commit message. Can you elaborate please.
> 
> The above can be:
> 
> "
>   The following pattern of code is tempting to add a new member for
>   of_property_for_each_*() family as an idiom.
>   
>     for (i = 0;
>         !of_parse_phandle_with_args(np, list, cells, i, args); i++)
>                   <do something with "args">;
> "

I really do like commit messages to be full enough that a future reader
can figure out why a patch was written. ie:

	"Iterating over a property containing a list of phandles with
	arguments is a common operation for device drivers. This patch
	adds a new of_property_for_each_phandle_with_args() macro to
	make the iteration simpler."

g.

> 
> Actual usage is here:
> 
>         int i;
>         struct of_phandle_args args;
> 
>         of_property_for_each_phandle_with_args(dev->of_node, "iommus",
>                                                "#iommu-cells", i, &args) {
>                 pr_debug("%s(i=%d) %s\n", __func__, i, dev_name(dev));
> 
>                 if (!of_find_iommu_by_node(args.np))
>                         return -EPROBE_DEFER;
> 
> Is this acceptable?

[PATCHv5 1/9] of: introduce of_property_for_earch_phandle_with_args()

[Hiroshi Doyu]

Grant Likely <grant.likely@linaro.org> wrote @ Thu, 21 Nov 2013 16:56:49 +0100:

> On Thu, 21 Nov 2013 15:12:18 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> > On Thu, 21 Nov 2013 13:43:28 +0100
> > Grant Likely <grant.likely@linaro.org> wrote:
> > 
> > > On Tue, 19 Nov 2013 11:33:05 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> > > > The following pattern of code is tempting:
> > > > 
> > > >   for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++)
> > > > 
> > > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > > 
> > > That's a very minimal commit message. Can you elaborate please.
> > 
> > The above can be:
> > 
> > "
> >   The following pattern of code is tempting to add a new member for
> >   of_property_for_each_*() family as an idiom.
> >   
> >     for (i = 0;
> >         !of_parse_phandle_with_args(np, list, cells, i, args); i++)
> >                   <do something with "args">;
> > "
> 
> I really do like commit messages to be full enough that a future reader
> can figure out why a patch was written. ie:

Updated as:

  [PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args()
    http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007063.html

This doesn't depend on anything and this can be merged
independetly. Thanks for your help.

[PATCHv5 1/9] of: introduce of_property_for_earch_phandle_with_args()

[Stephen Warren]

On 11/21/2013 10:20 AM, Hiroshi Doyu wrote:
> Grant Likely <grant.likely@linaro.org> wrote @ Thu, 21 Nov 2013 16:56:49 +0100:
> 
>> On Thu, 21 Nov 2013 15:12:18 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
>>> On Thu, 21 Nov 2013 13:43:28 +0100
>>> Grant Likely <grant.likely@linaro.org> wrote:
>>>
>>>> On Tue, 19 Nov 2013 11:33:05 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
>>>>> The following pattern of code is tempting:
>>>>>
>>>>>   for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++)
>>>>>
>>>>> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
>>>>
>>>> That's a very minimal commit message. Can you elaborate please.
>>>
>>> The above can be:
>>>
>>> "
>>>   The following pattern of code is tempting to add a new member for
>>>   of_property_for_each_*() family as an idiom.
>>>   
>>>     for (i = 0;
>>>         !of_parse_phandle_with_args(np, list, cells, i, args); i++)
>>>                   <do something with "args">;
>>> "
>>
>> I really do like commit messages to be full enough that a future reader
>> can figure out why a patch was written. ie:
> 
> Updated as:
> 
>   [PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args()
>     http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007063.html
> 
> This doesn't depend on anything and this can be merged
> independetly. Thanks for your help.

Well, that patch doesn't depend /on/ anything, but something in the rest
of the series does depend /on it/. As such, this patch can't be merged
completely independently; it has to either:

a) Go into whatever branch the rest of the series goes into.

b) Go into a topic branch in the DT tree, which is then both merged into
the main/regular DT tree /and/ used as a base for the rest of this series.

Dependencies work two ways!

(That is, unless this 1 patch gets merged into 3.14, and the rest of
series doesn't get merged until 3.15. In that case, we can ignore the
dependencies).

[PATCHv5 1/9] of: introduce of_property_for_earch_phandle_with_args()

[Rob Herring]

On Thu, Nov 21, 2013 at 11:20 AM, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> Grant Likely <grant.likely@linaro.org> wrote @ Thu, 21 Nov 2013 16:56:49 +0100:
>
>> On Thu, 21 Nov 2013 15:12:18 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
>> > On Thu, 21 Nov 2013 13:43:28 +0100
>> > Grant Likely <grant.likely@linaro.org> wrote:
>> >
>> > > On Tue, 19 Nov 2013 11:33:05 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
>> > > > The following pattern of code is tempting:
>> > > >
>> > > >   for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++)
>> > > >
>> > > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
>> > >
>> > > That's a very minimal commit message. Can you elaborate please.
>> >
>> > The above can be:
>> >
>> > "
>> >   The following pattern of code is tempting to add a new member for
>> >   of_property_for_each_*() family as an idiom.
>> >
>> >     for (i = 0;
>> >         !of_parse_phandle_with_args(np, list, cells, i, args); i++)
>> >                   <do something with "args">;
>> > "
>>
>> I really do like commit messages to be full enough that a future reader
>> can figure out why a patch was written. ie:
>
> Updated as:
>
>   [PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args()

You also have a typo in the subject.

s/earch/each/

Rob

[PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

[Hiroshi Doyu]

IOMMU devices on the bus need to be poplulated first, then iommu
master devices are done later.

With CONFIG_OF_IOMMU, "iommus=" DT binding would be used to identify
whether a device can be an iommu msater or not. If a device can, 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 master devices are populated and
configured for IOMMU with help of the already populated iommu device
via iommu_ops->add_device(). Multiple IOMMUs can be listed on this
"iommus" binding so that a device can have multiple IOMMUs attached.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
v5:
Use "iommus=" binding instread of arm,smmu's "#stream-id-cells".

v4:
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 | 22 ++++++++++++++++++++++
 include/linux/of_iommu.h |  7 +++++++
 3 files changed, 34 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..4aef2b2 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,23 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+int of_iommu_attach(struct device *dev)
+{
+	int i;
+	struct of_phandle_args args;
+	struct iommu_ops *ops = dev->bus->iommu_ops;
+
+	of_property_for_each_phandle_with_args(dev->of_node, "iommus",
+					       "#iommu-cells", i, &args) {
+		pr_debug("%s(i=%d) ops=%p %s\n",
+			 __func__, i, ops, dev_name(dev));
+
+		if (!ops)
+			return -EPROBE_DEFER;
+	}
+
+	if (i && 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

[PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

[Thierry Reding]

On Tue, Nov 19, 2013 at 11:33:06AM +0200, Hiroshi Doyu wrote:
> IOMMU devices on the bus need to be poplulated first, then iommu
> master devices are done later.
> 
> With CONFIG_OF_IOMMU, "iommus=" DT binding would be used to identify
> whether a device can be an iommu msater or not. If a device can, 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 master devices are populated and
> configured for IOMMU with help of the already populated iommu device
> via iommu_ops->add_device(). Multiple IOMMUs can be listed on this
> "iommus" binding so that a device can have multiple IOMMUs attached.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> ---
> v5:
> Use "iommus=" binding instread of arm,smmu's "#stream-id-cells".
> 
> v4:
> 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 | 22 ++++++++++++++++++++++
>  include/linux/of_iommu.h |  7 +++++++
>  3 files changed, 34 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..4aef2b2 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,23 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(of_get_dma_window);
> +
> +int of_iommu_attach(struct device *dev)
> +{
> +	int i;
> +	struct of_phandle_args args;
> +	struct iommu_ops *ops = dev->bus->iommu_ops;
> +
> +	of_property_for_each_phandle_with_args(dev->of_node, "iommus",
> +					       "#iommu-cells", i, &args) {
> +		pr_debug("%s(i=%d) ops=%p %s\n",
> +			 __func__, i, ops, dev_name(dev));
> +
> +		if (!ops)
> +			return -EPROBE_DEFER;
> +	}
> +
> +	if (i && ops->add_device)
> +		return ops->add_device(dev);
> +	return 0;
> +}

I don't think this does what it's supposed to do. As far as I can tell
there's no way the above loop won't run to parse all phandles and their
arguments unless the DT is actually wrong.

>From earlier discussions I thought the goal was to actually defer this
until all nodes referred to by the iommus property were actually
registered. The above only checks that the phandles can be resolved to
valid struct device_node:s. That doesn't mean that an actual IOMMU has
been registered for it, only that the devices have been created.

I think within that loop you need to look up the IOMMU corresponding to
the struct device_node in args.np. If no match is found, then return
-EPROBE_DEFER.

If you really only rely on dev->bus->iommu_ops to be present, then there
is no need to go through the loop in the first place, since you have
access to it immediately through the struct device that's passed into
the function.

Furthermore, relying on dev->bus->iommu_ops will prevent multiple IOMMUs
from being used at all, since only one IOMMU can register iommu_ops with
the bus, right? So I think what we really need here is a way to resolve
the IOMMU using a phandle and return the associated struct iommu_ops.

I also have some trouble understanding how the current IOMMU framework
is supposed to work together with multiple IOMMUs for one device. The
.add_device() callback seems to be missing crucial information to help
decide whether the device to be added is actually one that it covers.
Also with an of_iommu_attach() function, doesn't that become more or
less redundant?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131119/dac87db1/attachment.sig>

[PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

[Hiroshi Doyu]

Hi Thierry,

Thierry Reding <thierry.reding@gmail.com> wrote @ Tue, 19 Nov 2013 11:25:07 +0100:

> From earlier discussions I thought the goal was to actually defer this
> until all nodes referred to by the iommus property were actually
> registered. The above only checks that the phandles can be resolved to
> valid struct device_node:s. That doesn't mean that an actual IOMMU has
> been registered for it, only that the devices have been created.

Currently "bus->iommu_ops" is set at the end of tegra_smmu_probe(). So
if "bus->iommu_ops" is set, it means that an iommu instance is
populated at that time.

> If you really only rely on dev->bus->iommu_ops to be present, then there
> is no need to go through the loop in the first place, since you have
> access to it immediately through the struct device that's passed into
> the function.

As mentioned in the above, "bus->iommu_ops" is set when an iommu
instance is populated. "iommus=" is used only to defer a device probe
until an iommu instance shows up although multiple IOMMUs are not
supported yet correctly as you pointed out.

> Furthermore, relying on dev->bus->iommu_ops will prevent multiple IOMMUs
> from being used at all, since only one IOMMU can register iommu_ops with
> the bus, right? So I think what we really need here is a way to resolve
> the IOMMU using a phandle and return the associated struct
> iommu_ops.

Multiple IOMMU support doesn't work right now. It needs to be
discussed a bit more.

> I also have some trouble understanding how the current IOMMU framework
> is supposed to work together with multiple IOMMUs for one device. The
> .add_device() callback seems to be missing crucial information to help
> decide whether the device to be added is actually one that it covers.
> Also with an of_iommu_attach() function, doesn't that become more or
> less redundant?

I understand what you meant. In the current tegra SMMU implementation,
iommu_ops is set when an iommu is populated so that we cannot use
iommu_ops before that time. That's why of_iommu_attach() was
introduced here. I couldn't set iommu_ops at drvier init yet because
we support SMMU and GART with a single image and the current IOMMU
framework assume that there's one IOMMU on the bus. If we set
iommu_ops at driver init, the latter one would overwrite it.

[PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

[Stephen Warren]

On 11/19/2013 05:03 AM, Hiroshi Doyu wrote:
> Hi Thierry,
> 
> Thierry Reding <thierry.reding@gmail.com> wrote @ Tue, 19 Nov 2013 11:25:07 +0100:
> 
>> From earlier discussions I thought the goal was to actually defer this
>> until all nodes referred to by the iommus property were actually
>> registered. The above only checks that the phandles can be resolved to
>> valid struct device_node:s. That doesn't mean that an actual IOMMU has
>> been registered for it, only that the devices have been created.
> 
> Currently "bus->iommu_ops" is set at the end of tegra_smmu_probe(). So
> if "bus->iommu_ops" is set, it means that an iommu instance is
> populated at that time.

Yes, but that's the register bus, upon which the device is a client, not
the bus upon which the device is a bus master. They aren't necessarily
the same.

There's no getting around the fact that, as Thierry said, you need to
search for a registered IOMMU device for each phandle, and defer probe
if any aren't registered yet.

If we do that, then you shouldn't need to look at the value of
dev->bus->iommu_ops at all; if all IOMMUs in the list were registered,
then iommu_ops must have been set when (one of them) was registered, and
if not, then it possibly wasn't, so defer probe.

That way, this code won't have to change if the core IOMMU code gets
extended to support multiple IOMMUs, devices mastering transactions onto
buses other than their register bus, etc.

[PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

[Hiroshi Doyu]

Stephen Warren <swarren@wwwdotorg.org> wrote @ Tue, 19 Nov 2013 22:22:47 +0100:

> On 11/19/2013 05:03 AM, Hiroshi Doyu wrote:
> > Hi Thierry,
> > 
> > Thierry Reding <thierry.reding@gmail.com> wrote @ Tue, 19 Nov 2013 11:25:07 +0100:
> > 
> >> From earlier discussions I thought the goal was to actually defer this
> >> until all nodes referred to by the iommus property were actually
> >> registered. The above only checks that the phandles can be resolved to
> >> valid struct device_node:s. That doesn't mean that an actual IOMMU has
> >> been registered for it, only that the devices have been created.
> > 
> > Currently "bus->iommu_ops" is set at the end of tegra_smmu_probe(). So
> > if "bus->iommu_ops" is set, it means that an iommu instance is
> > populated at that time.
> 
> Yes, but that's the register bus, upon which the device is a client, not
> the bus upon which the device is a bus master. They aren't necessarily
> the same.
> 
> There's no getting around the fact that, as Thierry said, you need to
> search for a registered IOMMU device for each phandle, and defer probe
> if any aren't registered yet.
> 
> If we do that, then you shouldn't need to look at the value of
> dev->bus->iommu_ops at all; if all IOMMUs in the list were registered,
> then iommu_ops must have been set when (one of them) was registered, and
> if not, then it possibly wasn't, so defer probe.
> 
> That way, this code won't have to change if the core IOMMU code gets
> extended to support multiple IOMMUs, devices mastering transactions onto
> buses other than their register bus, etc.

Does the above mean the following?

int of_iommu_attach(struct device *dev)
{
	int i;
	struct of_phandle_args args;

	of_property_for_each_phandle_with_args(dev->of_node, "iommus",
					       "#iommu-cells", i, &args)
		if (!args->np->dev->driver)
			return -EPROBE_DEFER;
	return 0;
}


"args->np->dev->driver" needs the following patch:

  http://lists.linuxfoundation.org/pipermail/iommu/2013-July/006023.html

[PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

[Thierry Reding]

On Wed, Nov 20, 2013 at 04:17:08AM +0100, Hiroshi Doyu wrote:
> Stephen Warren <swarren@wwwdotorg.org> wrote @ Tue, 19 Nov 2013 22:22:47 +0100:
> 
> > On 11/19/2013 05:03 AM, Hiroshi Doyu wrote:
> > > Hi Thierry,
> > > 
> > > Thierry Reding <thierry.reding@gmail.com> wrote @ Tue, 19 Nov 2013 11:25:07 +0100:
> > > 
> > >> From earlier discussions I thought the goal was to actually defer this
> > >> until all nodes referred to by the iommus property were actually
> > >> registered. The above only checks that the phandles can be resolved to
> > >> valid struct device_node:s. That doesn't mean that an actual IOMMU has
> > >> been registered for it, only that the devices have been created.
> > > 
> > > Currently "bus->iommu_ops" is set at the end of tegra_smmu_probe(). So
> > > if "bus->iommu_ops" is set, it means that an iommu instance is
> > > populated at that time.
> > 
> > Yes, but that's the register bus, upon which the device is a client, not
> > the bus upon which the device is a bus master. They aren't necessarily
> > the same.
> > 
> > There's no getting around the fact that, as Thierry said, you need to
> > search for a registered IOMMU device for each phandle, and defer probe
> > if any aren't registered yet.
> > 
> > If we do that, then you shouldn't need to look at the value of
> > dev->bus->iommu_ops at all; if all IOMMUs in the list were registered,
> > then iommu_ops must have been set when (one of them) was registered, and
> > if not, then it possibly wasn't, so defer probe.
> > 
> > That way, this code won't have to change if the core IOMMU code gets
> > extended to support multiple IOMMUs, devices mastering transactions onto
> > buses other than their register bus, etc.
> 
> Does the above mean the following?
> 
> int of_iommu_attach(struct device *dev)
> {
> 	int i;
> 	struct of_phandle_args args;
> 
> 	of_property_for_each_phandle_with_args(dev->of_node, "iommus",
> 					       "#iommu-cells", i, &args)
> 		if (!args->np->dev->driver)
> 			return -EPROBE_DEFER;
> 	return 0;
> }

Not quite. The above would only check that a driver was bound to the
device. But if that device isn't an IOMMU then this doesn't help you.

The standard way to solve this issue is to add the IOMMU to a global
list upon registration. Typically subsystems have some way to do that
already, but it seems like IOMMU doesn't. It looks like that's one of
the side-effects of the assumption that there will always only be a
single IOMMU (per bus).

There's also no base object that IOMMU drivers implement, which is the
way it's usually done in other subsystems. The absence of that makes it
more difficult. I suspect the easiest way to do that would be to add a
new type, something like this:

	struct iommu {
		struct list_head list;
		struct device *dev;
	};

Then modify the driver to do something like this (tegra-smmu.c):

	struct smmu_device {
		struct iommu iommu;

		...
	};

	...

	static int tegra_smmu_probe(struct platform_device *pdev)
	{
		struct smmu_device *smmu;

		...

		smmu->iommu.dev = &pdev->dev;

		iommu_add(&smmu->iommu);

		...
	}

	static int tegra_smmu_remove(struct platform_device *pdev)
	{
		struct smmu_device *smmu;

		...

		iommu_del(&smmu->iommu);
	}

The implementation of iommu_add() and iommu_del() could be as simple as:

	static DEFINE_MUTEX(iommus_lock);
	static LIST_HEAD(iommus_list);

	void iommu_add(struct iommu *iommu)
	{
		INIT_LIST_HEAD(&iommu->list);
		mutex_lock(&iommus_lock);
		list_add_tail(&iommu->list, &iommus_list);
		mutex_unlock(&iommus_lock);
	}

	void iommu_del(struct iommu *iommu)
	{
		INIT_LIST_HEAD(&iommu->list);
		mutex_lock(&iommus_lock);
		list_del(&iommu->list);
		mutex_unlock(&iommus_lock);
	}

And then you can implement a lookup function to match an IOMMU to the
phandle, like so:

	struct iommu *of_find_iommu_by_node(struct device_node *np)
	{
		struct iommu *iommu;

		mutex_lock(&iommus_lock);

		list_for_each_entry(iommu, &iommus_list, list) {
			if (iommu->dev->of_node == np) {
				mutex_unlock(&iommus_lock);
				return iommu;
			}
		}

		mutex_unlock(&iommus_lock);
		return NULL;
	}

With that you can use of_find_iommu_by_node() in the loop to check
whether an IOMMU has really been registered.

Of course that all is somewhat intrusive and Joerg might have some
objections.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131120/d6017976/attachment.sig>

[PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

[Hiroshi Doyu]

Thierry Reding <thierry.reding@gmail.com> wrote @ Wed, 20 Nov 2013 14:14:48 +0100:
....
> > Does the above mean the following?
> > 
> > int of_iommu_attach(struct device *dev)
> > {
> > 	int i;
> > 	struct of_phandle_args args;
> > 
> > 	of_property_for_each_phandle_with_args(dev->of_node, "iommus",
> > 					       "#iommu-cells", i, &args)
> > 		if (!args->np->dev->driver)
> > 			return -EPROBE_DEFER;
> > 	return 0;
> > }
> 
> Not quite. The above would only check that a driver was bound to the
> device. But if that device isn't an IOMMU then this doesn't help you.

I thought that, as long as a device is a normal one, it's ok to let it
go to be populated. We only care about that, IOMMU devices comes
first, and clients should come later than IOMMUs, for population. In
the above if all IOMMUs are not populated, client devices are always
deferred. "args->np->dev" always points an IOMMU device in a
loop. Otherwise(no "iommus=") it goes out from the loop immediately.

+#define of_property_for_each_phandle_with_args(np, list, cells, i, args) \
+	 for (i = 0; !of_parse_phandle_with_args(np, list, cells, i, args); i++)

> The standard way to solve this issue is to add the IOMMU to a global
> list upon registration. Typically subsystems have some way to do that
> already,

Your implementation has some possibiity that we could construct any
hierarchy of IOMMUs.

> already, but it seems like IOMMU doesn't. It looks like that's one of
> the side-effects of the assumption that there will always only be a
> single IOMMU (per bus).

There's the following case at least we have already had.

"memory controller"---"smmu_a"---bus--+--"smmu_b"--"device_a"
                                      |
                                      |
                                      +--"device_b"

"smmu_b" isn't related to a bus at all.

> There's also no base object that IOMMU drivers implement, which is the
> way it's usually done in other subsystems. The absence of that makes it
> more difficult. I suspect the easiest way to do that would be to add a
> new type, something like this:
....

> With that you can use of_find_iommu_by_node() in the loop to check
> whether an IOMMU has really been registered.

Do you think if it's acceptable to see if a device is populated or not
via "dev->driver"[1]? Grant Likely proposed to use flag[2] instead of
struct device[2], though.

[1] http://lists.linuxfoundation.org/pipermail/iommu/2013-July/006023.html
[2] http://lists.linuxfoundation.org/pipermail/iommu/2013-October/006763.html

[PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

[Stephen Warren]

On 11/20/2013 07:03 AM, Hiroshi Doyu wrote:
> Thierry Reding <thierry.reding@gmail.com> wrote @ Wed, 20 Nov 2013 14:14:48 +0100:

(Yes, what Thierry said)

> ....
>>> Does the above mean the following?
>>>
>>> int of_iommu_attach(struct device *dev)
>>> {
>>> 	int i;
>>> 	struct of_phandle_args args;
>>>
>>> 	of_property_for_each_phandle_with_args(dev->of_node, "iommus",
>>> 					       "#iommu-cells", i, &args)
>>> 		if (!args->np->dev->driver)
>>> 			return -EPROBE_DEFER;
>>> 	return 0;
>>> }
>>
>> Not quite. The above would only check that a driver was bound to the
>> device. But if that device isn't an IOMMU then this doesn't help you.
> 
> I thought that, as long as a device is a normal one, it's ok to let it
> go to be populated.

I don't understand what that means.

> We only care about that, IOMMU devices comes
> first, and clients should come later than IOMMUs, for population. In
> the above if all IOMMUs are not populated, client devices are always
> deferred. "args->np->dev" always points an IOMMU device in a
> loop. Otherwise(no "iommus=") it goes out from the loop immediately.

I'm not sure what that means. Perhaps you're sauying the dev->driver
isn't set until the driver is probe()d for the device, so if
dev->driver!=NULL, then we know the driver probed() successfully for it?
That does go most of the way, but as Thierry pointed out, it doesn't
guarantee that the dev->driver is an IOMMU driver, just that it's *some*
driver. Perhaps this won't actually make any difference in practice, but
AFAIK, all other subsystems do perform the strict check, so I don't see
why the IOMMU subsystem shouldn't.

...
> There's the following case at least we have already had.
> 
> "memory controller"---"smmu_a"---bus--+--"smmu_b"--"device_a"
>                                       |
>                                       |
>                                       +--"device_b"
> 
> "smmu_b" isn't related to a bus at all.

Yes, smmu_b is related to a bus.

Admittedly perhaps not a bus that the CPU can master transactions onto,
but there's still a (perhaps point-point) bus that device_a masters, and
smmu_b acts as a slave.

Note that not all buses in the diagram above are represented as bus
objects in the Linux kernel (or even in DT), since those tend to
concentrate only on representing buses that the CPU masters, rather than
additionally representing buses that only HW devices master.

[PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

[Hiroshi Doyu]

On Wed, 20 Nov 2013 17:30:35 +0100
Stephen Warren <swarren@wwwdotorg.org> wrote:
...
> >>> Does the above mean the following?
> >>>
> >>> int of_iommu_attach(struct device *dev)
> >>> {
> >>> 	int i;
> >>> 	struct of_phandle_args args;
> >>>
> >>> 	of_property_for_each_phandle_with_args(dev->of_node, "iommus",
> >>> 					       "#iommu-cells", i, &args)
> >>> 		if (!args->np->dev->driver)
> >>> 			return -EPROBE_DEFER;
> >>> 	return 0;
> >>> }
> >>
> >> Not quite. The above would only check that a driver was bound to the
> >> device. But if that device isn't an IOMMU then this doesn't help you.
> > 
> > I thought that, as long as a device is a normal one, it's ok to let it
> > go to be populated.
> 
> I don't understand what that means.
> 
> > We only care about that, IOMMU devices comes
> > first, and clients should come later than IOMMUs, for population. In
> > the above if all IOMMUs are not populated, client devices are always
> > deferred. "args->np->dev" always points an IOMMU device in a
> > loop. Otherwise(no "iommus=") it goes out from the loop immediately.
> 
> I'm not sure what that means. Perhaps you're sauying the dev->driver
> isn't set until the driver is probe()d for the device, so if
> dev->driver!=NULL, then we know the driver probed() successfully for it?

Yes

> That does go most of the way, but as Thierry pointed out, it doesn't
> guarantee that the dev->driver is an IOMMU driver, just that it's *some*
> driver. Perhaps this won't actually make any difference in practice, but
> AFAIK, all other subsystems do perform the strict check, so I don't see
> why the IOMMU subsystem shouldn't.

Ok, now I got the one Thierry pointed out. Will implement that.

[PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

[Grant Likely]

On Tue, 19 Nov 2013 11:33:06 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> IOMMU devices on the bus need to be poplulated first, then iommu
> master devices are done later.
> 
> With CONFIG_OF_IOMMU, "iommus=" DT binding would be used to identify
> whether a device can be an iommu msater or not. If a device can, 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 master devices are populated and
> configured for IOMMU with help of the already populated iommu device
> via iommu_ops->add_device(). Multiple IOMMUs can be listed on this
> "iommus" binding so that a device can have multiple IOMMUs attached.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> ---
> v5:
> Use "iommus=" binding instread of arm,smmu's "#stream-id-cells".
> 
> v4:
> 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 | 22 ++++++++++++++++++++++
>  include/linux/of_iommu.h |  7 +++++++
>  3 files changed, 34 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)

I'm very concerned about this approach. Hooking into the core probe
path for things like this is not going to scale well. I'm not thrilled
with the pinctrl hook being here either, but that is already merged. :-(
Also, hooking in here is going affect every single device device driver
probe path, and a large number of them are never, ever, going to have
iommu interactions.

There needs to be a less invasive way of doing what you want. I still
feel like the individual device drivers themselves need to be aware that
they might be hooking through an IOMMU. Or, if they are hooking through
a bus like PCIe, then have the PCIe bus defer creating child devices
until the IOMMU is configured and in place.

g.

[PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

[Stephen Warren]

On 11/21/2013 06:15 AM, Grant Likely wrote:
> On Tue, 19 Nov 2013 11:33:06 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
>> IOMMU devices on the bus need to be poplulated first, then iommu
>> master devices are done later.
>>
>> With CONFIG_OF_IOMMU, "iommus=" DT binding would be used to identify
>> whether a device can be an iommu msater or not. If a device can, 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 master devices are populated and
>> configured for IOMMU with help of the already populated iommu device
>> via iommu_ops->add_device(). Multiple IOMMUs can be listed on this
>> "iommus" binding so that a device can have multiple IOMMUs attached.
>>
>> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
>> ---
>> v5:
>> Use "iommus=" binding instread of arm,smmu's "#stream-id-cells".
>>
>> v4:
>> 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 | 22 ++++++++++++++++++++++
>>  include/linux/of_iommu.h |  7 +++++++
>>  3 files changed, 34 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)
> 
> I'm very concerned about this approach. Hooking into the core probe
> path for things like this is not going to scale well. I'm not thrilled
> with the pinctrl hook being here either, but that is already merged. :-(
> Also, hooking in here is going affect every single device device driver
> probe path, and a large number of them are never, ever, going to have
> iommu interactions.
> 
> There needs to be a less invasive way of doing what you want. I still
> feel like the individual device drivers themselves need to be aware that
> they might be hooking through an IOMMU. Or, if they are hooking through
> a bus like PCIe, then have the PCIe bus defer creating child devices
> until the IOMMU is configured and in place.

I general though, couldn't any MMIO on-SoC device potentially be
affected by an IOMMU? The point of putting the call to of_iommu_attach()
here rather than inside individual driver's probe() is to avoid
requiring "every" driver having to paste more boiler-plate into probe().

Perhaps what we need is a flag in struct device to indicate that the
driver/device is MMIO, and hence potentially affected by an IOMMU. would
the following work better for you?

+	if (drv->is_mmio) { // let's bikeshed on the field name:-)
+		ret = of_iommu_attach(dev);
+		if (ret)
+			goto probe_failed;
+	}

I've often thought that struct device_driver (or similar) should declare
the set of resources that a device needs (e.g. a list of GPIO names,
regulator names, etc.), so that the driver core can parse all these
standard properties from DT/... before calling the custom probe()
function for all the non-standard stuff. This "is_mmio" flag would fit
into that model well.

[PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

[Grant Likely]

On Thu, 21 Nov 2013 12:04:18 -0700, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 11/21/2013 06:15 AM, Grant Likely wrote:
> > On Tue, 19 Nov 2013 11:33:06 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> >> IOMMU devices on the bus need to be poplulated first, then iommu
> >> master devices are done later.
> >>
> >> With CONFIG_OF_IOMMU, "iommus=" DT binding would be used to identify
> >> whether a device can be an iommu msater or not. If a device can, 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 master devices are populated and
> >> configured for IOMMU with help of the already populated iommu device
> >> via iommu_ops->add_device(). Multiple IOMMUs can be listed on this
> >> "iommus" binding so that a device can have multiple IOMMUs attached.
> >>
> >> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> >> ---
> >> v5:
> >> Use "iommus=" binding instread of arm,smmu's "#stream-id-cells".
> >>
> >> v4:
> >> 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 | 22 ++++++++++++++++++++++
> >>  include/linux/of_iommu.h |  7 +++++++
> >>  3 files changed, 34 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)
> > 
> > I'm very concerned about this approach. Hooking into the core probe
> > path for things like this is not going to scale well. I'm not thrilled
> > with the pinctrl hook being here either, but that is already merged. :-(
> > Also, hooking in here is going affect every single device device driver
> > probe path, and a large number of them are never, ever, going to have
> > iommu interactions.
> > 
> > There needs to be a less invasive way of doing what you want. I still
> > feel like the individual device drivers themselves need to be aware that
> > they might be hooking through an IOMMU. Or, if they are hooking through
> > a bus like PCIe, then have the PCIe bus defer creating child devices
> > until the IOMMU is configured and in place.
> 
> I general though, couldn't any MMIO on-SoC device potentially be
> affected by an IOMMU? The point of putting the call to of_iommu_attach()
> here rather than inside individual driver's probe() is to avoid
> requiring "every" driver having to paste more boiler-plate into probe().

It seems more that IOMMU attachment is closer to being a property of the
bus rather than a property of the device itself. In that context it
would make more sense for the bus device to hold off child device
registration or probing until the IOMMU is available. That keeps the
logic out of both the core code and the individual device drivers.

g.

[PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

[Stephen Warren]

On 11/22/2013 12:41 AM, Grant Likely wrote:
> On Thu, 21 Nov 2013 12:04:18 -0700, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 11/21/2013 06:15 AM, Grant Likely wrote:
>>> On Tue, 19 Nov 2013 11:33:06 +0200, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
>>>> IOMMU devices on the bus need to be poplulated first, then iommu
>>>> master devices are done later.
>>>>
>>>> With CONFIG_OF_IOMMU, "iommus=" DT binding would be used to identify
>>>> whether a device can be an iommu msater or not. If a device can, 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 master devices are populated and
>>>> configured for IOMMU with help of the already populated iommu device
>>>> via iommu_ops->add_device(). Multiple IOMMUs can be listed on this
>>>> "iommus" binding so that a device can have multiple IOMMUs attached.
>>>>
>>>> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
>>>> ---
>>>> v5:
>>>> Use "iommus=" binding instread of arm,smmu's "#stream-id-cells".
>>>>
>>>> v4:
>>>> 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 | 22 ++++++++++++++++++++++
>>>>  include/linux/of_iommu.h |  7 +++++++
>>>>  3 files changed, 34 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)
>>>
>>> I'm very concerned about this approach. Hooking into the core probe
>>> path for things like this is not going to scale well. I'm not thrilled
>>> with the pinctrl hook being here either, but that is already merged. :-(
>>> Also, hooking in here is going affect every single device device driver
>>> probe path, and a large number of them are never, ever, going to have
>>> iommu interactions.
>>>
>>> There needs to be a less invasive way of doing what you want. I still
>>> feel like the individual device drivers themselves need to be aware that
>>> they might be hooking through an IOMMU. Or, if they are hooking through
>>> a bus like PCIe, then have the PCIe bus defer creating child devices
>>> until the IOMMU is configured and in place.
>>
>> I general though, couldn't any MMIO on-SoC device potentially be
>> affected by an IOMMU? The point of putting the call to of_iommu_attach()
>> here rather than inside individual driver's probe() is to avoid
>> requiring "every" driver having to paste more boiler-plate into probe().
> 
> It seems more that IOMMU attachment is closer to being a property of the
> bus rather than a property of the device itself. In that context it
> would make more sense for the bus device to hold off child device
> registration or probing until the IOMMU is available. That keeps the
> logic out of both the core code and the individual device drivers.

The bus structure that DT and Linux know about is the register bus.
There's no reason that devices have to emit their master transactions
onto that same bus, or onto only that same bus.

[PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

[Will Deacon]

On Fri, Nov 22, 2013 at 05:35:58PM +0000, Stephen Warren wrote:
> On 11/22/2013 12:41 AM, Grant Likely wrote:
> > It seems more that IOMMU attachment is closer to being a property of the
> > bus rather than a property of the device itself. In that context it
> > would make more sense for the bus device to hold off child device
> > registration or probing until the IOMMU is available. That keeps the
> > logic out of both the core code and the individual device drivers.
> 
> The bus structure that DT and Linux know about is the register bus.
> There's no reason that devices have to emit their master transactions
> onto that same bus, or onto only that same bus.

Agreed. Dave (CC'd) and I actually had a lot of discussion around the DT bus
abstractions last week and we ended up with a binding that looked sane enough
to start a meaningful discussion in this area.

Dave -- care to post what we came up with? It certainly has a bunch of
overlap with the IOMMU problems being discussed here.

Will

[PATCHv5 3/9] 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@nvidia.com>
---
v5:
Added new macro TEGRA_SWGROUP_CELLS() and WO_U32_OF_U64().

v4:
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 | 50 ++++++++++++++++++++++++++++++
 1 file changed, 50 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..73079ad
--- /dev/null
+++ b/include/dt-bindings/memory/tegra-swgroup.h
@@ -0,0 +1,50 @@
+/*
+ * 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 TWO_U32_OF_U64(x)	((x) & ~0UL) ((x) >> 32)
+#define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)
+#define TEGRA_SWGROUP_CELLS(x)	TWO_U32_OF_U64(TEGRA_SWGROUP_BIT(x))
+
+#define TEGRA_SWGROUP_MAX	64
+
+#endif /* _DT_BINDINGS_MEMORY_TEGRA_SWGROUP_H */
-- 
1.8.1.5

[PATCHv5 3/9] ARM: tegra: create a DT header defining SWGROUP ID

[Stephen Warren]

On 11/19/2013 02:33 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 TWO_U32_OF_U64(x)	((x) & ~0UL) ((x) >> 32)
> +#define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)
> +#define TEGRA_SWGROUP_CELLS(x)	TWO_U32_OF_U64(TEGRA_SWGROUP_BIT(x))

This still doesn't actually compile in dtc:

$ cat > tmp.dts <<ENDOFHERE
/dts-v1/;

#define TEGRA_SWGROUP_PPCS2	32	/* 0xab0 */

#define TWO_U32_OF_U64(x)	((x) & ~0UL) ((x) >> 32)
#define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)
#define TEGRA_SWGROUP_CELLS(x)	TWO_U32_OF_U64(TEGRA_SWGROUP_BIT(x))

/ {
    prop = <TEGRA_SWGROUP_CELLS(PPCS2)>;
};
ENDOFHERE

$ gcc -nostdinc -undef -D__DTS__ -E -x assembler-with-cpp -o tmp.dts.i \
	tmp.dts

$ ./scripts/dtc/dtc -O dts -o tmp-compiled.dts -I dts tmp.dts.i
Error: tmp.dts:10.35-36 integer value out of range 0000000000000020 \
	(32 bits)
FATAL ERROR: Syntax error parsing input tree

The reason is that "& ~0UL" expands to "& 0xffffffffffffffff" since dtc
doesn't know about the size difference between UL and ULL. You need to
change that to "& 0xffffffff" and it works, at least in dtc.

Please test!

[PATCHv5 4/9] iommu/tegra: smmu: register device to iommu 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@nvidia.com>
---
v5:
Add check NUM_OF_STATIC_MAPS < #asids.

v4:
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 | 70 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 70 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..d836a6b 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -39,6 +39,9 @@
 
 #include <asm/page.h>
 #include <asm/cacheflush.h>
+#include <asm/dma-iommu.h>
+
+#include <dt-bindings/memory/tegra-swgroup.h>
 
 enum smmu_hwgrp {
 	HWGRP_AFI,
@@ -319,6 +322,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 +952,44 @@ 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,
+	NUM_OF_STATIC_MAPS,
+};
+
+static int smmu_iommu_add_device(struct device *dev)
+{
+	int err = -EPROBE_DEFER;
+	u32 swgroups = dev->platform_data;
+	struct dma_iommu_mapping *map = NULL;
+
+	if (test_bit(TEGRA_SWGROUP_PPCS, swgroups))
+		map = smmu_handle->map[SYSTEM_PROTECTED];
+	else
+		map = smmu_handle->map[SYSTEM_DEFAULT];
+
+	if (map)
+		err = arm_iommu_attach_device(dev, map);
+	else
+		return -EPROBE_DEFER;
+
+	pr_debug("swgroups=%08lx 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 +999,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 +1189,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 +1222,18 @@ 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);
+	if (asids < NUM_OF_STATIC_MAPS)
+		return -EINVAL;
+
+	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 +1303,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

[PATCHv5 4/9] iommu/tegra: smmu: register device to iommu dynamically

[Stephen Warren]

On 11/19/2013 02:33 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;
> +	u32 swgroups = dev->platform_data;
> +	struct dma_iommu_mapping *map = NULL;
> +
> +	if (test_bit(TEGRA_SWGROUP_PPCS, swgroups))
> +		map = smmu_handle->map[SYSTEM_PROTECTED];
> +	else
> +		map = smmu_handle->map[SYSTEM_DEFAULT];
> +
> +	if (map)
> +		err = arm_iommu_attach_device(dev, map);
> +	else
> +		return -EPROBE_DEFER;

Given that patch 2 exists, if this test fails, then surely the
appropriate error code is some fatal error, not -EPROBE_DEFER; any
deferrals should have happened long before this point.

[PATCHv5 4/9] iommu/tegra: smmu: register device to iommu dynamically

[Grant Likely]

On Tue, 19 Nov 2013 14:39:54 -0700, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 11/19/2013 02:33 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;
> > +	u32 swgroups = dev->platform_data;
> > +	struct dma_iommu_mapping *map = NULL;
> > +
> > +	if (test_bit(TEGRA_SWGROUP_PPCS, swgroups))
> > +		map = smmu_handle->map[SYSTEM_PROTECTED];
> > +	else
> > +		map = smmu_handle->map[SYSTEM_DEFAULT];
> > +
> > +	if (map)
> > +		err = arm_iommu_attach_device(dev, map);
> > +	else
> > +		return -EPROBE_DEFER;
> 
> Given that patch 2 exists, if this test fails, then surely the
> appropriate error code is some fatal error, not -EPROBE_DEFER; any
> deferrals should have happened long before this point.

Will need to be revisited. Patching into the core really_probe() like
patch #2 does is a really bad approach which would mean that returning
EPROBE_DEFER is appropriate here.

g.

[PATCHv5 4/9] iommu/tegra: smmu: register device to iommu dynamically

[Hiroshi Doyu]

On Thu, 21 Nov 2013 14:23:22 +0100
Grant Likely <grant.likely@linaro.org> wrote:

> > > +static int smmu_iommu_add_device(struct device *dev)
> > > +{
> > > +	int err = -EPROBE_DEFER;
> > > +	u32 swgroups = dev->platform_data;
> > > +	struct dma_iommu_mapping *map = NULL;
> > > +
> > > +	if (test_bit(TEGRA_SWGROUP_PPCS, swgroups))
> > > +		map = smmu_handle->map[SYSTEM_PROTECTED];
> > > +	else
> > > +		map = smmu_handle->map[SYSTEM_DEFAULT];
> > > +
> > > +	if (map)
> > > +		err = arm_iommu_attach_device(dev, map);
> > > +	else
> > > +		return -EPROBE_DEFER;
> > 
> > Given that patch 2 exists, if this test fails, then surely the
> > appropriate error code is some fatal error, not -EPROBE_DEFER; any
> > deferrals should have happened long before this point.
> 
> Will need to be revisited. Patching into the core really_probe() like
> patch #2 does is a really bad approach which would mean that returning
> EPROBE_DEFER is appropriate here.

I'm going to send PATCHv6, where it's a little bit easier to discuss
further since some code are cleaned up with feedback.

We once discussed to make use of bus_notifier instead of embedding a
hook in really_probe(). This would require some change where
bus_notifer return code needs to be used to postpone device
population.

[PATCHv5 5/9] 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@nvidia.com>
---
v5:
Added SMMU_ASID_BASE(== SMMU_AFI_ASID).
Removed unused ASID offset definitions.
Use 'unsigned long *' instead of u64 for swgroups bitmap.

v4:
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 | 119 +++++++++------------------------------------
 1 file changed, 23 insertions(+), 96 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index d836a6b..e999ad0 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -43,46 +43,6 @@
 
 #include <dt-bindings/memory/tegra-swgroup.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)
-
 /* bitmap of the page sizes currently supported */
 #define SMMU_IOMMU_PGSIZES	(SZ_4K)
 
@@ -152,21 +112,7 @@ enum {
 #define SMMU_TRANSLATION_ENABLE_2		0x230
 
 #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 SMMU_ASID_BASE	SMMU_AFI_ASID
 
 #define SMMU_PDE_NEXT_SHIFT		28
 
@@ -238,27 +184,7 @@ enum {
 #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 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_ASID_BASE)
 
 /*
  * Per client for address space
@@ -267,7 +193,7 @@ struct smmu_client {
 	struct device		*dev;
 	struct list_head	list;
 	struct smmu_as		*as;
-	u32			hwgrp;
+	unsigned long		hwgrp[2];
 };
 
 /*
@@ -384,41 +310,37 @@ 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)
+				   unsigned long *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;
 
-	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, map, TEGRA_SWGROUP_MAX) {
 		offs = HWGRP_ASID_REG(i);
 		val = smmu_read(smmu, offs);
 		if (on) {
 			if (WARN_ON(val & mask))
 				goto err_hw_busy;
 			val |= mask;
+			memcpy(c->hwgrp, map, sizeof(u64));
 		} else {
 			WARN_ON((val & mask) == mask);
 			val &= ~mask;
 		}
 		smmu_write(smmu, val, offs);
 	}
+
 	FLUSH_SMMU_REGS(smmu);
-	c->hwgrp = map;
 	return 0;
 
 err_hw_busy:
-	for_each_set_bit(i, &map, HWGRP_COUNT) {
+	for_each_set_bit(i, map, TEGRA_SWGROUP_MAX) {
 		offs = HWGRP_ASID_REG(i);
 		val = smmu_read(smmu, offs);
 		val &= ~mask;
@@ -427,17 +349,18 @@ err_hw_busy:
 	return -EBUSY;
 }
 
-static int smmu_client_set_hwgrp(struct smmu_client *c, u32 map, int on)
+static int smmu_client_set_hwgrp(struct smmu_client *c,
+				 unsigned long *map, int on)
 {
-	u32 val;
+	int err;
 	unsigned long flags;
 	struct smmu_as *as = c->as;
 	struct smmu_device *smmu = as->smmu;
 
 	spin_lock_irqsave(&smmu->lock, flags);
-	val = __smmu_client_set_hwgrp(c, map, on);
+	err = __smmu_client_set_hwgrp(c, map, on);
 	spin_unlock_irqrestore(&smmu->lock, flags);
-	return val;
+	return err;
 }
 
 /*
@@ -796,7 +719,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 	struct smmu_as *as = domain->priv;
 	struct smmu_device *smmu = as->smmu;
 	struct smmu_client *client, *c;
-	u32 map;
+	unsigned long *map;
 	int err;
 
 	client = devm_kzalloc(smmu->dev, sizeof(*c), GFP_KERNEL);
@@ -804,7 +727,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 		return -ENOMEM;
 	client->dev = dev;
 	client->as = as;
-	map = (unsigned long)dev->platform_data;
+	map = (unsigned long *)dev->platform_data;
 	if (!map)
 		return -EINVAL;
 
@@ -828,7 +751,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 (test_bit(TEGRA_SWGROUP_AVPC, map)) {
 		struct page *page;
 
 		page = as->smmu->avp_vector_page;
@@ -965,10 +888,14 @@ enum {
 
 static int smmu_iommu_add_device(struct device *dev)
 {
-	int err = -EPROBE_DEFER;
-	u32 swgroups = dev->platform_data;
+	int err;
+	unsigned long swgroups[2];
 	struct dma_iommu_mapping *map = NULL;
 
+	err = smmu_of_get_swgroups(dev, swgroups);
+	if (err)
+		return -ENODEV;
+
 	if (test_bit(TEGRA_SWGROUP_PPCS, swgroups))
 		map = smmu_handle->map[SYSTEM_PROTECTED];
 	else
-- 
1.8.1.5

[PATCHv5 6/9] iommu/tegra: smmu: get swgroups from DT "iommus="

[Hiroshi Doyu]

This provides the info about which swgroups a device belongs to. This
info is passed from DT. This is necessary for the unified SMMU driver
among Tegra SoCs since each has different H/W accelerators.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
v5:
"iommu=" in a device DT is used instead of "mmu-masters" in an iommu
DT. This is "iommu=" version of:

  [PATCHv4 5/7] iommu/tegra: smmu: Support "mmu-masters" binding
---
 .../bindings/iommu/nvidia,tegra30-smmu.txt         |  17 ++-
 drivers/iommu/tegra-smmu.c                         | 125 ++++++++++++++++++---
 2 files changed, 126 insertions(+), 16 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
index 89fb543..44a4dc3 100644
--- a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
@@ -8,9 +8,12 @@ Required properties:
 - nvidia,#asids : # of ASIDs
 - dma-window : IOVA start address and length.
 - nvidia,ahb : phandle to the ahb bus connected to SMMU.
+- iommus: phandle to an iommu device which a device is
+  attached to and indicates which swgroups a device belongs to(SWGROUP ID).
+  SWGROUP ID is from 0 to 63, and a device can belong to multiple SWGROUPS.
 
 Example:
-	smmu {
+	smmu: iommu {
 		compatible = "nvidia,tegra30-smmu";
 		reg = <0x7000f010 0x02c
 		       0x7000f1f0 0x010
@@ -18,4 +21,16 @@ Example:
 		nvidia,#asids = <4>;		/* # of ASIDs */
 		dma-window = <0 0x40000000>;	/* IOVA start & length */
 		nvidia,ahb = <&ahb>;
+		#iommu-cells = <2>;
 	};
+
+	host1x {
+		compatible = "nvidia,tegra30-host1x", "simple-bus";
+		iommus = <&smmu TEGRA_SWGROUP_CELLS(HC)>;
+		....
+		gr3d {
+			compatible = "nvidia,tegra30-gr3d";
+			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_CELLS(NV)
+						       TEGRA_SWGROUP_CELLS(NV2)>;
+			....
+		};
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index e999ad0..e915201a 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -190,6 +190,8 @@ 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;
@@ -233,6 +235,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 */
 
 	/*
@@ -310,6 +313,96 @@ 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 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, unsigned long *swgroups)
+{
+	struct smmu_client *client;
+
+	client = find_smmu_client(smmu, dev->of_node);
+	if (client) {
+		dev_err(dev,
+			"rejecting multiple registrations for client device %s\n",
+			dev->of_node->full_name);
+		return -EBUSY;
+	}
+
+	client = devm_kzalloc(smmu->dev, sizeof(*client), GFP_KERNEL);
+	if (!client)
+		return -ENOMEM;
+
+	client->dev = dev;
+	client->of_node = dev->of_node;
+	memcpy(client->hwgrp, swgroups, sizeof(u64));
+	return insert_smmu_client(smmu, client);
+}
+
+static int smmu_of_get_swgroups(struct device *dev, unsigned long *swgroups)
+{
+	int i;
+	struct of_phandle_args args;
+
+	of_property_for_each_phandle_with_args(dev->of_node, "iommus",
+					       "#iommu-cells", i, &args) {
+		if (args.np != smmu_handle->dev->of_node)
+			continue;
+
+		BUG_ON(args.args_count != 2);
+
+		memcpy(swgroups, args.args, sizeof(u64));
+		pr_debug("swgroups=%08lx %08lx ops=%p %s\n",
+			 swgroups[0], swgroups[1],
+			 dev->bus->iommu_ops, dev_name(dev));
+		return 0;
+	}
+
+	return -ENODEV;
+}
+
 static int __smmu_client_set_hwgrp(struct smmu_client *c,
 				   unsigned long *map, int on)
 {
@@ -719,21 +812,16 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 	struct smmu_as *as = domain->priv;
 	struct smmu_device *smmu = as->smmu;
 	struct smmu_client *client, *c;
-	unsigned long *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->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) {
@@ -751,7 +839,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 	 * Reserve "page zero" for AVP vectors using a common dummy
 	 * page.
 	 */
-	if (test_bit(TEGRA_SWGROUP_AVPC, map)) {
+	if (test_bit(TEGRA_SWGROUP_AVPC, client->hwgrp)) {
 		struct page *page;
 
 		page = as->smmu->avp_vector_page;
@@ -766,8 +854,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;
 }
 
@@ -784,7 +870,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));
@@ -896,6 +981,15 @@ static int smmu_iommu_add_device(struct device *dev)
 	if (err)
 		return -ENODEV;
 
+	if (!find_smmu_client(smmu_handle, dev->of_node)) {
+		err = register_smmu_client(smmu_handle, dev, swgroups);
+		if (err) {
+			dev_err(dev, "failed to add client %s\n",
+				dev_name(dev));
+			return -EINVAL;
+		}
+	}
+
 	if (test_bit(TEGRA_SWGROUP_PPCS, swgroups))
 		map = smmu_handle->map[SYSTEM_PROTECTED];
 	else
@@ -906,8 +1000,8 @@ static int smmu_iommu_add_device(struct device *dev)
 	else
 		return -EPROBE_DEFER;
 
-	pr_debug("swgroups=%08lx map=%p err=%d %s\n",
-		 swgroups, map, err, dev_name(dev));
+	pr_debug("swgroups=%08lx %08lx map=%p err=%d %s\n",
+		 swgroups[0], swgroups[1], map, err, dev_name(dev));
 	return err;
 }
 
@@ -1160,6 +1254,7 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	smmu->clients = RB_ROOT;
 	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),
-- 
1.8.1.5

[PATCHv5 6/9] iommu/tegra: smmu: get swgroups from DT "iommus="

[Stephen Warren]

On 11/19/2013 02:33 AM, Hiroshi Doyu wrote:
> This provides the info about which swgroups a device belongs to. This
> info is passed from DT. This is necessary for the unified SMMU driver
> among Tegra SoCs since each has different H/W accelerators.

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

>  - nvidia,#asids : # of ASIDs
>  - dma-window : IOVA start address and length.
>  - nvidia,ahb : phandle to the ahb bus connected to SMMU.
> +- iommus: phandle to an iommu device which a device is
> +  attached to and indicates which swgroups a device belongs to(SWGROUP ID).
> +  SWGROUP ID is from 0 to 63, and a device can belong to multiple SWGROUPS.

I'm sure I've said this before:

#iommu-cells isn't documented.

The list of properties you added to is a list of properties that the
IOMMU node should contain. However, the iommus property is something
that *client* nodes should contain, not the IOMMU node itself.

Instead, I think you want something like:

-Required properties:
+Required properties in the IOMMU node:
 ... the current list of properties
+#iommu-cells. Should be 2. In client IOMMU specifiers, the two cells
+  represent a 64-bit bitmask of SWGROUP IDs under which the device
+  initiates transactions. The least significant word is first. See
+  <dt-bindings/memory/tegra-swgroup.h> for a list of valid values.
+
+Required properties in device nodes affected by the IOMMU:
+- iommus: A list of phandle plus specifier pairs for each IOMMU that
+    affects master transactions initiated by the device. The number of
+    cells in each specifier is defined by the #iommu-cells property in
+    the IOMMU node referred to by the phandle. The meaning of the
+    specifier cells is defined by the referenced IOMMU's binding.

Aside from those layout/wording issues, this is certainly the binding
that I think makes sense.

> +	host1x {
> +		compatible = "nvidia,tegra30-host1x", "simple-bus";
> +		iommus = <&smmu TEGRA_SWGROUP_CELLS(HC)>;
> +		....
> +		gr3d {
> +			compatible = "nvidia,tegra30-gr3d";
> +			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_CELLS(NV)
> +						       TEGRA_SWGROUP_CELLS(NV2)>;

I think you forgot to update "nvidia,memory-clients" to "iommus" there.

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

> +static int smmu_of_get_swgroups(struct device *dev, unsigned long *swgroups)

This doesn't seem to be used by anything in this patch.

[PATCHv5 7/9] 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@nvidia.com>
---
v4:
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 | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index e915201a..c2ed075 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -418,9 +418,13 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,
 		offs = HWGRP_ASID_REG(i);
 		val = smmu_read(smmu, offs);
 		if (on) {
-			if (WARN_ON(val & mask))
-				goto err_hw_busy;
-			val |= mask;
+			if (val) {
+				if (WARN_ON(val != mask))
+					return -EINVAL;
+				goto skip;
+			}
+
+			val = mask;
 			memcpy(c->hwgrp, map, sizeof(u64));
 		} else {
 			WARN_ON((val & mask) == mask);
@@ -430,16 +434,8 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,
 	}
 
 	FLUSH_SMMU_REGS(smmu);
+skip:
 	return 0;
-
-err_hw_busy:
-	for_each_set_bit(i, map, TEGRA_SWGROUP_MAX) {
-		offs = HWGRP_ASID_REG(i);
-		val = smmu_read(smmu, offs);
-		val &= ~mask;
-		smmu_write(smmu, val, offs);
-	}
-	return -EBUSY;
 }
 
 static int smmu_client_set_hwgrp(struct smmu_client *c,
-- 
1.8.1.5

[PATCHv5 8/9] 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@nvidia.com>
---
v4:
New for v4
---
 drivers/iommu/tegra-smmu.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index c2ed075..003a491 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -179,12 +179,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_ASID_REG(x) ((x) * sizeof(u32) + SMMU_ASID_BASE)
+#define SWGROUPS_ASID_REG(x) ((x) * sizeof(u32) + SMMU_ASID_BASE)
 
 /*
  * Per client for address space
@@ -195,7 +195,7 @@ struct smmu_client {
 	struct device		*dev;
 	struct list_head	list;
 	struct smmu_as		*as;
-	unsigned long		hwgrp[2];
+	unsigned long		swgroups[2];
 };
 
 /*
@@ -377,7 +377,7 @@ static int register_smmu_client(struct smmu_device *smmu,
 
 	client->dev = dev;
 	client->of_node = dev->of_node;
-	memcpy(client->hwgrp, swgroups, sizeof(u64));
+	memcpy(client->swgroups, swgroups, sizeof(u64));
 	return insert_smmu_client(smmu, client);
 }
 
@@ -403,7 +403,7 @@ static int smmu_of_get_swgroups(struct device *dev, unsigned long *swgroups)
 	return -ENODEV;
 }
 
-static int __smmu_client_set_hwgrp(struct smmu_client *c,
+static int __smmu_client_set_swgroups(struct smmu_client *c,
 				   unsigned long *map, int on)
 {
 	int i;
@@ -412,10 +412,10 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,
 	struct smmu_device *smmu = as->smmu;
 
 	if (!on)
-		map = c->hwgrp;
+		map = c->swgroups;
 
 	for_each_set_bit(i, map, TEGRA_SWGROUP_MAX) {
-		offs = HWGRP_ASID_REG(i);
+		offs = SWGROUPS_ASID_REG(i);
 		val = smmu_read(smmu, offs);
 		if (on) {
 			if (val) {
@@ -425,7 +425,7 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,
 			}
 
 			val = mask;
-			memcpy(c->hwgrp, map, sizeof(u64));
+			memcpy(c->swgroups, map, sizeof(u64));
 		} else {
 			WARN_ON((val & mask) == mask);
 			val &= ~mask;
@@ -438,7 +438,7 @@ skip:
 	return 0;
 }
 
-static int smmu_client_set_hwgrp(struct smmu_client *c,
+static int smmu_client_set_swgroups(struct smmu_client *c,
 				 unsigned long *map, int on)
 {
 	int err;
@@ -447,7 +447,7 @@ static int smmu_client_set_hwgrp(struct smmu_client *c,
 	struct smmu_device *smmu = as->smmu;
 
 	spin_lock_irqsave(&smmu->lock, flags);
-	err = __smmu_client_set_hwgrp(c, map, on);
+	err = __smmu_client_set_swgroups(c, map, on);
 	spin_unlock_irqrestore(&smmu->lock, flags);
 	return err;
 }
@@ -487,7 +487,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);
@@ -815,7 +815,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 		return -ENOMEM;
 
 	client->as = as;
-	err = smmu_client_enable_hwgrp(client, client->hwgrp);
+	err = smmu_client_enable_swgroups(client, client->swgroups);
 	if (err)
 		return -EINVAL;
 
@@ -835,7 +835,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 	 * Reserve "page zero" for AVP vectors using a common dummy
 	 * page.
 	 */
-	if (test_bit(TEGRA_SWGROUP_AVPC, client->hwgrp)) {
+	if (test_bit(TEGRA_SWGROUP_AVPC, client->swgroups)) {
 		struct page *page;
 
 		page = as->smmu->avp_vector_page;
@@ -848,7 +848,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;
 }
@@ -864,7 +864,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,
-- 
1.8.1.5

[PATCHv5 9/9] [FOR TEST] ARM: dt: tegra30: add "iommus" binding

[Hiroshi Doyu]

"iommus" binding implies that a device can be attached to IOMMU
devices. An iommu device needs to set #iommus-cells in it. "iommus"
can have multiple iommu device phandles as below if needed.

  iommus = <&smmu arg1 arg2>,
  	   <&gart arg1 arg2>;

Not yet ready for merge. Need to add iommus for other devices.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
v5:
Use "iommus=" instead of "mmu-masters".
---
 arch/arm/boot/dts/tegra30.dtsi | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 2bd55cf..03b7887 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -1,6 +1,7 @@
 #include <dt-bindings/clock/tegra30-car.h>
 #include <dt-bindings/gpio/tegra-gpio.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/memory/tegra-swgroup.h>
 
 #include "skeleton.dtsi"
 
@@ -92,6 +93,7 @@
 		interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, /* syncpt */
 			     <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>; /* general */
 		clocks = <&tegra_car TEGRA30_CLK_HOST1X>;
+		iommus = <&smmu TEGRA_SWGROUP_CELLS(HC)>;
 
 		#address-cells = <1>;
 		#size-cells = <1>;
@@ -103,6 +105,7 @@
 			reg = <0x54040000 0x00040000>;
 			interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_MPE>;
+			iommus = <&smmu TEGRA_SWGROUP_CELLS(MPE)>;
 		};
 
 		vi {
@@ -110,6 +113,7 @@
 			reg = <0x54080000 0x00040000>;
 			interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_VI>;
+			iommus = <&smmu TEGRA_SWGROUP_CELLS(VI)>;
 		};
 
 		epp {
@@ -117,6 +121,7 @@
 			reg = <0x540c0000 0x00040000>;
 			interrupts = <GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_EPP>;
+			iommus = <&smmu TEGRA_SWGROUP_CELLS(EPP)>;
 		};
 
 		isp {
@@ -124,6 +129,7 @@
 			reg = <0x54100000 0x00040000>;
 			interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_ISP>;
+			iommus = <&smmu TEGRA_SWGROUP_CELLS(ISP)>;
 		};
 
 		gr2d {
@@ -131,6 +137,7 @@
 			reg = <0x54140000 0x00040000>;
 			interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_GR2D>;
+			iommus = <&smmu TEGRA_SWGROUP_CELLS(G2)>;
 		};
 
 		gr3d {
@@ -139,6 +146,8 @@
 			clocks = <&tegra_car TEGRA30_CLK_GR3D
 				  &tegra_car TEGRA30_CLK_GR3D2>;
 			clock-names = "3d", "3d2";
+			iommus = <&smmu TEGRA_SWGROUP_CELLS(NV)
+						TEGRA_SWGROUP_CELLS(NV2)>;
 		};
 
 		dc at 54200000 {
@@ -148,6 +157,7 @@
 			clocks = <&tegra_car TEGRA30_CLK_DISP1>,
 				 <&tegra_car TEGRA30_CLK_PLL_P>;
 			clock-names = "disp1", "parent";
+			iommus = <&smmu TEGRA_SWGROUP_CELLS(DC)>;
 
 			rgb {
 				status = "disabled";
@@ -161,6 +171,7 @@
 			clocks = <&tegra_car TEGRA30_CLK_DISP2>,
 				 <&tegra_car TEGRA30_CLK_PLL_P>;
 			clock-names = "disp2", "parent";
+			iommus = <&smmu TEGRA_SWGROUP_CELLS(DCB)>;
 
 			rgb {
 				status = "disabled";
@@ -317,6 +328,7 @@
 		interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
 		nvidia,dma-request-selector = <&apbdma 8>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTA>;
+		iommus = <&smmu TEGRA_SWGROUP_CELLS(PPCS)>;
 		status = "disabled";
 	};
 
@@ -327,6 +339,7 @@
 		interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
 		nvidia,dma-request-selector = <&apbdma 9>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTB>;
+		iommus = <&smmu TEGRA_SWGROUP_CELLS(PPCS)>;
 		status = "disabled";
 	};
 
@@ -337,6 +350,7 @@
 		interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
 		nvidia,dma-request-selector = <&apbdma 10>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTC>;
+		iommus = <&smmu TEGRA_SWGROUP_CELLS(PPCS)>;
 		status = "disabled";
 	};
 
@@ -347,6 +361,7 @@
 		interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
 		nvidia,dma-request-selector = <&apbdma 19>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTD>;
+		iommus = <&smmu TEGRA_SWGROUP_CELLS(PPCS)>;
 		status = "disabled";
 	};
 
@@ -357,6 +372,7 @@
 		interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
 		nvidia,dma-request-selector = <&apbdma 20>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTE>;
+		iommus = <&smmu TEGRA_SWGROUP_CELLS(PPCS)>;
 		status = "disabled";
 	};
 
@@ -525,7 +541,7 @@
 		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
 	};
 
-	iommu {
+	smmu: iommu {
 		compatible = "nvidia,tegra30-smmu";
 		reg = <0x7000f010 0x02c
 		       0x7000f1f0 0x010
@@ -533,6 +549,7 @@
 		nvidia,#asids = <4>;		/* # of ASIDs */
 		dma-window = <0 0x40000000>;	/* IOVA start & length */
 		nvidia,ahb = <&ahb>;
+		#iommu-cells = <2>;
 	};
 
 	ahub {
@@ -605,6 +622,7 @@
 		reg = <0x78000000 0x200>;
 		interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA30_CLK_SDMMC1>;
+		iommus = <&smmu TEGRA_SWGROUP_CELLS(PPCS)>;
 		status = "disabled";
 	};
 
@@ -613,6 +631,7 @@
 		reg = <0x78000200 0x200>;
 		interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA30_CLK_SDMMC2>;
+		iommus = <&smmu TEGRA_SWGROUP_CELLS(PPCS)>;
 		status = "disabled";
 	};
 
@@ -621,6 +640,7 @@
 		reg = <0x78000400 0x200>;
 		interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA30_CLK_SDMMC3>;
+		iommus = <&smmu TEGRA_SWGROUP_CELLS(PPCS)>;
 		status = "disabled";
 	};
 
@@ -629,6 +649,7 @@
 		reg = <0x78000600 0x200>;
 		interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA30_CLK_SDMMC4>;
+		iommus = <&smmu TEGRA_SWGROUP_CELLS(PPCS)>;
 		status = "disabled";
 	};
 
-- 
1.8.1.5