[PATCH] of: Set the DMA mask to 64 bits on ARM LPAE systems

[PATCH] of: Set the DMA mask to 64 bits on ARM LPAE systems

[Laura Abbott]

By default on ARM systems, the coherent DMA mask (lowest
address) is set to ~0 or 0xFFFFFFFFFFFFFFFF. Currently,
of_platform_device_create_pdata sets the coherent DMA mask to
32 bits. This prevents coherent dma allocations from working by default
without clients setting the DMA mask. Rather than make every client
on an LPAE system set the mask, set the mask to a 64 bit value on
ARM LPAE systems.

Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
---
 drivers/of/platform.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 0970505..18b69c1 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -214,7 +214,11 @@ struct platform_device *of_platform_device_create_pdata(
 #if defined(CONFIG_MICROBLAZE)
 	dev->archdata.dma_mask = 0xffffffffUL;
 #endif
+#ifdef CONFIG_ARM_LPAE
+	dev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
+#else
 	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+#endif
 	dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] of: Set the DMA mask to 64 bits on ARM LPAE systems

[Catalin Marinas]

On Wed, Apr 24, 2013 at 01:50:49AM +0100, Laura Abbott wrote:
> By default on ARM systems, the coherent DMA mask (lowest
> address) is set to ~0 or 0xFFFFFFFFFFFFFFFF. Currently,
> of_platform_device_create_pdata sets the coherent DMA mask to
> 32 bits. This prevents coherent dma allocations from working by default
> without clients setting the DMA mask. Rather than make every client
> on an LPAE system set the mask, set the mask to a 64 bit value on
> ARM LPAE systems.
> 
> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> ---
>  drivers/of/platform.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 0970505..18b69c1 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -214,7 +214,11 @@ struct platform_device *of_platform_device_create_pdata(
>  #if defined(CONFIG_MICROBLAZE)
>  	dev->archdata.dma_mask = 0xffffffffUL;
>  #endif
> +#ifdef CONFIG_ARM_LPAE
> +	dev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
> +#else
>  	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> +#endif

I wouldn't add CONFIG_ARM_LPAE checks in here, you can use
CONFIG_ARCH_DMA_ADDR_T_64BIT (types.h uses this for the dma_addr_t
definition).

-- 
Catalin

[PATCH] of: Set the DMA mask to 64 bits on ARM LPAE systems

[Laura Abbott]

On 4/25/2013 6:33 AM, Catalin Marinas wrote:
> On Wed, Apr 24, 2013 at 01:50:49AM +0100, Laura Abbott wrote:
>> By default on ARM systems, the coherent DMA mask (lowest
>> address) is set to ~0 or 0xFFFFFFFFFFFFFFFF. Currently,
>> of_platform_device_create_pdata sets the coherent DMA mask to
>> 32 bits. This prevents coherent dma allocations from working by default
>> without clients setting the DMA mask. Rather than make every client
>> on an LPAE system set the mask, set the mask to a 64 bit value on
>> ARM LPAE systems.
>>
>> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
>> ---
>>   drivers/of/platform.c |    4 ++++
>>   1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 0970505..18b69c1 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -214,7 +214,11 @@ struct platform_device *of_platform_device_create_pdata(
>>   #if defined(CONFIG_MICROBLAZE)
>>   	dev->archdata.dma_mask = 0xffffffffUL;
>>   #endif
>> +#ifdef CONFIG_ARM_LPAE
>> +	dev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
>> +#else
>>   	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>> +#endif
>
> I wouldn't add CONFIG_ARM_LPAE checks in here, you can use
> CONFIG_ARCH_DMA_ADDR_T_64BIT (types.h uses this for the dma_addr_t
> definition).
>

I thought about this as well but in arch/arm/mm/mm.h:

#ifdef CONFIG_ZONE_DMA
extern phys_addr_t arm_dma_limit;
#else
#define arm_dma_limit ((phys_addr_t)~0)
#endif

arm_dma_limit is explicitly cast to phys_addr_t, which means that 
arm_dma_limit will be always be sizeof(phys_addr_t) regardless of 
sizeof(dma_addr_t). Is it safe to assume that 
CONFIG_ARCH_DMA_ADDR_T_64BIT will always be selected if 
sizeof(phys_addr_t) == 8? If not, we've defeated the point of the patch.

Alternatively, should the type of arm_dma_limit be dma_addr_t instead of 
phys_addr_t?

Thanks,
Laura

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] of: Set the DMA mask to 64 bits on ARM LPAE systems

[Russell King - ARM Linux]

On Thu, Apr 25, 2013 at 10:09:57AM -0700, Laura Abbott wrote:
> I thought about this as well but in arch/arm/mm/mm.h:
>
> #ifdef CONFIG_ZONE_DMA
> extern phys_addr_t arm_dma_limit;
> #else
> #define arm_dma_limit ((phys_addr_t)~0)
> #endif
>
> arm_dma_limit is explicitly cast to phys_addr_t, which means that  
> arm_dma_limit will be always be sizeof(phys_addr_t) regardless of  
> sizeof(dma_addr_t).

This is the size of the DMA zone, which controls the _minimum_ DMA mask
that can be used in the system.  DMA masks must always be greater than
this limit.

[PATCH] of: Set the DMA mask to 64 bits on ARM LPAE systems

[Catalin Marinas]

On Thu, Apr 25, 2013 at 06:09:57PM +0100, Laura Abbott wrote:
> On 4/25/2013 6:33 AM, Catalin Marinas wrote:
> > On Wed, Apr 24, 2013 at 01:50:49AM +0100, Laura Abbott wrote:
> >> By default on ARM systems, the coherent DMA mask (lowest
> >> address) is set to ~0 or 0xFFFFFFFFFFFFFFFF. Currently,
> >> of_platform_device_create_pdata sets the coherent DMA mask to
> >> 32 bits. This prevents coherent dma allocations from working by default
> >> without clients setting the DMA mask. Rather than make every client
> >> on an LPAE system set the mask, set the mask to a 64 bit value on
> >> ARM LPAE systems.
> >>
> >> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> >> ---
> >>   drivers/of/platform.c |    4 ++++
> >>   1 files changed, 4 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> >> index 0970505..18b69c1 100644
> >> --- a/drivers/of/platform.c
> >> +++ b/drivers/of/platform.c
> >> @@ -214,7 +214,11 @@ struct platform_device *of_platform_device_create_pdata(
> >>   #if defined(CONFIG_MICROBLAZE)
> >>   	dev->archdata.dma_mask = 0xffffffffUL;
> >>   #endif
> >> +#ifdef CONFIG_ARM_LPAE
> >> +	dev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
> >> +#else
> >>   	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> >> +#endif
> >
> > I wouldn't add CONFIG_ARM_LPAE checks in here, you can use
> > CONFIG_ARCH_DMA_ADDR_T_64BIT (types.h uses this for the dma_addr_t
> > definition).
> >
> 
> I thought about this as well but in arch/arm/mm/mm.h:
> 
> #ifdef CONFIG_ZONE_DMA
> extern phys_addr_t arm_dma_limit;
> #else
> #define arm_dma_limit ((phys_addr_t)~0)
> #endif

Russell replied already on the meaning of this variable, so I don't
think its type is relevant here. It is also used when calling
dma_contiguous_reserve() which takes a phys_addr_t.

> arm_dma_limit is explicitly cast to phys_addr_t, which means that 
> arm_dma_limit will be always be sizeof(phys_addr_t) regardless of 
> sizeof(dma_addr_t). Is it safe to assume that 
> CONFIG_ARCH_DMA_ADDR_T_64BIT will always be selected if 
> sizeof(phys_addr_t) == 8? If not, we've defeated the point of the patch.

sizeof(dma_addr_t) <= sizeof(phys_addr_t). I had this discussion with
Russell when upstreaming the LPAE patches and the conclusion was that
while the phys_addr_t is 64-bit with LPAE, the dma_addr_t size is a
feature of the SoC and must be selected independently. However, I think
with multi-platform support and LPAE enabled, we should probably set the
dma_addr_t to 64-bit.

> Alternatively, should the type of arm_dma_limit be dma_addr_t instead of 
> phys_addr_t?

arm_dma_limit should be phys_addr_t since this is an address as seen by
the CPU. dma_addr_t OTOH is a bus address as seen by the device. You can
in theory have a DMA buffer with physical address beyond 32-bit (from a
CPU perspective) but with a bus address withing 32-bit as seen by the
device.

There is some confusion in the kernel (you can google for past
discussions) around the meaning of the DMA mask. If your RAM starts at
0, it doesn't matter much and there are several such assumptions in the
kernel.

So I still consider that you should use #ifdef
CONFIG_ARCH_DMA_ADDR_T_64BIT and select this option on your platform.

-- 
Catalin

[PATCH] of: Set the DMA mask to 64 bits on ARM LPAE systems

[Laura Abbott]

On 4/26/2013 3:00 AM, Catalin Marinas wrote:
> On Thu, Apr 25, 2013 at 06:09:57PM +0100, Laura Abbott wrote:
>> On 4/25/2013 6:33 AM, Catalin Marinas wrote:
>>> On Wed, Apr 24, 2013 at 01:50:49AM +0100, Laura Abbott wrote:
>>>> By default on ARM systems, the coherent DMA mask (lowest
>>>> address) is set to ~0 or 0xFFFFFFFFFFFFFFFF. Currently,
>>>> of_platform_device_create_pdata sets the coherent DMA mask to
>>>> 32 bits. This prevents coherent dma allocations from working by default
>>>> without clients setting the DMA mask. Rather than make every client
>>>> on an LPAE system set the mask, set the mask to a 64 bit value on
>>>> ARM LPAE systems.
>>>>
>>>> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
>>>> ---
>>>>    drivers/of/platform.c |    4 ++++
>>>>    1 files changed, 4 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>>> index 0970505..18b69c1 100644
>>>> --- a/drivers/of/platform.c
>>>> +++ b/drivers/of/platform.c
>>>> @@ -214,7 +214,11 @@ struct platform_device *of_platform_device_create_pdata(
>>>>    #if defined(CONFIG_MICROBLAZE)
>>>>    	dev->archdata.dma_mask = 0xffffffffUL;
>>>>    #endif
>>>> +#ifdef CONFIG_ARM_LPAE
>>>> +	dev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
>>>> +#else
>>>>    	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>>>> +#endif
>>>
>>> I wouldn't add CONFIG_ARM_LPAE checks in here, you can use
>>> CONFIG_ARCH_DMA_ADDR_T_64BIT (types.h uses this for the dma_addr_t
>>> definition).
>>>
>>
>> I thought about this as well but in arch/arm/mm/mm.h:
>>
>> #ifdef CONFIG_ZONE_DMA
>> extern phys_addr_t arm_dma_limit;
>> #else
>> #define arm_dma_limit ((phys_addr_t)~0)
>> #endif
>
> Russell replied already on the meaning of this variable, so I don't
> think its type is relevant here. It is also used when calling
> dma_contiguous_reserve() which takes a phys_addr_t.
>
>> arm_dma_limit is explicitly cast to phys_addr_t, which means that
>> arm_dma_limit will be always be sizeof(phys_addr_t) regardless of
>> sizeof(dma_addr_t). Is it safe to assume that
>> CONFIG_ARCH_DMA_ADDR_T_64BIT will always be selected if
>> sizeof(phys_addr_t) == 8? If not, we've defeated the point of the patch.
>
> sizeof(dma_addr_t) <= sizeof(phys_addr_t). I had this discussion with
> Russell when upstreaming the LPAE patches and the conclusion was that
> while the phys_addr_t is 64-bit with LPAE, the dma_addr_t size is a
> feature of the SoC and must be selected independently. However, I think
> with multi-platform support and LPAE enabled, we should probably set the
> dma_addr_t to 64-bit.
>
>> Alternatively, should the type of arm_dma_limit be dma_addr_t instead of
>> phys_addr_t?
>
> arm_dma_limit should be phys_addr_t since this is an address as seen by
> the CPU. dma_addr_t OTOH is a bus address as seen by the device. You can
> in theory have a DMA buffer with physical address beyond 32-bit (from a
> CPU perspective) but with a bus address withing 32-bit as seen by the
> device.
>
> There is some confusion in the kernel (you can google for past
> discussions) around the meaning of the DMA mask. If your RAM starts at
> 0, it doesn't matter much and there are several such assumptions in the
> kernel.
>
> So I still consider that you should use #ifdef
> CONFIG_ARCH_DMA_ADDR_T_64BIT and select this option on your platform.
>

Thanks for the clarification on all this. It's non-obvious but I see why 
things are set up the way they are. I'll re-submit with 
CONFIG_ARCH_DMA_ADDR_T_64BIT.

Laura

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation