[PATCH 2/2] MIPS: Loongson64: Add cache_sync to loongson_dma_map_ops

[PATCH 2/2] MIPS: Loongson64: Add cache_sync to loongson_dma_map_ops

[Huacai Chen]

To support coherent & non-coherent DMA co-exsistance, we should add
cache_sync to loongson_dma_map_ops.

Signed-off-by: Huacai Chen <chenhc@lemote.com>
---
 arch/mips/include/asm/dma-mapping.h       | 3 +++
 arch/mips/loongson64/common/dma-swiotlb.c | 1 +
 arch/mips/mm/dma-default.c                | 2 +-
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/mips/include/asm/dma-mapping.h b/arch/mips/include/asm/dma-mapping.h
index 0d9418d..5544276 100644
--- a/arch/mips/include/asm/dma-mapping.h
+++ b/arch/mips/include/asm/dma-mapping.h
@@ -37,4 +37,7 @@ static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
 #endif
 }
 
+void mips_dma_cache_sync(struct device *dev, void *vaddr,
+		size_t size, enum dma_data_direction direction);
+
 #endif /* _ASM_DMA_MAPPING_H */
diff --git a/arch/mips/loongson64/common/dma-swiotlb.c b/arch/mips/loongson64/common/dma-swiotlb.c
index ef07740..17956f2 100644
--- a/arch/mips/loongson64/common/dma-swiotlb.c
+++ b/arch/mips/loongson64/common/dma-swiotlb.c
@@ -120,6 +120,7 @@ static const struct dma_map_ops loongson_dma_map_ops = {
 	.sync_sg_for_device = loongson_dma_sync_sg_for_device,
 	.mapping_error = swiotlb_dma_mapping_error,
 	.dma_supported = loongson_dma_supported,
+	.cache_sync = mips_dma_cache_sync,
 };
 
 void __init plat_swiotlb_setup(void)
diff --git a/arch/mips/mm/dma-default.c b/arch/mips/mm/dma-default.c
index e3e94d0..e86bf5d 100644
--- a/arch/mips/mm/dma-default.c
+++ b/arch/mips/mm/dma-default.c
@@ -383,7 +383,7 @@ static int mips_dma_supported(struct device *dev, u64 mask)
 	return plat_dma_supported(dev, mask);
 }
 
-static void mips_dma_cache_sync(struct device *dev, void *vaddr, size_t size,
+void mips_dma_cache_sync(struct device *dev, void *vaddr, size_t size,
 			 enum dma_data_direction direction)
 {
 	BUG_ON(direction == DMA_NONE);
-- 
2.7.0

Re: [PATCH 2/2] MIPS: Loongson64: Add cache_sync to loongson_dma_map_ops

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 3284 bytes --]

On Thu, Nov 16, 2017 at 04:35:55PM +0800, Huacai Chen wrote:
> To support coherent & non-coherent DMA co-exsistance, we should add
> cache_sync to loongson_dma_map_ops.
> 
> Signed-off-by: Huacai Chen <chenhc@lemote.com>

I presume this was broken by commit c9eb6172c328 ("dma-mapping: turn
dma_cache_sync into a dma_map_ops method") in 4.15-rc1? (Christoph Cc'd)

In that case:

1) we should have a fixes tag:
Fixes: c9eb6172c328 ("dma-mapping: turn dma_cache_sync into a dma_map_ops method")

2) we should get this into 4.15 final (though its probably pushing it a
bit now).

3) Loongson might not be the only MIPS platform that was broken by that
commit. Octeon appears to be coherent, so thats fine. However Netlogic
appears not to be (Jayachandran Cc'd).

Should the following be added?

diff --git a/arch/mips/netlogic/common/nlm-dma.c b/arch/mips/netlogic/common/nlm-dma.c
index 0ec9d9da6d51..58049da72c82 100644
--- a/arch/mips/netlogic/common/nlm-dma.c
+++ b/arch/mips/netlogic/common/nlm-dma.c
@@ -79,7 +79,8 @@ const struct dma_map_ops nlm_swiotlb_dma_ops = {
 	.sync_sg_for_cpu = swiotlb_sync_sg_for_cpu,
 	.sync_sg_for_device = swiotlb_sync_sg_for_device,
 	.mapping_error = swiotlb_dma_mapping_error,
-	.dma_supported = swiotlb_dma_supported
+	.dma_supported = swiotlb_dma_supported,
+	.cache_sync = mips_dma_cache_sync,
 };
 
 void __init plat_swiotlb_setup(void)

Cheers
James

> ---
>  arch/mips/include/asm/dma-mapping.h       | 3 +++
>  arch/mips/loongson64/common/dma-swiotlb.c | 1 +
>  arch/mips/mm/dma-default.c                | 2 +-
>  3 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/mips/include/asm/dma-mapping.h b/arch/mips/include/asm/dma-mapping.h
> index 0d9418d..5544276 100644
> --- a/arch/mips/include/asm/dma-mapping.h
> +++ b/arch/mips/include/asm/dma-mapping.h
> @@ -37,4 +37,7 @@ static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
>  #endif
>  }
>  
> +void mips_dma_cache_sync(struct device *dev, void *vaddr,
> +		size_t size, enum dma_data_direction direction);
> +
>  #endif /* _ASM_DMA_MAPPING_H */
> diff --git a/arch/mips/loongson64/common/dma-swiotlb.c b/arch/mips/loongson64/common/dma-swiotlb.c
> index ef07740..17956f2 100644
> --- a/arch/mips/loongson64/common/dma-swiotlb.c
> +++ b/arch/mips/loongson64/common/dma-swiotlb.c
> @@ -120,6 +120,7 @@ static const struct dma_map_ops loongson_dma_map_ops = {
>  	.sync_sg_for_device = loongson_dma_sync_sg_for_device,
>  	.mapping_error = swiotlb_dma_mapping_error,
>  	.dma_supported = loongson_dma_supported,
> +	.cache_sync = mips_dma_cache_sync,
>  };
>  
>  void __init plat_swiotlb_setup(void)
> diff --git a/arch/mips/mm/dma-default.c b/arch/mips/mm/dma-default.c
> index e3e94d0..e86bf5d 100644
> --- a/arch/mips/mm/dma-default.c
> +++ b/arch/mips/mm/dma-default.c
> @@ -383,7 +383,7 @@ static int mips_dma_supported(struct device *dev, u64 mask)
>  	return plat_dma_supported(dev, mask);
>  }
>  
> -static void mips_dma_cache_sync(struct device *dev, void *vaddr, size_t size,
> +void mips_dma_cache_sync(struct device *dev, void *vaddr, size_t size,
>  			 enum dma_data_direction direction)
>  {
>  	BUG_ON(direction == DMA_NONE);
> -- 
> 2.7.0
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] MIPS: Loongson64: Add cache_sync to loongson_dma_map_ops

[Christoph Hellwig]

On Wed, Jan 24, 2018 at 02:02:35PM +0000, James Hogan wrote:
> On Thu, Nov 16, 2017 at 04:35:55PM +0800, Huacai Chen wrote:
> > To support coherent & non-coherent DMA co-exsistance, we should add
> > cache_sync to loongson_dma_map_ops.
> > 
> > Signed-off-by: Huacai Chen <chenhc@lemote.com>
> 
> I presume this was broken by commit c9eb6172c328 ("dma-mapping: turn
> dma_cache_sync into a dma_map_ops method") in 4.15-rc1? (Christoph Cc'd)

I don't think that is the case.

In mips only mips_default_dma_map_ops supports DMA_ATTR_NON_CONSISTENT,
and mips_default_dma_map_ops grew support for the dma_cache_sync
operation.

Neither Octeon nor longsoon respect the DMA_ATTR_NON_CONSISTENT argument
to dma_alloc_attrs, so there is no point in implementing dma_cache_sync
for them.

Re: [PATCH 2/2] MIPS: Loongson64: Add cache_sync to loongson_dma_map_ops

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]

On Wed, Jan 24, 2018 at 03:11:44PM +0100, Christoph Hellwig wrote:
> On Wed, Jan 24, 2018 at 02:02:35PM +0000, James Hogan wrote:
> > On Thu, Nov 16, 2017 at 04:35:55PM +0800, Huacai Chen wrote:
> > > To support coherent & non-coherent DMA co-exsistance, we should add
> > > cache_sync to loongson_dma_map_ops.
> > > 
> > > Signed-off-by: Huacai Chen <chenhc@lemote.com>
> > 
> > I presume this was broken by commit c9eb6172c328 ("dma-mapping: turn
> > dma_cache_sync into a dma_map_ops method") in 4.15-rc1? (Christoph Cc'd)
> 
> I don't think that is the case.
> 
> In mips only mips_default_dma_map_ops supports DMA_ATTR_NON_CONSISTENT,
> and mips_default_dma_map_ops grew support for the dma_cache_sync
> operation.
> 
> Neither Octeon nor longsoon respect the DMA_ATTR_NON_CONSISTENT argument
> to dma_alloc_attrs, so there is no point in implementing dma_cache_sync
> for them.

I see, that makes sense. Thanks Christoph. I'll assume this patch isn't
applicable then unless Huacai adds some explanation.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] MIPS: Loongson64: Add cache_sync to loongson_dma_map_ops

[Christoph Hellwig]

On Wed, Jan 24, 2018 at 03:03:05PM +0000, James Hogan wrote:
> I see, that makes sense. Thanks Christoph. I'll assume this patch isn't
> applicable then unless Huacai adds some explanation.

In addition there also is no driver that can be used on loongsoon
that actually calls dma_cache_sync either.

Re: [PATCH 2/2] MIPS: Loongson64: Add cache_sync to loongson_dma_map_ops

[Huacai Chen]

Hi, James and Christoph,

We have modified arch/mips/loongson64/common/dma-swiotlb.c to make
Loongson support coherent&non-coherent devices co-exist. You can see
code here:
https://github.com/linux-loongson/linux-loongson/commit/3f212e36b2432a7c4a843649e4b79c9c0837d1d2
When device is non-coherent, we will call dma_cache_sync().

Then, if dma_cache_sync() is only designed for
DMA_ATTR_NON_CONSISTENT, what can I use?
1, __dma_sync_virtual() and __dma_sync() are both static functions, so
can't be called in other files.
2, mips_dma_cache_sync() is not static, but I think it isn't designed
to be called directly (So it should be static).
3, dma_cache_wback(), dma_cache_inv() and dma_cache_wback_inv() does't
take a "direction" argument, so I should determine the direction first
(then I will reimplement __dma_cache_sync for myself).
So, It seems that I can only use dma_cache_sync().

Huacai

On Wed, Jan 24, 2018 at 11:29 PM, Christoph Hellwig <hch@lst.de> wrote:
> On Wed, Jan 24, 2018 at 03:03:05PM +0000, James Hogan wrote:
>> I see, that makes sense. Thanks Christoph. I'll assume this patch isn't
>> applicable then unless Huacai adds some explanation.
>
> In addition there also is no driver that can be used on loongsoon
> that actually calls dma_cache_sync either.
>

Re: [PATCH 2/2] MIPS: Loongson64: Add cache_sync to loongson_dma_map_ops

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 2071 bytes --]

Hi Huacai,

On Thu, Jan 25, 2018 at 03:09:33PM +0800, Huacai Chen wrote:
> Hi, James and Christoph,
> 
> We have modified arch/mips/loongson64/common/dma-swiotlb.c to make
> Loongson support coherent&non-coherent devices co-exist. You can see
> code here:
> https://github.com/linux-loongson/linux-loongson/commit/3f212e36b2432a7c4a843649e4b79c9c0837d1d2
> When device is non-coherent, we will call dma_cache_sync().
> 
> Then, if dma_cache_sync() is only designed for
> DMA_ATTR_NON_CONSISTENT, what can I use?

How did you allocate the memory? The appropriate generic API for the
type of memory should always be used in drivers over arch specific
stuff.

AFAIK (see e.g. Documentation/DMA-API.txt):
- dma_alloc_coherent() shouldn't require syncing, though it may require
  flushing of write combiners
- dma_alloc_attrs() only requires syncing when DMA_ATTR_NON_CONSISTENT
  is used, otherwise is the same as dma_alloc_coherent()
- guaranteed contiguous memory within PA range (e.g. kmalloc()'d memory
  with the appropriate GFP_DMA flags) can be synced using the
  dma_map_*() and dma_unmap_*() functions.

Cheers
James

> 1, __dma_sync_virtual() and __dma_sync() are both static functions, so
> can't be called in other files.
> 2, mips_dma_cache_sync() is not static, but I think it isn't designed
> to be called directly (So it should be static).
> 3, dma_cache_wback(), dma_cache_inv() and dma_cache_wback_inv() does't
> take a "direction" argument, so I should determine the direction first
> (then I will reimplement __dma_cache_sync for myself).
> So, It seems that I can only use dma_cache_sync().
> 
> Huacai
> 
> On Wed, Jan 24, 2018 at 11:29 PM, Christoph Hellwig <hch@lst.de> wrote:
> > On Wed, Jan 24, 2018 at 03:03:05PM +0000, James Hogan wrote:
> >> I see, that makes sense. Thanks Christoph. I'll assume this patch isn't
> >> applicable then unless Huacai adds some explanation.
> >
> > In addition there also is no driver that can be used on loongsoon
> > that actually calls dma_cache_sync either.
> >

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] MIPS: Loongson64: Add cache_sync to loongson_dma_map_ops

[Huacai Chen]

On Thu, Jan 25, 2018 at 3:55 PM, James Hogan <jhogan@kernel.org> wrote:
> Hi Huacai,
>
> On Thu, Jan 25, 2018 at 03:09:33PM +0800, Huacai Chen wrote:
>> Hi, James and Christoph,
>>
>> We have modified arch/mips/loongson64/common/dma-swiotlb.c to make
>> Loongson support coherent&non-coherent devices co-exist. You can see
>> code here:
>> https://github.com/linux-loongson/linux-loongson/commit/3f212e36b2432a7c4a843649e4b79c9c0837d1d2
>> When device is non-coherent, we will call dma_cache_sync().
>>
>> Then, if dma_cache_sync() is only designed for
>> DMA_ATTR_NON_CONSISTENT, what can I use?
>
> How did you allocate the memory? The appropriate generic API for the
> type of memory should always be used in drivers over arch specific
> stuff.
>
> AFAIK (see e.g. Documentation/DMA-API.txt):
> - dma_alloc_coherent() shouldn't require syncing, though it may require
>   flushing of write combiners
> - dma_alloc_attrs() only requires syncing when DMA_ATTR_NON_CONSISTENT
>   is used, otherwise is the same as dma_alloc_coherent()
> - guaranteed contiguous memory within PA range (e.g. kmalloc()'d memory
>   with the appropriate GFP_DMA flags) can be synced using the
>   dma_map_*() and dma_unmap_*() functions.

Yes, kmalloc()'d memory with the appropriate GFP_DMA flags can be
synced using the dma_map_*() and dma_unmap_*() functions. So,
loongson_dma_map_page()/loongson_dma_unmap_page() (which is the
backend of dma_map_*() and dma_unmap_*()) should call dma_cache_sync()
for non-coherent devices, right?

Huacai

>
> Cheers
> James
>
>> 1, __dma_sync_virtual() and __dma_sync() are both static functions, so
>> can't be called in other files.
>> 2, mips_dma_cache_sync() is not static, but I think it isn't designed
>> to be called directly (So it should be static).
>> 3, dma_cache_wback(), dma_cache_inv() and dma_cache_wback_inv() does't
>> take a "direction" argument, so I should determine the direction first
>> (then I will reimplement __dma_cache_sync for myself).
>> So, It seems that I can only use dma_cache_sync().
>>
>> Huacai
>>
>> On Wed, Jan 24, 2018 at 11:29 PM, Christoph Hellwig <hch@lst.de> wrote:
>> > On Wed, Jan 24, 2018 at 03:03:05PM +0000, James Hogan wrote:
>> >> I see, that makes sense. Thanks Christoph. I'll assume this patch isn't
>> >> applicable then unless Huacai adds some explanation.
>> >
>> > In addition there also is no driver that can be used on loongsoon
>> > that actually calls dma_cache_sync either.
>> >

Re: [PATCH 2/2] MIPS: Loongson64: Add cache_sync to loongson_dma_map_ops

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 3135 bytes --]

On Thu, Jan 25, 2018 at 04:44:36PM +0800, Huacai Chen wrote:
> On Thu, Jan 25, 2018 at 3:55 PM, James Hogan <jhogan@kernel.org> wrote:
> > Hi Huacai,
> >
> > On Thu, Jan 25, 2018 at 03:09:33PM +0800, Huacai Chen wrote:
> >> Hi, James and Christoph,
> >>
> >> We have modified arch/mips/loongson64/common/dma-swiotlb.c to make
> >> Loongson support coherent&non-coherent devices co-exist. You can see
> >> code here:
> >> https://github.com/linux-loongson/linux-loongson/commit/3f212e36b2432a7c4a843649e4b79c9c0837d1d2
> >> When device is non-coherent, we will call dma_cache_sync().
> >>
> >> Then, if dma_cache_sync() is only designed for
> >> DMA_ATTR_NON_CONSISTENT, what can I use?
> >
> > How did you allocate the memory? The appropriate generic API for the
> > type of memory should always be used in drivers over arch specific
> > stuff.
> >
> > AFAIK (see e.g. Documentation/DMA-API.txt):
> > - dma_alloc_coherent() shouldn't require syncing, though it may require
> >   flushing of write combiners
> > - dma_alloc_attrs() only requires syncing when DMA_ATTR_NON_CONSISTENT
> >   is used, otherwise is the same as dma_alloc_coherent()
> > - guaranteed contiguous memory within PA range (e.g. kmalloc()'d memory
> >   with the appropriate GFP_DMA flags) can be synced using the
> >   dma_map_*() and dma_unmap_*() functions.

I also see there is a dma_sync_*_for_{cpu,device} to avoid "mapping" and
"unmapping" the same area repeatedly if it is reused.

> Yes, kmalloc()'d memory with the appropriate GFP_DMA flags can be
> synced using the dma_map_*() and dma_unmap_*() functions. So,
> loongson_dma_map_page()/loongson_dma_unmap_page() (which is the
> backend of dma_map_*() and dma_unmap_*()) should call dma_cache_sync()
> for non-coherent devices, right?

Yeh, I suppose thats effectively what it should do (though probably via
an arch specific function rather than dma_cache_sync() itself). Also the
sync_*_for_{cpu,device} callbacks should probably do it too. See all the
calls to __dma_sync() in the MIPS dma-default.c implementation.

Cheers
James

> 
> Huacai
> 
> >
> > Cheers
> > James
> >
> >> 1, __dma_sync_virtual() and __dma_sync() are both static functions, so
> >> can't be called in other files.
> >> 2, mips_dma_cache_sync() is not static, but I think it isn't designed
> >> to be called directly (So it should be static).
> >> 3, dma_cache_wback(), dma_cache_inv() and dma_cache_wback_inv() does't
> >> take a "direction" argument, so I should determine the direction first
> >> (then I will reimplement __dma_cache_sync for myself).
> >> So, It seems that I can only use dma_cache_sync().
> >>
> >> Huacai
> >>
> >> On Wed, Jan 24, 2018 at 11:29 PM, Christoph Hellwig <hch@lst.de> wrote:
> >> > On Wed, Jan 24, 2018 at 03:03:05PM +0000, James Hogan wrote:
> >> >> I see, that makes sense. Thanks Christoph. I'll assume this patch isn't
> >> >> applicable then unless Huacai adds some explanation.
> >> >
> >> > In addition there also is no driver that can be used on loongsoon
> >> > that actually calls dma_cache_sync either.
> >> >

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] MIPS: Loongson64: Add cache_sync to loongson_dma_map_ops

[Huacai Chen]

If call dma_cache_sync() is incorrect, then which is the correct way?
1, call mips_dma_cache_sync()?
2, remove static of __dma_sync_virtual() and call it?

Huacai

On Thu, Jan 25, 2018 at 6:54 PM, James Hogan <jhogan@kernel.org> wrote:
> On Thu, Jan 25, 2018 at 04:44:36PM +0800, Huacai Chen wrote:
>> On Thu, Jan 25, 2018 at 3:55 PM, James Hogan <jhogan@kernel.org> wrote:
>> > Hi Huacai,
>> >
>> > On Thu, Jan 25, 2018 at 03:09:33PM +0800, Huacai Chen wrote:
>> >> Hi, James and Christoph,
>> >>
>> >> We have modified arch/mips/loongson64/common/dma-swiotlb.c to make
>> >> Loongson support coherent&non-coherent devices co-exist. You can see
>> >> code here:
>> >> https://github.com/linux-loongson/linux-loongson/commit/3f212e36b2432a7c4a843649e4b79c9c0837d1d2
>> >> When device is non-coherent, we will call dma_cache_sync().
>> >>
>> >> Then, if dma_cache_sync() is only designed for
>> >> DMA_ATTR_NON_CONSISTENT, what can I use?
>> >
>> > How did you allocate the memory? The appropriate generic API for the
>> > type of memory should always be used in drivers over arch specific
>> > stuff.
>> >
>> > AFAIK (see e.g. Documentation/DMA-API.txt):
>> > - dma_alloc_coherent() shouldn't require syncing, though it may require
>> >   flushing of write combiners
>> > - dma_alloc_attrs() only requires syncing when DMA_ATTR_NON_CONSISTENT
>> >   is used, otherwise is the same as dma_alloc_coherent()
>> > - guaranteed contiguous memory within PA range (e.g. kmalloc()'d memory
>> >   with the appropriate GFP_DMA flags) can be synced using the
>> >   dma_map_*() and dma_unmap_*() functions.
>
> I also see there is a dma_sync_*_for_{cpu,device} to avoid "mapping" and
> "unmapping" the same area repeatedly if it is reused.
>
>> Yes, kmalloc()'d memory with the appropriate GFP_DMA flags can be
>> synced using the dma_map_*() and dma_unmap_*() functions. So,
>> loongson_dma_map_page()/loongson_dma_unmap_page() (which is the
>> backend of dma_map_*() and dma_unmap_*()) should call dma_cache_sync()
>> for non-coherent devices, right?
>
> Yeh, I suppose thats effectively what it should do (though probably via
> an arch specific function rather than dma_cache_sync() itself). Also the
> sync_*_for_{cpu,device} callbacks should probably do it too. See all the
> calls to __dma_sync() in the MIPS dma-default.c implementation.
>
> Cheers
> James
>
>>
>> Huacai
>>
>> >
>> > Cheers
>> > James
>> >
>> >> 1, __dma_sync_virtual() and __dma_sync() are both static functions, so
>> >> can't be called in other files.
>> >> 2, mips_dma_cache_sync() is not static, but I think it isn't designed
>> >> to be called directly (So it should be static).
>> >> 3, dma_cache_wback(), dma_cache_inv() and dma_cache_wback_inv() does't
>> >> take a "direction" argument, so I should determine the direction first
>> >> (then I will reimplement __dma_cache_sync for myself).
>> >> So, It seems that I can only use dma_cache_sync().
>> >>
>> >> Huacai
>> >>
>> >> On Wed, Jan 24, 2018 at 11:29 PM, Christoph Hellwig <hch@lst.de> wrote:
>> >> > On Wed, Jan 24, 2018 at 03:03:05PM +0000, James Hogan wrote:
>> >> >> I see, that makes sense. Thanks Christoph. I'll assume this patch isn't
>> >> >> applicable then unless Huacai adds some explanation.
>> >> >
>> >> > In addition there also is no driver that can be used on loongsoon
>> >> > that actually calls dma_cache_sync either.
>> >> >

Re: [PATCH 2/2] MIPS: Loongson64: Add cache_sync to loongson_dma_map_ops

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 3911 bytes --]

On Thu, Jan 25, 2018 at 07:04:37PM +0800, Huacai Chen wrote:
> If call dma_cache_sync() is incorrect,

(it wouldn't work since the cache_sync callback wouldn't be set).

> then which is the correct way?
> 1, call mips_dma_cache_sync()?
> 2, remove static of __dma_sync_virtual() and call it?

Both take a virtual address, which is obviously not directly usable for
functions which get a DMA handle, so __dma_sync() may be better.

Cheers
James

> 
> Huacai
> 
> On Thu, Jan 25, 2018 at 6:54 PM, James Hogan <jhogan@kernel.org> wrote:
> > On Thu, Jan 25, 2018 at 04:44:36PM +0800, Huacai Chen wrote:
> >> On Thu, Jan 25, 2018 at 3:55 PM, James Hogan <jhogan@kernel.org> wrote:
> >> > Hi Huacai,
> >> >
> >> > On Thu, Jan 25, 2018 at 03:09:33PM +0800, Huacai Chen wrote:
> >> >> Hi, James and Christoph,
> >> >>
> >> >> We have modified arch/mips/loongson64/common/dma-swiotlb.c to make
> >> >> Loongson support coherent&non-coherent devices co-exist. You can see
> >> >> code here:
> >> >> https://github.com/linux-loongson/linux-loongson/commit/3f212e36b2432a7c4a843649e4b79c9c0837d1d2
> >> >> When device is non-coherent, we will call dma_cache_sync().
> >> >>
> >> >> Then, if dma_cache_sync() is only designed for
> >> >> DMA_ATTR_NON_CONSISTENT, what can I use?
> >> >
> >> > How did you allocate the memory? The appropriate generic API for the
> >> > type of memory should always be used in drivers over arch specific
> >> > stuff.
> >> >
> >> > AFAIK (see e.g. Documentation/DMA-API.txt):
> >> > - dma_alloc_coherent() shouldn't require syncing, though it may require
> >> >   flushing of write combiners
> >> > - dma_alloc_attrs() only requires syncing when DMA_ATTR_NON_CONSISTENT
> >> >   is used, otherwise is the same as dma_alloc_coherent()
> >> > - guaranteed contiguous memory within PA range (e.g. kmalloc()'d memory
> >> >   with the appropriate GFP_DMA flags) can be synced using the
> >> >   dma_map_*() and dma_unmap_*() functions.
> >
> > I also see there is a dma_sync_*_for_{cpu,device} to avoid "mapping" and
> > "unmapping" the same area repeatedly if it is reused.
> >
> >> Yes, kmalloc()'d memory with the appropriate GFP_DMA flags can be
> >> synced using the dma_map_*() and dma_unmap_*() functions. So,
> >> loongson_dma_map_page()/loongson_dma_unmap_page() (which is the
> >> backend of dma_map_*() and dma_unmap_*()) should call dma_cache_sync()
> >> for non-coherent devices, right?
> >
> > Yeh, I suppose thats effectively what it should do (though probably via
> > an arch specific function rather than dma_cache_sync() itself). Also the
> > sync_*_for_{cpu,device} callbacks should probably do it too. See all the
> > calls to __dma_sync() in the MIPS dma-default.c implementation.
> >
> > Cheers
> > James
> >
> >>
> >> Huacai
> >>
> >> >
> >> > Cheers
> >> > James
> >> >
> >> >> 1, __dma_sync_virtual() and __dma_sync() are both static functions, so
> >> >> can't be called in other files.
> >> >> 2, mips_dma_cache_sync() is not static, but I think it isn't designed
> >> >> to be called directly (So it should be static).
> >> >> 3, dma_cache_wback(), dma_cache_inv() and dma_cache_wback_inv() does't
> >> >> take a "direction" argument, so I should determine the direction first
> >> >> (then I will reimplement __dma_cache_sync for myself).
> >> >> So, It seems that I can only use dma_cache_sync().
> >> >>
> >> >> Huacai
> >> >>
> >> >> On Wed, Jan 24, 2018 at 11:29 PM, Christoph Hellwig <hch@lst.de> wrote:
> >> >> > On Wed, Jan 24, 2018 at 03:03:05PM +0000, James Hogan wrote:
> >> >> >> I see, that makes sense. Thanks Christoph. I'll assume this patch isn't
> >> >> >> applicable then unless Huacai adds some explanation.
> >> >> >
> >> >> > In addition there also is no driver that can be used on loongsoon
> >> >> > that actually calls dma_cache_sync either.
> >> >> >

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] MIPS: Loongson64: Add cache_sync to loongson_dma_map_ops

[Christoph Hellwig]

On Thu, Jan 25, 2018 at 04:44:36PM +0800, Huacai Chen wrote:
> Yes, kmalloc()'d memory with the appropriate GFP_DMA flags can be

You should never use GFP_DMA in kmalloc calls in new code.

> synced using the dma_map_*() and dma_unmap_*() functions. So,
> loongson_dma_map_page()/loongson_dma_unmap_page() (which is the
> backend of dma_map_*() and dma_unmap_*()) should call dma_cache_sync()
> for non-coherent devices, right?

No, it should call the internal dma sync for device/cpu calls, as already
explained by James.  Note that for 4.17 or 4.18 I plan to extent
the dma-direct work to not dma coherent systems, so we'll get an
explicit and document ABI for those across platforms.  That is if
I can pull it off as it doesn't seem entirely trivial.