Re: [PATCH v2 2/2] watchdog: s3c2410_wdt: Add exynos5515-wdt compatible data

Hi Guenter,

> Aiden,
> 
> On 8/18/26 08:53, [email protected] wrote:
> > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> > - [High] The `drv_data_exynos5515` variant includes `QUIRK_HAS_PMU_AUTO_DISABLE` but fails to define `.disable_reg`, resulting in PMU register corruption at offset 0x0.
> > - [High] The `drv_data_exynos5515` variant uses an INT_EN register for masking but fails to set `.mask_reset_inv = true`, inverting the watchdog masking logic.
> > --
> >
> Please either address or explain why those are non-issues.
> 

For the 'disable_reg' issue Sashiko flagged, downstream says this:

> /* PMU registers are changed.
>  * MASK_RESET register was replaced by CLUSTERx_NONCPU_INT_TYPE register.
>  * DISABLE register was removed and its value was always fixed by 0.
>  */

Which is why I omitted disable_reg. It no longer exists.

Looking closer just now, A few months ago when I was implementing this
I mistook pmu_reset_func for auto_disable_func. There is no
auto_disable_func set in the struct corresponding to exynos5515 and so
the QUIRK_HAS_PMU_AUTO_DISABLE flag should be removed. I'm not sure how
I did not catch that. Will fix in v3.

As for the mask inversion issue, similar story. Will also fix in v3.

I've been using this driver for the past few months and it has been
working, I assumed it poked the watchdog every so often but I suppose
with these problems it disables it completely by default. I will keep
that in mind when testing future contributions to this driver.

> Guenter
> 
> > --- Patch [2]: [PATCH v2 2/2] watchdog: s3c2410_wdt: Add exynos5515-wdt compatible data ---
> > commit fd223d0c16451bec63089d85a77c3b012b8f28ed
> > Author: Aiden Isik <[email protected]>
> >
> > watchdog: s3c2410_wdt: Add exynos5515-wdt compatible data
> >
> > Add driver data for the Exynos5515 SoC's watchdog timer.
> >
> >> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> >> index e31f93db05096..7299dfdb0c099 100644
> >> --- a/drivers/watchdog/s3c2410_wdt.c
> >> +++ b/drivers/watchdog/s3c2410_wdt.c
> >> @@ -232,6 +232,16 @@ static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
> >>   		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_AUTO_DISABLE,
> >>   };
> >>
> >> +static const struct s3c2410_wdt_variant drv_data_exynos5515 = {
> >> +	.mask_reset_reg = EXYNOSAUTOV920_CLUSTER0_NONCPU_INT_EN,
> >
> > [Severity: High]
> > This variant uses an INT_EN register for masking but omits .mask_reset_inv.
> > Could this cause the watchdog masking logic to be inverted?
> >
> > Looking at s3c2410wdt_mask_wdt_reset(), the logic evaluates to false when
> > unmasking if .mask_reset_inv is not set to true:
> >
> > s3c2410wdt_mask_wdt_reset() {
> > 	...
> > 	const bool val_inv = wdt->drv_data->mask_reset_inv;
> > 	const u32 val = (mask ^ val_inv) ? mask_val : 0;
> > 	...
> > 	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->mask_reset_reg,
> > 				 mask_val, val);
> >
> > Does this mean it will write a 0 when unmasking, disabling the reset output
> > instead of enabling it?
> >
> >> +	.mask_bit = 2,
> >> +	.rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
> >> +	.rst_stat_bit = 24,
> >> +	.quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | \
> >> +		  QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_AUTO_DISABLE | \
> >
> > [Severity: High]
> > Since QUIRK_HAS_PMU_AUTO_DISABLE is set but .disable_reg is omitted, will
> > .disable_reg default to 0x0?
> >
> > During initialization and suspend, s3c2410wdt_disable_wdt_reset() applies
> > updates to wdt->drv_data->disable_reg:
> >
> > s3c2410wdt_disable_wdt_reset() {
> > 	...
> > 	ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->disable_reg,
> > 				 mask_val, val);
> >
> > Could this unintentionally corrupt bit 2 at PMU offset 0x0?
> >
> >> +		  QUIRK_HAS_DBGACK_BIT,
> >> +};
> >> +
> >
> > [ ... ]
> >
> 
> 

Kind regards,
- Aiden Isik