bugfixs and add support for non-pci devices

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 <[email protected]>

---

drivers/iommu/arm-smmu-v3.c +4 -0

+static struct arm_smmu_option_prop arm_smmu_options[] = {
+	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
+	{ 0, NULL},
+};

drivers/iommu/arm-smmu-v3.c +4 -0

+static void parse_driver_options(struct arm_smmu_device *smmu)
+{
+	int i = 0;
+

Quoted 4 lines · 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.

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

Quoted 2 lines

Signed-off-by Zhen Lei <[email protected]>

---

[...]

Quoted 4 lines

drivers/iommu/arm-smmu-v3.c +4 -0

+static struct arm_smmu_option_prop arm_smmu_options[] = {
+	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
+	{ 0, NULL},
+};

[...]

Quoted 4 lines

drivers/iommu/arm-smmu-v3.c +4 -0

+static void parse_driver_options(struct arm_smmu_device *smmu)
+{
+	int i = 0;
+

drivers/iommu/arm-smmu-v3.c struct arm_smmu_domain { · line 637

Quoted 10 lines

drivers/iommu/arm-smmu-v3.c +10 -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++)

Hi Robin,

FWIW, I already queued this ;)

Quoted 6 lines · Robin Murphy

Quoted 4 lines · 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.

Quoted 2 lines

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

I've added that.

Quoted 29 lines

Quoted 3 lines

Signed-off-by Zhen Lei <[email protected]>

---

[...]

Quoted 4 lines

drivers/iommu/arm-smmu-v3.c +4 -0

+static struct arm_smmu_option_prop arm_smmu_options[] = {
+	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
+	{ 0, NULL},
+};

[...]

Quoted 4 lines

drivers/iommu/arm-smmu-v3.c +4 -0

+static void parse_driver_options(struct arm_smmu_device *smmu)
+{
+	int i = 0;
+

drivers/iommu/arm-smmu-v3.c struct arm_smmu_domain { · line 637

Quoted 10 lines

drivers/iommu/arm-smmu-v3.c +10 -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?

Quoted 1 line

	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

.

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 <[email protected]>

---
  drivers/iommu/arm-smmu-v3.c | 119 +++++++++++++++++++++++++++++++++-----------
  1 file changed, 89 insertions(+), 30 deletions(-)

drivers/iommu/arm-smmu-v3.c +18 -3

index d6e3494..c569539 100644
@@ -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 */

drivers/iommu/arm-smmu-v3.c +33 -26

-	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;
+	}

drivers/iommu/arm-smmu-v3.c +23 -0

+			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,

drivers/iommu/arm-smmu-v3.c +5 -1

  	.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 <[email protected]>");
  MODULE_LICENSE("GPL v2");
--

1.8.0

_______________________________________________
iommu mailing list
iommu at lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Quoted 2 lines · Zhen Lei

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.

Quoted 45 lines

Signed-off-by Zhen Lei <[email protected]>

---
  drivers/iommu/arm-smmu-v3.c | 119 +++++++++++++++++++++++++++++++++-----------
  1 file changed, 89 insertions(+), 30 deletions(-)

drivers/iommu/arm-smmu-v3.c +18 -3

index d6e3494..c569539 100644
@@ -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;
 

drivers/iommu/arm-smmu-v3.c static struct arm_smmu_device *arm_smmu_get_for_pci_dev(struct pci_dev *pdev) · line 1769

Quoted 5 lines

drivers/iommu/arm-smmu-v3.c +0 -0

      /* 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.

Quoted 104 lines

drivers/iommu/arm-smmu-v3.c +37 -26

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

drivers/iommu/arm-smmu-v3.c out_unlock: · line 1943

Quoted 6 lines

drivers/iommu/arm-smmu-v3.c +6 -0

+    } 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.

Quoted 27 lines

drivers/iommu/arm-smmu-v3.c +23 -0

+            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,

drivers/iommu/arm-smmu-v3.c static struct iommu_ops arm_smmu_ops = { · line 1921

Quoted 6 lines

drivers/iommu/arm-smmu-v3.c +0 -1

      .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.

Quoted 32 lines

drivers/iommu/arm-smmu-v3.c +5 -0

      .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 <[email protected]>");
  MODULE_LICENSE("GPL v2");

Signature · 7 lines

1.8.0


_______________________________________________
iommu mailing list
iommu at lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

.

Quoted 7 lines · Robin Murphy

Quoted 2 lines · Zhen Lei

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;

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

Signed-off-by Zhen Lei <[email protected]>

---
  drivers/iommu/arm-smmu-v3.c | 22 ++--------------------
  1 file changed, 2 insertions(+), 20 deletions(-)

drivers/iommu/arm-smmu-v3.c +1 -11

index c569539..39c55f6 100644
@@ -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);

drivers/iommu/arm-smmu-v3.c +0 -1

  	if (!smmu)
  		return -ENODEV;
 
--

1.8.0

_______________________________________________
iommu mailing list
iommu at lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Quoted 42 lines · Zhen Lei

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

Signed-off-by Zhen Lei <[email protected]>

---
  drivers/iommu/arm-smmu-v3.c | 22 ++--------------------
  1 file changed, 2 insertions(+), 20 deletions(-)

drivers/iommu/arm-smmu-v3.c +1 -10

index c569539..39c55f6 100644
@@ -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;
 

drivers/iommu/arm-smmu-v3.c out_free_structures: · line 2689

Quoted 11 lines

drivers/iommu/arm-smmu-v3.c +1 -10

-    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)?

Quoted 13 lines

drivers/iommu/arm-smmu-v3.c +0 -0

      if (!smmu)
          return -ENODEV;
 

Signature · 7 lines

1.8.0


_______________________________________________
iommu mailing list
iommu at lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

.

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 <[email protected]>

---
  drivers/iommu/arm-smmu-v3.c | 39 +++++++++++++++++++++++++++++++++++++--
  1 file changed, 37 insertions(+), 2 deletions(-)

drivers/iommu/arm-smmu-v3.c +6 -2

index 274d059..582cd1e 100644
@@ -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) {

drivers/iommu/arm-smmu-v3.c +16 -1

+			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

Quoted 2 lines · Zhen Lei

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.

Quoted 34 lines

Signed-off-by Zhen Lei <[email protected]>

---
  drivers/iommu/arm-smmu-v3.c | 39 +++++++++++++++++++++++++++++++++++++--
  1 file changed, 37 insertions(+), 2 deletions(-)

drivers/iommu/arm-smmu-v3.c +10 -2

index 274d059..582cd1e 100644
@@ -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);

drivers/iommu/arm-smmu-v3.c static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args) · line 1947

Quoted 6 lines

drivers/iommu/arm-smmu-v3.c +6 -0

+            }
+
+            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.

drivers/iommu/arm-smmu-v3.c static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args) · line 1952

Quoted 6 lines

drivers/iommu/arm-smmu-v3.c +5 -0

+            }
+        }
+        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.

Quoted 36 lines

drivers/iommu/arm-smmu-v3.c +16 -0

+            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);
  }
 

Signature · 7 lines

1.8.0


_______________________________________________
iommu mailing list
iommu at lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

.

Quoted 1 line

Quoted 1 line

(quoted diff) +0 -0

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

Quoted 5 lines

Quoted 1 line

+ 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.

On 09/07/15 02:56, leizhen wrote:
[...]

Quoted 1 line

Quoted 1 line

Quoted 1 line

(quoted diff) +0 -0

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

[...]

Quoted 10 lines

Quoted 5 lines

Quoted 1 line

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

Quoted 7 lines

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.

.

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.