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

[Mikko Perttunen <cyndis@xxxxxxxx>]

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 <mperttunen@xxxxxxxxxx>
> > ---
> >   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 = &iommu_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, &info);
> >       } 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, force_dma, NULL, NULL);
> > +}
> > +static inline int of_dma_configure_id(struct device *dev,
> > +                      struct device_node *np,
> > +                      bool force_dma, const u32 *id)
> > +{
> > +    return __of_dma_configure(dev, np, force_dma, id, NULL);
> > +}
> > +static inline int
> > +of_dma_configure_iommu_spec(struct device *dev, struct device_node *np,
> > +                bool force_dma, struct of_phandle_args *iommu_spec)
> > +{
> > +    return __of_dma_configure(dev, np, force_dma, NULL, iommu_spec);
> >   }
> >   #else /* CONFIG_OF */
> > @@ -112,18 +125,25 @@ static inline struct device_node 
> > *of_cpu_device_node_get(int cpu)
> >       return NULL;
> >   }
> > -static inline int of_dma_configure_id(struct device *dev,
> > +static inline int of_dma_configure(struct device *dev,
> >                      struct device_node *np,
> >                      bool force_dma)
> >   {
> >       return 0;
> >   }
> > -static inline int of_dma_configure(struct device *dev,
> > -                   struct device_node *np,
> > -                   bool force_dma)
> > +
> > +static inline int of_dma_configure_id(struct device *dev,
> > +                      struct device_node *np,
> > +                      bool force_dma, const u32 *id)
> >   {
> >       return 0;
> >   }
> > +
> > +static inline int
> > +of_dma_configure_iommu_spec(struct device *dev, struct device_node *np,
> > +                bool force_dma, struct of_phandle_args *iommu_spec)
> > +{    return 0;
> > +}
> >   #endif /* CONFIG_OF */
> >   #endif /* _LINUX_OF_DEVICE_H */
> > diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> > index 16f4b3e87f20..e8d1e6d32d77 100644
> > --- a/include/linux/of_iommu.h
> > +++ b/include/linux/of_iommu.h
> > @@ -14,7 +14,8 @@ extern int of_get_dma_window(struct device_node *dn, 
> > const char *prefix,
> >   extern 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);
> >   #else
> > @@ -27,7 +28,8 @@ static inline int of_get_dma_window(struct 
> > device_node *dn, const char *prefix,
> >   static inline 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);
> >   {
> >       return NULL;
> >   }