Re: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Video for Linux]     [Yosemite News]     [Linux S/390]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux