On Sun, 4 Mar 2018, Michael Schmitz wrote:
> From: Michael Schmitz <schm...@debian.org>
>
> New combined SCSI driver for all ESP based Zorro SCSI boards for
> m68k Amiga.
>
Nice work!
Shouldn't both patches be combined into one? The first patch can't be used
without this one.
> Code largely based on board specific parts of the old drivers (blz1230.c,
> blz2060.c, cyberstorm.c, cyberstormII.c, fastlane.c which were removed
> after the 2.6 kernel series for lack of maintenance) with contributions
> by Tuomas Vainikka (TCQ bug tests and workaround) and Finn Thain (TCQ
> bugfix by use of PIO in extended message in transfer).
>
> Use DMA transfers wherever possible, with board-specific DMA set-up
> functions copied from the old driver code. Three byte reselection messages
> do appear to cause DMA timeouts. So wire up a PIO transfer routine for
> these instead.
>
> PIO code taken from mac_esp.c where the reselection timeout issue was
> debugged and fixed first, with the following modifications:
>
> esp_reselect_with_tag explicitly sets esp->cmd_block_dma as target address
> for the message bytes. Fixup to use kernel virtual address esp->cmd_block
> in PIO transfer if DMA address is esp->cmd_block_dma. Use phys_to_virt(addr)
> to determine kernel virtual address in all other cases (this works on m68k).
>
> A 'select with attention and stop' command raises ESP_INTR_FDONE, so
> adjust interrupt mask used to end PIO transfer accordingly.
>
> Signed-off-by: Michael Schmitz <schmitz...@gmail.com>
> ---
> drivers/scsi/zorro_esp.c | 785
> ++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 785 insertions(+), 0 deletions(-)
> create mode 100644 drivers/scsi/zorro_esp.c
>
> diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
> new file mode 100644
> index 0000000..03c4ad2
> --- /dev/null
> +++ b/drivers/scsi/zorro_esp.c
> @@ -0,0 +1,785 @@
> +/* zorro_esp.c: ESP front-end for Amiga ZORRO SCSI systems.
> + *
> + * Copyright (C) 1996 Jesper Skov (js...@cygnus.co.uk)
> + *
> + * Copyright (C) 2011,2018 Michael Schmitz (schm...@debian.org) for
> + * migration to ESP SCSI core
You can blame me for some of this ;-)
> + */
> +/*
> + * ZORRO bus code from:
> + */
> +/*
> + * Detection routine for the NCR53c710 based Amiga SCSI Controllers for
> Linux.
> + * Amiga MacroSystemUS WarpEngine SCSI controller.
> + * Amiga Technologies/DKB A4091 SCSI controller.
> + *
> + * Written 1997 by Alan Hourihane <al...@fairlite.demon.co.uk>
> + * plus modifications of the 53c7xx.c driver to support the Amiga.
> + *
> + * Rewritten to use 53c700.c by Kars de Jong <jo...@linux-m68k.org>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/ratelimit.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/scatterlist.h>
> +#include <linux/delay.h>
> +#include <linux/zorro.h>
> +#include <linux/slab.h>
> +
> +
Seems to be an excess blank line.
> +#include <asm/page.h>
> +#include <asm/pgtable.h>
> +#include <asm/cacheflush.h>
> +#include <asm/amigahw.h>
> +#include <asm/amigaints.h>
> +
> +#include <scsi/scsi_host.h>
> +#include <scsi/scsi_transport_spi.h>
> +#include <scsi/scsi_device.h>
> +#include <scsi/scsi_tcq.h>
> +
> +#include "esp_scsi.h"
> +
> +#define DRV_MODULE_NAME "zorro_esp"
> +#define PFX DRV_MODULE_NAME ": "
This should be pr_fmt(). mac_esp used PFX because it is older than the
pr_*() stuff.
Also, KBUILD_MODNAME might be appropriate here? This idiom is common:
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +MODULE_AUTHOR("Michael Schmitz <schm...@debian.org>");
> +MODULE_DESCRIPTION("Amiga Zorro NCR5C9x (ESP) driver");
> +MODULE_LICENSE("GPL");
> +
> +#define DMA_WRITE 0x80000000
> +
> +static struct zorro_driver_data {
> + const char *name;
> + unsigned long offset;
> + unsigned long dma_offset;
> + int absolute;
> + int zorro3; /* offset is absolute address */
> +} zorro_esp_driver_data[] = {
> + { .name = "CyberStormI", .offset = 0xf400, .dma_offset = 0xf800,
> + .absolute = 0, .zorro3 = 0 },
> + { .name = "CyberStormII", .offset = 0x1ff03, .dma_offset = 0x1ff43,
> + .absolute = 0, .zorro3 = 0 },
> + { .name = "Blizzard 2060", .offset = 0x1ff00, .dma_offset = 0x1ffe0,
> + .absolute = 0, .zorro3 = 0 },
> + { .name = "Blizzard 1230", .offset = 0x8000, .dma_offset = 0x10000,
> + .absolute = 0, .zorro3 = 0 },
> + { .name = "Blizzard 1230II", .offset = 0x10000, .dma_offset = 0x10021,
> + .absolute = 0, .zorro3 = 0 },
> + { .name = "Fastlane", .offset = 0x1000001, .dma_offset = 0x1000041,
> + .absolute = 0, .zorro3 = 1 },
> + { 0 }
> +};
> +
> +static struct zorro_device_id zorro_esp_zorro_tbl[] = {
> + {
> + .id = ZORRO_PROD_PHASE5_BLIZZARD_1220_CYBERSTORM,
> + .driver_data = (unsigned long)&zorro_esp_driver_data[0],
> + },
> + {
> + .id =
> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
> + .driver_data = (unsigned long)&zorro_esp_driver_data[0],
> + },
> + {
> + .id = ZORRO_PROD_PHASE5_CYBERSTORM_MK_II,
> + .driver_data = (unsigned long)&zorro_esp_driver_data[1],
> + },
> + {
> + .id = ZORRO_PROD_PHASE5_BLIZZARD_2060,
> + .driver_data = (unsigned long)&zorro_esp_driver_data[2],
> + },
> + {
> + .id = ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260,
> + .driver_data = (unsigned long)&zorro_esp_driver_data[3],
> + },
> + {
> + .id =
> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
> + .driver_data = (unsigned long)&zorro_esp_driver_data[4],
> + },
> + { 0 }
> +};
> +MODULE_DEVICE_TABLE(zorro, zorro_esp_zorro_tbl);
> +
> +struct blz1230_dma_registers {
> + volatile unsigned char dma_addr; /* DMA address [0x0000] */
> + unsigned char dmapad2[0x7fff];
> + volatile unsigned char dma_latch; /* DMA latch [0x8000] */
> +};
> +
> +struct blz2060_dma_registers {
> + volatile unsigned char dma_led_ctrl; /* DMA led control [0x000] */
> + unsigned char dmapad1[0x0f];
> + volatile unsigned char dma_addr0; /* DMA address (MSB) [0x010] */
> + unsigned char dmapad2[0x03];
> + volatile unsigned char dma_addr1; /* DMA address [0x014] */
> + unsigned char dmapad3[0x03];
> + volatile unsigned char dma_addr2; /* DMA address [0x018] */
> + unsigned char dmapad4[0x03];
> + volatile unsigned char dma_addr3; /* DMA address (LSB) [0x01c] */
> +};
> +
> +/* DMA control bits */
> +#define BLZ2060_DMA_LED 0x02 /* HD led control 1 =
> off */
> +
> +struct cyber_dma_registers {
> + volatile unsigned char dma_addr0; /* DMA address (MSB) [0x000] */
> + unsigned char dmapad1[1];
> + volatile unsigned char dma_addr1; /* DMA address [0x002] */
> + unsigned char dmapad2[1];
> + volatile unsigned char dma_addr2; /* DMA address [0x004] */
> + unsigned char dmapad3[1];
> + volatile unsigned char dma_addr3; /* DMA address (LSB) [0x006] */
> + unsigned char dmapad4[0x3fb];
> + volatile unsigned char cond_reg; /* DMA cond (ro) [0x402] */
> +#define ctrl_reg cond_reg /* DMA control (wo) [0x402] */
> +};
> +
> +/* DMA control bits */
> +#define CYBER_DMA_LED 0x80 /* HD led control 1 = on */
> +#define CYBER_DMA_WRITE 0x40 /* DMA direction. 1 = write */
> +#define CYBER_DMA_Z3 0x20 /* 16 (Z2) or 32 (CHIP/Z3) bit DMA
> transfer */
> +
> +/* DMA status bits */
> +#define CYBER_DMA_HNDL_INTR 0x80 /* DMA IRQ pending? */
> +
> +/* The bits below appears to be Phase5 Debug bits only; they were not
> + * described by Phase5 so using them may seem a bit stupid...
> + */
> +#define CYBER_HOST_ID 0x02 /* If set, host ID should be 7, otherwise
> + * it should be 6.
> + */
> +#define CYBER_SLOW_CABLE 0x08 /* If *not* set, assume SLOW_CABLE */
> +
> +static unsigned char ctrl_data; /* Keep backup of the stuff written
> + * to ctrl_reg. Always write a copy
> + * to this register when writing to
> + * the hardware register!
> + */
> +
> +/*
> + * private data used for PIO
> + */
> +struct zorro_esp_priv {
> + struct esp *esp;
You don't need this backpointer -- every time you use the zorro_esp_priv
struct, you already have the esp pointer.
> + int error;
> +} zorro_esp_private_data[8];
> +
Many of these 8 structs probably aren't going to get used, which seems
wasteful. I'd allocate the private struct dynamically, as mac_esp does.
> +#define ZORRO_ESP_GET_PRIV(esp) ((struct zorro_esp_priv *) \
> + &zorro_esp_private_data[(esp->host->host_no)])
> +
How do you know that host_no won't exceed the array bounds?
Why not use dev_{set,get}_drvdata(esp->dev)? -- much as mac_esp uses
platform_{set,get}_drvdata() here.
> +/*
> + * On all implementations except for the Oktagon, padding between ESP
> + * registers is three bytes.
> + * On Oktagon, it is one byte - use a different accessor there.
> + *
> + * Oktagon currently unsupported!
> + */
> +
> +static void zorro_esp_write8(struct esp *esp, u8 val, unsigned long reg)
> +{
> + writeb(val, esp->regs + (reg * 4UL));
> +}
> +
> +static u8 zorro_esp_read8(struct esp *esp, unsigned long reg)
> +{
> + return readb(esp->regs + (reg * 4UL));
> +}
> +
> +static dma_addr_t zorro_esp_map_single(struct esp *esp, void *buf,
> + size_t sz, int dir)
> +{
> + return dma_map_single(esp->dev, buf, sz, dir);
> +}
> +
> +static int zorro_esp_map_sg(struct esp *esp, struct scatterlist *sg,
> + int num_sg, int dir)
> +{
> + return dma_map_sg(esp->dev, sg, num_sg, dir);
> +}
> +
> +static void zorro_esp_unmap_single(struct esp *esp, dma_addr_t addr,
> + size_t sz, int dir)
> +{
> + dma_unmap_single(esp->dev, addr, sz, dir);
> +}
> +
> +static void zorro_esp_unmap_sg(struct esp *esp, struct scatterlist *sg,
> + int num_sg, int dir)
> +{
> + dma_unmap_sg(esp->dev, sg, num_sg, dir);
> +}
> +
> +static int zorro_esp_irq_pending(struct esp *esp)
> +{
> + /* check ESP status register; DMA has no status reg. */
> +
> + if (zorro_esp_read8(esp, ESP_STATUS) & ESP_STAT_INTR)
> + return 1;
> +
> + return 0;
> +}
> +
> +static int cyber_esp_irq_pending(struct esp *esp)
> +{
> + /* It's important to check the DMA IRQ bit in the correct way! */
> + return ((zorro_esp_read8(esp, ESP_STATUS) & ESP_STAT_INTR) &&
> + ((((struct cyber_dma_registers *)(esp->dma_regs))->cond_reg) &
> + CYBER_DMA_HNDL_INTR));
> + return 0;
> +}
> +
> +static void zorro_esp_reset_dma(struct esp *esp)
> +{
> + /* nothing to do here */
> +}
> +
> +static void zorro_esp_dma_drain(struct esp *esp)
> +{
> + /* nothing to do here */
> +}
> +
> +static void zorro_esp_dma_invalidate(struct esp *esp)
> +{
> + /* nothing to do here */
> +}
> +
> +/*
> + * Programmed IO routines follow.
> + */
> +
> +static inline unsigned int zorro_esp_wait_for_fifo(struct esp *esp)
> +{
> + int i = 500000;
> +
> + do {
> + unsigned int fbytes = zorro_esp_read8(esp, ESP_FFLAGS)
> + & ESP_FF_FBYTES;
> +
> + if (fbytes)
> + return fbytes;
> +
> + udelay(2);
> + } while (--i);
> +
> + pr_err(PFX "FIFO is empty (sreg %02x)\n",
> + zorro_esp_read8(esp, ESP_STATUS));
> + return 0;
> +}
> +
> +static inline int zorro_esp_wait_for_intr(struct esp *esp)
> +{
> + struct zorro_esp_priv *zep = ZORRO_ESP_GET_PRIV(esp);
> + int i = 500000;
> +
> + do {
> + esp->sreg = zorro_esp_read8(esp, ESP_STATUS);
> + if (esp->sreg & ESP_STAT_INTR)
> + return 0;
> +
> + udelay(2);
> + } while (--i);
> +
> + pr_err(PFX "IRQ timeout (sreg %02x)\n", esp->sreg);
> + zep->error = 1;
> + return 1;
> +}
> +
> +#define ZORRO_ESP_PIO_LOOP(operands, reg1) \
> + { \
> + asm volatile ( \
> + "1: moveb " operands "\n" \
> + " subqw #1,%1 \n" \
> + " jbne 1b \n" \
> + : "+a" (addr), "+r" (reg1) \
> + : "a" (fifo)); \
> + }
> +
> +#define ZORRO_ESP_PIO_FILL(operands, reg1) \
> + { \
> + asm volatile ( \
> + " moveb " operands "\n" \
> + " moveb " operands "\n" \
> + " moveb " operands "\n" \
> + " moveb " operands "\n" \
> + " moveb " operands "\n" \
> + " moveb " operands "\n" \
> + " moveb " operands "\n" \
> + " moveb " operands "\n" \
> + " moveb " operands "\n" \
> + " moveb " operands "\n" \
> + " moveb " operands "\n" \
> + " moveb " operands "\n" \
> + " moveb " operands "\n" \
> + " moveb " operands "\n" \
> + " moveb " operands "\n" \
> + " moveb " operands "\n" \
> + " subqw #8,%1 \n" \
> + " subqw #8,%1 \n" \
> + : "+a" (addr), "+r" (reg1) \
> + : "a" (fifo)); \
> + }
> +
> +#define ZORRO_ESP_FIFO_SIZE 16
> +
> +static void zorro_esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
> + u32 dma_count, int write, u8 cmd)
> +{
> + struct zorro_esp_priv *zep = ZORRO_ESP_GET_PRIV(esp);
> + u8 __iomem *fifo = esp->regs + ESP_FDATA * 16;
> + u8 phase = esp->sreg & ESP_STAT_PMASK;
> +
> + cmd &= ~ESP_CMD_DMA;
> + zep->error = 0;
> +
> + /* We are passed DMA addresses i.e. physical addresses, but must use
> + * kernel virtual addresses here, so remap to virtual. This is easy
IMHO "convert" is probably more clear than "remap" here.
> + * enough for the case of residual bytes of an extended message in
> + * transfer - we know the address must be esp->command_block_dma.
> + * In other cases, hope that phys_to_virt() works ...
> + */
> + if (addr == esp->command_block_dma)
> + addr = (u32) esp->command_block;
> + else
> + addr = (u32) phys_to_virt(addr);
> +
> + if (write) {
> + u8 *dst = (u8 *)addr;
> + u8 mask = ~(phase == ESP_MIP ? ESP_INTR_FDONE : ESP_INTR_BSERV);
> +
> + scsi_esp_cmd(esp, cmd);
> +
> + while (1) {
> + if (!zorro_esp_wait_for_fifo(esp))
> + break;
> +
> + *dst++ = zorro_esp_read8(esp, ESP_FDATA);
> + --esp_count;
> +
> + if (!esp_count)
> + break;
> +
> + if (zorro_esp_wait_for_intr(esp))
> + break;
> +
> + if ((esp->sreg & ESP_STAT_PMASK) != phase)
> + break;
> +
> + esp->ireg = zorro_esp_read8(esp, ESP_INTRPT);
> + if (esp->ireg & mask) {
> + zep->error = 1;
> + break;
> + }
> +
> + if (phase == ESP_MIP)
> + scsi_esp_cmd(esp, ESP_CMD_MOK);
> +
> + scsi_esp_cmd(esp, ESP_CMD_TI);
> + }
> + } else {
> + scsi_esp_cmd(esp, ESP_CMD_FLUSH);
> +
> + if (esp_count >= ZORRO_ESP_FIFO_SIZE) {
> + ZORRO_ESP_PIO_FILL("%0@+,%2@", esp_count);
> + } else {
> + ZORRO_ESP_PIO_LOOP("%0@+,%2@", esp_count);
> + }
> +
> + scsi_esp_cmd(esp, cmd);
> +
> + while (esp_count) {
> + unsigned int n;
> +
> + if (zorro_esp_wait_for_intr(esp))
> + break;
> +
> + if ((esp->sreg & ESP_STAT_PMASK) != phase)
> + break;
> +
> + esp->ireg = zorro_esp_read8(esp, ESP_INTRPT);
> + /* This is tricky - ~ESP_INTR_BSERV is the wrong mask
> + * a ESP_CMD_SELAS command which also generates
> + * ESP_INTR_FDONE! Use ~(ESP_INTR_BSERV|ESP_INTR_FDONE)
> + * since ESP_INTR_FDONE is not raised by any other
> + * error condition. Unfortunately, this is still not
> + * sufficient to make the single message byte transfer
> + * of ESP_CMD_SELAS work ...
Is that comment correct? This is the !write branch, that is, read acccess
from memory. But tag bytes move in the other direction for ESP_CMD_SELAS.
I suspect that the mac_esp PIO code should work fine here unchanged. If
so, let's avoid a new variation on that code if that's possible (?)
> + */
> + if (esp->ireg & ~(ESP_INTR_BSERV | ESP_INTR_FDONE)) {
> + zep->error = 1;
> + break;
> + }
> +
> + n = ZORRO_ESP_FIFO_SIZE -
> + (zorro_esp_read8(esp, ESP_FFLAGS) & ESP_FF_FBYTES);
> + if (n > esp_count)
> + n = esp_count;
> +
> + if (n == ZORRO_ESP_FIFO_SIZE) {
> + ZORRO_ESP_PIO_FILL("%0@+,%2@", esp_count);
> + } else {
> + esp_count -= n;
> + ZORRO_ESP_PIO_LOOP("%0@+,%2@", n);
> + }
> +
> + scsi_esp_cmd(esp, ESP_CMD_TI);
> + }
> + }
> +}
> +
> +// Blizzard 1230/60 SCSI-IV DMA
> +
> +static void zorro_esp_send_blz1230_dma_cmd(struct esp *esp, u32 addr,
> + u32 esp_count, u32 dma_count, int write, u8 cmd)
> +{
> + struct blz1230_dma_registers *dregs =
> + (struct blz1230_dma_registers *) (esp->dma_regs);
> + u8 phase = esp->sreg & ESP_STAT_PMASK;
> +
> + if (phase == ESP_MIP) {
> + zorro_esp_send_pio_cmd(esp, addr, esp_count,
> + dma_count, write, cmd);
> + return;
> + }
> +
> + BUG_ON(!(cmd & ESP_CMD_DMA));
WARN_ON is preferred. Please see Johannes Thumshirn's message from last
week,
https://www.spinics.net/lists/linux-scsi/msg117919.html
> +
> + if (write)
> + cache_clear(addr, esp_count);
> + else
> + cache_push(addr, esp_count);
> +
> + addr >>= 1;
> + if (write)
> + addr &= ~(DMA_WRITE);
> + else
> + addr |= DMA_WRITE;
> +
> + dregs->dma_latch = (addr >> 24) & 0xff;
> + dregs->dma_addr = (addr >> 24) & 0xff;
> + dregs->dma_addr = (addr >> 16) & 0xff;
> + dregs->dma_addr = (addr >> 8) & 0xff;
> + dregs->dma_addr = addr & 0xff;
> +
> + scsi_esp_cmd(esp, ESP_CMD_DMA);
> + zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
> + zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
> +// zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
Maybe set esp_ops.dma_length_limit and drop the dead code.
See also mac_esp_dma_length_limit.
> +
> + scsi_esp_cmd(esp, cmd);
> +}
> +
> +// Blizzard 2060 DMA
> +
> +static void zorro_esp_send_blz2060_dma_cmd(struct esp *esp, u32 addr,
> + u32 esp_count, u32 dma_count, int write, u8 cmd)
> +{
> + struct blz2060_dma_registers *dregs =
> + (struct blz2060_dma_registers *) (esp->dma_regs);
> + u8 phase = esp->sreg & ESP_STAT_PMASK;
> +
> + if (phase == ESP_MIP) {
> + zorro_esp_send_pio_cmd(esp, addr, esp_count,
> + dma_count, write, cmd);
> + return;
> + }
> +
> + BUG_ON(!(cmd & ESP_CMD_DMA));
> +
> + if (write)
> + cache_clear(addr, esp_count);
> + else
> + cache_push(addr, esp_count);
> +
> + addr >>= 1;
> + if (write)
> + addr &= ~(DMA_WRITE);
> + else
> + addr |= DMA_WRITE;
> +
> + dregs->dma_addr3 = addr & 0xff;
> + dregs->dma_addr2 = (addr >> 8) & 0xff;
> + dregs->dma_addr1 = (addr >> 16) & 0xff;
> + dregs->dma_addr0 = (addr >> 24) & 0xff;
> +
> + scsi_esp_cmd(esp, ESP_CMD_DMA);
> + zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
> + zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
> +// zorro_esp_write8(esp, (esp_count >> 16) & 0xff, ESP_TCHI);
Same here.
> +
> + scsi_esp_cmd(esp, cmd);
> +}
> +
> +static void zorro_esp_send_cyber_dma_cmd(struct esp *esp, u32 addr,
> + u32 esp_count, u32 dma_count, int write, u8 cmd)
> +{
> + struct cyber_dma_registers *dregs =
> + (struct cyber_dma_registers *) (esp->dma_regs);
> + u8 phase = esp->sreg & ESP_STAT_PMASK;
> +
> + if (phase == ESP_MIP) {
> + zorro_esp_send_pio_cmd(esp, addr, esp_count,
> + dma_count, write, cmd);
> + return;
> + }
> +
> + BUG_ON(!(cmd & ESP_CMD_DMA));
> + zorro_esp_write8(esp, (esp_count >> 0) & 0xff, ESP_TCLOW);
> + zorro_esp_write8(esp, (esp_count >> 8) & 0xff, ESP_TCMED);
> +
> + /* On the Sparc, DMA_ST_WRITE means "move data from device to memory"
> + * so when (write) is true, it actually means READ (from the ESP)!
> + */
It's not a SPARC thing. A "DMA write" is literally a memory access. So a
"SCSI READ" command is always a "write access" when DMA is involved. I
suggest a comment here only needs to remind the reader that the write flag
indicates a DMA receive operation. E.g.
> + if (write) {
/* DMA receive */
> + cache_clear(addr, esp_count);
> + addr &= ~(1);
> + } else {
/* DMA send */
> + cache_push(addr, esp_count);
> + addr |= 1;
> + }
> +
> + dregs->dma_addr0 = (addr >> 24) & 0xff;
> + dregs->dma_addr1 = (addr >> 16) & 0xff;
> + dregs->dma_addr2 = (addr >> 8) & 0xff;
> + dregs->dma_addr3 = addr & 0xff;
> +
> + if (write)
> + ctrl_data &= ~(CYBER_DMA_WRITE);
> + else
> + ctrl_data |= CYBER_DMA_WRITE;
> +
> + ctrl_data &= ~(CYBER_DMA_Z3); /* Z2, do 16 bit DMA */
> +
> + dregs->ctrl_reg = ctrl_data;
> +
> + scsi_esp_cmd(esp, cmd);
> +}
> +
> +static int zorro_esp_dma_error(struct esp *esp)
> +{
> + struct zorro_esp_priv *zep = ZORRO_ESP_GET_PRIV(esp);
> + u8 phase = esp->sreg & ESP_STAT_PMASK;
> +
> + /* if doing PIO, check for error */
> + if (phase == ESP_MIP && zep->error == 1)
> + return 1;
Don't check the bus phase here, the target controls that. Just make sure
zep->error gets set to zero when not using PIO; that's what
mac_esp_send_pdma_cmd() does.
> +
> + /* do nothing - there seems to be no way to check for DMA errors */
> + return 0;
> +}
> +
> +static struct esp_driver_ops zorro_esp_ops = {
> + .esp_write8 = zorro_esp_write8,
> + .esp_read8 = zorro_esp_read8,
> + .map_single = zorro_esp_map_single,
> + .map_sg = zorro_esp_map_sg,
> + .unmap_single = zorro_esp_unmap_single,
> + .unmap_sg = zorro_esp_unmap_sg,
> + .irq_pending = zorro_esp_irq_pending,
> + .reset_dma = zorro_esp_reset_dma,
> + .dma_drain = zorro_esp_dma_drain,
> + .dma_invalidate = zorro_esp_dma_invalidate,
> + .send_dma_cmd = zorro_esp_send_blz2060_dma_cmd,
> + .dma_error = zorro_esp_dma_error,
> +};
> +
> +static int zorro_esp_init_one(struct zorro_dev *z,
> + const struct zorro_device_id *ent)
> +{
> + struct scsi_host_template *tpnt = &scsi_esp_template;
> + struct Scsi_Host *host;
> + struct esp *esp;
> + struct zorro_driver_data *zdd;
> + struct zorro_esp_priv *zep;
> + unsigned long board, ioaddr, dmaaddr;
> + int err = -ENOMEM;
> +
> + board = zorro_resource_start(z);
> + zdd = (struct zorro_driver_data *)ent->driver_data;
> +
> + pr_info(PFX "%s found at address 0x%lx.\n", zdd->name, board);
> +
> + if (zdd->absolute) {
> + ioaddr = zdd->offset;
> + dmaaddr = zdd->dma_offset;
> + } else {
> + ioaddr = board + zdd->offset;
> + dmaaddr = board + zdd->dma_offset;
> + }
> +
> + if (!zorro_request_device(z, zdd->name)) {
> + pr_err(PFX "cannot reserve region 0x%lx, abort\n",
> + board);
> + return -EBUSY;
> + }
> +
> + /* Fill in the required pieces of hostdata */
> +
> + host = scsi_host_alloc(tpnt, sizeof(struct esp));
> +
> + if (!host) {
> + pr_err(PFX "No host detected; board configuration problem?\n");
> + goto out_free;
> + }
> +
> + host->base = ioaddr;
> + host->this_id = 7;
> +
> + esp = shost_priv(host);
> + esp->host = host;
> + esp->dev = &z->dev;
> +
> + esp->scsi_id = host->this_id;
> + esp->scsi_id_mask = (1 << esp->scsi_id);
> +
> + /* Switch to the correct the DMA routine and clock frequency. */
> + switch (ent->id) {
> + case ZORRO_PROD_PHASE5_BLIZZARD_2060: {
> + zorro_esp_ops.send_dma_cmd = zorro_esp_send_blz2060_dma_cmd;
> + esp->cfreq = 40000000;
This constant is the same in each case; you might want to lift it out of
the switch statement.
> + break;
> + }
> + case ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260: {
> + zorro_esp_ops.send_dma_cmd = zorro_esp_send_blz1230_dma_cmd;
> + esp->cfreq = 40000000;
> + break;
> + }
> + case ZORRO_PROD_PHASE5_BLIZZARD_1220_CYBERSTORM: {
> + zorro_esp_ops.send_dma_cmd = zorro_esp_send_cyber_dma_cmd;
> + zorro_esp_ops.irq_pending = cyber_esp_irq_pending;
> + esp->cfreq = 40000000;
> + break;
> + }
> + default: {
> + /* Oh noes */
> + pr_err(PFX "Unsupported board!\n");
I think you should set err = -ENODEV here.
> + goto fail_free_host;
> + }
Can you remove these pairs of braces?
> + }
> +
> + esp->ops = &zorro_esp_ops;
> +
> + if (ioaddr > 0xffffff)
> + esp->regs = ioremap_nocache(ioaddr, 0x20);
> + else
> + esp->regs = (void __iomem *)ZTWO_VADDR(ioaddr);
> +
> + if (!esp->regs)
> + goto fail_free_host;
> +
> + /* Let's check whether a Blizzard 12x0 really has SCSI */
> + if ((ent->id == ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260) ||
> + (ent->id ==
> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060)) {
> + zorro_esp_write8(esp, (ESP_CONFIG1_PENABLE | 7), ESP_CFG1);
> + if (zorro_esp_read8(esp, ESP_CFG1) != (ESP_CONFIG1_PENABLE|7))
> + goto fail_free_host;
goto fail_unmap_regs?
> + }
> +
> + if (ioaddr > 0xffffff) {
> + // I guess the Fastlane Z3 will be the only one to catch this?
> + // Here's a placeholder for now...
> + if (ent->id == ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260)
> + esp->dma_regs = ioremap_nocache(dmaaddr,
> + sizeof(struct blz1230_dma_registers));
> + } else
> + esp->dma_regs = (void __iomem *)ZTWO_VADDR(dmaaddr);
> +
> + if (!esp->dma_regs)
> + goto fail_unmap_regs;
> +
> + esp->command_block = dma_alloc_coherent(esp->dev, 16,
> + &esp->command_block_dma,
> + GFP_KERNEL);
> +
> + if (!esp->command_block)
> + goto fail_unmap_dma_regs;
> +
> + host->irq = IRQ_AMIGA_PORTS;
> + err = request_irq(host->irq, scsi_esp_intr, IRQF_SHARED,
> + "Amiga Zorro ESP", esp);
> + if (err < 0)
> + goto fail_free_command_block;
> +
> + /* register the chip */
> + err = scsi_esp_register(esp, &z->dev);
> +
> + if (err)
> + goto fail_free_irq;
> +
> + zorro_set_drvdata(z, host);
> +
> + zep = ZORRO_ESP_GET_PRIV(esp);
> + zep->esp = esp;
> +
> + return 0;
> +
> +fail_free_irq:
> + free_irq(host->irq, esp);
> +
> +fail_free_command_block:
> + dma_free_coherent(esp->dev, 16,
> + esp->command_block,
> + esp->command_block_dma);
> +
> +fail_unmap_dma_regs:
> + if (ioaddr > 0xffffff)
> + iounmap(esp->dma_regs);
I think you need to test for ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260 here?
> +
> +fail_unmap_regs:
> + if (ioaddr > 0xffffff)
> + iounmap(esp->regs);
> +
> +fail_free_host:
> + scsi_host_put(host);
> +
> +out_free:
> + zorro_release_device(z);
> +
> + return -ENODEV;
return err?
> +}
> +
> +static void zorro_esp_remove_one(struct zorro_dev *z)
> +{
> + struct Scsi_Host *host = zorro_get_drvdata(z);
> + struct esp *esp = shost_priv(host);
> +
> + scsi_esp_unregister(esp);
> +
> + /* Disable interrupts. Perhaps use disable_irq instead ... */
> +
> + free_irq(host->irq, esp);
> + dma_free_coherent(esp->dev, 16,
> + esp->command_block,
> + esp->command_block_dma);
> +
> + if (host->base > 0xffffff) {
> + iounmap(esp->dma_regs);
Do you need to test for ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260 first?
> + iounmap(esp->regs);
> + }
Too much indentation here.
> +
> + scsi_host_put(host);
> +
> + zorro_release_device(z);
> +}
> +
> +static struct zorro_driver zorro_esp_driver = {
> + .name = "zorro_esp-scsi",
Maybe DRV_MODULE_NAME or KBUILD_MODNAME would be more appropriate here?
> + .id_table = zorro_esp_zorro_tbl,
> + .probe = zorro_esp_init_one,
> + .remove = zorro_esp_remove_one,
> +};
> +
> +static int __init zorro_esp_scsi_init(void)
> +{
> + return zorro_register_driver(&zorro_esp_driver);
> +}
> +
> +static void __exit zorro_esp_scsi_exit(void)
> +{
> + zorro_unregister_driver(&zorro_esp_driver);
> +}
> +
> +module_init(zorro_esp_scsi_init);
> +module_exit(zorro_esp_scsi_exit);
>
Thanks.
--
