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 11D09C4345F for ; Mon, 29 Apr 2024 01:50:30 +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:To:From:Subject:Message-ID:Reply-To:Cc:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=mnNd5G8OwYkkh2oO4wF9dcO8A8oIHYonELF/j9myzgA=; b=1b+nqrWWWIBij9 gfiLbfgP3tAvuT8kYTn5jvm9U3FjjTP0Tt5UYcXE8DqlaFNX0+yyG/RAPp4DgPkenPqCFxH50Ss+u a15uAW7vMP7hOXB+faFrdgFxduyOg3vFh7uc639tvUSnoIzE7fbgPW3s9g4EKzPEo+UFySvwtP8so CK+iCVJ5hMhTQjLWKNBrzWsZBC5Siku3X1mznWwH2GToiligxIk22RV2qBlmh7dHX8xzkQxrQqMUJ 2OVKFJVmz+9vX3wLfa9xl8qJGRk+SVIdL5Pwpcr+xa51fCDj1NHLknr7tEeuBQtgntbj4nNEphTOD 1F2nQVCHU3gxAzCh40iQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1s1G9j-00000001Bxu-31FH; Mon, 29 Apr 2024 01:50:11 +0000 Received: from pi.codeconstruct.com.au ([203.29.241.158] helo=codeconstruct.com.au) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1s1G9g-00000001Bvp-2soC for linux-arm-kernel@lists.infradead.org; Mon, 29 Apr 2024 01:50:10 +0000 Received: from [192.168.68.112] (ppp14-2-127-66.adl-apt-pir-bras32.tpg.internode.on.net [14.2.127.66]) by mail.codeconstruct.com.au (Postfix) with ESMTPSA id 177562012A; Mon, 29 Apr 2024 09:49:57 +0800 (AWST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=codeconstruct.com.au; s=2022a; t=1714355401; bh=fW7TDqW/JWuLZi/1JzQzm9k+gq4/M8LUsR6rg8cfq/4=; h=Subject:From:To:Date:In-Reply-To:References; b=jZpQYljdIEhLKdS2flf7FPGA3AiPsZcGTztc0/0T9Ywojfx9J118PhLBFUm37LIvg g7vCxJyluqrzSQ6ej00koBxYGs5uli2i+uiothyECrXkxrKI0w0tZetHXns+SLIWhm 0hEKmip+L8etRdbq5ZhkZ1XY9w8PKLVNU99LEu04VzzsB1VCXGAtV6RG4zAslgFEsY y1PZ/lV/Oi3sYsri6A8UDiiO+zfnhWsrk5pMPxAm3KwjkjnAPykjy8JaDFDwJ4CNL9 Ys4agRzeuQd0x2t70a9MTQTYvkYk3BUj8s8D3NWFcosniVYdRBc1iRRBvo13jEVAke p7ut3OdU/QgBw== Message-ID: Subject: Re: [PATCH v8 1/1] drivers: watchdog: revise watchdog bootstatus From: Andrew Jeffery To: Peter Yin , patrick@stwcx.xyz, Wim Van Sebroeck , Guenter Roeck , Joel Stanley , linux-watchdog@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-aspeed@lists.ozlabs.org, linux-kernel@vger.kernel.org Date: Mon, 29 Apr 2024 11:19:54 +0930 In-Reply-To: <20240428142937.785925-2-peteryin.openbmc@gmail.com> References: <20240428142937.785925-1-peteryin.openbmc@gmail.com> <20240428142937.785925-2-peteryin.openbmc@gmail.com> 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-20240428_185009_184180_C166F353 X-CRM114-Status: GOOD ( 40.82 ) 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 Hi Peter, Thanks for reworking the patch to reduce the branching in probe(), it looks a lot tidier. First, regarding the patch subject, looking at recent changes to the watchdog subsystem the desired pattern appears to be `watchdog: : `. I expect you should change it to `watchdog: aspeed: Revise handling of bootstatus`. Currently the subject contains `drivers: ` which feels a bit redundant, and fails to mention `aspeed`, which will bound the scope of the patch for people skimming the mailing list. I have a bit of feedback below. It looks like a lot but mostly it's nitpicking at how we're naming things. Maybe the comments are a bit subjective but I think addressing them will help provide consistency for readers of the code. On Sun, 2024-04-28 at 22:29 +0800, Peter Yin wrote: > Regarding the AST2600 specification, the WDTn Timeout Status Register > (WDT10) has bit 1 reserved. Bit 1 of the status register indicates > on ast2500 if the boot was from the second boot source. > It does not indicate that the most recent reset was triggered by > the watchdog. The code should just be changed to set WDIOF_CARDRESET > if bit 0 of the status register is set. However, this bit can be clear when > watchdog register 0x0c bit1(Reset System after timeout) is enabled. > Thereforce include SCU register to veriy WDIOF_EXTERN1 and WDIOF_CARDRESET > in ast2600 SCU74 or ast2400/ast2500 SCU3C. > > Signed-off-by: Peter Yin > --- > drivers/watchdog/aspeed_wdt.c | 78 +++++++++++++++++++++++++++++++---- > 1 file changed, 70 insertions(+), 8 deletions(-) > > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c > index b4773a6aaf8c..4393625c2e96 100644 > --- a/drivers/watchdog/aspeed_wdt.c > +++ b/drivers/watchdog/aspeed_wdt.c > @@ -11,10 +11,12 @@ > #include > #include > #include > +#include > #include > #include > #include > #include > +#include > #include > > static bool nowayout = WATCHDOG_NOWAYOUT; > @@ -22,10 +24,32 @@ module_param(nowayout, bool, 0); > MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" > __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); > > +//AST SCU Register Can you unpack in the comment which register this refers to? Also I have a mild preference for `/* */-style comments and against the `//`- style comments, but I won't hold the patch up on it. > +#define POWERON_RESET_FLAG BIT(0) > +#define EXTERN_RESET_FLAG BIT(1) IMO an `AST_` prefix would be helpful. At least, it would help me orient myself when reading use of the macro in the code. Further, can we include `SCU` in the symbol name to indicate we're not actually referring to a register in the WDT controller (and update the register and flag macros below as well)? Finally, including an indication of the register name (System Reset Control/Status Register for the AST2500, System Reset Status Register for the AST2600) is helpful too: Perhaps: ``` #define AST_SCU_SYS_RESET_POWERON_FLAG ... #define AST_SCU_SYS_RESET_EXTERN_FLAG ... ``` I'd like to see these approaches applied to the other macros you've introduced as well. > + > +#define AST2400_AST2500_SYSTEM_RESET_EVENT 0x3C If the AST2500 register offset is compatible with the AST2400 then IMO you can drop `_AST2500` from the macro name. The location of relevance for a potential bug is the assignment into the `reset_event` struct member below, which is straight-forward to inspect for correctness. With the prior requests in mind I'd propose: ``` #define AST2400_SCU_SYS_RESET_STATUS ... ``` > +#define AST2400_WATCHDOG_RESET_FLAG BIT(1) > +#define AST2400_RESET_FLAG_CLEAR GENMASK(2, 0) > + > +#define AST2500_WATCHDOG_RESET_FLAG GENMASK(4, 2) While the individual bits in the register are flags, we're extracting a collection of the bits from the register. My feeling is that we should s/_FLAG/_MASK/ in the macro names, including `AST2400_WATCHDOG_RESET_FLAG` for consistency (even though it is only a single-bit mask). > +#define AST2500_RESET_FLAG_CLEAR (AST2500_WATCHDOG_RESET_FLAG | \ > + POWERON_RESET_FLAG | EXTERN_RESET_FLAG) > + > +#define AST2600_SYSTEM_RESET_EVENT 0x74 > +#define AST2600_WATCHDOG_RESET_FLAG GENMASK(31, 16) > +#define AST2600_RESET_FLAG_CLEAR (AST2600_WATCHDOG_RESET_FLAG | \ > + POWERON_RESET_FLAG | EXTERN_RESET_FLAG) > + > struct aspeed_wdt_config { > u32 ext_pulse_width_mask; > u32 irq_shift; > u32 irq_mask; > + const char *compatible; Hmm, a compatible string for what though? From the looks of the code, this is for the SCU. I think it would be be helpful to prefix this with `scu_` to make it clear, though see the struct-style consideration below. > + u32 reset_event; The datasheets refer to the register as 'status' and not 'event', so I suggest we use `reset_status` here. I also prefer we suffix this with `_reg` to actively differentiate it from the other field types (_flag) we're defining (so `reset_status_reg`. > + u32 watchdog_reset_flag; > + u32 extern_reset_flag; s/_flag/_mask/ if we have consensus on that macro name discussion above. > + u32 reset_flag_clear; I'd prefix these with `scu_` as well. Or perhaps a nested struct? struct aspeed_wdt_config { ... struct { const char *compatible; u32 reset_event_reg; u32 watchdog_reset_mask; u32 extern_reset_mask; u32 reset_flag_clear; } scu; That way the accesses look like wdt->cfg->scu.reset_event_reg` and provide some context via the type system instead of deferring to object naming convention. > }; > > struct aspeed_wdt { > @@ -39,18 +63,33 @@ static const struct aspeed_wdt_config ast2400_config = { > .ext_pulse_width_mask = 0xff, > .irq_shift = 0, > .irq_mask = 0, > + .compatible = "aspeed,ast2400-scu", > + .reset_event = AST2400_AST2500_SYSTEM_RESET_EVENT, > + .watchdog_reset_flag = AST2400_WATCHDOG_RESET_FLAG, > + .extern_reset_flag = 0, > + .reset_flag_clear = AST2400_RESET_FLAG_CLEAR, > }; > > static const struct aspeed_wdt_config ast2500_config = { > .ext_pulse_width_mask = 0xfffff, > .irq_shift = 12, > .irq_mask = GENMASK(31, 12), > + .compatible = "aspeed,ast2500-scu", > + .reset_event = AST2400_AST2500_SYSTEM_RESET_EVENT, > + .watchdog_reset_flag = AST2500_WATCHDOG_RESET_FLAG, > + .extern_reset_flag = EXTERN_RESET_FLAG, > + .reset_flag_clear = AST2500_RESET_FLAG_CLEAR, > }; > > static const struct aspeed_wdt_config ast2600_config = { > .ext_pulse_width_mask = 0xfffff, > .irq_shift = 0, > .irq_mask = GENMASK(31, 10), > + .compatible = "aspeed,ast2600-scu", > + .reset_event = AST2600_SYSTEM_RESET_EVENT, > + .watchdog_reset_flag = AST2600_WATCHDOG_RESET_FLAG, > + .extern_reset_flag = EXTERN_RESET_FLAG, > + .reset_flag_clear = AST2600_RESET_FLAG_CLEAR, > }; > > static const struct of_device_id aspeed_wdt_of_table[] = { > @@ -310,6 +349,7 @@ static int aspeed_wdt_probe(struct platform_device *pdev) > const struct of_device_id *ofdid; > struct aspeed_wdt *wdt; > struct device_node *np; > + struct regmap *scu_base; I don't think it's necessary to have the `_base` suffix as we're not dealing directly with a mapped address. > const char *reset_type; > u32 duration; > u32 status; > @@ -458,14 +498,36 @@ static int aspeed_wdt_probe(struct platform_device *pdev) > writel(duration - 1, wdt->base + WDT_RESET_WIDTH); > } > > - status = readl(wdt->base + WDT_TIMEOUT_STATUS); > - if (status & WDT_TIMEOUT_STATUS_BOOT_SECONDARY) { Dropping this condition suggests the patch is a fix. Has there been any discussion of adding a Fixes: tag? Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel