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
