Message ID | 1520121265-18449-3-git-send-email-schmitzmic@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
On Sun, 4 Mar 2018, Michael Schmitz wrote: > From: Michael Schmitz <schmitz@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 <schmitzmic@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 (jskov@cygnus.co.uk) > + * > + * Copyright (C) 2011,2018 Michael Schmitz (schmitz@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 <alanh@fairlite.demon.co.uk> > + * plus modifications of the 53c7xx.c driver to support the Amiga. > + * > + * Rewritten to use 53c700.c by Kars de Jong <jongk@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 <schmitz@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. --
On Sun, 4 Mar 2018, I wrote: > > + } 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. > Nevermind -- I got the direction wrong there. But if your comment is correct and the mask is wrong, that problem should show up in mac_esp too, right? The whole TCQ problem showed up, and was fixed by checking ESP_INTR_FDONE in the Message In transfer. > > + if (esp->ireg & ~(ESP_INTR_BSERV | ESP_INTR_FDONE)) { > > + zep->error = 1; > > + break; > > + } If you really do get ESP_INTR_FDONE instead of ESP_INTR_BSERV for Message Out, wouldn't it be safer (and more consistent) to do, if (esp->ireg & mask) { mep->error = 1; break; } and initialize mask as, u8 mask = ~(phase == ESP_MIP || phase == ESP_MOP ? ESP_INTR_FDONE : ESP_INTR_BSERV); to cover both !write and write? --
Hi Finn, thanks for your review! Am 04.03.2018 um 15:55 schrieb Finn Thain: > On Sun, 4 Mar 2018, Michael Schmitz wrote: > >> From: Michael Schmitz <schmitz@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. I can certainly do that, if that's the preferred way. >> +/* zorro_esp.c: ESP front-end for Amiga ZORRO SCSI systems. >> + * >> + * Copyright (C) 1996 Jesper Skov (jskov@cygnus.co.uk) >> + * >> + * Copyright (C) 2011,2018 Michael Schmitz (schmitz@debian.org) for >> + * migration to ESP SCSI core > > You can blame me for some of this ;-) Oops - did acknowledge you in the commit message but forgot to do it in the code ... or did you mean something else? >> +#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 OK, I'll change the logging to current standard. >> +/* >> + * 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. True - not sure what I was anticipating to use that for anymore. >> + 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. I had tried that, but couldn't get it stored in either the esp or the zorro_device structs. >> +#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. I don't think you can have more than 8 Zorro cards in an Amiga. And I had tried zorro_{set,get}_drvdata() to no avail (the private data pinter is used for something else there). Will try dev_{set,get}_drvdata() now. >> + /* 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. Right. >> + 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 (?) The Mac PIO code did not work unchanged here, that's why I checked the interrupt bit raised and changed the mask. But that was only for one quirky target (CD-ROM, SCSI-1 CCS IIRC) and only when testing the entire driver without DMA. Doesn't happen in DMA mode. You would have seen the error only with such a special target, and probably not in PDMA mode... Regarding your other comment on this code: I'll have to review what phase was present when the select with attention and stop condition occurred, but setting the mask up front according to the bus phase would make a lot of sense. >> + 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 Well, should we ever see this condition trigger (I have never!), I don't think there is a way we can recover from it. We expect a DMA command (DMA i.e. physical address), how can we reliably determine whether the calling code just forgot to set the bit, or really passed us a kernel virtual address? Since I've never seen this trigger, may as well remove the test. > >> + >> + 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. I need to go back to the old driver code and see whether there were any card variants that allow such large transfer sizes. >> + /* 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 Yes, that's from the old driver where the author evidently saw things more from the SCSI controller perspective... No longer needed now. > "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 */ I can do that. >> +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. You mean before sending each DMA command? Couldn't find a way to figure out whether we're been doing DMA or PIO otherwise. >> + /* 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. It is for the currently supported boards - not sure for CyberstormII and Fastlane. Will check again, and move that outside the switch if the same there. > >> + 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. I'm always returning -ENODEV at present which is clearly wrong, so yes. >> + goto fail_free_host; >> + } > > Can you remove these pairs of braces? They're indeed redundant ... copy&paste error. > >> + } >> + >> + 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? My bad ... of course. > >> + } >> + >> + 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? You're correct there. >> + >> +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? Yep. >> +} >> + >> +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? Indeed! >> + iounmap(esp->regs); >> + } > > Too much indentation here. Ouch ... >> + >> + 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? May as well use that. > >> + .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. Thanks for reviewing my sloppy work - I'll respin after testing the changes! Cheers, Michael
On Sun, 4 Mar 2018, Michael Schmitz wrote: > Am 04.03.2018 um 15:55 schrieb Finn Thain: > >> +/* zorro_esp.c: ESP front-end for Amiga ZORRO SCSI systems. > >> + * > >> + * Copyright (C) 1996 Jesper Skov (jskov@cygnus.co.uk) > >> + * > >> + * Copyright (C) 2011,2018 Michael Schmitz (schmitz@debian.org) for > >> + * migration to ESP SCSI core > > > > You can blame me for some of this ;-) > > Oops - did acknowledge you in the commit message but forgot to do it in > the code ... or did you mean something else? > Right. The commit message is sufficient credit for my needs. > >> +#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. > > I don't think you can have more than 8 Zorro cards in an Amiga. The host_no range is not limited to just zorro cards. It's also libata hosts, USB Attached Storage hosts, scsi_debug etc. > > > > 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 (?) > > The Mac PIO code did not work unchanged here, that's why I checked the > interrupt bit raised and changed the mask. But that was only for one > quirky target (CD-ROM, SCSI-1 CCS IIRC) and only when testing the entire > driver without DMA. Doesn't happen in DMA mode. > > You would have seen the error only with such a special target, and > probably not in PDMA mode... > The TCQ issue showed up on the AV Quadras becasue mac_esp doesn't know how to do PDMA or DMA on that hardware, and so it always uses PIO. It sounds like this bug would show up too given the right kind of target. If so, I will need to patch mac_esp too. > Regarding your other comment on this code: I'll have to review what > phase was present when the select with attention and stop condition > occurred, Thanks. That's what I'd prefer to use in mac_esp (if it works). > but setting the mask up front according to the bus phase would make a > lot of sense. > The idea is that ESP_INTR_FDONE is an error when the phase is not appropriate. In the version you sent, the loop permits ESP_INTR_BSERV | ESP_INTR_FDONE regardless of phase. > > WARN_ON is preferred. Please see Johannes Thumshirn's message from last > > week, > > https://www.spinics.net/lists/linux-scsi/msg117919.html > > Well, should we ever see this condition trigger (I have never!), I don't > think there is a way we can recover from it. We expect a DMA command > (DMA i.e. physical address), how can we reliably determine whether the > calling code just forgot to set the bit, or really passed us a kernel > virtual address? > Thinking about this a bit more, BUG_ON may be the right thing to do here. If the core driver will not catch the DMA error, that might lead to data corruption. You'd better check this before switching to WARN_ON. We can't assume that end users will notice the warning in the logs. > >> +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. > > You mean before sending each DMA command? Couldn't find a way to figure > out whether we're been doing DMA or PIO otherwise. > Both mac_esp_send_pdma_cmd() and mac_esp_send_pio_cmd() just initialize this to zero at the outset. Any time that mep->error is set, it means the most recent transfer failed, which I think is what matters here. --
Hi Finn, >> +/* zorro_esp.c: ESP front-end for Amiga ZORRO SCSI systems. >> + * >> + * Copyright (C) 1996 Jesper Skov (jskov@cygnus.co.uk) >> + * >> + * Copyright (C) 2011,2018 Michael Schmitz (schmitz@debian.org) for >> + * migration to ESP SCSI core > > You can blame me for some of this ;-) I'll add a line here pointing out the source for the PIO code, so others will have a chance to keep these in sync if needs be. >> +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? On second thought - no, I don't. the ID check above only determines what Zorro-3 address is ioremapped, but in each case where ioaddr > 0xffffff, something will have been mapped at this point. >> +} >> + >> +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? I can't - ent->id is not available here. But again, if host->base is outside the Zorro-2 space, the DMA register mapping has been done through ioremap() and needs to be undone here. Cheers, Michael
On Mon, 5 Mar 2018, Michael Schmitz wrote: > >> +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? > > On second thought - no, I don't. the ID check above only determines what > Zorro-3 address is ioremapped, but in each case where ioaddr > 0xffffff, > something will have been mapped at this point. > I think you are talking about esp->regs? For esp->dma_regs, the ioremap is conditional on ent->id, but the unmap is not. > >> +} > >> + > >> +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? > > I can't - ent->id is not available here... Maybe store ent->id in the private struct to get around that? --
Hi Finn, >> >> +#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. >> >> I don't think you can have more than 8 Zorro cards in an Amiga. > > The host_no range is not limited to just zorro cards. It's also libata > hosts, USB Attached Storage hosts, scsi_debug etc. Max. libata hosts is one, no USB host adapters I know of, leaves scsi_debug. But I'll get rid of this if at all possible. >> > 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 (?) >> >> The Mac PIO code did not work unchanged here, that's why I checked the >> interrupt bit raised and changed the mask. But that was only for one >> quirky target (CD-ROM, SCSI-1 CCS IIRC) and only when testing the entire >> driver without DMA. Doesn't happen in DMA mode. >> >> You would have seen the error only with such a special target, and >> probably not in PDMA mode... >> > > The TCQ issue showed up on the AV Quadras becasue mac_esp doesn't know how > to do PDMA or DMA on that hardware, and so it always uses PIO. It sounds > like this bug would show up too given the right kind of target. If so, I > will need to patch mac_esp too. It did show up in PIO mode so it's a matter of having the right target to trigger it. dmesg on elgar says: [ 26.960000] scsi host1: esp [ 27.270000] scsi 1:0:1:0: Direct-Access IBMRAID DFHSS2F9337 4I4I PQ: 0 ANSI: 2 [ 27.280000] scsi target1:0:1: Beginning Domain Validation [ 27.360000] scsi target1:0:1: FAST-10 SCSI 10.0 MB/s ST (100 ns, offset 15) [ 27.380000] scsi target1:0:1: Domain Validation skipping write tests [ 27.390000] scsi target1:0:1: Ending Domain Validation [ 27.450000] scsi 1:0:2:0: CD-ROM SONY CD-ROM CDU-561 1.9m PQ: 0 ANSI: 2 CCS [ 27.460000] scsi target1:0:2: Beginning Domain Validation [ 27.580000] scsi target1:0:2: FAST-5 SCSI 4.0 MB/s ST (248 ns, offset 15) [ 27.600000] scsi target1:0:2: Domain Validation skipping write tests [ 27.620000] scsi target1:0:2: Ending Domain Validation [ 27.750000] scsi host1: scsi scan: INQUIRY result too short (5), using 36 To the best of my knowledge, the CD-ROM triggered that error. Looking at the NCR53CF94_96 databook, select with attention and stop is used when multiple message bytes need to be transferred (example: sync negotiation - that's when esp->flags |= ESP_FLAG_DOING_SLOWCMD is used in the ESP core which switches to using this selection mode). One message byte is transferred before the ESP raises both ESP_INTR_FDONE and ESP_INTR_BSERV. From all appearances, the bus will still be in message out phase at that stage. u8 mask = ~(phase == ESP_MIP ? ESP_INTR_FDONE : (phase == ESP_MOP ? ESP_INTR_FDONE|ESP_INTR_BSERV : ESP_INTR_BSERV) ); would set the correct interrupt status mask to avoid detecting this condition as error. Doing sync negotiation is fairly basic - I wonder why you haven't seen this before. Ah - the Mac driver supports sync transfers only when using PDMA, that's why. The description of 'select with attention' also states the same two interrupt bits are set, so maybe the bug is elsewhere. According to the manual, only the first message byte should be loaded into the FIFO when stopping for additional message bytes. We load the entire message instead... > >> Regarding your other comment on this code: I'll have to review what >> phase was present when the select with attention and stop condition >> occurred, > > Thanks. That's what I'd prefer to use in mac_esp (if it works). > >> but setting the mask up front according to the bus phase would make a >> lot of sense. >> > > The idea is that ESP_INTR_FDONE is an error when the phase is not > appropriate. In the version you sent, the loop permits ESP_INTR_BSERV | > ESP_INTR_FDONE regardless of phase. Deciding whether an error exists just based on the phase might be impossible now I come to think of it - the combination of ESP_INTR_FDONE|ESP_INTR_BSERV is only legitimate for selection commands. We might need to look at additional information to figure out whether getting ESP_INTR_FDONE in message out phase was an error. This is getting a little academic for zorro_esp - if a particular board needs to do PIO all the time, we need a way to set the ESP_FLAG_USE_FIFO flag in the core driver instead. Much safer. > >> > WARN_ON is preferred. Please see Johannes Thumshirn's message from last >> > week, >> > https://www.spinics.net/lists/linux-scsi/msg117919.html >> >> Well, should we ever see this condition trigger (I have never!), I don't >> think there is a way we can recover from it. We expect a DMA command >> (DMA i.e. physical address), how can we reliably determine whether the >> calling code just forgot to set the bit, or really passed us a kernel >> virtual address? >> > > Thinking about this a bit more, BUG_ON may be the right thing to do here. > If the core driver will not catch the DMA error, that might lead to data > corruption. You'd better check this before switching to WARN_ON. We can't > assume that end users will notice the warning in the logs. Reading that thread, leaving BUG_ON would make Linus pretty angry. Setting zep->error and letting the core driver deal with it might be safer. > >> >> +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. >> >> You mean before sending each DMA command? Couldn't find a way to figure >> out whether we're been doing DMA or PIO otherwise. >> > > Both mac_esp_send_pdma_cmd() and mac_esp_send_pio_cmd() just initialize > this to zero at the outset. Any time that mep->error is set, it means the > most recent transfer failed, which I think is what matters here. I'm clearing zep->error now when entering the DMA set-up, and set it when PIO failed (or the DMA flag was not set). Thanks again, Michael > > --
Hi Finn, On Mon, Mar 5, 2018 at 2:01 PM, Finn Thain <fthain@telegraphics.com.au> wrote: > On Mon, 5 Mar 2018, Michael Schmitz wrote: > >> >> +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? >> >> On second thought - no, I don't. the ID check above only determines what >> Zorro-3 address is ioremapped, but in each case where ioaddr > 0xffffff, >> something will have been mapped at this point. >> > > I think you are talking about esp->regs? For esp->dma_regs, the ioremap is > conditional on ent->id, but the unmap is not. The details of the ioremap are conditional on the ID, but the fact that the ioremap happens (and hence esp->dma_regs is an ioremapped address) is not. All Zorro-3 boards have to have both their regs and dma_regs remapped. What's confusing is that there is only a single Zorro-3 board currently supported by the driver. Others will be added and I"ll use a switch statement to pick the appropriate address based on the ID. That might make it clearer. > >> >> +} >> >> + >> >> +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? >> >> I can't - ent->id is not available here... > > Maybe store ent->id in the private struct to get around that? Yes, that could be done. I still think it's not needed. Cheers, Michael > > --
On Mon, 5 Mar 2018, Michael Schmitz wrote: > > The TCQ issue showed up on the AV Quadras becasue mac_esp doesn't know > > how to do PDMA or DMA on that hardware, and so it always uses PIO. It > > sounds like this bug would show up too given the right kind of target. > > If so, I will need to patch mac_esp too. > > It did show up in PIO mode so it's a matter of having the right target > to trigger it. dmesg on elgar says: > > [ 26.960000] scsi host1: esp > [ 27.270000] scsi 1:0:1:0: Direct-Access IBMRAID DFHSS2F9337 4I4I PQ: 0 ANSI: 2 > [ 27.280000] scsi target1:0:1: Beginning Domain Validation > [ 27.360000] scsi target1:0:1: FAST-10 SCSI 10.0 MB/s ST (100 ns, offset 15) > [ 27.380000] scsi target1:0:1: Domain Validation skipping write tests > [ 27.390000] scsi target1:0:1: Ending Domain Validation > [ 27.450000] scsi 1:0:2:0: CD-ROM SONY CD-ROM CDU-561 1.9m PQ: 0 ANSI: 2 CCS > [ 27.460000] scsi target1:0:2: Beginning Domain Validation > [ 27.580000] scsi target1:0:2: FAST-5 SCSI 4.0 MB/s ST (248 ns, offset 15) > [ 27.600000] scsi target1:0:2: Domain Validation skipping write tests > [ 27.620000] scsi target1:0:2: Ending Domain Validation > [ 27.750000] scsi host1: scsi scan: INQUIRY result too short (5), using 36 > > To the best of my knowledge, the CD-ROM triggered that error. Looking at > the NCR53CF94_96 databook, select with attention and stop is used when > multiple message bytes need to be transferred (example: sync negotiation > - that's when esp->flags |= ESP_FLAG_DOING_SLOWCMD is used in the ESP > core which switches to using this selection mode). > > One message byte is transferred before the ESP raises both > ESP_INTR_FDONE and ESP_INTR_BSERV. From all appearances, the bus will > still be in message out phase at that stage. > > u8 mask = ~(phase == ESP_MIP ? ESP_INTR_FDONE : (phase == ESP_MOP ? > ESP_INTR_FDONE|ESP_INTR_BSERV : ESP_INTR_BSERV) ); > > would set the correct interrupt status mask to avoid detecting this > condition as error. Well, it is consistent with zorro_esp_send_pio_cmd as far as ESP_MOP is concerned. > Doing sync negotiation is fairly basic - I wonder why you haven't seen > this before. Ah - the Mac driver supports sync transfers only when using > PDMA, that's why. > I suspect that if you try PIO for all transfers, not just Message In, you'll need to drop async support too. But I don't recall why async was a problem back when I was developing mac_esp. It was too long ago. > The description of 'select with attention' also states the same two > interrupt bits are set, so maybe the bug is elsewhere. According to the > manual, only the first message byte should be loaded into the FIFO when > stopping for additional message bytes. We load the entire message > instead... > FDONE is not a problem once all the bytes are in the fifo. The mac_esp algorithm already accomodates that. Which manual is this, BTW? > > > >> Regarding your other comment on this code: I'll have to review what > >> phase was present when the select with attention and stop condition > >> occurred, > > > > Thanks. That's what I'd prefer to use in mac_esp (if it works). > > > >> but setting the mask up front according to the bus phase would make a > >> lot of sense. > >> > > > > The idea is that ESP_INTR_FDONE is an error when the phase is not > > appropriate. In the version you sent, the loop permits ESP_INTR_BSERV > > | ESP_INTR_FDONE regardless of phase. > > Deciding whether an error exists just based on the phase might be > impossible now I come to think of it - the combination of > ESP_INTR_FDONE|ESP_INTR_BSERV is only legitimate for selection commands. Well, I'm not trying to decide whether any error exists, I'm just trying to detect an unexpected interrupt in the middle of ESP_DOP. Hence my suggestion to add a test for ESP_MOP. > We might need to look at additional information to figure out whether > getting ESP_INTR_FDONE in message out phase was an error. > Your algorithm is pretty clear on that point: FDONE is not an error during message out... but I'm beginning to think that it really is an error during message out, except at the end of the transfer. > This is getting a little academic for zorro_esp - if a particular board > needs to do PIO all the time, we need a way to set the ESP_FLAG_USE_FIFO > flag in the core driver instead. Much safer. > Unfortunately, that flag doesn't always do what is needed for PIO. Anyway, this discussion is completely academic when you consider this: if (phase == ESP_MIP) { zorro_esp_send_pio_cmd(esp, addr, esp_count, dma_count, write, cmd); return; } Based on this, you should be able to use the algorithm from mac_esp_send_pio_cmd() unaltered, since your change to the !write branch is irrelevant during Message In phase. My main concern here is to avoid two slightly different copies of that code. > > > > Thinking about this a bit more, BUG_ON may be the right thing to do > > here. If the core driver will not catch the DMA error, that might lead > > to data corruption. You'd better check this before switching to > > WARN_ON. We can't assume that end users will notice the warning in the > > logs. > > Reading that thread, leaving BUG_ON would make Linus pretty angry. Maybe remove it if you think it can't fire. > Setting zep->error and letting the core driver deal with it might be > safer. > True enough ;-) --
On Mon, 5 Mar 2018, Michael Schmitz wrote: > All Zorro-3 boards have to have both their regs and dma_regs remapped. > I see. > What's confusing is that there is only a single Zorro-3 board currently > supported by the driver. Others will be added and I"ll use a switch > statement to pick the appropriate address based on the ID. That might > make it clearer. > Maybe you should add a comment to explain that. > > Maybe store ent->id in the private struct to get around that? > > Yes, that could be done. I still think it's not needed. > I see what you mean. Given your explanation above, you could simply remove the confusing if (ent->id == ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260) conditional. That's because the (ioaddr > 0xffffff) test already takes care of the zorro2/zorro3 distinction, right? That would avoid the confusing ioremap/iounmap asymmetry.
2018-03-04 0:54 GMT+01:00 Michael Schmitz <schmitzmic@gmail.com>: > +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); The ID ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060 is used at index 1 and index 5. I'm guessing only the first one will be detected. Regards, Kars.
Hi Michael, On Sun, Mar 4, 2018 at 12:54 AM, Michael Schmitz <schmitzmic@gmail.com> wrote: > From: Michael Schmitz <schmitz@debian.org> > > New combined SCSI driver for all ESP based Zorro SCSI boards for > m68k Amiga. Thanks for your patch! > --- /dev/null > +++ b/drivers/scsi/zorro_esp.c > @@ -0,0 +1,785 @@ > +static struct zorro_driver_data { > + const char *name; > + unsigned long offset; > + unsigned long dma_offset; > + int absolute; > + int zorro3; /* offset is absolute address */ zorro3 is unused. > +} 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 } I think it's better to not use an array here, but individual structs: static struct zorro_driver_data cyberstorm_data = ... static struct zorro_driver_data cyberstorm2_data = ... ... That makes it easier to review the references from zorro_esp_zorro_tbl[] below. > +}; > + > +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], > + }, [...] > +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! > + */ This should be part of the device's zorro_esp_priv. > +/* > + * private data used for PIO > + */ > +struct zorro_esp_priv { > + struct esp *esp; > + int error; > +} zorro_esp_private_data[8]; Dynamic allocation, please. > +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 > + * 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); To avoid having a need for phys_to_virt(), you should remember the addresses passed to/returned from dma_map_*(). if you assign the address to a different variable with the proper type, you don't need the cast below.... + if (write) { + u8 *dst = (u8 *)addr; ... here. > + } else { The read case doesn't use addr? > +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)); > + > + if (write) > + cache_clear(addr, esp_count); > + else > + cache_push(addr, esp_count); dma_sync_*() > +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; Initialization not needed. > + > + 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; > + 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; Store send_dma_cmd and cfreq in zorro_driver_data? > + } > + default: { > + /* Oh noes */ > + pr_err(PFX "Unsupported board!\n"); > + goto fail_free_host; > + } > + } > + > + 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)) { Add a flag to zorro_driver_data (optional_scsi?) instead of checking for IDs here. 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
On 05.03.2018 03:11, Michael Schmitz wrote: > Hi Finn, > > On Mon, Mar 5, 2018 at 2:01 PM, Finn Thain <fthain@telegraphics.com.au> wrote: >> On Mon, 5 Mar 2018, Michael Schmitz wrote: >> >>>>> +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? >>> On second thought - no, I don't. the ID check above only determines what >>> Zorro-3 address is ioremapped, but in each case where ioaddr > 0xffffff, >>> something will have been mapped at this point. >>> >> I think you are talking about esp->regs? For esp->dma_regs, the ioremap is >> conditional on ent->id, but the unmap is not. > The details of the ioremap are conditional on the ID, but the fact > that the ioremap happens (and hence esp->dma_regs is an ioremapped > address) is not. All Zorro-3 boards have to have both their regs and > dma_regs remapped. > > What's confusing is that there is only a single Zorro-3 board > currently supported by the driver. Others will be added and I"ll use a > switch statement to pick the appropriate address based on the ID. That > might make it clearer. Fastlane might be the only Z3 SCSI board that has the chip. -Tuomas > >>>>> +} >>>>> + >>>>> +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? >>> I can't - ent->id is not available here... >> Maybe store ent->id in the private struct to get around that? > Yes, that could be done. I still think it's not needed. > > Cheers, > > Michael > > >> -- > -- > To unsubscribe from this list: send the line "unsubscribe linux-m68k" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Kars, Index 1 should have been ZORRO_PROD_PHASE5_CYBERSTORM_MK_II, I've corrected that in the meantime. Fastlane / Blizzard 1230_II distinction is something I an not quite sure about - does the probe function get called twice if the device table contains the same ID twice but with different driver_data contents? Cheers, Michael On Mon, Mar 5, 2018 at 9:02 PM, Kars de Jong <jongk@linux-m68k.org> wrote: > 2018-03-04 0:54 GMT+01:00 Michael Schmitz <schmitzmic@gmail.com>: >> +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); > > > The ID ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060 > is used at index 1 and index 5. > I'm guessing only the first one will be detected. > > Regards, > > Kars.
Hi Geert, thanks, will comment on a few points that were not already raised by Finn below. On Tue, Mar 6, 2018 at 12:29 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> +static struct zorro_driver_data { >> + const char *name; >> + unsigned long offset; >> + unsigned long dma_offset; >> + int absolute; >> + int zorro3; /* offset is absolute address */ > > zorro3 is unused. Fastlane support isn't yet included, I expect to use it there. > >> +} 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 } > > I think it's better to not use an array here, but individual structs: > > static struct zorro_driver_data cyberstorm_data = ... > static struct zorro_driver_data cyberstorm2_data = ... > ... > > That makes it easier to review the references from zorro_esp_zorro_tbl[] > below. Might have avoided the error of using &zorro_esp_driver_data[0] twice ... >> +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! >> + */ > > This should be part of the device's zorro_esp_priv. It's only used in the cyber_dma_setup, and I not actually read anywhere else. Might as well be on the stack instead of static... >> +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 >> + * 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); > > To avoid having a need for phys_to_virt(), you should remember the addresses > passed to/returned from dma_map_*(). Interesting - can we be certain that only one mapping is being used at any one time? > > if you assign the address to a different variable with the proper > type, you don't > need the cast below.... > > + if (write) { > + u8 *dst = (u8 *)addr; > > ... here. > >> + } else { > > The read case doesn't use addr? It's hidden in ZORRO_ESP_PIO_LOOP and ZORRO_ESP_PIO_FILL, but it's there. Might be the reason for the u32 type? >> +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)); >> + >> + if (write) >> + cache_clear(addr, esp_count); >> + else >> + cache_push(addr, esp_count); > > dma_sync_*() > >> +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; > > Initialization not needed. The initialized err variable is used when bailing out .. >> + >> + 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; >> + } here. But I can add the err=-NOMEM here. >> + >> + 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; >> + 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; > > Store send_dma_cmd and cfreq in zorro_driver_data? Could do that, yes. >> + } >> + default: { >> + /* Oh noes */ >> + pr_err(PFX "Unsupported board!\n"); >> + goto fail_free_host; >> + } >> + } >> + >> + 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)) { > > Add a flag to zorro_driver_data (optional_scsi?) instead of checking > for IDs here. That's a better way perhaps. Thanks! 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
On Tue, 6 Mar 2018, Michael Schmitz wrote: > >> +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 > >> + * 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); > > > > To avoid having a need for phys_to_virt(), you should remember the > > addresses passed to/returned from dma_map_*(). > > Interesting - can we be certain that only one mapping is being used at > any one time? > I don't know how Geert's suggestion would work in this case. But I think we covered this problem back in December: https://marc.info/?l=linux-m68k&m=151365452606870 (That thread also helps explain the PIO vs. ESP_CMD_SELAS issue.) Is it sufficient to solve just the tag byte/MSG IN problem to get the driver working? (That is, the addr == esp->command_block_dma case.) If so, might it be simpler to address the full PIO problem in a separate patch? (Including the ESP_INTR_FDONE/MSG OUT issue.) --
Hi Finn, Am 06.03.2018 um 16:04 schrieb Finn Thain: > On Tue, 6 Mar 2018, Michael Schmitz wrote: > >>>> +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 >>>> + * 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); >>> >>> To avoid having a need for phys_to_virt(), you should remember the >>> addresses passed to/returned from dma_map_*(). >> >> Interesting - can we be certain that only one mapping is being used at >> any one time? >> > > I don't know how Geert's suggestion would work in this case. But I think I suppose storing the virtual address in the driver private data struct, and looking up that stored virtual address in the PIO transfer function. > we covered this problem back in December: > https://marc.info/?l=linux-m68k&m=151365452606870 > > (That thread also helps explain the PIO vs. ESP_CMD_SELAS issue.) Yes, rereading that thread now suggests we had explored a few options but the issue of autosense commands was making it a little too complicated. > Is it sufficient to solve just the tag byte/MSG IN problem to get the > driver working? (That is, the addr == esp->command_block_dma case.) > > If so, might it be simpler to address the full PIO problem in a separate > patch? (Including the ESP_INTR_FDONE/MSG OUT issue.) The driver works just fine using PIO exclusively for transferring the extended message bytes in message in phase, and doing everything else by DMA. The whole !write branch will never be executed, and I could just omit it entirely for now, or leave it as it was in the Mac driver. Cheers, Michael
Hi Michael, On Tue, Mar 6, 2018 at 2:11 AM, Michael Schmitz <schmitzmic@gmail.com> wrote: > Index 1 should have been ZORRO_PROD_PHASE5_CYBERSTORM_MK_II, I've > corrected that in the meantime. > > Fastlane / Blizzard 1230_II distinction is something I an not quite > sure about - does the probe function get called twice if the device > table contains the same ID twice but with different driver_data > contents? No, the probe function gets called on the first match only. Cfr. drivers/zorro/zorro-driver.c:zorro_device_probe(). 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
Hi Michael, On Tue, Mar 6, 2018 at 2:33 AM, Michael Schmitz <schmitzmic@gmail.com> wrote: > On Tue, Mar 6, 2018 at 12:29 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >>> +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! >>> + */ >> >> This should be part of the device's zorro_esp_priv. > > It's only used in the cyber_dma_setup, and I not actually read > anywhere else. Might as well be on the stack instead of static... OK, I missed that. Yes, local variable is better. >>> +static int zorro_esp_init_one(struct zorro_dev *z, >>> + const struct zorro_device_id *ent) BTW, please call the probe/remove functions zorro_esp_probe() resp. zorro_esp_remove(). >>> +{ >>> + 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; >> >> Initialization not needed. > > The initialized err variable is used when bailing out .. > >>> + >>> + 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; >>> + } > > here. But I can add the err=-NOMEM here. After out_free it returns fixed -ENODEV ;-) Doing "err = -ENOMEM" here, and returning err at the end is better, as it propagates meaningful error codes. 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
On Tue, 6 Mar 2018, Michael Schmitz wrote: > The whole !write branch will never be executed, and I could just omit it > entirely for now, or leave it as it was in the Mac driver. > We could make use of the !write branch in zorro_esp, even if it was only to figure out the SELAS/MSG OUT issue for the benefit of mac_esp. But let's keep them in sync.
Hi Finn, I'll leave the unused !write branch in the original condition. I'd rather keep the address conversion inside the PIO code than duplicating it in each DMA setup routine though. You had asked what manual I had used a while ago: it's http://bitsavers.trailing-edge.com/components/ncr/scsi/53CF94_96-2_Fast_SCSI_Controller_Data_Manual_Apr1993.pdf Cheers, Michael On Tue, Mar 6, 2018 at 9:37 PM, Finn Thain <fthain@telegraphics.com.au> wrote: > On Tue, 6 Mar 2018, Michael Schmitz wrote: > >> The whole !write branch will never be executed, and I could just omit it >> entirely for now, or leave it as it was in the Mac driver. >> > > We could make use of the !write branch in zorro_esp, even if it was only > to figure out the SELAS/MSG OUT issue for the benefit of mac_esp. But > let's keep them in sync. > > -- > >> Cheers, >> >> Michael >>
Hi Geert, On Tue, Mar 6, 2018 at 8:48 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > BTW, please call the probe/remove functions zorro_esp_probe() resp. > zorro_esp_remove(). Fair enough. >>>> + if (!host) { >>>> + pr_err(PFX "No host detected; board configuration problem?\n"); >>>> + goto out_free; >>>> + } >> >> here. But I can add the err=-NOMEM here. > > After out_free it returns fixed -ENODEV ;-) Fixed that in my tree already. But I now have six err=-ENOMEM peppered all over the probe code. Still, if it makes it more readable ... > Doing "err = -ENOMEM" here, and returning err at the end is better, as > it propagates meaningful error codes. Yes, I've belatedly realized that now. The major obstacle now seems to be dynamic allocation of the driver private data and storing a pointer to that in a way that it can be retrieved using just the esp pointer. dev_set_drvdata(esp->dev, zep) causes the module load to crash ... 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
On Wed, 7 Mar 2018, Michael Schmitz wrote: > The major obstacle now seems to be dynamic allocation of the driver > private data and storing a pointer to that in a way that it can be > retrieved using just the esp pointer. dev_set_drvdata(esp->dev, zep) > causes the module load to crash ... I've just noticed that most esp drivers do this: static int esp_foo_probe(struct platform_device *dev) { ... esp->dev = dev; ... } But the esp->dev->dev dereferencing sometimes gets overlooked, resulting in a pointer to a struct platform_device being used where a pointer to a struct device should be used (i.e. dma_*() calls). I will look into fixing this up. sun_esp.c doesn't have this problem, but the other drivers do. I don't think any of that applies to your zorro_esp code because the version you sent does this, esp->dev = &z->dev; which seems fine to me. But it could end up more convenient to use the sun_esp approach and set esp->dev = z. I suspect that the problem with zorro_esp is that sometimes you use the esp->dev->driver_data pointer as the struct Scsi_Host pointer, and at other times you use it for the struct zorro_driver_data pointer. (I think I see now why you put the esp pointer in struct zorro_driver_data.) If you like, email the current version to me or push it to a repo somewhere and I'll take a look at it. --
Hi Finn, Am 07.03.2018 um 13:54 schrieb Finn Thain: > On Wed, 7 Mar 2018, Michael Schmitz wrote: > >> The major obstacle now seems to be dynamic allocation of the driver >> private data and storing a pointer to that in a way that it can be >> retrieved using just the esp pointer. dev_set_drvdata(esp->dev, zep) >> causes the module load to crash ... > > I've just noticed that most esp drivers do this: > > static int esp_foo_probe(struct platform_device *dev) > { > ... > esp->dev = dev; > ... > } > > But the esp->dev->dev dereferencing sometimes gets overlooked, resulting > in a pointer to a struct platform_device being used where a pointer to a > struct device should be used (i.e. dma_*() calls). I will look into fixing > this up. sun_esp.c doesn't have this problem, but the other drivers do. > > I don't think any of that applies to your zorro_esp code because the > version you sent does this, > > esp->dev = &z->dev; > > which seems fine to me. But it could end up more convenient to use the > sun_esp approach and set esp->dev = z. It just adds another dereferencing step in the dma_map functions which we only need to do once here. But sun_esp also does dev_set_drvdata(&op->dev, esp); i.e. not only does it store the struct device pointer in the esp struct (by indirection through the platform device struct), but also the struct esp pointer in struct device. > I suspect that the problem with zorro_esp is that sometimes you use the > esp->dev->driver_data pointer as the struct Scsi_Host pointer, and at > other times you use it for the struct zorro_driver_data pointer. (I think > I see now why you put the esp pointer in struct zorro_driver_data.) The zorro_esp_priv pointer (zorro_esp_driver_data pointer are only used to store board specific config data needed for probe), but yes, you've found my error. I overlooked that dev_set_drvdata() and zorro_set_drvdata() will store their payload in the same struct device instance. Using one for struct zorro_esp_priv and the other for struct Scsi_host is just asking for trouble. Thanks for jogging my memory ... Since there is no other place for me to put driver private data, I need to change the use of that field to point to the current zorro_esp_priv instance, and retrieve the struct esp pointer from there. I can retrieve the host pointer from struct esp so all should be well. This is a little unusual so I better add a few comments to save the next maintainer from unnecessary headache. > If you like, email the current version to me or push it to a repo > somewhere and I'll take a look at it. I'll take you up on that offer another time, but with the use of dev->driver_data fixed, the driver no longer crashes. I shouldn't hack kernel code in a rush... Now on to mangle the rest of the issues raised in the review... Cheers, Michael
Hi Geert, OK, in that case I'll need to work out something similar to the test for optional SCSI function on the Blizzard 1230/1260 to find out what board I have when dealing with the duplicate Fastlane/Blizzard1230II ID. Is the board base address as returned by zorro_resource_start() reliable to distinguish between Zorro II and Zorro III boards? Cheers, Michael Am 06.03.2018 um 20:43 schrieb Geert Uytterhoeven: > Hi Michael, > > On Tue, Mar 6, 2018 at 2:11 AM, Michael Schmitz <schmitzmic@gmail.com> wrote: >> Index 1 should have been ZORRO_PROD_PHASE5_CYBERSTORM_MK_II, I've >> corrected that in the meantime. >> >> Fastlane / Blizzard 1230_II distinction is something I an not quite >> sure about - does the probe function get called twice if the device >> table contains the same ID twice but with different driver_data >> contents? > > No, the probe function gets called on the first match only. > Cfr. drivers/zorro/zorro-driver.c:zorro_device_probe(). > > 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 >
Hi Michael, On Wed, Mar 7, 2018 at 8:55 AM, Michael Schmitz <schmitzmic@gmail.com> wrote: > OK, in that case I'll need to work out something similar to the test for > optional SCSI function on the Blizzard 1230/1260 to find out what board > I have when dealing with the duplicate Fastlane/Blizzard1230II ID. > > Is the board base address as returned by zorro_resource_start() reliable > to distinguish between Zorro II and Zorro III boards? The board base address is assigned by AmigaOS based on the values in the Expansion ROM (mainly ExpansionRom.er_Type) on the board. More specifically, AmigaOS creates struct ConfigDev from struct ExpansionRom. So yes, it must be reliable. > Am 06.03.2018 um 20:43 schrieb Geert Uytterhoeven: >> On Tue, Mar 6, 2018 at 2:11 AM, Michael Schmitz <schmitzmic@gmail.com> wrote: >>> Index 1 should have been ZORRO_PROD_PHASE5_CYBERSTORM_MK_II, I've >>> corrected that in the meantime. >>> >>> Fastlane / Blizzard 1230_II distinction is something I an not quite >>> sure about - does the probe function get called twice if the device >>> table contains the same ID twice but with different driver_data >>> contents? >> >> No, the probe function gets called on the first match only. >> Cfr. drivers/zorro/zorro-driver.c:zorro_device_probe(). 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
Hi Geert, fine, I'll rely on the base address and the z->rom.er_Type value to pick the correct config data. Cheers, Michael On Wed, Mar 7, 2018 at 9:06 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Michael, > > On Wed, Mar 7, 2018 at 8:55 AM, Michael Schmitz <schmitzmic@gmail.com> wrote: >> OK, in that case I'll need to work out something similar to the test for >> optional SCSI function on the Blizzard 1230/1260 to find out what board >> I have when dealing with the duplicate Fastlane/Blizzard1230II ID. >> >> Is the board base address as returned by zorro_resource_start() reliable >> to distinguish between Zorro II and Zorro III boards? > > The board base address is assigned by AmigaOS based on the values in the > Expansion ROM (mainly ExpansionRom.er_Type) on the board. > More specifically, AmigaOS creates struct ConfigDev from struct ExpansionRom. > So yes, it must be reliable. > >> Am 06.03.2018 um 20:43 schrieb Geert Uytterhoeven: >>> On Tue, Mar 6, 2018 at 2:11 AM, Michael Schmitz <schmitzmic@gmail.com> wrote: >>>> Index 1 should have been ZORRO_PROD_PHASE5_CYBERSTORM_MK_II, I've >>>> corrected that in the meantime. >>>> >>>> Fastlane / Blizzard 1230_II distinction is something I an not quite >>>> sure about - does the probe function get called twice if the device >>>> table contains the same ID twice but with different driver_data >>>> contents? >>> >>> No, the probe function gets called on the first match only. >>> Cfr. drivers/zorro/zorro-driver.c:zorro_device_probe(). > > 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
Tuomas, Am 06.03.2018 um 04:31 schrieb Tuomas Vainikka: >>> I think you are talking about esp->regs? For esp->dma_regs, the >>> ioremap is >>> conditional on ent->id, but the unmap is not. >> The details of the ioremap are conditional on the ID, but the fact >> that the ioremap happens (and hence esp->dma_regs is an ioremapped >> address) is not. All Zorro-3 boards have to have both their regs and >> dma_regs remapped. >> >> What's confusing is that there is only a single Zorro-3 board >> currently supported by the driver. Others will be added and I"ll use a >> switch statement to pick the appropriate address based on the ID. That >> might make it clearer. > > Fastlane might be the only Z3 SCSI board that has the chip. Good to know - I've rewritten the probe code to check for the type of board (Z2 or Z3) based on the ROM data, and make the ioremap/iounmap conditional on that. Cheers, Michael > > -Tuomas > >> >>>>>> +} >>>>>> + >>>>>> +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? >>>> I can't - ent->id is not available here... >>> Maybe store ent->id in the private struct to get around that? >> Yes, that could be done. I still think it's not needed. >> >> Cheers, >> >> Michael >> >> >>> -- >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >
Hi Finn, Geert, >>>>> + /* We are passed DMA addresses i.e. physical addresses, but must use >>>>> + * kernel virtual addresses here, so remap to virtual. This is easy >>>>> + * 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); >>>> >>>> To avoid having a need for phys_to_virt(), you should remember the >>>> addresses passed to/returned from dma_map_*(). >>> >>> Interesting - can we be certain that only one mapping is being used at >>> any one time? Tests have revealed that we can't rely on a single mapping active at any one time (it's rare, but I get some messages printed when I log zorro_esp_map_single() being called with a mapping already saved). >>> >> >> I don't know how Geert's suggestion would work in this case. But I think > > I suppose storing the virtual address in the driver private data struct, > and looking up that stored virtual address in the PIO transfer function. > >> we covered this problem back in December: >> https://marc.info/?l=linux-m68k&m=151365452606870 >> >> (That thread also helps explain the PIO vs. ESP_CMD_SELAS issue.) > > Yes, rereading that thread now suggests we had explored a few options > but the issue of autosense commands was making it a little too complicated. We have no need to find out the correct virtual address for a given DMA mapping as long as we only use PIO for message byte transfer. I'll leave finding a proper solution for a rainy day (or rather, a few rainy weeks). Anyone familiar with Auckland will know what that means :-) Cheers, Michael
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 (jskov@cygnus.co.uk) + * + * Copyright (C) 2011,2018 Michael Schmitz (schmitz@debian.org) for + * migration to ESP SCSI core + */ +/* + * 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 <alanh@fairlite.demon.co.uk> + * plus modifications of the 53c7xx.c driver to support the Amiga. + * + * Rewritten to use 53c700.c by Kars de Jong <jongk@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> + + +#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 ": " + +MODULE_AUTHOR("Michael Schmitz <schmitz@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; + int error; +} zorro_esp_private_data[8]; + +#define ZORRO_ESP_GET_PRIV(esp) ((struct zorro_esp_priv *) \ + &zorro_esp_private_data[(esp->host->host_no)]) + +/* + * 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 + * 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 ... + */ + 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)); + + 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); + + 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); + + 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)! + */ + if (write) { + cache_clear(addr, esp_count); + addr &= ~(1); + } else { + 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; + + /* 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; + 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"); + goto fail_free_host; + } + } + + 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; + } + + 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); + +fail_unmap_regs: + if (ioaddr > 0xffffff) + iounmap(esp->regs); + +fail_free_host: + scsi_host_put(host); + +out_free: + zorro_release_device(z); + + return -ENODEV; +} + +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); + iounmap(esp->regs); + } + + scsi_host_put(host); + + zorro_release_device(z); +} + +static struct zorro_driver zorro_esp_driver = { + .name = "zorro_esp-scsi", + .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);