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@xxxxxxxxxx>
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@xxxxxxxxxxxx)
+ *
+ * Copyright (C) 2011,2018 Michael Schmitz (schmitz@xxxxxxxxxx) 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
--
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
