| Message ID | 20240116-wdt-v2-1-501c7694c3f0@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] watchdog: qcom: fine tune the max timeout value calculation | expand |
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,
On 1/16/2024 8:32 PM, Guenter Roeck wrote: > 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> Guenter / Will, Can this be picked for v6.9? I don't see this in linux- next yet, so please consider this as a gentle reminder! > >> --- >> 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, >
On 2/3/2024 12:07 PM, Kathiravan Thirumoorthy wrote: > > > On 1/16/2024 8:32 PM, Guenter Roeck wrote: >> 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> > > > Guenter / Will, Can this be picked for v6.9? I don't see this in linux- > next yet, so please consider this as a gentle reminder! Guenter / Will, Gentle Reminder... is this change queued for v6.9? > >> >>> --- >>> 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, >> >
Ji Kathiravan, > > On 2/3/2024 12:07 PM, Kathiravan Thirumoorthy wrote: > > > > > >On 1/16/2024 8:32 PM, Guenter Roeck wrote: > >>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> > > > > > >Guenter / Will, Can this be picked for v6.9? I don't see this in > >linux- next yet, so please consider this as a gentle reminder! > > > Guenter / Will, Gentle Reminder... is this change queued for v6.9? Yes, it is. Kind regards, Wim.
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;
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> --- 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(-) --- base-commit: 9e21984d62c56a0f6d1fc6f76b646212cfd7fe88 change-id: 20240111-wdt-5bd079ecf14d Best regards,