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

[Guenter Roeck <linux@roeck-us.net>]

On 11/21/23 09:52, Sam Protsenko wrote:
> On Mon, Nov 20, 2023 at 3:21 PM Peter Griffin <peter.griffin@linaro.org> 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@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.
>>    */
>>   #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,
>>   };
>>
> 
> 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.

Guenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel