[PATCH 1/2] watchdog: imx7ulp_wdt: Clarify timing units

[Stefan Wahren]

Am 27.10.24 um 15:28 schrieb Guenter Roeck:
> On 10/27/24 03:53, Stefan Wahren wrote:
>> imx7ulp_wdt mixes a lot of timing units (frequency, clocks, seconds)
>> in a not obvious way. So improve readability of imx7ulp_wdt by
>> clarifying the relevant units.
>>
>> Signed-off-by: Stefan Wahren <wahrenst at gmx.net>
>> ---
>>   drivers/watchdog/imx7ulp_wdt.c | 18 ++++++++++--------
>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/watchdog/imx7ulp_wdt.c
>> b/drivers/watchdog/imx7ulp_wdt.c
>> index 0f13a3053357..0f92d2217088 100644
>> --- a/drivers/watchdog/imx7ulp_wdt.c
>> +++ b/drivers/watchdog/imx7ulp_wdt.c
>> @@ -19,7 +19,7 @@
>>   #define WDOG_CS_PRES        BIT(12)
>>   #define WDOG_CS_ULK        BIT(11)
>>   #define WDOG_CS_RCS        BIT(10)
>> -#define LPO_CLK            0x1
>> +#define LPO_CLK            0x1    /* 32 kHz */
>
> This configures the clock source to be the LPO clock, which according
> to the
> chip datasheets is a 1kHz clock for all chips except IMX93. It is only
> 32kHz
> for IMX93, and the prescaler is enabled for that chip.
>
> The comment is not only misleading because it selects the clock source,
> not the rate, but wrong, because it only selects a 32kHz clock for IMX93,
> and that value is then prescaled.
Okay, I will drop it
>
>>   #define LPO_CLK_SHIFT        8
>>   #define WDOG_CS_CLK        (LPO_CLK << LPO_CLK_SHIFT)
>>   #define WDOG_CS_EN        BIT(7)
>> @@ -39,8 +39,8 @@
>>   #define UNLOCK_SEQ1    0xD928
>>   #define UNLOCK        ((UNLOCK_SEQ1 << 16) | UNLOCK_SEQ0)
>>
>> -#define DEFAULT_TIMEOUT    60
>> -#define MAX_TIMEOUT    128
>> +#define DEFAULT_TIMEOUT    60    /* seconds */
>> +#define MAX_TIMEOUT    128    /* seconds */
>>   #define WDOG_CLOCK_RATE    1000
>>   #define WDOG_ULK_WAIT_TIMEOUT    1000
>>   #define WDOG_RCS_WAIT_TIMEOUT    10000
>> @@ -240,7 +240,8 @@ static const struct watchdog_info
>> imx7ulp_wdt_info = {
>>               WDIOF_MAGICCLOSE,
>>   };
>>
>> -static int _imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt,
>> unsigned int timeout, unsigned int cs)
>> +static int _imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt,
>> +                 unsigned int timeout_clks, unsigned int cs)
>
> I don't think "_clks" adds any clarity because the value doesn't set a
> "clock".
> "_ticks", maybe.
I'm fine with _ticks

Thanks
>
>>   {
>>       u32 val;
>>       int ret;
>> @@ -263,7 +264,7 @@ static int _imx7ulp_wdt_init(struct
>> imx7ulp_wdt_device *wdt, unsigned int timeou
>>           goto init_out;
>>
>>       /* set an initial timeout value in TOVAL */
>> -    writel(timeout, wdt->base + WDOG_TOVAL);
>> +    writel(timeout_clks, wdt->base + WDOG_TOVAL);
>>       writel(cs, wdt->base + WDOG_CS);
>>       local_irq_enable();
>>       ret = imx7ulp_wdt_wait_rcs(wdt);
>> @@ -275,7 +276,8 @@ static int _imx7ulp_wdt_init(struct
>> imx7ulp_wdt_device *wdt, unsigned int timeou
>>       return ret;
>>   }
>>
>> -static int imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt, unsigned
>> int timeout)
>> +static int imx7ulp_wdt_init(struct imx7ulp_wdt_device *wdt,
>> +                unsigned int timeout_clks)
>>   {
>>       /* enable 32bit command sequence and reconfigure */
>>       u32 val = WDOG_CS_CMD32EN | WDOG_CS_CLK | WDOG_CS_UPDATE |
>> @@ -296,11 +298,11 @@ static int imx7ulp_wdt_init(struct
>> imx7ulp_wdt_device *wdt, unsigned int timeout
>>       }
>>
>>       do {
>> -        ret = _imx7ulp_wdt_init(wdt, timeout, val);
>> +        ret = _imx7ulp_wdt_init(wdt, timeout_clks, val);
>>           toval = readl(wdt->base + WDOG_TOVAL);
>>           cs = readl(wdt->base + WDOG_CS);
>>           cs &= ~(WDOG_CS_FLG | WDOG_CS_ULK | WDOG_CS_RCS);
>> -    } while (--loop > 0 && (cs != val || toval != timeout || ret));
>> +    } while (--loop > 0 && (cs != val || toval != timeout_clks ||
>> ret));
>>
>>       if (loop == 0)
>>           return -EBUSY;
>> --
>> 2.34.1
>>
>
>