arm64: Add pdev_archdata for dmamask

The dma_mask for a device structure is a pointer. This pointer needs to be set up before the dma mask can actually be set. Most frameworks in the kernel take care of setting this up properly but platform devices that don't follow a regular bus structure may not ever have this set. As a result, checks such as dma_capable will always return false on a raw platform device and dma_set_mask will always return -EIO. Fix this by adding a dma_mask in the platform_device archdata and setting it to be the dma_mask. Devices used in other frameworks can change this as needed.

Cc Will Deacon <[email protected]>

Cc Catalin Marinas <[email protected]>

Suggested-by Kumar Gala <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

---
 arch/arm64/include/asm/device.h |    1 +
 arch/arm64/kernel/setup.c       |    7 +++++++
 2 files changed, 8 insertions(+), 0 deletions(-)

arch/arm64/include/asm/device.h +1 -0

index cf98b36..209d40c 100644
@@ -24,6 +24,7 @@ struct dev_archdata {
 };
 
 struct pdev_archdata {
+	u64 dma_mask;
 };
 
 #endif

arch/arm64/kernel/setup.c +1 -0

index bd9bbd0..f164347 100644
@@ -41,6 +41,7 @@
 #include <linux/memblock.h>
 #include <linux/of_fdt.h>
 #include <linux/of_platform.h>
+#include <linux/dma-mapping.h>
 
 #include <asm/cputype.h>
 #include <asm/elf.h>

Quoted 10 lines · Laura Abbott

The dma_mask for a device structure is a pointer. This pointer needs to be set up before the dma mask can actually be set. Most frameworks in the kernel take care of setting this up properly but platform devices that don't follow a regular bus structure may not ever have this set. As a result, checks such as dma_capable will always return false on a raw platform device and dma_set_mask will always return -EIO. Fix this by adding a dma_mask in the platform_device archdata and setting it to be the dma_mask. Devices used in other frameworks can change this as needed.

Quoted 4 lines

Cc Will Deacon <[email protected]>

Cc Catalin Marinas <[email protected]>

Suggested-by Kumar Gala <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

Hello,

there is another non-platform dependant approach available, that might be worth to evaluate:

	http://mid.gmane.org/1390817152-30898-1-git-send-email-ydroneaud at opteya.com

Quoted 27 lines

---
  arch/arm64/include/asm/device.h |    1 +
  arch/arm64/kernel/setup.c       |    7 +++++++
  2 files changed, 8 insertions(+), 0 deletions(-)

arch/arm64/include/asm/device.h +1 -0

index cf98b36..209d40c 100644
@@ -24,6 +24,7 @@ struct dev_archdata {
  };
 
  struct pdev_archdata {
+	u64 dma_mask;
  };
 
  #endif

arch/arm64/kernel/setup.c +1 -0

index bd9bbd0..f164347 100644
@@ -41,6 +41,7 @@
  #include <linux/memblock.h>
  #include <linux/of_fdt.h>
  #include <linux/of_platform.h>
+#include <linux/dma-mapping.h>
 

arch/arm64/kernel/setup.c const struct seq_operations cpuinfo_op = { · line 346

Quoted 12 lines

arch/arm64/kernel/setup.c +6 -0

  #include <asm/cputype.h>
  #include <asm/elf.h>

@@ -337,3 +338,9 @@ const struct seq_operations cpuinfo_op = {
  	.stop	= c_stop,
  	.show	= c_show
  };
+
+void arch_setup_pdev_archdata(struct platform_device *pdev)
+{
+	pdev->archdata.dma_mask = DMA_BIT_MASK(32);
+	pdev->dev.dma_mask = &pdev->archdata.dma_mask;
+}

Is it save to assume a default of DMA_BIT_MASK(32)?

Best regards
Uwe

Quoted 10 lines

The dma_mask for a device structure is a pointer. This pointer needs to be set up before the dma mask can actually be set. Most frameworks in the kernel take care of setting this up properly but platform devices that don't follow a regular bus structure may not ever have this set. As a result, checks such as dma_capable will always return false on a raw platform device and dma_set_mask will always return -EIO. Fix this by adding a dma_mask in the platform_device archdata and setting it to be the dma_mask. Devices used in other frameworks can change this as needed.

Quoted 4 lines

Cc Will Deacon <[email protected]>

Cc Catalin Marinas <[email protected]>

Suggested-by Kumar Gala <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

Hello,

there is another non-platform dependant approach available, that might be worth to evaluate:

	http://mid.gmane.org/1390817152-30898-1-git-send-email-ydroneaud at opteya.com

ARM, even AAAAARGH64 [1], doesn't need a special treatement regarding the infamous dma_mask pointer. So perhaps my solution is better.

This solution (adding dma_mask in pdev_archdata) is already in use in powerpc architecture. See arch/powerpc/kernel/setup-common.c

The advantage of this solution is that it makes a dma_mask placeholder available to statically allocated platform_device struct, while mine only address the problem for platform_device struct allocated with platform_device_alloc().

I'm also considering using dma_set_mask_and_coherent() in platform_device_register_full() and drivers using platform_device_alloc().

Quoted 10 lines

ARM, even AAAAARGH64 [1], doesn't need a special treatement regarding the infamous dma_mask pointer. So perhaps my solution is better.

This solution (adding dma_mask in pdev_archdata) is already in use in powerpc architecture. See arch/powerpc/kernel/setup-common.c

The advantage of this solution is that it makes a dma_mask placeholder available to statically allocated platform_device struct, while mine only address the problem for platform_device struct allocated with platform_device_alloc().

As I've already said in this thread, the basic problem comes from DT's platform device creation. It's the responsibility of the device creator to set the dma_mask pointer appropriately, and DT doesn't do that. So, DT needs to be fixed rather than everyone introducing their own workarounds for this.

Quoted 3 lines

I'm also considering using dma_set_mask_and_coherent() in platform_device_register_full() and drivers using platform_device_alloc().

As the one who introduced dma_set_mask_and_coherent, consider this a strong NAK on that. The reason is dma_set_mask_and_coherent() is for drivers to set their requirements, not for the bus requirements to be set in the first place.

It also means that drivers which need no DMA support are subjected to DMA restrictions (in that dma_set_mask_and_coherent can error out if the platform can't support the DMA mask.)

The dma_mask for a device structure is a pointer. This pointer needs to be set up before the dma mask can actually be set. Most frameworks in the kernel take care of setting this up properly but platform devices that don't follow a regular bus structure may not ever have this set. As a result, checks such as dma_capable will always return false on a raw platform device and dma_set_mask will always return -EIO. Fix this by adding a dma_mask in the platform_device archdata and setting it to be the dma_mask. Devices used in other frameworks can change this as needed.

Quoted 9 lines · Laura Abbott

The dma_mask for a device structure is a pointer. This pointer needs to be set up before the dma mask can actually be set. Most frameworks in the kernel take care of setting this up properly but platform devices that don't follow a regular bus structure may not ever have this set. As a result, checks such as dma_capable will always return false on a raw platform device and dma_set_mask will always return -EIO. Fix this by adding a dma_mask in the platform_device archdata and setting it to be the dma_mask. Devices used in other frameworks can change this as needed.

You shouldn't need to do this. I went through a lot of the drivers we currently have, fixing them up in various manners. The basic rules for this stuff are:

- It is the responsibility of the code creating the device to set a reasonable default for the dma mask according to the bus and whether DMA is supportable.

- It is the responsibility of the driver _always_ to make a call to dma_set_mask() and/or dma_set_coherent_mask() according to the driver's needs if the driver is going to be using DMA.

As a work-around for the buggy situation we have in the kernel with DT, various buggy workarounds have been incorporated into drivers which involve writing directly to the DMA masks, and other such games. None of that is necessary when the dma_coerce_*() functions are used - but these are a stop-gap until the DT code gets fixed.

The real answer here is to make DT conform to the first point above and not add yet another different hack to the kernel.

Quoted 23 lines · Russell King - ARM Linux

Quoted 9 lines · Laura Abbott

The dma_mask for a device structure is a pointer. This pointer needs to be set up before the dma mask can actually be set. Most frameworks in the kernel take care of setting this up properly but platform devices that don't follow a regular bus structure may not ever have this set. As a result, checks such as dma_capable will always return false on a raw platform device and dma_set_mask will always return -EIO. Fix this by adding a dma_mask in the platform_device archdata and setting it to be the dma_mask. Devices used in other frameworks can change this as needed.

You shouldn't need to do this. I went through a lot of the drivers we currently have, fixing them up in various manners. The basic rules for this stuff are:

- It is the responsibility of the code creating the device to set a reasonable default for the dma mask according to the bus and whether DMA is supportable.

- It is the responsibility of the driver _always_ to make a call to dma_set_mask() and/or dma_set_coherent_mask() according to the driver's needs if the driver is going to be using DMA.

Quoted 9 lines

As a work-around for the buggy situation we have in the kernel with DT, various buggy workarounds have been incorporated into drivers which involve writing directly to the DMA masks, and other such games. None of that is necessary when the dma_coerce_*() functions are used - but these are a stop-gap until the DT code gets fixed.

The real answer here is to make DT conform to the first point above and not add yet another different hack to the kernel.

powerpc ran into this exact problem before and fixed it using this method (a77ce8167cc1d0370fcb1d79b367d62e050cb2b0
"driver core: Add ability for arch code to setup pdev_archdata" and
314b02f503c2c219fde0fcf6f086fda415f8a847 "powerpc: implement
arch_setup_pdev_archdata") so there is at least some precedent for this method.

Are there patches/discussion somewhere else on what a proper solution would be in the DT?

Quoted 23 lines · Russell King - ARM Linux

Quoted 9 lines · Laura Abbott

The dma_mask for a device structure is a pointer. This pointer needs to be set up before the dma mask can actually be set. Most frameworks in the kernel take care of setting this up properly but platform devices that don't follow a regular bus structure may not ever have this set. As a result, checks such as dma_capable will always return false on a raw platform device and dma_set_mask will always return -EIO. Fix this by adding a dma_mask in the platform_device archdata and setting it to be the dma_mask. Devices used in other frameworks can change this as needed.

You shouldn't need to do this. I went through a lot of the drivers we currently have, fixing them up in various manners. The basic rules for this stuff are:

- It is the responsibility of the code creating the device to set a reasonable default for the dma mask according to the bus and whether DMA is supportable.

- It is the responsibility of the driver _always_ to make a call to dma_set_mask() and/or dma_set_coherent_mask() according to the driver's needs if the driver is going to be using DMA.

Quoted 9 lines

As a work-around for the buggy situation we have in the kernel with DT, various buggy workarounds have been incorporated into drivers which involve writing directly to the DMA masks, and other such games. None of that is necessary when the dma_coerce_*() functions are used - but these are a stop-gap until the DT code gets fixed.

The real answer here is to make DT conform to the first point above and not add yet another different hack to the kernel.

powerpc ran into this exact problem before and fixed it using this method (a77ce8167cc1d0370fcb1d79b367d62e050cb2b0
"driver core: Add ability for arch code to setup pdev_archdata" and
314b02f503c2c219fde0fcf6f086fda415f8a847 "powerpc: implement
arch_setup_pdev_archdata") so there is at least some precedent for this method.