[PATCH v1 0/3] Converting m68k WD33C93 drivers to DMA API

[PATCH v1 0/3] Converting m68k WD33C93 drivers to DMA API

[Michael Schmitz]

V1 of a patch series to convert m68k Amiga WD33C93 drivers to the
DMA API.

This series was precipitated by Arnd removing CONFIG_VIRT_TO_BUS. The
m68k WD33C93 still used virt_to_bus to convert virtual addresses to
physical addresses suitable for the DMA engines (note m68k does not
have an IOMMU and uses a direct mapping for DMA addresses). 

Arnd suggested to use dma_map_single() to set up dma mappings instead
of open-coding much the same in every driver dma_setup() function.

It appears that m68k (MMU, except for coldfire) will set up pages for
DMA transfers as non-cacheable, thus obviating the need for explicit
cache management. 

DMA setup on a3000 host adapters can be simplified to skip bounce
buffer use (assuming SCSI buffers passed to the driver are cache
line aligned; a safe bet except for maybe sg.c input). 

On gvp11 and a2091 host adapters, only the lowest 16 MB of physical
memory can be directy addressed by DMA, and bounce buffers from that
space must still be used (possibly allocated from chip RAM using the
custom allocator) if buffers are located in the higher memory regions. 

The m68k VME mvme147 driver has no DMA addressing or alignment
restrictions and can be converted in the same way as the Amiga a3000
one, but will require conversion to a platform device driver first.

Only compile tested so far, and hardware testing might be hard to do.
I'd appreciate someone giving this a thorough review.

Thanks, and Cheers,

   Michael

[PATCH v1 1/3] scsi - a3000.c: convert m68k WD33C93 drivers to DMA API

[Michael Schmitz]

Use dma_map_single() for a3000 driver (leave bounce buffer
logic unchanged).

Use dma_set_mask_and_coherent() to avoid explicit cache
flushes.

Incorporates review comments by ArndB:
- drop bounce buffer handling - non-cacheline aligned
  buffers ought not to happen.

Compile-tested only.

CC: linux-scsi@vger.kernel.org
Link: https://lore.kernel.org/r/6d1d88ee-1cf6-c735-1e6d-bafd2096e322@gmail.com
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---
 drivers/scsi/a3000.c | 60 ++++++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/drivers/scsi/a3000.c b/drivers/scsi/a3000.c
index dd161885eed1..0e2e0d69118b 100644
--- a/drivers/scsi/a3000.c
+++ b/drivers/scsi/a3000.c
@@ -7,6 +7,7 @@
 #include <linux/spinlock.h>
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
 #include <linux/module.h>
 
 #include <asm/page.h>
@@ -25,8 +26,11 @@
 struct a3000_hostdata {
 	struct WD33C93_hostdata wh;
 	struct a3000_scsiregs *regs;
+	struct device *dev;
 };
 
