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
