Re: [PATCH v3 1/2] ata: pata_cswarp: Add Amiga cslab ata support

Geert Uytterhoeven
2025-03-24T13:22:12Z

Hi Paolo,

On Mon, 24 Mar 2025 at 13:16, Paolo Pisati <p.pisati@gmail.com> wrote:
> Driver for the on-board IDE interface on CS-Lab Warp Expansion Card
> (NOTE that idemode=native has to be set in Warp Configuration)
>
> Signed-off-by: Paolo Pisati <p.pisati@gmail.com>

Thanks for your patch!

> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -1025,6 +1025,17 @@ config PATA_GAYLE
>
> If unsure, say N.
>
> +config PATA_CSWARP
> + tristate "Amiga CS Warp PATA support"
> + depends on M68K && AMIGA

&& ZORRO?

> + help
> + This option enables support for the on-board IDE interface on
> + CS-Lab Warp Expansion Card (NOTE that idemode=native has to be
> + set in Warp Configuration)
> +
> + If unsure, say N.
> +
> +
> config PATA_BUDDHA
> tristate "Buddha/Catweasel/X-Surf PATA support"
> depends on ZORRO

> --- /dev/null
> +++ b/drivers/ata/pata_cswarp.c
> @@ -0,0 +1,186 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Amiga CS Warp PATA controller driver
> + *
> + * Copyright (c) 2024 CS-Lab s.c.
> + * http://www.cs-lab.eu
> + *
> + * Based on pata_gayle.c, pata_buddha.c and warpATA.device:
> + *
> + * Created 2 Jun 2024 by Andrzej Rogozynski
> + */
> +
> +#include <linux/ata.h>
> +#include <linux/blkdev.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/libata.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/zorro.h>
> +#include <scsi/scsi_cmnd.h>
> +#include <scsi/scsi_host.h>
> +
> +#include <asm/amigahw.h>
> +#include <asm/amigaints.h>
> +#include <asm/amigayle.h>
> +#include <asm/setup.h>
> +
> +#define DRV_NAME "pata_cswarp"
> +
> +#define WARP_OFFSET_ATA 0x6000
> +
> +static const struct scsi_host_template pata_cswarp_sht = {
> + ATA_PIO_SHT(DRV_NAME),
> +};
> +
> +static unsigned int pata_cswarp_data_xfer(struct ata_queued_cmd *qc,
> + unsigned char *buf,
> + unsigned int buflen, int rw)

It is a pity ata_port_operations.sff_data_xfer() doesn't take a void *buf,
else the casts below can be dropped.

Until that has changed, I think it is worthwhile to do the cast only once,
by introducing a temporary variable:

u16 *buf16 = (u16 *)buf;

> +{
> + struct ata_device *dev = qc->dev;
> + struct ata_port *ap = dev->link->ap;
> + void __iomem *data_addr = ap->ioaddr.data_addr;

"volatile void __iomem *data_addr ...", so you can drop the casts below.

> + unsigned int words = buflen >> 1;
> +
> + /* Transfer multiple of 2 bytes */
> + if (rw == READ)
> + raw_insw((u16 *)data_addr, (u16 *)buf, words);
> + else
> + raw_outsw((u16 *)data_addr, (u16 *)buf, words);
> +
> + /* Transfer trailing byte, if any. */
> + if (unlikely(buflen & 0x01)) {
> + unsigned char pad[2] = { };
> +
> + /* Point buf to the tail of buffer */
> + buf += buflen - 1;

buf16 += words;

> +
> + if (rw == READ) {
> + raw_insw((u16 *)data_addr, (u16 *)pad, 1);

No need to loop, just use raw_inw() for a single word.

> + *buf = pad[0];
> + } else {
> + pad[0] = *buf;
> + raw_outsw((u16 *)data_addr, (u16 *)pad, 1);

likewise, raw_outw().

> + }
> + words++;
> + }
> +
> + return words << 1;

This can be one more than buflen, when the latter is odd.
Why not just return buflen instead?

> +}

> +static int pata_cswarp_probe(struct zorro_dev *z,
> + const struct zorro_device_id *ent)
> +{
> + static const char board_name[] = "csWarp";
> + struct ata_host *host;
> + struct ata_port *ap;
> + void __iomem *cswarp_ctrl_board, *base;
> + unsigned long board = z->resource.start;
> +
> + dev_info(&z->dev, "%s IDE controller (board: 0x%lx)\n", board_name,
> + board);
> +
> + if (!devm_request_mem_region(&z->dev, board + WARP_OFFSET_ATA, 0x1800,
> + DRV_NAME))
> + return -ENXIO;
> +
> + host = ata_host_alloc(&z->dev, 1);
> + if (!host)
> + return -ENXIO;
> +
> + cswarp_ctrl_board = (void *)board;

ioremap(board + WARP_OFFSET_ATA, ...)?

> +
> + ap = host->ports[0];
> + base = cswarp_ctrl_board + WARP_OFFSET_ATA;
> +
> + ap->ops = &pata_cswarp_ops;
> +
> + ap->pio_mask = ATA_PIO4;
> + ap->flags |= ATA_FLAG_SLAVE_POSS | ATA_FLAG_NO_IORDY |
> + ATA_FLAG_PIO_POLLING;
> +
> + ap->ioaddr.data_addr = base;
> + ap->ioaddr.error_addr = base + 1 * 4;
> + ap->ioaddr.feature_addr = base + 1 * 4;
> + ap->ioaddr.nsect_addr = base + 2 * 4;
> + ap->ioaddr.lbal_addr = base + 3 * 4;
> + ap->ioaddr.lbam_addr = base + 4 * 4;
> + ap->ioaddr.lbah_addr = base + 5 * 4;
> + ap->ioaddr.device_addr = base + 6 * 4;
> + ap->ioaddr.status_addr = base + 7 * 4;
> + ap->ioaddr.command_addr = base + 7 * 4;
> +
> + ap->ioaddr.altstatus_addr = base + (0x1000 | (6UL << 2));
> + ap->ioaddr.ctl_addr = base + (0x1000 | (6UL << 2));
> +
> + ata_port_desc(ap, " cmd 0x%lx ctl 0x%lx", (unsigned long)base,
> + (unsigned long)ap->ioaddr.ctl_addr);

These two printed addresses are virtual addresses.
However, due to the use of a cast instead of ioremap(), they are identical.
Still, I think it is better to print board + WARP_OFFSET_ATA and
board + WARP_OFFSET_ATA + 0x1000 | (6UL << 2) instead.

> +
> + return ata_host_activate(host, 0, NULL,
> + IRQF_SHARED, &pata_cswarp_sht);
> +}

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