[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <WOMHLS.HLNVQWWVER5T1@gmail.com>
Date: Thu, 17 Oct 2024 09:43:44 +0300
From: yassine.oudjana@...il.com
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: Wim Van Sebroeck <wim@...ux-watchdog.org>, Guenter Roeck
<linux@...ck-us.net>, 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 Wed, Oct 16 2024 at 11:56:31 +02:00:00, AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com> 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@...tonmail.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@...tonmail.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.
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.
Powered by blists - more mailing lists