Hi Arnd,

On 29/06/22 09:55, Arnd Bergmann wrote:

On Tue, Jun 28, 2022 at 11:38 PM Michael Schmitz [off-list ref] wrote:

quoted

On 28/06/22 19:08, Arnd Bergmann wrote:

quoted

I see two other problems with your patch though:

a) you still duplicate the cache handling: the cache_clear()/cache_push()
is supposed to already be done by dma_map_single() when the device
is not cache-coherent.

That's one of the 'liberties' I alluded to. The reason I left these in
is that I'm none too certain what device feature the DMA API uses to
decide a device isn't cache-coherent. If it's dev->coherent_dma_mask,
the way I set up the device in the a3000 driver should leave the
coherent mask unchanged. For the Zorro drivers, devices are set up to
use the same storage to store normal and coherent masks - something we
most likely want to change. I need to think about the ramifications of
that.

Note that zorro_esp.c uses dma_sync_single_for_device() and uses a 32
bit coherent DMA mask which does work OK. I might  ask Adrian to test a
change to only set dev->dma_mask, and drop the
dma_sync_single_for_device() calls if there's any doubt on this aspect.

The "coherent_mask" is independent of the cache flushing. On some
architectures, a device can indicate whether it needs cache management
or not to guarantee coherency, but on m68k it appears that we always
assume it does, see arch/m68k/kernel/dma.c

Thanks - what I see there indicates that on the relevant platforms, 
pages mapped for DMA have their page table cache bits modified to make 
them non-cacheable (and I suppose unmapping restores the default cache 
bits). That means I should use dma_set_mask_and_coherent() here to take 
advantage of this, and no need to mess around with 
dma_sync_single_for_device() in the drivers' dma_setup() functions.

quoted

quoted

b) The bounce buffer is never mapped here, instead you have the
virt_to_phys() here, which is not the same. I think you need to map
the pointer that actually gets passed down to the device after deciding
to use a bouce buffer or not.

I hadn't realized that I can map the bounce buffer just as it's done for
the SCp data buffer. Should have been obvious, but I'm still learning
about the DMA API.

I've updated the patch now, will re-send as part of a complete series
once done.

I suppose you can just drop the bounce buffer if this just comes
from kmalloc().

That's only true for a3000 and mvme147 though.

Cheers,

     Michael

        Arnd