Hi Finn,
On Sun, Mar 11, 2018 at 4:58 PM, Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx> wrote:
FYI, the 'To:' header was empty, so this will need to be resubmitted
eventually.
Oops, didn't notice I mangled the git send-email command line so
badly. I'll respin.
On Sun, 11 Mar 2018, Michael Schmitz wrote:
From: Michael Schmitz <schmitz@xxxxxxxxxx>
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. Use phys_to_virt()
to determine kernel virtual address in all other cases (this works on m68k).
You should put a '---' separator here, to keep the change log out of the
commit log. The signed-off-by tags etc, which appear in the commit log, go
above the separator.
I've added a few more comments below.
Please go ahead and add my reviewed-by tag if you wish.
Reviewed-by: Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx>
Thanks, will do.
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.
Still to do:
- avoid BUG_ON() in DMA setup. In order to downgrade tp WARN_ON(), I
need to verify dma_error=1 will completely abort the affected command
and run error recivery. Can anyone confirm this?
I see that all the other esp drivers (except mac_scsi) use BUG_ON(!(cmd &
ESP_CMD_DMA)). Maybe it is a good idea; I don't know.
I can't see how WARN_ON will prevent data corruption so I think a WARN_ON
"downgrade" is questionable and I retract what I said about doing so here.
I was hoping from a yay or nay from Dave. The other drivers' BUG_ON()
predate Linus' strong words on the matter so I'd expect these to go
away eventually.
I'll take a closer look at esp_scsi.c.
As per your later mail, it looks as though there's no possible way
(short of changes to the core) that this can trigger. I'll remove the
assertions.
diff --git a/drivers/scsi/zorro_esp.c b/drivers/scsi/zorro_esp.c
new file mode 100644
index 0000000..4b544aa
--- /dev/null
+++ b/drivers/scsi/zorro_esp.c
@@ -0,0 +1,1142 @@
The SPDX-License-Identifier goes here. See
Documentation/process/license-rules.rst
Saw that in Adrian's libata thread but it slipped my mind...
+/* zorro_esp.c: ESP front-end for Amiga ZORRO SCSI systems.
+ *
+ * Copyright (C) 1996 Jesper Skov (jskov@xxxxxxxxxxxx)
+ *
+ * Copyright (C) 2011,2018 Michael Schmitz (schmitz@xxxxxxxxxx) for
+ * migration to ESP SCSI core
+ *
+ * Copyright (C) 2013 Tuomas Vainikka (tuomas.vainikka@xxxxxxxx) for
+ * Blizzard 1230 DMA and probe function fixes
+ *
+ * Copyright (C) 2017 Finn Thain (fthain@xxxxxxxxxxxxxxxxxxx) for
+ * PIO code from Mac ESP driver adapted here
You can omit my email address. I've been removing it from source files so
I only have to keep the MAINTAINERS file current. The git commit logs have
email addresses.
Will do.
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/ratelimit.h>
Unused header?
Quite possibly.
+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
I think it's good style to put a comma after the last initializer, so that
any patch to update the struct doesn't churn unaffected lines.
Thanks, hadn't thought of that aspect.
+/* DMA control bits */
+#define FASTLANE_DMA_FCODE 0xa0
+#define FASTLANE_DMA_MASK 0xf3
+#define FASTLANE_DMA_WRITE 0x08 /* 1 = write */
+#define FASTLANE_DMA_ENABLE 0x04 /* Enable DMA */
+#define FASTLANE_DMA_EDI 0x02 /* Enable DMA IRQ ? */
+#define FASTLANE_DMA_ESI 0x01 /* Enable SCSI IRQ */
It might be nice to line up those comments.
Tabs got lost in transit ... I've readded them now.
+ /* We are passed DMA addresses i.e. physical addresses, but must use
+ * kernel virtual addresses here, so convert 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 (it should on m68k).
+ */
+ if (addr == esp->command_block_dma)
+ addr = (u32) esp->command_block;
+ else {
+ u32 orig_addr = addr;
+
+ addr = (u32) phys_to_virt(addr);
+ pr_warn("mapped DMA address %lx to virt %p (phys %p)\n",
+ orig_addr, addr, virt_to_phys(addr));
You could remove this branch, if you call zorro_esp_send_pio_cmd or avoid
calling it, conditional on dma address. E.g.
#define AVOID_DMA(esp, addr) (((esp->sreg & ESP_STAT_PMASK) == ESP_MIP) && \
(addr == esp->command_block_dma))
...
if (AVOID_DMA(esp, addr) {
zorro_esp_send_pio_cmd(esp,...);
return;
}
That way this decision is made entirely on a card-by-card basis, in case
different implementations have different issues, e.g. some might need to
check dma_count.
This is just a suggestion. I don't mind either way. Geert may have
something to say about this code anyway. In which case, please disregard
my suggestion.
I got the impression Geert didn't quite share my confidence in
phys_to_virt() doing the Right Thing here, so best avoid ever hitting
that branch. Will probably retain the full conditional above to allow
per-board tweaks.
+ if (write) {
+ *ctrl_data = (*ctrl_data & FASTLANE_DMA_MASK) |
+ FASTLANE_DMA_ENABLE;
+
Excess blank line?
Well spotted ...
+ if (ioaddr > 0xffffff) {
+ /* I guess the Fastlane Z3 will be the only one to catch this? */
+ if (ent->id == ZORRO_PROD_PHASE5_BLIZZARD_1230_II_FASTLANE_Z3_CYBERSCSI_CYBERSTORM060)
Shouldn't this match the conditionals used in the iounmap calls below?
I'll change this conditional to 'if (zep->zorro3)' instead. ioaddr >
0xffffff is a necessary condition for zep->zorro3 == 1.
Regarding the board ID test, I could just as well skip that for now,
because the Fastlane is the only Zorro III board we support. Should
there ever be other boards, they will all ioremap their DMA register
space here. ID test only serves to map a sufficient number of bytes.
I've removed the ID test, and added a comment to that effect.
+ esp->dma_regs = ioremap_nocache(dmaaddr,
+ sizeof(struct fastlane_dma_registers));
+ } else
+ esp->dma_regs = (void __iomem *)ZTWO_VADDR(dmaaddr);
+
+ if (!esp->dma_regs) {
+ err = -ENOMEM;
+ goto fail_unmap_regs;
+ }
+
+ esp->command_block = dma_alloc_coherent(esp->dev, 16,
+ &esp->command_block_dma,
+ GFP_KERNEL);
+
+fail_unmap_fastlane:
+ if (zep->zorro3)
Also, this conditional doesn't match the one below.
zep->zorro3 and ioaddr > 0xffffff are synonymous. But yes, it's
confusing so I've changed the remove code to match.
+ zorro_release_device(z);
I think you need kfree(zep) here.
Yup.
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
