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.
Cheers,
Patrick
> + }
> }
>
