Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] soc: qcom: Introduce subsystem sleep stats driver
@ 2019-09-05  9:17 Maulik Shah
  2019-09-05 18:37 ` Stephen Boyd
  0 siblings, 1 reply; 2+ messages in thread
From: Maulik Shah @ 2019-09-05  9:17 UTC (permalink / raw)
  To: swboyd, agross, david.brown, linux-arm-msm
  Cc: linux-kernel, linux-pm, bjorn.andersson, evgreen, dianders,
	rnayak, ilina, lsrao, Maulik Shah

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 <mkshah@codeaurora.org>
---
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

diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
index 18b7dc929234..1f8bb201246a 100644
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -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 <mkshah@codeaurora.org>
+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 <mario.limonciello@dell.com>
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 880cf0290962..da53a96c6cce 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -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
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index ffe519b0cb66..f00f21b87a22 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -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
diff --git a/drivers/soc/qcom/subsystem_sleep_stats.c b/drivers/soc/qcom/subsystem_sleep_stats.c
new file mode 100644
index 000000000000..5379714b6ba4
--- /dev/null
+++ b/drivers/soc/qcom/subsystem_sleep_stats.c
@@ -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;
+};
+
+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},
+};
+
+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,
+					    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);
+}
+
+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;
+
+	/* 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))
+			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;
+
+	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;
+
+	ret = sysfs_create_file(prvdata->kobj, &prvdata->ka.attr);
+	if (ret) {
+		pr_err("sysfs_create_file failed\n");
+		kobject_put(prvdata->kobj);
+		kfree(prvdata);
+		return ret;
+	}
+
+	return ret;
+}
+
+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);
+module_exit(subsystem_sleep_stats_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc subsystem sleep stats driver");
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH v2] soc: qcom: Introduce subsystem sleep stats driver
  2019-09-05  9:17 [PATCH v2] soc: qcom: Introduce subsystem sleep stats driver Maulik Shah
@ 2019-09-05 18:37 ` Stephen Boyd
  0 siblings, 0 replies; 2+ messages in thread
From: Stephen Boyd @ 2019-09-05 18:37 UTC (permalink / raw)
  To: Maulik Shah, agross, david.brown, linux-arm-msm
  Cc: linux-kernel, linux-pm, bjorn.andersson, evgreen, dianders,
	rnayak, ilina, lsrao, Maulik Shah

Quoting Maulik Shah (2019-09-05 02:17:07)
> 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 <mkshah@codeaurora.org>
> ---
> 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
> 
> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
> index 18b7dc929234..1f8bb201246a 100644
> --- a/Documentation/ABI/testing/sysfs-power
> +++ b/Documentation/ABI/testing/sysfs-power
> @@ -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.

> +Contact:       Maulik Shah <mkshah@codeaurora.org>
> +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/

>  What:          /sys/power/resume_offset
>  Date:          April 2018
>  Contact:       Mario Limonciello <mario.limonciello@dell.com>
> diff --git a/drivers/soc/qcom/subsystem_sleep_stats.c b/drivers/soc/qcom/subsystem_sleep_stats.c
> new file mode 100644
> index 000000000000..5379714b6ba4
> --- /dev/null
> +++ b/drivers/soc/qcom/subsystem_sleep_stats.c
> @@ -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;
> +};
> +
> +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.

> +};
> +
> +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,
> +                                           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.

> +}
> +
> +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;
> +
> +       /* 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?

> +                       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;
> +
> +       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?

> +
> +       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.

> +               kobject_put(prvdata->kobj);
> +               kfree(prvdata);
> +               return ret;
> +       }
> +
> +       return ret;
> +}
> +
> +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.


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-05  9:17 [PATCH v2] soc: qcom: Introduce subsystem sleep stats driver Maulik Shah
2019-09-05 18:37 ` Stephen Boyd

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org linux-arm-msm@archiver.kernel.org
	public-inbox-index linux-arm-msm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox