Re: [RFC PATCH 1/8] of/device: Allow specifying a custom iommu_spec to of_dma_configure

2021-02-16 Thread Mikko Perttunen 

On 2/16/21 2:47 PM, Robin Murphy wrote:

Hi Mikko,

On 2021-02-08 16:38, Mikko Perttunen wrote:

To allow for more customized device tree bindings that point to IOMMUs,
allow manual specification of iommu_spec to of_dma_configure.

The initial use case for this is with Host1x, where the driver manages
a set of device tree-defined IOMMU contexts that are dynamically
allocated to various users. These contexts don't correspond to
platform devices and are instead attached to dummy devices on a custom
software bus.


I'd suggest taking a closer look at the patches that made this 
of_dma_configure_id() in the first place, and the corresponding bus code 
in fsl-mc. At this level, Host1x sounds effectively identical to DPAA2 
in terms of being a bus of logical devices composed from bits of 
implicit behind-the-scenes hardware. I mean, compare your series title 
to the fact that their identifiers are literally named "Isolation 
Context ID" ;)


Please just use the existing mechanisms to describe a mapping between 
Host1x context IDs and SMMU Stream IDs, rather than what looks like a 
giant hacky mess here.


(This also reminds me I wanted to rip out all the PCI special-cases and 
convert pci_dma_configure() over to passing its own IDs too, so thanks 
for the memory-jog...)


Thanks Robin, not sure how I missed that the first time :) Maybe because 
Host1x doesn't have a concept of its own "IDs" for these per se - the 
hardware just uses stream IDs as is. I would need to count the number of 
mapped IDs from the iommu-map property and introduce some 0..N range of 
IDs at the software level. But maybe that's not too bad if we're able to 
use the existing paths and bindings then.


I'll take a look at switching to iommu-map.

Thanks,
Mikko



Robin.


Signed-off-by: Mikko Perttunen 
---
  drivers/iommu/of_iommu.c  | 12 
  drivers/of/device.c   |  9 +
  include/linux/of_device.h | 34 +++---
  include/linux/of_iommu.h  |  6 --
  4 files changed, 44 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e505b9130a1c..3fefa6c63863 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -87,8 +87,7 @@ int of_get_dma_window(struct device_node *dn, const 
char *prefix, int index,

  }
  EXPORT_SYMBOL_GPL(of_get_dma_window);
