Hi Christoph,
thank you for taking the time to review and comment - my responses
inline below.
Am 03.04.2018 um 19:45 schrieb Christoph Hellwig:
Just a few style comments:
+static int zorro_esp_irq_pending(struct esp *);
+static int cyber_esp_irq_pending(struct esp *);
+static int fastlane_esp_irq_pending(struct esp *);
Please avoid forward declarations wherever possible.
+struct zorro_driver_data {
+ const char *name;
+ unsigned long offset;
+ unsigned long dma_offset;
+ int absolute; /* offset is absolute address */
+ int scsi_option;
+ int (*irq_pending)(struct esp *esp);
+ void (*dma_invalidate)(struct esp *esp);
+ void (*send_dma_cmd)(struct esp *esp, u32 dma_addr, u32 esp_count,
+ u32 dma_count, int write, u8 cmd);
+};
Please use different esp_driver_ops instances for the different board
types.
+static const struct zorro_driver_data cyberstormI_data = {
+ .name = "CyberStormI",
+ .offset = 0xf400,
+ .dma_offset = 0xf800,
+ .absolute = 0,
+ .scsi_option = 0,
You can remove all the xero initializations on static data.
Also please align the = signs with tabs in struct declarations.
Will do all that.
+static dma_addr_t zorro_esp_map_single(struct esp *esp, void *buf,
+ size_t sz, int dir)
+{
+ return dma_map_single(esp->dev, buf, sz, dir);
+}
+
+static int zorro_esp_map_sg(struct esp *esp, struct scatterlist *sg,
+ int num_sg, int dir)
+{
+ return dma_map_sg(esp->dev, sg, num_sg, dir);
+}
+
+static void zorro_esp_unmap_single(struct esp *esp, dma_addr_t addr,
+ size_t sz, int dir)
+{
+ dma_unmap_single(esp->dev, addr, sz, dir);
+}
+
+static void zorro_esp_unmap_sg(struct esp *esp, struct scatterlist *sg,
+ int num_sg, int dir)
+{
+ dma_unmap_sg(esp->dev, sg, num_sg, dir);
+}
These wrappers seem rather pointless.
They are nonetheless necessary, for two reasons as far as I can see:
ESP driver DMA ops hooks as defined in esp_scsi.h use struct esp * as
first parameter, not struct dev *. Changing that would require changing
all ESP based drivers (sun_esp.c, jazz_esp.c, am53c974.c, sun3_esp.c,
mac_esp.c) and the driver core. I'd like to hear Dave's view on that.
The generic dma_map_* and dma_unmap_* ops are defined as inline
functions in dma-mapping.h. There may be a good reason for that choice.
You probably know better than me what the reason for inlining was.
All other ESP drivers use the same wrapper mechanism, and most of them
don't do more than dereference the struct device pointer and pass that
and the rest of parameters on to the generic or PCI API hooks. I see no
reason to deviate from that convention.
+/* PIO macros as used in mac_esp.c.
+ * Note that addr and fifo arguments are local-scope variables declared
+ * in zorro_esp_send_pio_cmd(), the macros are only used in that function,
+ * and addr and fifo are referenced in each use of the macros so there
+ * is no need to pass them as macro parameters.
+ */
Please use normal Linux comment style:
/*
* Blah, blah, blah.
*/
Thanks, done.
+#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)); \
+ }
What is the point of the curly braces around the asm statement?
My overeager attempt at shutting up checkpatch ('need braces around
complex macro'). I'll revert to what Finn used in mac_esp.c (which is
where this code was taken from). That did pass review at the time, hope
this counts for something.
+static void zorro_esp_send_pio_cmd(struct esp *esp, u32 addr, u32 esp_count,
+ u32 dma_count, int write, u8 cmd)
+{
+ struct zorro_esp_priv *zep = dev_get_drvdata(esp->dev);
+ u8 __iomem *fifo = esp->regs + ESP_FDATA * 16;
+ u8 phase = esp->sreg & ESP_STAT_PMASK;
+
+ cmd &= ~ESP_CMD_DMA;
+
+ if (write) {
It seems like this routine would benefit from being split into a read
and a write one.
The ESP driver uses just a single send_dma_cmd() function, so we'd have
to conditionalize which of the two PIO functions to call from within
each board specific send_dma_cmd(). IMO that makes the code harder to
follow. (I wouldn't go as far as Finn and add another layer of
indirection, but the impact is much the same.)
Finn also provided another justification for using a single routine for
read and write - we plan to share that code between mac_esp.c and
zorro_esp.c so I'd rather keep it as close to the mac_esp.c version as
possible.
+// Blizzard 1230/60 SCSI-IV DMA
/* ... */
+ /* Use PIO if transferring message bytes to esp->command_block_dma.
+ * PIO requires a virtual address, so substitute esp->command_block
+ * for addr.
+ */
Comment style, again.
+static struct esp_driver_ops zorro_esp_ops = {
should be marked const.
Will be when I use per-board esp_driver_ops.
+ .esp_write8 = zorro_esp_write8,
Just use a space after the '='.
+
+ if (zep->zorro3) {
+ /* Only Fastlane Z3 for now - add switch for correct struct
+ * dma_registers size if adding any more
+ */
+ esp->dma_regs = ioremap_nocache(dmaaddr,
+ sizeof(struct fastlane_dma_registers));
+ } else
+ esp->dma_regs = (void __iomem *)ZTWO_VADDR(dmaaddr);
doesn't this need a __force (and a comment on why it is ѕafe) to make
sparse happy?
Not with my (admittedly rather dated) version of sparse. But according
to the definition of ZTWO_ADDR:
#define zTwoBase (0x80000000)
#define ZTWO_VADDR(x) ((void __iomem *)(((unsigned long)(x))+zTwoBase))
the cast itself here looks redundant.
As to why this is safe - all of the Zorro-II address space is mapped
non-cacheable at offset zTwoBase during early startup (in
arch/m68k/kernel/head.S, look for L(mmu_init_amiga)). I'll add a comment
to that effect.
Finn has suggested making the zep->board base type void __iomem * to
shut up sparse - done that now, I'll just have to test the changes
before submitting v6...
Thanks again!
Cheers,
Michael
--
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
