| Message ID | 1410539695-29128-6-git-send-email-will.deacon@arm.com |
|---|---|
| State | New |
| Headers | show |
Hi Will, Thank you for the patch. 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.deacon@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. > +} > + > +static void of_dma_deconfigure(struct platform_device *pdev) > +{ > + arch_teardown_dma_ops(&pdev->dev); > } > > /** > @@ -224,16 +238,12 @@ static struct platform_device > *of_platform_device_create_pdata( if (!dev) > goto err_clear_flag; > > - of_dma_configure(dev); > dev->dev.bus = &platform_bus_type; > dev->dev.platform_data = platform_data; > - > - /* We do not fill the DMA ops for platform devices by default. > - * This is currently the responsibility of the platform code > - * to do such, possibly using a device notifier > - */ > + of_dma_configure(dev); > > if (of_device_add(dev) != 0) { > + of_dma_deconfigure(dev); > platform_device_put(dev); > goto err_clear_flag; > } > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h > index e60e52d82db9..47e1ac30e300 100644 > --- a/include/linux/dma-mapping.h > +++ b/include/linux/dma-mapping.h > @@ -138,7 +138,13 @@ static inline int dma_coerce_mask_and_coherent(struct > device *dev, u64 mask) extern u64 dma_get_required_mask(struct device > *dev); > > #ifndef arch_setup_dma_ops > -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) { } > +#endif > + > +#ifndef arch_teardown_dma_ops > +static inline void arch_teardown_dma_ops(struct device *dev) { } > #endif > > static inline unsigned int dma_get_max_seg_size(struct device *dev)
On Thu, Sep 18, 2014 at 02:17:33PM +0300, Laurent Pinchart wrote: > Hi Will, > > Thank you for the patch. > > 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.deacon@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. Thierry
On Thu, Sep 18, 2014 at 12:17:33PM +0100, Laurent Pinchart wrote: > Hi Will, Hello again, > On Friday 12 September 2014 17:34:53 Will Deacon wrote: > > @@ -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 ? Indeed, I've not done anything to the ARM dma-mapping ops other than plug-in the existing code, which doesn't use these new features. I think Marek was going to look at that. > By the way, now that I think about it, I find struct iommu_dma_mapping and > struct dma_iommu_mapping very confusing. Yup; I'd like to see some generic code that uses the per-IOMMU-instance domain and allocator which is passed to arch_setup_dma_ops. Then we could simply move arch/arm/ (and arm64) over to that, which would get rid of dma_iommu_mapping entirely. Will
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.deacon@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
Hi Will, On Monday 22 September 2014 18:50:27 Will Deacon wrote: > 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.deacon@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 ;) Just for the record, I've brought up the topic of masters being served by multiple IOMMUs, but don't have a use case for it (yet at least). I do have masters served through multiple streams with separate stream IDs, but all by the same IOMMU.
On Tue, Oct 14, 2014 at 01:53:59PM +0100, Laurent Pinchart wrote: > Hi Will, Hi Laurent, > On Monday 22 September 2014 18:50:27 Will Deacon wrote: > > On Mon, Sep 22, 2014 at 10:29:10AM +0100, Thierry Reding wrote: > > > 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 ;) > > Just for the record, I've brought up the topic of masters being served by > multiple IOMMUs, but don't have a use case for it (yet at least). I do have > masters served through multiple streams with separate stream IDs, but all by > the same IOMMU. Ok. I spoke to Arnd, David and Joerg at LPC and the consensus was that the DMA-mapping API should *not* be exposed to the details of masters that master through multiple IOMMUs. Instead, that should be abstracted by the device API by exposing that device as a single struct device. So, that's certainly an area that needs more work and I'll drop the limited support I'd cooked up from this patch set in the next version. Will
Hello, On 2014-10-27 11:51, Will Deacon wrote: > On Tue, Oct 14, 2014 at 01:53:59PM +0100, Laurent Pinchart wrote: >> On Monday 22 September 2014 18:50:27 Will Deacon wrote: >>> On Mon, Sep 22, 2014 at 10:29:10AM +0100, Thierry Reding wrote: >>>> 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 ;) >> Just for the record, I've brought up the topic of masters being served by >> multiple IOMMUs, but don't have a use case for it (yet at least). I do have >> masters served through multiple streams with separate stream IDs, but all by >> the same IOMMU. > Ok. I spoke to Arnd, David and Joerg at LPC and the consensus was that the > DMA-mapping API should *not* be exposed to the details of masters that > master through multiple IOMMUs. Instead, that should be abstracted by the > device API by exposing that device as a single struct device. > > So, that's certainly an area that needs more work and I'll drop the limited > support I'd cooked up from this patch set in the next version. Great! That's more or less something I've already implemented on top of your previous patchset, as I didn't have any good idea how to manage multiple masters separately. I'm waiting for your next update and I will rebase my patches soon. Best regards
Hi Will, On Monday 27 October 2014 10:51:59 Will Deacon wrote: > On Tue, Oct 14, 2014 at 01:53:59PM +0100, Laurent Pinchart wrote: > > On Monday 22 September 2014 18:50:27 Will Deacon wrote: > > > On Mon, Sep 22, 2014 at 10:29:10AM +0100, Thierry Reding wrote: > > > > 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 ;) > > > > Just for the record, I've brought up the topic of masters being served by > > multiple IOMMUs, but don't have a use case for it (yet at least). I do > > have masters served through multiple streams with separate stream IDs, but > > all by the same IOMMU. > > Ok. I spoke to Arnd, David and Joerg at LPC and the consensus was that the > DMA-mapping API should *not* be exposed to the details of masters that > master through multiple IOMMUs. Instead, that should be abstracted by the > device API by exposing that device as a single struct device. I'm not sure to follow you here. Aren't we already exposing masters that master through multiple IOMMUs as single instances of struct device ? > So, that's certainly an area that needs more work and I'll drop the limited > support I'd cooked up from this patch set in the next version. How about masters connected to multiple stream IDs of the same IOMMU ?
On Mon, Oct 27, 2014 at 11:30:33AM +0000, Laurent Pinchart wrote: > Hi Will, Hey Laurent, > On Monday 27 October 2014 10:51:59 Will Deacon wrote: > > On Tue, Oct 14, 2014 at 01:53:59PM +0100, Laurent Pinchart wrote: > > > On Monday 22 September 2014 18:50:27 Will Deacon wrote: > > > > On Mon, Sep 22, 2014 at 10:29:10AM +0100, Thierry Reding wrote: > > > > > 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 ;) > > > > > > Just for the record, I've brought up the topic of masters being served by > > > multiple IOMMUs, but don't have a use case for it (yet at least). I do > > > have masters served through multiple streams with separate stream IDs, but > > > all by the same IOMMU. > > > > Ok. I spoke to Arnd, David and Joerg at LPC and the consensus was that the > > DMA-mapping API should *not* be exposed to the details of masters that > > master through multiple IOMMUs. Instead, that should be abstracted by the > > device API by exposing that device as a single struct device. > > I'm not sure to follow you here. Aren't we already exposing masters that > master through multiple IOMMUs as single instances of struct device ? Hmm, yes, now you've confused me too! The conclusion was certainly that dma-mapping should not be the one dealing with the I/O topology. Domain allocation would then be an iommu callback (something like ->get_default_domain), but the rest of the details weren't fleshed out. Joerg? > > So, that's certainly an area that needs more work and I'll drop the limited > > support I'd cooked up from this patch set in the next version. > > How about masters connected to multiple stream IDs of the same IOMMU ? That should still be handled, as I believe that will be a common case. Will
On Mon, Oct 27, 2014 at 04:02:16PM +0000, Will Deacon wrote: > On Mon, Oct 27, 2014 at 11:30:33AM +0000, Laurent Pinchart wrote: > > I'm not sure to follow you here. Aren't we already exposing masters that > > master through multiple IOMMUs as single instances of struct device ? > > Hmm, yes, now you've confused me too! The conclusion was certainly that > dma-mapping should not be the one dealing with the I/O topology. Domain > allocation would then be an iommu callback (something like > ->get_default_domain), but the rest of the details weren't fleshed out. The idea is that the IOMMU core code will allocate a default domain for each iommu-group at initialization time. This domain can be requested later by a new iommu-api function and used for DMA-API mappings. A device still can be assigned to another domain by driver code (like VFIO). But if the device is later de-assigned the IOMMU core-code automatically puts it back into the default domain. Joerg
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); +} + +static void of_dma_deconfigure(struct platform_device *pdev) +{ + arch_teardown_dma_ops(&pdev->dev); } /** @@ -224,16 +238,12 @@ static struct platform_device *of_platform_device_create_pdata( if (!dev) goto err_clear_flag; - of_dma_configure(dev); dev->dev.bus = &platform_bus_type; dev->dev.platform_data = platform_data; - - /* We do not fill the DMA ops for platform devices by default. - * This is currently the responsibility of the platform code - * to do such, possibly using a device notifier - */ + of_dma_configure(dev); if (of_device_add(dev) != 0) { + of_dma_deconfigure(dev); platform_device_put(dev); goto err_clear_flag; } diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index e60e52d82db9..47e1ac30e300 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -138,7 +138,13 @@ static inline int dma_coerce_mask_and_coherent(struct device *dev, u64 mask) extern u64 dma_get_required_mask(struct device *dev); #ifndef arch_setup_dma_ops -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) { } +#endif + +#ifndef arch_teardown_dma_ops +static inline void arch_teardown_dma_ops(struct device *dev) { } #endif static inline unsigned int dma_get_max_seg_size(struct device *dev)
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.deacon@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(-)