| Message ID | 1398353407-2345-5-git-send-email-santosh.shilimkar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 24 Apr 2014 11:30:04 -0400, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > Retrieve DMA configuration from DT and setup platform device's DMA > parameters. The DMA configuration in DT has to be specified using > "dma-ranges" and "dma-coherent" properties if supported. > > We setup dma_pfn_offset using "dma-ranges" and dma_coherent_ops > using "dma-coherent" device tree properties. > > The set_arch_dma_coherent_ops macro has to be defined by arch if > it supports coherent dma_ops. Otherwise, set_arch_dma_coherent_ops() is > declared as nop. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Olof Johansson <olof@lixom.net> > Cc: Grant Likely <grant.likely@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > --- > drivers/of/platform.c | 48 ++++++++++++++++++++++++++++++++++++++++--- > include/linux/dma-mapping.h | 7 +++++++ > 2 files changed, 52 insertions(+), 3 deletions(-) > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index 48de98f..270c0b9 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -187,6 +187,50 @@ struct platform_device *of_device_alloc(struct device_node *np, > EXPORT_SYMBOL(of_device_alloc); > > /** > + * of_dma_configure - Setup DMA configuration > + * @dev: Device to apply DMA configuration > + * > + * Try to get devices's DMA configuration from DT and update it > + * accordingly. > + * > + * In case if platform code need to use own special DMA configuration,it > + * can use Platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE event > + * to fix up DMA configuration. > + */ > +static void of_dma_configure(struct device *dev) > +{ > + u64 dma_addr, paddr, size; > + int ret; > + > + dev->coherent_dma_mask = DMA_BIT_MASK(32); > + if (!dev->dma_mask) > + dev->dma_mask = &dev->coherent_dma_mask; > + > + /* > + * if dma-coherent property exist, call arch hook to setup > + * dma coherent operations. > + */ > + if (of_dma_is_coherent(dev->of_node)) { > + set_arch_dma_coherent_ops(dev); > + dev_dbg(dev, "device is dma coherent\n"); > + } > + > + /* > + * if dma-ranges property doesn't exist - just return else > + * setup the dma offset > + */ > + ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size); > + if ((ret == -ENODEV) || (ret < 0)) { > + dev_dbg(dev, "no dma range information to setup\n"); > + return; > + } > + > + /* DMA ranges found. Calculate and set dma_pfn_offset */ > + dev->dma_pfn_offset = PFN_DOWN(paddr - dma_addr); > + dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset); I've got two concerns here. of_dma_get_range() retrieves only the first tuple from the dma-ranges property, but it is perfectly valid for dma-ranges to contain multiple tuples. How should we handle it if a device has multiple ranges it can DMA from? Second, while the pfn offset is being determined, I don't see anything making use of either the base address or size. How is the device constrained to only getting DMA buffers from within that range? Is the driver expected to manage that directly? g. > +} > + > +/** > * of_platform_device_create_pdata - Alloc, initialize and register an of_device > * @np: pointer to node to create device for > * @bus_id: name to assign device > @@ -214,9 +258,7 @@ static struct platform_device *of_platform_device_create_pdata( > #if defined(CONFIG_MICROBLAZE) > dev->archdata.dma_mask = 0xffffffffUL; > #endif > - dev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > - if (!dev->dev.dma_mask) > - dev->dev.dma_mask = &dev->dev.coherent_dma_mask; > + of_dma_configure(&dev->dev); > dev->dev.bus = &platform_bus_type; > dev->dev.platform_data = platform_data; > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h > index fd4aee2..c7d9b1b 100644 > --- a/include/linux/dma-mapping.h > +++ b/include/linux/dma-mapping.h > @@ -123,6 +123,13 @@ static inline int dma_coerce_mask_and_coherent(struct device *dev, u64 mask) > > extern u64 dma_get_required_mask(struct device *dev); > > +#ifndef set_arch_dma_coherent_ops > +static inline int set_arch_dma_coherent_ops(struct device *dev) > +{ > + return 0; > +} > +#endif > + > static inline unsigned int dma_get_max_seg_size(struct device *dev) > { > return dev->dma_parms ? dev->dma_parms->max_segment_size : 65536; > -- > 1.7.9.5 >
Hi Grant, On Tuesday 29 April 2014 10:41 AM, Grant Likely wrote: > On Thu, 24 Apr 2014 11:30:04 -0400, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: >> Retrieve DMA configuration from DT and setup platform device's DMA >> parameters. The DMA configuration in DT has to be specified using >> "dma-ranges" and "dma-coherent" properties if supported. >> >> We setup dma_pfn_offset using "dma-ranges" and dma_coherent_ops >> using "dma-coherent" device tree properties. >> >> The set_arch_dma_coherent_ops macro has to be defined by arch if >> it supports coherent dma_ops. Otherwise, set_arch_dma_coherent_ops() is >> declared as nop. >> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: Russell King <linux@arm.linux.org.uk> >> Cc: Arnd Bergmann <arnd@arndb.de> >> Cc: Olof Johansson <olof@lixom.net> >> Cc: Grant Likely <grant.likely@linaro.org> >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Linus Walleij <linus.walleij@linaro.org> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >> --- >> drivers/of/platform.c | 48 ++++++++++++++++++++++++++++++++++++++++--- >> include/linux/dma-mapping.h | 7 +++++++ >> 2 files changed, 52 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c >> index 48de98f..270c0b9 100644 >> --- a/drivers/of/platform.c >> +++ b/drivers/of/platform.c >> @@ -187,6 +187,50 @@ struct platform_device *of_device_alloc(struct device_node *np, >> EXPORT_SYMBOL(of_device_alloc); >> >> /** >> + * of_dma_configure - Setup DMA configuration >> + * @dev: Device to apply DMA configuration >> + * >> + * Try to get devices's DMA configuration from DT and update it >> + * accordingly. >> + * >> + * In case if platform code need to use own special DMA configuration,it >> + * can use Platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE event >> + * to fix up DMA configuration. >> + */ >> +static void of_dma_configure(struct device *dev) >> +{ >> + u64 dma_addr, paddr, size; >> + int ret; >> + >> + dev->coherent_dma_mask = DMA_BIT_MASK(32); >> + if (!dev->dma_mask) >> + dev->dma_mask = &dev->coherent_dma_mask; >> + >> + /* >> + * if dma-coherent property exist, call arch hook to setup >> + * dma coherent operations. >> + */ >> + if (of_dma_is_coherent(dev->of_node)) { >> + set_arch_dma_coherent_ops(dev); >> + dev_dbg(dev, "device is dma coherent\n"); >> + } >> + >> + /* >> + * if dma-ranges property doesn't exist - just return else >> + * setup the dma offset >> + */ >> + ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size); >> + if ((ret == -ENODEV) || (ret < 0)) { >> + dev_dbg(dev, "no dma range information to setup\n"); >> + return; >> + } >> + >> + /* DMA ranges found. Calculate and set dma_pfn_offset */ >> + dev->dma_pfn_offset = PFN_DOWN(paddr - dma_addr); >> + dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset); > > I've got two concerns here. of_dma_get_range() retrieves only the first > tuple from the dma-ranges property, but it is perfectly valid for > dma-ranges to contain multiple tuples. How should we handle it if a > device has multiple ranges it can DMA from? > We've not found any cases in current Linux where more than one dma-ranges would be used. Moreover, The MM (definitely for ARM) isn't supported such cases at all (if i understand everything right). - there are only one arm_dma_pfn_limit - there is only one MM zone is used for ARM - some arches like x86,mips can support 2 zones (per arch - not per device or bus) DMA & DMA32, but they configured once and forever per arch. Example: static void *loongson_dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle, gfp_t gfp, struct dma_attrs *attrs) { ... /* ignore region specifiers */ gfp &= ~(__GFP_DMA | __GFP_DMA32 | __GFP_HIGHMEM); #ifdef CONFIG_ISA if (dev == NULL) gfp |= __GFP_DMA; else #endif #ifdef CONFIG_ZONE_DMA if (dev->coherent_dma_mask < DMA_BIT_MASK(32)) gfp |= __GFP_DMA; else #endif #ifdef CONFIG_ZONE_DMA32 if (dev->coherent_dma_mask < DMA_BIT_MASK(40)) gfp |= __GFP_DMA32; else #endif ... } Any ways, it can be added later if we have an usecase for that. > Second, while the pfn offset is being determined, I don't see anything > making use of either the base address or size. How is the device > constrained to only getting DMA buffers from within that range? Is the > driver expected to manage that directly? > Drivers don't have to do anything special apart from setting the correct mask. The pfn_offset case, we use DMA_ZONE which takes care of masks already. Size is suppose to be used for dma_mask setup which we discussed in previous threads. Regards, Santosh
On Wed, 30 Apr 2014 10:19:15 -0400, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > Hi Grant, > > On Tuesday 29 April 2014 10:41 AM, Grant Likely wrote: > > On Thu, 24 Apr 2014 11:30:04 -0400, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > >> Retrieve DMA configuration from DT and setup platform device's DMA > >> parameters. The DMA configuration in DT has to be specified using > >> "dma-ranges" and "dma-coherent" properties if supported. > >> > >> We setup dma_pfn_offset using "dma-ranges" and dma_coherent_ops > >> using "dma-coherent" device tree properties. > >> > >> The set_arch_dma_coherent_ops macro has to be defined by arch if > >> it supports coherent dma_ops. Otherwise, set_arch_dma_coherent_ops() is > >> declared as nop. > >> > >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > >> Cc: Russell King <linux@arm.linux.org.uk> > >> Cc: Arnd Bergmann <arnd@arndb.de> > >> Cc: Olof Johansson <olof@lixom.net> > >> Cc: Grant Likely <grant.likely@linaro.org> > >> Cc: Rob Herring <robh+dt@kernel.org> > >> Cc: Catalin Marinas <catalin.marinas@arm.com> > >> Cc: Linus Walleij <linus.walleij@linaro.org> > >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > >> --- > >> drivers/of/platform.c | 48 ++++++++++++++++++++++++++++++++++++++++--- > >> include/linux/dma-mapping.h | 7 +++++++ > >> 2 files changed, 52 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c > >> index 48de98f..270c0b9 100644 > >> --- a/drivers/of/platform.c > >> +++ b/drivers/of/platform.c > >> @@ -187,6 +187,50 @@ struct platform_device *of_device_alloc(struct device_node *np, > >> EXPORT_SYMBOL(of_device_alloc); > >> > >> /** > >> + * of_dma_configure - Setup DMA configuration > >> + * @dev: Device to apply DMA configuration > >> + * > >> + * Try to get devices's DMA configuration from DT and update it > >> + * accordingly. > >> + * > >> + * In case if platform code need to use own special DMA configuration,it > >> + * can use Platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE event > >> + * to fix up DMA configuration. > >> + */ > >> +static void of_dma_configure(struct device *dev) > >> +{ > >> + u64 dma_addr, paddr, size; > >> + int ret; > >> + > >> + dev->coherent_dma_mask = DMA_BIT_MASK(32); > >> + if (!dev->dma_mask) > >> + dev->dma_mask = &dev->coherent_dma_mask; > >> + > >> + /* > >> + * if dma-coherent property exist, call arch hook to setup > >> + * dma coherent operations. > >> + */ > >> + if (of_dma_is_coherent(dev->of_node)) { > >> + set_arch_dma_coherent_ops(dev); > >> + dev_dbg(dev, "device is dma coherent\n"); > >> + } > >> + > >> + /* > >> + * if dma-ranges property doesn't exist - just return else > >> + * setup the dma offset > >> + */ > >> + ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size); > >> + if ((ret == -ENODEV) || (ret < 0)) { > >> + dev_dbg(dev, "no dma range information to setup\n"); > >> + return; > >> + } > >> + > >> + /* DMA ranges found. Calculate and set dma_pfn_offset */ > >> + dev->dma_pfn_offset = PFN_DOWN(paddr - dma_addr); > >> + dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset); > > > > I've got two concerns here. of_dma_get_range() retrieves only the first > > tuple from the dma-ranges property, but it is perfectly valid for > > dma-ranges to contain multiple tuples. How should we handle it if a > > device has multiple ranges it can DMA from? > > > > We've not found any cases in current Linux where more than one dma-ranges > would be used. Moreover, The MM (definitely for ARM) isn't supported such > cases at all (if i understand everything right). > - there are only one arm_dma_pfn_limit > - there is only one MM zone is used for ARM > - some arches like x86,mips can support 2 zones (per arch - not per device or bus) > DMA & DMA32, but they configured once and forever per arch. Okay. If anyone ever does implement multiple ranges then this code will need to be revisited. > > Second, while the pfn offset is being determined, I don't see anything > > making use of either the base address or size. How is the device > > constrained to only getting DMA buffers from within that range? Is the > > driver expected to manage that directly? > > > Drivers don't have to do anything special apart from setting > the correct mask. The pfn_offset case, we use DMA_ZONE which takes > care of masks already. Size is suppose to be used for dma_mask > setup which we discussed in previous threads. okay. g.
On Thursday 01 May 2014 09:12 AM, Grant Likely wrote: > On Wed, 30 Apr 2014 10:19:15 -0400, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: >> Hi Grant, >> >> On Tuesday 29 April 2014 10:41 AM, Grant Likely wrote: >>> On Thu, 24 Apr 2014 11:30:04 -0400, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: >>>> Retrieve DMA configuration from DT and setup platform device's DMA >>>> parameters. The DMA configuration in DT has to be specified using >>>> "dma-ranges" and "dma-coherent" properties if supported. >>>> >>>> We setup dma_pfn_offset using "dma-ranges" and dma_coherent_ops >>>> using "dma-coherent" device tree properties. >>>> >>>> The set_arch_dma_coherent_ops macro has to be defined by arch if >>>> it supports coherent dma_ops. Otherwise, set_arch_dma_coherent_ops() is >>>> declared as nop. >>>> >>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>>> Cc: Russell King <linux@arm.linux.org.uk> >>>> Cc: Arnd Bergmann <arnd@arndb.de> >>>> Cc: Olof Johansson <olof@lixom.net> >>>> Cc: Grant Likely <grant.likely@linaro.org> >>>> Cc: Rob Herring <robh+dt@kernel.org> >>>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>>> Cc: Linus Walleij <linus.walleij@linaro.org> >>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >>>> --- >>>> drivers/of/platform.c | 48 ++++++++++++++++++++++++++++++++++++++++--- >>>> include/linux/dma-mapping.h | 7 +++++++ >>>> 2 files changed, 52 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c >>>> index 48de98f..270c0b9 100644 >>>> --- a/drivers/of/platform.c >>>> +++ b/drivers/of/platform.c >>>> @@ -187,6 +187,50 @@ struct platform_device *of_device_alloc(struct device_node *np, >>>> EXPORT_SYMBOL(of_device_alloc); >>>> >>>> /** >>>> + * of_dma_configure - Setup DMA configuration >>>> + * @dev: Device to apply DMA configuration >>>> + * >>>> + * Try to get devices's DMA configuration from DT and update it >>>> + * accordingly. >>>> + * >>>> + * In case if platform code need to use own special DMA configuration,it >>>> + * can use Platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE event >>>> + * to fix up DMA configuration. >>>> + */ >>>> +static void of_dma_configure(struct device *dev) >>>> +{ >>>> + u64 dma_addr, paddr, size; >>>> + int ret; >>>> + >>>> + dev->coherent_dma_mask = DMA_BIT_MASK(32); >>>> + if (!dev->dma_mask) >>>> + dev->dma_mask = &dev->coherent_dma_mask; >>>> + >>>> + /* >>>> + * if dma-coherent property exist, call arch hook to setup >>>> + * dma coherent operations. >>>> + */ >>>> + if (of_dma_is_coherent(dev->of_node)) { >>>> + set_arch_dma_coherent_ops(dev); >>>> + dev_dbg(dev, "device is dma coherent\n"); >>>> + } >>>> + >>>> + /* >>>> + * if dma-ranges property doesn't exist - just return else >>>> + * setup the dma offset >>>> + */ >>>> + ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size); >>>> + if ((ret == -ENODEV) || (ret < 0)) { >>>> + dev_dbg(dev, "no dma range information to setup\n"); >>>> + return; >>>> + } >>>> + >>>> + /* DMA ranges found. Calculate and set dma_pfn_offset */ >>>> + dev->dma_pfn_offset = PFN_DOWN(paddr - dma_addr); >>>> + dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset); >>> >>> I've got two concerns here. of_dma_get_range() retrieves only the first >>> tuple from the dma-ranges property, but it is perfectly valid for >>> dma-ranges to contain multiple tuples. How should we handle it if a >>> device has multiple ranges it can DMA from? >>> >> >> We've not found any cases in current Linux where more than one dma-ranges >> would be used. Moreover, The MM (definitely for ARM) isn't supported such >> cases at all (if i understand everything right). >> - there are only one arm_dma_pfn_limit >> - there is only one MM zone is used for ARM >> - some arches like x86,mips can support 2 zones (per arch - not per device or bus) >> DMA & DMA32, but they configured once and forever per arch. > > Okay. If anyone ever does implement multiple ranges then this code will > need to be revisited. > Sure. Thanks for the review !! Regards, Santosh
On Thu, Apr 24, 2014 at 10:30 AM, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > Retrieve DMA configuration from DT and setup platform device's DMA > parameters. The DMA configuration in DT has to be specified using > "dma-ranges" and "dma-coherent" properties if supported. > > We setup dma_pfn_offset using "dma-ranges" and dma_coherent_ops > using "dma-coherent" device tree properties. > > The set_arch_dma_coherent_ops macro has to be defined by arch if > it supports coherent dma_ops. Otherwise, set_arch_dma_coherent_ops() is > declared as nop. > [...] > + /* > + * if dma-ranges property doesn't exist - just return else > + * setup the dma offset > + */ > + ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size); > + if ((ret == -ENODEV) || (ret < 0)) { The 1st condition is redundant. > + dev_dbg(dev, "no dma range information to setup\n"); > + return; > + } > + > + /* DMA ranges found. Calculate and set dma_pfn_offset */ > + dev->dma_pfn_offset = PFN_DOWN(paddr - dma_addr); > + dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset); > +} > + > +/** > * of_platform_device_create_pdata - Alloc, initialize and register an of_device > * @np: pointer to node to create device for > * @bus_id: name to assign device > @@ -214,9 +258,7 @@ static struct platform_device *of_platform_device_create_pdata( > #if defined(CONFIG_MICROBLAZE) > dev->archdata.dma_mask = 0xffffffffUL; > #endif This is also dma related setup. Please move *all* dma setup into the function. > - dev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > - if (!dev->dev.dma_mask) > - dev->dev.dma_mask = &dev->dev.coherent_dma_mask; > + of_dma_configure(&dev->dev); > dev->dev.bus = &platform_bus_type; > dev->dev.platform_data = platform_data; > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h > index fd4aee2..c7d9b1b 100644 > --- a/include/linux/dma-mapping.h > +++ b/include/linux/dma-mapping.h > @@ -123,6 +123,13 @@ static inline int dma_coerce_mask_and_coherent(struct device *dev, u64 mask) > > extern u64 dma_get_required_mask(struct device *dev); > > +#ifndef set_arch_dma_coherent_ops > +static inline int set_arch_dma_coherent_ops(struct device *dev) > +{ > + return 0; > +} > +#endif > + > static inline unsigned int dma_get_max_seg_size(struct device *dev) > { > return dev->dma_parms ? dev->dma_parms->max_segment_size : 65536; > -- > 1.7.9.5 >
On Thursday 01 May 2014 14:12:10 Grant Likely wrote: > > > I've got two concerns here. of_dma_get_range() retrieves only the first > > > tuple from the dma-ranges property, but it is perfectly valid for > > > dma-ranges to contain multiple tuples. How should we handle it if a > > > device has multiple ranges it can DMA from? > > > > > > > We've not found any cases in current Linux where more than one dma-ranges > > would be used. Moreover, The MM (definitely for ARM) isn't supported such > > cases at all (if i understand everything right). > > - there are only one arm_dma_pfn_limit > > - there is only one MM zone is used for ARM > > - some arches like x86,mips can support 2 zones (per arch - not per device or bus) > > DMA & DMA32, but they configured once and forever per arch. > > Okay. If anyone ever does implement multiple ranges then this code will > need to be revisited. I wonder if it's needed for platforms implementing the standard "ARM memory map" [1]. The document only talks about addresses as seen from the CPU, and I can see two logical interpretations how the RAM is supposed to be visible from a device: either all RAM would be visible contiguously at DMA address zero, or everything would be visible at the same physical address as the CPU sees it. If anyone picks the first interpretation, we will have to implement that in Linux. We can of course hope that all hardware designs follow the second interpretation, which would be more convenient for us here. Arnd [1] http://infocenter.arm.com/help/topic/com.arm.doc.den0001c/DEN0001C_principles_of_arm_memory_maps.pdf
On Friday 02 May 2014 05:58 AM, Arnd Bergmann wrote: > On Thursday 01 May 2014 14:12:10 Grant Likely wrote: >>>> I've got two concerns here. of_dma_get_range() retrieves only the first >>>> tuple from the dma-ranges property, but it is perfectly valid for >>>> dma-ranges to contain multiple tuples. How should we handle it if a >>>> device has multiple ranges it can DMA from? >>>> >>> >>> We've not found any cases in current Linux where more than one dma-ranges >>> would be used. Moreover, The MM (definitely for ARM) isn't supported such >>> cases at all (if i understand everything right). >>> - there are only one arm_dma_pfn_limit >>> - there is only one MM zone is used for ARM >>> - some arches like x86,mips can support 2 zones (per arch - not per device or bus) >>> DMA & DMA32, but they configured once and forever per arch. >> >> Okay. If anyone ever does implement multiple ranges then this code will >> need to be revisited. > > I wonder if it's needed for platforms implementing the standard "ARM memory map" [1]. > The document only talks about addresses as seen from the CPU, and I can see > two logical interpretations how the RAM is supposed to be visible from a device: > either all RAM would be visible contiguously at DMA address zero, or everything > would be visible at the same physical address as the CPU sees it. > > If anyone picks the first interpretation, we will have to implement that > in Linux. We can of course hope that all hardware designs follow the second > interpretation, which would be more convenient for us here. > not sure if I got your point correctly but DMA address 0 isn't used as DRAM start in any of the ARM SOC today, mainly because of the boot architecture where address 0 is typically used by ROM code. RAM start will be at some offset always and hence I believe ARM SOCs will follow second interpretation. This was one of the main reason we ended up fixing the max*pfn stuff. 26ba47b {ARM: 7805/1: mm: change max*pfn to include the physical offset of memory} > > [1] http://infocenter.arm.com/help/topic/com.arm.doc.den0001c/DEN0001C_principles_of_arm_memory_maps.pdf > Regards, Santosh
On Friday 02 May 2014 09:13:48 Santosh Shilimkar wrote: > On Friday 02 May 2014 05:58 AM, Arnd Bergmann wrote: > > On Thursday 01 May 2014 14:12:10 Grant Likely wrote: > >>>> I've got two concerns here. of_dma_get_range() retrieves only the first > >>>> tuple from the dma-ranges property, but it is perfectly valid for > >>>> dma-ranges to contain multiple tuples. How should we handle it if a > >>>> device has multiple ranges it can DMA from? > >>>> > >>> > >>> We've not found any cases in current Linux where more than one dma-ranges > >>> would be used. Moreover, The MM (definitely for ARM) isn't supported such > >>> cases at all (if i understand everything right). > >>> - there are only one arm_dma_pfn_limit > >>> - there is only one MM zone is used for ARM > >>> - some arches like x86,mips can support 2 zones (per arch - not per device or bus) > >>> DMA & DMA32, but they configured once and forever per arch. > >> > >> Okay. If anyone ever does implement multiple ranges then this code will > >> need to be revisited. > > > > I wonder if it's needed for platforms implementing the standard "ARM memory map" [1]. > > The document only talks about addresses as seen from the CPU, and I can see > > two logical interpretations how the RAM is supposed to be visible from a device: > > either all RAM would be visible contiguously at DMA address zero, or everything > > would be visible at the same physical address as the CPU sees it. > > > > If anyone picks the first interpretation, we will have to implement that > > in Linux. We can of course hope that all hardware designs follow the second > > interpretation, which would be more convenient for us here. > > > not sure if I got your point correctly but DMA address 0 isn't used as DRAM start in > any of the ARM SOC today, mainly because of the boot architecture where address 0 is > typically used by ROM code. RAM start will be at some offset always and hence I > believe ARM SOCs will follow second interpretation. This was one of the main reason > we ended up fixing the max*pfn stuff. > 26ba47b {ARM: 7805/1: mm: change max*pfn to include the physical offset of memory} Marvell normally has memory starting at physical address zero. Even if RAM starts elsewhere, I don't think that is a reason to have the DMA address do the same. The memory controller internally obviously starts at zero, and it wouldn't be unreasonable to have the DMA space match what the memory controller sees rather than have it match what the CPU sees. If you look at the table 3.1.4, you have both addresses listed: Physical Addresses in SoC Offset Internal DRAM address 2 GBytes 0x00 8000 0000 – -0x00 8000 0000 0x00 0000 0000 – 0x00 FFFF FFFF 0x00 7FFF FFFF 30 GBytes 0x08 8000 0000 – -0x08 0000 0000 0x00 8000 0000 – 0x0F FFFF FFFF 0x07 FFFF FFFF 32 GBytes 0x88 0000 0000 - -0x80 0000 0000 0x08 0000 0000 - 0x8F FFFF FFFF 0x0F FFFF FFFF The wording "Physical Addresses in SoC" would indeed suggest that the same address is used for DMA, but I would trust everybody to do that. Arnd
On Thu, Apr 24, 2014 at 11:30:04AM -0400, Santosh Shilimkar wrote: > Retrieve DMA configuration from DT and setup platform device's DMA > parameters. The DMA configuration in DT has to be specified using > "dma-ranges" and "dma-coherent" properties if supported. > > We setup dma_pfn_offset using "dma-ranges" and dma_coherent_ops > using "dma-coherent" device tree properties. > > The set_arch_dma_coherent_ops macro has to be defined by arch if > it supports coherent dma_ops. Otherwise, set_arch_dma_coherent_ops() is > declared as nop. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Olof Johansson <olof@lixom.net> > Cc: Grant Likely <grant.likely@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > --- > drivers/of/platform.c | 48 ++++++++++++++++++++++++++++++++++++++++--- > include/linux/dma-mapping.h | 7 +++++++ > 2 files changed, 52 insertions(+), 3 deletions(-) > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index 48de98f..270c0b9 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -187,6 +187,50 @@ struct platform_device *of_device_alloc(struct device_node *np, > EXPORT_SYMBOL(of_device_alloc); > > /** > + * of_dma_configure - Setup DMA configuration > + * @dev: Device to apply DMA configuration > + * > + * Try to get devices's DMA configuration from DT and update it > + * accordingly. > + * > + * In case if platform code need to use own special DMA configuration,it > + * can use Platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE event > + * to fix up DMA configuration. > + */ > +static void of_dma_configure(struct device *dev) > +{ > + u64 dma_addr, paddr, size; > + int ret; > + > + dev->coherent_dma_mask = DMA_BIT_MASK(32); > + if (!dev->dma_mask) > + dev->dma_mask = &dev->coherent_dma_mask; > + > + /* > + * if dma-coherent property exist, call arch hook to setup > + * dma coherent operations. > + */ > + if (of_dma_is_coherent(dev->of_node)) { > + set_arch_dma_coherent_ops(dev); > + dev_dbg(dev, "device is dma coherent\n"); > + } > + > + /* > + * if dma-ranges property doesn't exist - just return else > + * setup the dma offset > + */ > + ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size); > + if ((ret == -ENODEV) || (ret < 0)) { > + dev_dbg(dev, "no dma range information to setup\n"); > + return; > + } > + > + /* DMA ranges found. Calculate and set dma_pfn_offset */ > + dev->dma_pfn_offset = PFN_DOWN(paddr - dma_addr); > + dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset); Is this effectively the same as an IOMMU that applies a constant offset to the bus address? Could or should this be done by adding a simple IOMMU driver instead of adding dma_pfn_offset to struct device? If we had both dma-ranges (and we set dma_pfn_offset as you do here) and an IOMMU, how would the combination work? If the IOMMU driver managed dma_pfn_offset internally, it seems like we'd have two entities dealing with it. If the IOMMU driver doesn't use dma_pfn_offset, it seems like it would be exposing a weird intermediate address space that's not usable by either CPU or device.
On Friday 02 May 2014 10:54:59 Bjorn Helgaas wrote: > > +static void of_dma_configure(struct device *dev) > > +{ > > + u64 dma_addr, paddr, size; > > + int ret; > > + > > + dev->coherent_dma_mask = DMA_BIT_MASK(32); > > + if (!dev->dma_mask) > > + dev->dma_mask = &dev->coherent_dma_mask; > > + > > + /* > > + * if dma-coherent property exist, call arch hook to setup > > + * dma coherent operations. > > + */ > > + if (of_dma_is_coherent(dev->of_node)) { > > + set_arch_dma_coherent_ops(dev); > > + dev_dbg(dev, "device is dma coherent\n"); > > + } > > + > > + /* > > + * if dma-ranges property doesn't exist - just return else > > + * setup the dma offset > > + */ > > + ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size); > > + if ((ret == -ENODEV) || (ret < 0)) { > > + dev_dbg(dev, "no dma range information to setup\n"); > > + return; > > + } > > + > > + /* DMA ranges found. Calculate and set dma_pfn_offset */ > > + dev->dma_pfn_offset = PFN_DOWN(paddr - dma_addr); > > + dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset); > > Is this effectively the same as an IOMMU that applies a constant offset > to the bus address? Could or should this be done by adding a simple IOMMU > driver instead of adding dma_pfn_offset to struct device? We currently have two dma_map_ops variants on ARM (plus another set for coherent/noncoherent differences, but we can ignore that for the sake of this discussion): one that handles linear mappings and one that handles IOMMUs by calling into the linux/iommu.h APIs. I guess what you mean by 'a simple IOMMU driver' would be another dma_map_ops implementation that is separate from real IOMMUs, right? That could certainly be done, but in effect it is almost the same as the linear mapping we already have. > If we had both dma-ranges (and we set dma_pfn_offset as you do here) and an > IOMMU, how would the combination work? If the IOMMU driver managed > dma_pfn_offset internally, it seems like we'd have two entities dealing > with it. If the IOMMU driver doesn't use dma_pfn_offset, it seems like > it would be exposing a weird intermediate address space that's not usable > by either CPU or device. The iommu dma_map_ops implementation does not need to worry about the dma_pfn_offset. We are still debating how to represent IOMMUs in DT at the moment, so it's not clear yet if we would consider a device node with both a dma-ranges property and an iommu reference as valid. What we will probably need is a way to represent the valid bus addresses that can be used for transfers from the DMA master through the IOMMU. This could be done through dma-ranges, or some other property, we will have to decide that soon. Arnd
[+cc Ben, Chris] On Fri, May 2, 2014 at 12:59 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Friday 02 May 2014 10:54:59 Bjorn Helgaas wrote: >> > +static void of_dma_configure(struct device *dev) >> > +{ >> > + u64 dma_addr, paddr, size; >> > + int ret; >> > + >> > + dev->coherent_dma_mask = DMA_BIT_MASK(32); >> > + if (!dev->dma_mask) >> > + dev->dma_mask = &dev->coherent_dma_mask; >> > + >> > + /* >> > + * if dma-coherent property exist, call arch hook to setup >> > + * dma coherent operations. >> > + */ >> > + if (of_dma_is_coherent(dev->of_node)) { >> > + set_arch_dma_coherent_ops(dev); >> > + dev_dbg(dev, "device is dma coherent\n"); >> > + } >> > + >> > + /* >> > + * if dma-ranges property doesn't exist - just return else >> > + * setup the dma offset >> > + */ >> > + ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size); >> > + if ((ret == -ENODEV) || (ret < 0)) { >> > + dev_dbg(dev, "no dma range information to setup\n"); >> > + return; >> > + } >> > + >> > + /* DMA ranges found. Calculate and set dma_pfn_offset */ >> > + dev->dma_pfn_offset = PFN_DOWN(paddr - dma_addr); >> > + dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset); >> >> Is this effectively the same as an IOMMU that applies a constant offset >> to the bus address? Could or should this be done by adding a simple IOMMU >> driver instead of adding dma_pfn_offset to struct device? > > We currently have two dma_map_ops variants on ARM (plus another set for > coherent/noncoherent differences, but we can ignore that for the sake > of this discussion): one that handles linear mappings and one that > handles IOMMUs by calling into the linux/iommu.h APIs. > > I guess what you mean by 'a simple IOMMU driver' would be another > dma_map_ops implementation that is separate from real IOMMUs, right? I suppose so; it seems like the offset could be managed inside arm_dma_ops. My idea of an IOMMU is something that maps bus addresses to physical memory addresses. That's what we're doing here; it's just that the mapping function is very simple. So why add something new in struct device for it? I think powerpc and tile do something similar in dma_direct_map_page() and tile_pci_dma_map_page() (they store the offset in struct dev_archdata). Bjorn
On Monday 05 May 2014 14:45:21 Bjorn Helgaas wrote: > [+cc Ben, Chris] > > On Fri, May 2, 2014 at 12:59 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Friday 02 May 2014 10:54:59 Bjorn Helgaas wrote: > >> > +static void of_dma_configure(struct device *dev) > >> > +{ > >> > + u64 dma_addr, paddr, size; > >> > + int ret; > >> > + > >> > + dev->coherent_dma_mask = DMA_BIT_MASK(32); > >> > + if (!dev->dma_mask) > >> > + dev->dma_mask = &dev->coherent_dma_mask; > >> > + > >> > + /* > >> > + * if dma-coherent property exist, call arch hook to setup > >> > + * dma coherent operations. > >> > + */ > >> > + if (of_dma_is_coherent(dev->of_node)) { > >> > + set_arch_dma_coherent_ops(dev); > >> > + dev_dbg(dev, "device is dma coherent\n"); > >> > + } > >> > + > >> > + /* > >> > + * if dma-ranges property doesn't exist - just return else > >> > + * setup the dma offset > >> > + */ > >> > + ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size); > >> > + if ((ret == -ENODEV) || (ret < 0)) { > >> > + dev_dbg(dev, "no dma range information to setup\n"); > >> > + return; > >> > + } > >> > + > >> > + /* DMA ranges found. Calculate and set dma_pfn_offset */ > >> > + dev->dma_pfn_offset = PFN_DOWN(paddr - dma_addr); > >> > + dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset); > >> > >> Is this effectively the same as an IOMMU that applies a constant offset > >> to the bus address? Could or should this be done by adding a simple IOMMU > >> driver instead of adding dma_pfn_offset to struct device? > > > > We currently have two dma_map_ops variants on ARM (plus another set for > > coherent/noncoherent differences, but we can ignore that for the sake > > of this discussion): one that handles linear mappings and one that > > handles IOMMUs by calling into the linux/iommu.h APIs. > > > > I guess what you mean by 'a simple IOMMU driver' would be another > > dma_map_ops implementation that is separate from real IOMMUs, right? > > I suppose so; it seems like the offset could be managed inside > arm_dma_ops. My idea of an IOMMU is something that maps bus addresses > to physical memory addresses. That's what we're doing here; it's just > that the mapping function is very simple. So why add something new in > struct device for it? > > I think powerpc and tile do something similar in dma_direct_map_page() > and tile_pci_dma_map_page() (they store the offset in struct > dev_archdata). Ah, so the only question is whether to store it in dev_archdata or in device. I think the argument for using struct device directly was that it lets us access this field from architecture independent code. It's not important to the overall design though: we could easily put it in dev_archdata and call an architecture specific function to set it, if there are good reasons for keeping it out of the generic device structure. Arnd
On Mon, May 5, 2014 at 2:55 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Monday 05 May 2014 14:45:21 Bjorn Helgaas wrote: >> [+cc Ben, Chris] >> >> On Fri, May 2, 2014 at 12:59 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Friday 02 May 2014 10:54:59 Bjorn Helgaas wrote: >> >> > +static void of_dma_configure(struct device *dev) >> >> > +{ >> >> > + u64 dma_addr, paddr, size; >> >> > + int ret; >> >> > + >> >> > + dev->coherent_dma_mask = DMA_BIT_MASK(32); >> >> > + if (!dev->dma_mask) >> >> > + dev->dma_mask = &dev->coherent_dma_mask; >> >> > + >> >> > + /* >> >> > + * if dma-coherent property exist, call arch hook to setup >> >> > + * dma coherent operations. >> >> > + */ >> >> > + if (of_dma_is_coherent(dev->of_node)) { >> >> > + set_arch_dma_coherent_ops(dev); >> >> > + dev_dbg(dev, "device is dma coherent\n"); >> >> > + } >> >> > + >> >> > + /* >> >> > + * if dma-ranges property doesn't exist - just return else >> >> > + * setup the dma offset >> >> > + */ >> >> > + ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size); >> >> > + if ((ret == -ENODEV) || (ret < 0)) { >> >> > + dev_dbg(dev, "no dma range information to setup\n"); >> >> > + return; >> >> > + } >> >> > + >> >> > + /* DMA ranges found. Calculate and set dma_pfn_offset */ >> >> > + dev->dma_pfn_offset = PFN_DOWN(paddr - dma_addr); >> >> > + dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset); >> >> >> >> Is this effectively the same as an IOMMU that applies a constant offset >> >> to the bus address? Could or should this be done by adding a simple IOMMU >> >> driver instead of adding dma_pfn_offset to struct device? >> > >> > We currently have two dma_map_ops variants on ARM (plus another set for >> > coherent/noncoherent differences, but we can ignore that for the sake >> > of this discussion): one that handles linear mappings and one that >> > handles IOMMUs by calling into the linux/iommu.h APIs. >> > >> > I guess what you mean by 'a simple IOMMU driver' would be another >> > dma_map_ops implementation that is separate from real IOMMUs, right? >> >> I suppose so; it seems like the offset could be managed inside >> arm_dma_ops. My idea of an IOMMU is something that maps bus addresses >> to physical memory addresses. That's what we're doing here; it's just >> that the mapping function is very simple. So why add something new in >> struct device for it? >> >> I think powerpc and tile do something similar in dma_direct_map_page() >> and tile_pci_dma_map_page() (they store the offset in struct >> dev_archdata). > > Ah, so the only question is whether to store it in dev_archdata or > in device. I think the argument for using struct device directly > was that it lets us access this field from architecture independent > code. It's not important to the overall design though: we could easily > put it in dev_archdata and call an architecture specific function to > set it, if there are good reasons for keeping it out of the generic > device structure. Well, it wasn't my *only* question, since I didn't know about the powerpc and tile usage originally :). Putting it in dev_archdata does seem better because it's a similar solution to a similar problem, and it's less public. I still wonder whether arm, powerpc, and tile (and I just noticed microblaze also has a similar dma_direct_map_page()) could all be handled by attaching devices to a generic trivial IOMMU driver parameterized with the appropriate constant offset. What arch-independent code would access this data? I expect that drivers will use dma_map_page(), etc., so they should already have both the bus and the physical address and wouldn't need it. Shouldn't generic code just rely on the DMA API, which might use an IOMMU or this dma_pfn_offset internally? Bjorn
On Mon, 2014-05-05 at 16:28 -0600, Bjorn Helgaas wrote: > I still wonder whether arm, powerpc, and tile (and I just noticed > microblaze also has a similar dma_direct_map_page()) could all be > handled by attaching devices to a generic trivial IOMMU driver > parameterized with the appropriate constant offset. On powerpc, the offset is not constant, it can be per-device Cheers, Ben.
On Tuesday 06 May 2014 13:44:38 Benjamin Herrenschmidt wrote: > On Mon, 2014-05-05 at 16:28 -0600, Bjorn Helgaas wrote: > > I still wonder whether arm, powerpc, and tile (and I just noticed > > microblaze also has a similar dma_direct_map_page()) could all be > > handled by attaching devices to a generic trivial IOMMU driver > > parameterized with the appropriate constant offset. It gets complex at that point, but it can be done. We are going to need support for IOMMU and swiotlb in the same place, as well as coherent and noncoherent ops, so the code here will be something like /* default is noncoherent, non-offset, no iommu */ if (offset) { if (coherent) set_arch_dma_coherent_offset_ops(dev); else set_arch_dma_offset_ops(dev); } else if (coherent) set_arch_dma_coherent_ops(dev); if (iommu) { if (coherent) set_arch_dma_coherent_iommu_ops(dev); else set_arch_dma_iommu_ops(dev); } Doing it in the default ops would reduce the number of cases from 5 to 3. It may be easier to replace set_arch_dma_coherent_ops() with a generic function that handles all cases: int set_arch_dma_ops(struct device *dev, bool coherent, phys_addr_t offset, struct device_node *iommu); and let the architecture handle the cases it needs. > On powerpc, the offset is not constant, it can be per-device. I think that's the case on all of them. The code under review here is what parses the dma-ranges property in order to put the correct value into the per-device structure. Arnd
On Tuesday 06 May 2014 05:54 AM, Arnd Bergmann wrote: > On Tuesday 06 May 2014 13:44:38 Benjamin Herrenschmidt wrote: >> On Mon, 2014-05-05 at 16:28 -0600, Bjorn Helgaas wrote: >>> I still wonder whether arm, powerpc, and tile (and I just noticed >>> microblaze also has a similar dma_direct_map_page()) could all be >>> handled by attaching devices to a generic trivial IOMMU driver >>> parameterized with the appropriate constant offset. > [..] > It may be easier to replace set_arch_dma_coherent_ops() with > a generic function that handles all cases: > > int set_arch_dma_ops(struct device *dev, bool coherent, > phys_addr_t offset, struct device_node *iommu); > > and let the architecture handle the cases it needs. > >> On powerpc, the offset is not constant, it can be per-device. > > I think that's the case on all of them. The code under review here > is what parses the dma-ranges property in order to put the correct > value into the per-device structure. > Yep. The per-device property need is one of the reason, we added the information to struct device. The constant need not be same though on keystone it is same for all devices. Regards, Santosh
On Fri, 02 May 2014 11:58:30 +0200, Arnd Bergmann <arnd@arndb.de> wrote: > On Thursday 01 May 2014 14:12:10 Grant Likely wrote: > > > > I've got two concerns here. of_dma_get_range() retrieves only the first > > > > tuple from the dma-ranges property, but it is perfectly valid for > > > > dma-ranges to contain multiple tuples. How should we handle it if a > > > > device has multiple ranges it can DMA from? > > > > > > > > > > We've not found any cases in current Linux where more than one dma-ranges > > > would be used. Moreover, The MM (definitely for ARM) isn't supported such > > > cases at all (if i understand everything right). > > > - there are only one arm_dma_pfn_limit > > > - there is only one MM zone is used for ARM > > > - some arches like x86,mips can support 2 zones (per arch - not per device or bus) > > > DMA & DMA32, but they configured once and forever per arch. > > > > Okay. If anyone ever does implement multiple ranges then this code will > > need to be revisited. > > I wonder if it's needed for platforms implementing the standard "ARM memory map" [1]. > The document only talks about addresses as seen from the CPU, and I can see > two logical interpretations how the RAM is supposed to be visible from a device: > either all RAM would be visible contiguously at DMA address zero, or everything > would be visible at the same physical address as the CPU sees it. > > If anyone picks the first interpretation, we will have to implement that > in Linux. We can of course hope that all hardware designs follow the second > interpretation, which would be more convenient for us here. Indeed. Hope though we might, I would not be surprised to see a platform that does the first. In that case we could probably handle it with a ranges property that is DMA-controller facing instead of device facing. That would be able to handle the translation between CPU addressing and DMA addressing. Come to think of it, doesn't PCI DMA have to deal with that situation if the PCI window is not 1:1 mapped into the CPU address space? g.
On Tuesday 27 May 2014 13:56:55 Grant Likely wrote: > On Fri, 02 May 2014 11:58:30 +0200, Arnd Bergmann <arnd@arndb.de> wrote: > > On Thursday 01 May 2014 14:12:10 Grant Likely wrote: > > > > > I've got two concerns here. of_dma_get_range() retrieves only the first > > > > > tuple from the dma-ranges property, but it is perfectly valid for > > > > > dma-ranges to contain multiple tuples. How should we handle it if a > > > > > device has multiple ranges it can DMA from? > > > > > > > > > > > > > We've not found any cases in current Linux where more than one dma-ranges > > > > would be used. Moreover, The MM (definitely for ARM) isn't supported such > > > > cases at all (if i understand everything right). > > > > - there are only one arm_dma_pfn_limit > > > > - there is only one MM zone is used for ARM > > > > - some arches like x86,mips can support 2 zones (per arch - not per device or bus) > > > > DMA & DMA32, but they configured once and forever per arch. > > > > > > Okay. If anyone ever does implement multiple ranges then this code will > > > need to be revisited. > > > > I wonder if it's needed for platforms implementing the standard "ARM memory map" [1]. > > The document only talks about addresses as seen from the CPU, and I can see > > two logical interpretations how the RAM is supposed to be visible from a device: > > either all RAM would be visible contiguously at DMA address zero, or everything > > would be visible at the same physical address as the CPU sees it. > > > > If anyone picks the first interpretation, we will have to implement that > > in Linux. We can of course hope that all hardware designs follow the second > > interpretation, which would be more convenient for us here. > > Indeed. Hope though we might, I would not be surprised to see a platform > that does the first. In that case we could probably handle it with a > ranges property that is DMA-controller facing instead of device facing. > That would be able to handle the translation between CPU addressing and > DMA addressing. > > Come to think of it, doesn't PCI DMA have to deal with that situation if > the PCI window is not 1:1 mapped into the CPU address space? I think all PCI buses we support so far only need a single entry in the dma-ranges property. Arnd
diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 48de98f..270c0b9 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -187,6 +187,50 @@ struct platform_device *of_device_alloc(struct device_node *np, EXPORT_SYMBOL(of_device_alloc); /** + * of_dma_configure - Setup DMA configuration + * @dev: Device to apply DMA configuration + * + * Try to get devices's DMA configuration from DT and update it + * accordingly. + * + * In case if platform code need to use own special DMA configuration,it + * can use Platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE event + * to fix up DMA configuration. + */ +static void of_dma_configure(struct device *dev) +{ + u64 dma_addr, paddr, size; + int ret; + + dev->coherent_dma_mask = DMA_BIT_MASK(32); + if (!dev->dma_mask) + dev->dma_mask = &dev->coherent_dma_mask; + + /* + * if dma-coherent property exist, call arch hook to setup + * dma coherent operations. + */ + if (of_dma_is_coherent(dev->of_node)) { + set_arch_dma_coherent_ops(dev); + dev_dbg(dev, "device is dma coherent\n"); + } + + /* + * if dma-ranges property doesn't exist - just return else + * setup the dma offset + */ + ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size); + if ((ret == -ENODEV) || (ret < 0)) { + dev_dbg(dev, "no dma range information to setup\n"); + return; + } + + /* DMA ranges found. Calculate and set dma_pfn_offset */ + dev->dma_pfn_offset = PFN_DOWN(paddr - dma_addr); + dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", dev->dma_pfn_offset); +} + +/** * of_platform_device_create_pdata - Alloc, initialize and register an of_device * @np: pointer to node to create device for * @bus_id: name to assign device @@ -214,9 +258,7 @@ static struct platform_device *of_platform_device_create_pdata( #if defined(CONFIG_MICROBLAZE) dev->archdata.dma_mask = 0xffffffffUL; #endif - dev->dev.coherent_dma_mask = DMA_BIT_MASK(32); - if (!dev->dev.dma_mask) - dev->dev.dma_mask = &dev->dev.coherent_dma_mask; + of_dma_configure(&dev->dev); dev->dev.bus = &platform_bus_type; dev->dev.platform_data = platform_data; diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index fd4aee2..c7d9b1b 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -123,6 +123,13 @@ static inline int dma_coerce_mask_and_coherent(struct device *dev, u64 mask) extern u64 dma_get_required_mask(struct device *dev); +#ifndef set_arch_dma_coherent_ops +static inline int set_arch_dma_coherent_ops(struct device *dev) +{ + return 0; +} +#endif + static inline unsigned int dma_get_max_seg_size(struct device *dev) { return dev->dma_parms ? dev->dma_parms->max_segment_size : 65536;