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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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@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 :-)
> 
> 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.

> 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 :-)

So, no, that's not your case here.

> According to the datasheet, setting WDT_MODE_IRQ_EN 
> configures TOPRGU to send an IRQ on system reset event (either watchdog timeout or 
> software watchdog reset) while clearing it makes it actually issue a system reset. 
> What can I do in this case?

I think that we can try with something like...

static void 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;

	/*
	 * Even though the watchdog may be running, avoid setting the
	 * WDOG_HW_RUNNING bit: that will ensure that we call the start
	 * callback to setup the hardware with the desired features
	 */
	if (readl(wdt_base + WDT_MODE) & WDT_MODE_EN)
		mtk_wdt_set_timeout(wdt_dev, wdt_dev->timeout);
}


That should work... and perhaps a better idea would be to just call
mtk_wdt_start(), if WDT_MODE_EN at boot... so...

Okay, I'll stop blathering. You probably got the point anyway :-)

Cheers,
Angelo