On Sat, Feb 08, 2020 at 01:07:01PM +0800, Nathanael Rensen wrote: > The diff below adds support for the watchdog as found in the embedded > AMD FCH (fusion controller hub) as found on APU2. > > Index: sys/dev/pci/piixpm.c > =================================================================== > RCS file: /cvs/src/sys/dev/pci/piixpm.c,v > retrieving revision 1.42 > diff -u -p -r1.42 piixpm.c > --- sys/dev/pci/piixpm.c 21 Jan 2020 06:37:24 -0000 1.42 > +++ sys/dev/pci/piixpm.c 8 Feb 2020 04:44:42 -0000 > @@ -54,8 +54,10 @@ struct piixpm_softc { > struct device sc_dev; > > bus_space_tag_t sc_iot; > + bus_space_tag_t sc_memt; > bus_space_handle_t sc_ioh; > bus_space_handle_t sc_sb800_ioh; > + bus_space_handle_t sc_wdt_mh; > void * sc_ih; > int sc_poll; > int sc_is_sb800; > @@ -83,6 +85,8 @@ int piixpm_i2c_exec(void *, i2c_op_t, i2 > > int piixpm_intr(void *); > > +int piixpm_wdt_cb(void *arg, int period); > + > struct cfattach piixpm_ca = { > sizeof(struct piixpm_softc), > piixpm_match, > @@ -127,7 +131,7 @@ piixpm_attach(struct device *parent, str > struct piixpm_softc *sc = (struct piixpm_softc *)self; > struct pci_attach_args *pa = aux; > bus_space_handle_t ioh; > - u_int16_t val, smb0en; > + u_int16_t val, smb0en, wdten = 0; > bus_addr_t base; > pcireg_t conf; > pci_intr_handle_t ih; > @@ -136,6 +140,7 @@ piixpm_attach(struct device *parent, str > int numbusses, i; > > sc->sc_iot = pa->pa_iot; > + sc->sc_memt = pa->pa_memt; > numbusses = 1; > > if ((PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD && > @@ -160,7 +165,7 @@ piixpm_attach(struct device *parent, str > > /* > * AMD Bolton matches PCI_PRODUCT_AMD_HUDSON2_SMB but > - * uses old register layout. Therefor check PCI_REVISION. > + * uses old register layout. Therefore check PCI_REVISION. > */ > if (PCI_VENDOR(pa->pa_id) == PCI_VENDOR_AMD && > ((PCI_PRODUCT(pa->pa_id) == PCI_PRODUCT_AMD_HUDSON2_SMB && > @@ -170,6 +175,7 @@ piixpm_attach(struct device *parent, str > AMDFCH41_PM_DECODE_EN); > val = bus_space_read_1(sc->sc_iot, ioh, 1); > smb0en = val & AMDFCH41_SMBUS_EN; > + wdten = val & AMDFCH41_WDT_EN; > > bus_space_write_1(sc->sc_iot, ioh, 0, > AMDFCH41_PM_DECODE_EN + 1); > @@ -282,7 +288,59 @@ piixpm_attach(struct device *parent, str > config_found(self, &iba, iicbus_print); > } >
Why are you registering the watchdog late and not in the FCH specific block above? Do other AMD SBx00 or KERNCZ chips support the watchdog in the same way? > + /* Register watchdog */ > + if (wdten && bus_space_map(sc->sc_memt, AMDFCH41_WDTREG_BASE, > + AMDFCH41_WDTREG_SIZE, 0, &sc->sc_wdt_mh) == 0) { > + val = bus_space_read_1(sc->sc_memt, sc->sc_wdt_mh, > + AMDFCH41_WDTREG_CTL); > + if (val & AMDFCH41_WDTREG_CTL_FIRED) { > + printf("%s watchdog caused previous restart\n", > + sc->sc_dev.dv_xname); > + bus_space_write_1(sc->sc_memt, sc->sc_wdt_mh, > + AMDFCH41_WDTREG_CTL, > + val | AMDFCH41_WDTREG_CTL_FIRED); > + } > + > + if (val & AMDFCH41_WDTREG_CTL_DISABLED) > + printf("%s watchdog disabled\n", sc->sc_dev.dv_xname); > + else { > + printf("%s watchdog found\n", sc->sc_dev.dv_xname); The printf used here are non standard. Ideally this printing is folded into the attach line of piixpm(4), e.g: piixpm0 at pci0 dev 20 function 0 "AMD Hudson-2 SMBus" rev 0x42: SMI, watchdog Same for the 'watchdog caused previous restart'. I think the common idiom is to print ', watchdog' and ', reboot on timeout' for the two cases. Also move the if (val & AMDFCH41_WDTREG_CTL_FIRED) check after if (val & AMDFCH41_WDTREG_CTL_DISABLED) > + > + /* Set 1 second counter period */ > + bus_space_write_1(sc->sc_iot, sc->sc_sb800_ioh, 0, > + AMDFCH41_PM_DECODE_EN + 3); > + val = bus_space_read_1(sc->sc_iot, sc->sc_sb800_ioh, 1); > + val = (val & ~AMDFCH41_WDT_MASK) | AMDFCH41_WDT_1S; > + bus_space_write_1(sc->sc_iot, sc->sc_sb800_ioh, 1, val); > + > + wdog_register(piixpm_wdt_cb, sc); > + } > + } > + > return; > +} > + > +int > +piixpm_wdt_cb(void *arg, int period) > +{ > + struct piixpm_softc *sc = (struct piixpm_softc *)arg; > + u_int16_t val; > + > + val = bus_space_read_1(sc->sc_memt, sc->sc_wdt_mh, AMDFCH41_WDTREG_CTL); > + > + if (period > 0xffff) > + period = 0xffff; > + if (period > 0) { > + bus_space_write_2(sc->sc_memt, sc->sc_wdt_mh, > + AMDFCH41_WDTREG_COUNT, period); > + val |= AMDFCH41_WDTREG_CTL_RUN | AMDFCH41_WDTREG_CTL_TRIGGER; > + } > + else KNF, move else up > + val &= ~AMDFCH41_WDTREG_CTL_RUN; > + > + bus_space_write_1(sc->sc_memt, sc->sc_wdt_mh, AMDFCH41_WDTREG_CTL, val); > + > + return period; > } I think for watchdogs you need at least a DVACT_POWERDOWN activate handler where wdog_shutdown() is called. Maybe more is needed for DVACT_SUSPEND and DVACT_RESUME (stop wdog on suspend and restart wdog on resume). > int > Index: sys/dev/pci/piixreg.h > =================================================================== > RCS file: /cvs/src/sys/dev/pci/piixreg.h,v > retrieving revision 1.6 > diff -u -p -r1.6 piixreg.h > --- sys/dev/pci/piixreg.h 21 Jan 2020 06:37:24 -0000 1.6 > +++ sys/dev/pci/piixreg.h 8 Feb 2020 04:44:42 -0000 > @@ -87,5 +87,19 @@ > #define AMDFCH41_PM_DECODE_EN 0x00 /* 16-bit register */ > #define AMDFCH41_PM_PORT_INDEX 0x02 > #define AMDFCH41_SMBUS_EN 0x10 > +#define AMDFCH41_WDT_EN 0x80 > +#define AMDFCH41_WDT_MASK 0x0f > +#define AMDFCH41_WDT_32US 0x00 > +#define AMDFCH41_WDT_10MS 0x01 > +#define AMDFCH41_WDT_100MS 0x02 > +#define AMDFCH41_WDT_1S 0x03 > +#define AMDFCH41_WDTREG_BASE 0xfeb00000 > +#define AMDFCH41_WDTREG_SIZE 0x10 > +#define AMDFCH41_WDTREG_CTL 0x00 > +#define AMDFCH41_WDTREG_CTL_RUN 0x01 > +#define AMDFCH41_WDTREG_CTL_FIRED 0x02 > +#define AMDFCH41_WDTREG_CTL_DISABLED 0x08 > +#define AMDFCH41_WDTREG_CTL_TRIGGER 0x80 > +#define AMDFCH41_WDTREG_COUNT 0x04 > > #endif /* !_DEV_PCI_PIIXREG_H_ */ > -- :wq Claudio