On Wed, Mar 08, 2000 at 10:11:15PM +0000, Roger Brooks wrote:
> [...]

So some comments on the code:

Content-Description: UDMA/66 patch for HPT366
> diff -c --new-file pci.old/pcidevs pci/pcidevs
> *** pci.old/pcidevs	Wed Feb 16 12:22:58 2000
> --- pci/pcidevs	Sat Feb 26 23:57:55 2000
> ***************
> *** 1349,1354 ****
> --- 1349,1357 ----
>   product TRIDENT	TGUI_9680	0x9680	TGUI 9680
>   product TRIDENT	TGUI_9682	0x9682	TGUI 9682
>   
> + /* Triones Technologies products */
> + product TRIONES HPT366          0x0004  HPT366 UDMA/66 Controller
> + 
>   /* TriTech Microelectronics products*/
>   product TRITECH	TR25202		0xfc02	Pyramid3D TR25202
>   
> diff -c --new-file pci.old/pciide.c pci/pciide.c
> *** pci.old/pciide.c	Wed Jan 19 12:31:49 2000
> --- pci/pciide.c	Wed Mar  8 00:03:32 2000
> ***************
> *** 115,120 ****
> --- 115,121 ----
>   #include <dev/pci/pciide_sis_reg.h>
>   #include <dev/pci/pciide_acer_reg.h>
>   #include <dev/pci/pciide_pdc202xx_reg.h>
> + #include <dev/pci/pciide_hpt366_reg.h>
>   
>   /* inlines for reading/writing 8-bit PCI registers */
>   static __inline u_int8_t pciide_pci_read __P((pci_chipset_tag_t, pcitag_t,
> ***************
> *** 181,186 ****
> --- 182,193 ----
>   void pdc202xx_setup_channel __P((struct channel_softc*));
>   int  pdc202xx_pci_intr __P((void *));
>   
> + void hpt366_chip_map __P((struct pciide_softc*, struct pci_attach_args*));
> + void hpt366_setup_channel __P((struct channel_softc*));
> + int  hpt366_claim_hw __P((void *, int));
> + void hpt366_free_hw __P((void *));
> + int  hpt366_pci_intr __P((void *));
> + 
>   void pciide_channel_dma_setup __P((struct pciide_channel *));
>   int  pciide_dma_table_setup __P((struct pciide_softc*, int, int));
>   int  pciide_dma_init __P((void*, int, int, void *, size_t, int));
> ***************
> *** 336,341 ****
> --- 343,360 ----
>   	}
>   };
>   
> + const struct pciide_product_desc pciide_triones_products[] =  {
> + 	{ PCI_PRODUCT_TRIONES_HPT366,
> + 	  IDE_PCI_CLASS_OVERRIDE,
> + 	  "Triones/Highpoint HPT366 UDMA/66 IDE Controller",
> + 	  hpt366_chip_map,
> + 	},
> + 	{ 0,
> + 	  0,
> + 	  NULL,
> + 	}
> + };
> + 
>   struct pciide_vendor_desc {
>   	u_int32_t ide_vendor;
>   	const struct pciide_product_desc *ide_products;
> ***************
> *** 349,354 ****
> --- 368,374 ----
>   	{ PCI_VENDOR_SIS, pciide_sis_products },
>   	{ PCI_VENDOR_ALI, pciide_acer_products },
>   	{ PCI_VENDOR_PROMISE, pciide_promise_products },
> + 	{ PCI_VENDOR_TRIONES, pciide_triones_products },
>   	{ 0, NULL }
>   };
>   
> ***************
> *** 2704,2706 ****
> --- 2724,2924 ----
>   	}
>   	return rv;
>   }
> + 
> + void
> + hpt366_chip_map(sc, pa)
> +         struct pciide_softc *sc;
> + 	struct pci_attach_args *pa;
> + {
> + 	struct pciide_channel *cp;
> + 	u_int8_t cable, reg51h;
> + 	pcireg_t interface;
> + 	bus_size_t cmdsize, ctlsize;
> + 	static struct channel_queue *queue = NULL;
> + 	static int device = -1;
> + 	char *qtype = "shared";
> + 
> + 	if (pciide_chipen(sc, pa) == 0)
> + 		return;
> + 
> + 	/*
> + 	 * can't rely on the PCI_CLASS_REG content if the chip was in raid
> + 	 * mode. We have to fake interface
> + 	 */
> + 	interface = PCIIDE_INTERFACE_SETTABLE(0) | PCIIDE_INTERFACE_PCI(0);
> + 
> + 	printf("%s: bus-master DMA support present",
> + 		sc->sc_wdcdev.sc_dev.dv_xname);
> + 	pciide_mapreg_dma(sc, pa);
> + 	printf("\n");
> + 	sc->sc_wdcdev.cap = WDC_CAPABILITY_DATA16 | WDC_CAPABILITY_DATA32 |
> + 	    WDC_CAPABILITY_MODE | WDC_CAPABILITY_DMA | WDC_CAPABILITY_UDMA;
> + 	sc->sc_wdcdev.PIO_cap = 4;
> + 	sc->sc_wdcdev.DMA_cap = 2;
> + 	sc->sc_wdcdev.UDMA_cap = 4;
> + 
> + 	sc->sc_wdcdev.set_modes = hpt366_setup_channel;
> + 	sc->sc_wdcdev.channels = sc->wdc_chanarray;
> + 	sc->sc_wdcdev.nchannels = 1;
> + 
> + 	reg51h = pciide_pci_read( sc->sc_pc, sc->sc_tag, 0x51 );
> + 	printf( "%s: reg51h = 0x%02x\n", 
> + 		sc->sc_wdcdev.sc_dev.dv_xname, reg51h );
> + 	/*
> + 	 *  Turn off fast interrupt prediction
> + 	 */
> + 	pciide_pci_write( sc->sc_pc, sc->sc_tag, 0x51, reg51h & ~0x80 );
> + 	cable = pciide_pci_read(sc->sc_pc, sc->sc_tag, 0x5a );
> + 	printf( "%s: channel %d UDMA/%s cable (reg5ah = 0x%02x)\n", 
> + 		sc->sc_wdcdev.sc_dev.dv_xname, 0,
> + 		(cable & 0x02) ? "33" : "66", cable );
> + 	cp = &sc->pciide_channels[0];
> + 
> + 	sc->wdc_chanarray[0] = &cp->wdc_channel;
> + 	cp->name = PCIIDE_CHANNEL_NAME(0);
> + 	cp->wdc_channel.channel = 0;
> + 	cp->wdc_channel.wdc = &sc->sc_wdcdev;
> + 	sc->sc_wdcdev.claim_hw = hpt366_claim_hw;
> + 	sc->sc_wdcdev.free_hw = hpt366_free_hw;
> + 	sc->sc_wdcdev.cap |= WDC_CAPABILITY_HWLOCK;
> + 	if( pa->pa_device != device || queue == NULL) {
> + 		queue = malloc(sizeof(struct channel_queue), M_DEVBUF, M_NOWAIT);
> + 		if (queue == NULL) {
> + 			printf("%s %s channel: "
> + 		    	    "can't allocate memory for command queue",
> + 			sc->sc_wdcdev.sc_dev.dv_xname, cp->name);
> + 			return;
> + 		}
> + 		device = pa->pa_device;
> + 		qtype = "shareable";
> + 	}
> + 	cp->wdc_channel.ch_queue = queue;
> + 	printf("%s: %s channel configured to native-PCI mode (%s queue)\n",
> + 	    	    sc->sc_wdcdev.sc_dev.dv_xname, cp->name, qtype);
> + 	
> + 	pciide_mapchan(pa, cp, interface, &cmdsize, &ctlsize, hpt366_pci_intr);
> + 	if (cp->hw_ok == 0)
> + 		return;
> + 	hpt366_setup_channel(&cp->wdc_channel);
> + 
> + 	return;
> + }
> + 
> + 
> + void
> + hpt366_setup_channel(chp)
> + 	struct channel_softc *chp;
> + {
> +         struct ata_drive_datas *drvp;
> + 	int drive;
> + 	u_int32_t tim;
> + 	u_int32_t idedma_ctl;
> + 	struct pciide_channel *cp = (struct pciide_channel*)chp;
> + 	struct pciide_softc *sc = (struct pciide_softc *)cp->wdc_channel.wdc;
> + 
> + 
> + 	/* setup DMA if needed */
> + 	pciide_channel_dma_setup(cp);
> + 
> + 	idedma_ctl = 0;
> + 
> + 	/* Per channel settings */
> + 	for (drive = 0; drive < 2; drive++) {
> + 		drvp = &chp->ch_drive[drive];
> + 		/* If no drive, skip */
> + 		if ((drvp->drive_flags & DRIVE) == 0)
> + 			continue;
> + 
> + 		tim = pci_conf_read(sc->sc_pc, sc->sc_tag,
> + 					HPT366_DATA_TIM(drive));
> + 		printf( "%s: bus speed register = 0x%08x\n",
> + 			drvp->drv_softc->dv_xname, tim );
> + 
> +                 /* add timing values, setup DMA if needed */
> +                 tim = hpt366_data_tim_pio[drvp->PIO_mode];
> +                 if (drvp->drive_flags & DRIVE_DMA) {
> +                         /*
> +                          * use Multiword DMA.
> +                          * Timings will be used for both PIO and DMA, so adjust
> +                          * DMA mode if needed
> +                          */

This is not needed if you're using Ultra-DMA. You could do:
                 if (drvp->drive_flags & DRIVE_UDMA) {
                         tim = hpt366_data_tim_udma[drvp->DMA_mode];
                         idedma_ctl |= IDEDMA_CTL_DRV_DMA(drive);
                 } else if (drvp->drive_flags & DRIVE_DMA) {
                         /*
                          * use Multiword DMA.
                          * Timings will be used for both PIO and DMA, so adjust
                          * DMA mode if needed
                          */
			  [...]


> +                         if (drvp->PIO_mode >= 3 &&
> +                             (drvp->DMA_mode + 2) > drvp->PIO_mode) {
> +                                 drvp->DMA_mode = drvp->PIO_mode - 2;
> +                             }
> +                         tim = hpt366_data_tim_dma[drvp->DMA_mode];
> +                         idedma_ctl |= IDEDMA_CTL_DRV_DMA(drive);
> +                 }
> +                 if (drvp->drive_flags & DRIVE_UDMA) {
> +                         tim = hpt366_data_tim_udma[drvp->DMA_mode];
> +                         idedma_ctl |= IDEDMA_CTL_DRV_DMA(drive);
> +                 }
> + 		tim &= ~0x80000000;
> + 		pci_conf_write(sc->sc_pc, sc->sc_tag,
> +                                   HPT366_DATA_TIM(drive), tim);
> +                                                                                                                     
> + 	}
> + 	if (idedma_ctl != 0) {
> + 		/* Add software bits in status register */
> + 		bus_space_write_1(sc->sc_dma_iot, sc->sc_dma_ioh,
> + 		    IDEDMA_CTL, idedma_ctl);
> + 	}
> + 	pciide_print_modes(cp);
> + }
> + 
> + static int hpt366_busy = 0;
> + 
> + int
> + hpt366_claim_hw( cp, n )
> + 	void *cp;
> + 	int n;
> + {
> + #if 0
> + 	struct channel_softc *chp = (struct channel_softc *) cp;
> + #endif
> + 	int s = splbio();
> + 	int rv = 0;
> + 
> + 	if( !hpt366_busy )
> + 		rv = hpt366_busy = 1;
> + 	splx( s );
> + 	return (rv);
> + }		
> + 
> + void
> + hpt366_free_hw( cp )
> + 	void *cp;
> + {
> + #if 0
> + 	struct channel_softc *chp = (struct channel_softc *) cp;
> + #endif
> + 
> + 	hpt366_busy = 0;
> + }		
> + 
> + int
> + hpt366_pci_intr(arg)
> + 	void *arg;
> + {
> + 	struct pciide_softc *sc = arg;
> + 	struct pciide_channel *cp = &sc->pciide_channels[0];
> + 	struct channel_softc *wdc_cp = &cp->wdc_channel;
> + 	int rv = 0;
> + #if 0
> + 	u_int8_t dmastat;
> + 
> + 	/*
> + 	 * FreeBSD has this code to avoid calling interrupt handler with
> + 	 * other channel, but I don't think it's needed in NetBSD.
> + 	 */
> + 	dmastat = pciide_pci_read( sc->sc_pc, sc->sc_tag, IDEDMA_CTL );
> + 	if( !(dmastat & IDEDMA_CTL_INTR) )
> + 		return 0;
> + 	pciide_pci_write( sc->sc_pc, sc->sc_tag, IDEDMA_CTL, dmastat );


Hum. I think this is really needed !
Look at pdc202xx_pci_intr() for example (although as you've a single-channel
attachemement the above code should be enouth).
I suspect that under some conditions a hard hang can occur if we read the
status register before IRQ occured (this may be related to some controllers
doing funny things with registers). We can also lose interrupt because of
this. This can only happen with interrupt sharing, so it's not needed for
controllers in legaty mode.

Also, as a general note I prefer to #define registers names and contents
in pciide_xxx_reg.h and use symbolic names in the driver :)

--
Manuel Bouyer <bouyer@antioche.eu.org>
--