diff mbox series

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

Message ID 20231120212037.911774-16-peter.griffin@linaro.org
State New
Headers show
Series Add minimal Tensor/GS101 SoC support and Oriole/Pixel6 board | expand

Commit Message

Peter Griffin Nov. 20, 2023, 9:20 p.m. UTC
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@linaro.org>
---
 drivers/watchdog/s3c2410_wdt.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

Comments

Krzysztof Kozlowski Nov. 22, 2023, 7:53 a.m. UTC | #1
On 21/11/2023 19:10, Guenter Roeck wrote:

>>>   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,
>>>   };
>>>
>>
>> This patch states it's adding the feature, but in fact it's also
>> enabling this feature for gs101. Suggest moving this patch before the
>> one enabling gs101 wdt. This way, one patch will only add the feature,
>> and another patch will enable gs101 entirely (with this feature used).
>> At least it seems like more atomic approach to me.
>>
> 
> Both approaches have their merits and their downsides. I for my part am not
> even sure if DBGACK_MASK should be enabled unconditionally if supported.
> With your approach, it would be impossible (or at least more difficult) to
> revert if it turns out to be broken and/or have unexpected side effects.
> That seems less desirable to me than the current approach.

Reversing the patches does not change this. It is enabled
unconditionally in current order as well.

Sam's idea is correct here - first you add support for new quirk, then
you add new SoC which will use this quirk. Doing the other way - first
SoC and then new quirk - looks like SoC was added incomplete.

Best regards,
Krzysztof
diff mbox series

Patch

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.
  */
 #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)
+{
+	unsigned long wtcon;
+
+	if (!(wdt->drv_data->quirks & QUIRK_HAS_DBGACK_BIT))
+		return;
+
+	wtcon = readl(wdt->reg_base + S3C2410_WTCON);
+	if (mask)
+		wtcon |= S3C2410_WTCON_DBGACK_MASK;
+	else
+		wtcon &= ~S3C2410_WTCON_DBGACK_MASK;
+	writel(wtcon, wdt->reg_base + S3C2410_WTCON);
+}
+
 static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
 {
 	struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
@@ -737,6 +757,8 @@  static int s3c2410wdt_probe(struct platform_device *pdev)
 	wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
 	wdt->wdt_device.parent = dev;
 
+	s3c2410wdt_mask_dbgack(wdt, true);
+
 	/*
 	 * If "tmr_atboot" param is non-zero, start the watchdog right now. Also
 	 * set WDOG_HW_RUNNING bit, so that watchdog core can kick the watchdog.