Message-ID: <e4b4fbc7-53ee-987b-7849-bf625fc2155e@traphandler.com>
Date:   Mon, 21 Feb 2022 10:13:21 +0100
From:   Jean-Jacques Hiblot <jjhiblot@...phandler.com>
To:     Guenter Roeck <linux@...ck-us.net>,
        Geert Uytterhoeven <geert@...ux-m68k.org>
CC:     <linux-renesas-soc@...r.kernel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, <linux-clk@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        <linux-watchdog@...r.kernel.org>
Subject: Re: [PATCH v2 6/6] clk: renesas: r9a06g032: Disable the watchdog
 reset sources when halting

Hi,

On 14/02/2022 16:34, Guenter Roeck wrote:
> On 2/14/22 02:45, Geert Uytterhoeven wrote:
>> Hi Jean-Jacques,
>>
>> CC watchdog people, who only received some patches.
>>
>> On Tue, Feb 8, 2022 at 7:35 PM Jean-Jacques Hiblot
>> <jjhiblot@...phandler.com> wrote:
>>> The watchdog reset sources must be disabled when the system is halted.
>>> Otherwise the watchdogs will trigger a reset if they have been armed.
>>>
>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@...phandler.com>
>>
>> Thanks for your patch!
>>
>> [inserting changelog]
>>
>> | Changes v1 -> v2:
>> | * Modified the clock driver to not enable the watchdog reset sources.
>> |   On other renesas platforms, those bits are by the bootloader. The
>> |   watchdog reset sources are still disabled when the platform is 
>> halted
>> |   to prevent a watchdog reset.
>>
>> I still have my doubts about this part. So on halt, you override the
>> policy configured by the boot loader, which means the watchdog is no
>> longer triggered on halt.
>>
>>> From a system perspective, the system can be in five states:
>>    1. Running,
>>    2. Crashed/lock-ed up,
>>    3. Halt,
>>    4. Reboot,
>>    5. Poweroff.
>>
>> Now, from a policy perspective, what is the difference between a
>> system that crashes or locks up, and a system that halts?
>> I.e. should the system reboot on halt, or not?
>>
>> I think halting a system where the watchdog has been activated makes
>> no sense, and the user either wants to explicitly reboot the system, or
>> power it off, but never halt it.  So I think this patch is not needed.

I don't see halting the machine as a must-have feature either, but it 
seemed to me

that there could be other people relying on it. I'll remove this patch 
from the series.

>>
>> Watchdog people: what is your opinion?
>
> In my understanding the shutdown code is always executed, ie also for
> restarts and poweroff. Disabling the watchdog in that situation is not
> always desirable, though sometimes necessary depending on the hardware.
> Disabling it through the backdoor (instead of calling 
> watchdog_stop_on_reboot)
> seems odd, though.


Unfortunately, in this case it is not possible to stop the watchdog once 
started.

The only way to halt the machine is to disable the watchdog reset source.


JJ


>
> Guenter
>
>> Thanks!
>>
>>> --- a/drivers/clk/renesas/r9a06g032-clocks.c
>>> +++ b/drivers/clk/renesas/r9a06g032-clocks.c
>>> @@ -129,6 +129,11 @@ enum { K_GATE = 0, K_FFC, K_DIV, K_BITSEL, 
>>> K_DUALGATE };
>>>
>>>   #define R9A06G032_CLOCK_COUNT (R9A06G032_UART_GROUP_34567 + 1)
>>>
>>> +#define R9A06G032_SYSCTRL_REG_RSTEN            0x120
>>> +#define WDA7RST1       BIT(2)
>>> +#define WDA7RST0       BIT(1)
>>> +#define MRESET         BIT(0)
>>> +
>>>   static const struct r9a06g032_clkdesc r9a06g032_clocks[] = {
>>>          D_ROOT(CLKOUT, "clkout", 25, 1),
>>>          D_ROOT(CLK_PLL_USB, "clk_pll_usb", 12, 10),
>>> @@ -893,6 +898,19 @@ static void 
>>> r9a06g032_clocks_del_clk_provider(void *data)
>>>          of_clk_del_provider(data);
>>>   }
>>>
>>> +static void r9a06g032_reset_sources(struct r9a06g032_priv *clocks,
>>> +                       uint32_t mask, uint32_t value)
>>> +{
>>> +       uint32_t rsten;
>>> +       unsigned long flags;
>>> +
>>> +       spin_lock_irqsave(&clocks->lock, flags);
>>> +       rsten = readl(clocks->reg);
>>> +       rsten = (rsten & ~mask) | (value & mask);
>>> +       writel(rsten, clocks->reg + R9A06G032_SYSCTRL_REG_RSTEN);
>>> +       spin_unlock_irqrestore(&clocks->lock, flags);
>>> +}
>>> +
>>>   static int __init r9a06g032_clocks_probe(struct platform_device 
>>> *pdev)
>>>   {
>>>          struct device *dev = &pdev->dev;
>>> @@ -910,6 +928,8 @@ static int __init r9a06g032_clocks_probe(struct 
>>> platform_device *pdev)
>>>          if (!clocks || !clks)
>>>                  return -ENOMEM;
>>>
>>> +       platform_set_drvdata(pdev, clocks);
>>> +
>>>          spin_lock_init(&clocks->lock);
>>>
>>>          clocks->data.clks = clks;
>>> @@ -963,9 +983,18 @@ static int __init r9a06g032_clocks_probe(struct 
>>> platform_device *pdev)
>>>          if (error)
>>>                  return error;
>>>
>>> +
>>>          return r9a06g032_add_clk_domain(dev);
>>>   }
>>>
>>> +static void r9a06g032_clocks_shutdown(struct platform_device *pdev)
>>> +{
>>> +       struct r9a06g032_priv *clocks = platform_get_drvdata(pdev);
>>> +
>>> +       /* Disable the watchdog reset sources */
>>> +       r9a06g032_reset_sources(clocks, WDA7RST0 | WDA7RST1, 0);
>>> +}
>>> +
>>>   static const struct of_device_id r9a06g032_match[] = {
>>>          { .compatible = "renesas,r9a06g032-sysctrl" },
>>>          { }
>>> @@ -976,6 +1005,7 @@ static struct platform_driver 
>>> r9a06g032_clock_driver = {
>>>                  .name   = "renesas,r9a06g032-sysctrl",
>>>                  .of_match_table = r9a06g032_match,
>>>          },
>>> +       .shutdown = r9a06g032_clocks_shutdown,
>>>   };
>>>
>>>   static int __init r9a06g032_clocks_init(void)
>>
>> Gr{oetje,eeting}s,
>>
>>                          Geert
>>
>> -- 
>> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
>> geert@...ux-m68k.org
>>
>> In personal conversations with technical people, I call myself a 
>> hacker. But
>> when I'm talking to journalists I just say "programmer" or something 
>> like that.
>>                                  -- Linus Torvalds
>

Powered by blists - more mailing lists