* [PATCH V2 0/3] watchdog: stm32: add dynamic prescaler support @ 2019-05-02 14:08 Ludovic Barre 2019-05-02 14:08 ` [PATCH V2 1/3] watchdog: stm32: update to devm_watchdog_register_device Ludovic Barre ` (3 more replies) 0 siblings, 4 replies; 9+ messages in thread From: Ludovic Barre @ 2019-05-02 14:08 UTC (permalink / raw) To: Wim Van Sebroeck, Guenter Roeck, Rob Herring Cc: Maxime Coquelin, Alexandre Torgue, linux-watchdog, linux-arm-kernel, linux-kernel, devicetree, linux-stm32, Ludovic Barre From: Ludovic Barre <ludovic.barre@st.com> This patch series updates stm32 watchdog driver on: -use devm_watchdog_register_device -Guenter's recommendation about return value: set_timeout return 0 on succes, -EINVAL for "parameter out of range" and -EIO for "could not write value to the watchdog". Set of reload and prescaler registers are stay in start function, because the stm32 watchdog must be enabled to write these registers. -adds dynamic prescaler support Ludovic Barre (3): watchdog: stm32: update to devm_watchdog_register_device watchdog: stm32: update return values recommended by watchdog kernel api watchdog: stm32: add dynamic prescaler support drivers/watchdog/stm32_iwdg.c | 96 ++++++++++++++++++++++++------------------- 1 file changed, 54 insertions(+), 42 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V2 1/3] watchdog: stm32: update to devm_watchdog_register_device 2019-05-02 14:08 [PATCH V2 0/3] watchdog: stm32: add dynamic prescaler support Ludovic Barre @ 2019-05-02 14:08 ` Ludovic Barre 2019-05-02 20:21 ` Guenter Roeck 2019-05-02 14:08 ` [PATCH V2 2/3] watchdog: stm32: update return values recommended by watchdog kernel api Ludovic Barre ` (2 subsequent siblings) 3 siblings, 1 reply; 9+ messages in thread From: Ludovic Barre @ 2019-05-02 14:08 UTC (permalink / raw) To: Wim Van Sebroeck, Guenter Roeck, Rob Herring Cc: Maxime Coquelin, Alexandre Torgue, linux-watchdog, linux-arm-kernel, linux-kernel, devicetree, linux-stm32, Ludovic Barre From: Ludovic Barre <ludovic.barre@st.com> This patch updates to devm_watchdog_register_device interface Signed-off-by: Ludovic Barre <ludovic.barre@st.com> --- drivers/watchdog/stm32_iwdg.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c index e00e3b3..e191bd8 100644 --- a/drivers/watchdog/stm32_iwdg.c +++ b/drivers/watchdog/stm32_iwdg.c @@ -243,7 +243,7 @@ static int stm32_iwdg_probe(struct platform_device *pdev) dev_warn(&pdev->dev, "unable to set timeout value, using default\n"); - ret = watchdog_register_device(wdd); + ret = devm_watchdog_register_device(&pdev->dev, wdd); if (ret) { dev_err(&pdev->dev, "failed to register watchdog device\n"); goto err; @@ -263,7 +263,6 @@ static int stm32_iwdg_remove(struct platform_device *pdev) { struct stm32_iwdg *wdt = platform_get_drvdata(pdev); - watchdog_unregister_device(&wdt->wdd); clk_disable_unprepare(wdt->clk_lsi); clk_disable_unprepare(wdt->clk_pclk); -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V2 1/3] watchdog: stm32: update to devm_watchdog_register_device 2019-05-02 14:08 ` [PATCH V2 1/3] watchdog: stm32: update to devm_watchdog_register_device Ludovic Barre @ 2019-05-02 20:21 ` Guenter Roeck 2019-05-03 7:58 ` Ludovic BARRE 0 siblings, 1 reply; 9+ messages in thread From: Guenter Roeck @ 2019-05-02 20:21 UTC (permalink / raw) To: Ludovic Barre Cc: Wim Van Sebroeck, Rob Herring, Maxime Coquelin, Alexandre Torgue, linux-watchdog, linux-arm-kernel, linux-kernel, devicetree, linux-stm32 On Thu, May 02, 2019 at 04:08:44PM +0200, Ludovic Barre wrote: > From: Ludovic Barre <ludovic.barre@st.com> > > This patch updates to devm_watchdog_register_device interface > Not that easy. See below. A more complete solution is at https://patchwork.kernel.org/patch/10894355 I have a total of three patches for this driver pending for the next kernel release. Maybe it would make sense to (re-) start this series from there after the next commit window closes. Guenter > Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > --- > drivers/watchdog/stm32_iwdg.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c > index e00e3b3..e191bd8 100644 > --- a/drivers/watchdog/stm32_iwdg.c > +++ b/drivers/watchdog/stm32_iwdg.c > @@ -243,7 +243,7 @@ static int stm32_iwdg_probe(struct platform_device *pdev) > dev_warn(&pdev->dev, > "unable to set timeout value, using default\n"); > > - ret = watchdog_register_device(wdd); > + ret = devm_watchdog_register_device(&pdev->dev, wdd); > if (ret) { > dev_err(&pdev->dev, "failed to register watchdog device\n"); > goto err; > @@ -263,7 +263,6 @@ static int stm32_iwdg_remove(struct platform_device *pdev) > { > struct stm32_iwdg *wdt = platform_get_drvdata(pdev); > > - watchdog_unregister_device(&wdt->wdd); > clk_disable_unprepare(wdt->clk_lsi); > clk_disable_unprepare(wdt->clk_pclk); This disables the clock while the watchdog is still registered and running. That is not a good idea. > > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2 1/3] watchdog: stm32: update to devm_watchdog_register_device 2019-05-02 20:21 ` Guenter Roeck @ 2019-05-03 7:58 ` Ludovic BARRE 0 siblings, 0 replies; 9+ messages in thread From: Ludovic BARRE @ 2019-05-03 7:58 UTC (permalink / raw) To: Guenter Roeck Cc: Wim Van Sebroeck, Rob Herring, Maxime Coquelin, Alexandre Torgue, linux-watchdog, linux-arm-kernel, linux-kernel, devicetree, linux-stm32 hi Guenter On 5/2/19 10:21 PM, Guenter Roeck wrote: > On Thu, May 02, 2019 at 04:08:44PM +0200, Ludovic Barre wrote: >> From: Ludovic Barre <ludovic.barre@st.com> >> >> This patch updates to devm_watchdog_register_device interface >> > Not that easy. See below. > > A more complete solution is at > https://patchwork.kernel.org/patch/10894355 > > I have a total of three patches for this driver pending for > the next kernel release. Maybe it would make sense to (re-) > start this series from there after the next commit window > closes. > I used the repository defined in MAINTAINERS file git://www.linux-watchdog.org/linux-watchdog.git but there is no next branch. Today, I see your kernel.org repository https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git/ And I see your next branch, so I will use it. Regards, Ludo > Guenter > >> Signed-off-by: Ludovic Barre <ludovic.barre@st.com> >> --- >> drivers/watchdog/stm32_iwdg.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c >> index e00e3b3..e191bd8 100644 >> --- a/drivers/watchdog/stm32_iwdg.c >> +++ b/drivers/watchdog/stm32_iwdg.c >> @@ -243,7 +243,7 @@ static int stm32_iwdg_probe(struct platform_device *pdev) >> dev_warn(&pdev->dev, >> "unable to set timeout value, using default\n"); >> >> - ret = watchdog_register_device(wdd); >> + ret = devm_watchdog_register_device(&pdev->dev, wdd); >> if (ret) { >> dev_err(&pdev->dev, "failed to register watchdog device\n"); >> goto err; >> @@ -263,7 +263,6 @@ static int stm32_iwdg_remove(struct platform_device *pdev) >> { >> struct stm32_iwdg *wdt = platform_get_drvdata(pdev); >> >> - watchdog_unregister_device(&wdt->wdd); >> clk_disable_unprepare(wdt->clk_lsi); >> clk_disable_unprepare(wdt->clk_pclk); > > This disables the clock while the watchdog is still registered > and running. That is not a good idea. > >> >> -- >> 2.7.4 >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V2 2/3] watchdog: stm32: update return values recommended by watchdog kernel api 2019-05-02 14:08 [PATCH V2 0/3] watchdog: stm32: add dynamic prescaler support Ludovic Barre 2019-05-02 14:08 ` [PATCH V2 1/3] watchdog: stm32: update to devm_watchdog_register_device Ludovic Barre @ 2019-05-02 14:08 ` Ludovic Barre 2019-05-02 20:23 ` Guenter Roeck 2019-05-02 14:08 ` [PATCH V2 3/3] watchdog: stm32: add dynamic prescaler support Ludovic Barre 2019-05-02 20:37 ` [PATCH V2 0/3] " Guenter Roeck 3 siblings, 1 reply; 9+ messages in thread From: Ludovic Barre @ 2019-05-02 14:08 UTC (permalink / raw) To: Wim Van Sebroeck, Guenter Roeck, Rob Herring Cc: Maxime Coquelin, Alexandre Torgue, linux-watchdog, linux-arm-kernel, linux-kernel, devicetree, linux-stm32, Ludovic Barre From: Ludovic Barre <ludovic.barre@st.com> This patch updates return values on watchdog-kernel-api.txt: return 0 on succes, -EINVAL for "parameter out of range" and -EIO for "could not write value to the watchdog". Signed-off-by: Ludovic Barre <ludovic.barre@st.com> --- drivers/watchdog/stm32_iwdg.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c index e191bd8..f19a6d4 100644 --- a/drivers/watchdog/stm32_iwdg.c +++ b/drivers/watchdog/stm32_iwdg.c @@ -81,7 +81,6 @@ static int stm32_iwdg_start(struct watchdog_device *wdd) struct stm32_iwdg *wdt = watchdog_get_drvdata(wdd); u32 val = FLAG_PVU | FLAG_RVU; u32 reload; - int ret; dev_dbg(wdd->parent, "%s\n", __func__); @@ -98,13 +97,11 @@ static int stm32_iwdg_start(struct watchdog_device *wdd) reg_write(wdt->regs, IWDG_KR, KR_KEY_ENABLE); /* wait for the registers to be updated (max 100ms) */ - ret = readl_relaxed_poll_timeout(wdt->regs + IWDG_SR, val, - !(val & (FLAG_PVU | FLAG_RVU)), - SLEEP_US, TIMEOUT_US); - if (ret) { - dev_err(wdd->parent, - "Fail to set prescaler or reload registers\n"); - return ret; + if (readl_relaxed_poll_timeout(wdt->regs + IWDG_SR, val, + !(val & (FLAG_PVU | FLAG_RVU)), + SLEEP_US, TIMEOUT_US)) { + dev_err(wdd->parent, "Fail to set prescaler, reload regs\n"); + return -EIO; } /* reload watchdog */ @@ -128,8 +125,16 @@ static int stm32_iwdg_ping(struct watchdog_device *wdd) static int stm32_iwdg_set_timeout(struct watchdog_device *wdd, unsigned int timeout) { + unsigned int tout = clamp(timeout, wdd->min_timeout, + wdd->max_hw_heartbeat_ms / 1000); + dev_dbg(wdd->parent, "%s timeout: %d sec\n", __func__, timeout); + if (tout != timeout) { + dev_err(wdd->parent, "parameter out of range\n"); + return -EINVAL; + } + wdd->timeout = timeout; if (watchdog_active(wdd)) -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V2 2/3] watchdog: stm32: update return values recommended by watchdog kernel api 2019-05-02 14:08 ` [PATCH V2 2/3] watchdog: stm32: update return values recommended by watchdog kernel api Ludovic Barre @ 2019-05-02 20:23 ` Guenter Roeck 0 siblings, 0 replies; 9+ messages in thread From: Guenter Roeck @ 2019-05-02 20:23 UTC (permalink / raw) To: Ludovic Barre Cc: Wim Van Sebroeck, Rob Herring, Maxime Coquelin, Alexandre Torgue, linux-watchdog, linux-arm-kernel, linux-kernel, devicetree, linux-stm32 On Thu, May 02, 2019 at 04:08:45PM +0200, Ludovic Barre wrote: > From: Ludovic Barre <ludovic.barre@st.com> > > This patch updates return values on watchdog-kernel-api.txt: > return 0 on succes, -EINVAL for "parameter out of range" > and -EIO for "could not write value to the watchdog". > > Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > --- > drivers/watchdog/stm32_iwdg.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c > index e191bd8..f19a6d4 100644 > --- a/drivers/watchdog/stm32_iwdg.c > +++ b/drivers/watchdog/stm32_iwdg.c > @@ -81,7 +81,6 @@ static int stm32_iwdg_start(struct watchdog_device *wdd) > struct stm32_iwdg *wdt = watchdog_get_drvdata(wdd); > u32 val = FLAG_PVU | FLAG_RVU; > u32 reload; > - int ret; > > dev_dbg(wdd->parent, "%s\n", __func__); > > @@ -98,13 +97,11 @@ static int stm32_iwdg_start(struct watchdog_device *wdd) > reg_write(wdt->regs, IWDG_KR, KR_KEY_ENABLE); > > /* wait for the registers to be updated (max 100ms) */ > - ret = readl_relaxed_poll_timeout(wdt->regs + IWDG_SR, val, > - !(val & (FLAG_PVU | FLAG_RVU)), > - SLEEP_US, TIMEOUT_US); > - if (ret) { > - dev_err(wdd->parent, > - "Fail to set prescaler or reload registers\n"); > - return ret; > + if (readl_relaxed_poll_timeout(wdt->regs + IWDG_SR, val, > + !(val & (FLAG_PVU | FLAG_RVU)), > + SLEEP_US, TIMEOUT_US)) { > + dev_err(wdd->parent, "Fail to set prescaler, reload regs\n"); > + return -EIO; Please don't. Overriding error values tends to result in complaints by static analyzers, and we don't want to have to deal with those. > } > > /* reload watchdog */ > @@ -128,8 +125,16 @@ static int stm32_iwdg_ping(struct watchdog_device *wdd) > static int stm32_iwdg_set_timeout(struct watchdog_device *wdd, > unsigned int timeout) > { > + unsigned int tout = clamp(timeout, wdd->min_timeout, > + wdd->max_hw_heartbeat_ms / 1000); > + > dev_dbg(wdd->parent, "%s timeout: %d sec\n", __func__, timeout); > > + if (tout != timeout) { > + dev_err(wdd->parent, "parameter out of range\n"); > + return -EINVAL; > + } This is simply wrong. The whole point of max_hw_heartbeat_ms is to enable situations where the selected timeout is larger than the timeout supported by the hardware. In that situation, the kernel pings the hardware until the next ping from userspace is due. > + > wdd->timeout = timeout; > > if (watchdog_active(wdd)) > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V2 3/3] watchdog: stm32: add dynamic prescaler support 2019-05-02 14:08 [PATCH V2 0/3] watchdog: stm32: add dynamic prescaler support Ludovic Barre 2019-05-02 14:08 ` [PATCH V2 1/3] watchdog: stm32: update to devm_watchdog_register_device Ludovic Barre 2019-05-02 14:08 ` [PATCH V2 2/3] watchdog: stm32: update return values recommended by watchdog kernel api Ludovic Barre @ 2019-05-02 14:08 ` Ludovic Barre 2019-05-02 20:35 ` Guenter Roeck 2019-05-02 20:37 ` [PATCH V2 0/3] " Guenter Roeck 3 siblings, 1 reply; 9+ messages in thread From: Ludovic Barre @ 2019-05-02 14:08 UTC (permalink / raw) To: Wim Van Sebroeck, Guenter Roeck, Rob Herring Cc: Maxime Coquelin, Alexandre Torgue, linux-watchdog, linux-arm-kernel, linux-kernel, devicetree, linux-stm32, Ludovic Barre From: Ludovic Barre <ludovic.barre@st.com> This patch allows to define the max prescaler by compatible. To set a large range of timeout, the prescaler should be set dynamically (from the timeout request) to improve the resolution in order to have a timeout close to the expected value. Signed-off-by: Ludovic Barre <ludovic.barre@st.com> --- drivers/watchdog/stm32_iwdg.c | 76 ++++++++++++++++++++++++------------------- 1 file changed, 42 insertions(+), 34 deletions(-) diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c index f19a6d4..0c765d4 100644 --- a/drivers/watchdog/stm32_iwdg.c +++ b/drivers/watchdog/stm32_iwdg.c @@ -34,18 +34,12 @@ #define KR_KEY_EWA 0x5555 /* write access enable */ #define KR_KEY_DWA 0x0000 /* write access disable */ -/* IWDG_PR register bit values */ -#define PR_4 0x00 /* prescaler set to 4 */ -#define PR_8 0x01 /* prescaler set to 8 */ -#define PR_16 0x02 /* prescaler set to 16 */ -#define PR_32 0x03 /* prescaler set to 32 */ -#define PR_64 0x04 /* prescaler set to 64 */ -#define PR_128 0x05 /* prescaler set to 128 */ -#define PR_256 0x06 /* prescaler set to 256 */ +#define PR_SHIFT 2 +#define PR_MIN BIT(PR_SHIFT) /* IWDG_RLR register values */ -#define RLR_MIN 0x07C /* min value supported by reload register */ -#define RLR_MAX 0xFFF /* max value supported by reload register */ +#define RLR_MIN 0x2 /* min value recommended */ +#define RLR_MAX GENMASK(11, 0) /* max value of reload register */ /* IWDG_SR register bit mask */ #define FLAG_PVU BIT(0) /* Watchdog prescaler value update */ @@ -55,15 +49,28 @@ #define TIMEOUT_US 100000 #define SLEEP_US 1000 -#define HAS_PCLK true +struct stm32_iwdg_data { + bool has_pclk; + u32 max_prescaler; +}; + +static const struct stm32_iwdg_data stm32_iwdg_data = { + .has_pclk = false, + .max_prescaler = 256, +}; + +static const struct stm32_iwdg_data stm32mp1_iwdg_data = { + .has_pclk = true, + .max_prescaler = 1024, +}; struct stm32_iwdg { struct watchdog_device wdd; + const struct stm32_iwdg_data *data; void __iomem *regs; struct clk *clk_lsi; struct clk *clk_pclk; unsigned int rate; - bool has_pclk; }; static inline u32 reg_read(void __iomem *base, u32 reg) @@ -79,26 +86,28 @@ static inline void reg_write(void __iomem *base, u32 reg, u32 val) static int stm32_iwdg_start(struct watchdog_device *wdd) { struct stm32_iwdg *wdt = watchdog_get_drvdata(wdd); - u32 val = FLAG_PVU | FLAG_RVU; - u32 reload; + u32 presc, iwdg_rlr, iwdg_pr, iwdg_sr; dev_dbg(wdd->parent, "%s\n", __func__); - /* prescaler fixed to 256 */ - reload = clamp_t(unsigned int, ((wdd->timeout * wdt->rate) / 256) - 1, - RLR_MIN, RLR_MAX); + presc = DIV_ROUND_UP(wdd->timeout * wdt->rate, RLR_MAX + 1); + + /* The prescaler is align on power of 2 and start at 2 ^ PR_SHIFT. */ + presc = roundup_pow_of_two(presc); + iwdg_pr = presc <= 1 << PR_SHIFT ? 0 : ilog2(presc) - PR_SHIFT; + iwdg_rlr = ((wdd->timeout * wdt->rate) / presc) - 1; + /* enable watchdog */ + reg_write(wdt->regs, IWDG_KR, KR_KEY_ENABLE); /* enable write access */ reg_write(wdt->regs, IWDG_KR, KR_KEY_EWA); - /* set prescaler & reload registers */ - reg_write(wdt->regs, IWDG_PR, PR_256); /* prescaler fix to 256 */ - reg_write(wdt->regs, IWDG_RLR, reload); - reg_write(wdt->regs, IWDG_KR, KR_KEY_ENABLE); + reg_write(wdt->regs, IWDG_PR, iwdg_pr); + reg_write(wdt->regs, IWDG_RLR, iwdg_rlr); /* wait for the registers to be updated (max 100ms) */ - if (readl_relaxed_poll_timeout(wdt->regs + IWDG_SR, val, - !(val & (FLAG_PVU | FLAG_RVU)), + if (readl_relaxed_poll_timeout(wdt->regs + IWDG_SR, iwdg_sr, + !(iwdg_sr & (FLAG_PVU | FLAG_RVU)), SLEEP_US, TIMEOUT_US)) { dev_err(wdd->parent, "Fail to set prescaler, reload regs\n"); return -EIO; @@ -155,7 +164,7 @@ static int stm32_iwdg_clk_init(struct platform_device *pdev, } /* optional peripheral clock */ - if (wdt->has_pclk) { + if (wdt->data->has_pclk) { wdt->clk_pclk = devm_clk_get(&pdev->dev, "pclk"); if (IS_ERR(wdt->clk_pclk)) { dev_err(&pdev->dev, "Unable to get pclk clock\n"); @@ -196,8 +205,8 @@ static const struct watchdog_ops stm32_iwdg_ops = { }; static const struct of_device_id stm32_iwdg_of_match[] = { - { .compatible = "st,stm32-iwdg", .data = (void *)!HAS_PCLK }, - { .compatible = "st,stm32mp1-iwdg", .data = (void *)HAS_PCLK }, + { .compatible = "st,stm32-iwdg", .data = &stm32_iwdg_data }, + { .compatible = "st,stm32mp1-iwdg", .data = &stm32mp1_iwdg_data }, { /* end node */ } }; MODULE_DEVICE_TABLE(of, stm32_iwdg_of_match); @@ -205,20 +214,17 @@ MODULE_DEVICE_TABLE(of, stm32_iwdg_of_match); static int stm32_iwdg_probe(struct platform_device *pdev) { struct watchdog_device *wdd; - const struct of_device_id *match; struct stm32_iwdg *wdt; struct resource *res; int ret; - match = of_match_device(stm32_iwdg_of_match, &pdev->dev); - if (!match) - return -ENODEV; - wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); if (!wdt) return -ENOMEM; - wdt->has_pclk = match->data; + wdt->data = of_device_get_match_data(&pdev->dev); + if (!wdt->data) + return -ENODEV; /* This is the timer base. */ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -236,8 +242,10 @@ static int stm32_iwdg_probe(struct platform_device *pdev) wdd = &wdt->wdd; wdd->info = &stm32_iwdg_info; wdd->ops = &stm32_iwdg_ops; - wdd->min_timeout = ((RLR_MIN + 1) * 256) / wdt->rate; - wdd->max_hw_heartbeat_ms = ((RLR_MAX + 1) * 256 * 1000) / wdt->rate; + wdd->min_timeout = max_t(unsigned int, 1, + (((RLR_MIN + 1) * PR_MIN) / wdt->rate)); + wdd->max_hw_heartbeat_ms = ((RLR_MAX + 1) * wdt->data->max_prescaler * + 1000) / wdt->rate; wdd->parent = &pdev->dev; watchdog_set_drvdata(wdd, wdt); -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V2 3/3] watchdog: stm32: add dynamic prescaler support 2019-05-02 14:08 ` [PATCH V2 3/3] watchdog: stm32: add dynamic prescaler support Ludovic Barre @ 2019-05-02 20:35 ` Guenter Roeck 0 siblings, 0 replies; 9+ messages in thread From: Guenter Roeck @ 2019-05-02 20:35 UTC (permalink / raw) To: Ludovic Barre Cc: Wim Van Sebroeck, Rob Herring, Maxime Coquelin, Alexandre Torgue, linux-watchdog, linux-arm-kernel, linux-kernel, devicetree, linux-stm32 On Thu, May 02, 2019 at 04:08:46PM +0200, Ludovic Barre wrote: > From: Ludovic Barre <ludovic.barre@st.com> > > This patch allows to define the max prescaler by compatible. > To set a large range of timeout, the prescaler should be set > dynamically (from the timeout request) to improve the resolution > in order to have a timeout close to the expected value. > > Signed-off-by: Ludovic Barre <ludovic.barre@st.com> > --- > drivers/watchdog/stm32_iwdg.c | 76 ++++++++++++++++++++++++------------------- > 1 file changed, 42 insertions(+), 34 deletions(-) > > diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c > index f19a6d4..0c765d4 100644 > --- a/drivers/watchdog/stm32_iwdg.c > +++ b/drivers/watchdog/stm32_iwdg.c > @@ -34,18 +34,12 @@ > #define KR_KEY_EWA 0x5555 /* write access enable */ > #define KR_KEY_DWA 0x0000 /* write access disable */ > > -/* IWDG_PR register bit values */ > -#define PR_4 0x00 /* prescaler set to 4 */ > -#define PR_8 0x01 /* prescaler set to 8 */ > -#define PR_16 0x02 /* prescaler set to 16 */ > -#define PR_32 0x03 /* prescaler set to 32 */ > -#define PR_64 0x04 /* prescaler set to 64 */ > -#define PR_128 0x05 /* prescaler set to 128 */ > -#define PR_256 0x06 /* prescaler set to 256 */ > +#define PR_SHIFT 2 > +#define PR_MIN BIT(PR_SHIFT) > > /* IWDG_RLR register values */ > -#define RLR_MIN 0x07C /* min value supported by reload register */ > -#define RLR_MAX 0xFFF /* max value supported by reload register */ > +#define RLR_MIN 0x2 /* min value recommended */ > +#define RLR_MAX GENMASK(11, 0) /* max value of reload register */ > > /* IWDG_SR register bit mask */ > #define FLAG_PVU BIT(0) /* Watchdog prescaler value update */ > @@ -55,15 +49,28 @@ > #define TIMEOUT_US 100000 > #define SLEEP_US 1000 > > -#define HAS_PCLK true > +struct stm32_iwdg_data { > + bool has_pclk; > + u32 max_prescaler; > +}; > + > +static const struct stm32_iwdg_data stm32_iwdg_data = { > + .has_pclk = false, > + .max_prescaler = 256, > +}; > + > +static const struct stm32_iwdg_data stm32mp1_iwdg_data = { > + .has_pclk = true, > + .max_prescaler = 1024, > +}; > > struct stm32_iwdg { > struct watchdog_device wdd; > + const struct stm32_iwdg_data *data; > void __iomem *regs; > struct clk *clk_lsi; > struct clk *clk_pclk; > unsigned int rate; > - bool has_pclk; > }; > > static inline u32 reg_read(void __iomem *base, u32 reg) > @@ -79,26 +86,28 @@ static inline void reg_write(void __iomem *base, u32 reg, u32 val) > static int stm32_iwdg_start(struct watchdog_device *wdd) > { > struct stm32_iwdg *wdt = watchdog_get_drvdata(wdd); > - u32 val = FLAG_PVU | FLAG_RVU; > - u32 reload; > + u32 presc, iwdg_rlr, iwdg_pr, iwdg_sr; > > dev_dbg(wdd->parent, "%s\n", __func__); > > - /* prescaler fixed to 256 */ > - reload = clamp_t(unsigned int, ((wdd->timeout * wdt->rate) / 256) - 1, > - RLR_MIN, RLR_MAX); > + presc = DIV_ROUND_UP(wdd->timeout * wdt->rate, RLR_MAX + 1); > + > + /* The prescaler is align on power of 2 and start at 2 ^ PR_SHIFT. */ > + presc = roundup_pow_of_two(presc); > + iwdg_pr = presc <= 1 << PR_SHIFT ? 0 : ilog2(presc) - PR_SHIFT; > + iwdg_rlr = ((wdd->timeout * wdt->rate) / presc) - 1; > > + /* enable watchdog */ > + reg_write(wdt->regs, IWDG_KR, KR_KEY_ENABLE); > /* enable write access */ > reg_write(wdt->regs, IWDG_KR, KR_KEY_EWA); > - > /* set prescaler & reload registers */ > - reg_write(wdt->regs, IWDG_PR, PR_256); /* prescaler fix to 256 */ > - reg_write(wdt->regs, IWDG_RLR, reload); > - reg_write(wdt->regs, IWDG_KR, KR_KEY_ENABLE); > + reg_write(wdt->regs, IWDG_PR, iwdg_pr); > + reg_write(wdt->regs, IWDG_RLR, iwdg_rlr); > > /* wait for the registers to be updated (max 100ms) */ > - if (readl_relaxed_poll_timeout(wdt->regs + IWDG_SR, val, > - !(val & (FLAG_PVU | FLAG_RVU)), > + if (readl_relaxed_poll_timeout(wdt->regs + IWDG_SR, iwdg_sr, > + !(iwdg_sr & (FLAG_PVU | FLAG_RVU)), > SLEEP_US, TIMEOUT_US)) { > dev_err(wdd->parent, "Fail to set prescaler, reload regs\n"); > return -EIO; As mentioned in the other patch, pelase return the error returned from readl_relaxed_poll_timeout(). If that returns a timeout, we want to know about it and not just hide it behind -EIO. > @@ -155,7 +164,7 @@ static int stm32_iwdg_clk_init(struct platform_device *pdev, > } > > /* optional peripheral clock */ > - if (wdt->has_pclk) { > + if (wdt->data->has_pclk) { > wdt->clk_pclk = devm_clk_get(&pdev->dev, "pclk"); > if (IS_ERR(wdt->clk_pclk)) { > dev_err(&pdev->dev, "Unable to get pclk clock\n"); > @@ -196,8 +205,8 @@ static const struct watchdog_ops stm32_iwdg_ops = { > }; > > static const struct of_device_id stm32_iwdg_of_match[] = { > - { .compatible = "st,stm32-iwdg", .data = (void *)!HAS_PCLK }, > - { .compatible = "st,stm32mp1-iwdg", .data = (void *)HAS_PCLK }, > + { .compatible = "st,stm32-iwdg", .data = &stm32_iwdg_data }, > + { .compatible = "st,stm32mp1-iwdg", .data = &stm32mp1_iwdg_data }, > { /* end node */ } > }; > MODULE_DEVICE_TABLE(of, stm32_iwdg_of_match); > @@ -205,20 +214,17 @@ MODULE_DEVICE_TABLE(of, stm32_iwdg_of_match); > static int stm32_iwdg_probe(struct platform_device *pdev) > { > struct watchdog_device *wdd; > - const struct of_device_id *match; > struct stm32_iwdg *wdt; > struct resource *res; > int ret; > > - match = of_match_device(stm32_iwdg_of_match, &pdev->dev); > - if (!match) > - return -ENODEV; > - > wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > if (!wdt) > return -ENOMEM; > > - wdt->has_pclk = match->data; > + wdt->data = of_device_get_match_data(&pdev->dev); > + if (!wdt->data) > + return -ENODEV; > > /* This is the timer base. */ > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > @@ -236,8 +242,10 @@ static int stm32_iwdg_probe(struct platform_device *pdev) > wdd = &wdt->wdd; > wdd->info = &stm32_iwdg_info; > wdd->ops = &stm32_iwdg_ops; > - wdd->min_timeout = ((RLR_MIN + 1) * 256) / wdt->rate; > - wdd->max_hw_heartbeat_ms = ((RLR_MAX + 1) * 256 * 1000) / wdt->rate; > + wdd->min_timeout = max_t(unsigned int, 1, > + (((RLR_MIN + 1) * PR_MIN) / wdt->rate)); Is that any different to DIV_ROUND_UP() ? > + wdd->max_hw_heartbeat_ms = ((RLR_MAX + 1) * wdt->data->max_prescaler * > + 1000) / wdt->rate; > wdd->parent = &pdev->dev; > > watchdog_set_drvdata(wdd, wdt); > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V2 0/3] watchdog: stm32: add dynamic prescaler support 2019-05-02 14:08 [PATCH V2 0/3] watchdog: stm32: add dynamic prescaler support Ludovic Barre ` (2 preceding siblings ...) 2019-05-02 14:08 ` [PATCH V2 3/3] watchdog: stm32: add dynamic prescaler support Ludovic Barre @ 2019-05-02 20:37 ` Guenter Roeck 3 siblings, 0 replies; 9+ messages in thread From: Guenter Roeck @ 2019-05-02 20:37 UTC (permalink / raw) To: Ludovic Barre Cc: Wim Van Sebroeck, Rob Herring, Maxime Coquelin, Alexandre Torgue, linux-watchdog, linux-arm-kernel, linux-kernel, devicetree, linux-stm32 On Thu, May 02, 2019 at 04:08:43PM +0200, Ludovic Barre wrote: > From: Ludovic Barre <ludovic.barre@st.com> > > This patch series updates stm32 watchdog driver on: > -use devm_watchdog_register_device > -Guenter's recommendation about return value: > set_timeout return 0 on succes, -EINVAL for "parameter out of range" > and -EIO for "could not write value to the watchdog". No, sorry, that is not what I meant. set_timeout() should set ->timeout either to the requested value if equal to or larger than the maximum timeout supported by the hardware, and to the actually selected timeout otherwise. Guenter > Set of reload and prescaler registers are stay in start function, > because the stm32 watchdog must be enabled to write these registers. > -adds dynamic prescaler support > > Ludovic Barre (3): > watchdog: stm32: update to devm_watchdog_register_device > watchdog: stm32: update return values recommended by watchdog kernel > api > watchdog: stm32: add dynamic prescaler support > > drivers/watchdog/stm32_iwdg.c | 96 ++++++++++++++++++++++++------------------- > 1 file changed, 54 insertions(+), 42 deletions(-) > > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-05-03 7:59 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-02 14:08 [PATCH V2 0/3] watchdog: stm32: add dynamic prescaler support Ludovic Barre 2019-05-02 14:08 ` [PATCH V2 1/3] watchdog: stm32: update to devm_watchdog_register_device Ludovic Barre 2019-05-02 20:21 ` Guenter Roeck 2019-05-03 7:58 ` Ludovic BARRE 2019-05-02 14:08 ` [PATCH V2 2/3] watchdog: stm32: update return values recommended by watchdog kernel api Ludovic Barre 2019-05-02 20:23 ` Guenter Roeck 2019-05-02 14:08 ` [PATCH V2 3/3] watchdog: stm32: add dynamic prescaler support Ludovic Barre 2019-05-02 20:35 ` Guenter Roeck 2019-05-02 20:37 ` [PATCH V2 0/3] " Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).