Messages in this thread | | | Date | Wed, 8 Mar 2023 03:47:10 -0800 | From | Guenter Roeck <> | Subject | Re: [PATCH 2/2] watchdog: mtk_wdt: Add support for MT6735 WDT |
| |
On 3/2/23 04:40, Yassine Oudjana wrote: > From: Yassine Oudjana <y.oudjana@protonmail.com> > > Add support for the watchdog timer/top reset generation unit found on MT6735. > Disable WDT_MODE_IRQ_EN in mtk_wdt_restart in order to make TOPRGU assert > the SYSRST pin instead of issuing an IRQ. This change may be needed in other > SoCs as well. >
This is two functional changes in one patch. Also, the "may be needed" is vague. It is either needed or it isn't.
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com> > --- > drivers/watchdog/mtk_wdt.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c > index a9c437598e7e..5a7a7b2b3727 100644 > --- a/drivers/watchdog/mtk_wdt.c > +++ b/drivers/watchdog/mtk_wdt.c > @@ -10,6 +10,7 @@ > */ > > #include <dt-bindings/reset/mt2712-resets.h> > +#include <dt-bindings/reset/mediatek,mt6735-wdt.h> > #include <dt-bindings/reset/mediatek,mt6795-resets.h> > #include <dt-bindings/reset/mt7986-resets.h> > #include <dt-bindings/reset/mt8183-resets.h> > @@ -82,6 +83,10 @@ static const struct mtk_wdt_data mt2712_data = { > .toprgu_sw_rst_num = MT2712_TOPRGU_SW_RST_NUM, > }; > > +static const struct mtk_wdt_data mt6735_data = { > + .toprgu_sw_rst_num = MT6735_TOPRGU_RST_NUM, > +}; > + > static const struct mtk_wdt_data mt6795_data = { > .toprgu_sw_rst_num = MT6795_TOPRGU_SW_RST_NUM, > }; > @@ -187,9 +192,15 @@ static int mtk_wdt_restart(struct watchdog_device *wdt_dev, > { > struct mtk_wdt_dev *mtk_wdt = watchdog_get_drvdata(wdt_dev); > void __iomem *wdt_base; > + u32 reg; > > wdt_base = mtk_wdt->wdt_base; > > + /* Enable reset in order to issue a system reset instead of an IRQ */ > + reg = readl(wdt_base + WDT_MODE); > + reg &= ~WDT_MODE_IRQ_EN; > + writel(reg | WDT_MODE_KEY, wdt_base + WDT_MODE); > +
This is at the very least misleading. It appears to confuse "reset" (which is triggered by writing a specific value into the WDT_SWRST register) with the action to be taken when the watchdog triggers. The code below does not trigger the watchdog; it is supposed to trigger a soft reset, which should be independent of the above.
So this needs more explanation than just "issue a system reset instead of an IRQ", because that is presumably not supposed to be what is happening when writing into the WDT_SWRST register.
The above also doesn't explain what is supposed to happen if WDT_MODE_EXRST_EN is not set, adding more to the confusion.
Guenter
> while (1) { > writel(WDT_SWRST_KEY, wdt_base + WDT_SWRST); > mdelay(5); > @@ -443,6 +454,7 @@ static int mtk_wdt_resume(struct device *dev) > static const struct of_device_id mtk_wdt_dt_ids[] = { > { .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data }, > { .compatible = "mediatek,mt6589-wdt" }, > + { .compatible = "mediatek,mt6735-wdt", .data = &mt6735_data }, > { .compatible = "mediatek,mt6795-wdt", .data = &mt6795_data }, > { .compatible = "mediatek,mt7986-wdt", .data = &mt7986_data }, > { .compatible = "mediatek,mt8183-wdt", .data = &mt8183_data },
| |