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

[Catalin Marinas <catalin.marinas@arm.com>]

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