Re: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Finn Thain]

On Fri, 16 Mar 2018, Michael Schmitz wrote:

> Hi Finn, Geert,
> 
> In the interest of making minimal changes between the Mac and Amiga 
> versions, I'd leave the macros as they are, and add a comment to the 
> macro definitions stating that both addr and fifo are local-scope 
> variables in the only scope the macro is used in, to address reviewer's 
> concerns. Can you both live with that?
> 

OK.

> Placing the two macros in a suitable header in arch/m68k/include/asm/ so 
> Mac and Amiga can share the same code without duplicating it in two 
> files would be another option (that forces use of addr and fifo as 
> parameters), but let's not overengineer things.

Yes, deduplication would be nice but I'd like to extend that to the entire 
PIO implementation.

We should be using the portable IO routines but I'd want to do some 
timings first. Inline assembly was unavoidable for the PDMA loops. Later 
when I came to write the PIO versions, I just re-used the existing code 
without benchmarking. So there's a slim chance that insb/outsb are not 
slower.

It might be sufficient to unroll the m68k raw_insb() and raw_outsb() 
loops, just as raw_insw() and raw_outsw() have been unrolled. But again, 
I'd want to measure that.

-- 

> I don't expect any other driver would need to share this code, or the 
> PDMA macros also in the Mac driver...
> 
> Other than that, I've implemented and tested all the suggested changes 
> and could post v4 of this patch now.
> 
> Cheers,
> 
> ??? Michael

Re: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Geert Uytterhoeven]

Hi Michael,

On Fri, Mar 16, 2018 at 8:26 AM, Michael Schmitz  wrote:
> In the interest of making minimal changes between the Mac and Amiga
> versions, I'd leave the macros as they are, and add a comment to the
> macro definitions stating that both addr and fifo are local-scope
> variables in the only scope the macro is used in, to address reviewer's
> concerns. Can you both live with that?

Yes I can. Thanks!

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 v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Michael Schmitz]

Hi Finn, Geert,

In the interest of making minimal changes between the Mac and Amiga
versions, I'd leave the macros as they are, and add a comment to the
macro definitions stating that both addr and fifo are local-scope
variables in the only scope the macro is used in, to address reviewer's
concerns. Can you both live with that?

Placing the two macros in a suitable header in arch/m68k/include/asm/ so
Mac and Amiga can share the same code without duplicating it in two
files would be another option (that forces use of addr and fifo as
parameters), but let's not overengineer things. I don't expect any other
driver would need to share this code, or the PDMA macros also in the Mac
driver...

Other than that, I've implemented and tested all the suggested changes
and could post v4 of this patch now.

Cheers,

    Michael


Am 16.03.18 um 00:17 schrieb Finn Thain:
> On Wed, 14 Mar 2018, Michael Schmitz wrote:
>
>>> Please pass "addr" and "fifo" as macro parameters, too, so it's easier 
>>> for the reviewer to notice they are used.
>> Yes, I can do that (meaning Finn would need to make the same change to 
>> keep our versions in sync).
> Personally, I wouldn't want to change it. This wasn't an oversight. Maybe 
> if you (Geert) take a look at MAC_ESP_PDMA_LOOP, etc. this style might 
> make more sense.
>
> These are not macros in a header file that might get used in any random 
> scope. There is exactly one scope in which the macro appears, and the 
> variables that appear in the macro are simply the variables from that 
> scope.
>
> If you apply the rule, "the macro's parameters should exhaustively list 
> all the macro's symbols", then (in this case) you'd violate the rule 
> "Don't Repeat Yourself". And if you'd adhere to the latter rule then you'd 
> violate the former. So I chose the more readable option.
>
> The preprocessor allows us to pretend we are doing symbolic code 
> transformation, but that's not needed here. This was meant to be a textual 
> device.
>

Re: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Finn Thain]

On Wed, 14 Mar 2018, Michael Schmitz wrote:

> > 
> > Please pass "addr" and "fifo" as macro parameters, too, so it's easier 
> > for the reviewer to notice they are used.
> 
> Yes, I can do that (meaning Finn would need to make the same change to 
> keep our versions in sync).

Personally, I wouldn't want to change it. This wasn't an oversight. Maybe 
if you (Geert) take a look at MAC_ESP_PDMA_LOOP, etc. this style might 
make more sense.

These are not macros in a header file that might get used in any random 
scope. There is exactly one scope in which the macro appears, and the 
variables that appear in the macro are simply the variables from that 
scope.