+#define DMA_DIR(d)   ((d == DATA_OUT_DIR) ? DMA_TO_DEVICE : DMA_FROM_DEVICE)
+
 static irqreturn_t a3000_intr(int irq, void *data)
 {
 	struct Scsi_Host *instance = data;
@@ -49,37 +53,40 @@ static irqreturn_t a3000_intr(int irq, void *data)
 static int dma_setup(struct scsi_cmnd *cmd, int dir_in)
 {
 	struct scsi_pointer *scsi_pointer = WD33C93_scsi_pointer(cmd);
+	unsigned long len = scsi_pointer->this_residual;
 	struct Scsi_Host *instance = cmd->device->host;
 	struct a3000_hostdata *hdata = shost_priv(instance);
 	struct WD33C93_hostdata *wh = &hdata->wh;
 	struct a3000_scsiregs *regs = hdata->regs;
 	unsigned short cntr = CNTR_PDMD | CNTR_INTEN;
-	unsigned long addr = virt_to_bus(scsi_pointer->ptr);
+	dma_addr_t addr;
+
+	addr = dma_map_single(hdata->dev, scsi_pointer->ptr,
+			      len, DMA_DIR(dir_in));
+	if (dma_mapping_error(hdata->dev, addr)) {
+		dev_warn(hdata->dev, "cannot map SCSI data block %p\n",
+			 scsi_pointer->ptr);
+		return 1;
+	}
+	scsi_pointer->dma_handle = addr;
 
 	/*
 	 * if the physical address has the wrong alignment, or if
 	 * physical address is bad, or if it is a write and at the
 	 * end of a physical memory chunk, then allocate a bounce
 	 * buffer
+	 * MSch 20220629 - only wrong alignment tested, skip bounce
+	 * buffer set-up and just do PIO here. Buffers not aligned
+	 * to cachelines ought to not happen anymore.
 	 */
 	if (addr & A3000_XFER_MASK) {
-		wh->dma_bounce_len = (scsi_pointer->this_residual + 511) & ~0x1ff;
-		wh->dma_bounce_buffer = kmalloc(wh->dma_bounce_len,
-						GFP_KERNEL);
-
-		/* can't allocate memory; use PIO */
-		if (!wh->dma_bounce_buffer) {
-			wh->dma_bounce_len = 0;
-			return 1;
-		}
-
-		if (!dir_in) {
-			/* copy to bounce buffer for a write */
-			memcpy(wh->dma_bounce_buffer, scsi_pointer->ptr,
-			       scsi_pointer->this_residual);
-		}
-
-		addr = virt_to_bus(wh->dma_bounce_buffer);
+		WARN_ONCE(1, "Invalid alignment for DMA!");
+		/* drop useless mapping */
+		dma_unmap_single(hdata->dev, scsi_pointer->dma_handle,
+				 scsi_pointer->this_residual,
+				 DMA_DIR(dir_in));
+		scsi_pointer->dma_handle = (dma_addr_t) NULL;
+		return 1;
 	}
 
 	/* setup dma direction */
@@ -94,13 +101,7 @@ static int dma_setup(struct scsi_cmnd *cmd, int dir_in)
 	/* setup DMA *physical* address */
 	regs->ACR = addr;
 
-	if (dir_in) {
-		/* invalidate any cache */
-		cache_clear(addr, scsi_pointer->this_residual);
-	} else {
-		/* push any dirty cache */
-		cache_push(addr, scsi_pointer->this_residual);
-	}
+	/* no more cache flush here - coherent DMA setup used */
 
 	/* start DMA */
 	mb();			/* make sure setup is completed */
@@ -166,6 +167,9 @@ static void dma_stop(struct Scsi_Host *instance, struct scsi_cmnd *SCpnt,
 			wh->dma_bounce_len = 0;
 		}
 	}
+	dma_unmap_single(hdata->dev, scsi_pointer->dma_handle,
+			 scsi_pointer->this_residual,
+			 DMA_DIR(wh->dma_dir));
 }
 
 static struct scsi_host_template amiga_a3000_scsi_template = {
@@ -193,6 +197,11 @@ static int __init amiga_a3000_scsi_probe(struct platform_device *pdev)
 	wd33c93_regs wdregs;
 	struct a3000_hostdata *hdata;
 
+	if (dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32))) {
+		dev_warn(&pdev->dev, "cannot use 32 bit DMA\n");
+		return -ENODEV;
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
 		return -ENODEV;
@@ -216,6 +225,7 @@ static int __init amiga_a3000_scsi_probe(struct platform_device *pdev)
 	wdregs.SCMD = &regs->SCMD;
 
 	hdata = shost_priv(instance);
+	hdata->dev = &pdev->dev;
 	hdata->wh.no_sync = 0xff;
 	hdata->wh.fast = 0;
 	hdata->wh.dma_mode = CTRL_DMA;
-- 
2.17.1

Re: [PATCH v1 1/3] scsi - a3000.c: convert m68k WD33C93 drivers to DMA API

[Arnd Bergmann]

On Wed, Jun 29, 2022 at 3:16 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>
> Use dma_map_single() for a3000 driver (leave bounce buffer
> logic unchanged).
>
> Use dma_set_mask_and_coherent() to avoid explicit cache
> flushes.
>
> Incorporates review comments by ArndB:
> - drop bounce buffer handling - non-cacheline aligned
>   buffers ought not to happen.
>
> Compile-tested only.
>
> CC: linux-scsi@vger.kernel.org
> Link: https://lore.kernel.org/r/6d1d88ee-1cf6-c735-1e6d-bafd2096e322@gmail.com
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

[PATCH v1 2/3] scsi - a2091.c: convert m68k WD33C93 drivers to DMA API

[Michael Schmitz]

Use dma_map_single() for a2091 driver (leave bounce buffer
logic unchanged).

Use dma_set_mask_and_coherent() to avoid explicit cache
flushes.

Compile-tested only.

CC: linux-scsi@vger.kernel.org
Link: https://lore.kernel.org/r/6d1d88ee-1cf6-c735-1e6d-bafd2096e322@gmail.com
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---
 drivers/scsi/a2091.c | 53 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/a2091.c b/drivers/scsi/a2091.c
index cf703a1ecdda..2338c75c34ed 100644
--- a/drivers/scsi/a2091.c
+++ b/drivers/scsi/a2091.c
@@ -24,8 +24,11 @@
 struct a2091_hostdata {
 	struct WD33C93_hostdata wh;
 	struct a2091_scsiregs *regs;
+	struct device *dev;
 };
 
+#define DMA_DIR(d)   ((d == DATA_OUT_DIR) ? DMA_TO_DEVICE : DMA_FROM_DEVICE)
+
 static irqreturn_t a2091_intr(int irq, void *data)
 {
 	struct Scsi_Host *instance = data;
@@ -45,15 +48,31 @@ static irqreturn_t a2091_intr(int irq, void *data)
 static int dma_setup(struct scsi_cmnd *cmd, int dir_in)
 {
 	struct scsi_pointer *scsi_pointer = WD33C93_scsi_pointer(cmd);
+	unsigned long len = scsi_pointer->this_residual;
 	struct Scsi_Host *instance = cmd->device->host;
 	struct a2091_hostdata *hdata = shost_priv(instance);
 	struct WD33C93_hostdata *wh = &hdata->wh;
 	struct a2091_scsiregs *regs = hdata->regs;
 	unsigned short cntr = CNTR_PDMD | CNTR_INTEN;
-	unsigned long addr = virt_to_bus(scsi_pointer->ptr);
+	dma_addr_t addr;
+
+	addr = dma_map_single(hdata->dev, scsi_pointer->ptr,
+			      len, DMA_DIR(dir_in));
+	if (dma_mapping_error(hdata->dev, addr)) {
+		dev_warn(hdata->dev, "cannot map SCSI data block %p\n",
+			 scsi_pointer->ptr);
+		return 1;
+	}
+	scsi_pointer->dma_handle = addr;
 
 	/* don't allow DMA if the physical address is bad */
 	if (addr & A2091_XFER_MASK) {
+		/* drop useless mapping */
+		dma_unmap_single(hdata->dev, scsi_pointer->dma_handle,
+				 scsi_pointer->this_residual,
+				 DMA_DIR(dir_in));
+		scsi_pointer->dma_handle = (dma_addr_t) NULL;
+
 		wh->dma_bounce_len = (scsi_pointer->this_residual + 511) & ~0x1ff;
 		wh->dma_bounce_buffer = kmalloc(wh->dma_bounce_len,
 						GFP_KERNEL);
@@ -64,9 +83,6 @@ static int dma_setup(struct scsi_cmnd *cmd, int dir_in)
 			return 1;
 		}
 
-		/* get the physical address of the bounce buffer */
-		addr = virt_to_bus(wh->dma_bounce_buffer);
-
 		/* the bounce buffer may not be in the first 16M of physmem */
 		if (addr & A2091_XFER_MASK) {
 			/* we could use chipmem... maybe later */
@@ -81,6 +97,17 @@ static int dma_setup(struct scsi_cmnd *cmd, int dir_in)
 			memcpy(wh->dma_bounce_buffer, scsi_pointer->ptr,
 			       scsi_pointer->this_residual);
 		}
+
+		/* may flush/invalidate cache for us */
+		addr = dma_map_single(hdata->dev, wh->dma_bounce_buffer,
+				      wh->dma_bounce_len, DMA_DIR(dir_in));
+		/* can't map buffer; use PIO */
+		if (dma_mapping_error(hdata->dev, addr)) {
+			dev_warn(hdata->dev, "cannot map bounce buffer %p\n",
+				 wh->dma_bounce_buffer);
+			return 1;
+		}
+		scsi_pointer->dma_handle = addr;
 	}
 
 	/* setup dma direction */
@@ -95,13 +122,8 @@ static int dma_setup(struct scsi_cmnd *cmd, int dir_in)
 	/* setup DMA *physical* address */
 	regs->ACR = addr;
 
-	if (dir_in) {
-		/* invalidate any cache */
-		cache_clear(addr, scsi_pointer->this_residual);
-	} else {
-		/* push any dirty cache */
-		cache_push(addr, scsi_pointer->this_residual);
-	}
+	/* no more cache flush here - using coherent DMA alloc */
+
 	/* start DMA */
 	regs->ST_DMA = 1;
 
@@ -151,6 +173,9 @@ static void dma_stop(struct Scsi_Host *instance, struct scsi_cmnd *SCpnt,
 		wh->dma_bounce_buffer = NULL;
 		wh->dma_bounce_len = 0;
 	}
+	dma_unmap_single(hdata->dev, scsi_pointer->dma_handle,
+			 scsi_pointer->this_residual,
+			 DMA_DIR(wh->dma_dir));
 }
 
 static struct scsi_host_template a2091_scsi_template = {
@@ -178,6 +203,11 @@ static int a2091_probe(struct zorro_dev *z, const struct zorro_device_id *ent)
 	wd33c93_regs wdregs;
 	struct a2091_hostdata *hdata;
 
+	if (dma_set_mask_and_coherent(&z->dev, DMA_BIT_MASK(24))) {
+		dev_warn(&z->dev, "cannot use 24 bit DMA\n");
+		return -ENODEV;
+	}
+
 	if (!request_mem_region(z->resource.start, 256, "wd33c93"))
 		return -EBUSY;
 
@@ -198,6 +228,7 @@ static int a2091_probe(struct zorro_dev *z, const struct zorro_device_id *ent)
 	wdregs.SCMD = &regs->SCMD;
 
 	hdata = shost_priv(instance);
+	hdata->dev = &z->dev;
 	hdata->wh.no_sync = 0xff;
 	hdata->wh.fast = 0;
 	hdata->wh.dma_mode = CTRL_DMA;
-- 
2.17.1

Re: [PATCH v1 2/3] scsi - a2091.c: convert m68k WD33C93 drivers to DMA API

[Arnd Bergmann]

On Wed, Jun 29, 2022 at 3:16 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>
> Use dma_map_single() for a2091 driver (leave bounce buffer
> logic unchanged).
>
> Use dma_set_mask_and_coherent() to avoid explicit cache
> flushes.
>
> Compile-tested only.
>
> CC: linux-scsi@vger.kernel.org
> Link: https://lore.kernel.org/r/6d1d88ee-1cf6-c735-1e6d-bafd2096e322@gmail.com
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

> +
> +       addr = dma_map_single(hdata->dev, scsi_pointer->ptr,
> +                             len, DMA_DIR(dir_in));
> +       if (dma_mapping_error(hdata->dev, addr)) {
> +               dev_warn(hdata->dev, "cannot map SCSI data block %p\n",
> +                        scsi_pointer->ptr);
> +               return 1;
> +       }
> +       scsi_pointer->dma_handle = addr;
>
>         /* don't allow DMA if the physical address is bad */
>         if (addr & A2091_XFER_MASK) {
> +               /* drop useless mapping */
> +               dma_unmap_single(hdata->dev, scsi_pointer->dma_handle,
> +                                scsi_pointer->this_residual,
> +                                DMA_DIR(dir_in));

I think you could save the extra map/unmap here if you wanted, but that
would risk introducing bugs since it requires a larger rework.

> +               scsi_pointer->dma_handle = (dma_addr_t) NULL;
> +
>                 wh->dma_bounce_len = (scsi_pointer->this_residual + 511) & ~0x1ff;
>                 wh->dma_bounce_buffer = kmalloc(wh->dma_bounce_len,
>                                                 GFP_KERNEL);

Not your bug, but if there is memory above the A2091_XFER_MASK limit,
this would need to use GFP_DMA instead of GFP_KERNEL.

         Arnd

Re: [PATCH v1 2/3] scsi - a2091.c: convert m68k WD33C93 drivers to DMA API

[Michael Schmitz]

Hi Arnd,

thanks for your review!

Am 29.06.2022 um 18:06 schrieb Arnd Bergmann:
> On Wed, Jun 29, 2022 at 3:16 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>>
>> Use dma_map_single() for a2091 driver (leave bounce buffer
>> logic unchanged).
>>
>> Use dma_set_mask_and_coherent() to avoid explicit cache
>> flushes.
>>
>> Compile-tested only.
>>
>> CC: linux-scsi@vger.kernel.org
>> Link: https://lore.kernel.org/r/6d1d88ee-1cf6-c735-1e6d-bafd2096e322@gmail.com
>> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
>
>> +
>> +       addr = dma_map_single(hdata->dev, scsi_pointer->ptr,
>> +                             len, DMA_DIR(dir_in));
>> +       if (dma_mapping_error(hdata->dev, addr)) {
>> +               dev_warn(hdata->dev, "cannot map SCSI data block %p\n",
>> +                        scsi_pointer->ptr);
>> +               return 1;
>> +       }
>> +       scsi_pointer->dma_handle = addr;
>>
>>         /* don't allow DMA if the physical address is bad */
>>         if (addr & A2091_XFER_MASK) {
>> +               /* drop useless mapping */
>> +               dma_unmap_single(hdata->dev, scsi_pointer->dma_handle,
>> +                                scsi_pointer->this_residual,
>> +                                DMA_DIR(dir_in));
>
> I think you could save the extra map/unmap here if you wanted, but that
> would risk introducing bugs since it requires a larger rework.

Not sure how to do that ...

>> +               scsi_pointer->dma_handle = (dma_addr_t) NULL;
>> +
>>                 wh->dma_bounce_len = (scsi_pointer->this_residual + 511) & ~0x1ff;
>>                 wh->dma_bounce_buffer = kmalloc(wh->dma_bounce_len,
>>                                                 GFP_KERNEL);
>
> Not your bug, but if there is memory above the A2091_XFER_MASK limit,
> this would need to use GFP_DMA instead of GFP_KERNEL.

Same argument goes for gvp11 - though last time I checked (and certainly 
at the time these drivers were written), there really was no difference 
between using GFP_DMA and GFP_KERNEL on m68k, hence the need to check 
the allocated buffer is indeed suitable for DMA, and perhaps revert to 
chip RAM (slow, useless for most other purposes but definitely below 16 
MB) if that fails (as the gvp11 driver does).

Most users will opt for loading the kernel to a RAM chunk at a higher 
physical address, making the lower chunk unusable (except as chip RAM), 
meaning allocating a bounce buffer within the first 16 MB will most 
likely fail anyway AIUI (but Geert would know that for sure).

Cheers,

	Michael

>
>          Arnd
>

Re: [PATCH v1 2/3] scsi - a2091.c: convert m68k WD33C93 drivers to DMA API

[Geert Uytterhoeven]

Hi Michael,

On Wed, Jun 29, 2022 at 9:27 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Most users will opt for loading the kernel to a RAM chunk at a higher
> physical address, making the lower chunk unusable (except as chip RAM),
> meaning allocating a bounce buffer within the first 16 MB will most
> likely fail anyway AIUI (but Geert would know that for sure).

Exactly.  On Zorro III-capable machines, Zorro II RAM is not mapped
for general use, but can be used by the z2ram block device.

Note that you can use it as a bounce buffer, by looking for and marking
free chunks in the zorro_unused_z2ram bitmap.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 2/3] scsi - a2091.c: convert m68k WD33C93 drivers to DMA API

[Michael Schmitz]

Hi Geert,

On 30/06/22 03:55, Geert Uytterhoeven wrote:
> Hi Michael,
>
> On Wed, Jun 29, 2022 at 9:27 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>> Most users will opt for loading the kernel to a RAM chunk at a higher
>> physical address, making the lower chunk unusable (except as chip RAM),
>> meaning allocating a bounce buffer within the first 16 MB will most
>> likely fail anyway AIUI (but Geert would know that for sure).
> Exactly.  On Zorro III-capable machines, Zorro II RAM is not mapped
> for general use, but can be used by the z2ram block device.
Is there any DMA capable memory other than chip and ZorroII RAM on these 
machines?
> Note that you can use it as a bounce buffer, by looking for and marking
> free chunks in the zorro_unused_z2ram bitmap.

That would be similar to what amiga_chip_alloc() does, right? Any 
advantages over using chip RAM (faster, perhaps)?

Cheers,

     Michael

>
> Gr{oetje,eeting}s,
>
>                          Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds

Re: [PATCH v1 2/3] scsi - a2091.c: convert m68k WD33C93 drivers to DMA API

[Geert Uytterhoeven]

Hi Michael,

On Wed, Jun 29, 2022 at 10:48 PM Michael Schmitz <schmitzmic@gmail.com> wrote:
> On 30/06/22 03:55, Geert Uytterhoeven wrote:
> > On Wed, Jun 29, 2022 at 9:27 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> >> Most users will opt for loading the kernel to a RAM chunk at a higher
> >> physical address, making the lower chunk unusable (except as chip RAM),
> >> meaning allocating a bounce buffer within the first 16 MB will most
> >> likely fail anyway AIUI (but Geert would know that for sure).
> > Exactly.  On Zorro III-capable machines, Zorro II RAM is not mapped
> > for general use, but can be used by the z2ram block device.
> Is there any DMA capable memory other than chip and ZorroII RAM on these
> machines?
> > Note that you can use it as a bounce buffer, by looking for and marking
> > free chunks in the zorro_unused_z2ram bitmap.
>
> That would be similar to what amiga_chip_alloc() does, right? Any
> advantages over using chip RAM (faster, perhaps)?

Yes, Z2RAM is faster, as Amiga custom chip DMA does not steal cycles
from Z2RAM.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 2/3] scsi - a2091.c: convert m68k WD33C93 drivers to DMA API

[Michael Schmitz]

Hi Geert,

On 30/06/22 21:27, Geert Uytterhoeven wrote:
>
>>> Note that you can use it as a bounce buffer, by looking for and marking
>>> free chunks in the zorro_unused_z2ram bitmap.
>> That would be similar to what amiga_chip_alloc() does, right? Any
>> advantages over using chip RAM (faster, perhaps)?
> Yes, Z2RAM is faster, as Amiga custom chip DMA does not steal cycles
> from Z2RAM.

OK, would make sense to use that, but that's well outside scope for this 
series.

Cheers,

     Michael


>
> Gr{oetje,eeting}s,
>
>                          Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds

[PATCH v1 3/3] scsi - gvp11.c: convert m68k WD33C93 drivers to DMA API

[Michael Schmitz]

Use dma_map_single() for gvp11 driver (leave bounce buffer
logic unchanged).

Use dma_set_mask_and_coherent() to avoid explicit cache
flushes.

Compile-tested only.

CC: linux-scsi@vger.kernel.org
Link: https://lore.kernel.org/r/6d1d88ee-1cf6-c735-1e6d-bafd2096e322@gmail.com
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---
 drivers/scsi/gvp11.c | 78 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 65 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/gvp11.c b/drivers/scsi/gvp11.c
index 2f6c56aabe1d..a793323e008f 100644
--- a/drivers/scsi/gvp11.c
+++ b/drivers/scsi/gvp11.c
@@ -26,8 +26,12 @@
 struct gvp11_hostdata {
 	struct WD33C93_hostdata wh;
 	struct gvp11_scsiregs *regs;
+	struct device *dev;
 };
 
+#define DMA_DIR(d)   ((d == DATA_OUT_DIR) ? DMA_TO_DEVICE : DMA_FROM_DEVICE)
+#define TO_DMA_MASK(m)	((~(m & 0xfffffff0))-1)
+
 static irqreturn_t gvp11_intr(int irq, void *data)
 {
 	struct Scsi_Host *instance = data;
@@ -54,17 +58,33 @@ void gvp11_setup(char *str, int *ints)
 static int dma_setup(struct scsi_cmnd *cmd, int dir_in)
 {
 	struct scsi_pointer *scsi_pointer = WD33C93_scsi_pointer(cmd);
+	unsigned long len = scsi_pointer->this_residual;
 	struct Scsi_Host *instance = cmd->device->host;
 	struct gvp11_hostdata *hdata = shost_priv(instance);
 	struct WD33C93_hostdata *wh = &hdata->wh;
 	struct gvp11_scsiregs *regs = hdata->regs;
 	unsigned short cntr = GVP11_DMAC_INT_ENABLE;
-	unsigned long addr = virt_to_bus(scsi_pointer->ptr);
+	dma_addr_t addr;
 	int bank_mask;
 	static int scsi_alloc_out_of_range = 0;
 
+	addr = dma_map_single(hdata->dev, scsi_pointer->ptr,
+			      len, DMA_DIR(dir_in));
+	if (dma_mapping_error(hdata->dev, addr)) {
+		dev_warn(hdata->dev, "cannot map SCSI data block %p\n",
+			 scsi_pointer->ptr);
+		return 1;
+	}
+	scsi_pointer->dma_handle = addr;
+
 	/* use bounce buffer if the physical address is bad */
 	if (addr & wh->dma_xfer_mask) {
+		/* drop useless mapping */
+		dma_unmap_single(hdata->dev, scsi_pointer->dma_handle,
+				 scsi_pointer->this_residual,
+				 DMA_DIR(dir_in));
+		scsi_pointer->dma_handle = (dma_addr_t) NULL;
+
 		wh->dma_bounce_len = (scsi_pointer->this_residual + 511) & ~0x1ff;
 
 		if (!scsi_alloc_out_of_range) {
@@ -87,10 +107,21 @@ static int dma_setup(struct scsi_cmnd *cmd, int dir_in)
 			wh->dma_buffer_pool = BUF_CHIP_ALLOCED;
 		}
 
-		/* check if the address of the bounce buffer is OK */
-		addr = virt_to_bus(wh->dma_bounce_buffer);
+		/* may flush/invalidate cache for us */
+		addr = dma_map_single(hdata->dev, wh->dma_bounce_buffer,
+				      wh->dma_bounce_len, DMA_DIR(dir_in));
+		/* can't map buffer; use PIO */
+		if (dma_mapping_error(hdata->dev, addr)) {
+			dev_warn(hdata->dev, "cannot map bounce buffer %p\n",
+				 wh->dma_bounce_buffer);
+			return 1;
+		}
 
 		if (addr & wh->dma_xfer_mask) {
+			/* drop useless mapping */
+			dma_unmap_single(hdata->dev, scsi_pointer->dma_handle,
+					 scsi_pointer->this_residual,
+					 DMA_DIR(dir_in));
 			/* fall back to Chip RAM if address out of range */
 			if (wh->dma_buffer_pool == BUF_SCSI_ALLOCED) {
 				kfree(wh->dma_bounce_buffer);
@@ -108,9 +139,19 @@ static int dma_setup(struct scsi_cmnd *cmd, int dir_in)
 				return 1;
 			}
 
-			addr = virt_to_bus(wh->dma_bounce_buffer);
+			/* may flush/invalidate cache for us */
+			addr = dma_map_single(hdata->dev, wh->dma_bounce_buffer,
+					      wh->dma_bounce_len, DMA_DIR(dir_in));
+			/* can't map buffer; use PIO */
+			if (dma_mapping_error(hdata->dev, addr)) {
+				dev_warn(hdata->dev, "cannot map bounce buffer %p\n",
+					 wh->dma_bounce_buffer);
+				return 1;
+			}
 			wh->dma_buffer_pool = BUF_CHIP_ALLOCED;
 		}
+		/* finally, have OK mapping (punted for PIO else) */
+		scsi_pointer->dma_handle = addr;
 
 		if (!dir_in) {
 			/* copy to bounce buffer for a write */
@@ -129,13 +170,7 @@ static int dma_setup(struct scsi_cmnd *cmd, int dir_in)
 	/* setup DMA *physical* address */
 	regs->ACR = addr;
 
-	if (dir_in) {
-		/* invalidate any cache */
-		cache_clear(addr, scsi_pointer->this_residual);
-	} else {
-		/* push any dirty cache */
-		cache_push(addr, scsi_pointer->this_residual);
-	}
+	/* no more cache flush here - using coherent memory! */
 
 	bank_mask = (~wh->dma_xfer_mask >> 18) & 0x01c0;
 	if (bank_mask)
@@ -175,6 +210,9 @@ static void dma_stop(struct Scsi_Host *instance, struct scsi_cmnd *SCpnt,
 		wh->dma_bounce_buffer = NULL;
 		wh->dma_bounce_len = 0;
 	}
+	dma_unmap_single(hdata->dev, scsi_pointer->dma_handle,
+			 scsi_pointer->this_residual,
+			 DMA_DIR(wh->dma_dir));
 }
 
 static struct scsi_host_template gvp11_scsi_template = {
@@ -287,6 +325,13 @@ static int gvp11_probe(struct zorro_dev *z, const struct zorro_device_id *ent)
 
 	default_dma_xfer_mask = ent->driver_data;
 
+	if (dma_set_mask_and_coherent(&z->dev,
+		TO_DMA_MASK(default_dma_xfer_mask))) {
+		dev_warn(&z->dev, "cannot use DMA mask %x\n",
+			 TO_DMA_MASK(default_dma_xfer_mask));
+		return -ENODEV;
+	}
+
 	/*
 	 * Rumors state that some GVP ram boards use the same product
 	 * code as the SCSI controllers. Therefore if the board-size
@@ -327,9 +372,16 @@ static int gvp11_probe(struct zorro_dev *z, const struct zorro_device_id *ent)
 	wdregs.SCMD = &regs->SCMD;
 
 	hdata = shost_priv(instance);
-	if (gvp11_xfer_mask)
+	if (gvp11_xfer_mask) {
 		hdata->wh.dma_xfer_mask = gvp11_xfer_mask;
-	else
+		if (dma_set_mask_and_coherent(&z->dev,
+			TO_DMA_MASK(gvp11_xfer_mask))) {
+			dev_warn(&z->dev, "cannot use DMA mask %x\n",
+				 TO_DMA_MASK(gvp11_xfer_mask));
+			error = -ENODEV;
+			goto fail_check_or_alloc;
+		}
+	} else
 		hdata->wh.dma_xfer_mask = default_dma_xfer_mask;
 
 	hdata->wh.no_sync = 0xff;
-- 
2.17.1

Re: [PATCH v1 3/3] scsi - gvp11.c: convert m68k WD33C93 drivers to DMA API

[Arnd Bergmann]

On Wed, Jun 29, 2022 at 3:16 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>
> Use dma_map_single() for gvp11 driver (leave bounce buffer
> logic unchanged).
>
> Use dma_set_mask_and_coherent() to avoid explicit cache
> flushes.
>
> Compile-tested only.
>
> CC: linux-scsi@vger.kernel.org
> Link: https://lore.kernel.org/r/6d1d88ee-1cf6-c735-1e6d-bafd2096e322@gmail.com
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH v1 0/3] Converting m68k WD33C93 drivers to DMA API

[Arnd Bergmann]

On Wed, Jun 29, 2022 at 3:16 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>
> V1 of a patch series to convert m68k Amiga WD33C93 drivers to the
> DMA API.
>
> This series was precipitated by Arnd removing CONFIG_VIRT_TO_BUS. The
> m68k WD33C93 still used virt_to_bus to convert virtual addresses to
> physical addresses suitable for the DMA engines (note m68k does not
> have an IOMMU and uses a direct mapping for DMA addresses).
>
> Arnd suggested to use dma_map_single() to set up dma mappings instead
> of open-coding much the same in every driver dma_setup() function.
>
> It appears that m68k (MMU, except for coldfire) will set up pages for
> DMA transfers as non-cacheable, thus obviating the need for explicit
> cache management.

To clarify, the dma-mapping API has two ways of dealing with this:

- the streaming API (dma_map/unmap_...) uses explicit cache flushes

- the coherent API (dma_alloc_coherent etc) uses page protections
  to prevent caching.

These three drivers use the streaming API because they operate on
data passed in from the outside, so the non-cacheable protection bits
are not used here.

> DMA setup on a3000 host adapters can be simplified to skip bounce
> buffer use (assuming SCSI buffers passed to the driver are cache> Thanks, and Cheers,
>
>    Michael
>

> line aligned; a safe bet except for maybe sg.c input).
>
> On gvp11 and a2091 host adapters, only the lowest 16 MB of physical
> memory can be directy addressed by DMA, and bounce buffers from that
> space must still be used (possibly allocated from chip RAM using the
> custom allocator) if buffers are located in the higher memory regions.
>
> The m68k VME mvme147 driver has no DMA addressing or alignment
> restrictions and can be converted in the same way as the Amiga a3000
> one, but will require conversion to a platform device driver first.

Right, it seems that the old hack of passing a NULL device pointer
no longer works, and that is probably for the better.

If you want an easy way out, the driver can just call
platform_device_register_full() to create its own device with a
dma_mask set up, and use that device for the DMA API, but
not actually match the device to a driver.

> Only compile tested so far, and hardware testing might be hard to do.
> I'd appreciate someone giving this a thorough review.

Looks all good to me.

        Arnd

Re: [PATCH v1 0/3] Converting m68k WD33C93 drivers to DMA API

[Michael Schmitz]

Hi Arnd,

Am 29.06.2022 um 18:19 schrieb Arnd Bergmann:
> On Wed, Jun 29, 2022 at 3:16 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
>>
>> V1 of a patch series to convert m68k Amiga WD33C93 drivers to the
>> DMA API.
>>
>> This series was precipitated by Arnd removing CONFIG_VIRT_TO_BUS. The
>> m68k WD33C93 still used virt_to_bus to convert virtual addresses to
>> physical addresses suitable for the DMA engines (note m68k does not
>> have an IOMMU and uses a direct mapping for DMA addresses).
>>
>> Arnd suggested to use dma_map_single() to set up dma mappings instead
>> of open-coding much the same in every driver dma_setup() function.
>>
>> It appears that m68k (MMU, except for coldfire) will set up pages for
>> DMA transfers as non-cacheable, thus obviating the need for explicit
>> cache management.
>
> To clarify, the dma-mapping API has two ways of dealing with this:
>
> - the streaming API (dma_map/unmap_...) uses explicit cache flushes
>
> - the coherent API (dma_alloc_coherent etc) uses page protections
>   to prevent caching.
>
> These three drivers use the streaming API because they operate on
> data passed in from the outside, so the non-cacheable protection bits
> are not used here.

I had feared you'd say something along these lines ...

Now that throws up a possible problem for the gvp11 driver: due to the 
need to first map an allocated chunk, then possibly discard that and try 
another allocation strategy, copying of data to the bounce buffer is 
deferred until after the final mapping has been successful. This means 
for writes, we have done the cache flushing before we have actually 
written any data to the buffer.

I don't think it is safe to omit the explicit cache flush for writes in 
this case.

>> DMA setup on a3000 host adapters can be simplified to skip bounce
>> buffer use (assuming SCSI buffers passed to the driver are cache> Thanks, and Cheers,
>>
>>    Michael
>>
>
>> line aligned; a safe bet except for maybe sg.c input).
>>
>> On gvp11 and a2091 host adapters, only the lowest 16 MB of physical
>> memory can be directy addressed by DMA, and bounce buffers from that
>> space must still be used (possibly allocated from chip RAM using the
>> custom allocator) if buffers are located in the higher memory regions.
>>
>> The m68k VME mvme147 driver has no DMA addressing or alignment
>> restrictions and can be converted in the same way as the Amiga a3000
>> one, but will require conversion to a platform device driver first.
>
> Right, it seems that the old hack of passing a NULL device pointer
> no longer works, and that is probably for the better.
>
> If you want an easy way out, the driver can just call
> platform_device_register_full() to create its own device with a
> dma_mask set up, and use that device for the DMA API, but
> not actually match the device to a driver.

I'll leave it to Geert to decide whether he prefers setting up a 
platform device in arch/m68k/mvme147/config.c or use the shortcut. I've 
used platform_device_register_simple() in a few other drivers so don't 
mind that much.

Cheers,

	Michael

>
>> Only compile tested so far, and hardware testing might be hard to do.
>> I'd appreciate someone giving this a thorough review.
>
> Looks all good to me.
>
>         Arnd
>

Re: [PATCH v1 0/3] Converting m68k WD33C93 drivers to DMA API

[Arnd Bergmann]

On Wed, Jun 29, 2022 at 9:36 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> Am 29.06.2022 um 18:19 schrieb Arnd Bergmann:
> > On Wed, Jun 29, 2022 at 3:16 AM Michael Schmitz <schmitzmic@gmail.com> wrote:
> >>
> >> V1 of a patch series to convert m68k Amiga WD33C93 drivers to the
> >> DMA API.
> >>
> >> This series was precipitated by Arnd removing CONFIG_VIRT_TO_BUS. The
> >> m68k WD33C93 still used virt_to_bus to convert virtual addresses to
> >> physical addresses suitable for the DMA engines (note m68k does not
> >> have an IOMMU and uses a direct mapping for DMA addresses).
> >>
> >> Arnd suggested to use dma_map_single() to set up dma mappings instead
> >> of open-coding much the same in every driver dma_setup() function.
> >>
> >> It appears that m68k (MMU, except for coldfire) will set up pages for
> >> DMA transfers as non-cacheable, thus obviating the need for explicit
> >> cache management.
> >
> > To clarify, the dma-mapping API has two ways of dealing with this:
> >
> > - the streaming API (dma_map/unmap_...) uses explicit cache flushes
> >
> > - the coherent API (dma_alloc_coherent etc) uses page protections
> >   to prevent caching.
> >
> > These three drivers use the streaming API because they operate on
> > data passed in from the outside, so the non-cacheable protection bits
> > are not used here.
>
> I had feared you'd say something along these lines ...
>
> Now that throws up a possible problem for the gvp11 driver: due to the
> need to first map an allocated chunk, then possibly discard that and try
> another allocation strategy, copying of data to the bounce buffer is
> deferred until after the final mapping has been successful. This means
> for writes, we have done the cache flushing before we have actually
> written any data to the buffer.
>
> I don't think it is safe to omit the explicit cache flush for writes in
> this case.

I think it's fine as long as you do things in the correct order: the
copy into the bounce buffer has to be done before the
dma_map_single() here, and conversely, the copy out of the
bounce buffer must happen after the dma_unmap_single().

Regarding the amiga_chip_alloc(), I don't know what this means
for caching. If chip memory is cache-coherent (either uncached
or by snooping), then there should not be any
dma_map()/dma_unmap() for that case, but instead the
amiga_chip_alloc() function should return both the pointer
and the dma_addr_t token.

         Arnd

Re: [PATCH v1 0/3] Converting m68k WD33C93 drivers to DMA API

[Geert Uytterhoeven]

Hi Arnd,

On Wed, Jun 29, 2022 at 10:21 AM Arnd Bergmann <arnd@kernel.org> wrote:
> Regarding the amiga_chip_alloc(), I don't know what this means
> for caching. If chip memory is cache-coherent (either uncached
> or by snooping), then there should not be any
> dma_map()/dma_unmap() for that case, but instead the
> amiga_chip_alloc() function should return both the pointer
> and the dma_addr_t token.

Chip RAM is mapped uncached.
Amifb remaps it using ioremap_wt() to get a write-through frame buffer.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 0/3] Converting m68k WD33C93 drivers to DMA API

[Arnd Bergmann]

On Wed, Jun 29, 2022 at 5:48 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> On Wed, Jun 29, 2022 at 10:21 AM Arnd Bergmann <arnd@kernel.org> wrote:
> > Regarding the amiga_chip_alloc(), I don't know what this means
> > for caching. If chip memory is cache-coherent (either uncached
> > or by snooping), then there should not be any
> > dma_map()/dma_unmap() for that case, but instead the
> > amiga_chip_alloc() function should return both the pointer
> > and the dma_addr_t token.
>
> Chip RAM is mapped uncached.
> Amifb remaps it using ioremap_wt() to get a write-through frame buffer.

Ok, so in this case the driver should skip the dma_map_single() cache
management and instead keep converting the chipram address to
a bus address directly. While the driver does an extra cache
flush on the uncached address today, it's clearly not needed and there
is probably a performance benefit to not doing it.

For simplicity, the normal bounce buffer could similarly use
dma_alloc_coherent(), which will also result in an uncached
buffer. If I read this correctly, this will also ensure that the buffer
is within the DMA mask, so if ZONE_DMA is larger than the mask,
it just returns NULL and you have to fall back to the chipram
page, rather than checking the address and freeing the buffer.

       Arnd

Re: [PATCH v1 0/3] Converting m68k WD33C93 drivers to DMA API

[Michael Schmitz]

Hi Arnd,

Am 30.06.2022 um 04:44 schrieb Arnd Bergmann:
> For simplicity, the normal bounce buffer could similarly use
> dma_alloc_coherent(), which will also result in an uncached
> buffer. If I read this correctly, this will also ensure that the buffer

No sure we can rule out calling dma_setup() in interrupt context.

> is within the DMA mask, so if ZONE_DMA is larger than the mask,
> it just returns NULL and you have to fall back to the chipram
> page, rather than checking the address and freeing the buffer.

Still need to check what ZONE_DMA is set to on Amiga.

Cheers,
	
	Michael

>
>        Arnd
>

Re: [PATCH v1 0/3] Converting m68k WD33C93 drivers to DMA API

[Michael Schmitz]

Hi Arnd,

On 29/06/22 20:20, Arnd Bergmann wrote:
>
>>> To clarify, the dma-mapping API has two ways of dealing with this:
>>>
>>> - the streaming API (dma_map/unmap_...) uses explicit cache flushes
>>>
>>> - the coherent API (dma_alloc_coherent etc) uses page protections
>>>    to prevent caching.
>>>
>>> These three drivers use the streaming API because they operate on
>>> data passed in from the outside, so the non-cacheable protection bits
>>> are not used here.
>> I had feared you'd say something along these lines ...
>>
>> Now that throws up a possible problem for the gvp11 driver: due to the
>> need to first map an allocated chunk, then possibly discard that and try
>> another allocation strategy, copying of data to the bounce buffer is
>> deferred until after the final mapping has been successful. This means
>> for writes, we have done the cache flushing before we have actually
>> written any data to the buffer.
>>
>> I don't think it is safe to omit the explicit cache flush for writes in
>> this case.
> I think it's fine as long as you do things in the correct order: the
> copy into the bounce buffer has to be done before the
> dma_map_single() here, and conversely, the copy out of the
> bounce buffer must happen after the dma_unmap_single().

Ah - I had missed the latter (due to dma_setup previously doing all 
cache management, and I had expected dma_map_single to do the same, i.e. 
invalidate the affected cache lines at time of mapping). Will fix.

The former is possible to do, but may incur an extra memcpy on gvp11 
(filling a bounce buffer that we may then discard because 
dma_map_single() returns a DMA handle outside our DMA range). The driver 
does switch to only allocating bounce buffers from chip RAM once a 
kmalloc allocation failed to yield DMA-able RAM, so the performance 
impact ought to be minimal.

> Regarding the amiga_chip_alloc(), I don't know what this means
> for caching. If chip memory is cache-coherent (either uncached
> or by snooping), then there should not be any
> dma_map()/dma_unmap() for that case, but instead the
> amiga_chip_alloc() function should return both the pointer
> and the dma_addr_t token.

amiga_chip_alloc() is used in many places around the kernel, I'd rather 
not change all that (or more precisely. I'd rather Geert does change 
amiga_chip_alloc() if need be).

I'll drop use of dma_map_single() on chip RAM and will rely on that 
having been mapped non-cacheable.

Cheers,

     Michael

>
>           Arnd