Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-09 Thread Michael Schmitz
Hi Finn, Geert,

> +   /* 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?

Tests have revealed that we can't rely on a single mapping active at any
one time (it's rare, but I get some messages printed when I log
zorro_esp_map_single() being called with a mapping already saved).

>>>
>>
>> I don't know how Geert's suggestion would work in this case. But I think 
> 
> I suppose storing the virtual address in the driver private data struct,
> and looking up that stored virtual address in the PIO transfer function.
> 
>> we covered this problem back in December: 
>> https://marc.info/?l=linux-m68k=151365452606870
>>
>> (That thread also helps explain the PIO vs. ESP_CMD_SELAS issue.)
> 
> Yes, rereading that thread now suggests we had explored a few options
> but the issue of autosense commands was making it a little too complicated.

We have no need to find out the correct virtual address for a given DMA
mapping as long as we only use PIO for message byte transfer. I'll leave
finding a proper solution for a rainy day (or rather, a few rainy
weeks). Anyone familiar with Auckland will know what that means :-)

Cheers,

Michael


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-07 Thread Michael Schmitz
Tuomas,


Am 06.03.2018 um 04:31 schrieb Tuomas Vainikka:
>>> I think you are talking about esp->regs? For esp->dma_regs, the
>>> ioremap is
>>> conditional on ent->id, but the unmap is not.
>> The details of the ioremap are conditional on the ID, but the fact
>> that the ioremap happens (and hence esp->dma_regs is an ioremapped
>> address) is not. All Zorro-3 boards have to have both their regs and
>> dma_regs remapped.
>>
>> What's confusing is that there is only a single Zorro-3 board
>> currently supported by the driver. Others will be added and I"ll use a
>> switch statement to pick the appropriate address based on the ID. That
>> might make it clearer.
> 
> Fastlane might be the only Z3 SCSI board that has the chip.

Good to know - I've rewritten the probe code to check for the type of
board (Z2 or Z3) based on the ROM data, and make the ioremap/iounmap
conditional on that.

Cheers,

Michael


> 
> -Tuomas
> 
>>
>> +}
>> +
>> +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 > 0xff) {
>> + iounmap(esp->dma_regs);
> Do you need to test for ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260 first?
 I can't - ent->id is not available here...
>>> Maybe store ent->id in the private struct to get around that?
>> Yes, that could be done. I still think it's not needed.
>>
>> Cheers,
>>
>>Michael
>>
>>
>>> -- 
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-07 Thread Michael Schmitz
Hi Geert,

fine, I'll rely on the base address and the z->rom.er_Type value to
pick the correct config data.

Cheers,

  Michael


On Wed, Mar 7, 2018 at 9:06 PM, Geert Uytterhoeven  wrote:
> Hi Michael,
>
> On Wed, Mar 7, 2018 at 8:55 AM, Michael Schmitz  wrote:
>> OK, in that case I'll need to work out something similar to the test for
>> optional SCSI function on the Blizzard 1230/1260 to find out what board
>> I have when dealing with the duplicate Fastlane/Blizzard1230II ID.
>>
>> Is the board base address as returned by zorro_resource_start() reliable
>> to distinguish between Zorro II and Zorro III boards?
>
> The board base address is assigned by AmigaOS based on the values in the
> Expansion ROM (mainly ExpansionRom.er_Type) on the board.
> More specifically, AmigaOS creates struct ConfigDev from struct ExpansionRom.
> So yes, it must be reliable.
>
>> Am 06.03.2018 um 20:43 schrieb Geert Uytterhoeven:
>>> On Tue, Mar 6, 2018 at 2:11 AM, Michael Schmitz  
>>> wrote:
 Index 1 should have been ZORRO_PROD_PHASE5_CYBERSTORM_MK_II, I've
 corrected that in the meantime.

 Fastlane / Blizzard 1230_II distinction is something I an not quite
 sure about - does the probe function get called twice if the device
 table contains the same ID twice but with different driver_data
 contents?
>>>
>>> No, the probe function gets called on the first match only.
>>> Cfr. drivers/zorro/zorro-driver.c:zorro_device_probe().
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
>
> 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


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-07 Thread Geert Uytterhoeven
Hi Michael,

On Wed, Mar 7, 2018 at 8:55 AM, Michael Schmitz  wrote:
> OK, in that case I'll need to work out something similar to the test for
> optional SCSI function on the Blizzard 1230/1260 to find out what board
> I have when dealing with the duplicate Fastlane/Blizzard1230II ID.
>
> Is the board base address as returned by zorro_resource_start() reliable
> to distinguish between Zorro II and Zorro III boards?

The board base address is assigned by AmigaOS based on the values in the
Expansion ROM (mainly ExpansionRom.er_Type) on the board.
More specifically, AmigaOS creates struct ConfigDev from struct ExpansionRom.
So yes, it must be reliable.

> Am 06.03.2018 um 20:43 schrieb Geert Uytterhoeven:
>> On Tue, Mar 6, 2018 at 2:11 AM, Michael Schmitz  wrote:
>>> Index 1 should have been ZORRO_PROD_PHASE5_CYBERSTORM_MK_II, I've
>>> corrected that in the meantime.
>>>
>>> Fastlane / Blizzard 1230_II distinction is something I an not quite
>>> sure about - does the probe function get called twice if the device
>>> table contains the same ID twice but with different driver_data
>>> contents?
>>
>> No, the probe function gets called on the first match only.
>> Cfr. drivers/zorro/zorro-driver.c:zorro_device_probe().

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

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


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-06 Thread Michael Schmitz
Hi Geert,

OK, in that case I'll need to work out something similar to the test for
optional SCSI function on the Blizzard 1230/1260 to find out what board
I have when dealing with the duplicate Fastlane/Blizzard1230II ID.

Is the board base address as returned by zorro_resource_start() reliable
to distinguish between Zorro II and Zorro III boards?

Cheers,

Michael

Am 06.03.2018 um 20:43 schrieb Geert Uytterhoeven:
> Hi Michael,
> 
> On Tue, Mar 6, 2018 at 2:11 AM, Michael Schmitz  wrote:
>> Index 1 should have been ZORRO_PROD_PHASE5_CYBERSTORM_MK_II, I've
>> corrected that in the meantime.
>>
>> Fastlane / Blizzard 1230_II distinction is something I an not quite
>> sure about - does the probe function get called twice if the device
>> table contains the same ID twice but with different driver_data
>> contents?
> 
> No, the probe function gets called on the first match only.
> Cfr. drivers/zorro/zorro-driver.c:zorro_device_probe().
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
> 
> 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
> 


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-06 Thread Michael Schmitz
Hi Finn,

Am 07.03.2018 um 13:54 schrieb Finn Thain:
> On Wed, 7 Mar 2018, Michael Schmitz wrote:
> 
>> The major obstacle now seems to be dynamic allocation of the driver 
>> private data and storing a pointer to that in a way that it can be 
>> retrieved using just the esp pointer. dev_set_drvdata(esp->dev, zep) 
>> causes the module load to crash ...
> 
> I've just noticed that most esp drivers do this:
> 
> static int esp_foo_probe(struct platform_device *dev)
> {
>   ...
>   esp->dev = dev;
>   ...
> }
> 
> But the esp->dev->dev dereferencing sometimes gets overlooked, resulting 
> in a pointer to a struct platform_device being used where a pointer to a 
> struct device should be used (i.e. dma_*() calls). I will look into fixing 
> this up. sun_esp.c doesn't have this problem, but the other drivers do.
> 
> I don't think any of that applies to your zorro_esp code because the 
> version you sent does this,
> 
>   esp->dev = >dev;
> 
> which seems fine to me. But it could end up more convenient to use the 
> sun_esp approach and set esp->dev = z.

It just adds another dereferencing step in the dma_map functions which
we only need to do once here.

But sun_esp also does

dev_set_drvdata(>dev, esp);

i.e. not only does it store the struct device pointer in the esp struct
(by indirection through the platform device struct), but also the struct
esp pointer in struct device.

> I suspect that the problem with zorro_esp is that sometimes you use the 
> esp->dev->driver_data pointer as the struct Scsi_Host pointer, and at 
> other times you use it for the struct zorro_driver_data pointer. (I think 
> I see now why you put the esp pointer in struct zorro_driver_data.)

The zorro_esp_priv pointer (zorro_esp_driver_data pointer are only used
to store board specific config data needed for probe), but yes, you've
found my error.

I overlooked that dev_set_drvdata() and zorro_set_drvdata() will store
their payload in the same struct device instance. Using one for struct
zorro_esp_priv and the other for struct Scsi_host is just asking for
trouble. Thanks for jogging my memory ...

Since there is no other place for me to put driver private data, I need
to change the use of that field to point to the current zorro_esp_priv
instance, and retrieve the struct esp pointer from there. I can retrieve
the host pointer from  struct esp so all should be well.

This is a little unusual so I better add a few comments to save the next
maintainer from unnecessary headache.

> If you like, email the current version to me or push it to a repo 
> somewhere and I'll take a look at it.

I'll take you up on that offer another time, but with the use of
dev->driver_data fixed, the driver no longer crashes. I shouldn't
hack kernel code in a rush...

Now on to mangle the rest of the issues raised in the review...

Cheers,

Michael


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-06 Thread Finn Thain
On Wed, 7 Mar 2018, Michael Schmitz wrote:

> The major obstacle now seems to be dynamic allocation of the driver 
> private data and storing a pointer to that in a way that it can be 
> retrieved using just the esp pointer. dev_set_drvdata(esp->dev, zep) 
> causes the module load to crash ...

I've just noticed that most esp drivers do this:

static int esp_foo_probe(struct platform_device *dev)
{
...
esp->dev = dev;
...
}

But the esp->dev->dev dereferencing sometimes gets overlooked, resulting 
in a pointer to a struct platform_device being used where a pointer to a 
struct device should be used (i.e. dma_*() calls). I will look into fixing 
this up. sun_esp.c doesn't have this problem, but the other drivers do.

I don't think any of that applies to your zorro_esp code because the 
version you sent does this,

esp->dev = >dev;

which seems fine to me. But it could end up more convenient to use the 
sun_esp approach and set esp->dev = z.

I suspect that the problem with zorro_esp is that sometimes you use the 
esp->dev->driver_data pointer as the struct Scsi_Host pointer, and at 
other times you use it for the struct zorro_driver_data pointer. (I think 
I see now why you put the esp pointer in struct zorro_driver_data.)

If you like, email the current version to me or push it to a repo 
somewhere and I'll take a look at it.

-- 


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-06 Thread Michael Schmitz
Hi Geert,

On Tue, Mar 6, 2018 at 8:48 PM, Geert Uytterhoeven  wrote:
> BTW, please call the probe/remove functions zorro_esp_probe() resp.
> zorro_esp_remove().

Fair enough.

 +   if (!host) {
 +   pr_err(PFX "No host detected; board configuration 
 problem?\n");
 +   goto out_free;
 +   }
>>
>> here. But I can add the err=-NOMEM here.
>
> After out_free it returns fixed -ENODEV ;-)

Fixed that in my tree already. But I now have six err=-ENOMEM peppered
all over the probe code. Still, if it makes it more readable ...

> Doing "err = -ENOMEM" here, and returning err at the end is better, as
> it propagates meaningful error codes.

Yes, I've belatedly realized that now.

The major obstacle now seems to be dynamic allocation of the driver
private data and storing a pointer to that in a way that it can be
retrieved using just the esp pointer. dev_set_drvdata(esp->dev, zep)
causes the module load to crash ...

Cheers,

  Michael


>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@linux-m68k.org
>
> 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


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-06 Thread Michael Schmitz
Hi Finn,

I'll leave the unused !write branch in the original condition. I'd
rather keep the address conversion inside the PIO code than
duplicating it in each DMA setup routine though.

You had asked what manual I had used a while ago: it's
http://bitsavers.trailing-edge.com/components/ncr/scsi/53CF94_96-2_Fast_SCSI_Controller_Data_Manual_Apr1993.pdf

Cheers,

  Michael


On Tue, Mar 6, 2018 at 9:37 PM, Finn Thain  wrote:
> On Tue, 6 Mar 2018, Michael Schmitz wrote:
>
>> The whole !write branch will never be executed, and I could just omit it
>> entirely for now, or leave it as it was in the Mac driver.
>>
>
> We could make use of the !write branch in zorro_esp, even if it was only
> to figure out the SELAS/MSG OUT issue for the benefit of mac_esp. But
> let's keep them in sync.
>
> --
>
>> Cheers,
>>
>>   Michael
>>


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-06 Thread Finn Thain
On Tue, 6 Mar 2018, Michael Schmitz wrote:

> The whole !write branch will never be executed, and I could just omit it 
> entirely for now, or leave it as it was in the Mac driver.
> 

We could make use of the !write branch in zorro_esp, even if it was only 
to figure out the SELAS/MSG OUT issue for the benefit of mac_esp. But 
let's keep them in sync.

-- 

> Cheers,
> 
>   Michael
> 


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-05 Thread Geert Uytterhoeven
Hi Michael,

On Tue, Mar 6, 2018 at 2:33 AM, Michael Schmitz  wrote:
> On Tue, Mar 6, 2018 at 12:29 AM, Geert Uytterhoeven
>  wrote:

>>> +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...

OK, I missed that. Yes, local variable is better.

>>> +static int zorro_esp_init_one(struct zorro_dev *z,
>>> +  const struct zorro_device_id *ent)

BTW, please call the probe/remove functions zorro_esp_probe() resp.
zorro_esp_remove().

>>> +{
>>> +   struct scsi_host_template *tpnt = _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.

After out_free it returns fixed -ENODEV ;-)

Doing "err = -ENOMEM" here, and returning err at the end is better, as
it propagates meaningful error codes.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

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


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-05 Thread Geert Uytterhoeven
Hi Michael,

On Tue, Mar 6, 2018 at 2:11 AM, Michael Schmitz  wrote:
> Index 1 should have been ZORRO_PROD_PHASE5_CYBERSTORM_MK_II, I've
> corrected that in the meantime.
>
> Fastlane / Blizzard 1230_II distinction is something I an not quite
> sure about - does the probe function get called twice if the device
> table contains the same ID twice but with different driver_data
> contents?

No, the probe function gets called on the first match only.
Cfr. drivers/zorro/zorro-driver.c:zorro_device_probe().

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

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


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-05 Thread Finn Thain
On Tue, 6 Mar 2018, Michael Schmitz wrote:

> >> +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?
> 

I don't know how Geert's suggestion would work in this case. But I think 
we covered this problem back in December: 
https://marc.info/?l=linux-m68k=151365452606870

(That thread also helps explain the PIO vs. ESP_CMD_SELAS issue.)

Is it sufficient to solve just the tag byte/MSG IN problem to get the 
driver working? (That is, the addr == esp->command_block_dma case.)

If so, might it be simpler to address the full PIO problem in a separate 
patch? (Including the ESP_INTR_FDONE/MSG OUT issue.)

-- 


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-05 Thread Michael Schmitz
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
 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 = 0x1,
>> + .absolute = 0, .zorro3 = 0 },
>> +   { .name = "Blizzard 1230II", .offset = 0x1, .dma_offset = 
>> 0x10021,
>> + .absolute = 0, .zorro3 = 0 },
>> +   { .name = "Fastlane", .offset = 0x101, .dma_offset = 0x141,
>> + .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 _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 = _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);
>> +   

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-05 Thread Michael Schmitz
Hi Kars,

Index 1 should have been ZORRO_PROD_PHASE5_CYBERSTORM_MK_II, I've
corrected that in the meantime.

Fastlane / Blizzard 1230_II distinction is something I an not quite
sure about - does the probe function get called twice if the device
table contains the same ID twice but with different driver_data
contents?

Cheers,

  Michael


On Mon, Mar 5, 2018 at 9:02 PM, Kars de Jong  wrote:
> 2018-03-04 0:54 GMT+01:00 Michael Schmitz :
>> +static struct zorro_device_id zorro_esp_zorro_tbl[] = {
>> +   {
>> +   .id = ZORRO_PROD_PHASE5_BLIZZARD_1220_CYBERSTORM,
>> +   .driver_data = (unsigned long)_esp_driver_data[0],
>> +   },
>> +   {
>> +   .id = 
>> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
>> +   .driver_data = (unsigned long)_esp_driver_data[0],
>> +   },
>> +   {
>> +   .id = ZORRO_PROD_PHASE5_CYBERSTORM_MK_II,
>> +   .driver_data = (unsigned long)_esp_driver_data[1],
>> +   },
>> +   {
>> +   .id = ZORRO_PROD_PHASE5_BLIZZARD_2060,
>> +   .driver_data = (unsigned long)_esp_driver_data[2],
>> +   },
>> +   {
>> +   .id = ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260,
>> +   .driver_data = (unsigned long)_esp_driver_data[3],
>> +   },
>> +   {
>> +   .id = 
>> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
>> +   .driver_data = (unsigned long)_esp_driver_data[4],
>> +   },
>> +   { 0 }
>> +};
>> +MODULE_DEVICE_TABLE(zorro, zorro_esp_zorro_tbl);
>
>
> The ID ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060
> is used at index 1 and index 5.
> I'm guessing only the first one will be detected.
>
> Regards,
>
> Kars.


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-05 Thread Tuomas Vainikka

On 05.03.2018 03:11, Michael Schmitz wrote:

Hi Finn,

On Mon, Mar 5, 2018 at 2:01 PM, Finn Thain  wrote:

On Mon, 5 Mar 2018, Michael Schmitz wrote:


+fail_unmap_dma_regs:
+ if (ioaddr > 0xff)
+ iounmap(esp->dma_regs);

I think you need to test for ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260 here?

On second thought - no, I don't. the ID check above only determines what
Zorro-3 address is ioremapped, but in each case where ioaddr > 0xff,
something will have been mapped at this point.


I think you are talking about esp->regs? For esp->dma_regs, the ioremap is
conditional on ent->id, but the unmap is not.

The details of the ioremap are conditional on the ID, but the fact
that the ioremap happens (and hence esp->dma_regs is an ioremapped
address) is not. All Zorro-3 boards have to have both their regs and
dma_regs remapped.

What's confusing is that there is only a single Zorro-3 board
currently supported by the driver. Others will be added and I"ll use a
switch statement to pick the appropriate address based on the ID. That
might make it clearer.


Fastlane might be the only Z3 SCSI board that has the chip.

-Tuomas




+}
+
+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 > 0xff) {
+ iounmap(esp->dma_regs);

Do you need to test for ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260 first?

I can't - ent->id is not available here...

Maybe store ent->id in the private struct to get around that?

Yes, that could be done. I still think it's not needed.

Cheers,

   Michael



--

--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html





Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-05 Thread Geert Uytterhoeven
Hi Michael,

On Sun, Mar 4, 2018 at 12:54 AM, Michael Schmitz  wrote:
> From: Michael Schmitz 
>
> New combined SCSI driver for all ESP based Zorro SCSI boards for
> m68k Amiga.

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/scsi/zorro_esp.c
> @@ -0,0 +1,785 @@

> +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.

> +} 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 = 0x1,
> + .absolute = 0, .zorro3 = 0 },
> +   { .name = "Blizzard 1230II", .offset = 0x1, .dma_offset = 0x10021,
> + .absolute = 0, .zorro3 = 0 },
> +   { .name = "Fastlane", .offset = 0x101, .dma_offset = 0x141,
> + .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.

> +};
> +
> +static struct zorro_device_id zorro_esp_zorro_tbl[] = {
> +   {
> +   .id = ZORRO_PROD_PHASE5_BLIZZARD_1220_CYBERSTORM,
> +   .driver_data = (unsigned long)_esp_driver_data[0],
> +   },

[...]

> +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.

> +/*
> + * private data used for PIO
> + */
> +struct zorro_esp_priv {
> +   struct esp *esp;
> +   int error;
> +} zorro_esp_private_data[8];

Dynamic allocation, please.

> +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_*().

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?

> +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 = _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.

> +
> +   board = zorro_resource_start(z);
> +   zdd = (struct zorro_driver_data *)ent->driver_data;
> +
> +   pr_info(PFX "%s found at address 

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-05 Thread Kars de Jong
2018-03-04 0:54 GMT+01:00 Michael Schmitz :
> +static struct zorro_device_id zorro_esp_zorro_tbl[] = {
> +   {
> +   .id = ZORRO_PROD_PHASE5_BLIZZARD_1220_CYBERSTORM,
> +   .driver_data = (unsigned long)_esp_driver_data[0],
> +   },
> +   {
> +   .id = 
> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
> +   .driver_data = (unsigned long)_esp_driver_data[0],
> +   },
> +   {
> +   .id = ZORRO_PROD_PHASE5_CYBERSTORM_MK_II,
> +   .driver_data = (unsigned long)_esp_driver_data[1],
> +   },
> +   {
> +   .id = ZORRO_PROD_PHASE5_BLIZZARD_2060,
> +   .driver_data = (unsigned long)_esp_driver_data[2],
> +   },
> +   {
> +   .id = ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260,
> +   .driver_data = (unsigned long)_esp_driver_data[3],
> +   },
> +   {
> +   .id = 
> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
> +   .driver_data = (unsigned long)_esp_driver_data[4],
> +   },
> +   { 0 }
> +};
> +MODULE_DEVICE_TABLE(zorro, zorro_esp_zorro_tbl);


The ID ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060
is used at index 1 and index 5.
I'm guessing only the first one will be detected.

Regards,

Kars.


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-04 Thread Finn Thain
On Mon, 5 Mar 2018, Michael Schmitz wrote:

> All Zorro-3 boards have to have both their regs and dma_regs remapped.
> 

I see.

> What's confusing is that there is only a single Zorro-3 board currently 
> supported by the driver. Others will be added and I"ll use a switch 
> statement to pick the appropriate address based on the ID. That might 
> make it clearer.
> 

Maybe you should add a comment to explain that.

> > Maybe store ent->id in the private struct to get around that?
> 
> Yes, that could be done. I still think it's not needed.
> 

I see what you mean.

Given your explanation above, you could simply remove the confusing 

if (ent->id == ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260)

conditional. That's because the (ioaddr > 0xff) test already takes 
care of the zorro2/zorro3 distinction, right?

That would avoid the confusing ioremap/iounmap asymmetry.

-- 

> Cheers,
> 
>   Michael
> 


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-04 Thread Finn Thain
On Mon, 5 Mar 2018, Michael Schmitz wrote:

> > The TCQ issue showed up on the AV Quadras becasue mac_esp doesn't know 
> > how to do PDMA or DMA on that hardware, and so it always uses PIO. It 
> > sounds like this bug would show up too given the right kind of target. 
> > If so, I will need to patch mac_esp too.
> 
> It did show up in PIO mode so it's a matter of having the right target 
> to trigger it. dmesg on elgar says:
> 
> [   26.96] scsi host1: esp
> [   27.27] scsi 1:0:1:0: Direct-Access IBMRAID  DFHSS2F9337   4I4I 
> PQ: 0 ANSI: 2
> [   27.28] scsi target1:0:1: Beginning Domain Validation
> [   27.36] scsi target1:0:1: FAST-10 SCSI 10.0 MB/s ST (100 ns, offset 15)
> [   27.38] scsi target1:0:1: Domain Validation skipping write tests
> [   27.39] scsi target1:0:1: Ending Domain Validation
> [   27.45] scsi 1:0:2:0: CD-ROMSONY CD-ROM CDU-561   1.9m 
> PQ: 0 ANSI: 2 CCS
> [   27.46] scsi target1:0:2: Beginning Domain Validation
> [   27.58] scsi target1:0:2: FAST-5 SCSI 4.0 MB/s ST (248 ns, offset 15)
> [   27.60] scsi target1:0:2: Domain Validation skipping write tests
> [   27.62] scsi target1:0:2: Ending Domain Validation
> [   27.75] scsi host1: scsi scan: INQUIRY result too short (5), using 36
> 
> To the best of my knowledge, the CD-ROM triggered that error. Looking at 
> the NCR53CF94_96 databook, select with attention and stop is used when 
> multiple message bytes need to be transferred (example: sync negotiation 
> - that's when esp->flags |= ESP_FLAG_DOING_SLOWCMD is used in the ESP 
> core which switches to using this selection mode).
> 
> One message byte is transferred before the ESP raises both 
> ESP_INTR_FDONE and ESP_INTR_BSERV. From all appearances, the bus will 
> still be in message out phase at that stage.
> 
> u8 mask = ~(phase == ESP_MIP ? ESP_INTR_FDONE : (phase == ESP_MOP ?
> ESP_INTR_FDONE|ESP_INTR_BSERV : ESP_INTR_BSERV) );
> 
> would set the correct interrupt status mask to avoid detecting this 
> condition as error.

Well, it is consistent with zorro_esp_send_pio_cmd as far as ESP_MOP is 
concerned.

> Doing sync negotiation is fairly basic - I wonder why you haven't seen 
> this before. Ah - the Mac driver supports sync transfers only when using 
> PDMA, that's why.
> 

I suspect that if you try PIO for all transfers, not just Message In, 
you'll need to drop async support too. But I don't recall why async was a 
problem back when I was developing mac_esp. It was too long ago.

> The description of 'select with attention' also states the same two 
> interrupt bits are set, so maybe the bug is elsewhere. According to the 
> manual, only the first message byte should be loaded into the FIFO when 
> stopping for additional message bytes. We load the entire message 
> instead...
> 

FDONE is not a problem once all the bytes are in the fifo. The mac_esp 
algorithm already accomodates that.

Which manual is this, BTW?

> >
> >> 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,
> >
> > Thanks. That's what I'd prefer to use in mac_esp (if it works).
> >
> >> but setting the mask up front according to the bus phase would make a 
> >> lot of sense.
> >>
> >
> > The idea is that ESP_INTR_FDONE is an error when the phase is not 
> > appropriate. In the version you sent, the loop permits ESP_INTR_BSERV 
> > | ESP_INTR_FDONE regardless of phase.
> 
> Deciding whether an error exists just based on the phase might be 
> impossible now I come to think of it - the combination of 
> ESP_INTR_FDONE|ESP_INTR_BSERV is only legitimate for selection commands. 

Well, I'm not trying to decide whether any error exists, I'm just trying 
to detect an unexpected interrupt in the middle of ESP_DOP. Hence my 
suggestion to add a test for ESP_MOP.

> We might need to look at additional information to figure out whether 
> getting ESP_INTR_FDONE in message out phase was an error.
> 

Your algorithm is pretty clear on that point: FDONE is not an error during 
message out... but I'm beginning to think that it really is an error 
during message out, except at the end of the transfer.

> This is getting a little academic for zorro_esp - if a particular board 
> needs to do PIO all the time, we need a way to set the ESP_FLAG_USE_FIFO 
> flag in the core driver instead. Much safer.
> 

Unfortunately, that flag doesn't always do what is needed for PIO.

Anyway, this discussion is completely academic when you consider this:

if (phase == ESP_MIP) {
zorro_esp_send_pio_cmd(esp, addr, esp_count,
   dma_count, write, cmd);
return;
}

Based on this, you should be able to use the algorithm from 
mac_esp_send_pio_cmd() unaltered, since your change to the !write branch 
is irrelevant during Message In phase.

My main concern here is to avoid two slightly 

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-04 Thread Michael Schmitz
Hi Finn,

On Mon, Mar 5, 2018 at 2:01 PM, Finn Thain  wrote:
> On Mon, 5 Mar 2018, Michael Schmitz wrote:
>
>> >> +fail_unmap_dma_regs:
>> >> + if (ioaddr > 0xff)
>> >> + iounmap(esp->dma_regs);
>> >
>> > I think you need to test for ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260 here?
>>
>> On second thought - no, I don't. the ID check above only determines what
>> Zorro-3 address is ioremapped, but in each case where ioaddr > 0xff,
>> something will have been mapped at this point.
>>
>
> I think you are talking about esp->regs? For esp->dma_regs, the ioremap is
> conditional on ent->id, but the unmap is not.

The details of the ioremap are conditional on the ID, but the fact
that the ioremap happens (and hence esp->dma_regs is an ioremapped
address) is not. All Zorro-3 boards have to have both their regs and
dma_regs remapped.

What's confusing is that there is only a single Zorro-3 board
currently supported by the driver. Others will be added and I"ll use a
switch statement to pick the appropriate address based on the ID. That
might make it clearer.

>
>> >> +}
>> >> +
>> >> +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 > 0xff) {
>> >> + iounmap(esp->dma_regs);
>> >
>> > Do you need to test for ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260 first?
>>
>> I can't - ent->id is not available here...
>
> Maybe store ent->id in the private struct to get around that?

Yes, that could be done. I still think it's not needed.

Cheers,

  Michael


>
> --


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-04 Thread Michael Schmitz
Hi Finn,

>> >> +#define ZORRO_ESP_GET_PRIV(esp) ((struct zorro_esp_priv *) \
>> >> +  _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.
>
> The host_no range is not limited to just zorro cards. It's also libata
> hosts, USB Attached Storage hosts, scsi_debug etc.

Max. libata hosts is one, no USB host adapters I know of, leaves
scsi_debug. But I'll get rid of this if at all possible.

>> > 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...
>>
>
> The TCQ issue showed up on the AV Quadras becasue mac_esp doesn't know how
> to do PDMA or DMA on that hardware, and so it always uses PIO. It sounds
> like this bug would show up too given the right kind of target. If so, I
> will need to patch mac_esp too.

It did show up in PIO mode so it's a matter of having the right target
to trigger it.
dmesg  on elgar says:

[   26.96] scsi host1: esp
[   27.27] scsi 1:0:1:0: Direct-Access IBMRAID  DFHSS2F9337
  4I4I PQ: 0 ANSI: 2
[   27.28] scsi target1:0:1: Beginning Domain Validation
[   27.36] scsi target1:0:1: FAST-10 SCSI 10.0 MB/s ST (100 ns, offset 15)
[   27.38] scsi target1:0:1: Domain Validation skipping write tests
[   27.39] scsi target1:0:1: Ending Domain Validation
[   27.45] scsi 1:0:2:0: CD-ROMSONY CD-ROM CDU-561
  1.9m PQ: 0 ANSI: 2 CCS
[   27.46] scsi target1:0:2: Beginning Domain Validation
[   27.58] scsi target1:0:2: FAST-5 SCSI 4.0 MB/s ST (248 ns, offset 15)
[   27.60] scsi target1:0:2: Domain Validation skipping write tests
[   27.62] scsi target1:0:2: Ending Domain Validation
[   27.75] scsi host1: scsi scan: INQUIRY result too short (5), using 36

To the best of my knowledge, the CD-ROM triggered that error. Looking
at the NCR53CF94_96 databook, select with attention and stop is used
when multiple message bytes need to be transferred (example: sync
negotiation - that's when esp->flags |= ESP_FLAG_DOING_SLOWCMD is used
in the ESP core which switches to using this selection mode).

One message byte is transferred before the ESP raises both
ESP_INTR_FDONE and ESP_INTR_BSERV. From all appearances, the bus will
still be in message out phase at that stage.

u8 mask = ~(phase == ESP_MIP ? ESP_INTR_FDONE : (phase == ESP_MOP ?
ESP_INTR_FDONE|ESP_INTR_BSERV : ESP_INTR_BSERV) );

would set the correct interrupt status mask to avoid detecting this
condition as error. Doing sync negotiation is fairly basic - I wonder
why you haven't seen this before. Ah - the Mac driver supports sync
transfers only when using PDMA, that's why.

The description of 'select with attention' also states the same two
interrupt bits are set, so maybe the bug is elsewhere. According to
the manual, only the first message byte should be loaded into the FIFO
when stopping for additional message bytes. We load the entire message
instead...

>
>> 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,
>
> Thanks. That's what I'd prefer to use in mac_esp (if it works).
>
>> but setting the mask up front according to the bus phase would make a
>> lot of sense.
>>
>
> The idea is that ESP_INTR_FDONE is an error when the phase is not
> appropriate. In the version you sent, the loop permits ESP_INTR_BSERV |
> ESP_INTR_FDONE regardless of phase.

Deciding whether an error exists just based on the phase might be
impossible now I come to think of it - the combination of
ESP_INTR_FDONE|ESP_INTR_BSERV is only legitimate for selection
commands. We might need to look at additional information to figure
out whether getting  ESP_INTR_FDONE in message out phase was an error.

This is getting a little academic for zorro_esp - if a particular
board needs to do PIO all the time, we need a way to set the
ESP_FLAG_USE_FIFO flag in the core driver instead. Much safer.

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

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-04 Thread Finn Thain
On Mon, 5 Mar 2018, Michael Schmitz wrote:

> >> +fail_unmap_dma_regs:
> >> + if (ioaddr > 0xff)
> >> + iounmap(esp->dma_regs);
> >
> > I think you need to test for ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260 here?
> 
> On second thought - no, I don't. the ID check above only determines what 
> Zorro-3 address is ioremapped, but in each case where ioaddr > 0xff, 
> something will have been mapped at this point.
> 

I think you are talking about esp->regs? For esp->dma_regs, the ioremap is 
conditional on ent->id, but the unmap is not.

> >> +}
> >> +
> >> +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 > 0xff) {
> >> + iounmap(esp->dma_regs);
> >
> > Do you need to test for ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260 first?
> 
> I can't - ent->id is not available here...

Maybe store ent->id in the private struct to get around that?

-- 


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-04 Thread Michael Schmitz
Hi Finn,

>> +/* zorro_esp.c: ESP front-end for Amiga ZORRO SCSI systems.
>> + *
>> + * Copyright (C) 1996 Jesper Skov (js...@cygnus.co.uk)
>> + *
>> + * Copyright (C) 2011,2018 Michael Schmitz (schm...@debian.org) for
>> + *   migration to ESP SCSI core
>
> You can blame me for some of this ;-)

I'll add a line here pointing out the source for the PIO code, so
others will have a chance to keep these in sync if needs be.


>> +fail_unmap_dma_regs:
>> + if (ioaddr > 0xff)
>> + iounmap(esp->dma_regs);
>
> I think you need to test for ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260 here?

On second thought - no, I don't. the ID check above only determines
what Zorro-3 address is ioremapped, but in each case where ioaddr >
0xff, something will have been mapped at this point.

>> +}
>> +
>> +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 > 0xff) {
>> + iounmap(esp->dma_regs);
>
> Do you need to test for ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260 first?

I can't - ent->id is not available here. But again, if host->base is
outside the Zorro-2 space, the DMA register mapping has been done
through ioremap() and needs to be undone here.

Cheers,

  Michael


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-04 Thread Finn Thain
On Sun, 4 Mar 2018, Michael Schmitz wrote:

> Am 04.03.2018 um 15:55 schrieb Finn Thain:

> >> +/* zorro_esp.c: ESP front-end for Amiga ZORRO SCSI systems.
> >> + *
> >> + * Copyright (C) 1996 Jesper Skov (js...@cygnus.co.uk)
> >> + *
> >> + * Copyright (C) 2011,2018 Michael Schmitz (schm...@debian.org) 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?
> 

Right. The commit message is sufficient credit for my needs.

> >> +#define ZORRO_ESP_GET_PRIV(esp) ((struct zorro_esp_priv *) \
> >> +  _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.

The host_no range is not limited to just zorro cards. It's also libata 
hosts, USB Attached Storage hosts, scsi_debug etc.

> > 
> > 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...
> 

The TCQ issue showed up on the AV Quadras becasue mac_esp doesn't know how 
to do PDMA or DMA on that hardware, and so it always uses PIO. It sounds 
like this bug would show up too given the right kind of target. If so, I 
will need to patch mac_esp too.

> 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,

Thanks. That's what I'd prefer to use in mac_esp (if it works).

> but setting the mask up front according to the bus phase would make a 
> lot of sense.
> 

The idea is that ESP_INTR_FDONE is an error when the phase is not 
appropriate. In the version you sent, the loop permits ESP_INTR_BSERV | 
ESP_INTR_FDONE regardless of phase.

> > 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?
> 

Thinking about this a bit more, BUG_ON may be the right thing to do here. 
If the core driver will not catch the DMA error, that might lead to data 
corruption. You'd better check this before switching to WARN_ON. We can't 
assume that end users will notice the warning in the logs.

> >> +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.
> 

Both mac_esp_send_pdma_cmd() and mac_esp_send_pio_cmd() just initialize 
this to zero at the outset. Any time that mep->error is set, it means the 
most recent transfer failed, which I think is what matters here.

-- 


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-04 Thread Michael Schmitz
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 
>>
>> 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 (js...@cygnus.co.uk)
>> + *
>> + * Copyright (C) 2011,2018 Michael Schmitz (schm...@debian.org) 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 *) \
>> +_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 >> 

Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-03 Thread Finn Thain
On Sun, 4 Mar 2018, I wrote:

> > +   } else {
> > +   scsi_esp_cmd(esp, ESP_CMD_FLUSH);
> > +
> > +   if (esp_count >= ZORRO_ESP_FIFO_SIZE) {
> > +   ZORRO_ESP_PIO_FILL("%0@+,%2@", esp_count);
> > +   } else {
> > +   ZORRO_ESP_PIO_LOOP("%0@+,%2@", esp_count);
> > +   }
> > +
> > +   scsi_esp_cmd(esp, cmd);
> > +
> > +   while (esp_count) {
> > +   unsigned int n;
> > +
> > +   if (zorro_esp_wait_for_intr(esp))
> > +   break;
> > +
> > +   if ((esp->sreg & ESP_STAT_PMASK) != phase)
> > +   break;
> > +
> > +   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.
> 

Nevermind -- I got the direction wrong there.

But if your comment is correct and the mask is wrong, that problem should 
show up in mac_esp too, right?

The whole TCQ problem showed up, and was fixed by checking ESP_INTR_FDONE 
in the Message In transfer.

> > +   if (esp->ireg & ~(ESP_INTR_BSERV | ESP_INTR_FDONE)) {
> > +   zep->error = 1;
> > +   break;
> > +   }

If you really do get ESP_INTR_FDONE instead of ESP_INTR_BSERV for Message 
Out, wouldn't it be safer (and more consistent) to do,

if (esp->ireg & mask) {
mep->error = 1;
break;
}

and initialize mask as,

u8 mask = ~(phase == ESP_MIP || phase == ESP_MOP ? ESP_INTR_FDONE
 : ESP_INTR_BSERV);

to cover both !write and write?

-- 


Re: [PATCH 2/2] m68k/amiga - Zorro ESP: new zorro_esp.c

2018-03-03 Thread Finn Thain
On Sun, 4 Mar 2018, Michael Schmitz wrote:

> From: Michael Schmitz 
> 
> 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.

> 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).
> 
> 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. Use phys_to_virt(addr)
> to determine kernel virtual address in all other cases (this works on m68k).
> 
> A 'select with attention and stop' command raises ESP_INTR_FDONE, so
> adjust interrupt mask used to end PIO transfer accordingly.
> 
> Signed-off-by: Michael Schmitz 
> ---
>  drivers/scsi/zorro_esp.c |  785 
> ++
>  1 files changed, 785 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/scsi/zorro_esp.c
> 
> diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
> new file mode 100644
> index 000..03c4ad2
> --- /dev/null
> +++ b/drivers/scsi/zorro_esp.c
> @@ -0,0 +1,785 @@
> +/* zorro_esp.c: ESP front-end for Amiga ZORRO SCSI systems.
> + *
> + * Copyright (C) 1996 Jesper Skov (js...@cygnus.co.uk)
> + *
> + * Copyright (C) 2011,2018 Michael Schmitz (schm...@debian.org) for
> + *   migration to ESP SCSI core

You can blame me for some of this ;-)

> + */
> +/*
> + * ZORRO bus code from:
> + */
> +/*
> + * Detection routine for the NCR53c710 based Amiga SCSI Controllers for 
> Linux.
> + *   Amiga MacroSystemUS WarpEngine SCSI controller.
> + *   Amiga Technologies/DKB A4091 SCSI controller.
> + *
> + * Written 1997 by Alan Hourihane 
> + * plus modifications of the 53c7xx.c driver to support the Amiga.
> + *
> + * Rewritten to use 53c700.c by Kars de Jong 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +

Seems to be an excess blank line.

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "esp_scsi.h"
> +
> +#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

> +
> +MODULE_AUTHOR("Michael Schmitz ");
> +MODULE_DESCRIPTION("Amiga Zorro NCR5C9x (ESP) driver");
> +MODULE_LICENSE("GPL");
> +
> +#define DMA_WRITE 0x8000
> +
> +static struct zorro_driver_data {
> + const char *name;
> + unsigned long offset;
> + unsigned long dma_offset;
> + int absolute;
> + int zorro3; /* offset is absolute address */
> +} 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 = 0x1,
> +   .absolute = 0, .zorro3 = 0 },
> + { .name = "Blizzard 1230II", .offset = 0x1, .dma_offset = 0x10021,
> +   .absolute = 0, .zorro3 = 0 },
> + { .name = "Fastlane", .offset = 0x101, .dma_offset = 0x141,
> +   .absolute = 0, .zorro3 = 1 },
> + { 0 }
> +};
> +
> +static struct zorro_device_id zorro_esp_zorro_tbl[] = {
> + {
> + .id = ZORRO_PROD_PHASE5_BLIZZARD_1220_CYBERSTORM,
> + .driver_data = (unsigned long)_esp_driver_data[0],
> + },
> + {
> + .id = 
> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
> + .driver_data = (unsigned long)_esp_driver_data[0],
> + },
> + {
> + .id = ZORRO_PROD_PHASE5_CYBERSTORM_MK_II,
> + .driver_data = (unsigned long)_esp_driver_data[1],
> + },
> +