If you apply the rule, "the macro's parameters should exhaustively list 
all the macro's symbols", then (in this case) you'd violate the rule 
"Don't Repeat Yourself". And if you'd adhere to the latter rule then you'd 
violate the former. So I chose the more readable option.

The preprocessor allows us to pretend we are doing symbolic code 
transformation, but that's not needed here. This was meant to be a textual 
device.

-- 

Re: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Michael Schmitz]

Hi Geert,

Am 14.03.2018 um 21:30 schrieb Geert Uytterhoeven:
> Hi Michael,
> 
> On Wed, Mar 14, 2018 at 9:23 AM, Michael Schmitz  wrote:
>> thanks for the review - largely uncontroversial except for the volatile...
> 
> The presence of volatile in drivers is always considered controversial ;-)

Yes indeed. I just hadn't looked at raw_io.h in ages (too many unhappy
memories), and didn't remember the __force volatile trick there.

>> Meaning writeb(val, reg) instead of reg = val?
>>
>> #define out_8(addr,b) (void)((*(__force volatile u8 *) (addr)) = (b))
>>
>> nicely hides the 'volatile' but suggests I also need to pass it a
>> pointer, so
>>
>> writeb((addr >> 24) & 0xff, >dma_addr)
> 
> Yes, you have to pass it an (__iomem) pointer.

Thanks.

>> I'll have to compare the assembly generated by the two versions before I
>> dare test that, but I'll give that a try. Liberal use of wmb() did fix
>> the miscompile but that just looked too ugly.
> 
> Using the macros should have the same effect due to the embedded volatile.

I tend to agree, but crashing elgar or trashing the SCSI filesystem
there isn't something I would like to risk (Adrian has been very
accommodating allowing me to test the driver there, but a reset tends to
be taking a few hours thanks to the time zone difference). So I'd rather
make sure nothing can go wrong.

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 v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Michael Schmitz]

Hi Tuomas,

oddly enough, it does. How embarrassing...

Hope I got the names of the boards right this time.

Thanks for spotting this!

Cheers,

Michael

Am 14.03.2018 um 20:13 schrieb Vainikka Tuomas:
> The config description lists 53c700 devices, not ESP devices?
> 
> -Tuomas
> 
> From: linux-m68k-ow...@vger.kernel.org <linux-m68k-ow...@vger.kernel.org> on 
> behalf of Michael Schmitz <schmitz...@gmail.com>
> Sent: 12 March 2018 09:26:40
> To: linux-scsi@vger.kernel.org
> Cc: linux-m...@vger.kernel.org; da...@davemloft.net; ge...@linux-m68k.org; 
> fth...@telegraphics.com.au; Michael Schmitz; Michael Schmitz
> Subject: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c
> 
> From: Michael Schmitz <schm...@debian.org>
> 
> New combined SCSI driver for all ESP based Zorro SCSI boards for
> m68k Amiga.
> 
> 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).
> 
> New Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
> driver included in this patch.
> 
> 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.
> 
> Signed-off-by: Michael Schmitz <schmitz...@gmail.com>
> 
> ---
> 
> Changes since v1:
> 
> Fixed issues raised by Finn Thain in initial code review:
> 
> - use KBUILD_MODNAME for driver name string.
> - use pr_fmt() for pr_* format prefix.
> - clean up DMA error reporting: clear error flag before each DMA
>   operation, set error flag on PIO error. Don't test phase in dma_err hook.
> - change confusing comment about semantics of read flag, add comments
>   indicating DMA direction to DMA setup hooks.
> - drop spurious braces around switch clauses.
> - lift cfreq setting out of ID switch clauses.
> - fix indentation.
> - fix error codes on probe fail.
> - drop check for board ID when unmapping DMA regs in error handling:
>   the ioaddr > 0xff test already catches all cases where the DMA
>   registers were ioremapped.
> - dynamically alloc zorro_private_data.
> - fix use of driver_data field (don't mix host and zorro_esp_priv
>   pointers). Note: require esp pointer in zorro_esp_priv to find host
>   pointer!
> - back out phase bits changes to pio_cmd !write branch introduced
>   to cope with ESP SELAS command. We don't use that code so keep
>   it in sync with Finn's version.
> - use esp_ops.dma_length_limit() to limit transfer size. After review
>   of old driver code, use 0xff max transfer size throughout.
> 
> Fixed issues raised by Geert Uytterhoven:
> 
> - dynamically alloc zorro_private_data, store as device drvdata.
> - store ctrl_data for CyberStormI in driver private data.
> - use dma_sync_single_for_device() instead of cache_push/clear.
> - handle case of duplicate board identity - check whether board is
>   Zorro III or Zorro II (use ROM resource data for this). Also fix
>   up DMA mask for Zorro II boards to 32 bits (these are really CPU
>   expansion slot boards).
> - remove zorro3 field from driver_data struct (now in private data).
> - add braces around ambiguous if - else construct.
> - use named structs instead of array for board config data.
> - use scsi_option driver data flag for boards with optional ESP.
> 
> Other improvements and bugfixes
> 
> - fix Zorro device table error (duplicate ID used, also raised
>   by Kars de Jong).
> - error code fixup in error handling path.
> - add separate DMA setup for Blizzard 1230 II board.
> - add support for Cyberstorm II board.
> - add register structs and DMA setup for Zorro III Fastlane board,
>   following logic from old fastlane.c driver. Wire up Fastlane DMA
>   and interrupt status routines, map the necessary low 24 bit board
>   address space used for DMA target address setting. Clean up DMA
>   register space ioremap() branch for Zorro III boards (currently
>   Fastlane only) to end confusion about what to do in e

