| Message ID | 1598600968-28498-2-git-send-email-freddy.hsin@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refine mtk wdt driver init flow | expand |
On 8/28/20 12:49 AM, Freddy Hsin wrote: > 1. add a hw initialization function > 2. enable/disable the watchdog depends on the original hw setting > 3. set WDOD_HW_RUNNING in start function in order to start > kicker after driver probe and clear the bit in stop function > > Change-Id: I25aa797f3b88288f26984455e499e599e27f09fa > Signed-off-by: Freddy Hsin <freddy.hsin@mediatek.com> The subject is misleading - what this really does it to honor that/if a watchdog is already running. However, WDOG_HW_RUNNING should not generally be set/cleared in the start/stop function. Also, calling mtk_wdt_stop is pointless if WDT_MODE_EN is not set, since clearing WDT_MODE_EN is all it does. On top of that, setting WDOG_HW_RUNNING in the probe function requires that max_hw_heartbeat_ms is set (instead of max_timeout). So if you really want to honor that the watchdog is already running at boot, you would have to initialize max_hw_heartbeat_ms, set WDOG_HW_RUNNING explicitly, and possibly call the ping function before changing the timeout. Thanks, Guenter > --- > drivers/watchdog/mtk_wdt.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c > index d6a6393..59b5061 100644 > --- a/drivers/watchdog/mtk_wdt.c > +++ b/drivers/watchdog/mtk_wdt.c > @@ -57,6 +57,9 @@ > static bool nowayout = WATCHDOG_NOWAYOUT; > static unsigned int timeout; > > +static int mtk_wdt_start(struct watchdog_device *wdt_dev); > +static int mtk_wdt_stop(struct watchdog_device *wdt_dev); > + > struct mtk_wdt_dev { > struct watchdog_device wdt_dev; > void __iomem *wdt_base; > @@ -148,6 +151,19 @@ static int toprgu_register_reset_controller(struct platform_device *pdev, > return ret; > } > > +static int mtk_wdt_init(struct watchdog_device *wdt_dev) > +{ > + struct mtk_wdt_dev *mtk_wdt = watchdog_get_drvdata(wdt_dev); > + void __iomem *wdt_base; > + > + wdt_base = mtk_wdt->wdt_base; > + > + if (readl(wdt_base + WDT_MODE) & WDT_MODE_EN) > + mtk_wdt_start(wdt_dev); > + else > + mtk_wdt_stop(wdt_dev); > +} > + > static int mtk_wdt_restart(struct watchdog_device *wdt_dev, > unsigned long action, void *data) > { > @@ -206,6 +222,8 @@ static int mtk_wdt_stop(struct watchdog_device *wdt_dev) > reg |= WDT_MODE_KEY; > iowrite32(reg, wdt_base + WDT_MODE); > > + clear_bit(WDOG_HW_RUNNING, &wdt_dev->status); > + > return 0; > } > > @@ -225,6 +243,8 @@ static int mtk_wdt_start(struct watchdog_device *wdt_dev) > reg |= (WDT_MODE_EN | WDT_MODE_KEY); > iowrite32(reg, wdt_base + WDT_MODE); > > + set_bit(WDOG_HW_RUNNING, &wdt_dev->status); > + > return 0; > } > > @@ -274,7 +294,7 @@ static int mtk_wdt_probe(struct platform_device *pdev) > > watchdog_set_drvdata(&mtk_wdt->wdt_dev, mtk_wdt); > > - mtk_wdt_stop(&mtk_wdt->wdt_dev); > + mtk_wdt_init(&mtk_wdt->wdt_dev); > > watchdog_stop_on_reboot(&mtk_wdt->wdt_dev); > err = devm_watchdog_register_device(dev, &mtk_wdt->wdt_dev); >
On Fri, 2020-08-28 at 07:26 -0700, Guenter Roeck wrote: > On 8/28/20 12:49 AM, Freddy Hsin wrote: > > 1. add a hw initialization function > > 2. enable/disable the watchdog depends on the original hw setting > > 3. set WDOD_HW_RUNNING in start function in order to start > > kicker after driver probe and clear the bit in stop function > > > > Change-Id: I25aa797f3b88288f26984455e499e599e27f09fa > > Signed-off-by: Freddy Hsin <freddy.hsin@mediatek.com> > > The subject is misleading - what this really does it to honor > that/if a watchdog is already running. However, WDOG_HW_RUNNING > should not generally be set/cleared in the start/stop function. > Also, calling mtk_wdt_stop is pointless if WDT_MODE_EN is not set, > since clearing WDT_MODE_EN is all it does. On top of that, > setting WDOG_HW_RUNNING in the probe function requires that > max_hw_heartbeat_ms is set (instead of max_timeout). > > So if you really want to honor that the watchdog is already running > at boot, you would have to initialize max_hw_heartbeat_ms, set > WDOG_HW_RUNNING explicitly, and possibly call the ping function > before changing the timeout. > > Thanks, > Guenter Hi Guenter, Thanks for you suggestion. I will submit a new patch according to your advice. The purpose of this patch is to control WDT_MODE_EN in the first stage of preloader bring up, and it should not be disabled afterward to prevent the system freeze and cannot be reset, so remove the mtk_wdt_stop function and keep the original state set in preloader. > > > --- > > drivers/watchdog/mtk_wdt.c | 22 +++++++++++++++++++++- > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c > > index d6a6393..59b5061 100644 > > --- a/drivers/watchdog/mtk_wdt.c > > +++ b/drivers/watchdog/mtk_wdt.c > > @@ -57,6 +57,9 @@ > > static bool nowayout = WATCHDOG_NOWAYOUT; > > static unsigned int timeout; > > > > +static int mtk_wdt_start(struct watchdog_device *wdt_dev); > > +static int mtk_wdt_stop(struct watchdog_device *wdt_dev); > > + > > struct mtk_wdt_dev { > > struct watchdog_device wdt_dev; > > void __iomem *wdt_base; > > @@ -148,6 +151,19 @@ static int toprgu_register_reset_controller(struct platform_device *pdev, > > return ret; > > } > > > > +static int mtk_wdt_init(struct watchdog_device *wdt_dev) > > +{ > > + struct mtk_wdt_dev *mtk_wdt = watchdog_get_drvdata(wdt_dev); > > + void __iomem *wdt_base; > > + > > + wdt_base = mtk_wdt->wdt_base; > > + > > + if (readl(wdt_base + WDT_MODE) & WDT_MODE_EN) > > + mtk_wdt_start(wdt_dev); > > + else > > + mtk_wdt_stop(wdt_dev); > > +} > > + > > static int mtk_wdt_restart(struct watchdog_device *wdt_dev, > > unsigned long action, void *data) > > { > > @@ -206,6 +222,8 @@ static int mtk_wdt_stop(struct watchdog_device *wdt_dev) > > reg |= WDT_MODE_KEY; > > iowrite32(reg, wdt_base + WDT_MODE); > > > > + clear_bit(WDOG_HW_RUNNING, &wdt_dev->status); > > + > > return 0; > > } > > > > @@ -225,6 +243,8 @@ static int mtk_wdt_start(struct watchdog_device *wdt_dev) > > reg |= (WDT_MODE_EN | WDT_MODE_KEY); > > iowrite32(reg, wdt_base + WDT_MODE); > > > > + set_bit(WDOG_HW_RUNNING, &wdt_dev->status); > > + > > return 0; > > } > > > > @@ -274,7 +294,7 @@ static int mtk_wdt_probe(struct platform_device *pdev) > > > > watchdog_set_drvdata(&mtk_wdt->wdt_dev, mtk_wdt); > > > > - mtk_wdt_stop(&mtk_wdt->wdt_dev); > > + mtk_wdt_init(&mtk_wdt->wdt_dev); > > > > watchdog_stop_on_reboot(&mtk_wdt->wdt_dev); > > err = devm_watchdog_register_device(dev, &mtk_wdt->wdt_dev); > > >
diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c index d6a6393..59b5061 100644 --- a/drivers/watchdog/mtk_wdt.c +++ b/drivers/watchdog/mtk_wdt.c @@ -57,6 +57,9 @@ static bool nowayout = WATCHDOG_NOWAYOUT; static unsigned int timeout; +static int mtk_wdt_start(struct watchdog_device *wdt_dev); +static int mtk_wdt_stop(struct watchdog_device *wdt_dev); + struct mtk_wdt_dev { struct watchdog_device wdt_dev; void __iomem *wdt_base; @@ -148,6 +151,19 @@ static int toprgu_register_reset_controller(struct platform_device *pdev, return ret; } +static int mtk_wdt_init(struct watchdog_device *wdt_dev) +{ + struct mtk_wdt_dev *mtk_wdt = watchdog_get_drvdata(wdt_dev); + void __iomem *wdt_base; + + wdt_base = mtk_wdt->wdt_base; + + if (readl(wdt_base + WDT_MODE) & WDT_MODE_EN) + mtk_wdt_start(wdt_dev); + else + mtk_wdt_stop(wdt_dev); +} + static int mtk_wdt_restart(struct watchdog_device *wdt_dev, unsigned long action, void *data) { @@ -206,6 +222,8 @@ static int mtk_wdt_stop(struct watchdog_device *wdt_dev) reg |= WDT_MODE_KEY; iowrite32(reg, wdt_base + WDT_MODE); + clear_bit(WDOG_HW_RUNNING, &wdt_dev->status); + return 0; } @@ -225,6 +243,8 @@ static int mtk_wdt_start(struct watchdog_device *wdt_dev) reg |= (WDT_MODE_EN | WDT_MODE_KEY); iowrite32(reg, wdt_base + WDT_MODE); + set_bit(WDOG_HW_RUNNING, &wdt_dev->status); + return 0; } @@ -274,7 +294,7 @@ static int mtk_wdt_probe(struct platform_device *pdev) watchdog_set_drvdata(&mtk_wdt->wdt_dev, mtk_wdt); - mtk_wdt_stop(&mtk_wdt->wdt_dev); + mtk_wdt_init(&mtk_wdt->wdt_dev); watchdog_stop_on_reboot(&mtk_wdt->wdt_dev); err = devm_watchdog_register_device(dev, &mtk_wdt->wdt_dev);
1. add a hw initialization function 2. enable/disable the watchdog depends on the original hw setting 3. set WDOD_HW_RUNNING in start function in order to start kicker after driver probe and clear the bit in stop function Change-Id: I25aa797f3b88288f26984455e499e599e27f09fa Signed-off-by: Freddy Hsin <freddy.hsin@mediatek.com> --- drivers/watchdog/mtk_wdt.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)