ARM: dma: Support dma-ranges and dma-coherent

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

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 <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Olof Johansson <[email protected]>

Signed-off-by Grygorii Strashko <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

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

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 <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Olof Johansson <[email protected]>

Signed-off-by Grygorii Strashko <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

Quoted 17 lines · Santosh Shilimkar

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

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.

Quoted 5 lines

Cc Russell King <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Olof Johansson <[email protected]>

Signed-off-by Grygorii Strashko <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

This looks like exactly what I need to proceed to multiplatform on Integrator.

Acked-by Linus Walleij <[email protected]>

From: Grygorii Strashko <[email protected]>

Remove dma_to_virt() as there are no in-tree users of it.

Cc Russell King <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Olof Johansson <[email protected]>

Cc Greg Ungerer <[email protected]>

Cc Tony Lindgren <[email protected]>

Signed-off-by Grygorii Strashko <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

From: Grygorii Strashko <[email protected]>

Remove dma_to_virt() as there are no in-tree users of it.

Cc Russell King <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Olof Johansson <[email protected]>

Cc Greg Ungerer <[email protected]>

Cc Tony Lindgren <[email protected]>

Signed-off-by Grygorii Strashko <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---
 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(-)

arch/arm/include/asm/dma-mapping.h +1 -13

index 247ed72..e365158 100644
@@ -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);

arch/arm/mach-iop13xx/include/mach/memory.h +0 -11

index 7c032d0..1223c85 100644
@@ -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;					\

arch/arm/mach-ks8695/include/mach/memory.h +0 -2

index 95e731a..f42477c 100644
@@ -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); })

arch/arm/mach-omap1/include/mach/memory.h +0 -4

index 3c25305..73f86db 100644
@@ -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) : \

drivers/dma/of-dma.c +12 -0

+
+/**
+ * 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 = np;
+

Quoted 13 lines · On Thursday 27 February 2014 16:17:49 Santosh Shilimkar wrote:

drivers/dma/of-dma.c +12 -0

+
+/**
+ * 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 = np;
+

drivers/dma/of-dma.c out: · line 328

Quoted 10 lines

drivers/dma/of-dma.c +10 -0

+       while (node) {
+               if (of_property_read_bool(node, "dma-coherent")) {
+                       of_node_put(node);
+                       return true;
+               }
+               node = of_get_next_parent(node);
+       }
+       return false;
+}
+EXPORT_SYMBOL_GPL(of_dma_is_coherent);

This won't work on architectures that are always coherent and did not need 'dma-coherent' properties before, such as IBM Power servers.

That said, I think the property makes sense, and we already have platforms using it (highbank is the one I'm aware of).

We probably need ways to override this function in both ways:
"always coherent" (powerpc, x86), and "never coherent" (keystone without LPAE) from platform code, and it would be nice to put either option into DT in a global location as well. We may have to go through a few iterations of this patch to get the best algorithm, but I think the interface is good at least.

Quoted 32 lines · Arnd Bergmann

Quoted 13 lines · On Thursday 27 February 2014 16:17:49 Santosh Shilimkar wrote:

drivers/dma/of-dma.c +12 -0

+
+/**
+ * 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 = np;
+

drivers/dma/of-dma.c out: · line 328

Quoted 10 lines

drivers/dma/of-dma.c +10 -0

+       while (node) {
+               if (of_property_read_bool(node, "dma-coherent")) {
+                       of_node_put(node);
+                       return true;
+               }
+               node = of_get_next_parent(node);
+       }
+       return false;
+}
+EXPORT_SYMBOL_GPL(of_dma_is_coherent);

This won't work on architectures that are always coherent and did not need 'dma-coherent' properties before, such as IBM Power servers.

That said, I think the property makes sense, and we already have platforms using it (highbank is the one I'm aware of).

Quoted 6 lines

We probably need ways to override this function in both ways:
"always coherent" (powerpc, x86), and "never coherent" (keystone without LPAE) from platform code, and it would be nice to put either option into DT in a global location as well. We may have to go through a few iterations of this patch to get the best algorithm, but I think the interface is good at least.

Probably we should discuss bit more next week at connect. The current 'dma-coherent' is a per device property. For arch's which are always coherent, the per device property doesn't make sense.

BTW, the current users of this API is only ARM32 bit port and if this satisfies the ARM platforms, we should get this in kernel and then address other cases on need basis.

Quoted 13 lines · On Thursday 27 February 2014 16:17:49 Santosh Shilimkar wrote:

drivers/dma/of-dma.c +12 -0

+
+/**
+ * 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 = np;
+

drivers/dma/of-dma.c out: · line 328

Quoted 10 lines

drivers/dma/of-dma.c +10 -0

+       while (node) {
+               if (of_property_read_bool(node, "dma-coherent")) {
+                       of_node_put(node);
+                       return true;
+               }
+               node = of_get_next_parent(node);
+       }
+       return false;
+}
+EXPORT_SYMBOL_GPL(of_dma_is_coherent);

This won't work on architectures that are always coherent and did not need 'dma-coherent' properties before, such as IBM Power servers.

That said, I think the property makes sense, and we already have platforms using it (highbank is the one I'm aware of).

We probably need ways to override this function in both ways:
"always coherent" (powerpc, x86), and "never coherent" (keystone without LPAE) from platform code, and it would be nice to put either option into DT in a global location as well. We may have to go through a few iterations of this patch to get the best algorithm, but I think the interface is good at least.

Quoted 32 lines · Arnd Bergmann

Quoted 13 lines · On Thursday 27 February 2014 16:17:49 Santosh Shilimkar wrote:

drivers/dma/of-dma.c +12 -0

+
+/**
+ * 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 = np;
+

drivers/dma/of-dma.c out: · line 328

Quoted 10 lines

drivers/dma/of-dma.c +10 -0

+       while (node) {
+               if (of_property_read_bool(node, "dma-coherent")) {
+                       of_node_put(node);
+                       return true;
+               }
+               node = of_get_next_parent(node);
+       }
+       return false;
+}
+EXPORT_SYMBOL_GPL(of_dma_is_coherent);

This won't work on architectures that are always coherent and did not need 'dma-coherent' properties before, such as IBM Power servers.

That said, I think the property makes sense, and we already have platforms using it (highbank is the one I'm aware of).

Quoted 6 lines

We probably need ways to override this function in both ways:
"always coherent" (powerpc, x86), and "never coherent" (keystone without LPAE) from platform code, and it would be nice to put either option into DT in a global location as well. We may have to go through a few iterations of this patch to get the best algorithm, but I think the interface is good at least.

I know Will D was not a fan of this property. Primarily I believe because you may need to describe more than just a boolean in more complex bus topologies.

Effectively, highbank is always coherent. It was only PCI that is non-coherent, but I can safely say PCI will never be enabled at this point. There are no designs with PCI beyond 1 or 2 validation boards (total boards, not designs), and getting PCI to work was quite hacky due to only a 1MB window. The other masters are programmable, but only the coherent path is used as the non-coherent path actually has some issues. I had expected the opposite believing the ACP port would actually have issues which is also why I made it configurable.

I know Will D was not a fan of this property. Primarily I believe because you may need to describe more than just a boolean in more complex bus topologies.

Effectively, highbank is always coherent. It was only PCI that is non-coherent, but I can safely say PCI will never be enabled at this point. There are no designs with PCI beyond 1 or 2 validation boards (total boards, not designs), and getting PCI to work was quite hacky due to only a 1MB window. The other masters are programmable, but only the coherent path is used as the non-coherent path actually has some issues. I had expected the opposite believing the ACP port would actually have issues which is also why I made it configurable.

Quoted 4 lines · On Friday 28 February 2014 09:14:19 Rob Herring wrote:

I know Will D was not a fan of this property. Primarily I believe because you may need to describe more than just a boolean in more complex bus topologies.

I can't think of any example where it's not per-device. Do you think we can end up with a device that has multiple bus master ports, only some of which are coherent, or is there a different concern?

Quoted 9 lines

Effectively, highbank is always coherent. It was only PCI that is non-coherent, but I can safely say PCI will never be enabled at this point. There are no designs with PCI beyond 1 or 2 validation boards (total boards, not designs), and getting PCI to work was quite hacky due to only a 1MB window. The other masters are programmable, but only the coherent path is used as the non-coherent path actually has some issues. I had expected the opposite believing the ACP port would actually have issues which is also why I made it configurable.

Ok, I see. I still expect that we will see systems that are only partially coherent in the future, but it's good to know we don't really have to deal with backwards-compatibility as long as we can just hardcode highbank to be always coherent.

I'm especially thankful we don't have to deal with the PCI implementation.

Adding Will...

Quoted 10 lines · On Friday 28 February 2014 09:14:19 Rob Herring wrote:

Quoted 4 lines

I know Will D was not a fan of this property. Primarily I believe because you may need to describe more than just a boolean in more complex bus topologies.

I can't think of any example where it's not per-device. Do you think we can end up with a device that has multiple bus master ports, only some of which are coherent, or is there a different concern?

Perhaps Will can post his slides from the ARM kernel summit or chime in here, but I believe it was more that coherency is just one aspect of bus master bus topology. The DT models the slave bus hierarchy and doesn't model the master side which is becoming more complex and needing to be described. In the case of highbank, you have an ACP bus with some number of masters on it. They are configurable, but it is not the device that is configurable, but really which bus they are connected to.

arch/arm/kernel/devtree.c +9 -0

index f751714..926b5dd 100644
@@ -235,3 +238,61 @@ const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
 
 	return mdesc;
 }
+
+void arm_dt_dma_configure(struct device *dev)
+{
+	dma_addr_t dma_addr;
+	phys_addr_t paddr, size;
+	dma_addr_t dma_mask;
+	int ret;
+
+	/*

drivers/of/platform.c +0 -0

index 404d1da..97d5533 100644
@@ -213,10 +213,13 @@ static struct platform_device *of_platform_device_create_pdata(
 

Quoted 23 lines · On Thursday 27 February 2014 16:17:50 Santosh Shilimkar wrote:

arch/arm/kernel/devtree.c +15 -0

index f751714..926b5dd 100644
@@ -235,3 +238,61 @@ const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
 
  	return mdesc;
  }
+
+void arm_dt_dma_configure(struct device *dev)
+{
+	dma_addr_t dma_addr;
+	phys_addr_t paddr, size;
+	dma_addr_t dma_mask;
+	int ret;
+
+	/*
+	 * if dma-ranges property doesn't exist - use 32 bits DMA mask
+	 * by default and don't set skip archdata.dma_pfn_offset
+	 */
+	ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
+	if (ret == -ENODEV) {
+		dev->coherent_dma_mask = DMA_BIT_MASK(32);

arch/arm/kernel/devtree.c const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys) · line 259

Quoted 4 lines

arch/arm/kernel/devtree.c +4 -0

+		if (!dev->dma_mask)
+			dev->dma_mask = &dev->coherent_dma_mask;
+		return;
+	}

I think this is a reasonable default, but I also want Russell's opinion on this, since I suspect he will argue that we shouldn't default to setting a DMA mask for devices that are not DMA capable.

Maybe someone has an idea how we can detect all three important cases:

a) A device is marked as DMA capable using a dma-ranges property
b) A device is known not to be DMA capable
c) we don't have any dma-ranges properties in an old dtb file but still want 32 bit masks by default.

arch/arm/kernel/devtree.c const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys) · line 265

Quoted 6 lines

arch/arm/kernel/devtree.c +5 -0

+	/* if failed - disable DMA for device */
+	if (ret < 0) {
+		dev_err(dev, "failed to configure DMA\n");
+		return;
+	}

I guess this is also where other platforms (shmobile, highbank, ...) will want the IOMMU detection to happen.

arch/arm/kernel/devtree.c const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys) · line 273

Quoted 8 lines

arch/arm/kernel/devtree.c +7 -0

+	/* DMA ranges found. Calculate and set dma_pfn_offset */
+	dev->archdata.dma_pfn_offset = PFN_DOWN(paddr - dma_addr);
+
+	/* Configure DMA mask */
+	dev->dma_mask = kmalloc(sizeof(*dev->dma_mask), GFP_KERNEL);
+	if (!dev->dma_mask)
+		return;

Do we have to worry about freeing this? We could in theory put the mask into pdev_archdata (as on microblaze), or point to coherent_dma_mask (as of_platform_device_create_pdata does). I can't think of a case where the latter won't actually work, since coherent_dma_mask!=*dma_mask doesn't happen on any platform device I have ever seen. coherent_dma_mask was introduced to handle some special requirements of PCI devices on ia64 or parisc.

arch/arm/kernel/devtree.c const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys) · line 275

Quoted 2 lines

arch/arm/kernel/devtree.c +1 -0

+       dma_mask = dma_addr + size - 1;

I can never remember if this is actually correct, or if it would have to be

	dma_mask = size - 1;

instead. Russell knows.

arch/arm/kernel/devtree.c const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys) · line 283

Quoted 7 lines

