Re: [PATCH v2] watchdog: qcom: fine tune the max timeout value calculation

[Guenter Roeck <linux@roeck-us.net>]

On 1/16/24 00:22, Kathiravan Thirumoorthy wrote:
> To determine the max_timeout value, the below calculation is used.
> 
> 	max_timeout = 0x10000000 / clk_rate
> 
> cat /sys/devices/platform/soc@0/b017000.watchdog/watchdog/watchdog0/max_timeout
> 8388
> 
> However, this is not valid for all the platforms. IPQ SoCs starting from
> IPQ40xx and recent Snapdragron SoCs also has the bark and bite time field
> length of 20bits, which can hold max up to 32 seconds if the clk_rate is
> 32KHz.
> 
> If the user tries to configure the timeout more than 32s, then the value
> will be truncated and the actual value will not be reflected in the HW.
> 
> To avoid this, lets add a variable called max_tick_count in the device data,
> which defines max counter value of the WDT controller. Using this, max-timeout
> will be calculated in runtime for various WDT contorllers.
> 
> With this change, we get the proper max_timeout as below and restricts
> the user from configuring the timeout higher than this.
> 
> cat /sys/devices/platform/soc@0/b017000.watchdog/watchdog/watchdog0/max_timeout
> 32
> 
> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> Changes in v2:
> - drop the minimum timeout change from 30s to 32s
> - Link to v1: https://lore.kernel.org/r/20240111-wdt-v1-1-28c648b3b1f3@quicinc.com
> ---
>   drivers/watchdog/qcom-wdt.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 9e790f0c2096..006f9c61aa64 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -41,6 +41,7 @@ static const u32 reg_offset_data_kpss[] = {
>   struct qcom_wdt_match_data {
>   	const u32 *offset;
>   	bool pretimeout;
> +	u32 max_tick_count;
>   };
>   
>   struct qcom_wdt {
> @@ -177,11 +178,13 @@ static const struct watchdog_info qcom_wdt_pt_info = {
>   static const struct qcom_wdt_match_data match_data_apcs_tmr = {
>   	.offset = reg_offset_data_apcs_tmr,
>   	.pretimeout = false,
> +	.max_tick_count = 0x10000000U,
>   };
>   
>   static const struct qcom_wdt_match_data match_data_kpss = {
>   	.offset = reg_offset_data_kpss,
>   	.pretimeout = true,
> +	.max_tick_count = 0xFFFFFU,
>   };
>   
>   static int qcom_wdt_probe(struct platform_device *pdev)
> @@ -236,7 +239,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>   	 */
>   	wdt->rate = clk_get_rate(clk);
>   	if (wdt->rate == 0 ||
> -	    wdt->rate > 0x10000000U) {
> +	    wdt->rate > data->max_tick_count) {
>   		dev_err(dev, "invalid clock rate\n");
>   		return -EINVAL;
>   	}
> @@ -260,7 +263,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>   
>   	wdt->wdd.ops = &qcom_wdt_ops;
>   	wdt->wdd.min_timeout = 1;
> -	wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
> +	wdt->wdd.max_timeout = data->max_tick_count / wdt->rate;
>   	wdt->wdd.parent = dev;
>   	wdt->layout = data->offset;
>   
> 
> ---
> base-commit: 9e21984d62c56a0f6d1fc6f76b646212cfd7fe88
> change-id: 20240111-wdt-5bd079ecf14d
> 
> Best regards,