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

[Peter Griffin <peter.griffin@xxxxxxxxxx>]

Hi Guenter,

On Mon, 20 Nov 2023 at 23:03, Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxx>
> >>> ---
> >>>    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.

>
> "Setting the DBGACK_MASK bit disables the watchdog outputs when the SoC is
>   in debug mode. Debug mode is determined by the DBGACK CPU signal."
>
> That seems to be much clearer to me, though I think there should still
> be a comment along the line of "disable watchdog output if CPU is in
> debug mode" in the code.

I will update the quirk comment with the less ambiguous wording and
also add an extra comment like you suggest.

>
> That doesn't really explain _why_ the watchdog is disabled in this mode,
> but at least it makes it obvious what is happening.
>
> >>
> >>>     */
> >>>    #define QUIRK_HAS_WTCLRINT_REG                      (1 << 0)
> >>>    #define QUIRK_HAS_PMU_MASK_RESET            (1 << 1)
> >>>    #define QUIRK_HAS_PMU_RST_STAT                      (1 << 2)
> >>>    #define QUIRK_HAS_PMU_AUTO_DISABLE          (1 << 3)
> >>>    #define QUIRK_HAS_PMU_CNT_EN                        (1 << 4)
> >>> +#define QUIRK_HAS_DBGACK_BIT                 (1 << 5)
> >>>
> >>>    /* These quirks require that we have a PMU register map */
> >>>    #define QUIRKS_HAVE_PMUREG \
> >>> @@ -279,7 +284,7 @@ static const struct s3c2410_wdt_variant drv_data_gs101_cl0 = {
> >>>        .cnt_en_reg = GS_CLUSTER0_NONCPU_OUT,
> >>>        .cnt_en_bit = 8,
> >>>        .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN |
> >>> -               QUIRK_HAS_WTCLRINT_REG,
> >>> +               QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT,
> >>>    };
> >>>
> >>>    static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = {
> >>> @@ -291,7 +296,7 @@ static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = {
> >>>        .cnt_en_reg = GS_CLUSTER1_NONCPU_OUT,
> >>>        .cnt_en_bit = 7,
> >>>        .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN |
> >>> -               QUIRK_HAS_WTCLRINT_REG,
> >>> +               QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT,
> >>>    };
> >>>
> >>>    static const struct of_device_id s3c2410_wdt_match[] = {
> >>> @@ -408,6 +413,21 @@ static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en)
> >>>        return 0;
> >>>    }
> >>>
> >>> +static void s3c2410wdt_mask_dbgack(struct s3c2410_wdt *wdt, bool mask)
> >>
> >> I think I must be missing something. This is only ever called with mask==true,
> >> meaning the bit, if present, is always set.
> >>
> >> Why not call the function s3c2410wdt_set_dbgack() and drop the unnecessary
> >> parameter ?
> >
> > I can update like you suggest, it would simplify the logic a little bit.
> >
>
> Please do.

Will do

Thanks,

Peter