[PATCHv6 00/13] Unifying SMMU driver among Tegra SoCs

[PATCHv6 00/13] Unifying SMMU driver among Tegra SoCs

[Hiroshi Doyu]

Hi,

This series provide:

(0) IOMMU standard DT binding("iommus")
(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:

  [PATCHv6 04/13] 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. Then,
those defered iommu master devices are populated and configured with
help of the already populated iommu device via a new IOMMU API
iommu_ops->driver_bound().

This "iommus=" binding is expected used as the global/standard binding.

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

v5:
Use "iommus=" DT bindings as a standard IOMMU binding.

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@nv-tegra.nvidia.com/user/hdoyu/linux.git smmu-upstreaming@20131121


Hiroshi Doyu (13):
  of: introduce of_property_for_earch_phandle_with_args()
  iommu/of: introduce a global iommu device list
  iommu/of: check if dependee iommu is ready or not
  driver/core: populate devices in order for IOMMUs
  iommu/core: add ops->{bound,unbind}_driver()
  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
  iommu/tegra: smmu: add SMMU to an global iommu list
  [FOR TEST] ARM: dt: tegra30: add "iommus" binding

 .../bindings/iommu/nvidia,tegra30-smmu.txt         |  30 +-
 arch/arm/boot/dts/tegra30.dtsi                     |  23 +-
 drivers/base/dd.c                                  |   5 +
 drivers/iommu/Kconfig                              |   1 +
 drivers/iommu/iommu.c                              |  13 +-
 drivers/iommu/of_iommu.c                           |  53 +++
 drivers/iommu/tegra-smmu.c                         | 383 +++++++++++++--------
 include/dt-bindings/memory/tegra-swgroup.h         |  50 +++
 include/linux/iommu.h                              |   4 +
 include/linux/of.h                                 |   3 +
 include/linux/of_iommu.h                           |  22 ++
 11 files changed, 436 insertions(+), 151 deletions(-)
 create mode 100644 include/dt-bindings/memory/tegra-swgroup.h

-- 
1.8.1.5

[PATCHv6 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Hiroshi Doyu]

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">;

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

[PATCHv6 02/13] iommu/of: introduce a global iommu device list

[Hiroshi Doyu]

This enables to find an populated IOMMU device via a device node. This
can be used to see if an dependee IOMMU is populated or not to keep
correct device population order. Client devices need to wait an IOMMU
to be populated.

Suggested by Thierry Reding and copied his example code.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
---
v6:
New for v6.
---
 drivers/iommu/of_iommu.c | 37 +++++++++++++++++++++++++++++++++++++
 include/linux/of_iommu.h | 16 ++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index ee249bc..5d1aeb9 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -20,6 +20,43 @@
 #include <linux/export.h>
 #include <linux/limits.h>
 #include <linux/of.h>
+#include <linux/of_iommu.h>
+#include <linux/device.h>
+
+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);
+}
+
+static 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;
+}
 
 /**
  * of_get_dma_window - Parse *dma-window property and returns 0 if found.
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 51a560f..a0aa9d4 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -3,10 +3,18 @@
 
 #ifdef CONFIG_OF_IOMMU
 
+struct iommu {
+	struct list_head list;
+	struct device *dev;
+};
+
 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 void iommu_add(struct iommu *iommu);
+extern void iommu_del(struct iommu *iommu);
+
 #else
 
 static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
@@ -16,6 +24,14 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix,
 	return -EINVAL;
 }
 
+static inline void iommu_add(struct iommu *iommu)
+{
+}
+
+static inline void iommu_del(struct iommu *iommu)
+{
+}
+
 #endif	/* CONFIG_OF_IOMMU */
 
 #endif /* __OF_IOMMU_H */
-- 
1.8.1.5

[PATCHv6 03/13] iommu/of: check if dependee iommu is ready or not

[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 depending iommu device is
populated.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
v6:
Spinned off only of_iommu part from:
  [PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

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/iommu/of_iommu.c | 16 ++++++++++++++++
 include/linux/of_iommu.h |  6 ++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 5d1aeb9..6ad4997e 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -125,3 +125,19 @@ 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;
+
+	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;
+	}
+
+	return 0;
+}
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index a0aa9d4..14c9a5c 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -14,6 +14,7 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix,
 
 extern void iommu_add(struct iommu *iommu);
 extern void iommu_del(struct iommu *iommu);
+extern int of_iommu_attach(struct device *dev);
 
 #else
 
@@ -32,6 +33,11 @@ static inline void iommu_del(struct iommu *iommu)
 {
 }
 
+static inline int of_iommu_attach(struct device *dev)
+{
+	return 0;
+}
+
 #endif	/* CONFIG_OF_IOMMU */
 
 #endif /* __OF_IOMMU_H */
-- 
1.8.1.5

[PATCHv6 04/13] 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. Then,
those deferred iommu master devices are populated and configured with
help of the already populated iommu device.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
v6:
Spinned off only driver core part from:
  [PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

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 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 0605176..0605f52 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)
-- 
1.8.1.5

[PATCHv6 05/13] iommu/core: add ops->{bound,unbind}_driver()

[Hiroshi Doyu]

ops->{bound,unbind}_driver() functions are called at
BUS_NOTIFY_{BOUND,UNBIND}_DRIVER respectively.

This is necessary to control the device population order. IOMMU master
devices depend on an IOMMU device instanciation. IOMMU master devices
can be registered to an IOMMU only after it's successfully
populated. This IOMMU registration is done via
ops->bound_driver(). Currently this population can be deferred if
depending IOMMU device hasn't yet been populated in driver core. This
cannot be done via ops->add_device() since after add_device() device's
population/instanciation can be still deferred via probe().

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
v6:
New for v6.
---
 drivers/iommu/iommu.c | 13 +++++++++++--
 include/linux/iommu.h |  4 ++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e5555fc..5469d36 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -540,14 +540,23 @@ static int iommu_bus_notifier(struct notifier_block *nb,
 	 * ADD/DEL call into iommu driver ops if provided, which may
 	 * result in ADD/DEL notifiers to group->notifier
 	 */
-	if (action == BUS_NOTIFY_ADD_DEVICE) {
+	switch (action) {
+	case BUS_NOTIFY_ADD_DEVICE:
 		if (ops->add_device)
 			return ops->add_device(dev);
-	} else if (action == BUS_NOTIFY_DEL_DEVICE) {
+	case BUS_NOTIFY_DEL_DEVICE:
 		if (ops->remove_device && dev->iommu_group) {
 			ops->remove_device(dev);
 			return 0;
 		}
+	case BUS_NOTIFY_BOUND_DRIVER:
+		if (ops->bound_driver)
+			ops->bound_driver(dev);
+		break;
+	case BUS_NOTIFY_UNBIND_DRIVER:
+		if (ops->unbind_driver)
+			ops->unbind_driver(dev);
+		break;
 	}
 
 	/*
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index a444c79..a0e92be 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -96,6 +96,8 @@ enum iommu_attr {
  * @domain_has_cap: domain capabilities query
  * @add_device: add device to iommu grouping
  * @remove_device: remove device from iommu grouping
+ * @bound_driver: called at BUS_NOTIFY_BOUND_DRIVER
+ * @unbind_driver: called at BUS_NOTIFY_UNBIND_DRIVER
  * @domain_get_attr: Query domain attributes
  * @domain_set_attr: Change domain attributes
  * @pgsize_bitmap: bitmap of supported page sizes
@@ -114,6 +116,8 @@ struct iommu_ops {
 			      unsigned long cap);
 	int (*add_device)(struct device *dev);
 	void (*remove_device)(struct device *dev);
+	int (*bound_driver)(struct device *dev);
+	void (*unbind_driver)(struct device *dev);
 	int (*device_group)(struct device *dev, unsigned int *groupid);
 	int (*domain_get_attr)(struct iommu_domain *domain,
 			       enum iommu_attr attr, void *data);
-- 
1.8.1.5

[PATCHv6 06/13] 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>
---
v6:
Use 0xffffffff instead of ~0UL since dtc expand this to ~0ULL.

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..9c279f1
--- /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) & 0xffffffff) ((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

[PATCHv6 07/13] 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>
---
v6:
Use smmu_iommu_{bound,unbind}_driver() instead of
smmu_iommu_{add,del}_device() to register devices to SMMU.

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 3e7fdbb..0a86d63 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 3c772c9..99b4bd4 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_bound_driver(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_unbind_driver(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,
+	.bound_driver	= smmu_iommu_bound_driver,
+	.unbind_driver	= smmu_iommu_unbind_driver,
 	.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

[PATCHv6 08/13] 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 | 111 +++++++--------------------------------------
 1 file changed, 17 insertions(+), 94 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 99b4bd4..6ab977a 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;
-- 
1.8.1.5

[PATCHv6 09/13] 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>
---
v6:
- Explained "#iommu-cells" in the binding document.
- Fixed old "nvidia,memory-clients" with 'iommus" in the binding
  document.
- Move smmu_of_get_swgroups() here from the previous patch not to break
  git bisecting.

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         |  30 ++++-
 drivers/iommu/tegra-smmu.c                         | 133 ++++++++++++++++++---
 2 files changed, 144 insertions(+), 19 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
index 89fb543..fd53f54 100644
--- a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
@@ -1,6 +1,6 @@
 NVIDIA Tegra 30 IOMMU H/W, SMMU (System Memory Management Unit)
 
-Required properties:
+Required properties in the IOMMU node:
 - compatible : "nvidia,tegra30-smmu"
 - reg : Should contain 3 register banks(address and length) for each
   of the SMMU register blocks.
@@ -8,9 +8,23 @@ 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.
+- #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.
 
 Example:
-	smmu {
+	smmu: iommu {
 		compatible = "nvidia,tegra30-smmu";
 		reg = <0x7000f010 0x02c
 		       0x7000f1f0 0x010
@@ -18,4 +32,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";
+			iommus = <&smmu TEGRA_SWGROUP_CELLS(NV)
+					TEGRA_SWGROUP_CELLS(NV2)>;
+			....
+		};
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6ab977a..1a1bcdf 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));
@@ -888,10 +973,23 @@ enum {
 
 static int smmu_iommu_bound_driver(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 (!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
@@ -902,8 +1000,8 @@ static int smmu_iommu_bound_driver(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;
 }
 
@@ -1156,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

[PATCHv6 10/13] 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 1a1bcdf..76356db 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

[PATCHv6 11/13] 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 76356db..1544f7c 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

[PATCHv6 12/13] iommu/tegra: smmu: add SMMU to an global iommu list

[Hiroshi Doyu]

This allows to inquire if SMMU is populated or not.

Suggested by Thierry Reding and copied his example code.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
---
v6:
New for v6.
---
 drivers/iommu/tegra-smmu.c | 55 +++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 25 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 1544f7c..590c9fe 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -225,6 +225,8 @@ struct smmu_debugfs_info {
  * Per SMMU device - IOMMU device
  */
 struct smmu_device {
+	struct iommu	iommu;
+
 	void __iomem	*regbase;	/* register offset base */
 	void __iomem	**regs;		/* register block start address array */
 	void __iomem	**rege;		/* register block end address array */
@@ -234,7 +236,6 @@ struct smmu_device {
 	unsigned long	page_count;	/* total remappable size */
 	spinlock_t	lock;
 	char		*name;
-	struct device	*dev;
 	struct rb_root	clients;
 	struct page *avp_vector_page;	/* dummy page shared by all AS's */
 
@@ -371,7 +372,7 @@ static int register_smmu_client(struct smmu_device *smmu,
 		return -EBUSY;
 	}
 
-	client = devm_kzalloc(smmu->dev, sizeof(*client), GFP_KERNEL);
+	client = devm_kzalloc(smmu->iommu.dev, sizeof(*client), GFP_KERNEL);
 	if (!client)
 		return -ENOMEM;
 
@@ -388,7 +389,7 @@ static int smmu_of_get_swgroups(struct device *dev, unsigned long *swgroups)
 
 	of_property_for_each_phandle_with_args(dev->of_node, "iommus",
 					       "#iommu-cells", i, &args) {
-		if (args.np != smmu_handle->dev->of_node)
+		if (args.np != smmu_handle->iommu.dev->of_node)
 			continue;
 
 		BUG_ON(args.args_count != 2);
@@ -527,7 +528,7 @@ static void free_ptbl(struct smmu_as *as, dma_addr_t iova)
 	unsigned long *pdir = (unsigned long *)page_address(as->pdir_page);
 
 	if (pdir[pdn] != _PDE_VACANT(pdn)) {
-		dev_dbg(as->smmu->dev, "pdn: %lx\n", pdn);
+		dev_dbg(as->smmu->iommu.dev, "pdn: %lx\n", pdn);
 
 		ClearPageReserved(SMMU_EX_PTBL_PAGE(pdir[pdn]));
 		__free_page(SMMU_EX_PTBL_PAGE(pdir[pdn]));
@@ -542,7 +543,7 @@ static void free_pdir(struct smmu_as *as)
 {
 	unsigned addr;
 	int count;
-	struct device *dev = as->smmu->dev;
+	struct device *dev = as->smmu->iommu.dev;
 
 	if (!as->pdir_page)
 		return;
@@ -585,11 +586,11 @@ static unsigned long *locate_pte(struct smmu_as *as,
 		unsigned long addr = SMMU_PDN_TO_ADDR(pdn);
 
 		/* Vacant - allocate a new page table */
-		dev_dbg(as->smmu->dev, "New PTBL pdn: %lx\n", pdn);
+		dev_dbg(as->smmu->iommu.dev, "New PTBL pdn: %lx\n", pdn);
 
 		*ptbl_page_p = alloc_page(GFP_ATOMIC);
 		if (!*ptbl_page_p) {
-			dev_err(as->smmu->dev,
+			dev_err(as->smmu->iommu.dev,
 				"failed to allocate smmu_device page table\n");
 			return NULL;
 		}
@@ -649,7 +650,7 @@ static int alloc_pdir(struct smmu_as *as)
 	/*
 	 * do the allocation, then grab as->lock
 	 */
-	cnt = devm_kzalloc(smmu->dev,
+	cnt = devm_kzalloc(smmu->iommu.dev,
 			   sizeof(cnt[0]) * SMMU_PDIR_COUNT,
 			   GFP_KERNEL);
 	page = alloc_page(GFP_KERNEL | __GFP_DMA);
@@ -663,7 +664,8 @@ static int alloc_pdir(struct smmu_as *as)
 	}
 
 	if (!page || !cnt) {
-		dev_err(smmu->dev, "failed to allocate at %s\n", __func__);
+		dev_err(smmu->iommu.dev,
+			"failed to allocate at %s\n", __func__);
 		err = -ENOMEM;
 		goto err_out;
 	}
@@ -693,7 +695,7 @@ static int alloc_pdir(struct smmu_as *as)
 err_out:
 	spin_unlock_irqrestore(&as->lock, flags);
 
-	devm_kfree(smmu->dev, cnt);
+	devm_kfree(smmu->iommu.dev, cnt);
 	if (page)
 		__free_page(page);
 	return err;
@@ -748,7 +750,7 @@ static int smmu_iommu_map(struct iommu_domain *domain, unsigned long iova,
 	unsigned long pfn = __phys_to_pfn(pa);
 	unsigned long flags;
 
-	dev_info(as->smmu->dev, "[%d] %08lx:%pa\n", as->asid, iova, &pa);
+	dev_info(as->smmu->iommu.dev, "[%d] %08lx:%pa\n", as->asid, iova, &pa);
 
 	if (!pfn_valid(pfn))
 		return -ENOMEM;
@@ -765,7 +767,7 @@ static size_t smmu_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
 	struct smmu_as *as = domain->priv;
 	unsigned long flags;
 
-	dev_dbg(as->smmu->dev, "[%d] %08lx\n", as->asid, iova);
+	dev_dbg(as->smmu->iommu.dev, "[%d] %08lx\n", as->asid, iova);
 
 	spin_lock_irqsave(&as->lock, flags);
 	__smmu_iommu_unmap(as, iova);
@@ -788,7 +790,7 @@ static phys_addr_t smmu_iommu_iova_to_phys(struct iommu_domain *domain,
 	pte = locate_pte(as, iova, true, &page, &count);
 	pfn = *pte & SMMU_PFN_MASK;
 	WARN_ON(!pfn_valid(pfn));
-	dev_dbg(as->smmu->dev,
+	dev_dbg(as->smmu->iommu.dev,
 		"iova:%08llx pfn:%08lx asid:%d\n", (unsigned long long)iova,
 		 pfn, as->asid);
 
@@ -822,7 +824,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 	spin_lock(&as->client_lock);
 	list_for_each_entry(c, &as->client, list) {
 		if (c->dev == dev) {
-			dev_err(smmu->dev,
+			dev_err(smmu->iommu.dev,
 				"%s is already attached\n", dev_name(c->dev));
 			err = -EINVAL;
 			goto err_client;
@@ -844,7 +846,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 		pr_info("Reserve \"page zero\" for AVP vectors using a common dummy\n");
 	}
 
-	dev_dbg(smmu->dev, "%s is attached\n", dev_name(dev));
+	dev_dbg(smmu->iommu.dev, "%s is attached\n", dev_name(dev));
 	return 0;
 
 err_client:
@@ -867,12 +869,12 @@ static void smmu_iommu_detach_dev(struct iommu_domain *domain,
 			smmu_client_disable_swgroups(c);
 			list_del(&c->list);
 			c->as = NULL;
-			dev_dbg(smmu->dev,
+			dev_dbg(smmu->iommu.dev,
 				"%s is detached\n", dev_name(c->dev));
 			goto out;
 		}
 	}
-	dev_err(smmu->dev, "Couldn't find %s\n", dev_name(dev));
+	dev_err(smmu->iommu.dev, "Couldn't find %s\n", dev_name(dev));
 out:
 	spin_unlock(&as->client_lock);
 }
@@ -899,7 +901,7 @@ static int smmu_iommu_domain_init(struct iommu_domain *domain)
 			break;
 	}
 	if (i == smmu->num_as)
-		dev_err(smmu->dev,  "no free AS\n");
+		dev_err(smmu->iommu.dev,  "no free AS\n");
 	return err;
 
 found:
@@ -920,7 +922,7 @@ found:
 		smmu->page_count * SMMU_PAGE_SIZE - 1;
 	domain->geometry.force_aperture = true;
 
-	dev_dbg(smmu->dev, "smmu_as@%p\n", as);
+	dev_dbg(smmu->iommu.dev, "smmu_as@%p\n", as);
 
 	return 0;
 }
@@ -953,7 +955,7 @@ static void smmu_iommu_domain_destroy(struct iommu_domain *domain)
 	spin_unlock_irqrestore(&as->lock, flags);
 
 	domain->priv = NULL;
-	dev_dbg(smmu->dev, "smmu_as@%p\n", as);
+	dev_dbg(smmu->iommu.dev, "smmu_as@%p\n", as);
 }
 
 /*
@@ -1085,7 +1087,7 @@ static ssize_t smmu_debugfs_stats_write(struct file *file,
 		break;
 	}
 
-	dev_dbg(smmu->dev, "%s() %08x, %08x @%08x\n", __func__,
+	dev_dbg(smmu->iommu.dev, "%s() %08x, %08x @%08x\n", __func__,
 		val, smmu_read(smmu, offs), offs);
 
 	return count;
@@ -1107,7 +1109,7 @@ static int smmu_debugfs_stats_show(struct seq_file *s, void *v)
 		val = smmu_read(smmu, offs);
 		seq_printf(s, "%s:%08x ", stats[i], val);
 
-		dev_dbg(smmu->dev, "%s() %s %08x @%08x\n", __func__,
+		dev_dbg(smmu->iommu.dev, "%s() %s %08x @%08x\n", __func__,
 			stats[i], val, offs);
 	}
 	seq_printf(s, "\n");
@@ -1145,7 +1147,7 @@ static void smmu_debugfs_create(struct smmu_device *smmu)
 	if (!smmu->debugfs_info)
 		return;
 
-	root = debugfs_create_dir(dev_name(smmu->dev), NULL);
+	root = debugfs_create_dir(dev_name(smmu->iommu.dev), NULL);
 	if (!root)
 		goto err_out;
 	smmu->debugfs_root = root;
@@ -1217,7 +1219,7 @@ static void tegra_smmu_create_default_map(struct smmu_device *smmu)
 		smmu->map[i] = arm_iommu_create_mapping(&platform_bus_type,
 							base, size, 0);
 		if (IS_ERR(smmu->map[i]))
-			dev_err(smmu->dev,
+			dev_err(smmu->iommu.dev,
 				"Couldn't create: asid=%d map=%p %pa-%pa\n",
 				i, smmu->map[i], &base, &base + size - 1);
 	}
@@ -1285,7 +1287,7 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	if (!smmu->ahb)
 		return -ENODEV;
 
-	smmu->dev = dev;
+	smmu->iommu.dev = dev;
 	smmu->num_as = asids;
 	smmu->iovmm_base = base;
 	smmu->page_count = size;
@@ -1322,6 +1324,8 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	smmu_handle = smmu;
 	bus_set_iommu(&platform_bus_type, &smmu_iommu_ops);
 	tegra_smmu_create_default_map(smmu);
+
+	iommu_add(&smmu->iommu);
 	return 0;
 }
 
@@ -1337,6 +1341,7 @@ static int tegra_smmu_remove(struct platform_device *pdev)
 		free_pdir(&smmu->as[i]);
 	__free_page(smmu->avp_vector_page);
 	smmu_handle = NULL;
+	iommu_del(&smmu->iommu);
 	return 0;
 }
 
-- 
1.8.1.5

[PATCHv6 13/13] [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".

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 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@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

[PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Hiroshi Doyu]

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.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
v6+:
Use the description, which Grant Likely proposed, to be full enough
that a future reader can figure out why a patch was written.
  http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007062.html

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

Re: [PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Stephen Warren]

On 11/21/2013 10:17 AM, Hiroshi Doyu wrote:
> 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.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> ---
> v6+:
> Use the description, which Grant Likely proposed, to be full enough
> that a future reader can figure out why a patch was written.
>   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007062.html

This new version only addresses one of the concerns that Grant had,
namely the commit message.

> diff --git a/include/linux/of.h b/include/linux/of.h

> +#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++)
> +

Grant also wanted the actual implementation fixed so that it wasn't so
inefficient.

What this current patch does is basically:

for every entry in the property:
    for every entry in the property before the current index:
        parse the phandle+specifier

That's roughly O(n^2). (n is # entries in the property)

Instead, what should happen is:

for every entry in the property:
    parse the phandle+specifier
    yield the result

That's roughly O(n).

In other words, an implementation more along the lines of
include/linux/of.h's:

#define of_property_for_each_u32(np, propname, prop, p, u)      \
        for (prop = of_find_property(np, propname, NULL),       \
                p = of_prop_next_u32(prop, NULL, &u);           \
                p;                                              \
                p = of_prop_next_u32(prop, p, &u))

... so you'd need functions like of_prop_first_specifier() and
of_prop_next_specifier(), and perhaps some associated set of state
variables, perhaps with all the state wrapped into a single struct for
simplicity.

Re: [PATCHv6 05/13] iommu/core: add ops->{bound,unbind}_driver()

[Hiroshi Doyu]

Hi Joerg,

Do you have some time to review this patch along with the following ones?

  [PATCHv6 02/13] iommu/of: introduce a global iommu device list
  http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007050.html

  [PATCHv6 03/13] iommu/of: check if dependee iommu is ready or not
  http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007051.html

With those patches, now I'm trying to populate iommu master devices
after an IOMMU device is populated. Originally [PATCHv6 02/13] was
proposed by Thierry. I'm a bit afraid of adding new IOMMU API as
this, but I think that this new {bound,unbind}_driver() is the right
timiing that depeding devices are populated instead of add_device()
since, even after add_device, a device won't be populated. I'm not so
sure how this affects on the existing IOMMUs.

It would be nice if you give some feedback on this.

On Thu, 21 Nov 2013 14:40:41 +0100
Hiroshi Doyu <hdoyu@nvidia.com> wrote:

> ops->{bound,unbind}_driver() functions are called at
> BUS_NOTIFY_{BOUND,UNBIND}_DRIVER respectively.
> 
> This is necessary to control the device population order. IOMMU master
> devices depend on an IOMMU device instanciation. IOMMU master devices
> can be registered to an IOMMU only after it's successfully
> populated. This IOMMU registration is done via
> ops->bound_driver(). Currently this population can be deferred if
> depending IOMMU device hasn't yet been populated in driver core. This
> cannot be done via ops->add_device() since after add_device() device's
> population/instanciation can be still deferred via probe().
> 
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> ---
> v6:
> New for v6.
> ---
>  drivers/iommu/iommu.c | 13 +++++++++++--
>  include/linux/iommu.h |  4 ++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index e5555fc..5469d36 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -540,14 +540,23 @@ static int iommu_bus_notifier(struct notifier_block *nb,
>  	 * ADD/DEL call into iommu driver ops if provided, which may
>  	 * result in ADD/DEL notifiers to group->notifier
>  	 */
> -	if (action == BUS_NOTIFY_ADD_DEVICE) {
> +	switch (action) {
> +	case BUS_NOTIFY_ADD_DEVICE:
>  		if (ops->add_device)
>  			return ops->add_device(dev);
> -	} else if (action == BUS_NOTIFY_DEL_DEVICE) {
> +	case BUS_NOTIFY_DEL_DEVICE:
>  		if (ops->remove_device && dev->iommu_group) {
>  			ops->remove_device(dev);
>  			return 0;
>  		}
> +	case BUS_NOTIFY_BOUND_DRIVER:
> +		if (ops->bound_driver)
> +			ops->bound_driver(dev);
> +		break;
> +	case BUS_NOTIFY_UNBIND_DRIVER:
> +		if (ops->unbind_driver)
> +			ops->unbind_driver(dev);
> +		break;
>  	}
>  
>  	/*
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index a444c79..a0e92be 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -96,6 +96,8 @@ enum iommu_attr {
>   * @domain_has_cap: domain capabilities query
>   * @add_device: add device to iommu grouping
>   * @remove_device: remove device from iommu grouping
> + * @bound_driver: called at BUS_NOTIFY_BOUND_DRIVER
> + * @unbind_driver: called at BUS_NOTIFY_UNBIND_DRIVER
>   * @domain_get_attr: Query domain attributes
>   * @domain_set_attr: Change domain attributes
>   * @pgsize_bitmap: bitmap of supported page sizes
> @@ -114,6 +116,8 @@ struct iommu_ops {
>  			      unsigned long cap);
>  	int (*add_device)(struct device *dev);
>  	void (*remove_device)(struct device *dev);
> +	int (*bound_driver)(struct device *dev);
> +	void (*unbind_driver)(struct device *dev);
>  	int (*device_group)(struct device *dev, unsigned int *groupid);
>  	int (*domain_get_attr)(struct iommu_domain *domain,
>  			       enum iommu_attr attr, void *data);
> -- 
> 1.8.1.5
> 

[RFC][PATCHv6++ 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Hiroshi Doyu]

Stephen Warren <swarren@wwwdotorg.org> wrote @ Thu, 21 Nov 2013 19:57:00 +0100:

> On 11/21/2013 10:17 AM, Hiroshi Doyu wrote:
> > 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.
> > 
> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > ---
> > v6+:
> > Use the description, which Grant Likely proposed, to be full enough
> > that a future reader can figure out why a patch was written.
> >   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007062.html
> 
> This new version only addresses one of the concerns that Grant had,
> namely the commit message.
> 
> > diff --git a/include/linux/of.h b/include/linux/of.h
> 
> > +#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++)
> > +
> 
> Grant also wanted the actual implementation fixed so that it wasn't so
> inefficient.
> 
> What this current patch does is basically:
> 
> for every entry in the property:
>     for every entry in the property before the current index:
>         parse the phandle+specifier
> 
> That's roughly O(n^2). (n is # entries in the property)
> 
> Instead, what should happen is:
> 
> for every entry in the property:
>     parse the phandle+specifier
>     yield the result
> 
> That's roughly O(n).
> 
> In other words, an implementation more along the lines of
> include/linux/of.h's:
> 
> #define of_property_for_each_u32(np, propname, prop, p, u)      \
>         for (prop = of_find_property(np, propname, NULL),       \
>                 p = of_prop_next_u32(prop, NULL, &u);           \
>                 p;                                              \
>                 p = of_prop_next_u32(prop, p, &u))
> 
> ... so you'd need functions like of_prop_first_specifier() and
> of_prop_next_specifier(), and perhaps some associated set of state
> variables, perhaps with all the state wrapped into a single struct for
> simplicity.

Although I couldn't invent any struct to hold params and state here,
I'd like you to review the following interface is ok or not.

At first, I thought to refactor __of_parse_phandle_with_args() but
it's a bit highly optimized by Stephen and it looked a bit hard to
refactor without perf regressions. Instread, I introduced 2 new
functions "of_parse_{first,next}_phandle_with_args()" to parse
phandles.

If this interface is ok, I'll include this into the next v7 series.

-----8<-----8<-----8<-----8<-----8<-----8<-----8<-----8<-----8<-----
From: Hiroshi Doyu <hdoyu@nvidia.com>

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.

Introduced "of_parse_{first,next}_phandle_with_args()", where "const
__be32 **list" is used to hold the next list to be processed as a
state pramameter, and both "of_parse_{first,next}_phandle_with_args()"
returns the remaining list in the number of cell(4 byte). If any error
happens, "list" is set NULL not to proceed the rest.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
v6++:
Optimized to avoid O(n^2), suggested by Stephen Warren.
http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007066.html

v6+:
Use the description, which Grant Likely proposed, to be full enough
that a future reader can figure out why a patch was written.

v5:
New patch for v5.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 drivers/of/base.c  | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h | 52 ++++++++++++++++++++++++++++++++++
 2 files changed, 134 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index f807d0e..3e29b10 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1201,6 +1201,88 @@ void of_print_phandle_args(const char *msg, const struct of_phandle_args *args)
 	printk("\n");
 }
 
+int __of_parse_next_phandle_with_args(const __be32 **plist,
+				      const char *cells_name, int cell_count,
+				      struct of_phandle_args *out_args)
+{
+	phandle phandle;
+	int i, count = 0, err;
+	struct device_node *dn;
+	const __be32 *list;
+
+	BUG_ON(!out_args);
+	BUG_ON(!cells_name && !cell_count);
+
+	/*
+	 * "*plist" should hold phandle, and it's updated to the next
+	 * phandle at return if no error.
+	 */
+	list = *plist;
+	out_args->np = NULL;
+
+	phandle = be32_to_cpup(list);
+	if (!phandle)
+		goto err_out;
+
+	dn = of_find_node_by_phandle(phandle);
+	if (!dn)
+		goto err_out;
+
+	if (cells_name) {
+		err = of_property_read_u32(dn, cells_name, &count);
+		if (err)
+			goto err_out;
+	} else {
+		count = cell_count;
+	}
+
+	out_args->np = dn;
+	out_args->args_count = count;
+	for (i = 0; i < count; i++)
+		out_args->args[i] = be32_to_cpup(list + 1 + i);
+
+	*plist = list + count + 1; /* update to the next list */
+	return count + 1;
+
+err_out:
+	*plist = NULL;
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(__of_parse_next_phandle_with_args);
+
+int __of_parse_first_phandle_with_args(const struct device_node *np,
+				       const __be32 **out_list,
+				       const char *list_name,
+				       const char *cells_name,
+				       int cell_count,
+				       struct of_phandle_args *out_args)
+{
+	int rem, count;
+	const __be32 *list;
+
+	list = of_get_property(np, list_name, &rem);
+	if (!list)
+		goto err_out;
+
+	rem /= sizeof(*list);
+	if (!rem)
+		goto err_out;
+
+	count = __of_parse_next_phandle_with_args(&list, cells_name,
+						  cell_count, out_args);
+	if (!list)
+		goto err_out;
+
+	rem -= count;
+	*out_list = list; /* update to the next list */
+	return rem;
+
+err_out:
+	*out_list = NULL;
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(__of_parse_first_phandle_with_args);
+
 static int __of_parse_phandle_with_args(const struct device_node *np,
 					const char *list_name,
 					const char *cells_name,
diff --git a/include/linux/of.h b/include/linux/of.h
index 276c546..9f15622 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -303,6 +303,35 @@ extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
 extern int of_count_phandle_with_args(const struct device_node *np,
 	const char *list_name, const char *cells_name);
 
+extern int __of_parse_first_phandle_with_args(const struct device_node *np,
+					    const __be32 **out_list,
+					    const char *list_name,
+					    const char *cells_name,
+					    int cell_count,
+					    struct of_phandle_args *out_args);
+
+extern int __of_parse_next_phandle_with_args(const __be32 **plist,
+					   const char *cells_name,
+					   int cell_count,
+					   struct of_phandle_args *out_args);
+
+static inline int of_parse_first_phandle_with_args(const struct device_node *np,
+						   const __be32 **out_list,
+						   const char *list_name,
+						   const char *cells_name,
+						   struct of_phandle_args *out_args)
+{
+	return __of_parse_first_phandle_with_args(np, out_list, list_name,
+						  cells_name, 0, out_args);
+}
+
+static inline int of_parse_next_phandle_with_args(const __be32 **plist,
+						  const char *cells_name,
+						  struct of_phandle_args *out_args)
+{
+	return __of_parse_next_phandle_with_args(plist, cells_name, 0, out_args);
+}
+
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
 
@@ -527,6 +556,24 @@ static inline int of_count_phandle_with_args(struct device_node *np,
 	return -ENOSYS;
 }
 
+static inline int __of_parse_first_phandle_with_args(
+	const struct device_node *np, const __be32 **out_list,
+	const char *list_name, const char *cells_name, int cell_count,
+	struct of_phandle_args *out_args)
+{
+	*out_list = NULL;
+	return -ENOSYS;
+}
+
+
+static inline int __of_parse_next_phandle_with_args(
+	const __be32 **plist, const char *cells_name, int cell_count,
+	struct of_phandle_args *out_args)
+{
+	*plist = NULL;
+	return -ENOSYS;
+}
+
 static inline int of_alias_get_id(struct device_node *np, const char *stem)
 {
 	return -ENOSYS;
@@ -613,6 +660,11 @@ 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(node, list, list_name, cells_name, rem,  args) \
+	for (rem = of_parse_first_phandle_with_args(node, &list, list_name, cells_name, &args); \
+	     list && rem >= 0;						\
+	     rem -= of_parse_next_phandle_with_args(&list, cells_name, &args))
+
 #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

[RFC][PATCHv6+++ 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Hiroshi Doyu]

Hiroshi Doyu <hdoyu@nvidia.com> wrote @ Thu, 28 Nov 2013 14:58:18 +0200 (EET):
....
> > In other words, an implementation more along the lines of
> > include/linux/of.h's:
> > 
> > #define of_property_for_each_u32(np, propname, prop, p, u)      \
> >         for (prop = of_find_property(np, propname, NULL),       \
> >                 p = of_prop_next_u32(prop, NULL, &u);           \
> >                 p;                                              \
> >                 p = of_prop_next_u32(prop, p, &u))
> > 
> > ... so you'd need functions like of_prop_first_specifier() and
> > of_prop_next_specifier(), and perhaps some associated set of state
> > variables, perhaps with all the state wrapped into a single struct for
> > simplicity.
> 
> Although I couldn't invent any struct to hold params and state here,
> I'd like you to review the following interface is ok or not.

Tried again to introduce a new struct to keep track of iteration state
as below:

----8<---------8<---------8<---------8<---------8<---------8<--
From: Hiroshi Doyu <hdoyu@nvidia.com>

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.

Introduced a new struct "of_phandle_iter" to keep the state when
iterating over the list.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
v6+++:
Introduced a new struct "of_phandle_iter" to keep the state when
iterating over the list.

v6++:
Optimized to avoid O(n^2), suggested by Stephen Warren.
http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007066.html

I didn't introduce any struct to hold params and state here.

v6+:
Use the description, which Grant Likely proposed, to be full enough
that a future reader can figure out why a patch was written.

v5:
New patch for v5.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 drivers/of/base.c  | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h | 45 ++++++++++++++++++++++++++++++++++
 2 files changed, 116 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index f807d0e..16fb2d9 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1201,6 +1201,77 @@ void of_print_phandle_args(const char *msg, const struct of_phandle_args *args)
 	printk("\n");
 }
 
+void of_phandle_iter_next(struct of_phandle_iter *iter,
+			  struct of_phandle_args *out_args)
+{
+	phandle phandle;
+	struct device_node *dn;
+	int i, count = iter->cell_count;
+
+	iter->err = -EINVAL;
+	if (!iter->cells_name && !iter->cell_count)
+		return;
+
+	phandle = be32_to_cpup(iter->cur++);
+	if (!phandle)
+		return;
+
+	dn = of_find_node_by_phandle(phandle);
+	if (!dn)
+		return;
+
+	if (iter->cells_name)
+		if (of_property_read_u32(dn, iter->cells_name, &count))
+			return;
+
+	out_args->np = dn;
+	out_args->args_count = count;
+	for (i = 0; i < count; i++)
+		out_args->args[i] = be32_to_cpup(iter->cur++);
+
+	iter->err = 0;
+}
+EXPORT_SYMBOL_GPL(of_phandle_iter_next);
+
+static void __of_phandle_iter_set(struct of_phandle_iter *iter,
+				  const struct device_node *np,
+				  const char *list_name)
+{
+	size_t bytes;
+	const __be32 *prop;
+
+	prop = of_get_property(np, list_name, &bytes);
+	if (!prop) {
+		iter->err = -EINVAL;
+		return;
+	}
+
+	iter->cur = prop;
+	iter->end = prop + bytes / sizeof(*prop);
+	iter->err = 0;
+}
+
+void of_phandle_iter_start(struct of_phandle_iter *iter,
+			   const struct device_node *np,
+			   const char *list_name,
+			   const char *cells_name,
+			   int cell_count,
+			   struct of_phandle_args *out_args)
+{
+	iter->err = -EINVAL;
+	if (!cells_name && !cell_count)
+		return;
+
+	iter->cells_name = cells_name;
+	iter->cell_count = cell_count;
+	__of_phandle_iter_set(iter, np, list_name);
+	if (iter->err)
+		return;
+
+	of_phandle_iter_next(iter, out_args);
+}
+EXPORT_SYMBOL_GPL(of_phandle_iter_start);
+
 static int __of_parse_phandle_with_args(const struct device_node *np,
 					const char *list_name,
 					const char *cells_name,
diff --git a/include/linux/of.h b/include/linux/of.h
index 276c546..1132b49 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -74,6 +74,18 @@ struct of_phandle_args {
 	uint32_t args[MAX_PHANDLE_ARGS];
 };
 
+/*
+ * keep the state at iterating a list of phandles with variable number
+ * of args
+ */
+struct of_phandle_iter {
+	int		err;
+	const __be32	*cur;		/* current phandle */
+	const __be32	*end;		/* end of the last phandle */
+	const char	*cells_name;
+	int		cell_count;
+};
+
 #ifdef CONFIG_OF_DYNAMIC
 extern struct device_node *of_node_get(struct device_node *node);
 extern void of_node_put(struct device_node *node);
@@ -303,6 +315,16 @@ extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
 extern int of_count_phandle_with_args(const struct device_node *np,
 	const char *list_name, const char *cells_name);
 
+extern void of_phandle_iter_start(struct of_phandle_iter *iter,
+				  const struct device_node *np,
+				  const char *list_name,
+				  const char *cells_name,
+				  int cell_count,
+				  struct of_phandle_args *out_args);
+
+extern void of_phandle_iter_next(struct of_phandle_iter *iter,
+				 struct of_phandle_args *out_args);
+
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
 
@@ -527,6 +549,22 @@ static inline int of_count_phandle_with_args(struct device_node *np,
 	return -ENOSYS;
 }
 
+static inline void of_phandle_iter_start(struct of_phandle_iter *iter,
+					 const struct device_node *np,
+					 const char *list_name,
+					 const char *cells_name,
+					 const int cell_count,
+					 struct of_phandle_args *out_args)
+{
+	iter->err = -ENOSYS;
+}
+
+static inline void of_phandle_iter_next(struct of_phandle_iter *iter,
+					struct of_phandle_args *out_args)
+{
+	iter->err = -ENOSYS;
+}
+
 static inline int of_alias_get_id(struct device_node *np, const char *stem)
 {
 	return -ENOSYS;
@@ -613,6 +651,13 @@ 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(iter, node, list_name,	\
+					       cells_name, cell_count, out_args) \
+	for (of_phandle_iter_start(&iter, node, list_name,		\
+				   cells_name, cell_count, &out_args);	\
+	     !iter.err && iter.end - iter.cur >= 0 ;			\
+	     of_phandle_iter_next(&iter, &out_args))
+
 #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

Re: [RFC][PATCHv6+++ 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Stephen Warren]

On 11/29/2013 04:46 AM, Hiroshi Doyu wrote:
...
> 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.
> 
> Introduced a new struct "of_phandle_iter" to keep the state when
> iterating over the list.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> ---
> v6+++:

Surely that's v9; "+++" is rather unusual.

> diff --git a/drivers/of/base.c b/drivers/of/base.c

> +void of_phandle_iter_next(struct of_phandle_iter *iter,
> +			  struct of_phandle_args *out_args)
> +{
> +	phandle phandle;
> +	struct device_node *dn;
> +	int i, count = iter->cell_count;
> +
> +	iter->err = -EINVAL;
> +	if (!iter->cells_name && !iter->cell_count)
> +		return;

Wasn't that already checked in _start()?

Why not set err = -EINVAL inside the if, rather than setting it to an
error value here by default, then having to over-write it at the end of
the function?

> +static void __of_phandle_iter_set(struct of_phandle_iter *iter,

This is only used in one place; why not simply inline this into
of_phandle_iter_start()? It would make the code simpler.

> +void of_phandle_iter_start(struct of_phandle_iter *iter,

> +	iter->cells_name = cells_name;
> +	iter->cell_count = cell_count;

Why not pass these values into of_phandle_iter_next() instead? There's
no need to pass them just to _start() so they can be read by _next()
instead.

> diff --git a/include/linux/of.h b/include/linux/of.h

> +/*
> + * keep the state at iterating a list of phandles with variable number
> + * of args
> + */
> +struct of_phandle_iter {
> +	int		err;
> +	const __be32	*cur;		/* current phandle */
> +	const __be32	*end;		/* end of the last phandle */

Can't you detect an error case by e.g. (cur == NULL) and thus avoid
requiring an explicit err field?

Together with removing:

> +	const char	*cells_name;
> +	int		cell_count;

... then you'd only be left with cur/end, so I think you could get away
without a struct at all, but simply "cur" as the iterator variable, plus
"end" as the one temp variable.

Re: [RFC][PATCHv6+++ 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Hiroshi Doyu]

Stephen Warren <swarren@wwwdotorg.org> wrote @ Sun, 1 Dec 2013 20:00:09 +0100:

> On 11/29/2013 04:46 AM, Hiroshi Doyu wrote:
> ...
> > 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.
> > 
> > Introduced a new struct "of_phandle_iter" to keep the state when
> > iterating over the list.
> > 
> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > ---
> > v6+++:
> 
> Surely that's v9; "+++" is rather unusual.

My intention was to put this into the next v7 series after I get this
reviewed as RFC.
...
> Together with removing:
> 
> > +	const char	*cells_name;
> > +	int		cell_count;
> 
> ... then you'd only be left with cur/end, so I think you could get away
> without a struct at all, but simply "cur" as the iterator variable, plus
> "end" as the one temp variable.

Although the above proposal would be alsmot same as "[RFC][PATCHv6++
01/13]"(*1) where I use *list(cur) and rem as remaining count, here's
the update. I'll put this into v7 series.

---8<------8<------8<------8<------8<------8<------8<------8<------8<---
From: Hiroshi Doyu <hdoyu@nvidia.com>

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.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
v6++++:
Iterate without intrducing a new struct.

v6+++:
Introduced a new struct "of_phandle_iter" to keep the state when
iterating over the list.

v6++:
Optimized to avoid O(n^2), suggested by Stephen Warren.
http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007066.html

I didn't introduce any struct to hold params and state here.

v6+:
Use the description, which Grant Likely proposed, to be full enough
that a future reader can figure out why a patch was written.

v5:
New patch for v5.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 drivers/of/base.c  | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index f807d0e..7501f24 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1201,6 +1201,59 @@ void of_print_phandle_args(const char *msg, const struct of_phandle_args *args)
 	printk("\n");
 }
 
+const __be32 *of_phandle_iter_next(const char *cells_name, int cell_count,
+				   const __be32 *cur, const __be32 *end,
+				   struct of_phandle_args *out_args)
+{
+	phandle phandle;
+	struct device_node *dn;
+	int i;
+
+	if (!cells_name && !cell_count)
+		return NULL;
+
+	if (!cur)
+		return NULL;
+
+	if (end - cur <= 0)
+		return NULL;
+
+	phandle = be32_to_cpup(cur++);
+	if (!phandle)
+		return NULL;
+
+	dn = of_find_node_by_phandle(phandle);
+	if (!dn)
+		return NULL;
+
+	if (cells_name)
+		if (of_property_read_u32(dn, cells_name, &cell_count))
+			return NULL;
+
+	out_args->np = dn;
+	out_args->args_count = cell_count;
+	for (i = 0; i < cell_count; i++)
+		out_args->args[i] = be32_to_cpup(cur++);
+
+	return cur;
+}
+EXPORT_SYMBOL_GPL(of_phandle_iter_next);
+
+const __be32 *of_phandle_iter_init(const struct device_node *np,
+				   const char *list_name,
+				   const __be32 **end)
+{
+	size_t bytes;
+	const __be32 *cur;
+
+	cur = of_get_property(np, list_name, &bytes);
+	if (bytes)
+		*end = cur + bytes / sizeof(*cur);
+
+	return cur;
+}
+EXPORT_SYMBOL_GPL(of_phandle_iter_init);
+
 static int __of_parse_phandle_with_args(const struct device_node *np,
 					const char *list_name,
 					const char *cells_name,
diff --git a/include/linux/of.h b/include/linux/of.h
index 276c546..c23710b 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -303,6 +303,14 @@ extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
 extern int of_count_phandle_with_args(const struct device_node *np,
 	const char *list_name, const char *cells_name);
 
+extern const __be32 *of_phandle_iter_init(const struct device_node *np,
+					  const char *list_name,
+					  const __be32 **end);
+extern const __be32 *of_phandle_iter_next(const char *cells_name,
+					  int cell_count,
+					  const __be32 *cur, const __be32 *end,
+					  struct of_phandle_args *out_args);
+
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
 
@@ -527,6 +535,24 @@ static inline int of_count_phandle_with_args(struct device_node *np,
 	return -ENOSYS;
 }
 
+static inline const __be32 *of_phandle_iter_init(const struct device_node *np,
+						 const char *list_name,
+						 const char *cells_name,
+						 const int cell_count,
+						 struct of_phandle_args *out_args)
+{
+	return NULL;
+}
+
+static inline const __be32 *of_phandle_iter_next(const char *cells_name,
+						 int cell_count,
+						 const __be32 *cur,
+						 const __be32 *end,
+						 struct of_phandle_args *out_args);
+{
+	return NULL;
+}
+
 static inline int of_alias_get_id(struct device_node *np, const char *stem)
 {
 	return -ENOSYS;
@@ -613,6 +639,14 @@ 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(node, list_name,	cells_name, \
+					       cell_count, out_args, cur, end) \
+	for (cur = of_phandle_iter_init(node, list_name, &end),		\
+		     cur = of_phandle_iter_next(cells_name, cell_count, \
+						cur, end, &out_args);	\
+	     cur;							\
+	     cur = of_phandle_iter_next(cells_name, cell_count, cur, end, &out_args))
+
 #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





*1:
 http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007086.html

Re: [RFC][PATCHv6+++ 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Rob Herring]

On Mon, Dec 2, 2013 at 5:02 AM, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> Stephen Warren <swarren@wwwdotorg.org> wrote @ Sun, 1 Dec 2013 20:00:09 +0100:
>
>> On 11/29/2013 04:46 AM, Hiroshi Doyu wrote:
>> ...
>> > 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.
>> >
>> > Introduced a new struct "of_phandle_iter" to keep the state when
>> > iterating over the list.
>> >
>> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
>> > ---
>> > v6+++:
>>
>> Surely that's v9; "+++" is rather unusual.
>
> My intention was to put this into the next v7 series after I get this
> reviewed as RFC.
> ...
>> Together with removing:
>>
>> > +   const char      *cells_name;
>> > +   int             cell_count;
>>
>> ... then you'd only be left with cur/end, so I think you could get away
>> without a struct at all, but simply "cur" as the iterator variable, plus
>> "end" as the one temp variable.
>
> Although the above proposal would be alsmot same as "[RFC][PATCHv6++
> 01/13]"(*1) where I use *list(cur) and rem as remaining count, here's
> the update. I'll put this into v7 series.
>
> ---8<------8<------8<------8<------8<------8<------8<------8<------8<---
> From: Hiroshi Doyu <hdoyu@nvidia.com>
>
> 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.
>
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> ---
> v6++++:
> Iterate without intrducing a new struct.
>
> v6+++:
> Introduced a new struct "of_phandle_iter" to keep the state when
> iterating over the list.
>
> v6++:
> Optimized to avoid O(n^2), suggested by Stephen Warren.
> http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007066.html
>
> I didn't introduce any struct to hold params and state here.
>
> v6+:
> Use the description, which Grant Likely proposed, to be full enough
> that a future reader can figure out why a patch was written.
>
> v5:
> New patch for v5.
>
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> ---
>  drivers/of/base.c  | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of.h | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 87 insertions(+)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index f807d0e..7501f24 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1201,6 +1201,59 @@ void of_print_phandle_args(const char *msg, const struct of_phandle_args *args)
>         printk("\n");
>  }
>
> +const __be32 *of_phandle_iter_next(const char *cells_name, int cell_count,
> +                                  const __be32 *cur, const __be32 *end,
> +                                  struct of_phandle_args *out_args)
> +{
> +       phandle phandle;
> +       struct device_node *dn;
> +       int i;
> +
> +       if (!cells_name && !cell_count)
> +               return NULL;
> +
> +       if (!cur)
> +               return NULL;
> +
> +       if (end - cur <= 0)

These 2 if's can be combined: if (!cur || (end - cur <= 0))

> +               return NULL;
> +
> +       phandle = be32_to_cpup(cur++);
> +       if (!phandle)
> +               return NULL;

Won't of_find_node_by_phandle do this check?

> +
> +       dn = of_find_node_by_phandle(phandle);
> +       if (!dn)
> +               return NULL;
> +
> +       if (cells_name)
> +               if (of_property_read_u32(dn, cells_name, &cell_count))
> +                       return NULL;
> +
> +       out_args->np = dn;
> +       out_args->args_count = cell_count;
> +       for (i = 0; i < cell_count; i++)
> +               out_args->args[i] = be32_to_cpup(cur++);
> +
> +       return cur;
> +}
> +EXPORT_SYMBOL_GPL(of_phandle_iter_next);
> +
> +const __be32 *of_phandle_iter_init(const struct device_node *np,
> +                                  const char *list_name,
> +                                  const __be32 **end)
> +{
> +       size_t bytes;
> +       const __be32 *cur;
> +
> +       cur = of_get_property(np, list_name, &bytes);
> +       if (bytes)
> +               *end = cur + bytes / sizeof(*cur);
> +
> +       return cur;
> +}
> +EXPORT_SYMBOL_GPL(of_phandle_iter_init);
> +
>  static int __of_parse_phandle_with_args(const struct device_node *np,
>                                         const char *list_name,
>                                         const char *cells_name,
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 276c546..c23710b 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -303,6 +303,14 @@ extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
>  extern int of_count_phandle_with_args(const struct device_node *np,
>         const char *list_name, const char *cells_name);
>
> +extern const __be32 *of_phandle_iter_init(const struct device_node *np,
> +                                         const char *list_name,
> +                                         const __be32 **end);
> +extern const __be32 *of_phandle_iter_next(const char *cells_name,
> +                                         int cell_count,
> +                                         const __be32 *cur, const __be32 *end,
> +                                         struct of_phandle_args *out_args);
> +
>  extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
>  extern int of_alias_get_id(struct device_node *np, const char *stem);
>
> @@ -527,6 +535,24 @@ static inline int of_count_phandle_with_args(struct device_node *np,
>         return -ENOSYS;
>  }
>
> +static inline const __be32 *of_phandle_iter_init(const struct device_node *np,
> +                                                const char *list_name,
> +                                                const char *cells_name,
> +                                                const int cell_count,
> +                                                struct of_phandle_args *out_args)

Need to update the arguments.

> +{
> +       return NULL;
> +}
> +
> +static inline const __be32 *of_phandle_iter_next(const char *cells_name,
> +                                                int cell_count,
> +                                                const __be32 *cur,
> +                                                const __be32 *end,
> +                                                struct of_phandle_args *out_args);
> +{
> +       return NULL;
> +}
> +
>  static inline int of_alias_get_id(struct device_node *np, const char *stem)
>  {
>         return -ENOSYS;
> @@ -613,6 +639,14 @@ 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(node, list_name,        cells_name, \

Some extra whitespace here.

> +                                              cell_count, out_args, cur, end) \
> +       for (cur = of_phandle_iter_init(node, list_name, &end),         \
> +                    cur = of_phandle_iter_next(cells_name, cell_count, \
> +                                               cur, end, &out_args);   \
> +            cur;                                                       \
> +            cur = of_phandle_iter_next(cells_name, cell_count, cur, end, &out_args))
> +
>  #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
>
>
>
>
>
> *1:
>  http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007086.html

Re: [RFC][PATCHv6+++ 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Hiroshi Doyu]

On Mon, 2 Dec 2013 15:39:25 +0100
Rob Herring <robherring2@gmail.com> wrote:

> On Mon, Dec 2, 2013 at 5:02 AM, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> > Stephen Warren <swarren@wwwdotorg.org> wrote @ Sun, 1 Dec 2013 20:00:09 +0100:
> >
> >> On 11/29/2013 04:46 AM, Hiroshi Doyu wrote:
> >> ...
> >> > 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.
> >> >
> >> > Introduced a new struct "of_phandle_iter" to keep the state when
> >> > iterating over the list.
> >> >
> >> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
....

Rob, thank you for review.
I'll fix them and put this one into the next v7 series.

Re: [RFC][PATCHv6+++ 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Stephen Warren]

On 12/02/2013 04:02 AM, Hiroshi Doyu wrote:
...
> 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.

> diff --git a/drivers/of/base.c b/drivers/of/base.c

> +const __be32 *of_phandle_iter_init(const struct device_node *np,
> +				   const char *list_name,
> +				   const __be32 **end)
> +{
> +	size_t bytes;
> +	const __be32 *cur;
> +
> +	cur = of_get_property(np, list_name, &bytes);
> +	if (bytes)
> +		*end = cur + bytes / sizeof(*cur);
> +
> +	return cur;
> +}

"bytes" doesn't seem like the correct thing to check in that if
statement. The property might exist but be zero length. Perhaps if (cur)
would be better, or just initializing *end in all cases (the value won't
be used if (!cur), so setting *end to a bogus value in that case won't
matter).

> +const __be32 *of_phandle_iter_next(const char *cells_name, int cell_count,
> +				   const __be32 *cur, const __be32 *end,
> +				   struct of_phandle_args *out_args)
...
> +	if (!cur)
> +		return NULL;
> +
> +	if (end - cur <= 0)
> +		return NULL;

Related, don't you want if (cur >= end) there; just compare the pointers
directly rather than explicitly calculating and testing the difference?

Re: [PATCHv6 05/13] iommu/core: add ops->{bound,unbind}_driver()

[Hiroshi Doyu]

On Mon, 25 Nov 2013 14:49:37 +0100
Hiroshi Doyu <hdoyu@nvidia.com> wrote:

> Hi Joerg,
> 
> Do you have some time to review this patch along with the following ones?
> 
>   [PATCHv6 02/13] iommu/of: introduce a global iommu device list
>   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007050.html
> 
>   [PATCHv6 03/13] iommu/of: check if dependee iommu is ready or not
>   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007051.html

Any chance to get some feedback on them?

> With those patches, now I'm trying to populate iommu master devices
> after an IOMMU device is populated. Originally [PATCHv6 02/13] was
> proposed by Thierry. I'm a bit afraid of adding new IOMMU API as
> this, but I think that this new {bound,unbind}_driver() is the right
> timiing that depeding devices are populated instead of add_device()
> since, even after add_device, a device won't be populated. I'm not so
> sure how this affects on the existing IOMMUs.
> 
> It would be nice if you give some feedback on this.
> 
> On Thu, 21 Nov 2013 14:40:41 +0100
> Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> 
> > ops->{bound,unbind}_driver() functions are called at
> > BUS_NOTIFY_{BOUND,UNBIND}_DRIVER respectively.
> > 
> > This is necessary to control the device population order. IOMMU master
> > devices depend on an IOMMU device instanciation. IOMMU master devices
> > can be registered to an IOMMU only after it's successfully
> > populated. This IOMMU registration is done via
> > ops->bound_driver(). Currently this population can be deferred if
> > depending IOMMU device hasn't yet been populated in driver core. This
> > cannot be done via ops->add_device() since after add_device() device's
> > population/instanciation can be still deferred via probe().
> > 
> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > ---
> > v6:
> > New for v6.
> > ---
> >  drivers/iommu/iommu.c | 13 +++++++++++--
> >  include/linux/iommu.h |  4 ++++
> >  2 files changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index e5555fc..5469d36 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -540,14 +540,23 @@ static int iommu_bus_notifier(struct notifier_block *nb,
> >  	 * ADD/DEL call into iommu driver ops if provided, which may
> >  	 * result in ADD/DEL notifiers to group->notifier
> >  	 */
> > -	if (action == BUS_NOTIFY_ADD_DEVICE) {
> > +	switch (action) {
> > +	case BUS_NOTIFY_ADD_DEVICE:
> >  		if (ops->add_device)
> >  			return ops->add_device(dev);
> > -	} else if (action == BUS_NOTIFY_DEL_DEVICE) {
> > +	case BUS_NOTIFY_DEL_DEVICE:
> >  		if (ops->remove_device && dev->iommu_group) {
> >  			ops->remove_device(dev);
> >  			return 0;
> >  		}
> > +	case BUS_NOTIFY_BOUND_DRIVER:
> > +		if (ops->bound_driver)
> > +			ops->bound_driver(dev);
> > +		break;
> > +	case BUS_NOTIFY_UNBIND_DRIVER:
> > +		if (ops->unbind_driver)
> > +			ops->unbind_driver(dev);
> > +		break;
> >  	}
> >  
> >  	/*
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index a444c79..a0e92be 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -96,6 +96,8 @@ enum iommu_attr {
> >   * @domain_has_cap: domain capabilities query
> >   * @add_device: add device to iommu grouping
> >   * @remove_device: remove device from iommu grouping
> > + * @bound_driver: called at BUS_NOTIFY_BOUND_DRIVER
> > + * @unbind_driver: called at BUS_NOTIFY_UNBIND_DRIVER
> >   * @domain_get_attr: Query domain attributes
> >   * @domain_set_attr: Change domain attributes
> >   * @pgsize_bitmap: bitmap of supported page sizes
> > @@ -114,6 +116,8 @@ struct iommu_ops {
> >  			      unsigned long cap);
> >  	int (*add_device)(struct device *dev);
> >  	void (*remove_device)(struct device *dev);
> > +	int (*bound_driver)(struct device *dev);
> > +	void (*unbind_driver)(struct device *dev);
> >  	int (*device_group)(struct device *dev, unsigned int *groupid);
> >  	int (*domain_get_attr)(struct iommu_domain *domain,
> >  			       enum iommu_attr attr, void *data);
> > -- 
> > 1.8.1.5
> > 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCHv6 05/13] iommu/core: add ops->{bound,unbind}_driver()

[Will Deacon]

Hi Hiroshi,

On Wed, Dec 04, 2013 at 07:40:27AM +0000, Hiroshi Doyu wrote:
> On Mon, 25 Nov 2013 14:49:37 +0100
> Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> 
> > Hi Joerg,
> > 
> > Do you have some time to review this patch along with the following ones?
> > 
> >   [PATCHv6 02/13] iommu/of: introduce a global iommu device list
> >   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007050.html
> > 
> >   [PATCHv6 03/13] iommu/of: check if dependee iommu is ready or not
> >   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007051.html
> 
> Any chance to get some feedback on them?

Joerg is on holiday at the moment, but Alex Williamson is serving as interim
IOMMU maintainer at the moment. Adding him to CC, but this probably needs an
Ack from Joerg regardless, so you will need to wait for him to come back.

Will

Re: [PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Grant Likely]

On Thu, 21 Nov 2013 18:17:20 +0100, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> 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.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
Acked-by: Grant Likely <grant.likely@linaro.org>

This patch can be merged with the rest of the series.

g.

> ---
> v6+:
> Use the description, which Grant Likely proposed, to be full enough
> that a future reader can figure out why a patch was written.
>   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007062.html
> 
> 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

Re: [PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Grant Likely]

On Thu, 21 Nov 2013 11:57:00 -0700, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 11/21/2013 10:17 AM, Hiroshi Doyu wrote:
> > 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.
> > 
> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > ---
> > v6+:
> > Use the description, which Grant Likely proposed, to be full enough
> > that a future reader can figure out why a patch was written.
> >   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007062.html
> 
> This new version only addresses one of the concerns that Grant had,
> namely the commit message.
> 
> > diff --git a/include/linux/of.h b/include/linux/of.h
> 
> > +#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++)
> > +
> 
> Grant also wanted the actual implementation fixed so that it wasn't so
> inefficient.
> 
> What this current patch does is basically:
> 
> for every entry in the property:
>     for every entry in the property before the current index:
>         parse the phandle+specifier
> 
> That's roughly O(n^2). (n is # entries in the property)
> 
> Instead, what should happen is:
> 
> for every entry in the property:
>     parse the phandle+specifier
>     yield the result
> 
> That's roughly O(n).
> 
> In other words, an implementation more along the lines of
> include/linux/of.h's:
> 
> #define of_property_for_each_u32(np, propname, prop, p, u)      \
>         for (prop = of_find_property(np, propname, NULL),       \
>                 p = of_prop_next_u32(prop, NULL, &u);           \
>                 p;                                              \
>                 p = of_prop_next_u32(prop, p, &u))
> 
> ... so you'd need functions like of_prop_first_specifier() and
> of_prop_next_specifier(), and perhaps some associated set of state
> variables, perhaps with all the state wrapped into a single struct for
> simplicity.

That's right, I forgot I said that. Yes please fix the implementation.

g.

Re: [PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Hiroshi Doyu]

Hi Grant,

Grant Likely <grant.likely@linaro.org> wrote @ Wed, 11 Dec 2013 14:28:45 +0100:

> On Thu, 21 Nov 2013 11:57:00 -0700, Stephen Warren <swarren@wwwdotorg.org> wrote:
> > On 11/21/2013 10:17 AM, Hiroshi Doyu wrote:
> > > 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.
> > > 
> > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > > ---
> > > v6+:
> > > Use the description, which Grant Likely proposed, to be full enough
> > > that a future reader can figure out why a patch was written.
> > >   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007062.html
...

> That's right, I forgot I said that. Yes please fix the implementation.

Here's the latest. I'll include this with the next v7 series.

Can I get your Acked-by with this?

--8<----

>From 8f7c0404aa68f0e8dbe0babc240590f6528ecc1f Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <hdoyu@nvidia.com>
Date: Fri, 15 Nov 2013 10:52:53 +0200
Subject: [PATCH] of: introduce of_property_for_each_phandle_with_args()

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.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
Cc: Rob Herring <robherring2@gmail.com>
---
v7:
Fixed some minors pointed by Rob and Stephen.

v6++++:
Iterate without intrducing a new struct.

v6+++:
Introduced a new struct "of_phandle_iter" to keep the state when
iterating over the list.

v6++:
Optimized to avoid O(n^2), suggested by Stephen Warren.
http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007066.html

I didn't introduce any struct to hold params and state here.

v6+:
Use the description, which Grant Likely proposed, to be full enough
that a future reader can figure out why a patch was written.

v5:
New patch for v5.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 drivers/of/base.c  | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index f807d0e..cd4ab05 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1201,6 +1201,52 @@ void of_print_phandle_args(const char *msg, const struct of_phandle_args *args)
 	printk("\n");
 }
 
+const __be32 *of_phandle_iter_next(const char *cells_name, int cell_count,
+				   const __be32 *cur, const __be32 *end,
+				   struct of_phandle_args *out_args)
+{
+	struct device_node *dn;
+	int i;
+
+	if (!cells_name && !cell_count)
+		return NULL;
+
+	if (!cur || (cur >= end))
+		return NULL;
+
+	dn = of_find_node_by_phandle(be32_to_cpup(cur++));
+	if (!dn)
+		return NULL;
+
+	if (cells_name)
+		if (of_property_read_u32(dn, cells_name, &cell_count))
+			return NULL;
+
+	out_args->np = dn;
+	out_args->args_count = cell_count;
+	for (i = 0; i < cell_count; i++)
+		out_args->args[i] = be32_to_cpup(cur++);
+
+	return cur;
+}
+EXPORT_SYMBOL_GPL(of_phandle_iter_next);
+
+const __be32 *of_phandle_iter_init(const struct device_node *np,
+				   const char *list_name,
+				   const __be32 **end)
+{
+	size_t bytes;
+	const __be32 *cur;
+
+	cur = of_get_property(np, list_name, &bytes);
+	*end = cur;
+	if (bytes)
+		*end += bytes / sizeof(*cur);
+
+	return cur;
+}
+EXPORT_SYMBOL_GPL(of_phandle_iter_init);
+
 static int __of_parse_phandle_with_args(const struct device_node *np,
 					const char *list_name,
 					const char *cells_name,
diff --git a/include/linux/of.h b/include/linux/of.h
index 276c546..4345582 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -303,6 +303,14 @@ extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
 extern int of_count_phandle_with_args(const struct device_node *np,
 	const char *list_name, const char *cells_name);
 
+extern const __be32 *of_phandle_iter_init(const struct device_node *np,
+					  const char *list_name,
+					  const __be32 **end);
+extern const __be32 *of_phandle_iter_next(const char *cells_name,
+					  int cell_count,
+					  const __be32 *cur, const __be32 *end,
+					  struct of_phandle_args *out_args);
+
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
 
@@ -527,6 +535,22 @@ static inline int of_count_phandle_with_args(struct device_node *np,
 	return -ENOSYS;
 }
 
+static inline const __be32 *of_phandle_iter_init(const struct device_node *np,
+						 const char *list_name,
+						 const __be32 **end)
+{
+	return NULL;
+}
+
+static inline const __be32 *of_phandle_iter_next(const char *cells_name,
+						 int cell_count,
+						 const __be32 *cur,
+						 const __be32 *end,
+						 struct of_phandle_args *out_args);
+{
+	return NULL;
+}
+
 static inline int of_alias_get_id(struct device_node *np, const char *stem)
 {
 	return -ENOSYS;
@@ -613,6 +637,14 @@ 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(node, list_name, cells_name, \
+					       cell_count, out_args, cur, end) \
+	for (cur = of_phandle_iter_init(node, list_name, &end),		\
+		     cur = of_phandle_iter_next(cells_name, cell_count, \
+						cur, end, &out_args);	\
+	     cur;							\
+	     cur = of_phandle_iter_next(cells_name, cell_count, cur, end, &out_args))
+
 #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

Re: [PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Grant Likely]

On Wed, 11 Dec 2013 14:33:38 +0100, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> Hi Grant,
> 
> Grant Likely <grant.likely@linaro.org> wrote @ Wed, 11 Dec 2013 14:28:45 +0100:
> 
> > On Thu, 21 Nov 2013 11:57:00 -0700, Stephen Warren <swarren@wwwdotorg.org> wrote:
> > > On 11/21/2013 10:17 AM, Hiroshi Doyu wrote:
> > > > 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.
> > > > 
> > > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > > > ---
> > > > v6+:
> > > > Use the description, which Grant Likely proposed, to be full enough
> > > > that a future reader can figure out why a patch was written.
> > > >   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007062.html
> ...
> 
> > That's right, I forgot I said that. Yes please fix the implementation.
> 
> Here's the latest. I'll include this with the next v7 series.
> 
> Can I get your Acked-by with this?
> 
> --8<----
> 
> From 8f7c0404aa68f0e8dbe0babc240590f6528ecc1f Mon Sep 17 00:00:00 2001
> From: Hiroshi Doyu <hdoyu@nvidia.com>
> Date: Fri, 15 Nov 2013 10:52:53 +0200
> Subject: [PATCH] of: introduce of_property_for_each_phandle_with_args()
> 
> 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.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> Cc: Rob Herring <robherring2@gmail.com>
> ---
> v7:
> Fixed some minors pointed by Rob and Stephen.
> 
> v6++++:
> Iterate without intrducing a new struct.
> 
> v6+++:
> Introduced a new struct "of_phandle_iter" to keep the state when
> iterating over the list.
> 
> v6++:
> Optimized to avoid O(n^2), suggested by Stephen Warren.
> http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007066.html
> 
> I didn't introduce any struct to hold params and state here.
> 
> v6+:
> Use the description, which Grant Likely proposed, to be full enough
> that a future reader can figure out why a patch was written.
> 
> v5:
> New patch for v5.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> ---
>  drivers/of/base.c  | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of.h | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 78 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index f807d0e..cd4ab05 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1201,6 +1201,52 @@ void of_print_phandle_args(const char *msg, const struct of_phandle_args *args)
>  	printk("\n");
>  }
>  
> +const __be32 *of_phandle_iter_next(const char *cells_name, int cell_count,
> +				   const __be32 *cur, const __be32 *end,
> +				   struct of_phandle_args *out_args)

Having to pass in cells_name, cell_count, cur and end each time seems a
little odd. Can a state structure be used instead?

struct of_phandle_iter_state {
	const char *cells_name;
	int cells_count;
	const __be32 *cur;
	const __be32 *end;
	struct of_phandle_args out_args;
}

Make the caller provide one of those and fill it in with the init
function.

> +{
> +	struct device_node *dn;
> +	int i;
> +
> +	if (!cells_name && !cell_count)
> +		return NULL;
> +
> +	if (!cur || (cur >= end))
> +		return NULL;
> +
> +	dn = of_find_node_by_phandle(be32_to_cpup(cur++));
> +	if (!dn)
> +		return NULL;
> +
> +	if (cells_name)
> +		if (of_property_read_u32(dn, cells_name, &cell_count))
> +			return NULL;
> +
> +	out_args->np = dn;
> +	out_args->args_count = cell_count;
> +	for (i = 0; i < cell_count; i++)
> +		out_args->args[i] = be32_to_cpup(cur++);
> +
> +	return cur;
> +}
> +EXPORT_SYMBOL_GPL(of_phandle_iter_next);
> +
> +const __be32 *of_phandle_iter_init(const struct device_node *np,
> +				   const char *list_name,
> +				   const __be32 **end)
> +{
> +	size_t bytes;
> +	const __be32 *cur;
> +
> +	cur = of_get_property(np, list_name, &bytes);
> +	*end = cur;
> +	if (bytes)
> +		*end += bytes / sizeof(*cur);
> +
> +	return cur;
> +}
> +EXPORT_SYMBOL_GPL(of_phandle_iter_init);
> +
>  static int __of_parse_phandle_with_args(const struct device_node *np,
>  					const char *list_name,
>  					const char *cells_name,
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 276c546..4345582 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -303,6 +303,14 @@ extern int of_parse_phandle_with_fixed_args(const struct device_node *np,
>  extern int of_count_phandle_with_args(const struct device_node *np,
>  	const char *list_name, const char *cells_name);
>  
> +extern const __be32 *of_phandle_iter_init(const struct device_node *np,
> +					  const char *list_name,
> +					  const __be32 **end);
> +extern const __be32 *of_phandle_iter_next(const char *cells_name,
> +					  int cell_count,
> +					  const __be32 *cur, const __be32 *end,
> +					  struct of_phandle_args *out_args);
> +
>  extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
>  extern int of_alias_get_id(struct device_node *np, const char *stem);
>  
> @@ -527,6 +535,22 @@ static inline int of_count_phandle_with_args(struct device_node *np,
>  	return -ENOSYS;
>  }
>  
> +static inline const __be32 *of_phandle_iter_init(const struct device_node *np,
> +						 const char *list_name,
> +						 const __be32 **end)
> +{
> +	return NULL;
> +}
> +
> +static inline const __be32 *of_phandle_iter_next(const char *cells_name,
> +						 int cell_count,
> +						 const __be32 *cur,
> +						 const __be32 *end,
> +						 struct of_phandle_args *out_args);
> +{
> +	return NULL;
> +}
> +
>  static inline int of_alias_get_id(struct device_node *np, const char *stem)
>  {
>  	return -ENOSYS;
> @@ -613,6 +637,14 @@ 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(node, list_name, cells_name, \
> +					       cell_count, out_args, cur, end) \
> +	for (cur = of_phandle_iter_init(node, list_name, &end),		\
> +		     cur = of_phandle_iter_next(cells_name, cell_count, \
> +						cur, end, &out_args);	\

The above construct is a little odd. Why wouldn't the initializer
provide the first element (or NULL if empty) right at the start. That in
combination with the suggestion I made above would change the macro to
be:

#define of_property_for_each_phandle_with_args(node, list_name, cells_name, \
					       cell_count, &iter_state) \
	for (cur = of_phandle_iter_init(node, list_name, cells_name, \
					cells_count, &iter_state); \
	    cur; cur = of_phandle_iter_next(&iter_state)) \

Simpler, right? It also means whatever the user passed in for
cells_name, cell_count won't get evaluated every time through the loop.

g.

> +	     cur;							\
> +	     cur = of_phandle_iter_next(cells_name, cell_count, cur, end, &out_args))
> +
>  #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

Re: [PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Hiroshi Doyu]

Grant Likely <grant.likely@linaro.org> wrote @ Thu, 12 Dec 2013 12:34:17 +0100:

> On Wed, 11 Dec 2013 14:33:38 +0100, Hiroshi Doyu <hdoyu@nvidia.com> wrote:
> > Hi Grant,
> >
> > Grant Likely <grant.likely@linaro.org> wrote @ Wed, 11 Dec 2013 14:28:45 +0100:
> >
> > > On Thu, 21 Nov 2013 11:57:00 -0700, Stephen Warren <swarren@wwwdotorg.org> wrote:
> > > > On 11/21/2013 10:17 AM, Hiroshi Doyu wrote:
> > > > > 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.
> > > > >
> > > > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > > > > ---
> > > > > v6+:
> > > > > Use the description, which Grant Likely proposed, to be full enough
> > > > > that a future reader can figure out why a patch was written.
> > > > >   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007062.html
> > ...
> >
> > > That's right, I forgot I said that. Yes please fix the implementation.
> >
> > Here's the latest. I'll include this with the next v7 series.
> >
> > Can I get your Acked-by with this?
> >
> > --8<----
> >
> > From 8f7c0404aa68f0e8dbe0babc240590f6528ecc1f Mon Sep 17 00:00:00 2001
> > From: Hiroshi Doyu <hdoyu@nvidia.com>
> > Date: Fri, 15 Nov 2013 10:52:53 +0200
> > Subject: [PATCH] of: introduce of_property_for_each_phandle_with_args()
> >
> > 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.
> >
> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > Cc: Rob Herring <robherring2@gmail.com>
> > ---
> > v7:
> > Fixed some minors pointed by Rob and Stephen.
> >
> > v6++++:
> > Iterate without intrducing a new struct.
> >
> > v6+++:
> > Introduced a new struct "of_phandle_iter" to keep the state when
> > iterating over the list.
> >
> > v6++:
> > Optimized to avoid O(n^2), suggested by Stephen Warren.
> > http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007066.html
> >
> > I didn't introduce any struct to hold params and state here.
> >
> > v6+:
> > Use the description, which Grant Likely proposed, to be full enough
> > that a future reader can figure out why a patch was written.
> >
> > v5:
> > New patch for v5.
> >
> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > ---
> >  drivers/of/base.c  | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/of.h | 32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 78 insertions(+)
> >
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index f807d0e..cd4ab05 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -1201,6 +1201,52 @@ void of_print_phandle_args(const char *msg, const struct of_phandle_args *args)
> >       printk("\n");
> >  }
> >
> > +const __be32 *of_phandle_iter_next(const char *cells_name, int cell_count,
> > +                                const __be32 *cur, const __be32 *end,
> > +                                struct of_phandle_args *out_args)
> 
> Having to pass in cells_name, cell_count, cur and end each time seems a
> little odd. Can a state structure be used instead?
> 
> struct of_phandle_iter_state {
>         const char *cells_name;
>         int cells_count;
>         const __be32 *cur;
>         const __be32 *end;
>         struct of_phandle_args out_args;
> }
> 
> Make the caller provide one of those and fill it in with the init
> function.

I rewrote this a few times and so now I have a few version of this
implementations :-) The above proposal is similar to the version v6+++
mentioned in the above patch note:

> > v6+++:
> > Introduced a new struct "of_phandle_iter" to keep the state when
> > iterating over the list.

which is:

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

Stephen seemed to prefer the version without state struct. I like the
idea to not pass the same arguments repeatly. Instead, wrapping them
in a struct with state may look better.

So if Stephen agrees, I'll rewrite the version with state struct
again.

Re: [PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Hiroshi Doyu]

Hiroshi Doyu <hdoyu@nvidia.com> wrote @ Thu, 12 Dec 2013 14:14:04 +0200 (EET):

> > > From 8f7c0404aa68f0e8dbe0babc240590f6528ecc1f Mon Sep 17 00:00:00 2001
> > > From: Hiroshi Doyu <hdoyu@nvidia.com>
> > > Date: Fri, 15 Nov 2013 10:52:53 +0200
> > > Subject: [PATCH] of: introduce of_property_for_each_phandle_with_args()
> > >
> > > 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.
> > >
> > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > > Cc: Rob Herring <robherring2@gmail.com>
....
> > > ---
> > >  drivers/of/base.c  | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/of.h | 32 ++++++++++++++++++++++++++++++++
> > >  2 files changed, 78 insertions(+)
> > >
> > > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > > index f807d0e..cd4ab05 100644
> > > --- a/drivers/of/base.c
> > > +++ b/drivers/of/base.c
> > > @@ -1201,6 +1201,52 @@ void of_print_phandle_args(const char *msg, const struct of_phandle_args *args)
> > >       printk("\n");
> > >  }
> > >
> > > +const __be32 *of_phandle_iter_next(const char *cells_name, int cell_count,
> > > +                                const __be32 *cur, const __be32 *end,
> > > +                                struct of_phandle_args *out_args)
> > 
> > Having to pass in cells_name, cell_count, cur and end each time seems a
> > little odd. Can a state structure be used instead?
> > 
> > struct of_phandle_iter_state {
> >         const char *cells_name;
> >         int cells_count;
> >         const __be32 *cur;
> >         const __be32 *end;
> >         struct of_phandle_args out_args;
> > }
> > 
> > Make the caller provide one of those and fill it in with the init
> > function.
> 
> I rewrote this a few times and so now I have a few version of this
> implementations :-) The above proposal is similar to the version v6+++
> mentioned in the above patch note:
> 
> > > v6+++:
> > > Introduced a new struct "of_phandle_iter" to keep the state when
> > > iterating over the list.
> 
> which is:
> 
>   [RFC][PATCHv6+++ 01/13] of: introduce of_property_for_earch_phandle_with_args()
>     http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007087.html
> 
> Stephen seemed to prefer the version without state struct. I like the
> idea to not pass the same arguments repeatly. Instead, wrapping them
> in a struct with state may look better.
> 
> So if Stephen agrees, I'll rewrite the version with state struct
> again.

Stephen, let me know what you think.

Re: [PATCHv6+ 01/13] of: introduce of_property_for_earch_phandle_with_args()

[Stephen Warren]

On 12/14/2013 08:51 AM, Hiroshi Doyu wrote:
> Hiroshi Doyu <hdoyu@nvidia.com> wrote @ Thu, 12 Dec 2013 14:14:04 +0200 (EET):
> 
>>>> From 8f7c0404aa68f0e8dbe0babc240590f6528ecc1f Mon Sep 17 00:00:00 2001
>>>> From: Hiroshi Doyu <hdoyu@nvidia.com>
>>>> Date: Fri, 15 Nov 2013 10:52:53 +0200
>>>> Subject: [PATCH] of: introduce of_property_for_each_phandle_with_args()
>>>>
>>>> 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.
>>>>
>>>> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
>>>> Cc: Rob Herring <robherring2@gmail.com>
> ....
>>>> ---
>>>>  drivers/of/base.c  | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/of.h | 32 ++++++++++++++++++++++++++++++++
>>>>  2 files changed, 78 insertions(+)
>>>>
>>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>>> index f807d0e..cd4ab05 100644
>>>> --- a/drivers/of/base.c
>>>> +++ b/drivers/of/base.c
>>>> @@ -1201,6 +1201,52 @@ void of_print_phandle_args(const char *msg, const struct of_phandle_args *args)
>>>>       printk("\n");
>>>>  }
>>>>
>>>> +const __be32 *of_phandle_iter_next(const char *cells_name, int cell_count,
>>>> +                                const __be32 *cur, const __be32 *end,
>>>> +                                struct of_phandle_args *out_args)
>>>
>>> Having to pass in cells_name, cell_count, cur and end each time seems a
>>> little odd. Can a state structure be used instead?
>>>
>>> struct of_phandle_iter_state {
>>>         const char *cells_name;
>>>         int cells_count;
>>>         const __be32 *cur;
>>>         const __be32 *end;
>>>         struct of_phandle_args out_args;
>>> }
>>>
>>> Make the caller provide one of those and fill it in with the init
>>> function.
>>
>> I rewrote this a few times and so now I have a few version of this
>> implementations :-) The above proposal is similar to the version v6+++
>> mentioned in the above patch note:
>>
>>>> v6+++:
>>>> Introduced a new struct "of_phandle_iter" to keep the state when
>>>> iterating over the list.
>>
>> which is:
>>
>>   [RFC][PATCHv6+++ 01/13] of: introduce of_property_for_earch_phandle_with_args()
>>     http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007087.html
>>
>> Stephen seemed to prefer the version without state struct. I like the
>> idea to not pass the same arguments repeatly. Instead, wrapping them
>> in a struct with state may look better.
>>
>> So if Stephen agrees, I'll rewrite the version with state struct
>> again.
> 
> Stephen, let me know what you think.

It's hard to follow there have been so many revisions. As long as the
result is reasonable, I'll be OK with it. IIRC, I objected to using a
state structure for values that weren't needed by both functions, but if
both functions need the values, that's fine. I suppose even if they
don't both need the values, putting them in a state structure is OK too
if it means the patch gets finished!