lkml.org 
[lkml]   [2019]   [Nov]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] soc: qcom: Introduce subsystem sleep stats driver
From
Date

On 9/6/2019 12:07 AM, Stephen Boyd wrote:
> 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.

Hi Stephen,

Keeping it 2017 since driver is present from 2017 (driver copyright is
also from 2017-2019)

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

Stats can be used by userspace for the purpose of computing battery
utilization.

Sure i will include Rafael in next revision.

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

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.

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

Updated to check only IS_ERR.

Thanks for pointing.

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

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.

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

\
 
 \ /
  Last update: 2019-11-06 10:23    [W:0.072 / U:1.460 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site