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

[Guenter Roeck <linux@roeck-us.net>]

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 <aidenisik@member.fsf.org>
>>>>>>>
>>>>>>> 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.

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.

Guenter