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
