| Message ID | 20241027105323.93699-2-wahrenst@gmx.net |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog: imx7ulp_wdt: Improve readability | expand |
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@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. > #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. > { > 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 >
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@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 >> > >
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 */ #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) { 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;
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@gmx.net> --- drivers/watchdog/imx7ulp_wdt.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) -- 2.34.1