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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <TYZPR06MB52035407C39249441F09AA18B2E1A@TYZPR06MB5203.apcprd06.prod.outlook.com>
Date: Wed, 8 Oct 2025 03:29:51 +0000
From: Chin-Ting Kuo <chin-ting_kuo@...eedtech.com>
To: Guenter Roeck <linux@...ck-us.net>, "wim@...ux-watchdog.org"
	<wim@...ux-watchdog.org>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "joel@....id.au" <joel@....id.au>,
	"andrew@...econstruct.com.au" <andrew@...econstruct.com.au>,
	"linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-aspeed@...ts.ozlabs.org"
	<linux-aspeed@...ts.ozlabs.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, BMC-SW <BMC-SW@...eedtech.com>
Subject: RE: [PATCH 2/3] watchdog: aspeed: Support variable number of reset
 mask registers

Hi Guenter,

Thanks for the quick review.

> -----Original Message-----
> From: Guenter Roeck <groeck7@...il.com> On Behalf Of Guenter Roeck
> Sent: Tuesday, October 7, 2025 10:55 PM
> Subject: Re: [PATCH 2/3] watchdog: aspeed: Support variable number of reset
> mask registers
> 
> On 10/7/25 01:36, Chin-Ting Kuo wrote:
> > Starting from the AST2600 platform, the SoC design has become more
> > complex, with an increased number of reset mask registers.
> > To support this, introduce a new field 'num_reset_masks' in the
> > 'aspeed_wdt_config' structure to specify the number of reset mask
> > registers per platform. This change removes the need for hardcoded
> > platform-specific logic and improves scalability for future SoCs.
> >
> > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@...eedtech.com>
> > ---
> >   drivers/watchdog/aspeed_wdt.c | 12 ++++++++----
> >   1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/watchdog/aspeed_wdt.c
> > b/drivers/watchdog/aspeed_wdt.c index 837e15701c0e..e15f70c5e416
> > 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -35,6 +35,7 @@ struct aspeed_wdt_config {
> >   	u32 irq_shift;
> >   	u32 irq_mask;
> >   	struct aspeed_wdt_scu scu;
> > +	u32 num_reset_masks;
> >   };
> >
> >   struct aspeed_wdt {
> > @@ -54,6 +55,7 @@ static const struct aspeed_wdt_config ast2400_config =
> {
> >   		.wdt_reset_mask = 0x1,
> >   		.wdt_reset_mask_shift = 1,
> >   	},
> > +	.num_reset_masks = 1,
> 
> Looking at this again: Why set it on ast2400 ?
> 

This should be removed for AST2400 platform. I will update it in the next patch version.

> >   };
> >
> >   static const struct aspeed_wdt_config ast2500_config = { @@ -66,6
> > +68,7 @@ static const struct aspeed_wdt_config ast2500_config = {
> >   		.wdt_reset_mask = 0x1,
> >   		.wdt_reset_mask_shift = 2,
> >   	},
> > +	.num_reset_masks = 1,
> >   };
> >
> >   static const struct aspeed_wdt_config ast2600_config = { @@ -78,6
> > +81,7 @@ static const struct aspeed_wdt_config ast2600_config = {
> >   		.wdt_reset_mask = 0xf,
> >   		.wdt_reset_mask_shift = 16,
> >   	},
> > +	.num_reset_masks = 2,
> >   };
> >
> >   static const struct of_device_id aspeed_wdt_of_table[] = { @@ -482,8
> > +486,9 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> >   	if ((of_device_is_compatible(np, "aspeed,ast2500-wdt")) ||
> >   		(of_device_is_compatible(np, "aspeed,ast2600-wdt"))) {
> 
> ... because the code here only evaluates it if this is an ast2500 or ast2600.
> 
> If num_reset_masks would be set to 0 for ast2400, the value could be used
> here.
> 
> 	if (wdt->cfg->num_reset_masks) {
> 		...
> 	}
> 
> and it would not be necessary to add of_device_is_compatible() for new chips.
> 

This "if" conditional statement includes not only reset mask configuration but also pulse polarity and driving type of reset output signal.
How about changing this "if" statement to the below one?
	if (!of_device_is_compatible(np, "aspeed,ast2400-wdt")) {
		...
	}
It will also not need to add of_device_is_compatible() for new chips.

> Guenter
> 
> >   		u32 reset_mask[2];
> > -		size_t nrstmask = of_device_is_compatible(np,
> "aspeed,ast2600-wdt") ? 2 : 1;
> > +		size_t nrstmask = wdt->cfg->num_reset_masks;
> >   		u32 reg = readl(wdt->base + WDT_RESET_WIDTH);
> > +		int i;
> >
> >   		reg &= wdt->cfg->ext_pulse_width_mask;
> >   		if (of_property_read_bool(np, "aspeed,ext-active-high")) @@
> -503,9
> > +508,8 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> >
> >   		ret = of_property_read_u32_array(np, "aspeed,reset-mask",
> reset_mask, nrstmask);
> >   		if (!ret) {
> > -			writel(reset_mask[0], wdt->base + WDT_RESET_MASK1);
> > -			if (nrstmask > 1)
> > -				writel(reset_mask[1], wdt->base + WDT_RESET_MASK2);
> > +			for (i = 0; i < nrstmask; i++)
> > +				writel(reset_mask[i], wdt->base + WDT_RESET_MASK1 + i
> * 4);
> >   		}
> >   	}
> >

Chin-Ting

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