From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 8CEA1E81806 for ; Tue, 26 Sep 2023 00:58:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Date:Cc:To:From:Subject:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=F7lx8XInImArO/+g4wc3wdX2lpE9xAAde581yL+mMKM=; b=QrECY5tqRllzNL 1cmHArAkbnY/XRRIhsTlHAWKv9iIOF7+la5CEdLeUbhwZr6WV0Vq0d4uwbUu6bbuy6OYPF8aOL+ZF Ymzs8uwmaYneeXaGkIM72UVxwl67u6TXGgQyM40i55DyjLvPVey6hjJaksW3GPHhuniSiEaFB+5Sv CZ/z3bjgABPoJNd6ufE9Zey6qVxqEs02bunCJmalSq5e1duxF+RNiUXYqi1b89U5wlRZrkBltvoW6 VydPLhY/FD28gw7phWs9UXNDzvnmwt6RGEOgZAgQrit2yISrkF0thzw9Gsd4SUhNxKBY846DSv5co VsyKt5+bWQsgMomxz5JA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qkwOt-00FK5Q-0Z; Tue, 26 Sep 2023 00:58:07 +0000 Received: from pi.codeconstruct.com.au ([203.29.241.158] helo=codeconstruct.com.au) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qkwOp-00FK3d-0Y for linux-arm-kernel@lists.infradead.org; Tue, 26 Sep 2023 00:58:05 +0000 Received: from [192.168.68.112] (ppp118-210-175-231.adl-adc-lon-bras34.tpg.internode.on.net [118.210.175.231]) by mail.codeconstruct.com.au (Postfix) with ESMTPSA id DDB5B20075; Tue, 26 Sep 2023 08:57:46 +0800 (AWST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=codeconstruct.com.au; s=2022a; t=1695689871; bh=il9pZHitpvQPocfosyxq64m7qx5tIUZKMM6PXb+s62k=; h=Subject:From:To:Cc:Date:In-Reply-To:References; b=MQzeTH5alD6g2hNmisWGJknBk91CflzzODcRg5LoMfyPQAMZYJu4LuTjAcjDtAOLR shyC8m27Jg9jxVYShrkXV0nhIP9cx3Re+BR4kTiYh47rjO6QTuwd0WlK0qbnqP9HCH RGhQSEtWrIGM5BTcmNRPvJINxUL5jF1p7Hr+b7FqVMaAyitbEBYDx19/htNdYJzb1n obNcw4r/mJ72J//8KfkkycqdOLS4sjTFYVLYiKCKDnusyaBuZP3DLvGq12QlvW/6Mh JC1Ay0NyAeSyCpkGmljO+HgTr1kW07AB6CenNX/wTExtaIXg7gGEIaIydNXg5zO0K+ QmwvbfUNLkhoQ== Message-ID: <20e3bef8282b1dd412020ecb24d90cbd89f39756.camel@codeconstruct.com.au> Subject: Re: [PATCH 1/2] dt-bindings: watchdog: aspeed-wdt: Add aspeed,reset-mask property From: Andrew Jeffery To: Guenter Roeck , Zev Weiss Cc: devicetree@vger.kernel.org, Conor Dooley , Wim Van Sebroeck , linux-watchdog@vger.kernel.org, linux-aspeed@lists.ozlabs.org, Andrew Jeffery , openbmc@lists.ozlabs.org, Eddie James , linux-kernel@vger.kernel.org, Thomas =?ISO-8859-1?Q?Wei=DFschuh?= , g@hatter.bewilderbeest.net, Rob Herring , Joel Stanley , Krzysztof Kozlowski , Ivan Mikhaylov , "Milton D. Miller II" , linux-arm-kernel@lists.infradead.org Date: Tue, 26 Sep 2023 10:27:45 +0930 In-Reply-To: References: <20230922104231.1434-4-zev@bewilderbeest.net> <20230922104231.1434-5-zev@bewilderbeest.net> <6df088a6-75ab-42f2-ba39-3f155714ed2d@app.fastmail.com> <6b0d4901-d543-4a06-a1e4-7f1558f5361f@hatter.bewilderbeest.net> User-Agent: Evolution 3.46.4-2 MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230925_175803_613384_D2BED3EF X-CRM114-Status: GOOD ( 56.08 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, 2023-09-25 at 17:35 -0700, Guenter Roeck wrote: > 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 > > > > --- > > > > .../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@1e785000 { > > > > compatible = "aspeed,ast2400-wdt"; > > > > @@ -55,3 +63,11 @@ Example: > > > > aspeed,reset-type = "system"; > > > > aspeed,external-signal; > > > > }; > > > > + > > > > + #include > > > > + wdt2: watchdog@1e785040 { > > > > + compatible = "aspeed,ast2600-wdt"; > > > > + reg = <0x1e785040 0x40>; > > > > + aspeed,reset-mask = > > > + (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 = ; > > > > > > 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 = ; > > > > > > "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. > No worries. I figured I should at least give the idea some air-time, even if we did end up discounting it. I feel my description and Zev's queries make it sound more complex than it might be in practice but I also haven't written the patch, so never mind! Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel