[PATCH v2 0/9] bugfixs and add support for non-pci devices

[PATCH v2 0/9] bugfixs and add support for non-pci devices

[Zhen Lei]

Changelog:
v1 -> v2:
update the implementation of patch 1/9 according to Will Deacon's suggestion.
update the comment of patch 3/9 and 4/9.
use arm_smmu_options to skip the execution of command CMD_PREFETCH_CONFIG, see patch 5/9.
patch 6/9 is base on Laurent's series, to support probe deferral.
patch 7/9 according to Robin Murphy's suggestion, remove global variable arm_smmu_devices, thanks.
patch 9/9 add support for a master with multiple stream IDs.

Addition:
scripts/checkpatch 6/9 reports a warning, this is temporary. I don't want
patch 9/9 to display too many differences just because of add tabs.

scripts/checkpatch.pl 0006-iommu-arm-smmu-to-support-probe-deferral.patch
WARNING: else is not generally useful after a break or return
#161: FILE: drivers/iommu/arm-smmu-v3.c:1938:
+		return -ENODEV;
+	} else {

Zhen Lei (9):
  iommu/arm-smmu: fix the assignment of L1 table log2entries
  iommu/arm-smmu: fix the index calculation of strtab
  iommu/arm-smmu: fix the values of ARM64_TCR_IRGN0_SHIFT and
    ARM64_TCR_ORGN0_SHIFT
  iommu/arm-smmu: enlarge STRTAB_L1_SZ_SHIFT to support larger sidsize
  iommu/arm-smmu: skip the execution of CMD_PREFETCH_CONFIG
  iommu/arm-smmu: to support probe deferral
  iommu/arm-smmu: remove arm_smmu_devices
  iommu/arm-smmu: rename __arm_smmu_get_pci_sid
  iommu/arm-smmu: add support for non-pci devices

 drivers/iommu/arm-smmu-v3.c | 238 ++++++++++++++++++++++++++++++++------------
 1 file changed, 173 insertions(+), 65 deletions(-)

--
1.8.0

[PATCH v2 0/9] bugfixs and add support for non-pci devices

[Zhen Lei]

Changelog:
v1 -> v2:
update the implementation of patch 1/9 according to Will Deacon's suggestion.
update the comment of patch 3/9 and 4/9.
use arm_smmu_options to skip the execution of command CMD_PREFETCH_CONFIG, see patch 5/9.
patch 6/9 is base on Laurent's series, to support probe deferral.
patch 7/9 according to Robin Murphy's suggestion, remove global variable arm_smmu_devices, thanks.
patch 9/9 add support for a master with multiple stream IDs.

Addition:
scripts/checkpatch 6/9 reports a warning, this is temporary. I don't want
patch 9/9 to display too many differences just because of add tabs.

scripts/checkpatch.pl 0006-iommu-arm-smmu-to-support-probe-deferral.patch
WARNING: else is not generally useful after a break or return
#161: FILE: drivers/iommu/arm-smmu-v3.c:1938:
+		return -ENODEV;
+	} else {

Zhen Lei (9):
  iommu/arm-smmu: fix the assignment of L1 table log2entries
  iommu/arm-smmu: fix the index calculation of strtab
  iommu/arm-smmu: fix the values of ARM64_TCR_IRGN0_SHIFT and
    ARM64_TCR_ORGN0_SHIFT
  iommu/arm-smmu: enlarge STRTAB_L1_SZ_SHIFT to support larger sidsize
  iommu/arm-smmu: skip the execution of CMD_PREFETCH_CONFIG
  iommu/arm-smmu: to support probe deferral
  iommu/arm-smmu: remove arm_smmu_devices
  iommu/arm-smmu: rename __arm_smmu_get_pci_sid
  iommu/arm-smmu: add support for non-pci devices

 drivers/iommu/arm-smmu-v3.c | 238 ++++++++++++++++++++++++++++++++------------
 1 file changed, 173 insertions(+), 65 deletions(-)

--
1.8.0

[PATCH v2 1/9] iommu/arm-smmu: fix the assignment of L1 table log2entries

[Zhen Lei]

Add a new local variable to store the value of L1 talbe size, so that it
will not conflict with L1 talbe log2entries(stored in variable size).

Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/arm-smmu-v3.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index f141301..56a1fbe 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2021,21 +2021,23 @@ static int arm_smmu_init_strtab_2lvl(struct arm_smmu_device *smmu)
 {
 	void *strtab;
 	u64 reg;
-	u32 size;
+	u32 size, l1size;
 	int ret;
 	struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;

 	/* Calculate the L1 size, capped to the SIDSIZE */
 	size = STRTAB_L1_SZ_SHIFT - (ilog2(STRTAB_L1_DESC_DWORDS) + 3);
 	size = min(size, smmu->sid_bits - STRTAB_SPLIT);
-	if (size + STRTAB_SPLIT < smmu->sid_bits)
+	cfg->num_l1_ents = 1 << size;
+
+	size += STRTAB_SPLIT;
+	if (size < smmu->sid_bits)
 		dev_warn(smmu->dev,
 			 "2-level strtab only covers %u/%u bits of SID\n",
-			 size + STRTAB_SPLIT, smmu->sid_bits);
+			 size, smmu->sid_bits);

-	cfg->num_l1_ents = 1 << size;
-	size = cfg->num_l1_ents * (STRTAB_L1_DESC_DWORDS << 3);
-	strtab = dma_zalloc_coherent(smmu->dev, size, &cfg->strtab_dma,
+	l1size = cfg->num_l1_ents * (STRTAB_L1_DESC_DWORDS << 3);
+	strtab = dma_zalloc_coherent(smmu->dev, l1size, &cfg->strtab_dma,
 				     GFP_KERNEL);
 	if (!strtab) {
 		dev_err(smmu->dev,
@@ -2056,8 +2058,7 @@ static int arm_smmu_init_strtab_2lvl(struct arm_smmu_device *smmu)
 	ret = arm_smmu_init_l1_strtab(smmu);
 	if (ret)
 		dma_free_coherent(smmu->dev,
-				  cfg->num_l1_ents *
-				  (STRTAB_L1_DESC_DWORDS << 3),
+				  l1size,
 				  strtab,
 				  cfg->strtab_dma);
 	return ret;
--
1.8.0

[PATCH v2 1/9] iommu/arm-smmu: fix the assignment of L1 table log2entries

[Zhen Lei]

Add a new local variable to store the value of L1 talbe size, so that it
will not conflict with L1 talbe log2entries(stored in variable size).

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index f141301..56a1fbe 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2021,21 +2021,23 @@ static int arm_smmu_init_strtab_2lvl(struct arm_smmu_device *smmu)
 {
 	void *strtab;
 	u64 reg;
-	u32 size;
+	u32 size, l1size;
 	int ret;
 	struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;

 	/* Calculate the L1 size, capped to the SIDSIZE */
 	size = STRTAB_L1_SZ_SHIFT - (ilog2(STRTAB_L1_DESC_DWORDS) + 3);
 	size = min(size, smmu->sid_bits - STRTAB_SPLIT);
-	if (size + STRTAB_SPLIT < smmu->sid_bits)
+	cfg->num_l1_ents = 1 << size;
+
+	size += STRTAB_SPLIT;
+	if (size < smmu->sid_bits)
 		dev_warn(smmu->dev,
 			 "2-level strtab only covers %u/%u bits of SID\n",
-			 size + STRTAB_SPLIT, smmu->sid_bits);
+			 size, smmu->sid_bits);

-	cfg->num_l1_ents = 1 << size;
-	size = cfg->num_l1_ents * (STRTAB_L1_DESC_DWORDS << 3);
-	strtab = dma_zalloc_coherent(smmu->dev, size, &cfg->strtab_dma,
+	l1size = cfg->num_l1_ents * (STRTAB_L1_DESC_DWORDS << 3);
+	strtab = dma_zalloc_coherent(smmu->dev, l1size, &cfg->strtab_dma,
 				     GFP_KERNEL);
 	if (!strtab) {
 		dev_err(smmu->dev,
@@ -2056,8 +2058,7 @@ static int arm_smmu_init_strtab_2lvl(struct arm_smmu_device *smmu)
 	ret = arm_smmu_init_l1_strtab(smmu);
 	if (ret)
 		dma_free_coherent(smmu->dev,
-				  cfg->num_l1_ents *
-				  (STRTAB_L1_DESC_DWORDS << 3),
+				  l1size,
 				  strtab,
 				  cfg->strtab_dma);
 	return ret;
--
1.8.0

[PATCH v2 2/9] iommu/arm-smmu: fix the index calculation of strtab

[Zhen Lei]

The element size of cfg->strtab is just one DWORD, should use multiply
operation.

Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/arm-smmu-v3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 56a1fbe..48be7eb 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1064,7 +1064,7 @@ static int arm_smmu_init_l2_strtab(struct arm_smmu_device *smmu, u32 sid)
 		return 0;

 	size = 1 << (STRTAB_SPLIT + ilog2(STRTAB_STE_DWORDS) + 3);
-	strtab = &cfg->strtab[sid >> STRTAB_SPLIT << STRTAB_L1_DESC_DWORDS];
+	strtab = &cfg->strtab[(sid >> STRTAB_SPLIT) * STRTAB_L1_DESC_DWORDS];

 	desc->span = STRTAB_SPLIT + 1;
 	desc->l2ptr = dma_zalloc_coherent(smmu->dev, size, &desc->l2ptr_dma,
--
1.8.0

[PATCH v2 2/9] iommu/arm-smmu: fix the index calculation of strtab

[Zhen Lei]

The element size of cfg->strtab is just one DWORD, should use multiply
operation.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 56a1fbe..48be7eb 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1064,7 +1064,7 @@ static int arm_smmu_init_l2_strtab(struct arm_smmu_device *smmu, u32 sid)
 		return 0;

 	size = 1 << (STRTAB_SPLIT + ilog2(STRTAB_STE_DWORDS) + 3);
-	strtab = &cfg->strtab[sid >> STRTAB_SPLIT << STRTAB_L1_DESC_DWORDS];
+	strtab = &cfg->strtab[(sid >> STRTAB_SPLIT) * STRTAB_L1_DESC_DWORDS];

 	desc->span = STRTAB_SPLIT + 1;
 	desc->l2ptr = dma_zalloc_coherent(smmu->dev, size, &desc->l2ptr_dma,
--
1.8.0

[PATCH v2 3/9] iommu/arm-smmu: fix the values of ARM64_TCR_IRGN0_SHIFT and ARM64_TCR_ORGN0_SHIFT

[Zhen Lei]

In SMMU_CBn_TCR when LPAE enabled, the offset of IRGN0 is 8, the offset
of ORGN0 is 10.

Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/arm-smmu-v3.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 48be7eb..0723034 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -269,10 +269,10 @@
 #define ARM64_TCR_TG0_SHIFT		14
 #define ARM64_TCR_TG0_MASK		0x3UL
 #define CTXDESC_CD_0_TCR_IRGN0_SHIFT	8
-#define ARM64_TCR_IRGN0_SHIFT		24
+#define ARM64_TCR_IRGN0_SHIFT		8
 #define ARM64_TCR_IRGN0_MASK		0x3UL
 #define CTXDESC_CD_0_TCR_ORGN0_SHIFT	10
-#define ARM64_TCR_ORGN0_SHIFT		26
+#define ARM64_TCR_ORGN0_SHIFT		10
 #define ARM64_TCR_ORGN0_MASK		0x3UL
 #define CTXDESC_CD_0_TCR_SH0_SHIFT	12
 #define ARM64_TCR_SH0_SHIFT		12
--
1.8.0

[PATCH v2 3/9] iommu/arm-smmu: fix the values of ARM64_TCR_IRGN0_SHIFT and ARM64_TCR_ORGN0_SHIFT

[Zhen Lei]

In SMMU_CBn_TCR when LPAE enabled, the offset of IRGN0 is 8, the offset
of ORGN0 is 10.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 48be7eb..0723034 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -269,10 +269,10 @@
 #define ARM64_TCR_TG0_SHIFT		14
 #define ARM64_TCR_TG0_MASK		0x3UL
 #define CTXDESC_CD_0_TCR_IRGN0_SHIFT	8
-#define ARM64_TCR_IRGN0_SHIFT		24
+#define ARM64_TCR_IRGN0_SHIFT		8
 #define ARM64_TCR_IRGN0_MASK		0x3UL
 #define CTXDESC_CD_0_TCR_ORGN0_SHIFT	10
-#define ARM64_TCR_ORGN0_SHIFT		26
+#define ARM64_TCR_ORGN0_SHIFT		10
 #define ARM64_TCR_ORGN0_MASK		0x3UL
 #define CTXDESC_CD_0_TCR_SH0_SHIFT	12
 #define ARM64_TCR_SH0_SHIFT		12
--
1.8.0

[PATCH v2 4/9] iommu/arm-smmu: enlarge STRTAB_L1_SZ_SHIFT to support larger sidsize

[Zhen Lei]

Because we will choose the minimum value between STRTAB_L1_SZ_SHIFT and
IDR1.SIDSIZE, so enlarge STRTAB_L1_SZ_SHIFT will not impact the platforms
whose IDR1.SIDSIZE is smaller than old STRTAB_L1_SZ_SHIFT value.

Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/arm-smmu-v3.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 0723034..4e7fd4e 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -199,9 +199,10 @@
  * Stream table.
  *
  * Linear: Enough to cover 1 << IDR1.SIDSIZE entries
- * 2lvl: 8k L1 entries, 256 lazy entries per table (each table covers a PCI bus)
+ * 2lvl: 128k L1 entries,
+ *       256 lazy entries per table (each table covers a PCI bus)
  */
-#define STRTAB_L1_SZ_SHIFT		16
+#define STRTAB_L1_SZ_SHIFT		20
 #define STRTAB_SPLIT			8

 #define STRTAB_L1_DESC_DWORDS		1
--
1.8.0

[PATCH v2 4/9] iommu/arm-smmu: enlarge STRTAB_L1_SZ_SHIFT to support larger sidsize

[Zhen Lei]

Because we will choose the minimum value between STRTAB_L1_SZ_SHIFT and
IDR1.SIDSIZE, so enlarge STRTAB_L1_SZ_SHIFT will not impact the platforms
whose IDR1.SIDSIZE is smaller than old STRTAB_L1_SZ_SHIFT value.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 0723034..4e7fd4e 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -199,9 +199,10 @@
  * Stream table.
  *
  * Linear: Enough to cover 1 << IDR1.SIDSIZE entries
- * 2lvl: 8k L1 entries, 256 lazy entries per table (each table covers a PCI bus)
+ * 2lvl: 128k L1 entries,
+ *       256 lazy entries per table (each table covers a PCI bus)
  */
-#define STRTAB_L1_SZ_SHIFT		16
+#define STRTAB_L1_SZ_SHIFT		20
 #define STRTAB_SPLIT			8

 #define STRTAB_L1_DESC_DWORDS		1
--
1.8.0

[PATCH v2 5/9] iommu/arm-smmu: skip the execution of CMD_PREFETCH_CONFIG

[Zhen Lei]

Hisilicon SMMUv3 devices treat CMD_PREFETCH_CONFIG as a illegal command,
execute it will trigger GERROR interrupt. Although the gerror code manage
to turn the prefetch into a SYNC, and the system can continue to run
normally, but it's ugly to print error information.

Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/arm-smmu-v3.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 4e7fd4e..d6e3494 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -543,6 +543,9 @@ struct arm_smmu_device {
 #define ARM_SMMU_FEAT_HYP		(1 << 12)
 	u32				features;

+#define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
+	u32				options;
+
 	struct arm_smmu_cmdq		cmdq;
 	struct arm_smmu_evtq		evtq;
 	struct arm_smmu_priq		priq;
@@ -603,11 +606,35 @@ struct arm_smmu_domain {
 static DEFINE_SPINLOCK(arm_smmu_devices_lock);
 static LIST_HEAD(arm_smmu_devices);

+struct arm_smmu_option_prop {
+	u32 opt;
+	const char *prop;
+};
+
+static struct arm_smmu_option_prop arm_smmu_options[] = {
+	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
+	{ 0, NULL},
+};
+
 static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
 {
 	return container_of(dom, struct arm_smmu_domain, domain);
 }

+static void parse_driver_options(struct arm_smmu_device *smmu)
+{
+	int i = 0;
+
+	do {
+		if (of_property_read_bool(smmu->dev->of_node,
+						arm_smmu_options[i].prop)) {
+			smmu->options |= arm_smmu_options[i].opt;
+			dev_notice(smmu->dev, "option %s\n",
+				arm_smmu_options[i].prop);
+		}
+	} while (arm_smmu_options[++i].opt);
+}
+
 /* Low-level queue manipulation functions */
 static bool queue_full(struct arm_smmu_queue *q)
 {
@@ -1037,7 +1064,8 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid,
 	arm_smmu_sync_ste_for_sid(smmu, sid);

 	/* It's likely that we'll want to use the new STE soon */
-	arm_smmu_cmdq_issue_cmd(smmu, &prefetch_cmd);
+	if (!(smmu->options & ARM_SMMU_OPT_SKIP_PREFETCH))
+		arm_smmu_cmdq_issue_cmd(smmu, &prefetch_cmd);
 }

 static void arm_smmu_init_bypass_stes(u64 *strtab, unsigned int nent)
@@ -2576,6 +2604,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	if (irq > 0)
 		smmu->gerr_irq = irq;

+	parse_driver_options(smmu);
+
 	/* Probe the h/w */
 	ret = arm_smmu_device_probe(smmu);
 	if (ret)
--
1.8.0

[PATCH v2 5/9] iommu/arm-smmu: skip the execution of CMD_PREFETCH_CONFIG

[Zhen Lei]

Hisilicon SMMUv3 devices treat CMD_PREFETCH_CONFIG as a illegal command,
execute it will trigger GERROR interrupt. Although the gerror code manage
to turn the prefetch into a SYNC, and the system can continue to run
normally, but it's ugly to print error information.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 4e7fd4e..d6e3494 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -543,6 +543,9 @@ struct arm_smmu_device {
 #define ARM_SMMU_FEAT_HYP		(1 << 12)
 	u32				features;

+#define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
+	u32				options;
+
 	struct arm_smmu_cmdq		cmdq;
 	struct arm_smmu_evtq		evtq;
 	struct arm_smmu_priq		priq;
@@ -603,11 +606,35 @@ struct arm_smmu_domain {
 static DEFINE_SPINLOCK(arm_smmu_devices_lock);
 static LIST_HEAD(arm_smmu_devices);

+struct arm_smmu_option_prop {
+	u32 opt;
+	const char *prop;
+};
+
+static struct arm_smmu_option_prop arm_smmu_options[] = {
+	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
+	{ 0, NULL},
+};
+
 static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
 {
 	return container_of(dom, struct arm_smmu_domain, domain);
 }

+static void parse_driver_options(struct arm_smmu_device *smmu)
+{
+	int i = 0;
+
+	do {
+		if (of_property_read_bool(smmu->dev->of_node,
+						arm_smmu_options[i].prop)) {
+			smmu->options |= arm_smmu_options[i].opt;
+			dev_notice(smmu->dev, "option %s\n",
+				arm_smmu_options[i].prop);
+		}
+	} while (arm_smmu_options[++i].opt);
+}
+
 /* Low-level queue manipulation functions */
 static bool queue_full(struct arm_smmu_queue *q)
 {
@@ -1037,7 +1064,8 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_device *smmu, u32 sid,
 	arm_smmu_sync_ste_for_sid(smmu, sid);

 	/* It's likely that we'll want to use the new STE soon */
-	arm_smmu_cmdq_issue_cmd(smmu, &prefetch_cmd);
+	if (!(smmu->options & ARM_SMMU_OPT_SKIP_PREFETCH))
+		arm_smmu_cmdq_issue_cmd(smmu, &prefetch_cmd);
 }

 static void arm_smmu_init_bypass_stes(u64 *strtab, unsigned int nent)
@@ -2576,6 +2604,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	if (irq > 0)
 		smmu->gerr_irq = irq;

+	parse_driver_options(smmu);
+
 	/* Probe the h/w */
 	ret = arm_smmu_device_probe(smmu);
 	if (ret)
--
1.8.0

[PATCH v2 6/9] iommu/arm-smmu: to support probe deferral

[Zhen Lei]

For pci devices, only the root nodes have "iommus" property. So we
should traverse all of its sub nodes in of_xlate.

Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/arm-smmu-v3.c | 119 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 89 insertions(+), 30 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index d6e3494..c569539 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -30,6 +30,8 @@
 #include <linux/of_address.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/of_iommu.h>
+#include <linux/of_platform.h>

 #include "io-pgtable.h"

@@ -1741,10 +1743,23 @@ static void __arm_smmu_release_pci_iommudata(void *data)
 	kfree(data);
 }

-static struct arm_smmu_device *arm_smmu_get_for_pci_dev(struct pci_dev *pdev)
+static struct arm_smmu_device *find_smmu_by_node(struct device_node *np)
+{
+	struct platform_device *pdev;
+
+	/* to ensure np is a smmu device node */
+	if (!of_iommu_get_ops(np))
+		return NULL;
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev)
+		return NULL;
+
+	return platform_get_drvdata(pdev);
+}
+
+static struct device_node *arm_smmu_get_pci_dev_root(struct pci_dev *pdev)
 {
-	struct device_node *of_node;
-	struct arm_smmu_device *curr, *smmu = NULL;
 	struct pci_bus *bus = pdev->bus;

 	/* Walk up to the root bus */
@@ -1752,21 +1767,7 @@ static struct arm_smmu_device *arm_smmu_get_for_pci_dev(struct pci_dev *pdev)
 		bus = bus->parent;

 	/* Follow the "iommus" phandle from the host controller */
-	of_node = of_parse_phandle(bus->bridge->parent->of_node, "iommus", 0);
-	if (!of_node)
-		return NULL;
-
-	/* See if we can find an SMMU corresponding to the phandle */
-	spin_lock(&arm_smmu_devices_lock);
-	list_for_each_entry(curr, &arm_smmu_devices, list) {
-		if (curr->dev->of_node == of_node) {
-			smmu = curr;
-			break;
-		}
-	}
-	spin_unlock(&arm_smmu_devices_lock);
-	of_node_put(of_node);
-	return smmu;
+	return bus->bridge->parent->of_node;
 }

 static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
@@ -1779,27 +1780,21 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
 	return sid < limit;
 }

-static int arm_smmu_add_device(struct device *dev)
+static int arm_smmu_add_device(struct device *dev, u32 sid)
 {
 	int i, ret;
-	u32 sid, *sids;
-	struct pci_dev *pdev;
+	u32 *sids;
 	struct iommu_group *group;
 	struct arm_smmu_group *smmu_group;
 	struct arm_smmu_device *smmu;

-	/* We only support PCI, for now */
-	if (!dev_is_pci(dev))
-		return -ENODEV;
-
-	pdev = to_pci_dev(dev);
 	group = iommu_group_get_for_dev(dev);
 	if (IS_ERR(group))
 		return PTR_ERR(group);

 	smmu_group = iommu_group_get_iommudata(group);
 	if (!smmu_group) {
-		smmu = arm_smmu_get_for_pci_dev(pdev);
+		smmu = dev->archdata.iommu;
 		if (!smmu) {
 			ret = -ENOENT;
 			goto out_put_group;
@@ -1819,8 +1814,6 @@ static int arm_smmu_add_device(struct device *dev)
 		smmu = smmu_group->smmu;
 	}

-	/* Assume SID == RID until firmware tells us otherwise */
-	pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid, &sid);
 	for (i = 0; i < smmu_group->num_sids; ++i) {
 		/* If we already know about this SID, then we're done */
 		if (smmu_group->sids[i] == sid)
@@ -1862,6 +1855,7 @@ out_put_group:

 static void arm_smmu_remove_device(struct device *dev)
 {
+	dev->archdata.iommu = NULL;
 	iommu_group_remove_device(dev);
 }

@@ -1909,7 +1903,68 @@ out_unlock:
 	return ret;
 }

+static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
+{
+	int ret;
+	struct arm_smmu_device *smmu;
+
+	/*
+	 * We can sure that args->np is a smmu device node, because this
+	 * function was called by of_xlate hook.
+	 *
+	 * And in arm_smmu_device_dt_probe:
+	 *	platform_set_drvdata(pdev, smmu);
+	 *	of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);
+	 *
+	 * It seems impossible return NULL in normal times.
+	 */
+	smmu = find_smmu_by_node(args->np);
+	if (!smmu) {
+		dev_err(dev, "unknown error caused smmu driver crashed\n");
+		return -ENODEV;
+	}
+
+	if (!dev->archdata.iommu)
+		dev->archdata.iommu = smmu;
+
+	if (dev->archdata.iommu != smmu) {
+		dev_err(dev, "behinds more than one smmu\n");
+		return -EINVAL;
+	}
+
+	/* We only support PCI, for now */
+	if (!dev_is_pci(dev)) {
+		return -ENODEV;
+	} else {
+		u32 sid;
+		struct device_node *of_root;
+		struct pci_dev *pdev = NULL;
+
+		for_each_pci_dev(pdev) {
+			of_root = arm_smmu_get_pci_dev_root(pdev);
+			if (of_root != dev->of_node)
+				continue;
+
+			/*
+			 * Assume SID == RID until firmware tells us otherwise
+			 */
+			pci_for_each_dma_alias(pdev,
+					__arm_smmu_get_pci_sid, &sid);
+
+			pdev->dev.archdata.iommu = smmu;
+			ret = arm_smmu_add_device(dev, sid);
+			if (ret) {
+				dev_err(dev, "failed to add into SMMU\n");
+				return ret;
+			}
+		}
+	}
+
+	return 0;
+}
+
 static struct iommu_ops arm_smmu_ops = {
+	.of_xlate		= arm_smmu_of_xlate,
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,
 	.domain_free		= arm_smmu_domain_free,
@@ -1918,7 +1973,6 @@ static struct iommu_ops arm_smmu_ops = {
 	.map			= arm_smmu_map,
 	.unmap			= arm_smmu_unmap,
 	.iova_to_phys		= arm_smmu_iova_to_phys,
-	.add_device		= arm_smmu_add_device,
 	.remove_device		= arm_smmu_remove_device,
 	.domain_get_attr	= arm_smmu_domain_get_attr,
 	.domain_set_attr	= arm_smmu_domain_set_attr,
@@ -2626,6 +2680,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	spin_lock(&arm_smmu_devices_lock);
 	list_add(&smmu->list, &arm_smmu_devices);
 	spin_unlock(&arm_smmu_devices_lock);
+	platform_set_drvdata(pdev, smmu);
+	of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);
+
 	return 0;

 out_free_structures:
@@ -2697,6 +2754,8 @@ static void __exit arm_smmu_exit(void)
 subsys_initcall(arm_smmu_init);
 module_exit(arm_smmu_exit);

+IOMMU_OF_DECLARE(arm_smmu_v3, "arm,smmu-v3", NULL);
+
 MODULE_DESCRIPTION("IOMMU API for ARM architected SMMUv3 implementations");
 MODULE_AUTHOR("Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>");
 MODULE_LICENSE("GPL v2");
--
1.8.0

[PATCH v2 6/9] iommu/arm-smmu: to support probe deferral

[Zhen Lei]

For pci devices, only the root nodes have "iommus" property. So we
should traverse all of its sub nodes in of_xlate.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 119 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 89 insertions(+), 30 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index d6e3494..c569539 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -30,6 +30,8 @@
 #include <linux/of_address.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/of_iommu.h>
+#include <linux/of_platform.h>

 #include "io-pgtable.h"

@@ -1741,10 +1743,23 @@ static void __arm_smmu_release_pci_iommudata(void *data)
 	kfree(data);
 }

-static struct arm_smmu_device *arm_smmu_get_for_pci_dev(struct pci_dev *pdev)
+static struct arm_smmu_device *find_smmu_by_node(struct device_node *np)
+{
+	struct platform_device *pdev;
+
+	/* to ensure np is a smmu device node */
+	if (!of_iommu_get_ops(np))
+		return NULL;
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev)
+		return NULL;
+
+	return platform_get_drvdata(pdev);
+}
+
+static struct device_node *arm_smmu_get_pci_dev_root(struct pci_dev *pdev)
 {
-	struct device_node *of_node;
-	struct arm_smmu_device *curr, *smmu = NULL;
 	struct pci_bus *bus = pdev->bus;

 	/* Walk up to the root bus */
@@ -1752,21 +1767,7 @@ static struct arm_smmu_device *arm_smmu_get_for_pci_dev(struct pci_dev *pdev)
 		bus = bus->parent;

 	/* Follow the "iommus" phandle from the host controller */
-	of_node = of_parse_phandle(bus->bridge->parent->of_node, "iommus", 0);
-	if (!of_node)
-		return NULL;
-
-	/* See if we can find an SMMU corresponding to the phandle */
-	spin_lock(&arm_smmu_devices_lock);
-	list_for_each_entry(curr, &arm_smmu_devices, list) {
-		if (curr->dev->of_node == of_node) {
-			smmu = curr;
-			break;
-		}
-	}
-	spin_unlock(&arm_smmu_devices_lock);
-	of_node_put(of_node);
-	return smmu;
+	return bus->bridge->parent->of_node;
 }

 static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
@@ -1779,27 +1780,21 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
 	return sid < limit;
 }

-static int arm_smmu_add_device(struct device *dev)
+static int arm_smmu_add_device(struct device *dev, u32 sid)
 {
 	int i, ret;
-	u32 sid, *sids;
-	struct pci_dev *pdev;
+	u32 *sids;
 	struct iommu_group *group;
 	struct arm_smmu_group *smmu_group;
 	struct arm_smmu_device *smmu;

-	/* We only support PCI, for now */
-	if (!dev_is_pci(dev))
-		return -ENODEV;
-
-	pdev = to_pci_dev(dev);
 	group = iommu_group_get_for_dev(dev);
 	if (IS_ERR(group))
 		return PTR_ERR(group);

 	smmu_group = iommu_group_get_iommudata(group);
 	if (!smmu_group) {
-		smmu = arm_smmu_get_for_pci_dev(pdev);
+		smmu = dev->archdata.iommu;
 		if (!smmu) {
 			ret = -ENOENT;
 			goto out_put_group;
@@ -1819,8 +1814,6 @@ static int arm_smmu_add_device(struct device *dev)
 		smmu = smmu_group->smmu;
 	}

-	/* Assume SID == RID until firmware tells us otherwise */
-	pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid, &sid);
 	for (i = 0; i < smmu_group->num_sids; ++i) {
 		/* If we already know about this SID, then we're done */
 		if (smmu_group->sids[i] == sid)
@@ -1862,6 +1855,7 @@ out_put_group:

 static void arm_smmu_remove_device(struct device *dev)
 {
+	dev->archdata.iommu = NULL;
 	iommu_group_remove_device(dev);
 }

@@ -1909,7 +1903,68 @@ out_unlock:
 	return ret;
 }

+static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
+{
+	int ret;
+	struct arm_smmu_device *smmu;
+
+	/*
+	 * We can sure that args->np is a smmu device node, because this
+	 * function was called by of_xlate hook.
+	 *
+	 * And in arm_smmu_device_dt_probe:
+	 *	platform_set_drvdata(pdev, smmu);
+	 *	of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);
+	 *
+	 * It seems impossible return NULL in normal times.
+	 */
+	smmu = find_smmu_by_node(args->np);
+	if (!smmu) {
+		dev_err(dev, "unknown error caused smmu driver crashed\n");
+		return -ENODEV;
+	}
+
+	if (!dev->archdata.iommu)
+		dev->archdata.iommu = smmu;
+
+	if (dev->archdata.iommu != smmu) {
+		dev_err(dev, "behinds more than one smmu\n");
+		return -EINVAL;
+	}
+
+	/* We only support PCI, for now */
+	if (!dev_is_pci(dev)) {
+		return -ENODEV;
+	} else {
+		u32 sid;
+		struct device_node *of_root;
+		struct pci_dev *pdev = NULL;
+
+		for_each_pci_dev(pdev) {
+			of_root = arm_smmu_get_pci_dev_root(pdev);
+			if (of_root != dev->of_node)
+				continue;
+
+			/*
+			 * Assume SID == RID until firmware tells us otherwise
+			 */
+			pci_for_each_dma_alias(pdev,
+					__arm_smmu_get_pci_sid, &sid);
+
+			pdev->dev.archdata.iommu = smmu;
+			ret = arm_smmu_add_device(dev, sid);
+			if (ret) {
+				dev_err(dev, "failed to add into SMMU\n");
+				return ret;
+			}
+		}
+	}
+
+	return 0;
+}
+
 static struct iommu_ops arm_smmu_ops = {
+	.of_xlate		= arm_smmu_of_xlate,
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,
 	.domain_free		= arm_smmu_domain_free,
@@ -1918,7 +1973,6 @@ static struct iommu_ops arm_smmu_ops = {
 	.map			= arm_smmu_map,
 	.unmap			= arm_smmu_unmap,
 	.iova_to_phys		= arm_smmu_iova_to_phys,
-	.add_device		= arm_smmu_add_device,
 	.remove_device		= arm_smmu_remove_device,
 	.domain_get_attr	= arm_smmu_domain_get_attr,
 	.domain_set_attr	= arm_smmu_domain_set_attr,
@@ -2626,6 +2680,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	spin_lock(&arm_smmu_devices_lock);
 	list_add(&smmu->list, &arm_smmu_devices);
 	spin_unlock(&arm_smmu_devices_lock);
+	platform_set_drvdata(pdev, smmu);
+	of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);
+
 	return 0;

 out_free_structures:
@@ -2697,6 +2754,8 @@ static void __exit arm_smmu_exit(void)
 subsys_initcall(arm_smmu_init);
 module_exit(arm_smmu_exit);

+IOMMU_OF_DECLARE(arm_smmu_v3, "arm,smmu-v3", NULL);
+
 MODULE_DESCRIPTION("IOMMU API for ARM architected SMMUv3 implementations");
 MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>");
 MODULE_LICENSE("GPL v2");
--
1.8.0

[PATCH v2 7/9] iommu/arm-smmu: remove arm_smmu_devices

[Zhen Lei]

It can be replaced by of_iommu_list(in of_iommu.c).

Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/arm-smmu-v3.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index c569539..39c55f6 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -604,10 +604,6 @@ struct arm_smmu_domain {
 	struct iommu_domain		domain;
 };

-/* Our list of SMMU instances */
-static DEFINE_SPINLOCK(arm_smmu_devices_lock);
-static LIST_HEAD(arm_smmu_devices);
-
 struct arm_smmu_option_prop {
 	u32 opt;
 	const char *prop;
@@ -2675,11 +2671,6 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	if (ret)
 		goto out_free_structures;

-	/* Record our private device structure */
-	INIT_LIST_HEAD(&smmu->list);
-	spin_lock(&arm_smmu_devices_lock);
-	list_add(&smmu->list, &arm_smmu_devices);
-	spin_unlock(&arm_smmu_devices_lock);
 	platform_set_drvdata(pdev, smmu);
 	of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);

@@ -2692,19 +2683,10 @@ out_free_structures:

 static int arm_smmu_device_remove(struct platform_device *pdev)
 {
-	struct arm_smmu_device *curr, *smmu = NULL;
+	struct arm_smmu_device *smmu;
 	struct device *dev = &pdev->dev;

-	spin_lock(&arm_smmu_devices_lock);
-	list_for_each_entry(curr, &arm_smmu_devices, list) {
-		if (curr->dev == dev) {
-			smmu = curr;
-			list_del(&smmu->list);
-			break;
-		}
-	}
-	spin_unlock(&arm_smmu_devices_lock);
-
+	smmu = find_smmu_by_node(dev->of_node);
 	if (!smmu)
 		return -ENODEV;

--
1.8.0

[PATCH v2 7/9] iommu/arm-smmu: remove arm_smmu_devices

[Zhen Lei]

It can be replaced by of_iommu_list(in of_iommu.c).

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index c569539..39c55f6 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -604,10 +604,6 @@ struct arm_smmu_domain {
 	struct iommu_domain		domain;
 };

-/* Our list of SMMU instances */
-static DEFINE_SPINLOCK(arm_smmu_devices_lock);
-static LIST_HEAD(arm_smmu_devices);
-
 struct arm_smmu_option_prop {
 	u32 opt;
 	const char *prop;
@@ -2675,11 +2671,6 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	if (ret)
 		goto out_free_structures;

-	/* Record our private device structure */
-	INIT_LIST_HEAD(&smmu->list);
-	spin_lock(&arm_smmu_devices_lock);
-	list_add(&smmu->list, &arm_smmu_devices);
-	spin_unlock(&arm_smmu_devices_lock);
 	platform_set_drvdata(pdev, smmu);
 	of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);

@@ -2692,19 +2683,10 @@ out_free_structures:

 static int arm_smmu_device_remove(struct platform_device *pdev)
 {
-	struct arm_smmu_device *curr, *smmu = NULL;
+	struct arm_smmu_device *smmu;
 	struct device *dev = &pdev->dev;

-	spin_lock(&arm_smmu_devices_lock);
-	list_for_each_entry(curr, &arm_smmu_devices, list) {
-		if (curr->dev == dev) {
-			smmu = curr;
-			list_del(&smmu->list);
-			break;
-		}
-	}
-	spin_unlock(&arm_smmu_devices_lock);
-
+	smmu = find_smmu_by_node(dev->of_node);
 	if (!smmu)
 		return -ENODEV;

--
1.8.0

[PATCH v2 8/9] iommu/arm-smmu: rename __arm_smmu_get_pci_sid

[Zhen Lei]

Remove the words "pci", to make this function can also be used by
non-pci devices.

Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/arm-smmu-v3.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 39c55f6..274d059 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1734,7 +1734,7 @@ static int __arm_smmu_get_pci_sid(struct pci_dev *pdev, u16 alias, void *sidp)
 	return 0; /* Continue walking */
 }

-static void __arm_smmu_release_pci_iommudata(void *data)
+static void __arm_smmu_release_iommudata(void *data)
 {
 	kfree(data);
 }
@@ -1805,7 +1805,7 @@ static int arm_smmu_add_device(struct device *dev, u32 sid)
 		smmu_group->ste.valid	= true;
 		smmu_group->smmu	= smmu;
 		iommu_group_set_iommudata(group, smmu_group,
-					  __arm_smmu_release_pci_iommudata);
+					  __arm_smmu_release_iommudata);
 	} else {
 		smmu = smmu_group->smmu;
 	}
--
1.8.0

[PATCH v2 8/9] iommu/arm-smmu: rename __arm_smmu_get_pci_sid

[Zhen Lei]

Remove the words "pci", to make this function can also be used by
non-pci devices.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 39c55f6..274d059 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1734,7 +1734,7 @@ static int __arm_smmu_get_pci_sid(struct pci_dev *pdev, u16 alias, void *sidp)
 	return 0; /* Continue walking */
 }

-static void __arm_smmu_release_pci_iommudata(void *data)
+static void __arm_smmu_release_iommudata(void *data)
 {
 	kfree(data);
 }
@@ -1805,7 +1805,7 @@ static int arm_smmu_add_device(struct device *dev, u32 sid)
 		smmu_group->ste.valid	= true;
 		smmu_group->smmu	= smmu;
 		iommu_group_set_iommudata(group, smmu_group,
-					  __arm_smmu_release_pci_iommudata);
+					  __arm_smmu_release_iommudata);
 	} else {
 		smmu = smmu_group->smmu;
 	}
--
1.8.0

[PATCH v2 9/9] iommu/arm-smmu: add support for non-pci devices

[Zhen Lei]

This patch support a master with multiple stream IDs, but doesn't support a
master behinds more than one SMMUs.

Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/arm-smmu-v3.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 274d059..582cd1e 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -30,6 +30,7 @@
 #include <linux/of_address.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/amba/bus.h>
 #include <linux/of_iommu.h>
 #include <linux/of_platform.h>

@@ -1928,9 +1929,35 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
 		return -EINVAL;
 	}

-	/* We only support PCI, for now */
 	if (!dev_is_pci(dev)) {
-		return -ENODEV;
+		int i;
+		struct iommu_group *group;
+
+		group = iommu_group_get(dev);
+		if (!group) {
+			group = iommu_group_alloc();
+			if (IS_ERR(group)) {
+				dev_err(dev, "failed to allocate IOMMU group\n");
+				return PTR_ERR(group);
+			}
+
+			ret = iommu_group_add_device(group, dev);
+			if (ret) {
+				dev_err(dev, "failed to add device into IOMMU group\n");
+				return PTR_ERR(group);
+			}
+		}
+		iommu_group_put(group);
+
+		for (i = 0; i < args->args_count; i++) {
+			ret = arm_smmu_add_device(dev, args->args[i]);
+			if (ret) {
+				dev_err(dev,
+					"failed to add sid=%d into SMMU\n",
+					args->args[i]);
+				return ret;
+			}
+		}
 	} else {
 		u32 sid;
 		struct device_node *of_root;
@@ -2725,6 +2752,14 @@ static int __init arm_smmu_init(void)
 	if (ret)
 		return ret;

+	if (!iommu_present(&platform_bus_type))
+		bus_set_iommu(&platform_bus_type, &arm_smmu_ops);
+
+#ifdef CONFIG_ARM_AMBA
+	if (!iommu_present(&amba_bustype))
+		bus_set_iommu(&amba_bustype, &arm_smmu_ops);
+#endif
+
 	return bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
 }

--
1.8.0

[PATCH v2 9/9] iommu/arm-smmu: add support for non-pci devices

[Zhen Lei]

This patch support a master with multiple stream IDs, but doesn't support a
master behinds more than one SMMUs.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/arm-smmu-v3.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 274d059..582cd1e 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -30,6 +30,7 @@
 #include <linux/of_address.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/amba/bus.h>
 #include <linux/of_iommu.h>
 #include <linux/of_platform.h>

@@ -1928,9 +1929,35 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
 		return -EINVAL;
 	}

-	/* We only support PCI, for now */
 	if (!dev_is_pci(dev)) {
-		return -ENODEV;
+		int i;
+		struct iommu_group *group;
+
+		group = iommu_group_get(dev);
+		if (!group) {
+			group = iommu_group_alloc();
+			if (IS_ERR(group)) {
+				dev_err(dev, "failed to allocate IOMMU group\n");
+				return PTR_ERR(group);
+			}
+
+			ret = iommu_group_add_device(group, dev);
+			if (ret) {
+				dev_err(dev, "failed to add device into IOMMU group\n");
+				return PTR_ERR(group);
+			}
+		}
+		iommu_group_put(group);
+
+		for (i = 0; i < args->args_count; i++) {
+			ret = arm_smmu_add_device(dev, args->args[i]);
+			if (ret) {
+				dev_err(dev,
+					"failed to add sid=%d into SMMU\n",
+					args->args[i]);
+				return ret;
+			}
+		}
 	} else {
 		u32 sid;
 		struct device_node *of_root;
@@ -2725,6 +2752,14 @@ static int __init arm_smmu_init(void)
 	if (ret)
 		return ret;

+	if (!iommu_present(&platform_bus_type))
+		bus_set_iommu(&platform_bus_type, &arm_smmu_ops);
+
+#ifdef CONFIG_ARM_AMBA
+	if (!iommu_present(&amba_bustype))
+		bus_set_iommu(&amba_bustype, &arm_smmu_ops);
+#endif
+
 	return bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
 }

--
1.8.0

Re: [PATCH v2 0/9] bugfixs and add support for non-pci devices

[Will Deacon]

Hi Zhen Lei,

On Tue, Jul 07, 2015 at 04:30:13AM +0100, Zhen Lei wrote:
> Changelog:
> v1 -> v2:
> update the implementation of patch 1/9 according to Will Deacon's suggestion.
> update the comment of patch 3/9 and 4/9.
> use arm_smmu_options to skip the execution of command CMD_PREFETCH_CONFIG, see patch 5/9.
> patch 6/9 is base on Laurent's series, to support probe deferral.
> patch 7/9 according to Robin Murphy's suggestion, remove global variable arm_smmu_devices, thanks.
> patch 9/9 add support for a master with multiple stream IDs.

FYI, I already picked some of your previous series on my iommu/devel branch:

  https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/log/?h=iommu/devel

I'll go over your v2 today, but I'm mainly interested in getting fixes
queued for 4.2 atm.

> Addition:
> scripts/checkpatch 6/9 reports a warning, this is temporary. I don't want
> patch 9/9 to display too many differences just because of add tabs.

That's fine. Whilst checkpatch can occassionally be a useful tool, it's
usually far too noisy imo.

Will

[PATCH v2 0/9] bugfixs and add support for non-pci devices

[Will Deacon]

Hi Zhen Lei,

On Tue, Jul 07, 2015 at 04:30:13AM +0100, Zhen Lei wrote:
> Changelog:
> v1 -> v2:
> update the implementation of patch 1/9 according to Will Deacon's suggestion.
> update the comment of patch 3/9 and 4/9.
> use arm_smmu_options to skip the execution of command CMD_PREFETCH_CONFIG, see patch 5/9.
> patch 6/9 is base on Laurent's series, to support probe deferral.
> patch 7/9 according to Robin Murphy's suggestion, remove global variable arm_smmu_devices, thanks.
> patch 9/9 add support for a master with multiple stream IDs.

FYI, I already picked some of your previous series on my iommu/devel branch:

  https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/log/?h=iommu/devel

I'll go over your v2 today, but I'm mainly interested in getting fixes
queued for 4.2 atm.

> Addition:
> scripts/checkpatch 6/9 reports a warning, this is temporary. I don't want
> patch 9/9 to display too many differences just because of add tabs.

That's fine. Whilst checkpatch can occassionally be a useful tool, it's
usually far too noisy imo.

Will

Re: [PATCH v2 5/9] iommu/arm-smmu: skip the execution of CMD_PREFETCH_CONFIG

[Robin Murphy]

Hi,

Thanks for the respin. I still have a few comments on the series, 
starting here;

On 07/07/15 04:30, Zhen Lei wrote:
> Hisilicon SMMUv3 devices treat CMD_PREFETCH_CONFIG as a illegal command,
> execute it will trigger GERROR interrupt. Although the gerror code manage
> to turn the prefetch into a SYNC, and the system can continue to run
> normally, but it's ugly to print error information.

No mention of the DT binding change, and no corresponding documentation 
update either.

> Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
[...]
> +static struct arm_smmu_option_prop arm_smmu_options[] = {
> +	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
> +	{ 0, NULL},
> +};
[...]
> +static void parse_driver_options(struct arm_smmu_device *smmu)
> +{
> +	int i = 0;
> +
> +	do {
> +		if (of_property_read_bool(smmu->dev->of_node,
> +						arm_smmu_options[i].prop)) {
> +			smmu->options |= arm_smmu_options[i].opt;
> +			dev_notice(smmu->dev, "option %s\n",
> +				arm_smmu_options[i].prop);
> +		}
> +	} while (arm_smmu_options[++i].opt);
> +}
> +

Nitpicking for sure, but I'm still waiting for a good excuse to rewrite 
this overcomplicated loop logic in the SMMUv2 driver - can't we just 
treat a static array as a static array and iterate over the thing in the 
obvious way?

	for (i = 0; i < ARRAY_SIZE(arm_smmu_options); i++)

Robin.

[PATCH v2 5/9] iommu/arm-smmu: skip the execution of CMD_PREFETCH_CONFIG

[Robin Murphy]

Hi,

Thanks for the respin. I still have a few comments on the series, 
starting here;

On 07/07/15 04:30, Zhen Lei wrote:
> Hisilicon SMMUv3 devices treat CMD_PREFETCH_CONFIG as a illegal command,
> execute it will trigger GERROR interrupt. Although the gerror code manage
> to turn the prefetch into a SYNC, and the system can continue to run
> normally, but it's ugly to print error information.

No mention of the DT binding change, and no corresponding documentation 
update either.

> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
[...]
> +static struct arm_smmu_option_prop arm_smmu_options[] = {
> +	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
> +	{ 0, NULL},
> +};
[...]
> +static void parse_driver_options(struct arm_smmu_device *smmu)
> +{
> +	int i = 0;
> +
> +	do {
> +		if (of_property_read_bool(smmu->dev->of_node,
> +						arm_smmu_options[i].prop)) {
> +			smmu->options |= arm_smmu_options[i].opt;
> +			dev_notice(smmu->dev, "option %s\n",
> +				arm_smmu_options[i].prop);
> +		}
> +	} while (arm_smmu_options[++i].opt);
> +}
> +

Nitpicking for sure, but I'm still waiting for a good excuse to rewrite 
this overcomplicated loop logic in the SMMUv2 driver - can't we just 
treat a static array as a static array and iterate over the thing in the 
obvious way?

	for (i = 0; i < ARRAY_SIZE(arm_smmu_options); i++)

Robin.

Re: [PATCH v2 6/9] iommu/arm-smmu: to support probe deferral

[Robin Murphy]

On 07/07/15 04:30, Zhen Lei wrote:
> For pci devices, only the root nodes have "iommus" property. So we
> should traverse all of its sub nodes in of_xlate.

I don't really follow this description; only the host controller is 
described in DT - the devices behind it are probed dynamically and don't 
have nodes to traverse.

> Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>   drivers/iommu/arm-smmu-v3.c | 119 +++++++++++++++++++++++++++++++++-----------
>   1 file changed, 89 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index d6e3494..c569539 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -30,6 +30,8 @@
>   #include <linux/of_address.h>
>   #include <linux/pci.h>
>   #include <linux/platform_device.h>
> +#include <linux/of_iommu.h>
> +#include <linux/of_platform.h>
>
>   #include "io-pgtable.h"
>
> @@ -1741,10 +1743,23 @@ static void __arm_smmu_release_pci_iommudata(void *data)
>   	kfree(data);
>   }
>
> -static struct arm_smmu_device *arm_smmu_get_for_pci_dev(struct pci_dev *pdev)
> +static struct arm_smmu_device *find_smmu_by_node(struct device_node *np)
> +{
> +	struct platform_device *pdev;
> +
> +	/* to ensure np is a smmu device node */
> +	if (!of_iommu_get_ops(np))
> +		return NULL;
> +
> +	pdev = of_find_device_by_node(np);
> +	if (!pdev)
> +		return NULL;
> +
> +	return platform_get_drvdata(pdev);
> +}
> +
> +static struct device_node *arm_smmu_get_pci_dev_root(struct pci_dev *pdev)
>   {
> -	struct device_node *of_node;
> -	struct arm_smmu_device *curr, *smmu = NULL;
>   	struct pci_bus *bus = pdev->bus;
>
>   	/* Walk up to the root bus */
> @@ -1752,21 +1767,7 @@ static struct arm_smmu_device *arm_smmu_get_for_pci_dev(struct pci_dev *pdev)
>   		bus = bus->parent;
>
>   	/* Follow the "iommus" phandle from the host controller */

Either update comments to reflect what the new code does, or remove them 
along with the code they describe.

> -	of_node = of_parse_phandle(bus->bridge->parent->of_node, "iommus", 0);
> -	if (!of_node)
> -		return NULL;
> -
> -	/* See if we can find an SMMU corresponding to the phandle */
> -	spin_lock(&arm_smmu_devices_lock);
> -	list_for_each_entry(curr, &arm_smmu_devices, list) {
> -		if (curr->dev->of_node == of_node) {
> -			smmu = curr;
> -			break;
> -		}
> -	}
> -	spin_unlock(&arm_smmu_devices_lock);
> -	of_node_put(of_node);
> -	return smmu;
> +	return bus->bridge->parent->of_node;
>   }
>
>   static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
> @@ -1779,27 +1780,21 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
>   	return sid < limit;
>   }
>
> -static int arm_smmu_add_device(struct device *dev)
> +static int arm_smmu_add_device(struct device *dev, u32 sid)
>   {
>   	int i, ret;
> -	u32 sid, *sids;
> -	struct pci_dev *pdev;
> +	u32 *sids;
>   	struct iommu_group *group;
>   	struct arm_smmu_group *smmu_group;
>   	struct arm_smmu_device *smmu;
>
> -	/* We only support PCI, for now */
> -	if (!dev_is_pci(dev))
> -		return -ENODEV;
> -
> -	pdev = to_pci_dev(dev);
>   	group = iommu_group_get_for_dev(dev);
>   	if (IS_ERR(group))
>   		return PTR_ERR(group);
>
>   	smmu_group = iommu_group_get_iommudata(group);
>   	if (!smmu_group) {
> -		smmu = arm_smmu_get_for_pci_dev(pdev);
> +		smmu = dev->archdata.iommu;
>   		if (!smmu) {
>   			ret = -ENOENT;
>   			goto out_put_group;
> @@ -1819,8 +1814,6 @@ static int arm_smmu_add_device(struct device *dev)
>   		smmu = smmu_group->smmu;
>   	}
>
> -	/* Assume SID == RID until firmware tells us otherwise */
> -	pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid, &sid);
>   	for (i = 0; i < smmu_group->num_sids; ++i) {
>   		/* If we already know about this SID, then we're done */
>   		if (smmu_group->sids[i] == sid)
> @@ -1862,6 +1855,7 @@ out_put_group:
>
>   static void arm_smmu_remove_device(struct device *dev)
>   {
> +	dev->archdata.iommu = NULL;
>   	iommu_group_remove_device(dev);
>   }
>
> @@ -1909,7 +1903,68 @@ out_unlock:
>   	return ret;
>   }
>
> +static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
> +{
> +	int ret;
> +	struct arm_smmu_device *smmu;
> +
> +	/*
> +	 * We can sure that args->np is a smmu device node, because this
> +	 * function was called by of_xlate hook.
> +	 *
> +	 * And in arm_smmu_device_dt_probe:
> +	 *	platform_set_drvdata(pdev, smmu);
> +	 *	of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);
> +	 *
> +	 * It seems impossible return NULL in normal times.
> +	 */
> +	smmu = find_smmu_by_node(args->np);
> +	if (!smmu) {
> +		dev_err(dev, "unknown error caused smmu driver crashed\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!dev->archdata.iommu)
> +		dev->archdata.iommu = smmu;
> +
> +	if (dev->archdata.iommu != smmu) {
> +		dev_err(dev, "behinds more than one smmu\n");
> +		return -EINVAL;
> +	}
> +
> +	/* We only support PCI, for now */
> +	if (!dev_is_pci(dev)) {
> +		return -ENODEV;
> +	} else {
> +		u32 sid;
> +		struct device_node *of_root;
> +		struct pci_dev *pdev = NULL;
> +
> +		for_each_pci_dev(pdev) {

Given that we get here before the host controller's driver probe, is 
this really going to work? Either way, it looks very dodgy.

> +			of_root = arm_smmu_get_pci_dev_root(pdev);
> +			if (of_root != dev->of_node)
> +				continue;
> +
> +			/*
> +			 * Assume SID == RID until firmware tells us otherwise
> +			 */
> +			pci_for_each_dma_alias(pdev,
> +					__arm_smmu_get_pci_sid, &sid);
> +
> +			pdev->dev.archdata.iommu = smmu;
> +			ret = arm_smmu_add_device(dev, sid);
> +			if (ret) {
> +				dev_err(dev, "failed to add into SMMU\n");
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   static struct iommu_ops arm_smmu_ops = {
> +	.of_xlate		= arm_smmu_of_xlate,
>   	.capable		= arm_smmu_capable,
>   	.domain_alloc		= arm_smmu_domain_alloc,
>   	.domain_free		= arm_smmu_domain_free,
> @@ -1918,7 +1973,6 @@ static struct iommu_ops arm_smmu_ops = {
>   	.map			= arm_smmu_map,
>   	.unmap			= arm_smmu_unmap,
>   	.iova_to_phys		= arm_smmu_iova_to_phys,
> -	.add_device		= arm_smmu_add_device,

It might not be an immediate concern, but I think subverting the normal 
add_device process this way also completely breaks any kind of device 
hotplug.

>   	.remove_device		= arm_smmu_remove_device,
>   	.domain_get_attr	= arm_smmu_domain_get_attr,
>   	.domain_set_attr	= arm_smmu_domain_set_attr,
> @@ -2626,6 +2680,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>   	spin_lock(&arm_smmu_devices_lock);
>   	list_add(&smmu->list, &arm_smmu_devices);
>   	spin_unlock(&arm_smmu_devices_lock);
> +	platform_set_drvdata(pdev, smmu);
> +	of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);
> +
>   	return 0;
>
>   out_free_structures:
> @@ -2697,6 +2754,8 @@ static void __exit arm_smmu_exit(void)
>   subsys_initcall(arm_smmu_init);
>   module_exit(arm_smmu_exit);
>
> +IOMMU_OF_DECLARE(arm_smmu_v3, "arm,smmu-v3", NULL);
> +
>   MODULE_DESCRIPTION("IOMMU API for ARM architected SMMUv3 implementations");
>   MODULE_AUTHOR("Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>");
>   MODULE_LICENSE("GPL v2");
> --
> 1.8.0
>
>
> _______________________________________________
> iommu mailing list
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>

[PATCH v2 6/9] iommu/arm-smmu: to support probe deferral

[Robin Murphy]

On 07/07/15 04:30, Zhen Lei wrote:
> For pci devices, only the root nodes have "iommus" property. So we
> should traverse all of its sub nodes in of_xlate.

I don't really follow this description; only the host controller is 
described in DT - the devices behind it are probed dynamically and don't 
have nodes to traverse.

> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>   drivers/iommu/arm-smmu-v3.c | 119 +++++++++++++++++++++++++++++++++-----------
>   1 file changed, 89 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index d6e3494..c569539 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -30,6 +30,8 @@
>   #include <linux/of_address.h>
>   #include <linux/pci.h>
>   #include <linux/platform_device.h>
> +#include <linux/of_iommu.h>
> +#include <linux/of_platform.h>
>
>   #include "io-pgtable.h"
>
> @@ -1741,10 +1743,23 @@ static void __arm_smmu_release_pci_iommudata(void *data)
>   	kfree(data);
>   }
>
> -static struct arm_smmu_device *arm_smmu_get_for_pci_dev(struct pci_dev *pdev)
> +static struct arm_smmu_device *find_smmu_by_node(struct device_node *np)
> +{
> +	struct platform_device *pdev;
> +
> +	/* to ensure np is a smmu device node */
> +	if (!of_iommu_get_ops(np))
> +		return NULL;
> +
> +	pdev = of_find_device_by_node(np);
> +	if (!pdev)
> +		return NULL;
> +
> +	return platform_get_drvdata(pdev);
> +}
> +
> +static struct device_node *arm_smmu_get_pci_dev_root(struct pci_dev *pdev)
>   {
> -	struct device_node *of_node;
> -	struct arm_smmu_device *curr, *smmu = NULL;
>   	struct pci_bus *bus = pdev->bus;
>
>   	/* Walk up to the root bus */
> @@ -1752,21 +1767,7 @@ static struct arm_smmu_device *arm_smmu_get_for_pci_dev(struct pci_dev *pdev)
>   		bus = bus->parent;
>
>   	/* Follow the "iommus" phandle from the host controller */

Either update comments to reflect what the new code does, or remove them 
along with the code they describe.

> -	of_node = of_parse_phandle(bus->bridge->parent->of_node, "iommus", 0);
> -	if (!of_node)
> -		return NULL;
> -
> -	/* See if we can find an SMMU corresponding to the phandle */
> -	spin_lock(&arm_smmu_devices_lock);
> -	list_for_each_entry(curr, &arm_smmu_devices, list) {
> -		if (curr->dev->of_node == of_node) {
> -			smmu = curr;
> -			break;
> -		}
> -	}
> -	spin_unlock(&arm_smmu_devices_lock);
> -	of_node_put(of_node);
> -	return smmu;
> +	return bus->bridge->parent->of_node;
>   }
>
>   static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
> @@ -1779,27 +1780,21 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
>   	return sid < limit;
>   }
>
> -static int arm_smmu_add_device(struct device *dev)
> +static int arm_smmu_add_device(struct device *dev, u32 sid)
>   {
>   	int i, ret;
> -	u32 sid, *sids;
> -	struct pci_dev *pdev;
> +	u32 *sids;
>   	struct iommu_group *group;
>   	struct arm_smmu_group *smmu_group;
>   	struct arm_smmu_device *smmu;
>
> -	/* We only support PCI, for now */
> -	if (!dev_is_pci(dev))
> -		return -ENODEV;
> -
> -	pdev = to_pci_dev(dev);
>   	group = iommu_group_get_for_dev(dev);
>   	if (IS_ERR(group))
>   		return PTR_ERR(group);
>
>   	smmu_group = iommu_group_get_iommudata(group);
>   	if (!smmu_group) {
> -		smmu = arm_smmu_get_for_pci_dev(pdev);
> +		smmu = dev->archdata.iommu;
>   		if (!smmu) {
>   			ret = -ENOENT;
>   			goto out_put_group;
> @@ -1819,8 +1814,6 @@ static int arm_smmu_add_device(struct device *dev)
>   		smmu = smmu_group->smmu;
>   	}
>
> -	/* Assume SID == RID until firmware tells us otherwise */
> -	pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid, &sid);
>   	for (i = 0; i < smmu_group->num_sids; ++i) {
>   		/* If we already know about this SID, then we're done */
>   		if (smmu_group->sids[i] == sid)
> @@ -1862,6 +1855,7 @@ out_put_group:
>
>   static void arm_smmu_remove_device(struct device *dev)
>   {
> +	dev->archdata.iommu = NULL;
>   	iommu_group_remove_device(dev);
>   }
>
> @@ -1909,7 +1903,68 @@ out_unlock:
>   	return ret;
>   }
>
> +static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
> +{
> +	int ret;
> +	struct arm_smmu_device *smmu;
> +
> +	/*
> +	 * We can sure that args->np is a smmu device node, because this
> +	 * function was called by of_xlate hook.
> +	 *
> +	 * And in arm_smmu_device_dt_probe:
> +	 *	platform_set_drvdata(pdev, smmu);
> +	 *	of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);
> +	 *
> +	 * It seems impossible return NULL in normal times.
> +	 */
> +	smmu = find_smmu_by_node(args->np);
> +	if (!smmu) {
> +		dev_err(dev, "unknown error caused smmu driver crashed\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!dev->archdata.iommu)
> +		dev->archdata.iommu = smmu;
> +
> +	if (dev->archdata.iommu != smmu) {
> +		dev_err(dev, "behinds more than one smmu\n");
> +		return -EINVAL;
> +	}
> +
> +	/* We only support PCI, for now */
> +	if (!dev_is_pci(dev)) {
> +		return -ENODEV;
> +	} else {
> +		u32 sid;
> +		struct device_node *of_root;
> +		struct pci_dev *pdev = NULL;
> +
> +		for_each_pci_dev(pdev) {

Given that we get here before the host controller's driver probe, is 
this really going to work? Either way, it looks very dodgy.

> +			of_root = arm_smmu_get_pci_dev_root(pdev);
> +			if (of_root != dev->of_node)
> +				continue;
> +
> +			/*
> +			 * Assume SID == RID until firmware tells us otherwise
> +			 */
> +			pci_for_each_dma_alias(pdev,
> +					__arm_smmu_get_pci_sid, &sid);
> +
> +			pdev->dev.archdata.iommu = smmu;
> +			ret = arm_smmu_add_device(dev, sid);
> +			if (ret) {
> +				dev_err(dev, "failed to add into SMMU\n");
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   static struct iommu_ops arm_smmu_ops = {
> +	.of_xlate		= arm_smmu_of_xlate,
>   	.capable		= arm_smmu_capable,
>   	.domain_alloc		= arm_smmu_domain_alloc,
>   	.domain_free		= arm_smmu_domain_free,
> @@ -1918,7 +1973,6 @@ static struct iommu_ops arm_smmu_ops = {
>   	.map			= arm_smmu_map,
>   	.unmap			= arm_smmu_unmap,
>   	.iova_to_phys		= arm_smmu_iova_to_phys,
> -	.add_device		= arm_smmu_add_device,

It might not be an immediate concern, but I think subverting the normal 
add_device process this way also completely breaks any kind of device 
hotplug.

>   	.remove_device		= arm_smmu_remove_device,
>   	.domain_get_attr	= arm_smmu_domain_get_attr,
>   	.domain_set_attr	= arm_smmu_domain_set_attr,
> @@ -2626,6 +2680,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>   	spin_lock(&arm_smmu_devices_lock);
>   	list_add(&smmu->list, &arm_smmu_devices);
>   	spin_unlock(&arm_smmu_devices_lock);
> +	platform_set_drvdata(pdev, smmu);
> +	of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);
> +
>   	return 0;
>
>   out_free_structures:
> @@ -2697,6 +2754,8 @@ static void __exit arm_smmu_exit(void)
>   subsys_initcall(arm_smmu_init);
>   module_exit(arm_smmu_exit);
>
> +IOMMU_OF_DECLARE(arm_smmu_v3, "arm,smmu-v3", NULL);
> +
>   MODULE_DESCRIPTION("IOMMU API for ARM architected SMMUv3 implementations");
>   MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>");
>   MODULE_LICENSE("GPL v2");
> --
> 1.8.0
>
>
> _______________________________________________
> iommu mailing list
> iommu at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>

Re: [PATCH v2 7/9] iommu/arm-smmu: remove arm_smmu_devices

[Robin Murphy]

On 07/07/15 04:30, Zhen Lei wrote:
> It can be replaced by of_iommu_list(in of_iommu.c).
>
> Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>   drivers/iommu/arm-smmu-v3.c | 22 ++--------------------
>   1 file changed, 2 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index c569539..39c55f6 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -604,10 +604,6 @@ struct arm_smmu_domain {
>   	struct iommu_domain		domain;
>   };
>
> -/* Our list of SMMU instances */
> -static DEFINE_SPINLOCK(arm_smmu_devices_lock);
> -static LIST_HEAD(arm_smmu_devices);
> -
>   struct arm_smmu_option_prop {
>   	u32 opt;
>   	const char *prop;
> @@ -2675,11 +2671,6 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto out_free_structures;
>
> -	/* Record our private device structure */
> -	INIT_LIST_HEAD(&smmu->list);
> -	spin_lock(&arm_smmu_devices_lock);
> -	list_add(&smmu->list, &arm_smmu_devices);
> -	spin_unlock(&arm_smmu_devices_lock);
>   	platform_set_drvdata(pdev, smmu);
>   	of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);
>
> @@ -2692,19 +2683,10 @@ out_free_structures:
>
>   static int arm_smmu_device_remove(struct platform_device *pdev)
>   {
> -	struct arm_smmu_device *curr, *smmu = NULL;
> +	struct arm_smmu_device *smmu;
>   	struct device *dev = &pdev->dev;
>
> -	spin_lock(&arm_smmu_devices_lock);
> -	list_for_each_entry(curr, &arm_smmu_devices, list) {
> -		if (curr->dev == dev) {
> -			smmu = curr;
> -			list_del(&smmu->list);
> -			break;
> -		}
> -	}
> -	spin_unlock(&arm_smmu_devices_lock);
> -
> +	smmu = find_smmu_by_node(dev->of_node);

Isn't that just a really long round-trip to platform_get_drvdata(pdev)?

>   	if (!smmu)
>   		return -ENODEV;
>
> --
> 1.8.0
>
>
> _______________________________________________
> iommu mailing list
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>

[PATCH v2 7/9] iommu/arm-smmu: remove arm_smmu_devices

[Robin Murphy]

On 07/07/15 04:30, Zhen Lei wrote:
> It can be replaced by of_iommu_list(in of_iommu.c).
>
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>   drivers/iommu/arm-smmu-v3.c | 22 ++--------------------
>   1 file changed, 2 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index c569539..39c55f6 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -604,10 +604,6 @@ struct arm_smmu_domain {
>   	struct iommu_domain		domain;
>   };
>
> -/* Our list of SMMU instances */
> -static DEFINE_SPINLOCK(arm_smmu_devices_lock);
> -static LIST_HEAD(arm_smmu_devices);
> -
>   struct arm_smmu_option_prop {
>   	u32 opt;
>   	const char *prop;
> @@ -2675,11 +2671,6 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto out_free_structures;
>
> -	/* Record our private device structure */
> -	INIT_LIST_HEAD(&smmu->list);
> -	spin_lock(&arm_smmu_devices_lock);
> -	list_add(&smmu->list, &arm_smmu_devices);
> -	spin_unlock(&arm_smmu_devices_lock);
>   	platform_set_drvdata(pdev, smmu);
>   	of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);
>
> @@ -2692,19 +2683,10 @@ out_free_structures:
>
>   static int arm_smmu_device_remove(struct platform_device *pdev)
>   {
> -	struct arm_smmu_device *curr, *smmu = NULL;
> +	struct arm_smmu_device *smmu;
>   	struct device *dev = &pdev->dev;
>
> -	spin_lock(&arm_smmu_devices_lock);
> -	list_for_each_entry(curr, &arm_smmu_devices, list) {
> -		if (curr->dev == dev) {
> -			smmu = curr;
> -			list_del(&smmu->list);
> -			break;
> -		}
> -	}
> -	spin_unlock(&arm_smmu_devices_lock);
> -
> +	smmu = find_smmu_by_node(dev->of_node);

Isn't that just a really long round-trip to platform_get_drvdata(pdev)?

>   	if (!smmu)
>   		return -ENODEV;
>
> --
> 1.8.0
>
>
> _______________________________________________
> iommu mailing list
> iommu at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>

Re: [PATCH v2 9/9] iommu/arm-smmu: add support for non-pci devices

[Robin Murphy]

On 07/07/15 04:30, Zhen Lei wrote:
> This patch support a master with multiple stream IDs, but doesn't support a
> master behinds more than one SMMUs.

This should probably include a binding documentation update to make it 
clear what values of #iommu-cells the driver supports in what 
circumstances, and that the cells themselves should contain raw stream IDs.

> Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
>   drivers/iommu/arm-smmu-v3.c | 39 +++++++++++++++++++++++++++++++++++++--
>   1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 274d059..582cd1e 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -30,6 +30,7 @@
>   #include <linux/of_address.h>
>   #include <linux/pci.h>
>   #include <linux/platform_device.h>
> +#include <linux/amba/bus.h>
>   #include <linux/of_iommu.h>
>   #include <linux/of_platform.h>
>
> @@ -1928,9 +1929,35 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
>   		return -EINVAL;
>   	}
>
> -	/* We only support PCI, for now */
>   	if (!dev_is_pci(dev)) {
> -		return -ENODEV;
> +		int i;
> +		struct iommu_group *group;
> +
> +		group = iommu_group_get(dev);
> +		if (!group) {
> +			group = iommu_group_alloc();
> +			if (IS_ERR(group)) {
> +				dev_err(dev, "failed to allocate IOMMU group\n");
> +				return PTR_ERR(group);
> +			}
> +
> +			ret = iommu_group_add_device(group, dev);
> +			if (ret) {
> +				dev_err(dev, "failed to add device into IOMMU group\n");
> +				return PTR_ERR(group);

This leaks the group.

> +			}
> +		}
> +		iommu_group_put(group);
> +
> +		for (i = 0; i < args->args_count; i++) {

I'm dubious of the value of looping here - having n>1 #iommu-cells per 
phandle means that every platform device behind one SMMU must have the 
same number of stream IDs, or you still have to have repeated phandles 
for every device with some greater multiple of n stream IDs each, but 
ruling out any device with <n. I'm not sure how realistic that is, and 
whether there's any real benefit beyond saving a handful of bytes in the 
DTB.

> +			ret = arm_smmu_add_device(dev, args->args[i]);
> +			if (ret) {
> +				dev_err(dev,
> +					"failed to add sid=%d into SMMU\n",
> +					args->args[i]);
> +				return ret;
> +			}
> +		}
>   	} else {
>   		u32 sid;
>   		struct device_node *of_root;
> @@ -2725,6 +2752,14 @@ static int __init arm_smmu_init(void)
>   	if (ret)
>   		return ret;
>
> +	if (!iommu_present(&platform_bus_type))
> +		bus_set_iommu(&platform_bus_type, &arm_smmu_ops);
> +
> +#ifdef CONFIG_ARM_AMBA
> +	if (!iommu_present(&amba_bustype))
> +		bus_set_iommu(&amba_bustype, &arm_smmu_ops);
> +#endif
> +
>   	return bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
>   }
>
> --
> 1.8.0
>
>
> _______________________________________________
> iommu mailing list
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>

[PATCH v2 9/9] iommu/arm-smmu: add support for non-pci devices

[Robin Murphy]

On 07/07/15 04:30, Zhen Lei wrote:
> This patch support a master with multiple stream IDs, but doesn't support a
> master behinds more than one SMMUs.

This should probably include a binding documentation update to make it 
clear what values of #iommu-cells the driver supports in what 
circumstances, and that the cells themselves should contain raw stream IDs.

> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>   drivers/iommu/arm-smmu-v3.c | 39 +++++++++++++++++++++++++++++++++++++--
>   1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 274d059..582cd1e 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -30,6 +30,7 @@
>   #include <linux/of_address.h>
>   #include <linux/pci.h>
>   #include <linux/platform_device.h>
> +#include <linux/amba/bus.h>
>   #include <linux/of_iommu.h>
>   #include <linux/of_platform.h>
>
> @@ -1928,9 +1929,35 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
>   		return -EINVAL;
>   	}
>
> -	/* We only support PCI, for now */
>   	if (!dev_is_pci(dev)) {
> -		return -ENODEV;
> +		int i;
> +		struct iommu_group *group;
> +
> +		group = iommu_group_get(dev);
> +		if (!group) {
> +			group = iommu_group_alloc();
> +			if (IS_ERR(group)) {
> +				dev_err(dev, "failed to allocate IOMMU group\n");
> +				return PTR_ERR(group);
> +			}
> +
> +			ret = iommu_group_add_device(group, dev);
> +			if (ret) {
> +				dev_err(dev, "failed to add device into IOMMU group\n");
> +				return PTR_ERR(group);

This leaks the group.

> +			}
> +		}
> +		iommu_group_put(group);
> +
> +		for (i = 0; i < args->args_count; i++) {

I'm dubious of the value of looping here - having n>1 #iommu-cells per 
phandle means that every platform device behind one SMMU must have the 
same number of stream IDs, or you still have to have repeated phandles 
for every device with some greater multiple of n stream IDs each, but 
ruling out any device with <n. I'm not sure how realistic that is, and 
whether there's any real benefit beyond saving a handful of bytes in the 
DTB.

> +			ret = arm_smmu_add_device(dev, args->args[i]);
> +			if (ret) {
> +				dev_err(dev,
> +					"failed to add sid=%d into SMMU\n",
> +					args->args[i]);
> +				return ret;
> +			}
> +		}
>   	} else {
>   		u32 sid;
>   		struct device_node *of_root;
> @@ -2725,6 +2752,14 @@ static int __init arm_smmu_init(void)
>   	if (ret)
>   		return ret;
>
> +	if (!iommu_present(&platform_bus_type))
> +		bus_set_iommu(&platform_bus_type, &arm_smmu_ops);
> +
> +#ifdef CONFIG_ARM_AMBA
> +	if (!iommu_present(&amba_bustype))
> +		bus_set_iommu(&amba_bustype, &arm_smmu_ops);
> +#endif
> +
>   	return bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
>   }
>
> --
> 1.8.0
>
>
> _______________________________________________
> iommu mailing list
> iommu at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>

Re: [PATCH v2 5/9] iommu/arm-smmu: skip the execution of CMD_PREFETCH_CONFIG

[Will Deacon]

Hi Robin,

FWIW, I already queued this ;)

On Wed, Jul 08, 2015 at 02:03:13PM +0100, Robin Murphy wrote:
> On 07/07/15 04:30, Zhen Lei wrote:
> > Hisilicon SMMUv3 devices treat CMD_PREFETCH_CONFIG as a illegal command,
> > execute it will trigger GERROR interrupt. Although the gerror code manage
> > to turn the prefetch into a SYNC, and the system can continue to run
> > normally, but it's ugly to print error information.
> 
> No mention of the DT binding change, and no corresponding documentation 
> update either.

I've added that.

> > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> > ---
> [...]
> > +static struct arm_smmu_option_prop arm_smmu_options[] = {
> > +	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
> > +	{ 0, NULL},
> > +};
> [...]
> > +static void parse_driver_options(struct arm_smmu_device *smmu)
> > +{
> > +	int i = 0;
> > +
> > +	do {
> > +		if (of_property_read_bool(smmu->dev->of_node,
> > +						arm_smmu_options[i].prop)) {
> > +			smmu->options |= arm_smmu_options[i].opt;
> > +			dev_notice(smmu->dev, "option %s\n",
> > +				arm_smmu_options[i].prop);
> > +		}
> > +	} while (arm_smmu_options[++i].opt);
> > +}
> > +
> 
> Nitpicking for sure, but I'm still waiting for a good excuse to rewrite 
> this overcomplicated loop logic in the SMMUv2 driver - can't we just 
> treat a static array as a static array and iterate over the thing in the 
> obvious way?
> 
> 	for (i = 0; i < ARRAY_SIZE(arm_smmu_options); i++)

I'd rather have consistency with the other driver and I really have no
personal preference about how we iterate over an array. If you have a
technical reason to change both the drivers, please send a patch.

Will

[PATCH v2 5/9] iommu/arm-smmu: skip the execution of CMD_PREFETCH_CONFIG

[Will Deacon]

Hi Robin,

FWIW, I already queued this ;)

On Wed, Jul 08, 2015 at 02:03:13PM +0100, Robin Murphy wrote:
> On 07/07/15 04:30, Zhen Lei wrote:
> > Hisilicon SMMUv3 devices treat CMD_PREFETCH_CONFIG as a illegal command,
> > execute it will trigger GERROR interrupt. Although the gerror code manage
> > to turn the prefetch into a SYNC, and the system can continue to run
> > normally, but it's ugly to print error information.
> 
> No mention of the DT binding change, and no corresponding documentation 
> update either.

I've added that.

> > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> > ---
> [...]
> > +static struct arm_smmu_option_prop arm_smmu_options[] = {
> > +	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
> > +	{ 0, NULL},
> > +};
> [...]
> > +static void parse_driver_options(struct arm_smmu_device *smmu)
> > +{
> > +	int i = 0;
> > +
> > +	do {
> > +		if (of_property_read_bool(smmu->dev->of_node,
> > +						arm_smmu_options[i].prop)) {
> > +			smmu->options |= arm_smmu_options[i].opt;
> > +			dev_notice(smmu->dev, "option %s\n",
> > +				arm_smmu_options[i].prop);
> > +		}
> > +	} while (arm_smmu_options[++i].opt);
> > +}
> > +
> 
> Nitpicking for sure, but I'm still waiting for a good excuse to rewrite 
> this overcomplicated loop logic in the SMMUv2 driver - can't we just 
> treat a static array as a static array and iterate over the thing in the 
> obvious way?
> 
> 	for (i = 0; i < ARRAY_SIZE(arm_smmu_options); i++)

I'd rather have consistency with the other driver and I really have no
personal preference about how we iterate over an array. If you have a
technical reason to change both the drivers, please send a patch.

Will

Re: [PATCH v2 5/9] iommu/arm-smmu: skip the execution of CMD_PREFETCH_CONFIG

[leizhen]

On 2015/7/9 1:11, Will Deacon wrote:
> Hi Robin,
> 
> FWIW, I already queued this ;)
> 
> On Wed, Jul 08, 2015 at 02:03:13PM +0100, Robin Murphy wrote:
>> On 07/07/15 04:30, Zhen Lei wrote:
>>> Hisilicon SMMUv3 devices treat CMD_PREFETCH_CONFIG as a illegal command,
>>> execute it will trigger GERROR interrupt. Although the gerror code manage
>>> to turn the prefetch into a SYNC, and the system can continue to run
>>> normally, but it's ugly to print error information.
>>
>> No mention of the DT binding change, and no corresponding documentation 
>> update either.
> 
> I've added that.

Thank you very much. You are a real gentleman.

> 
>>> Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>> ---
>> [...]
>>> +static struct arm_smmu_option_prop arm_smmu_options[] = {
>>> +	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
>>> +	{ 0, NULL},
>>> +};
>> [...]
>>> +static void parse_driver_options(struct arm_smmu_device *smmu)
>>> +{
>>> +	int i = 0;
>>> +
>>> +	do {
>>> +		if (of_property_read_bool(smmu->dev->of_node,
>>> +						arm_smmu_options[i].prop)) {
>>> +			smmu->options |= arm_smmu_options[i].opt;
>>> +			dev_notice(smmu->dev, "option %s\n",
>>> +				arm_smmu_options[i].prop);
>>> +		}
>>> +	} while (arm_smmu_options[++i].opt);
>>> +}
>>> +
>>
>> Nitpicking for sure, but I'm still waiting for a good excuse to rewrite 
>> this overcomplicated loop logic in the SMMUv2 driver - can't we just 
>> treat a static array as a static array and iterate over the thing in the 
>> obvious way?
>>
>> 	for (i = 0; i < ARRAY_SIZE(arm_smmu_options); i++)
> 
> I'd rather have consistency with the other driver and I really have no
> personal preference about how we iterate over an array. If you have a
> technical reason to change both the drivers, please send a patch.
> 
> Will
> 
> .
> 

[PATCH v2 5/9] iommu/arm-smmu: skip the execution of CMD_PREFETCH_CONFIG

[leizhen]

On 2015/7/9 1:11, Will Deacon wrote:
> Hi Robin,
> 
> FWIW, I already queued this ;)
> 
> On Wed, Jul 08, 2015 at 02:03:13PM +0100, Robin Murphy wrote:
>> On 07/07/15 04:30, Zhen Lei wrote:
>>> Hisilicon SMMUv3 devices treat CMD_PREFETCH_CONFIG as a illegal command,
>>> execute it will trigger GERROR interrupt. Although the gerror code manage
>>> to turn the prefetch into a SYNC, and the system can continue to run
>>> normally, but it's ugly to print error information.
>>
>> No mention of the DT binding change, and no corresponding documentation 
>> update either.
> 
> I've added that.

Thank you very much. You are a real gentleman.

> 
>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>> ---
>> [...]
>>> +static struct arm_smmu_option_prop arm_smmu_options[] = {
>>> +	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
>>> +	{ 0, NULL},
>>> +};
>> [...]
>>> +static void parse_driver_options(struct arm_smmu_device *smmu)
>>> +{
>>> +	int i = 0;
>>> +
>>> +	do {
>>> +		if (of_property_read_bool(smmu->dev->of_node,
>>> +						arm_smmu_options[i].prop)) {
>>> +			smmu->options |= arm_smmu_options[i].opt;
>>> +			dev_notice(smmu->dev, "option %s\n",
>>> +				arm_smmu_options[i].prop);
>>> +		}
>>> +	} while (arm_smmu_options[++i].opt);
>>> +}
>>> +
>>
>> Nitpicking for sure, but I'm still waiting for a good excuse to rewrite 
>> this overcomplicated loop logic in the SMMUv2 driver - can't we just 
>> treat a static array as a static array and iterate over the thing in the 
>> obvious way?
>>
>> 	for (i = 0; i < ARRAY_SIZE(arm_smmu_options); i++)
> 
> I'd rather have consistency with the other driver and I really have no
> personal preference about how we iterate over an array. If you have a
> technical reason to change both the drivers, please send a patch.
> 
> Will
> 
> .
> 

Re: [PATCH v2 9/9] iommu/arm-smmu: add support for non-pci devices

[leizhen]

On 2015/7/8 21:22, Robin Murphy wrote:
> On 07/07/15 04:30, Zhen Lei wrote:
>> This patch support a master with multiple stream IDs, but doesn't support a
>> master behinds more than one SMMUs.
> 
> This should probably include a binding documentation update to make it clear what values of #iommu-cells the driver supports in what circumstances, and that the cells themselves should contain raw
> stream IDs.

OK.

> 
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>   drivers/iommu/arm-smmu-v3.c | 39 +++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 274d059..582cd1e 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -30,6 +30,7 @@
>>   #include <linux/of_address.h>
>>   #include <linux/pci.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/amba/bus.h>
>>   #include <linux/of_iommu.h>
>>   #include <linux/of_platform.h>
>>
>> @@ -1928,9 +1929,35 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
>>           return -EINVAL;
>>       }
>>
>> -    /* We only support PCI, for now */
>>       if (!dev_is_pci(dev)) {
>> -        return -ENODEV;
>> +        int i;
>> +        struct iommu_group *group;
>> +
>> +        group = iommu_group_get(dev);
>> +        if (!group) {
>> +            group = iommu_group_alloc();
>> +            if (IS_ERR(group)) {
>> +                dev_err(dev, "failed to allocate IOMMU group\n");
>> +                return PTR_ERR(group);
>> +            }
>> +
>> +            ret = iommu_group_add_device(group, dev);
>> +            if (ret) {
>> +                dev_err(dev, "failed to add device into IOMMU group\n");
>> +                return PTR_ERR(group);
> 
> This leaks the group.

OK, I got it. Thanks.

> 
>> +            }
>> +        }
>> +        iommu_group_put(group);
>> +
>> +        for (i = 0; i < args->args_count; i++) {
> 
> I'm dubious of the value of looping here - having n>1 #iommu-cells per phandle means that every platform device behind one SMMU must have the same number of stream IDs, or you still have to have
> repeated phandles for every device with some greater multiple of n stream IDs each, but ruling out any device with <n. I'm not sure how realistic that is, and whether there's any real benefit beyond
> saving a handful of bytes in the DTB.

As you mentioned before, a master with two streamIDs can be written as below(This is also mentioned in Documentation\devicetree\bindings\iommu\iommu.txt):
#iommu-cells = <1>;
iommus = <&{/iommu} 23>, <&{/iommu} 24>;

On my hardware platform, a master only have one streamID. But I tested two cases:
#iommu-cells = <1>;
1. iommus = <&smmu0 good-sid>, <&smmu0 bad-sid>;
2. iommus = <&smmu0 bad-sid>, <&smmu0 good-sid>;
All of these two cases worked well.

Well, if each master contains two streamIDs(or more but equal), this for loop is needed.

> 
>> +            ret = arm_smmu_add_device(dev, args->args[i]);
>> +            if (ret) {
>> +                dev_err(dev,
>> +                    "failed to add sid=%d into SMMU\n",
>> +                    args->args[i]);
>> +                return ret;
>> +            }
>> +        }
>>       } else {
>>           u32 sid;
>>           struct device_node *of_root;
>> @@ -2725,6 +2752,14 @@ static int __init arm_smmu_init(void)
>>       if (ret)
>>           return ret;
>>
>> +    if (!iommu_present(&platform_bus_type))
>> +        bus_set_iommu(&platform_bus_type, &arm_smmu_ops);
>> +
>> +#ifdef CONFIG_ARM_AMBA
>> +    if (!iommu_present(&amba_bustype))
>> +        bus_set_iommu(&amba_bustype, &arm_smmu_ops);
>> +#endif
>> +
>>       return bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
>>   }
>>
>> -- 
>> 1.8.0
>>
>>
>> _______________________________________________
>> iommu mailing list
>> iommu@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
> 
> 
> .
> 

[PATCH v2 9/9] iommu/arm-smmu: add support for non-pci devices

[leizhen]

On 2015/7/8 21:22, Robin Murphy wrote:
> On 07/07/15 04:30, Zhen Lei wrote:
>> This patch support a master with multiple stream IDs, but doesn't support a
>> master behinds more than one SMMUs.
> 
> This should probably include a binding documentation update to make it clear what values of #iommu-cells the driver supports in what circumstances, and that the cells themselves should contain raw
> stream IDs.

OK.

> 
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>   drivers/iommu/arm-smmu-v3.c | 39 +++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 274d059..582cd1e 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -30,6 +30,7 @@
>>   #include <linux/of_address.h>
>>   #include <linux/pci.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/amba/bus.h>
>>   #include <linux/of_iommu.h>
>>   #include <linux/of_platform.h>
>>
>> @@ -1928,9 +1929,35 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
>>           return -EINVAL;
>>       }
>>
>> -    /* We only support PCI, for now */
>>       if (!dev_is_pci(dev)) {
>> -        return -ENODEV;
>> +        int i;
>> +        struct iommu_group *group;
>> +
>> +        group = iommu_group_get(dev);
>> +        if (!group) {
>> +            group = iommu_group_alloc();
>> +            if (IS_ERR(group)) {
>> +                dev_err(dev, "failed to allocate IOMMU group\n");
>> +                return PTR_ERR(group);
>> +            }
>> +
>> +            ret = iommu_group_add_device(group, dev);
>> +            if (ret) {
>> +                dev_err(dev, "failed to add device into IOMMU group\n");
>> +                return PTR_ERR(group);
> 
> This leaks the group.

OK, I got it. Thanks.

> 
>> +            }
>> +        }
>> +        iommu_group_put(group);
>> +
>> +        for (i = 0; i < args->args_count; i++) {
> 
> I'm dubious of the value of looping here - having n>1 #iommu-cells per phandle means that every platform device behind one SMMU must have the same number of stream IDs, or you still have to have
> repeated phandles for every device with some greater multiple of n stream IDs each, but ruling out any device with <n. I'm not sure how realistic that is, and whether there's any real benefit beyond
> saving a handful of bytes in the DTB.

As you mentioned before, a master with two streamIDs can be written as below(This is also mentioned in Documentation\devicetree\bindings\iommu\iommu.txt):
#iommu-cells = <1>;
iommus = <&{/iommu} 23>, <&{/iommu} 24>;

On my hardware platform, a master only have one streamID. But I tested two cases:
#iommu-cells = <1>;
1. iommus = <&smmu0 good-sid>, <&smmu0 bad-sid>;
2. iommus = <&smmu0 bad-sid>, <&smmu0 good-sid>;
All of these two cases worked well.

Well, if each master contains two streamIDs(or more but equal), this for loop is needed.

> 
>> +            ret = arm_smmu_add_device(dev, args->args[i]);
>> +            if (ret) {
>> +                dev_err(dev,
>> +                    "failed to add sid=%d into SMMU\n",
>> +                    args->args[i]);
>> +                return ret;
>> +            }
>> +        }
>>       } else {
>>           u32 sid;
>>           struct device_node *of_root;
>> @@ -2725,6 +2752,14 @@ static int __init arm_smmu_init(void)
>>       if (ret)
>>           return ret;
>>
>> +    if (!iommu_present(&platform_bus_type))
>> +        bus_set_iommu(&platform_bus_type, &arm_smmu_ops);
>> +
>> +#ifdef CONFIG_ARM_AMBA
>> +    if (!iommu_present(&amba_bustype))
>> +        bus_set_iommu(&amba_bustype, &arm_smmu_ops);
>> +#endif
>> +
>>       return bus_set_iommu(&pci_bus_type, &arm_smmu_ops);
>>   }
>>
>> -- 
>> 1.8.0
>>
>>
>> _______________________________________________
>> iommu mailing list
>> iommu at lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
> 
> 
> .
> 

Re: [PATCH v2 7/9] iommu/arm-smmu: remove arm_smmu_devices

[leizhen]

On 2015/7/8 21:13, Robin Murphy wrote:
> On 07/07/15 04:30, Zhen Lei wrote:
>> It can be replaced by of_iommu_list(in of_iommu.c).
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>>   drivers/iommu/arm-smmu-v3.c | 22 ++--------------------
>>   1 file changed, 2 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index c569539..39c55f6 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -604,10 +604,6 @@ struct arm_smmu_domain {
>>       struct iommu_domain        domain;
>>   };
>>
>> -/* Our list of SMMU instances */
>> -static DEFINE_SPINLOCK(arm_smmu_devices_lock);
>> -static LIST_HEAD(arm_smmu_devices);
>> -
>>   struct arm_smmu_option_prop {
>>       u32 opt;
>>       const char *prop;
>> @@ -2675,11 +2671,6 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>>       if (ret)
>>           goto out_free_structures;
>>
>> -    /* Record our private device structure */
>> -    INIT_LIST_HEAD(&smmu->list);
>> -    spin_lock(&arm_smmu_devices_lock);
>> -    list_add(&smmu->list, &arm_smmu_devices);
>> -    spin_unlock(&arm_smmu_devices_lock);
>>       platform_set_drvdata(pdev, smmu);
>>       of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);
>>
>> @@ -2692,19 +2683,10 @@ out_free_structures:
>>
>>   static int arm_smmu_device_remove(struct platform_device *pdev)
>>   {
>> -    struct arm_smmu_device *curr, *smmu = NULL;
>> +    struct arm_smmu_device *smmu;
>>       struct device *dev = &pdev->dev;
>>
>> -    spin_lock(&arm_smmu_devices_lock);
>> -    list_for_each_entry(curr, &arm_smmu_devices, list) {
>> -        if (curr->dev == dev) {
>> -            smmu = curr;
>> -            list_del(&smmu->list);
>> -            break;
>> -        }
>> -    }
>> -    spin_unlock(&arm_smmu_devices_lock);
>> -
>> +    smmu = find_smmu_by_node(dev->of_node);
> 
> Isn't that just a really long round-trip to platform_get_drvdata(pdev)?

But we should first check that pdev is a real smmu device, not others. So we can not omit:
if (!of_iommu_get_ops(np))

I think arm_smmu_device_remove will rarely be called, so it will not impact performance.

> 
>>       if (!smmu)
>>           return -ENODEV;
>>
>> -- 
>> 1.8.0
>>
>>
>> _______________________________________________
>> iommu mailing list
>> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
> 
> 
> .
> 

[PATCH v2 7/9] iommu/arm-smmu: remove arm_smmu_devices

[leizhen]

On 2015/7/8 21:13, Robin Murphy wrote:
> On 07/07/15 04:30, Zhen Lei wrote:
>> It can be replaced by of_iommu_list(in of_iommu.c).
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>   drivers/iommu/arm-smmu-v3.c | 22 ++--------------------
>>   1 file changed, 2 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index c569539..39c55f6 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -604,10 +604,6 @@ struct arm_smmu_domain {
>>       struct iommu_domain        domain;
>>   };
>>
>> -/* Our list of SMMU instances */
>> -static DEFINE_SPINLOCK(arm_smmu_devices_lock);
>> -static LIST_HEAD(arm_smmu_devices);
>> -
>>   struct arm_smmu_option_prop {
>>       u32 opt;
>>       const char *prop;
>> @@ -2675,11 +2671,6 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>>       if (ret)
>>           goto out_free_structures;
>>
>> -    /* Record our private device structure */
>> -    INIT_LIST_HEAD(&smmu->list);
>> -    spin_lock(&arm_smmu_devices_lock);
>> -    list_add(&smmu->list, &arm_smmu_devices);
>> -    spin_unlock(&arm_smmu_devices_lock);
>>       platform_set_drvdata(pdev, smmu);
>>       of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);
>>
>> @@ -2692,19 +2683,10 @@ out_free_structures:
>>
>>   static int arm_smmu_device_remove(struct platform_device *pdev)
>>   {
>> -    struct arm_smmu_device *curr, *smmu = NULL;
>> +    struct arm_smmu_device *smmu;
>>       struct device *dev = &pdev->dev;
>>
>> -    spin_lock(&arm_smmu_devices_lock);
>> -    list_for_each_entry(curr, &arm_smmu_devices, list) {
>> -        if (curr->dev == dev) {
>> -            smmu = curr;
>> -            list_del(&smmu->list);
>> -            break;
>> -        }
>> -    }
>> -    spin_unlock(&arm_smmu_devices_lock);
>> -
>> +    smmu = find_smmu_by_node(dev->of_node);
> 
> Isn't that just a really long round-trip to platform_get_drvdata(pdev)?

But we should first check that pdev is a real smmu device, not others. So we can not omit:
if (!of_iommu_get_ops(np))

I think arm_smmu_device_remove will rarely be called, so it will not impact performance.

> 
>>       if (!smmu)
>>           return -ENODEV;
>>
>> -- 
>> 1.8.0
>>
>>
>> _______________________________________________
>> iommu mailing list
>> iommu at lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
> 
> 
> .
> 

Re: [PATCH v2 6/9] iommu/arm-smmu: to support probe deferral

[leizhen]

On 2015/7/8 21:13, Robin Murphy wrote:
> On 07/07/15 04:30, Zhen Lei wrote:
>> For pci devices, only the root nodes have "iommus" property. So we
>> should traverse all of its sub nodes in of_xlate.
> 
> I don't really follow this description; only the host controller is described in DT - the devices behind it are probed dynamically and don't have nodes to traverse.

The devices behind host controller may have nodes, but have no "iommus" property.

I got this conclusion base on the original code as below:

	struct pci_bus *bus = pdev->bus;

	/* Walk up to the root bus */
	while (!pci_is_root_bus(bus))
		bus = bus->parent;

	/* Follow the "iommus" phandle from the host controller */
	of_node = of_parse_phandle(bus->bridge->parent->of_node, "iommus", 0);
	if (!of_node)
		return NULL;

> 
>> Signed-off-by: Zhen Lei <thunder.leizhen-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> ---
>>   drivers/iommu/arm-smmu-v3.c | 119 +++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 89 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index d6e3494..c569539 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -30,6 +30,8 @@
>>   #include <linux/of_address.h>
>>   #include <linux/pci.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/of_iommu.h>
>> +#include <linux/of_platform.h>
>>
>>   #include "io-pgtable.h"
>>
>> @@ -1741,10 +1743,23 @@ static void __arm_smmu_release_pci_iommudata(void *data)
>>       kfree(data);
>>   }
>>
>> -static struct arm_smmu_device *arm_smmu_get_for_pci_dev(struct pci_dev *pdev)
>> +static struct arm_smmu_device *find_smmu_by_node(struct device_node *np)
>> +{
>> +    struct platform_device *pdev;
>> +
>> +    /* to ensure np is a smmu device node */
>> +    if (!of_iommu_get_ops(np))
>> +        return NULL;
>> +
>> +    pdev = of_find_device_by_node(np);
>> +    if (!pdev)
>> +        return NULL;
>> +
>> +    return platform_get_drvdata(pdev);
>> +}
>> +
>> +static struct device_node *arm_smmu_get_pci_dev_root(struct pci_dev *pdev)
>>   {
>> -    struct device_node *of_node;
>> -    struct arm_smmu_device *curr, *smmu = NULL;
>>       struct pci_bus *bus = pdev->bus;
>>
>>       /* Walk up to the root bus */
>> @@ -1752,21 +1767,7 @@ static struct arm_smmu_device *arm_smmu_get_for_pci_dev(struct pci_dev *pdev)
>>           bus = bus->parent;
>>
>>       /* Follow the "iommus" phandle from the host controller */
> 
> Either update comments to reflect what the new code does, or remove them along with the code they describe.

OK, I will remove it, thanks.

> 
>> -    of_node = of_parse_phandle(bus->bridge->parent->of_node, "iommus", 0);
>> -    if (!of_node)
>> -        return NULL;
>> -
>> -    /* See if we can find an SMMU corresponding to the phandle */
>> -    spin_lock(&arm_smmu_devices_lock);
>> -    list_for_each_entry(curr, &arm_smmu_devices, list) {
>> -        if (curr->dev->of_node == of_node) {
>> -            smmu = curr;
>> -            break;
>> -        }
>> -    }
>> -    spin_unlock(&arm_smmu_devices_lock);
>> -    of_node_put(of_node);
>> -    return smmu;
>> +    return bus->bridge->parent->of_node;
>>   }
>>
>>   static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
>> @@ -1779,27 +1780,21 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
>>       return sid < limit;
>>   }
>>
>> -static int arm_smmu_add_device(struct device *dev)
>> +static int arm_smmu_add_device(struct device *dev, u32 sid)
>>   {
>>       int i, ret;
>> -    u32 sid, *sids;
>> -    struct pci_dev *pdev;
>> +    u32 *sids;
>>       struct iommu_group *group;
>>       struct arm_smmu_group *smmu_group;
>>       struct arm_smmu_device *smmu;
>>
>> -    /* We only support PCI, for now */
>> -    if (!dev_is_pci(dev))
>> -        return -ENODEV;
>> -
>> -    pdev = to_pci_dev(dev);
>>       group = iommu_group_get_for_dev(dev);
>>       if (IS_ERR(group))
>>           return PTR_ERR(group);
>>
>>       smmu_group = iommu_group_get_iommudata(group);
>>       if (!smmu_group) {
>> -        smmu = arm_smmu_get_for_pci_dev(pdev);
>> +        smmu = dev->archdata.iommu;
>>           if (!smmu) {
>>               ret = -ENOENT;
>>               goto out_put_group;
>> @@ -1819,8 +1814,6 @@ static int arm_smmu_add_device(struct device *dev)
>>           smmu = smmu_group->smmu;
>>       }
>>
>> -    /* Assume SID == RID until firmware tells us otherwise */
>> -    pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid, &sid);
>>       for (i = 0; i < smmu_group->num_sids; ++i) {
>>           /* If we already know about this SID, then we're done */
>>           if (smmu_group->sids[i] == sid)
>> @@ -1862,6 +1855,7 @@ out_put_group:
>>
>>   static void arm_smmu_remove_device(struct device *dev)
>>   {
>> +    dev->archdata.iommu = NULL;
>>       iommu_group_remove_device(dev);
>>   }
>>
>> @@ -1909,7 +1903,68 @@ out_unlock:
>>       return ret;
>>   }
>>
>> +static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
>> +{
>> +    int ret;
>> +    struct arm_smmu_device *smmu;
>> +
>> +    /*
>> +     * We can sure that args->np is a smmu device node, because this
>> +     * function was called by of_xlate hook.
>> +     *
>> +     * And in arm_smmu_device_dt_probe:
>> +     *    platform_set_drvdata(pdev, smmu);
>> +     *    of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);
>> +     *
>> +     * It seems impossible return NULL in normal times.
>> +     */
>> +    smmu = find_smmu_by_node(args->np);
>> +    if (!smmu) {
>> +        dev_err(dev, "unknown error caused smmu driver crashed\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    if (!dev->archdata.iommu)
>> +        dev->archdata.iommu = smmu;
>> +
>> +    if (dev->archdata.iommu != smmu) {
>> +        dev_err(dev, "behinds more than one smmu\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    /* We only support PCI, for now */
>> +    if (!dev_is_pci(dev)) {
>> +        return -ENODEV;
>> +    } else {
>> +        u32 sid;
>> +        struct device_node *of_root;
>> +        struct pci_dev *pdev = NULL;
>> +
>> +        for_each_pci_dev(pdev) {
> 
> Given that we get here before the host controller's driver probe, is this really going to work? Either way, it looks very dodgy.

It will be a problem. See the next answer, I think it will be resolved. Based on the following reasoning:
1. if .add_device happened before .of_xlate, then this version have no problem. Because the old version worked well, so arm_smmu_get_pci_dev_root will work well too, we can base on current method find
all sub nodes(which should be processed in .add_device for the old version) of the host controller.

2. if .add_device happened after.of_xlate, because the function of the new .add_device is the same to the old .add_device, so it will work well as the old version.

I will send patch v3.

> 
>> +            of_root = arm_smmu_get_pci_dev_root(pdev);
>> +            if (of_root != dev->of_node)
>> +                continue;
>> +
>> +            /*
>> +             * Assume SID == RID until firmware tells us otherwise
>> +             */
>> +            pci_for_each_dma_alias(pdev,
>> +                    __arm_smmu_get_pci_sid, &sid);
>> +
>> +            pdev->dev.archdata.iommu = smmu;
>> +            ret = arm_smmu_add_device(dev, sid);
>> +            if (ret) {
>> +                dev_err(dev, "failed to add into SMMU\n");
>> +                return ret;
>> +            }
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static struct iommu_ops arm_smmu_ops = {
>> +    .of_xlate        = arm_smmu_of_xlate,
>>       .capable        = arm_smmu_capable,
>>       .domain_alloc        = arm_smmu_domain_alloc,
>>       .domain_free        = arm_smmu_domain_free,
>> @@ -1918,7 +1973,6 @@ static struct iommu_ops arm_smmu_ops = {
>>       .map            = arm_smmu_map,
>>       .unmap            = arm_smmu_unmap,
>>       .iova_to_phys        = arm_smmu_iova_to_phys,
>> -    .add_device        = arm_smmu_add_device,
> 
> It might not be an immediate concern, but I think subverting the normal add_device process this way also completely breaks any kind of device hotplug.

Yes, I had aslo worried about it.

I think we can resovle it like below:
.add_device        = arm_smmu_add_device

arm_smmu_add_device:
	/* We only support pci device hotplug */
	if (!dev_is_pci(dev))
		return -ENODEV;

	pdev = to_pci_dev(dev);
	root = arm_smmu_get_pci_dev_root(pdev);		//arm_smmu_get_pci_dev_root return value should be modified that: return bus->bridge->parent;
	if (!root.archdata.iommu)			//should add "root.archdata.iommu = smmu" in of_xlate
		return -ENODEV;

	//add this pci device to smmu

> 
>>       .remove_device        = arm_smmu_remove_device,
>>       .domain_get_attr    = arm_smmu_domain_get_attr,
>>       .domain_set_attr    = arm_smmu_domain_set_attr,
>> @@ -2626,6 +2680,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>>       spin_lock(&arm_smmu_devices_lock);
>>       list_add(&smmu->list, &arm_smmu_devices);
>>       spin_unlock(&arm_smmu_devices_lock);
>> +    platform_set_drvdata(pdev, smmu);
>> +    of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);
>> +
>>       return 0;
>>
>>   out_free_structures:
>> @@ -2697,6 +2754,8 @@ static void __exit arm_smmu_exit(void)
>>   subsys_initcall(arm_smmu_init);
>>   module_exit(arm_smmu_exit);
>>
>> +IOMMU_OF_DECLARE(arm_smmu_v3, "arm,smmu-v3", NULL);
>> +
>>   MODULE_DESCRIPTION("IOMMU API for ARM architected SMMUv3 implementations");
>>   MODULE_AUTHOR("Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>");
>>   MODULE_LICENSE("GPL v2");
>> -- 
>> 1.8.0
>>
>>
>> _______________________________________________
>> iommu mailing list
>> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
> 
> 
> .
> 

[PATCH v2 6/9] iommu/arm-smmu: to support probe deferral

[leizhen]

On 2015/7/8 21:13, Robin Murphy wrote:
> On 07/07/15 04:30, Zhen Lei wrote:
>> For pci devices, only the root nodes have "iommus" property. So we
>> should traverse all of its sub nodes in of_xlate.
> 
> I don't really follow this description; only the host controller is described in DT - the devices behind it are probed dynamically and don't have nodes to traverse.

The devices behind host controller may have nodes, but have no "iommus" property.

I got this conclusion base on the original code as below:

	struct pci_bus *bus = pdev->bus;

	/* Walk up to the root bus */
	while (!pci_is_root_bus(bus))
		bus = bus->parent;

	/* Follow the "iommus" phandle from the host controller */
	of_node = of_parse_phandle(bus->bridge->parent->of_node, "iommus", 0);
	if (!of_node)
		return NULL;

> 
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>   drivers/iommu/arm-smmu-v3.c | 119 +++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 89 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index d6e3494..c569539 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -30,6 +30,8 @@
>>   #include <linux/of_address.h>
>>   #include <linux/pci.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/of_iommu.h>
>> +#include <linux/of_platform.h>
>>
>>   #include "io-pgtable.h"
>>
>> @@ -1741,10 +1743,23 @@ static void __arm_smmu_release_pci_iommudata(void *data)
>>       kfree(data);
>>   }
>>
>> -static struct arm_smmu_device *arm_smmu_get_for_pci_dev(struct pci_dev *pdev)
>> +static struct arm_smmu_device *find_smmu_by_node(struct device_node *np)
>> +{
>> +    struct platform_device *pdev;
>> +
>> +    /* to ensure np is a smmu device node */
>> +    if (!of_iommu_get_ops(np))
>> +        return NULL;
>> +
>> +    pdev = of_find_device_by_node(np);
>> +    if (!pdev)
>> +        return NULL;
>> +
>> +    return platform_get_drvdata(pdev);
>> +}
>> +
>> +static struct device_node *arm_smmu_get_pci_dev_root(struct pci_dev *pdev)
>>   {
>> -    struct device_node *of_node;
>> -    struct arm_smmu_device *curr, *smmu = NULL;
>>       struct pci_bus *bus = pdev->bus;
>>
>>       /* Walk up to the root bus */
>> @@ -1752,21 +1767,7 @@ static struct arm_smmu_device *arm_smmu_get_for_pci_dev(struct pci_dev *pdev)
>>           bus = bus->parent;
>>
>>       /* Follow the "iommus" phandle from the host controller */
> 
> Either update comments to reflect what the new code does, or remove them along with the code they describe.

OK, I will remove it, thanks.

> 
>> -    of_node = of_parse_phandle(bus->bridge->parent->of_node, "iommus", 0);
>> -    if (!of_node)
>> -        return NULL;
>> -
>> -    /* See if we can find an SMMU corresponding to the phandle */
>> -    spin_lock(&arm_smmu_devices_lock);
>> -    list_for_each_entry(curr, &arm_smmu_devices, list) {
>> -        if (curr->dev->of_node == of_node) {
>> -            smmu = curr;
>> -            break;
>> -        }
>> -    }
>> -    spin_unlock(&arm_smmu_devices_lock);
>> -    of_node_put(of_node);
>> -    return smmu;
>> +    return bus->bridge->parent->of_node;
>>   }
>>
>>   static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
>> @@ -1779,27 +1780,21 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
>>       return sid < limit;
>>   }
>>
>> -static int arm_smmu_add_device(struct device *dev)
>> +static int arm_smmu_add_device(struct device *dev, u32 sid)
>>   {
>>       int i, ret;
>> -    u32 sid, *sids;
>> -    struct pci_dev *pdev;
>> +    u32 *sids;
>>       struct iommu_group *group;
>>       struct arm_smmu_group *smmu_group;
>>       struct arm_smmu_device *smmu;
>>
>> -    /* We only support PCI, for now */
>> -    if (!dev_is_pci(dev))
>> -        return -ENODEV;
>> -
>> -    pdev = to_pci_dev(dev);
>>       group = iommu_group_get_for_dev(dev);
>>       if (IS_ERR(group))
>>           return PTR_ERR(group);
>>
>>       smmu_group = iommu_group_get_iommudata(group);
>>       if (!smmu_group) {
>> -        smmu = arm_smmu_get_for_pci_dev(pdev);
>> +        smmu = dev->archdata.iommu;
>>           if (!smmu) {
>>               ret = -ENOENT;
>>               goto out_put_group;
>> @@ -1819,8 +1814,6 @@ static int arm_smmu_add_device(struct device *dev)
>>           smmu = smmu_group->smmu;
>>       }
>>
>> -    /* Assume SID == RID until firmware tells us otherwise */
>> -    pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid, &sid);
>>       for (i = 0; i < smmu_group->num_sids; ++i) {
>>           /* If we already know about this SID, then we're done */
>>           if (smmu_group->sids[i] == sid)
>> @@ -1862,6 +1855,7 @@ out_put_group:
>>
>>   static void arm_smmu_remove_device(struct device *dev)
>>   {
>> +    dev->archdata.iommu = NULL;
>>       iommu_group_remove_device(dev);
>>   }
>>
>> @@ -1909,7 +1903,68 @@ out_unlock:
>>       return ret;
>>   }
>>
>> +static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
>> +{
>> +    int ret;
>> +    struct arm_smmu_device *smmu;
>> +
>> +    /*
>> +     * We can sure that args->np is a smmu device node, because this
>> +     * function was called by of_xlate hook.
>> +     *
>> +     * And in arm_smmu_device_dt_probe:
>> +     *    platform_set_drvdata(pdev, smmu);
>> +     *    of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);
>> +     *
>> +     * It seems impossible return NULL in normal times.
>> +     */
>> +    smmu = find_smmu_by_node(args->np);
>> +    if (!smmu) {
>> +        dev_err(dev, "unknown error caused smmu driver crashed\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    if (!dev->archdata.iommu)
>> +        dev->archdata.iommu = smmu;
>> +
>> +    if (dev->archdata.iommu != smmu) {
>> +        dev_err(dev, "behinds more than one smmu\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    /* We only support PCI, for now */
>> +    if (!dev_is_pci(dev)) {
>> +        return -ENODEV;
>> +    } else {
>> +        u32 sid;
>> +        struct device_node *of_root;
>> +        struct pci_dev *pdev = NULL;
>> +
>> +        for_each_pci_dev(pdev) {
> 
> Given that we get here before the host controller's driver probe, is this really going to work? Either way, it looks very dodgy.

It will be a problem. See the next answer, I think it will be resolved. Based on the following reasoning:
1. if .add_device happened before .of_xlate, then this version have no problem. Because the old version worked well, so arm_smmu_get_pci_dev_root will work well too, we can base on current method find
all sub nodes(which should be processed in .add_device for the old version) of the host controller.

2. if .add_device happened after.of_xlate, because the function of the new .add_device is the same to the old .add_device, so it will work well as the old version.

I will send patch v3.

> 
>> +            of_root = arm_smmu_get_pci_dev_root(pdev);
>> +            if (of_root != dev->of_node)
>> +                continue;
>> +
>> +            /*
>> +             * Assume SID == RID until firmware tells us otherwise
>> +             */
>> +            pci_for_each_dma_alias(pdev,
>> +                    __arm_smmu_get_pci_sid, &sid);
>> +
>> +            pdev->dev.archdata.iommu = smmu;
>> +            ret = arm_smmu_add_device(dev, sid);
>> +            if (ret) {
>> +                dev_err(dev, "failed to add into SMMU\n");
>> +                return ret;
>> +            }
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static struct iommu_ops arm_smmu_ops = {
>> +    .of_xlate        = arm_smmu_of_xlate,
>>       .capable        = arm_smmu_capable,
>>       .domain_alloc        = arm_smmu_domain_alloc,
>>       .domain_free        = arm_smmu_domain_free,
>> @@ -1918,7 +1973,6 @@ static struct iommu_ops arm_smmu_ops = {
>>       .map            = arm_smmu_map,
>>       .unmap            = arm_smmu_unmap,
>>       .iova_to_phys        = arm_smmu_iova_to_phys,
>> -    .add_device        = arm_smmu_add_device,
> 
> It might not be an immediate concern, but I think subverting the normal add_device process this way also completely breaks any kind of device hotplug.

Yes, I had aslo worried about it.

I think we can resovle it like below:
.add_device        = arm_smmu_add_device

arm_smmu_add_device:
	/* We only support pci device hotplug */
	if (!dev_is_pci(dev))
		return -ENODEV;

	pdev = to_pci_dev(dev);
	root = arm_smmu_get_pci_dev_root(pdev);		//arm_smmu_get_pci_dev_root return value should be modified that: return bus->bridge->parent;
	if (!root.archdata.iommu)			//should add "root.archdata.iommu = smmu" in of_xlate
		return -ENODEV;

	//add this pci device to smmu

> 
>>       .remove_device        = arm_smmu_remove_device,
>>       .domain_get_attr    = arm_smmu_domain_get_attr,
>>       .domain_set_attr    = arm_smmu_domain_set_attr,
>> @@ -2626,6 +2680,9 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
>>       spin_lock(&arm_smmu_devices_lock);
>>       list_add(&smmu->list, &arm_smmu_devices);
>>       spin_unlock(&arm_smmu_devices_lock);
>> +    platform_set_drvdata(pdev, smmu);
>> +    of_iommu_set_ops(smmu->dev->of_node, &arm_smmu_ops);
>> +
>>       return 0;
>>
>>   out_free_structures:
>> @@ -2697,6 +2754,8 @@ static void __exit arm_smmu_exit(void)
>>   subsys_initcall(arm_smmu_init);
>>   module_exit(arm_smmu_exit);
>>
>> +IOMMU_OF_DECLARE(arm_smmu_v3, "arm,smmu-v3", NULL);
>> +
>>   MODULE_DESCRIPTION("IOMMU API for ARM architected SMMUv3 implementations");
>>   MODULE_AUTHOR("Will Deacon <will.deacon@arm.com>");
>>   MODULE_LICENSE("GPL v2");
>> -- 
>> 1.8.0
>>
>>
>> _______________________________________________
>> iommu mailing list
>> iommu at lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
> 
> 
> .
> 

Re: [PATCH v2 6/9] iommu/arm-smmu: to support probe deferral

[Will Deacon]

On Thu, Jul 09, 2015 at 12:10:11PM +0100, leizhen wrote:
> On 2015/7/8 21:13, Robin Murphy wrote:
> > On 07/07/15 04:30, Zhen Lei wrote:
> >> For pci devices, only the root nodes have "iommus" property. So we
> >> should traverse all of its sub nodes in of_xlate.
> > 
> > I don't really follow this description; only the host controller is
> > described in DT - the devices behind it are probed dynamically and don't
> > have nodes to traverse.
> 
> The devices behind host controller may have nodes, but have no "iommus" property.

No, the PCI masters won't have nodes in the DT on arm/arm64 systems (it
makes hotplug difficult).

> I got this conclusion base on the original code as below:
> 
> 	struct pci_bus *bus = pdev->bus;
> 
> 	/* Walk up to the root bus */
> 	while (!pci_is_root_bus(bus))
> 		bus = bus->parent;

So this walks up the PCI topology, created by probing the bus, until we
get to the top-level bus...

> 	/* Follow the "iommus" phandle from the host controller */
> 	of_node = of_parse_phandle(bus->bridge->parent->of_node, "iommus", 0);
> 	if (!of_node)
> 		return NULL;

... then we find the host controller device, which *does* have a
device-tree node and use *that* to find out the IOMMU corresponding to
the PCI device.

Will

[PATCH v2 6/9] iommu/arm-smmu: to support probe deferral

[Will Deacon]

On Thu, Jul 09, 2015 at 12:10:11PM +0100, leizhen wrote:
> On 2015/7/8 21:13, Robin Murphy wrote:
> > On 07/07/15 04:30, Zhen Lei wrote:
> >> For pci devices, only the root nodes have "iommus" property. So we
> >> should traverse all of its sub nodes in of_xlate.
> > 
> > I don't really follow this description; only the host controller is
> > described in DT - the devices behind it are probed dynamically and don't
> > have nodes to traverse.
> 
> The devices behind host controller may have nodes, but have no "iommus" property.

No, the PCI masters won't have nodes in the DT on arm/arm64 systems (it
makes hotplug difficult).

> I got this conclusion base on the original code as below:
> 
> 	struct pci_bus *bus = pdev->bus;
> 
> 	/* Walk up to the root bus */
> 	while (!pci_is_root_bus(bus))
> 		bus = bus->parent;

So this walks up the PCI topology, created by probing the bus, until we
get to the top-level bus...

> 	/* Follow the "iommus" phandle from the host controller */
> 	of_node = of_parse_phandle(bus->bridge->parent->of_node, "iommus", 0);
> 	if (!of_node)
> 		return NULL;

... then we find the host controller device, which *does* have a
device-tree node and use *that* to find out the IOMMU corresponding to
the PCI device.

Will

Re: [PATCH v2 9/9] iommu/arm-smmu: add support for non-pci devices

[Robin Murphy]

On 09/07/15 02:56, leizhen wrote:
[...]
>>> @@ -1928,9 +1929,35 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
[...]
>>> +        for (i = 0; i < args->args_count; i++) {
>>
>> I'm dubious of the value of looping here - having n>1 #iommu-cells per phandle means that every platform device behind one SMMU must have the same number of stream IDs, or you still have to have
>> repeated phandles for every device with some greater multiple of n stream IDs each, but ruling out any device with <n. I'm not sure how realistic that is, and whether there's any real benefit beyond
>> saving a handful of bytes in the DTB.
>
> As you mentioned before, a master with two streamIDs can be written as below(This is also mentioned in Documentation\devicetree\bindings\iommu\iommu.txt):
> #iommu-cells = <1>;
> iommus = <&{/iommu} 23>, <&{/iommu} 24>;
>
> On my hardware platform, a master only have one streamID. But I tested two cases:
> #iommu-cells = <1>;
> 1. iommus = <&smmu0 good-sid>, <&smmu0 bad-sid>;
> 2. iommus = <&smmu0 bad-sid>, <&smmu0 good-sid>;
> All of these two cases worked well.
>
> Well, if each master contains two streamIDs(or more but equal), this for loop is needed.

My point is that args_count == #iommu-cells here. If you mandate 
#iommu-cells == 1 as per your example (which in my opinion *is* the 
right thing to do), then looping over something which is guaranteed to 
be a single item is pointless.

You'd only need the loop if args_count > 1, for example:

	#iommu-cells = <2>;
	1: iommus = <&smmu0 sid1 sid2>;
	2: iommus = <&smmu0 sid1 sid2>, <&smmu0 sid3 sid4>;

but then you'd be also stuck describing any device with an odd number of 
stream IDs on that SMMU:

	3: iommus = <&smmu0 sid1 what-do-I-put-here?>;

which is why I don't think it's worth trying to accommodate anything 
other than #iommu-cells == 1, and for that case you know you will only 
ever have to deal with args[0] in each of_xlate() call.

Robin.

[PATCH v2 9/9] iommu/arm-smmu: add support for non-pci devices

[Robin Murphy]

On 09/07/15 02:56, leizhen wrote:
[...]
>>> @@ -1928,9 +1929,35 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
[...]
>>> +        for (i = 0; i < args->args_count; i++) {
>>
>> I'm dubious of the value of looping here - having n>1 #iommu-cells per phandle means that every platform device behind one SMMU must have the same number of stream IDs, or you still have to have
>> repeated phandles for every device with some greater multiple of n stream IDs each, but ruling out any device with <n. I'm not sure how realistic that is, and whether there's any real benefit beyond
>> saving a handful of bytes in the DTB.
>
> As you mentioned before, a master with two streamIDs can be written as below(This is also mentioned in Documentation\devicetree\bindings\iommu\iommu.txt):
> #iommu-cells = <1>;
> iommus = <&{/iommu} 23>, <&{/iommu} 24>;
>
> On my hardware platform, a master only have one streamID. But I tested two cases:
> #iommu-cells = <1>;
> 1. iommus = <&smmu0 good-sid>, <&smmu0 bad-sid>;
> 2. iommus = <&smmu0 bad-sid>, <&smmu0 good-sid>;
> All of these two cases worked well.
>
> Well, if each master contains two streamIDs(or more but equal), this for loop is needed.

My point is that args_count == #iommu-cells here. If you mandate 
#iommu-cells == 1 as per your example (which in my opinion *is* the 
right thing to do), then looping over something which is guaranteed to 
be a single item is pointless.

You'd only need the loop if args_count > 1, for example:

	#iommu-cells = <2>;
	1: iommus = <&smmu0 sid1 sid2>;
	2: iommus = <&smmu0 sid1 sid2>, <&smmu0 sid3 sid4>;

but then you'd be also stuck describing any device with an odd number of 
stream IDs on that SMMU:

	3: iommus = <&smmu0 sid1 what-do-I-put-here?>;

which is why I don't think it's worth trying to accommodate anything 
other than #iommu-cells == 1, and for that case you know you will only 
ever have to deal with args[0] in each of_xlate() call.

Robin.

Re: [PATCH v2 9/9] iommu/arm-smmu: add support for non-pci devices

[leizhen]

On 2015/7/9 20:01, Robin Murphy wrote:
> On 09/07/15 02:56, leizhen wrote:
> [...]
>>>> @@ -1928,9 +1929,35 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
> [...]
>>>> +        for (i = 0; i < args->args_count; i++) {
>>>
>>> I'm dubious of the value of looping here - having n>1 #iommu-cells per phandle means that every platform device behind one SMMU must have the same number of stream IDs, or you still have to have
>>> repeated phandles for every device with some greater multiple of n stream IDs each, but ruling out any device with <n. I'm not sure how realistic that is, and whether there's any real benefit beyond
>>> saving a handful of bytes in the DTB.
>>
>> As you mentioned before, a master with two streamIDs can be written as below(This is also mentioned in Documentation\devicetree\bindings\iommu\iommu.txt):
>> #iommu-cells = <1>;
>> iommus = <&{/iommu} 23>, <&{/iommu} 24>;
>>
>> On my hardware platform, a master only have one streamID. But I tested two cases:
>> #iommu-cells = <1>;
>> 1. iommus = <&smmu0 good-sid>, <&smmu0 bad-sid>;
>> 2. iommus = <&smmu0 bad-sid>, <&smmu0 good-sid>;
>> All of these two cases worked well.
>>
>> Well, if each master contains two streamIDs(or more but equal), this for loop is needed.
> 
> My point is that args_count == #iommu-cells here. If you mandate #iommu-cells == 1 as per your example (which in my opinion *is* the right thing to do), then looping over something which is guaranteed
> to be a single item is pointless.
> 
> You'd only need the loop if args_count > 1, for example:
> 
>     #iommu-cells = <2>;
>     1: iommus = <&smmu0 sid1 sid2>;
>     2: iommus = <&smmu0 sid1 sid2>, <&smmu0 sid3 sid4>;
> 
> but then you'd be also stuck describing any device with an odd number of stream IDs on that SMMU:
> 
>     3: iommus = <&smmu0 sid1 what-do-I-put-here?>;
> 
> which is why I don't think it's worth trying to accommodate anything other than #iommu-cells == 1, and for that case you know you will only ever have to deal with args[0] in each of_xlate() call.

OK, I will consider it in v3.

> 
> Robin.
> 
> 
> .
> 

[PATCH v2 9/9] iommu/arm-smmu: add support for non-pci devices

[leizhen]

On 2015/7/9 20:01, Robin Murphy wrote:
> On 09/07/15 02:56, leizhen wrote:
> [...]
>>>> @@ -1928,9 +1929,35 @@ static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
> [...]
>>>> +        for (i = 0; i < args->args_count; i++) {
>>>
>>> I'm dubious of the value of looping here - having n>1 #iommu-cells per phandle means that every platform device behind one SMMU must have the same number of stream IDs, or you still have to have
>>> repeated phandles for every device with some greater multiple of n stream IDs each, but ruling out any device with <n. I'm not sure how realistic that is, and whether there's any real benefit beyond
>>> saving a handful of bytes in the DTB.
>>
>> As you mentioned before, a master with two streamIDs can be written as below(This is also mentioned in Documentation\devicetree\bindings\iommu\iommu.txt):
>> #iommu-cells = <1>;
>> iommus = <&{/iommu} 23>, <&{/iommu} 24>;
>>
>> On my hardware platform, a master only have one streamID. But I tested two cases:
>> #iommu-cells = <1>;
>> 1. iommus = <&smmu0 good-sid>, <&smmu0 bad-sid>;
>> 2. iommus = <&smmu0 bad-sid>, <&smmu0 good-sid>;
>> All of these two cases worked well.
>>
>> Well, if each master contains two streamIDs(or more but equal), this for loop is needed.
> 
> My point is that args_count == #iommu-cells here. If you mandate #iommu-cells == 1 as per your example (which in my opinion *is* the right thing to do), then looping over something which is guaranteed
> to be a single item is pointless.
> 
> You'd only need the loop if args_count > 1, for example:
> 
>     #iommu-cells = <2>;
>     1: iommus = <&smmu0 sid1 sid2>;
>     2: iommus = <&smmu0 sid1 sid2>, <&smmu0 sid3 sid4>;
> 
> but then you'd be also stuck describing any device with an odd number of stream IDs on that SMMU:
> 
>     3: iommus = <&smmu0 sid1 what-do-I-put-here?>;
> 
> which is why I don't think it's worth trying to accommodate anything other than #iommu-cells == 1, and for that case you know you will only ever have to deal with args[0] in each of_xlate() call.

OK, I will consider it in v3.

> 
> Robin.
> 
> 
> .
>