[PATCH v4 15/19] watchdog: s3c2410_wdt: Add support for WTCON register DBGACK_MASK bit

[Guenter Roeck]

On 11/20/23 15:20, Peter Griffin wrote:
> Hi Guenter,
> 
> On Mon, 20 Nov 2023 at 23:03, Guenter Roeck <linux at roeck-us.net> wrote:
>>
>> On 11/20/23 14:45, Peter Griffin wrote:
>>> Hi Guenter,
>>>
>>> Thanks for the review.
>>>
>>> On Mon, 20 Nov 2023 at 22:00, Guenter Roeck <linux at roeck-us.net> wrote:
>>>>
>>>> On 11/20/23 13:20, Peter Griffin wrote:
>>>>> The WDT uses the CPU core signal DBGACK to determine whether the SoC
>>>>> is running in debug mode or not. If the DBGACK signal is asserted and
>>>>> DBGACK_MASK is enabled, then WDT output and interrupt is masked.
>>>>>
>>>>> Presence of the DBGACK_MASK bit is determined by adding a new
>>>>> QUIRK_HAS_DBGACK_BIT quirk. Currently only gs101 SoC is known to have
>>>>> the DBGACK_MASK bit so add the quirk to drv_data_gs101_cl1 and
>>>>> drv_data_gs101_cl1 quirks.
>>>>>
>>>>> Signed-off-by: Peter Griffin <peter.griffin at linaro.org>
>>>>> ---
>>>>>     drivers/watchdog/s3c2410_wdt.c | 32 +++++++++++++++++++++++++++-----
>>>>>     1 file changed, 27 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
>>>>> index 08b8c57dd812..ed561deeeed9 100644
>>>>> --- a/drivers/watchdog/s3c2410_wdt.c
>>>>> +++ b/drivers/watchdog/s3c2410_wdt.c
>>>>> @@ -34,9 +34,10 @@
>>>>>
>>>>>     #define S3C2410_WTCNT_MAXCNT        0xffff
>>>>>
>>>>> -#define S3C2410_WTCON_RSTEN  (1 << 0)
>>>>> -#define S3C2410_WTCON_INTEN  (1 << 2)
>>>>> -#define S3C2410_WTCON_ENABLE (1 << 5)
>>>>> +#define S3C2410_WTCON_RSTEN          (1 << 0)
>>>>> +#define S3C2410_WTCON_INTEN          (1 << 2)
>>>>> +#define S3C2410_WTCON_ENABLE         (1 << 5)
>>>>> +#define S3C2410_WTCON_DBGACK_MASK    (1 << 16)
>>>>>
>>>>>     #define S3C2410_WTCON_DIV16 (0 << 3)
>>>>>     #define S3C2410_WTCON_DIV32 (1 << 3)
>>>>> @@ -107,12 +108,16 @@
>>>>>      * %QUIRK_HAS_PMU_CNT_EN: PMU block has some register (e.g. CLUSTERx_NONCPU_OUT)
>>>>>      * with "watchdog counter enable" bit. That bit should be set to make watchdog
>>>>>      * counter running.
>>>>> + *
>>>>> + * %QUIRK_HAS_DBGACK_BIT: WTCON register has DBGACK_MASK bit. Enables masking
>>>>> + * WDT interrupt and reset request according to CPU core DBGACK signal.
>>>>
>>>> This is a bit difficult to understand. I _think_ it means that the DBGACK_MASK bit
>>>> has to be set to be able to trigger interrupt and reset requests.
>>>
>>> Not quite, it is a bit that controls masking the watchdog outputs when the SoC
>>> is in debug mode.
>>>
>>>> "masking" normally refers to disabling something (at least in interrupt context).
>>>> "Enables masking WDT interrupt" sounds like the bit has to be set in order to
>>>> be able to disable interupts, and the code below suggests that the bit has to be
>>>> set for the driver to work. Is that the case ? It might make sense to explain this
>>>> a bit further.
>>>
>>> Maybe I explained it more clearly in the commit message than the comment
>>>
>>> "The WDT uses the CPU core signal DBGACK to determine whether the SoC
>>> is running in debug mode or not. If the DBGACK signal is asserted and
>>> DBGACK_MASK is enabled, then WDT output and interrupt is masked."
>>>
>>> Is that any clearer? Or maybe simpler again
>>>
>>> "Enabling DBGACK_MASK bit masks the watchdog outputs when the SoC is
>>> in debug mode. Debug mode is determined by the DBGACK CPU signal."
>>>
>>> Let me know what you think is the clearest and most succinct and I can
>>> update the comment.
>>>
>>
>> You are still using the term "masked" which I think just hides what
>> the code is really doing. Why not just say "disable" ?
> 
> The reason for using the "masked" terminology was that is what the
> Watchdog IP TRM uses throughout to describe the feature. But I agree
> just saying disable is clearer.
> 

At least please say something like "masked (disabled)" if you want to use
the term.

Thanks,
Guenter