Message-ID: <daacbe14-b6f8-4de2-8bb1-b3db62245a66@collabora.com>
Date: Mon, 21 Oct 2024 14:35:30 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Guenter Roeck <linux@...ck-us.net>, yassine.oudjana@...il.com
Cc: 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

Il 17/10/24 17:39, Guenter Roeck ha scritto:
> On 10/17/24 08:16, AngeloGioacchino Del Regno 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!
>>
>>
>> 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.
>>
>> 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.
>>
> 
> That won't work because interrupts are likely disabled when the reset callback
> executes. The reset handler must not depend on an interrupt. It has to be
> self-contained: It has to configure the hardware to issue a reset.
> On some systems, that is done by setting the watchdog timeout to a really low value
> and enabling it. Others have a special configuration in the watchdog register set
> which triggers a reset immediately. If the hardware supports pretimeout, that would
> have to be disabled because the idea is to trigger a reset signal, not an interrupt.
> 
> To repeat, setting up the system and then waiting for an interrupt to do something
> defeats the purpose of the reset handler, which is to issue a reset signal.
> Somehow. If that can be done from an interrupt handler, it can and should be done
> immediately instead.
> 
>> 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!
>>
> 
> All that should be configured in the reset handler. It has to disable interrupts
> even if there is interrupt support because that is not what is wanted at this point.
> 

Ack.
After re-reading this and thinking about it for a bit - you're definitely right.

Let's go for the setup in the reset handler then.

Though, I think that a v2 is still needed here - one patch to fix the reset handler
(at this point, with a Fixes tag for backporting..!), and one to add support for
the MT6735 resets.

Cheers,
Angelo

Powered by blists - more mailing lists