Here is an updated version of [2] based on discussion. Series introduces
support for setting up dma parameters based on device tree properties
like 'dma-ranges' and 'dma-coherent' and also update to ARM 32 bit port.
Earlier version of the same series is here [1].
The 'dma-ranges' helps to take care of few DMAable system memory restrictions
by use of dma_pfn_offset which we maintain now per device. Arch code then
uses it for dma address translations for such cases. We update the
dma_pfn_offset accordingly during DT the device creation process.The
'dma-coherent' property is used to setup arch's coherent dma_ops.
After some off-list discussion with RMK and Arnd, I have now dropped the
controversial dma_mask setup code from the series which actually isn't blocking
me as such. Considering rest of the parts of the series are already aligned,
am hoping to get this version merged for 3.16 merge window.
We agreed in last discussion that drivers have the ultimate
responsibility to setup the correct dma mask but then we need to have some
means to see if bus can support what driver has requested for a case where
driver request for bigger mask than what bus supports. I can follow up on
the mask topic if we have broken drivers.
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Olof Johansson <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Grygorii Strashko <[email protected]>
Grygorii Strashko (2):
of: introduce of_dma_get_range() helper
ARM: dma: Use dma_pfn_offset for dma address translation
Santosh Shilimkar (5):
device: introduce per device dma_pfn_offset
of: introduce of_dma_is_coherent() helper
of: configure the platform device dma parameters
ARM: dma: implement set_arch_dma_coherent_ops()
ARM: mm: use phys_addr_t in __dma_page_[cpu_to_dev/dev_to_cpu]
arch/arm/include/asm/dma-mapping.h | 24 +++++-
arch/arm/mm/dma-mapping.c | 4 +-
drivers/of/platform.c | 151 ++++++++++++++++++++++++++++++++++++
include/linux/device.h | 2 +
include/linux/dma-mapping.h | 7 ++
include/linux/of_platform.h | 15 ++++
6 files changed, 197 insertions(+), 6 deletions(-)
Regards,
Santosh
[1] http://www.spinics.net/lists/arm-kernel/msg311678.html
[2] https://lkml.org/lkml/2014/3/6/186
--
1.7.9.5
On a 32 bit ARM architecture with LPAE extension physical addresses
cannot fit into unsigned long variable.
So fix it by using phys_addr_t instead of unsigned long.
Signed-off-by: Santosh Shilimkar <[email protected]>
---
arch/arm/mm/dma-mapping.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index f62aa06..5260f43 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -885,7 +885,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
static void __dma_page_cpu_to_dev(struct page *page, unsigned long off,
size_t size, enum dma_data_direction dir)
{
- unsigned long paddr;
+ phys_addr_t paddr;
dma_cache_maint_page(page, off, size, dir, dmac_map_area);
@@ -901,7 +901,7 @@ static void __dma_page_cpu_to_dev(struct page *page, unsigned long off,
static void __dma_page_dev_to_cpu(struct page *page, unsigned long off,
size_t size, enum dma_data_direction dir)
{
- unsigned long paddr = page_to_phys(page) + off;
+ phys_addr_t paddr = page_to_phys(page) + off;
/* FIXME: non-speculating: not required */
/* don't bother invalidating if DMA to device */
--
1.7.9.5
From: Grygorii Strashko <[email protected]>
In most of cases DMA addresses can be performed using offset value of
Bus address space relatively to physical address space as following:
PFN->DMA:
__pfn_to_phys(pfn + [-]dma_pfn_offset)
DMA->PFN:
__phys_to_pfn(dma_addr) + [-]dma_pfn_offset
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Olof Johansson <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Linus Walleij <[email protected]>
Signed-off-by: Grygorii Strashko <[email protected]>
Signed-off-by: Santosh Shilimkar <[email protected]>
---
arch/arm/include/asm/dma-mapping.h | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index e701a4d..8c12149 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -58,22 +58,31 @@ static inline int dma_set_mask(struct device *dev, u64 mask)
#ifndef __arch_pfn_to_dma
static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn)
{
- return (dma_addr_t)__pfn_to_bus(pfn);
+ if (!dev)
+ return DMA_ERROR_CODE;
+ return (dma_addr_t)__pfn_to_bus(pfn - dev->dma_pfn_offset);
}
static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr)
{
- return __bus_to_pfn(addr);
+ if (!dev)
+ return 0;
+ return __bus_to_pfn(addr) + dev->dma_pfn_offset;
}
static inline void *dma_to_virt(struct device *dev, dma_addr_t addr)
{
- return (void *)__bus_to_virt((unsigned long)addr);
+ if (!dev)
+ return NULL;
+ return (void *)__bus_to_virt(__pfn_to_bus(dma_to_pfn(dev, addr)));
}
static inline dma_addr_t virt_to_dma(struct device *dev, void *addr)
{
- return (dma_addr_t)__virt_to_bus((unsigned long)(addr));
+ if (!dev)
+ return DMA_ERROR_CODE;
+ return pfn_to_dma(dev,
+ __bus_to_pfn(__virt_to_bus((unsigned long)(addr))));
}
#else
--
1.7.9.5
The of_dma_is_coherent() helper parses the given DT device
node to see if the "dma-coherent" property is supported and
returns true or false accordingly.
If the arch is always coherent or always noncoherent, then the default
DMA ops has to be specified accordingly.
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Olof Johansson <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Linus Walleij <[email protected]>
Signed-off-by: Santosh Shilimkar <[email protected]>
Signed-off-by: Grygorii Strashko <[email protected]>
---
drivers/of/platform.c | 23 +++++++++++++++++++++++
include/linux/of_platform.h | 7 +++++++
2 files changed, 30 insertions(+)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 2265a55..573db15 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -570,4 +570,27 @@ out:
return ret;
}
EXPORT_SYMBOL_GPL(of_dma_get_range);
+
+/**
+ * of_dma_is_coherent - Check if device is coherent
+ * @np: device node
+ *
+ * It returns true if "dma-coherent" property was found
+ * for this device in DT.
+ */
+bool of_dma_is_coherent(struct device_node *np)
+{
+ struct device_node *node = of_node_get(np);
+
+ while (node) {
+ if (of_property_read_bool(node, "dma-coherent")) {
+ of_node_put(node);
+ return true;
+ }
+ node = of_get_next_parent(node);
+ }
+ of_node_put(node);
+ return false;
+}
+EXPORT_SYMBOL_GPL(of_dma_is_coherent);
#endif /* CONFIG_OF_ADDRESS */
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index ba7d3dc..dc52931 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -74,6 +74,8 @@ extern int of_platform_populate(struct device_node *root,
struct device *parent);
extern int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr,
phys_addr_t *paddr, phys_addr_t *size);
+extern bool of_dma_is_coherent(struct device_node *np);
+
#else
static inline int of_platform_populate(struct device_node *root,
const struct of_device_id *matches,
@@ -88,6 +90,11 @@ static inline int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr,
{
return -ENODEV;
}
+
+static inline bool of_dma_is_coherent(struct device_node *np)
+{
+ return false;
+}
#endif
#endif /* _LINUX_OF_PLATFORM_H */
--
1.7.9.5
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 <[email protected]>
Cc: Russell King <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Olof Johansson <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Linus Walleij <[email protected]>
Signed-off-by: Grygorii Strashko <[email protected]>
Signed-off-by: Santosh Shilimkar <[email protected]>
---
drivers/of/platform.c | 43 +++++++++++++++++++++++++++++++++++++++++++
include/linux/dma-mapping.h | 7 +++++++
2 files changed, 50 insertions(+)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 573db15..7e4a43b 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -187,6 +187,47 @@ 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)
+{
+ dma_addr_t dma_addr;
+ phys_addr_t paddr, size;
+ int ret;
+
+ /*
+ * 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 < 0) {
+ dev_dbg(dev, "no dma range information to setup\n");
+ return;
+ } else {
+ /* 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
@@ -217,6 +258,8 @@ static struct platform_device *of_platform_device_create_pdata(
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
From: Grygorii Strashko <[email protected]>
The of_dma_get_range() allows to find "dma-range" property for
the specified device and parse it.
dma-ranges format:
DMA addr (dma_addr) : naddr cells
CPU addr (phys_addr_t) : pna cells
size : nsize cells
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Olof Johansson <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Linus Walleij <[email protected]>
Signed-off-by: Grygorii Strashko <[email protected]>
Signed-off-by: Santosh Shilimkar <[email protected]>
---
drivers/of/platform.c | 85 +++++++++++++++++++++++++++++++++++++++++++
include/linux/of_platform.h | 8 ++++
2 files changed, 93 insertions(+)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 404d1da..2265a55 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -485,4 +485,89 @@ int of_platform_populate(struct device_node *root,
return rc;
}
EXPORT_SYMBOL_GPL(of_platform_populate);
+
+/**
+ * of_dma_get_range - Get DMA range info
+ * @np: device node to get DMA range info
+ * @dma_addr: pointer to store initial DMA address of DMA range
+ * @paddr: pointer to store initial CPU address of DMA range
+ * @size: pointer to store size of DMA range
+ *
+ * Look in bottom up direction for the first "dma-range" property
+ * and parse it.
+ * dma-ranges format:
+ * DMA addr (dma_addr) : naddr cells
+ * CPU addr (phys_addr_t) : pna cells
+ * size : nsize cells
+ *
+ * It returns -ENODEV if "dma-ranges" property was not found
+ * for this device in DT.
+ */
+extern int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr,
+ phys_addr_t *paddr, phys_addr_t *size)
+{
+ struct device_node *node = np;
+ const u32 *ranges = NULL;
+ int len, naddr, nsize, pna;
+ int ret = 0;
+
+ if (!node)
+ return -EINVAL;
+
+ while (1) {
+ naddr = of_n_addr_cells(node);
+ nsize = of_n_size_cells(node);
+ node = of_get_next_parent(node);
+ if (!node)
+ break;
+
+ ranges = of_get_property(node, "dma-ranges", &len);
+
+ /* Ignore empty ranges, they imply no translation required */
+ if (ranges && len > 0)
+ break;
+
+ /*
+ * At least empty ranges has to be defined for parent node if
+ * DMA is supported
+ */
+ if (!ranges)
+ break;
+ }
+
+ if (!ranges) {
+ pr_debug("%s: no dma-ranges found for node(%s)\n",
+ __func__, np->full_name);
+ ret = -ENODEV;
+ goto out;
+ }
+
+ len /= sizeof(u32);
+
+ pna = of_n_addr_cells(node);
+
+ /* dma-ranges format:
+ * DMA addr : naddr cells
+ * CPU addr : pna cells
+ * size : nsize cells
+ */
+ *dma_addr = of_read_number(ranges, naddr);
+ *paddr = of_translate_dma_address(np, ranges);
+ if (*paddr == OF_BAD_ADDR) {
+ pr_err("%s: translation of DMA address(%pad) to CPU address failed node(%s)\n",
+ __func__, dma_addr, np->full_name);
+ ret = -EINVAL;
+ }
+
+ *size = of_read_number(ranges + naddr + pna, nsize);
+
+ pr_debug("dma_addr(%pad) cpu_addr(%pa) size(%pa)\n",
+ dma_addr, paddr, size);
+
+out:
+ of_node_put(node);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(of_dma_get_range);
#endif /* CONFIG_OF_ADDRESS */
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index 05cb4a9..ba7d3dc 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -72,6 +72,8 @@ extern int of_platform_populate(struct device_node *root,
const struct of_device_id *matches,
const struct of_dev_auxdata *lookup,
struct device *parent);
+extern int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr,
+ phys_addr_t *paddr, phys_addr_t *size);
#else
static inline int of_platform_populate(struct device_node *root,
const struct of_device_id *matches,
@@ -80,6 +82,12 @@ static inline int of_platform_populate(struct device_node *root,
{
return -ENODEV;
}
+
+static inline int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr,
+ phys_addr_t *paddr, phys_addr_t *size)
+{
+ return -ENODEV;
+}
#endif
#endif /* _LINUX_OF_PLATFORM_H */
--
1.7.9.5
Implement the set_arch_dma_coherent_ops() for ARM architecture.
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Olof Johansson <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Linus Walleij <[email protected]>
Signed-off-by: Grygorii Strashko <[email protected]>
Signed-off-by: Santosh Shilimkar <[email protected]>
---
arch/arm/include/asm/dma-mapping.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index 8c12149..765c9e7 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -114,6 +114,13 @@ static inline unsigned long dma_max_pfn(struct device *dev)
}
#define dma_max_pfn(dev) dma_max_pfn(dev)
+static inline int set_arch_dma_coherent_ops(struct device *dev)
+{
+ set_dma_ops(dev, &arm_coherent_dma_ops);
+ return 0;
+}
+#define set_arch_dma_coherent_ops(dev) set_arch_dma_coherent_ops(dev)
+
static inline dma_addr_t phys_to_dma(struct device *dev, phys_addr_t paddr)
{
unsigned int offset = paddr & ~PAGE_MASK;
--
1.7.9.5
On few architectures, there are few restrictions on DMAble area of system
RAM. That also means that devices needs to know about this restrictions so
that the dma_masks can be updated accordingly and dma address translation
helpers can add/subtract the dma offset.
In most of cases DMA addresses can be performed using offset value of
Bus address space relatively to physical address space as following:
PFN->DMA: __pfn_to_phys(pfn + [-]dma_pfn_offset)
DMA->PFN: __phys_to_pfn(dma_addr) + [-]dma_pfn_offset
So we introduce per device dma_pfn_offset which can be popullated
by architecture init code while creating the devices.
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Russell King <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Olof Johansson <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Linus Walleij <[email protected]>
Signed-off-by: Grygorii Strashko <[email protected]>
Signed-off-by: Santosh Shilimkar <[email protected]>
---
include/linux/device.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/device.h b/include/linux/device.h
index 233bbbe..85a52d6 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -691,6 +691,7 @@ struct acpi_dev_node {
* @coherent_dma_mask: Like dma_mask, but for alloc_coherent mapping as not all
* hardware supports 64-bit addresses for consistent allocations
* such descriptors.
+ * @dma_pfn_offset: offset of DMA memory range relatively of RAM
* @dma_parms: A low level driver may set these to teach IOMMU code about
* segment limitations.
* @dma_pools: Dma pools (if dma'ble device).
@@ -756,6 +757,7 @@ struct device {
not all hardware supports
64 bit addresses for consistent
allocations such descriptors. */
+ unsigned long dma_pfn_offset;
struct device_dma_parameters *dma_parms;
--
1.7.9.5
Dear Santosh Shilimkar,
On Sat, 19 Apr 2014 10:32:45 -0400, Santosh Shilimkar wrote:
> Here is an updated version of [2] based on discussion. Series introduces
> support for setting up dma parameters based on device tree properties
> like 'dma-ranges' and 'dma-coherent' and also update to ARM 32 bit port.
> Earlier version of the same series is here [1].
>
> The 'dma-ranges' helps to take care of few DMAable system memory restrictions
> by use of dma_pfn_offset which we maintain now per device. Arch code then
> uses it for dma address translations for such cases. We update the
> dma_pfn_offset accordingly during DT the device creation process.The
> 'dma-coherent' property is used to setup arch's coherent dma_ops.
>
> After some off-list discussion with RMK and Arnd, I have now dropped the
> controversial dma_mask setup code from the series which actually isn't blocking
> me as such. Considering rest of the parts of the series are already aligned,
> am hoping to get this version merged for 3.16 merge window.
>
> We agreed in last discussion that drivers have the ultimate
> responsibility to setup the correct dma mask but then we need to have some
> means to see if bus can support what driver has requested for a case where
> driver request for bigger mask than what bus supports. I can follow up on
> the mask topic if we have broken drivers.
I am not sure whether there is an intersection or not, but I wanted to
mention that the mvebu platform (in mach-mvebu) supports hardware I/O
coherency, which makes it a coherent DMA platform. However, we are not
able to use arm_coherent_dma_ops for this platform, because when a
transfer is being made DMA_FROM_DEVICE, at the end of the transfer, we
need to perform an I/O barrier to wait for the snooping unit to
complete its coherency work. So we're coherent, but not with
arm_coherent_dma_ops: we have our own dma operation implementation (see
arch/arm/mach-mvebu/coherency.c).
However, it seems that your patch series, at least in PATCH 6/7 makes
the assumption that for all DMA coherent platforms,
arm_coherent_dma_ops is going to be OK.
Also, I haven't followed all the discussions, but what is the intended
usage of of_dma_is_coherent() and set_arch_dma_coherent_ops() (device
drivers? platform code?).
In mach-mvebu, what we do is that we register a bus notifier on the
platform bus, so that we can set our custom DMA operations for all
platform devices in the system. Should this be done in a different way
after your series?
Thanks,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On Sat, Apr 19, 2014 at 10:32:50AM -0400, Santosh Shilimkar wrote:
> From: Grygorii Strashko <[email protected]>
>
> In most of cases DMA addresses can be performed using offset value of
> Bus address space relatively to physical address space as following:
>
> PFN->DMA:
> __pfn_to_phys(pfn + [-]dma_pfn_offset)
>
> DMA->PFN:
> __phys_to_pfn(dma_addr) + [-]dma_pfn_offset
>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Olof Johansson <[email protected]>
> Cc: Grant Likely <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Linus Walleij <[email protected]>
> Signed-off-by: Grygorii Strashko <[email protected]>
> Signed-off-by: Santosh Shilimkar <[email protected]>
> ---
> arch/arm/include/asm/dma-mapping.h | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
> index e701a4d..8c12149 100644
> --- a/arch/arm/include/asm/dma-mapping.h
> +++ b/arch/arm/include/asm/dma-mapping.h
> @@ -58,22 +58,31 @@ static inline int dma_set_mask(struct device *dev, u64 mask)
> #ifndef __arch_pfn_to_dma
> static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn)
> {
> - return (dma_addr_t)__pfn_to_bus(pfn);
> + if (!dev)
> + return DMA_ERROR_CODE;
> + return (dma_addr_t)__pfn_to_bus(pfn - dev->dma_pfn_offset);
How do ISA devices (iow, those which pass a NULL device) work with this?
This looks to me like it ends up breaking some drivers.
I've also seen some drivers (such as the Freescale FEC driver) which
perform DMA coherent allocations with a NULL device - technically, that's
a bug in the driver, but the above change will cause them to regress.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
On Saturday 19 April 2014 12:25 PM, Thomas Petazzoni wrote:
> Dear Santosh Shilimkar,
>
> On Sat, 19 Apr 2014 10:32:45 -0400, Santosh Shilimkar wrote:
>> Here is an updated version of [2] based on discussion. Series introduces
>> support for setting up dma parameters based on device tree properties
>> like 'dma-ranges' and 'dma-coherent' and also update to ARM 32 bit port.
>> Earlier version of the same series is here [1].
>>
>> The 'dma-ranges' helps to take care of few DMAable system memory restrictions
>> by use of dma_pfn_offset which we maintain now per device. Arch code then
>> uses it for dma address translations for such cases. We update the
>> dma_pfn_offset accordingly during DT the device creation process.The
>> 'dma-coherent' property is used to setup arch's coherent dma_ops.
>>
>> After some off-list discussion with RMK and Arnd, I have now dropped the
>> controversial dma_mask setup code from the series which actually isn't blocking
>> me as such. Considering rest of the parts of the series are already aligned,
>> am hoping to get this version merged for 3.16 merge window.
>>
>> We agreed in last discussion that drivers have the ultimate
>> responsibility to setup the correct dma mask but then we need to have some
>> means to see if bus can support what driver has requested for a case where
>> driver request for bigger mask than what bus supports. I can follow up on
>> the mask topic if we have broken drivers.
>
> I am not sure whether there is an intersection or not, but I wanted to
> mention that the mvebu platform (in mach-mvebu) supports hardware I/O
> coherency, which makes it a coherent DMA platform. However, we are not
> able to use arm_coherent_dma_ops for this platform, because when a
> transfer is being made DMA_FROM_DEVICE, at the end of the transfer, we
> need to perform an I/O barrier to wait for the snooping unit to
> complete its coherency work. So we're coherent, but not with
> arm_coherent_dma_ops: we have our own dma operation implementation (see
> arch/arm/mach-mvebu/coherency.c).
>
I have seen that.
> However, it seems that your patch series, at least in PATCH 6/7 makes
> the assumption that for all DMA coherent platforms,
> arm_coherent_dma_ops is going to be OK.
>
No it doesn't. The infrastructure is for all the common cases which can
just use arm generic dma_ops.
> Also, I haven't followed all the discussions, but what is the intended
> usage of of_dma_is_coherent() and set_arch_dma_coherent_ops() (device
> drivers? platform code?).
>
The intention is for sub arch's like ARM, ARM64, powerPC etc can set
the dma_ops based on the device tree property. Today the ARM coherent
machines are doing that in machine code and we want to move that
to more generic code.
> In mach-mvebu, what we do is that we register a bus notifier on the
> platform bus, so that we can set our custom DMA operations for all
> platform devices in the system. Should this be done in a different way
> after your series?
>
Nope. Since you have a very custom SOC specific case, you can continue
what you are doing.
Regards,
Santosh
On Saturday 19 April 2014 03:43 PM, Russell King - ARM Linux wrote:
> On Sat, Apr 19, 2014 at 10:32:50AM -0400, Santosh Shilimkar wrote:
>> From: Grygorii Strashko <[email protected]>
>>
>> In most of cases DMA addresses can be performed using offset value of
>> Bus address space relatively to physical address space as following:
>>
>> PFN->DMA:
>> __pfn_to_phys(pfn + [-]dma_pfn_offset)
>>
>> DMA->PFN:
>> __phys_to_pfn(dma_addr) + [-]dma_pfn_offset
>>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: Russell King <[email protected]>
>> Cc: Arnd Bergmann <[email protected]>
>> Cc: Olof Johansson <[email protected]>
>> Cc: Grant Likely <[email protected]>
>> Cc: Rob Herring <[email protected]>
>> Cc: Catalin Marinas <[email protected]>
>> Cc: Linus Walleij <[email protected]>
>> Signed-off-by: Grygorii Strashko <[email protected]>
>> Signed-off-by: Santosh Shilimkar <[email protected]>
>> ---
>> arch/arm/include/asm/dma-mapping.h | 17 +++++++++++++----
>> 1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
>> index e701a4d..8c12149 100644
>> --- a/arch/arm/include/asm/dma-mapping.h
>> +++ b/arch/arm/include/asm/dma-mapping.h
>> @@ -58,22 +58,31 @@ static inline int dma_set_mask(struct device *dev, u64 mask)
>> #ifndef __arch_pfn_to_dma
>> static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn)
>> {
>> - return (dma_addr_t)__pfn_to_bus(pfn);
>> + if (!dev)
>> + return DMA_ERROR_CODE;
>> + return (dma_addr_t)__pfn_to_bus(pfn - dev->dma_pfn_offset);
>
> How do ISA devices (iow, those which pass a NULL device) work with this?
> This looks to me like it ends up breaking some drivers.
>
> I've also seen some drivers (such as the Freescale FEC driver) which
> perform DMA coherent allocations with a NULL device - technically, that's
> a bug in the driver, but the above change will cause them to regress.
>
Good point. We can keep the NULL case working as well...
static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn)
{
if (!dev)
return (dma_addr_t)__pfn_to_bus(pfn);
else
return (dma_addr_t)__pfn_to_bus(pfn - dev->dma_pfn_offset);
}
I will update the patch accordingly.
Regards,
Santosh
(Adding few CC's)
On Saturday 19 April 2014 10:32 AM, Santosh Shilimkar wrote:
> On a 32 bit ARM architecture with LPAE extension physical addresses
> cannot fit into unsigned long variable.
>
> So fix it by using phys_addr_t instead of unsigned long.
>
> Signed-off-by: Santosh Shilimkar <[email protected]>
> ---
> arch/arm/mm/dma-mapping.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index f62aa06..5260f43 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -885,7 +885,7 @@ static void dma_cache_maint_page(struct page *page, unsigned long offset,
> static void __dma_page_cpu_to_dev(struct page *page, unsigned long off,
> size_t size, enum dma_data_direction dir)
> {
> - unsigned long paddr;
> + phys_addr_t paddr;
>
> dma_cache_maint_page(page, off, size, dir, dmac_map_area);
>
> @@ -901,7 +901,7 @@ static void __dma_page_cpu_to_dev(struct page *page, unsigned long off,
> static void __dma_page_dev_to_cpu(struct page *page, unsigned long off,
> size_t size, enum dma_data_direction dir)
> {
> - unsigned long paddr = page_to_phys(page) + off;
> + phys_addr_t paddr = page_to_phys(page) + off;
>
> /* FIXME: non-speculating: not required */
> /* don't bother invalidating if DMA to device */
>
On Sat, Apr 19, 2014 at 9:32 AM, Santosh Shilimkar
<[email protected]> wrote:
> Here is an updated version of [2] based on discussion. Series introduces
> support for setting up dma parameters based on device tree properties
> like 'dma-ranges' and 'dma-coherent' and also update to ARM 32 bit port.
> Earlier version of the same series is here [1].
>
> The 'dma-ranges' helps to take care of few DMAable system memory restrictions
> by use of dma_pfn_offset which we maintain now per device. Arch code then
> uses it for dma address translations for such cases. We update the
> dma_pfn_offset accordingly during DT the device creation process.The
> 'dma-coherent' property is used to setup arch's coherent dma_ops.
>
> After some off-list discussion with RMK and Arnd, I have now dropped the
> controversial dma_mask setup code from the series which actually isn't blocking
> me as such. Considering rest of the parts of the series are already aligned,
> am hoping to get this version merged for 3.16 merge window.
Can you briefly describe what the h/w looks like in terms of addresses
for the problem you are trying to solve? Something like: Cpu view of
RAM is X to Y address, X corresponds to DMA address Z. Max DMA address
is ?
Rob
On Sat, Apr 19, 2014 at 9:32 AM, Santosh Shilimkar
<[email protected]> 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 <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Olof Johansson <[email protected]>
> Cc: Grant Likely <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Linus Walleij <[email protected]>
> Signed-off-by: Grygorii Strashko <[email protected]>
> Signed-off-by: Santosh Shilimkar <[email protected]>
> ---
> drivers/of/platform.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/dma-mapping.h | 7 +++++++
> 2 files changed, 50 insertions(+)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 573db15..7e4a43b 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -187,6 +187,47 @@ 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)
> +{
> + dma_addr_t dma_addr;
> + phys_addr_t paddr, size;
A problem with using dma_addr_t and phys_addr_t is that they normally
depend on CONFIG_LPAE, but the address cell sizes are independent of
that. That is why all the memory related functions stick with u64. I
think you would have issues here if you have a platform with sizes of
4GB or more. You need to properly cap the sizes and addresses.
> + int ret;
> +
> + /*
> + * 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 < 0) {
Perhaps an error is not the right return for the default case. The
default should probably be dma_addr and paddr equal to 0 and size 4GB.
> + dev_dbg(dev, "no dma range information to setup\n");
> + return;
> + } else {
You don't need the else here.
> + /* 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
> @@ -217,6 +258,8 @@ static struct platform_device *of_platform_device_create_pdata(
> dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> if (!dev->dev.dma_mask)
> dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
You should move all DMA related setup into of_dma_configure.
> +
> + 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 Rob,
On Monday 21 April 2014 10:37 AM, Rob Herring wrote:
> On Sat, Apr 19, 2014 at 9:32 AM, Santosh Shilimkar
> <[email protected]> wrote:
>> Here is an updated version of [2] based on discussion. Series introduces
>> support for setting up dma parameters based on device tree properties
>> like 'dma-ranges' and 'dma-coherent' and also update to ARM 32 bit port.
>> Earlier version of the same series is here [1].
>>
>> The 'dma-ranges' helps to take care of few DMAable system memory restrictions
>> by use of dma_pfn_offset which we maintain now per device. Arch code then
>> uses it for dma address translations for such cases. We update the
>> dma_pfn_offset accordingly during DT the device creation process.The
>> 'dma-coherent' property is used to setup arch's coherent dma_ops.
>>
>> After some off-list discussion with RMK and Arnd, I have now dropped the
>> controversial dma_mask setup code from the series which actually isn't blocking
>> me as such. Considering rest of the parts of the series are already aligned,
>> am hoping to get this version merged for 3.16 merge window.
>
> Can you briefly describe what the h/w looks like in terms of addresses
> for the problem you are trying to solve? Something like: Cpu view of
> RAM is X to Y address, X corresponds to DMA address Z. Max DMA address
> is ?
>
Let me try with say 8 GB RAM example
CPU view of memory : 0x0000 0008 0000 0000 to 0x0000 000a 0000 0000
>From above memory range, first 2 GB of memory has an alias 32 bit
view in the hardware. Hardware internally map the address issued within
that first 2 GB to same memory.
DMA view of first 2 GB [ 0x0000 0008 0000 0000 to 0x0000 0008 7fff ffff]
is : 0x8000 0000 to 0xffff fffff.
Regards,
Santosh
On Sat, Apr 19, 2014 at 9:32 AM, Santosh Shilimkar
<[email protected]> wrote:
> From: Grygorii Strashko <[email protected]>
>
> The of_dma_get_range() allows to find "dma-range" property for
> the specified device and parse it.
> dma-ranges format:
> DMA addr (dma_addr) : naddr cells
> CPU addr (phys_addr_t) : pna cells
> size : nsize cells
>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Olof Johansson <[email protected]>
> Cc: Grant Likely <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Linus Walleij <[email protected]>
> Signed-off-by: Grygorii Strashko <[email protected]>
> Signed-off-by: Santosh Shilimkar <[email protected]>
> ---
> drivers/of/platform.c | 85 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/of_platform.h | 8 ++++
This belongs in drivers/of/address.c and of_address.h.
> 2 files changed, 93 insertions(+)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 404d1da..2265a55 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -485,4 +485,89 @@ int of_platform_populate(struct device_node *root,
> return rc;
> }
> EXPORT_SYMBOL_GPL(of_platform_populate);
> +
> +/**
> + * of_dma_get_range - Get DMA range info
> + * @np: device node to get DMA range info
> + * @dma_addr: pointer to store initial DMA address of DMA range
> + * @paddr: pointer to store initial CPU address of DMA range
> + * @size: pointer to store size of DMA range
> + *
> + * Look in bottom up direction for the first "dma-range" property
dma-ranges
> + * and parse it.
> + * dma-ranges format:
> + * DMA addr (dma_addr) : naddr cells
> + * CPU addr (phys_addr_t) : pna cells
> + * size : nsize cells
> + *
> + * It returns -ENODEV if "dma-ranges" property was not found
> + * for this device in DT.
> + */
> +extern int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr,
drop extern.
> + phys_addr_t *paddr, phys_addr_t *size)
> +{
> + struct device_node *node = np;
> + const u32 *ranges = NULL;
__be32
> + int len, naddr, nsize, pna;
> + int ret = 0;
> +
> + if (!node)
> + return -EINVAL;
> +
> + while (1) {
> + naddr = of_n_addr_cells(node);
> + nsize = of_n_size_cells(node);
> + node = of_get_next_parent(node);
> + if (!node)
> + break;
> +
> + ranges = of_get_property(node, "dma-ranges", &len);
> +
> + /* Ignore empty ranges, they imply no translation required */
> + if (ranges && len > 0)
> + break;
> +
> + /*
> + * At least empty ranges has to be defined for parent node if
> + * DMA is supported
> + */
> + if (!ranges)
> + break;
You need an of_node_put here if you are getting the next parent.
> + }
> +
> + if (!ranges) {
> + pr_debug("%s: no dma-ranges found for node(%s)\n",
> + __func__, np->full_name);
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + len /= sizeof(u32);
> +
> + pna = of_n_addr_cells(node);
> +
> + /* dma-ranges format:
> + * DMA addr : naddr cells
> + * CPU addr : pna cells
> + * size : nsize cells
> + */
> + *dma_addr = of_read_number(ranges, naddr);
You should set this after the following call succeeds.
> + *paddr = of_translate_dma_address(np, ranges);
This is going to search for dma-ranges again, but you have already
done that. Is there a case that just reading the CPU addr will not
work? I suppose we could have more than one level of translation. It
also seems a bit abusive that the DMA address is the ranges property.
I don't really have a better suggestion though.
> + if (*paddr == OF_BAD_ADDR) {
As I mentioned, this is potentially a size mismatch.
Rob
On Mon, Apr 21, 2014 at 10:13 AM, Santosh Shilimkar
<[email protected]> wrote:
> Hi Rob,
>
> On Monday 21 April 2014 10:37 AM, Rob Herring wrote:
>> On Sat, Apr 19, 2014 at 9:32 AM, Santosh Shilimkar
>> <[email protected]> wrote:
>>> Here is an updated version of [2] based on discussion. Series introduces
>>> support for setting up dma parameters based on device tree properties
>>> like 'dma-ranges' and 'dma-coherent' and also update to ARM 32 bit port.
>>> Earlier version of the same series is here [1].
>>>
>>> The 'dma-ranges' helps to take care of few DMAable system memory restrictions
>>> by use of dma_pfn_offset which we maintain now per device. Arch code then
>>> uses it for dma address translations for such cases. We update the
>>> dma_pfn_offset accordingly during DT the device creation process.The
>>> 'dma-coherent' property is used to setup arch's coherent dma_ops.
>>>
>>> After some off-list discussion with RMK and Arnd, I have now dropped the
>>> controversial dma_mask setup code from the series which actually isn't blocking
>>> me as such. Considering rest of the parts of the series are already aligned,
>>> am hoping to get this version merged for 3.16 merge window.
>>
>> Can you briefly describe what the h/w looks like in terms of addresses
>> for the problem you are trying to solve? Something like: Cpu view of
>> RAM is X to Y address, X corresponds to DMA address Z. Max DMA address
>> is ?
>>
> Let me try with say 8 GB RAM example
>
> CPU view of memory : 0x0000 0008 0000 0000 to 0x0000 000a 0000 0000
>
> From above memory range, first 2 GB of memory has an alias 32 bit
> view in the hardware. Hardware internally map the address issued within
> that first 2 GB to same memory.
>
> DMA view of first 2 GB [ 0x0000 0008 0000 0000 to 0x0000 0008 7fff ffff]
> is : 0x8000 0000 to 0xffff fffff.
Are you setting ZONE_DMA to be 2GB so allocations stay within DMAable
memory and that is enough that you don't need to set DMA masks?
Rob
On Monday 21 April 2014 11:35 AM, Rob Herring wrote:
> On Mon, Apr 21, 2014 at 10:13 AM, Santosh Shilimkar
> <[email protected]> wrote:
>> Hi Rob,
>>
>> On Monday 21 April 2014 10:37 AM, Rob Herring wrote:
>>> On Sat, Apr 19, 2014 at 9:32 AM, Santosh Shilimkar
>>> <[email protected]> wrote:
>>>> Here is an updated version of [2] based on discussion. Series introduces
>>>> support for setting up dma parameters based on device tree properties
>>>> like 'dma-ranges' and 'dma-coherent' and also update to ARM 32 bit port.
>>>> Earlier version of the same series is here [1].
>>>>
>>>> The 'dma-ranges' helps to take care of few DMAable system memory restrictions
>>>> by use of dma_pfn_offset which we maintain now per device. Arch code then
>>>> uses it for dma address translations for such cases. We update the
>>>> dma_pfn_offset accordingly during DT the device creation process.The
>>>> 'dma-coherent' property is used to setup arch's coherent dma_ops.
>>>>
>>>> After some off-list discussion with RMK and Arnd, I have now dropped the
>>>> controversial dma_mask setup code from the series which actually isn't blocking
>>>> me as such. Considering rest of the parts of the series are already aligned,
>>>> am hoping to get this version merged for 3.16 merge window.
>>>
>>> Can you briefly describe what the h/w looks like in terms of addresses
>>> for the problem you are trying to solve? Something like: Cpu view of
>>> RAM is X to Y address, X corresponds to DMA address Z. Max DMA address
>>> is ?
>>>
>> Let me try with say 8 GB RAM example
>>
>> CPU view of memory : 0x0000 0008 0000 0000 to 0x0000 000a 0000 0000
>>
>> From above memory range, first 2 GB of memory has an alias 32 bit
>> view in the hardware. Hardware internally map the address issued within
>> that first 2 GB to same memory.
>>
>> DMA view of first 2 GB [ 0x0000 0008 0000 0000 to 0x0000 0008 7fff ffff]
>> is : 0x8000 0000 to 0xffff fffff.
>
> Are you setting ZONE_DMA to be 2GB so allocations stay within DMAable
> memory and that is enough that you don't need to set DMA masks?
>
Yes. Thats right.
Regards,
Santosh
Dear Santosh Shilimkar,
On Mon, 21 Apr 2014 09:35:25 -0400, Santosh Shilimkar wrote:
> > In mach-mvebu, what we do is that we register a bus notifier on the
> > platform bus, so that we can set our custom DMA operations for all
> > platform devices in the system. Should this be done in a different way
> > after your series?
> >
> Nope. Since you have a very custom SOC specific case, you can continue
> what you are doing.
True, but as you said, the goal is to remove machine code. So instead
of having just a 'dma-coherent' property, shouldn't we have a
dma-method property, which could be dma-method = "coherent" or
dma-method = "marvell,io-coherent" and therefore allow the DT binding
to cover more use cases than just the default non-coherent and coherent
DMA operations?
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On Monday 21 April 2014 02:19 PM, Thomas Petazzoni wrote:
> Dear Santosh Shilimkar,
>
> On Mon, 21 Apr 2014 09:35:25 -0400, Santosh Shilimkar wrote:
>
>>> In mach-mvebu, what we do is that we register a bus notifier on the
>>> platform bus, so that we can set our custom DMA operations for all
>>> platform devices in the system. Should this be done in a different way
>>> after your series?
>>>
>> Nope. Since you have a very custom SOC specific case, you can continue
>> what you are doing.
>
> True, but as you said, the goal is to remove machine code. So instead
> of having just a 'dma-coherent' property, shouldn't we have a
> dma-method property, which could be dma-method = "coherent" or
> dma-method = "marvell,io-coherent" and therefore allow the DT binding
> to cover more use cases than just the default non-coherent and coherent
> DMA operations?
>
Please remember the infrastructure we are adding is not really for machines
(sub arch's) but for architectures. I don't think its worth adding methods
The whole reason of dma_ops being exported is take care of cases like yours
so we are just fine with that. If we see more cases likes your, we can
think about that.
regards,
Santosh
On 04/19/2014 09:32 AM, Santosh Shilimkar wrote:
> From: Grygorii Strashko <[email protected]>
[..]
> + * Look in bottom up direction for the first "dma-range" property
> + * and parse it.
> + * dma-ranges format:
> + * DMA addr (dma_addr) : naddr cells
> + * CPU addr (phys_addr_t) : pna cells
> + * size : nsize cells
> + *
> + * It returns -ENODEV if "dma-ranges" property was not found
> + * for this device in DT.
> + */
> +extern int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr,
> + phys_addr_t *paddr, phys_addr_t *size)
> +{
> + struct device_node *node = np;
> + const u32 *ranges = NULL;
> + int len, naddr, nsize, pna;
> + int ret = 0;
> +
> + if (!node)
> + return -EINVAL;
> +
> + while (1) {
> + naddr = of_n_addr_cells(node);
> + nsize = of_n_size_cells(node);
> + node = of_get_next_parent(node);
> + if (!node)
> + break;
> +
> + ranges = of_get_property(node, "dma-ranges", &len);
> +
> + /* Ignore empty ranges, they imply no translation required */
> + if (ranges && len > 0)
> + break;
> +
> + /*
> + * At least empty ranges has to be defined for parent node if
> + * DMA is supported
> + */
> + if (!ranges)
> + break;
> + }
> +
> + if (!ranges) {
> + pr_debug("%s: no dma-ranges found for node(%s)\n",
> + __func__, np->full_name);
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + len /= sizeof(u32);
> +
> + pna = of_n_addr_cells(node);
> +
> + /* dma-ranges format:
> + * DMA addr : naddr cells
> + * CPU addr : pna cells
> + * size : nsize cells
> + */
> + *dma_addr = of_read_number(ranges, naddr);
> + *paddr = of_translate_dma_address(np, ranges);
I am probably missing something but I'm wondering the need for a
translate step here instead of doing something like:
*paddr = of_read_number(ranges + naddr, pna);
Perhaps there is a need to do a translate step of the DMA Address in
dma-ranges all the way to the parent, which can be different from the
CPU Address (second address in dma-ranges).
in which case the format of dma-ranges after parsing looks like
<DMA-Addr Translate(DMA-Addr) Size>
to the caller, and not, <DMA-Addr CPU-Addr Size>
But for keystone if something like the following is used,
dma-ranges = <0x80000000 0x8 0x00000000 0x80000000>;
Then, the above fragment I proposed would return 0x8 0x00000000 which is
sufficient?
thanks,
-Joel
> + if (*paddr == OF_BAD_ADDR) {
> + pr_err("%s: translation of DMA address(%pad) to CPU address failed node(%s)\n",
> + __func__, dma_addr, np->full_name);
> + ret = -EINVAL;
> + }
> +
> + *size = of_read_number(ranges + naddr + pna, nsize);
> +
> + pr_debug("dma_addr(%pad) cpu_addr(%pa) size(%pa)\n",
> + dma_addr, paddr, size);
> +
> +out:
> + of_node_put(node);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(of_dma_get_range);
> #endif /* CONFIG_OF_ADDRESS */
> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
> index 05cb4a9..ba7d3dc 100644
> --- a/include/linux/of_platform.h
> +++ b/include/linux/of_platform.h
> @@ -72,6 +72,8 @@ extern int of_platform_populate(struct device_node *root,
> const struct of_device_id *matches,
> const struct of_dev_auxdata *lookup,
> struct device *parent);
> +extern int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr,
> + phys_addr_t *paddr, phys_addr_t *size);
> #else
> static inline int of_platform_populate(struct device_node *root,
> const struct of_device_id *matches,
> @@ -80,6 +82,12 @@ static inline int of_platform_populate(struct device_node *root,
> {
> return -ENODEV;
> }
> +
> +static inline int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr,
> + phys_addr_t *paddr, phys_addr_t *size)
> +{
> + return -ENODEV;
> +}
> #endif
>
> #endif /* _LINUX_OF_PLATFORM_H */
>
Hi Rob,
On 04/21/2014 06:29 PM, Rob Herring wrote:
> On Sat, Apr 19, 2014 at 9:32 AM, Santosh Shilimkar
> <[email protected]> wrote:
>> From: Grygorii Strashko <[email protected]>
>>
>> The of_dma_get_range() allows to find "dma-range" property for
>> the specified device and parse it.
>> dma-ranges format:
>> DMA addr (dma_addr) : naddr cells
>> CPU addr (phys_addr_t) : pna cells
>> size : nsize cells
>>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: Russell King <[email protected]>
>> Cc: Arnd Bergmann <[email protected]>
>> Cc: Olof Johansson <[email protected]>
>> Cc: Grant Likely <[email protected]>
>> Cc: Rob Herring <[email protected]>
>> Cc: Catalin Marinas <[email protected]>
>> Cc: Linus Walleij <[email protected]>
>> Signed-off-by: Grygorii Strashko <[email protected]>
>> Signed-off-by: Santosh Shilimkar <[email protected]>
>> ---
>> drivers/of/platform.c | 85 +++++++++++++++++++++++++++++++++++++++++++
>> include/linux/of_platform.h | 8 ++++
>
> This belongs in drivers/of/address.c and of_address.h.
>
>> 2 files changed, 93 insertions(+)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 404d1da..2265a55 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -485,4 +485,89 @@ int of_platform_populate(struct device_node *root,
>> return rc;
>> }
>> EXPORT_SYMBOL_GPL(of_platform_populate);
>> +
>> +/**
>> + * of_dma_get_range - Get DMA range info
>> + * @np: device node to get DMA range info
>> + * @dma_addr: pointer to store initial DMA address of DMA range
>> + * @paddr: pointer to store initial CPU address of DMA range
>> + * @size: pointer to store size of DMA range
>> + *
>> + * Look in bottom up direction for the first "dma-range" property
>
> dma-ranges
>
>> + * and parse it.
>> + * dma-ranges format:
>> + * DMA addr (dma_addr) : naddr cells
>> + * CPU addr (phys_addr_t) : pna cells
>> + * size : nsize cells
>> + *
>> + * It returns -ENODEV if "dma-ranges" property was not found
>> + * for this device in DT.
>> + */
>> +extern int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr,
>
> drop extern.
>
>> + phys_addr_t *paddr, phys_addr_t *size)
>> +{
>> + struct device_node *node = np;
>> + const u32 *ranges = NULL;
>
> __be32
>
>> + int len, naddr, nsize, pna;
>> + int ret = 0;
>> +
>> + if (!node)
>> + return -EINVAL;
>> +
>> + while (1) {
>> + naddr = of_n_addr_cells(node);
>> + nsize = of_n_size_cells(node);
>> + node = of_get_next_parent(node);
>> + if (!node)
>> + break;
>> +
>> + ranges = of_get_property(node, "dma-ranges", &len);
>> +
>> + /* Ignore empty ranges, they imply no translation required */
>> + if (ranges && len > 0)
>> + break;
>> +
>> + /*
>> + * At least empty ranges has to be defined for parent node if
>> + * DMA is supported
>> + */
>> + if (!ranges)
>> + break;
>
> You need an of_node_put here if you are getting the next parent.
Seems, It's not needed, because of_get_next_parent() does following:
parent = of_node_get(node->parent);
of_node_put(node);
>
>> + }
>> +
>> + if (!ranges) {
>> + pr_debug("%s: no dma-ranges found for node(%s)\n",
>> + __func__, np->full_name);
>> + ret = -ENODEV;
>> + goto out;
>> + }
>> +
>> + len /= sizeof(u32);
>> +
>> + pna = of_n_addr_cells(node);
>> +
>> + /* dma-ranges format:
>> + * DMA addr : naddr cells
>> + * CPU addr : pna cells
>> + * size : nsize cells
>> + */
>> + *dma_addr = of_read_number(ranges, naddr);
>
> You should set this after the following call succeeds.
>
>> + *paddr = of_translate_dma_address(np, ranges);
>
> This is going to search for dma-ranges again, but you have already
> done that. Is there a case that just reading the CPU addr will not
> work? I suppose we could have more than one level of translation. It
> also seems a bit abusive that the DMA address is the ranges property.
> I don't really have a better suggestion though.
I think, We need to keep using of_translate_dma_address(), because
this code intended to be generic and, as result, there is no
guarantee that we will always have one level of translations.
>
>> + if (*paddr == OF_BAD_ADDR) {
>
> As I mentioned, this is potentially a size mismatch.
Thanks for your review.
Regards,
Grygorii
Hi Rob,
On 04/21/2014 05:58 PM, Rob Herring wrote:
> On Sat, Apr 19, 2014 at 9:32 AM, Santosh Shilimkar
> <[email protected]> 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 <[email protected]>
>> Cc: Russell King <[email protected]>
>> Cc: Arnd Bergmann <[email protected]>
>> Cc: Olof Johansson <[email protected]>
>> Cc: Grant Likely <[email protected]>
>> Cc: Rob Herring <[email protected]>
>> Cc: Catalin Marinas <[email protected]>
>> Cc: Linus Walleij <[email protected]>
>> Signed-off-by: Grygorii Strashko <[email protected]>
>> Signed-off-by: Santosh Shilimkar <[email protected]>
>> ---
>> drivers/of/platform.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>> include/linux/dma-mapping.h | 7 +++++++
>> 2 files changed, 50 insertions(+)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 573db15..7e4a43b 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -187,6 +187,47 @@ 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)
>> +{
>> + dma_addr_t dma_addr;
>> + phys_addr_t paddr, size;
>
> A problem with using dma_addr_t and phys_addr_t is that they normally
> depend on CONFIG_LPAE, but the address cell sizes are independent of
> that. That is why all the memory related functions stick with u64. I
> think you would have issues here if you have a platform with sizes of
> 4GB or more. You need to properly cap the sizes and addresses.
>
>> + int ret;
>> +
>> + /*
>> + * 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 < 0) {
>
> Perhaps an error is not the right return for the default case. The
> default should probably be dma_addr and paddr equal to 0 and size 4GB.
The error code is needed here to properly distinguish the case when
there are no "dma-ranges" defined in DT. Also, I think, that
of_dma_get_range() shouldn't return any default values - It just
has to get data from DT. And the caller should decide what to do
with this data and how to handle error cases.
So, I prefer to keep behavior as is:
- in case of failure of_dma_get_range() will not touch values of
&dma_addr, &paddr, &size.
>
>> + dev_dbg(dev, "no dma range information to setup\n");
>> + return;
>> + } else {
>
> You don't need the else here.
>
[...]
Thanks for your comments.
Regards,
-Grygorii
On Tue, Apr 22, 2014 at 10:09 AM, Grygorii Strashko
<[email protected]> wrote:
> Hi Rob,
>
> On 04/21/2014 05:58 PM, Rob Herring wrote:
>> On Sat, Apr 19, 2014 at 9:32 AM, Santosh Shilimkar
>> <[email protected]> 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.
[...]
>>>
>>> + ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
>>> + if (ret < 0) {
>>
>> Perhaps an error is not the right return for the default case. The
>> default should probably be dma_addr and paddr equal to 0 and size 4GB.
>
> The error code is needed here to properly distinguish the case when
> there are no "dma-ranges" defined in DT. Also, I think, that
> of_dma_get_range() shouldn't return any default values - It just
> has to get data from DT. And the caller should decide what to do
> with this data and how to handle error cases.
>
> So, I prefer to keep behavior as is:
> - in case of failure of_dma_get_range() will not touch values of
> &dma_addr, &paddr, &size.
Fine, but that is not how of_dma_get_range currently behaves:
+ *dma_addr = of_read_number(ranges, naddr);
+ *paddr = of_translate_dma_address(np, ranges);
+ if (*paddr == OF_BAD_ADDR) {
+ pr_err("%s: translation of DMA address(%pad) to CPU
address failed node(%s)\n",
+ __func__, dma_addr, np->full_name);
+ ret = -EINVAL;
+ }
Rob
On 04/22/2014 05:44 PM, Rob Herring wrote:
> On Tue, Apr 22, 2014 at 10:09 AM, Grygorii Strashko
> <[email protected]> wrote:
>> Hi Rob,
>>
>> On 04/21/2014 05:58 PM, Rob Herring wrote:
>>> On Sat, Apr 19, 2014 at 9:32 AM, Santosh Shilimkar
>>> <[email protected]> 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.
>
> [...]
>
>>>>
>>>> + ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
>>>> + if (ret < 0) {
>>>
>>> Perhaps an error is not the right return for the default case. The
>>> default should probably be dma_addr and paddr equal to 0 and size 4GB.
>>
>> The error code is needed here to properly distinguish the case when
>> there are no "dma-ranges" defined in DT. Also, I think, that
>> of_dma_get_range() shouldn't return any default values - It just
>> has to get data from DT. And the caller should decide what to do
>> with this data and how to handle error cases.
>>
>> So, I prefer to keep behavior as is:
>> - in case of failure of_dma_get_range() will not touch values of
>> &dma_addr, &paddr, &size.
>
> Fine, but that is not how of_dma_get_range currently behaves:
Yep. That's will be fixed as you've commented :)
>
> + *dma_addr = of_read_number(ranges, naddr);
> + *paddr = of_translate_dma_address(np, ranges);
> + if (*paddr == OF_BAD_ADDR) {
> + pr_err("%s: translation of DMA address(%pad) to CPU
> address failed node(%s)\n",
> + __func__, dma_addr, np->full_name);
> + ret = -EINVAL;
> + }
>
> Rob
>
Regards
Grygorii
On Sat, Apr 19, 2014 at 05:25:28PM +0100, Thomas Petazzoni wrote:
> On Sat, 19 Apr 2014 10:32:45 -0400, Santosh Shilimkar wrote:
> > Here is an updated version of [2] based on discussion. Series introduces
> > support for setting up dma parameters based on device tree properties
> > like 'dma-ranges' and 'dma-coherent' and also update to ARM 32 bit port.
> > Earlier version of the same series is here [1].
> >
> > The 'dma-ranges' helps to take care of few DMAable system memory restrictions
> > by use of dma_pfn_offset which we maintain now per device. Arch code then
> > uses it for dma address translations for such cases. We update the
> > dma_pfn_offset accordingly during DT the device creation process.The
> > 'dma-coherent' property is used to setup arch's coherent dma_ops.
> >
> > After some off-list discussion with RMK and Arnd, I have now dropped the
> > controversial dma_mask setup code from the series which actually isn't blocking
> > me as such. Considering rest of the parts of the series are already aligned,
> > am hoping to get this version merged for 3.16 merge window.
> >
> > We agreed in last discussion that drivers have the ultimate
> > responsibility to setup the correct dma mask but then we need to have some
> > means to see if bus can support what driver has requested for a case where
> > driver request for bigger mask than what bus supports. I can follow up on
> > the mask topic if we have broken drivers.
>
> I am not sure whether there is an intersection or not, but I wanted to
> mention that the mvebu platform (in mach-mvebu) supports hardware I/O
> coherency, which makes it a coherent DMA platform. However, we are not
> able to use arm_coherent_dma_ops for this platform, because when a
> transfer is being made DMA_FROM_DEVICE, at the end of the transfer, we
> need to perform an I/O barrier to wait for the snooping unit to
> complete its coherency work. So we're coherent, but not with
> arm_coherent_dma_ops: we have our own dma operation implementation (see
> arch/arm/mach-mvebu/coherency.c).
Ordering between I/O, DMA and CPU memory accesses is the reason we added
rmb() to the readl() macro. The mvebu ops solve the DMA streaming case
but not the dma_alloc() buffers case where you no longer have a change
of ownership between device and CPU. We could handle this via per-SoC
__io*mb() barriers as function pointers with a bit of overhead (though
we already do an outer_sync() for wmb()).
--
Catalin
On Saturday 19 April 2014, Thomas Petazzoni wrote:
>
> I am not sure whether there is an intersection or not, but I wanted to
> mention that the mvebu platform (in mach-mvebu) supports hardware I/O
> coherency, which makes it a coherent DMA platform. However, we are not
> able to use arm_coherent_dma_ops for this platform, because when a
> transfer is being made DMA_FROM_DEVICE, at the end of the transfer, we
> need to perform an I/O barrier to wait for the snooping unit to
> complete its coherency work. So we're coherent, but not with
> arm_coherent_dma_ops: we have our own dma operation implementation (see
> arch/arm/mach-mvebu/coherency.c).
I had completely missed the fact that this support was merged already.
It's an interesting question if this should actually be called
'coherent' or not. It's certainly more coherent than without that
support, but then again, you still can't rely on incoming data to
be visible after a readl() from the device has returned or an MSI
interrupt has been delivered, which is what we normally expect.
In particular, it means you can't really use arm_coherent_dma_alloc(),
which is a shame, since that is a significante performance overhead.
I would hope we can find a way to avoid the platform notifiers for
mvebu as well and come up with a generic way to express this
'semi-coherent' mode. I believe x-gene has a similar issue, and
I wouldn't be surprised if there are others like this.
Arnd
On Tuesday 22 April 2014 11:02 AM, Arnd Bergmann wrote:
> On Saturday 19 April 2014, Thomas Petazzoni wrote:
>>
>> I am not sure whether there is an intersection or not, but I wanted to
>> mention that the mvebu platform (in mach-mvebu) supports hardware I/O
>> coherency, which makes it a coherent DMA platform. However, we are not
>> able to use arm_coherent_dma_ops for this platform, because when a
>> transfer is being made DMA_FROM_DEVICE, at the end of the transfer, we
>> need to perform an I/O barrier to wait for the snooping unit to
>> complete its coherency work. So we're coherent, but not with
>> arm_coherent_dma_ops: we have our own dma operation implementation (see
>> arch/arm/mach-mvebu/coherency.c).
>
> I had completely missed the fact that this support was merged already.
>
> It's an interesting question if this should actually be called
> 'coherent' or not. It's certainly more coherent than without that
> support, but then again, you still can't rely on incoming data to
> be visible after a readl() from the device has returned or an MSI
> interrupt has been delivered, which is what we normally expect.
>
> In particular, it means you can't really use arm_coherent_dma_alloc(),
> which is a shame, since that is a significante performance overhead.
>
> I would hope we can find a way to avoid the platform notifiers for
> mvebu as well and come up with a generic way to express this
> 'semi-coherent' mode. I believe x-gene has a similar issue, and
> I wouldn't be surprised if there are others like this.
>
As Catalin already pointed out, the mvebu issue really the barrier
than dma coherency. Infact for all the correct operations, they
need the __io_*mb() to be patched up same way as we patched up
the outer_sync() for Cortex-A9 implementation. With that in place
and the other dma streaming barrier patch [1] I posted, mvebu
case should work.
I don't think, the 'semi-coherent' makes much sense because
you can many parameters influencing that. Thanks to per
device property, we already do say" PCIE is not cohenernt
but USB, NETWORK drivers are on same SOC". Anything beyond
that will be non-scalable and won't be generic enough.
[1] http://www.spinics.net/lists/arm-kernel/msg324109.html
On Tue, Apr 22, 2014 at 04:02:19PM +0100, Arnd Bergmann wrote:
> On Saturday 19 April 2014, Thomas Petazzoni wrote:
> > I am not sure whether there is an intersection or not, but I wanted to
> > mention that the mvebu platform (in mach-mvebu) supports hardware I/O
> > coherency, which makes it a coherent DMA platform. However, we are not
> > able to use arm_coherent_dma_ops for this platform, because when a
> > transfer is being made DMA_FROM_DEVICE, at the end of the transfer, we
> > need to perform an I/O barrier to wait for the snooping unit to
> > complete its coherency work. So we're coherent, but not with
> > arm_coherent_dma_ops: we have our own dma operation implementation (see
> > arch/arm/mach-mvebu/coherency.c).
>
> I had completely missed the fact that this support was merged already.
>
> It's an interesting question if this should actually be called
> 'coherent' or not. It's certainly more coherent than without that
> support, but then again, you still can't rely on incoming data to
> be visible after a readl() from the device has returned or an MSI
> interrupt has been delivered, which is what we normally expect.
>
> In particular, it means you can't really use arm_coherent_dma_alloc(),
> which is a shame, since that is a significante performance overhead.
It should still work if __io*mb() macros do the extra work specific to
mvebu (similar to the L2x0 outer_sync(), though this was for
non-cacheable DMA buffers).
> I would hope we can find a way to avoid the platform notifiers for
> mvebu as well and come up with a generic way to express this
> 'semi-coherent' mode. I believe x-gene has a similar issue, and
> I wouldn't be surprised if there are others like this.
The solution is for the snooping unit to detect the DSB instruction
(which is propagated outside the CPU) and wait for the completion of the
coherency work (but we need more information from the hardware guys).
--
Catalin
On Tue, Apr 22, 2014 at 10:25 AM, Catalin Marinas
<[email protected]> wrote:
> On Tue, Apr 22, 2014 at 04:02:19PM +0100, Arnd Bergmann wrote:
>> On Saturday 19 April 2014, Thomas Petazzoni wrote:
[...]
>> I would hope we can find a way to avoid the platform notifiers for
>> mvebu as well and come up with a generic way to express this
>> 'semi-coherent' mode. I believe x-gene has a similar issue, and
>> I wouldn't be surprised if there are others like this.
>
> The solution is for the snooping unit to detect the DSB instruction
> (which is propagated outside the CPU) and wait for the completion of the
> coherency work (but we need more information from the hardware guys).
If the solution was fixing broken h/w, we'd all be retired (or h/w
designers). :)
Rob
On Tue, Apr 22, 2014 at 04:30:36PM +0100, Rob Herring wrote:
> On Tue, Apr 22, 2014 at 10:25 AM, Catalin Marinas
> <[email protected]> wrote:
> > On Tue, Apr 22, 2014 at 04:02:19PM +0100, Arnd Bergmann wrote:
> >> On Saturday 19 April 2014, Thomas Petazzoni wrote:
>
> [...]
>
> >> I would hope we can find a way to avoid the platform notifiers for
> >> mvebu as well and come up with a generic way to express this
> >> 'semi-coherent' mode. I believe x-gene has a similar issue, and
> >> I wouldn't be surprised if there are others like this.
> >
> > The solution is for the snooping unit to detect the DSB instruction
> > (which is propagated outside the CPU) and wait for the completion of the
> > coherency work (but we need more information from the hardware guys).
>
> If the solution was fixing broken h/w, we'd all be retired (or h/w
> designers). :)
At least they could admit it's a hardware bug and hopefully won't do the
same mistake in the future (wishful thinking ;)).
--
Catalin