> Date: Thu, 9 Dec 2021 11:31:25 +0100
> From: Christopher Zimmermann <chr...@openbsd.org>
>
> On Wed, Dec 08, 2021 at 05:20:39PM +0100, Mark Kettenis wrote:
> >> Date: Wed, 8 Dec 2021 16:31:50 +0100
> >> From: Christopher Zimmermann <chr...@openbsd.org>
> >>
> >> >On Wed, Dec 08, 2021 at 10:47:45AM +0100, Christopher Zimmermann wrote:
> >> >> Note that it attaches to "snps,dw-wdt"
> >>
> >> On Wed, Dec 08, 2021 at 08:57:53PM +1100, Jonathan Gray wrote:
> >> >We already have a driver for that.
> >>
> >> Oh no.
> >>
> >> OK?
> >
> >What problem are you trying to solve?
>
> I want a watchdog.
Fair enough.
> Here is a diff merging my driver with the current driver to support
> kern.watchdog sysctl. Tested on RockPro64. OK?
Your mail client is scrambling the diff.
A bit of advice. Please read the style(9) man page, and take a look
at other drivers in this directory. Try to keep your changes minimal.
Making arbitrary changes and adding lots of debug code makes it
difficult to review a diff.
> Index: sys/dev/fdt/dwdog.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/fdt/dwdog.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 dwdog.c
> --- sys/dev/fdt/dwdog.c 24 Oct 2021 17:52:26 -0000 1.3
> +++ sys/dev/fdt/dwdog.c 9 Dec 2021 10:13:21 -0000
> @@ -19,42 +19,63 @@
> #include <sys/systm.h>
> #include <sys/device.h>
>
> +#include <machine/intr.h>
> #include <machine/bus.h>
> #include <machine/fdt.h>
>
> #include <armv7/armv7/armv7_machdep.h>
If we start using this driver on arm64, we should drop this include.
Other watchdog drivers just use:
extern void (*cpuresetfn)(void);
> #include <dev/ofw/openfirm.h>
> +#include <dev/ofw/ofw_clock.h>
> #include <dev/ofw/fdt.h>
>
> +/* #define DEBUG */
> +
> /* Registers */
> -#define WDT_CR 0x0000
> -#define WDT_CR_WDT_EN (1 << 0)
> +#define WDT_CR 0x0000 /* control */
> +#define WDT_CR_ENABLE (1 << 0) /* enable counter */
> #define WDT_CR_RESP_MODE (1 << 1)
> +/*
> + * On some devices WDT_CR_ENABLE can only be set, but not be cleared.
> + *
> + * If WDT_CR_RESP_MODE is enabled the watchdog will count twice. After the
> + * first count the interrupt is raised as a warning. After the second count
> + * the reset happens only when the interrupt is not cleared.
> + */
> +#define WDT_CR_RST_PULSE_LENGTH_MASK 0x001c
> +#define WDT_CR_RST_PULSE_LENGTH_SHIFT 2
> + /* pulse length is 2^(n+1) clock cycles. Default is n=2 / 8 cycles. */
> +
> #define WDT_TORR 0x0004
> -#define WDT_CCVR 0x0008
> -#define WDT_CRR 0x000c
> -#define WDT_CRR_KICK 0x76
> -#define WDT_STAT 0x0010
> -#define WDT_EOI 0x0014
> +/* timeout range n={0..15}. Timeout is 2^(16+n)-1 ticks */
> +
> +#define WDT_CCVR 0x0008 /* current counter value */
> +#define WDT_CRR 0x000c /* counter restart by writing magic
> number */
> +#define WDT_CRR_MAGIC 0x76
> +#define WDT_STAT 0x0010 /* interrupt status */
> +#define WDT_EOI 0x0014 /* clear interrupt */
We typically don't annotate the register definitions this way,
especially when documentation is publically available. I'd prefer to
keep it that way.
If something non-obvious is happening, it is better to document it in
the code.
>
> #define HREAD4(sc, reg)
> \
> (bus_space_read_4((sc)->sc_iot, (sc)->sc_ioh, (reg)))
> #define HWRITE4(sc, reg, val)
> \
> bus_space_write_4((sc)->sc_iot, (sc)->sc_ioh, (reg), (val))
> -#define HSET4(sc, reg, bits) \
> - HWRITE4((sc), (reg), HREAD4((sc), (reg)) | (bits))
> -#define HCLR4(sc, reg, bits) \
> - HWRITE4((sc), (reg), HREAD4((sc), (reg)) & ~(bits))
>
> struct dwdog_softc {
> struct device sc_dev;
> bus_space_tag_t sc_iot;
> bus_space_handle_t sc_ioh;
> +
> + uint32_t sc_freq;
> + int sc_period;
> + int sc_reset;
> };
>
> -int dwdog_match(struct device *, void *, void *);
> -void dwdog_attach(struct device *, struct device *, void *);
> +int dwdog_match(struct device *, void *, void *);
> +void dwdog_attach(struct device *, struct device *, void *);
> +static void dwdog_dump(struct dwdog_softc *, const char *);
> +int dwdog_intr(void *);
> +int dwdog_cb(void *, int);
> +void dwdog_reset(void);
>
> const struct cfattach dwdog_ca = {
> sizeof(struct dwdog_softc), dwdog_match, dwdog_attach
> @@ -64,8 +85,6 @@ struct cfdriver dwdog_cd = {
> NULL, "dwdog", DV_DULL
> };
>
> -void dwdog_reset(void);
> -
> int
> dwdog_match(struct device *parent, void *match, void *aux)
> {
> @@ -86,33 +105,179 @@ dwdog_attach(struct device *parent, stru
> }
>
> sc->sc_iot = faa->fa_iot;
> + sc->sc_freq = 0;
>
> if (bus_space_map(sc->sc_iot, faa->fa_reg[0].addr,
> faa->fa_reg[0].size, 0, &sc->sc_ioh)) {
> printf(": can't map registers\n");
> + return;
> + }
> +
> + fdt_intr_establish(faa->fa_node, IPL_CLOCK, dwdog_intr,
> + sc, sc->sc_dev.dv_xname);
> + if (NULL == fdt_intr_establish(faa->fa_node, IPL_BIO,
> + dwdog_intr, sc, sc->sc_dev.dv_xname)) {
> + printf(": unable to establish interrupt\n");
> + return;
> }
This makes no sense. You're establishing the interrupt twice. There
is no real reason to check the return value.
>
> printf("\n");
>
> + dwdog_dump(sc, "initial dump");
Please remove all debug code from your diff.
> +
> +#ifndef SMALL_KERNEL
> + wdog_register(dwdog_cb, sc);
> +#endif
Best the register the watchdog at the end of the function, after the
hardware has been initialized properly.
> +
> + clock_set_assigned(faa->fa_node);
> + clock_enable_all(faa->fa_node);
> +
> if (cpuresetfn == NULL)
> cpuresetfn = dwdog_reset;
> }
>
> +static void
> +dwdog_dump(struct dwdog_softc *sc, const char *msg) {
> +#ifdef DEBUG
> + printf("%s: %s\n CR=%#x TORR=%#x(%u) CCVR=%#x STAT=%#x\n",
> + sc->sc_dev.dv_xname, msg,
> + HREAD4(sc, WDT_CR),
> + HREAD4(sc, WDT_TORR),
> + HREAD4(sc, WDT_TORR),
> + HREAD4(sc, WDT_CCVR),
> + HREAD4(sc, WDT_STAT));
> +#endif
> +}
> +
> +int
> +dwdog_intr(void *self) {
> + struct dwdog_softc *sc = self;
> +
> + dwdog_dump(sc, "interrupt");
> +
> + /* clear all interrupts */
> + HREAD4(sc, WDT_EOI);
> +
> + return 1;
> +}
This interrupt handler doesn't really do anything.
> +
> +int
> +dwdog_cb(void *self, int period)
> +{
> + struct dwdog_softc *sc = self;
> + unsigned tor = 0;
> + uint32_t control;
> +
> + dwdog_dump(sc, "watchdog callback");
> +
> + /*
> + * to enable set RC_ENABLE and clear RC_INTERRUPT
> + * to disable clear RC_ENABLE and set RC_INTERRUPT
> + * Even when RC_ENABLE cannot be cleared this prevents a reset because
> + * we acknowledge all interrupts.
> + */
> +
> + if (sc->sc_reset)
> + return -1;
> +
> + if (period > 0 && period == sc->sc_period) {
> +#ifdef DEBUG
> + printf("%s: just tickling\n", sc->sc_dev.dv_xname);
> +#endif
> + HWRITE4(sc, WDT_CRR, WDT_CRR_MAGIC);
> + return sc->sc_period;
> + }
> +
> + control = HREAD4(sc, WDT_CR);
> +
> + if (period == 0) {
> + /* in case counter cannot be disabled enable interrupt */
> + if ((control & WDT_CR_ENABLE) == 0)
> + return 0;
> + HWRITE4(sc, WDT_TORR, 15);
> + control &= ~WDT_CR_ENABLE;
> + control |= WDT_CR_RESP_MODE;
> + HWRITE4(sc, WDT_CR, control);
> + control = HREAD4(sc, WDT_CR);
> + dwdog_dump(sc, "disabled");
> +
> + if (control & WDT_CR_ENABLE) {
> + printf("%s: Cannot disable watchdog timer. "
> + "Please enable kern.watchdog.auto as a
> workaround\n",
> + sc->sc_dev.dv_xname);
> + return (sc->sc_freq ? 0x7fffffff / sc->sc_freq : 1);
> + }
> + else
> + return 0;
> + }
> +
> + if (sc->sc_freq == 0) {
> + uint32_t ticks, dt = 1000; /* us */
> +
> + HWRITE4(sc, WDT_TORR, 15);
> + control |= WDT_CR_ENABLE | WDT_CR_RESP_MODE;
> + HWRITE4(sc, WDT_CR, control);
> + HWRITE4(sc, WDT_CRR, WDT_CRR_MAGIC);
> +
> + ticks = HREAD4(sc, WDT_CCVR);
> + delay(dt);
> + ticks -= HREAD4(sc, WDT_CCVR);
> + sc->sc_freq = ticks * (1000000 / dt);
> +#ifdef DEBUG
> + printf("%s: %d / %u us => frequency %u\n",
> + sc->sc_dev.dv_xname, ticks, dt, sc->sc_freq);
> +#endif
> + }
> +
> + while (tor < 15 &&
> + 1 << tor < (sc->sc_freq >> 16) * period)
> + tor++;
> +
> +#ifdef DEBUG
> + printf("%s: target period=%ds ticks_wanted=%u tor=%u ticks=%#x(%u /
> %us)\n",
> + sc->sc_dev.dv_xname,
> + period,
> + sc->sc_freq * period - 1,
> + tor,
> + (1 << (16+tor)) - 1,
> + (1 << (16+tor)) - 1,
> + ((1 << (16+tor)) - 1) / sc->sc_freq);
> +#endif
> +
> + assert((1 << (16+tor)) - 1 >= sc->sc_freq);
> + sc->sc_period = ((1 << (16+tor)) - 1) / sc->sc_freq;
> +
> + HWRITE4(sc, WDT_TORR, tor);
> + control = HREAD4(sc, WDT_CR);
> + control &= ~WDT_CR_RESP_MODE;
> + control |= WDT_CR_ENABLE;
> + HWRITE4(sc, WDT_CR, control);
> + HWRITE4(sc, WDT_CRR, WDT_CRR_MAGIC);
> +
> + dwdog_dump(sc, "started watchdog");
> +
> + return sc->sc_period;
> +}
> +
This all looks horribly complicated. Isn't the clock frequency
provided by the device tree? I suspect you should be able to use
clock_get_frequency() to get it.
> void
> dwdog_reset(void)
> {
> struct dwdog_softc *sc = dwdog_cd.cd_devs[0];
> + uint32_t control;
>
> /*
> * Generate system reset when timer expires and select
> * smallest timeout.
> */
> - HCLR4(sc, WDT_CR, WDT_CR_RESP_MODE | WDT_CR_WDT_EN);
> - HWRITE4(sc, WDT_TORR, 0);
> - HSET4(sc, WDT_CR, WDT_CR_WDT_EN);
>
> - /* Kick the dog! */
> - HWRITE4(sc, WDT_CRR, WDT_CRR_KICK);
> + sc->sc_reset = 1;
> +
> + control = HREAD4(sc, WDT_CR);
> + control &= ~WDT_CR_RESP_MODE;
> + control |= WDT_CR_ENABLE;
> + HWRITE4(sc, WDT_TORR, 0);
> + HWRITE4(sc, WDT_CR, control);
> + HWRITE4(sc, WDT_CRR, WDT_CRR_MAGIC);
>
> delay(1000000);
> }
> Index: sys/arch/arm64/conf/GENERIC
> ===================================================================
> RCS file: /cvs/src/sys/arch/arm64/conf/GENERIC,v
> retrieving revision 1.215
> diff -u -p -r1.215 GENERIC
> --- sys/arch/arm64/conf/GENERIC 3 Dec 2021 19:16:29 -0000 1.215
> +++ sys/arch/arm64/conf/GENERIC 9 Dec 2021 10:13:21 -0000
> @@ -277,6 +277,7 @@ rkpwm* at fdt?
> rkrng* at fdt?
> rktemp* at fdt?
> rkvop* at fdt?
> +dwdog* at fdt?
> rkdwusb* at fdt?
> dwmmc* at fdt?
> sdmmc* at dwmmc?
>
