* [PATCH] arm64: Add pdev_archdata for dmamask
@ 2014-01-27 17:52 Laura Abbott
2014-01-27 18:18 ` Uwe Kleine-König
2014-01-27 19:31 ` Russell King - ARM Linux
0 siblings, 2 replies; 10+ messages in thread
From: Laura Abbott @ 2014-01-27 17:52 UTC (permalink / raw)
To: linux-arm-kernel
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 <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Suggested-by: Kumar Gala <galak@codeaurora.org>
Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
---
arch/arm64/include/asm/device.h | 1 +
arch/arm64/kernel/setup.c | 7 +++++++
2 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
index cf98b36..209d40c 100644
--- a/arch/arm64/include/asm/device.h
+++ b/arch/arm64/include/asm/device.h
@@ -24,6 +24,7 @@ struct dev_archdata {
};
struct pdev_archdata {
+ u64 dma_mask;
};
#endif
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index bd9bbd0..f164347 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -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>
@@ -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;
+}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] arm64: Add pdev_archdata for dmamask
2014-01-27 17:52 [PATCH] arm64: Add pdev_archdata for dmamask Laura Abbott
@ 2014-01-27 18:18 ` Uwe Kleine-König
2014-01-27 19:24 ` Laura Abbott
2014-01-27 20:25 ` Yann Droneaud
2014-01-27 19:31 ` Russell King - ARM Linux
1 sibling, 2 replies; 10+ messages in thread
From: Uwe Kleine-König @ 2014-01-27 18:18 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jan 27, 2014 at 09:52:57AM -0800, Laura Abbott wrote:
> 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 <will.deacon@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Suggested-by: Kumar Gala <galak@codeaurora.org>
> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
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
> ---
> arch/arm64/include/asm/device.h | 1 +
> arch/arm64/kernel/setup.c | 7 +++++++
> 2 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
> index cf98b36..209d40c 100644
> --- a/arch/arm64/include/asm/device.h
> +++ b/arch/arm64/include/asm/device.h
> @@ -24,6 +24,7 @@ struct dev_archdata {
> };
>
> struct pdev_archdata {
> + u64 dma_mask;
> };
>
> #endif
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index bd9bbd0..f164347 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -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>
> @@ -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
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] arm64: Add pdev_archdata for dmamask
2014-01-27 18:18 ` Uwe Kleine-König
@ 2014-01-27 19:24 ` Laura Abbott
2014-01-27 20:25 ` Yann Droneaud
1 sibling, 0 replies; 10+ messages in thread
From: Laura Abbott @ 2014-01-27 19:24 UTC (permalink / raw)
To: linux-arm-kernel
On 1/27/2014 10:18 AM, Uwe Kleine-K?nig wrote:
> On Mon, Jan 27, 2014 at 09:52:57AM -0800, Laura Abbott wrote:
>> 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 <will.deacon@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Suggested-by: Kumar Gala <galak@codeaurora.org>
>> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> 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
>
That covers dynamically allocated devices but it doesn't look like it
covers devices setup with just platform_device_register like
arch_setup_pdev_archdata does.
>> ---
>> arch/arm64/include/asm/device.h | 1 +
>> arch/arm64/kernel/setup.c | 7 +++++++
>> 2 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
>> index cf98b36..209d40c 100644
>> --- a/arch/arm64/include/asm/device.h
>> +++ b/arch/arm64/include/asm/device.h
>> @@ -24,6 +24,7 @@ struct dev_archdata {
>> };
>>
>> struct pdev_archdata {
>> + u64 dma_mask;
>> };
>>
>> #endif
>> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
>> index bd9bbd0..f164347 100644
>> --- a/arch/arm64/kernel/setup.c
>> +++ b/arch/arm64/kernel/setup.c
>> @@ -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>
>> @@ -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)?
>
This seemed like a reasonable default and matches what powerpc does. Any
device who wants to really guarantee a DMA mask should be setting the
dma mask explicitly and not relying on a default.
> Best regards
> Uwe
>
Thanks,
Laura
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] arm64: Add pdev_archdata for dmamask
2014-01-27 17:52 [PATCH] arm64: Add pdev_archdata for dmamask Laura Abbott
2014-01-27 18:18 ` Uwe Kleine-König
@ 2014-01-27 19:31 ` Russell King - ARM Linux
2014-01-28 1:42 ` Laura Abbott
1 sibling, 1 reply; 10+ messages in thread
From: Russell King - ARM Linux @ 2014-01-27 19:31 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jan 27, 2014 at 09:52:57AM -0800, Laura Abbott wrote:
> 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.
--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] arm64: Add pdev_archdata for dmamask
2014-01-27 18:18 ` Uwe Kleine-König
2014-01-27 19:24 ` Laura Abbott
@ 2014-01-27 20:25 ` Yann Droneaud
2014-01-27 20:36 ` Russell King - ARM Linux
1 sibling, 1 reply; 10+ messages in thread
From: Yann Droneaud @ 2014-01-27 20:25 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
Le lundi 27 janvier 2014 ? 19:18 +0100, Uwe Kleine-K?nig a ?crit :
> On Mon, Jan 27, 2014 at 09:52:57AM -0800, Laura Abbott wrote:
> > 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 <will.deacon@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Suggested-by: Kumar Gala <galak@codeaurora.org>
> > Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> 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().
A possible drawback of adding dma_mask in pdev_archdata and setting the
pointer in arch_setup_pdev_archdata() may be that a dma_mask setup prior
calling platform_device_register() got overwritten.
For example at91rm9200_usbh_device in
arch/arm/mach-at91/at91rm9200_devices.c (it's the first occurence
returned by cscope).
So I guess there's not yet a perfect solution (add dma_mask to struct
platform_device instead of hidding it either in struct pdev_archdata or
struct platform_object ?).
BTW, I've started to convert some drivers just to try my first option.
I'm also considering using dma_set_mask_and_coherent() in
platform_device_register_full() and drivers using
platform_device_alloc(). But I'm not sure about it : will it break
something with the additionals check on the mask, just as the failure
reported in the message ("dma_set_mask will always return -EIO.") ?
[1]
http://lkml.kernel.org/r/20140124154002.GF31570 at twins.programming.kicks-ass.net
Regards.
--
Yann Droneaud
OPTEYA
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] arm64: Add pdev_archdata for dmamask
2014-01-27 20:25 ` Yann Droneaud
@ 2014-01-27 20:36 ` Russell King - ARM Linux
2014-01-27 21:42 ` Yann Droneaud
0 siblings, 1 reply; 10+ messages in thread
From: Russell King - ARM Linux @ 2014-01-27 20:36 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jan 27, 2014 at 09:25:31PM +0100, Yann Droneaud wrote:
> 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.
> 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.)
--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] arm64: Add pdev_archdata for dmamask
2014-01-27 20:36 ` Russell King - ARM Linux
@ 2014-01-27 21:42 ` Yann Droneaud
0 siblings, 0 replies; 10+ messages in thread
From: Yann Droneaud @ 2014-01-27 21:42 UTC (permalink / raw)
To: linux-arm-kernel
Hi Russel,
Le lundi 27 janvier 2014 ? 20:36 +0000, Russell King - ARM Linux a
?crit :
> On Mon, Jan 27, 2014 at 09:25:31PM +0100, Yann Droneaud wrote:
> > 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.
>
Sure proliferation of fixes is not what we want.
Note that Uwe added me to the thread not because I tried to address
the same issue, but I tried to improve platform_device_register_full() to
not allocate and leak an u64 used to hold the dma mask provided as part of
platform_device_info. I believe he thought the two issues could be solved
at the same time.
> > 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.)
>
I'm not sure to understand your point here. If the driver explicitly set
a DMA mask in its call to platform_device_register_full(), can't we suppose
it should be subject to restriction from the platform ?
As I'm not sure to be clear on this particular point, here's the patch I
was considering, (sorry of being quite out of topic):
---8<---
Subject: [PATCH] driver core/platform: use dma_set_mask_and_coherent() in
platform_device_register_full()
dma_set_mask_and_coherent() is a nice function for setting the
various DMA masks in struct device. Additionnaly, it checks the
mask value against arch limits.
It may be used to control the validity of the dma_mask given to
platform_device_register_full() and setting the masks in struct
platform_device.
Note: as dma_set_mask() does nothing on m68k, or returns -EINVAL
on sparc (if PCI support not built), etc. this could break
in unpleasant ways I'm not yet able to detail ...
Cc: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: linux-m68k at lists.linux-m68k.org
Cc: "David S. Miller" <davem@davemloft.net>
Cc: sparclinux at vger.kernel.org
Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
drivers/base/platform.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index bc78848dd59a..ada1d366e1b6 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -446,8 +446,10 @@ struct platform_device *platform_device_register_full(
if (!pdev->dev.dma_mask)
goto err;
- *pdev->dev.dma_mask = pdevinfo->dma_mask;
- pdev->dev.coherent_dma_mask = pdevinfo->dma_mask;
+ ret = dma_set_mask_and_coherent(&pdev->dev,
+ pdevinfo->dma_mask);
+ if (ret)
+ goto err;
}
ret = platform_device_add_resources(pdev,
--
1.8.5.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] arm64: Add pdev_archdata for dmamask
2014-01-27 19:31 ` Russell King - ARM Linux
@ 2014-01-28 1:42 ` Laura Abbott
2014-02-17 12:29 ` Catalin Marinas
2014-02-17 12:52 ` Russell King - ARM Linux
0 siblings, 2 replies; 10+ messages in thread
From: Laura Abbott @ 2014-01-28 1:42 UTC (permalink / raw)
To: linux-arm-kernel
On 1/27/2014 11:31 AM, Russell King - ARM Linux wrote:
> On Mon, Jan 27, 2014 at 09:52:57AM -0800, Laura Abbott wrote:
>> 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.
>
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?
Thanks,
Laura
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] arm64: Add pdev_archdata for dmamask
2014-01-28 1:42 ` Laura Abbott
@ 2014-02-17 12:29 ` Catalin Marinas
2014-02-17 12:52 ` Russell King - ARM Linux
1 sibling, 0 replies; 10+ messages in thread
From: Catalin Marinas @ 2014-02-17 12:29 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jan 28, 2014 at 01:42:07AM +0000, Laura Abbott wrote:
> On 1/27/2014 11:31 AM, Russell King - ARM Linux wrote:
> > On Mon, Jan 27, 2014 at 09:52:57AM -0800, Laura Abbott wrote:
> >> 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.
> >
>
> 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?
I guess this comes under the system topology discussion we briefly
touched at the kernel summit. Dave Martin (cc'ed) is looking into this
now, so at some point we'll have some longer discussions on the list.
--
Catalin
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] arm64: Add pdev_archdata for dmamask
2014-01-28 1:42 ` Laura Abbott
2014-02-17 12:29 ` Catalin Marinas
@ 2014-02-17 12:52 ` Russell King - ARM Linux
1 sibling, 0 replies; 10+ messages in thread
From: Russell King - ARM Linux @ 2014-02-17 12:52 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jan 27, 2014 at 05:42:07PM -0800, Laura Abbott wrote:
> On 1/27/2014 11:31 AM, Russell King - ARM Linux wrote:
>> On Mon, Jan 27, 2014 at 09:52:57AM -0800, Laura Abbott wrote:
>>> 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.
>>
>
> 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.
The result being that all platform devices get their DMA masks setup
whether they like it or not - what they did was nothing more than a
work-around the problem.
That doesn't get away from what I stated above, which is what's expected
of drivers, and by drivers.
Drivers which perform DMA _must_ without fail call the appropriate DMA
mask setting functions.
Code declaring devices must set the DMA masks to a reasonable default
if the driver is to perform DMA with that device.
As the ARM architecture moves forward, and we start seeing 64-bit DMA
controllers, this becomes extremely important, because the first part
of that is part of the negotiation whether memory outside of the 4GB
DMA range can be passed to the DMA engine. We're also going to hit
problems where people have been lazy, and don't map/unmap/allocate DMA
memory against the DMA engine device, but against the DMA client device.
ASoC is going to be one such area of pain because the DMA engine backend
that was merged totally disregarded this point which was present in my
backend.
--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-02-17 12:52 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-27 17:52 [PATCH] arm64: Add pdev_archdata for dmamask Laura Abbott
2014-01-27 18:18 ` Uwe Kleine-König
2014-01-27 19:24 ` Laura Abbott
2014-01-27 20:25 ` Yann Droneaud
2014-01-27 20:36 ` Russell King - ARM Linux
2014-01-27 21:42 ` Yann Droneaud
2014-01-27 19:31 ` Russell King - ARM Linux
2014-01-28 1:42 ` Laura Abbott
2014-02-17 12:29 ` Catalin Marinas
2014-02-17 12:52 ` Russell King - ARM Linux
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).