From: Mahesh Sivasubramanian <[email protected]>
Let's add a driver to read the stats from remote processor and
export to debugfs.
The driver creates "qcom_sleep_stats" directory in debugfs and
adds files for various low power mode available. Below is sample
output with command
cat /sys/kernel/debug/qcom_sleep_stats/ddr
count = 0
Last Entered At = 0
Last Exited At = 0
Accumulated Duration = 0
Signed-off-by: Mahesh Sivasubramanian <[email protected]>
Signed-off-by: Lina Iyer <[email protected]>
[mkshah: add subsystem sleep stats, create one file for each stat]
Signed-off-by: Maulik Shah <[email protected]>
---
drivers/soc/qcom/Kconfig | 10 ++
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/qcom_sleep_stats.c | 259 ++++++++++++++++++++++++++++++++++++
3 files changed, 270 insertions(+)
create mode 100644 drivers/soc/qcom/qcom_sleep_stats.c
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index abfef20..c67fc00 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -182,6 +182,16 @@ 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_SLEEP_STATS
+ tristate "Qualcomm Technologies, Inc. (QTI) Sleep stats driver"
+ depends on (ARCH_QCOM && DEBUG_FS) || COMPILE_TEST
+ depends on QCOM_SMEM
+ help
+ Qualcomm Technologies, Inc. (QTI) Sleep stats driver to read
+ the shared memory exported by the remote processor related to
+ various SoC level low power modes statistics and export to debugfs
+ interface.
+
config QCOM_SOCINFO
tristate "Qualcomm socinfo driver"
depends on QCOM_SMEM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 24514c7..ad84f7b 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -19,6 +19,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_SLEEP_STATS) += qcom_sleep_stats.o
obj-$(CONFIG_QCOM_SOCINFO) += socinfo.o
obj-$(CONFIG_QCOM_SPM) += spm.o
obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
diff --git a/drivers/soc/qcom/qcom_sleep_stats.c b/drivers/soc/qcom/qcom_sleep_stats.c
new file mode 100644
index 0000000..2368279
--- /dev/null
+++ b/drivers/soc/qcom/qcom_sleep_stats.c
@@ -0,0 +1,259 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2011-2021, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/seq_file.h>
+
+#include <linux/soc/qcom/smem.h>
+#include <clocksource/arm_arch_timer.h>
+
+#define RPM_DYNAMIC_ADDR 0x14
+#define RPM_DYNAMIC_ADDR_MASK 0xFFFF
+
+#define STAT_TYPE_OFFSET 0x0
+#define COUNT_OFFSET 0x4
+#define LAST_ENTERED_AT_OFFSET 0x8
+#define LAST_EXITED_AT_OFFSET 0x10
+#define ACCUMULATED_OFFSET 0x18
+#define CLIENT_VOTES_OFFSET 0x1c
+
+struct subsystem_data {
+ const char *name;
+ u32 smem_item;
+ u32 pid;
+};
+
+static const struct subsystem_data subsystems[] = {
+ { "modem", 605, 1 },
+ { "wpss", 605, 13 },
+ { "adsp", 606, 2 },
+ { "cdsp", 607, 5 },
+ { "slpi", 608, 3 },
+ { "gpu", 609, 0 },
+ { "display", 610, 0 },
+ { "adsp_island", 613, 2 },
+ { "slpi_island", 613, 3 },
+};
+
+struct stats_config {
+ size_t stats_offset;
+ size_t num_records;
+ bool appended_stats_avail;
+ bool dynamic_offset;
+};
+
+struct stats_data {
+ bool appended_stats_avail;
+ void __iomem *base;
+};
+
+struct sleep_stats {
+ u32 stat_type;
+ u32 count;
+ u64 last_entered_at;
+ u64 last_exited_at;
+ u64 accumulated;
+};
+
+struct appended_stats {
+ u32 client_votes;
+ u32 reserved[3];
+};
+
+static void qcom_print_stats(struct seq_file *s, const struct sleep_stats *stat)
+{
+ u64 accumulated = stat->accumulated;
+ /*
+ * If a subsystem is in sleep when reading the sleep stats adjust
+ * the accumulated sleep duration to show actual sleep time.
+ */
+ if (stat->last_entered_at > stat->last_exited_at)
+ accumulated += arch_timer_read_counter() - stat->last_entered_at;
+
+ seq_printf(s, "Count: %u\n", stat->count);
+ seq_printf(s, "Last Entered At: %llu\n", stat->last_entered_at);
+ seq_printf(s, "Last Exited At: %llu\n", stat->last_exited_at);
+ seq_printf(s, "Accumulated Duration: %llu\n", accumulated);
+}
+
+static int qcom_subsystem_sleep_stats_show(struct seq_file *s, void *unused)
+{
+ struct subsystem_data *subsystem = s->private;
+ struct sleep_stats *stat;
+
+ /* Items are allocated lazily, so lookup pointer each time */
+ stat = qcom_smem_get(subsystem->pid, subsystem->smem_item, NULL);
+ if (IS_ERR(stat))
+ return -EIO;
+
+ qcom_print_stats(s, stat);
+
+ return 0;
+}
+
+static int qcom_soc_sleep_stats_show(struct seq_file *s, void *unused)
+{
+ struct stats_data *d = s->private;
+ void __iomem *reg = d->base;
+ struct sleep_stats stat;
+
+ memcpy_fromio(&stat, reg, sizeof(stat));
+ qcom_print_stats(s, &stat);
+
+ if (d->appended_stats_avail) {
+ struct appended_stats votes;
+
+ memcpy_fromio(&votes, reg + CLIENT_VOTES_OFFSET, sizeof(votes));
+ seq_printf(s, "Client Votes: %#x\n", votes.client_votes);
+ }
+
+ return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(qcom_soc_sleep_stats);
+DEFINE_SHOW_ATTRIBUTE(qcom_subsystem_sleep_stats);
+
+static void qcom_create_soc_sleep_stat_files(struct dentry *root, void __iomem *reg,
+ struct stats_data *d,
+ const struct stats_config *config)
+{
+ char stat_type[sizeof(u32) + 1] = {0};
+ size_t stats_offset = config->stats_offset;
+ u32 offset = 0, type;
+ int i, j;
+
+ /*
+ * On RPM targets, stats offset location is dynamic and changes from target
+ * to target and sometimes from build to build for same target.
+ *
+ * In such cases the dynamic address is present at 0x14 offset from base
+ * address in devicetree. The last 16bits indicates the stats_offset.
+ */
+ if (config->dynamic_offset) {
+ stats_offset = readl(reg + RPM_DYNAMIC_ADDR);
+ stats_offset &= RPM_DYNAMIC_ADDR_MASK;
+ }
+
+ for (i = 0; i < config->num_records; i++) {
+ d[i].base = reg + offset + stats_offset;
+
+ /*
+ * Read the low power mode name and create debugfs file for it.
+ * The names read could be of below,
+ * (may change depending on low power mode supported).
+ * For rpmh-sleep-stats: "aosd", "cxsd" and "ddr".
+ * For rpm-sleep-stats: "vmin" and "vlow".
+ */
+ type = readl(d[i].base);
+ for (j = 0; j < sizeof(u32); j++) {
+ stat_type[j] = type & 0xff;
+ type = type >> 8;
+ }
+ strim(stat_type);
+ debugfs_create_file(stat_type, 0400, root, &d[i],
+ &qcom_soc_sleep_stats_fops);
+
+ offset += sizeof(struct sleep_stats);
+ if (d[i].appended_stats_avail)
+ offset += sizeof(struct appended_stats);
+ }
+}
+
+static void qcom_create_subsystem_stat_files(struct dentry *root)
+{
+ const struct sleep_stats *stat;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
+ stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item, NULL);
+ if (IS_ERR(stat))
+ continue;
+
+ debugfs_create_file(subsystems[i].name, 0400, root, (void *)&subsystems[i],
+ &qcom_subsystem_sleep_stats_fops);
+ }
+}
+
+static int qcom_sleep_stats_probe(struct platform_device *pdev)
+{
+ void __iomem *reg;
+ struct dentry *root;
+ const struct stats_config *config;
+ struct stats_data *d;
+ int i;
+
+ config = device_get_match_data(&pdev->dev);
+ if (!config)
+ return -ENODEV;
+
+ reg = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
+ if (IS_ERR(reg))
+ return -ENOMEM;
+
+ d = devm_kcalloc(&pdev->dev, config->num_records,
+ sizeof(*d), GFP_KERNEL);
+ if (!d)
+ return -ENOMEM;
+
+ for (i = 0; i < config->num_records; i++)
+ d[i].appended_stats_avail = config->appended_stats_avail;
+
+ root = debugfs_create_dir("qcom_sleep_stats", NULL);
+
+ qcom_create_subsystem_stat_files(root);
+ qcom_create_soc_sleep_stat_files(root, reg, d, config);
+
+ platform_set_drvdata(pdev, root);
+
+ return 0;
+}
+
+static int qcom_sleep_stats_remove(struct platform_device *pdev)
+{
+ struct dentry *root = platform_get_drvdata(pdev);
+
+ debugfs_remove_recursive(root);
+
+ return 0;
+}
+
+static const struct stats_config rpm_data = {
+ .stats_offset = 0,
+ .num_records = 2,
+ .appended_stats_avail = true,
+ .dynamic_offset = true,
+};
+
+static const struct stats_config rpmh_data = {
+ .stats_offset = 0x48,
+ .num_records = 3,
+ .appended_stats_avail = false,
+ .dynamic_offset = false,
+};
+
+static const struct of_device_id qcom_sleep_stats_table[] = {
+ { .compatible = "qcom,rpm-sleep-stats", .data = &rpm_data },
+ { .compatible = "qcom,rpmh-sleep-stats", .data = &rpmh_data },
+ { }
+};
+MODULE_DEVICE_TABLE(of, qcom_sleep_stats_table);
+
+static struct platform_driver qcom_sleep_stats = {
+ .probe = qcom_sleep_stats_probe,
+ .remove = qcom_sleep_stats_remove,
+ .driver = {
+ .name = "qcom_sleep_stats",
+ .of_match_table = qcom_sleep_stats_table,
+ },
+};
+module_platform_driver(qcom_sleep_stats);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) Sleep Stats driver");
+MODULE_LICENSE("GPL v2");
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
Hi Maulik,
On Thu, Oct 07, 2021 at 03:27:26PM +0530, Maulik Shah wrote:
> From: Mahesh Sivasubramanian <[email protected]>
>
> Let's add a driver to read the stats from remote processor and
> export to debugfs.
>
> The driver creates "qcom_sleep_stats" directory in debugfs and
> adds files for various low power mode available. Below is sample
> output with command
>
> cat /sys/kernel/debug/qcom_sleep_stats/ddr
> count = 0
> Last Entered At = 0
> Last Exited At = 0
> Accumulated Duration = 0
>
> Signed-off-by: Mahesh Sivasubramanian <[email protected]>
> Signed-off-by: Lina Iyer <[email protected]>
> [mkshah: add subsystem sleep stats, create one file for each stat]
> Signed-off-by: Maulik Shah <[email protected]>
> ---
> drivers/soc/qcom/Kconfig | 10 ++
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/qcom_sleep_stats.c | 259 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 270 insertions(+)
> create mode 100644 drivers/soc/qcom/qcom_sleep_stats.c
>
> [...]
> +
> +static int qcom_subsystem_sleep_stats_show(struct seq_file *s, void *unused)
> +{
> + struct subsystem_data *subsystem = s->private;
> + struct sleep_stats *stat;
> +
> + /* Items are allocated lazily, so lookup pointer each time */
> + stat = qcom_smem_get(subsystem->pid, subsystem->smem_item, NULL);
> + if (IS_ERR(stat))
> + return -EIO;
> +
> [...]
> +
> +static void qcom_create_subsystem_stat_files(struct dentry *root)
> +{
> + const struct sleep_stats *stat;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
> + stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item, NULL);
> + if (IS_ERR(stat))
> + continue;
> +
> + debugfs_create_file(subsystems[i].name, 0400, root, (void *)&subsystems[i],
> + &qcom_subsystem_sleep_stats_fops);
This causes WARNINGs on MSM8996 and MSM8916:
[ 0.503054] ------------[ cut here ]------------
[ 0.503100] WARNING: CPU: 1 PID: 1 at drivers/soc/qcom/smem.c:587 qcom_smem_get+0x184/0x1b0
[ 0.503184] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.15.0-rc4+ #378
[ 0.503218] Hardware name: Xiaomi Mi Note 2 (DT)
[ 0.503278] pc : qcom_smem_get+0x184/0x1b0
[ 0.503307] lr : qcom_sleep_stats_probe+0xfc/0x20
[ 0.503875] Call trace:
[ 0.503896] qcom_smem_get+0x184/0x1b0
[ 0.503925] qcom_sleep_stats_probe+0xfc/0x270
AFAICT from downstream the smem subsystem information is only read in
the rpmh_master_stat.c driver, should this be specific to RPMh?
There is a rpm_master_stat.c too but that looks quite different,
so I guess the approach is different with RPM?
Two more (unrelated) issues here:
1. This will silently not register anything if SMEM probes after the
qcom-sleep-stats driver (qcom_smem_get() will return -EPROBE_DEFER)
and you will just skip registering the debugfs files.
2. In qcom_subsystem_sleep_stats_show() you say
/* Items are allocated lazily, so lookup pointer each time */
But, if the lookup fails here you don't register the debugfs file
at all. Does this work if the subsystem is started after this driver?
Thanks,
Stephan
Hi,
On 10/7/2021 11:59 PM, Stephan Gerhold wrote:
> Hi Maulik,
>
> On Thu, Oct 07, 2021 at 03:27:26PM +0530, Maulik Shah wrote:
>> From: Mahesh Sivasubramanian <[email protected]>
>>
>> Let's add a driver to read the stats from remote processor and
>> export to debugfs.
>>
>> The driver creates "qcom_sleep_stats" directory in debugfs and
>> adds files for various low power mode available. Below is sample
>> output with command
>>
>> cat /sys/kernel/debug/qcom_sleep_stats/ddr
>> count = 0
>> Last Entered At = 0
>> Last Exited At = 0
>> Accumulated Duration = 0
>>
>> Signed-off-by: Mahesh Sivasubramanian <[email protected]>
>> Signed-off-by: Lina Iyer <[email protected]>
>> [mkshah: add subsystem sleep stats, create one file for each stat]
>> Signed-off-by: Maulik Shah <[email protected]>
>> ---
>> drivers/soc/qcom/Kconfig | 10 ++
>> drivers/soc/qcom/Makefile | 1 +
>> drivers/soc/qcom/qcom_sleep_stats.c | 259 ++++++++++++++++++++++++++++++++++++
>> 3 files changed, 270 insertions(+)
>> create mode 100644 drivers/soc/qcom/qcom_sleep_stats.c
>>
>> [...]
>> +
>> +static int qcom_subsystem_sleep_stats_show(struct seq_file *s, void *unused)
>> +{
>> + struct subsystem_data *subsystem = s->private;
>> + struct sleep_stats *stat;
>> +
>> + /* Items are allocated lazily, so lookup pointer each time */
>> + stat = qcom_smem_get(subsystem->pid, subsystem->smem_item, NULL);
>> + if (IS_ERR(stat))
>> + return -EIO;
>> +
>> [...]
>> +
>> +static void qcom_create_subsystem_stat_files(struct dentry *root)
>> +{
>> + const struct sleep_stats *stat;
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
>> + stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item, NULL);
>> + if (IS_ERR(stat))
>> + continue;
>> +
>> + debugfs_create_file(subsystems[i].name, 0400, root, (void *)&subsystems[i],
>> + &qcom_subsystem_sleep_stats_fops);
>
> This causes WARNINGs on MSM8996 and MSM8916:
>
> [ 0.503054] ------------[ cut here ]------------
> [ 0.503100] WARNING: CPU: 1 PID: 1 at drivers/soc/qcom/smem.c:587 qcom_smem_get+0x184/0x1b0
> [ 0.503184] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.15.0-rc4+ #378
> [ 0.503218] Hardware name: Xiaomi Mi Note 2 (DT)
> [ 0.503278] pc : qcom_smem_get+0x184/0x1b0
> [ 0.503307] lr : qcom_sleep_stats_probe+0xfc/0x20
> [ 0.503875] Call trace:
> [ 0.503896] qcom_smem_get+0x184/0x1b0
> [ 0.503925] qcom_sleep_stats_probe+0xfc/0x270
>
> AFAICT from downstream the smem subsystem information is only read in
> the rpmh_master_stat.c driver, should this be specific to RPMh?
Thanks for checking this on MSM8996. Probably it doesnot have SMEM items
allocated so causes WARNINGs.
Keeping subsystem stats info in SMEM is not limited to only RPMH
targets. Downstream has some RPM targets which also uses SMEM to store
subsystem stats so the driver is kept generic.
>
> There is a rpm_master_stat.c too but that looks quite different,
> so I guess the approach is different with RPM?
Right. on existing upstream RPM targets i can skip to create/get SMEM
items since
they are not guranteed to be present and one should continue to use
rpm_master_stats.c to get subsystem stats. (this uses entirely different
data structure for sleep stats and are not part of RPM data ram/SMEM and
are deprecated in downstream).
>
> Two more (unrelated) issues here:
>
> 1. This will silently not register anything if SMEM probes after the
> qcom-sleep-stats driver (qcom_smem_get() will return -EPROBE_DEFER)
> and you will just skip registering the debugfs files.
I think module loading internally takes care of this.
we're making a direct function call into the qcom_smem driver, so we
already have a hard dependency on qcom_smem.ko being loaded.
>
> 2. In qcom_subsystem_sleep_stats_show() you say
> /* Items are allocated lazily, so lookup pointer each time */
>
> But, if the lookup fails here you don't register the debugfs file
> at all. Does this work if the subsystem is started after this driver?
Good catch. if the subsystem starts after this driver is loaded, the
look up fails during probe and we don't create debugfs file for the
subsystem.
one need to unload/load the driver again after sometime in bootup so by
that time all the subsytems (modem, adsp, cdsp, etc) are up and we
create debugfs file for them.
we have downstream fix for this to create the debugfs files irrespective
of look up fails or not. i have plan to add it once the base driver gets
merged.
Thanks,
Maulik
>
> Thanks,
> Stephan
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of Code Aurora Forum, hosted by The Linux Foundation
On Fri, Oct 08, 2021 at 02:45:20PM +0530, Maulik Shah wrote:
> On 10/7/2021 11:59 PM, Stephan Gerhold wrote:
> > On Thu, Oct 07, 2021 at 03:27:26PM +0530, Maulik Shah wrote:
> > > From: Mahesh Sivasubramanian <[email protected]>
> > >
> > > Let's add a driver to read the stats from remote processor and
> > > export to debugfs.
> > >
> > > The driver creates "qcom_sleep_stats" directory in debugfs and
> > > adds files for various low power mode available. Below is sample
> > > output with command
> > >
> > > cat /sys/kernel/debug/qcom_sleep_stats/ddr
> > > count = 0
> > > Last Entered At = 0
> > > Last Exited At = 0
> > > Accumulated Duration = 0
> > >
> > > Signed-off-by: Mahesh Sivasubramanian <[email protected]>
> > > Signed-off-by: Lina Iyer <[email protected]>
> > > [mkshah: add subsystem sleep stats, create one file for each stat]
> > > Signed-off-by: Maulik Shah <[email protected]>
> > > ---
> > > drivers/soc/qcom/Kconfig | 10 ++
> > > drivers/soc/qcom/Makefile | 1 +
> > > drivers/soc/qcom/qcom_sleep_stats.c | 259 ++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 270 insertions(+)
> > > create mode 100644 drivers/soc/qcom/qcom_sleep_stats.c
> > >
> > > [...]
> > > +
> > > +static int qcom_subsystem_sleep_stats_show(struct seq_file *s, void *unused)
> > > +{
> > > + struct subsystem_data *subsystem = s->private;
> > > + struct sleep_stats *stat;
> > > +
> > > + /* Items are allocated lazily, so lookup pointer each time */
> > > + stat = qcom_smem_get(subsystem->pid, subsystem->smem_item, NULL);
> > > + if (IS_ERR(stat))
> > > + return -EIO;
> > > +
> > > [...]
> > > +
> > > +static void qcom_create_subsystem_stat_files(struct dentry *root)
> > > +{
> > > + const struct sleep_stats *stat;
> > > + int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
> > > + stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item, NULL);
> > > + if (IS_ERR(stat))
> > > + continue;
> > > +
> > > + debugfs_create_file(subsystems[i].name, 0400, root, (void *)&subsystems[i],
> > > + &qcom_subsystem_sleep_stats_fops);
> >
> > This causes WARNINGs on MSM8996 and MSM8916:
> >
> > [ 0.503054] ------------[ cut here ]------------
> > [ 0.503100] WARNING: CPU: 1 PID: 1 at drivers/soc/qcom/smem.c:587 qcom_smem_get+0x184/0x1b0
> > [ 0.503184] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.15.0-rc4+ #378
> > [ 0.503218] Hardware name: Xiaomi Mi Note 2 (DT)
> > [ 0.503278] pc : qcom_smem_get+0x184/0x1b0
> > [ 0.503307] lr : qcom_sleep_stats_probe+0xfc/0x20
> > [ 0.503875] Call trace:
> > [ 0.503896] qcom_smem_get+0x184/0x1b0
> > [ 0.503925] qcom_sleep_stats_probe+0xfc/0x270
> >
> > AFAICT from downstream the smem subsystem information is only read in
> > the rpmh_master_stat.c driver, should this be specific to RPMh?
>
> Thanks for checking this on MSM8996. Probably it doesnot have SMEM items
> allocated so causes WARNINGs.
>
> Keeping subsystem stats info in SMEM is not limited to only RPMH targets.
> Downstream has some RPM targets which also uses SMEM to store
> subsystem stats so the driver is kept generic.
>
Thanks for clarifying. To be honest, I'm not sure if the WARN_ON() in
smem is very useful since this isn't really fundamentally different as
if the entry is not allocated. But at the end all that matters is that
there are no warnings when loading this driver on older targets.
> >
> > There is a rpm_master_stat.c too but that looks quite different,
> > so I guess the approach is different with RPM?
>
> Right. on existing upstream RPM targets i can skip to create/get SMEM items
> since
> they are not guranteed to be present and one should continue to use
> rpm_master_stats.c to get subsystem stats. (this uses entirely different
> data structure for sleep stats and are not part of RPM data ram/SMEM and are
> deprecated in downstream).
>
> >
> > Two more (unrelated) issues here:
> >
> > 1. This will silently not register anything if SMEM probes after the
> > qcom-sleep-stats driver (qcom_smem_get() will return -EPROBE_DEFER)
> > and you will just skip registering the debugfs files.
>
> I think module loading internally takes care of this.
> we're making a direct function call into the qcom_smem driver, so we
> already have a hard dependency on qcom_smem.ko being loaded.
>
The driver can also be built-in and in this case this is not guaranteed.
This actually happened to me when I tried this on MSM8916: I did not get
the WARNINGs because all the qcom_smem_get() just returned -EPROBE_DEFER.
qcom_smem was then probed after qcom_sleep_stats. Of course, the smem
items are not present there anyway so it did not matter.
> >
> > 2. In qcom_subsystem_sleep_stats_show() you say
> > /* Items are allocated lazily, so lookup pointer each time */
> >
> > But, if the lookup fails here you don't register the debugfs file
> > at all. Does this work if the subsystem is started after this driver?
>
> Good catch. if the subsystem starts after this driver is loaded, the look up
> fails during probe and we don't create debugfs file for the subsystem.
>
> one need to unload/load the driver again after sometime in bootup so by that
> time all the subsytems (modem, adsp, cdsp, etc) are up and we create debugfs
> file for them.
>
> we have downstream fix for this to create the debugfs files irrespective of
> look up fails or not. i have plan to add it once the base driver gets
> merged.
>
OK, I don't seem to have any recent Qualcomm platform that actually has
subsystem stats in SMEM so it doesn't bother me. I just wanted to
mention it for reference. :)
Thanks!
Stephan
Hi Stephan,
On 10/8/2021 4:04 PM, Stephan Gerhold wrote:
> On Fri, Oct 08, 2021 at 02:45:20PM +0530, Maulik Shah wrote:
>> On 10/7/2021 11:59 PM, Stephan Gerhold wrote:
>>> On Thu, Oct 07, 2021 at 03:27:26PM +0530, Maulik Shah wrote:
>>>> From: Mahesh Sivasubramanian <[email protected]>
>>>>
>>>> Let's add a driver to read the stats from remote processor and
>>>> export to debugfs.
>>>>
>>>> The driver creates "qcom_sleep_stats" directory in debugfs and
>>>> adds files for various low power mode available. Below is sample
>>>> output with command
>>>>
>>>> cat /sys/kernel/debug/qcom_sleep_stats/ddr
>>>> count = 0
>>>> Last Entered At = 0
>>>> Last Exited At = 0
>>>> Accumulated Duration = 0
>>>>
>>>> Signed-off-by: Mahesh Sivasubramanian <[email protected]>
>>>> Signed-off-by: Lina Iyer <[email protected]>
>>>> [mkshah: add subsystem sleep stats, create one file for each stat]
>>>> Signed-off-by: Maulik Shah <[email protected]>
>>>> ---
>>>> drivers/soc/qcom/Kconfig | 10 ++
>>>> drivers/soc/qcom/Makefile | 1 +
>>>> drivers/soc/qcom/qcom_sleep_stats.c | 259 ++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 270 insertions(+)
>>>> create mode 100644 drivers/soc/qcom/qcom_sleep_stats.c
>>>>
>>>> [...]
>>>> +
>>>> +static int qcom_subsystem_sleep_stats_show(struct seq_file *s, void *unused)
>>>> +{
>>>> + struct subsystem_data *subsystem = s->private;
>>>> + struct sleep_stats *stat;
>>>> +
>>>> + /* Items are allocated lazily, so lookup pointer each time */
>>>> + stat = qcom_smem_get(subsystem->pid, subsystem->smem_item, NULL);
>>>> + if (IS_ERR(stat))
>>>> + return -EIO;
>>>> +
>>>> [...]
>>>> +
>>>> +static void qcom_create_subsystem_stat_files(struct dentry *root)
>>>> +{
>>>> + const struct sleep_stats *stat;
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
>>>> + stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item, NULL);
>>>> + if (IS_ERR(stat))
>>>> + continue;
>>>> +
>>>> + debugfs_create_file(subsystems[i].name, 0400, root, (void *)&subsystems[i],
>>>> + &qcom_subsystem_sleep_stats_fops);
>>>
>>> This causes WARNINGs on MSM8996 and MSM8916:
>>>
>>> [ 0.503054] ------------[ cut here ]------------
>>> [ 0.503100] WARNING: CPU: 1 PID: 1 at drivers/soc/qcom/smem.c:587 qcom_smem_get+0x184/0x1b0
>>> [ 0.503184] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.15.0-rc4+ #378
>>> [ 0.503218] Hardware name: Xiaomi Mi Note 2 (DT)
>>> [ 0.503278] pc : qcom_smem_get+0x184/0x1b0
>>> [ 0.503307] lr : qcom_sleep_stats_probe+0xfc/0x20
>>> [ 0.503875] Call trace:
>>> [ 0.503896] qcom_smem_get+0x184/0x1b0
>>> [ 0.503925] qcom_sleep_stats_probe+0xfc/0x270
>>>
>>> AFAICT from downstream the smem subsystem information is only read in
>>> the rpmh_master_stat.c driver, should this be specific to RPMh?
>>
>> Thanks for checking this on MSM8996. Probably it doesnot have SMEM items
>> allocated so causes WARNINGs.
>>
>> Keeping subsystem stats info in SMEM is not limited to only RPMH targets.
>> Downstream has some RPM targets which also uses SMEM to store
>> subsystem stats so the driver is kept generic.
>>
>
> Thanks for clarifying. To be honest, I'm not sure if the WARN_ON() in
> smem is very useful since this isn't really fundamentally different as
> if the entry is not allocated. But at the end all that matters is that
> there are no warnings when loading this driver on older targets.
>
>>>
>>> There is a rpm_master_stat.c too but that looks quite different,
>>> so I guess the approach is different with RPM?
>>
>> Right. on existing upstream RPM targets i can skip to create/get SMEM items
>> since
>> they are not guranteed to be present and one should continue to use
>> rpm_master_stats.c to get subsystem stats. (this uses entirely different
>> data structure for sleep stats and are not part of RPM data ram/SMEM and are
>> deprecated in downstream).
>>
>>>
>>> Two more (unrelated) issues here:
>>>
>>> 1. This will silently not register anything if SMEM probes after the
>>> qcom-sleep-stats driver (qcom_smem_get() will return -EPROBE_DEFER)
>>> and you will just skip registering the debugfs files.
>>
>> I think module loading internally takes care of this.
>> we're making a direct function call into the qcom_smem driver, so we
>> already have a hard dependency on qcom_smem.ko being loaded.
>>
>
> The driver can also be built-in and in this case this is not guaranteed.
> This actually happened to me when I tried this on MSM8916: I did not get
> the WARNINGs because all the qcom_smem_get() just returned -EPROBE_DEFER.
> qcom_smem was then probed after qcom_sleep_stats. Of course, the smem
> items are not present there anyway so it did not matter.
I see SMEM is in arch init call, Updated in v12 to register driver in
late_init by the time SMEM would have probed already and should not need
to handle -EPROBE_DEFER.
>
>>>
>>> 2. In qcom_subsystem_sleep_stats_show() you say
>>> /* Items are allocated lazily, so lookup pointer each time */
>>>
>>> But, if the lookup fails here you don't register the debugfs file
>>> at all. Does this work if the subsystem is started after this driver?
>>
>> Good catch. if the subsystem starts after this driver is loaded, the look up
>> fails during probe and we don't create debugfs file for the subsystem.
>>
>> one need to unload/load the driver again after sometime in bootup so by that
>> time all the subsytems (modem, adsp, cdsp, etc) are up and we create debugfs
>> file for them.
>>
>> we have downstream fix for this to create the debugfs files irrespective of
>> look up fails or not. i have plan to add it once the base driver gets
>> merged.
>>
>
> OK, I don't seem to have any recent Qualcomm platform that actually has
> subsystem stats in SMEM so it doesn't bother me. I just wanted to
> mention it for reference. :)
Ok updated driver to not use SMEM for RPM based targets for existing
upstream targets (when such target goes upstream in future which uses
SMEM for subsystem stats we can enable back with new compatible say -v2).
Thanks,
Maulik
>
> Thanks!
> Stephan
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of Code Aurora Forum, hosted by The Linux Foundation