soc: qcom: Introduce subsystem sleep stats driver
Subsystems (2)
PATCH v2 1/1 soc: qcom: Introduce subsystem sleep stats driver
Multiple subsystems like modem, spss, adsp, cdsp present on Qualcomm Technologies Inc's (QTI) SoCs maintains low power mode statistics in shared memory (SMEM). Lets add a driver to read and display this information using sysfs.
---
Changes in v2:
- Correct Makefile to use QCOM_SS_SLEEP_STATS config
--- Documentation/ABI/testing/sysfs-power | 10 ++ drivers/soc/qcom/Kconfig | 9 ++ drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/subsystem_sleep_stats.c | 146 +++++++++++++++++++++++ 4 files changed, 166 insertions(+) create mode 100644 drivers/soc/qcom/subsystem_sleep_stats.c
Documentation/ABI/testing/sysfs-power +10 -0
@@ -288,6 +288,16 @@ Description: writing a "0" (default) to it disables them. Reads from this file return the current value. +What: /sys/power/subsystem_sleep/stats +Date: December 2017 +Contact: Maulik Shah <[email protected]> +Description: + The /sys/power/subsystem_sleep/stats file prints the subsystem + sleep information on Qualcomm Technologies, Inc. (QTI) SoCs. + + Reading from this file will display subsystem level low power + mode statistics. + What: /sys/power/resume_offset Date: April 2018 Contact: Mario Limonciello <[email protected]>
drivers/soc/qcom/Kconfig +9 -0
@@ -163,6 +163,15 @@ config QCOM_SMSM Say yes here to support the Qualcomm Shared Memory State Machine. The state machine is represented by bits in shared memory. +config QCOM_SS_SLEEP_STATS + tristate "Qualcomm Technologies Inc. Subsystem Sleep Stats driver" + depends on QCOM_SMEM + help + Say y here to enable support for the Qualcomm Technologies Inc (QTI) + SS sleep stats driver to read the sleep stats of various subsystems + from SMEM. The stats are exported to sysfs. The driver also maintains + application processor sleep stats. + config QCOM_WCNSS_CTRL tristate "Qualcomm WCNSS control driver" depends on ARCH_QCOM || COMPILE_TEST
drivers/soc/qcom/Makefile +1 -0
@@ -17,6 +17,7 @@ obj-$(CONFIG_QCOM_SMEM) += smem.o obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o obj-$(CONFIG_QCOM_SMP2P) += smp2p.o obj-$(CONFIG_QCOM_SMSM) += smsm.o +obj-$(CONFIG_QCOM_SS_SLEEP_STATS) += subsystem_sleep_stats.o obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o obj-$(CONFIG_QCOM_APR) += apr.o obj-$(CONFIG_QCOM_LLCC) += llcc-slice.o
drivers/soc/qcom/subsystem_sleep_stats.c new file +146 -0
146 lines not on this page · raw message
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.
Quoting Maulik Shah (2019-09-05 02:17:07)
Quoted 22 lines
Multiple subsystems like modem, spss, adsp, cdsp present on Qualcomm Technologies Inc's (QTI) SoCs maintains low power mode statistics in shared memory (SMEM). Lets add a driver to read and display this information using sysfs.
Signed-off-by Maulik Shah <[email protected]>---
Changes in v2:
- CorrectMakefileto useQCOM_SS_SLEEP_STATSconfig--- Documentation/ABI/testing/sysfs-power | 10 ++ drivers/soc/qcom/Kconfig | 9 ++ drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/subsystem_sleep_stats.c | 146 +++++++++++++++++++++++ 4 files changed, 166 insertions(+) create mode 100644 drivers/soc/qcom/subsystem_sleep_stats.cDocumentation/ABI/testing/sysfs-power +0
-0
Documentation/ABI/testing/sysfs-power Description: · line 292
Quoted 5 lines
Documentation/ABI/testing/sysfs-power +2
-0@@ -288,6 +288,16 @@ Description: writing a "0" (default) to it disables them. Reads from this file return the current value. +What: /sys/power/subsystem_sleep/stats +Date: December 2017
It isn't December 2017.
Documentation/ABI/testing/sysfs-power Description: · line 300
Quoted 8 lines
Documentation/ABI/testing/sysfs-power +8
-0+Contact: Maulik Shah <[email protected]> +Description: + The /sys/power/subsystem_sleep/stats file prints the subsystem + sleep information on Qualcomm Technologies, Inc. (QTI) SoCs. + + Reading from this file will display subsystem level low power + mode statistics. +
This directory doesn't make any sense to me. It's in the top-level power directory and it is specific to qcom. Is this debugging information? Why does userspace care about understanding the sleep stats information? Please Cc Rafael on anything touching /sys/power/
Quoted 46 lines
Documentation/ABI/testing/sysfs-power +0
-0What: /sys/power/resume_offset Date: April 2018 Contact: Mario Limonciello <[email protected]>drivers/soc/qcom/subsystem_sleep_stats.c new file +37
-0@@ -0,0 +1,146 @@ +// SPDX-License-Identifier: GPL-2.0-only + +/* +* Copyright (c) 2017-2019, The Linux Foundation. All rights reserved. +*/ + +#define pr_fmt (fmt) "%s: " fmt, KBUILD_MODNAME + +#include <linux/errno.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/types.h> + +#include <linux/soc/qcom/smem.h> + +enum subsystem_item_id { +MODEM =605 , +ADSP , +CDSP , +SLPI , +GPU , +DISPLAY , +}; + +enum subsystem_pid { +PID_APSS =0 , +PID_MODEM =1 , +PID_ADSP =2 , +PID_SLPI =3 , +PID_CDSP =5 , +PID_GPU =PID_APSS , +PID_DISPLAY =PID_APSS , +}; + +struct subsystem_data { +char *name; +enum subsystem_item_id item_id;
drivers/soc/qcom/subsystem_sleep_stats.c line 47
Quoted 10 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +10
-0+enum subsystem_pid pid; +}; + +static const struct subsystem_data subsystems[] = { + {"MODEM" ,MODEM ,PID_MODEM }, + {"ADSP" ,ADSP ,PID_ADSP }, + {"CDSP" ,CDSP ,PID_CDSP }, + {"SLPI" ,SLPI ,PID_SLPI }, + {"GPU" ,GPU ,PID_GPU }, + {"DISPLAY" ,DISPLAY ,PID_DISPLAY },
Please put spaces around braces.
Quoted 19 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +19
-0+}; + +struct subsystem_stats { +uint32_t version_id; +uint32_t count; +uint64_t last_entered; +uint64_t last_exited; +uint64_t accumulated_duration; +}; + +struct subsystem_stats_prv_data { +struct kobj_attribute ka; +struct kobject *kobj; +}; + +static struct subsystem_stats_prv_data *prvdata; + +static inline ssize_t subsystem_stats_print(char *prvbuf, ssize_t length, + struct subsystem_stats *record,
drivers/soc/qcom/subsystem_sleep_stats.c line 76
Quoted 10 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +10
-0+ const char *name) +{ +return scnprintf (prvbuf, length,"%s\n\tVersion:0x%x\n" +"\tSleep Count:0x%x\n" +"\tSleep Last Entered At:0x%llx\n" +"\tSleep Last Exited At:0x%llx\n" +"\tSleep Accumulated Duration:0x%llx\n\n" , + name, record->version_id, record->count, + record->last_entered, record->last_exited, + record->accumulated_duration);
Information in sysfs is supposed to be one value per file. This is a bunch of different values and it includes a version field. Looks almost like something we would put into /proc, but of course that doesn't make any sense to put in /proc either.
Please rethink the whole approach here. Can this be placed under the remoteproc nodes for each remote processor that's in the system? That would make it more discoverable by userspace looking at the remoteproc devices. I suppose GPU and DISPLAY aren't "remoteproc"s though so maybe this should be a new 'class' for devices that have an RPMh RSC? Maybe make a qcom_rpmh_rsc class and then have these be stats in there.
Quoted 6 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +6
-0+} + +static ssize_t subsystem_stats_show (struct kobject *kobj, +struct kobj_attribute *attr,char *buf) +{ +ssize_t length =0 ;
drivers/soc/qcom/subsystem_sleep_stats.c line 92
Quoted 10 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +10
-0+int i =0 ; +size_t size =0 ; +struct subsystem_stats *record =NULL ; + +/* Read SMEM data written by other subsystems */ +for (i =0 ; i <ARRAY_SIZE (subsystems); i++) { + record = (struct subsystem_stats *)qcom_smem_get ( + subsystems[i].pid, subsystems[i].item_id, &size); + +if (!IS_ERR_OR_NULL (record) && (PAGE_SIZE - length >0 ))
It can return ERR pointer or NULL? Why?
Quoted 20 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +20
-0+ length +=subsystem_stats_print (buf + length, +PAGE_SIZE - length, + record, + subsystems[i].name); + } + +return length; +} + +static int __init subsystem_sleep_stats_init (void ) +{ +struct kobject *ss_stats_kobj; +int ret; + + prvdata =kmalloc (sizeof (*prvdata),GFP_KERNEL ); +if (!prvdata) +return -ENOMEM ; + + ss_stats_kobj =kobject_create_and_add ("subsystem_sleep" , + power_kobj);
drivers/soc/qcom/subsystem_sleep_stats.c line 122
Quoted 10 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +10
-0+if (!ss_stats_kobj) +return -ENOMEM ; + + prvdata->kobj = ss_stats_kobj; + +sysfs_attr_init (&prvdata->ka.attr); + prvdata->ka.attr.mode =0444 ; + prvdata->ka.attr.name ="stats" ; + prvdata->ka.show = subsystem_stats_show; + prvdata->ka.store =NULL ;
Does it need to be set to NULL explicitly? Why not kzalloc() prvdata above?
drivers/soc/qcom/subsystem_sleep_stats.c line 126
Quoted 4 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +4
-0+ + ret =sysfs_create_file (prvdata->kobj, &prvdata->ka.attr); +if (ret) { +pr_err ("sysfs_create_file failed\n" );
Seems useless. Presumably sysfs_create_file() can complain itself.
Quoted 6 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +6
-0+kobject_put (prvdata->kobj); +kfree (prvdata); +return ret; + } + +return ret;
drivers/soc/qcom/subsystem_sleep_stats.c line 142
Quoted 10 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +10
-0+} + +static void __exit subsystem_sleep_stats_exit(void) +{ +sysfs_remove_file (prvdata->kobj, &prvdata->ka.attr); +kobject_put (prvdata->kobj); +kfree (prvdata); +} + +module_init (subsystem_sleep_stats_init);
So if this is compiled into an arm/arm64 image that doesn't include qcom platform support it will create this directory? That's just nonsensical.
Quoted 26 lines · Stephen Boyd
Quoting Maulik Shah (2019-09-05 02:17:07)
Quoted 25 lines
Multiple subsystems like modem, spss, adsp, cdsp present on Qualcomm Technologies Inc's (QTI) SoCs maintains low power mode statistics in shared memory (SMEM). Lets add a driver to read and display this information using sysfs.
Signed-off-by Maulik Shah <[email protected]>---
Changes in v2:
- CorrectMakefileto useQCOM_SS_SLEEP_STATSconfig--- Documentation/ABI/testing/sysfs-power | 10 ++ drivers/soc/qcom/Kconfig | 9 ++ drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/subsystem_sleep_stats.c | 146 +++++++++++++++++++++++ 4 files changed, 166 insertions(+)create mode 100644
drivers/soc/qcom/subsystem_sleep_stats.cDocumentation/ABI/testing/sysfs-power +0
-0@@ -288,6 +288,16 @@ Description: writing a "0" (default) to it disables them. Reads from this file return the current value.
Quoted 3 lines
Documentation/ABI/testing/sysfs-power Description: · line 292
Quoted 2 lines
Documentation/ABI/testing/sysfs-power +2
-0+What: /sys/power/subsystem_sleep/stats +Date: December 2017It isn't December 2017.
Hi Stephen,
Keeping it 2017 since driver is present from 2017 (driver copyright is also from 2017-2019)
Quoted 13 lines
Documentation/ABI/testing/sysfs-power Description: · line 300
Quoted 9 lines
Documentation/ABI/testing/sysfs-power +8
-0+Contact: Maulik Shah <[email protected]> +Description: + The /sys/power/subsystem_sleep/stats file prints the subsystem + sleep information on Qualcomm Technologies, Inc. (QTI) SoCs. + + Reading from this file will display subsystem level low power + mode statistics. +This directory doesn't make any sense to me. It's in the top-level power directory and it is specific to qcom. Is this debugging information? Why does userspace care about understanding the sleep stats information? Please Cc Rafael on anything touching
/sys/power/
Stats can be used by userspace for the purpose of computing battery utilization.
Sure i will include Rafael in next revision.
Quoted 50 lines
Quoted 50 lines
Documentation/ABI/testing/sysfs-power +0
-0What: /sys/power/resume_offset Date: April 2018 Contact: Mario Limonciello <[email protected]>drivers/soc/qcom/subsystem_sleep_stats.c new file +40
-0@@ -0,0 +1,146 @@ +// SPDX-License-Identifier: GPL-2.0-only + +/* +* Copyright (c) 2017-2019, The Linux Foundation. All rights reserved. +*/ + +#define pr_fmt (fmt) "%s: " fmt, KBUILD_MODNAME + +#include <linux/errno.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/types.h> + +#include <linux/soc/qcom/smem.h> + +enum subsystem_item_id { +MODEM =605 , +ADSP , +CDSP , +SLPI , +GPU , +DISPLAY , +}; + +enum subsystem_pid { +PID_APSS =0 , +PID_MODEM =1 , +PID_ADSP =2 , +PID_SLPI =3 , +PID_CDSP =5 , +PID_GPU =PID_APSS , +PID_DISPLAY =PID_APSS , +}; + +struct subsystem_data { +char *name; +enum subsystem_item_id item_id; +enum subsystem_pid pid; +}; +
Quoted 8 lines
drivers/soc/qcom/subsystem_sleep_stats.c line 47
Quoted 7 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +7
-0+static const struct subsystem_data subsystems[] = { + {"MODEM" ,MODEM ,PID_MODEM }, + {"ADSP" ,ADSP ,PID_ADSP }, + {"CDSP" ,CDSP ,PID_CDSP }, + {"SLPI" ,SLPI ,PID_SLPI }, + {"GPU" ,GPU ,PID_GPU }, + {"DISPLAY" ,DISPLAY ,PID_DISPLAY },Please put spaces around braces.
Sure.
Quoted 34 lines
Quoted 19 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +19
-0+}; + +struct subsystem_stats { +uint32_t version_id; +uint32_t count; +uint64_t last_entered; +uint64_t last_exited; +uint64_t accumulated_duration; +}; + +struct subsystem_stats_prv_data { +struct kobj_attribute ka; +struct kobject *kobj; +}; + +static struct subsystem_stats_prv_data *prvdata; + +static inline ssize_t subsystem_stats_print(char *prvbuf, ssize_t length, + struct subsystem_stats *record,drivers/soc/qcom/subsystem_sleep_stats.c line 76
Quoted 10 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +10
-0+ const char *name) +{ +return scnprintf (prvbuf, length,"%s\n\tVersion:0x%x\n" +"\tSleep Count:0x%x\n" +"\tSleep Last Entered At:0x%llx\n" +"\tSleep Last Exited At:0x%llx\n" +"\tSleep Accumulated Duration:0x%llx\n\n" , + name, record->version_id, record->count, + record->last_entered, record->last_exited, + record->accumulated_duration);Information in sysfs is supposed to be one value per file. This is a bunch of different values and it includes a version field. Looks almost like something we would put into /proc, but of course that doesn't make any sense to put in /proc either.
Quoted 6 lines
Please rethink the whole approach here. Can this be placed under the remoteproc nodes for each remote processor that's in the system? That would make it more discoverable by userspace looking at the remoteproc devices. I suppose GPU and DISPLAY aren't "remoteproc"s though so maybe this should be a new 'class' for devices that have an RPMh RSC? Maybe make a
qcom_rpmh_rscclass and then have these be stats in there.
since stats can be used by userspace for the purpose of computing battery utilization /sys/power seems to be good place to keep it to me.
Adding it under class may require it to be device. we are using it only as module.
Quoted 9 lines
Quoted 9 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +9
-0+} + +static ssize_t subsystem_stats_show (struct kobject *kobj, +struct kobj_attribute *attr,char *buf) +{ +ssize_t length =0 ; +int i =0 ; +size_t size =0 ; +struct subsystem_stats *record =NULL ;
Quoted 8 lines
drivers/soc/qcom/subsystem_sleep_stats.c line 92
Quoted 7 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +7
-0+ +/* Read SMEM data written by other subsystems */ +for (i =0 ; i <ARRAY_SIZE (subsystems); i++) { + record = (struct subsystem_stats *)qcom_smem_get ( + subsystems[i].pid, subsystems[i].item_id, &size); + +if (!IS_ERR_OR_NULL (record) && (PAGE_SIZE - length >0 ))It can return ERR pointer or NULL? Why?
Updated to check only IS_ERR.
Thanks for pointing.
Quoted 24 lines
Quoted 24 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +24
-0+ length +=subsystem_stats_print (buf + length, +PAGE_SIZE - length, + record, + subsystems[i].name); + } + +return length; +} + +static int __init subsystem_sleep_stats_init (void ) +{ +struct kobject *ss_stats_kobj; +int ret; + + prvdata =kmalloc (sizeof (*prvdata),GFP_KERNEL ); +if (!prvdata) +return -ENOMEM ; + + ss_stats_kobj =kobject_create_and_add ("subsystem_sleep" , + power_kobj); +if (!ss_stats_kobj) +return -ENOMEM ; + + prvdata->kobj = ss_stats_kobj;
Quoted 8 lines
drivers/soc/qcom/subsystem_sleep_stats.c line 122
Quoted 6 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +6
-0+ +sysfs_attr_init (&prvdata->ka.attr); + prvdata->ka.attr.mode =0444 ; + prvdata->ka.attr.name ="stats" ; + prvdata->ka.show = subsystem_stats_show; + prvdata->ka.store =NULL ;Does it need to be set to NULL explicitly? Why not
kzalloc()prvdata above?
Fixed.
Quoted 5 lines
drivers/soc/qcom/subsystem_sleep_stats.c line 126
Quoted 4 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +4
-0+ + ret =sysfs_create_file (prvdata->kobj, &prvdata->ka.attr); +if (ret) { +pr_err ("sysfs_create_file failed\n" );Seems useless. Presumably
sysfs_create_file()can complain itself.
Fixed.
Quoted 10 lines
Quoted 10 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +10
-0+kobject_put (prvdata->kobj); +kfree (prvdata); +return ret; + } + +return ret; +} + +static void __exit subsystem_sleep_stats_exit(void) +{
Quoted 8 lines
drivers/soc/qcom/subsystem_sleep_stats.c line 142
Quoted 6 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +6
-0+sysfs_remove_file (prvdata->kobj, &prvdata->ka.attr); +kobject_put (prvdata->kobj); +kfree (prvdata); +} + +module_init (subsystem_sleep_stats_init);So if this is compiled into an arm/arm64 image that doesn't include qcom platform support it will create this directory? That's just nonsensical.
Kconfig depends on QCOM_SMEM which inturn depends on ARCH_QCOM to get compiled into.
It won't get compiled for other than qcom platforms.
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Quoting Maulik Shah (2019-11-06 01:22:14)
Quoted 28 lines · Stephen Boyd
Quoted 19 lines
Quoting Maulik Shah (2019-09-05 02:17:07)
drivers/soc/qcom/subsystem_sleep_stats.c line 76
Quoted 13 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +13
-0+ +static inline ssize_t subsystem_stats_print (char *prvbuf,ssize_t length, +struct subsystem_stats *record, +const char *name) +{ +return scnprintf (prvbuf, length,"%s\n\tVersion:0x%x\n" +"\tSleep Count:0x%x\n" +"\tSleep Last Entered At:0x%llx\n" +"\tSleep Last Exited At:0x%llx\n" +"\tSleep Accumulated Duration:0x%llx\n\n" , + name, record->version_id, record->count, + record->last_entered, record->last_exited, + record->accumulated_duration);Information in sysfs is supposed to be one value per file. This is a bunch of different values and it includes a version field. Looks almost like something we would put into /proc, but of course that doesn't make any sense to put in /proc either.
Quoted 6 lines
Please rethink the whole approach here. Can this be placed under the remoteproc nodes for each remote processor that's in the system? That would make it more discoverable by userspace looking at the remoteproc devices. I suppose GPU and DISPLAY aren't "remoteproc"s though so maybe this should be a new 'class' for devices that have an RPMh RSC? Maybe make a
qcom_rpmh_rscclass and then have these be stats in there.
Quoted 6 lines
since stats can be used by userspace for the purpose of computing battery utilization
/sys/powerseems to be good place to keep it to me.Adding it under class may require it to be device. we are using it only as module.
I believe /sys/power is for the power management subsystem, not specifically battery utilization or remote processor power states. Wouldn't battery be /sys/class/power_supply? Why not put this underneath some /sys/class/remoteproc or so?
Quoted 19 lines
Quoted 10 lines
Quoted 10 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +10
-0+kobject_put (prvdata->kobj); +kfree (prvdata); +return ret; + } + +return ret; +} + +static void __exit subsystem_sleep_stats_exit(void) +{Quoted 8 lines
drivers/soc/qcom/subsystem_sleep_stats.c line 142
Quoted 6 lines
drivers/soc/qcom/subsystem_sleep_stats.c new file +6
-0+sysfs_remove_file (prvdata->kobj, &prvdata->ka.attr); +kobject_put (prvdata->kobj); +kfree (prvdata); +} + +module_init (subsystem_sleep_stats_init);So if this is compiled into an arm/arm64 image that doesn't include qcom platform support it will create this directory? That's just nonsensical.
Quoted 4 lines
Kconfigdepends onQCOM_SMEMwhich inturn depends onARCH_QCOMto get compiled into.It won't get compiled for other than qcom platforms.
Sure it won't get compiled for anything that doesn't have ARCH_QCOM enabled, but it can run on a board or SoC that isn't qcom. That's the concern.