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

[Patrick DELAUNAY]

Hi

On 5/12/23 15:55, Marek Vasut wrote:
> On 5/12/23 15:43, Patrick DELAUNAY wrote:
>> Hi,
>
> Hi,
>
>>>>> +        /* 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.


>
>> 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.


Patrick