[PATCH] tile,mn10300: add device parameter to dma_cache_sync()

[PATCH] tile,mn10300: add device parameter to dma_cache_sync()

[James Hogan]

From: James Hogan <james.hogan@imgtec.com>

Note: I'm not in a position to compile test this patch, but I don't
foresee any problems.

Since v2.6.20 "Pass struct dev pointer to dma_cache_sync()"
(d3fa72e4556ec1f04e46a0d561d9e785ecaa173d), dma_cache_sync() takes a
struct dev pointer, but these appear to be missing from the tile and
mn10300 implementations, so add them.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
---
 arch/mn10300/include/asm/dma-mapping.h |    2 +-
 arch/tile/include/asm/dma-mapping.h    |    3 ++-
 arch/tile/kernel/pci-dma.c             |    2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/mn10300/include/asm/dma-mapping.h
b/arch/mn10300/include/asm/dma-mapping.h
index c1be439..6876e2b 100644
--- a/arch/mn10300/include/asm/dma-mapping.h
+++ b/arch/mn10300/include/asm/dma-mapping.h
@@ -162,7 +162,7 @@ int dma_set_mask(struct device *dev, u64 mask)
 }

 static inline
-void dma_cache_sync(void *vaddr, size_t size,
+void dma_cache_sync(struct device *dev, void *vaddr, size_t size,
 		    enum dma_data_direction direction)
 {
 	mn10300_dcache_flush_inv();
diff --git a/arch/tile/include/asm/dma-mapping.h
b/arch/tile/include/asm/dma-mapping.h
index 15e1dce..eaa06d1 100644
--- a/arch/tile/include/asm/dma-mapping.h
+++ b/arch/tile/include/asm/dma-mapping.h
@@ -65,7 +65,8 @@ extern void dma_sync_single_range_for_cpu(struct
device *, dma_addr_t,
 extern void dma_sync_single_range_for_device(struct device *, dma_addr_t,
 					     unsigned long offset, size_t,
 					     enum dma_data_direction);
-extern void dma_cache_sync(void *vaddr, size_t, enum dma_data_direction);
+extern void dma_cache_sync(struct device *dev, void *vaddr, size_t,
+			   enum dma_data_direction);

 static inline int
 dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
diff --git a/arch/tile/kernel/pci-dma.c b/arch/tile/kernel/pci-dma.c
index 5ad5e13..c6812d2 100644
--- a/arch/tile/kernel/pci-dma.c
+++ b/arch/tile/kernel/pci-dma.c
@@ -244,7 +244,7 @@ EXPORT_SYMBOL(dma_sync_single_range_for_device);
  * dma_alloc_noncoherent() returns non-cacheable memory, so there's no
  * need to do any flushing here.
  */
-void dma_cache_sync(void *vaddr, size_t size,
+void dma_cache_sync(struct device *dev, void *vaddr, size_t size,
 		    enum dma_data_direction direction)
 {
 }
-- 
1.7.2.3

Re: [PATCH] tile,mn10300: add device parameter to dma_cache_sync()

[FUJITA Tomonori]

On Mon, 4 Apr 2011 16:21:47 +0100
James Hogan <james@albanarts.com> wrote:

> From: James Hogan <james.hogan@imgtec.com>
> 
> Note: I'm not in a position to compile test this patch, but I don't
> foresee any problems.
> 
> Since v2.6.20 "Pass struct dev pointer to dma_cache_sync()"
> (d3fa72e4556ec1f04e46a0d561d9e785ecaa173d), dma_cache_sync() takes a
> struct dev pointer, but these appear to be missing from the tile and
> mn10300 implementations, so add them.
> 
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> ---
>  arch/mn10300/include/asm/dma-mapping.h |    2 +-
>  arch/tile/include/asm/dma-mapping.h    |    3 ++-
>  arch/tile/kernel/pci-dma.c             |    2 +-
>  3 files changed, 4 insertions(+), 3 deletions(-)

Looks fine (I don't even compile it though).

btw, mn10300's dma_cache_sync looks wrong.

DMA-API says:

Do a partial sync of memory that was allocated by
dma_alloc_noncoherent(), starting at virtual address vaddr and
continuing on for size.  Again, you *must* observe the cache line
boundaries when doing this.

looks like mn10300's dma_alloc_noncoherent() returns consistent
memory. So dma_cache_sync should be a null function.

Re: [PATCH] tile,mn10300: add device parameter to dma_cache_sync()

[Chris Metcalf]

On 4/4/2011 11:21 AM, James Hogan wrote:
> From: James Hogan <james.hogan@imgtec.com>
>
> Note: I'm not in a position to compile test this patch, but I don't
> foresee any problems.
>
> Since v2.6.20 "Pass struct dev pointer to dma_cache_sync()"
> (d3fa72e4556ec1f04e46a0d561d9e785ecaa173d), dma_cache_sync() takes a
> struct dev pointer, but these appear to be missing from the tile and
> mn10300 implementations, so add them.
>
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> ---
>  arch/mn10300/include/asm/dma-mapping.h |    2 +-
>  arch/tile/include/asm/dma-mapping.h    |    3 ++-
>  arch/tile/kernel/pci-dma.c             |    2 +-

This change looks fine for tile (not compile-tested but pretty obvious), so

Acked-by: Chris Metcalf <cmetcalf@tilera.com>

assuming you're planning to push this up yourself.

Apparently we've never tried to build anything that uses this API
(sata_dwc_460ex.c, lasi_82596.c, 53c700.c, etc.).

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] tile,mn10300: add device parameter to dma_cache_sync()

[James Hogan]

On Tue, Apr 05, 2011 at 02:50:40AM +0900, FUJITA Tomonori wrote:
> On Mon, 4 Apr 2011 16:21:47 +0100
> James Hogan <james@albanarts.com> wrote:
> 
> > From: James Hogan <james.hogan@imgtec.com>
> > 
> > Note: I'm not in a position to compile test this patch, but I don't
> > foresee any problems.
> > 
> > Since v2.6.20 "Pass struct dev pointer to dma_cache_sync()"
> > (d3fa72e4556ec1f04e46a0d561d9e785ecaa173d), dma_cache_sync() takes a
> > struct dev pointer, but these appear to be missing from the tile and
> > mn10300 implementations, so add them.
> > 
> > Signed-off-by: James Hogan <james.hogan@imgtec.com>
> > ---
> >  arch/mn10300/include/asm/dma-mapping.h |    2 +-
> >  arch/tile/include/asm/dma-mapping.h    |    3 ++-
> >  arch/tile/kernel/pci-dma.c             |    2 +-
> >  3 files changed, 4 insertions(+), 3 deletions(-)
> 
> Looks fine (I don't even compile it though).
> 
> btw, mn10300's dma_cache_sync looks wrong.
> 
> DMA-API says:
> 
> Do a partial sync of memory that was allocated by
> dma_alloc_noncoherent(), starting at virtual address vaddr and
> continuing on for size.  Again, you *must* observe the cache line
> boundaries when doing this.
> 
> looks like mn10300's dma_alloc_noncoherent() returns consistent
> memory. So dma_cache_sync should be a null function.

I noticed it explicitly says:
442 Note: where the platform can return consistent memory, it will
443 guarantee that the sync points become nops.

I presume dma_cache_sync() is the "sync points" it is referring to.

By the looks of it though the following architectures implement it but
define the noncoherent functions to the coherent versions so shouldn't
implement it (as far as I can tell):
 avr,blackfin,frv,microblaze,powerpc,sh,unicore32,x86,xtensia

and ia64 says it isn't required so just does a full memory barrier
instead (mb()).

Cheers
James

Re: [PATCH] tile,mn10300: add device parameter to dma_cache_sync()

[James Hogan]

Hi Chris,

On Mon, Apr 04, 2011 at 03:16:03PM -0400, Chris Metcalf wrote:
> On 4/4/2011 11:21 AM, James Hogan wrote:
> > From: James Hogan <james.hogan@imgtec.com>
> >
> > Note: I'm not in a position to compile test this patch, but I don't
> > foresee any problems.
> >
> > Since v2.6.20 "Pass struct dev pointer to dma_cache_sync()"
> > (d3fa72e4556ec1f04e46a0d561d9e785ecaa173d), dma_cache_sync() takes a
> > struct dev pointer, but these appear to be missing from the tile and
> > mn10300 implementations, so add them.
> >
> > Signed-off-by: James Hogan <james.hogan@imgtec.com>
> > ---
> >  arch/mn10300/include/asm/dma-mapping.h |    2 +-
> >  arch/tile/include/asm/dma-mapping.h    |    3 ++-
> >  arch/tile/kernel/pci-dma.c             |    2 +-
> 
> This change looks fine for tile (not compile-tested but pretty obvious), so
> 
> Acked-by: Chris Metcalf <cmetcalf@tilera.com>

Thanks

> 
> assuming you're planning to push this up yourself.

I'm not sure. I'm not that familiar with the process (my other patches
tend to have just been picked up by other people).

Cheers
James

Re: [PATCH] tile,mn10300: add device parameter to dma_cache_sync()

[Chris Metcalf]

On 4/4/2011 7:20 PM, James Hogan wrote:
> On Mon, Apr 04, 2011 at 03:16:03PM -0400, Chris Metcalf wrote:
>> On 4/4/2011 11:21 AM, James Hogan wrote:
>>> From: James Hogan <james.hogan@imgtec.com>
>>>
>>> Note: I'm not in a position to compile test this patch, but I don't
>>> foresee any problems.
>>>
>>> Since v2.6.20 "Pass struct dev pointer to dma_cache_sync()"
>>> (d3fa72e4556ec1f04e46a0d561d9e785ecaa173d), dma_cache_sync() takes a
>>> struct dev pointer, but these appear to be missing from the tile and
>>> mn10300 implementations, so add them.
>>>
>>> Signed-off-by: James Hogan <james.hogan@imgtec.com>
>>> ---
>>>  arch/mn10300/include/asm/dma-mapping.h |    2 +-
>>>  arch/tile/include/asm/dma-mapping.h    |    3 ++-
>>>  arch/tile/kernel/pci-dma.c             |    2 +-
>> assuming you're planning to push this up yourself.
> I'm not sure. I'm not that familiar with the process (my other patches
> tend to have just been picked up by other people).

I'll take it into my tree as well then.  Thanks.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Re: [PATCH] tile,mn10300: add device parameter to dma_cache_sync()

[FUJITA Tomonori]

On Mon, 4 Apr 2011 20:50:41 +0100
James Hogan <james@albanarts.com> wrote:

> On Tue, Apr 05, 2011 at 02:50:40AM +0900, FUJITA Tomonori wrote:
> > On Mon, 4 Apr 2011 16:21:47 +0100
> > James Hogan <james@albanarts.com> wrote:
> > 
> > > From: James Hogan <james.hogan@imgtec.com>
> > > 
> > > Note: I'm not in a position to compile test this patch, but I don't
> > > foresee any problems.
> > > 
> > > Since v2.6.20 "Pass struct dev pointer to dma_cache_sync()"
> > > (d3fa72e4556ec1f04e46a0d561d9e785ecaa173d), dma_cache_sync() takes a
> > > struct dev pointer, but these appear to be missing from the tile and
> > > mn10300 implementations, so add them.
> > > 
> > > Signed-off-by: James Hogan <james.hogan@imgtec.com>
> > > ---
> > >  arch/mn10300/include/asm/dma-mapping.h |    2 +-
> > >  arch/tile/include/asm/dma-mapping.h    |    3 ++-
> > >  arch/tile/kernel/pci-dma.c             |    2 +-
> > >  3 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > Looks fine (I don't even compile it though).
> > 
> > btw, mn10300's dma_cache_sync looks wrong.
> > 
> > DMA-API says:
> > 
> > Do a partial sync of memory that was allocated by
> > dma_alloc_noncoherent(), starting at virtual address vaddr and
> > continuing on for size.  Again, you *must* observe the cache line
> > boundaries when doing this.
> > 
> > looks like mn10300's dma_alloc_noncoherent() returns consistent
> > memory. So dma_cache_sync should be a null function.
> 
> I noticed it explicitly says:
> 442 Note: where the platform can return consistent memory, it will
> 443 guarantee that the sync points become nops.
> 
> I presume dma_cache_sync() is the "sync points" it is referring to.

I think that it's the "sync points" for architectures that return
either consistent or non-consistent memory.


> By the looks of it though the following architectures implement it but
> define the noncoherent functions to the coherent versions so shouldn't
> implement it (as far as I can tell):
>  avr,blackfin,frv,microblaze,powerpc,sh,unicore32,x86,xtensia

Yeah, I knew when I clean up the DMA API. But few uses
dma_cache_sync() so I left them alone.


> and ia64 says it isn't required so just does a full memory barrier
> instead (mb()).

I think that drivers need to care about such.