Message-ID: <e6b82ec9-c19b-4210-9251-2beee30c6d90@roeck-us.net>
Date:   Mon, 25 Sep 2023 17:35:28 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Zev Weiss <zev@...ilderbeest.net>
Cc:     Andrew Jeffery <andrew@...id.au>, g@...ter.bewilderbeest.net,
        Conor Dooley <conor+dt@...nel.org>,
        Joel Stanley <joel@....id.au>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        "Milton D. Miller II" <mdmii@...look.com>,
        Rob Herring <robh+dt@...nel.org>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-aspeed@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        linux-watchdog@...r.kernel.org, openbmc@...ts.ozlabs.org,
        Eddie James <eajames@...ux.ibm.com>,
        Ivan Mikhaylov <i.mikhaylov@...ro.com>,
        Thomas Weißschuh <linux@...ssschuh.net>
Subject: Re: [PATCH 1/2] dt-bindings: watchdog: aspeed-wdt: Add
 aspeed,reset-mask property

On Mon, Sep 25, 2023 at 05:04:10PM -0700, Zev Weiss wrote:
> On Sun, Sep 24, 2023 at 07:42:45PM PDT, Andrew Jeffery wrote:
> > 
> > 
> > On Fri, 22 Sep 2023, at 20:12, Zev Weiss wrote:
> > > This property configures the Aspeed watchdog timer's reset mask, which
> > > controls which peripherals are reset when the watchdog timer expires.
> > > Some platforms require that certain devices be left untouched across a
> > > reboot; aspeed,reset-mask can now be used to express such constraints.
> > > 
> > > Signed-off-by: Zev Weiss <zev@...ilderbeest.net>
> > > ---
> > >  .../bindings/watchdog/aspeed-wdt.txt          | 18 +++-
> > >  include/dt-bindings/watchdog/aspeed-wdt.h     | 92 +++++++++++++++++++
> > >  2 files changed, 109 insertions(+), 1 deletion(-)
> > >  create mode 100644 include/dt-bindings/watchdog/aspeed-wdt.h
> > > 
> > > diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> > > b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> > > index a8197632d6d2..3208adb3e52e 100644
> > > --- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> > > +++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> > > @@ -47,7 +47,15 @@ Optional properties for AST2500-compatible watchdogs:
> > >  			   is configured as push-pull, then set the pulse
> > >  			   polarity to active-high. The default is active-low.
> > > 
> > > -Example:
> > > +Optional properties for AST2500- and AST2600-compatible watchdogs:
> > > + - aspeed,reset-mask: A bitmask indicating which peripherals will be reset if
> > > +		      the watchdog timer expires.  On AST2500 this should be a
> > > +		      single word defined using the AST2500_WDT_RESET_* macros;
> > > +		      on AST2600 this should be a two-word array with the first
> > > +		      word defined using the AST2600_WDT_RESET1_* macros and the
> > > +		      second word defined using the AST2600_WDT_RESET2_* macros.
> > > +
> > > +Examples:
> > > 
> > >  	wdt1: watchdog@...85000 {
> > >  		compatible = "aspeed,ast2400-wdt";
> > > @@ -55,3 +63,11 @@ Example:
> > >  		aspeed,reset-type = "system";
> > >  		aspeed,external-signal;
> > >  	};
> > > +
> > > +	#include <dt-bindings/watchdog/aspeed-wdt.h>
> > > +	wdt2: watchdog@...85040 {
> > > +		compatible = "aspeed,ast2600-wdt";
> > > +		reg = <0x1e785040 0x40>;
> > > +		aspeed,reset-mask = <AST2600_WDT_RESET1_DEFAULT
> > > +				     (AST2600_WDT_RESET2_DEFAULT & ~AST2600_WDT_RESET2_LPC)>;
> > > +	};
> > 
> > Rob has acked your current approach already, but I do wonder about an
> > alternative that aligns more with the clock/reset/interrupt properties.
> > Essentially, define a new generic watchdog property that is specified on
> > the controllers to be reset by the watchdog (or even on just the
> > watchdog node itself, emulating what you've proposed here):
> > 
> > watchdog-resets = <phandle index>;
> > 
> > The phandle links to the watchdog of interest, and the index specifies
> > the controller associated with the configuration. It might even be
> > useful to do:
> > 
> > watchdog-resets = <phandle index enable>;
> > 
> > "enable" could provide explicit control over whether somethings should
> > be reset or not (as a way to prevent reset if the controller targeted by
> > the provided index would otherwise be reset in accordance with the
> > default reset value in the watchdog controller).
> > 
> > The macros from the dt-bindings header can then use macros to name the
> > indexes rather than define a mask tied to the register layout. The index
> > may still in some way represent the mask position. This has the benefit
> > of hiding the issue of one vs two configuration registers between the
> > AST2500 and AST2600 while also allowing other controllers to exploit the
> > binding (Nuvoton BMCs? Though maybe it's generalising too early?).
> > 
> 
> Sorry, I'm having a bit of a hard time picturing exactly what you're
> suggesting here...to start with:
> 
> > property that is specified on the controllers to be reset by the
> > watchdog
> 
> and
> 
> > or even on just the watchdog node itself
> 
> seem on the face of it like two fairly different approaches to me.  The
> former sounds more like existing clock/reset/etc. stuff, where the
> peripheral has a property describing its relationship to the "central"
> subsystem, and various peripheral drivers are all individually responsible
> for observing that property and calling in to the central subsystem to
> configure things for that peripheral appropriately; if I'm understanding you
> correctly, it might look something like:
> 
>   &spi1 {
>     watchdog-resets = <&wdt1 WDT_INDEX_SPI1 0>;
>   };
> 
> Or maybe something more like how pinctrl works, via phandles to subnodes of
> the central device?
> 
>   &wdt1 {
>     wdt1_spi1_reset: spi1_reset {
>       reg = <0x1c>;
>       bit = <24>;
>     };
>   };
> 
>   &spi1 {
>     watchdog-resets = <&wdt1_spi1_reset 0>;
>   };
> 
> Either way, it seems like it'd be complicated by any insufficient
> granularity in the watchdog w.r.t. having independent control over the
> individual devices represented by separate DT nodes (such as how the AST2500
> watchdog has a single SPI controller reset bit instead of one per SPI
> interface, or its "misc SOC controller" bit governing all sorts of odds and
> ends).
> 
> In the latter case (property on the wdt node), would it essentially just be
> kind of an indirection layer mapping hardware-independent device indices to
> specific registers/bits?  It's not obvious to me what purpose a phandle to
> the peripheral device node would serve (would the wdt driver have a good way
> of identifying what specific peripheral it's pointing to to know what bit to
> twiddle?), but maybe I'm misunderstanding what you're suggesting...
> 
> 
> I guess my other uncertainty is the balance between generalization and
> applicability -- how many other watchdog devices have sufficient comparable
> configurability to make use of it?  I haven't pored over all of them, but
> from a random sampling of 20 so of the other existing wdt drivers I don't
> see any obvious candidates -- the closest I saw were cpwd.c, which
> apparently can distinguish between a CPU reset and a CPU/backplane/board
> reset, and realtek_otto_wdt.c, which can do a CPU or a SOC reset (though I
> don't have any of the hardware docs to know what capabilities other devices
> might provide that the drivers don't use).  Do the Nuvoton BMCs have
> watchdogs with peripheral-granularity reset configuration?
> 

Quite frankly, I don't like where this is going. It is getting way
too complicated. And when something is becoming way too complicated,
I tend to put it on my backburner list. The length of that list quickly
approaches maxint.

Guenter

Powered by blists - more mailing lists