Re: [PATCH v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[John Paul Adrian Glaubitz]

On 04/11/2018 10:55 AM, Michael Schmitz wrote:

Would be nice but this has been chugging along for over four years in my
queue so I'm sure it can wait a few more weeks :-)


But the driver works very well now, so it was worth the wait ;-).

--
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

Hi Geert, Christoph,

Am 11.04.2018 um 20:30 schrieb Geert Uytterhoeven:
> Hi Christoph,
> 
> On Wed, Apr 11, 2018 at 10:11 AM, Christoph Hellwig  
> wrote:
>> On Wed, Apr 11, 2018 at 10:03:12AM +0200, Geert Uytterhoeven wrote:
 That would be cool. Would that still be in time for the 4.17 merge?
>>>
>>> Nope, as new drivers need to be in linux-next before the merge window opens.
>>
>> I always throught that entirely new drivers were an exception to that.
> 
> Aren't you confusing this with the new device ID rules for stable?
> 
> It's debatable, and thus up to the maintainer...

Would be nice but this has been chugging along for over four years in my
queue so I'm sure it can wait a few more weeks :-)

Cheers,

Michael

Re: [PATCH v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

Hi Geert,

Am 11.04.2018 um 18:51 schrieb Geert Uytterhoeven:
> Hi Michael,
> 
> On Tue, Apr 10, 2018 at 11:50 PM, Michael Schmitz  
> wrote:
 Short of a complete rewrite of the Zorro driver support code to be
 closer to what PCI does, I don' see what can be done about the use of
 Zorro IDs. I don't think such a rewrite is planned in the near future,
 Geert?
>>>
>>> I think what Christoph means is the use of the define
>>> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060
>>> versus hardcoded numbers, or ZORRO_ID(PHASE5, 0x0B, 0).
>>>
>>> We have a long list of ZORRO_PROD_* definitions in
>>> include/uapi/linux/zorro_ids.h because of historical reasons.  The list
>>> isn't really changing (no new IDs in git history) due to almost no new
>>> Zorro boards being made, unlike for PCI, where keeping an in-kernel list
>>> is a lot of work, and not desirable.
>>
>> now I see, thanks.
>>
>> I could change the device table to use ZORRO_ID(PHASE5, ...) style IDs
>> instead of the longish defines if you're OK with that.
> 
> I don't have a preference. If you think it makes the driver easier to read,
> go for it.

It does get rid of a few 'line over 80 characters' warnings from
checkpatch, and with comments in the device table describing what the
IDs mean, it isn't any less readable. Let's see ...

> Note that we can't get rid of the longish defines anyway, as they're in a
> uapi header file, so you're free to keep on using it.

I don't mind these defines remaining in the header file at all - the
only place they are now used is in the device table, and hardcoded IDs
are as good as the defines there, I think.

Have a look at the result in v8 - I can easily revert to the version
with the defines if that's the consensus, it's all in git after all.

Cheers,

Michael

Re: [PATCH v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Geert Uytterhoeven]

Hi Christoph,

On Wed, Apr 11, 2018 at 10:11 AM, Christoph Hellwig  wrote:
> On Wed, Apr 11, 2018 at 10:03:12AM +0200, Geert Uytterhoeven wrote:
>> > That would be cool. Would that still be in time for the 4.17 merge?
>>
>> Nope, as new drivers need to be in linux-next before the merge window opens.
>
> I always throught that entirely new drivers were an exception to that.

Aren't you confusing this with the new device ID rules for stable?

It's debatable, and thus up to the maintainer...

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 v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Christoph Hellwig]

On Wed, Apr 11, 2018 at 10:03:12AM +0200, Geert Uytterhoeven wrote:
> > That would be cool. Would that still be in time for the 4.17 merge?
> 
> Nope, as new drivers need to be in linux-next before the merge window opens.

I always throught that entirely new drivers were an exception to that.

Re: [PATCH v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Geert Uytterhoeven]

Hi Adrian,

On Wed, Apr 11, 2018 at 9:59 AM, John Paul Adrian Glaubitz
 wrote:
> On 04/11/2018 08:51 AM, Geert Uytterhoeven wrote:
>> I don't have a preference. If you think it makes the driver easier to
>> read,
>> go for it.
>
> That would be cool. Would that still be in time for the 4.17 merge?

Nope, as new drivers need to be in linux-next before the merge window opens.

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 v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[John Paul Adrian Glaubitz]

On 04/11/2018 08:51 AM, Geert Uytterhoeven wrote:

I don't have a preference. If you think it makes the driver easier to read,
go for it.


That would be cool. Would that still be in time for the 4.17 merge?

Adrian

--
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Geert Uytterhoeven]

Hi Michael,

On Tue, Apr 10, 2018 at 11:50 PM, Michael Schmitz  wrote:
> On Tue, Apr 10, 2018 at 8:18 PM, Geert Uytterhoeven
>  wrote:
>> On Tue, Apr 10, 2018 at 4:16 AM, Michael Schmitz  
>> wrote:
>>> On Mon, Apr 9, 2018 at 7:50 PM, Christoph Hellwig  
>>> wrote:
 On Sun, Apr 08, 2018 at 02:45:32PM +1200, Michael Schmitz wrote:
> New combined SCSI driver for all ESP based Zorro SCSI boards for
> m68k Amiga.
> + {
> + .id = 
> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,

 In PCI Land we've usually stopped using PCI IDs unless they are used
 in multiple
>>
>> (missing "places"?)
>>
>>> Short of a complete rewrite of the Zorro driver support code to be
>>> closer to what PCI does, I don' see what can be done about the use of
>>> Zorro IDs. I don't think such a rewrite is planned in the near future,
>>> Geert?
>>
>> I think what Christoph means is the use of the define
>> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060
>> versus hardcoded numbers, or ZORRO_ID(PHASE5, 0x0B, 0).
>>
>> We have a long list of ZORRO_PROD_* definitions in
>> include/uapi/linux/zorro_ids.h because of historical reasons.  The list
>> isn't really changing (no new IDs in git history) due to almost no new
>> Zorro boards being made, unlike for PCI, where keeping an in-kernel list
>> is a lot of work, and not desirable.
>
> now I see, thanks.
>
> I could change the device table to use ZORRO_ID(PHASE5, ...) style IDs
> instead of the longish defines if you're OK with that.

I don't have a preference. If you think it makes the driver easier to read,
go for it.

Note that we can't get rid of the longish defines anyway, as they're in a
uapi header file, so you're free to keep on using it.

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 v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

Hi Geert,

now I see, thanks.

I could change the device table to use ZORRO_ID(PHASE5, ...) style IDs
instead of the longish defines if you're OK with that.

The other changes have passed tests and I'd otherwise just send what I
have now, adding Christoph's Reviewed-by.

Cheers,

  Michael


On Tue, Apr 10, 2018 at 8:18 PM, Geert Uytterhoeven
 wrote:
> Hi Michael,
>
> On Tue, Apr 10, 2018 at 4:16 AM, Michael Schmitz  wrote:
>> On Mon, Apr 9, 2018 at 7:50 PM, Christoph Hellwig  wrote:
>>> On Sun, Apr 08, 2018 at 02:45:32PM +1200, Michael Schmitz wrote:
 New combined SCSI driver for all ESP based Zorro SCSI boards for
 m68k Amiga.
 + {
 + .id = 
 ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
>>>
>>> In PCI Land we've usually stopped using PCI IDs unless they are used
>>> in multiple
>
> (missing "places"?)
>
>> Short of a complete rewrite of the Zorro driver support code to be
>> closer to what PCI does, I don' see what can be done about the use of
>> Zorro IDs. I don't think such a rewrite is planned in the near future,
>> Geert?
>
> I think what Christoph means is the use of the define
> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060
> versus hardcoded numbers, or ZORRO_ID(PHASE5, 0x0B, 0).
>
> We have a long list of ZORRO_PROD_* definitions in
> include/uapi/linux/zorro_ids.h because of historical reasons.  The list
> isn't really changing (no new IDs in git history) due to almost no new
> Zorro boards being made, unlike for PCI, where keeping an in-kernel list
> is a lot of work, and not desirable.
>
> 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 v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Geert Uytterhoeven]

Hi Michael,

On Tue, Apr 10, 2018 at 4:16 AM, Michael Schmitz  wrote:
> On Mon, Apr 9, 2018 at 7:50 PM, Christoph Hellwig  wrote:
>> On Sun, Apr 08, 2018 at 02:45:32PM +1200, Michael Schmitz wrote:
>>> New combined SCSI driver for all ESP based Zorro SCSI boards for
>>> m68k Amiga.
>>> + {
>>> + .id = 
>>> ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060,
>>
>> In PCI Land we've usually stopped using PCI IDs unless they are used
>> in multiple

(missing "places"?)

> Short of a complete rewrite of the Zorro driver support code to be
> closer to what PCI does, I don' see what can be done about the use of
> Zorro IDs. I don't think such a rewrite is planned in the near future,
> Geert?

I think what Christoph means is the use of the define
ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060
versus hardcoded numbers, or ZORRO_ID(PHASE5, 0x0B, 0).

We have a long list of ZORRO_PROD_* definitions in
include/uapi/linux/zorro_ids.h because of historical reasons.  The list
isn't really changing (no new IDs in git history) due to almost no new
Zorro boards being made, unlike for PCI, where keeping an in-kernel list
is a lot of work, and not desirable.

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 v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Michael Schmitz]

Hi Christoph,

thanks for your review.

Short of a complete rewrite of the Zorro driver support code to be
closer to what PCI does, I don' see what can be done about the use of
Zorro IDs. I don't think such a rewrite is planned in the near future,
Geert?

I can certainly use board type enums as index into a driver data (or
feature) struct array made up from the current board config data. Adds
another level of indirection while still keeping the probe code
readable. Even allows to drop the last use of these long zorro_id
macros in the probe function, and that's a clear win.
I just wouldn't want to revert to patching up bits of config data
based on board type later - we had arrived at the current code after
quite some review with input from Finn and Geert.

May take a while to test any changes though (my test box appears to be
on the move at present).

Cheers,

  Michael



On Mon, Apr 9, 2018 at 7:50 PM, Christoph Hellwig  wrote:
> On Sun, Apr 08, 2018 at 02:45:32PM +1200, Michael Schmitz wrote:
>> 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. esp_reselect_with_tag explicitly sets esp->cmd_block_dma as
>> target address for the message bytes but PIO requires a virtual address.
>> Substiute kernel virtual address esp->cmd_block in PIO transfer call if
>> DMA address is esp->cmd_block_dma and phase is message in.
>>
>> PIO code taken from mac_esp.c where the reselection timeout issue was
>> debugged and fixed first, with minor macro and function rename.
>>
>> Signed-off-by: Michael Schmitz 
>> Reviewed-by: Finn Thain 
>>
>> ---
>>
>> 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() 

Re: [PATCH v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Geert Uytterhoeven]

Hi Christoph,

On Mon, Apr 9, 2018 at 9:50 AM, Christoph Hellwig  wrote:
> On Sun, Apr 08, 2018 at 02:45:32PM +1200, Michael Schmitz wrote:
>> --- /dev/null
>> +++ b/drivers/scsi/zorro_esp.c

>> + .driver_data = (unsigned long)_data,
>
> What most (or at least many) drivers do in PCI land is to just use
> an enum of types in the driver_Data field and use that as an index for
> for decisions later.  It seems like that might be the cleaner method
> here as well.

That may work fine for a small number of differences.
For more differences, the pointer to the feature struct is what most DT
drivers use (of_device_id.data is a const void *).

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 v7] scsi: new zorro_esp.c for Amiga Zorro NCR53C9x boards

[Christoph Hellwig]

On Sun, Apr 08, 2018 at 02:45:32PM +1200, Michael Schmitz wrote:
> 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. esp_reselect_with_tag explicitly sets esp->cmd_block_dma as
> target address for the message bytes but PIO requires a virtual address.
> Substiute kernel virtual address esp->cmd_block in PIO transfer call if
> DMA address is esp->cmd_block_dma and phase is message in.
> 
> PIO code taken from mac_esp.c where the reselection timeout issue was
> debugged and fixed first, with minor macro and function rename.
> 
> Signed-off-by: Michael Schmitz 
> Reviewed-by: Finn Thain 
> 
> ---
> 
> 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:
> 
> - 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