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@xxxxxxxxxxxxxx> 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@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
