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 <schmitzmic@xxxxxxxxx>
> Reviewed-by: Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx>
>
> ---
>
> 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 > 0xffffff 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 0xffffff 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 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.
>
> Changes since v3:
>
> Finn Thain:
> - substitute esp->command_block for transfer address in each board DMA
> set-up, instead of in PIO transfer function.
> - use kzalloc for struct zorro_esp_priv.
> - clarify comment regarding selecting between Blizzard 1230 II or
> board (both share the ID with the Fastlane board).
> - use dev_get|set_drvdata instead of zorro_get|set_drvdata wrapper.
> - remove redundant comments.
>
> Geert Uytterhoeven:
> - use readb()/writeb() to access DMA registers, obviating the need for
> declaring register structs volatile.
> - use const for board config and driver data table.
> - drop ZORRO_ESP_GET_PRIV macro.
> - add comments to clarify use of addr and fifo variables in PIO macros.
> - add braces to Zorro II branch (DMA mask fixup).
> - define board ops irq_pending, dma_invalidate and send_dma_cmd via
> function pointers in driver data.
>
> Tuomas Vainikka:
> - fix wrong config description in Kconfig (from NCR7xx driver).
>
> Changes since v4:
>
> - change tag line
>
> Kars de Jong:
> - fix misspelling of Blizzard 1230 as 1320 in config description
>
> Changes since v5:
>
> Christoph Hellwig:
> - fix comment style
> - drop initialization to zero in driver data init
> - fix alignment in struct declarations
> - drop braces around asm macros
> - change board_base type to void __iomem *
> - drop unneeded void __iomem * cast from ZTWO_VADDR() use
> - add comment to explain why ZTWO_VADDR() use is safe
> - move board specific functions to per-board esp_ops
>
> Changes since v6:
>
> Finn Thain:
> - use z_writel to avoid sparse warnings in Fastlane DMA functions
>
> ---
> drivers/scsi/Kconfig | 14 +
> drivers/scsi/Makefile | 1 +
> drivers/scsi/zorro_esp.c | 1166 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1181 insertions(+), 0 deletions(-)
> create mode 100644 drivers/scsi/zorro_esp.c
>
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index 8a739b7..94eb3f3 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -1462,6 +1462,20 @@ config SCSI_ZORRO7XX
> accelerator card for the Amiga 1200,
> - the SCSI controller on the GVP Turbo 040/060 accelerator.
>
> +config SCSI_ZORRO_ESP
> + tristate "Zorro ESP SCSI support"
> + depends on ZORRO && SCSI
> + select SCSI_SPI_ATTRS
> + help
> + Support for various NCR53C9x (ESP) based SCSI controllers on Zorro
> + expansion boards for the Amiga.
> + This includes:
> + - the Phase5 Blizzard 1230 II and IV SCSI controllers,
> + - the Phase5 Blizzard 2060 SCSI controller,
> + - the Phase5 Blizzard Cyberstorm and Cyberstorm II SCSI
> + controllers,
> + - the Fastlane Zorro III SCSI controller.
> +
> config ATARI_SCSI
> tristate "Atari native SCSI support"
> depends on ATARI && SCSI
> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
> index de1b3fc..20992d7 100644
> --- a/drivers/scsi/Makefile
> +++ b/drivers/scsi/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_INFINIBAND_ISER) += libiscsi.o
> obj-$(CONFIG_ISCSI_BOOT_SYSFS) += iscsi_boot_sysfs.o
> obj-$(CONFIG_SCSI_A4000T) += 53c700.o a4000t.o
> obj-$(CONFIG_SCSI_ZORRO7XX) += 53c700.o zorro7xx.o
> +obj-$(CONFIG_SCSI_ZORRO_ESP) += esp_scsi.o zorro_esp.o
> obj-$(CONFIG_A3000_SCSI) += a3000.o wd33c93.o
> obj-$(CONFIG_A2091_SCSI) += a2091.o wd33c93.o
> obj-$(CONFIG_GVP11_SCSI) += gvp11.o wd33c93.o
> diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
> new file mode 100644
> index 0000000..9fae980
> --- /dev/null
> +++ b/drivers/scsi/zorro_esp.c
> @@ -0,0 +1,1166 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * zorro_esp.c: ESP front-end for Amiga ZORRO SCSI systems.
We usually avoid mentioning the file name of the actual file in
comments.
> + {
> + .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
> + .driver_data = (unsigned long)&blizzard1230II_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.
Except for these nitpicks this looks great to me:
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
