On Mon, Sep 22, 2014 at 10:29:10AM +0100, Thierry Reding wrote:
> On Thu, Sep 18, 2014 at 02:17:33PM +0300, Laurent Pinchart wrote:
> > On Friday 12 September 2014 17:34:53 Will Deacon wrote:
> > > This patch extends of_dma_configure so that it sets up the IOMMU for a
> > > device, as well as the coherent/non-coherent DMA mapping ops.
> > >
> > > Signed-off-by: Will Deacon <will.dea...@arm.com>
> > > ---
> > > arch/arm/include/asm/dma-mapping.h | 4 +++-
> > > drivers/of/platform.c | 24 +++++++++++++++++-------
> > > include/linux/dma-mapping.h | 8 +++++++-
> > > 3 files changed, 27 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/arch/arm/include/asm/dma-mapping.h
> > > b/arch/arm/include/asm/dma-mapping.h index 7e9ac4f604c3..a8bb0c494bb3
> > > 100644
> > > --- a/arch/arm/include/asm/dma-mapping.h
> > > +++ b/arch/arm/include/asm/dma-mapping.h
> > > @@ -121,7 +121,9 @@ static inline unsigned long dma_max_pfn(struct device
> > > *dev) }
> > > #define dma_max_pfn(dev) dma_max_pfn(dev)
> > >
> > > -static inline void arch_setup_dma_ops(struct device *dev, bool coherent)
> > > +static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
> > > + u64 size, struct iommu_dma_mapping *iommu,
> > > + bool coherent)
> > > {
> > > if (coherent)
> > > set_dma_ops(dev, &arm_coherent_dma_ops);
> > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > index 946dd7ae0394..95ebd38db545 100644
> > > --- a/drivers/of/platform.c
> > > +++ b/drivers/of/platform.c
> > > @@ -19,6 +19,7 @@
> > > #include <linux/slab.h>
> > > #include <linux/of_address.h>
> > > #include <linux/of_device.h>
> > > +#include <linux/of_iommu.h>
> > > #include <linux/of_irq.h>
> > > #include <linux/of_platform.h>
> > > #include <linux/platform_device.h>
> > > @@ -166,6 +167,7 @@ static void of_dma_configure(struct platform_device
> > > *pdev) int ret;
> > > bool coherent;
> > > unsigned long offset;
> > > + struct iommu_dma_mapping *iommu;
> > > struct device *dev = &pdev->dev;
> > >
> > > /*
> > > @@ -195,7 +197,19 @@ static void of_dma_configure(struct platform_device
> > > *pdev) dev_dbg(dev, "device is%sdma coherent\n",
> > > coherent ? " " : " not ");
> > >
> > > - arch_setup_dma_ops(dev, coherent);
> > > + iommu = of_iommu_configure(dev);
> > > + dev_dbg(dev, "device is%sbehind an iommu\n",
> > > + iommu ? " " : " not ");
> > > +
> > > + arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
> > > +
> > > + if (iommu)
> > > + kref_put(&iommu->kref, of_iommu_deconfigure);
> >
> > What's the expected life cycle of the iommu_dma_mapping structure ? It gets
> > created by of_iommu_configure() and the initial reference gets dropped
> > here. I
> > suppose you expect arch code to need to keep a reference to the structure,
> > but
> > your implementation in patch 7/7 doesn't. As far as I can see, you don't
> > even
> > use the contents of the structure in the ARM arch_setup_dma_ops()
> > implementation. Do you expect that to change later, or other architectures
> > to
> > need it ?
> >
> > By the way, now that I think about it, I find struct iommu_dma_mapping and
> > struct dma_iommu_mapping very confusing.
>
> Agreed. I wonder how useful it is to know the set of IOMMU instances
> that each device can master through. Wouldn't it be more useful to keep
> a list of master interfaces for each device? The set of IOMMU instances
> can trivially be derived from that.
I'm struggling to think how that would look. What do you mean by `master
interfaces' in terms of the code we have in Linux? At the end of the day,
the list of IOMMU instances (i.e. iommu_dma_mapping) exists because you
and Laurent have use-cases involving devices mastering through multiple
IOMMUs. If it doesn't work for you, it might be best for you to send me
the patch ;)
Will
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
