On Wed, Oct 16 2024 at 11:56:31 +02:00:00, AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
Il 16/10/24 11:26, Yassine Oudjana ha scritto:
On 02/03/2023 6:15 pm, AngeloGioacchino Del Regno wrote:
Il 02/03/23 13:40, Yassine Oudjana ha scritto:
From: Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxx>
---
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 :-)
Ok so mtk_wdt_start is never called.
mtk_wdt_init() says
if (readl(wdt_base + WDT_MODE) & WDT_MODE_EN) {
set_bit(WDOG_HW_RUNNING, &wdt_dev->status);
mtk_wdt_set_timeout(wdt_dev, wdt_dev->timeout);
}
Your bootloader starts the watchdog. This driver will set
WDOG_HW_RUNNING and
will hence prevent calling the .start() callback - that's why.
It doesn't.
WDT_MODE reads 0x5c in mtk_wdt_init if you want to see exactly how the
bootloader is configuring it.
I'm still not quite sure how the watchdog works but it seems to me
like it's supposed to be started from userspace.
No, it's not meant to be just only used in userspace.
I also see some drivers calling it in probe.
Say I don't want to use the watchdog (which I don't, all I need from
TOPRGU is the resets, I don't care about the watchdog). Not
starting the watchdog means I can't reset the system because all
mtk_wdt_restart will do is make TOPRGU send me an IRQ that I have
no use for.
If you don't want to use the watchdog, then you don't need to care
about bark
interrupts and you don't need any mtk_wdt_restart() functionality at
all :-)
I need mtk_wdt_restart to restart my system. I shouldn't need to take
off my phone's back cover and remove the battery every time :)
I think what Guenter said makes sense. We should make sure the watchdog
is started when calling mtk_wdt_restart or at least configured in such
a way that we are sure it will issue a system reset.
