Hi Juuso,
On 2026-07-10T04:51:17, Juuso Rinta <[email protected]> wrote:
> watchdog: octeontx_wdt: add get_timeout operation
>
> Implement the get_timeout operation for the OcteonTX watchdog driver.
>
> Add a timeout_ms field to the private driver data. octeontx_wdt_start()
> stores the timeout after applying overflow clamping.
>
> octeontx_wdt_get_timeout() stores the effective timeout
> in the timeout_ms output parameter.
>
> Signed-off-by: Juuso Rinta <[email protected]>
>
> drivers/watchdog/octeontx_wdt.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
> diff --git a/drivers/watchdog/octeontx_wdt.c b/drivers/watchdog/octeontx_wdt.c
> @@ -61,6 +62,11 @@ static int octeontx_wdt_start(struct udevice *dev, u64
> timeout_ms, ulong flags)
> + /* Store calculated timeout in milliseconds for get_timeout */
> + calculated_timeout = (tout_wdog << 8) << priv->data->timer_shift;
> +
> + priv->timeout_ms = clk_rate ? calculated_timeout * 1000 / clk_rate : 0;
The comment is misleading: at this point 'calculated_timeout' is in
clock cycles, not milliseconds - the conversion happens on the next
line. Please can you adjust it, or fold the two statements into one,
since 'calculated_timeout' has no other use?
Also, when clk_rate is 0 (possible in the gd->bus_clk path) this
stores 0, which get_timeout() reports as a successful 0ms timeout, so
wdt_start() prints '(0s timeout, requested 60s)' - arguably more
misleading than the print this series sets out to fix. Better to treat
0 as no valid value and return an error from get_timeout().
> diff --git a/drivers/watchdog/octeontx_wdt.c b/drivers/watchdog/octeontx_wdt.c
> @@ -88,6 +94,14 @@ static int octeontx_wdt_expire_now(struct udevice *dev,
> ulong flags)
> +static int octeontx_wdt_get_timeout(struct udevice *dev, u64 *timeout_ms)
> +{
> + struct octeontx_wdt *priv = dev_get_priv(dev);
> + *timeout_ms = priv->timeout_ms;
> +
> + return 0;
> +}
Since patch 2 lets 'wdt gettimeout' run at any time, calling this
before wdt_start() returns 0ms as a success. Returning -EINVAL (or
similar) when nothing has been stored would cover the clk_rate case
above too.
Alternatively, avoid the state field entirely by reading the register
back and reversing the calculation, e.g. FIELD_GET(WDOG_LEN,
readq(...)) - that reflects what the hardware is actually doing and
naturally handles the not-started case (WDOG_LEN is 0). What do you
think?
Regards,
Simon
