Date:   Tue, 8 Mar 2022 08:21:01 +0000
From:   <Claudiu.Beznea@...rochip.com>
To:     <clement.leger@...tlin.com>, <linux@...linux.org.uk>,
        <Nicolas.Ferre@...rochip.com>, <alexandre.belloni@...tlin.com>,
        <Ludovic.Desroches@...rochip.com>
CC:     <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v2 2/2] ARM: at91: pm: add support for sama5d2 secure
 suspend

Hi Clément,

On 07.03.2022 12:15, Clément Léger wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> When running with OP-TEE, the suspend control is handled securely.
> Suspend can be entered using PSCI support. Since the sama5d2 supports
> multiple suspend modes, add a new CONFIG_ATMEL_SECURE_PM which will
> send a SMC call to select the suspend mode at init time.
> 
> "atmel.pm_modes" boot argument is still supported for compatibility
> purposes but the standby value is actually ignored since PSCI suspend
> is used and it only support one mode (suspend).
> 
> Signed-off-by: Clément Léger <clement.leger@...tlin.com>
> ---
>  arch/arm/mach-at91/Kconfig      | 12 ++++++++++-
>  arch/arm/mach-at91/pm.c         | 38 +++++++++++++++++++++++++++++++++
>  arch/arm/mach-at91/sam_secure.h |  4 ++++
>  3 files changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
> index 02f6b108fd5d..5a6ca38d6303 100644
> --- a/arch/arm/mach-at91/Kconfig
> +++ b/arch/arm/mach-at91/Kconfig
> @@ -208,7 +208,17 @@ config SOC_SAMA5
>         select SRAM if PM
> 
>  config ATMEL_PM
> -       bool
> +       bool "Atmel PM support"
> +
> +config ATMEL_SECURE_PM
> +       bool "Atmel Secure PM support"
> +       depends on SOC_SAMA5D2 && ATMEL_PM
> +       select ARM_PSCI
> +       help
> +         When running under a TEE, the suspend mode must be requested to be set
> +         at TEE level. When enable, this option will use secure monitor calls
> +         to set the suspend level. PSCI is then used to enter suspend.
> +         NOTE: This support is mutually exclusive with CONFIG_ATMEL_PM

This note may be a bit confusing as ATMEL_SECURE_PM depends on CONFIG_ATMEL_PM.

> 
>  config SOC_SAMA7
>         bool
> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
> index dd6f4ce3f766..e40515691540 100644
> --- a/arch/arm/mach-at91/pm.c
> +++ b/arch/arm/mach-at91/pm.c
> @@ -27,6 +27,7 @@
> 
>  #include "generic.h"
>  #include "pm.h"
> +#include "sam_secure.h"
> 
>  #define BACKUP_DDR_PHY_CALIBRATION     (9)
> 
> @@ -856,6 +857,35 @@ static int __init at91_pm_backup_init(void)
>         return ret;
>  }
> 
> +static void at91_pm_secure_init(void)
> +{
> +       int suspend_mode;
> +       struct arm_smccc_res res;
> +
> +       suspend_mode = soc_pm.data.suspend_mode;
> +
> +       res = sam_smccc_call(SAMA5_SMC_SIP_SET_SUSPEND_MODE,
> +                            suspend_mode, 0);
> +       if (res.a0 == 0) {
> +               pr_info("AT91: Secure PM: suspend mode set to %s\n",
> +                       pm_modes[suspend_mode].pattern);
> +               return;
> +       }
> +
> +       pr_warn("AT91: Secure PM: %s mode not supported !\n",
> +               pm_modes[suspend_mode].pattern);
> +
> +       res = sam_smccc_call(SAMA5_SMC_SIP_GET_SUSPEND_MODE, 0, 0);
> +       if (res.a0 == 0) {
> +               pr_warn("AT91: Secure PM: failed to get default mode\n");
> +               return;
> +       }
> +
> +       pr_info("AT91: Secure PM: using default suspend mode %s\n",
> +               pm_modes[suspend_mode].pattern);
> +
> +       soc_pm.data.suspend_mode = res.a1;
> +}
>  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?
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?

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

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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Powered by blists - more mailing lists