Re: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Geert Uytterhoeven]

Hi Michael,

On Wed, Mar 14, 2018 at 9:23 AM, Michael Schmitz  wrote:
> thanks for the review - largely uncontroversial except for the volatile...

The presence of volatile in drivers is always considered controversial ;-)

> Am 14.03.2018 um 20:49 schrieb Geert Uytterhoeven:
>>> +/* Blizzard 1230 DMA interface */
>>> +
>>> +struct blz1230_dma_registers {
>>> +   volatile unsigned char dma_addr;/* DMA address  
>>> [0x] */
>>
>> volatile considered harmful.
>
> Yes, I saw that. I also saw gcc miscompile the DMA set-up (in
> particular, the case where three bytes of the transfer address are
> stuffed consecutively into the same DMA address register).
>
>> If you would use proper *{read,write}*() accessors instead of direct
>> assignments,
>> you can drop the volatile's here.
>
> Meaning writeb(val, reg) instead of reg = val?
>
> #define out_8(addr,b) (void)((*(__force volatile u8 *) (addr)) = (b))
>
> nicely hides the 'volatile' but suggests I also need to pass it a
> pointer, so
>
> writeb((addr >> 24) & 0xff, >dma_addr)

Yes, you have to pass it an (__iomem) pointer.

> would do the same as
>
> dregs->dma_addr  = (addr >> 24) & 0xff; ??

Right.

> I'll have to compare the assembly generated by the two versions before I
> dare test that, but I'll give that a try. Liberal use of wmb() did fix
> the miscompile but that just looked too ugly.

Using the macros should have the same effect due to the embedded volatile.

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 v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Michael Schmitz]

Hi Geert,

thanks for the review - largely uncontroversial except for the volatile...

Am 14.03.2018 um 20:49 schrieb Geert Uytterhoeven:
>> +/* Blizzard 1230 DMA interface */
>> +
>> +struct blz1230_dma_registers {
>> +   volatile unsigned char dma_addr;/* DMA address  [0x] 
>> */
> 
> volatile considered harmful.

Yes, I saw that. I also saw gcc miscompile the DMA set-up (in
particular, the case where three bytes of the transfer address are
stuffed consecutively into the same DMA address register).

> If you would use proper *{read,write}*() accessors instead of direct
> assignments,
> you can drop the volatile's here.

Meaning writeb(val, reg) instead of reg = val?

#define out_8(addr,b) (void)((*(__force volatile u8 *) (addr)) = (b))

nicely hides the 'volatile' but suggests I also need to pass it a
pointer, so

writeb((addr >> 24) & 0xff, >dma_addr)

would do the same as

dregs->dma_addr  = (addr >> 24) & 0xff; ??

I'll have to compare the assembly generated by the two versions before I
dare test that, but I'll give that a try. Liberal use of wmb() did fix
the miscompile but that just looked too ugly.

>> +#define ZORRO_ESP_PIO_LOOP(operands, reg1) \
>> +   { \
>> +   asm volatile ( \
>> +"1: moveb " operands "\n" \
>> +"   subqw #1,%1   \n" \
>> +"   jbne 1b   \n" \
>> +: "+a" (addr), "+r" (reg1) \
>> +: "a" (fifo)); \
>> +   }
> 
> Please pass "addr" and "fifo" as macro parameters, too, so it's easier for
> the reviewer to notice they are used.

Yes, I can do that (meaning Finn would need to make the same change to
keep our versions in sync).

>> +   /* 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;
> 
> Please use function pointers in struct zorro_driver_data, so you don't need
> a switch() here (except for Fastlane vs. B1230II).

At that point, the Blizzard 1230 II zorro_driver_data has been replaced
by the Fastlane one so the correct function pointer would be used. I
didn't realize that also nicely solves my problem here.

Thanks!

Cheers,

Michael

Re: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Geert Uytterhoeven]

Hi Michael,

On Mon, Mar 12, 2018 at 8:26 AM, Michael Schmitz  wrote:
> From: Michael Schmitz 
>
> New combined SCSI driver for all ESP based Zorro SCSI boards for
> m68k Amiga.
>
> 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).
>
> New Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
> driver included in this patch.
>
> 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.
>
> Signed-off-by: Michael Schmitz 
>
> ---
>
> Changes since v1:

[...]

Thanks for the update!

A few more comments below, mostly stylistic / practice.

> --- /dev/null
> +++ b/drivers/scsi/zorro_esp.c

> +struct zorro_driver_data {
> +   const char *name;
> +   unsigned long offset;
> +   unsigned long dma_offset;
> +   int absolute;   /* offset is absolute address */
> +   int scsi_option;
> +};
> +
> +static struct zorro_driver_data cyberstormI_data = {

const

> +static struct zorro_device_id zorro_esp_zorro_tbl[] = {

const

> +   {
> +   .id = ZORRO_PROD_PHASE5_BLIZZARD_1220_CYBERSTORM,
> +   .driver_data = (unsigned long)_data,
> +   },
> +   {
> +   .id = ZORRO_PROD_PHASE5_CYBERSTORM_MK_II,
> +   .driver_data = (unsigned long)_data,
> +   },
> +   {
> +   .id = ZORRO_PROD_PHASE5_BLIZZARD_2060,
> +   .driver_data = (unsigned long)_data,
> +   },
> +   {
> +   .id = ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260,
> +   .driver_data = (unsigned long)_data,
> +   },
> +   {
> +   .id = 
> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
> +   .driver_data = (unsigned long)_data,
> +   },
> +   { 0 }
> +};
> +MODULE_DEVICE_TABLE(zorro, zorro_esp_zorro_tbl);
> +
> +/* Blizzard 1230 DMA interface */
> +
> +struct blz1230_dma_registers {
> +   volatile unsigned char dma_addr;/* DMA address  [0x] 
> */

volatile considered harmful.
If you would use proper *{read,write}*() accessors instead of direct
assignments,
you can drop the volatile's here.

> +#define ZORRO_ESP_GET_PRIV(esp) ((struct zorro_esp_priv *) \
> +   dev_get_drvdata(esp->dev))

This macro can be dropped, just use "dev_get_drvdata(esp->dev)" directly.
No cast is needed.

> +static int fastlane_esp_irq_pending(struct esp *esp)
> +{
> +   struct fastlane_dma_registers *dregs =
> +   (struct fastlane_dma_registers *) (esp->dma_regs);

struct fastlane_dma_registers __iomem *dregs = esp->dma_regs;

(and make C=1 will become (more) happy)

> +   unsigned char dma_status;
> +
> +   dma_status = dregs->cond_reg;

readb().

> +#define ZORRO_ESP_PIO_LOOP(operands, reg1) \
> +   { \
> +   asm volatile ( \
> +"1: moveb " operands "\n" \
> +"   subqw #1,%1   \n" \
> +"   jbne 1b   \n" \
> +: "+a" (addr), "+r" (reg1) \
> +: "a" (fifo)); \
> +   }

Please pass "addr" and "fifo" as macro parameters, too, so it's easier for
the reviewer to notice they are used.

> +#define ZORRO_ESP_PIO_FILL(operands, reg1) \
> +   { \
> +   asm volatile ( \
> +"   moveb " operands "\n" \
> +"   moveb " operands "\n" \
> +"   moveb " operands "\n" \
> +"   moveb " operands "\n" \
> +"   moveb " operands "\n" \
> +"   moveb " operands "\n" \
> +"   moveb " operands "\n" \
> +"   moveb " operands "\n" \
> +"   moveb " operands "\n" \
> +"   moveb " operands "\n" \
> +"   moveb " operands "\n" \
> +"   moveb " operands "\n" \
> +"   moveb " operands "\n" \
> +"   moveb " operands "\n" \
> +"   moveb " operands "\n" \
> +"   moveb " operands "\n" \
> +"   subqw #8,%1   \n" \
> +"   subqw #8,%1   \n" \
> +: "+a" (addr), "+r" (reg1) \
> +

Re: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Vainikka Tuomas]

The config description lists 53c700 devices, not ESP devices?

-Tuomas

From: linux-m68k-ow...@vger.kernel.org <linux-m68k-ow...@vger.kernel.org> on 
behalf of Michael Schmitz <schmitz...@gmail.com>
Sent: 12 March 2018 09:26:40
To: linux-scsi@vger.kernel.org
Cc: linux-m...@vger.kernel.org; da...@davemloft.net; ge...@linux-m68k.org; 
fth...@telegraphics.com.au; Michael Schmitz; Michael Schmitz
Subject: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

From: Michael Schmitz <schm...@debian.org>

New combined SCSI driver for all ESP based Zorro SCSI boards for
m68k Amiga.

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

New Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
driver included in this patch.

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.

Signed-off-by: Michael Schmitz <schmitz...@gmail.com>

---

Changes since v1:

Fixed issues raised by Finn Thain in initial code review:

- use KBUILD_MODNAME for driver name string.
- use pr_fmt() for pr_* format prefix.
- clean up DMA error reporting: clear error flag before each DMA
  operation, set error flag on PIO error. Don't test phase in dma_err hook.
- change confusing comment about semantics of read flag, add comments
  indicating DMA direction to DMA setup hooks.
- drop spurious braces around switch clauses.
- lift cfreq setting out of ID switch clauses.
- fix indentation.
- fix error codes on probe fail.
- drop check for board ID when unmapping DMA regs in error handling:
  the ioaddr > 0xff test already catches all cases where the DMA
  registers were ioremapped.
- dynamically alloc zorro_private_data.
- fix use of driver_data field (don't mix host and zorro_esp_priv
  pointers). Note: require esp pointer in zorro_esp_priv to find host
  pointer!
- back out phase bits changes to pio_cmd !write branch introduced
  to cope with ESP SELAS command. We don't use that code so keep
  it in sync with Finn's version.
- use esp_ops.dma_length_limit() to limit transfer size. After review
  of old driver code, use 0xff max transfer size throughout.

Fixed issues raised by Geert Uytterhoven:

- dynamically alloc zorro_private_data, store as device drvdata.
- store ctrl_data for CyberStormI in driver private data.
- use dma_sync_single_for_device() instead of cache_push/clear.
- handle case of duplicate board identity - check whether board is
  Zorro III or Zorro II (use ROM resource data for this). Also fix
  up DMA mask for Zorro II boards to 32 bits (these are really CPU
  expansion slot boards).
- remove zorro3 field from driver_data struct (now in private data).
- add braces around ambiguous if - else construct.
- use named structs instead of array for board config data.
- use scsi_option driver data flag for boards with optional ESP.

Other improvements and bugfixes

- fix Zorro device table error (duplicate ID used, also raised
  by Kars de Jong).
- error code fixup in error handling path.
- add separate DMA setup for Blizzard 1230 II board.
- add support for Cyberstorm II board.
- add register structs and DMA setup for Zorro III Fastlane board,
  following logic from old fastlane.c driver. Wire up Fastlane DMA
  and interrupt status routines, map the necessary low 24 bit board
  address space used for DMA target address setting. Clean up DMA
  register space ioremap() branch for Zorro III boards (currently
  Fastlane only) to end confusion about what to do in error recovery.
- use esp_ops.fastlane_esp_dma_invalidate() on Fastlane (and skip
  fastlane_esp_reset_dma() in DMA setup).
- credit Tuomas Vainikka for contributing Blizzard 1230 code (and
  testing).
- clarify comment about unsupported Oktagon SCSI board.
- remove unused const definitions carried over from old driver.

Changes since v2:

Issues raised by Finn Thain:

- add SPDX-License-Identifier.
- remove unused ratelimit.h.
- drop phys_to_virt() in PIO transfer routine, after ensuring PIO is only
  used for message in transfers to esp->command_block. This obviates any
  need for finding the virtual address corresponding to a DMA handle.
- drop BUG_ON(!(cmd & ESP_CMD_DMA

Re: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Finn Thain]

> 
> I forgot to add your Reviewed-by tag - will do that for the next 
> version, OK?

Sure.

Geert's tag may be harder to bag though :-)

-- 

Re: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Michael Schmitz]

Hi Finn,


Am 12.03.2018 um 22:04 schrieb Finn Thain:
>> +if (addr == esp->command_block_dma)
>> +addr = (u32) esp->command_block;
> 
> Since you've removed the alternative branch and phys_to_virt(), I suggest 
> you do this at function invocation... (see below)

Keeps it together with the phase and address tests, so easier to follow.


>> +static int zorro_esp_probe(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;
>> +
>> +board = zorro_resource_start(z);
>> +zdd = (struct zorro_driver_data *)ent->driver_data;
>> +
>> +pr_info("%s found at address 0x%lx.\n", zdd->name, board);
>> +
>> +zep = kmalloc(sizeof(*zep), GFP_KERNEL);
>> +if (!zep) {
>> +pr_err("Can't allocate device private data!\n");
>> +return -ENOMEM;
>> +}
>> +
>> +/* let's figure out whether we have a Zorro II or Zorro III board */
>> +if ((z->rom.er_Type & ERT_TYPEMASK) == ERT_ZORROIII) {
>> +/* note this is a Zorro III board */
>> +if (board > 0xff)
>> +zep->zorro3 = 1;
> 
> The comment here seems to be redundant (?)

Not the only one.

> More importantly, I think you have to initialize zep->zorro3 = 0 in the 
> other branch, or else use kzalloc() instead of kmalloc() above.

Using kzalloc() saves the zep->error=0 initialization later on so it's a
win.

>> +case 
>> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060:
>> +if (zep->zorro3) {
>> +/* Fastlane Zorro III board */
>> +/* map address space up to ESP address for DMA */
> 
> Same here?

I've left the comment explaining what the board low address space is
used for.

>> +zep->board_base = ioremap_nocache(board,
>> +FASTLANE_ESP_ADDR-1);
>> +if (!zep->board_base) {
>> +pr_err("Cannot allocate board address space\n");
>> +err = -ENOMEM;
>> +goto fail_free_host;
>> +}
>> +/* initialize DMA control shadow register */
>> +zep->ctrl_data = (FASTLANE_DMA_FCODE |
>> +  FASTLANE_DMA_EDI | FASTLANE_DMA_ESI);
>> +zorro_esp_ops.send_dma_cmd =
>> +zorro_esp_send_fastlane_dma_cmd;
>> +zorro_esp_ops.irq_pending  = fastlane_esp_irq_pending;
>> +zorro_esp_ops.dma_invalidate =
>> +fastlane_esp_dma_invalidate;
>> +} else {
>> +/* Blizzard 1230 II Zorro II board */

and that one - the board might be a Cyberstorm060 which would require
replacement of driver data, and replacement of the ESP register mapping.
Might need to clarify that here again...

>> +zorro_esp_ops.send_dma_cmd =
>> +zorro_esp_send_blz1230II_dma_cmd;

>> +//zorro_set_drvdata(z, host);
>> +
> 
> Can that be deleted?

Leftover from when private data were static, so can go now.


>> +static void zorro_esp_remove(struct zorro_dev *z)
>> +{
>> +/* equivalent to dev_get_drvdata(z->dev) */
> 
> If zorro_get_drvdata(z) needs a comment to explain it then why not just 
> write dev_get_drvdata(z->dev) instead?

dev_get_drvdata(>dev) (struct zorro device incorporates the whole
struct device, not a pointer), but yes.

>> +struct zorro_esp_priv *zep = zorro_get_drvdata(z);
>> +struct esp *esp = zep->esp;
>> +struct Scsi_Host *host = esp->host;
>> +
>> +scsi_esp_unregister(esp);
>> +
>> +/* Disable interrupts. Perhaps use disable_irq instead ... */
>> +
> 
> Can you clarify? free_irq() calls irq_shutdown()...

Which calls disable_irq() eventually if it's the last of shared
interrupts, true. disable_irq() would clearly be wrong here.

> 
> Thanks!
> 

My pleasure. I forgot to add your Reviewed-by tag - will do that for the
next version, OK?

Cheers,

Michael

Re: [PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Finn Thain]

On Mon, 12 Mar 2018, Michael Schmitz wrote:

> diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
> new file mode 100644
> index 000..47053d1
> --- /dev/null
> +++ b/drivers/scsi/zorro_esp.c
> @@ -0,0 +1,1136 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* 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
> + *
> + * Copyright (C) 2013 Tuomas Vainikka (tuomas.vaini...@aalto.fi) for
> + *   Blizzard 1230 DMA and probe function fixes
> + *
> + * Copyright (C) 2017 Finn Thain for PIO code from Mac ESP driver adapted 
> here
> + */
> +/*
> + * 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 
> + */
> +
> +#define KBUILD_MODNAME "zorro_esp"

You don't need to define this. The compiler should be invoked with 
-DKBUILD_MODNAME='"zorro_esp"'


> +#define pr_fmt(fmt)KBUILD_MODNAME ": " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "esp_scsi.h"
> +
> +MODULE_AUTHOR("Michael Schmitz ");
> +MODULE_DESCRIPTION("Amiga Zorro NCR5C9x (ESP) driver");
> +MODULE_LICENSE("GPL");
> +
> +struct zorro_driver_data {
> + const char *name;
> + unsigned long offset;
> + unsigned long dma_offset;
> + int absolute;   /* offset is absolute address */
> + int scsi_option;
> +};
> +
> +static struct zorro_driver_data cyberstormI_data = {
> + .name = "CyberStormI",
> + .offset = 0xf400,
> + .dma_offset = 0xf800,
> + .absolute = 0,
> + .scsi_option = 0,
> +};
> +
> +static struct zorro_driver_data cyberstormII_data = {
> + .name = "CyberStormII",
> + .offset = 0x1ff03,
> + .dma_offset = 0x1ff43,
> + .absolute = 0,
> + .scsi_option = 1,
> +};
> +
> +static struct zorro_driver_data blizzard2060_data = {
> + .name = "Blizzard 2060",
> + .offset = 0x1ff00,
> + .dma_offset = 0x1ffe0,
> + .absolute = 0,
> + .scsi_option = 0,
> +};
> +
> +static struct zorro_driver_data blizzard1230_data = {
> + .name = "Blizzard 1230",
> + .offset = 0x8000,
> + .dma_offset = 0x1,
> + .absolute = 0,
> + .scsi_option = 1,
> +};
> +
> +static struct zorro_driver_data blizzard1230II_data = {
> + .name = "Blizzard 1230II",
> + .offset = 0x1,
> + .dma_offset = 0x10021,
> + .absolute = 0,
> + .scsi_option = 1,
> +};
> +
> +static struct zorro_driver_data fastlanez3_data = {
> + .name = "Fastlane",
> + .offset = 0x101,
> + .dma_offset = 0x141,
> + .absolute = 0,
> + .scsi_option = 0,
> +};
> +
> +static struct zorro_device_id zorro_esp_zorro_tbl[] = {
> + {
> + .id = ZORRO_PROD_PHASE5_BLIZZARD_1220_CYBERSTORM,
> + .driver_data = (unsigned long)_data,
> + },
> + {
> + .id = ZORRO_PROD_PHASE5_CYBERSTORM_MK_II,
> + .driver_data = (unsigned long)_data,
> + },
> + {
> + .id = ZORRO_PROD_PHASE5_BLIZZARD_2060,
> + .driver_data = (unsigned long)_data,
> + },
> + {
> + .id = ZORRO_PROD_PHASE5_BLIZZARD_1230_IV_1260,
> + .driver_data = (unsigned long)_data,
> + },
> + {
> + .id = 
> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
> + .driver_data = (unsigned long)_data,
> + },
> + { 0 }
> +};
> +MODULE_DEVICE_TABLE(zorro, zorro_esp_zorro_tbl);
> +
> +/* Blizzard 1230 DMA interface */
> +
> +struct blz1230_dma_registers {
> + volatile unsigned char dma_addr;/* DMA address  [0x] */
> + unsigned char dmapad2[0x7fff];
> + volatile unsigned char dma_latch;   /* DMA latch[0x8000] */
> +};
> +
> +/* Blizzard 1230II DMA interface */
> +
> +struct blz1230II_dma_registers {
> + volatile unsigned char dma_addr;/* DMA address  [0x] */
> + unsigned char dmapad2[0xf];
> + volatile unsigned char dma_latch;   /* DMA latch[0x0010] */
> +};
> +
> +/* Blizzard 2060 DMA interface */
> +
> +struct blz2060_dma_registers {
> + volatile unsigned char dma_led_ctrl;/* DMA led control   [0x000] */
> + unsigned char dmapad1[0x0f];
> + volatile unsigned char dma_addr0;   /* DMA address (MSB) [0x010] */
> + 

[PATCH v3] m68k/amiga - Amiga Zorro NCR53C9x boards: new zorro_esp.c

[Michael Schmitz]

From: Michael Schmitz 

New combined SCSI driver for all ESP based Zorro SCSI boards for
m68k Amiga.

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

New Kconfig option and Makefile entries for new Amiga Zorro ESP SCSI
driver included in this patch.

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. 

Signed-off-by: Michael Schmitz 

---

Changes since v1:

Fixed issues raised by Finn Thain in initial code review:

- use KBUILD_MODNAME for driver name string.
- use pr_fmt() for pr_* format prefix.
- clean up DMA error reporting: clear error flag before each DMA
  operation, set error flag on PIO error. Don't test phase in dma_err hook.
- change confusing comment about semantics of read flag, add comments
  indicating DMA direction to DMA setup hooks.
- drop spurious braces around switch clauses.
- lift cfreq setting out of ID switch clauses.
- fix indentation.
- fix error codes on probe fail.
- drop check for board ID when unmapping DMA regs in error handling:
  the ioaddr > 0xff test already catches all cases where the DMA
  registers were ioremapped.
- dynamically alloc zorro_private_data.
- fix use of driver_data field (don't mix host and zorro_esp_priv
  pointers). Note: require esp pointer in zorro_esp_priv to find host
  pointer!
- back out phase bits changes to pio_cmd !write branch introduced
  to cope with ESP SELAS command. We don't use that code so keep
  it in sync with Finn's version.
- use esp_ops.dma_length_limit() to limit transfer size. After review
  of old driver code, use 0xff max transfer size throughout.

Fixed issues raised by Geert Uytterhoven:

- dynamically alloc zorro_private_data, store as device drvdata.
- store ctrl_data for CyberStormI in driver private data.
- use dma_sync_single_for_device() instead of cache_push/clear.
- handle case of duplicate board identity - check whether board is
  Zorro III or Zorro II (use ROM resource data for this). Also fix
  up DMA mask for Zorro II boards to 32 bits (these are really CPU
  expansion slot boards).
- remove zorro3 field from driver_data struct (now in private data).
- add braces around ambiguous if - else construct.
- use named structs instead of array for board config data.
- use scsi_option driver data flag for boards with optional ESP.

Other improvements and bugfixes

- fix Zorro device table error (duplicate ID used, also raised
  by Kars de Jong).
- error code fixup in error handling path.
- add separate DMA setup for Blizzard 1230 II board.
- add support for Cyberstorm II board.
- add register structs and DMA setup for Zorro III Fastlane board,
  following logic from old fastlane.c driver. Wire up Fastlane DMA
  and interrupt status routines, map the necessary low 24 bit board
  address space used for DMA target address setting. Clean up DMA
  register space ioremap() branch for Zorro III boards (currently
  Fastlane only) to end confusion about what to do in error recovery.
- use esp_ops.fastlane_esp_dma_invalidate() on Fastlane (and skip
  fastlane_esp_reset_dma() in DMA setup).
- credit Tuomas Vainikka for contributing Blizzard 1230 code (and
  testing).
- clarify comment about unsupported Oktagon SCSI board.
- remove unused const definitions carried over from old driver.

Changes since v2:

Issues raised by Finn Thain: 

- add SPDX-License-Identifier.
- remove unused ratelimit.h.
- drop phys_to_virt() in PIO transfer routine, after ensuring PIO is only
  used for message in transfers to esp->command_block. This obviates any
  need for finding the virtual address corresponding to a DMA handle.
- drop BUG_ON(!(cmd & ESP_CMD_DMA)) assertion in DMA setup. Short of changes
  to the core ESP driver, this can never trigger.
- make ioremap() of DMA address range conditional on zep->zorro3 and use
  that same condition to unmap in error handling and driver exit.
  Omit board ID test as we only support a single Zorro III board, and add
  comment on what to do when adding support for more boards.
- free driver private data in driver exit.
- various whitespace related cleanup.

---
 drivers/scsi/Kconfig |   16 +
 drivers/scsi/Makefile|1 +
 drivers/scsi/zorro_esp.c | 1136