Re: [PATCH 2/3] amiga: cslab ata support

Damien Le Moal
2025-03-22T13:41:47Z

On 2025/03/21 11:14, Paolo Pisati 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>

Please change the commit title to something like:

ata: pata_cswarp: Add Amiga cslab ata support

> ---
> drivers/ata/Kconfig | 12 ++
> drivers/ata/Makefile | 1 +
> drivers/ata/pata_cswarp.c | 210 +++++++++++++++++++++++++++++++++
> drivers/zorro/zorro.ids | 5 +
> include/uapi/linux/zorro_ids.h | 6 +
> 5 files changed, 234 insertions(+)
> create mode 100644 drivers/ata/pata_cswarp.c
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index e00536b49552..9ec4d3a7ec1e 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -1025,6 +1025,18 @@ config PATA_GAYLE
>
> If unsure, say N.
>
> +config PATA_CSWARP
> + tristate "Amiga CS Warp PATA support"
> + depends on M68K && AMIGA
> + 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)

Looks like the lines are short. Please use up to 80 chars per line here.

> +
> + If unsure, say N.
> +
> +
> config PATA_BUDDHA
> tristate "Buddha/Catweasel/X-Surf PATA support"
> depends on ZORRO
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index 20e6645ab737..7b9b87ebacea 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -98,6 +98,7 @@ obj-$(CONFIG_PATA_WINBOND) += pata_sl82c105.o
> obj-$(CONFIG_PATA_CMD640_PCI) += pata_cmd640.o
> obj-$(CONFIG_PATA_FALCON) += pata_falcon.o
> obj-$(CONFIG_PATA_GAYLE) += pata_gayle.o
> +obj-$(CONFIG_PATA_CSWARP) += pata_cswarp.o
> obj-$(CONFIG_PATA_BUDDHA) += pata_buddha.o
> obj-$(CONFIG_PATA_ISAPNP) += pata_isapnp.o
> obj-$(CONFIG_PATA_IXP4XX_CF) += pata_ixp4xx_cf.o
> diff --git a/drivers/ata/pata_cswarp.c b/drivers/ata/pata_cswarp.c
> new file mode 100644
> index 000000000000..29d76b22f1b2
> --- /dev/null
> +++ b/drivers/ata/pata_cswarp.c
> @@ -0,0 +1,210 @@
> +// 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 DRV_VERSION "0.1.0"

There is no need for a driver version. The driver comes with the kernel, it is
not external.

> +
> +#define WARP_OFFSET_ATA 0x6000
> +#define REV16(x) ((uint16_t)((x << 8) | (x >> 8)))

Very generic macrom name... And that is just a byte swap. We have functions for
that already, so why define this ?

> +
> +static const struct scsi_host_template pata_cswarp_sht = {
> + ATA_PIO_SHT(DRV_NAME),
> +};
> +
> +/* FIXME: is this needed? */

If you need to fix it, then please do it. Otherwise, remove this comment please.

> +static unsigned int pata_cswarp_data_xfer(struct ata_queued_cmd *qc,
> + unsigned char *buf,
> + unsigned int buflen, int rw)
> +{
> + struct ata_device *dev = qc->dev;
> + struct ata_port *ap = dev->link->ap;
> + void __iomem *data_addr = ap->ioaddr.data_addr;
> + 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;
> +
> + if (rw == READ) {
> + raw_insw((u16 *)data_addr, (u16 *)pad, 1);
> + *buf = pad[0];
> + } else {
> + pad[0] = *buf;
> + raw_outsw((u16 *)data_addr, (u16 *)pad, 1);
> + }
> + words++;
> + }
> +
> + return words << 1;
> +}
> +
> +/*
> + * Provide our own set_mode() as we don't want to change anything that has
> + * already been configured..
> + */
> +static int pata_cswarp_set_mode(struct ata_link *link,
> + struct ata_device **unused)
> +{
> + struct ata_device *dev;
> +
> + ata_for_each_dev(dev, link, ENABLED) {
> + /* We don't really care */
> + dev->pio_mode = dev->xfer_mode = XFER_PIO_0;
> + dev->xfer_shift = ATA_SHIFT_PIO;
> + dev->flags |= ATA_DFLAG_PIO;
> + ata_dev_info(dev, "configured for PIO\n");
> + }
> + return 0;
> +}
> +
> +static struct ata_port_operations pata_cswarp_ops = {
> + .inherits = &ata_sff_port_ops,
> + .sff_data_xfer = pata_cswarp_data_xfer,
> + .cable_detect = ata_cable_unknown,
> + .set_mode = pata_cswarp_set_mode,
> +};
> +
> +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;
> + void __iomem *cswarp_ctrl_board;
> + unsigned long board;
> +
> + board = z->resource.start;

Please fold this above with the declaration:

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;
> +
> + /* allocate host */
> + host = ata_host_alloc(&z->dev, 1);
> + if (!host)
> + return -ENXIO;
> +
> + cswarp_ctrl_board = (void *)board;
> +
> + struct ata_port *ap = host->ports[0];
> + void __iomem *base = cswarp_ctrl_board + WARP_OFFSET_ATA;

Please do not mix code and declarations. Move the declarations at the top of the
functions. (yes, C allows this, but we do not use this style in the kernel).

> +
> + 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));

It would be nice to have macro definitions for all the magic numbers you use for
offsets into base, to document these values.

> +
> + ap->private_data = (void *)0;

= NULL please. And that should not be needed as this is the inital value anyway.

> +
> + ata_port_desc(ap, " cmd 0x%lx ctl 0x%lx", (unsigned long)base,
> + (unsigned long)ap->ioaddr.ctl_addr);
> +
> + ata_host_activate(host, 0, NULL,
> + IRQF_SHARED, &pata_cswarp_sht);
> +
> + return 0;
> +}
> +
> +static void pata_cswarp_remove(struct zorro_dev *z)
> +{
> + struct ata_host *host = dev_get_drvdata(&z->dev);
> +
> + ata_host_detach(host);
> +}
> +
> +static const struct zorro_device_id pata_cswarp_zorro_tbl[] = {
> + { ZORRO_PROD_CSLAB_WARP_CTRL, 0},
> + { 0 }
> +};
> +MODULE_DEVICE_TABLE(zorro, pata_cswarp_zorro_tbl);
> +
> +static struct zorro_driver pata_cswarp_driver = {
> + .name = "pata_cswarp",
> + .id_table = pata_cswarp_zorro_tbl,
> + .probe = pata_cswarp_probe,
> + .remove = pata_cswarp_remove,
> +};
> +
> +/*
> + * We cannot have a modalias for X-Surf boards, as it competes with the
> + * zorro8390 network driver. As a stopgap measure until we have proper
> + * MFD support for this board, we manually attach to it late after Zorro
> + * has enumerated its boards.
> + */
> +static int __init pata_cswarp_late_init(void)
> +{
> + struct zorro_dev *z = NULL;
> +
> + /* Auto-bind to regular boards */
> + zorro_register_driver(&pata_cswarp_driver);
> +
> + /* Manually bind to all boards */
> + while ((z = zorro_find_device(ZORRO_PROD_CSLAB_WARP_CTRL, z))) {
> + static struct zorro_device_id cswarp_ent = {
> + ZORRO_PROD_CSLAB_WARP_CTRL, 0
> + };
> +
> + pata_cswarp_probe(z, &cswarp_ent);
> + }
> + return 0;
> +}
> +late_initcall(pata_cswarp_late_init);
> +
> +MODULE_AUTHOR("Andrzej Rogozynski");
> +MODULE_DESCRIPTION("low-level driver for CSWarp PATA");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(DRV_VERSION);
> diff --git a/drivers/zorro/zorro.ids b/drivers/zorro/zorro.ids
> index 119abea8c6cb..33418af7488a 100644
> --- a/drivers/zorro/zorro.ids
> +++ b/drivers/zorro/zorro.ids
> @@ -400,6 +400,11 @@
> 0100 ISDN Blaster Z2 [ISDN Interface]
> 0200 HyperCom 4 [Multi I/O]
> 0600 HyperCom 4+ [Multi I/O]
> +1400 CSLab
> + 6000 Warp DDR3 Memory
> + 0001 Warp Video Memory
> + 0101 Warp Control Registers
> + 0201 Warp Flash ROM
> 157c Information
> 6400 ISDN Engine I [ISDN Interface]
> 2017 Vortex
> diff --git a/include/uapi/linux/zorro_ids.h b/include/uapi/linux/zorro_ids.h
> index 0be1fb0c3915..5736d2bf0295 100644
> --- a/include/uapi/linux/zorro_ids.h
> +++ b/include/uapi/linux/zorro_ids.h
> @@ -455,6 +455,12 @@
> #define ZORRO_PROD_CSLAB_WARP_CTRL ZORRO_ID(CSLAB, 0x65, 0)
> #define ZORRO_PROD_CSLAB_WARP_XROM ZORRO_ID(CSLAB, 0x66, 1)
>
> +#define ZORRO_MANUF_CSLAB 0x1400
> +#define ZORRO_PROD_CSLAB_WARP_DDR3 ZORRO_ID(CSLAB, 0x3c, 0)
> +#define ZORRO_PROD_CSLAB_WARP_VRAM ZORRO_ID(CSLAB, 0x64, 0)
> +#define ZORRO_PROD_CSLAB_WARP_CTRL ZORRO_ID(CSLAB, 0x65, 0)
> +#define ZORRO_PROD_CSLAB_WARP_XROM ZORRO_ID(CSLAB, 0x66, 1)
> +
> #define ZORRO_MANUF_INFORMATION 0x157C
> #define ZORRO_PROD_INFORMATION_ISDN_ENGINE_I ZORRO_ID(INFORMATION, 0x64, 0)
>

--
Damien Le Moal
Western Digital Research