On 11/20/23 15:20, Peter Griffin wrote:
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.
At least please say something like "masked (disabled)" if you want to use
the term.
Thanks,
Guenter
