Re: [PATCH v2 2/2] ARM: at91: pm: add support for sama5d2 secure suspend

[Clément Léger]

Le Tue, 8 Mar 2022 08:21:01 +0000,
<Claudiu.Beznea@xxxxxxxxxxxxx> a écrit :
[...]

> >  static const struct of_device_id atmel_shdwc_ids[] = {
> >         { .compatible = "atmel,sama5d2-shdwc" },
> >         { .compatible = "microchip,sam9x60-shdwc" },
> > @@ -1188,6 +1218,11 @@ void __init sama5d2_pm_init(void)
> >         if (!IS_ENABLED(CONFIG_SOC_SAMA5D2))
> >                 return;
> > 
> > +       if (IS_ENABLED(CONFIG_ATMEL_SECURE_PM)) {
> > +               at91_pm_secure_init();
> > +               return;
> > +       }
> > +
> >         at91_pm_modes_validate(modes, ARRAY_SIZE(modes));
> >         at91_pm_modes_init(iomaps, ARRAY_SIZE(iomaps));
> >         ret = at91_dt_ramc(false);
> > @@ -1262,6 +1297,9 @@ static int __init at91_pm_modes_select(char *str)
> >         soc_pm.data.standby_mode = standby;
> >         soc_pm.data.suspend_mode = suspend;
> > 
> > +       if (IS_ENABLED(CONFIG_ATMEL_SECURE_PM))
> > +               pr_warn("AT91: Secure PM: ignoring standby mode\n");  
> 
> What happens with soc_pm.data.standby_mode? Can Linux still suspend to it?

Unfortunately, no. PSCI suspend support only allows one mode
(SUSPEND_MEM, cf psci_suspend_ops) which should be the deepest suspend
mode supported by the platform. Since the sama5 family support more
than one suspend mode, we added this mechanism to keep the existing
support for atmel,pm_modes support.

We could potentially add a suspend support for sama5d2 that call the
SMC to set the suspend mode and then enters PSCI system suspend by
calling psci_system_suspend_enter(). But I'm not sure it is the idea
behind the PSCI system suspend call.

> If that's the case then the proper data for standby mode is not initialized
> as sama5d2_pm_init() returns if CONFIG_ATMEL_SECURE_PM is enabled, thus
> system may hang in such a case. Is this intended?

Since the platform_suspend_ops won't be registered, there should be no
call to enter sama5d2 suspend (either mem or standby) from this code.

> 
> Otherwise, maybe this pr_warn() could be moved in sama5d2_pm_init() as
> sama5d2 is the only user at the moment.

Ok, I will move the print.

Thanks,

Clément

> 
> Thank you,
> Claudiu Beznea
> 
> > +
> >         return 0;
> >  }
> >  early_param("atmel.pm_modes", at91_pm_modes_select);
> > diff --git a/arch/arm/mach-at91/sam_secure.h b/arch/arm/mach-at91/sam_secure.h
> > index 360036672f52..1e7d8b20ba1e 100644
> > --- a/arch/arm/mach-at91/sam_secure.h
> > +++ b/arch/arm/mach-at91/sam_secure.h
> > @@ -8,6 +8,10 @@
> > 
> >  #include <linux/arm-smccc.h>
> > 
> > +/* Secure Monitor mode APIs */
> > +#define SAMA5_SMC_SIP_SET_SUSPEND_MODE 0x400
> > +#define SAMA5_SMC_SIP_GET_SUSPEND_MODE 0x401
> > +
> >  void __init sam_secure_init(void);
> >  struct arm_smccc_res sam_smccc_call(u32 fn, u32 arg0, u32 arg1);
> > 
> > --
> > 2.34.1
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel  
> 



-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com