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

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.

Guenter

>>
>> Guenter
>>
>>
> 
> Kind regards,
> - Aiden Isik