* [PATCH 0/4] ARM: mm: Use dma-ranges for dma address translation
@ 2014-02-24 20:53 Santosh Shilimkar
2014-02-24 20:53 ` [PATCH 1/4] ARM: mm: Introduce archdata.dma_pfn_offset Santosh Shilimkar
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Santosh Shilimkar @ 2014-02-24 20:53 UTC (permalink / raw)
To: linux-arm-kernel
Following the RFC [1] comments and suggestions from Olof, Arnd, here is the
patchset tries to make use of dma-ranges property for dma address translations.
We add dma_pfn_offset to ARM archdata which can be updated per platform device
using a callback like dma-coherent callbacks.
I would like to get the fix for the problem in kernel sooner so appriciate
early feedback.
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Greg Ungerer <gerg@uclinux.org>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
CC: Grygorii Strashko <grygorii.strashko@ti.com>
Grygorii Strashko (4):
ARM: mm: Introduce archdata.dma_pfn_offset
ARM: mm: Remove unsed dma_to_virt()
ARM: dts: keystone: Use dma-ranges property
ARM: keystone: Use dma-ranges for dma_pfn_offset configuration
arch/arm/boot/dts/keystone.dtsi | 1 +
arch/arm/include/asm/device.h | 1 +
arch/arm/include/asm/dma-mapping.h | 25 ++++-----
arch/arm/mach-iop13xx/include/mach/memory.h | 11 ----
arch/arm/mach-keystone/keystone.c | 76 +++++++++++++++++++++++++--
arch/arm/mach-ks8695/include/mach/memory.h | 2 -
arch/arm/mach-omap1/include/mach/memory.h | 4 --
7 files changed, 86 insertions(+), 34 deletions(-)
Regards,
Santosh
[1] http://www.spinics.net/lists/arm-kernel/msg304555.html
--
1.7.9.5
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/4] ARM: mm: Introduce archdata.dma_pfn_offset
2014-02-24 20:53 [PATCH 0/4] ARM: mm: Use dma-ranges for dma address translation Santosh Shilimkar
@ 2014-02-24 20:53 ` Santosh Shilimkar
2014-02-24 20:53 ` [PATCH 2/4] ARM: mm: Remove unsed dma_to_virt() Santosh Shilimkar
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Santosh Shilimkar @ 2014-02-24 20:53 UTC (permalink / raw)
To: linux-arm-kernel
From: Grygorii Strashko <grygorii.strashko@ti.com>
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
This patch introduces new field dma_pfn_offset in ARM dev_archdata
structure which has to be filed per-device at arch init time
(simplest way is to use Platform bus notifier to handle
BUS_NOTIFY_ADD_DEVICE event) and updates DMA address translation
routines in order to accommodate bus offset value by default.
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
arch/arm/include/asm/device.h | 1 +
arch/arm/include/asm/dma-mapping.h | 17 +++++++++++++----
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h
index dc662fc..861961c 100644
--- a/arch/arm/include/asm/device.h
+++ b/arch/arm/include/asm/device.h
@@ -8,6 +8,7 @@
struct dev_archdata {
struct dma_map_ops *dma_ops;
+ unsigned long dma_pfn_offset;
#ifdef CONFIG_DMABOUNCE
struct dmabounce_device_info *dmabounce;
#endif
diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index e701a4d..247ed72 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->archdata.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->archdata.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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/4] ARM: mm: Remove unsed dma_to_virt()
2014-02-24 20:53 [PATCH 0/4] ARM: mm: Use dma-ranges for dma address translation Santosh Shilimkar
2014-02-24 20:53 ` [PATCH 1/4] ARM: mm: Introduce archdata.dma_pfn_offset Santosh Shilimkar
@ 2014-02-24 20:53 ` Santosh Shilimkar
2014-02-24 20:53 ` [PATCH 3/4] ARM: dts: keystone: Use dma-ranges property Santosh Shilimkar
2014-02-24 20:53 ` [PATCH 4/4] ARM: keystone: Use dma-ranges for dma_pfn_offset configuration Santosh Shilimkar
3 siblings, 0 replies; 13+ messages in thread
From: Santosh Shilimkar @ 2014-02-24 20:53 UTC (permalink / raw)
To: linux-arm-kernel
From: Grygorii Strashko <grygorii.strashko@ti.com>
Remove dma_to_virt() as there are no in-tree users of it.
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
Cc: Greg Ungerer <gerg@uclinux.org>
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
arch/arm/include/asm/dma-mapping.h | 14 +-------------
arch/arm/mach-iop13xx/include/mach/memory.h | 11 -----------
arch/arm/mach-ks8695/include/mach/memory.h | 2 --
arch/arm/mach-omap1/include/mach/memory.h | 4 ----
4 files changed, 1 insertion(+), 30 deletions(-)
diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index 247ed72..e365158 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -51,7 +51,7 @@ static inline int dma_set_mask(struct device *dev, u64 mask)
#endif
/*
- * dma_to_pfn/pfn_to_dma/dma_to_virt/virt_to_dma are architecture private
+ * dma_to_pfn/pfn_to_dma/virt_to_dma are architecture private
* functions used internally by the DMA-mapping API to provide DMA
* addresses. They must not be used by drivers.
*/
@@ -70,13 +70,6 @@ static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr)
return __bus_to_pfn(addr) + dev->archdata.dma_pfn_offset;
}
-static inline void *dma_to_virt(struct device *dev, dma_addr_t 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)
{
if (!dev)
@@ -96,11 +89,6 @@ static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr)
return __arch_dma_to_pfn(dev, addr);
}
-static inline void *dma_to_virt(struct device *dev, dma_addr_t addr)
-{
- return __arch_dma_to_virt(dev, addr);
-}
-
static inline dma_addr_t virt_to_dma(struct device *dev, void *addr)
{
return __arch_virt_to_dma(dev, addr);
diff --git a/arch/arm/mach-iop13xx/include/mach/memory.h b/arch/arm/mach-iop13xx/include/mach/memory.h
index 7c032d0..1223c85 100644
--- a/arch/arm/mach-iop13xx/include/mach/memory.h
+++ b/arch/arm/mach-iop13xx/include/mach/memory.h
@@ -36,17 +36,6 @@ static inline void __iomem *__lbus_to_virt(dma_addr_t x)
#define is_lbus_device(dev) \
(dev && strncmp(dev->bus->name, "platform", 8) == 0)
-#define __arch_dma_to_virt(dev, addr) \
- ({ \
- void * __virt; \
- dma_addr_t __dma = addr; \
- if (is_lbus_device(dev) && __is_lbus_dma(__dma)) \
- __virt = __lbus_to_virt(__dma); \
- else \
- __virt = (void *)__phys_to_virt(__dma); \
- __virt; \
- })
-
#define __arch_virt_to_dma(dev, addr) \
({ \
void * __virt = addr; \
diff --git a/arch/arm/mach-ks8695/include/mach/memory.h b/arch/arm/mach-ks8695/include/mach/memory.h
index 95e731a..f42477c 100644
--- a/arch/arm/mach-ks8695/include/mach/memory.h
+++ b/arch/arm/mach-ks8695/include/mach/memory.h
@@ -31,8 +31,6 @@
/* Platform-bus mapping */
extern struct bus_type platform_bus_type;
#define is_lbus_device(dev) (dev && dev->bus == &platform_bus_type)
-#define __arch_dma_to_virt(dev, x) ({ (void *) (is_lbus_device(dev) ? \
- __phys_to_virt(x) : __bus_to_virt(x)); })
#define __arch_virt_to_dma(dev, x) ({ is_lbus_device(dev) ? \
(dma_addr_t)__virt_to_phys((unsigned long)x) \
: (dma_addr_t)__virt_to_bus(x); })
diff --git a/arch/arm/mach-omap1/include/mach/memory.h b/arch/arm/mach-omap1/include/mach/memory.h
index 3c25305..73f86db 100644
--- a/arch/arm/mach-omap1/include/mach/memory.h
+++ b/arch/arm/mach-omap1/include/mach/memory.h
@@ -43,10 +43,6 @@
__phys_to_pfn(__dma); \
})
-#define __arch_dma_to_virt(dev, addr) ({ (void *) (is_lbus_device(dev) ? \
- lbus_to_virt(addr) : \
- __phys_to_virt(addr)); })
-
#define __arch_virt_to_dma(dev, addr) ({ unsigned long __addr = (unsigned long)(addr); \
(dma_addr_t) (is_lbus_device(dev) ? \
virt_to_lbus(__addr) : \
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/4] ARM: dts: keystone: Use dma-ranges property
2014-02-24 20:53 [PATCH 0/4] ARM: mm: Use dma-ranges for dma address translation Santosh Shilimkar
2014-02-24 20:53 ` [PATCH 1/4] ARM: mm: Introduce archdata.dma_pfn_offset Santosh Shilimkar
2014-02-24 20:53 ` [PATCH 2/4] ARM: mm: Remove unsed dma_to_virt() Santosh Shilimkar
@ 2014-02-24 20:53 ` Santosh Shilimkar
2014-02-24 20:53 ` [PATCH 4/4] ARM: keystone: Use dma-ranges for dma_pfn_offset configuration Santosh Shilimkar
3 siblings, 0 replies; 13+ messages in thread
From: Santosh Shilimkar @ 2014-02-24 20:53 UTC (permalink / raw)
To: linux-arm-kernel
From: Grygorii Strashko <grygorii.strashko@ti.com>
The dma-ranges property has to be specified per bus and has format:
< DMA addr > - Base DMA address for Bus (Bus format 32-bits)
< CPU addr > - Corresponding base CPU address (CPU format 64-bits)
< DMA range size > - Size of supported DMA range
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
arch/arm/boot/dts/keystone.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/keystone.dtsi b/arch/arm/boot/dts/keystone.dtsi
index b420290..171579d 100644
--- a/arch/arm/boot/dts/keystone.dtsi
+++ b/arch/arm/boot/dts/keystone.dtsi
@@ -96,6 +96,7 @@
compatible = "ti,keystone","simple-bus";
interrupt-parent = <&gic>;
ranges = <0x0 0x0 0x0 0xc0000000>;
+ dma-ranges = <0x80000000 0x8 0x00000000 0x80000000>;
rstctrl: reset-controller {
compatible = "ti,keystone-reset";
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/4] ARM: keystone: Use dma-ranges for dma_pfn_offset configuration
2014-02-24 20:53 [PATCH 0/4] ARM: mm: Use dma-ranges for dma address translation Santosh Shilimkar
` (2 preceding siblings ...)
2014-02-24 20:53 ` [PATCH 3/4] ARM: dts: keystone: Use dma-ranges property Santosh Shilimkar
@ 2014-02-24 20:53 ` Santosh Shilimkar
2014-02-24 21:11 ` Arnd Bergmann
3 siblings, 1 reply; 13+ messages in thread
From: Santosh Shilimkar @ 2014-02-24 20:53 UTC (permalink / raw)
To: linux-arm-kernel
From: Grygorii Strashko <grygorii.strashko@ti.com>
Keystone platforms have their physical memory mapped at an address
outside the 32-bit physical range. A Keystone machine with 16G of RAM
would find its memory at 0x0800000000 - 0x0bffffffff.
The system interconnect allows to perform DMA transfers from first 2G of
physical memory (0x08 0000 0000 to 08 7FFF FFFF) which aliased in
hardware to the 32-bit addressable space (0x80000000 - 0xffffffff),
because DMA HW supports only 32-bits addressing.
Get DMA range configuration from DT and use it to configure per
device dma_pfn_offset. DMA addresses translation will be performed
as following:
_dev->archdata.dma_pfn_offset = PFN_DOWN(<DMA addr> - <CPU addr>);
DMA->PFN:
__bus_to_pfn(addr) + dev->archdata.dma_pfn_offset
PFN->DMA:
__pfn_to_bus(pfn - dev->archdata.dma_pfn_offset)
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
arch/arm/mach-keystone/keystone.c | 76 +++++++++++++++++++++++++++++++++++--
1 file changed, 73 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-keystone/keystone.c b/arch/arm/mach-keystone/keystone.c
index e735555..ebe0a66 100644
--- a/arch/arm/mach-keystone/keystone.c
+++ b/arch/arm/mach-keystone/keystone.c
@@ -33,6 +33,7 @@
static void __iomem *keystone_rstctrl;
static struct notifier_block platform_nb;
+static unsigned long keystone_dma_pfn_offset __read_mostly;
static bool is_coherent(struct device *dev)
{
@@ -48,15 +49,82 @@ static bool is_coherent(struct device *dev)
return false;
}
+static unsigned long get_dma_pfn_offset(struct device *dev)
+{
+ struct device_node *node = of_node_get(dev->of_node);
+ const u32 *ranges = NULL;
+ int len, naddr, nsize, pna;
+ dma_addr_t dma_addr;
+ phys_addr_t cpu_addr, size;
+ unsigned long dma_pfn_offset = 0;
+
+ if (!node)
+ return 0;
+
+ 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;
+ }
+
+ if (!ranges) {
+ dev_dbg(dev, "no dma-ranges found\n");
+ goto out;
+ }
+
+ /*
+ * dma-ranges format:
+ * DMA addr : naddr cells
+ * CPU addr : pna cells
+ * size : nsize cells
+ */
+ len /= sizeof(u32);
+ pna = of_n_addr_cells(node);
+ dma_addr = of_read_number(ranges, nsize);
+ cpu_addr = of_translate_dma_address(dev->of_node, ranges);
+ size = of_read_number(ranges + naddr + pna, nsize);
+ dma_pfn_offset = PFN_DOWN(cpu_addr - dma_addr);
+
+ dev_dbg(dev, "%s: naddr-%u nsize-%u pna-%u len-%u\n",
+ node->name, naddr, nsize, pna, len);
+ dev_dbg(dev, "dma_addr-%08x cpu_addr-%pa size-%pa dma_pfn_offset-%08lx\n",
+ dma_addr, &cpu_addr, &size, dma_pfn_offset);
+
+out:
+ of_node_put(node);
+ return dma_pfn_offset;
+}
+
static int keystone_platform_notifier(struct notifier_block *nb,
unsigned long event, void *dev)
{
+ struct device *_dev = dev;
+
if (event != BUS_NOTIFY_ADD_DEVICE)
return NOTIFY_DONE;
- if (is_coherent(dev)) {
- set_dma_ops(dev, &arm_coherent_dma_ops);
- pr_info("\t\t%s: keystone device is coherent\n", dev_name(dev));
+ if (!_dev)
+ return NOTIFY_BAD;
+
+ if (!_dev->of_node)
+ _dev->archdata.dma_pfn_offset = keystone_dma_pfn_offset;
+ else
+ _dev->archdata.dma_pfn_offset = get_dma_pfn_offset(_dev);
+
+ dev_dbg(_dev, "set dma_pfn_offset %08lx\n",
+ _dev->archdata.dma_pfn_offset);
+
+ if (is_coherent(_dev)) {
+ set_dma_ops(_dev, &arm_coherent_dma_ops);
+ dev_info(_dev, "keystone device is coherent\n");
}
return NOTIFY_OK;
@@ -120,6 +188,8 @@ static void __init keystone_init_meminfo(void)
/* Populate the arch idmap hook */
arch_virt_to_idmap = keystone_virt_to_idmap;
+ keystone_dma_pfn_offset = PFN_DOWN(KEYSTONE_HIGH_PHYS_START -
+ KEYSTONE_LOW_PHYS_START);
platform_nb.notifier_call = keystone_platform_notifier;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/4] ARM: keystone: Use dma-ranges for dma_pfn_offset configuration
2014-02-24 20:53 ` [PATCH 4/4] ARM: keystone: Use dma-ranges for dma_pfn_offset configuration Santosh Shilimkar
@ 2014-02-24 21:11 ` Arnd Bergmann
2014-02-24 21:38 ` Santosh Shilimkar
0 siblings, 1 reply; 13+ messages in thread
From: Arnd Bergmann @ 2014-02-24 21:11 UTC (permalink / raw)
To: linux-arm-kernel
On Monday 24 February 2014 15:53:55 Santosh Shilimkar wrote:
> From: Grygorii Strashko <grygorii.strashko@ti.com>
> }
>
> +static unsigned long get_dma_pfn_offset(struct device *dev)
> +{
> + struct device_node *node = of_node_get(dev->of_node);
> + const u32 *ranges = NULL;
> + int len, naddr, nsize, pna;
> + dma_addr_t dma_addr;
> + phys_addr_t cpu_addr, size;
> + unsigned long dma_pfn_offset = 0;
> +
> + if (!node)
> + return 0;
Hmm, isn't this function the same as of_translate_dma_address()?
I think we should have the implementation in common code, not hidden
in the keystone platform, to avoid duplication. If of_translate_dma_address
doesn't work, what is the problem, and can you fix it there?
> static int keystone_platform_notifier(struct notifier_block *nb,
> unsigned long event, void *dev)
> {
> + struct device *_dev = dev;
> +
> if (event != BUS_NOTIFY_ADD_DEVICE)
> return NOTIFY_DONE;
Style: it would be nicer to name the local variable 'dev' and the
argument something else like 'p' or 'data'.
I also wonder if this shouldn't be in ARM architecture wide code
rather than platform code. Unfortunately it can't be in drivers/base
since the offset is stored in an ARM specific location.
Arnd
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/4] ARM: keystone: Use dma-ranges for dma_pfn_offset configuration
2014-02-24 21:11 ` Arnd Bergmann
@ 2014-02-24 21:38 ` Santosh Shilimkar
2014-02-25 8:35 ` Arnd Bergmann
2014-02-25 14:14 ` Grygorii Strashko
0 siblings, 2 replies; 13+ messages in thread
From: Santosh Shilimkar @ 2014-02-24 21:38 UTC (permalink / raw)
To: linux-arm-kernel
On Monday 24 February 2014 04:11 PM, Arnd Bergmann wrote:
> On Monday 24 February 2014 15:53:55 Santosh Shilimkar wrote:
>> From: Grygorii Strashko <grygorii.strashko@ti.com>
>> }
>>
>> +static unsigned long get_dma_pfn_offset(struct device *dev)
>> +{
>> + struct device_node *node = of_node_get(dev->of_node);
>> + const u32 *ranges = NULL;
>> + int len, naddr, nsize, pna;
>> + dma_addr_t dma_addr;
>> + phys_addr_t cpu_addr, size;
>> + unsigned long dma_pfn_offset = 0;
>> +
>> + if (!node)
>> + return 0;
>
> Hmm, isn't this function the same as of_translate_dma_address()?
>
> I think we should have the implementation in common code, not hidden
> in the keystone platform, to avoid duplication. If of_translate_dma_address
> doesn't work, what is the problem, and can you fix it there?
>
Will have a look at it and get back. At least it makes sense to
use/update the common function.
>> static int keystone_platform_notifier(struct notifier_block *nb,
>> unsigned long event, void *dev)
>> {
>> + struct device *_dev = dev;
>> +
>> if (event != BUS_NOTIFY_ADD_DEVICE)
>> return NOTIFY_DONE;
>
> Style: it would be nicer to name the local variable 'dev' and the
> argument something else like 'p' or 'data'.
>
Agree. Will update that.
> I also wonder if this shouldn't be in ARM architecture wide code
> rather than platform code. Unfortunately it can't be in drivers/base
> since the offset is stored in an ARM specific location.
>
Notifier callback is in mach code mainly because platform's might have to
do custom things here. Like setting up the coherent masters,
populating the dma_pfn_offset or populating custom dma_ops etc.
As such the in these callbacks is not much and I see only
couple of machines using it.
Regards,
Santosh
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/4] ARM: keystone: Use dma-ranges for dma_pfn_offset configuration
2014-02-24 21:38 ` Santosh Shilimkar
@ 2014-02-25 8:35 ` Arnd Bergmann
2014-02-25 14:14 ` Grygorii Strashko
1 sibling, 0 replies; 13+ messages in thread
From: Arnd Bergmann @ 2014-02-25 8:35 UTC (permalink / raw)
To: linux-arm-kernel
On Monday 24 February 2014 16:38:00 Santosh Shilimkar wrote:
> > I also wonder if this shouldn't be in ARM architecture wide code
> > rather than platform code. Unfortunately it can't be in drivers/base
> > since the offset is stored in an ARM specific location.
> >
> Notifier callback is in mach code mainly because platform's might have to
> do custom things here. Like setting up the coherent masters,
> populating the dma_pfn_offset or populating custom dma_ops etc.
> As such the in these callbacks is not much and I see only
> couple of machines using it.
Yes, but since "dma-ranges" is a standard property, I think we should
provide a generic implementation so other platforms don't have to
copy one. Most platforms today are never cache-coherent and have
a trivial mapping, so they don't need this function, but I suppose
the case you have is getting more popular. I think shmobile is in
exactly the same position with their new r-car parts for instance.
Arnd
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/4] ARM: keystone: Use dma-ranges for dma_pfn_offset configuration
2014-02-25 14:14 ` Grygorii Strashko
@ 2014-02-25 13:37 ` Arnd Bergmann
2014-02-25 15:19 ` Grygorii Strashko
0 siblings, 1 reply; 13+ messages in thread
From: Arnd Bergmann @ 2014-02-25 13:37 UTC (permalink / raw)
To: linux-arm-kernel
On Tuesday 25 February 2014, Grygorii Strashko wrote:
> On 02/24/2014 11:38 PM, Santosh Shilimkar wrote:
> > On Monday 24 February 2014 04:11 PM, Arnd Bergmann wrote:
> >> On Monday 24 February 2014 15:53:55 Santosh Shilimkar wrote:
> >>> From: Grygorii Strashko <grygorii.strashko@ti.com>
>
> >> Hmm, isn't this function the same as of_translate_dma_address()?
> >>
> >> I think we should have the implementation in common code, not hidden
> >> in the keystone platform, to avoid duplication. If of_translate_dma_address
> >> doesn't work, what is the problem, and can you fix it there?
> >>
> > Will have a look at it and get back. At least it makes sense to
> > use/update the common function.
>
> This function isn't the same as of_translate_dma_address(), more over
> it calls of_translate_dma_address() to get CPU address.
>
> To calculate DMA bus offset we need to know the first DMA address explicitly,
> so then we can translate it to the corresponding CPU address.
Ah, got it. Sorry for being sloppy in my review. You are absolutely right.
> get_dma_pfn_offset() function does:
> - traverse DT and look for the first "dma-ranges" prop;
> - reads the first DMA address of the range;
> - translates it in CPU address using of_translate_dma_address();
> - calculates DMA bus offset as PFN_DOWN(cpu_addr - dma_addr);
I was confused about the first step here and thought you were actually
doing the translation there. After a more careful reading, I only
have comments on details:
> +static unsigned long get_dma_pfn_offset(struct device *dev)
> +{
> + 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;
> + }
I think we should require an empty "dma-ranges" property to be
present to signify "no translation required" and interpret the
absence of the property as "no dma possible".
A special case is having no "dma-ranges" at all, which we have
on all existing systems, and I would interpret that as "all
devices can do 32-bit DMA, no more but no less".
> + /*
> + * dma-ranges format:
> + * DMA addr : naddr cells
> + * CPU addr : pna cells
> + * size : nsize cells
> + */
> + len /= sizeof(u32);
> + pna = of_n_addr_cells(node);
> + dma_addr = of_read_number(ranges, nsize);
This should probably use naddr, not nsize.
Arnd
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/4] ARM: keystone: Use dma-ranges for dma_pfn_offset configuration
2014-02-24 21:38 ` Santosh Shilimkar
2014-02-25 8:35 ` Arnd Bergmann
@ 2014-02-25 14:14 ` Grygorii Strashko
2014-02-25 13:37 ` Arnd Bergmann
1 sibling, 1 reply; 13+ messages in thread
From: Grygorii Strashko @ 2014-02-25 14:14 UTC (permalink / raw)
To: linux-arm-kernel
Hi Arnd,
On 02/24/2014 11:38 PM, Santosh Shilimkar wrote:
> On Monday 24 February 2014 04:11 PM, Arnd Bergmann wrote:
>> On Monday 24 February 2014 15:53:55 Santosh Shilimkar wrote:
>>> From: Grygorii Strashko <grygorii.strashko@ti.com>
>>> }
>>>
>>> +static unsigned long get_dma_pfn_offset(struct device *dev)
>>> +{
>>> + struct device_node *node = of_node_get(dev->of_node);
>>> + const u32 *ranges = NULL;
>>> + int len, naddr, nsize, pna;
>>> + dma_addr_t dma_addr;
>>> + phys_addr_t cpu_addr, size;
>>> + unsigned long dma_pfn_offset = 0;
>>> +
>>> + if (!node)
>>> + return 0;
>>
>> Hmm, isn't this function the same as of_translate_dma_address()?
>>
>> I think we should have the implementation in common code, not hidden
>> in the keystone platform, to avoid duplication. If of_translate_dma_address
>> doesn't work, what is the problem, and can you fix it there?
>>
> Will have a look at it and get back. At least it makes sense to
> use/update the common function.
This function isn't the same as of_translate_dma_address(), more over
it calls of_translate_dma_address() to get CPU address.
To calculate DMA bus offset we need to know the first DMA address explicitly,
so then we can translate it to the corresponding CPU address.
get_dma_pfn_offset() function does:
- traverse DT and look for the first "dma-ranges" prop;
- reads the first DMA address of the range;
- translates it in CPU address using of_translate_dma_address();
- calculates DMA bus offset as PFN_DOWN(cpu_addr - dma_addr);
Also, I've investigated all places in Kernel were "dma-ranges" is used:
- cell_iommu_get_fixed_address() - arch/powerpc/platforms/cell/iommu.c
- ppc4xx_parse_dma_ranges() - arch/powerpc/sysdev/ppc4xx_pci.c
As result, I see no reasons to modify of_translate_dma_address()
(which actually is just wrapper for common __of_translate_address()).
Of course, possibly I've not fully got your point. :)
>
>>> static int keystone_platform_notifier(struct notifier_block *nb,
>>> unsigned long event, void *dev)
>>> {
>>> + struct device *_dev = dev;
>>> +
>>> if (event != BUS_NOTIFY_ADD_DEVICE)
>>> return NOTIFY_DONE;
>>
>> Style: it would be nicer to name the local variable 'dev' and the
>> argument something else like 'p' or 'data'.
>>
> Agree. Will update that.
>
>> I also wonder if this shouldn't be in ARM architecture wide code
>> rather than platform code. Unfortunately it can't be in drivers/base
>> since the offset is stored in an ARM specific location.
>>
> Notifier callback is in mach code mainly because platform's might have to
> do custom things here. Like setting up the coherent masters,
> populating the dma_pfn_offset or populating custom dma_ops etc.
> As such the in these callbacks is not much and I see only
> couple of machines using it.
>
Thanks for your comments.
Regards,
-grygorii
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/4] ARM: keystone: Use dma-ranges for dma_pfn_offset configuration
2014-02-25 15:19 ` Grygorii Strashko
@ 2014-02-25 14:37 ` Arnd Bergmann
2014-02-25 16:05 ` Grygorii Strashko
0 siblings, 1 reply; 13+ messages in thread
From: Arnd Bergmann @ 2014-02-25 14:37 UTC (permalink / raw)
To: linux-arm-kernel
On Tuesday 25 February 2014 17:19:59 Grygorii Strashko wrote:
> The Keystone can work in two modes:
> - LPAE disabled: In this case Platform bus notifier will not be called at all
> and DMA range == MEM range (32 bits mode)
The hardware setting doesn't change here, just the available memory, right?
> - LPAE enabled: In this case, I'll update code to treat absence of "dam-ranges"
> property as "no dma possible" and clean up DMA mask. Is it ok?
> In this mode "dam-ranges" prop has to be defined always to enable DMA.
Ok, sounds good.
> Empty "dma-ranges" can't be treated as "no translation required" because it is used
> to move one level up while traversing DT to find the valid "dma-ranges" prop.
> Used to handle child devices like:
>
> bus {
> dma-ranges = <...>;
>
> Dev A {
> dma-ranges;
>
> child_dev_A1 { }
> child_dev_A2 { }
>
> This is standard behavior for "[dma-]ranges".
That is what I meant: "no translation required at this level". You still
have to go up to the root in order to know the full translation.
Arnd
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/4] ARM: keystone: Use dma-ranges for dma_pfn_offset configuration
2014-02-25 13:37 ` Arnd Bergmann
@ 2014-02-25 15:19 ` Grygorii Strashko
2014-02-25 14:37 ` Arnd Bergmann
0 siblings, 1 reply; 13+ messages in thread
From: Grygorii Strashko @ 2014-02-25 15:19 UTC (permalink / raw)
To: linux-arm-kernel
On 02/25/2014 03:37 PM, Arnd Bergmann wrote:
> On Tuesday 25 February 2014, Grygorii Strashko wrote:
>> On 02/24/2014 11:38 PM, Santosh Shilimkar wrote:
>>> On Monday 24 February 2014 04:11 PM, Arnd Bergmann wrote:
>>>> On Monday 24 February 2014 15:53:55 Santosh Shilimkar wrote:
>>>>> From: Grygorii Strashko <grygorii.strashko@ti.com>
>>
>>>> Hmm, isn't this function the same as of_translate_dma_address()?
>>>>
>>>> I think we should have the implementation in common code, not hidden
>>>> in the keystone platform, to avoid duplication. If of_translate_dma_address
>>>> doesn't work, what is the problem, and can you fix it there?
>>>>
>>> Will have a look at it and get back. At least it makes sense to
>>> use/update the common function.
>>
>> This function isn't the same as of_translate_dma_address(), more over
>> it calls of_translate_dma_address() to get CPU address.
>>
>> To calculate DMA bus offset we need to know the first DMA address explicitly,
>> so then we can translate it to the corresponding CPU address.
>
> Ah, got it. Sorry for being sloppy in my review. You are absolutely right.
>
>> get_dma_pfn_offset() function does:
>> - traverse DT and look for the first "dma-ranges" prop;
>> - reads the first DMA address of the range;
>> - translates it in CPU address using of_translate_dma_address();
>> - calculates DMA bus offset as PFN_DOWN(cpu_addr - dma_addr);
>
> I was confused about the first step here and thought you were actually
> doing the translation there. After a more careful reading, I only
> have comments on details:
>
>> +static unsigned long get_dma_pfn_offset(struct device *dev)
>> +{
>> + 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;
>> + }
>
> I think we should require an empty "dma-ranges" property to be
> present to signify "no translation required" and interpret the
> absence of the property as "no dma possible".
>
> A special case is having no "dma-ranges" at all, which we have
> on all existing systems, and I would interpret that as "all
> devices can do 32-bit DMA, no more but no less".
The Keystone can work in two modes:
- LPAE disabled: In this case Platform bus notifier will not be called at all
and DMA range == MEM range (32 bits mode)
- LPAE enabled: In this case, I'll update code to treat absence of "dam-ranges"
property as "no dma possible" and clean up DMA mask. Is it ok?
In this mode "dam-ranges" prop has to be defined always to enable DMA.
Empty "dma-ranges" can't be treated as "no translation required" because it is used
to move one level up while traversing DT to find the valid "dma-ranges" prop.
Used to handle child devices like:
bus {
dma-ranges = <...>;
Dev A {
dma-ranges;
child_dev_A1 { }
child_dev_A2 { }
This is standard behavior for "[dma-]ranges".
>
>
>> + /*
>> + * dma-ranges format:
>> + * DMA addr : naddr cells
>> + * CPU addr : pna cells
>> + * size : nsize cells
>> + */
>> + len /= sizeof(u32);
>> + pna = of_n_addr_cells(node);
>> + dma_addr = of_read_number(ranges, nsize);
>
> This should probably use naddr, not nsize.
agree.
Regards,
-grygorii
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/4] ARM: keystone: Use dma-ranges for dma_pfn_offset configuration
2014-02-25 14:37 ` Arnd Bergmann
@ 2014-02-25 16:05 ` Grygorii Strashko
0 siblings, 0 replies; 13+ messages in thread
From: Grygorii Strashko @ 2014-02-25 16:05 UTC (permalink / raw)
To: linux-arm-kernel
On 02/25/2014 04:37 PM, Arnd Bergmann wrote:
> On Tuesday 25 February 2014 17:19:59 Grygorii Strashko wrote:
>> The Keystone can work in two modes:
>> - LPAE disabled: In this case Platform bus notifier will not be called at all
>> and DMA range == MEM range (32 bits mode)
>
> The hardware setting doesn't change here, just the available memory, right?
Yes. The accessible RAM will be 0x8000 0000 - 0xFFFF FFFF -
the same as DMA range. But coherent DMA will not be supported.
So, no DMA bus offset required and actions in Platform bus notifier.
>
>> - LPAE enabled: In this case, I'll update code to treat absence of "dam-ranges"
>> property as "no dma possible" and clean up DMA mask. Is it ok?
>> In this mode "dam-ranges" prop has to be defined always to enable DMA.
>
> Ok, sounds good.
>
>> Empty "dma-ranges" can't be treated as "no translation required" because it is used
>> to move one level up while traversing DT to find the valid "dma-ranges" prop.
>> Used to handle child devices like:
>>
>> bus {
>> dma-ranges = <...>;
>>
>> Dev A {
>> dma-ranges;
>>
>> child_dev_A1 { }
>> child_dev_A2 { }
>>
>> This is standard behavior for "[dma-]ranges".
>
> That is what I meant: "no translation required at this level". You still
> have to go up to the root in order to know the full translation.
>
Regards,
-grygorii
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-02-25 16:05 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-24 20:53 [PATCH 0/4] ARM: mm: Use dma-ranges for dma address translation Santosh Shilimkar
2014-02-24 20:53 ` [PATCH 1/4] ARM: mm: Introduce archdata.dma_pfn_offset Santosh Shilimkar
2014-02-24 20:53 ` [PATCH 2/4] ARM: mm: Remove unsed dma_to_virt() Santosh Shilimkar
2014-02-24 20:53 ` [PATCH 3/4] ARM: dts: keystone: Use dma-ranges property Santosh Shilimkar
2014-02-24 20:53 ` [PATCH 4/4] ARM: keystone: Use dma-ranges for dma_pfn_offset configuration Santosh Shilimkar
2014-02-24 21:11 ` Arnd Bergmann
2014-02-24 21:38 ` Santosh Shilimkar
2014-02-25 8:35 ` Arnd Bergmann
2014-02-25 14:14 ` Grygorii Strashko
2014-02-25 13:37 ` Arnd Bergmann
2014-02-25 15:19 ` Grygorii Strashko
2014-02-25 14:37 ` Arnd Bergmann
2014-02-25 16:05 ` Grygorii Strashko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.