Message-Id: <SKIJLS.S7FSW9OJT5K82@gmail.com>
Date: Fri, 18 Oct 2024 10:10:04 +0300
From: Yassine Oudjana <yassine.oudjana@...il.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: Guenter Roeck <linux@...ck-us.net>, Wim Van Sebroeck
	<wim@...ux-watchdog.org>, Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Matthias Brugger
	<matthias.bgg@...il.com>, Philipp Zabel <p.zabel@...gutronix.de>,
	Yassine Oudjana <y.oudjana@...tonmail.com>, linux-watchdog@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH 2/2] watchdog: mtk_wdt: Add support for MT6735 WDT


On Thu, Oct 17 2024 at 17:16:54 +02:00:00, AngeloGioacchino Del Regno 
<angelogioacchino.delregno@...labora.com> wrote:
> Il 17/10/24 16:09, Guenter Roeck ha scritto:
>> On 10/16/24 23:43, yassine.oudjana@...il.com wrote:
>> ...
>>>>> 
>>>>> 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.
>>> 
>> 
>> It is more than that. There is no limitation in the watchdog API 
>> that says
>> "you must only use the watchdog kernel driver to reset the system if 
>> the
>> watchdog has been activated from userspace". Such a limitation would 
>> be
>> completely arbitrary and not make any sense. It is perfectly fine to 
>> enable
>> the watchdog from the restart callback if needed. Actually, all 
>> restart
>> handlers in watchdog drivers have to do that if they indeed use a 
>> watchdog
>> to reset the system.
>> 
>> Actually, I am not entirely sure I understand what we are arguing 
>> about.
>> 
> 
> Guenter:
> We're arguing about bad configuration and lots of misunderstanding.
> 
> Regarding WDT_MODE_EXRST_EN: when enabled, it enables an external 
> output
> reset signal - meaning that it's going to flip the state of a GPIO to 
> active
> (high in Yassine's case - as that's configured through WDT_MODE 
> BIT(1) and
> his 0x5c means that it's flipped on), signaling to another chip 
> (usually,
> the PMIC...!) that we want to reset the system.
> 
> Explaining what Yassine is doing with this commit: he is flipping the 
> IRQ_EN
> bit [BIT(5)] in WDT_MODE.
> 
> When bit 5 *is set*, the watchdog bark event will only raise an 
> interrupt and
> will not reset the system (that's left to be done to an interrupt 
> handler in
> the driver).
> 
> When bit 5 *is NOT set*, the watchdog bark event will trigger a CPU 
> reset.
> 
> Now, my confusion came from the fact that he's trying to fix a 
> watchdog bark
> event so that it triggers system reset, but I didn't understand the 
> actual
> reason why he wants to do that - which is powering off the system!

I'm currently aiming for reboot, not poweroff. There was some point 
during development where holding the power button wasn't enough to 
force restart and I'm still not sure why that was happening but it's 
working now. That's why I was removing the battery to power it off then 
turn it on again with the power button.

> 
> 
> Yassine:
> 
> You don't *have to* rely on the watchdog to reset the system, and if 
> you use
> only that - especially on a smartphone - I'm mostly sure that you'll 
> get
> power leakage.

I did notice that when I tried poweroff. It didn't fully power off and 
eventually drained the battery. But again I'm not looking into fixing 
poweroff for the time being, I'm focusing on reboot. I'll probably ask 
you again about it when I look into fixing poweroff since you seem to 
know something about the matter.

> 
> Before you read the following - please note that this is platform 
> dependent
> so, take this with a grain of salt: it is the PMIC that should get 
> configured
> to take your system down! I have a hunch that this works for you only 
> because
> the platform will reboot, and then the bootloader will decide to turn 
> off the
> system for you by default (that, unless you send a warm reboot 
> indication).
> 
> That flow looks more like a hack than a solution for an actual 
> problem.
> 
> 
> Now - whether you want to fix your platform or not, this is out of 
> the scope
> of this commit, which is - in the end - still fixing something that 
> is wrong.
> 
> Effectively, as Guenter said, if the watchdog is never started, the 
> restart
> function is not going to reboot the system, so yes this problem needs 
> to be
> fixed.
> 
> There are two problems in this driver that can be solved with:
>  1. Disable IRQ generation when *no irq* is found in DT; and
>  2. Implement support for reboot in mtk_wdt_isr() by reading the 
> WDT_STA
>     register and by then taking appropriate actions.
> 
> Of course my preference would be N.2 because:
>  - The pretimeout way is already supported in the driver, and if you 
> specify
>    a pretimeout, then the watchdog will never trigger SYSRST->XRST: 
> this
>    is actually a bug (IMO!!), as declaring an IRQ in DT means losing 
> reset (!);
>  - The WDT_STA register tells you more than just "SW/HW watchdog 
> reset asserted"
>    and that can be extended in the future to support more than that.
> 
> However, I recognize that this may be too much work for you, so, if 
> there's no
> way for you to properly add support for N.2 - I can chime in.
> 
> As for N.1, the solution is simple: check if 
> platform_get_irq_optional fails
> and - if it does - force unsetting (WDT_MODE_IRQ_EN | 
> WDT_MODE_DUAL_EN) in
> WDT_MODE, if and only if WDT_EN is not set.. but that, in the probe 
> function!

I still honestly don't see the point in either solution. N.1 sounds 
like a hack; the interrupt should be described in DT even if we don't 
want to use it since DT is ultimately hardware description and the 
interrupt line exists whether we are using it or not. N.2 is 
unnecessarily complicating things. I mean why should we make the 
watchdog issue an interrupt then in the interrupt handler perform the 
reset? When someone calls the restart function they expect an immediate 
reset. As Guenter said we should configure the hardware to issue reset 
in the reset handler, and I believe that's what I did originally.

Powered by blists - more mailing lists