Hi Paolo, On Mon, 24 Mar 2025 at 13:16, Paolo Pisati <p.pisati@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxxx 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