arch/arm/kernel/devtree.c +7 -0

+	ret = arm_dma_set_mask(dev, dma_mask);
+	if (ret < 0) {
+		dev_err(dev, "failed to set DMA mask %#08x\n", dma_mask);
+		kfree(dev->dma_mask);
+		dev->dma_mask = NULL;
+		return;
+	}

Again I'm hoping for Russell to provide the correct answer: Should we set the correct mask initially for the device here, or should we rely on dma_set_mask() to refuse a mask that is larger than we can handle?

For PCI devices, we normally assume that we can always set a 32-bit DMA mask, but drivers can set a smaller mask if the device can support a smaller space than the bus can. In this case, the mask is already the intersection of what the device and all the parent buses support, and I'm not sure how the API describe in Documentation/DMA-API-HOWTO.txt would deal with this.

drivers/of/platform.c static struct platform_device *of_platform_device_create_pdata( · line 216

Quoted 11 lines

drivers/of/platform.c +1 -1

index 404d1da..97d5533 100644
@@ -213,10 +213,13 @@ static struct platform_device *of_platform_device_create_pdata(
 
  #if defined(CONFIG_MICROBLAZE)
  	dev->archdata.dma_mask = 0xffffffffUL;
-#endif
+#elif defined(CONFIG_ARM_LPAE)

drivers/of/platform.c static struct platform_device *of_platform_device_create_pdata( · line 222

Quoted 6 lines

drivers/of/platform.c +3 -0

+	arm_dt_dma_configure(&dev->dev);
+#else
  	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
  	if (!dev->dev.dma_mask)
  		dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
+#endif

The dependency on CONFIG_ARM_LPAE is not correct the general case, that would be a special case on keystone. I'd suggest using CONFIG_ARM here, and finding a different way to return false for dma_is_coherent() on keystone with LPAE disabled.

Quoted 17 lines · Arnd Bergmann

Quoted 16 lines · On Thursday 27 February 2014 16:17:50 Santosh Shilimkar wrote:

arch/arm/kernel/devtree.c +15 -0

+
+void arm_dt_dma_configure(struct device *dev)
+{
+	dma_addr_t dma_addr;
+	phys_addr_t paddr, size;
+	dma_addr_t dma_mask;
+	int ret;
+
+	/*
+	 * if dma-ranges property doesn't exist - use 32 bits DMA mask
+	 * by default and don't set skip archdata.dma_pfn_offset
+	 */
+	ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
+	if (ret == -ENODEV) {
+		dev->coherent_dma_mask = DMA_BIT_MASK(32);

Quoted 8 lines

arch/arm/kernel/devtree.c const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys) · line 259

Quoted 4 lines

arch/arm/kernel/devtree.c +4 -0

+		if (!dev->dma_mask)
+			dev->dma_mask = &dev->coherent_dma_mask;
+		return;
+	}

I think this is a reasonable default, but I also want Russell's opinion on this, since I suspect he will argue that we shouldn't default to setting a DMA mask for devices that are not DMA capable.

Just to note, that's current default behavior used in of_platform_device_create_pdata()

Quoted 6 lines

Maybe someone has an idea how we can detect all three important cases:

a) A device is marked as DMA capable using a dma-ranges property
b) A device is known not to be DMA capable
c) we don't have any dma-ranges properties in an old dtb file but still want 32 bit masks by default.

Yep, This patch set supports [a, c]. But, case be [b] can be patched by arch/mach code using Platform Bus notifier if needed. (Platform Bus notifiers will be called after arm_dt_dma_configure is finished).

Quoted 8 lines

arch/arm/kernel/devtree.c const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys) · line 265

Quoted 5 lines

arch/arm/kernel/devtree.c +5 -0

+	/* if failed - disable DMA for device */
+	if (ret < 0) {
+		dev_err(dev, "failed to configure DMA\n");
+		return;
+	}

I guess this is also where other platforms (shmobile, highbank, ...) will want the IOMMU detection to happen.

This error path handling - means, DT contains wrong data :)

Quoted 8 lines

arch/arm/kernel/devtree.c const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys) · line 273

Quoted 7 lines

arch/arm/kernel/devtree.c +7 -0

+	/* DMA ranges found. Calculate and set dma_pfn_offset */
+	dev->archdata.dma_pfn_offset = PFN_DOWN(paddr - dma_addr);
+
+	/* Configure DMA mask */
+	dev->dma_mask = kmalloc(sizeof(*dev->dma_mask), GFP_KERNEL);
+	if (!dev->dma_mask)
+		return;

