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

> On 8/19/26 16:36, Aiden Isik wrote:
> > Hi again Guenter,
> >
> >> Hi Guenter,
> >>
> >>> On 8/18/26 12:35, Aiden Isik wrote:
> >>>> Hi Guenter,
> >>>>
> >>>>> On 8/18/26 11:33, Aiden Isik wrote:
> >>>>>>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> >>>>>>> - [Medium] Missing PMU counter enable configuration for Exynos5515 watchdog.
> >>>>>>> --
> >>>>>>>
> >>>>>>> --- Patch [2]: [PATCH v3 2/2] watchdog: s3c2410_wdt: Add exynos5515-wdt compatible data ---
> >>>>>>> commit 9ce360e3c933b7447ba5da9ba1b7ae3fee989703
> >>>>>>> Author: Aiden Isik <[email protected]>
> >>>>>>>
> >>>>>>> watchdog: s3c2410_wdt: Add exynos5515-wdt compatible data
> >>>>>>>
> >>>>>>> This commit adds 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..5592f25fe6c83 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,
> >>>>>>>> +	.mask_bit = 2,
> >>>>>>>> +	.mask_reset_inv = true,
> >>>>>>>> +	.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_DBGACK_BIT,
> >>>>>>>> +};
> >>>>>>>
> >>>>>>> [Severity: Medium]
> >>>>>>> Does the Exynos5515 PMU require explicit counter enablement like other recent
> >>>>>>> Exynos SoCs?
> >>>>>>>
> >>>>>>> Other variants in this driver (like Exynos850, AutoV920, and GS101) that use
> >>>>>>> the NONCPU_INT_EN PMU registers also pair it with a NONCPU_OUT register and
> >>>>>>> the QUIRK_HAS_PMU_CNT_EN flag to ensure the watchdog counter ticks.
> >>>>>>>
> >>>>>>> If the Exynos5515 hardware works similarly, omitting cnt_en_reg, cnt_en_bit,
> >>>>>>> and QUIRK_HAS_PMU_CNT_EN here might leave the counter disabled and the
> >>>>>>> watchdog silently non-functional. Could you confirm if this variant needs the
> >>>>>>> counter enable logic?
> >>>>>>
> >>>>>> Exynos5515 does not require this.
> >>>>>>
> >>>>>> Downstream on GS101, as an example, has this struct member set:
> >>>>>>
> >>>>>>> pmu_count_en_func
> >>>>>>
> >>>>>> This is not set downstream for Exynos5515, so it is not used for
> >>>>>> Exynos5515.
> >>>>>>
> >>>>>
> >>>>> You keep talking about "downstream". What is that ?
> >>>>
> >>>> By "downstream" I am referring specifically to Samsung's Linux kernel
> >>>> fork for Exynos5515.
> >>>>
> >>>> I host a copy here: https://github.com/Aiden-Isik/linux-android-lucky7
> >>>>
> >>>> Also in this message I refer to Google's GS101 kernel fork, which can
> >>>> be found here:
> >>>> https://github.com/LineageOS/android_kernel_google_gs101
> >>>>
> >>>
> >>> Both are completely irrelevant for the upstream kernel. I am most definitely
> >>> not going to look into that code. If there is anything relevant - such as
> >>> the comment you pointed to earlier - you'll need to provide that as independent
> >>> comment(s), not just refer to some downstream code as explanation. The
> >>> explanation needs to be provided in the _upstream_ kernel code.
> >>>
> >>
> >> There's not really any explanation for it other than "that is the way it is".
> >> Samsung designed the Exynos5515 watchdog timer to not have that quirk,
> >> I read the downstream code and learnt that fact, so we don't enable
> >> that quirk.
> >>
> >
> > Just in case I am misunderstanding what you are asking for and you want
> > examples of how I know this is correct to be posted on the mailing list,
> > here you go.
> >
> > Unfortunately Samsung doesn't publish technical specifications for their
> > modern SoCs, so the best documentation we have is their own source code,
> > which doesn't really explain the *why*, but it does explain the *what*
> > and the *how*.
> >
> > I'll do my best to explain it concisely and how we drive the hardware
> > in the same way upstream:
> >
> > Google (and/or Samsung since GS101 is Exynos-based) define the struct
> > for the watchdog timer like this:
> >
> >> static const struct s3c2410_wdt_variant drv_data_gs101_cl0 = {
> >> 	.noncpu_int_en = EXYNOS_CLUSTER0_NONCPU_INT_EN,
> >> 	.noncpu_out = EXYNOS_CLUSTER0_NONCPU_OUT,
> >> 	.mask_bit = 2,
> >> 	.cnt_en_bit = 8,
> >> 	.rst_stat_reg = EXYNOS_RST_STAT_REG_OFFSET,
> >> 	.rst_stat_bit = 0,      /* CLUSTER0 WDTRESET */
> >> 	.pmu_reset_func = s3c2410wdt_noncpu_int_en,
> >> 	.pmu_count_en_func = s3c2410wdt_noncpu_out,
> >> 	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT | QUIRK_HAS_WTCLRINT_REG |
> >> 		  QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_WTMINCNT_REG,
> >> };
> >
> > Note 'pmu_count_en_func' is set.
> >
> > Later on in the file, we have this code:
> >
> >> if (wdt->drv_data->pmu_count_en_func) {
> >> 	ret = wdt->drv_data->pmu_count_en_func(wdt, true);
> >> 	if (ret < 0) {
> >> 		dev_info(wdt->dev, "wdt count enable fail\n");
> >> 		return ret;
> >> 	}
> >> }
> >
> > Which will call the chosen counter enable function if that field is set,
> > which will, well, enable the counter.
> >
> > Now, for the struct corresponding to Exynos5515 in the same driver:
> >
> >> static const struct s3c2410_wdt_variant drv_data_exynos9_v4 = {
> >> 	.noncpu_int_en = EXYNOS_CLUSTER2_NONCPU_INT_EN,
> >> 	.mask_bit = 2,
> >> 	.rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
> >> 	.rst_stat_bit = 23,	/* CLUSTER2 WDTRESET */
> >> 	.pmu_reset_func = s3c2410wdt_noncpu_int_en,
> >> 	.quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT | QUIRK_HAS_WTCLRINT_REG |
> >> 		  QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_WTMINCNT_REG,
> > };
> >
> > We can see that 'pmu_count_en_func' is not set, meaning that code within
> > the if statement shown earlier is not called, and the counter is not manually
> > enabled.
> >
> > Translating this to upstream, if QUIRK_HAS_PMU_CNT_EN is set as a flag,
> > the following code within the if statement will execute:
> >
> >> if (wdt->drv_data->quirks & QUIRK_HAS_PMU_CNT_EN) {
> >> 	ret = s3c2410wdt_enable_counter(wdt, en);
> >> 		if (ret < 0)
> >> 			return ret;
> >> 	}
> >
> > Which as we can see does the same thing and enables the counter.
> >
> > So, since 'pmu_count_en_func' is enabled on GS101 in the downstream vendor
> > code, the QUIRK_HAS_PMU_CNT_EN flag is set, as the Sashiko AI points out.
> > However, since it is *not* set for Exynos5515, that means Exynos5515 does
> > not require manual counter enablement and that flag should not be set,
> > which is why the AI's concern is wrong.
> >
> > I also tested this (with the watchdog actually enabled this time), and it
> > behaves as expected.
> >
> 
> All ok, but you do not explain all this as comment in your patches, you just
> point to downstream code. At least some of that downstream code, if I recall
> your previous feedback correctly, actually _did_ explain at least some of
> this.

For this specific quirk there are no comments whatsoever. That comment I
sent is one of the only comments in that file.

> 
> Example: A comment along the line of "According to <place source here>,
> Exynos5515 PMU does not require explicit counter enablement" would do.
> Sashiko does not know the context. I don't either, and neither does
> anyone else.
> 
> So why exactly are you not willing to add such comments ? Please explain.

Initially I did not add a comment before I sent the modifications to
the mailing list because nothing else seems to have comments justifying
the flags set or not set.

After you requested an explanation in the code, I thought you wanted
a full explanation like what I sent above, which would be very unwieldy.
Which is why I then also sent you that explanation here, because I wasn't
sure what exactly you were asking for.

But if it's just a comment like the example you described, I can do that.
Do you want the source to be "Samsung's downstream code", or a link to
it?

> 
> Guenter
> 
> 

Kind regards,
- Aiden Isik