lkml.org 
[lkml]   [2020]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [v3] soc: qcom: Introduce subsystem sleep stats driver
From
Date

On 11/16/2019 2:15 AM, Stephen Boyd wrote:
> Quoting Maulik Shah (2019-11-06 03:19:25)
>> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
>> index 6f87b9d..e095eae 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.
> I still don't understand what this has to do with the kernel's power
> management support.

Using debugfs in v2 in single stats driver.

https://lore.kernel.org/patchwork/project/lkml/list/?series=430622

>
>> +
>> 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 79d8265..bed0704 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -185,6 +185,15 @@ config QCOM_SOCINFO
>> Say yes here to support the Qualcomm socinfo driver, providing
>> information about the SoC to user space.
>>
>> +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)
> This 'Inc' is missing the full stop like in the summary above. Please be
> consistent.
corrected.
>
>> + SS sleep stats driver to read the sleep stats of various subsystems
> what is 'SS'?
corrected.
>
>> + 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/subsystem_sleep_stats.c b/drivers/soc/qcom/subsystem_sleep_stats.c
>> new file mode 100644
>> index 00000000..724b213
>> --- /dev/null
>> +++ b/drivers/soc/qcom/subsystem_sleep_stats.c
>> @@ -0,0 +1,143 @@
>> +// 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>
> I think we need to include linux/kernel.h for scnprintf() too.
done.
>> +
>> +#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,
>> +};
> Can these just be defines? There seems to be no value in enum because
> we're not testing these in switch statements and they're randomly
> assigned values.

this is inline with subsystem_item_id enum which uses 605 to 610 item
ids in enum.

keeping it same for better readbility.

>
>> +
>> +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;
> We use u32 and u64 in kernel code. Also, is this the value in shared
> memory? Probably it's little endian so needs to be __le32 an __le64.
corrected.
>> +};
>> +
>> +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);
> This isn't one value per file as per sysfs rules. Why can't this go to
> debugfs? Otherwise, it would be better to split it up into multiple
> files.
>
> And it still looks like something that should be plumbed into the remote
> proc subsystem so we can see from userspace what remote processors are
> 1) present in the system and 2) how long they've been in a sleep state.
Using debugfs.
>
>> +}
>> +
>> +static ssize_t subsystem_stats_show(struct kobject *kobj,
>> + struct kobj_attribute *attr, char *buf)
>> +{
>> + ssize_t length = 0;
>> + int i = 0;
> Drop assignment to i here.
done.
>
>> + size_t size = 0;
> Why assign size to 0? It looks unused in this function besides to store
> the size in qcom_smem_get(). It looks like we can pass NULL for that
> argument if we don't care to actually look at the size of what is
> returned.
done.
>> + struct subsystem_stats *record = NULL;
> Please don't assign to NULL and then overwrite it without testing in
> between.
done.
>
>> +
>> + /* Read SMEM data written by other subsystems */
>> + for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
>> + record = (struct subsystem_stats *) qcom_smem_get(
> The cast is unnecessary, it returns a void * already.
done.
>> + subsystems[i].pid, subsystems[i].item_id, &size);
>> +
>> + if (!IS_ERR(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 = kzalloc(sizeof(*prvdata), GFP_KERNEL);
>> + if (!prvdata)
>> + return -ENOMEM;
>> +
>> + ss_stats_kobj = kobject_create_and_add("subsystem_sleep",
>> + power_kobj);
> If this module is loaded on non-qcom platforms we'll create
> subsystem_sleep directory still. Please don't do that. If this was
> connected to remote proc it would be easier to avoid this problem.
This is clubbed in single stats driver for both sleep stats in v2 link
pasted above.
>
>> + 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;
>> +
>> + ret = sysfs_create_file(prvdata->kobj, &prvdata->ka.attr);
>> + if (ret) {
>> + kobject_put(prvdata->kobj);
>> + kfree(prvdata);
>> + }
>> +
>> + return ret;
>> +}
>> +

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

\
 
 \ /
  Last update: 2020-02-21 10:48    [W:3.300 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site