Hi Michael,
On Mon, Mar 12, 2018 at 8:26 AM, Michael Schmitz <schmitzmic@xxxxxxxxx> wrote:
> From: Michael Schmitz <schmitz@xxxxxxxxxx>
>
> New combined SCSI driver for all ESP based Zorro SCSI boards for
> m68k Amiga.
>
> 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).
>
> New Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
> driver included in this patch.
>
> 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.
>
> Signed-off-by: Michael Schmitz <schmitzmic@xxxxxxxxx>
>
> ---
>
> Changes since v1:
[...]
Thanks for the update!
A few more comments below, mostly stylistic / practice.
> --- /dev/null
> +++ b/drivers/scsi/zorro_esp.c
> +struct zorro_driver_data {
> + const char *name;
> + unsigned long offset;
> + unsigned long dma_offset;
> + int absolute; /* offset is absolute address */
> + int scsi_option;
> +};
> +
> +static struct zorro_driver_data cyberstormI_data = {
const
> +static struct zorro_device_id zorro_esp_zorro_tbl[] = {
const
> + {
> + .id = ZORRO_PROD_PHASE5_BLIZZARD_1220_CYBERSTORM,
> + .driver_data = (unsigned long)&cyberstormI_data,
> + },
> + {
> + .id = ZORRO_PROD_PHASE5_CYBERSTORM_MK_II,
> + .driver_data = (unsigned long)&cyberstormII_data,
> + },
> + {
> + .id = ZORRO_PROD_PHASE5_BLIZZARD_2060,
> + .driver_data = (unsigned long)&blizzard2060_data,
> + },
> + {
> + .id = ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260,
> + .driver_data = (unsigned long)&blizzard1230_data,
> + },
> + {
> + .id = ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
> + .driver_data = (unsigned long)&blizzard1230II_data,
> + },
> + { 0 }
> +};
> +MODULE_DEVICE_TABLE(zorro, zorro_esp_zorro_tbl);
> +
> +/* Blizzard 1230 DMA interface */
> +
> +struct blz1230_dma_registers {
> + volatile unsigned char dma_addr; /* DMA address [0x0000] */
volatile considered harmful.
If you would use proper *{read,write}*() accessors instead of direct
assignments,
you can drop the volatile's here.
> +#define ZORRO_ESP_GET_PRIV(esp) ((struct zorro_esp_priv *) \
> + dev_get_drvdata(esp->dev))
This macro can be dropped, just use "dev_get_drvdata(esp->dev)" directly.
No cast is needed.
> +static int fastlane_esp_irq_pending(struct esp *esp)
> +{
> + struct fastlane_dma_registers *dregs =
> + (struct fastlane_dma_registers *) (esp->dma_regs);
struct fastlane_dma_registers __iomem *dregs = esp->dma_regs;
(and make C=1 will become (more) happy)
> + unsigned char dma_status;
> +
> + dma_status = dregs->cond_reg;
readb().
> +#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)); \
> + }
Please pass "addr" and "fifo" as macro parameters, too, so it's easier for
the reviewer to notice they are used.
> +#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)); \
> + }
Likewise.
> +static int zorro_esp_probe(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;
> +
> + board = zorro_resource_start(z);
> + zdd = (struct zorro_driver_data *)ent->driver_data;
> +
> + pr_info("%s found at address 0x%lx.\n", zdd->name, board);
> +
> + zep = kmalloc(sizeof(*zep), GFP_KERNEL);
> + if (!zep) {
> + pr_err("Can't allocate device private data!\n");
> + return -ENOMEM;
> + }
> +
> + /* let's figure out whether we have a Zorro II or Zorro III board */
> + if ((z->rom.er_Type & ERT_TYPEMASK) == ERT_ZORROIII) {
> + /* note this is a Zorro III board */
> + if (board > 0xffffff)
> + zep->zorro3 = 1;
> + } else
} else {
> + /* Even though most of these boards identify as Zorro II,
> + * they are in fact CPU expansion slot boards and have full
> + * access to all of memory. Fix up DMA bitmask here.
> + */
> + z->dev.coherent_dma_mask = DMA_BIT_MASK(32);
}
> + /* 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;
Please use function pointers in struct zorro_driver_data, so you don't need
a switch() here (except for Fastlane vs. B1230II).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
