> Date: Sun, 13 Feb 2022 14:56:11 +0100 > From: Patrick Wildt <[email protected]> > > Am Sun, Feb 13, 2022 at 02:22:53PM +0100 schrieb Tobias Heider: > > Hey, > > > > I'm trying to get the Turris Omnia running and one thing missing > > is a driver for the armada-380-wdg. We already have a similar driver > > called mvdog(4) that currently only supports the armada-3700 watchdog. > > The diff below adds support for disabling the armada-380-wdg. > > > > ok? > > First of all, it's -wdt. ;) It's a good start, comments inline. > > > > > Index: mvdog.c > > =================================================================== > > RCS file: /mount/openbsd/cvs/src/sys/dev/fdt/mvdog.c,v > > retrieving revision 1.2 > > diff -u -p -r1.2 mvdog.c > > --- mvdog.c 24 Oct 2021 17:52:26 -0000 1.2 > > +++ mvdog.c 13 Feb 2022 13:11:06 -0000 > > @@ -33,6 +33,10 @@ > > > > #define WDT_TIMER_SELECT 0x64 > > Might be worth renaming those as well. > > > +#define ARMADA_380_RSTOUT_MASK_BIT (1 << 10) > > +#define ARMADA_380_RSTOUT_ENABLE_BIT (1 << 8) > > +#define ARMADA_380_WDT_ENABLE_BIT (1 << 8) > > Hm, How about A380 as prefix? But that's just nitpicking. > > > #define HREAD4(sc, reg) > > \ > > (bus_space_read_4((sc)->sc_iot, (sc)->sc_ioh, (reg))) > > #define HWRITE4(sc, reg, val) > > \ > > @@ -47,6 +51,8 @@ struct mvdog_softc { > > bus_space_tag_t sc_iot; > > bus_space_handle_t sc_ioh; > > struct regmap *sc_rm; > > + bus_space_handle_t sc_ioh_rout; > > + bus_space_handle_t sc_ioh_routmask; > > }; > > > > int mvdog_match(struct device *, void *, void *); > > @@ -65,7 +71,8 @@ mvdog_match(struct device *parent, void > > { > > struct fdt_attach_args *faa = aux; > > > > - return OF_is_compatible(faa->fa_node, "marvell,armada-3700-wdt"); > > + return OF_is_compatible(faa->fa_node, "marvell,armada-3700-wdt") || > > + OF_is_compatible(faa->fa_node, "marvell,armada-380-wdt"); > > } > > > > void > > @@ -86,17 +93,39 @@ mvdog_attach(struct device *parent, stru > > return; > > } > > > > - sc->sc_rm = regmap_byphandle(OF_getpropint(faa->fa_node, > > - "marvell,system-controller", 0)); > > - if (sc->sc_rm == NULL) { > > - printf(": can't get regmap\n"); > > - return; > > - } > > - > > printf("\n"); > > This needs to be moved to the end, otherwise in the error case it will > be: > > mvdog0 at simplebus0\n > :can't ... > > > > > - /* Disable watchdog timer. */ > > - HCLR4(sc, CNTR_CTRL(CNTR_WDOG), CNTR_CTRL_ENABLE); > > - HCLR4(sc, CNTR_CTRL(CNTR_RETRIGGER), CNTR_CTRL_ENABLE); > > - regmap_write_4(sc->sc_rm, WDT_TIMER_SELECT, 0); > > + if (OF_is_compatible(faa->fa_node, "marvell,armada-3700-wdt")) { > > + sc->sc_rm = regmap_byphandle(OF_getpropint(faa->fa_node, > > + "marvell,system-controller", 0)); > > + if (sc->sc_rm == NULL) { > > + printf(": can't get regmap\n"); > > + return; > > + } > > + > > + /* Disable watchdog timer. */ > > + HCLR4(sc, CNTR_CTRL(CNTR_WDOG), CNTR_CTRL_ENABLE); > > + HCLR4(sc, CNTR_CTRL(CNTR_RETRIGGER), CNTR_CTRL_ENABLE); > > + regmap_write_4(sc->sc_rm, WDT_TIMER_SELECT, 0); > > + } else { > > While we tend do to if else quite often with those compatibles, I wonder > if more explicit 'if compatible 380' might be better for readability. > > > + if (bus_space_map(sc->sc_iot, faa->fa_reg[1].addr, > > + faa->fa_reg[1].size, 0, &sc->sc_ioh_rout)) { > > + printf(": can't map registers\n"); > > + return; > > + } > > + if (bus_space_map(sc->sc_iot, faa->fa_reg[2].addr, > > + faa->fa_reg[2].size, 0, &sc->sc_ioh_routmask)) { > > + printf(": can't map registers\n"); > > + return; > > + } > > + > > + /* Disable watchdog timer. */ > > + bus_space_write_4(sc->sc_iot, sc->sc_ioh_routmask, 0, > > + bus_space_read_4(sc->sc_iot, sc->sc_ioh_routmask, 0) | > > + ARMADA_380_RSTOUT_MASK_BIT); > > + bus_space_write_4(sc->sc_iot, sc->sc_ioh_rout, 0, > > + bus_space_read_4(sc->sc_iot, sc->sc_ioh_rout, 0) & > > + ~ARMADA_380_RSTOUT_ENABLE_BIT); > > + HCLR4(sc, 0, ARMADA_380_WDT_ENABLE_BIT); > > Not sure I like the mix of bus_space_* and H* macros. Maybe kettenis@ > wants to chime in, he always helps me when I'm overthinking stuff.
I'm not sure this code should share a driver with the A3700 code. The hardware doesn't seem to share any commonalities except for the Marvell name. Linux has a separate driver which it calls "orion_wdt". So maybe mvodog(4) is a good name for a separate driver. Also, since this block doesn't seem to be used on any 64-bit SoCs, the driver probably belongs in sys/arch/armv7/marvell/. Cheers, Mark
