Subject: Re: kern/33445: Fixes for Promise SATA (pdcsata) PCI driver
To: None <kern-bug-people@netbsd.org, gnats-admin@netbsd.org,>
From: Timo Schoeler <timo.schoeler@riscworks.net>
List: netbsd-bugs
Date: 05/19/2006 12:55:02
The following reply was made to PR kern/33445; it has been noted by GNATS.
From: Timo Schoeler <timo.schoeler@riscworks.net>
To: buhrow@lothlorien.nfbcal.org
Cc: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org,
gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: kern/33445: Fixes for Promise SATA (pdcsata) PCI driver
Date: Fri, 19 May 2006 14:51:39 +0200
thus Brian Buhrow spake:
> Hello Timo. I think I see your problem. I was a bit confused about
> the NetBSD interrupt frame work and wasn't sure if the return value from an
> interrupt routine mattered. It does, and your problem is that the revised
> interrupt routine always claimed all interrupts. That works fine for
> non-shared interrupts, but you're sharing interrupts with the fxp1 card and
> the pdcsata driver, so things went south.
> Here's a patch which fixes that issue. Could you try it and confirm
> that all is now well?
> -thanks
> -Brian
>
> Index: pdcsata.c
> ===================================================================
> RCS file: /cvsroot/src/sys/dev/pci/pdcsata.c,v
> retrieving revision 1.3.2.2
> diff -u -r1.3.2.2 pdcsata.c
> --- pdcsata.c 5 Feb 2006 17:13:57 -0000 1.3.2.2
> +++ pdcsata.c 17 May 2006 20:57:55 -0000
> @@ -48,17 +48,19 @@
>
> #define PDC203xx_BAR_IDEREGS 0x1c /* BAR where the IDE registers are mapped */
>
> +#define PDC_CHANNELBASE(ch) 0x200 + ((ch) * 0x80)
> +#define PDC_ERRMASK 0x00780700
> +
> static void pdcsata_chip_map(struct pciide_softc *, struct pci_attach_args *);
> static void pdc203xx_setup_channel(struct ata_channel *);
> -static int pdc203xx_pci_intr(void *);
> static void pdc203xx_irqack(struct ata_channel *);
> static int pdc203xx_dma_init(void *, int, int, void *, size_t, int);
> static void pdc203xx_dma_start(void *,int ,int);
> static int pdc203xx_dma_finish(void *, int, int, int);
> +static int pdcsata_pci_intr(void *);
> +static void pdcsata_do_reset(struct ata_channel *, int);
>
> /* PDC205xx, PDC405xx and PDC407xx. but tested only pdc40718 */
> -static int pdc205xx_pci_intr(void *);
> -static void pdc205xx_do_reset(struct ata_channel *, int);
> static void pdc205xx_drv_probe(struct ata_channel *);
>
> static int pdcsata_match(struct device *, struct cfdata *, void *);
> @@ -183,30 +185,8 @@
> return;
> }
> intrstr = pci_intr_string(pa->pa_pc, intrhandle);
> -
> - switch (sc->sc_pp->ide_product) {
> - case PCI_PRODUCT_PROMISE_PDC20318:
> - case PCI_PRODUCT_PROMISE_PDC20319:
> - case PCI_PRODUCT_PROMISE_PDC20371:
> - case PCI_PRODUCT_PROMISE_PDC20375:
> - case PCI_PRODUCT_PROMISE_PDC20376:
> - case PCI_PRODUCT_PROMISE_PDC20377:
> - case PCI_PRODUCT_PROMISE_PDC20378:
> - case PCI_PRODUCT_PROMISE_PDC20379:
> - default:
> - sc->sc_pci_ih = pci_intr_establish(pa->pa_pc,
> - intrhandle, IPL_BIO, pdc203xx_pci_intr, sc);
> - break;
> -
> - case PCI_PRODUCT_PROMISE_PDC40718:
> - case PCI_PRODUCT_PROMISE_PDC40719:
> - case PCI_PRODUCT_PROMISE_PDC20571:
> - case PCI_PRODUCT_PROMISE_PDC20575:
> - case PCI_PRODUCT_PROMISE_PDC20579:
> - sc->sc_pci_ih = pci_intr_establish(pa->pa_pc,
> - intrhandle, IPL_BIO, pdc205xx_pci_intr, sc);
> - break;
> - }
> + sc->sc_pci_ih = pci_intr_establish(pa->pa_pc,
> + intrhandle, IPL_BIO, pdcsata_pci_intr, sc);
>
> if (sc->sc_pci_ih == NULL) {
> aprint_error("%s: couldn't establish native-PCI interrupt",
> @@ -258,6 +238,8 @@
> sc->sc_wdcdev.sc_atac.atac_set_modes = pdc203xx_setup_channel;
> sc->sc_wdcdev.sc_atac.atac_channels = sc->wdc_chanarray;
>
> + sc->sc_wdcdev.reset = pdcsata_do_reset;
> +
> switch (sc->sc_pp->ide_product) {
> case PCI_PRODUCT_PROMISE_PDC20318:
> case PCI_PRODUCT_PROMISE_PDC20319:
> @@ -281,7 +263,6 @@
> bus_space_write_4(sc->sc_ba5_st, sc->sc_ba5_sh, 0x60, 0x00ff00ff);
> sc->sc_wdcdev.sc_atac.atac_nchannels = PDC40718_NCHANNELS;
>
> - sc->sc_wdcdev.reset = pdc205xx_do_reset;
> sc->sc_wdcdev.sc_atac.atac_probe = pdc205xx_drv_probe;
>
> break;
> @@ -290,7 +271,6 @@
> bus_space_write_4(sc->sc_ba5_st, sc->sc_ba5_sh, 0x60, 0x00ff00ff);
> sc->sc_wdcdev.sc_atac.atac_nchannels = PDC20575_NCHANNELS;
>
> - sc->sc_wdcdev.reset = pdc205xx_do_reset;
> sc->sc_wdcdev.sc_atac.atac_probe = pdc205xx_drv_probe;
>
> break;
> @@ -403,53 +383,37 @@
> }
>
> static int
> -pdc203xx_pci_intr(void *arg)
> +pdcsata_pci_intr(void *arg)
> {
> struct pciide_softc *sc = arg;
> struct pciide_channel *cp;
> struct ata_channel *wdc_cp;
> int i, rv, crv;
> - u_int32_t scr;
> -
> - rv = 0;
> - scr = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, 0x00040);
> -
> - for (i = 0; i < sc->sc_wdcdev.sc_atac.atac_nchannels; i++) {
> - cp = &sc->pciide_channels[i];
> - wdc_cp = &cp->ata_channel;
> - if (scr & (1 << (i + 1))) {
> - crv = wdcintr(wdc_cp);
> - if (crv == 0) {
> - printf("%s:%d: bogus intr (reg 0x%x)\n",
> - sc->sc_wdcdev.sc_atac.atac_dev.dv_xname,
> - i, scr);
> - } else
> - rv = 1;
> - }
> - }
> - return rv;
> -}
> -
> -static int
> -pdc205xx_pci_intr(void *arg)
> -{
> - struct pciide_softc *sc = arg;
> - struct pciide_channel *cp;
> - struct ata_channel *wdc_cp;
> - int i, rv, crv;
> - u_int32_t scr, status;
> + u_int32_t scr, status, chanbase;
>
> rv = 0;
> scr = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, 0x40);
> + if (scr == 0xffffffff) return(rv);
> bus_space_write_4(sc->sc_ba5_st, sc->sc_ba5_sh, 0x40, scr & 0x0000ffff);
> -
> - status = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, 0x60);
> - bus_space_write_4(sc->sc_ba5_st, sc->sc_ba5_sh, 0x60, status & 0x000000ff);
> + scr = scr & 0x0000ffff;
> + if (!scr) return(rv);
>
> for (i = 0; i < sc->sc_wdcdev.sc_atac.atac_nchannels; i++) {
> cp = &sc->pciide_channels[i];
> wdc_cp = &cp->ata_channel;
> if (scr & (1 << (i + 1))) {
> + chanbase = PDC_CHANNELBASE(i) + 0x48;
> + status = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase);
> + if (status & PDC_ERRMASK) {
> + chanbase = PDC_CHANNELBASE(i) + 0x60;
> + status = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase);
> + status |= 0x800;
> + bus_space_write_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase, status);
> + status &= ~0x800;
> + bus_space_write_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase, status);
> + status = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase);
> + continue;
> + }
> crv = wdcintr(wdc_cp);
> if (crv == 0) {
> printf("%s:%d: bogus intr (reg 0x%x)\n",
> @@ -541,24 +505,29 @@
>
>
> static void
> -pdc205xx_do_reset(struct ata_channel *chp, int poll)
> +pdcsata_do_reset(struct ata_channel *chp, int poll)
> {
> struct pciide_softc *sc = CHAN_TO_PCIIDE(chp);
> - u_int32_t scontrol;
> -
> - wdc_do_reset(chp, poll);
> + int reset, status, i, chanbase;
>
> /* reset SATA */
> - scontrol = SControl_DET_INIT | SControl_SPD_ANY | SControl_IPM_NONE;
> - SCONTROL_WRITE(sc, chp->ch_channel, scontrol);
> - delay(50*1000);
> -
> - scontrol &= ~SControl_DET_INIT;
> - SCONTROL_WRITE(sc, chp->ch_channel, scontrol);
> - delay(50*1000);
> -}
> + reset = (1 << 11);
> + chanbase = PDC_CHANNELBASE(chp->ch_channel) + 0x60;
> + for (i = 0; i < 11;i ++) {
> + status = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase);
> + if (status & reset) break;
> + delay(100);
> + status |= reset;
> + bus_space_write_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase, status);
> + }
> + status = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase);
> + status &= ~reset;
> + bus_space_write_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase, status);
> + status = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase);
>
> + wdc_do_reset(chp, poll);
>
> +}
>
> static void
> pdc205xx_drv_probe(struct ata_channel *chp)
hi brian,
thanks a lot for the patch; i applied it to todays netbsd-3 src and run
it right now. there are two 250GByte WD HDs connected to a SATA300 TX4,
which run very good. no lost interrupts. i built a RAID1 on them,
stressed them with multiple instances of bonnie and untarring pkgsrc,
src etc. no problems until now.
ping flooding the netbsd machine (fxp) does not lose packets, flooding
my G4 from the netbsd machine does. i'll test this in a different
scenario (attached to a managed nortel switch) later and report again.
thanks a lot!
--
Timo Schoeler | http://riscworks.net/~tis | timo.schoeler@riscworks.net
RISCworks -- Perfection is a powerful message
ISP | POWER & PowerPC afficinados | Networking, Security, BSD services
GPG Key fingerprint = B5F6 68A4 EC45 C309 6770 38C4 50E8 2740 9E0C F20A
There are 10 types of people in the world. Those who understand binary
and those who don't.