[PATCH] firmware/psci: Add debugfs support to ease debugging

[PATCH] firmware/psci: Add debugfs support to ease debugging

[Dmitry Baryshkov]

To ease debugging of PSCI supported features, add debugfs file called
'psci' describing PSCI and SMC CC versions, enabled features and
options.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/firmware/psci/psci.c | 112 ++++++++++++++++++++++++++++++++++-
 include/uapi/linux/psci.h    |   9 +++
 2 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index b907768eea01..6595cc964635 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -9,6 +9,7 @@
 #include <linux/acpi.h>
 #include <linux/arm-smccc.h>
 #include <linux/cpuidle.h>
+#include <linux/debugfs.h>
 #include <linux/errno.h>
 #include <linux/linkage.h>
 #include <linux/of.h>
@@ -324,12 +325,121 @@ static void psci_sys_poweroff(void)
 	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
 }
 
-static int __init psci_features(u32 psci_func_id)
+static int psci_features(u32 psci_func_id)
 {
 	return invoke_psci_fn(PSCI_1_0_FN_PSCI_FEATURES,
 			      psci_func_id, 0, 0);
 }
 
+#ifdef CONFIG_DEBUG_FS
+
+#define PSCI_ID(ver, _name) \
+	{ .fn = PSCI_##ver##_FN_##_name, .name = #_name, }
+#define PSCI_ID_NATIVE(ver, _name) \
+	{ .fn = PSCI_FN_NATIVE(ver, _name), .name = #_name, }
+
+/* A table of all optional functions */
+static const struct {
+	u32 fn;
+	const char *name;
+} psci_fn_ids[] = {
+	PSCI_ID_NATIVE(0_2, MIGRATE),
+	PSCI_ID(0_2, MIGRATE_INFO_TYPE),
+	PSCI_ID_NATIVE(0_2, MIGRATE_INFO_UP_CPU),
+	PSCI_ID(1_0, CPU_FREEZE),
+	PSCI_ID_NATIVE(1_0, CPU_DEFAULT_SUSPEND),
+	PSCI_ID_NATIVE(1_0, NODE_HW_STATE),
+	PSCI_ID_NATIVE(1_0, SYSTEM_SUSPEND),
+	PSCI_ID(1_0, SET_SUSPEND_MODE),
+	PSCI_ID_NATIVE(1_0, STAT_RESIDENCY),
+	PSCI_ID_NATIVE(1_0, STAT_COUNT),
+	PSCI_ID_NATIVE(1_1, SYSTEM_RESET2),
+};
+
+static int psci_debugfs_read(struct seq_file *s, void *data)
+{
+	int feature, type, i;
+	u32 ver;
+
+	ver = psci_ops.get_version();
+	seq_printf(s, "PSCIv%d.%d\n",
+		   PSCI_VERSION_MAJOR(ver),
+		   PSCI_VERSION_MINOR(ver));
+
+	/* PSCI_FEATURES is available only starting from 1.0 */
+	if (PSCI_VERSION_MAJOR(ver) < 1)
+		return 0;
+
+	feature = psci_features(ARM_SMCCC_VERSION_FUNC_ID);
+	if (feature != PSCI_RET_NOT_SUPPORTED) {
+		ver = invoke_psci_fn(ARM_SMCCC_VERSION_FUNC_ID, 0, 0, 0);
+		seq_printf(s, "SMC Calling Convention v%d.%d\n",
+			   PSCI_VERSION_MAJOR(ver),
+			   PSCI_VERSION_MINOR(ver));
+	} else {
+		seq_printf(s, "SMC Calling Convention v1.0 is assumed\n");
+	}
+
+	feature = psci_features(PSCI_FN_NATIVE(0_2, CPU_SUSPEND));
+	if (feature < 0) {
+		seq_printf(s, "PSCI_FEATURES(CPU_SUSPEND) error (%d)\n", feature);
+	} else {
+		seq_printf(s, "OSI is %ssupported\n",
+			   (feature & BIT(0)) ? "" : "not ");
+		seq_printf(s, "%s StateID format is used\n",
+			   (feature & BIT(1)) ? "Extended" : "Original");
+	}
+
+	type = psci_ops.migrate_info_type();
+	if (type == PSCI_0_2_TOS_UP_MIGRATE ||
+	    type == PSCI_0_2_TOS_UP_NO_MIGRATE) {
+		unsigned long cpuid;
+
+		seq_printf(s, "Trusted OS %smigrate capable\n",
+			   type == PSCI_0_2_TOS_UP_NO_MIGRATE ? "not " : "");
+		cpuid = psci_migrate_info_up_cpu();
+		seq_printf(s, "Trusted OS resident on physical CPU 0x%lx (#%d)\n", cpuid, resident_cpu);
+	} else if (type == PSCI_0_2_TOS_MP) {
+		seq_printf(s, "Trusted OS migration not required\n");
+	} else {
+		if (type != PSCI_RET_NOT_SUPPORTED)
+			seq_printf(s, "MIGRATE_INFO_TYPE returned unknown type (%d)\n", type);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(psci_fn_ids); i++) {
+		feature = psci_features(psci_fn_ids[i].fn);
+		if (feature == PSCI_RET_NOT_SUPPORTED)
+			continue;
+		if (feature < 0)
+			seq_printf(s, "PSCI_FEATURES(%s) error (%d)\n", psci_fn_ids[i].name, feature);
+		else
+			seq_printf(s, "%s is supported\n", psci_fn_ids[i].name);
+	}
+
+	return 0;
+}
+
+static int psci_debugfs_open(struct inode *inode, struct file *f)
+{
+	return single_open(f, psci_debugfs_read, NULL);
+}
+
+static const struct file_operations psci_debugfs_ops = {
+	.owner = THIS_MODULE,
+	.open = psci_debugfs_open,
+	.release = single_release,
+	.read = seq_read,
+	.llseek = seq_lseek
+};
+
+static int __init psci_debugfs_init(void)
+{
+	return PTR_ERR_OR_ZERO(debugfs_create_file("psci", S_IRUGO, NULL, NULL,
+						   &psci_debugfs_ops));
+}
+late_initcall(psci_debugfs_init)
+#endif
+
 #ifdef CONFIG_CPU_IDLE
 static int psci_suspend_finisher(unsigned long state)
 {
diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h
index 2bf93c0d6354..f6f0bad5858b 100644
--- a/include/uapi/linux/psci.h
+++ b/include/uapi/linux/psci.h
@@ -48,11 +48,20 @@
 #define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU	PSCI_0_2_FN64(7)
 
 #define PSCI_1_0_FN_PSCI_FEATURES		PSCI_0_2_FN(10)
+#define PSCI_1_0_FN_CPU_FREEZE			PSCI_0_2_FN(11)
+#define PSCI_1_0_FN_CPU_DEFAULT_SUSPEND		PSCI_0_2_FN(12)
+#define PSCI_1_0_FN_NODE_HW_STATE		PSCI_0_2_FN(13)
 #define PSCI_1_0_FN_SYSTEM_SUSPEND		PSCI_0_2_FN(14)
 #define PSCI_1_0_FN_SET_SUSPEND_MODE		PSCI_0_2_FN(15)
+#define PSCI_1_0_FN_STAT_RESIDENCY		PSCI_0_2_FN(16)
+#define PSCI_1_0_FN_STAT_COUNT			PSCI_0_2_FN(17)
 #define PSCI_1_1_FN_SYSTEM_RESET2		PSCI_0_2_FN(18)
 
+#define PSCI_1_0_FN64_CPU_DEFAULT_SUSPEND	PSCI_0_2_FN64(12)
+#define PSCI_1_0_FN64_NODE_HW_STATE		PSCI_0_2_FN64(13)
 #define PSCI_1_0_FN64_SYSTEM_SUSPEND		PSCI_0_2_FN64(14)
+#define PSCI_1_0_FN64_STAT_RESIDENCY		PSCI_0_2_FN64(16)
+#define PSCI_1_0_FN64_STAT_COUNT		PSCI_0_2_FN64(17)
 #define PSCI_1_1_FN64_SYSTEM_RESET2		PSCI_0_2_FN64(18)
 
 /* PSCI v0.2 power state encoding for CPU_SUSPEND function */
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware/psci: Add debugfs support to ease debugging

[Bhupesh Sharma]

Hi Dmitry,

On 7/28/22 1:39 AM, Dmitry Baryshkov wrote:
> To ease debugging of PSCI supported features, add debugfs file called
> 'psci' describing PSCI and SMC CC versions, enabled features and
> options.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/firmware/psci/psci.c | 112 ++++++++++++++++++++++++++++++++++-
>   include/uapi/linux/psci.h    |   9 +++
>   2 files changed, 120 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index b907768eea01..6595cc964635 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -9,6 +9,7 @@
>   #include <linux/acpi.h>
>   #include <linux/arm-smccc.h>
>   #include <linux/cpuidle.h>
> +#include <linux/debugfs.h>
>   #include <linux/errno.h>
>   #include <linux/linkage.h>
>   #include <linux/of.h>
> @@ -324,12 +325,121 @@ static void psci_sys_poweroff(void)
>   	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
>   }
>   
> -static int __init psci_features(u32 psci_func_id)
> +static int psci_features(u32 psci_func_id)

This change doesn't seem related to the patch $SUBJECT.
Also is it really needed? If yes, probably this should be a separate patch.

Thanks,
Bhupesh

>   {
>   	return invoke_psci_fn(PSCI_1_0_FN_PSCI_FEATURES,
>   			      psci_func_id, 0, 0);
>   }
>   
> +#ifdef CONFIG_DEBUG_FS
> +
> +#define PSCI_ID(ver, _name) \
> +	{ .fn = PSCI_##ver##_FN_##_name, .name = #_name, }
> +#define PSCI_ID_NATIVE(ver, _name) \
> +	{ .fn = PSCI_FN_NATIVE(ver, _name), .name = #_name, }
> +
> +/* A table of all optional functions */
> +static const struct {
> +	u32 fn;
> +	const char *name;
> +} psci_fn_ids[] = {
> +	PSCI_ID_NATIVE(0_2, MIGRATE),
> +	PSCI_ID(0_2, MIGRATE_INFO_TYPE),
> +	PSCI_ID_NATIVE(0_2, MIGRATE_INFO_UP_CPU),
> +	PSCI_ID(1_0, CPU_FREEZE),
> +	PSCI_ID_NATIVE(1_0, CPU_DEFAULT_SUSPEND),
> +	PSCI_ID_NATIVE(1_0, NODE_HW_STATE),
> +	PSCI_ID_NATIVE(1_0, SYSTEM_SUSPEND),
> +	PSCI_ID(1_0, SET_SUSPEND_MODE),
> +	PSCI_ID_NATIVE(1_0, STAT_RESIDENCY),
> +	PSCI_ID_NATIVE(1_0, STAT_COUNT),
> +	PSCI_ID_NATIVE(1_1, SYSTEM_RESET2),
> +};
> +
> +static int psci_debugfs_read(struct seq_file *s, void *data)
> +{
> +	int feature, type, i;
> +	u32 ver;
> +
> +	ver = psci_ops.get_version();
> +	seq_printf(s, "PSCIv%d.%d\n",
> +		   PSCI_VERSION_MAJOR(ver),
> +		   PSCI_VERSION_MINOR(ver));
> +
> +	/* PSCI_FEATURES is available only starting from 1.0 */
> +	if (PSCI_VERSION_MAJOR(ver) < 1)
> +		return 0;
> +
> +	feature = psci_features(ARM_SMCCC_VERSION_FUNC_ID);
> +	if (feature != PSCI_RET_NOT_SUPPORTED) {
> +		ver = invoke_psci_fn(ARM_SMCCC_VERSION_FUNC_ID, 0, 0, 0);
> +		seq_printf(s, "SMC Calling Convention v%d.%d\n",
> +			   PSCI_VERSION_MAJOR(ver),
> +			   PSCI_VERSION_MINOR(ver));
> +	} else {
> +		seq_printf(s, "SMC Calling Convention v1.0 is assumed\n");
> +	}
> +
> +	feature = psci_features(PSCI_FN_NATIVE(0_2, CPU_SUSPEND));
> +	if (feature < 0) {
> +		seq_printf(s, "PSCI_FEATURES(CPU_SUSPEND) error (%d)\n", feature);
> +	} else {
> +		seq_printf(s, "OSI is %ssupported\n",
> +			   (feature & BIT(0)) ? "" : "not ");
> +		seq_printf(s, "%s StateID format is used\n",
> +			   (feature & BIT(1)) ? "Extended" : "Original");
> +	}
> +
> +	type = psci_ops.migrate_info_type();
> +	if (type == PSCI_0_2_TOS_UP_MIGRATE ||
> +	    type == PSCI_0_2_TOS_UP_NO_MIGRATE) {
> +		unsigned long cpuid;
> +
> +		seq_printf(s, "Trusted OS %smigrate capable\n",
> +			   type == PSCI_0_2_TOS_UP_NO_MIGRATE ? "not " : "");
> +		cpuid = psci_migrate_info_up_cpu();
> +		seq_printf(s, "Trusted OS resident on physical CPU 0x%lx (#%d)\n", cpuid, resident_cpu);
> +	} else if (type == PSCI_0_2_TOS_MP) {
> +		seq_printf(s, "Trusted OS migration not required\n");
> +	} else {
> +		if (type != PSCI_RET_NOT_SUPPORTED)
> +			seq_printf(s, "MIGRATE_INFO_TYPE returned unknown type (%d)\n", type);
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(psci_fn_ids); i++) {
> +		feature = psci_features(psci_fn_ids[i].fn);
> +		if (feature == PSCI_RET_NOT_SUPPORTED)
> +			continue;
> +		if (feature < 0)
> +			seq_printf(s, "PSCI_FEATURES(%s) error (%d)\n", psci_fn_ids[i].name, feature);
> +		else
> +			seq_printf(s, "%s is supported\n", psci_fn_ids[i].name);
> +	}
> +
> +	return 0;
> +}
> +
> +static int psci_debugfs_open(struct inode *inode, struct file *f)
> +{
> +	return single_open(f, psci_debugfs_read, NULL);
> +}
> +
> +static const struct file_operations psci_debugfs_ops = {
> +	.owner = THIS_MODULE,
> +	.open = psci_debugfs_open,
> +	.release = single_release,
> +	.read = seq_read,
> +	.llseek = seq_lseek
> +};
> +
> +static int __init psci_debugfs_init(void)
> +{
> +	return PTR_ERR_OR_ZERO(debugfs_create_file("psci", S_IRUGO, NULL, NULL,
> +						   &psci_debugfs_ops));
> +}
> +late_initcall(psci_debugfs_init)
> +#endif
> +
>   #ifdef CONFIG_CPU_IDLE
>   static int psci_suspend_finisher(unsigned long state)
>   {
> diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h
> index 2bf93c0d6354..f6f0bad5858b 100644
> --- a/include/uapi/linux/psci.h
> +++ b/include/uapi/linux/psci.h
> @@ -48,11 +48,20 @@
>   #define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU	PSCI_0_2_FN64(7)
>   
>   #define PSCI_1_0_FN_PSCI_FEATURES		PSCI_0_2_FN(10)
> +#define PSCI_1_0_FN_CPU_FREEZE			PSCI_0_2_FN(11)
> +#define PSCI_1_0_FN_CPU_DEFAULT_SUSPEND		PSCI_0_2_FN(12)
> +#define PSCI_1_0_FN_NODE_HW_STATE		PSCI_0_2_FN(13)
>   #define PSCI_1_0_FN_SYSTEM_SUSPEND		PSCI_0_2_FN(14)
>   #define PSCI_1_0_FN_SET_SUSPEND_MODE		PSCI_0_2_FN(15)
> +#define PSCI_1_0_FN_STAT_RESIDENCY		PSCI_0_2_FN(16)
> +#define PSCI_1_0_FN_STAT_COUNT			PSCI_0_2_FN(17)
>   #define PSCI_1_1_FN_SYSTEM_RESET2		PSCI_0_2_FN(18)
>   
> +#define PSCI_1_0_FN64_CPU_DEFAULT_SUSPEND	PSCI_0_2_FN64(12)
> +#define PSCI_1_0_FN64_NODE_HW_STATE		PSCI_0_2_FN64(13)
>   #define PSCI_1_0_FN64_SYSTEM_SUSPEND		PSCI_0_2_FN64(14)
> +#define PSCI_1_0_FN64_STAT_RESIDENCY		PSCI_0_2_FN64(16)
> +#define PSCI_1_0_FN64_STAT_COUNT		PSCI_0_2_FN64(17)
>   #define PSCI_1_1_FN64_SYSTEM_RESET2		PSCI_0_2_FN64(18)
>   
>   /* PSCI v0.2 power state encoding for CPU_SUSPEND function */

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware/psci: Add debugfs support to ease debugging

[Dmitry Baryshkov]

On Wed, 27 Jul 2022 at 23:15, Bhupesh Sharma <bhupesh.sharma@linaro.org> wrote:
>
> Hi Dmitry,
>
> On 7/28/22 1:39 AM, Dmitry Baryshkov wrote:
> > To ease debugging of PSCI supported features, add debugfs file called
> > 'psci' describing PSCI and SMC CC versions, enabled features and
> > options.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/firmware/psci/psci.c | 112 ++++++++++++++++++++++++++++++++++-
> >   include/uapi/linux/psci.h    |   9 +++
> >   2 files changed, 120 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > index b907768eea01..6595cc964635 100644
> > --- a/drivers/firmware/psci/psci.c
> > +++ b/drivers/firmware/psci/psci.c
> > @@ -9,6 +9,7 @@
> >   #include <linux/acpi.h>
> >   #include <linux/arm-smccc.h>
> >   #include <linux/cpuidle.h>
> > +#include <linux/debugfs.h>
> >   #include <linux/errno.h>
> >   #include <linux/linkage.h>
> >   #include <linux/of.h>
> > @@ -324,12 +325,121 @@ static void psci_sys_poweroff(void)
> >       invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> >   }
> >
> > -static int __init psci_features(u32 psci_func_id)
> > +static int psci_features(u32 psci_func_id)
>
> This change doesn't seem related to the patch $SUBJECT.
> Also is it really needed? If yes, probably this should be a separate patch.

It is related and I don't think it should be moved to a separate
patch. Removing the __init annotation from psci_features() is
necessary to allow using psci_features() from psci_debufs_read(),
which is definitely not an __init code. Otherwise reading from
debugfs/psci would cause null pointer exceptions.

>
> Thanks,
> Bhupesh
>
> >   {
> >       return invoke_psci_fn(PSCI_1_0_FN_PSCI_FEATURES,
> >                             psci_func_id, 0, 0);
> >   }
> >
> > +#ifdef CONFIG_DEBUG_FS
> > +
> > +#define PSCI_ID(ver, _name) \
> > +     { .fn = PSCI_##ver##_FN_##_name, .name = #_name, }
> > +#define PSCI_ID_NATIVE(ver, _name) \
> > +     { .fn = PSCI_FN_NATIVE(ver, _name), .name = #_name, }
> > +
> > +/* A table of all optional functions */
> > +static const struct {
> > +     u32 fn;
> > +     const char *name;
> > +} psci_fn_ids[] = {
> > +     PSCI_ID_NATIVE(0_2, MIGRATE),
> > +     PSCI_ID(0_2, MIGRATE_INFO_TYPE),
> > +     PSCI_ID_NATIVE(0_2, MIGRATE_INFO_UP_CPU),
> > +     PSCI_ID(1_0, CPU_FREEZE),
> > +     PSCI_ID_NATIVE(1_0, CPU_DEFAULT_SUSPEND),
> > +     PSCI_ID_NATIVE(1_0, NODE_HW_STATE),
> > +     PSCI_ID_NATIVE(1_0, SYSTEM_SUSPEND),
> > +     PSCI_ID(1_0, SET_SUSPEND_MODE),
> > +     PSCI_ID_NATIVE(1_0, STAT_RESIDENCY),
> > +     PSCI_ID_NATIVE(1_0, STAT_COUNT),
> > +     PSCI_ID_NATIVE(1_1, SYSTEM_RESET2),
> > +};
> > +
> > +static int psci_debugfs_read(struct seq_file *s, void *data)
> > +{
> > +     int feature, type, i;
> > +     u32 ver;
> > +
> > +     ver = psci_ops.get_version();
> > +     seq_printf(s, "PSCIv%d.%d\n",
> > +                PSCI_VERSION_MAJOR(ver),
> > +                PSCI_VERSION_MINOR(ver));
> > +
> > +     /* PSCI_FEATURES is available only starting from 1.0 */
> > +     if (PSCI_VERSION_MAJOR(ver) < 1)
> > +             return 0;
> > +
> > +     feature = psci_features(ARM_SMCCC_VERSION_FUNC_ID);
> > +     if (feature != PSCI_RET_NOT_SUPPORTED) {
> > +             ver = invoke_psci_fn(ARM_SMCCC_VERSION_FUNC_ID, 0, 0, 0);
> > +             seq_printf(s, "SMC Calling Convention v%d.%d\n",
> > +                        PSCI_VERSION_MAJOR(ver),
> > +                        PSCI_VERSION_MINOR(ver));
> > +     } else {
> > +             seq_printf(s, "SMC Calling Convention v1.0 is assumed\n");
> > +     }
> > +
> > +     feature = psci_features(PSCI_FN_NATIVE(0_2, CPU_SUSPEND));
> > +     if (feature < 0) {
> > +             seq_printf(s, "PSCI_FEATURES(CPU_SUSPEND) error (%d)\n", feature);
> > +     } else {
> > +             seq_printf(s, "OSI is %ssupported\n",
> > +                        (feature & BIT(0)) ? "" : "not ");
> > +             seq_printf(s, "%s StateID format is used\n",
> > +                        (feature & BIT(1)) ? "Extended" : "Original");
> > +     }
> > +
> > +     type = psci_ops.migrate_info_type();
> > +     if (type == PSCI_0_2_TOS_UP_MIGRATE ||
> > +         type == PSCI_0_2_TOS_UP_NO_MIGRATE) {
> > +             unsigned long cpuid;
> > +
> > +             seq_printf(s, "Trusted OS %smigrate capable\n",
> > +                        type == PSCI_0_2_TOS_UP_NO_MIGRATE ? "not " : "");
> > +             cpuid = psci_migrate_info_up_cpu();
> > +             seq_printf(s, "Trusted OS resident on physical CPU 0x%lx (#%d)\n", cpuid, resident_cpu);
> > +     } else if (type == PSCI_0_2_TOS_MP) {
> > +             seq_printf(s, "Trusted OS migration not required\n");
> > +     } else {
> > +             if (type != PSCI_RET_NOT_SUPPORTED)
> > +                     seq_printf(s, "MIGRATE_INFO_TYPE returned unknown type (%d)\n", type);
> > +     }
> > +
> > +     for (i = 0; i < ARRAY_SIZE(psci_fn_ids); i++) {
> > +             feature = psci_features(psci_fn_ids[i].fn);
> > +             if (feature == PSCI_RET_NOT_SUPPORTED)
> > +                     continue;
> > +             if (feature < 0)
> > +                     seq_printf(s, "PSCI_FEATURES(%s) error (%d)\n", psci_fn_ids[i].name, feature);
> > +             else
> > +                     seq_printf(s, "%s is supported\n", psci_fn_ids[i].name);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int psci_debugfs_open(struct inode *inode, struct file *f)
> > +{
> > +     return single_open(f, psci_debugfs_read, NULL);
> > +}
> > +
> > +static const struct file_operations psci_debugfs_ops = {
> > +     .owner = THIS_MODULE,
> > +     .open = psci_debugfs_open,
> > +     .release = single_release,
> > +     .read = seq_read,
> > +     .llseek = seq_lseek
> > +};
> > +
> > +static int __init psci_debugfs_init(void)
> > +{
> > +     return PTR_ERR_OR_ZERO(debugfs_create_file("psci", S_IRUGO, NULL, NULL,
> > +                                                &psci_debugfs_ops));
> > +}
> > +late_initcall(psci_debugfs_init)
> > +#endif
> > +
> >   #ifdef CONFIG_CPU_IDLE
> >   static int psci_suspend_finisher(unsigned long state)
> >   {
> > diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h
> > index 2bf93c0d6354..f6f0bad5858b 100644
> > --- a/include/uapi/linux/psci.h
> > +++ b/include/uapi/linux/psci.h
> > @@ -48,11 +48,20 @@
> >   #define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU   PSCI_0_2_FN64(7)
> >
> >   #define PSCI_1_0_FN_PSCI_FEATURES           PSCI_0_2_FN(10)
> > +#define PSCI_1_0_FN_CPU_FREEZE                       PSCI_0_2_FN(11)
> > +#define PSCI_1_0_FN_CPU_DEFAULT_SUSPEND              PSCI_0_2_FN(12)
> > +#define PSCI_1_0_FN_NODE_HW_STATE            PSCI_0_2_FN(13)
> >   #define PSCI_1_0_FN_SYSTEM_SUSPEND          PSCI_0_2_FN(14)
> >   #define PSCI_1_0_FN_SET_SUSPEND_MODE                PSCI_0_2_FN(15)
> > +#define PSCI_1_0_FN_STAT_RESIDENCY           PSCI_0_2_FN(16)
> > +#define PSCI_1_0_FN_STAT_COUNT                       PSCI_0_2_FN(17)
> >   #define PSCI_1_1_FN_SYSTEM_RESET2           PSCI_0_2_FN(18)
> >
> > +#define PSCI_1_0_FN64_CPU_DEFAULT_SUSPEND    PSCI_0_2_FN64(12)
> > +#define PSCI_1_0_FN64_NODE_HW_STATE          PSCI_0_2_FN64(13)
> >   #define PSCI_1_0_FN64_SYSTEM_SUSPEND                PSCI_0_2_FN64(14)
> > +#define PSCI_1_0_FN64_STAT_RESIDENCY         PSCI_0_2_FN64(16)
> > +#define PSCI_1_0_FN64_STAT_COUNT             PSCI_0_2_FN64(17)
> >   #define PSCI_1_1_FN64_SYSTEM_RESET2         PSCI_0_2_FN64(18)
> >
> >   /* PSCI v0.2 power state encoding for CPU_SUSPEND function */



-- 
With best wishes
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware/psci: Add debugfs support to ease debugging

[Bhupesh Sharma]

On 7/28/22 2:23 AM, Dmitry Baryshkov wrote:
> On Wed, 27 Jul 2022 at 23:15, Bhupesh Sharma <bhupesh.sharma@linaro.org> wrote:
>>
>> Hi Dmitry,
>>
>> On 7/28/22 1:39 AM, Dmitry Baryshkov wrote:
>>> To ease debugging of PSCI supported features, add debugfs file called
>>> 'psci' describing PSCI and SMC CC versions, enabled features and
>>> options.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/firmware/psci/psci.c | 112 ++++++++++++++++++++++++++++++++++-
>>>    include/uapi/linux/psci.h    |   9 +++
>>>    2 files changed, 120 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
>>> index b907768eea01..6595cc964635 100644
>>> --- a/drivers/firmware/psci/psci.c
>>> +++ b/drivers/firmware/psci/psci.c
>>> @@ -9,6 +9,7 @@
>>>    #include <linux/acpi.h>
>>>    #include <linux/arm-smccc.h>
>>>    #include <linux/cpuidle.h>
>>> +#include <linux/debugfs.h>
>>>    #include <linux/errno.h>
>>>    #include <linux/linkage.h>
>>>    #include <linux/of.h>
>>> @@ -324,12 +325,121 @@ static void psci_sys_poweroff(void)
>>>        invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
>>>    }
>>>
>>> -static int __init psci_features(u32 psci_func_id)
>>> +static int psci_features(u32 psci_func_id)
>>
>> This change doesn't seem related to the patch $SUBJECT.
>> Also is it really needed? If yes, probably this should be a separate patch.
> 
> It is related and I don't think it should be moved to a separate
> patch. Removing the __init annotation from psci_features() is
> necessary to allow using psci_features() from psci_debufs_read(),
> which is definitely not an __init code. Otherwise reading from
> debugfs/psci would cause null pointer exceptions.

Ok, and what is the behavior with CONFIG_DEBUG_FS = n?
Does your patch work well in that case?

Thanks.

>>
>>>    {
>>>        return invoke_psci_fn(PSCI_1_0_FN_PSCI_FEATURES,
>>>                              psci_func_id, 0, 0);
>>>    }
>>>
>>> +#ifdef CONFIG_DEBUG_FS
>>> +
>>> +#define PSCI_ID(ver, _name) \
>>> +     { .fn = PSCI_##ver##_FN_##_name, .name = #_name, }
>>> +#define PSCI_ID_NATIVE(ver, _name) \
>>> +     { .fn = PSCI_FN_NATIVE(ver, _name), .name = #_name, }
>>> +
>>> +/* A table of all optional functions */
>>> +static const struct {
>>> +     u32 fn;
>>> +     const char *name;
>>> +} psci_fn_ids[] = {
>>> +     PSCI_ID_NATIVE(0_2, MIGRATE),
>>> +     PSCI_ID(0_2, MIGRATE_INFO_TYPE),
>>> +     PSCI_ID_NATIVE(0_2, MIGRATE_INFO_UP_CPU),
>>> +     PSCI_ID(1_0, CPU_FREEZE),
>>> +     PSCI_ID_NATIVE(1_0, CPU_DEFAULT_SUSPEND),
>>> +     PSCI_ID_NATIVE(1_0, NODE_HW_STATE),
>>> +     PSCI_ID_NATIVE(1_0, SYSTEM_SUSPEND),
>>> +     PSCI_ID(1_0, SET_SUSPEND_MODE),
>>> +     PSCI_ID_NATIVE(1_0, STAT_RESIDENCY),
>>> +     PSCI_ID_NATIVE(1_0, STAT_COUNT),
>>> +     PSCI_ID_NATIVE(1_1, SYSTEM_RESET2),
>>> +};
>>> +
>>> +static int psci_debugfs_read(struct seq_file *s, void *data)
>>> +{
>>> +     int feature, type, i;
>>> +     u32 ver;
>>> +
>>> +     ver = psci_ops.get_version();
>>> +     seq_printf(s, "PSCIv%d.%d\n",
>>> +                PSCI_VERSION_MAJOR(ver),
>>> +                PSCI_VERSION_MINOR(ver));
>>> +
>>> +     /* PSCI_FEATURES is available only starting from 1.0 */
>>> +     if (PSCI_VERSION_MAJOR(ver) < 1)
>>> +             return 0;
>>> +
>>> +     feature = psci_features(ARM_SMCCC_VERSION_FUNC_ID);
>>> +     if (feature != PSCI_RET_NOT_SUPPORTED) {
>>> +             ver = invoke_psci_fn(ARM_SMCCC_VERSION_FUNC_ID, 0, 0, 0);
>>> +             seq_printf(s, "SMC Calling Convention v%d.%d\n",
>>> +                        PSCI_VERSION_MAJOR(ver),
>>> +                        PSCI_VERSION_MINOR(ver));
>>> +     } else {
>>> +             seq_printf(s, "SMC Calling Convention v1.0 is assumed\n");
>>> +     }
>>> +
>>> +     feature = psci_features(PSCI_FN_NATIVE(0_2, CPU_SUSPEND));
>>> +     if (feature < 0) {
>>> +             seq_printf(s, "PSCI_FEATURES(CPU_SUSPEND) error (%d)\n", feature);
>>> +     } else {
>>> +             seq_printf(s, "OSI is %ssupported\n",
>>> +                        (feature & BIT(0)) ? "" : "not ");
>>> +             seq_printf(s, "%s StateID format is used\n",
>>> +                        (feature & BIT(1)) ? "Extended" : "Original");
>>> +     }
>>> +
>>> +     type = psci_ops.migrate_info_type();
>>> +     if (type == PSCI_0_2_TOS_UP_MIGRATE ||
>>> +         type == PSCI_0_2_TOS_UP_NO_MIGRATE) {
>>> +             unsigned long cpuid;
>>> +
>>> +             seq_printf(s, "Trusted OS %smigrate capable\n",
>>> +                        type == PSCI_0_2_TOS_UP_NO_MIGRATE ? "not " : "");
>>> +             cpuid = psci_migrate_info_up_cpu();
>>> +             seq_printf(s, "Trusted OS resident on physical CPU 0x%lx (#%d)\n", cpuid, resident_cpu);
>>> +     } else if (type == PSCI_0_2_TOS_MP) {
>>> +             seq_printf(s, "Trusted OS migration not required\n");
>>> +     } else {
>>> +             if (type != PSCI_RET_NOT_SUPPORTED)
>>> +                     seq_printf(s, "MIGRATE_INFO_TYPE returned unknown type (%d)\n", type);
>>> +     }
>>> +
>>> +     for (i = 0; i < ARRAY_SIZE(psci_fn_ids); i++) {
>>> +             feature = psci_features(psci_fn_ids[i].fn);
>>> +             if (feature == PSCI_RET_NOT_SUPPORTED)
>>> +                     continue;
>>> +             if (feature < 0)
>>> +                     seq_printf(s, "PSCI_FEATURES(%s) error (%d)\n", psci_fn_ids[i].name, feature);
>>> +             else
>>> +                     seq_printf(s, "%s is supported\n", psci_fn_ids[i].name);
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int psci_debugfs_open(struct inode *inode, struct file *f)
>>> +{
>>> +     return single_open(f, psci_debugfs_read, NULL);
>>> +}
>>> +
>>> +static const struct file_operations psci_debugfs_ops = {
>>> +     .owner = THIS_MODULE,
>>> +     .open = psci_debugfs_open,
>>> +     .release = single_release,
>>> +     .read = seq_read,
>>> +     .llseek = seq_lseek
>>> +};
>>> +
>>> +static int __init psci_debugfs_init(void)
>>> +{
>>> +     return PTR_ERR_OR_ZERO(debugfs_create_file("psci", S_IRUGO, NULL, NULL,
>>> +                                                &psci_debugfs_ops));
>>> +}
>>> +late_initcall(psci_debugfs_init)
>>> +#endif
>>> +
>>>    #ifdef CONFIG_CPU_IDLE
>>>    static int psci_suspend_finisher(unsigned long state)
>>>    {
>>> diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h
>>> index 2bf93c0d6354..f6f0bad5858b 100644
>>> --- a/include/uapi/linux/psci.h
>>> +++ b/include/uapi/linux/psci.h
>>> @@ -48,11 +48,20 @@
>>>    #define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU   PSCI_0_2_FN64(7)
>>>
>>>    #define PSCI_1_0_FN_PSCI_FEATURES           PSCI_0_2_FN(10)
>>> +#define PSCI_1_0_FN_CPU_FREEZE                       PSCI_0_2_FN(11)
>>> +#define PSCI_1_0_FN_CPU_DEFAULT_SUSPEND              PSCI_0_2_FN(12)
>>> +#define PSCI_1_0_FN_NODE_HW_STATE            PSCI_0_2_FN(13)
>>>    #define PSCI_1_0_FN_SYSTEM_SUSPEND          PSCI_0_2_FN(14)
>>>    #define PSCI_1_0_FN_SET_SUSPEND_MODE                PSCI_0_2_FN(15)
>>> +#define PSCI_1_0_FN_STAT_RESIDENCY           PSCI_0_2_FN(16)
>>> +#define PSCI_1_0_FN_STAT_COUNT                       PSCI_0_2_FN(17)
>>>    #define PSCI_1_1_FN_SYSTEM_RESET2           PSCI_0_2_FN(18)
>>>
>>> +#define PSCI_1_0_FN64_CPU_DEFAULT_SUSPEND    PSCI_0_2_FN64(12)
>>> +#define PSCI_1_0_FN64_NODE_HW_STATE          PSCI_0_2_FN64(13)
>>>    #define PSCI_1_0_FN64_SYSTEM_SUSPEND                PSCI_0_2_FN64(14)
>>> +#define PSCI_1_0_FN64_STAT_RESIDENCY         PSCI_0_2_FN64(16)
>>> +#define PSCI_1_0_FN64_STAT_COUNT             PSCI_0_2_FN64(17)
>>>    #define PSCI_1_1_FN64_SYSTEM_RESET2         PSCI_0_2_FN64(18)
>>>
>>>    /* PSCI v0.2 power state encoding for CPU_SUSPEND function */
> 
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware/psci: Add debugfs support to ease debugging

[Dmitry Baryshkov]

On Wed, 27 Jul 2022 at 23:55, Bhupesh Sharma <bhupesh.sharma@linaro.org> wrote:
>
>
>
> On 7/28/22 2:23 AM, Dmitry Baryshkov wrote:
> > On Wed, 27 Jul 2022 at 23:15, Bhupesh Sharma <bhupesh.sharma@linaro.org> wrote:
> >>
> >> Hi Dmitry,
> >>
> >> On 7/28/22 1:39 AM, Dmitry Baryshkov wrote:
> >>> To ease debugging of PSCI supported features, add debugfs file called
> >>> 'psci' describing PSCI and SMC CC versions, enabled features and
> >>> options.
> >>>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> ---
> >>>    drivers/firmware/psci/psci.c | 112 ++++++++++++++++++++++++++++++++++-
> >>>    include/uapi/linux/psci.h    |   9 +++
> >>>    2 files changed, 120 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> >>> index b907768eea01..6595cc964635 100644
> >>> --- a/drivers/firmware/psci/psci.c
> >>> +++ b/drivers/firmware/psci/psci.c
> >>> @@ -9,6 +9,7 @@
> >>>    #include <linux/acpi.h>
> >>>    #include <linux/arm-smccc.h>
> >>>    #include <linux/cpuidle.h>
> >>> +#include <linux/debugfs.h>
> >>>    #include <linux/errno.h>
> >>>    #include <linux/linkage.h>
> >>>    #include <linux/of.h>
> >>> @@ -324,12 +325,121 @@ static void psci_sys_poweroff(void)
> >>>        invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> >>>    }
> >>>
> >>> -static int __init psci_features(u32 psci_func_id)
> >>> +static int psci_features(u32 psci_func_id)
> >>
> >> This change doesn't seem related to the patch $SUBJECT.
> >> Also is it really needed? If yes, probably this should be a separate patch.
> >
> > It is related and I don't think it should be moved to a separate
> > patch. Removing the __init annotation from psci_features() is
> > necessary to allow using psci_features() from psci_debufs_read(),
> > which is definitely not an __init code. Otherwise reading from
> > debugfs/psci would cause null pointer exceptions.
>
> Ok, and what is the behavior with CONFIG_DEBUG_FS = n?
> Does your patch work well in that case?

Yes. Any particular reasons for the question?

>
> Thanks.
>
> >>
> >>>    {
> >>>        return invoke_psci_fn(PSCI_1_0_FN_PSCI_FEATURES,
> >>>                              psci_func_id, 0, 0);
> >>>    }
> >>>
> >>> +#ifdef CONFIG_DEBUG_FS
> >>> +
> >>> +#define PSCI_ID(ver, _name) \
> >>> +     { .fn = PSCI_##ver##_FN_##_name, .name = #_name, }
> >>> +#define PSCI_ID_NATIVE(ver, _name) \
> >>> +     { .fn = PSCI_FN_NATIVE(ver, _name), .name = #_name, }
> >>> +
> >>> +/* A table of all optional functions */
> >>> +static const struct {
> >>> +     u32 fn;
> >>> +     const char *name;
> >>> +} psci_fn_ids[] = {
> >>> +     PSCI_ID_NATIVE(0_2, MIGRATE),
> >>> +     PSCI_ID(0_2, MIGRATE_INFO_TYPE),
> >>> +     PSCI_ID_NATIVE(0_2, MIGRATE_INFO_UP_CPU),
> >>> +     PSCI_ID(1_0, CPU_FREEZE),
> >>> +     PSCI_ID_NATIVE(1_0, CPU_DEFAULT_SUSPEND),
> >>> +     PSCI_ID_NATIVE(1_0, NODE_HW_STATE),
> >>> +     PSCI_ID_NATIVE(1_0, SYSTEM_SUSPEND),
> >>> +     PSCI_ID(1_0, SET_SUSPEND_MODE),
> >>> +     PSCI_ID_NATIVE(1_0, STAT_RESIDENCY),
> >>> +     PSCI_ID_NATIVE(1_0, STAT_COUNT),
> >>> +     PSCI_ID_NATIVE(1_1, SYSTEM_RESET2),
> >>> +};
> >>> +
> >>> +static int psci_debugfs_read(struct seq_file *s, void *data)
> >>> +{
> >>> +     int feature, type, i;
> >>> +     u32 ver;
> >>> +
> >>> +     ver = psci_ops.get_version();
> >>> +     seq_printf(s, "PSCIv%d.%d\n",
> >>> +                PSCI_VERSION_MAJOR(ver),
> >>> +                PSCI_VERSION_MINOR(ver));
> >>> +
> >>> +     /* PSCI_FEATURES is available only starting from 1.0 */
> >>> +     if (PSCI_VERSION_MAJOR(ver) < 1)
> >>> +             return 0;
> >>> +
> >>> +     feature = psci_features(ARM_SMCCC_VERSION_FUNC_ID);
> >>> +     if (feature != PSCI_RET_NOT_SUPPORTED) {
> >>> +             ver = invoke_psci_fn(ARM_SMCCC_VERSION_FUNC_ID, 0, 0, 0);
> >>> +             seq_printf(s, "SMC Calling Convention v%d.%d\n",
> >>> +                        PSCI_VERSION_MAJOR(ver),
> >>> +                        PSCI_VERSION_MINOR(ver));
> >>> +     } else {
> >>> +             seq_printf(s, "SMC Calling Convention v1.0 is assumed\n");
> >>> +     }
> >>> +
> >>> +     feature = psci_features(PSCI_FN_NATIVE(0_2, CPU_SUSPEND));
> >>> +     if (feature < 0) {
> >>> +             seq_printf(s, "PSCI_FEATURES(CPU_SUSPEND) error (%d)\n", feature);
> >>> +     } else {
> >>> +             seq_printf(s, "OSI is %ssupported\n",
> >>> +                        (feature & BIT(0)) ? "" : "not ");
> >>> +             seq_printf(s, "%s StateID format is used\n",
> >>> +                        (feature & BIT(1)) ? "Extended" : "Original");
> >>> +     }
> >>> +
> >>> +     type = psci_ops.migrate_info_type();
> >>> +     if (type == PSCI_0_2_TOS_UP_MIGRATE ||
> >>> +         type == PSCI_0_2_TOS_UP_NO_MIGRATE) {
> >>> +             unsigned long cpuid;
> >>> +
> >>> +             seq_printf(s, "Trusted OS %smigrate capable\n",
> >>> +                        type == PSCI_0_2_TOS_UP_NO_MIGRATE ? "not " : "");
> >>> +             cpuid = psci_migrate_info_up_cpu();
> >>> +             seq_printf(s, "Trusted OS resident on physical CPU 0x%lx (#%d)\n", cpuid, resident_cpu);
> >>> +     } else if (type == PSCI_0_2_TOS_MP) {
> >>> +             seq_printf(s, "Trusted OS migration not required\n");
> >>> +     } else {
> >>> +             if (type != PSCI_RET_NOT_SUPPORTED)
> >>> +                     seq_printf(s, "MIGRATE_INFO_TYPE returned unknown type (%d)\n", type);
> >>> +     }
> >>> +
> >>> +     for (i = 0; i < ARRAY_SIZE(psci_fn_ids); i++) {
> >>> +             feature = psci_features(psci_fn_ids[i].fn);
> >>> +             if (feature == PSCI_RET_NOT_SUPPORTED)
> >>> +                     continue;
> >>> +             if (feature < 0)
> >>> +                     seq_printf(s, "PSCI_FEATURES(%s) error (%d)\n", psci_fn_ids[i].name, feature);
> >>> +             else
> >>> +                     seq_printf(s, "%s is supported\n", psci_fn_ids[i].name);
> >>> +     }
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static int psci_debugfs_open(struct inode *inode, struct file *f)
> >>> +{
> >>> +     return single_open(f, psci_debugfs_read, NULL);
> >>> +}
> >>> +
> >>> +static const struct file_operations psci_debugfs_ops = {
> >>> +     .owner = THIS_MODULE,
> >>> +     .open = psci_debugfs_open,
> >>> +     .release = single_release,
> >>> +     .read = seq_read,
> >>> +     .llseek = seq_lseek
> >>> +};
> >>> +
> >>> +static int __init psci_debugfs_init(void)
> >>> +{
> >>> +     return PTR_ERR_OR_ZERO(debugfs_create_file("psci", S_IRUGO, NULL, NULL,
> >>> +                                                &psci_debugfs_ops));
> >>> +}
> >>> +late_initcall(psci_debugfs_init)
> >>> +#endif
> >>> +
> >>>    #ifdef CONFIG_CPU_IDLE
> >>>    static int psci_suspend_finisher(unsigned long state)
> >>>    {
> >>> diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h
> >>> index 2bf93c0d6354..f6f0bad5858b 100644
> >>> --- a/include/uapi/linux/psci.h
> >>> +++ b/include/uapi/linux/psci.h
> >>> @@ -48,11 +48,20 @@
> >>>    #define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU   PSCI_0_2_FN64(7)
> >>>
> >>>    #define PSCI_1_0_FN_PSCI_FEATURES           PSCI_0_2_FN(10)
> >>> +#define PSCI_1_0_FN_CPU_FREEZE                       PSCI_0_2_FN(11)
> >>> +#define PSCI_1_0_FN_CPU_DEFAULT_SUSPEND              PSCI_0_2_FN(12)
> >>> +#define PSCI_1_0_FN_NODE_HW_STATE            PSCI_0_2_FN(13)
> >>>    #define PSCI_1_0_FN_SYSTEM_SUSPEND          PSCI_0_2_FN(14)
> >>>    #define PSCI_1_0_FN_SET_SUSPEND_MODE                PSCI_0_2_FN(15)
> >>> +#define PSCI_1_0_FN_STAT_RESIDENCY           PSCI_0_2_FN(16)
> >>> +#define PSCI_1_0_FN_STAT_COUNT                       PSCI_0_2_FN(17)
> >>>    #define PSCI_1_1_FN_SYSTEM_RESET2           PSCI_0_2_FN(18)
> >>>
> >>> +#define PSCI_1_0_FN64_CPU_DEFAULT_SUSPEND    PSCI_0_2_FN64(12)
> >>> +#define PSCI_1_0_FN64_NODE_HW_STATE          PSCI_0_2_FN64(13)
> >>>    #define PSCI_1_0_FN64_SYSTEM_SUSPEND                PSCI_0_2_FN64(14)
> >>> +#define PSCI_1_0_FN64_STAT_RESIDENCY         PSCI_0_2_FN64(16)
> >>> +#define PSCI_1_0_FN64_STAT_COUNT             PSCI_0_2_FN64(17)
> >>>    #define PSCI_1_1_FN64_SYSTEM_RESET2         PSCI_0_2_FN64(18)
> >>>
> >>>    /* PSCI v0.2 power state encoding for CPU_SUSPEND function */
> >
> >
> >



-- 
With best wishes
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware/psci: Add debugfs support to ease debugging

[Bhupesh Sharma]

On 7/28/22 2:26 AM, Dmitry Baryshkov wrote:
> On Wed, 27 Jul 2022 at 23:55, Bhupesh Sharma <bhupesh.sharma@linaro.org> wrote:
>>
>>
>>
>> On 7/28/22 2:23 AM, Dmitry Baryshkov wrote:
>>> On Wed, 27 Jul 2022 at 23:15, Bhupesh Sharma <bhupesh.sharma@linaro.org> wrote:
>>>>
>>>> Hi Dmitry,
>>>>
>>>> On 7/28/22 1:39 AM, Dmitry Baryshkov wrote:
>>>>> To ease debugging of PSCI supported features, add debugfs file called
>>>>> 'psci' describing PSCI and SMC CC versions, enabled features and
>>>>> options.
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>     drivers/firmware/psci/psci.c | 112 ++++++++++++++++++++++++++++++++++-
>>>>>     include/uapi/linux/psci.h    |   9 +++
>>>>>     2 files changed, 120 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
>>>>> index b907768eea01..6595cc964635 100644
>>>>> --- a/drivers/firmware/psci/psci.c
>>>>> +++ b/drivers/firmware/psci/psci.c
>>>>> @@ -9,6 +9,7 @@
>>>>>     #include <linux/acpi.h>
>>>>>     #include <linux/arm-smccc.h>
>>>>>     #include <linux/cpuidle.h>
>>>>> +#include <linux/debugfs.h>
>>>>>     #include <linux/errno.h>
>>>>>     #include <linux/linkage.h>
>>>>>     #include <linux/of.h>
>>>>> @@ -324,12 +325,121 @@ static void psci_sys_poweroff(void)
>>>>>         invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
>>>>>     }
>>>>>
>>>>> -static int __init psci_features(u32 psci_func_id)
>>>>> +static int psci_features(u32 psci_func_id)
>>>>
>>>> This change doesn't seem related to the patch $SUBJECT.
>>>> Also is it really needed? If yes, probably this should be a separate patch.
>>>
>>> It is related and I don't think it should be moved to a separate
>>> patch. Removing the __init annotation from psci_features() is
>>> necessary to allow using psci_features() from psci_debufs_read(),
>>> which is definitely not an __init code. Otherwise reading from
>>> debugfs/psci would cause null pointer exceptions.
>>
>> Ok, and what is the behavior with CONFIG_DEBUG_FS = n?
>> Does your patch work well in that case?
> 
> Yes. Any particular reasons for the question?

Your debugfs changes in this patch are protected with CONFIG_DEBUG_FS,
while the  __init code change is not.

So, IMO its not really needed if CONFIG_DEBUG_FS is set to =n (hence
probably needs to be a separate patch).

Thanks.

>>>>>     {
>>>>>         return invoke_psci_fn(PSCI_1_0_FN_PSCI_FEATURES,
>>>>>                               psci_func_id, 0, 0);
>>>>>     }
>>>>>
>>>>> +#ifdef CONFIG_DEBUG_FS
>>>>> +
>>>>> +#define PSCI_ID(ver, _name) \
>>>>> +     { .fn = PSCI_##ver##_FN_##_name, .name = #_name, }
>>>>> +#define PSCI_ID_NATIVE(ver, _name) \
>>>>> +     { .fn = PSCI_FN_NATIVE(ver, _name), .name = #_name, }
>>>>> +
>>>>> +/* A table of all optional functions */
>>>>> +static const struct {
>>>>> +     u32 fn;
>>>>> +     const char *name;
>>>>> +} psci_fn_ids[] = {
>>>>> +     PSCI_ID_NATIVE(0_2, MIGRATE),
>>>>> +     PSCI_ID(0_2, MIGRATE_INFO_TYPE),
>>>>> +     PSCI_ID_NATIVE(0_2, MIGRATE_INFO_UP_CPU),
>>>>> +     PSCI_ID(1_0, CPU_FREEZE),
>>>>> +     PSCI_ID_NATIVE(1_0, CPU_DEFAULT_SUSPEND),
>>>>> +     PSCI_ID_NATIVE(1_0, NODE_HW_STATE),
>>>>> +     PSCI_ID_NATIVE(1_0, SYSTEM_SUSPEND),
>>>>> +     PSCI_ID(1_0, SET_SUSPEND_MODE),
>>>>> +     PSCI_ID_NATIVE(1_0, STAT_RESIDENCY),
>>>>> +     PSCI_ID_NATIVE(1_0, STAT_COUNT),
>>>>> +     PSCI_ID_NATIVE(1_1, SYSTEM_RESET2),
>>>>> +};
>>>>> +
>>>>> +static int psci_debugfs_read(struct seq_file *s, void *data)
>>>>> +{
>>>>> +     int feature, type, i;
>>>>> +     u32 ver;
>>>>> +
>>>>> +     ver = psci_ops.get_version();
>>>>> +     seq_printf(s, "PSCIv%d.%d\n",
>>>>> +                PSCI_VERSION_MAJOR(ver),
>>>>> +                PSCI_VERSION_MINOR(ver));
>>>>> +
>>>>> +     /* PSCI_FEATURES is available only starting from 1.0 */
>>>>> +     if (PSCI_VERSION_MAJOR(ver) < 1)
>>>>> +             return 0;
>>>>> +
>>>>> +     feature = psci_features(ARM_SMCCC_VERSION_FUNC_ID);
>>>>> +     if (feature != PSCI_RET_NOT_SUPPORTED) {
>>>>> +             ver = invoke_psci_fn(ARM_SMCCC_VERSION_FUNC_ID, 0, 0, 0);
>>>>> +             seq_printf(s, "SMC Calling Convention v%d.%d\n",
>>>>> +                        PSCI_VERSION_MAJOR(ver),
>>>>> +                        PSCI_VERSION_MINOR(ver));
>>>>> +     } else {
>>>>> +             seq_printf(s, "SMC Calling Convention v1.0 is assumed\n");
>>>>> +     }
>>>>> +
>>>>> +     feature = psci_features(PSCI_FN_NATIVE(0_2, CPU_SUSPEND));
>>>>> +     if (feature < 0) {
>>>>> +             seq_printf(s, "PSCI_FEATURES(CPU_SUSPEND) error (%d)\n", feature);
>>>>> +     } else {
>>>>> +             seq_printf(s, "OSI is %ssupported\n",
>>>>> +                        (feature & BIT(0)) ? "" : "not ");
>>>>> +             seq_printf(s, "%s StateID format is used\n",
>>>>> +                        (feature & BIT(1)) ? "Extended" : "Original");
>>>>> +     }
>>>>> +
>>>>> +     type = psci_ops.migrate_info_type();
>>>>> +     if (type == PSCI_0_2_TOS_UP_MIGRATE ||
>>>>> +         type == PSCI_0_2_TOS_UP_NO_MIGRATE) {
>>>>> +             unsigned long cpuid;
>>>>> +
>>>>> +             seq_printf(s, "Trusted OS %smigrate capable\n",
>>>>> +                        type == PSCI_0_2_TOS_UP_NO_MIGRATE ? "not " : "");
>>>>> +             cpuid = psci_migrate_info_up_cpu();
>>>>> +             seq_printf(s, "Trusted OS resident on physical CPU 0x%lx (#%d)\n", cpuid, resident_cpu);
>>>>> +     } else if (type == PSCI_0_2_TOS_MP) {
>>>>> +             seq_printf(s, "Trusted OS migration not required\n");
>>>>> +     } else {
>>>>> +             if (type != PSCI_RET_NOT_SUPPORTED)
>>>>> +                     seq_printf(s, "MIGRATE_INFO_TYPE returned unknown type (%d)\n", type);
>>>>> +     }
>>>>> +
>>>>> +     for (i = 0; i < ARRAY_SIZE(psci_fn_ids); i++) {
>>>>> +             feature = psci_features(psci_fn_ids[i].fn);
>>>>> +             if (feature == PSCI_RET_NOT_SUPPORTED)
>>>>> +                     continue;
>>>>> +             if (feature < 0)
>>>>> +                     seq_printf(s, "PSCI_FEATURES(%s) error (%d)\n", psci_fn_ids[i].name, feature);
>>>>> +             else
>>>>> +                     seq_printf(s, "%s is supported\n", psci_fn_ids[i].name);
>>>>> +     }
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static int psci_debugfs_open(struct inode *inode, struct file *f)
>>>>> +{
>>>>> +     return single_open(f, psci_debugfs_read, NULL);
>>>>> +}
>>>>> +
>>>>> +static const struct file_operations psci_debugfs_ops = {
>>>>> +     .owner = THIS_MODULE,
>>>>> +     .open = psci_debugfs_open,
>>>>> +     .release = single_release,
>>>>> +     .read = seq_read,
>>>>> +     .llseek = seq_lseek
>>>>> +};
>>>>> +
>>>>> +static int __init psci_debugfs_init(void)
>>>>> +{
>>>>> +     return PTR_ERR_OR_ZERO(debugfs_create_file("psci", S_IRUGO, NULL, NULL,
>>>>> +                                                &psci_debugfs_ops));
>>>>> +}
>>>>> +late_initcall(psci_debugfs_init)
>>>>> +#endif
>>>>> +
>>>>>     #ifdef CONFIG_CPU_IDLE
>>>>>     static int psci_suspend_finisher(unsigned long state)
>>>>>     {
>>>>> diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h
>>>>> index 2bf93c0d6354..f6f0bad5858b 100644
>>>>> --- a/include/uapi/linux/psci.h
>>>>> +++ b/include/uapi/linux/psci.h
>>>>> @@ -48,11 +48,20 @@
>>>>>     #define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU   PSCI_0_2_FN64(7)
>>>>>
>>>>>     #define PSCI_1_0_FN_PSCI_FEATURES           PSCI_0_2_FN(10)
>>>>> +#define PSCI_1_0_FN_CPU_FREEZE                       PSCI_0_2_FN(11)
>>>>> +#define PSCI_1_0_FN_CPU_DEFAULT_SUSPEND              PSCI_0_2_FN(12)
>>>>> +#define PSCI_1_0_FN_NODE_HW_STATE            PSCI_0_2_FN(13)
>>>>>     #define PSCI_1_0_FN_SYSTEM_SUSPEND          PSCI_0_2_FN(14)
>>>>>     #define PSCI_1_0_FN_SET_SUSPEND_MODE                PSCI_0_2_FN(15)
>>>>> +#define PSCI_1_0_FN_STAT_RESIDENCY           PSCI_0_2_FN(16)
>>>>> +#define PSCI_1_0_FN_STAT_COUNT                       PSCI_0_2_FN(17)
>>>>>     #define PSCI_1_1_FN_SYSTEM_RESET2           PSCI_0_2_FN(18)
>>>>>
>>>>> +#define PSCI_1_0_FN64_CPU_DEFAULT_SUSPEND    PSCI_0_2_FN64(12)
>>>>> +#define PSCI_1_0_FN64_NODE_HW_STATE          PSCI_0_2_FN64(13)
>>>>>     #define PSCI_1_0_FN64_SYSTEM_SUSPEND                PSCI_0_2_FN64(14)
>>>>> +#define PSCI_1_0_FN64_STAT_RESIDENCY         PSCI_0_2_FN64(16)
>>>>> +#define PSCI_1_0_FN64_STAT_COUNT             PSCI_0_2_FN64(17)
>>>>>     #define PSCI_1_1_FN64_SYSTEM_RESET2         PSCI_0_2_FN64(18)
>>>>>
>>>>>     /* PSCI v0.2 power state encoding for CPU_SUSPEND function */
>>>
>>>
>>>
> 
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware/psci: Add debugfs support to ease debugging

[Dmitry Baryshkov]

On Wed, 27 Jul 2022 at 23:59, Bhupesh Sharma <bhupesh.sharma@linaro.org> wrote:
>
>
>
> On 7/28/22 2:26 AM, Dmitry Baryshkov wrote:
> > On Wed, 27 Jul 2022 at 23:55, Bhupesh Sharma <bhupesh.sharma@linaro.org> wrote:
> >>
> >>
> >>
> >> On 7/28/22 2:23 AM, Dmitry Baryshkov wrote:
> >>> On Wed, 27 Jul 2022 at 23:15, Bhupesh Sharma <bhupesh.sharma@linaro.org> wrote:
> >>>>
> >>>> Hi Dmitry,
> >>>>
> >>>> On 7/28/22 1:39 AM, Dmitry Baryshkov wrote:
> >>>>> To ease debugging of PSCI supported features, add debugfs file called
> >>>>> 'psci' describing PSCI and SMC CC versions, enabled features and
> >>>>> options.
> >>>>>
> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>>> ---
> >>>>>     drivers/firmware/psci/psci.c | 112 ++++++++++++++++++++++++++++++++++-
> >>>>>     include/uapi/linux/psci.h    |   9 +++
> >>>>>     2 files changed, 120 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> >>>>> index b907768eea01..6595cc964635 100644
> >>>>> --- a/drivers/firmware/psci/psci.c
> >>>>> +++ b/drivers/firmware/psci/psci.c
> >>>>> @@ -9,6 +9,7 @@
> >>>>>     #include <linux/acpi.h>
> >>>>>     #include <linux/arm-smccc.h>
> >>>>>     #include <linux/cpuidle.h>
> >>>>> +#include <linux/debugfs.h>
> >>>>>     #include <linux/errno.h>
> >>>>>     #include <linux/linkage.h>
> >>>>>     #include <linux/of.h>
> >>>>> @@ -324,12 +325,121 @@ static void psci_sys_poweroff(void)
> >>>>>         invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> >>>>>     }
> >>>>>
> >>>>> -static int __init psci_features(u32 psci_func_id)
> >>>>> +static int psci_features(u32 psci_func_id)
> >>>>
> >>>> This change doesn't seem related to the patch $SUBJECT.
> >>>> Also is it really needed? If yes, probably this should be a separate patch.
> >>>
> >>> It is related and I don't think it should be moved to a separate
> >>> patch. Removing the __init annotation from psci_features() is
> >>> necessary to allow using psci_features() from psci_debufs_read(),
> >>> which is definitely not an __init code. Otherwise reading from
> >>> debugfs/psci would cause null pointer exceptions.
> >>
> >> Ok, and what is the behavior with CONFIG_DEBUG_FS = n?
> >> Does your patch work well in that case?
> >
> > Yes. Any particular reasons for the question?
>
> Your debugfs changes in this patch are protected with CONFIG_DEBUG_FS,
> while the  __init code change is not.

Yes. I'm _removing_ the __init. Making the function available after
kernel frees the __init memory. I'd have understood your questions if
I were making an opposite change, marking the function with __init.
But in this case I doubt it makes any difference.

> So, IMO its not really needed if CONFIG_DEBUG_FS is set to =n (hence
> probably needs to be a separate patch).

An overhead is pretty minimal. And all the troubles to make __init
annotation depend on CONFIG_DEBUG_FS overweight this overhead.

>
> Thanks.
>
> >>>>>     {
> >>>>>         return invoke_psci_fn(PSCI_1_0_FN_PSCI_FEATURES,
> >>>>>                               psci_func_id, 0, 0);
> >>>>>     }
> >>>>>
> >>>>> +#ifdef CONFIG_DEBUG_FS
> >>>>> +
> >>>>> +#define PSCI_ID(ver, _name) \
> >>>>> +     { .fn = PSCI_##ver##_FN_##_name, .name = #_name, }
> >>>>> +#define PSCI_ID_NATIVE(ver, _name) \
> >>>>> +     { .fn = PSCI_FN_NATIVE(ver, _name), .name = #_name, }
> >>>>> +
> >>>>> +/* A table of all optional functions */
> >>>>> +static const struct {
> >>>>> +     u32 fn;
> >>>>> +     const char *name;
> >>>>> +} psci_fn_ids[] = {
> >>>>> +     PSCI_ID_NATIVE(0_2, MIGRATE),
> >>>>> +     PSCI_ID(0_2, MIGRATE_INFO_TYPE),
> >>>>> +     PSCI_ID_NATIVE(0_2, MIGRATE_INFO_UP_CPU),
> >>>>> +     PSCI_ID(1_0, CPU_FREEZE),
> >>>>> +     PSCI_ID_NATIVE(1_0, CPU_DEFAULT_SUSPEND),
> >>>>> +     PSCI_ID_NATIVE(1_0, NODE_HW_STATE),
> >>>>> +     PSCI_ID_NATIVE(1_0, SYSTEM_SUSPEND),
> >>>>> +     PSCI_ID(1_0, SET_SUSPEND_MODE),
> >>>>> +     PSCI_ID_NATIVE(1_0, STAT_RESIDENCY),
> >>>>> +     PSCI_ID_NATIVE(1_0, STAT_COUNT),
> >>>>> +     PSCI_ID_NATIVE(1_1, SYSTEM_RESET2),
> >>>>> +};
> >>>>> +
> >>>>> +static int psci_debugfs_read(struct seq_file *s, void *data)
> >>>>> +{
> >>>>> +     int feature, type, i;
> >>>>> +     u32 ver;
> >>>>> +
> >>>>> +     ver = psci_ops.get_version();
> >>>>> +     seq_printf(s, "PSCIv%d.%d\n",
> >>>>> +                PSCI_VERSION_MAJOR(ver),
> >>>>> +                PSCI_VERSION_MINOR(ver));
> >>>>> +
> >>>>> +     /* PSCI_FEATURES is available only starting from 1.0 */
> >>>>> +     if (PSCI_VERSION_MAJOR(ver) < 1)
> >>>>> +             return 0;
> >>>>> +
> >>>>> +     feature = psci_features(ARM_SMCCC_VERSION_FUNC_ID);
> >>>>> +     if (feature != PSCI_RET_NOT_SUPPORTED) {
> >>>>> +             ver = invoke_psci_fn(ARM_SMCCC_VERSION_FUNC_ID, 0, 0, 0);
> >>>>> +             seq_printf(s, "SMC Calling Convention v%d.%d\n",
> >>>>> +                        PSCI_VERSION_MAJOR(ver),
> >>>>> +                        PSCI_VERSION_MINOR(ver));
> >>>>> +     } else {
> >>>>> +             seq_printf(s, "SMC Calling Convention v1.0 is assumed\n");
> >>>>> +     }
> >>>>> +
> >>>>> +     feature = psci_features(PSCI_FN_NATIVE(0_2, CPU_SUSPEND));
> >>>>> +     if (feature < 0) {
> >>>>> +             seq_printf(s, "PSCI_FEATURES(CPU_SUSPEND) error (%d)\n", feature);
> >>>>> +     } else {
> >>>>> +             seq_printf(s, "OSI is %ssupported\n",
> >>>>> +                        (feature & BIT(0)) ? "" : "not ");
> >>>>> +             seq_printf(s, "%s StateID format is used\n",
> >>>>> +                        (feature & BIT(1)) ? "Extended" : "Original");
> >>>>> +     }
> >>>>> +
> >>>>> +     type = psci_ops.migrate_info_type();
> >>>>> +     if (type == PSCI_0_2_TOS_UP_MIGRATE ||
> >>>>> +         type == PSCI_0_2_TOS_UP_NO_MIGRATE) {
> >>>>> +             unsigned long cpuid;
> >>>>> +
> >>>>> +             seq_printf(s, "Trusted OS %smigrate capable\n",
> >>>>> +                        type == PSCI_0_2_TOS_UP_NO_MIGRATE ? "not " : "");
> >>>>> +             cpuid = psci_migrate_info_up_cpu();
> >>>>> +             seq_printf(s, "Trusted OS resident on physical CPU 0x%lx (#%d)\n", cpuid, resident_cpu);
> >>>>> +     } else if (type == PSCI_0_2_TOS_MP) {
> >>>>> +             seq_printf(s, "Trusted OS migration not required\n");
> >>>>> +     } else {
> >>>>> +             if (type != PSCI_RET_NOT_SUPPORTED)
> >>>>> +                     seq_printf(s, "MIGRATE_INFO_TYPE returned unknown type (%d)\n", type);
> >>>>> +     }
> >>>>> +
> >>>>> +     for (i = 0; i < ARRAY_SIZE(psci_fn_ids); i++) {
> >>>>> +             feature = psci_features(psci_fn_ids[i].fn);
> >>>>> +             if (feature == PSCI_RET_NOT_SUPPORTED)
> >>>>> +                     continue;
> >>>>> +             if (feature < 0)
> >>>>> +                     seq_printf(s, "PSCI_FEATURES(%s) error (%d)\n", psci_fn_ids[i].name, feature);
> >>>>> +             else
> >>>>> +                     seq_printf(s, "%s is supported\n", psci_fn_ids[i].name);
> >>>>> +     }
> >>>>> +
> >>>>> +     return 0;
> >>>>> +}
> >>>>> +
> >>>>> +static int psci_debugfs_open(struct inode *inode, struct file *f)
> >>>>> +{
> >>>>> +     return single_open(f, psci_debugfs_read, NULL);
> >>>>> +}
> >>>>> +
> >>>>> +static const struct file_operations psci_debugfs_ops = {
> >>>>> +     .owner = THIS_MODULE,
> >>>>> +     .open = psci_debugfs_open,
> >>>>> +     .release = single_release,
> >>>>> +     .read = seq_read,
> >>>>> +     .llseek = seq_lseek
> >>>>> +};
> >>>>> +
> >>>>> +static int __init psci_debugfs_init(void)
> >>>>> +{
> >>>>> +     return PTR_ERR_OR_ZERO(debugfs_create_file("psci", S_IRUGO, NULL, NULL,
> >>>>> +                                                &psci_debugfs_ops));
> >>>>> +}
> >>>>> +late_initcall(psci_debugfs_init)
> >>>>> +#endif
> >>>>> +
> >>>>>     #ifdef CONFIG_CPU_IDLE
> >>>>>     static int psci_suspend_finisher(unsigned long state)
> >>>>>     {
> >>>>> diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h
> >>>>> index 2bf93c0d6354..f6f0bad5858b 100644
> >>>>> --- a/include/uapi/linux/psci.h
> >>>>> +++ b/include/uapi/linux/psci.h
> >>>>> @@ -48,11 +48,20 @@
> >>>>>     #define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU   PSCI_0_2_FN64(7)
> >>>>>
> >>>>>     #define PSCI_1_0_FN_PSCI_FEATURES           PSCI_0_2_FN(10)
> >>>>> +#define PSCI_1_0_FN_CPU_FREEZE                       PSCI_0_2_FN(11)
> >>>>> +#define PSCI_1_0_FN_CPU_DEFAULT_SUSPEND              PSCI_0_2_FN(12)
> >>>>> +#define PSCI_1_0_FN_NODE_HW_STATE            PSCI_0_2_FN(13)
> >>>>>     #define PSCI_1_0_FN_SYSTEM_SUSPEND          PSCI_0_2_FN(14)
> >>>>>     #define PSCI_1_0_FN_SET_SUSPEND_MODE                PSCI_0_2_FN(15)
> >>>>> +#define PSCI_1_0_FN_STAT_RESIDENCY           PSCI_0_2_FN(16)
> >>>>> +#define PSCI_1_0_FN_STAT_COUNT                       PSCI_0_2_FN(17)
> >>>>>     #define PSCI_1_1_FN_SYSTEM_RESET2           PSCI_0_2_FN(18)
> >>>>>
> >>>>> +#define PSCI_1_0_FN64_CPU_DEFAULT_SUSPEND    PSCI_0_2_FN64(12)
> >>>>> +#define PSCI_1_0_FN64_NODE_HW_STATE          PSCI_0_2_FN64(13)
> >>>>>     #define PSCI_1_0_FN64_SYSTEM_SUSPEND                PSCI_0_2_FN64(14)
> >>>>> +#define PSCI_1_0_FN64_STAT_RESIDENCY         PSCI_0_2_FN64(16)
> >>>>> +#define PSCI_1_0_FN64_STAT_COUNT             PSCI_0_2_FN64(17)
> >>>>>     #define PSCI_1_1_FN64_SYSTEM_RESET2         PSCI_0_2_FN64(18)
> >>>>>
> >>>>>     /* PSCI v0.2 power state encoding for CPU_SUSPEND function */
> >>>
> >>>
> >>>
> >
> >
> >



-- 
With best wishes
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware/psci: Add debugfs support to ease debugging

[Sudeep Holla]

On Wed, Jul 27, 2022 at 11:09:01PM +0300, Dmitry Baryshkov wrote:
> To ease debugging of PSCI supported features, add debugfs file called
> 'psci' describing PSCI and SMC CC versions

These 2 are for sure in the boot log. Having them is debugfs accessible
via file system add not much value as we would hit issues quite early in
the boot for most of the things related to PSCI.

> enabled features and options.
> 

We have psci_checker.c which does some minimal testing of PSCI. I prefer
to add things to that rather than a debugfs as it is run during boot. IMO
it is usual useful to debug things that cause boot issue most of the time.
I am not against this so I will leave it to the maintainers.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware/psci: Add debugfs support to ease debugging

[Dmitry Baryshkov]

On Thu, 28 Jul 2022 at 12:08, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Jul 27, 2022 at 11:09:01PM +0300, Dmitry Baryshkov wrote:
> > To ease debugging of PSCI supported features, add debugfs file called
> > 'psci' describing PSCI and SMC CC versions
>
> These 2 are for sure in the boot log. Having them is debugfs accessible
> via file system add not much value as we would hit issues quite early in
> the boot for most of the things related to PSCI.

Yes, it was just to have all the information in a single place.

> > enabled features and options.
> >
>
> We have psci_checker.c which does some minimal testing of PSCI. I prefer
> to add things to that rather than a debugfs as it is run during boot. IMO
> it is usual useful to debug things that cause boot issue most of the time.
> I am not against this so I will leave it to the maintainers.

In my case I was not debugging the boot issues (which of course would
have required a different approach), but I was trying to understand
runtime capabilities, thus debugfs fits pretty well.

Another point for the debugfs entry: most of the people run the kernel
with the psci_checker being turned off, but with debugfs being
enabled. If we are trying to narrow down firmware capabilities of the
random device, it is much easier to ask them to cat the dbeugfs file
rather than to rebuild the kernel.

-- 
With best wishes
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware/psci: Add debugfs support to ease debugging

[Florian Fainelli]

On 7/28/22 02:20, Dmitry Baryshkov wrote:
> On Thu, 28 Jul 2022 at 12:08, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>> On Wed, Jul 27, 2022 at 11:09:01PM +0300, Dmitry Baryshkov wrote:
>>> To ease debugging of PSCI supported features, add debugfs file called
>>> 'psci' describing PSCI and SMC CC versions
>>
>> These 2 are for sure in the boot log. Having them is debugfs accessible
>> via file system add not much value as we would hit issues quite early in
>> the boot for most of the things related to PSCI.
> 
> Yes, it was just to have all the information in a single place.
> 
>>> enabled features and options.
>>>
>>
>> We have psci_checker.c which does some minimal testing of PSCI. I prefer
>> to add things to that rather than a debugfs as it is run during boot. IMO
>> it is usual useful to debug things that cause boot issue most of the time.
>> I am not against this so I will leave it to the maintainers.
> 
> In my case I was not debugging the boot issues (which of course would
> have required a different approach), but I was trying to understand
> runtime capabilities, thus debugfs fits pretty well.
> 
> Another point for the debugfs entry: most of the people run the kernel
> with the psci_checker being turned off, but with debugfs being
> enabled. If we are trying to narrow down firmware capabilities of the
> random device, it is much easier to ask them to cat the dbeugfs file
> rather than to rebuild the kernel.
> 

Yes I would agree with both of those points, in fact, I would go one step further and add the ability to probe an arbitrary PSCI function ID, since deployed firmware typically go beyond the standard PSCI scope and implement a variety of custom extensions (at least we do).

Thanks!
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware/psci: Add debugfs support to ease debugging

[Sudeep Holla]

On Fri, Jul 29, 2022 at 11:45:10AM -0700, Florian Fainelli wrote:
>
> Yes I would agree with both of those points, in fact, I would go one step
> further and add the ability to probe an arbitrary PSCI function ID, since
> deployed firmware typically go beyond the standard PSCI scope and implement
> a variety of custom extensions (at least we do).
>

Technically they must not be PSCI right ? Also I hope they are not using
Arm Architecture/CPU service range of FID and they use SiP/OEM service
range. From what I understand, you need a generic debug FS for all
SMCCC instead of just PSCI in your case. The latter must cover PSCI as
well while the reverse must not happen.

So if we need this beyond PSCI FID range, better you have it as generic
SMCCC debug FS. Thoughts ?

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware/psci: Add debugfs support to ease debugging

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 455 bytes --]

On Mon, Aug 01, 2022 at 10:59:10AM +0100, Sudeep Holla wrote:

> So if we need this beyond PSCI FID range, better you have it as generic
> SMCCC debug FS. Thoughts ?

That thought did cross my mind when reviewing Dmitry's patch but given
that as far as I'm aware SMCCC isn't particularly enumerable it seemed
like it might be more of a small library of helpers than something you
could write a general structure for.  I might be missing something
though.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware/psci: Add debugfs support to ease debugging

[Sudeep Holla]

On Mon, Aug 01, 2022 at 01:14:25PM +0100, Mark Brown wrote:
> On Mon, Aug 01, 2022 at 10:59:10AM +0100, Sudeep Holla wrote:
> 
> > So if we need this beyond PSCI FID range, better you have it as generic
> > SMCCC debug FS. Thoughts ?
> 
> That thought did cross my mind when reviewing Dmitry's patch but given
> that as far as I'm aware SMCCC isn't particularly enumerable it seemed
> like it might be more of a small library of helpers than something you
> could write a general structure for.  I might be missing something
> though.

Agreed. I don't know how feasible in that yet, but would like to know
Florian's thoughts/requirements to give it a thought. I don't think you
are missing anything. One thing I thought is to add SMCCC version as well
if we are adding PSCI version as lots of new additions have happened
and it is good to have info IMO. SMCCC_ARCH_FEATURES query APIs might be
useful. This is just initial thoughts.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware/psci: Add debugfs support to ease debugging

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 748 bytes --]

On Thu, Jul 28, 2022 at 10:08:06AM +0100, Sudeep Holla wrote:
> On Wed, Jul 27, 2022 at 11:09:01PM +0300, Dmitry Baryshkov wrote:

> > To ease debugging of PSCI supported features, add debugfs file called
> > 'psci' describing PSCI and SMC CC versions

> These 2 are for sure in the boot log. Having them is debugfs accessible
> via file system add not much value as we would hit issues quite early in
> the boot for most of the things related to PSCI.

It can be useful to have something that can be queried at any point when
collecting diagnostics, even if there's been a lot of logging or log
rotation since boot.  It makes it easier to give people instructions or
a tool which will reliably collect useful information when filing a bug
report.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware/psci: Add debugfs support to ease debugging

[Sudeep Holla]

On Thu, Jul 28, 2022 at 02:05:56PM +0100, Mark Brown wrote:
> On Thu, Jul 28, 2022 at 10:08:06AM +0100, Sudeep Holla wrote:
> > On Wed, Jul 27, 2022 at 11:09:01PM +0300, Dmitry Baryshkov wrote:
> 
> > > To ease debugging of PSCI supported features, add debugfs file called
> > > 'psci' describing PSCI and SMC CC versions
> 
> > These 2 are for sure in the boot log. Having them is debugfs accessible
> > via file system add not much value as we would hit issues quite early in
> > the boot for most of the things related to PSCI.
> 
> It can be useful to have something that can be queried at any point when
> collecting diagnostics, even if there's been a lot of logging or log
> rotation since boot.  It makes it easier to give people instructions or
> a tool which will reliably collect useful information when filing a bug
> report.

Fair enough. I agree this is will be useful to debug/analyse non boot
issues.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware/psci: Add debugfs support to ease debugging

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1191 bytes --]

On Wed, Jul 27, 2022 at 11:09:01PM +0300, Dmitry Baryshkov wrote:

> +} psci_fn_ids[] = {
> +	PSCI_ID_NATIVE(0_2, MIGRATE),
> +	PSCI_ID(0_2, MIGRATE_INFO_TYPE),
> +	PSCI_ID_NATIVE(0_2, MIGRATE_INFO_UP_CPU),
> +	PSCI_ID(1_0, CPU_FREEZE),
> +	PSCI_ID_NATIVE(1_0, CPU_DEFAULT_SUSPEND),
> +	PSCI_ID_NATIVE(1_0, NODE_HW_STATE),
> +	PSCI_ID_NATIVE(1_0, SYSTEM_SUSPEND),
> +	PSCI_ID(1_0, SET_SUSPEND_MODE),
> +	PSCI_ID_NATIVE(1_0, STAT_RESIDENCY),
> +	PSCI_ID_NATIVE(1_0, STAT_COUNT),
> +	PSCI_ID_NATIVE(1_1, SYSTEM_RESET2),
> +};

There's other functions like the MEM_PROTECT ones which we don't
currently use but it might be interesting to enumerate...

>  #define PSCI_1_0_FN_PSCI_FEATURES		PSCI_0_2_FN(10)
> +#define PSCI_1_0_FN_CPU_FREEZE			PSCI_0_2_FN(11)
> +#define PSCI_1_0_FN_CPU_DEFAULT_SUSPEND		PSCI_0_2_FN(12)

...we're already adding functions here.

> +#define PSCI_1_0_FN_NODE_HW_STATE		PSCI_0_2_FN(13)

> +#define PSCI_1_0_FN_STAT_RESIDENCY		PSCI_0_2_FN(16)
> +#define PSCI_1_0_FN_STAT_COUNT			PSCI_0_2_FN(17)

Some of these state query things might be interesting to actually call
and output results from at some point, doesn't seem like something that
should be a blocker though.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] firmware/psci: Add debugfs support to ease debugging

[Dmitry Baryshkov]

On Thu, 28 Jul 2022 at 16:38, Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Jul 27, 2022 at 11:09:01PM +0300, Dmitry Baryshkov wrote:
>
> > +} psci_fn_ids[] = {
> > +     PSCI_ID_NATIVE(0_2, MIGRATE),
> > +     PSCI_ID(0_2, MIGRATE_INFO_TYPE),
> > +     PSCI_ID_NATIVE(0_2, MIGRATE_INFO_UP_CPU),
> > +     PSCI_ID(1_0, CPU_FREEZE),
> > +     PSCI_ID_NATIVE(1_0, CPU_DEFAULT_SUSPEND),
> > +     PSCI_ID_NATIVE(1_0, NODE_HW_STATE),
> > +     PSCI_ID_NATIVE(1_0, SYSTEM_SUSPEND),
> > +     PSCI_ID(1_0, SET_SUSPEND_MODE),
> > +     PSCI_ID_NATIVE(1_0, STAT_RESIDENCY),
> > +     PSCI_ID_NATIVE(1_0, STAT_COUNT),
> > +     PSCI_ID_NATIVE(1_1, SYSTEM_RESET2),
> > +};
>
> There's other functions like the MEM_PROTECT ones which we don't
> currently use but it might be interesting to enumerate...

Argh, missed the next page in PSCI spec. Will fix in v2.

>
> >  #define PSCI_1_0_FN_PSCI_FEATURES            PSCI_0_2_FN(10)
> > +#define PSCI_1_0_FN_CPU_FREEZE                       PSCI_0_2_FN(11)
> > +#define PSCI_1_0_FN_CPU_DEFAULT_SUSPEND              PSCI_0_2_FN(12)
>
> ...we're already adding functions here.
>
> > +#define PSCI_1_0_FN_NODE_HW_STATE            PSCI_0_2_FN(13)
>
> > +#define PSCI_1_0_FN_STAT_RESIDENCY           PSCI_0_2_FN(16)
> > +#define PSCI_1_0_FN_STAT_COUNT                       PSCI_0_2_FN(17)
>
> Some of these state query things might be interesting to actually call
> and output results from at some point, doesn't seem like something that
> should be a blocker though.

I thought about it, but deferred for now.

-- 
With best wishes
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel