[PATCHv3 00/19] Unifying SMMU driver among Tegra SoCs

[PATCHv3 00/19] Unifying SMMU driver among Tegra SoCs

[Hiroshi Doyu]

Hi,

This series provides:

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

There's some discussion[1] about device population order, which could
solve the following patches.

  patch 1: [HACK] of: dev_node has struct device pointer
  patch 2: [HACK] ARM: tegra: Populate AHB/IOMMU earlier than others
  patch 3: [HACK] amba: Move AHB to core_initcall
  patch 4: [HACK] iommu/tegra: smmu: Move IOMMU to core_initcall

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

v3:
Updated based on Stephen Warren's feedback

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

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

Available in the git repository at:

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

Hiroshi Doyu (19):
  [HACK] of: dev_node has struct device pointer
  [HACK] ARM: tegra: Populate AHB/IOMMU earlier than others
  [HACK] amba: Move AHB to core_initcall
  [HACK] iommu/tegra: smmu: Move IOMMU to core_initcall
  ARM: dt: tegra114: iommu: Fix IOMMU register address
  iommu/tegra: smmu: Select ARM_DMA_USE_IOMMU in Kconfig
  iommu/tegra: smmu: Create default IOVA maps
  iommu/tegra: smmu: Register platform_device to IOMMU dynamically
  iommu/tegra: smmu: Calculate ASID register offset by ID
  iommu/tegra: smmu: Get "nvidia,swgroups" from DT
  ARM: dt: tegra30: iommu: Add "nvidia,swgroups"
  ARM: dt: tegra114: iommu: Add "nvidia,swgroups"
  iommu/tegra: smmu: Workaround PCIe IOMMU'able
  iommu/tegra: smmu: Get "nvidia,memory-clients" from DT
  ARM: tegra: Create a DT header defining SWGROUP ID
  iommu/tegra: smmu: Use dt-bindings MACRO
  ARM: dt: tegra30: iommu: Add "nvidia,memory-clients"
  ARM: dt: tegra114: iommu: Add "nvidia,memory-clients"
  iommu/tegra: smmu: Support Multiple ASID

 .../bindings/iommu/nvidia,tegra30-smmu.txt         |  22 ++-
 arch/arm/boot/dts/tegra114.dtsi                    |  15 +-
 arch/arm/boot/dts/tegra30.dtsi                     |  23 ++-
 arch/arm/mach-tegra/tegra.c                        |  22 +++
 drivers/amba/tegra-ahb.c                           |   7 +-
 drivers/iommu/Kconfig                              |   1 +
 drivers/iommu/tegra-smmu.c                         | 198 ++++++++++++---------
 drivers/of/platform.c                              |   6 +
 include/dt-bindings/memory/tegra-swgroup.h         |  46 +++++
 include/linux/of.h                                 |   1 +
 10 files changed, 253 insertions(+), 88 deletions(-)
 create mode 100644 include/dt-bindings/memory/tegra-swgroup.h

-- 
1.8.1.5

[PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Hiroshi Doyu]

To prevent of_platform_populate() from trying to populate duplicate
devices if a device has been already populated.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/of/platform.c | 6 ++++++
 include/linux/of.h    | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 9b439ac..994cea4 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -208,6 +208,11 @@ static struct platform_device *of_platform_device_create_pdata(
 	if (!of_device_is_available(np))
 		return NULL;
 
+	if (np->dev) {
+		dev_info(np->dev, "Already populated\n");
+		return to_platform_device(np->dev);
+	}
+
 	dev = of_device_alloc(np, bus_id, parent);
 	if (!dev)
 		return NULL;
@@ -232,6 +237,7 @@ static struct platform_device *of_platform_device_create_pdata(
 		return NULL;
 	}
 
+	np->dev = &dev->dev;
 	return dev;
 }
 
diff --git a/include/linux/of.h b/include/linux/of.h
index f95aee3..638a88a 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -60,6 +60,7 @@ struct device_node {
 	struct	kref kref;
 	unsigned long _flags;
 	void	*data;
+	struct device *dev;		/* Set only after populated */
 #if defined(CONFIG_SPARC)
 	const char *path_component_name;
 	unsigned int unique_id;
-- 
1.8.1.5

[PATCHv3 02/19] [HACK] ARM: tegra: Populate AHB/IOMMU earlier than others

[Hiroshi Doyu]

Populate AHB/IOMMU earlier than others. IOMMU depends on AHB. IOMMU
needs to be instanciated earlier than others so that IOMMU can
register other platform devices as IOMMU'able. Once IOMMU is
populated, IOMMU/AHB nodes are detached to prevent another
registeration.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-tegra/tegra.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm/mach-tegra/tegra.c b/arch/arm/mach-tegra/tegra.c
index 40b031c..9098a88 100644
--- a/arch/arm/mach-tegra/tegra.c
+++ b/arch/arm/mach-tegra/tegra.c
@@ -108,6 +108,26 @@ static void __init tegra_dt_init_irq(void)
 	tegra_legacy_irq_syscore_init();
 }
 
+static void tegra_of_platform_populate_iommu(void)
+{
+	int i;
+	struct platform_device *pdev;
+	const char * const path[] = {"/ahb", "/iommu", };
+
+	for (i = 0; i < ARRAY_SIZE(path); i++) {
+		struct device_node *np;
+
+		np  = of_find_node_by_path(path[i]);
+		if (!np)
+			break;
+
+		pdev = of_platform_device_create(np, NULL, NULL);
+		of_node_put(np);
+		if (!pdev)
+			break;
+	}
+}
+
 static void __init tegra_dt_init(void)
 {
 	struct soc_device_attribute *soc_dev_attr;
@@ -137,6 +157,8 @@ static void __init tegra_dt_init(void)
 
 	parent = soc_device_to_device(soc_dev);
 
+	tegra_of_platform_populate_iommu();
+
 	/*
 	 * Finished with the static registrations now; fill in the missing
 	 * devices
-- 
1.8.1.5

[PATCHv3 03/19] [HACK] amba: Move AHB to core_initcall

[Hiroshi Doyu]

Move AHB to core_initcall in order to instanciate this device earlier
than others since IOMMU depends on AHB and then IOMMU driver needs to
reigster other platform devices as IOMMU'able.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/amba/tegra-ahb.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/amba/tegra-ahb.c b/drivers/amba/tegra-ahb.c
index 1f44e56..08e898d 100644
--- a/drivers/amba/tegra-ahb.c
+++ b/drivers/amba/tegra-ahb.c
@@ -283,7 +283,12 @@ static struct platform_driver tegra_ahb_driver = {
 		.pm = &tegra_ahb_pm,
 	},
 };
-module_platform_driver(tegra_ahb_driver);
+
+static int tegra_ahb_init(void)
+{
+	return platform_driver_register(&tegra_ahb_driver);
+}
+core_initcall(tegra_ahb_init);
 
 MODULE_AUTHOR("Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
 MODULE_DESCRIPTION("Tegra AHB driver");
-- 
1.8.1.5

[PATCHv3 04/19] [HACK] iommu/tegra: smmu: Move IOMMU to core_initcall

[Hiroshi Doyu]

Move IOMMU to core_initcall in order to instanciate this device earlier
than others since IOMMU driver needs to reigster other platform
devices as IOMMU'able.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 34374b3..43af340 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -1286,7 +1286,7 @@ static void __exit tegra_smmu_exit(void)
 	platform_driver_unregister(&tegra_smmu_driver);
 }
 
-subsys_initcall(tegra_smmu_init);
+core_initcall(tegra_smmu_init);
 module_exit(tegra_smmu_exit);
 
 MODULE_DESCRIPTION("IOMMU API for SMMU in Tegra30");
-- 
1.8.1.5

[PATCHv3 05/19] ARM: dt: tegra114: iommu: Fix IOMMU register address

[Hiroshi Doyu]

Fix IOMMU register address.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra114.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
index 2905145..8d42787 100644
--- a/arch/arm/boot/dts/tegra114.dtsi
+++ b/arch/arm/boot/dts/tegra114.dtsi
@@ -318,9 +318,9 @@
 
 	iommu {
 		compatible = "nvidia,tegra114-smmu", "nvidia,tegra30-smmu";
-		reg = <0x7000f010 0x02c
-		       0x7000f1f0 0x010
-		       0x7000f228 0x074>;
+		reg = <0x70019010 0x02c
+		       0x700191f0 0x010
+		       0x70019228 0x074>;
 		nvidia,#asids = <4>;
 		dma-window = <0 0x40000000>;
 		nvidia,swgroups = <0x18659fe>;
-- 
1.8.1.5

[PATCHv3 06/19] iommu/tegra: smmu: Select ARM_DMA_USE_IOMMU in Kconfig

[Hiroshi Doyu]

SMMU expects ARM_DMA_USE_IOMMU to be enabled to use DMA mapping API by
default.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index fe302e3..bf03cd5 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
-- 
1.8.1.5

[PATCHv3 07/19] iommu/tegra: smmu: Create default IOVA maps

[Hiroshi Doyu]

Create default IOVA maps at boot-up which can be attached to devices.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 43af340..4671579 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -39,6 +39,7 @@
 
 #include <asm/page.h>
 #include <asm/cacheflush.h>
+#include <asm/dma-iommu.h>
 
 enum smmu_hwgrp {
 	HWGRP_AFI,
@@ -319,6 +320,8 @@ struct smmu_device {
 
 	struct device_node *ahb;
 
+	struct dma_iommu_mapping **map;
+
 	int		num_as;
 	struct smmu_as	as[0];		/* Run-time allocated array */
 };
@@ -1144,6 +1147,26 @@ static int tegra_smmu_resume(struct device *dev)
 	return err;
 }
 
+static int 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 (WARN_ON(IS_ERR(smmu->map[i])))
+			return -EINVAL;
+
+		dev_dbg(smmu->dev, "map:%p %08x-%08x\n",
+			smmu->map[i], base, base + size - 1);
+	}
+
+	return 0;
+}
+
 static int tegra_smmu_probe(struct platform_device *pdev)
 {
 	struct smmu_device *smmu;
@@ -1160,13 +1183,15 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	if (of_property_read_u32(dev->of_node, "nvidia,#asids", &asids))
 		return -ENODEV;
 
-	bytes = sizeof(*smmu) + asids * sizeof(*smmu->as);
+	bytes = sizeof(*smmu) + asids * (sizeof(*smmu->as) +
+					 sizeof(struct dma_iommu_mapping *));
 	smmu = devm_kzalloc(dev, bytes, GFP_KERNEL);
 	if (!smmu) {
 		dev_err(dev, "failed to allocate smmu_device\n");
 		return -ENOMEM;
 	}
 
+	smmu->map = (struct dma_iommu_mapping **)(smmu->as + asids);
 	smmu->nregs = pdev->num_resources;
 	smmu->regs = devm_kzalloc(dev, 2 * smmu->nregs * sizeof(*smmu->regs),
 				  GFP_KERNEL);
@@ -1236,7 +1261,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);
-	return 0;
+	return tegra_smmu_create_default_map(smmu);
 }
 
 static int tegra_smmu_remove(struct platform_device *pdev)
-- 
1.8.1.5

[PATCHv3 08/19] iommu/tegra: smmu: Register platform_device to IOMMU dynamically

[Hiroshi Doyu]

Register platform_devices to IOMMU dynamically via
ops->{add,remove}_device().

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 4671579..710c671 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -950,6 +950,39 @@ static void smmu_iommu_domain_destroy(struct iommu_domain *domain)
 	dev_dbg(smmu->dev, "smmu_as@%p\n", as);
 }
 
+/*
+ * ASID[0] for the system default
+ * ASID[1] for PPCS("AHB bus children"), which has SDMMC
+ * ASID[2][3].. open for drivers, first come, first served.
+ */
+enum {
+	SYSTEM_DEFAULT,
+	SYSTEM_PROTECTED,
+};
+
+static int smmu_iommu_add_device(struct device *dev)
+{
+	int err;
+	struct dma_iommu_mapping *map = smmu_handle->map[SYSTEM_DEFAULT];
+
+	if (!map)
+		goto out;
+
+	err = arm_iommu_attach_device(dev, map);
+	if (err)
+		return err;
+
+out:
+	dev_dbg(dev, "Attached to map %p\n", map);
+	return 0;
+}
+
+static void smmu_iommu_remove_device(struct device *dev)
+{
+	dev_dbg(dev, "Detaching from map %p\n", to_dma_iommu_mapping(dev));
+	arm_iommu_detach_device(dev);
+}
+
 static struct iommu_ops smmu_iommu_ops = {
 	.domain_init	= smmu_iommu_domain_init,
 	.domain_destroy	= smmu_iommu_domain_destroy,
@@ -959,6 +992,8 @@ static struct iommu_ops smmu_iommu_ops = {
 	.unmap		= smmu_iommu_unmap,
 	.iova_to_phys	= smmu_iommu_iova_to_phys,
 	.domain_has_cap	= smmu_iommu_domain_has_cap,
+	.add_device	= smmu_iommu_add_device,
+	.remove_device	= smmu_iommu_remove_device,
 	.pgsize_bitmap	= SMMU_IOMMU_PGSIZES,
 };
 
-- 
1.8.1.5

[PATCHv3 09/19] iommu/tegra: smmu: Calculate ASID register offset by ID

[Hiroshi Doyu]

Calculate ASID register offset by ID so that we can get rid of SoC
specific MACROs. This is needed for the unified SMMU driver over Tegra
SoCs.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 710c671..4dcdd6c 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -238,25 +238,7 @@ enum {
 
 #define HWGRP_INIT(client) [HWGRP_##client] = SMMU_##client##_ASID
 
-static const u32 smmu_hwgrp_asid_reg[] = {
-	HWGRP_INIT(AFI),
-	HWGRP_INIT(AVPC),
-	HWGRP_INIT(DC),
-	HWGRP_INIT(DCB),
-	HWGRP_INIT(EPP),
-	HWGRP_INIT(G2),
-	HWGRP_INIT(HC),
-	HWGRP_INIT(HDA),
-	HWGRP_INIT(ISP),
-	HWGRP_INIT(MPE),
-	HWGRP_INIT(NV),
-	HWGRP_INIT(NV2),
-	HWGRP_INIT(PPCS),
-	HWGRP_INIT(SATA),
-	HWGRP_INIT(VDE),
-	HWGRP_INIT(VI),
-};
-#define HWGRP_ASID_REG(x) (smmu_hwgrp_asid_reg[x])
+#define HWGRP_ASID_REG(x) ((x) * sizeof(u32) + SMMU_AFI_ASID)
 
 /*
  * Per client for address space
-- 
1.8.1.5

[PATCHv3 10/19] iommu/tegra: smmu: Get "nvidia,swgroups" from DT

[Hiroshi Doyu]

This provides the info about which H/W Accelerators are supported on
Tegra SoC. This info is passed from DT. This is necessary to have the
unified SMMU driver among Tegra SoCs. Instead of using platform data,
DT passes "nvidia,swgroups" now. DT is mandatory in Tegra.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../bindings/iommu/nvidia,tegra30-smmu.txt         |  6 +++++
 drivers/iommu/tegra-smmu.c                         | 31 ++++++++++------------
 2 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
index 89fb543..6a844b3 100644
--- a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
@@ -8,6 +8,11 @@ Required properties:
 - nvidia,#asids : # of ASIDs
 - dma-window : IOVA start address and length.
 - nvidia,ahb : phandle to the ahb bus connected to SMMU.
+- nvidia,swgroups: A bitmap of supported HardWare Accelerators(HWA).
+  Each bit represents one swgroup. The assignments may be found in header
+  file <dt-bindings/memory/tegra-swgroup.h>. Its max is 64. 2 cells
+  are required. This unique ID info can be used to calculate
+  MC_SMMU_<SWGROUP name>_ASID_0 offset and HOTRESET bit.
 
 Example:
 	smmu {
@@ -17,5 +22,6 @@ Example:
 		       0x7000f228 0x05c>;
 		nvidia,#asids = <4>;		/* # of ASIDs */
 		dma-window = <0 0x40000000>;	/* IOVA start & length */
+		nvidia,swgroups = <0x00000000 0x000779ff>;
 		nvidia,ahb = <&ahb>;
 	};
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 4dcdd6c..22951e6 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -247,7 +247,7 @@ struct smmu_client {
 	struct device		*dev;
 	struct list_head	list;
 	struct smmu_as		*as;
-	u32			hwgrp;
+	u64			hwgrp;
 };
 
 /*
@@ -289,6 +289,8 @@ struct smmu_device {
 	struct device	*dev;
 	struct page *avp_vector_page;	/* dummy page shared by all AS's */
 
+	u64 swgroups;			/* swgroup ID bitmap */
+
 	/*
 	 * Register image savers for suspend/resume
 	 */
@@ -364,15 +366,16 @@ 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)
+#define smmu_client_hwgrp(c)	(c->as->smmu->swgroups)
 
 static int __smmu_client_set_hwgrp(struct smmu_client *c,
-				   unsigned long map, int on)
+				   u64 map, int on)
 {
 	int i;
 	struct smmu_as *as = c->as;
 	u32 val, offs, mask = SMMU_ASID_ENABLE(as->asid);
 	struct smmu_device *smmu = as->smmu;
+	unsigned long *bitmap = (unsigned long *)&map;
 
 	WARN_ON(!on && map);
 	if (on && !map)
@@ -380,12 +383,10 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,
 	if (!on)
 		map = smmu_client_hwgrp(c);
 
-	for_each_set_bit(i, &map, HWGRP_COUNT) {
+	for_each_set_bit(i, bitmap, sizeof(map) * BITS_PER_BYTE) {
 		offs = HWGRP_ASID_REG(i);
 		val = smmu_read(smmu, offs);
 		if (on) {
-			if (WARN_ON(val & mask))
-				goto err_hw_busy;
 			val |= mask;
 		} else {
 			WARN_ON((val & mask) == mask);
@@ -396,15 +397,6 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,
 	FLUSH_SMMU_REGS(smmu);
 	c->hwgrp = map;
 	return 0;
-
-err_hw_busy:
-	for_each_set_bit(i, &map, HWGRP_COUNT) {
-		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, u32 map, int on)
@@ -776,7 +768,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;
+	u64 map;
 	int err;
 
 	client = devm_kzalloc(smmu->dev, sizeof(*c), GFP_KERNEL);
@@ -784,7 +776,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 = smmu->swgroups;
 	if (!map)
 		return -EINVAL;
 
@@ -1191,6 +1183,7 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	int i, asids, err = 0;
 	dma_addr_t uninitialized_var(base);
 	size_t bytes, uninitialized_var(size);
+	u64 swgroups;
 
 	if (smmu_handle)
 		return -EIO;
@@ -1200,6 +1193,9 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	if (of_property_read_u32(dev->of_node, "nvidia,#asids", &asids))
 		return -ENODEV;
 
+	if (of_property_read_u64(dev->of_node, "nvidia,swgroups", &swgroups))
+		return -ENODEV;
+
 	bytes = sizeof(*smmu) + asids * (sizeof(*smmu->as) +
 					 sizeof(struct dma_iommu_mapping *));
 	smmu = devm_kzalloc(dev, bytes, GFP_KERNEL);
@@ -1246,6 +1242,7 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 	smmu->num_as = asids;
 	smmu->iovmm_base = base;
 	smmu->page_count = size;
+	smmu->swgroups = swgroups;
 
 	smmu->translation_enable_0 = ~0;
 	smmu->translation_enable_1 = ~0;
-- 
1.8.1.5

[PATCHv3 11/19] ARM: dt: tegra30: iommu: Add "nvidia,swgroups"

[Hiroshi Doyu]

This is a bitmap that indicates which HardWare Accelerators(HWA) are
supported on Tegra30 SoC.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra30.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 0022c12..dfd3ca9 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -531,6 +531,7 @@
 		       0x7000f228 0x05c>;
 		nvidia,#asids = <4>;		/* # of ASIDs */
 		dma-window = <0 0x40000000>;	/* IOVA start & length */
+		nvidia,swgroups = <0x00000000 0x000779ff>;
 		nvidia,ahb = <&ahb>;
 	};
 
-- 
1.8.1.5

[PATCHv3 12/19] ARM: dt: tegra114: iommu: Add "nvidia,swgroups"

[Hiroshi Doyu]

This is a bitmap that indicates which HardWare Accelerators(HWA) are
supported on Tegra114 SoC.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra114.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
index 8d42787..4626e8a 100644
--- a/arch/arm/boot/dts/tegra114.dtsi
+++ b/arch/arm/boot/dts/tegra114.dtsi
@@ -323,7 +323,7 @@
 		       0x70019228 0x074>;
 		nvidia,#asids = <4>;
 		dma-window = <0 0x40000000>;
-		nvidia,swgroups = <0x18659fe>;
+		nvidia,swgroups = <0x00000000 0x01b659fe>;
 		nvidia,ahb = <&ahb>;
 	};
 
-- 
1.8.1.5

[PATCHv3 13/19] iommu/tegra: smmu: Workaround PCIe IOMMU'able

[Hiroshi Doyu]

Make PCIe work as it is. IOMMU support can be implemented later. We
need the same kind of registration framework for PCIe as platform_bus
does currently.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 22951e6..f0ff48e 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -384,6 +384,9 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,
 		map = smmu_client_hwgrp(c);
 
 	for_each_set_bit(i, bitmap, sizeof(map) * BITS_PER_BYTE) {
+		if (i == TEGRA_SWGROUP_AFI) /* FIXME: IOMMU'able PCIe */
+			continue;
+
 		offs = HWGRP_ASID_REG(i);
 		val = smmu_read(smmu, offs);
 		if (on) {
-- 
1.8.1.5

[PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../bindings/iommu/nvidia,tegra30-smmu.txt         | 16 ++++++++++++-
 drivers/iommu/tegra-smmu.c                         | 27 ++++++++++++++++++++--
 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
index 6a844b3..7456104 100644
--- a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
@@ -13,9 +13,12 @@ Required properties:
   file <dt-bindings/memory/tegra-swgroup.h>. Its max is 64. 2 cells
   are required. This unique ID info can be used to calculate
   MC_SMMU_<SWGROUP name>_ASID_0 offset and HOTRESET bit.
+- nvidia,memory-clients: phandle to a smmu device which a device is
+  attached to and indicates which swgroups a device belongs to(SWGROUP ID).
+  SWGROUP ID is from 0 to 63, and a device can belong to multiple SWGROUPS.
 
 Example:
-	smmu {
+	smmu: iommu {
 		compatible = "nvidia,tegra30-smmu";
 		reg = <0x7000f010 0x02c
 		       0x7000f1f0 0x010
@@ -25,3 +28,14 @@ Example:
 		nvidia,swgroups = <0x00000000 0x000779ff>;
 		nvidia,ahb = <&ahb>;
 	};
+
+	host1x {
+		compatible = "nvidia,tegra30-host1x", "simple-bus";
+		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_HC>;
+		....
+		gr3d {
+			compatible = "nvidia,tegra30-gr3d";
+			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_NV
+						       TEGRA_SWGROUP_NV2>;
+			....
+		};
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index f0ff48e..1db9e89 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -368,6 +368,29 @@ static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs)
 
 #define smmu_client_hwgrp(c)	(c->as->smmu->swgroups)
 
+static u64 smmu_of_get_memory_client(struct device *dev)
+{
+	size_t bytes;
+	const char *propname = "nvidia,memory-clients";
+	const __be32 *prop;
+	int i;
+	u64 swgroup = 0;
+	struct device_node *np;
+
+	np = of_parse_phandle(dev->of_node, propname, 0);
+	if (np != smmu_handle->dev->of_node)
+		return ~0;
+
+	prop = of_get_property(dev->of_node, propname, &bytes);
+	if (!prop || !bytes)
+		return ~0;
+
+	for (i = 1; i < bytes / sizeof(u32); i++, prop++)
+		swgroup |= 1ULL << be32_to_cpup(prop);
+
+	return swgroup;
+}
+
 static int __smmu_client_set_hwgrp(struct smmu_client *c,
 				   u64 map, int on)
 {
@@ -779,8 +802,8 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 		return -ENOMEM;
 	client->dev = dev;
 	client->as = as;
-	map = smmu->swgroups;
-	if (!map)
+	map = smmu_of_get_memory_client(dev);
+	if (map == ~0)
 		return -EINVAL;
 
 	err = smmu_client_enable_hwgrp(client, map);
-- 
1.8.1.5

[PATCHv3 15/19] 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. "swgroups" is a group of H/W clients which Tegra SoCs
support. 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.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 include/dt-bindings/memory/tegra-swgroup.h | 46 ++++++++++++++++++++++++++++++
 1 file changed, 46 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..434cdcb
--- /dev/null
+++ b/include/dt-bindings/memory/tegra-swgroup.h
@@ -0,0 +1,46 @@
+/*
+ * 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 *//* Exceptionally non-linear */
+#define TEGRA_SWGROUP_DC12	10	/* 0xa88 *//* Exceptionally 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 starts from here */
+#define TEGRA_SWGROUP_SDMMC2A	26	/* 0xa98 */
+#define TEGRA_SWGROUP_SDMMC3A	27	/* 0xa9c */
+#define TEGRA_SWGROUP_SDMMC4A	28	/* 0xaa0 */
+#define TEGRA_SWGROUP_ISP2B	29	/* 0xaa4 */
+#define TEGRA_SWGROUP_GPU	30	/* 0xaa8 */
+#define TEGRA_SWGROUP_GPUB	31	/* 0xaac */
+#define TEGRA_SWGROUP_PPCS2	32	/* 0xab0 */
+
+#define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)
+
+#endif /* _DT_BINDINGS_MEMORY_TEGRA_SWGROUP_H */
-- 
1.8.1.5

[PATCHv3 16/19] iommu/tegra: smmu: Use dt-bindings MACRO

[Hiroshi Doyu]

Use dt-bindings MACRO instead of SoC dependent MACROs.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c | 42 ++----------------------------------------
 1 file changed, 2 insertions(+), 40 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 1db9e89..66406b7 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -41,45 +41,7 @@
 #include <asm/cacheflush.h>
 #include <asm/dma-iommu.h>
 
-enum smmu_hwgrp {
-	HWGRP_AFI,
-	HWGRP_AVPC,
-	HWGRP_DC,
-	HWGRP_DCB,
-	HWGRP_EPP,
-	HWGRP_G2,
-	HWGRP_HC,
-	HWGRP_HDA,
-	HWGRP_ISP,
-	HWGRP_MPE,
-	HWGRP_NV,
-	HWGRP_NV2,
-	HWGRP_PPCS,
-	HWGRP_SATA,
-	HWGRP_VDE,
-	HWGRP_VI,
-
-	HWGRP_COUNT,
-
-	HWGRP_END = ~0,
-};
-
-#define HWG_AFI		(1 << HWGRP_AFI)
-#define HWG_AVPC	(1 << HWGRP_AVPC)
-#define HWG_DC		(1 << HWGRP_DC)
-#define HWG_DCB		(1 << HWGRP_DCB)
-#define HWG_EPP		(1 << HWGRP_EPP)
-#define HWG_G2		(1 << HWGRP_G2)
-#define HWG_HC		(1 << HWGRP_HC)
-#define HWG_HDA		(1 << HWGRP_HDA)
-#define HWG_ISP		(1 << HWGRP_ISP)
-#define HWG_MPE		(1 << HWGRP_MPE)
-#define HWG_NV		(1 << HWGRP_NV)
-#define HWG_NV2		(1 << HWGRP_NV2)
-#define HWG_PPCS	(1 << HWGRP_PPCS)
-#define HWG_SATA	(1 << HWGRP_SATA)
-#define HWG_VDE		(1 << HWGRP_VDE)
-#define HWG_VI		(1 << HWGRP_VI)
+#include <dt-bindings/memory/tegra-swgroup.h>
 
 /* bitmap of the page sizes currently supported */
 #define SMMU_IOMMU_PGSIZES	(SZ_4K)
@@ -826,7 +788,7 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
 	 * Reserve "page zero" for AVP vectors using a common dummy
 	 * page.
 	 */
-	if (map & HWG_AVPC) {
+	if (map & TEGRA_SWGROUP_BIT(AVPC)) {
 		struct page *page;
 
 		page = as->smmu->avp_vector_page;
-- 
1.8.1.5

[PATCHv3 17/19] ARM: dt: tegra30: iommu: Add "nvidia,memory-clients"

[Hiroshi Doyu]

Add "nvidia,memory-clients" to identify which swgroup ID a device
belongs to.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra30.dtsi | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index dfd3ca9..4173a2a 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>;
+		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_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>;
+			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_MPE>;
 		};
 
 		vi {
@@ -110,6 +113,7 @@
 			reg = <0x54080000 0x00040000>;
 			interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_VI>;
+			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_VI>;
 		};
 
 		epp {
@@ -117,6 +121,7 @@
 			reg = <0x540c0000 0x00040000>;
 			interrupts = <GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_EPP>;
+			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_EPP>;
 		};
 
 		isp {
@@ -124,6 +129,7 @@
 			reg = <0x54100000 0x00040000>;
 			interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_ISP>;
+			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_ISP>;
 		};
 
 		gr2d {
@@ -131,6 +137,7 @@
 			reg = <0x54140000 0x00040000>;
 			interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_GR2D>;
+			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_G2>;
 		};
 
 		gr3d {
@@ -138,6 +145,8 @@
 			reg = <0x54180000 0x00040000>;
 			clocks = <&tegra_car 24 &tegra_car 98>;
 			clock-names = "3d", "3d2";
+			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_NV
+						TEGRA_SWGROUP_NV2>;
 		};
 
 		dc@54200000 {
@@ -147,6 +156,7 @@
 			clocks = <&tegra_car TEGRA30_CLK_DISP1>,
 				 <&tegra_car TEGRA30_CLK_PLL_P>;
 			clock-names = "disp1", "parent";
+			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_DC>;
 
 			rgb {
 				status = "disabled";
@@ -160,6 +170,7 @@
 			clocks = <&tegra_car TEGRA30_CLK_DISP2>,
 				 <&tegra_car TEGRA30_CLK_PLL_P>;
 			clock-names = "disp2", "parent";
+			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_DCB>;
 
 			rgb {
 				status = "disabled";
@@ -316,6 +327,7 @@
 		interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
 		nvidia,dma-request-selector = <&apbdma 8>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTA>;
+		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
 		status = "disabled";
 	};
 
@@ -326,6 +338,7 @@
 		interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
 		nvidia,dma-request-selector = <&apbdma 9>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTB>;
+		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
 		status = "disabled";
 	};
 
@@ -336,6 +349,7 @@
 		interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
 		nvidia,dma-request-selector = <&apbdma 10>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTC>;
+		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
 		status = "disabled";
 	};
 
@@ -346,6 +360,7 @@
 		interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
 		nvidia,dma-request-selector = <&apbdma 19>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTD>;
+		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
 		status = "disabled";
 	};
 
@@ -356,6 +371,7 @@
 		interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
 		nvidia,dma-request-selector = <&apbdma 20>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTE>;
+		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
 		status = "disabled";
 	};
 
@@ -524,7 +540,7 @@
 		interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
 	};
 
-	iommu {
+	smmu: iommu {
 		compatible = "nvidia,tegra30-smmu";
 		reg = <0x7000f010 0x02c
 		       0x7000f1f0 0x010
@@ -605,6 +621,7 @@
 		reg = <0x78000000 0x200>;
 		interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA30_CLK_SDMMC1>;
+		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
 		status = "disabled";
 	};
 
@@ -613,6 +630,7 @@
 		reg = <0x78000200 0x200>;
 		interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA30_CLK_SDMMC2>;
+		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
 		status = "disabled";
 	};
 
@@ -621,6 +639,7 @@
 		reg = <0x78000400 0x200>;
 		interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA30_CLK_SDMMC3>;
+		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
 		status = "disabled";
 	};
 
@@ -629,6 +648,7 @@
 		reg = <0x78000600 0x200>;
 		interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA30_CLK_SDMMC4>;
+		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
 		status = "disabled";
 	};
 
-- 
1.8.1.5

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

[PATCHv3 18/19] ARM: dt: tegra114: iommu: Add "nvidia, memory-clients"

[Hiroshi Doyu]

Add "nvidia,memory-clients" to identify which swgroup ID a device
belongs to.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra114.dtsi | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
index 4626e8a..db22daa 100644
--- a/arch/arm/boot/dts/tegra114.dtsi
+++ b/arch/arm/boot/dts/tegra114.dtsi
@@ -1,6 +1,7 @@
 #include <dt-bindings/clock/tegra114-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"
 
@@ -316,7 +317,7 @@
 		clock-names = "pclk", "clk32k_in";
 	};
 
-	iommu {
+	smmu: iommu {
 		compatible = "nvidia,tegra114-smmu", "nvidia,tegra30-smmu";
 		reg = <0x70019010 0x02c
 		       0x700191f0 0x010
@@ -403,6 +404,7 @@
 		reg = <0x78000000 0x200>;
 		interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA114_CLK_SDMMC1>;
+		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
 		status = "disable";
 	};
 
@@ -411,6 +413,7 @@
 		reg = <0x78000200 0x200>;
 		interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA114_CLK_SDMMC2>;
+		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
 		status = "disable";
 	};
 
@@ -419,6 +422,7 @@
 		reg = <0x78000400 0x200>;
 		interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA114_CLK_SDMMC3>;
+		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
 		status = "disable";
 	};
 
@@ -427,6 +431,7 @@
 		reg = <0x78000600 0x200>;
 		interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA114_CLK_SDMMC4>;
+		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
 		status = "disable";
 	};
 
-- 
1.8.1.5

[PATCHv3 19/19] iommu/tegra: smmu: Support Multiple ASID

[Hiroshi Doyu]

Support Multiple Address Space(AS). Tegra SMMU can have multiple
ASes. 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.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 66406b7..e703741 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -925,7 +925,18 @@ enum {
 static int smmu_iommu_add_device(struct device *dev)
 {
 	int err;
-	struct dma_iommu_mapping *map = smmu_handle->map[SYSTEM_DEFAULT];
+	u64 swgroups;
+	struct dma_iommu_mapping *map = NULL;
+
+	swgroups = smmu_of_get_memory_client(dev);
+	switch (swgroups) {
+	case TEGRA_SWGROUP_BIT(PPCS):
+		map = smmu_handle->map[SYSTEM_PROTECTED];
+		break;
+	default:
+		map = smmu_handle->map[SYSTEM_DEFAULT];
+		break;
+	}
 
 	if (!map)
 		goto out;
@@ -935,7 +946,7 @@ static int smmu_iommu_add_device(struct device *dev)
 		return err;
 
 out:
-	dev_dbg(dev, "Attached to map %p\n", map);
+	dev_dbg(dev, "Attached to map %p, swgroup:%08llx\n", map, swgroups);
 	return 0;
 }
 
-- 
1.8.1.5

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Grant Likely]

On Fri, 18 Oct 2013 13:26:42 +0300, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> To prevent of_platform_populate() from trying to populate duplicate
> devices if a device has been already populated.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/of/platform.c | 6 ++++++
>  include/linux/of.h    | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 9b439ac..994cea4 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -208,6 +208,11 @@ static struct platform_device *of_platform_device_create_pdata(
>  	if (!of_device_is_available(np))
>  		return NULL;
>  
> +	if (np->dev) {
> +		dev_info(np->dev, "Already populated\n");
> +		return to_platform_device(np->dev);
> +	}
> +

This looks racy if the intent is to prevent a device from being created
twice. If two threads call of_platform_device_create() in parallel, this
will fail to properly protect against dual creation. Checking the node
and marking it as used needs to be atomic.

>  	dev = of_device_alloc(np, bus_id, parent);
>  	if (!dev)
>  		return NULL;
> @@ -232,6 +237,7 @@ static struct platform_device *of_platform_device_create_pdata(
>  		return NULL;
>  	}
>  
> +	np->dev = &dev->dev;
>  	return dev;
>  }
>  
> diff --git a/include/linux/of.h b/include/linux/of.h
> index f95aee3..638a88a 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -60,6 +60,7 @@ struct device_node {
>  	struct	kref kref;
>  	unsigned long _flags;
>  	void	*data;
> +	struct device *dev;		/* Set only after populated */

Is this being used merely to indicate that a device has been processed
by of_platform_device_create()? Or do you intend to dereference this
pointer? I've avoided putting the struct device in to the device_node
structure up to this point simply becuase there aren't any good clues
for what /kind/ of device it actually points to. I worry that bad
assumptions will get made when other subsystems try to use the
same pointer. ie. if one subsystem creates its own device and sets this
pointer, and then of_platform_device_create() comes along behind, sees
that it is already created, and then returns a platform_device pointer
*for something that isn't a struct platform_device*. This is very bad.

Instead of using a pointer to the struct device, would a flag be
sufficient for your purposes? Would it be fine to return NULL if the
device has already been created?

g.

>  #if defined(CONFIG_SPARC)
>  	const char *path_component_name;
>  	unsigned int unique_id;
> -- 
> 1.8.1.5
> 

Re: [PATCHv3 10/19] iommu/tegra: smmu: Get "nvidia,swgroups" from DT

[Grant Likely]

On Fri, 18 Oct 2013 13:26:51 +0300, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> This provides the info about which H/W Accelerators are supported on
> Tegra SoC. This info is passed from DT. This is necessary to have the
> unified SMMU driver among Tegra SoCs. Instead of using platform data,
> DT passes "nvidia,swgroups" now. DT is mandatory in Tegra.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> @@ -1200,6 +1193,9 @@ static int tegra_smmu_probe(struct platform_device *pdev)
>  	if (of_property_read_u32(dev->of_node, "nvidia,#asids", &asids))
>  		return -ENODEV;
>  
> +	if (of_property_read_u64(dev->of_node, "nvidia,swgroups", &swgroups))
> +		return -ENODEV;
> +

Best practice: A new property has been created here. If the property
isn't present, then it should default to the previous meaning instead of
outright failing. (or make a strong argument that no one is using this
yet and therefore is not breakage).

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Grant Likely]

On Fri, 18 Oct 2013 13:26:55 +0300, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> This provides the info about which swgroups a device belongs to. This
> info is passed from DT. This is necessary for the unified SMMU driver
> among Tegra SoCs since each has different H/W accelerators.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  .../bindings/iommu/nvidia,tegra30-smmu.txt         | 16 ++++++++++++-
>  drivers/iommu/tegra-smmu.c                         | 27 ++++++++++++++++++++--
>  2 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
> index 6a844b3..7456104 100644
> --- a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
> +++ b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
> @@ -13,9 +13,12 @@ Required properties:
>    file <dt-bindings/memory/tegra-swgroup.h>. Its max is 64. 2 cells
>    are required. This unique ID info can be used to calculate
>    MC_SMMU_<SWGROUP name>_ASID_0 offset and HOTRESET bit.
> +- nvidia,memory-clients: phandle to a smmu device which a device is
> +  attached to and indicates which swgroups a device belongs to(SWGROUP ID).
> +  SWGROUP ID is from 0 to 63, and a device can belong to multiple SWGROUPS.
>  
>  Example:
> -	smmu {
> +	smmu: iommu {
>  		compatible = "nvidia,tegra30-smmu";
>  		reg = <0x7000f010 0x02c
>  		       0x7000f1f0 0x010
> @@ -25,3 +28,14 @@ Example:
>  		nvidia,swgroups = <0x00000000 0x000779ff>;
>  		nvidia,ahb = <&ahb>;
>  	};
> +
> +	host1x {
> +		compatible = "nvidia,tegra30-host1x", "simple-bus";
> +		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_HC>;
> +		....
> +		gr3d {
> +			compatible = "nvidia,tegra30-gr3d";
> +			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_NV
> +						       TEGRA_SWGROUP_NV2>;
> +			....
> +		};
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index f0ff48e..1db9e89 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -368,6 +368,29 @@ static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs)
>  
>  #define smmu_client_hwgrp(c)	(c->as->smmu->swgroups)
>  
> +static u64 smmu_of_get_memory_client(struct device *dev)
> +{
> +	size_t bytes;
> +	const char *propname = "nvidia,memory-clients";
> +	const __be32 *prop;
> +	int i;
> +	u64 swgroup = 0;
> +	struct device_node *np;
> +
> +	np = of_parse_phandle(dev->of_node, propname, 0);
> +	if (np != smmu_handle->dev->of_node)
> +		return ~0;
> +
> +	prop = of_get_property(dev->of_node, propname, &bytes);
> +	if (!prop || !bytes)
> +		return ~0;
> +
> +	for (i = 1; i < bytes / sizeof(u32); i++, prop++)
> +		swgroup |= 1ULL << be32_to_cpup(prop);
> +
> +	return swgroup;
> +}
> +
>  static int __smmu_client_set_hwgrp(struct smmu_client *c,
>  				   u64 map, int on)
>  {
> @@ -779,8 +802,8 @@ static int smmu_iommu_attach_dev(struct iommu_domain *domain,
>  		return -ENOMEM;
>  	client->dev = dev;
>  	client->as = as;
> -	map = smmu->swgroups;
> -	if (!map)
> +	map = smmu_of_get_memory_client(dev);
> +	if (map == ~0)
>  		return -EINVAL;

Ditto to my previous comment. Can you return the currently used value if
the property isn't present? (it's okay to warn at the same time, but it
is best to avoid breakage).

g.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Hiroshi Doyu]

Hi Grant,

Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote @ Thu, 24 Oct 2013 10:55:31 +0200:
....
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index f95aee3..638a88a 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -60,6 +60,7 @@ struct device_node {
> >  	struct	kref kref;
> >  	unsigned long _flags;
> >  	void	*data;
> > +	struct device *dev;		/* Set only after populated */
> 
> Is this being used merely to indicate that a device has been processed
> by of_platform_device_create()? Or do you intend to dereference this
> pointer? I've avoided putting the struct device in to the device_node
> structure up to this point simply becuase there aren't any good clues
> for what /kind/ of device it actually points to. I worry that bad
> assumptions will get made when other subsystems try to use the
> same pointer. ie. if one subsystem creates its own device and sets this
> pointer, and then of_platform_device_create() comes along behind, sees
> that it is already created, and then returns a platform_device pointer
> *for something that isn't a struct platform_device*. This is very bad.
> 
> Instead of using a pointer to the struct device, would a flag be
> sufficient for your purposes? Would it be fine to return NULL if the
> device has already been created?

Yes, a flag would be enough for this purpose.

This patch is a part of HACK to control device instanciation order. We
have an IOMMU device(platform) which needs to be instanciated earlier
than other (platform)devices so that IOMMU driver would configure them
as IOMMU'able device.

Is there any better way to control device instanciation order from DT?

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Kumar Gala]

On Oct 24, 2013, at 4:21 AM, Hiroshi Doyu wrote:

> Hi Grant,
> 
> Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote @ Thu, 24 Oct 2013 10:55:31 +0200:
> ....
>>> diff --git a/include/linux/of.h b/include/linux/of.h
>>> index f95aee3..638a88a 100644
>>> --- a/include/linux/of.h
>>> +++ b/include/linux/of.h
>>> @@ -60,6 +60,7 @@ struct device_node {
>>> 	struct	kref kref;
>>> 	unsigned long _flags;
>>> 	void	*data;
>>> +	struct device *dev;		/* Set only after populated */
>> 
>> Is this being used merely to indicate that a device has been processed
>> by of_platform_device_create()? Or do you intend to dereference this
>> pointer? I've avoided putting the struct device in to the device_node
>> structure up to this point simply becuase there aren't any good clues
>> for what /kind/ of device it actually points to. I worry that bad
>> assumptions will get made when other subsystems try to use the
>> same pointer. ie. if one subsystem creates its own device and sets this
>> pointer, and then of_platform_device_create() comes along behind, sees
>> that it is already created, and then returns a platform_device pointer
>> *for something that isn't a struct platform_device*. This is very bad.
>> 
>> Instead of using a pointer to the struct device, would a flag be
>> sufficient for your purposes? Would it be fine to return NULL if the
>> device has already been created?
> 
> Yes, a flag would be enough for this purpose.
> 
> This patch is a part of HACK to control device instanciation order. We
> have an IOMMU device(platform) which needs to be instanciated earlier
> than other (platform)devices so that IOMMU driver would configure them
> as IOMMU'able device.
> 
> Is there any better way to control device instanciation order from DT?

I was also thinking being able to call of_platform_populate multiple times and have explicit lists to control device init order might be a workable solution.  So might be worth continuing down this path to make device nodes that have already be created.

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

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Grant Likely]

On Thu, 24 Oct 2013 11:21:15 +0200, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Hi Grant,
> 
> Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote @ Thu, 24 Oct 2013 10:55:31 +0200:
> ....
> > > diff --git a/include/linux/of.h b/include/linux/of.h
> > > index f95aee3..638a88a 100644
> > > --- a/include/linux/of.h
> > > +++ b/include/linux/of.h
> > > @@ -60,6 +60,7 @@ struct device_node {
> > >  	struct	kref kref;
> > >  	unsigned long _flags;
> > >  	void	*data;
> > > +	struct device *dev;		/* Set only after populated */
> > 
> > Is this being used merely to indicate that a device has been processed
> > by of_platform_device_create()? Or do you intend to dereference this
> > pointer? I've avoided putting the struct device in to the device_node
> > structure up to this point simply becuase there aren't any good clues
> > for what /kind/ of device it actually points to. I worry that bad
> > assumptions will get made when other subsystems try to use the
> > same pointer. ie. if one subsystem creates its own device and sets this
> > pointer, and then of_platform_device_create() comes along behind, sees
> > that it is already created, and then returns a platform_device pointer
> > *for something that isn't a struct platform_device*. This is very bad.
> > 
> > Instead of using a pointer to the struct device, would a flag be
> > sufficient for your purposes? Would it be fine to return NULL if the
> > device has already been created?
> 
> Yes, a flag would be enough for this purpose.
> 
> This patch is a part of HACK to control device instanciation order. We
> have an IOMMU device(platform) which needs to be instanciated earlier
> than other (platform)devices so that IOMMU driver would configure them
> as IOMMU'able device.

Ideally the drivers depending on the IOMMU would return -EPROBE_DEFER if
the IOMMU driver isn't set up so that you don't need to play games with
probe order. Creating certain platform devices early is a really ugly
and fragile solution.

Besides, probe order of device drivers is far more about link order of
the kernel than it is about when of_platform_device_create() is called.
Fiddling with the initcall level on the IOMMU driver (while not
recommended) may very well have the effect you desire.

g.

> 
> Is there any better way to control device instanciation order from DT?

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Grant Likely]

On Thu, 24 Oct 2013 04:36:30 -0500, Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> 
> On Oct 24, 2013, at 4:21 AM, Hiroshi Doyu wrote:
> 
> > Hi Grant,
> > 
> > Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote @ Thu, 24 Oct 2013 10:55:31 +0200:
> > ....
> >>> diff --git a/include/linux/of.h b/include/linux/of.h
> >>> index f95aee3..638a88a 100644
> >>> --- a/include/linux/of.h
> >>> +++ b/include/linux/of.h
> >>> @@ -60,6 +60,7 @@ struct device_node {
> >>> 	struct	kref kref;
> >>> 	unsigned long _flags;
> >>> 	void	*data;
> >>> +	struct device *dev;		/* Set only after populated */
> >> 
> >> Is this being used merely to indicate that a device has been processed
> >> by of_platform_device_create()? Or do you intend to dereference this
> >> pointer? I've avoided putting the struct device in to the device_node
> >> structure up to this point simply becuase there aren't any good clues
> >> for what /kind/ of device it actually points to. I worry that bad
> >> assumptions will get made when other subsystems try to use the
> >> same pointer. ie. if one subsystem creates its own device and sets this
> >> pointer, and then of_platform_device_create() comes along behind, sees
> >> that it is already created, and then returns a platform_device pointer
> >> *for something that isn't a struct platform_device*. This is very bad.
> >> 
> >> Instead of using a pointer to the struct device, would a flag be
> >> sufficient for your purposes? Would it be fine to return NULL if the
> >> device has already been created?
> > 
> > Yes, a flag would be enough for this purpose.
> > 
> > This patch is a part of HACK to control device instanciation order. We
> > have an IOMMU device(platform) which needs to be instanciated earlier
> > than other (platform)devices so that IOMMU driver would configure them
> > as IOMMU'able device.
> > 
> > Is there any better way to control device instanciation order from DT?
> 
> I was also thinking being able to call of_platform_populate multiple times and have explicit lists to control device init order might be a workable solution.  So might be worth continuing down this path to make device nodes that have already be created.

Does that actually solve the problem though? If the multiple calls to
of_platform_populate are all before the driver initcall, then the order
of probing is most likely going to be more influenced by kernel link
order.

g.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 5061 bytes --]

On Fri, Oct 25, 2013 at 01:10:38AM +0100, Grant Likely wrote:
> On Thu, 24 Oct 2013 11:21:15 +0200, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > Hi Grant,
> > 
> > Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote @ Thu, 24 Oct 2013 10:55:31 +0200:
> > ....
> > > > diff --git a/include/linux/of.h b/include/linux/of.h
> > > > index f95aee3..638a88a 100644
> > > > --- a/include/linux/of.h
> > > > +++ b/include/linux/of.h
> > > > @@ -60,6 +60,7 @@ struct device_node {
> > > >  	struct	kref kref;
> > > >  	unsigned long _flags;
> > > >  	void	*data;
> > > > +	struct device *dev;		/* Set only after populated */
> > > 
> > > Is this being used merely to indicate that a device has been processed
> > > by of_platform_device_create()? Or do you intend to dereference this
> > > pointer? I've avoided putting the struct device in to the device_node
> > > structure up to this point simply becuase there aren't any good clues
> > > for what /kind/ of device it actually points to. I worry that bad
> > > assumptions will get made when other subsystems try to use the
> > > same pointer. ie. if one subsystem creates its own device and sets this
> > > pointer, and then of_platform_device_create() comes along behind, sees
> > > that it is already created, and then returns a platform_device pointer
> > > *for something that isn't a struct platform_device*. This is very bad.
> > > 
> > > Instead of using a pointer to the struct device, would a flag be
> > > sufficient for your purposes? Would it be fine to return NULL if the
> > > device has already been created?
> > 
> > Yes, a flag would be enough for this purpose.
> > 
> > This patch is a part of HACK to control device instanciation order. We
> > have an IOMMU device(platform) which needs to be instanciated earlier
> > than other (platform)devices so that IOMMU driver would configure them
> > as IOMMU'able device.
> 
> Ideally the drivers depending on the IOMMU would return -EPROBE_DEFER if
> the IOMMU driver isn't set up so that you don't need to play games with
> probe order. Creating certain platform devices early is a really ugly
> and fragile solution.
> 
> Besides, probe order of device drivers is far more about link order of
> the kernel than it is about when of_platform_device_create() is called.
> Fiddling with the initcall level on the IOMMU driver (while not
> recommended) may very well have the effect you desire.

This is actually "the other problem that I'm aware of that could benefit
from [interrupt resolution at probe time]". My idea was that once we had
a way within the driver core to resolve interrupt references at probe
time it could be used for potentially many other resources as well. Some
of the resources like GPIOs and regulators are obviously not something
that the core can or should be requesting, but mostly resources that you
don't actually need to control after probing (such as interrupts) would
be a good fit because otherwise people would write the same boilerplate
over and over again.

IOMMUs seem to me to be in that same category. As far as I can tell, an
IOMMU driver registers the IOMMU for a given bus, upon which every
device can simply be used (mostly transparently) with that IOMMU. While
I haven't figured out how exactly, I'm pretty sure we can take advantage
of the resolution of resources at probe time within the core to both
keep drivers from having to do anything in particular and at the same
time have code to determine if the IOMMU driver hasn't been probed yet
and return -EPROBE_DEFER appropriately.

I suspect that there will be enough differences between the various
IOMMU implementations that we won't be able to have a unified binding
(especially for how to associate devices with a particular virtual
address space), but perhaps that could be solved with something like an
.of_xlate() that IOMMU drivers implement, much like we've done with most
other subsystems too.

The binding for Tegra's IOMMU currently only uses the HW IDs (or a mask)
to put a device into a given address space, but I think that could be
easily extended to something like:

	memory-clients = <&iommu MASK>;

Or similar. If other information is required, we could encode that into
a multi-cell specifier. Perhaps we could even leave away the phandle
since typically there will only be a single IOMMU in the system?

Does that sound reasonable? Hiroshi is much more familiar with IOMMUs,
so I'd like to get his opinion on the above as well.

Grant, I wonder if it might be more useful if I concentrate on solving
this problem first, before tackling the interrupt reference resolution.
I expect this to be able to use the same core functionality, so it could
be used as a less invasive means of introducing some pieces we can later
reuse. Also this only adds new functionality and doesn't touch anything
that currently works, so there's less (i.e. no) risk of breaking any
existing functionality.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Hiroshi Doyu]

Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Fri, 25 Oct 2013 09:56:55 +0200:

> I suspect that there will be enough differences between the various
> IOMMU implementations that we won't be able to have a unified binding
> (especially for how to associate devices with a particular virtual
> address space), but perhaps that could be solved with something like an
> .of_xlate() that IOMMU drivers implement, much like we've done with most
> other subsystems too.
> 
> The binding for Tegra's IOMMU currently only uses the HW IDs (or a mask)
> to put a device into a given address space, but I think that could be
> easily extended to something like:
> 
> 	memory-clients = <&iommu MASK>;
> 
> Or similar. If other information is required, we could encode that into
> a multi-cell specifier. Perhaps we could even leave away the phandle
> since typically there will only be a single IOMMU in the system?
> 
> Does that sound reasonable? Hiroshi is much more familiar with IOMMUs,
> so I'd like to get his opinion on the above as well.

I think that the above may be possible, but I'd like to listen from
other IOMMU SOC maintainers.

A brief explanation for "memory-clients":

In tegra, every H/W has its own memory-client ID, and it can be
associated to any address spaces. The above "memory-cilents" is used
to indicate which ID a device has in DT. If the other SOC IOMMUs need
this kind of "memory-clients", this would be standarized. Any comment?

At least arm-smmu seems to have the following. multiple IOMMUs can be
handled with this.


- smmu-parent   : When multiple SMMUs are chained together, this
                  property can be used to provide a phandle to the
                  parent SMMU (that is the next SMMU on the path going
                  from the mmu-masters towards memory) node for this
                  SMMU.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Hiroshi Doyu]

Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Fri, 25 Oct 2013 09:56:55 +0200:

> > > This patch is a part of HACK to control device instanciation order. We
> > > have an IOMMU device(platform) which needs to be instanciated earlier
> > > than other (platform)devices so that IOMMU driver would configure them
> > > as IOMMU'able device.
> > 
> > Ideally the drivers depending on the IOMMU would return -EPROBE_DEFER if
> > the IOMMU driver isn't set up so that you don't need to play games with
> > probe order. Creating certain platform devices early is a really ugly
> > and fragile solution.
> > 
> > Besides, probe order of device drivers is far more about link order of
> > the kernel than it is about when of_platform_device_create() is called.
> > Fiddling with the initcall level on the IOMMU driver (while not
> > recommended) may very well have the effect you desire.
> 
> This is actually "the other problem that I'm aware of that could benefit
> from [interrupt resolution at probe time]". My idea was that once we had
> a way within the driver core to resolve interrupt references at probe
> time it could be used for potentially many other resources as well. Some
> of the resources like GPIOs and regulators are obviously not something
> that the core can or should be requesting, but mostly resources that you
> don't actually need to control after probing (such as interrupts) would
> be a good fit because otherwise people would write the same boilerplate
> over and over again.
> 
> IOMMUs seem to me to be in that same category. As far as I can tell, an
> IOMMU driver registers the IOMMU for a given bus, upon which every
> device can simply be used (mostly transparently) with that IOMMU. While
> I haven't figured out how exactly, I'm pretty sure we can take advantage
> of the resolution of resources at probe time within the core to both
> keep drivers from having to do anything in particular and at the same
> time have code to determine if the IOMMU driver hasn't been probed yet
> and return -EPROBE_DEFER appropriately.

Can you explain the above a bit more?

Originally I thought that what Grant suggested would work ok with this patch.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4996 bytes --]

On Fri, Oct 25, 2013 at 10:25:49AM +0200, Hiroshi Doyu wrote:
> Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Fri, 25 Oct 2013 09:56:55 +0200:
> 
> > > > This patch is a part of HACK to control device instanciation order. We
> > > > have an IOMMU device(platform) which needs to be instanciated earlier
> > > > than other (platform)devices so that IOMMU driver would configure them
> > > > as IOMMU'able device.
> > > 
> > > Ideally the drivers depending on the IOMMU would return -EPROBE_DEFER if
> > > the IOMMU driver isn't set up so that you don't need to play games with
> > > probe order. Creating certain platform devices early is a really ugly
> > > and fragile solution.
> > > 
> > > Besides, probe order of device drivers is far more about link order of
> > > the kernel than it is about when of_platform_device_create() is called.
> > > Fiddling with the initcall level on the IOMMU driver (while not
> > > recommended) may very well have the effect you desire.
> > 
> > This is actually "the other problem that I'm aware of that could benefit
> > from [interrupt resolution at probe time]". My idea was that once we had
> > a way within the driver core to resolve interrupt references at probe
> > time it could be used for potentially many other resources as well. Some
> > of the resources like GPIOs and regulators are obviously not something
> > that the core can or should be requesting, but mostly resources that you
> > don't actually need to control after probing (such as interrupts) would
> > be a good fit because otherwise people would write the same boilerplate
> > over and over again.
> > 
> > IOMMUs seem to me to be in that same category. As far as I can tell, an
> > IOMMU driver registers the IOMMU for a given bus, upon which every
> > device can simply be used (mostly transparently) with that IOMMU. While
> > I haven't figured out how exactly, I'm pretty sure we can take advantage
> > of the resolution of resources at probe time within the core to both
> > keep drivers from having to do anything in particular and at the same
> > time have code to determine if the IOMMU driver hasn't been probed yet
> > and return -EPROBE_DEFER appropriately.
> 
> Can you explain the above a bit more?
> 
> Originally I thought that what Grant suggested would work ok with this
> patch.

I think the objection to these patches is that they special case the
instantiation of some devices. It's not a proper solution because it
implies various things. For example merely instantiating the IOMMU
device earlier than others is only going to work *if* the driver is
actually probed before the drivers of other devices. If you want to
build the driver as a module for example, probe order becomes entirely
non-deterministic.

So what Grant was saying is that you could possibly make it work by
forcing the driver to be loaded earlier using explicit initcall
ordering. But he also said that's not recommended because it's not a
proper solution and therefore not guaranteed to always work.

Explicit initcall ordering used to be heavily used in the past, but
there have been many efforts to move away from it. One of the solutions
introduced to help with that is deferred probing, which essentially adds
a new error code (EPROBE_DEFER) which a driver's .probe() can return to
cause it to be probed at a later point again, after other drivers have
been probed.

How this works is basically that a driver's .probe() requests whatever
resources it needs (GPIOs, clocks, regulators, ...). If any of those
resources isn't there yet (presumably because the driver providing it
hasn't been probed yet), it can return -EPROBE_DEFER to signal that not
all of its dependencies are available yet.

Instead of handling such dependencies implicitly by making sure all
resource providers are probed earlier than any of their consumers, the
dependencies are handled more explicitly, which turns out to simplify
things a lot. There's some additional work required in the core, but if
done consistently no driver needs to care about the dependencies and it
no longer matters where the resources come from. The problem is reduced
to essentially this:

	while (!resource_available())
		load_more_drivers();

So my proposed solution for the IOMMU case is to treat it the same as
any other resources. Perhaps resource isn't the right word, but at the
core the issue is the same. A device requires the services of an IOMMU
so that it can be put into the correct address space. If the IOMMU is
not available yet it cannot do that, so we simply return -EPROBE_DEFER
and cause the probe to be retried later.

Eventually the IOMMU driver will be probed and register the IOMMU. When
the earlier driver is probed again, it will be able to successfully
request to be put into the proper address space and continue with the
initialization.

Does that answer your question?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Hiroshi Doyu]

Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Fri, 25 Oct 2013 11:11:05 +0200:

> > > This is actually "the other problem that I'm aware of that could benefit
> > > from [interrupt resolution at probe time]". My idea was that once we had
> > > a way within the driver core to resolve interrupt references at probe
> > > time it could be used for potentially many other resources as well. Some
> > > of the resources like GPIOs and regulators are obviously not something
> > > that the core can or should be requesting, but mostly resources that you
> > > don't actually need to control after probing (such as interrupts) would
> > > be a good fit because otherwise people would write the same boilerplate
> > > over and over again.
> > > 
> > > IOMMUs seem to me to be in that same category. As far as I can tell, an
> > > IOMMU driver registers the IOMMU for a given bus, upon which every
> > > device can simply be used (mostly transparently) with that IOMMU. While
> > > I haven't figured out how exactly, I'm pretty sure we can take advantage
> > > of the resolution of resources at probe time within the core to both
> > > keep drivers from having to do anything in particular and at the same
> > > time have code to determine if the IOMMU driver hasn't been probed yet
> > > and return -EPROBE_DEFER appropriately.
> > 
> > Can you explain the above a bit more?
> > 
> > Originally I thought that what Grant suggested would work ok with this
> > patch.
> 
> I think the objection to these patches is that they special case the
> instantiation of some devices. It's not a proper solution because it
> implies various things. For example merely instantiating the IOMMU
> device earlier than others is only going to work *if* the driver is
> actually probed before the drivers of other devices. If you want to
> build the driver as a module for example, probe order becomes entirely
> non-deterministic.

I understand the above limitation. What I thought for the solution is
that I can make use of iommu_bus even before IOMMU is
instanciated. iommu_bus has its notifier which calls
iommu_ops()->add_device(). This could return -EPROBE_DEFER when IOMMU
isn't ready. Only the problem is the current "bus_notifier" doesn't
have the ability to return error. I'll see if it can be modified. Even
with this, at least IOMMU *driver* needs to be init'ed enough earlier
to prevent devices from being registered without IOMMU.

> Instead of handling such dependencies implicitly by making sure all
> resource providers are probed earlier than any of their consumers, the
> dependencies are handled more explicitly, which turns out to simplify
> things a lot. There's some additional work required in the core, but if
> done consistently no driver needs to care about the dependencies and it
> no longer matters where the resources come from. The problem is reduced
> to essentially this:
> 
> 	while (!resource_available())
> 		load_more_drivers();
> 
> So my proposed solution for the IOMMU case is to treat it the same as
> any other resources. Perhaps resource isn't the right word, but at the
> core the issue is the same. A device requires the services of an IOMMU
> so that it can be put into the correct address space. If the IOMMU is
> not available yet it cannot do that, so we simply return -EPROBE_DEFER
> and cause the probe to be retried later.

This looks somewhat similar to the above iommu_bus notifier.

Is there any way to implement the same mechanism rather than using
bus?

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4560 bytes --]

On Fri, Oct 25, 2013 at 11:49:05AM +0200, Hiroshi Doyu wrote:
> Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Fri, 25 Oct 2013 11:11:05 +0200:
> 
> > > > This is actually "the other problem that I'm aware of that could benefit
> > > > from [interrupt resolution at probe time]". My idea was that once we had
> > > > a way within the driver core to resolve interrupt references at probe
> > > > time it could be used for potentially many other resources as well. Some
> > > > of the resources like GPIOs and regulators are obviously not something
> > > > that the core can or should be requesting, but mostly resources that you
> > > > don't actually need to control after probing (such as interrupts) would
> > > > be a good fit because otherwise people would write the same boilerplate
> > > > over and over again.
> > > > 
> > > > IOMMUs seem to me to be in that same category. As far as I can tell, an
> > > > IOMMU driver registers the IOMMU for a given bus, upon which every
> > > > device can simply be used (mostly transparently) with that IOMMU. While
> > > > I haven't figured out how exactly, I'm pretty sure we can take advantage
> > > > of the resolution of resources at probe time within the core to both
> > > > keep drivers from having to do anything in particular and at the same
> > > > time have code to determine if the IOMMU driver hasn't been probed yet
> > > > and return -EPROBE_DEFER appropriately.
> > > 
> > > Can you explain the above a bit more?
> > > 
> > > Originally I thought that what Grant suggested would work ok with this
> > > patch.
> > 
> > I think the objection to these patches is that they special case the
> > instantiation of some devices. It's not a proper solution because it
> > implies various things. For example merely instantiating the IOMMU
> > device earlier than others is only going to work *if* the driver is
> > actually probed before the drivers of other devices. If you want to
> > build the driver as a module for example, probe order becomes entirely
> > non-deterministic.
> 
> I understand the above limitation. What I thought for the solution is
> that I can make use of iommu_bus even before IOMMU is
> instanciated. iommu_bus has its notifier which calls
> iommu_ops()->add_device(). This could return -EPROBE_DEFER when IOMMU
> isn't ready. Only the problem is the current "bus_notifier" doesn't
> have the ability to return error. I'll see if it can be modified. Even
> with this, at least IOMMU *driver* needs to be init'ed enough earlier
> to prevent devices from being registered without IOMMU.

The way notifiers work is that they run completely hidden from whatever
triggers them. For instance you register the IOMMU bus notifier from the
IOMMU driver (by calling bus_set_iommu()). That registers a function to
be called when some event happens on that bus. When a device's driver is
probed successfully, the driver core will notify the bus, which causes
the IOMMU callback to be run.

Some of this code runs before the driver has successfully been probed,
so I imagine it would be possible to use it to abort probing. But that's
not possible at least with the current code.

> > Instead of handling such dependencies implicitly by making sure all
> > resource providers are probed earlier than any of their consumers, the
> > dependencies are handled more explicitly, which turns out to simplify
> > things a lot. There's some additional work required in the core, but if
> > done consistently no driver needs to care about the dependencies and it
> > no longer matters where the resources come from. The problem is reduced
> > to essentially this:
> > 
> > 	while (!resource_available())
> > 		load_more_drivers();
> > 
> > So my proposed solution for the IOMMU case is to treat it the same as
> > any other resources. Perhaps resource isn't the right word, but at the
> > core the issue is the same. A device requires the services of an IOMMU
> > so that it can be put into the correct address space. If the IOMMU is
> > not available yet it cannot do that, so we simply return -EPROBE_DEFER
> > and cause the probe to be retried later.
> 
> This looks somewhat similar to the above iommu_bus notifier.
> 
> Is there any way to implement the same mechanism rather than using
> bus?

Yes, I think it should be possible to get this to work without using the
bus notifier at all. I can try to code something up but wanted to wait
for feedback from Grant first.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Hiroshi Doyu]

Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Fri, 25 Oct 2013 12:49:38 +0200:

> The way notifiers work is that they run completely hidden from whatever
> triggers them. For instance you register the IOMMU bus notifier from the
> IOMMU driver (by calling bus_set_iommu()). That registers a function to
> be called when some event happens on that bus. When a device's driver is
> probed successfully, the driver core will notify the bus, which causes
> the IOMMU callback to be run.
> 
> Some of this code runs before the driver has successfully been probed,
> so I imagine it would be possible to use it to abort probing. But that's
> not possible at least with the current code.

At least I'll experiment this way to see if it's acceptable or not.

> > This looks somewhat similar to the above iommu_bus notifier.
> > 
> > Is there any way to implement the same mechanism rather than using
> > bus?
> 
> Yes, I think it should be possible to get this to work without using the
> bus notifier at all. I can try to code something up but wanted to wait
> for feedback from Grant first.

That's interesting too.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Will Deacon]

On Fri, Oct 25, 2013 at 09:22:02AM +0100, Hiroshi Doyu wrote:
> Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Fri, 25 Oct 2013 09:56:55 +0200:
> 
> > I suspect that there will be enough differences between the various
> > IOMMU implementations that we won't be able to have a unified binding
> > (especially for how to associate devices with a particular virtual
> > address space), but perhaps that could be solved with something like an
> > .of_xlate() that IOMMU drivers implement, much like we've done with most
> > other subsystems too.
> > 
> > The binding for Tegra's IOMMU currently only uses the HW IDs (or a mask)
> > to put a device into a given address space, but I think that could be
> > easily extended to something like:
> > 
> > 	memory-clients = <&iommu MASK>;
> > 
> > Or similar. If other information is required, we could encode that into
> > a multi-cell specifier. Perhaps we could even leave away the phandle
> > since typically there will only be a single IOMMU in the system?
> > 
> > Does that sound reasonable? Hiroshi is much more familiar with IOMMUs,
> > so I'd like to get his opinion on the above as well.
> 
> I think that the above may be possible, but I'd like to listen from
> other IOMMU SOC maintainers.
> 
> A brief explanation for "memory-clients":
> 
> In tegra, every H/W has its own memory-client ID, and it can be
> associated to any address spaces. The above "memory-cilents" is used
> to indicate which ID a device has in DT. If the other SOC IOMMUs need
> this kind of "memory-clients", this would be standarized. Any comment?
> 
> At least arm-smmu seems to have the following. multiple IOMMUs can be
> handled with this.
> 
> 
> - smmu-parent   : When multiple SMMUs are chained together, this
>                   property can be used to provide a phandle to the
>                   parent SMMU (that is the next SMMU on the path going
>                   from the mmu-masters towards memory) node for this
>                   SMMU.

This property is for the case when IOMMUs are connected in series, which is
fairly horrendous. However, it is extremely common to have multiple IOMMU
instances within an Soc (Exynos SoCs have one IOMMU per device iirc), so
that certainly needs to be handled.

Will

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2748 bytes --]

On Fri, Oct 25, 2013 at 02:20:51PM +0100, Will Deacon wrote:
> On Fri, Oct 25, 2013 at 09:22:02AM +0100, Hiroshi Doyu wrote:
> > Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Fri, 25 Oct 2013 09:56:55 +0200:
> > 
> > > I suspect that there will be enough differences between the various
> > > IOMMU implementations that we won't be able to have a unified binding
> > > (especially for how to associate devices with a particular virtual
> > > address space), but perhaps that could be solved with something like an
> > > .of_xlate() that IOMMU drivers implement, much like we've done with most
> > > other subsystems too.
> > > 
> > > The binding for Tegra's IOMMU currently only uses the HW IDs (or a mask)
> > > to put a device into a given address space, but I think that could be
> > > easily extended to something like:
> > > 
> > > 	memory-clients = <&iommu MASK>;
> > > 
> > > Or similar. If other information is required, we could encode that into
> > > a multi-cell specifier. Perhaps we could even leave away the phandle
> > > since typically there will only be a single IOMMU in the system?
> > > 
> > > Does that sound reasonable? Hiroshi is much more familiar with IOMMUs,
> > > so I'd like to get his opinion on the above as well.
> > 
> > I think that the above may be possible, but I'd like to listen from
> > other IOMMU SOC maintainers.
> > 
> > A brief explanation for "memory-clients":
> > 
> > In tegra, every H/W has its own memory-client ID, and it can be
> > associated to any address spaces. The above "memory-cilents" is used
> > to indicate which ID a device has in DT. If the other SOC IOMMUs need
> > this kind of "memory-clients", this would be standarized. Any comment?
> > 
> > At least arm-smmu seems to have the following. multiple IOMMUs can be
> > handled with this.
> > 
> > 
> > - smmu-parent   : When multiple SMMUs are chained together, this
> >                   property can be used to provide a phandle to the
> >                   parent SMMU (that is the next SMMU on the path going
> >                   from the mmu-masters towards memory) node for this
> >                   SMMU.
> 
> This property is for the case when IOMMUs are connected in series, which is
> fairly horrendous. However, it is extremely common to have multiple IOMMU
> instances within an Soc (Exynos SoCs have one IOMMU per device iirc), so
> that certainly needs to be handled.

Okay, in that case perhaps some sort of generic binding should mandate
the phandle, even for systems that only have a single one. I suspect a
generic variant of smmu-parent (iommu-parent?) would also be useful to
describe setups with cascaded IOMMUs.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Grant Likely]

On Fri, 25 Oct 2013 12:49:38 +0200, Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Oct 25, 2013 at 11:49:05AM +0200, Hiroshi Doyu wrote:
> > Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Fri, 25 Oct 2013 11:11:05 +0200:
> > 
> > > > > This is actually "the other problem that I'm aware of that could benefit
> > > > > from [interrupt resolution at probe time]". My idea was that once we had
> > > > > a way within the driver core to resolve interrupt references at probe
> > > > > time it could be used for potentially many other resources as well. Some
> > > > > of the resources like GPIOs and regulators are obviously not something
> > > > > that the core can or should be requesting, but mostly resources that you
> > > > > don't actually need to control after probing (such as interrupts) would
> > > > > be a good fit because otherwise people would write the same boilerplate
> > > > > over and over again.
> > > > > 
> > > > > IOMMUs seem to me to be in that same category. As far as I can tell, an
> > > > > IOMMU driver registers the IOMMU for a given bus, upon which every
> > > > > device can simply be used (mostly transparently) with that IOMMU. While
> > > > > I haven't figured out how exactly, I'm pretty sure we can take advantage
> > > > > of the resolution of resources at probe time within the core to both
> > > > > keep drivers from having to do anything in particular and at the same
> > > > > time have code to determine if the IOMMU driver hasn't been probed yet
> > > > > and return -EPROBE_DEFER appropriately.
> > > > 
> > > > Can you explain the above a bit more?
> > > > 
> > > > Originally I thought that what Grant suggested would work ok with this
> > > > patch.
> > > 
> > > I think the objection to these patches is that they special case the
> > > instantiation of some devices. It's not a proper solution because it
> > > implies various things. For example merely instantiating the IOMMU
> > > device earlier than others is only going to work *if* the driver is
> > > actually probed before the drivers of other devices. If you want to
> > > build the driver as a module for example, probe order becomes entirely
> > > non-deterministic.
> > 
> > I understand the above limitation. What I thought for the solution is
> > that I can make use of iommu_bus even before IOMMU is
> > instanciated. iommu_bus has its notifier which calls
> > iommu_ops()->add_device(). This could return -EPROBE_DEFER when IOMMU
> > isn't ready. Only the problem is the current "bus_notifier" doesn't
> > have the ability to return error. I'll see if it can be modified. Even
> > with this, at least IOMMU *driver* needs to be init'ed enough earlier
> > to prevent devices from being registered without IOMMU.
> 
> The way notifiers work is that they run completely hidden from whatever
> triggers them. For instance you register the IOMMU bus notifier from the
> IOMMU driver (by calling bus_set_iommu()). That registers a function to
> be called when some event happens on that bus. When a device's driver is
> probed successfully, the driver core will notify the bus, which causes
> the IOMMU callback to be run.
> 
> Some of this code runs before the driver has successfully been probed,
> so I imagine it would be possible to use it to abort probing. But that's
> not possible at least with the current code.
> 
> > > Instead of handling such dependencies implicitly by making sure all
> > > resource providers are probed earlier than any of their consumers, the
> > > dependencies are handled more explicitly, which turns out to simplify
> > > things a lot. There's some additional work required in the core, but if
> > > done consistently no driver needs to care about the dependencies and it
> > > no longer matters where the resources come from. The problem is reduced
> > > to essentially this:
> > > 
> > > 	while (!resource_available())
> > > 		load_more_drivers();
> > > 
> > > So my proposed solution for the IOMMU case is to treat it the same as
> > > any other resources. Perhaps resource isn't the right word, but at the
> > > core the issue is the same. A device requires the services of an IOMMU
> > > so that it can be put into the correct address space. If the IOMMU is
> > > not available yet it cannot do that, so we simply return -EPROBE_DEFER
> > > and cause the probe to be retried later.
> > 
> > This looks somewhat similar to the above iommu_bus notifier.
> > 
> > Is there any way to implement the same mechanism rather than using
> > bus?
> 
> Yes, I think it should be possible to get this to work without using the
> bus notifier at all. I can try to code something up but wanted to wait
> for feedback from Grant first.

I've lost track. What feedback are you waiting for from me? I've not dug
into this entire series so I may not provide clueful feedback.

g.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5400 bytes --]

On Fri, Oct 25, 2013 at 08:01:36PM +0100, Grant Likely wrote:
> On Fri, 25 Oct 2013 12:49:38 +0200, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Fri, Oct 25, 2013 at 11:49:05AM +0200, Hiroshi Doyu wrote:
> > > Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Fri, 25 Oct 2013 11:11:05 +0200:
> > > 
> > > > > > This is actually "the other problem that I'm aware of that could benefit
> > > > > > from [interrupt resolution at probe time]". My idea was that once we had
> > > > > > a way within the driver core to resolve interrupt references at probe
> > > > > > time it could be used for potentially many other resources as well. Some
> > > > > > of the resources like GPIOs and regulators are obviously not something
> > > > > > that the core can or should be requesting, but mostly resources that you
> > > > > > don't actually need to control after probing (such as interrupts) would
> > > > > > be a good fit because otherwise people would write the same boilerplate
> > > > > > over and over again.
> > > > > > 
> > > > > > IOMMUs seem to me to be in that same category. As far as I can tell, an
> > > > > > IOMMU driver registers the IOMMU for a given bus, upon which every
> > > > > > device can simply be used (mostly transparently) with that IOMMU. While
> > > > > > I haven't figured out how exactly, I'm pretty sure we can take advantage
> > > > > > of the resolution of resources at probe time within the core to both
> > > > > > keep drivers from having to do anything in particular and at the same
> > > > > > time have code to determine if the IOMMU driver hasn't been probed yet
> > > > > > and return -EPROBE_DEFER appropriately.
> > > > > 
> > > > > Can you explain the above a bit more?
> > > > > 
> > > > > Originally I thought that what Grant suggested would work ok with this
> > > > > patch.
> > > > 
> > > > I think the objection to these patches is that they special case the
> > > > instantiation of some devices. It's not a proper solution because it
> > > > implies various things. For example merely instantiating the IOMMU
> > > > device earlier than others is only going to work *if* the driver is
> > > > actually probed before the drivers of other devices. If you want to
> > > > build the driver as a module for example, probe order becomes entirely
> > > > non-deterministic.
> > > 
> > > I understand the above limitation. What I thought for the solution is
> > > that I can make use of iommu_bus even before IOMMU is
> > > instanciated. iommu_bus has its notifier which calls
> > > iommu_ops()->add_device(). This could return -EPROBE_DEFER when IOMMU
> > > isn't ready. Only the problem is the current "bus_notifier" doesn't
> > > have the ability to return error. I'll see if it can be modified. Even
> > > with this, at least IOMMU *driver* needs to be init'ed enough earlier
> > > to prevent devices from being registered without IOMMU.
> > 
> > The way notifiers work is that they run completely hidden from whatever
> > triggers them. For instance you register the IOMMU bus notifier from the
> > IOMMU driver (by calling bus_set_iommu()). That registers a function to
> > be called when some event happens on that bus. When a device's driver is
> > probed successfully, the driver core will notify the bus, which causes
> > the IOMMU callback to be run.
> > 
> > Some of this code runs before the driver has successfully been probed,
> > so I imagine it would be possible to use it to abort probing. But that's
> > not possible at least with the current code.
> > 
> > > > Instead of handling such dependencies implicitly by making sure all
> > > > resource providers are probed earlier than any of their consumers, the
> > > > dependencies are handled more explicitly, which turns out to simplify
> > > > things a lot. There's some additional work required in the core, but if
> > > > done consistently no driver needs to care about the dependencies and it
> > > > no longer matters where the resources come from. The problem is reduced
> > > > to essentially this:
> > > > 
> > > > 	while (!resource_available())
> > > > 		load_more_drivers();
> > > > 
> > > > So my proposed solution for the IOMMU case is to treat it the same as
> > > > any other resources. Perhaps resource isn't the right word, but at the
> > > > core the issue is the same. A device requires the services of an IOMMU
> > > > so that it can be put into the correct address space. If the IOMMU is
> > > > not available yet it cannot do that, so we simply return -EPROBE_DEFER
> > > > and cause the probe to be retried later.
> > > 
> > > This looks somewhat similar to the above iommu_bus notifier.
> > > 
> > > Is there any way to implement the same mechanism rather than using
> > > bus?
> > 
> > Yes, I think it should be possible to get this to work without using the
> > bus notifier at all. I can try to code something up but wanted to wait
> > for feedback from Grant first.
> 
> I've lost track. What feedback are you waiting for from me? I've not dug
> into this entire series so I may not provide clueful feedback.

I think the context was lost. Here's a link to the relevant message:

	http://www.spinics.net/lists/devicetree/msg09709.html

The message ID is:

	20131025075652.GB19622-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv3 00/19] Unifying SMMU driver among Tegra SoCs

[Hiroshi Doyu]

Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote @ Fri, 18 Oct 2013 12:26:41 +0200:

> Hi,
> 
> This series provides:
> 
> (1) Unified SMMU driver among Tegra SoCs
> (2) Multiple Address Space support(MASID) in IOMMU(SMMMU)
> (3) Tegra IOMMU'able devices, most of platform devices are IOMMU'able.
> 
> There's some discussion[1] about device population order, which could
> solve the following patches.
> 
>   patch 1: [HACK] of: dev_node has struct device pointer
>   patch 2: [HACK] ARM: tegra: Populate AHB/IOMMU earlier than others
>   patch 3: [HACK] amba: Move AHB to core_initcall
>   patch 4: [HACK] iommu/tegra: smmu: Move IOMMU to core_initcall

Joerg, Stephen,

Would it be possible to merge this series without the above ordering patches?

Re: [PATCHv3 00/19] Unifying SMMU driver among Tegra SoCs

[Stephen Warren]

On 10/30/2013 10:34 AM, Hiroshi Doyu wrote:
> Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote @ Fri, 18 Oct 2013 12:26:41 +0200:
> 
>> Hi,
>>
>> This series provides:
>>
>> (1) Unified SMMU driver among Tegra SoCs
>> (2) Multiple Address Space support(MASID) in IOMMU(SMMMU)
>> (3) Tegra IOMMU'able devices, most of platform devices are IOMMU'able.
>>
>> There's some discussion[1] about device population order, which could
>> solve the following patches.
>>
>>   patch 1: [HACK] of: dev_node has struct device pointer
>>   patch 2: [HACK] ARM: tegra: Populate AHB/IOMMU earlier than others
>>   patch 3: [HACK] amba: Move AHB to core_initcall
>>   patch 4: [HACK] iommu/tegra: smmu: Move IOMMU to core_initcall
> 
> Joerg, Stephen,
> 
> Would it be possible to merge this series without the above ordering patches?

What are the dependencies between the patches? Surely if those patches
were at the start of the series, the rest relies on them. Otherwise, why
was this all once series?

Anyway, I haven't had a chance to look at this yet, and it was sent too
late for 3.13. Hopefully we can work this out and apply it early for 3.14.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Stephen Warren]

On 10/25/2013 07:20 AM, Will Deacon wrote:
> On Fri, Oct 25, 2013 at 09:22:02AM +0100, Hiroshi Doyu wrote:
>> Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Fri, 25 Oct 2013 09:56:55 +0200:
>>
>>> I suspect that there will be enough differences between the various
>>> IOMMU implementations that we won't be able to have a unified binding
>>> (especially for how to associate devices with a particular virtual
>>> address space), but perhaps that could be solved with something like an
>>> .of_xlate() that IOMMU drivers implement, much like we've done with most
>>> other subsystems too.
>>>
>>> The binding for Tegra's IOMMU currently only uses the HW IDs (or a mask)
>>> to put a device into a given address space, but I think that could be
>>> easily extended to something like:
>>>
>>> 	memory-clients = <&iommu MASK>;
>>>
>>> Or similar. If other information is required, we could encode that into
>>> a multi-cell specifier. Perhaps we could even leave away the phandle
>>> since typically there will only be a single IOMMU in the system?
>>>
>>> Does that sound reasonable? Hiroshi is much more familiar with IOMMUs,
>>> so I'd like to get his opinion on the above as well.
>>
>> I think that the above may be possible, but I'd like to listen from
>> other IOMMU SOC maintainers.
>>
>> A brief explanation for "memory-clients":
>>
>> In tegra, every H/W has its own memory-client ID, and it can be
>> associated to any address spaces. The above "memory-cilents" is used
>> to indicate which ID a device has in DT. If the other SOC IOMMUs need
>> this kind of "memory-clients", this would be standarized. Any comment?
>>
>> At least arm-smmu seems to have the following. multiple IOMMUs can be
>> handled with this.
>>
>>
>> - smmu-parent   : When multiple SMMUs are chained together, this
>>                   property can be used to provide a phandle to the
>>                   parent SMMU (that is the next SMMU on the path going
>>                   from the mmu-masters towards memory) node for this
>>                   SMMU.
> 
> This property is for the case when IOMMUs are connected in series, which is
> fairly horrendous. However, it is extremely common to have multiple IOMMU
> instances within an Soc (Exynos SoCs have one IOMMU per device iirc), so
> that certainly needs to be handled.

That property sounds a bit scary; is the value a single parent IOMMU?
What if an IOMMU splits its requests between two different parent IOMMUs
based on some property of the request. I hope smmu-parent isn't going to
get us into another interrupts -vs- interrupts-extended situation.

(/me checks the documentation of that property)

I suppose smmu-parent is documented in a device-specific binding rather
than as a generic property, although there's no vendor-prefix on the
property name:-(, so we havne't cornered ourselves too much here; any
IOMMU device which actually has multiple parents can always define its
own custom property to represent that.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Stephen Warren]

On 10/25/2013 03:11 AM, Thierry Reding wrote:
...
> So my proposed solution for the IOMMU case is to treat it the same
> as any other resources. Perhaps resource isn't the right word, but
> at the core the issue is the same. A device requires the services
> of an IOMMU so that it can be put into the correct address space.
> If the IOMMU is not available yet it cannot do that, so we simply
> return -EPROBE_DEFER and cause the probe to be retried later.

Personally, I view deferred probe as being used when one device
requires either a resource /or/ a service provided by another, not
/just/ when there's a resource dependency. Hence, I think it fits
perfectly here.

So I agree with Thierry: In other words, I think the solution is for
all devices that are affected by an IOMMU to have a property such as:

iommu = <&iommu_phandle iommu_specifier>;

(and the DT node for the IOMMU will contain e.g. an #iommu-cells property)

... and for the driver to explicitly parse that property, and wait
until the driver for iommu_phandle is ready. Exactly the same as any
other resource/service dependency.

That will solve all the problems.

The only downside is that every driver needs to contain code to parse
that property. However, I think that's just one function call; the
actual implementation of that function can be unified somewhere inside
core code in drivers/iommu/.

Re: [PATCHv3 02/19] [HACK] ARM: tegra: Populate AHB/IOMMU earlier than others

[Stephen Warren]

On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
> Populate AHB/IOMMU earlier than others. IOMMU depends on AHB. IOMMU
> needs to be instanciated earlier than others so that IOMMU can
> register other platform devices as IOMMU'able. Once IOMMU is
> populated, IOMMU/AHB nodes are detached to prevent another
> registeration.

I think NAK on patches 2-4; this is just too fragile, and the opposite
direction of where we've been going.

Also once we get to ARMv8, we apparently won't be able to have a per-SoC
file like tegra.c in which to put hacks like this, so it'll never work
there even if it does for ARMv7.

Re: [PATCHv3 05/19] ARM: dt: tegra114: iommu: Fix IOMMU register address

[Stephen Warren]

On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
> Fix IOMMU register address.

That's quite unfortunate. Why do we always have trouble getting the
IOMMU register address correct in DT; I think we had a similar issue
with the Tegra30 DT.

Anyway, I guess I should take this for 3.13 and stable since it's a bugfix.

Re: [PATCHv3 06/19] iommu/tegra: smmu: Select ARM_DMA_USE_IOMMU in Kconfig

[Stephen Warren]

On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
> SMMU expects ARM_DMA_USE_IOMMU to be enabled to use DMA mapping API by
> default.

Is the select required for the driver to be useful at all (in which case
it's fine), or simply for things to work "better" (define "better"), in
which case perhaps leaving it optional is more correct.

Re: [PATCHv3 07/19] iommu/tegra: smmu: Create default IOVA maps

[Stephen Warren]

On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
> Create default IOVA maps at boot-up which can be attached to devices.

What's the content of those default maps? I assume no RAM is mapped,
judging by the code. If so, perhaps s/default/default empty/ in the
patch description?

> @@ -1236,7 +1261,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);
> -	return 0;
> +	return tegra_smmu_create_default_map(smmu);
>  }

If tegra_smmu_create_default_map() fails, don't you need to unwind e.g.
bus_set_iommu(), and any other setup that happened before?

Re: [PATCHv3 00/19] Unifying SMMU driver among Tegra SoCs

[Stephen Warren]

On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:

> v3:
> Updated based on Stephen Warren's feedback

Updated how? The last series was posted a long time ago, and I have no
idea what feedback I gave...

Re: [PATCHv3 10/19] iommu/tegra: smmu: Get "nvidia,swgroups" from DT

[Stephen Warren]

On 10/24/2013 02:58 AM, Grant Likely wrote:
> On Fri, 18 Oct 2013 13:26:51 +0300, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> This provides the info about which H/W Accelerators are supported on
>> Tegra SoC. This info is passed from DT. This is necessary to have the
>> unified SMMU driver among Tegra SoCs. Instead of using platform data,
>> DT passes "nvidia,swgroups" now. DT is mandatory in Tegra.
>>
>> Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>> @@ -1200,6 +1193,9 @@ static int tegra_smmu_probe(struct platform_device *pdev)
>>  	if (of_property_read_u32(dev->of_node, "nvidia,#asids", &asids))
>>  		return -ENODEV;
>>  
>> +	if (of_property_read_u64(dev->of_node, "nvidia,swgroups", &swgroups))
>> +		return -ENODEV;
>> +
> 
> Best practice: A new property has been created here. If the property
> isn't present, then it should default to the previous meaning instead of
> outright failing. (or make a strong argument that no one is using this
> yet and therefore is not breakage).

We definitely don't have anyone using this binding yet upstream, and
even the reg properties in the DT were wrong for the device, so it
couldn't possibly have worked:-(

Re: [PATCHv3 10/19] iommu/tegra: smmu: Get "nvidia,swgroups" from DT

[Stephen Warren]

On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
> This provides the info about which H/W Accelerators are supported on
> Tegra SoC. This info is passed from DT. This is necessary to have the
> unified SMMU driver among Tegra SoCs. Instead of using platform data,
> DT passes "nvidia,swgroups" now. DT is mandatory in Tegra.

> diff --git a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
> index 89fb543..6a844b3 100644
> --- a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
> +++ b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
> @@ -8,6 +8,11 @@ Required properties:
>  - nvidia,#asids : # of ASIDs
>  - dma-window : IOVA start address and length.
>  - nvidia,ahb : phandle to the ahb bus connected to SMMU.
> +- nvidia,swgroups: A bitmap of supported HardWare Accelerators(HWA).
> +  Each bit represents one swgroup. The assignments may be found in header
> +  file <dt-bindings/memory/tegra-swgroup.h>. Its max is 64. 2 cells

That file doesn't exist at this point in the series. I think you should
create a patch up-front that adds that header, and modifies the binding
document, all in one go, and separate from any driver code changes.
Separate DT/driver patches were IIRC agreed to be preferablte at the
recent ARM workshop.

> +  are required. This unique ID info can be used to calculate
> +  MC_SMMU_<SWGROUP name>_ASID_0 offset and HOTRESET bit.

I'm afraid I still don't quite understand what a swgroup is.

IIUC, the HW works like this based on comments in a previous patch:

Each bus-master attached to the MMU passes a "memory client ID" along
with the transaction. Some devices can generate transactions with
different "memory client IDs". There is a mapping inside the SMMU from
"memory client ID" to "address space ID". (I don't know what form that
mapping takes; can you point out where it's set up?). Each "address
space ID" has its own set of page tables. Is "swgroup" simply another
name for "memory client ID"? If so, it'd be good to use just one term
consistently.

Assuming "swgroup" is "memory client ID", why can't the driver just
create a list/... of known swgroups at runtime, based on the swgroup
values that each device uses, which would presumably be either
hard-coded in the client device's driver, or represented in the DT smmu
property's "iommu specifier" value.

> @@ -380,12 +383,10 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,
>  	if (!on)
>  		map = smmu_client_hwgrp(c);
>  
> -	for_each_set_bit(i, &map, HWGRP_COUNT) {
> +	for_each_set_bit(i, bitmap, sizeof(map) * BITS_PER_BYTE) {
>  		offs = HWGRP_ASID_REG(i);
>  		val = smmu_read(smmu, offs);
>  		if (on) {
> -			if (WARN_ON(val & mask))
> -				goto err_hw_busy;
>  			val |= mask;
>  		} else {
>  			WARN_ON((val & mask) == mask);
> @@ -396,15 +397,6 @@ static int __smmu_client_set_hwgrp(struct smmu_client *c,
>  	FLUSH_SMMU_REGS(smmu);
>  	c->hwgrp = map;
>  	return 0;
> -
> -err_hw_busy:
> -	for_each_set_bit(i, &map, HWGRP_COUNT) {
> -		offs = HWGRP_ASID_REG(i);
> -		val = smmu_read(smmu, offs);
> -		val &= ~mask;
> -		smmu_write(smmu, val, offs);
> -	}
> -	return -EBUSY;
>  }

I must admit to having zero idea what that part of the patch is doing
semantically. Some form of explanation in the commit description would
be useful.

Re: [PATCHv3 11/19] ARM: dt: tegra30: iommu: Add "nvidia,swgroups"

[Stephen Warren]

On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
> This is a bitmap that indicates which HardWare Accelerators(HWA) are
> supported on Tegra30 SoC.

Feel free to squash patches 11 and 12 into one. They're doing the same
logical thing.

Also, can you please separate the series into separate series for the
IOMMU driver and DT changes? I hope there are no dependencies between
the two (especially since the driver clearly isn't working yet due to
the incorrect reg values in the Tegra114 DT) and hence the DT and driver
changes can be applied in any order?

Re: [PATCHv3 13/19] iommu/tegra: smmu: Workaround PCIe IOMMU'able

[Stephen Warren]

On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
> Make PCIe work as it is. IOMMU support can be implemented later. We
> need the same kind of registration framework for PCIe as platform_bus
> does currently.

Some more explanation is needed here re: exactly why PCIe doesn't work,
and how this patch makes it work.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2511 bytes --]

On Wed, Oct 30, 2013 at 03:58:58PM -0600, Stephen Warren wrote:
> On 10/25/2013 03:11 AM, Thierry Reding wrote:
> ...
> > So my proposed solution for the IOMMU case is to treat it the same
> > as any other resources. Perhaps resource isn't the right word, but
> > at the core the issue is the same. A device requires the services
> > of an IOMMU so that it can be put into the correct address space.
> > If the IOMMU is not available yet it cannot do that, so we simply
> > return -EPROBE_DEFER and cause the probe to be retried later.
> 
> Personally, I view deferred probe as being used when one device
> requires either a resource /or/ a service provided by another, not
> /just/ when there's a resource dependency. Hence, I think it fits
> perfectly here.
> 
> So I agree with Thierry: In other words, I think the solution is for
> all devices that are affected by an IOMMU to have a property such as:
> 
> iommu = <&iommu_phandle iommu_specifier>;
> 
> (and the DT node for the IOMMU will contain e.g. an #iommu-cells property)
> 
> ... and for the driver to explicitly parse that property, and wait
> until the driver for iommu_phandle is ready. Exactly the same as any
> other resource/service dependency.
> 
> That will solve all the problems.
> 
> The only downside is that every driver needs to contain code to parse
> that property. However, I think that's just one function call; the
> actual implementation of that function can be unified somewhere inside
> core code in drivers/iommu/.

My earlier proposal for deferred interrupt reference resolution actually
tries to solve that problem within the core. Essentially what it does is
add a new function that gets called right before the driver's .probe()
function, so that it can parse standard resources and services from DT,
such as the interrupts property. This could theoretically be done for
other resources such as reg as well, but it really only matters where
the resource can be dynamic (as is the case for interrupts).

In this case I would envision a standard OF function to associate a
device with its IOMMU. Perhaps something like:

	int of_iommu_attach(struct device *dev);

That could be called by the core independent of the specific device and
IOMMU. IOMMU-specifics can probably be handled using .of_xlate(), quite
in a similar way to GPIO or regulators.

That way drivers can remain agnostic of the IOMMU while still being able
to take advantage of deferred probing.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Stephen Warren]

On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
> This provides the info about which swgroups a device belongs to. This
> info is passed from DT. This is necessary for the unified SMMU driver

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

> @@ -13,9 +13,12 @@ Required properties:
>    file <dt-bindings/memory/tegra-swgroup.h>. Its max is 64. 2 cells
>    are required. This unique ID info can be used to calculate
>    MC_SMMU_<SWGROUP name>_ASID_0 offset and HOTRESET bit.
> +- nvidia,memory-clients: phandle to a smmu 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.

You added that to a list of properties that are required for the SMMU
node. However, judging by the example below, this property should be
added to the SMMU client nodes. Please keep the two lists separate, for
example:

Required properties for SMMU node:
...

Required properties for SMMU client nodes:
...

> +	host1x {
> +		compatible = "nvidia,tegra30-host1x", "simple-bus";
> +		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_HC>;
> +		....
> +		gr3d {
> +			compatible = "nvidia,tegra30-gr3d";
> +			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_NV
> +						       TEGRA_SWGROUP_NV2>;

Why one cell for the host1x property, and two cells for the gr3d
property; shouldn't they be the same length?

Bikeshedding a bit, but "nvidia,smmu" seems like it'd be a better
property name, since the property is really about defining which other
node is the SMMU that affects the device, and the SWGROUP IDs are just
part of the SMMU specifier that goes along with the phandle.

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

> +static u64 smmu_of_get_memory_client(struct device *dev)

> +	np = of_parse_phandle(dev->of_node, propname, 0);
> +	if (np != smmu_handle->dev->of_node)
> +		return ~0;
> +
> +	prop = of_get_property(dev->of_node, propname, &bytes);
> +	if (!prop || !bytes)
> +		return ~0;

of_parse_phandle_with_fixed_args() might be a good fit here, or perhaps
require a property #smmu-cells in the SMMU node, thus allowing the
non-fixed-length of_parse_phandle_with_args() to be used here.

> +	for (i = 1; i < bytes / sizeof(u32); i++, prop++)
> +		swgroup |= 1ULL << be32_to_cpup(prop);

Don't you need to shift swgroup too? I thought the two cells in DT were
LSB and MSB, yet here you're ORing both cells into the LSBs.

Re: [PATCHv3 15/19] ARM: tegra: Create a DT header defining SWGROUP ID

[Stephen Warren]

On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
> Create a header file to define the swgroup IDs used by the IOMMU(SMMU)
> binding. "swgroups" is a group of H/W clients which Tegra SoCs
> support. 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.

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

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

Isn't the DT match all 32-bit, such that the ULL suffix does nothing,
and TEGRA_SWGROUP_BIT(PPCS2) == 1<<32 == fail?

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Hiroshi Doyu]

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 22:58:58 +0100:

> On 10/25/2013 03:11 AM, Thierry Reding wrote:
> ...
> > So my proposed solution for the IOMMU case is to treat it the same
> > as any other resources. Perhaps resource isn't the right word, but
> > at the core the issue is the same. A device requires the services
> > of an IOMMU so that it can be put into the correct address space.
> > If the IOMMU is not available yet it cannot do that, so we simply
> > return -EPROBE_DEFER and cause the probe to be retried later.
> 
> Personally, I view deferred probe as being used when one device
> requires either a resource /or/ a service provided by another, not
> /just/ when there's a resource dependency. Hence, I think it fits
> perfectly here.
> 
> So I agree with Thierry: In other words, I think the solution is for
> all devices that are affected by an IOMMU to have a property such as:
> 
> iommu = <&iommu_phandle iommu_specifier>;

Agree

> (and the DT node for the IOMMU will contain e.g. an #iommu-cells property)

As explained in another mail[1], "#iommu-cells" could vary, depending
on a device since it's arbitral number of arguments(IDs) right
now. This could be a fixed size of bitmap(64), 2 cells since we know
"64" would be enough in the future as well as below.

Now:

	deviceA {
	       iommu = <&smmu 6>;
	};

	deviceB {
	       iommu = <&smmu 2 16>;
	};

New:
	smmu: iommu {
	      #iommu-cells = <2>;
	};

	deviceA {
	       iommu = <&smmu 0x00000000 0x00000040>;
	};

	deviceB {
	       iommu = <&smmu 0x00000000 0x00000104>;
	};

But then we cannot use SWGROUP ID "macros" in DT files since DTC
cannot perse OR("|") operations as below. Or can we?

#define SWGROUP_ID_A BIT(2)
#define SWGROUP_ID_B BIT(16)

	deviceB {
	       iommu = <&smmu  SWGROUP_ID_A | SWGROUP_ID_B>;
	};

So I thought that arbitral length of arguments may be easier to read as
below:

#define SWGROUP_ID_A  2
#define SWGROUP_ID_B 16

	deviceB {
	       iommu = <&smmu SWGROUP_ID_A
	       	       	      SWGROUP_ID_B>;
	};


> ... and for the driver to explicitly parse that property, and wait
> until the driver for iommu_phandle is ready. Exactly the same as any
> other resource/service dependency.
> 
> That will solve all the problems.
> 
> The only downside is that every driver needs to contain code to parse
> that property. However, I think that's just one function call; the
> actual implementation of that function can be unified somewhere inside
> core code in drivers/iommu/.

Yes, but only missing part now is that, we could do this with
"bus_notifier", but the current bus_notifier doesn't have the feature
to return error(-EPROBE_DEFER). This could be modified so that
bus_notifier could return (-EPROBE_DEFER) to postpone
probing. Alternatively this could be done in some core probe code as
well as Thierry pointed out.

[1] In the reply of "[PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT"

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Hiroshi Doyu]

Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:41:09 +0100:

> My earlier proposal for deferred interrupt reference resolution actually
> tries to solve that problem within the core. Essentially what it does is
> add a new function that gets called right before the driver's .probe()
> function, so that it can parse standard resources and services from DT,
> such as the interrupts property. This could theoretically be done for
> other resources such as reg as well, but it really only matters where
> the resource can be dynamic (as is the case for interrupts).
> 
> In this case I would envision a standard OF function to associate a
> device with its IOMMU. Perhaps something like:
> 
> 	int of_iommu_attach(struct device *dev);
> 
> That could be called by the core independent of the specific device and
> IOMMU. IOMMU-specifics can probably be handled using .of_xlate(), quite
> in a similar way to GPIO or regulators.
> 
> That way drivers can remain agnostic of the IOMMU while still being able
> to take advantage of deferred probing.

This could be simpler than making "bus_notifier" return "-EPROBE_DEFER".

The disadvantage of this is that we may need to provide the similar
of_<"subsystem">_attach() per subsytem if needed.

The version of "bus_notifier()" may be flexible since it just uses
the existing "bus_notifier()" framework although the return value
needs to be checked. This impact on the existing code isn't so small
unfortunately since NOTIFY_{DONE,OK,STOP,MASK,BAD} macros are used a
lot:(

I'll implement the version of "bus_notifier()" experimentally to see
how this looks like.

Re: [PATCHv3 05/19] ARM: dt: tegra114: iommu: Fix IOMMU register address

[Hiroshi Doyu]

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:07:17 +0100:

> On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
> > Fix IOMMU register address.
> 
> That's quite unfortunate. Why do we always have trouble getting the
> IOMMU register address correct in DT; I think we had a similar issue
> with the Tegra30 DT.

FYI: It was the same T114, which means, this one. We just haven't had
     a chance to get this series in yet.

> Anyway, I guess I should take this for 3.13 and stable since it's a bugfix.

Thanks. It helps.

Re: [PATCHv3 06/19] iommu/tegra: smmu: Select ARM_DMA_USE_IOMMU in Kconfig

[Hiroshi Doyu]

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:15:00 +0100:

> On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
> > SMMU expects ARM_DMA_USE_IOMMU to be enabled to use DMA mapping API by
> > default.
> 
> Is the select required for the driver to be useful at all (in which case
> it's fine), or simply for things to work "better" (define "better"), in
> which case perhaps leaving it optional is more correct.

Practically DMA API is the only interface for IOMMU(ARM_DMA_USE_IOMMU)
for us, but technically they are/should be independent.

SMMU driver attaches a device to some specific "map" out of multiple
maps. "map" is the concept from DMA API. Attaching a device to a
specific "map" belongs to "policy", which board files used to deal
with. But I couldn't find any good place to do this now. Any idea?

Re: [PATCHv3 10/19] iommu/tegra: smmu: Get "nvidia,swgroups" from DT

[Hiroshi Doyu]

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:33:32 +0100:

> > +  are required. This unique ID info can be used to calculate
> > +  MC_SMMU_<SWGROUP name>_ASID_0 offset and HOTRESET bit.
> 
> I'm afraid I still don't quite understand what a swgroup is.
> 
> IIUC, the HW works like this based on comments in a previous patch:
> 
> Each bus-master attached to the MMU passes a "memory client ID" along
> with the transaction. Some devices can generate transactions with
> different "memory client IDs". There is a mapping inside the SMMU from
> "memory client ID" to "address space ID". (I don't know what form that
> mapping takes; can you point out where it's set up?). Each "address
> space ID" has its own set of page tables.

Right.
"memory client ID" is used to find out MC_SMMU_<swgroup>_ASID_0
register. This register is used to associate <swgroup> to address
space(AS). <swgroup> == H/W. <swgroup> can be attached to any AS.

> Is "swgroup" simply another name for "memory client ID"? If so, it'd
> be good to use just one term consistently.

I used the name "memory client ID" because this ID can be used to find
out HOTRESET bit in MC_CLIENT_HOTRESET_*_0 registers in addition to
find the MC_SMMU_<swgroup>_ASID_0 offset. But maybe it's easy to use
the consistent name as "swgroup". If laster HOTRESET wants automatic
calculation they could borrow/redefine the same ID list, just
replacing the prefix. What do you think?

> Assuming "swgroup" is "memory client ID",

Yes

> why can't the driver just
> create a list/... of known swgroups at runtime, based on the swgroup
> values that each device uses, which would presumably be either
> hard-coded in the client device's driver, or represented in the DT smmu
> property's "iommu specifier" value.

Why we have "nvidia,swgroups" is just to avoid a device node having a
wrong "nvidia,memory-clients" which is not supported by that Tegra
SoC, which could crash kernel eventually. This info may be residual
since we define both "nvidia,swgroups" and "nvidia,memory-clients" at
once in DT. I'll remove "nvidia,swgroups" in SMMU node.

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Hiroshi Doyu]

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:44:04 +0100:

> > +	host1x {
> > +		compatible = "nvidia,tegra30-host1x", "simple-bus";
> > +		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_HC>;
> > +		....
> > +		gr3d {
> > +			compatible = "nvidia,tegra30-gr3d";
> > +			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_NV
> > +						       TEGRA_SWGROUP_NV2>;
> 
> Why one cell for the host1x property, and two cells for the gr3d
> property; shouldn't they be the same length?

That can vary. Even a single device node in DT can belong to multiple
software groups.

> > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> 
> > +static u64 smmu_of_get_memory_client(struct device *dev)
> 
> > +	np = of_parse_phandle(dev->of_node, propname, 0);
> > +	if (np != smmu_handle->dev->of_node)
> > +		return ~0;
> > +
> > +	prop = of_get_property(dev->of_node, propname, &bytes);
> > +	if (!prop || !bytes)
> > +		return ~0;
> 
> of_parse_phandle_with_fixed_args() might be a good fit here, or perhaps
> require a property #smmu-cells in the SMMU node, thus allowing the
> non-fixed-length of_parse_phandle_with_args() to be used here.

I once considered of_parse_phandle_with_args() but I couldn't use that
since "#smmu-cells" needed to be fixed in smmu node, not in client
node. In this case, the number of argument(number of swgroup IDs)
varies per client device.

Re: [PATCHv3 15/19] ARM: tegra: Create a DT header defining SWGROUP ID

[Hiroshi Doyu]

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:48:38 +0100:

> On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
> > Create a header file to define the swgroup IDs used by the IOMMU(SMMU)
> > binding. "swgroups" is a group of H/W clients which Tegra SoCs
> > support. 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.
> 
> > diff --git a/include/dt-bindings/memory/tegra-swgroup.h b/include/dt-bindings/memory/tegra-swgroup.h
> 
> > +#define TEGRA_SWGROUP_PPCS2	32	/* 0xab0 */
> > +
> > +#define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)
> 
> Isn't the DT match all 32-bit, such that the ULL suffix does nothing,

For the future Tegra SoC, the number of HWA grows a bit more, and it's
supposed that "64" would be enough.

> and TEGRA_SWGROUP_BIT(PPCS2) == 1<<32 == fail?

I guess that C implict type conversion can handle this correctly.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Stephen Warren]

On 10/31/2013 02:12 AM, Hiroshi Doyu wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 22:58:58 +0100:
> 
>> On 10/25/2013 03:11 AM, Thierry Reding wrote:
>> ...
>>> So my proposed solution for the IOMMU case is to treat it the same
>>> as any other resources. Perhaps resource isn't the right word, but
>>> at the core the issue is the same. A device requires the services
>>> of an IOMMU so that it can be put into the correct address space.
>>> If the IOMMU is not available yet it cannot do that, so we simply
>>> return -EPROBE_DEFER and cause the probe to be retried later.
>>
>> Personally, I view deferred probe as being used when one device
>> requires either a resource /or/ a service provided by another, not
>> /just/ when there's a resource dependency. Hence, I think it fits
>> perfectly here.
>>
>> So I agree with Thierry: In other words, I think the solution is for
>> all devices that are affected by an IOMMU to have a property such as:
>>
>> iommu = <&iommu_phandle iommu_specifier>;
> 
> Agree
> 
>> (and the DT node for the IOMMU will contain e.g. an #iommu-cells property)
> 
> As explained in another mail[1], "#iommu-cells" could vary, depending
> on a device since it's arbitral number of arguments(IDs) right
> now. This could be a fixed size of bitmap(64), 2 cells since we know
> "64" would be enough in the future as well as below.

So, a device can generate transactions using n different IDs, yet those
IDs all fit into some specific number of bits if represented as a
bitmask rather than a list. The size of that bitmask would be a good
candidate for #iommu-cells.

...
> 	smmu: iommu {
> 	      #iommu-cells = <2>;
> 	};
> 
> 	deviceA {
> 	       iommu = <&smmu 0x00000000 0x00000040>;
> 	};
> 
> 	deviceB {
> 	       iommu = <&smmu 0x00000000 0x00000104>;
> 	};

So yes, that seems like a reasonable representation.

> But then we cannot use SWGROUP ID "macros" in DT files since DTC
> cannot perse OR("|") operations as below. Or can we?
...
> 	deviceB {
> 	       iommu = <&smmu  SWGROUP_ID_A | SWGROUP_ID_B>;
> 	};

dtc now supports math expressions. You need to wrap expressions in ()
for them to be recognized, so if you write the above as:

	       iommu = <&smmu (SWGROUP_ID_A | SWGROUP_ID_B)>;

... it should work fine.

> So I thought that arbitral length of arguments may be easier to read as
> below:
...
> 	deviceB {
> 	       iommu = <&smmu SWGROUP_ID_A
> 	       	       	      SWGROUP_ID_B>;
> 	};

The problem with that is that it doesn't allow for multiple IOMMUs to be
represented by one property. You need a fixed number (e.g. #iommu-cells)
of cells after the phandle in order to know where one IOMMU specifier
ends, and the next phandle/specifier starts. While multiple IOMMUs is
likely quite rare, I see no reason to force a lack of support for that
scenario by ignoring the standard phandle/fixed-length-specifier
property format.

>> ... and for the driver to explicitly parse that property, and wait
>> until the driver for iommu_phandle is ready. Exactly the same as any
>> other resource/service dependency.
>>
>> That will solve all the problems.
>>
>> The only downside is that every driver needs to contain code to parse
>> that property. However, I think that's just one function call; the
>> actual implementation of that function can be unified somewhere inside
>> core code in drivers/iommu/.
> 
> Yes, but only missing part now is that, we could do this with
> "bus_notifier", but the current bus_notifier doesn't have the feature
> to return error(-EPROBE_DEFER). This could be modified so that
> bus_notifier could return (-EPROBE_DEFER) to postpone
> probing. Alternatively this could be done in some core probe code as
> well as Thierry pointed out.
> 
> [1] In the reply of "[PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT"

I think this should be done explicitly in drivers. It's much simpler,
and doesn't encode any knowledge of driver-specific bindings into some
common bus notifier code.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Stephen Warren]

On 10/31/2013 02:14 AM, Hiroshi Doyu wrote:
> Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:41:09 +0100:
> 
>> My earlier proposal for deferred interrupt reference resolution actually
>> tries to solve that problem within the core. Essentially what it does is
>> add a new function that gets called right before the driver's .probe()
>> function, so that it can parse standard resources and services from DT,
>> such as the interrupts property. This could theoretically be done for
>> other resources such as reg as well, but it really only matters where
>> the resource can be dynamic (as is the case for interrupts).
>>
>> In this case I would envision a standard OF function to associate a
>> device with its IOMMU. Perhaps something like:
>>
>> 	int of_iommu_attach(struct device *dev);
>>
>> That could be called by the core independent of the specific device and
>> IOMMU. IOMMU-specifics can probably be handled using .of_xlate(), quite
>> in a similar way to GPIO or regulators.
>>
>> That way drivers can remain agnostic of the IOMMU while still being able
>> to take advantage of deferred probing.
> 
> This could be simpler than making "bus_notifier" return "-EPROBE_DEFER".
> 
> The disadvantage of this is that we may need to provide the similar
> of_<"subsystem">_attach() per subsytem if needed.

Well, "per subsystem" here means per subsystem that is providing
resources, not per subsystem that is consuming resources. How many
subsystems are you expecting to provide resources like this? At present,
I believe IOMMU is the only subsystem missing some kind of API for
clients to acquire the relevant resource. I don't think there's any
scalability problem here.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Hiroshi Doyu]

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Thu, 31 Oct 2013 17:35:24 +0100:

...
> >> ... and for the driver to explicitly parse that property, and wait
> >> until the driver for iommu_phandle is ready. Exactly the same as any
> >> other resource/service dependency.
> >>
> >> That will solve all the problems.
> >>
> >> The only downside is that every driver needs to contain code to parse
> >> that property. However, I think that's just one function call; the
> >> actual implementation of that function can be unified somewhere inside
> >> core code in drivers/iommu/.
> > 
> > Yes, but only missing part now is that, we could do this with
> > "bus_notifier", but the current bus_notifier doesn't have the feature
> > to return error(-EPROBE_DEFER). This could be modified so that
> > bus_notifier could return (-EPROBE_DEFER) to postpone
> > probing. Alternatively this could be done in some core probe code as
> > well as Thierry pointed out.
> > 
> > [1] In the reply of "[PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT"
> 
> I think this should be done explicitly in drivers. It's much
> simpler,

Yes, we need to insert some IOMMU specific code in driver?

> and doesn't encode any knowledge of driver-specific bindings into some
> common bus notifier code.

I think that we cannot do that in drivers. We want to use drivers as
they are without any modifications indicating its dependency on
IOMMU because most of drivers are existing ones which nvidia doesn't
implement. We want to set up any IOMMU related thingy implicitly,
hided from driver. That's why we need to do this in bus_notifier or
driver core code.

Re: [PATCHv3 12/19] ARM: dt: tegra114: iommu: Add "nvidia,swgroups"

[Mark Rutland]

On Fri, Oct 18, 2013 at 11:26:53AM +0100, Hiroshi Doyu wrote:
> This is a bitmap that indicates which HardWare Accelerators(HWA) are
> supported on Tegra114 SoC.

The title and patch disagree here; the patch _changes_ the existing
nvidia,swgroups property, and the commit message doesn't explain why.

Mark.

> 
> Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/boot/dts/tegra114.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
> index 8d42787..4626e8a 100644
> --- a/arch/arm/boot/dts/tegra114.dtsi
> +++ b/arch/arm/boot/dts/tegra114.dtsi
> @@ -323,7 +323,7 @@
>  		       0x70019228 0x074>;
>  		nvidia,#asids = <4>;
>  		dma-window = <0 0x40000000>;
> -		nvidia,swgroups = <0x18659fe>;
> +		nvidia,swgroups = <0x00000000 0x01b659fe>;
>  		nvidia,ahb = <&ahb>;
>  	};
>  
> -- 
> 1.8.1.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Hiroshi Doyu]

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Thu, 31 Oct 2013 17:37:40 +0100:

> On 10/31/2013 02:14 AM, Hiroshi Doyu wrote:
> > Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:41:09 +0100:
> > 
> >> My earlier proposal for deferred interrupt reference resolution actually
> >> tries to solve that problem within the core. Essentially what it does is
> >> add a new function that gets called right before the driver's .probe()
> >> function, so that it can parse standard resources and services from DT,
> >> such as the interrupts property. This could theoretically be done for
> >> other resources such as reg as well, but it really only matters where
> >> the resource can be dynamic (as is the case for interrupts).
> >>
> >> In this case I would envision a standard OF function to associate a
> >> device with its IOMMU. Perhaps something like:
> >>
> >> 	int of_iommu_attach(struct device *dev);
> >>
> >> That could be called by the core independent of the specific device and
> >> IOMMU. IOMMU-specifics can probably be handled using .of_xlate(), quite
> >> in a similar way to GPIO or regulators.
> >>
> >> That way drivers can remain agnostic of the IOMMU while still being able
> >> to take advantage of deferred probing.
> > 
> > This could be simpler than making "bus_notifier" return "-EPROBE_DEFER".
> > 
> > The disadvantage of this is that we may need to provide the similar
> > of_<"subsystem">_attach() per subsytem if needed.
> 
> Well, "per subsystem" here means per subsystem that is providing
> resources, not per subsystem that is consuming resources. How many
> subsystems are you expecting to provide resources like this? At present,
> I believe IOMMU is the only subsystem missing some kind of API for
> clients to acquire the relevant resource. I don't think there's any
> scalability problem here.

Honestly I have no idea for other subsystem which needs
of_<"subsystem">_attach(). But IIRC, it's almost similar one to the
following "pinctrl_bind_pins()"? Or something other way?

drivers/base/dd.c:

static int really_probe(struct device *dev, struct device_driver *drv)
{
	int ret = 0;

	atomic_inc(&probe_count);
	pr_debug("bus: '%s': %s: probing driver %s with device %s\n",
		 drv->bus->name, __func__, drv->name, dev_name(dev));
	WARN_ON(!list_empty(&dev->devres_head));

	dev->driver = drv;

	/* If using pinctrl, bind pins now before probing */
	ret = pinctrl_bind_pins(dev);
              ^^^^^^^^^^^^^^^^^^^^^^^

Re: [PATCHv3 05/19] ARM: dt: tegra114: iommu: Fix IOMMU register address

[Mark Rutland]

On Fri, Oct 18, 2013 at 11:26:46AM +0100, Hiroshi Doyu wrote:
> Fix IOMMU register address.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/boot/dts/tegra114.dtsi | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
> index 2905145..8d42787 100644
> --- a/arch/arm/boot/dts/tegra114.dtsi
> +++ b/arch/arm/boot/dts/tegra114.dtsi
> @@ -318,9 +318,9 @@
>  
>  	iommu {
>  		compatible = "nvidia,tegra114-smmu", "nvidia,tegra30-smmu";
> -		reg = <0x7000f010 0x02c
> -		       0x7000f1f0 0x010
> -		       0x7000f228 0x074>;
> +		reg = <0x70019010 0x02c
> +		       0x700191f0 0x010
> +		       0x70019228 0x074>;

Minor cleanup/consistency request: as all these lines are changing anyway,
would you mind bracketing each entry individually:

		reg = <0x70019010 0x02c>,
		      <0x700191f0 0x010>,
		      <0x70019228 0x074>;

Cheers,
Mark.

>  		nvidia,#asids = <4>;
>  		dma-window = <0 0x40000000>;
>  		nvidia,swgroups = <0x18659fe>;
> -- 
> 1.8.1.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCHv3 05/19] ARM: dt: tegra114: iommu: Fix IOMMU register address

[Hiroshi Doyu]

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]

Hi Mark,

Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote @ Thu, 31 Oct 2013 17:51:27 +0100:

> On Fri, Oct 18, 2013 at 11:26:46AM +0100, Hiroshi Doyu wrote:
> > Fix IOMMU register address.
> > 
> > Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> >  arch/arm/boot/dts/tegra114.dtsi | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
> > index 2905145..8d42787 100644
> > --- a/arch/arm/boot/dts/tegra114.dtsi
> > +++ b/arch/arm/boot/dts/tegra114.dtsi
> > @@ -318,9 +318,9 @@
> >  
> >  	iommu {
> >  		compatible = "nvidia,tegra114-smmu", "nvidia,tegra30-smmu";
> > -		reg = <0x7000f010 0x02c
> > -		       0x7000f1f0 0x010
> > -		       0x7000f228 0x074>;
> > +		reg = <0x70019010 0x02c
> > +		       0x700191f0 0x010
> > +		       0x70019228 0x074>;
> 
> Minor cleanup/consistency request: as all these lines are changing anyway,
> would you mind bracketing each entry individually:
> 
> 		reg = <0x70019010 0x02c>,
> 		      <0x700191f0 0x010>,
> 		      <0x70019228 0x074>;

No, at all. Attached the updated "[PATCHv3' 05/19]".

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ARM-dt-tegra114-iommu-Fix-IOMMU-register-address.patch --]
[-- Type: text/x-patch; name="0001-ARM-dt-tegra114-iommu-Fix-IOMMU-register-address.patch", Size: 1042 bytes --]

From d545c279d1af9e81743e668c5d99b1471c1e535b Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <hdoyu@nvidia.com>
Date: Mon, 17 Jun 2013 15:37:35 +0300
Subject: [PATCHv3' 05/19] ARM: dt: tegra114: iommu: Fix IOMMU register address

Fix IOMMU register address.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
v3': Update the format suggested by Mark Rutland.
---
 arch/arm/boot/dts/tegra114.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
index 2905145..bb18ae5 100644
--- a/arch/arm/boot/dts/tegra114.dtsi
+++ b/arch/arm/boot/dts/tegra114.dtsi
@@ -318,9 +318,9 @@
 
 	iommu {
 		compatible = "nvidia,tegra114-smmu", "nvidia,tegra30-smmu";
-		reg = <0x7000f010 0x02c
-		       0x7000f1f0 0x010
-		       0x7000f228 0x074>;
+		reg = <0x70019010 0x02c>,
+		       0x700191f0 0x010>,
+		       0x70019228 0x074>;
 		nvidia,#asids = <4>;
 		dma-window = <0 0x40000000>;
 		nvidia,swgroups = <0x18659fe>;
-- 
1.8.1.5

Re: [PATCHv3 05/19] ARM: dt: tegra114: iommu: Fix IOMMU register address

[Hiroshi Doyu]

[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]

Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote @ Thu, 31 Oct 2013 19:05:24 +0200 (EET):

> Hi Mark,
> 
> Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote @ Thu, 31 Oct 2013 17:51:27 +0100:
> 
> > On Fri, Oct 18, 2013 at 11:26:46AM +0100, Hiroshi Doyu wrote:
> > > Fix IOMMU register address.
> > > 
> > > Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  arch/arm/boot/dts/tegra114.dtsi | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
> > > index 2905145..8d42787 100644
> > > --- a/arch/arm/boot/dts/tegra114.dtsi
> > > +++ b/arch/arm/boot/dts/tegra114.dtsi
> > > @@ -318,9 +318,9 @@
> > >  
> > >  	iommu {
> > >  		compatible = "nvidia,tegra114-smmu", "nvidia,tegra30-smmu";
> > > -		reg = <0x7000f010 0x02c
> > > -		       0x7000f1f0 0x010
> > > -		       0x7000f228 0x074>;
> > > +		reg = <0x70019010 0x02c
> > > +		       0x700191f0 0x010
> > > +		       0x70019228 0x074>;
> > 
> > Minor cleanup/consistency request: as all these lines are changing anyway,
> > would you mind bracketing each entry individually:
> > 
> > 		reg = <0x70019010 0x02c>,
> > 		      <0x700191f0 0x010>,
> > 		      <0x70019228 0x074>;
> 
> No, at all. Attached the updated "[PATCHv3' 05/19]".

Oops, here's the right one, "[PATCHv3'' 05/19]".


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ARM-dt-tegra114-iommu-Fix-IOMMU-register-address.patch --]
[-- Type: text/x-patch; name="0001-ARM-dt-tegra114-iommu-Fix-IOMMU-register-address.patch", Size: 985 bytes --]

From 156a6ad059f902590094570697cb33e5b892d835 Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <hdoyu@nvidia.com>
Date: Mon, 17 Jun 2013 15:37:35 +0300
Subject: [PATCHv3'' 1/1] ARM: dt: tegra114: iommu: Fix IOMMU register address

Fix IOMMU register address.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 arch/arm/boot/dts/tegra114.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
index 2905145..e629042 100644
--- a/arch/arm/boot/dts/tegra114.dtsi
+++ b/arch/arm/boot/dts/tegra114.dtsi
@@ -318,9 +318,9 @@
 
 	iommu {
 		compatible = "nvidia,tegra114-smmu", "nvidia,tegra30-smmu";
-		reg = <0x7000f010 0x02c
-		       0x7000f1f0 0x010
-		       0x7000f228 0x074>;
+		reg = <0x70019010 0x02c>,
+		      <0x700191f0 0x010>,
+		      <0x70019228 0x074>;
 		nvidia,#asids = <4>;
 		dma-window = <0 0x40000000>;
 		nvidia,swgroups = <0x18659fe>;
-- 
1.8.1.5


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCHv3 10/19] iommu/tegra: smmu: Get "nvidia,swgroups" from DT

[Mark Rutland]

On Fri, Oct 18, 2013 at 11:26:51AM +0100, Hiroshi Doyu wrote:
> This provides the info about which H/W Accelerators are supported on
> Tegra SoC. This info is passed from DT. This is necessary to have the
> unified SMMU driver among Tegra SoCs. Instead of using platform data,
> DT passes "nvidia,swgroups" now. DT is mandatory in Tegra.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  .../bindings/iommu/nvidia,tegra30-smmu.txt         |  6 +++++
>  drivers/iommu/tegra-smmu.c                         | 31 ++++++++++------------
>  2 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
> index 89fb543..6a844b3 100644
> --- a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
> +++ b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
> @@ -8,6 +8,11 @@ Required properties:
>  - nvidia,#asids : # of ASIDs
>  - dma-window : IOVA start address and length.
>  - nvidia,ahb : phandle to the ahb bus connected to SMMU.
> +- nvidia,swgroups: A bitmap of supported HardWare Accelerators(HWA).
> +  Each bit represents one swgroup. The assignments may be found in header
> +  file <dt-bindings/memory/tegra-swgroup.h>. Its max is 64. 2 cells
> +  are required. This unique ID info can be used to calculate
> +  MC_SMMU_<SWGROUP name>_ASID_0 offset and HOTRESET bit.

I'd prefer that you pointed out the type of the property explicitly at the
beginning of the description. It's difficult to spot it in the middle, and
while the current description defines that the property is two cells, it
doesn't point out that they are expected to form a (big endian) u64 (rather
than an array of two u32s).

While we don't yet have a formal set of rules for consistent binding
documentation, defining the explicitly is one of the requirements I'd like to
push for in new bindings. Placing this at the start of the description makes it
far easier to skim a binding document and perform basic sanity checks of the
binding itself, dts using the binding, and kernel code parsing it.

Cheers,
Mark.

Re: [PATCHv3 10/19] iommu/tegra: smmu: Get "nvidia,swgroups" from DT

[Mark Rutland]

On Wed, Oct 30, 2013 at 10:33:32PM +0000, Stephen Warren wrote:
> On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
> > This provides the info about which H/W Accelerators are supported on
> > Tegra SoC. This info is passed from DT. This is necessary to have the
> > unified SMMU driver among Tegra SoCs. Instead of using platform data,
> > DT passes "nvidia,swgroups" now. DT is mandatory in Tegra.
> 
> > diff --git a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
> > index 89fb543..6a844b3 100644
> > --- a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
> > +++ b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
> > @@ -8,6 +8,11 @@ Required properties:
> >  - nvidia,#asids : # of ASIDs
> >  - dma-window : IOVA start address and length.
> >  - nvidia,ahb : phandle to the ahb bus connected to SMMU.
> > +- nvidia,swgroups: A bitmap of supported HardWare Accelerators(HWA).
> > +  Each bit represents one swgroup. The assignments may be found in header
> > +  file <dt-bindings/memory/tegra-swgroup.h>. Its max is 64. 2 cells
> 
> That file doesn't exist at this point in the series. I think you should
> create a patch up-front that adds that header, and modifies the binding
> document, all in one go, and separate from any driver code changes.
> Separate DT/driver patches were IIRC agreed to be preferablte at the
> recent ARM workshop.

That certainly matches what I recall from the ARM minisummit.

> 
> > +  are required. This unique ID info can be used to calculate
> > +  MC_SMMU_<SWGROUP name>_ASID_0 offset and HOTRESET bit.
> 
> I'm afraid I still don't quite understand what a swgroup is.
> 
> IIUC, the HW works like this based on comments in a previous patch:
> 
> Each bus-master attached to the MMU passes a "memory client ID" along
> with the transaction. Some devices can generate transactions with
> different "memory client IDs". There is a mapping inside the SMMU from
> "memory client ID" to "address space ID". (I don't know what form that
> mapping takes; can you point out where it's set up?). Each "address
> space ID" has its own set of page tables. Is "swgroup" simply another
> name for "memory client ID"? If so, it'd be good to use just one term
> consistently.

The ARM SMMU refers to these as "stream IDs", as that's the architected name
that appears in all the hardware documentation. If "swgroup" is the term used
in the hardware documentation I think it makes sense to stick with it, as long
as there's a description in the binding document that points out what an
"swgroup" is. If there's a common term that both binding documents could refer
to to define what stream-id/swgroup are, that would be nice.

There are a few other differences in approach with the current ARM SMMU binding:
Documentation/devicetree/bindings/iommu/arm,smmu.txt

We should probably begin to look for commonality such that the next iommu
device that gets a binding is closer to a generic iommu class binding.

> 
> Assuming "swgroup" is "memory client ID", why can't the driver just
> create a list/... of known swgroups at runtime, based on the swgroup
> values that each device uses, which would presumably be either
> hard-coded in the client device's driver, or represented in the DT smmu
> property's "iommu specifier" value.

Assuming that the swgroup values of IP block instances are static, that sounds
like the ARM SMMU binding approach. Are the IDs static, or can they be assigned
at runtime?

Thanks,
Mark.

Re: [PATCHv3 05/19] ARM: dt: tegra114: iommu: Fix IOMMU register address

[Mark Rutland]

On Thu, Oct 31, 2013 at 05:11:26PM +0000, Hiroshi Doyu wrote:
> Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote @ Thu, 31 Oct 2013 19:05:24 +0200 (EET):
> 
> > Hi Mark,
> > 
> > Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote @ Thu, 31 Oct 2013 17:51:27 +0100:
> > 
> > > On Fri, Oct 18, 2013 at 11:26:46AM +0100, Hiroshi Doyu wrote:
> > > > Fix IOMMU register address.
> > > > 
> > > > Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > > ---
> > > >  arch/arm/boot/dts/tegra114.dtsi | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
> > > > index 2905145..8d42787 100644
> > > > --- a/arch/arm/boot/dts/tegra114.dtsi
> > > > +++ b/arch/arm/boot/dts/tegra114.dtsi
> > > > @@ -318,9 +318,9 @@
> > > >  
> > > >  	iommu {
> > > >  		compatible = "nvidia,tegra114-smmu", "nvidia,tegra30-smmu";
> > > > -		reg = <0x7000f010 0x02c
> > > > -		       0x7000f1f0 0x010
> > > > -		       0x7000f228 0x074>;
> > > > +		reg = <0x70019010 0x02c
> > > > +		       0x700191f0 0x010
> > > > +		       0x70019228 0x074>;
> > > 
> > > Minor cleanup/consistency request: as all these lines are changing anyway,
> > > would you mind bracketing each entry individually:
> > > 
> > > 		reg = <0x70019010 0x02c>,
> > > 		      <0x700191f0 0x010>,
> > > 		      <0x70019228 0x074>;
> > 
> > No, at all. Attached the updated "[PATCHv3' 05/19]".
> 
> Oops, here's the right one, "[PATCHv3'' 05/19]".

:)

Acked-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>

Cheers,
Mark.

> From 156a6ad059f902590094570697cb33e5b892d835 Mon Sep 17 00:00:00 2001
> From: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Date: Mon, 17 Jun 2013 15:37:35 +0300
> Subject: [PATCHv3'' 1/1] ARM: dt: tegra114: iommu: Fix IOMMU register address
> 
> Fix IOMMU register address.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/boot/dts/tegra114.dtsi | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
> index 2905145..e629042 100644
> --- a/arch/arm/boot/dts/tegra114.dtsi
> +++ b/arch/arm/boot/dts/tegra114.dtsi
> @@ -318,9 +318,9 @@
>  
>  	iommu {
>  		compatible = "nvidia,tegra114-smmu", "nvidia,tegra30-smmu";
> -		reg = <0x7000f010 0x02c
> -		       0x7000f1f0 0x010
> -		       0x7000f228 0x074>;
> +		reg = <0x70019010 0x02c>,
> +		      <0x700191f0 0x010>,
> +		      <0x70019228 0x074>;
>  		nvidia,#asids = <4>;
>  		dma-window = <0 0x40000000>;
>  		nvidia,swgroups = <0x18659fe>;
> -- 
> 1.8.1.5
> 

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Mark Rutland]

On Thu, Oct 31, 2013 at 08:18:08AM +0000, Hiroshi Doyu wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:44:04 +0100:
> 
> > > +	host1x {
> > > +		compatible = "nvidia,tegra30-host1x", "simple-bus";
> > > +		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_HC>;
> > > +		....
> > > +		gr3d {
> > > +			compatible = "nvidia,tegra30-gr3d";
> > > +			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_NV
> > > +						       TEGRA_SWGROUP_NV2>;
> > 
> > Why one cell for the host1x property, and two cells for the gr3d
> > property; shouldn't they be the same length?
> 
> That can vary. Even a single device node in DT can belong to multiple
> software groups.

Same with ARM SMMU.

> 
> > > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > 
> > > +static u64 smmu_of_get_memory_client(struct device *dev)
> > 
> > > +	np = of_parse_phandle(dev->of_node, propname, 0);
> > > +	if (np != smmu_handle->dev->of_node)
> > > +		return ~0;
> > > +
> > > +	prop = of_get_property(dev->of_node, propname, &bytes);
> > > +	if (!prop || !bytes)
> > > +		return ~0;
> > 
> > of_parse_phandle_with_fixed_args() might be a good fit here, or perhaps
> > require a property #smmu-cells in the SMMU node, thus allowing the
> > non-fixed-length of_parse_phandle_with_args() to be used here.
> 
> I once considered of_parse_phandle_with_args() but I couldn't use that
> since "#smmu-cells" needed to be fixed in smmu node, not in client
> node. In this case, the number of argument(number of swgroup IDs)
> varies per client device.

For the ARM SMMU binding, each device has a #stream-id-cells property
describing how many IDs it has, and then the SMMU node has a phandle+args
linkage to each of the devices attached to it, describing their stream IDs.
While this does have some limitations (a device can't be plugged into multiple
SMMUs with a different number of IDs), it seems generally sane.

Thanks,
Mark.

Re: [PATCHv3 15/19] ARM: tegra: Create a DT header defining SWGROUP ID

[Mark Rutland]

On Thu, Oct 31, 2013 at 08:19:42AM +0000, Hiroshi Doyu wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:48:38 +0100:
> 
> > On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
> > > Create a header file to define the swgroup IDs used by the IOMMU(SMMU)
> > > binding. "swgroups" is a group of H/W clients which Tegra SoCs
> > > support. 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.
> > 
> > > diff --git a/include/dt-bindings/memory/tegra-swgroup.h b/include/dt-bindings/memory/tegra-swgroup.h
> > 
> > > +#define TEGRA_SWGROUP_PPCS2	32	/* 0xab0 */
> > > +
> > > +#define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)
> > 
> > Isn't the DT match all 32-bit, such that the ULL suffix does nothing,
> 
> For the future Tegra SoC, the number of HWA grows a bit more, and it's
> supposed that "64" would be enough.

Does DTC accept 64-bit values as-is? I recall DTC shouting at me in the past,
and having to split values across two u32 cells (i.e. <0xffffffff 0xffffffff>
rather than <0xffffffffffffffff>).

Thanks,
Mark.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Stephen Warren]

On 10/31/2013 10:46 AM, Hiroshi Doyu wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Thu, 31 Oct 2013 17:35:24 +0100:
> 
> ...
>>>> ... and for the driver to explicitly parse that property, and wait
>>>> until the driver for iommu_phandle is ready. Exactly the same as any
>>>> other resource/service dependency.
>>>>
>>>> That will solve all the problems.
>>>>
>>>> The only downside is that every driver needs to contain code to parse
>>>> that property. However, I think that's just one function call; the
>>>> actual implementation of that function can be unified somewhere inside
>>>> core code in drivers/iommu/.
>>>
>>> Yes, but only missing part now is that, we could do this with
>>> "bus_notifier", but the current bus_notifier doesn't have the feature
>>> to return error(-EPROBE_DEFER). This could be modified so that
>>> bus_notifier could return (-EPROBE_DEFER) to postpone
>>> probing. Alternatively this could be done in some core probe code as
>>> well as Thierry pointed out.
>>>
>>> [1] In the reply of "[PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT"
>>
>> I think this should be done explicitly in drivers. It's much
>> simpler,
> 
> Yes, we need to insert some IOMMU specific code in driver?
> 
>> and doesn't encode any knowledge of driver-specific bindings into some
>> common bus notifier code.
> 
> I think that we cannot do that in drivers. We want to use drivers as
> they are without any modifications indicating its dependency on
> IOMMU because most of drivers are existing ones which nvidia doesn't
> implement. We want to set up any IOMMU related thingy implicitly,
> hided from driver. That's why we need to do this in bus_notifier or
> driver core code.

We're talking about memory-mapped on-SoC devices here, that generally
only exist inside Tegra SoCs.

Even ignoring that (i.e. expanding the argument to arbitrary modules),
having drivers that perform bus-master transactions call a function
of_iommu_attach() or similar, which does nothing if the device isn't
behind an IOMMU but otherwise does whatever is required, seems like it
isn't much of an imposition.

If this turns out to be something that lots of devices benefit from, we
could do the same thing that pinctrl does for "hogs", and make the
function call in the driver core. But note that's still part of the
probing flow (just implemented in the driver core) rather than bus
notifier based, hence keeps the same simplicity.

Re: [PATCHv3 10/19] iommu/tegra: smmu: Get "nvidia,swgroups" from DT

[Will Deacon]

On Thu, Oct 31, 2013 at 05:31:33PM +0000, Mark Rutland wrote:
> On Wed, Oct 30, 2013 at 10:33:32PM +0000, Stephen Warren wrote:
> > I'm afraid I still don't quite understand what a swgroup is.
> > 
> > IIUC, the HW works like this based on comments in a previous patch:
> > 
> > Each bus-master attached to the MMU passes a "memory client ID" along
> > with the transaction. Some devices can generate transactions with
> > different "memory client IDs". There is a mapping inside the SMMU from
> > "memory client ID" to "address space ID". (I don't know what form that
> > mapping takes; can you point out where it's set up?). Each "address
> > space ID" has its own set of page tables. Is "swgroup" simply another
> > name for "memory client ID"? If so, it'd be good to use just one term
> > consistently.
> 
> The ARM SMMU refers to these as "stream IDs", as that's the architected name
> that appears in all the hardware documentation. If "swgroup" is the term used
> in the hardware documentation I think it makes sense to stick with it, as long
> as there's a description in the binding document that points out what an
> "swgroup" is. If there's a common term that both binding documents could refer
> to to define what stream-id/swgroup are, that would be nice.

Sounds like:

  stream id == memory client id   (part of the binding)
  context id == address space id  (internal to the driver)

> There are a few other differences in approach with the current ARM SMMU binding:
> Documentation/devicetree/bindings/iommu/arm,smmu.txt
> 
> We should probably begin to look for commonality such that the next iommu
> device that gets a binding is closer to a generic iommu class binding.
> 
> > 
> > Assuming "swgroup" is "memory client ID", why can't the driver just
> > create a list/... of known swgroups at runtime, based on the swgroup
> > values that each device uses, which would presumably be either
> > hard-coded in the client device's driver, or represented in the DT smmu
> > property's "iommu specifier" value.
> 
> Assuming that the swgroup values of IP block instances are static, that sounds
> like the ARM SMMU binding approach. Are the IDs static, or can they be assigned
> at runtime?

The only valid case I can think of for dynamic IDs is for hotpluggable
devices sitting being a form of host controller (e.g. PCIe). In that case,
the host controller should handle the ID assignment, which must remain
static for the lifetime of a device.

Will

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Will Deacon]

On Thu, Oct 31, 2013 at 05:40:29PM +0000, Mark Rutland wrote:
> For the ARM SMMU binding, each device has a #stream-id-cells property
> describing how many IDs it has, and then the SMMU node has a phandle+args
> linkage to each of the devices attached to it, describing their stream IDs.
> While this does have some limitations (a device can't be plugged into multiple
> SMMUs with a different number of IDs), it seems generally sane.

If a device masters through two separate SMMUs, I think you need to describe
it as two devices. Even if you could describe the thing in DT, the Linux
driver/iommu model will quickly fall over.

Will

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Stephen Warren]

On 10/31/2013 12:02 PM, Will Deacon wrote:
> On Thu, Oct 31, 2013 at 05:40:29PM +0000, Mark Rutland wrote:
>> For the ARM SMMU binding, each device has a #stream-id-cells property
>> describing how many IDs it has, and then the SMMU node has a phandle+args
>> linkage to each of the devices attached to it, describing their stream IDs.
>> While this does have some limitations (a device can't be plugged into multiple
>> SMMUs with a different number of IDs), it seems generally sane.
> 
> If a device masters through two separate SMMUs, I think you need to describe
> it as two devices. Even if you could describe the thing in DT, the Linux
> driver/iommu model will quickly fall over.

Isn't that an internal Linux driver model issue? If there's a single
logical HW block, it seems like it should have a single DT node. The
fact that the DT node then somehow needs to instantiate two struct
device objects within Linux seems like it shouldn't influence the DT design.

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Stephen Warren]

On 10/31/2013 11:40 AM, Mark Rutland wrote:
> On Thu, Oct 31, 2013 at 08:18:08AM +0000, Hiroshi Doyu wrote:
>> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:44:04 +0100:
>>
>>>> +	host1x {
>>>> +		compatible = "nvidia,tegra30-host1x", "simple-bus";
>>>> +		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_HC>;
>>>> +		....
>>>> +		gr3d {
>>>> +			compatible = "nvidia,tegra30-gr3d";
>>>> +			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_NV
>>>> +						       TEGRA_SWGROUP_NV2>;
>>>
>>> Why one cell for the host1x property, and two cells for the gr3d
>>> property; shouldn't they be the same length?
>>
>> That can vary. Even a single device node in DT can belong to multiple
>> software groups.
> 
> Same with ARM SMMU.
> 
>>
>>>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>>>
>>>> +static u64 smmu_of_get_memory_client(struct device *dev)
>>>
>>>> +	np = of_parse_phandle(dev->of_node, propname, 0);
>>>> +	if (np != smmu_handle->dev->of_node)
>>>> +		return ~0;
>>>> +
>>>> +	prop = of_get_property(dev->of_node, propname, &bytes);
>>>> +	if (!prop || !bytes)
>>>> +		return ~0;
>>>
>>> of_parse_phandle_with_fixed_args() might be a good fit here, or perhaps
>>> require a property #smmu-cells in the SMMU node, thus allowing the
>>> non-fixed-length of_parse_phandle_with_args() to be used here.
>>
>> I once considered of_parse_phandle_with_args() but I couldn't use that
>> since "#smmu-cells" needed to be fixed in smmu node, not in client
>> node. In this case, the number of argument(number of swgroup IDs)
>> varies per client device.
> 
> For the ARM SMMU binding, each device has a #stream-id-cells property
> describing how many IDs it has, and then the SMMU node has a phandle+args
> linkage to each of the devices attached to it, describing their stream IDs.
> While this does have some limitations (a device can't be plugged into multiple
> SMMUs with a different number of IDs), it seems generally sane.

Hmm. That's interesting. I see that the ARM SMMU has a list of the
clients it affects, whereas this Tegra series puts information into each
client device about the SMMU(s) that affect it. Is it better to flip the
Tegra binding around to match the style of the ARM SMMU?

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Stephen Warren]

On 10/31/2013 02:18 AM, Hiroshi Doyu wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:44:04 +0100:
> 
>>> +	host1x {
>>> +		compatible = "nvidia,tegra30-host1x", "simple-bus";
>>> +		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_HC>;
>>> +		....
>>> +		gr3d {
>>> +			compatible = "nvidia,tegra30-gr3d";
>>> +			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_NV
>>> +						       TEGRA_SWGROUP_NV2>;
>>
>> Why one cell for the host1x property, and two cells for the gr3d
>> property; shouldn't they be the same length?
> 
> That can vary. Even a single device node in DT can belong to multiple
> software groups.
> 
>>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>>
>>> +static u64 smmu_of_get_memory_client(struct device *dev)
>>
>>> +	np = of_parse_phandle(dev->of_node, propname, 0);
>>> +	if (np != smmu_handle->dev->of_node)
>>> +		return ~0;
>>> +
>>> +	prop = of_get_property(dev->of_node, propname, &bytes);
>>> +	if (!prop || !bytes)
>>> +		return ~0;
>>
>> of_parse_phandle_with_fixed_args() might be a good fit here, or perhaps
>> require a property #smmu-cells in the SMMU node, thus allowing the
>> non-fixed-length of_parse_phandle_with_args() to be used here.
> 
> I once considered of_parse_phandle_with_args() but I couldn't use that
> since "#smmu-cells" needed to be fixed in smmu node, not in client
> node. In this case, the number of argument(number of swgroup IDs)
> varies per client device.

You could fix that by using a bitmask instead of a list. But, inverting
the order of the properties to match the ARM SMMU binding might also
solve this...

Re: [PATCHv3 15/19] ARM: tegra: Create a DT header defining SWGROUP ID

[Stephen Warren]

On 10/31/2013 11:46 AM, Mark Rutland wrote:
> On Thu, Oct 31, 2013 at 08:19:42AM +0000, Hiroshi Doyu wrote:
>> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:48:38 +0100:
>>
>>> On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
>>>> Create a header file to define the swgroup IDs used by the IOMMU(SMMU)
>>>> binding. "swgroups" is a group of H/W clients which Tegra SoCs
>>>> support. 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.
>>>
>>>> diff --git a/include/dt-bindings/memory/tegra-swgroup.h b/include/dt-bindings/memory/tegra-swgroup.h
>>>
>>>> +#define TEGRA_SWGROUP_PPCS2	32	/* 0xab0 */
>>>> +
>>>> +#define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)
>>>
>>> Isn't the DT match all 32-bit, such that the ULL suffix does nothing,
>>
>> For the future Tegra SoC, the number of HWA grows a bit more, and it's
>> supposed that "64" would be enough.
> 
> Does DTC accept 64-bit values as-is? I recall DTC shouting at me in the past,
> and having to split values across two u32 cells (i.e. <0xffffffff 0xffffffff>
> rather than <0xffffffffffffffff>).

Yes, you need to explicitly split the value across two cells in DT,
since cells are 32-bit.

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Will Deacon]

On Thu, Oct 31, 2013 at 07:13:01PM +0000, Stephen Warren wrote:
> On 10/31/2013 12:02 PM, Will Deacon wrote:
> > On Thu, Oct 31, 2013 at 05:40:29PM +0000, Mark Rutland wrote:
> >> For the ARM SMMU binding, each device has a #stream-id-cells property
> >> describing how many IDs it has, and then the SMMU node has a phandle+args
> >> linkage to each of the devices attached to it, describing their stream IDs.
> >> While this does have some limitations (a device can't be plugged into multiple
> >> SMMUs with a different number of IDs), it seems generally sane.
> > 
> > If a device masters through two separate SMMUs, I think you need to describe
> > it as two devices. Even if you could describe the thing in DT, the Linux
> > driver/iommu model will quickly fall over.
> 
> Isn't that an internal Linux driver model issue? If there's a single
> logical HW block, it seems like it should have a single DT node. The
> fact that the DT node then somehow needs to instantiate two struct
> device objects within Linux seems like it shouldn't influence the DT design.

Sure, I was just pointing out that you're going to need two struct devices
for that scenario, which isn't how things would work in Linux today.

Will

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Stephen Warren]

On 10/31/2013 01:16 PM, Stephen Warren wrote:
> On 10/31/2013 11:40 AM, Mark Rutland wrote:
>> On Thu, Oct 31, 2013 at 08:18:08AM +0000, Hiroshi Doyu wrote:
>>> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:44:04 +0100:
>>>
>>>>> +	host1x {
>>>>> +		compatible = "nvidia,tegra30-host1x", "simple-bus";
>>>>> +		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_HC>;
>>>>> +		....
>>>>> +		gr3d {
>>>>> +			compatible = "nvidia,tegra30-gr3d";
>>>>> +			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_NV
>>>>> +						       TEGRA_SWGROUP_NV2>;
>>>>
>>>> Why one cell for the host1x property, and two cells for the gr3d
>>>> property; shouldn't they be the same length?
>>>
>>> That can vary. Even a single device node in DT can belong to multiple
>>> software groups.
>>
>> Same with ARM SMMU.
>>
>>>
>>>>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>>>>
>>>>> +static u64 smmu_of_get_memory_client(struct device *dev)
>>>>
>>>>> +	np = of_parse_phandle(dev->of_node, propname, 0);
>>>>> +	if (np != smmu_handle->dev->of_node)
>>>>> +		return ~0;
>>>>> +
>>>>> +	prop = of_get_property(dev->of_node, propname, &bytes);
>>>>> +	if (!prop || !bytes)
>>>>> +		return ~0;
>>>>
>>>> of_parse_phandle_with_fixed_args() might be a good fit here, or perhaps
>>>> require a property #smmu-cells in the SMMU node, thus allowing the
>>>> non-fixed-length of_parse_phandle_with_args() to be used here.
>>>
>>> I once considered of_parse_phandle_with_args() but I couldn't use that
>>> since "#smmu-cells" needed to be fixed in smmu node, not in client
>>> node. In this case, the number of argument(number of swgroup IDs)
>>> varies per client device.
>>
>> For the ARM SMMU binding, each device has a #stream-id-cells property
>> describing how many IDs it has, and then the SMMU node has a phandle+args
>> linkage to each of the devices attached to it, describing their stream IDs.
>> While this does have some limitations (a device can't be plugged into multiple
>> SMMUs with a different number of IDs), it seems generally sane.
> 
> Hmm. That's interesting. I see that the ARM SMMU has a list of the
> clients it affects, whereas this Tegra series puts information into each
> client device about the SMMU(s) that affect it. Is it better to flip the
> Tegra binding around to match the style of the ARM SMMU?

One question here: How do you ensure that the SMMU driver probe()s
before probe() runs for devices affected by the SMMU?

With the DT binding in this series, there's a property in each client
node that describes the SMMU that affects the device. That can feed into
deferred probe very nicely.

However, if all the information is in the SMMU node, how can a client
find that, and hence defer its probe?

Perhaps the ARM SMMU assumes that it sits at the root of a bus, and
hence must be probe()d before any of its child devices can be probe()d?
I'm not sure that there's any general guarantee that a device's control
bus is the same bus it issues bus-master transactions too though, so I'm
not sure that's a general solution.

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Will Deacon]

On Thu, Oct 31, 2013 at 07:25:25PM +0000, Stephen Warren wrote:
> On 10/31/2013 01:16 PM, Stephen Warren wrote:
> > Hmm. That's interesting. I see that the ARM SMMU has a list of the
> > clients it affects, whereas this Tegra series puts information into each
> > client device about the SMMU(s) that affect it. Is it better to flip the
> > Tegra binding around to match the style of the ARM SMMU?
> 
> One question here: How do you ensure that the SMMU driver probe()s
> before probe() runs for devices affected by the SMMU?

I think we get away with this by virtue of nobody actually creating IOMMU
mappings from within drivers, so the ordering is already handled. However, I
also have a patch queued to move the SMMU initialisation to a
subsys_initcall. I'm not particularly fond of that, but it seems to be the
done thing (even for other architectures).

Will

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Hiroshi Doyu]

On Thu, 31 Oct 2013 18:53:22 +0100
Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
...
> We're talking about memory-mapped on-SoC devices here, that generally
> only exist inside Tegra SoCs.
> 
> Even ignoring that (i.e. expanding the argument to arbitrary modules),
> having drivers that perform bus-master transactions call a function
> of_iommu_attach() or similar, which does nothing if the device isn't
> behind an IOMMU but otherwise does whatever is required, seems like it
> isn't much of an imposition.

Where do you expect of_iommu_attach() to be called?
I thought something below:

	Modified drivers/base/dd.c
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 35fa368..92ec2e9 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -278,6 +278,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 	if (ret)
 		goto probe_failed;
 
+	ret = of_iommu_attach(dev);
+	if (ret)
+		goto probe_failed;
+
 	if (driver_sysfs_add(dev)) {
 		printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n",
 			__func__, dev_name(dev));

Re: [PATCHv3 10/19] iommu/tegra: smmu: Get "nvidia,swgroups" from DT

[Hiroshi Doyu]

Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote @ Thu, 31 Oct 2013 18:31:33 +0100:

> > Assuming "swgroup" is "memory client ID", why can't the driver just
> > create a list/... of known swgroups at runtime, based on the swgroup
> > values that each device uses, which would presumably be either
> > hard-coded in the client device's driver, or represented in the DT smmu
> > property's "iommu specifier" value.
> 
> Assuming that the swgroup values of IP block instances are static, that sounds
> like the ARM SMMU binding approach. Are the IDs static, or can they be assigned
> at runtime?

It's static.

But swgroup ID isn't documented in TRM. ID is basically calculated from register offset.

<swgroup ID> = (MC_SMMU_<swgroup name>_ASID_0 - MC_SMMU_<the 1st swgroup name>_ASID_0) / 4

Or 

MC_SMMU_<swgroup name>_ASID_0 = 4 * <swgroup ID> + MC_SMMU_<the 1st swgroup name>_ASID_0)

Re: [PATCHv3 10/19] iommu/tegra: smmu: Get "nvidia,swgroups" from DT

[Hiroshi Doyu]

> > The ARM SMMU refers to these as "stream IDs", as that's the architected name
> > that appears in all the hardware documentation. If "swgroup" is the term used
> > in the hardware documentation I think it makes sense to stick with it, as long
> > as there's a description in the binding document that points out what an
> > "swgroup" is. If there's a common term that both binding documents could refer
> > to to define what stream-id/swgroup are, that would be nice.
>
> Sounds like:
>
>   stream id == memory client id   (part of the binding)
>   context id == address space id  (internal to the driver)

Good explanation. This would be helpful when I read ARM SMMU TRM;)

> > There are a few other differences in approach with the current ARM SMMU binding:
> > Documentation/devicetree/bindings/iommu/arm,smmu.txt
> >
> > We should probably begin to look for commonality such that the next iommu
> > device that gets a binding is closer to a generic iommu class binding.

I think that now I got the "mmu-masters".

- mmu-masters   : A list of phandles to device nodes representing bus
		  masters for which the SMMU can provide a translation
		  and their corresponding StreamIDs (see example below).
		  Each device node linked from this list must have a
		  "#stream-id-cells" property, indicating the number of
		  StreamIDs associated with it.

If I apply this to Tegra, this would be:

	host1x {
            #swgroup-id-cells = <1>;

               gr3d {
                   #swgroup-id-cells = <2>;
               };

	};

	smmu: iommu {
		compatible = "nvidia,tegra114-smmu", "nvidia,tegra30-smmu";
		reg = <0x70019010 0x02c>,
		      <0x700191f0 0x010>,
		      <0x70019228 0x074>;
		.......
		mmu-masters = <&host1x TEGRA_SWGROUP_HC>,
			      <&mpe TEGRA_SWGROUP_MPE>,
			      ..........
			      <&gr3d TEGRA_SWGROUP_NV TEGRA_SWGROUP_NV2>;
	};

From consistency POV, this may look better. Also if "stream-id-cells",
"mmu-masters" is defined as IOMMU standard tag, it would be more
easier. Otherwise, I may need to stick to "swgroup-id-cells".

> > > Assuming "swgroup" is "memory client ID", why can't the driver just
> > > create a list/... of known swgroups at runtime, based on the swgroup
> > > values that each device uses, which would presumably be either
> > > hard-coded in the client device's driver, or represented in the DT smmu
> > > property's "iommu specifier" value.
> >
> > Assuming that the swgroup values of IP block instances are static, that sounds
> > like the ARM SMMU binding approach. Are the IDs static, or can they be assigned
> > at runtime?
>
> The only valid case I can think of for dynamic IDs is for hotpluggable
> devices sitting being a form of host controller (e.g. PCIe). In that case,
> the host controller should handle the ID assignment, which must remain
> static for the lifetime of a device.

In Tegra PCIe clients belong to one swgroup statically.

Re: [PATCHv3 10/19] iommu/tegra: smmu: Get "nvidia,swgroups" from DT

[Hiroshi Doyu]

> > The ARM SMMU refers to these as "stream IDs", as that's the architected name
> > that appears in all the hardware documentation. If "swgroup" is the term used
> > in the hardware documentation I think it makes sense to stick with it, as long
> > as there's a description in the binding document that points out what an
> > "swgroup" is. If there's a common term that both binding documents could refer
> > to to define what stream-id/swgroup are, that would be nice.
>
> Sounds like:
>
>   stream id == memory client id   (part of the binding)
>   context id == address space id  (internal to the driver)

Good explanation. This would be helpful when I read ARM SMMU TRM;)

> > There are a few other differences in approach with the current ARM SMMU binding:
> > Documentation/devicetree/bindings/iommu/arm,smmu.txt
> >
> > We should probably begin to look for commonality such that the next iommu
> > device that gets a binding is closer to a generic iommu class binding.

I think that now I got the "mmu-masters".

- mmu-masters   : A list of phandles to device nodes representing bus
		  masters for which the SMMU can provide a translation
		  and their corresponding StreamIDs (see example below).
		  Each device node linked from this list must have a
		  "#stream-id-cells" property, indicating the number of
		  StreamIDs associated with it.

If I apply this to Tegra, this would be:

	host1x {
            #swgroup-id-cells = <1>;

               gr3d {
                   #swgroup-id-cells = <2>;
               };

	};

	smmu: iommu {
		compatible = "nvidia,tegra114-smmu", "nvidia,tegra30-smmu";
		reg = <0x70019010 0x02c>,
		      <0x700191f0 0x010>,
		      <0x70019228 0x074>;
		.......
		mmu-masters = <&host1x TEGRA_SWGROUP_HC>,
			      <&mpe TEGRA_SWGROUP_MPE>,
			      ..........
			      <&gr3d TEGRA_SWGROUP_NV TEGRA_SWGROUP_NV2>;
	};

>From consistency POV, this may look better. Also if "stream-id-cells",
"mmu-masters" is defined as IOMMU standard tag, it would be more
easier. Otherwise, I may need to stick to "swgroup-id-cells".

> > > Assuming "swgroup" is "memory client ID", why can't the driver just
> > > create a list/... of known swgroups at runtime, based on the swgroup
> > > values that each device uses, which would presumably be either
> > > hard-coded in the client device's driver, or represented in the DT smmu
> > > property's "iommu specifier" value.
> >
> > Assuming that the swgroup values of IP block instances are static, that sounds
> > like the ARM SMMU binding approach. Are the IDs static, or can they be assigned
> > at runtime?
>
> The only valid case I can think of for dynamic IDs is for hotpluggable
> devices sitting being a form of host controller (e.g. PCIe). In that case,
> the host controller should handle the ID assignment, which must remain
> static for the lifetime of a device.

In Tegra PCIe clients belong to one swgroup statically.

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Hiroshi Doyu]

On Thu, 31 Oct 2013 18:40:29 +0100
Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:

> For the ARM SMMU binding, each device has a #stream-id-cells property
> describing how many IDs it has, and then the SMMU node has a phandle+args
> linkage to each of the devices attached to it, describing their stream IDs.
> While this does have some limitations (a device can't be plugged into multiple
> SMMUs with a different number of IDs), it seems generally sane.

I may guess that the concept of "iommu domain" can hold the nested
IOMMU in an unified way?

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Hiroshi Doyu]

On Thu, 31 Oct 2013 20:16:18 +0100
Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:

> Hmm. That's interesting. I see that the ARM SMMU has a list of the
> clients it affects, whereas this Tegra series puts information into each
> client device about the SMMU(s) that affect it. Is it better to flip the
> Tegra binding around to match the style of the ARM SMMU?

That's better from consistency POV.

Can this be used to control the device instanciation order?

I'm considering the following scenario:
If a device phandle is listed in mmu-masters, that device probe would
be defered till IOMMU is instanciated.

How does ARM SMMU deal with this issue now?

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Hiroshi Doyu]

On Thu, 31 Oct 2013 20:39:41 +0100
Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:

> On Thu, Oct 31, 2013 at 07:25:25PM +0000, Stephen Warren wrote:
> > On 10/31/2013 01:16 PM, Stephen Warren wrote:
> > > Hmm. That's interesting. I see that the ARM SMMU has a list of the
> > > clients it affects, whereas this Tegra series puts information into each
> > > client device about the SMMU(s) that affect it. Is it better to flip the
> > > Tegra binding around to match the style of the ARM SMMU?
> > 
> > One question here: How do you ensure that the SMMU driver probe()s
> > before probe() runs for devices affected by the SMMU?
> 
> I think we get away with this by virtue of nobody actually creating IOMMU
> mappings from within drivers, so the ordering is already handled. However, I

Could you explain the above a bit more?

Currently I have a problem that IOMMU(tegra smmu) needs to be
instanciated earlier than other IOMMU'able devices since IOMMU needs
to configure devices IOMMU'ed at their instanciation time.

Now I'm considering to use iommu_bus_notifier or inserting a hook in
dd.c:really_probe() in order to defer a device instanciation til
IOMMU's instanciated.

Re: [PATCHv3 15/19] ARM: tegra: Create a DT header defining SWGROUP ID

[Hiroshi Doyu]

On Thu, 31 Oct 2013 18:46:06 +0100
Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:

> On Thu, Oct 31, 2013 at 08:19:42AM +0000, Hiroshi Doyu wrote:
> > Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:48:38 +0100:
> > 
> > > On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
> > > > Create a header file to define the swgroup IDs used by the IOMMU(SMMU)
> > > > binding. "swgroups" is a group of H/W clients which Tegra SoCs
> > > > support. 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.
> > > 
> > > > diff --git a/include/dt-bindings/memory/tegra-swgroup.h b/include/dt-bindings/memory/tegra-swgroup.h
> > > 
> > > > +#define TEGRA_SWGROUP_PPCS2	32	/* 0xab0 */
> > > > +
> > > > +#define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)
> > > 
> > > Isn't the DT match all 32-bit, such that the ULL suffix does nothing,
> > 
> > For the future Tegra SoC, the number of HWA grows a bit more, and it's
> > supposed that "64" would be enough.
> 
> Does DTC accept 64-bit values as-is? I recall DTC shouting at me in the past,
> and having to split values across two u32 cells (i.e. <0xffffffff 0xffffffff>
> rather than <0xffffffffffffffff>).

TEGRA_SWGROUP_BIT was only used in driver code since DT couldn't handle OR("|").

Stephen pointed out the latest DT can do. So this macro should be
revised to be used in DTS as well.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3371 bytes --]

On Thu, Oct 31, 2013 at 11:53:22AM -0600, Stephen Warren wrote:
> On 10/31/2013 10:46 AM, Hiroshi Doyu wrote:
> > Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Thu, 31 Oct 2013 17:35:24 +0100:
> > 
> > ...
> >>>> ... and for the driver to explicitly parse that property, and wait
> >>>> until the driver for iommu_phandle is ready. Exactly the same as any
> >>>> other resource/service dependency.
> >>>>
> >>>> That will solve all the problems.
> >>>>
> >>>> The only downside is that every driver needs to contain code to parse
> >>>> that property. However, I think that's just one function call; the
> >>>> actual implementation of that function can be unified somewhere inside
> >>>> core code in drivers/iommu/.
> >>>
> >>> Yes, but only missing part now is that, we could do this with
> >>> "bus_notifier", but the current bus_notifier doesn't have the feature
> >>> to return error(-EPROBE_DEFER). This could be modified so that
> >>> bus_notifier could return (-EPROBE_DEFER) to postpone
> >>> probing. Alternatively this could be done in some core probe code as
> >>> well as Thierry pointed out.
> >>>
> >>> [1] In the reply of "[PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT"
> >>
> >> I think this should be done explicitly in drivers. It's much
> >> simpler,
> > 
> > Yes, we need to insert some IOMMU specific code in driver?
> > 
> >> and doesn't encode any knowledge of driver-specific bindings into some
> >> common bus notifier code.
> > 
> > I think that we cannot do that in drivers. We want to use drivers as
> > they are without any modifications indicating its dependency on
> > IOMMU because most of drivers are existing ones which nvidia doesn't
> > implement. We want to set up any IOMMU related thingy implicitly,
> > hided from driver. That's why we need to do this in bus_notifier or
> > driver core code.
> 
> We're talking about memory-mapped on-SoC devices here, that generally
> only exist inside Tegra SoCs.
> 
> Even ignoring that (i.e. expanding the argument to arbitrary modules),
> having drivers that perform bus-master transactions call a function
> of_iommu_attach() or similar, which does nothing if the device isn't
> behind an IOMMU but otherwise does whatever is required, seems like it
> isn't much of an imposition.
> 
> If this turns out to be something that lots of devices benefit from, we
> could do the same thing that pinctrl does for "hogs", and make the
> function call in the driver core. But note that's still part of the
> probing flow (just implemented in the driver core) rather than bus
> notifier based, hence keeps the same simplicity.

That's exactly what I was proposing in the first place. I did the same
thing in my patches for late interrupt reference resolution. The reason
why I think it makes sense to put it into the core is that it's
something that likely most devices will have to do anyway. And it also
is completely transparent to drivers, right? If they aren't attached to
an IOMMU then they just aren't but will continue to work properly. And
as you said, having it in the core makes drivers simpler, while at the
same time keeping the explicitness of having a function call (rather
than a somewhat obfuscated bus notifier) and the flexibility of deferred
probing.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1986 bytes --]

On Fri, Nov 01, 2013 at 08:49:09AM +0200, Hiroshi Doyu wrote:
> On Thu, 31 Oct 2013 18:53:22 +0100
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> ...
> > We're talking about memory-mapped on-SoC devices here, that generally
> > only exist inside Tegra SoCs.
> > 
> > Even ignoring that (i.e. expanding the argument to arbitrary modules),
> > having drivers that perform bus-master transactions call a function
> > of_iommu_attach() or similar, which does nothing if the device isn't
> > behind an IOMMU but otherwise does whatever is required, seems like it
> > isn't much of an imposition.
> 
> Where do you expect of_iommu_attach() to be called?
> I thought something below:
> 
> 	Modified drivers/base/dd.c
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 35fa368..92ec2e9 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -278,6 +278,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  	if (ret)
>  		goto probe_failed;
>  
> +	ret = of_iommu_attach(dev);
> +	if (ret)
> +		goto probe_failed;
> +
>  	if (driver_sysfs_add(dev)) {
>  		printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n",
>  			__func__, dev_name(dev));
> 

The patches for late interrupt reference resolution introduced a
separate function, of_platform_probe(), with the intent of having it
call potentially many resource allocation hooks. The function needs to
be platform_device specific, and therefore is called from within the
platform_drv_probe() function. The reason is that interrupts are stored
as resources within struct platform_device, so you need to have access
to a platform device.

I think that devices that require attachment to an IOMMU will always end
up being platform devices too, so even if it isn't a strict requirement
here it would still make sense to use a similar infrastructure to avoid
cluttering the core code with too many loose function calls.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2049 bytes --]

On Thu, Oct 31, 2013 at 10:37:40AM -0600, Stephen Warren wrote:
> On 10/31/2013 02:14 AM, Hiroshi Doyu wrote:
> > Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:41:09 +0100:
> > 
> >> My earlier proposal for deferred interrupt reference resolution actually
> >> tries to solve that problem within the core. Essentially what it does is
> >> add a new function that gets called right before the driver's .probe()
> >> function, so that it can parse standard resources and services from DT,
> >> such as the interrupts property. This could theoretically be done for
> >> other resources such as reg as well, but it really only matters where
> >> the resource can be dynamic (as is the case for interrupts).
> >>
> >> In this case I would envision a standard OF function to associate a
> >> device with its IOMMU. Perhaps something like:
> >>
> >> 	int of_iommu_attach(struct device *dev);
> >>
> >> That could be called by the core independent of the specific device and
> >> IOMMU. IOMMU-specifics can probably be handled using .of_xlate(), quite
> >> in a similar way to GPIO or regulators.
> >>
> >> That way drivers can remain agnostic of the IOMMU while still being able
> >> to take advantage of deferred probing.
> > 
> > This could be simpler than making "bus_notifier" return "-EPROBE_DEFER".
> > 
> > The disadvantage of this is that we may need to provide the similar
> > of_<"subsystem">_attach() per subsytem if needed.
> 
> Well, "per subsystem" here means per subsystem that is providing
> resources, not per subsystem that is consuming resources. How many
> subsystems are you expecting to provide resources like this? At present,
> I believe IOMMU is the only subsystem missing some kind of API for
> clients to acquire the relevant resource. I don't think there's any
> scalability problem here.

Furthermore if the subsystem wants to provide resources then it needs
such a function anyway, so there really isn't any overhead here.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Hiroshi Doyu]

Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Fri, 1 Nov 2013 10:46:45 +0100:

> * PGP Signed by an unknown key
> 
> On Thu, Oct 31, 2013 at 11:53:22AM -0600, Stephen Warren wrote:
> > On 10/31/2013 10:46 AM, Hiroshi Doyu wrote:
> > > Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Thu, 31 Oct 2013 17:35:24 +0100:
> > > 
> > > ...
> > >>>> ... and for the driver to explicitly parse that property, and wait
> > >>>> until the driver for iommu_phandle is ready. Exactly the same as any
> > >>>> other resource/service dependency.
> > >>>>
> > >>>> That will solve all the problems.
> > >>>>
> > >>>> The only downside is that every driver needs to contain code to parse
> > >>>> that property. However, I think that's just one function call; the
> > >>>> actual implementation of that function can be unified somewhere inside
> > >>>> core code in drivers/iommu/.
> > >>>
> > >>> Yes, but only missing part now is that, we could do this with
> > >>> "bus_notifier", but the current bus_notifier doesn't have the feature
> > >>> to return error(-EPROBE_DEFER). This could be modified so that
> > >>> bus_notifier could return (-EPROBE_DEFER) to postpone
> > >>> probing. Alternatively this could be done in some core probe code as
> > >>> well as Thierry pointed out.
> > >>>
> > >>> [1] In the reply of "[PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT"
> > >>
> > >> I think this should be done explicitly in drivers. It's much
> > >> simpler,
> > > 
> > > Yes, we need to insert some IOMMU specific code in driver?
> > > 
> > >> and doesn't encode any knowledge of driver-specific bindings into some
> > >> common bus notifier code.
> > > 
> > > I think that we cannot do that in drivers. We want to use drivers as
> > > they are without any modifications indicating its dependency on
> > > IOMMU because most of drivers are existing ones which nvidia doesn't
> > > implement. We want to set up any IOMMU related thingy implicitly,
> > > hided from driver. That's why we need to do this in bus_notifier or
> > > driver core code.
> > 
> > We're talking about memory-mapped on-SoC devices here, that generally
> > only exist inside Tegra SoCs.
> > 
> > Even ignoring that (i.e. expanding the argument to arbitrary modules),
> > having drivers that perform bus-master transactions call a function
> > of_iommu_attach() or similar, which does nothing if the device isn't
> > behind an IOMMU but otherwise does whatever is required, seems like it
> > isn't much of an imposition.
> > 
> > If this turns out to be something that lots of devices benefit from, we
> > could do the same thing that pinctrl does for "hogs", and make the
> > function call in the driver core. But note that's still part of the
> > probing flow (just implemented in the driver core) rather than bus
> > notifier based, hence keeps the same simplicity.
> 
> That's exactly what I was proposing in the first place. I did the same
> thing in my patches for late interrupt reference resolution. The reason
> why I think it makes sense to put it into the core is that it's
> something that likely most devices will have to do anyway. And it also
> is completely transparent to drivers, right? If they aren't attached to
> an IOMMU then they just aren't but will continue to work properly. And
> as you said, having it in the core makes drivers simpler, while at the
> same time keeping the explicitness of having a function call (rather
> than a somewhat obfuscated bus notifier) and the flexibility of deferred
> probing.

One idea is that, rather than inserting a hook(function) per
subsystems in driver core, if we invent a new /special section/ which
collects all hooks in sequence like initcalls, the subsystem just
would declare a hook function for that special section. All hooks in
that section can be called from driver core iterately. Then we
wouldn't touch driver core code at all, once we insert a func to call
hooks repeatedly. Looks simpler than tweaking bus notifier.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Hiroshi Doyu]

Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Fri, 1 Nov 2013 10:52:24 +0100:

> * PGP Signed by an unknown key
> 
> On Fri, Nov 01, 2013 at 08:49:09AM +0200, Hiroshi Doyu wrote:
> > On Thu, 31 Oct 2013 18:53:22 +0100
> > Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> > ...
> > > We're talking about memory-mapped on-SoC devices here, that generally
> > > only exist inside Tegra SoCs.
> > > 
> > > Even ignoring that (i.e. expanding the argument to arbitrary modules),
> > > having drivers that perform bus-master transactions call a function
> > > of_iommu_attach() or similar, which does nothing if the device isn't
> > > behind an IOMMU but otherwise does whatever is required, seems like it
> > > isn't much of an imposition.
> > 
> > Where do you expect of_iommu_attach() to be called?
> > I thought something below:
> > 
> > 	Modified drivers/base/dd.c
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index 35fa368..92ec2e9 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -278,6 +278,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)
> >  	if (ret)
> >  		goto probe_failed;
> >  
> > +	ret = of_iommu_attach(dev);
> > +	if (ret)
> > +		goto probe_failed;
> > +
> >  	if (driver_sysfs_add(dev)) {
> >  		printk(KERN_ERR "%s: driver_sysfs_add(%s) failed\n",
> >  			__func__, dev_name(dev));
> > 
> 
> The patches for late interrupt reference resolution introduced a
> separate function, of_platform_probe(), with the intent of having it
> call potentially many resource allocation hooks. The function needs to
> be platform_device specific, and therefore is called from within the
> platform_drv_probe() function. The reason is that interrupts are stored
> as resources within struct platform_device, so you need to have access
> to a platform device.

Ok.
Also I think that a special new /section/ with hooks[1] would fit with
this as well.

> I think that devices that require attachment to an IOMMU will always end
> up being platform devices too, so even if it isn't a strict requirement
> here it would still make sense to use a similar infrastructure to avoid
> cluttering the core code with too many loose function calls.

FYI: PCIe devices can also be IOMMU'able although not yet enabled yet.

[1] http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006874.html

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Hiroshi Doyu]

Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Fri, 1 Nov 2013 10:53:50 +0100:

> * PGP Signed by an unknown key
> 
> On Thu, Oct 31, 2013 at 10:37:40AM -0600, Stephen Warren wrote:
> > On 10/31/2013 02:14 AM, Hiroshi Doyu wrote:
> > > Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:41:09 +0100:
> > > 
> > >> My earlier proposal for deferred interrupt reference resolution actually
> > >> tries to solve that problem within the core. Essentially what it does is
> > >> add a new function that gets called right before the driver's .probe()
> > >> function, so that it can parse standard resources and services from DT,
> > >> such as the interrupts property. This could theoretically be done for
> > >> other resources such as reg as well, but it really only matters where
> > >> the resource can be dynamic (as is the case for interrupts).
> > >>
> > >> In this case I would envision a standard OF function to associate a
> > >> device with its IOMMU. Perhaps something like:
> > >>
> > >> 	int of_iommu_attach(struct device *dev);
> > >>
> > >> That could be called by the core independent of the specific device and
> > >> IOMMU. IOMMU-specifics can probably be handled using .of_xlate(), quite
> > >> in a similar way to GPIO or regulators.
> > >>
> > >> That way drivers can remain agnostic of the IOMMU while still being able
> > >> to take advantage of deferred probing.
> > > 
> > > This could be simpler than making "bus_notifier" return "-EPROBE_DEFER".
> > > 
> > > The disadvantage of this is that we may need to provide the similar
> > > of_<"subsystem">_attach() per subsytem if needed.
> > 
> > Well, "per subsystem" here means per subsystem that is providing
> > resources, not per subsystem that is consuming resources. How many
> > subsystems are you expecting to provide resources like this? At present,
> > I believe IOMMU is the only subsystem missing some kind of API for
> > clients to acquire the relevant resource. I don't think there's any
> > scalability problem here.
> 
> Furthermore if the subsystem wants to provide resources then it needs
> such a function anyway, so there really isn't any overhead here.

Ok, the scalability won't be an issue since the number of subsystems
to provide resources/services is usually limited, anyway.

Re: [PATCHv3 15/19] ARM: tegra: Create a DT header defining SWGROUP ID

[Stephen Warren]

On 11/01/2013 02:06 AM, Hiroshi Doyu wrote:
> On Thu, 31 Oct 2013 18:46:06 +0100
> Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> 
>> On Thu, Oct 31, 2013 at 08:19:42AM +0000, Hiroshi Doyu wrote:
>>> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:48:38 +0100:
>>>
>>>> On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
>>>>> Create a header file to define the swgroup IDs used by the IOMMU(SMMU)
>>>>> binding. "swgroups" is a group of H/W clients which Tegra SoCs
>>>>> support. 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.
>>>>
>>>>> diff --git a/include/dt-bindings/memory/tegra-swgroup.h b/include/dt-bindings/memory/tegra-swgroup.h
>>>>
>>>>> +#define TEGRA_SWGROUP_PPCS2	32	/* 0xab0 */
>>>>> +
>>>>> +#define TEGRA_SWGROUP_BIT(x)	(1ULL << TEGRA_SWGROUP_##x)
>>>>
>>>> Isn't the DT match all 32-bit, such that the ULL suffix does nothing,
>>>
>>> For the future Tegra SoC, the number of HWA grows a bit more, and it's
>>> supposed that "64" would be enough.
>>
>> Does DTC accept 64-bit values as-is? I recall DTC shouting at me in the past,
>> and having to split values across two u32 cells (i.e. <0xffffffff 0xffffffff>
>> rather than <0xffffffffffffffff>).
> 
> TEGRA_SWGROUP_BIT was only used in driver code since DT couldn't handle OR("|").
> 
> Stephen pointed out the latest DT can do. So this macro should be
> revised to be used in DTS as well.

One note: If there was no intention to use the #defines in
<dt-bindings/memory/tegra-swgroup.h> in the DT before, that header
shouldn't have been in the <dt-bindings> directory. That said, given the
bug is really that the DT wasn't using the header but should have been,
fixing the DT is the right way to go.

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Stephen Warren]

On 10/31/2013 01:39 PM, Will Deacon wrote:
> On Thu, Oct 31, 2013 at 07:25:25PM +0000, Stephen Warren wrote:
>> On 10/31/2013 01:16 PM, Stephen Warren wrote:
>>> Hmm. That's interesting. I see that the ARM SMMU has a list of the
>>> clients it affects, whereas this Tegra series puts information into each
>>> client device about the SMMU(s) that affect it. Is it better to flip the
>>> Tegra binding around to match the style of the ARM SMMU?
>>
>> One question here: How do you ensure that the SMMU driver probe()s
>> before probe() runs for devices affected by the SMMU?
> 
> I think we get away with this by virtue of nobody actually creating IOMMU
> mappings from within drivers,

Where are the mapping set up? What prevents that action from happening
before the IOMMU is set up and the various devices have their DMA ops
hooked through the IOMMU? It's fairly clear how deferred probe of client
devices could achieve that, but I'm quite unsure how some what other
mechanism would ensure that.

> so the ordering is already handled. However, I
> also have a patch queued to move the SMMU initialisation to a
> subsys_initcall. I'm not particularly fond of that, but it seems to be the
> done thing (even for other architectures).

We've been trying very hard not to rely on initcall ordering to solve
issues like this, simply it doesn't scale to complex use-cases, and
different HW/drivers/... might have conflicting requirements that aren't
possible to solve by tweaking initcall levels. It seems much more
preferable to use something that's guaranteed to work and adapt to any
situation, such as deferred probe.

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Will Deacon]

On Fri, Nov 01, 2013 at 04:08:52PM +0000, Stephen Warren wrote:
> On 10/31/2013 01:39 PM, Will Deacon wrote:
> > On Thu, Oct 31, 2013 at 07:25:25PM +0000, Stephen Warren wrote:
> >> On 10/31/2013 01:16 PM, Stephen Warren wrote:
> >>> Hmm. That's interesting. I see that the ARM SMMU has a list of the
> >>> clients it affects, whereas this Tegra series puts information into each
> >>> client device about the SMMU(s) that affect it. Is it better to flip the
> >>> Tegra binding around to match the style of the ARM SMMU?
> >>
> >> One question here: How do you ensure that the SMMU driver probe()s
> >> before probe() runs for devices affected by the SMMU?
> > 
> > I think we get away with this by virtue of nobody actually creating IOMMU
> > mappings from within drivers,
> 
> Where are the mapping set up? What prevents that action from happening
> before the IOMMU is set up and the various devices have their DMA ops
> hooked through the IOMMU? It's fairly clear how deferred probe of client
> devices could achieve that, but I'm quite unsure how some what other
> mechanism would ensure that.

This is the part where I start to struggle. If you look at a mainline
kernel, arm_iommu_create_mapping isn't really used, so it's difficult to
know at which point the IOMMU needs to be initialised across different ARM
SoCs.

For other architectures, it looks like the IOMMU initialisation hangs
directly off arch code (which in turn are driven from initcalls), but that's
generally possible because either the IOMMU is architected, or it's strongly
tied to PCI initialisation.

What are the requirements for Tegra? If the IOMMU isn't initialised, does it
act as a passthrough?

> > so the ordering is already handled. However, I
> > also have a patch queued to move the SMMU initialisation to a
> > subsys_initcall. I'm not particularly fond of that, but it seems to be the
> > done thing (even for other architectures).
> 
> We've been trying very hard not to rely on initcall ordering to solve
> issues like this, simply it doesn't scale to complex use-cases, and
> different HW/drivers/... might have conflicting requirements that aren't
> possible to solve by tweaking initcall levels. It seems much more
> preferable to use something that's guaranteed to work and adapt to any
> situation, such as deferred probe.

Indeed. As I said, I'm not a huge fan of initcalls either, but that's the
way things are at the moment. If deferred probe can be used to make that
less fragile, then that sounds like a good thing to do, perhaps across other
architectures too.

Will

Re: [PATCHv3 10/19] iommu/tegra: smmu: Get "nvidia,swgroups" from DT

[Stephen Warren]

On 10/31/2013 02:17 AM, Hiroshi Doyu wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:33:32 +0100:
...
> Right.
> "memory client ID" is used to find out MC_SMMU_<swgroup>_ASID_0
> register. This register is used to associate <swgroup> to address
> space(AS). <swgroup> == H/W. <swgroup> can be attached to any AS.
> 
>> Is "swgroup" simply another name for "memory client ID"? If so, it'd
>> be good to use just one term consistently.
> 
> I used the name "memory client ID" because this ID can be used to find
> out HOTRESET bit in MC_CLIENT_HOTRESET_*_0 registers in addition to
> find the MC_SMMU_<swgroup>_ASID_0 offset. But maybe it's easy to use
> the consistent name as "swgroup". If laster HOTRESET wants automatic
> calculation they could borrow/redefine the same ID list, just
> replacing the prefix. What do you think?
> 
>> Assuming "swgroup" is "memory client ID",
> 
> Yes

I took a look at the TRM, and it does look like there's a distinction
between client IDs and swgroups. The basic idea is that each swgroup
encompasses n (n >= 1) memory client IDs. So, we do have to use both terms.

See for example the tables in section 20.11.1.12 and 20.11.1.15 of
Tegra4 (Tegra114) TRM v01 that describe (some?) client ID -> swgroup
mappings. Section 20.3.4 states that whenever a translation error
occurs, both the module ID (I assume swgroup) and client ID will be
recorded, although the interrupt registers don't appear to be
documented, so I can't check this:-(

However, the bits in the HOTRESET register appear mostly align with
swgroup IDs rather client IDs, so I think some of your explanation above
is innaccurate.

I do want to point out that the HOTRESET bits don't 100% match the
swgroup IDs though, so the driver needs to be careful, and perhaps
contain some kind of mapping table between the two numbering spaces. In
particular, on Tegra114, swgroups and ASID registers appear in order
PPCS, <gap>, SATA, whereas there's no gap between the HOTRESET bits for
PPCS and SATA, assuming the TRM is correct.

Re: [PATCHv3 06/19] iommu/tegra: smmu: Select ARM_DMA_USE_IOMMU in Kconfig

[Stephen Warren]

On 10/31/2013 02:16 AM, Hiroshi Doyu wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:15:00 +0100:
> 
>> On 10/18/2013 04:26 AM, Hiroshi Doyu wrote:
>>> SMMU expects ARM_DMA_USE_IOMMU to be enabled to use DMA mapping API by
>>> default.
>>
>> Is the select required for the driver to be useful at all (in which case
>> it's fine), or simply for things to work "better" (define "better"), in
>> which case perhaps leaving it optional is more correct.
> 
> Practically DMA API is the only interface for IOMMU(ARM_DMA_USE_IOMMU)
> for us, but technically they are/should be independent.

I would be tempted to deal with this in the defconfig, but I suppose
handling it as a Kconfig select is fine too; there's probably no real
disadvantage in doing so.

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Stephen Warren]

On 11/01/2013 10:34 AM, Will Deacon wrote:
> On Fri, Nov 01, 2013 at 04:08:52PM +0000, Stephen Warren wrote:
>> On 10/31/2013 01:39 PM, Will Deacon wrote:
>>> On Thu, Oct 31, 2013 at 07:25:25PM +0000, Stephen Warren wrote:
>>>> On 10/31/2013 01:16 PM, Stephen Warren wrote:
>>>>> Hmm. That's interesting. I see that the ARM SMMU has a list of the
>>>>> clients it affects, whereas this Tegra series puts information into each
>>>>> client device about the SMMU(s) that affect it. Is it better to flip the
>>>>> Tegra binding around to match the style of the ARM SMMU?
>>>>
>>>> One question here: How do you ensure that the SMMU driver probe()s
>>>> before probe() runs for devices affected by the SMMU?
>>>
>>> I think we get away with this by virtue of nobody actually creating IOMMU
>>> mappings from within drivers,
>>
>> Where are the mapping set up? What prevents that action from happening
>> before the IOMMU is set up and the various devices have their DMA ops
>> hooked through the IOMMU? It's fairly clear how deferred probe of client
>> devices could achieve that, but I'm quite unsure how some what other
>> mechanism would ensure that.
> 
> This is the part where I start to struggle. If you look at a mainline
> kernel, arm_iommu_create_mapping isn't really used, so it's difficult to
> know at which point the IOMMU needs to be initialised across different ARM
> SoCs.
> 
> For other architectures, it looks like the IOMMU initialisation hangs
> directly off arch code (which in turn are driven from initcalls), but that's
> generally possible because either the IOMMU is architected, or it's strongly
> tied to PCI initialisation.
> 
> What are the requirements for Tegra? If the IOMMU isn't initialised, does it
> act as a passthrough?

I believe so, yes. In particular we have the following register bits:

1) Register bit AHB_ARBITRATION_XBAR_CTRL_SMMU_INIT_DONE is an overall
SMMU enable flag. This sits outside the SMMU module itself, but the SMMU
probe function requests drivers/amba/tegra-ahb.c:tegra_ahb_enable_smmu()
to go enable it.

2) Each memory client ID has a bit in SMMU_TRANSLATION_ENABLE_* to
enable translation for the client. This is also enabled during the SMMU
probe function.

3) Each swgroup ID has a register indicating which ASID to use for
translation, and an associated enable bit. These are also set up during
SMMU probe at present.

I believe when any of those bits aren't enabled, the SMMU is a
pass-through, considering that I'm pretty sure if I disable the SMMU
driver in the kernel, everything will still work just fine.

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Stephen Warren]

On 11/01/2013 10:34 AM, Will Deacon wrote:
> On Fri, Nov 01, 2013 at 04:08:52PM +0000, Stephen Warren wrote:
>> On 10/31/2013 01:39 PM, Will Deacon wrote:
>>> On Thu, Oct 31, 2013 at 07:25:25PM +0000, Stephen Warren wrote:
>>>> On 10/31/2013 01:16 PM, Stephen Warren wrote:
>>>>> Hmm. That's interesting. I see that the ARM SMMU has a list of the
>>>>> clients it affects, whereas this Tegra series puts information into each
>>>>> client device about the SMMU(s) that affect it. Is it better to flip the
>>>>> Tegra binding around to match the style of the ARM SMMU?
...
>>> so the ordering is already handled. However, I
>>> also have a patch queued to move the SMMU initialisation to a
>>> subsys_initcall. I'm not particularly fond of that, but it seems to be the
>>> done thing (even for other architectures).
>>
>> We've been trying very hard not to rely on initcall ordering to solve
>> issues like this, simply it doesn't scale to complex use-cases, and
>> different HW/drivers/... might have conflicting requirements that aren't
>> possible to solve by tweaking initcall levels. It seems much more
>> preferable to use something that's guaranteed to work and adapt to any
>> situation, such as deferred probe.
> 
> Indeed. As I said, I'm not a huge fan of initcalls either, but that's the
> way things are at the moment. If deferred probe can be used to make that
> less fragile, then that sounds like a good thing to do, perhaps across other
> architectures too.

Well, the reason I ask is that if the SMMU clients know which SMMU
they're affected by, it's easy for them to implement deferred probe. If
the SMMU knows which clients it affects, the clients won't have access
to that information, and hence implementing deferred probe will be hard.
That then impacts what might be the preferred style of DT bindings for
SMMUs. Of course, that is driver implementation details forcing a
particular DT binding, which isn't great, but I expect similar issues
would exist on any OS, so this doesn't seem too terrible in this case.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Stephen Warren]

On 11/01/2013 12:49 AM, Hiroshi Doyu wrote:
> On Thu, 31 Oct 2013 18:53:22 +0100
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> ...
>> We're talking about memory-mapped on-SoC devices here, that generally
>> only exist inside Tegra SoCs.
>>
>> Even ignoring that (i.e. expanding the argument to arbitrary modules),
>> having drivers that perform bus-master transactions call a function
>> of_iommu_attach() or similar, which does nothing if the device isn't
>> behind an IOMMU but otherwise does whatever is required, seems like it
>> isn't much of an imposition.
> 
> Where do you expect of_iommu_attach() to be called?
> I thought something below:

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

> @@ -278,6 +278,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  	if (ret)
>  		goto probe_failed;
>  
> +	ret = of_iommu_attach(dev);
> +	if (ret)
> +		goto probe_failed;

That sounds reasonable.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Stephen Warren]

On 11/01/2013 03:59 AM, Hiroshi Doyu wrote:
...
> One idea is that, rather than inserting a hook(function) per
> subsystems in driver core, if we invent a new /special section/ which
> collects all hooks in sequence like initcalls, the subsystem just
> would declare a hook function for that special section. All hooks in
> that section can be called from driver core iterately. Then we
> wouldn't touch driver core code at all, once we insert a func to call
> hooks repeatedly. Looks simpler than tweaking bus notifier.

That sounds like a lot of effort to avoid adding a simple function call
to the core's probe path.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Hiroshi Doyu]

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Fri, 1 Nov 2013 22:45:19 +0100:

> On 11/01/2013 03:59 AM, Hiroshi Doyu wrote:
> ...
> > One idea is that, rather than inserting a hook(function) per
> > subsystems in driver core, if we invent a new /special section/ which
> > collects all hooks in sequence like initcalls, the subsystem just
> > would declare a hook function for that special section. All hooks in
> > that section can be called from driver core iterately. Then we
> > wouldn't touch driver core code at all, once we insert a func to call
> > hooks repeatedly. Looks simpler than tweaking bus notifier.
> 
> That sounds like a lot of effort to avoid adding a simple function call
> to the core's probe path.

True.

Can be implemented/revisited later if needed after we once have a
simple function call, of_iommu_attach() in drvier core.

Re: [PATCHv3 14/19] iommu/tegra: smmu: Get "nvidia,memory-clients" from DT

[Hiroshi Doyu]

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Fri, 1 Nov 2013 18:05:09 +0100:

> > What are the requirements for Tegra? If the IOMMU isn't initialised, does it
> > act as a passthrough?
> 
> I believe so, yes. In particular we have the following register bits:
> 
> 1) Register bit AHB_ARBITRATION_XBAR_CTRL_SMMU_INIT_DONE is an overall
> SMMU enable flag. This sits outside the SMMU module itself, but the SMMU
> probe function requests drivers/amba/tegra-ahb.c:tegra_ahb_enable_smmu()
> to go enable it.
> 
> 2) Each memory client ID has a bit in SMMU_TRANSLATION_ENABLE_* to
> enable translation for the client. This is also enabled during the SMMU
> probe function.
> 
> 3) Each swgroup ID has a register indicating which ASID to use for
> translation, and an associated enable bit. These are also set up during
> SMMU probe at present.
> 
> I believe when any of those bits aren't enabled, the SMMU is a
> pass-through, considering that I'm pretty sure if I disable the SMMU
> driver in the kernel, everything will still work just fine.

Yes, and this is also one of the requirements that we always need to
make sure that all Tegra peripheal devices work with and without
IOMMU, mainly for debugging purpose.

Re: [PATCHv3 10/19] iommu/tegra: smmu: Get "nvidia,swgroups" from DT

[Hiroshi Doyu]

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Fri, 1 Nov 2013 17:54:37 +0100:

> On 10/31/2013 02:17 AM, Hiroshi Doyu wrote:
> > Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Wed, 30 Oct 2013 23:33:32 +0100:
> ...
> > Right.
> > "memory client ID" is used to find out MC_SMMU_<swgroup>_ASID_0
> > register. This register is used to associate <swgroup> to address
> > space(AS). <swgroup> == H/W. <swgroup> can be attached to any AS.
> > 
> >> Is "swgroup" simply another name for "memory client ID"? If so, it'd
> >> be good to use just one term consistently.
> > 
> > I used the name "memory client ID" because this ID can be used to find
> > out HOTRESET bit in MC_CLIENT_HOTRESET_*_0 registers in addition to
> > find the MC_SMMU_<swgroup>_ASID_0 offset. But maybe it's easy to use
> > the consistent name as "swgroup". If laster HOTRESET wants automatic
> > calculation they could borrow/redefine the same ID list, just
> > replacing the prefix. What do you think?
> > 
> >> Assuming "swgroup" is "memory client ID",
> > 
> > Yes
> 
> I took a look at the TRM, and it does look like there's a distinction
> between client IDs and swgroups. The basic idea is that each swgroup
> encompasses n (n >= 1) memory client IDs. So, we do have to use both terms.

Well, I considred that memory-client's' ~= 'a' swgroup, and the term
"swgroup" isn't used in HOTREST, where actually each bit represents
a "swgroup".

> However, the bits in the HOTRESET register appear mostly align with
> swgroup IDs rather client IDs, so I think some of your explanation above
> is innaccurate.
> 
> I do want to point out that the HOTRESET bits don't 100% match the
> swgroup IDs though, so the driver needs to be careful, and perhaps
> contain some kind of mapping table between the two numbering spaces. In
> particular, on Tegra114, swgroups and ASID registers appear in order
> PPCS, <gap>, SATA, whereas there's no gap between the HOTRESET bits for
> PPCS and SATA, assuming the TRM is correct.

In our downstream, basically I provided to two conversion tables, one
for "MC_SMMU_<swgroup ID>_ASID_0" and the other for "HOTRESET"
bit. That table converts ID to them respectively. I think that this is
acceptable.

[RFC][PATCHv3+ 1/2] driver/core: Add of_iommu_attach()

[Hiroshi Doyu]

[-- Attachment #1: Type: text/plain, Size: 4463 bytes --]

Hi,

Experimentally I have implemented "of_iommu_attach()" called from
drvier/core to control the order of device instanciation.

In the Tegra SMMU PATCHv3, we've discussed how to control the order of
device instanciation. Thierry/Stephen proposed to insert a hook in
driver/core to control this order, depending on whether an iommu
device is ready or not."of_iommu_attach()" is implement for that
purpose now. Along with this patch, I attached DT part of modication
to this mail. I used the same iommu bindings which arm,smmu uses.

[RFC][PATCHv3+ 2/2] ARM: DT: tegra30: iommu: Add "stream-id-cells"/"mmu-masters"

"#stream-id-cells" is used to identify whether a device is IOMMU'able
or not. If a device is IOMMU'able, we'll defer to instanciate that
device till an iommu device is instanciated/ready. Once an iommu device
is instanciated, "dev->bus->iommu_ops" is set in the bus. After an
iommu device is instanciated, those defered devices are instanciated
as IOMMU'able with help of the iommu driver via iommu_ops->add_device().

We don't call bus_set_iommu() until an iommu device is instanciated
because we need to support 2 kind of IOMMU drivers, SMMU and GART so
that this bus_set_iommu() needs to be defered till an iommu device is
instanciated. So the single image can support 2 iommu drivers at once.

With this patch, the following HACK patches in v3[1] are not needed
anymore.

  patch 1: [HACK] of: dev_node has struct device pointer
  patch 2: [HACK] ARM: tegra: Populate AHB/IOMMU earlier than others
  patch 3: [HACK] amba: Move AHB to core_initcall
  patch 4: [HACK] iommu/tegra: smmu: Move IOMMU to core_initcall

Any comment would be really appreciated.

[1] Original v3:
http://lists.linuxfoundation.org/pipermail/iommu/2013-October/006724.html

[2]
http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006864.html

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/base/dd.c        |  5 +++++
 drivers/iommu/of_iommu.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/iommu.h    |  7 +++++++
 3 files changed, 46 insertions(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 35fa368..ca76290 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/iommu.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -273,6 +274,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 
 	dev->driver = drv;
 
+	ret = of_iommu_attach(dev);
+	if (ret)
+		goto probe_failed;
+
 	/* If using pinctrl, bind pins now before probing */
 	ret = pinctrl_bind_pins(dev);
 	if (ret)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index ee249bc..f211cdc 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -20,6 +20,9 @@
 #include <linux/export.h>
 #include <linux/limits.h>
 #include <linux/of.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/iommu.h>
 
 /**
  * of_get_dma_window - Parse *dma-window property and returns 0 if found.
@@ -88,3 +91,34 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+static bool of_is_iommuable(struct device *dev)
+{
+	size_t bytes;
+	const __be32 *prop;
+	const char *propname = "#stream-id-cells";
+
+	prop = of_get_property(dev->of_node, propname, &bytes);
+	if (!prop || !bytes)
+		return false;
+
+	pr_debug("%s=%d %s\n", propname, bytes, dev_name(dev));
+	return true;
+}
+
+int of_iommu_attach(struct device *dev)
+{
+	struct iommu_ops *ops;
+
+	if (!of_is_iommuable(dev))
+		return 0;
+
+	ops = dev->bus->iommu_ops;
+	if (!ops)
+		return -EPROBE_DEFER;
+
+	if (ops->add_device)
+		return ops->add_device(dev);
+
+	return 0;
+}
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index a444c79..de16bf2 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -232,6 +232,8 @@ static inline int report_iommu_fault(struct iommu_domain *domain,
 	return ret;
 }
 
+extern int of_iommu_attach(struct device *dev);
+
 #else /* CONFIG_IOMMU_API */
 
 struct iommu_ops {};
@@ -390,6 +392,11 @@ static inline int iommu_domain_set_attr(struct iommu_domain *domain,
 	return -EINVAL;
 }
 
+static inline int of_iommu_attach(struct device *dev)
+{
+	return 0;
+}
+
 #endif /* CONFIG_IOMMU_API */
 
 #endif /* __LINUX_IOMMU_H */
-- 
1.8.1.5


[-- Attachment #2: 0002-ARM-DT-tegra30-iommu-Add-stream-id-cells-mmu-masters.patch --]
[-- Type: text/x-diff, Size: 7181 bytes --]

From fa49c864c1b05df6de16cfc2d12f74fcae843e55 Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Date: Wed, 6 Nov 2013 15:27:03 +0200
Subject: [RFC][PATCHv3+ 2/2] ARM: DT: tegra30: iommu: Add
 "stream-id-cells"/"mmu-masters"

Follow the same syntax as arm,smmu.

FIXME: "nvidia,memory-clients" should be removed.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra30.dtsi | 63 +++++++++++++++++++++++++++++++++---------
 1 file changed, 50 insertions(+), 13 deletions(-)

diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 4c7f296..5913ec9 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -87,60 +87,66 @@
 		};
 	};
 
-	host1x {
+	host1x: host1x {
 		compatible = "nvidia,tegra30-host1x", "simple-bus";
 		reg = <0x50000000 0x00024000>;
 		interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, /* syncpt */
 			     <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>; /* general */
 		clocks = <&tegra_car TEGRA30_CLK_HOST1X>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_HC>;
+		#stream-id-cells = <1>;
 
 		#address-cells = <1>;
 		#size-cells = <1>;
 
 		ranges = <0x54000000 0x54000000 0x04000000>;
 
-		mpe {
+		mpe: mpe {
 			compatible = "nvidia,tegra30-mpe";
 			reg = <0x54040000 0x00040000>;
 			interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_MPE>;
 			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_MPE>;
+			#stream-id-cells = <1>;
 		};
 
-		vi {
+		vi: vi {
 			compatible = "nvidia,tegra30-vi";
 			reg = <0x54080000 0x00040000>;
 			interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_VI>;
 			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_VI>;
+			#stream-id-cells = <1>;
 		};
 
-		epp {
+		epp: epp {
 			compatible = "nvidia,tegra30-epp";
 			reg = <0x540c0000 0x00040000>;
 			interrupts = <GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_EPP>;
 			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_EPP>;
+			#stream-id-cells = <1>;
 		};
 
-		isp {
+		isp: isp {
 			compatible = "nvidia,tegra30-isp";
 			reg = <0x54100000 0x00040000>;
 			interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_ISP>;
 			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_ISP>;
+			#stream-id-cells = <1>;
 		};
 
-		gr2d {
+		gr2d: gr2d {
 			compatible = "nvidia,tegra30-gr2d";
 			reg = <0x54140000 0x00040000>;
 			interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_GR2D>;
 			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_G2>;
+			#stream-id-cells = <1>;
 		};
 
-		gr3d {
+		gr3d: gr3d {
 			compatible = "nvidia,tegra30-gr3d";
 			reg = <0x54180000 0x00040000>;
 			clocks = <&tegra_car TEGRA30_CLK_GR3D
@@ -148,9 +154,10 @@
 			clock-names = "3d", "3d2";
 			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_NV
 						TEGRA_SWGROUP_NV2>;
+			#stream-id-cells = <2>;
 		};
 
-		dc@54200000 {
+		dc: dc@54200000 {
 			compatible = "nvidia,tegra30-dc", "nvidia,tegra20-dc";
 			reg = <0x54200000 0x00040000>;
 			interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;
@@ -158,13 +165,14 @@
 				 <&tegra_car TEGRA30_CLK_PLL_P>;
 			clock-names = "disp1", "parent";
 			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_DC>;
+			#stream-id-cells = <1>;
 
 			rgb {
 				status = "disabled";
 			};
 		};
 
-		dc@54240000 {
+		dcb: dc@54240000 {
 			compatible = "nvidia,tegra30-dc";
 			reg = <0x54240000 0x00040000>;
 			interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>;
@@ -172,6 +180,7 @@
 				 <&tegra_car TEGRA30_CLK_PLL_P>;
 			clock-names = "disp2", "parent";
 			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_DCB>;
+			#stream-id-cells = <1>;
 
 			rgb {
 				status = "disabled";
@@ -329,6 +338,7 @@
 		nvidia,dma-request-selector = <&apbdma 8>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTA>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
@@ -340,6 +350,7 @@
 		nvidia,dma-request-selector = <&apbdma 9>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTB>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
@@ -351,6 +362,7 @@
 		nvidia,dma-request-selector = <&apbdma 10>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTC>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
@@ -362,6 +374,7 @@
 		nvidia,dma-request-selector = <&apbdma 19>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTD>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
@@ -373,6 +386,7 @@
 		nvidia,dma-request-selector = <&apbdma 20>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTE>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
@@ -550,6 +564,25 @@
 		dma-window = <0 0x40000000>;	/* IOVA start & length */
 		nvidia,swgroups = <0x00000000 0x000779ff>;
 		nvidia,ahb = <&ahb>;
+
+		mmu-masters = <&host1x TEGRA_SWGROUP_HC>,
+			      <&mpe TEGRA_SWGROUP_MPE>,
+			      <&vi TEGRA_SWGROUP_VI>,
+			      <&epp TEGRA_SWGROUP_EPP>,
+			      <&isp TEGRA_SWGROUP_ISP>,
+			      <&gr2d TEGRA_SWGROUP_G2>,
+			      <&gr3d TEGRA_SWGROUP_NV TEGRA_SWGROUP_NV2>,
+			      <&dc TEGRA_SWGROUP_DC>,
+			      <&dcb TEGRA_SWGROUP_DCB>,
+			      <&uarta TEGRA_SWGROUP_PPCS>,
+			      <&uartb TEGRA_SWGROUP_PPCS>,
+			      <&uartc TEGRA_SWGROUP_PPCS>,
+			      <&uartd TEGRA_SWGROUP_PPCS>,
+			      <&uarte TEGRA_SWGROUP_PPCS>,
+			      <&sdhci0 TEGRA_SWGROUP_PPCS>,
+			      <&sdhci1 TEGRA_SWGROUP_PPCS>,
+			      <&sdhci2 TEGRA_SWGROUP_PPCS>,
+			      <&sdhci3 TEGRA_SWGROUP_PPCS>;
 	};
 
 	ahub {
@@ -617,39 +650,43 @@
 		};
 	};
 
-	sdhci@78000000 {
+	sdhci0: sdhci@78000000 {
 		compatible = "nvidia,tegra30-sdhci", "nvidia,tegra20-sdhci";
 		reg = <0x78000000 0x200>;
 		interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA30_CLK_SDMMC1>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
-	sdhci@78000200 {
+	sdhci1: sdhci@78000200 {
 		compatible = "nvidia,tegra30-sdhci", "nvidia,tegra20-sdhci";
 		reg = <0x78000200 0x200>;
 		interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA30_CLK_SDMMC2>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
-	sdhci@78000400 {
+	sdhci2: sdhci@78000400 {
 		compatible = "nvidia,tegra30-sdhci", "nvidia,tegra20-sdhci";
 		reg = <0x78000400 0x200>;
 		interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA30_CLK_SDMMC3>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
-	sdhci@78000600 {
+	sdhci3: sdhci@78000600 {
 		compatible = "nvidia,tegra30-sdhci", "nvidia,tegra20-sdhci";
 		reg = <0x78000600 0x200>;
 		interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA30_CLK_SDMMC4>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
-- 
1.8.1.5

[RFC][PATCHv3+ 1/2] driver/core: Add of_iommu_attach()

[Hiroshi Doyu]

[-- Attachment #1: Type: text/plain, Size: 4463 bytes --]

Hi,

Experimentally I have implemented "of_iommu_attach()" called from
drvier/core to control the order of device instanciation.

In the Tegra SMMU PATCHv3, we've discussed how to control the order of
device instanciation. Thierry/Stephen proposed to insert a hook in
driver/core to control this order, depending on whether an iommu
device is ready or not."of_iommu_attach()" is implement for that
purpose now. Along with this patch, I attached DT part of modication
to this mail. I used the same iommu bindings which arm,smmu uses.

[RFC][PATCHv3+ 2/2] ARM: DT: tegra30: iommu: Add "stream-id-cells"/"mmu-masters"

"#stream-id-cells" is used to identify whether a device is IOMMU'able
or not. If a device is IOMMU'able, we'll defer to instanciate that
device till an iommu device is instanciated/ready. Once an iommu device
is instanciated, "dev->bus->iommu_ops" is set in the bus. After an
iommu device is instanciated, those defered devices are instanciated
as IOMMU'able with help of the iommu driver via iommu_ops->add_device().

We don't call bus_set_iommu() until an iommu device is instanciated
because we need to support 2 kind of IOMMU drivers, SMMU and GART so
that this bus_set_iommu() needs to be defered till an iommu device is
instanciated. So the single image can support 2 iommu drivers at once.

With this patch, the following HACK patches in v3[1] are not needed
anymore.

  patch 1: [HACK] of: dev_node has struct device pointer
  patch 2: [HACK] ARM: tegra: Populate AHB/IOMMU earlier than others
  patch 3: [HACK] amba: Move AHB to core_initcall
  patch 4: [HACK] iommu/tegra: smmu: Move IOMMU to core_initcall

Any comment would be really appreciated.

[1] Original v3:
http://lists.linuxfoundation.org/pipermail/iommu/2013-October/006724.html

[2]
http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006864.html

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/base/dd.c        |  5 +++++
 drivers/iommu/of_iommu.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/iommu.h    |  7 +++++++
 3 files changed, 46 insertions(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 35fa368..ca76290 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/iommu.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -273,6 +274,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 
 	dev->driver = drv;
 
+	ret = of_iommu_attach(dev);
+	if (ret)
+		goto probe_failed;
+
 	/* If using pinctrl, bind pins now before probing */
 	ret = pinctrl_bind_pins(dev);
 	if (ret)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index ee249bc..f211cdc 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -20,6 +20,9 @@
 #include <linux/export.h>
 #include <linux/limits.h>
 #include <linux/of.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/iommu.h>
 
 /**
  * of_get_dma_window - Parse *dma-window property and returns 0 if found.
@@ -88,3 +91,34 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
+
+static bool of_is_iommuable(struct device *dev)
+{
+	size_t bytes;
+	const __be32 *prop;
+	const char *propname = "#stream-id-cells";
+
+	prop = of_get_property(dev->of_node, propname, &bytes);
+	if (!prop || !bytes)
+		return false;
+
+	pr_debug("%s=%d %s\n", propname, bytes, dev_name(dev));
+	return true;
+}
+
+int of_iommu_attach(struct device *dev)
+{
+	struct iommu_ops *ops;
+
+	if (!of_is_iommuable(dev))
+		return 0;
+
+	ops = dev->bus->iommu_ops;
+	if (!ops)
+		return -EPROBE_DEFER;
+
+	if (ops->add_device)
+		return ops->add_device(dev);
+
+	return 0;
+}
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index a444c79..de16bf2 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -232,6 +232,8 @@ static inline int report_iommu_fault(struct iommu_domain *domain,
 	return ret;
 }
 
+extern int of_iommu_attach(struct device *dev);
+
 #else /* CONFIG_IOMMU_API */
 
 struct iommu_ops {};
@@ -390,6 +392,11 @@ static inline int iommu_domain_set_attr(struct iommu_domain *domain,
 	return -EINVAL;
 }
 
+static inline int of_iommu_attach(struct device *dev)
+{
+	return 0;
+}
+
 #endif /* CONFIG_IOMMU_API */
 
 #endif /* __LINUX_IOMMU_H */
-- 
1.8.1.5


[-- Attachment #2: 0002-ARM-DT-tegra30-iommu-Add-stream-id-cells-mmu-masters.patch --]
[-- Type: text/x-diff, Size: 7182 bytes --]

>From fa49c864c1b05df6de16cfc2d12f74fcae843e55 Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Date: Wed, 6 Nov 2013 15:27:03 +0200
Subject: [RFC][PATCHv3+ 2/2] ARM: DT: tegra30: iommu: Add
 "stream-id-cells"/"mmu-masters"

Follow the same syntax as arm,smmu.

FIXME: "nvidia,memory-clients" should be removed.

Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra30.dtsi | 63 +++++++++++++++++++++++++++++++++---------
 1 file changed, 50 insertions(+), 13 deletions(-)

diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 4c7f296..5913ec9 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -87,60 +87,66 @@
 		};
 	};
 
-	host1x {
+	host1x: host1x {
 		compatible = "nvidia,tegra30-host1x", "simple-bus";
 		reg = <0x50000000 0x00024000>;
 		interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>, /* syncpt */
 			     <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>; /* general */
 		clocks = <&tegra_car TEGRA30_CLK_HOST1X>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_HC>;
+		#stream-id-cells = <1>;
 
 		#address-cells = <1>;
 		#size-cells = <1>;
 
 		ranges = <0x54000000 0x54000000 0x04000000>;
 
-		mpe {
+		mpe: mpe {
 			compatible = "nvidia,tegra30-mpe";
 			reg = <0x54040000 0x00040000>;
 			interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_MPE>;
 			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_MPE>;
+			#stream-id-cells = <1>;
 		};
 
-		vi {
+		vi: vi {
 			compatible = "nvidia,tegra30-vi";
 			reg = <0x54080000 0x00040000>;
 			interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_VI>;
 			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_VI>;
+			#stream-id-cells = <1>;
 		};
 
-		epp {
+		epp: epp {
 			compatible = "nvidia,tegra30-epp";
 			reg = <0x540c0000 0x00040000>;
 			interrupts = <GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_EPP>;
 			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_EPP>;
+			#stream-id-cells = <1>;
 		};
 
-		isp {
+		isp: isp {
 			compatible = "nvidia,tegra30-isp";
 			reg = <0x54100000 0x00040000>;
 			interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_ISP>;
 			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_ISP>;
+			#stream-id-cells = <1>;
 		};
 
-		gr2d {
+		gr2d: gr2d {
 			compatible = "nvidia,tegra30-gr2d";
 			reg = <0x54140000 0x00040000>;
 			interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&tegra_car TEGRA30_CLK_GR2D>;
 			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_G2>;
+			#stream-id-cells = <1>;
 		};
 
-		gr3d {
+		gr3d: gr3d {
 			compatible = "nvidia,tegra30-gr3d";
 			reg = <0x54180000 0x00040000>;
 			clocks = <&tegra_car TEGRA30_CLK_GR3D
@@ -148,9 +154,10 @@
 			clock-names = "3d", "3d2";
 			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_NV
 						TEGRA_SWGROUP_NV2>;
+			#stream-id-cells = <2>;
 		};
 
-		dc@54200000 {
+		dc: dc@54200000 {
 			compatible = "nvidia,tegra30-dc", "nvidia,tegra20-dc";
 			reg = <0x54200000 0x00040000>;
 			interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;
@@ -158,13 +165,14 @@
 				 <&tegra_car TEGRA30_CLK_PLL_P>;
 			clock-names = "disp1", "parent";
 			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_DC>;
+			#stream-id-cells = <1>;
 
 			rgb {
 				status = "disabled";
 			};
 		};
 
-		dc@54240000 {
+		dcb: dc@54240000 {
 			compatible = "nvidia,tegra30-dc";
 			reg = <0x54240000 0x00040000>;
 			interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>;
@@ -172,6 +180,7 @@
 				 <&tegra_car TEGRA30_CLK_PLL_P>;
 			clock-names = "disp2", "parent";
 			nvidia,memory-clients = <&smmu TEGRA_SWGROUP_DCB>;
+			#stream-id-cells = <1>;
 
 			rgb {
 				status = "disabled";
@@ -329,6 +338,7 @@
 		nvidia,dma-request-selector = <&apbdma 8>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTA>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
@@ -340,6 +350,7 @@
 		nvidia,dma-request-selector = <&apbdma 9>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTB>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
@@ -351,6 +362,7 @@
 		nvidia,dma-request-selector = <&apbdma 10>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTC>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
@@ -362,6 +374,7 @@
 		nvidia,dma-request-selector = <&apbdma 19>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTD>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
@@ -373,6 +386,7 @@
 		nvidia,dma-request-selector = <&apbdma 20>;
 		clocks = <&tegra_car TEGRA30_CLK_UARTE>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
@@ -550,6 +564,25 @@
 		dma-window = <0 0x40000000>;	/* IOVA start & length */
 		nvidia,swgroups = <0x00000000 0x000779ff>;
 		nvidia,ahb = <&ahb>;
+
+		mmu-masters = <&host1x TEGRA_SWGROUP_HC>,
+			      <&mpe TEGRA_SWGROUP_MPE>,
+			      <&vi TEGRA_SWGROUP_VI>,
+			      <&epp TEGRA_SWGROUP_EPP>,
+			      <&isp TEGRA_SWGROUP_ISP>,
+			      <&gr2d TEGRA_SWGROUP_G2>,
+			      <&gr3d TEGRA_SWGROUP_NV TEGRA_SWGROUP_NV2>,
+			      <&dc TEGRA_SWGROUP_DC>,
+			      <&dcb TEGRA_SWGROUP_DCB>,
+			      <&uarta TEGRA_SWGROUP_PPCS>,
+			      <&uartb TEGRA_SWGROUP_PPCS>,
+			      <&uartc TEGRA_SWGROUP_PPCS>,
+			      <&uartd TEGRA_SWGROUP_PPCS>,
+			      <&uarte TEGRA_SWGROUP_PPCS>,
+			      <&sdhci0 TEGRA_SWGROUP_PPCS>,
+			      <&sdhci1 TEGRA_SWGROUP_PPCS>,
+			      <&sdhci2 TEGRA_SWGROUP_PPCS>,
+			      <&sdhci3 TEGRA_SWGROUP_PPCS>;
 	};
 
 	ahub {
@@ -617,39 +650,43 @@
 		};
 	};
 
-	sdhci@78000000 {
+	sdhci0: sdhci@78000000 {
 		compatible = "nvidia,tegra30-sdhci", "nvidia,tegra20-sdhci";
 		reg = <0x78000000 0x200>;
 		interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA30_CLK_SDMMC1>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
-	sdhci@78000200 {
+	sdhci1: sdhci@78000200 {
 		compatible = "nvidia,tegra30-sdhci", "nvidia,tegra20-sdhci";
 		reg = <0x78000200 0x200>;
 		interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA30_CLK_SDMMC2>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
-	sdhci@78000400 {
+	sdhci2: sdhci@78000400 {
 		compatible = "nvidia,tegra30-sdhci", "nvidia,tegra20-sdhci";
 		reg = <0x78000400 0x200>;
 		interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA30_CLK_SDMMC3>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
-	sdhci@78000600 {
+	sdhci3: sdhci@78000600 {
 		compatible = "nvidia,tegra30-sdhci", "nvidia,tegra20-sdhci";
 		reg = <0x78000600 0x200>;
 		interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA30_CLK_SDMMC4>;
 		nvidia,memory-clients = <&smmu TEGRA_SWGROUP_PPCS>;
+		#stream-id-cells = <1>;
 		status = "disabled";
 	};
 
-- 
1.8.1.5

Re: [RFC][PATCHv3+ 1/2] driver/core: Add of_iommu_attach()

[Hiroshi Doyu]

Hi Stephen,

Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote @ Wed, 6 Nov 2013 15:06:23 +0100:

> Experimentally I have implemented "of_iommu_attach()" called from
> drvier/core to control the order of device instanciation.
> 
> In the Tegra SMMU PATCHv3, we've discussed how to control the order of
> device instanciation. Thierry/Stephen proposed to insert a hook in
> driver/core to control this order, depending on whether an iommu
> device is ready or not."of_iommu_attach()" is implement for that
> purpose now. Along with this patch, I attached DT part of modication
> to this mail. I used the same iommu bindings which arm,smmu uses.
> 
> [RFC][PATCHv3+ 2/2] ARM: DT: tegra30: iommu: Add "stream-id-cells"/"mmu-masters"
> 
> "#stream-id-cells" is used to identify whether a device is IOMMU'able
> or not. If a device is IOMMU'able, we'll defer to instanciate that
> device till an iommu device is instanciated/ready. Once an iommu device
> is instanciated, "dev->bus->iommu_ops" is set in the bus. After an
> iommu device is instanciated, those defered devices are instanciated
> as IOMMU'able with help of the iommu driver via iommu_ops->add_device().
> 
> We don't call bus_set_iommu() until an iommu device is instanciated
> because we need to support 2 kind of IOMMU drivers, SMMU and GART so
> that this bus_set_iommu() needs to be defered till an iommu device is
> instanciated. So the single image can support 2 iommu drivers at once.
> 
> With this patch, the following HACK patches in v3[1] are not needed
> anymore.
> 
>   patch 1: [HACK] of: dev_node has struct device pointer
>   patch 2: [HACK] ARM: tegra: Populate AHB/IOMMU earlier than others
>   patch 3: [HACK] amba: Move AHB to core_initcall
>   patch 4: [HACK] iommu/tegra: smmu: Move IOMMU to core_initcall
> 
> Any comment would be really appreciated.

Have you had any chance to take a look at this one?

I plan to send version 4 of this series with the above design next
week. In v4, I use "mmu-masters" binding stolen from "arm,smmu" DT.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Grant Likely]

On Mon, 28 Oct 2013 08:31:34 +0100, Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Oct 25, 2013 at 08:01:36PM +0100, Grant Likely wrote:
> > On Fri, 25 Oct 2013 12:49:38 +0200, Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > On Fri, Oct 25, 2013 at 11:49:05AM +0200, Hiroshi Doyu wrote:
> > > > Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote @ Fri, 25 Oct 2013 11:11:05 +0200:
> > > > 
> > > > > > > This is actually "the other problem that I'm aware of that could benefit
> > > > > > > from [interrupt resolution at probe time]". My idea was that once we had
> > > > > > > a way within the driver core to resolve interrupt references at probe
> > > > > > > time it could be used for potentially many other resources as well. Some
> > > > > > > of the resources like GPIOs and regulators are obviously not something
> > > > > > > that the core can or should be requesting, but mostly resources that you
> > > > > > > don't actually need to control after probing (such as interrupts) would
> > > > > > > be a good fit because otherwise people would write the same boilerplate
> > > > > > > over and over again.
> > > > > > > 
> > > > > > > IOMMUs seem to me to be in that same category. As far as I can tell, an
> > > > > > > IOMMU driver registers the IOMMU for a given bus, upon which every
> > > > > > > device can simply be used (mostly transparently) with that IOMMU. While
> > > > > > > I haven't figured out how exactly, I'm pretty sure we can take advantage
> > > > > > > of the resolution of resources at probe time within the core to both
> > > > > > > keep drivers from having to do anything in particular and at the same
> > > > > > > time have code to determine if the IOMMU driver hasn't been probed yet
> > > > > > > and return -EPROBE_DEFER appropriately.
> > > > > > 
> > > > > > Can you explain the above a bit more?
> > > > > > 
> > > > > > Originally I thought that what Grant suggested would work ok with this
> > > > > > patch.
> > > > > 
> > > > > I think the objection to these patches is that they special case the
> > > > > instantiation of some devices. It's not a proper solution because it
> > > > > implies various things. For example merely instantiating the IOMMU
> > > > > device earlier than others is only going to work *if* the driver is
> > > > > actually probed before the drivers of other devices. If you want to
> > > > > build the driver as a module for example, probe order becomes entirely
> > > > > non-deterministic.
> > > > 
> > > > I understand the above limitation. What I thought for the solution is
> > > > that I can make use of iommu_bus even before IOMMU is
> > > > instanciated. iommu_bus has its notifier which calls
> > > > iommu_ops()->add_device(). This could return -EPROBE_DEFER when IOMMU
> > > > isn't ready. Only the problem is the current "bus_notifier" doesn't
> > > > have the ability to return error. I'll see if it can be modified. Even
> > > > with this, at least IOMMU *driver* needs to be init'ed enough earlier
> > > > to prevent devices from being registered without IOMMU.
> > > 
> > > The way notifiers work is that they run completely hidden from whatever
> > > triggers them. For instance you register the IOMMU bus notifier from the
> > > IOMMU driver (by calling bus_set_iommu()). That registers a function to
> > > be called when some event happens on that bus. When a device's driver is
> > > probed successfully, the driver core will notify the bus, which causes
> > > the IOMMU callback to be run.
> > > 
> > > Some of this code runs before the driver has successfully been probed,
> > > so I imagine it would be possible to use it to abort probing. But that's
> > > not possible at least with the current code.
> > > 
> > > > > Instead of handling such dependencies implicitly by making sure all
> > > > > resource providers are probed earlier than any of their consumers, the
> > > > > dependencies are handled more explicitly, which turns out to simplify
> > > > > things a lot. There's some additional work required in the core, but if
> > > > > done consistently no driver needs to care about the dependencies and it
> > > > > no longer matters where the resources come from. The problem is reduced
> > > > > to essentially this:
> > > > > 
> > > > > 	while (!resource_available())
> > > > > 		load_more_drivers();
> > > > > 
> > > > > So my proposed solution for the IOMMU case is to treat it the same as
> > > > > any other resources. Perhaps resource isn't the right word, but at the
> > > > > core the issue is the same. A device requires the services of an IOMMU
> > > > > so that it can be put into the correct address space. If the IOMMU is
> > > > > not available yet it cannot do that, so we simply return -EPROBE_DEFER
> > > > > and cause the probe to be retried later.
> > > > 
> > > > This looks somewhat similar to the above iommu_bus notifier.
> > > > 
> > > > Is there any way to implement the same mechanism rather than using
> > > > bus?
> > > 
> > > Yes, I think it should be possible to get this to work without using the
> > > bus notifier at all. I can try to code something up but wanted to wait
> > > for feedback from Grant first.
> > 
> > I've lost track. What feedback are you waiting for from me? I've not dug
> > into this entire series so I may not provide clueful feedback.
> 
> I think the context was lost. Here's a link to the relevant message:
> 
> 	http://www.spinics.net/lists/devicetree/msg09709.html
> 
> The message ID is:
> 
> 	20131025075652.GB19622-AwZRO8vwLAwmlAP/+Wk3EA@public.gmane.org

I think getting the interrupts resolved at probe time would be the
better thing to do first. That problem is well understood.

I unfortunately don't know enough about IOMMU configuration to comment
intelligently about how the binding should be set up. My gut instinct is
that device nodes should have references to the IOMMU so that all the
information is easily at hand when a driver attempts to probe. Having
references from the IOMMU node to the devices /seems/ backwards. Will
Deacon I think would be a better person to talk about it with.

g.

Re: [PATCHv3 01/19] [HACK] of: dev_node has struct device pointer

[Grant Likely]

On Wed, 30 Oct 2013 15:58:58 -0600, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 10/25/2013 03:11 AM, Thierry Reding wrote:
> ...
> > So my proposed solution for the IOMMU case is to treat it the same
> > as any other resources. Perhaps resource isn't the right word, but
> > at the core the issue is the same. A device requires the services
> > of an IOMMU so that it can be put into the correct address space.
> > If the IOMMU is not available yet it cannot do that, so we simply
> > return -EPROBE_DEFER and cause the probe to be retried later.
> 
> Personally, I view deferred probe as being used when one device
> requires either a resource /or/ a service provided by another, not
> /just/ when there's a resource dependency. Hence, I think it fits
> perfectly here.

How are those two things different? :-)

g.