[RFC PATCH v2 2/4] psci: add features/reset2 support

[Patrick DELAUNAY]

Hi Igor,

On 3/30/21 11:16 PM, Igor Opaniuk wrote:
> From: Igor Opaniuk <igor.opaniuk at foundries.io>
>
> Adds support for:
> * PSCI_FEATURES, which was introduced in PSCI 1.0. This provides API
> that allows discovering whether a specific PSCI function is implemented
> and its features.
> * SYSTEM_RESET2, which was introduced in PSCI 1.1, which extends existing
> SYSTEM_RESET. It provides support for vendor-specific resets, providing
> reset_type as an additional param.
>
> For additional details visit [1].
>
> Implementations of some functions were borrowed from Linux PSCI driver
> code [2].
>
> [1] https://developer.arm.com/documentation/den0022/latest/
> [2] drivers/firmware/psci/psci.c
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk at foundries.io>
> ---
>
> (no changes since v1)
>
>   drivers/firmware/psci.c | 70 +++++++++++++++++++++++++++++++++++++++++
>   include/linux/psci.h    |  3 ++
>   2 files changed, 73 insertions(+)
>
> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> index 68953cc4f4..8416cd445f 100644
> --- a/drivers/firmware/psci.c
> +++ b/drivers/firmware/psci.c
> @@ -13,6 +13,7 @@
>   #include <log.h>
>   #include <dm/lists.h>
>   #include <efi_loader.h>
> +#include <sysreset.h>
>   #include <linux/delay.h>
>   #include <linux/libfdt.h>
>   #include <linux/arm-smccc.h>
> @@ -26,12 +27,26 @@
>   #define PSCI_METHOD_HVC 1
>   #define PSCI_METHOD_SMC 2
>   
> +/*
> + * While a 64-bit OS can make calls with SMC32 calling conventions, for some
> + * calls it is necessary to use SMC64 to pass or return 64-bit values.
> + * For such calls PSCI_FN_NATIVE(version, name) will choose the appropriate
> + * (native-width) function ID.
> + */
> +#if defined(CONFIG_ARM64)
> +#define PSCI_FN_NATIVE(version, name)	PSCI_##version##_FN64_##name
> +#else
> +#define PSCI_FN_NATIVE(version, name)	PSCI_##version##_FN_##name
> +#endif
> +
>   #if CONFIG_IS_ENABLED(EFI_LOADER)
>   int __efi_runtime_data psci_method;
>   #else
>   int psci_method __attribute__ ((section(".data")));
>   #endif
>   
> +static bool reset2_supported;
> +

I think this global variable here can cause issue before relocation and 
for  UEFI ?

=> solved for psci_method...


I think it should be moved in psci private data if it is needed (see after)


>   unsigned long __efi_runtime invoke_psci_fn
>   		(unsigned long function_id, unsigned long arg0,
>   		 unsigned long arg1, unsigned long arg2)
> @@ -53,6 +68,27 @@ unsigned long __efi_runtime invoke_psci_fn
>   	return res.a0;
>   }
>   
> +static int psci_features(u32 psci_func_id)
> +{
> +	return invoke_psci_fn(PSCI_1_0_FN_PSCI_FEATURES,
> +			      psci_func_id, 0, 0);
> +}
> +
> +static u32 psci_0_2_get_version(void)
> +{
> +	return invoke_psci_fn(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
> +}
> +
> +static void psci_init_system_reset2(void)
> +{
> +	int ret;
> +
> +	ret = psci_features(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2));
> +
> +	if (ret != PSCI_RET_NOT_SUPPORTED)
> +		reset2_supported = true;
> +}
> +
>   static int psci_bind(struct udevice *dev)
>   {
>   	/* No SYSTEM_RESET support for PSCI 0.1 */
> @@ -73,6 +109,7 @@ static int psci_bind(struct udevice *dev)
>   static int psci_probe(struct udevice *dev)
>   {
>   	const char *method;
> +	u32 ver;
>   
>   #if defined(CONFIG_ARM64)
>   	if (current_el() == 3)
> @@ -94,6 +131,16 @@ static int psci_probe(struct udevice *dev)
>   		return -EINVAL;
>   	}

> +	ver = psci_0_2_get_version();
> +

priv->ver = psci_0_2_get_version();


> +	pr_debug("PSCIv%d.%d detected in firmware.\n",
> +		 PSCI_VERSION_MAJOR(ver),
> +		 PSCI_VERSION_MINOR(ver));
> +
> +	if (PSCI_VERSION_MAJOR(ver) >= 1) {
> +		psci_init_system_reset2();

in other functions, the version check is based on the compatible..

static int psci_bind(struct udevice *dev)
{
     /* No SYSTEM_RESET support for PSCI 0.1 */
     if (device_is_compatible(dev, "arm,psci-0.2") ||
         device_is_compatible(dev, "arm,psci-1.0")) {

it should be updated also ?

.....

priv->reset2 = psci_init_system_reset2();

> +	}
> +
>   	return 0;
>   }
>   
> @@ -141,6 +188,29 @@ void reset_misc(void)
>   }
>   #endif /* CONFIG_PSCI_RESET */
>   
> +void psci_sys_reset(u32 type)
> +{
> +	do_psci_probe();
> +

dev = do_psci_probe();

priv = get_priv(dev);


Then private data are available (it is safer than global)


> +	if (type == SYSRESET_WARM && reset2_supported) {
> +		/*
> +		 * reset_type[31] = 0 (architectural)
> +		 * reset_type[30:0] = 0 (SYSTEM_WARM_RESET)
> +		 * cookie = 0 (ignored by the implementation)
> +		 */
> +		invoke_psci_fn(PSCI_FN_NATIVE(1_1, SYSTEM_RESET2), 0, 0, 0);
> +	} else {
> +		invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
> +	}
> +}

These almost same function already exist in 
./drivers/sysreset/sysreset_psci.c

=> handle in do_reset() / do_poweroff() : drivers/sysreset/sysreset-uclass.c

bind by:

drivers/firmware/psci.c:64:        ret = device_bind_driver(dev, 
"psci-sysreset", "psci-sysreset",


I think your modication should be moved in this part.


> +
> +void psci_sys_poweroff(void)
> +{
> +	do_psci_probe();
> +
> +	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> +}
> +

>   #ifdef CONFIG_CMD_POWEROFF
>   int do_poweroff(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   {
> diff --git a/include/linux/psci.h b/include/linux/psci.h
> index 38edde3137..c78c1079a8 100644
> --- a/include/linux/psci.h
> +++ b/include/linux/psci.h
> @@ -118,6 +118,9 @@
>   #ifdef CONFIG_ARM_PSCI_FW
>   unsigned long invoke_psci_fn(unsigned long a0, unsigned long a1,
>   			     unsigned long a2, unsigned long a3);
> +void psci_sys_reset(u32 type);
> +void psci_sys_poweroff(void);
> +
>   #else
>   static inline unsigned long invoke_psci_fn(unsigned long a0, unsigned long a1,
>   					   unsigned long a2, unsigned long a3)


Regards


Patrick