lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f7148ed6d4ef835d60fd1aa7141b5f0a9cb69eff.camel@codeconstruct.com.au>
Date: Thu, 31 Oct 2024 10:33:40 +1030
From: Andrew Jeffery <andrew@...econstruct.com.au>
To: Chin-Ting Kuo <chin-ting_kuo@...eedtech.com>, patrick@...cx.xyz, 
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, joel@....id.au, 
 wim@...ux-watchdog.org, linux@...ck-us.net,
 linux-arm-kernel@...ts.infradead.org,  linux-aspeed@...ts.ozlabs.org,
 linux-kernel@...r.kernel.org,  linux-watchdog@...r.kernel.org
Cc: Peter.Yin@...ntatw.com, Patrick_NC_Lin@...ynn.com, Bonnie_Lo@...ynn.com,
  DELPHINE_CHIU@...ynn.com, bmc-sw@...eedtech.com,
 chnguyen@...erecomputing.com
Subject: Re: [PATCH v3 2/2] watchdog: aspeed: Add support for SW restart

On Wed, 2024-10-30 at 18:47 +0800, Chin-Ting Kuo wrote:
> Since AST2600, except for HW WDT counter timeout, HW WDT
> reset can also be triggered by just cinfiguring some
> HW registers by SW directly. We named it "SW restart".
> Although it is "SW" restart, its mechanism is implemented
> by HW.
> 
> Originally, system can only know it is reset by WDT
> through a reset flag. However, since AST2600, SW can
> trigger the reset event consciously and directly without
> wait for WDT timeout. WDT counter is not enabled when
> SW restart is adopted. After that, an independent reset
> event flag will be set after systemis reset by SW.
> 
> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@...eedtech.com>
> ---
>  drivers/watchdog/aspeed_wdt.c | 40
> +++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/watchdog/aspeed_wdt.c
> b/drivers/watchdog/aspeed_wdt.c
> index add76be3ee42..1e9808d42023 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -42,6 +42,9 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be
> stopped once started (default="
>  
>  #define WDT_REG_OFFSET_MASK            0x00000fff
>  
> +/* WDT behavior control flag */
> +#define WDT_RESTART_SYSTEM_SW_SUPPORT  0x00000001
> +
>  struct aspeed_wdt_scu {
>         const char *compatible;
>         u32 reset_status_reg;
> @@ -55,6 +58,7 @@ struct aspeed_wdt_config {
>         u32 irq_shift;
>         u32 irq_mask;
>         u32 reg_size;
> +       u32 flags;

Why add the flags member rather than change the restart callback for
the 2600? The latter seems more direct to me.

>         struct aspeed_wdt_scu scu;
>  };
>  
> @@ -71,6 +75,7 @@ static const struct aspeed_wdt_config
> ast2400_config = {
>         .irq_shift = 0,
>         .irq_mask = 0,
>         .reg_size = 0x20,
> +       .flags = 0,
>         .scu = {
>                 .compatible = "aspeed,ast2400-scu",
>                 .reset_status_reg = AST2400_SCU_SYS_RESET_STATUS,
> @@ -85,6 +90,7 @@ static const struct aspeed_wdt_config
> ast2500_config = {
>         .irq_shift = 12,
>         .irq_mask = GENMASK(31, 12),
>         .reg_size = 0x20,
> +       .flags = 0,
>         .scu = {
>                 .compatible = "aspeed,ast2500-scu",
>                 .reset_status_reg = AST2400_SCU_SYS_RESET_STATUS,
> @@ -99,6 +105,7 @@ static const struct aspeed_wdt_config
> ast2600_config = {
>         .irq_shift = 0,
>         .irq_mask = GENMASK(31, 10),
>         .reg_size = 0x40,
> +       .flags = WDT_RESTART_SYSTEM_SW_SUPPORT,
>         .scu = {
>                 .compatible = "aspeed,ast2600-scu",
>                 .reset_status_reg = AST2600_SCU_SYS_RESET_STATUS,
> @@ -136,6 +143,11 @@ MODULE_DEVICE_TABLE(of, aspeed_wdt_of_table);
>  #define   WDT_CLEAR_TIMEOUT_AND_BOOT_CODE_SELECTION    BIT(0)
>  #define WDT_RESET_MASK1                0x1c
>  #define WDT_RESET_MASK2                0x20
> +#define WDT_SW_RESET_CTRL      0x24
> +#define   WDT_SW_RESET_COUNT_CLEAR     0xDEADDEAD
> +#define   WDT_SW_RESET_ENABLE  0xAEEDF123
> +#define WDT_SW_RESET_MASK1     0x28
> +#define WDT_SW_RESET_MASK2     0x2c
>  
>  /*
>   * WDT_RESET_WIDTH controls the characteristics of the external
> pulse (if
> @@ -255,10 +267,31 @@ static int aspeed_wdt_set_pretimeout(struct
> watchdog_device *wdd,
>         return 0;
>  }
>  
> +static void aspeed_wdt_sw_reset(struct watchdog_device *wdd)
> +{
> +       struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> +       u32 ctrl = WDT_CTRL_RESET_MODE_SOC |
> +                  WDT_CTRL_RESET_SYSTEM;
> +
> +       writel(ctrl, wdt->base + WDT_CTRL);
> +       writel(WDT_SW_RESET_COUNT_CLEAR,
> +              wdt->base + WDT_SW_RESET_CTRL);
> +       writel(WDT_SW_RESET_ENABLE, wdt->base + WDT_SW_RESET_CTRL);
> +
> +       /* system must be reset immediately */
> +       mdelay(1000);
> +}
> +
>  static int aspeed_wdt_restart(struct watchdog_device *wdd,
>                               unsigned long action, void *data)
>  {
>         struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> +       const struct aspeed_wdt_config *cfg = wdt->cfg;
> +
> +       if (cfg->flags & WDT_RESTART_SYSTEM_SW_SUPPORT) {
> +               aspeed_wdt_sw_reset(wdd);
> +               return 0;
> +       }
>  
>         wdt->ctrl &= ~WDT_CTRL_BOOT_SECONDARY;
>         aspeed_wdt_enable(wdt, 128 * WDT_RATE_1MHZ / 1000);
> @@ -529,6 +562,13 @@ static int aspeed_wdt_probe(struct
> platform_device *pdev)
>                         if (nrstmask > 1)
>                                 writel(reset_mask[1], wdt->base +
> WDT_RESET_MASK2);
>                 }
> +
> +               if (wdt->cfg->flags & WDT_RESTART_SYSTEM_SW_SUPPORT)

This condition could be a match against the compatible if you drop the
flags member.

Or, assuming the software reset masks are not adjusted elsewhere, move
the copies below into the ast2600-specific restart callback
implementation?

Andrew

> {
> +                       reg = readl(wdt->base + WDT_RESET_MASK1);
> +                       writel(reg, wdt->base + WDT_SW_RESET_MASK1);
> +                       reg = readl(wdt->base + WDT_RESET_MASK2);
> +                       writel(reg, wdt->base + WDT_SW_RESET_MASK2);
> +               }
>         }
>  
>         if (!of_property_read_u32(np, "aspeed,ext-pulse-duration",
> &duration)) {

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