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

[Michael Schmitz]

Hi Finn,

Am 07.04.2018 um 22:19 schrieb Finn Thain:
> On Sat, 7 Apr 2018, Michael Schmitz wrote:
> 
>>>
>>>   CHECK   /home/fthain/src/kernel.org/linux/drivers/scsi/zorro_esp.c
>>> drivers/scsi/zorro_esp.c:274:14: warning: cast removes address space of 
>>> expression
>>> drivers/scsi/zorro_esp.c:712:14: warning: cast removes address space of 
>>> expression
>>>
>>> Why not just call writel() and get rid of the 't' temporary pointer?
>>
>> As far as I can see, that will invoke cpu_to_le32() on the data written.
> 
> Good point.
> 
> Since this is zorro card driver, why not use z_writel()?

Equally good point - that's what it's there for.

Cheers,

Michael

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

[Michael Schmitz]

Hi Finn,

Am 07.04.2018 um 12:59 schrieb Finn Thain:
> On Sat, 7 Apr 2018, Michael Schmitz wrote:
> 
>> 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
>>
> 
> The sparse warnings have changed to this:
> 
>   CHECK   /home/fthain/src/kernel.org/linux/drivers/scsi/zorro_esp.c
> drivers/scsi/zorro_esp.c:274:14: warning: cast removes address space of 
> expression
> drivers/scsi/zorro_esp.c:712:14: warning: cast removes address space of 
> expression
> 
> Why not just call writel() and get rid of the 't' temporary pointer?

As far as I can see, that will invoke cpu_to_le32() on the data written.
Geert?

Rewriting line this

*((__force unsigned long *)zep->board_base) = 0;

and this

*(__force unsigned long *)((addr & 0x00ff) + zep->board_base) = addr;

does suppress the warning and omits the temp pointer. But is that any
better?

Cheers,

Michael

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

[Finn Thain]

On Sat, 7 Apr 2018, Michael Schmitz wrote:

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

The sparse warnings have changed to this:

  CHECK   /home/fthain/src/kernel.org/linux/drivers/scsi/zorro_esp.c
drivers/scsi/zorro_esp.c:274:14: warning: cast removes address space of 
expression
drivers/scsi/zorro_esp.c:712:14: warning: cast removes address space of 
expression

Why not just call writel() and get rid of the 't' temporary pointer?

-- 

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

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