[PATCH 1/2] watchdog: imx7ulp_wdt: Clarify timing units
Stefan Wahren
wahrenst at gmx.net
Sun Oct 27 08:56:23 PDT 2024
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
>>
>
>
More information about the linux-arm-kernel
mailing list