Re: DMA support for SCSI adapter of AV Mac

rin@ wrote:

> I updated the patch for DMA support for SCSI adapter of AV Mac
> (Quadra / Centris 660AV / 840AV), written by Michael Zucca in
> PR port-mac68k/24883.
> 
> http://www.netbsd.org/~rin/avdma_20181002.patch
 :
> http://gnats.netbsd.org/24883

I'd like to note some dumb comments:

> --- sys/arch/mac68k/include/psc.h	11 Dec 2005 12:18:03 -0000	1.7
> +++ sys/arch/mac68k/include/psc.h	2 Oct 2018 06:38:51 -0000
> @@ -25,6 +25,8 @@
>   *
>   */
>  
> +#include <machine/bus.h>
> +

Nowadays <sys/bus.h> is better, but I wonder if it should be included
in C source. (see below)

>  --- sys/arch/mac68k/obio/esp.c	18 Feb 2012 23:51:27 -0000	1.55
>  +++ sys/arch/mac68k/obio/esp.c	2 Oct 2018 06:45:18 -0000
>  @@ -90,6 +94,7 @@ __KERNEL_RCSID(0, "$NetBSD: esp.c,v 1.55
>  #include <sys/proc.h>
>  #include <sys/queue.h>
>  #include <sys/mutex.h>
> +#include <sys/malloc.h>

Is this necessary? (no malloc(9) but only bus_dmamem_alloc(9))
 
> @@ -103,6 +108,10 @@ __KERNEL_RCSID(0, "$NetBSD: esp.c,v 1.55
>  #include <dev/ic/ncr53c9xreg.h>
>  #include <dev/ic/ncr53c9xvar.h>
>  
> +#include <uvm/uvm_extern.h>
> +
> +#include <machine/bus.h>
> +#include <machine/psc.h>

<sys/bus.h> and <machine/psc.h> as noted above?
 
> @@ -134,6 +143,16 @@ int	esp_quick_dma_setup(struct ncr53c9x_
>  	     size_t *);
>  void	esp_quick_dma_go(struct ncr53c9x_softc *);
>  
> +void	esp_av_dma_reset(struct ncr53c9x_softc *);
> +void	esp_av_dma_stop(struct ncr53c9x_softc *);
> +void	esp_av_write_reg(struct ncr53c9x_softc *, int, u_char);

I prefer uint8_t rather than u_char.

> @@ -321,6 +364,53 @@ espattach(device_t parent, device_t self
 :
> +		/*
> +		 * Allocate memory which is friendly to the DMA
> +		 * engine (16 byte aligned) for use as the
> +		 * SCSI message buffers.
> +		 */
> +		if (bus_dmamem_alloc(esc->sc_dmat, (bus_size_t)NBPG, 16,
> +		    (bus_size_t)NBPG, &segs, 1, &rsegs, BUS_DMA_NOWAIT)) {
> +			printf("failed to allocate omess buffer!\n") ;
> +		}
> +		
> +		if (bus_dmamem_map(esc->sc_dmat, &segs, rsegs,
> +		    (size_t)NCR_MAX_MSG_LEN, (void **)&sc->sc_omess,
> +		    BUS_DMA_NOWAIT | BUS_DMA_COHERENT)) {
> +			printf("failed to map omess buffer!\n") ;
> +		}
> +		
> +		if (bus_dmamem_alloc(esc->sc_dmat, (bus_size_t)NBPG, 16,
> +		    (bus_size_t)NBPG, &segs, 1, &rsegs, BUS_DMA_NOWAIT)) {
> +			printf("failed to allocate imess buffer!\n") ;
> +		}
> +		
> +		if (bus_dmamem_map(esc->sc_dmat, &segs, rsegs,
> +		    (size_t)(NCR_MAX_MSG_LEN + 1), (void **)&sc->sc_imess,
> +		    BUS_DMA_NOWAIT | BUS_DMA_COHERENT)) {
> +			printf("failed to map imess buffer") ;
> +		}
> +	}

Maybe one NBPG (4KB) buffer is enough for two 16 bytes aligned buffers?

> @@ -900,3 +990,345 @@ esp_dualbus_intr(void *sc)
>  		ncr53c9x_intr((struct ncr53c9x_softc *)esp1);
>  	}
>  }
> +
> +void
> +esp_av_dma_stop(struct ncr53c9x_softc *sc)
> +{
> +	struct esp_softc *esc = (struct esp_softc *)sc;
> +	u_int32_t res;

uint32_t is better.

I also wonder if this esp dma_stop glue function should also handle
bus_dmamap_unload() (as pcscp.c does, but I forget details).

> +esp_av_dma_reset(struct ncr53c9x_softc *sc)
> +{
> +	struct esp_softc *esc = (struct esp_softc *)sc;
> +	u_int32_t res;

Also should be uint32_t.

> +void
> +esp_av_write_reg(struct ncr53c9x_softc *sc, int reg, u_char val)
> +{
> +	struct esp_softc *esc = (struct esp_softc *)sc;
> +	u_char v;

also uint8_t

> +int
> +esp_av_pio_intr(struct ncr53c9x_softc *sc)
> +{

 :

> +	if (esc->sc_dmasize == 0)
> +		printf("data interrupt, but no count left.");

IIRC some document said this (zero sized DMA) could happen
in the "transfer pad" operation on slow devices. (see pcscp.c)

> +int
> +esp_av_dma_intr(struct ncr53c9x_softc *sc)
> +{
> +	struct esp_softc *esc = (struct esp_softc *)sc;
> +	u_int32_t dma_resid;

also uint32_t

> +#if DEBUG
> +		printf("[av_dma_intr: Servicing Transfer Pad interrupt]\n") ;
> +#endif
> +		/* A "Transfer Pad" operation completed */
> +		return 0;
> +	}

See above.

> +	/* Halt the DMA engine */
> +	stop_psc_dma(PSC_DMA_CHANNEL_SCSI, esc->sc_rset, &dma_resid, 
> +	    esc->sc_datain);
> +
> +	esc->sc_active = 0;
> +	
> +	bus_dmamap_unload(esc->sc_dmat, esc->sc_dmap);

Strictly speaking, bus_dmamap_sync(9) for POSTREAD or POSTWRITE op
(per datain) is necessary before bus_dmamap_unload(9).

---
Izumi Tsutsui