Message ID | 20230302124015.75546-3-y.oudjana@protonmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | MediaTek MT6735 TOPRGU/WDT support | expand |
Il 02/03/23 13:40, Yassine Oudjana ha scritto: > 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. > > 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 unnecessary and already done in mtk_wdt_start(). If you think you *require* this snippet, you most likely misconfigured the devicetree node for your device :-) Please remove this snippet. Regards, Angelo
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 },
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); + 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 },