-static int of_iommu_xlate(struct device *dev,
-  struct of_phandle_args *iommu_spec)
+int of_iommu_xlate(struct device *dev, struct of_phandle_args 
*iommu_spec)

  {
  const struct iommu_ops *ops;
  struct fwnode_handle *fwnode = _spec->np->fwnode;
@@ -117,6 +116,7 @@ static int of_iommu_xlate(struct device *dev,
  module_put(ops->owner);
  return ret;
  }
+EXPORT_SYMBOL_GPL(of_iommu_xlate);
  static int of_iommu_configure_dev_id(struct device_node *master_np,
   struct device *dev,
@@ -177,7 +177,8 @@ static int of_iommu_configure_device(struct 
device_node *master_np,

  const struct iommu_ops *of_iommu_configure(struct device *dev,
 struct device_node *master_np,
-   const u32 *id)
+   const u32 *id,
+   struct of_phandle_args *iommu_spec)
  {
  const struct iommu_ops *ops = NULL;
  struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
@@ -209,7 +210,10 @@ const struct iommu_ops *of_iommu_configure(struct 
device *dev,

  err = pci_for_each_dma_alias(to_pci_dev(dev),
   of_pci_iommu_init, );
  } else {
-    err = of_iommu_configure_device(master_np, dev, id);
+    if (iommu_spec)
+    err = of_iommu_xlate(dev, iommu_spec);
+    else
+    err = of_iommu_configure_device(master_np, dev, id);
  fwspec = dev_iommu_fwspec_get(dev);
  if (!err && fwspec)
diff --git a/drivers/of/device.c b/drivers/of/device.c
index aedfaaafd3e7..84ada2110c5b 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -88,8 +88,9 @@ int of_device_add(struct platform_device *ofdev)
   * can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE 
events

   * to fix up DMA configuration.
   */
-int of_dma_configure_id(struct device *dev, struct device_node *np,
-    bool force_dma, const u32 *id)
+int __of_dma_configure(struct device *dev, struct device_node *np,
+    bool force_dma, const u32 *id,
+    struct of_phandle_args *iommu_spec)
  {
  const struct iommu_ops *iommu;
  const struct bus_dma_region *map = NULL;
@@ -170,7 +171,7 @@ int of_dma_configure_id(struct device *dev, struct 
device_node *np,

  dev_dbg(dev, "device is%sdma coherent\n",
  coherent ? " " : " not ");
-    iommu = of_iommu_configure(dev, np, id);
+    iommu = of_iommu_configure(dev, np, id, iommu_spec);
  if (PTR_ERR(iommu) == -EPROBE_DEFER) {
  kfree(map);
  return -EPROBE_DEFER;
@@ -184,7 +185,7 @@ int of_dma_configure_id(struct device *dev,

Re: [RFC PATCH 1/8] of/device: Allow specifying a custom iommu_spec to of_dma_configure

2021-02-16 Thread Robin Murphy 

Hi Mikko,

On 2021-02-08 16:38, Mikko Perttunen wrote:

To allow for more customized device tree bindings that point to IOMMUs,
allow manual specification of iommu_spec to of_dma_configure.

The initial use case for this is with Host1x, where the driver manages
a set of device tree-defined IOMMU contexts that are dynamically
allocated to various users. These contexts don't correspond to
platform devices and are instead attached to dummy devices on a custom
software bus.


I'd suggest taking a closer look at the patches that made this 
of_dma_configure_id() in the first place, and the corresponding bus code 
in fsl-mc. At this level, Host1x sounds effectively identical to DPAA2 
in terms of being a bus of logical devices composed from bits of 
implicit behind-the-scenes hardware. I mean, compare your series title 
to the fact that their identifiers are literally named "Isolation 
Context ID" ;)


Please just use the existing mechanisms to describe a mapping between 
Host1x context IDs and SMMU Stream IDs, rather than what looks like a 
giant hacky mess here.


(This also reminds me I wanted to rip out all the PCI special-cases and 
convert pci_dma_configure() over to passing its own IDs too, so thanks 
for the memory-jog...)


Robin.


Signed-off-by: Mikko Perttunen 
---
  drivers/iommu/of_iommu.c  | 12 
  drivers/of/device.c   |  9 +
  include/linux/of_device.h | 34 +++---
  include/linux/of_iommu.h  |  6 --
  4 files changed, 44 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e505b9130a1c..3fefa6c63863 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -87,8 +87,7 @@ int of_get_dma_window(struct device_node *dn, const char 
*prefix, int index,
  }
  EXPORT_SYMBOL_GPL(of_get_dma_window);
  
-static int of_iommu_xlate(struct device *dev,

- struct of_phandle_args *iommu_spec)
+int of_iommu_xlate(struct device *dev, struct of_phandle_args *iommu_spec)
  {
const struct iommu_ops *ops;
struct fwnode_handle *fwnode = _spec->np->fwnode;
@@ -117,6 +116,7 @@ static int of_iommu_xlate(struct device *dev,
module_put(ops->owner);
return ret;
  }
+EXPORT_SYMBOL_GPL(of_iommu_xlate);
  
  static int of_iommu_configure_dev_id(struct device_node *master_np,

 struct device *dev,
@@ -177,7 +177,8 @@ static int of_iommu_configure_device(struct device_node 
*master_np,
  
  const struct iommu_ops *of_iommu_configure(struct device *dev,

   struct device_node *master_np,
-  const u32 *id)
+  const u32 *id,
+  struct of_phandle_args *iommu_spec)
  {
const struct iommu_ops *ops = NULL;
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
@@ -209,7 +210,10 @@ const struct iommu_ops *of_iommu_configure(struct device 
*dev,
err = pci_for_each_dma_alias(to_pci_dev(dev),
 of_pci_iommu_init, );
} else {
-   err = of_iommu_configure_device(master_np, dev, id);
+   if (iommu_spec)
+   err = of_iommu_xlate(dev, iommu_spec);
+   else
+   err = of_iommu_configure_device(master_np, dev, id);
  
  		fwspec = dev_iommu_fwspec_get(dev);

if (!err && fwspec)
diff --git a/drivers/of/device.c b/drivers/of/device.c
index aedfaaafd3e7..84ada2110c5b 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -88,8 +88,9 @@ int of_device_add(struct platform_device *ofdev)
   * can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events
   * to fix up DMA configuration.
   */
-int of_dma_configure_id(struct device *dev, struct device_node *np,
-   bool force_dma, const u32 *id)
+int __of_dma_configure(struct device *dev, struct device_node *np,
+   bool force_dma, const u32 *id,
+   struct of_phandle_args *iommu_spec)
  {
const struct iommu_ops *iommu;
const struct bus_dma_region *map = NULL;
@@ -170,7 +171,7 @@ int of_dma_configure_id(struct device *dev, struct 
device_node *np,
dev_dbg(dev, "device is%sdma coherent\n",
coherent ? " " : " not ");
  
-	iommu = of_iommu_configure(dev, np, id);

+   iommu = of_iommu_configure(dev, np, id, iommu_spec);
if (PTR_ERR(iommu) == -EPROBE_DEFER) {
kfree(map);
return -EPROBE_DEFER;
@@ -184,7 +185,7 @@ int of_dma_configure_id(struct device *dev, struct 
device_node *np,
dev->dma_range_map = map;
return 0;
  }
-EXPORT_SYMBOL_GPL(of_dma_configure_id);
+EXPORT_SYMBOL_GPL(__of_dma_configure);
  
  int of_device_register(struct platform_device *pdev)

  {
diff --git

Re: [RFC PATCH 1/8] of/device: Allow specifying a custom iommu_spec to of_dma_configure

2021-02-08 Thread Mikko Perttunen 

On 2/8/21 6:38 PM, Mikko Perttunen wrote:

...
-static int of_iommu_xlate(struct device *dev,
- struct of_phandle_args *iommu_spec)
+int of_iommu_xlate(struct device *dev, struct of_phandle_args *iommu_spec)
...
+EXPORT_SYMBOL_GPL(of_iommu_xlate);
  


These two chunks should not be needed.. looks like I left them in from 
some earlier experiments.


Sending patches out is really the best way to notice mistakes, even 
after reading through them 5 times before.. :)


Mikko

[RFC PATCH 1/8] of/device: Allow specifying a custom iommu_spec to of_dma_configure

2021-02-08 Thread Mikko Perttunen 
To allow for more customized device tree bindings that point to IOMMUs,
allow manual specification of iommu_spec to of_dma_configure.

The initial use case for this is with Host1x, where the driver manages
a set of device tree-defined IOMMU contexts that are dynamically
allocated to various users. These contexts don't correspond to
platform devices and are instead attached to dummy devices on a custom
software bus.

Signed-off-by: Mikko Perttunen 
---
 drivers/iommu/of_iommu.c  | 12 
 drivers/of/device.c   |  9 +
 include/linux/of_device.h | 34 +++---
 include/linux/of_iommu.h  |  6 --
 4 files changed, 44 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e505b9130a1c..3fefa6c63863 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -87,8 +87,7 @@ int of_get_dma_window(struct device_node *dn, const char 
*prefix, int index,
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
 
-static int of_iommu_xlate(struct device *dev,
- struct of_phandle_args *iommu_spec)
+int of_iommu_xlate(struct device *dev, struct of_phandle_args *iommu_spec)
 {
const struct iommu_ops *ops;
struct fwnode_handle *fwnode = _spec->np->fwnode;
@@ -117,6 +116,7 @@ static int of_iommu_xlate(struct device *dev,
module_put(ops->owner);
return ret;
 }
+EXPORT_SYMBOL_GPL(of_iommu_xlate);
 
 static int of_iommu_configure_dev_id(struct device_node *master_np,
 struct device *dev,
@@ -177,7 +177,8 @@ static int of_iommu_configure_device(struct device_node 
*master_np,
 
 const struct iommu_ops *of_iommu_configure(struct device *dev,
   struct device_node *master_np,
-  const u32 *id)
+  const u32 *id,
+  struct of_phandle_args *iommu_spec)
 {
const struct iommu_ops *ops = NULL;
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
@@ -209,7 +210,10 @@ const struct iommu_ops *of_iommu_configure(struct device 
*dev,
err = pci_for_each_dma_alias(to_pci_dev(dev),
 of_pci_iommu_init, );
} else {
-   err = of_iommu_configure_device(master_np, dev, id);
+   if (iommu_spec)
+   err = of_iommu_xlate(dev, iommu_spec);
+   else
+   err = of_iommu_configure_device(master_np, dev, id);
 
fwspec = dev_iommu_fwspec_get(dev);
if (!err && fwspec)
diff --git a/drivers/of/device.c b/drivers/of/device.c
index aedfaaafd3e7..84ada2110c5b 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -88,8 +88,9 @@ int of_device_add(struct platform_device *ofdev)
  * can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events
  * to fix up DMA configuration.
  */
-int of_dma_configure_id(struct device *dev, struct device_node *np,
-   bool force_dma, const u32 *id)
+int __of_dma_configure(struct device *dev, struct device_node *np,
+   bool force_dma, const u32 *id,
+   struct of_phandle_args *iommu_spec)
 {
const struct iommu_ops *iommu;
const struct bus_dma_region *map = NULL;
@@ -170,7 +171,7 @@ int of_dma_configure_id(struct device *dev, struct 
device_node *np,
dev_dbg(dev, "device is%sdma coherent\n",
coherent ? " " : " not ");
 
-   iommu = of_iommu_configure(dev, np, id);
+   iommu = of_iommu_configure(dev, np, id, iommu_spec);
if (PTR_ERR(iommu) == -EPROBE_DEFER) {
kfree(map);
return -EPROBE_DEFER;
@@ -184,7 +185,7 @@ int of_dma_configure_id(struct device *dev, struct 
device_node *np,
dev->dma_range_map = map;
return 0;
 }
-EXPORT_SYMBOL_GPL(of_dma_configure_id);
+EXPORT_SYMBOL_GPL(__of_dma_configure);
 
 int of_device_register(struct platform_device *pdev)
 {
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index 07ca187fc5e4..40cc3e788cb9 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -55,14 +55,27 @@ static inline struct device_node 
*of_cpu_device_node_get(int cpu)
return of_node_get(cpu_dev->of_node);
 }
 
-int of_dma_configure_id(struct device *dev,
+int __of_dma_configure(struct device *dev,
 struct device_node *np,
-bool force_dma, const u32 *id);
+bool force_dma, const u32 *id,
+struct of_phandle_args *iommu_spec);
 static inline int of_dma_configure(struct device *dev,
   struct device_node *np,
   bool force_dma)
 {
-   return of_dma_configure_id(dev, np, force_dma, NULL);
+   return __of_dma_configure(dev, np,