RE: [PATCH 2/3] watchdog: aspeed: Support variable number of reset mask registers

[Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>]

Hi Guenter,

Thanks for the quick review.

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.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@aspeedtech.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