Hi Arnd,
On 28/06/22 19:08, Arnd Bergmann wrote:
On Tue, Jun 28, 2022 at 5:25 AM Michael Schmitz <schmitzmic@xxxxxxxxx> wrote:
Am 28.06.2022 um 09:12 schrieb Michael Schmitz:
Leaving the bounce buffer handling in place, and taking a few other
liberties - this is what converting the easiest case (a3000 SCSI) might
look like. Any obvious mistakes? The mvme147 driver would be very
similar to handle (after conversion to a platform device).
The driver allocates bounce buffers using kmalloc if it hits an
unaligned data buffer - can such buffers still even happen these days?
If I understand dma_map_single() correctly, the resulting dma handle
would be equally misaligned?
To allocate a bounce buffer, would it be OK to use dma_alloc_coherent()
even though AFAIU memory used for DMA buffers generally isn't consistent
on m68k?
I think it makes sense to skip the bounce buffering as you do here:
the only standardized way we have for integrating that part is to
use the swiotlb infrastructure, but as you mentioned earlier that
part is probably too resource-heavy here for Amiga.
OK, leaving the old custom logic in place allows to convert the 24 bit
DMA drivers more easily.
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.
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.
Cheers,
Michael
Arnd