Quoted 7 lines

Do we have to worry about freeing this? We could in theory put the mask into pdev_archdata (as on microblaze), or point to coherent_dma_mask (as of_platform_device_create_pdata does). I can't think of a case where the latter won't actually work, since coherent_dma_mask!=*dma_mask doesn't happen on any platform device I have ever seen. coherent_dma_mask was introduced to handle some special requirements of PCI devices on ia64 or parisc.

I've used platform_device_register_full() as ref here. It actually contains good comment regarding this mem leak issue:
/*
* This memory isn't freed when the device is put,
* I don't have a nice idea for that though. Conceptually
* dma_mask in struct device should not be a pointer.
* See http://thread.gmane.org/gmane.linux.kernel.pci/9081
*/

Quoted 11 lines

Again I'm hoping for Russell to provide the correct answer: Should we set the correct mask initially for the device here, or should we rely on dma_set_mask() to refuse a mask that is larger than we can handle?

For PCI devices, we normally assume that we can always set a 32-bit DMA mask, but drivers can set a smaller mask if the device can support a smaller space than the bus can. In this case, the mask is already the intersection of what the device and all the parent buses support, and I'm not sure how the API describe in Documentation/DMA-API-HOWTO.txt would deal with this.

As mentioned by Santosh in cover letter,
PCI (and other buses) is problem here as they may have different "dma-ranges" prop format (PCI #address-cells = <3>) and need to handled in different way.

May be, this code can be limited to platform_bus_type devices only somehow.

Quoted 6 lines

drivers/of/platform.c +0 -0

Quoted 5 lines

index 404d1da..97d5533 100644

drivers/of/platform.c +0 -0

@@ -213,10 +213,13 @@ static struct platform_device *of_platform_device_create_pdata(
 

Quoted 10 lines

drivers/of/platform.c static struct platform_device *of_platform_device_create_pdata( · line 222

Quoted 10 lines

drivers/of/platform.c +4 -1

 #if defined(CONFIG_MICROBLAZE)
 	dev->archdata.dma_mask = 0xffffffffUL;
-#endif
+#elif defined(CONFIG_ARM_LPAE)
+	arm_dt_dma_configure(&dev->dev);
+#else
 	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
 	if (!dev->dev.dma_mask)
 		dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
+#endif

The dependency on CONFIG_ARM_LPAE is not correct the general case, that would be a special case on keystone. I'd suggest using CONFIG_ARM here, and finding a different way to return false for dma_is_coherent() on keystone with LPAE disabled.

I made that LPAE specific assuming the 32 machines anyway are happy with default as they are today. We can keep CONFIG_ARM and handle the special case in machine platform notifier.

Quoted 3 lines · On Friday 28 February 2014 10:06:25 Santosh Shilimkar wrote:

I made that LPAE specific assuming the 32 machines anyway are happy with default as they are today. We can keep CONFIG_ARM and handle the special case in machine platform notifier.

I think all other machines that support LPAE can also run with LPAE disabled and still use coherent DMA, they may just not support all the available RAM.

Quoted 23 lines · On Thursday 27 February 2014 16:17:50 Santosh Shilimkar wrote:

arch/arm/kernel/devtree.c +15 -0

index f751714..926b5dd 100644
@@ -235,3 +238,61 @@ const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
 
      return mdesc;
 }
+
+void arm_dt_dma_configure(struct device *dev)
+{
+     dma_addr_t dma_addr;
+     phys_addr_t paddr, size;
+     dma_addr_t dma_mask;
+     int ret;
+
+     /*
+      * if dma-ranges property doesn't exist - use 32 bits DMA mask
+      * by default and don't set skip archdata.dma_pfn_offset
+      */
+     ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
+     if (ret == -ENODEV) {
+             dev->coherent_dma_mask = DMA_BIT_MASK(32);

arch/arm/kernel/devtree.c const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys) · line 259

Quoted 4 lines

arch/arm/kernel/devtree.c +4 -0

+             if (!dev->dma_mask)
+                     dev->dma_mask = &dev->coherent_dma_mask;
+             return;
+     }

I think this is a reasonable default, but I also want Russell's opinion on this, since I suspect he will argue that we shouldn't default to setting a DMA mask for devices that are not DMA capable.

Maybe someone has an idea how we can detect all three important cases:

a) A device is marked as DMA capable using a dma-ranges property
b) A device is known not to be DMA capable
c) we don't have any dma-ranges properties in an old dtb file but still want 32 bit masks by default.

From: Grygorii Strashko <[email protected]>

This patch introduces ARM specific function arm_dt_dma_configure() which intended to 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 "dam-coherent" properties if supported. The DMA configuration applied

by arm_dt_dma_configure() as following:
- call of_get_dma_range() and get supported DMA range info (dma_addr, cpu_addr, dma_size);
- if "not found" then fill dma_mask as DMA_BIT_MASK(32) (this is default behaviour);
- if "failed" then clean up dma_mask (DMA not supported)
- if ok then update devices DMA configuration:
set dma_mask to (dma_addr + dma_size - 1) set dma_pfn_offset to PFN_DOWN(cpu_addr - dma_addr)

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]>

Signed-off-by Grygorii Strashko <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

---
 arch/arm/include/asm/prom.h |    3 +++
 arch/arm/kernel/devtree.c   |   61 +++++++++++++++++++++++++++++++++++++++++++
 drivers/of/platform.c       |    5 +++-
 include/linux/of.h          |    2 +-
 4 files changed, 69 insertions(+), 2 deletions(-)

arch/arm/include/asm/prom.h +3 -0

index b681575..1acb732 100644
@@ -11,11 +11,13 @@
 #ifndef __ASMARM_PROM_H
 #define __ASMARM_PROM_H
 
+struct device;
 #ifdef CONFIG_OF
 
 extern const struct machine_desc *setup_machine_fdt(unsigned int dt_phys);
 extern void arm_dt_memblock_reserve(void);
 extern void __init arm_dt_init_cpu_maps(void);
+extern void arm_dt_dma_configure(struct device *dev);
 
 #else /* CONFIG_OF */
 

@@ -26,6 +28,7 @@ static inline const struct machine_desc *setup_machine_fdt(unsigned int dt_phys)
 
 static inline void arm_dt_memblock_reserve(void) { }
 static inline void arm_dt_init_cpu_maps(void) { }
+static inline void arm_dt_dma_configure(struct device *dev) { }
 
 #endif /* CONFIG_OF */
 #endif /* ASMARM_PROM_H */

arch/arm/kernel/devtree.c +3 -0

index f751714..926b5dd 100644
@@ -18,6 +18,9 @@
 #include <linux/of_fdt.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/of_dma.h>
+#include <linux/dma-mapping.h>
+#include <linux/slab.h>
 
 #include <asm/cputype.h>
 #include <asm/setup.h>

arch/arm/kernel/devtree.c +19 -0

+{
+       dma_addr_t dma_addr;
+       phys_addr_t paddr, size;
+       dma_addr_t dma_mask;
+       int ret;
+
+       /*
+        * if dma-ranges property doesn't exist - use 32 bits DMA mask
+        * by default and don't set skip archdata.dma_pfn_offset
+        */
+       ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
+       if (ret == -ENODEV) {
+               dev->coherent_dma_mask = DMA_BIT_MASK(32);
+               if (!dev->dma_mask)
+                       dev->dma_mask = &dev->coherent_dma_mask;
+               return;
+       }
+
+       /* if failed - disable DMA for device */

arch/arm/kernel/devtree.c +8 -0

+       if (!dev->dma_mask)
+               return;
+
+       dma_mask = dma_addr + size - 1;
+
+       ret = arm_dma_set_mask(dev, dma_mask);
+       if (ret < 0) {
+               dev_err(dev, "failed to set DMA mask %#08x\n", dma_mask);

arch/arm/kernel/devtree.c +9 -0

+               dev_dbg(dev, "device is dma coherent\n");
+       }
+
+       ret = dma_set_coherent_mask(dev, dma_mask);
+       if (ret < 0) {
+               dev_err(dev, "failed to set coherent DMA mask %#08x\n",
+                       dma_mask);
+       }
+}

drivers/of/platform.c +0 -0

index 404d1da..97d5533 100644

drivers/of/platform.c +4 -1

-#endif
+#elif defined(CONFIG_ARM_LPAE)
+       arm_dt_dma_configure(&dev->dev);
+#else
        dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
        if (!dev->dev.dma_mask)
                dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
+#endif
        dev->dev.bus = &platform_bus_type;
        dev->dev.platform_data = platform_data;
 

include/linux/of.h +0 -0

index 70c64ba..a321058 100644