arm64: default dma_ops is set to coherent_dma_ops results into DMA FAILURE

arm64: default dma_ops is set to coherent_dma_ops results into DMA FAILURE

[Ritesh Harjani]

Hi Catalin/Will,

This is regarding the default dma_ops that we populate for arm64.

PROBLEM:
Currently in arch/arm64/mm/dma-mapping.c we set dma_ops =
&coherent_swiotlb_dma_ops.

The problem with this is, lets say that there is a dma device which
has not populated its dev->archdata.dma_ops, then this dma device will
get the coherent dma_ops in which we dont do any cache maintainance.

So, if the dma driver do kmalloc, make some changes to the buffer and
after dma_map_single gives it to the dma for the transfer, due to no
cache maintenance performed, result will be DMA transfer failed.



Earlier noncoherent ops code was not present at all for arm64, so may
be we were setting default ops to coherent ops. But now that we have
noncoherent ops in place shall we make the default dma_ops to
noncoherent (as what arm also does) ?


Please let me know if I am missing something here.


Thanks
Ritesh

arm64: default dma_ops is set to coherent_dma_ops results into DMA FAILURE

[Will Deacon]

On Tue, Apr 22, 2014 at 01:48:12PM +0100, Ritesh Harjani wrote:
> Hi Catalin/Will,

Hi Ritesh,

> This is regarding the default dma_ops that we populate for arm64.
> 
> PROBLEM:
> Currently in arch/arm64/mm/dma-mapping.c we set dma_ops =
> &coherent_swiotlb_dma_ops.
> 
> The problem with this is, lets say that there is a dma device which
> has not populated its dev->archdata.dma_ops, then this dma device will
> get the coherent dma_ops in which we dont do any cache maintainance.
> 
> So, if the dma driver do kmalloc, make some changes to the buffer and
> after dma_map_single gives it to the dma for the transfer, due to no
> cache maintenance performed, result will be DMA transfer failed.
> 
> 
> 
> Earlier noncoherent ops code was not present at all for arm64, so may
> be we were setting default ops to coherent ops. But now that we have
> noncoherent ops in place shall we make the default dma_ops to
> noncoherent (as what arm also does) ?

The problem with changing the default assumption to non-coherent is that
existing coherent platforms that rely on the coherent DMA ops being selected
without any additional DT properties (e.g. dma-coherent) will break with
this change.

This puts us into a nasty situation where we have the opposite policy from
arch/arm/ regarding default DMA ops, rendering device-trees potentially
incompatible between the two architectures.

I'd be inclined to align the two, but it will break any users relying on the
current behaviour (I believe this includes Applied with their X-gene SoC).

Will

arm64: default dma_ops is set to coherent_dma_ops results into DMA FAILURE

[Catalin Marinas]

On Tue, Apr 22, 2014 at 02:20:45PM +0100, Will Deacon wrote:
> On Tue, Apr 22, 2014 at 01:48:12PM +0100, Ritesh Harjani wrote:
> > This is regarding the default dma_ops that we populate for arm64.
> > 
> > PROBLEM:
> > Currently in arch/arm64/mm/dma-mapping.c we set dma_ops =
> > &coherent_swiotlb_dma_ops.
> > 
> > The problem with this is, lets say that there is a dma device which
> > has not populated its dev->archdata.dma_ops, then this dma device will
> > get the coherent dma_ops in which we dont do any cache maintainance.
> > 
> > So, if the dma driver do kmalloc, make some changes to the buffer and
> > after dma_map_single gives it to the dma for the transfer, due to no
> > cache maintenance performed, result will be DMA transfer failed.
> > 
> > Earlier noncoherent ops code was not present at all for arm64, so may
> > be we were setting default ops to coherent ops. But now that we have
> > noncoherent ops in place shall we make the default dma_ops to
> > noncoherent (as what arm also does) ?
> 
> The problem with changing the default assumption to non-coherent is that
> existing coherent platforms that rely on the coherent DMA ops being selected
> without any additional DT properties (e.g. dma-coherent) will break with
> this change.
> 
> This puts us into a nasty situation where we have the opposite policy from
> arch/arm/ regarding default DMA ops, rendering device-trees potentially
> incompatible between the two architectures.
> 
> I'd be inclined to align the two, but it will break any users relying on the
> current behaviour (I believe this includes Applied with their X-gene SoC).

If there aren't any DMA-capable X-gene drivers in mainline yet, we could
make this change (probably even if they are, though we need input from
the Applied guys on how disruptive this is).

But do we have a documented way on describing which SoC is coherent? The
longer term plan is for a full description of the system topology but we
are not there yet. IIRC Santosh has been looking at this as well but I
haven't followed the recent developments.

There are some instances of "dma-coherent" properties described in
Documentation/devicetree/, we could use them and just need to find a way
in the arm64 to handle this automatically without SoC-specific
initialisation (and independent of the driver code).

-- 
Catalin

arm64: default dma_ops is set to coherent_dma_ops results into DMA FAILURE

[Will Deacon]

On Tue, Apr 22, 2014 at 03:04:13PM +0100, Catalin Marinas wrote:
> On Tue, Apr 22, 2014 at 02:20:45PM +0100, Will Deacon wrote:
> > On Tue, Apr 22, 2014 at 01:48:12PM +0100, Ritesh Harjani wrote:
> > > This is regarding the default dma_ops that we populate for arm64.
> > > 
> > > PROBLEM:
> > > Currently in arch/arm64/mm/dma-mapping.c we set dma_ops =
> > > &coherent_swiotlb_dma_ops.
> > > 
> > > The problem with this is, lets say that there is a dma device which
> > > has not populated its dev->archdata.dma_ops, then this dma device will
> > > get the coherent dma_ops in which we dont do any cache maintainance.
> > > 
> > > So, if the dma driver do kmalloc, make some changes to the buffer and
> > > after dma_map_single gives it to the dma for the transfer, due to no
> > > cache maintenance performed, result will be DMA transfer failed.
> > > 
> > > Earlier noncoherent ops code was not present at all for arm64, so may
> > > be we were setting default ops to coherent ops. But now that we have
> > > noncoherent ops in place shall we make the default dma_ops to
> > > noncoherent (as what arm also does) ?
> > 
> > The problem with changing the default assumption to non-coherent is that
> > existing coherent platforms that rely on the coherent DMA ops being selected
> > without any additional DT properties (e.g. dma-coherent) will break with
> > this change.
> > 
> > This puts us into a nasty situation where we have the opposite policy from
> > arch/arm/ regarding default DMA ops, rendering device-trees potentially
> > incompatible between the two architectures.
> > 
> > I'd be inclined to align the two, but it will break any users relying on the
> > current behaviour (I believe this includes Applied with their X-gene SoC).
> 
> If there aren't any DMA-capable X-gene drivers in mainline yet, we could
> make this change (probably even if they are, though we need input from
> the Applied guys on how disruptive this is).

I'd certainly be supportive of such a change.

> But do we have a documented way on describing which SoC is coherent? The
> longer term plan is for a full description of the system topology but we
> are not there yet. IIRC Santosh has been looking at this as well but I
> haven't followed the recent developments.
> 
> There are some instances of "dma-coherent" properties described in
> Documentation/devicetree/, we could use them and just need to find a way
> in the arm64 to handle this automatically without SoC-specific
> initialisation (and independent of the driver code).

I think we can default to non-coherent ops without merging support for the
"dma-coherent" properties initially. Then we can ask the Applied folks to
get involved with Santosh's thread, which will hopefully result in something
usable for both arm and arm64.

Will

arm64: default dma_ops is set to coherent_dma_ops results into DMA FAILURE

[Ritesh Harjani]

Hi Will/Catalin, 

Thanks for explaining this. As you guys said we can
default to noncoherent ops for arm64 as well, I am
submitting this patch for the same.

Catalin, as you said that there are no current dma-capable
driver which assumes coherent ops, so I guess this will not
break anything.

Ritesh Harjani (1):
  arm64: Make default dma_ops to be noncoherent

 arch/arm64/mm/dma-mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
1.8.1.3

[PATCH] arm64: Make default dma_ops to be noncoherent

[Ritesh Harjani]

Currently arm64 dma_ops is by default made coherent
which makes it opposite in default policy
from arm.

Make default dma_ops to be noncoherent (same
as arm), as currently there aren't any
dma-capable drivers which assumes coherent ops

Signed-off-by: Ritesh Harjani <ritesh.harjani@gmail.com>
---
 arch/arm64/mm/dma-mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 0ba347e..1f65963 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -311,7 +311,7 @@ static int __init swiotlb_late_init(void)
 {
 	size_t swiotlb_size = min(SZ_64M, MAX_ORDER_NR_PAGES << PAGE_SHIFT);
 
-	dma_ops = &coherent_swiotlb_dma_ops;
+	dma_ops = &noncoherent_swiotlb_dma_ops;
 
 	return swiotlb_late_init_with_default_size(swiotlb_size);
 }
-- 
1.8.1.3

[PATCH] arm64: Make default dma_ops to be noncoherent

[Will Deacon]

On Wed, Apr 23, 2014 at 06:29:46AM +0100, Ritesh Harjani wrote:
> Currently arm64 dma_ops is by default made coherent
> which makes it opposite in default policy
> from arm.
> 
> Make default dma_ops to be noncoherent (same
> as arm), as currently there aren't any
> dma-capable drivers which assumes coherent ops
> 
> Signed-off-by: Ritesh Harjani <ritesh.harjani@gmail.com>

  Acked-by: Will Deacon <will.deacon@arm.com>

Will

[PATCH] arm64: Make default dma_ops to be noncoherent

[Ritesh Harjani]

On Wed, Apr 23, 2014 at 2:35 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, Apr 23, 2014 at 06:29:46AM +0100, Ritesh Harjani wrote:
>> Currently arm64 dma_ops is by default made coherent
>> which makes it opposite in default policy
>> from arm.
>>
>> Make default dma_ops to be noncoherent (same
>> as arm), as currently there aren't any
>> dma-capable drivers which assumes coherent ops
>>
>> Signed-off-by: Ritesh Harjani <ritesh.harjani@gmail.com>
>
>   Acked-by: Will Deacon <will.deacon@arm.com>

Thanks!!

>
> Will


Ritesh

arm64: default dma_ops is set to coherent_dma_ops results into DMA FAILURE

[Catalin Marinas]

On Fri, Apr 25, 2014 at 06:43:45AM +0100, Loc Ho wrote:
> > > This is regarding the default dma_ops that we populate for arm64.
> > >
> > > PROBLEM:
> > > Currently in arch/arm64/mm/dma-mapping.c we set dma_ops =
> > > &coherent_swiotlb_dma_ops.
> > >
> > > The problem with this is, lets say that there is a dma device which
> > > has not populated its dev->archdata.dma_ops, then this dma device will
> > > get the coherent dma_ops in which we dont do any cache maintainance.
> > >
> > > So, if the dma driver do kmalloc, make some changes to the buffer and
> > > after dma_map_single gives it to the dma for the transfer, due to no
> > > cache maintenance performed, result will be DMA transfer failed.
> > >
> > >
> > >
> > > Earlier noncoherent ops code was not present at all for arm64, so may
> > > be we were setting default ops to coherent ops. But now that we have
> > > noncoherent ops in place shall we make the default dma_ops to
> > > noncoherent (as what arm also does) ?
> >
> > The problem with changing the default assumption to non-coherent is that
> > existing coherent platforms that rely on the coherent DMA ops being selected
> > without any additional DT properties (e.g. dma-coherent) will break with
> > this change.
> >
> > This puts us into a nasty situation where we have the opposite policy from
> > arch/arm/ regarding default DMA ops, rendering device-trees potentially
> > incompatible between the two architectures.
> >
> > I'd be inclined to align the two, but it will break any users relying on the
> > current behaviour (I believe this includes Applied with their X-gene SoC).
> 
> APM X-Gene SoC SMP system is fully coherent. It is preferred no cache
> maintenance operations as it is an over head. Currently, only the SATA
> driver is in 3.15-rc2 that is DMA master. If it is switched to
> non-coherent, this just adds cache maintenance over head which doesn't
> affect the system besides flushing over head.

The intention is not to make performance worse for this device but
rather add the following to the DT file:

diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
index 93f4b2dd9248..f8c40a66e65d 100644
--- a/arch/arm64/boot/dts/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm-storm.dtsi
@@ -307,6 +307,7 @@
 			      <0x0 0x1f21e000 0x0 0x1000>,
 			      <0x0 0x1f217000 0x0 0x1000>;
 			interrupts = <0x0 0x86 0x4>;
+			dma-coherent;
 			status = "disabled";
 			clocks = <&sata01clk 0>;
 			phys = <&phy1 0>;
@@ -321,6 +322,7 @@
 			      <0x0 0x1f22e000 0x0 0x1000>,
 			      <0x0 0x1f227000 0x0 0x1000>;
 			interrupts = <0x0 0x87 0x4>;
+			dma-coherent;
 			status = "ok";
 			clocks = <&sata23clk 0>;
 			phys = <&phy2 0>;
@@ -334,6 +336,7 @@
 			      <0x0 0x1f23d000 0x0 0x1000>,
 			      <0x0 0x1f23e000 0x0 0x1000>;
 			interrupts = <0x0 0x88 0x4>;
+			dma-coherent;
 			status = "ok";
 			clocks = <&sata45clk 0>;
 			phys = <&phy3 0>;

Since we don't have any stable release with your driver yet, it makes
sense to make such change as soon as possible.

-- 
Catalin