On Fri, Mar 06, 2020 at 04:27:44PM +0300, Sergey.Semin@xxxxxxxxxxxxxxxxxxxx wrote:
> From: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
>
> In case if the DW Watchdog IP core is synthesised with
> WDT_USE_FIX_TOP == false, the TOP interval indexes make the device
> to load a custom periods to the counter. These periods are hardwired
> at the synthesis stage and can be within [2^8, 2^(WDT_CNT_WIDTH - 1)].
> Alas their values can't be detected at runtime and must be somehow
> supplied to the driver so one could properly determine the watchdog
> timeout intervals. For this purpose we suggest to have a vendor-
> specific dts property "snps,watchdog-tops" utilized, which would
> provide an array of sixteen counter values. At device probe stage they
> will be used to initialize the watchdog device timeouts determined
> from the array values and current clocks source rate.
>
> In order to have custom TOP values supported the driver must be
> altered in the following way. First of all the fixed-top values
> ready-to-use array must be determined for compatibility with currently
> supported devices, which were synthesised with WDT_USE_FIX_TOP == true.
> Secondly we must redefine the timer period search functions. For
> generality they are redesigned in a way to support the TOP array with
> no limitations on the items order or value. Finally an array with
> pre-defined timeouts must be calculated at probe stage from either
> the custom or fixed TOP values depending on the DW watchdog component
> parameter WDT_USE_FIX_TOP value.
>
> Signed-off-by: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@xxxxxxxxxxxxxxxxxxxx>
> Cc: Thomas Bogendoerfer <tsbogend@xxxxxxxxxxxxxxxx>
> Cc: Paul Burton <paulburton@xxxxxxxxxx>
> Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> ---
> drivers/watchdog/dw_wdt.c | 145 +++++++++++++++++++++++++++++++-------
> 1 file changed, 119 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c
> index fba21de2bbad..4a57b7d777dc 100644
> --- a/drivers/watchdog/dw_wdt.c
> +++ b/drivers/watchdog/dw_wdt.c
> @@ -13,6 +13,7 @@
> */
>
> #include <linux/bitops.h>
> +#include <linux/limits.h>
> #include <linux/clk.h>
> #include <linux/delay.h>
> #include <linux/err.h>
> @@ -34,12 +35,24 @@
> #define WDOG_CURRENT_COUNT_REG_OFFSET 0x08
> #define WDOG_COUNTER_RESTART_REG_OFFSET 0x0c
> #define WDOG_COUNTER_RESTART_KICK_VALUE 0x76
> +#define WDOG_COMP_PARAMS_1_REG_OFFSET 0xf4
> +#define WDOG_COMP_PARAMS_1_USE_FIX_TOP BIT(6)
>
> -/* The maximum TOP (timeout period) value that can be set in the watchdog. */
> -#define DW_WDT_MAX_TOP 15
> +/* There are sixteen TOPs (timeout periods) that can be set in the watchdog. */
> +#define DW_WDT_NUM_TOPS 16
> +#define DW_WDT_FIX_TOP(_idx) (1U << (16 + _idx))
>
> #define DW_WDT_DEFAULT_SECONDS 30
>
> +static const u32 dw_wdt_fix_tops[DW_WDT_NUM_TOPS] = {
> + DW_WDT_FIX_TOP(0), DW_WDT_FIX_TOP(1), DW_WDT_FIX_TOP(2),
> + DW_WDT_FIX_TOP(3), DW_WDT_FIX_TOP(4), DW_WDT_FIX_TOP(5),
> + DW_WDT_FIX_TOP(6), DW_WDT_FIX_TOP(7), DW_WDT_FIX_TOP(8),
> + DW_WDT_FIX_TOP(9), DW_WDT_FIX_TOP(10), DW_WDT_FIX_TOP(11),
> + DW_WDT_FIX_TOP(12), DW_WDT_FIX_TOP(13), DW_WDT_FIX_TOP(14),
> + DW_WDT_FIX_TOP(15)
> +};
> +
> static bool nowayout = WATCHDOG_NOWAYOUT;
> module_param(nowayout, bool, 0);
> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
> @@ -49,6 +62,8 @@ struct dw_wdt {
> void __iomem *regs;
> struct clk *clk;
> unsigned long rate;
> + unsigned int max_top;
> + unsigned int timeouts[DW_WDT_NUM_TOPS];
> struct watchdog_device wdd;
> struct reset_control *rst;
> /* Save/restore */
> @@ -64,20 +79,68 @@ static inline int dw_wdt_is_enabled(struct dw_wdt *dw_wdt)
> WDOG_CONTROL_REG_WDT_EN_MASK;
> }
>
> -static inline int dw_wdt_top_in_seconds(struct dw_wdt *dw_wdt, unsigned top)
> +static unsigned int dw_wdt_find_best_top(struct dw_wdt *dw_wdt,
> + unsigned int timeout, u32 *top)
> {
> + u32 diff = UINT_MAX, tmp;
> + int idx;
> +
> /*
> - * There are 16 possible timeout values in 0..15 where the number of
> - * cycles is 2 ^ (16 + i) and the watchdog counts down.
> + * In general case of non-fixed timeout values they can be arranged in
> + * any order so we have to traverse all the array values. We also try
> + * to find a closest timeout number and make sure its value is greater
> + * than the requested timeout. Note we'll return a maximum timeout
> + * if reachable value couldn't be found.
> */
> - return (1U << (16 + top)) / dw_wdt->rate;
> + for (*top = dw_wdt->max_top, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
> + if (dw_wdt->timeouts[idx] < timeout)
> + continue;
> +
> + tmp = dw_wdt->timeouts[idx] - timeout;
> + if (tmp < diff) {
> + diff = tmp;
> + *top = idx;
> + }
> + }
> +
> + return dw_wdt->timeouts[*top];
> +}
> +
> +static unsigned int dw_wdt_find_min_timeout(struct dw_wdt *dw_wdt)
I would appreciate if the names of functions returning ms end with _ms
to avoid confusion.
> +{
> + u32 min_timeout = UINT_MAX, top;
> + int idx;
> +
> + for (top = 0, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
> + if (dw_wdt->timeouts[idx] <= min_timeout) {
> + min_timeout = dw_wdt->timeouts[idx];
> + top = idx;
> + }
> + }
> +
> + return dw_wdt->timeouts[top];
> +}
> +
> +static unsigned int dw_wdt_find_max_top(struct dw_wdt *dw_wdt, u32 *top)
> +{
> + u32 max_timeout = 0;
> + int idx;
> +
> + for (*top = 0, idx = 0; idx < DW_WDT_NUM_TOPS; ++idx) {
> + if (dw_wdt->timeouts[idx] >= max_timeout) {
> + max_timeout = dw_wdt->timeouts[idx];
> + *top = idx;
> + }
> + }
> +
> + return dw_wdt->timeouts[*top];
> }
>
> -static int dw_wdt_get_top(struct dw_wdt *dw_wdt)
> +static unsigned int dw_wdt_get_timeout(struct dw_wdt *dw_wdt)
> {
> int top = readl(dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET) & 0xF;
>
> - return dw_wdt_top_in_seconds(dw_wdt, top);
> + return dw_wdt->timeouts[top];
> }
>
> static int dw_wdt_ping(struct watchdog_device *wdd)
> @@ -90,20 +153,13 @@ static int dw_wdt_ping(struct watchdog_device *wdd)
> return 0;
> }
>
> -static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
> +static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int req)
> {
> struct dw_wdt *dw_wdt = to_dw_wdt(wdd);
> - int i, top_val = DW_WDT_MAX_TOP;
> + unsigned int timeout;
> + u32 top;
>
> - /*
> - * Iterate over the timeout values until we find the closest match. We
> - * always look for >=.
> - */
> - for (i = 0; i <= DW_WDT_MAX_TOP; ++i)
> - if (dw_wdt_top_in_seconds(dw_wdt, i) >= top_s) {
> - top_val = i;
> - break;
> - }
> + timeout = dw_wdt_find_best_top(dw_wdt, req * MSEC_PER_SEC, &top);
>
> /*
> * Set the new value in the watchdog. Some versions of dw_wdt
> @@ -111,7 +167,7 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
> * CP_WDT_DUAL_TOP in WDT_COMP_PARAMS_1). On those we
> * effectively get a pat of the watchdog right here.
> */
> - writel(top_val | top_val << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
> + writel(top | top << WDOG_TIMEOUT_RANGE_TOPINIT_SHIFT,
> dw_wdt->regs + WDOG_TIMEOUT_RANGE_REG_OFFSET);
>
> /*
> @@ -119,10 +175,10 @@ static int dw_wdt_set_timeout(struct watchdog_device *wdd, unsigned int top_s)
> * kernel(watchdog_dev.c) helps to feed watchdog before
> * wdd->max_hw_heartbeat_ms
> */
> - if (top_s * 1000 <= wdd->max_hw_heartbeat_ms)
> - wdd->timeout = dw_wdt_top_in_seconds(dw_wdt, top_val);
> + if (req * MSEC_PER_SEC > wdd->max_hw_heartbeat_ms)
> + wdd->timeout = req;
> else
> - wdd->timeout = top_s;
> + wdd->timeout = timeout / MSEC_PER_SEC;
>
> return 0;
> }
> @@ -238,6 +294,41 @@ static int dw_wdt_resume(struct device *dev)
>
> static SIMPLE_DEV_PM_OPS(dw_wdt_pm_ops, dw_wdt_suspend, dw_wdt_resume);
>
> +static void dw_wdt_init_timeouts(struct dw_wdt *dw_wdt, struct device *dev)
> +{
> + u32 data, of_tops[DW_WDT_NUM_TOPS];
> + const u32 *tops;
> + int ret, idx;
> +
> + /*
> + * Retrieve custom or fixed counter values depending on the
> + * WDT_USE_FIX_TOP flag found in the component specific parameters
> + * #1 register.
> + */
> + data = readl(dw_wdt->regs + WDOG_COMP_PARAMS_1_REG_OFFSET);
> + if (data & WDOG_COMP_PARAMS_1_USE_FIX_TOP) {
> + tops = dw_wdt_fix_tops;
> + } else {
> + ret = of_property_read_variable_u32_array(dev_of_node(dev),
> + "snps,watchdog-tops", of_tops, DW_WDT_NUM_TOPS,
> + DW_WDT_NUM_TOPS);
> + if (ret < 0) {
> + dev_warn(dev, "No valid TOPs array specified\n");
> + tops = dw_wdt_fix_tops;
> + } else {
> + tops = of_tops;
> + }
> + }
> +
> + /*
> + * We'll keep the timeout values in ms to approximate requested
> + * timeouts with better accuracy.
> + */
> + for (idx = 0; idx < DW_WDT_NUM_TOPS; ++idx)
> + dw_wdt->timeouts[idx] =
> + mult_frac(tops[idx], MSEC_PER_SEC, dw_wdt->rate);
tops[idx] type is u32. Its value can be up to 0xffffffff. That means
dw_wdt->rate must be >= 1000 to avoid overflow, which you should check.
Note that I don't see the point of keeping the timeout values in ms.
The code selects a larger value (in seconds) anyway. All this does is
to add a lot of multiply and divide operations, plus a source of bugs
and confusion, for little if any gain.
> +}
> +
> static int dw_wdt_drv_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -275,12 +366,14 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
>
> reset_control_deassert(dw_wdt->rst);
>
> + dw_wdt_init_timeouts(dw_wdt, dev);
> +
> wdd = &dw_wdt->wdd;
> wdd->info = &dw_wdt_ident;
> wdd->ops = &dw_wdt_ops;
> - wdd->min_timeout = 1;
> + wdd->min_timeout = dw_wdt_find_min_timeout(dw_wdt) / MSEC_PER_SEC;
dw_wdt_find_min_timeout can return a value < 1000. In that case min_timeout
would be 0, ie unspecified.
> wdd->max_hw_heartbeat_ms =
> - dw_wdt_top_in_seconds(dw_wdt, DW_WDT_MAX_TOP) * 1000;
> + dw_wdt_find_max_top(dw_wdt, &dw_wdt->max_top);
> wdd->parent = dev;
>
> watchdog_set_drvdata(wdd, dw_wdt);
> @@ -293,7 +386,7 @@ static int dw_wdt_drv_probe(struct platform_device *pdev)
> * devicetree.
> */
> if (dw_wdt_is_enabled(dw_wdt)) {
> - wdd->timeout = dw_wdt_get_top(dw_wdt);
> + wdd->timeout = dw_wdt_get_timeout(dw_wdt) / MSEC_PER_SEC;
> set_bit(WDOG_HW_RUNNING, &wdd->status);
> } else {
> wdd->timeout = DW_WDT_DEFAULT_SECONDS;
