Re: [PATCH 2/2] watchdog: mtk_wdt: Add support for MT6735 WDT

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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.







[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux