| Message ID | 20190906205411.31666-2-jorge.ramirez-ortiz@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | qcom_wdt bark irq support | expand |
On Fri, Sep 06, 2019 at 10:54:10PM +0200, Jorge Ramirez-Ortiz wrote: > Use the bark interrupt as the pre-timeout notifier whenever this > interrupt is available. > > By default, the pretimeout notification shall occur one second earlier > than the timeout. > > Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> Nitpick below, otherwise: Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/watchdog/qcom-wdt.c | 70 ++++++++++++++++++++++++++++++++++--- > 1 file changed, 65 insertions(+), 5 deletions(-) > > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c > index 7be7f87be28f..935c78a882a3 100644 > --- a/drivers/watchdog/qcom-wdt.c > +++ b/drivers/watchdog/qcom-wdt.c > @@ -1,8 +1,10 @@ > // SPDX-License-Identifier: GPL-2.0-only > /* Copyright (c) 2014, The Linux Foundation. All rights reserved. > */ > +#include <linux/bits.h> > #include <linux/clk.h> > #include <linux/delay.h> > +#include <linux/interrupt.h> > #include <linux/io.h> > #include <linux/kernel.h> > #include <linux/module.h> > @@ -19,6 +21,9 @@ enum wdt_reg { > WDT_BITE_TIME, > }; > > +#define QCOM_WDT_ENABLE BIT(0) > +#define QCOM_WDT_ENABLE_IRQ BIT(1) > + > static const u32 reg_offset_data_apcs_tmr[] = { > [WDT_RST] = 0x38, > [WDT_EN] = 0x40, > @@ -54,15 +59,35 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd) > return container_of(wdd, struct qcom_wdt, wdd); > } > > +static inline int qcom_get_enable(struct watchdog_device *wdd) > +{ > + int enable = QCOM_WDT_ENABLE; > + > + if (wdd->pretimeout) > + enable |= QCOM_WDT_ENABLE_IRQ; > + > + return enable; > +} > + > +static irqreturn_t qcom_wdt_isr(int irq, void *arg) > +{ > + struct watchdog_device *wdd = arg; > + > + watchdog_notify_pretimeout(wdd); > + > + return IRQ_HANDLED; > +} > + > static int qcom_wdt_start(struct watchdog_device *wdd) > { > struct qcom_wdt *wdt = to_qcom_wdt(wdd); > + unsigned int bark = wdd->timeout - wdd->pretimeout; > > writel(0, wdt_addr(wdt, WDT_EN)); > writel(1, wdt_addr(wdt, WDT_RST)); > - writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME)); > + writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME)); > writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME)); > - writel(1, wdt_addr(wdt, WDT_EN)); > + writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN)); > return 0; > } > > @@ -89,6 +114,13 @@ static int qcom_wdt_set_timeout(struct watchdog_device *wdd, > return qcom_wdt_start(wdd); > } > > +static int qcom_wdt_set_pretimeout(struct watchdog_device *wdd, > + unsigned int timeout) > +{ > + wdd->pretimeout = timeout; > + return qcom_wdt_start(wdd); > +} > + > static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action, > void *data) > { > @@ -105,7 +137,7 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action, > writel(1, wdt_addr(wdt, WDT_RST)); > writel(timeout, wdt_addr(wdt, WDT_BARK_TIME)); > writel(timeout, wdt_addr(wdt, WDT_BITE_TIME)); > - writel(1, wdt_addr(wdt, WDT_EN)); > + writel(QCOM_WDT_ENABLE, wdt_addr(wdt, WDT_EN)); > > /* > * Actually make sure the above sequence hits hardware before sleeping. > @@ -121,6 +153,7 @@ static const struct watchdog_ops qcom_wdt_ops = { > .stop = qcom_wdt_stop, > .ping = qcom_wdt_ping, > .set_timeout = qcom_wdt_set_timeout, > + .set_pretimeout = qcom_wdt_set_pretimeout, > .restart = qcom_wdt_restart, > .owner = THIS_MODULE, > }; > @@ -133,6 +166,15 @@ static const struct watchdog_info qcom_wdt_info = { > .identity = KBUILD_MODNAME, > }; > > +static const struct watchdog_info qcom_wdt_pt_info = { > + .options = WDIOF_KEEPALIVEPING > + | WDIOF_MAGICCLOSE > + | WDIOF_SETTIMEOUT > + | WDIOF_PRETIMEOUT > + | WDIOF_CARDRESET, > + .identity = KBUILD_MODNAME, > +}; > + > static void qcom_clk_disable_unprepare(void *data) > { > clk_disable_unprepare(data); > @@ -146,7 +188,7 @@ static int qcom_wdt_probe(struct platform_device *pdev) > struct device_node *np = dev->of_node; > const u32 *regs; > u32 percpu_offset; > - int ret; > + int irq, ret; > > regs = of_device_get_match_data(dev); > if (!regs) { > @@ -204,7 +246,25 @@ static int qcom_wdt_probe(struct platform_device *pdev) > return -EINVAL; > } > > - wdt->wdd.info = &qcom_wdt_info; > + /* check if there is pretimeout support */ > + irq = platform_get_irq(pdev, 0); > + if (irq > 0) { > + ret = devm_request_irq(dev, irq, qcom_wdt_isr, > + IRQF_TRIGGER_RISING, > + "wdt_bark", &wdt->wdd); > + if (ret) > + return ret; > + > + wdt->wdd.info = &qcom_wdt_pt_info; > + wdt->wdd.pretimeout = 1; > + } else { > + if (irq == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > + wdt->wdd.info = &qcom_wdt_info; > + wdt->wdd.pretimeout = 0; It is not necessary to set pretimeout to 0; the data structure was allocated with devm_kzalloc(). The compiler won't know that and generate unnecessary code otherwise. > + } > + > wdt->wdd.ops = &qcom_wdt_ops; > wdt->wdd.min_timeout = 1; > wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
On 10/09/19 11:06:55, Guenter Roeck wrote: > On Fri, Sep 06, 2019 at 10:54:10PM +0200, Jorge Ramirez-Ortiz wrote: > > Use the bark interrupt as the pre-timeout notifier whenever this > > interrupt is available. > > > > By default, the pretimeout notification shall occur one second earlier > > than the timeout. > > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> > > Nitpick below, otherwise: > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > > --- > > drivers/watchdog/qcom-wdt.c | 70 ++++++++++++++++++++++++++++++++++--- > > 1 file changed, 65 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c > > index 7be7f87be28f..935c78a882a3 100644 > > --- a/drivers/watchdog/qcom-wdt.c > > +++ b/drivers/watchdog/qcom-wdt.c > > @@ -1,8 +1,10 @@ > > // SPDX-License-Identifier: GPL-2.0-only > > /* Copyright (c) 2014, The Linux Foundation. All rights reserved. > > */ > > +#include <linux/bits.h> > > #include <linux/clk.h> > > #include <linux/delay.h> > > +#include <linux/interrupt.h> > > #include <linux/io.h> > > #include <linux/kernel.h> > > #include <linux/module.h> > > @@ -19,6 +21,9 @@ enum wdt_reg { > > WDT_BITE_TIME, > > }; > > > > +#define QCOM_WDT_ENABLE BIT(0) > > +#define QCOM_WDT_ENABLE_IRQ BIT(1) > > + > > static const u32 reg_offset_data_apcs_tmr[] = { > > [WDT_RST] = 0x38, > > [WDT_EN] = 0x40, > > @@ -54,15 +59,35 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd) > > return container_of(wdd, struct qcom_wdt, wdd); > > } > > > > +static inline int qcom_get_enable(struct watchdog_device *wdd) > > +{ > > + int enable = QCOM_WDT_ENABLE; > > + > > + if (wdd->pretimeout) > > + enable |= QCOM_WDT_ENABLE_IRQ; > > + > > + return enable; > > +} > > + > > +static irqreturn_t qcom_wdt_isr(int irq, void *arg) > > +{ > > + struct watchdog_device *wdd = arg; > > + > > + watchdog_notify_pretimeout(wdd); > > + > > + return IRQ_HANDLED; > > +} > > + > > static int qcom_wdt_start(struct watchdog_device *wdd) > > { > > struct qcom_wdt *wdt = to_qcom_wdt(wdd); > > + unsigned int bark = wdd->timeout - wdd->pretimeout; > > > > writel(0, wdt_addr(wdt, WDT_EN)); > > writel(1, wdt_addr(wdt, WDT_RST)); > > - writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME)); > > + writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME)); > > writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME)); > > - writel(1, wdt_addr(wdt, WDT_EN)); > > + writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN)); > > return 0; > > } > > > > @@ -89,6 +114,13 @@ static int qcom_wdt_set_timeout(struct watchdog_device *wdd, > > return qcom_wdt_start(wdd); > > } > > > > +static int qcom_wdt_set_pretimeout(struct watchdog_device *wdd, > > + unsigned int timeout) > > +{ > > + wdd->pretimeout = timeout; > > + return qcom_wdt_start(wdd); > > +} > > + > > static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action, > > void *data) > > { > > @@ -105,7 +137,7 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action, > > writel(1, wdt_addr(wdt, WDT_RST)); > > writel(timeout, wdt_addr(wdt, WDT_BARK_TIME)); > > writel(timeout, wdt_addr(wdt, WDT_BITE_TIME)); > > - writel(1, wdt_addr(wdt, WDT_EN)); > > + writel(QCOM_WDT_ENABLE, wdt_addr(wdt, WDT_EN)); > > > > /* > > * Actually make sure the above sequence hits hardware before sleeping. > > @@ -121,6 +153,7 @@ static const struct watchdog_ops qcom_wdt_ops = { > > .stop = qcom_wdt_stop, > > .ping = qcom_wdt_ping, > > .set_timeout = qcom_wdt_set_timeout, > > + .set_pretimeout = qcom_wdt_set_pretimeout, > > .restart = qcom_wdt_restart, > > .owner = THIS_MODULE, > > }; > > @@ -133,6 +166,15 @@ static const struct watchdog_info qcom_wdt_info = { > > .identity = KBUILD_MODNAME, > > }; > > > > +static const struct watchdog_info qcom_wdt_pt_info = { > > + .options = WDIOF_KEEPALIVEPING > > + | WDIOF_MAGICCLOSE > > + | WDIOF_SETTIMEOUT > > + | WDIOF_PRETIMEOUT > > + | WDIOF_CARDRESET, > > + .identity = KBUILD_MODNAME, > > +}; > > + > > static void qcom_clk_disable_unprepare(void *data) > > { > > clk_disable_unprepare(data); > > @@ -146,7 +188,7 @@ static int qcom_wdt_probe(struct platform_device *pdev) > > struct device_node *np = dev->of_node; > > const u32 *regs; > > u32 percpu_offset; > > - int ret; > > + int irq, ret; > > > > regs = of_device_get_match_data(dev); > > if (!regs) { > > @@ -204,7 +246,25 @@ static int qcom_wdt_probe(struct platform_device *pdev) > > return -EINVAL; > > } > > > > - wdt->wdd.info = &qcom_wdt_info; > > + /* check if there is pretimeout support */ > > + irq = platform_get_irq(pdev, 0); > > + if (irq > 0) { > > + ret = devm_request_irq(dev, irq, qcom_wdt_isr, > > + IRQF_TRIGGER_RISING, > > + "wdt_bark", &wdt->wdd); > > + if (ret) > > + return ret; > > + > > + wdt->wdd.info = &qcom_wdt_pt_info; > > + wdt->wdd.pretimeout = 1; > > + } else { > > + if (irq == -EPROBE_DEFER) > > + return -EPROBE_DEFER; > > + > > + wdt->wdd.info = &qcom_wdt_info; > > + wdt->wdd.pretimeout = 0; > > It is not necessary to set pretimeout to 0; the data structure was > allocated with devm_kzalloc(). The compiler won't know that and > generate unnecessary code otherwise. will you need me to send another version or could you pick it up as is? > > > + } > > + > > wdt->wdd.ops = &qcom_wdt_ops; > > wdt->wdd.min_timeout = 1; > > wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
On Thu, Sep 12, 2019 at 09:24:54AM +0200, Jorge Ramirez-Ortiz, Linaro wrote: > On 10/09/19 11:06:55, Guenter Roeck wrote: > > On Fri, Sep 06, 2019 at 10:54:10PM +0200, Jorge Ramirez-Ortiz wrote: > > > Use the bark interrupt as the pre-timeout notifier whenever this > > > interrupt is available. > > > > > > By default, the pretimeout notification shall occur one second earlier > > > than the timeout. > > > > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> > > > > Nitpick below, otherwise: > > > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > > > > + wdt->wdd.pretimeout = 0; > > > > It is not necessary to set pretimeout to 0; the data structure was > > allocated with devm_kzalloc(). The compiler won't know that and > > generate unnecessary code otherwise. > > will you need me to send another version or could you pick it up as is? > I applied the patch to my watchdog-next branch, with the line removed. Let's assume that Wim will pick it up from there. If not, and the line stays in, no real damage. In short, no need to resubmit. Guenter
diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c index 7be7f87be28f..935c78a882a3 100644 --- a/drivers/watchdog/qcom-wdt.c +++ b/drivers/watchdog/qcom-wdt.c @@ -1,8 +1,10 @@ // SPDX-License-Identifier: GPL-2.0-only /* Copyright (c) 2014, The Linux Foundation. All rights reserved. */ +#include <linux/bits.h> #include <linux/clk.h> #include <linux/delay.h> +#include <linux/interrupt.h> #include <linux/io.h> #include <linux/kernel.h> #include <linux/module.h> @@ -19,6 +21,9 @@ enum wdt_reg { WDT_BITE_TIME, }; +#define QCOM_WDT_ENABLE BIT(0) +#define QCOM_WDT_ENABLE_IRQ BIT(1) + static const u32 reg_offset_data_apcs_tmr[] = { [WDT_RST] = 0x38, [WDT_EN] = 0x40, @@ -54,15 +59,35 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd) return container_of(wdd, struct qcom_wdt, wdd); } +static inline int qcom_get_enable(struct watchdog_device *wdd) +{ + int enable = QCOM_WDT_ENABLE; + + if (wdd->pretimeout) + enable |= QCOM_WDT_ENABLE_IRQ; + + return enable; +} + +static irqreturn_t qcom_wdt_isr(int irq, void *arg) +{ + struct watchdog_device *wdd = arg; + + watchdog_notify_pretimeout(wdd); + + return IRQ_HANDLED; +} + static int qcom_wdt_start(struct watchdog_device *wdd) { struct qcom_wdt *wdt = to_qcom_wdt(wdd); + unsigned int bark = wdd->timeout - wdd->pretimeout; writel(0, wdt_addr(wdt, WDT_EN)); writel(1, wdt_addr(wdt, WDT_RST)); - writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME)); + writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME)); writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME)); - writel(1, wdt_addr(wdt, WDT_EN)); + writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN)); return 0; } @@ -89,6 +114,13 @@ static int qcom_wdt_set_timeout(struct watchdog_device *wdd, return qcom_wdt_start(wdd); } +static int qcom_wdt_set_pretimeout(struct watchdog_device *wdd, + unsigned int timeout) +{ + wdd->pretimeout = timeout; + return qcom_wdt_start(wdd); +} + static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action, void *data) { @@ -105,7 +137,7 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action, writel(1, wdt_addr(wdt, WDT_RST)); writel(timeout, wdt_addr(wdt, WDT_BARK_TIME)); writel(timeout, wdt_addr(wdt, WDT_BITE_TIME)); - writel(1, wdt_addr(wdt, WDT_EN)); + writel(QCOM_WDT_ENABLE, wdt_addr(wdt, WDT_EN)); /* * Actually make sure the above sequence hits hardware before sleeping. @@ -121,6 +153,7 @@ static const struct watchdog_ops qcom_wdt_ops = { .stop = qcom_wdt_stop, .ping = qcom_wdt_ping, .set_timeout = qcom_wdt_set_timeout, + .set_pretimeout = qcom_wdt_set_pretimeout, .restart = qcom_wdt_restart, .owner = THIS_MODULE, }; @@ -133,6 +166,15 @@ static const struct watchdog_info qcom_wdt_info = { .identity = KBUILD_MODNAME, }; +static const struct watchdog_info qcom_wdt_pt_info = { + .options = WDIOF_KEEPALIVEPING + | WDIOF_MAGICCLOSE + | WDIOF_SETTIMEOUT + | WDIOF_PRETIMEOUT + | WDIOF_CARDRESET, + .identity = KBUILD_MODNAME, +}; + static void qcom_clk_disable_unprepare(void *data) { clk_disable_unprepare(data); @@ -146,7 +188,7 @@ static int qcom_wdt_probe(struct platform_device *pdev) struct device_node *np = dev->of_node; const u32 *regs; u32 percpu_offset; - int ret; + int irq, ret; regs = of_device_get_match_data(dev); if (!regs) { @@ -204,7 +246,25 @@ static int qcom_wdt_probe(struct platform_device *pdev) return -EINVAL; } - wdt->wdd.info = &qcom_wdt_info; + /* check if there is pretimeout support */ + irq = platform_get_irq(pdev, 0); + if (irq > 0) { + ret = devm_request_irq(dev, irq, qcom_wdt_isr, + IRQF_TRIGGER_RISING, + "wdt_bark", &wdt->wdd); + if (ret) + return ret; + + wdt->wdd.info = &qcom_wdt_pt_info; + wdt->wdd.pretimeout = 1; + } else { + if (irq == -EPROBE_DEFER) + return -EPROBE_DEFER; + + wdt->wdd.info = &qcom_wdt_info; + wdt->wdd.pretimeout = 0; + } + wdt->wdd.ops = &qcom_wdt_ops; wdt->wdd.min_timeout = 1; wdt->wdd.max_timeout = 0x10000000U / wdt->rate;
Use the bark interrupt as the pre-timeout notifier whenever this interrupt is available. By default, the pretimeout notification shall occur one second earlier than the timeout. Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> --- drivers/watchdog/qcom-wdt.c | 70 ++++++++++++++++++++++++++++++++++--- 1 file changed, 65 insertions(+), 5 deletions(-)