Re: DMA support for SCSI adapter of AV Mac

Thank you very much for your kind comments, and sorry for not being to
reply immediately.

I updated the patch in according to your comments. Also, your codes in
arch/arc/jazz/asc.c and dev/pci/pcscp.c are very helpful for me. Let me
thank you again for that :-).

I already committed the patch. I think that it is almost fine, and
remaining problems can be addressed in tree.

I will reply to your each comment below:

On 2018/10/16 22:43, Izumi Tsutsui wrote:
> 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)

I replaced it with <sys/bus.h>. This is necessary since there is a
function one of whose argument is bus_addr_t.

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

No, I removed it.

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

Fixed.

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

u_char and u_int*_t are replaced by uint*_t.

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

Two buffers are necessary due to m68k-specific behavior of
bus_dmamem_map(9); it rounds size up to NBPG:

https://nxr.netbsd.org/xref/src/sys/arch/m68k/m68k/bus_dma.c#735

I added a comment on it.

>> @@ -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).

bus_dmamap_unload(9) is called from dma_stop function also in
arch/arc/jazz/asp.c. I added it here, and observe no new problems.
(Without it, resource leaks occur in some error paths?)

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

I handled it. However, this function is used only for transfer data
not fit into 16-byte boundaries, which AV DMA cannot handle. I wonder
whether sc_dmasize == 0 is possible; see esp_av_dma_setup().

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

Handled.

>> +	/* 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).

I added it.

Thank you again for your helpful comments!

rin

> ---
> Izumi Tsutsui
>