| Message ID | 20220705142444.17063-1-pboos@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] watchdog: meson: keep running if already active | expand |
Hi, On 05/07/2022 16:24, Philippe Boos wrote: > If the watchdog is already running (e.g.: started by bootloader) then > the kernel driver should keep the watchdog active but the amlogic driver > turns it off. > > Let the driver fix the clock rate then restart the watchdog if it was > previously active. > > Reviewed-by: Jerome Brunet <jbrunet@baylibre.com> Please drop this review tag since it was done off-list > Signed-off-by: Philippe Boos <pboos@baylibre.com> > --- > drivers/watchdog/meson_gxbb_wdt.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c > index 5a9ca10fbcfa..8c2c6f7f3bb5 100644 > --- a/drivers/watchdog/meson_gxbb_wdt.c > +++ b/drivers/watchdog/meson_gxbb_wdt.c > @@ -146,6 +146,7 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct meson_gxbb_wdt *data; > int ret; > + u32 regval; > > data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > if (!data) > @@ -177,6 +178,8 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev) > data->wdt_dev.timeout = DEFAULT_TIMEOUT; > watchdog_set_drvdata(&data->wdt_dev, data); > > + regval = readl(data->reg_base + GXBB_WDT_CTRL_REG); > + > /* Setup with 1ms timebase */ > writel(((clk_get_rate(data->clk) / 1000) & GXBB_WDT_CTRL_DIV_MASK) | > GXBB_WDT_CTRL_EE_RESET | > @@ -186,6 +189,13 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev) > > meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout); > > + if ((regval & GXBB_WDT_CTRL_EN) != 0) { > + ret = meson_gxbb_wdt_start(&data->wdt_dev); > + if (ret) > + return ret; > + set_bit(WDOG_HW_RUNNING, &data->wdt_dev.status); > + } > + > watchdog_stop_on_reboot(&data->wdt_dev); > return devm_watchdog_register_device(dev, &data->wdt_dev); > } I think it would be much claner to leave the watchdog enabled, and get the parameters from the registers and update the wdt_dev accordingly. Neil
On Tue 05 Jul 2022 at 16:39, Neil Armstrong <narmstrong@baylibre.com> wrote: > Hi, > > On 05/07/2022 16:24, Philippe Boos wrote: >> If the watchdog is already running (e.g.: started by bootloader) then >> the kernel driver should keep the watchdog active but the amlogic driver >> turns it off. >> Let the driver fix the clock rate then restart the watchdog if it was >> previously active. >> Reviewed-by: Jerome Brunet <jbrunet@baylibre.com> > > Please drop this review tag since it was done off-list Indeed a review was done off-list. Reviewed-by says a review has been done. I was not aware this applied to public reviews only. I probably missed that, would you mind pointing me to that rule please ? > >> Signed-off-by: Philippe Boos <pboos@baylibre.com> >> --- >> drivers/watchdog/meson_gxbb_wdt.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> diff --git a/drivers/watchdog/meson_gxbb_wdt.c >> b/drivers/watchdog/meson_gxbb_wdt.c >> index 5a9ca10fbcfa..8c2c6f7f3bb5 100644 >> --- a/drivers/watchdog/meson_gxbb_wdt.c >> +++ b/drivers/watchdog/meson_gxbb_wdt.c >> @@ -146,6 +146,7 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev) >> struct device *dev = &pdev->dev; >> struct meson_gxbb_wdt *data; >> int ret; >> + u32 regval; >> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); >> if (!data) >> @@ -177,6 +178,8 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev) >> data->wdt_dev.timeout = DEFAULT_TIMEOUT; >> watchdog_set_drvdata(&data->wdt_dev, data); >> + regval = readl(data->reg_base + GXBB_WDT_CTRL_REG); > + >> /* Setup with 1ms timebase */ >> writel(((clk_get_rate(data->clk) / 1000) & GXBB_WDT_CTRL_DIV_MASK) | >> GXBB_WDT_CTRL_EE_RESET | >> @@ -186,6 +189,13 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev) >> meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout); >> + if ((regval & GXBB_WDT_CTRL_EN) != 0) { >> + ret = meson_gxbb_wdt_start(&data->wdt_dev); >> + if (ret) >> + return ret; >> + set_bit(WDOG_HW_RUNNING, &data->wdt_dev.status); >> + } >> + >> watchdog_stop_on_reboot(&data->wdt_dev); >> return devm_watchdog_register_device(dev, &data->wdt_dev); >> } > > I think it would be much claner to leave the watchdog enabled, and get the > parameters > from the registers and update the wdt_dev accordingly. The problem is updating the time base (ie, the clock divider) while the watchdog is running. We should not make an assumption about what the bootloader is going to leave us. It could be safe if we could update the divider, the timeout and ping the watchdog with a single write. There is several registers for that, so not possible. It is safer to update the divider with the watchdog off. There is indeed a small window of opportunity were a crash could happen with the watchdog off. What is proposed here is progress compared to what we have right now, which is waiting for userspace to come up to start the watchdog again. The only safe way to keep the watchdog on throughout is to keep whatever time base was set by the bootloader. That means major rework since the whole driver rely on a 1ms time base. I'm not sure re-using the bootloader settings is such a great idea anyway. > > Neil
On Tue, Jul 05, 2022 at 09:29:35PM +0200, Jerome Brunet wrote: > > On Tue 05 Jul 2022 at 16:39, Neil Armstrong <narmstrong@baylibre.com> wrote: > > > Hi, > > > > On 05/07/2022 16:24, Philippe Boos wrote: > >> If the watchdog is already running (e.g.: started by bootloader) then > >> the kernel driver should keep the watchdog active but the amlogic driver > >> turns it off. > >> Let the driver fix the clock rate then restart the watchdog if it was > >> previously active. > >> Reviewed-by: Jerome Brunet <jbrunet@baylibre.com> > > > > Please drop this review tag since it was done off-list > > Indeed a review was done off-list. > > Reviewed-by says a review has been done. I was not aware this applied to > public reviews only. I probably missed that, would you mind pointing me > to that rule please ? > Public or not doesn't really matter. However, you can only apply a Reviewed-by: tag (or any tag, really) if you explicitly received one. The exchange seems to suggest that you did not receive that tag. Please never add any tags on your own. On the other side, if the reviewer did send a Reviewed-by: tag off list, I would kindly ask the reviewer to not do that in the future to avoid misunderstandings. If you don't want your Reviewed-by: tag attached to a patch, don't send one. Not everyone will even realize that you sent your tag off-list, and no one can be expected to know that you didn't really mean it when you sent your tag. Thanks, Guenter
On Wed 06 Jul 2022 at 05:41, Guenter Roeck <linux@roeck-us.net> wrote: > On Tue, Jul 05, 2022 at 09:29:35PM +0200, Jerome Brunet wrote: >> >> On Tue 05 Jul 2022 at 16:39, Neil Armstrong <narmstrong@baylibre.com> wrote: >> >> > Hi, >> > >> > On 05/07/2022 16:24, Philippe Boos wrote: >> >> If the watchdog is already running (e.g.: started by bootloader) then >> >> the kernel driver should keep the watchdog active but the amlogic driver >> >> turns it off. >> >> Let the driver fix the clock rate then restart the watchdog if it was >> >> previously active. >> >> Reviewed-by: Jerome Brunet <jbrunet@baylibre.com> >> > >> > Please drop this review tag since it was done off-list >> >> Indeed a review was done off-list. >> >> Reviewed-by says a review has been done. I was not aware this applied to >> public reviews only. I probably missed that, would you mind pointing me >> to that rule please ? >> > > Public or not doesn't really matter. However, you can only apply a > Reviewed-by: tag (or any tag, really) if you explicitly received one. > The exchange seems to suggest that you did not receive that tag. Philippe did receive that Reviewed-by. I gave it to him. Doing his first public patches, he first requested a review off-list to try to get things right and not bother people too much (so much for that :/) > Please never add any tags on your own. He did not. > > On the other side, if the reviewer did send a Reviewed-by: tag off list, > I would kindly ask the reviewer to not do that in the future to avoid > misunderstandings. No worries. That being said, I have gone over https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst It just says that Reviewed-by sent on the list should be collected for the following version. Nothing against adding the tag if the job has been done, on or off list. Same goes for Suggested-by, Tested-by, etc. If I missed something or it is non-written rule, please let me know. > If you don't want your Reviewed-by: tag attached to > a patch, don't send one. Not everyone will even realize that you sent > your tag off-list, and no one can be expected to know that you didn't > really mean it when you sent your tag. I do want Philippe to add it. I would not have given it the first place if it was not the case. > > Thanks, > Guenter
On Wed, Jul 06, 2022 at 03:24:27PM +0200, Jerome Brunet wrote: > [ ... ] > > No worries. That being said, I have gone over > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst > > It just says that Reviewed-by sent on the list should be collected for > the following version. Nothing against adding the tag if the job has > been done, on or off list. Same goes for Suggested-by, Tested-by, etc. > > If I missed something or it is non-written rule, please let me know. Your interpretation is quite a strict one. I don't think there is a rule that states that tags not sent to a list must not be collected. Anyway, I would have called it common sense, especially since it does happen that someone accidentally hits "reply" instead of "reply all" and replies end up not being sent to the list. If you expect me to dig through e-mail headers to determine if you meant your tags to be published or not, sorry, that won't happen. Do not send me e-mails with any tags unless you accept that they may be published. Guenter
On Wed 06 Jul 2022 at 09:43, Guenter Roeck <linux@roeck-us.net> wrote: > On Wed, Jul 06, 2022 at 03:24:27PM +0200, Jerome Brunet wrote: >> > [ ... ] > >> >> No worries. That being said, I have gone over >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst >> >> It just says that Reviewed-by sent on the list should be collected for >> the following version. Nothing against adding the tag if the job has >> been done, on or off list. Same goes for Suggested-by, Tested-by, etc. >> >> If I missed something or it is non-written rule, please let me know. > > Your interpretation is quite a strict one. I don't think there is a rule > that states that tags not sent to a list must not be collected. > > Anyway, I would have called it common sense, especially since it does > happen that someone accidentally hits "reply" instead of "reply all" > and replies end up not being sent to the list. If you expect me to dig > through e-mail headers to determine if you meant your tags to be published > or not, sorry, that won't happen. Do not send me e-mails with any tags > unless you accept that they may be published. > > Guenter Exactly. Not expecting anything. I'm 100% aligned with you. Maybe this is where is misunderstanding is. The Reviewed-by was there when Philippe sent his mail. I was meant to be there and taken. Plain and simple. No other expectation whatsoever. I'm not asking for the tag to dropped, quite the opposite actually.
diff --git a/drivers/watchdog/meson_gxbb_wdt.c b/drivers/watchdog/meson_gxbb_wdt.c index 5a9ca10fbcfa..8c2c6f7f3bb5 100644 --- a/drivers/watchdog/meson_gxbb_wdt.c +++ b/drivers/watchdog/meson_gxbb_wdt.c @@ -146,6 +146,7 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct meson_gxbb_wdt *data; int ret; + u32 regval; data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); if (!data) @@ -177,6 +178,8 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev) data->wdt_dev.timeout = DEFAULT_TIMEOUT; watchdog_set_drvdata(&data->wdt_dev, data); + regval = readl(data->reg_base + GXBB_WDT_CTRL_REG); + /* Setup with 1ms timebase */ writel(((clk_get_rate(data->clk) / 1000) & GXBB_WDT_CTRL_DIV_MASK) | GXBB_WDT_CTRL_EE_RESET | @@ -186,6 +189,13 @@ static int meson_gxbb_wdt_probe(struct platform_device *pdev) meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout); + if ((regval & GXBB_WDT_CTRL_EN) != 0) { + ret = meson_gxbb_wdt_start(&data->wdt_dev); + if (ret) + return ret; + set_bit(WDOG_HW_RUNNING, &data->wdt_dev.status); + } + watchdog_stop_on_reboot(&data->wdt_dev); return devm_watchdog_register_device(dev, &data->wdt_dev); }