[PATCH] ARM: stm32: Add IWDG handling into PSCI suspend code

[Marek Vasut]

On 5/25/23 13:57, Patrick DELAUNAY wrote:
> Hi

Hello Patrick,

>>>>>> +        /* Ping IWDG2 and ACK pretimer IRQ */
>>>>>> +        if (iwdg2_wake) {
>>>>>> +            writel(IWDG_KR_RELOAD_KEY, STM32_IWDG2_BASE + IWDG_KR);
>>>>>> +            writel(IWDG_EWCR_EWIC, STM32_IWDG2_BASE + IWDG_EWCR);
>>>>>> +        }
>>>>>> +    }
>>>>>>         writel(0x3, STM32_RCC_BASE + RCC_MP_SREQCLRR);
>>>>>>       ddr_sw_self_refresh_exit();
>>>>>
>>>>>
>>>>> Reviewed-by: Patrice Chotard <patrice.chotard at foss.st.com>
>>>>
>>>> I feel like maybe I need to whack the IWDG also BEFORE entering 
>>>> suspend, expect a V2 shortly.
>>>>
>>>> Do you think this IWDG approach is fine to keep the system from 
>>>> rebooting in suspend ? Or do you see any better option for the MP1 ?
>>>
>>>
>>> I think it is fine if you want to have watchdog running in STANDBY 
>>> even this patch force a wakeup.
>>>
>>> but if IWDG 1 or 2 should be not running in STANDBY (but also for 
>>> STOP) for your product,
>>
>> Why ?
>>
>> I want the watchdog to monitor that the platform is still OK, even in 
>> suspend. Also, as far as I can tell, once the IWDG is started, it 
>> cannot be stopped, right ?
> 
> 
> without change OTP (so with the default behavior)
> 
> => yes the watchdog IWDG is running without way to stop it.

ACK

>>> it can be managed directly by the hardware with OTP, to freeze watchdog.
>>>
>>> => it is a decision for each product of the desired behavior of the 
>>> IWDG in low power mode.
>>>
>>>
>>> See Ref Manual: 48.3 IWDG implementation
>>>
>>> Table 332. STM32MP157x IWDG features
>>>
>>> Option bytes to control the activity in Standby mode (6)
>>>
>>> 6. Controlled via option bytes OTP_IWDG1_FZ_STANDBY and 
>>> OTP_IWDG2_FZ_STANDBY, respectively, for IWDG1 and IWDG2.
>>>
>>> and in Table17 => OTP 18 = HW2 bit 5 to 8
>>
>> I saw the OTP fuses, but I don't want to blow them. Or is that really 
>> the suggestion ST would provide for suspend/resume, blow the fuses ?
>>
>> (I wonder, why isn't the logic of those fuses inverted then, i.e. by 
>> default stop the watchdog in suspend AND blow fuses to keep it running 
>> in suspend?)
> 
> 
> it is a SoC design choice => the default behavior is watchdog is running 
> in standby and stop, so you need to reload it
> 
> That imply the limit of the duration of max duration of standby to avoid 
> reset....
> 
> and the usage of IWDG pre-timeout in this case it a good place to wakeup 
> the SoC
> 
> and add sanity checks (check the system is still running) before to 
> reload it.

OK