2021-09-06 05:30:00

by Maulik Shah

[permalink] [raw]
Subject: [PATCH v9 0/5] Introduce SoC sleep stats driver

Changes in v9:
- Remove soft dependency on smem module
- Return -EIO to userspace in case of error
- Make struct sleep_stats *stat a const pointer
- Remove the driver from soc_sleep_stats_driver name
- Remove offset address and directly mention the msgram address in dtsi
- Use devm_platform_get_and_ioremap_resource() to ioremap dtsi address
- Update device node name to mention aop_msgram instead rpmh-sleep-stats
- Update dtsi and documentation accordingly but retain the reviews

Changes in v8:
- Addressed bjorn's comments in driver from v7
- Update aoss_qmp device node reg size for sc7280

Changes in v7:
- Fix example in bindings documentation as per #address/size-cells = <1>.
- Add comment in driver from where 'ddr' subsystems name is read.
- Update comment in driver to s/beside/besides and others from v6.
- Rename debugfs_create_entries() from v6.
- Drop use of memcpy_fromio() to find the name.
- Use sizeof(*prv_data) in devm_kzalloc().
- Add change to define readq() if its not yet defined for compile support.
- Add wpss subsystem in the list of subsystems.
- Add module soft dependency on smem module.
- Add new change to add device node for sc7280.

Changes in v6:
- Address stephen's comments from v5 which includes below
- Pad 0 in documentation example to make address 8 digit
- define macro to calculate offset in driver
- Add appended_stats_avail to prv_data instead of using entire stats_config
- make array subsystems[] as const
- Add comment for SSR case
- Use memcpy_fromio() and devm_kcalloc() during probe
- Change file permission mode from 444 to 400

- Address guenter's comments to add depends on QCOM_SMEM

- Add adsp_island and cdsp_island subsystems
- Use strim() to remove whitespace in stat name

Changes in v5:
- Remove underscore from node name in Documentation and DTSI change
- Remove global config from driver change

Changes in v4:
- Address bjorn's comments from v3 on change 2.
- Add bjorn's Reviewed-by on change 3 and 4.

Changes in v3:
- Address stephen's comments from v2 in change 1 and 2.
- Address bjorn's comments from v2 in change 3 and 4.
- Add Rob and bjorn's Reviewed-by on YAML change.

Changes in v2:
- Convert Documentation to YAML.
- Address stephen's comments from v1.
- Use debugfs instead of sysfs.
- Add sc7180 dts changes for sleep stats
- Add defconfig changes to enable driver
- Include subsystem stats from [1] in this single stats driver.
- Address stephen's comments from [1]
- Update cover letter inline to mention [1]

Qualcomm Technologies, Inc. (QTI)'s chipsets support SoC level low power
modes. SoCs Always On Processor/Resource Power Manager produces statistics
of the SoC sleep modes involving lowering or powering down of the rails and
the oscillator clock.

Additionally multiple subsystems present on SoC like modem, spss, adsp,
cdsp maintains their low power mode statistics in shared memory (SMEM).

Statistics includes SoC sleep mode type, number of times LPM entered, time
of last entry, exit, and accumulated sleep duration in seconds.

This series adds a driver to read the stats and export to debugfs.

[1] https://lore.kernel.org/patchwork/patch/1149381/

Mahesh Sivasubramanian (2):
dt-bindings: Introduce SoC sleep stats bindings
soc: qcom: Add SoC sleep stats driver

Maulik Shah (3):
arm64: dts: qcom: sc7180: Enable SoC sleep stats
arm64: defconfig: Enable SoC sleep stats driver
arm64: dts: qcom: sc7280: Enable SoC sleep stats

.../bindings/soc/qcom/soc-sleep-stats.yaml | 48 ++++
arch/arm64/boot/dts/qcom/sc7180.dtsi | 7 +-
arch/arm64/boot/dts/qcom/sc7280.dtsi | 7 +-
arch/arm64/configs/defconfig | 1 +
drivers/soc/qcom/Kconfig | 10 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/soc_sleep_stats.c | 241 +++++++++++++++++++++
7 files changed, 313 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml
create mode 100644 drivers/soc/qcom/soc_sleep_stats.c

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


2021-09-06 05:30:10

by Maulik Shah

[permalink] [raw]
Subject: [PATCH v9 2/5] soc: qcom: Add SoC sleep stats driver

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/soc_sleep_stats.c | 241 +++++++++++++++++++++++++++++++++++++
3 files changed, 252 insertions(+)
create mode 100644 drivers/soc/qcom/soc_sleep_stats.c

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 79b568f..e80b63a 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -190,6 +190,16 @@ config QCOM_SOCINFO
Say yes here to support the Qualcomm socinfo driver, providing
information about the SoC to user space.

+config QCOM_SOC_SLEEP_STATS
+ tristate "Qualcomm Technologies, Inc. (QTI) SoC sleep stats driver"
+ depends on ARCH_QCOM && DEBUG_FS || COMPILE_TEST
+ depends on QCOM_SMEM
+ help
+ Qualcomm Technologies, Inc. (QTI) SoC 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_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 ad675a6..5f30d74 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
obj-$(CONFIG_QCOM_SMSM) += smsm.o
obj-$(CONFIG_QCOM_SOCINFO) += socinfo.o
+obj-$(CONFIG_QCOM_SOC_SLEEP_STATS) += soc_sleep_stats.o
obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
obj-$(CONFIG_QCOM_APR) += apr.o
obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
new file mode 100644
index 0000000..db28dd2
--- /dev/null
+++ b/drivers/soc/qcom/soc_sleep_stats.c
@@ -0,0 +1,241 @@
+// 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 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 offset_addr;
+ size_t num_records;
+ bool appended_stats_avail;
+};
+
+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, u32 num_records)
+{
+ char stat_type[sizeof(u32) + 1] = {0};
+ u32 offset = 0, type;
+ int i, j;
+
+ for (i = 0; i < num_records; i++) {
+ d[i].base = reg + 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_soc_sleep_stats_probe(struct platform_device *pdev)
+{
+ struct resource *res;
+ 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;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return PTR_ERR(res);
+
+ reg = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
+ if (!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->num_records);
+
+ platform_set_drvdata(pdev, root);
+
+ return 0;
+}
+
+static int qcom_soc_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 = {
+ .num_records = 2,
+ .appended_stats_avail = true,
+};
+
+static const struct stats_config rpmh_data = {
+ .num_records = 3,
+ .appended_stats_avail = false,
+};
+
+static const struct of_device_id qcom_soc_sleep_stats_table[] = {
+ { .compatible = "qcom,rpm-sleep-stats", .data = &rpm_data },
+ { .compatible = "qcom,rpmh-sleep-stats", .data = &rpmh_data },
+ { }
+};
+
+static struct platform_driver soc_sleep_stats = {
+ .probe = qcom_soc_sleep_stats_probe,
+ .remove = qcom_soc_sleep_stats_remove,
+ .driver = {
+ .name = "soc_sleep_stats",
+ .of_match_table = qcom_soc_sleep_stats_table,
+ },
+};
+module_platform_driver(soc_sleep_stats);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) SoC 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

2021-09-06 05:30:59

by Maulik Shah

[permalink] [raw]
Subject: [PATCH v9 4/5] arm64: defconfig: Enable SoC sleep stats driver

Enable SoC sleep stats driver. The driver gives statistics for
various low power modes on Qualcomm Technologies, Inc. (QTI) SoCs.

Signed-off-by: Maulik Shah <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Reviewed-by: Stephen Boyd <[email protected]>
---
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 3bf6f8a..ba7c403 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1040,6 +1040,7 @@ CONFIG_QCOM_SMD_RPM=y
CONFIG_QCOM_SMP2P=y
CONFIG_QCOM_SMSM=y
CONFIG_QCOM_SOCINFO=m
+CONFIG_QCOM_SOC_SLEEP_STATS=m
CONFIG_QCOM_APR=m
CONFIG_ARCH_R8A774A1=y
CONFIG_ARCH_R8A774B1=y
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

2021-09-06 05:39:48

by Maulik Shah

[permalink] [raw]
Subject: [PATCH v9 5/5] arm64: dts: qcom: sc7280: Enable SoC sleep stats

Add device node for SoC sleep stats driver which provides various
low power mode stats.

Also update the reg size of aoss_qmp device to 0x400.

Cc: [email protected]
Signed-off-by: Maulik Shah <[email protected]>
---
arch/arm64/boot/dts/qcom/sc7280.dtsi | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 53a21d0..aba7dd1 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -1473,7 +1473,7 @@

aoss_qmp: power-controller@c300000 {
compatible = "qcom,sc7280-aoss-qmp";
- reg = <0 0x0c300000 0 0x100000>;
+ reg = <0 0x0c300000 0 0x400>;
interrupts-extended = <&ipcc IPCC_CLIENT_AOP
IPCC_MPROC_SIGNAL_GLINK_QMP
IRQ_TYPE_EDGE_RISING>;
@@ -1484,6 +1484,11 @@
#power-domain-cells = <1>;
};

+ aop_msgram@c3f0048 {
+ compatible = "qcom,rpmh-sleep-stats";
+ reg = <0 0x0c3f0048 0 0x400>;
+ };
+
spmi_bus: spmi@c440000 {
compatible = "qcom,spmi-pmic-arb";
reg = <0 0x0c440000 0 0x1100>,
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

2021-09-25 06:38:37

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH v9 2/5] soc: qcom: Add SoC sleep stats driver

On Mon 06 Sep 00:28 CDT 2021, 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/soc_sleep_stats.c | 241 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 252 insertions(+)
> create mode 100644 drivers/soc/qcom/soc_sleep_stats.c
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 79b568f..e80b63a 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -190,6 +190,16 @@ config QCOM_SOCINFO
> Say yes here to support the Qualcomm socinfo driver, providing
> information about the SoC to user space.
>
> +config QCOM_SOC_SLEEP_STATS
> + tristate "Qualcomm Technologies, Inc. (QTI) SoC sleep stats driver"
> + depends on ARCH_QCOM && DEBUG_FS || COMPILE_TEST
> + depends on QCOM_SMEM
> + help
> + Qualcomm Technologies, Inc. (QTI) SoC 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_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 ad675a6..5f30d74 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
> obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
> obj-$(CONFIG_QCOM_SMSM) += smsm.o
> obj-$(CONFIG_QCOM_SOCINFO) += socinfo.o
> +obj-$(CONFIG_QCOM_SOC_SLEEP_STATS) += soc_sleep_stats.o

I know that the rest of the modules here does a bad job and have
completely generic names, but could we rename this "qcom_sleep_stats"
instead?

> obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
> obj-$(CONFIG_QCOM_APR) += apr.o
> obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
[..]
> +static int qcom_soc_sleep_stats_probe(struct platform_device *pdev)
> +{
> + struct resource *res;
> + 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;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return PTR_ERR(res);

You no longer use this "res".

> +
> + reg = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> + if (!reg)

IS_ERR()

> + 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->num_records);
> +
> + platform_set_drvdata(pdev, root);
> +
> + return 0;
> +}
> +
> +static int qcom_soc_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 = {
> + .num_records = 2,
> + .appended_stats_avail = true,
> +};
> +
> +static const struct stats_config rpmh_data = {
> + .num_records = 3,
> + .appended_stats_avail = false,
> +};
> +
> +static const struct of_device_id qcom_soc_sleep_stats_table[] = {
> + { .compatible = "qcom,rpm-sleep-stats", .data = &rpm_data },
> + { .compatible = "qcom,rpmh-sleep-stats", .data = &rpmh_data },
> + { }
> +};

MODULE_DEVICE_TABLE(of, qcom_soc_sleep_stats_table);

Otherwise the module doesn't load automatically.

Regards,
Bjorn

> +
> +static struct platform_driver soc_sleep_stats = {
> + .probe = qcom_soc_sleep_stats_probe,
> + .remove = qcom_soc_sleep_stats_remove,
> + .driver = {
> + .name = "soc_sleep_stats",
> + .of_match_table = qcom_soc_sleep_stats_table,
> + },
> +};
> +module_platform_driver(soc_sleep_stats);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) SoC 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
>

2021-10-05 08:58:26

by Maulik Shah

[permalink] [raw]
Subject: Re: [PATCH v9 2/5] soc: qcom: Add SoC sleep stats driver

Hi,

On 9/25/2021 4:50 AM, Bjorn Andersson wrote:
> On Mon 06 Sep 00:28 CDT 2021, 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/soc_sleep_stats.c | 241 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 252 insertions(+)
>> create mode 100644 drivers/soc/qcom/soc_sleep_stats.c
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index 79b568f..e80b63a 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -190,6 +190,16 @@ config QCOM_SOCINFO
>> Say yes here to support the Qualcomm socinfo driver, providing
>> information about the SoC to user space.
>>
>> +config QCOM_SOC_SLEEP_STATS
>> + tristate "Qualcomm Technologies, Inc. (QTI) SoC sleep stats driver"
>> + depends on ARCH_QCOM && DEBUG_FS || COMPILE_TEST
>> + depends on QCOM_SMEM
>> + help
>> + Qualcomm Technologies, Inc. (QTI) SoC 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_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 ad675a6..5f30d74 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -20,6 +20,7 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
>> obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
>> obj-$(CONFIG_QCOM_SMSM) += smsm.o
>> obj-$(CONFIG_QCOM_SOCINFO) += socinfo.o
>> +obj-$(CONFIG_QCOM_SOC_SLEEP_STATS) += soc_sleep_stats.o
>
> I know that the rest of the modules here does a bad job and have
> completely generic names, but could we rename this "qcom_sleep_stats"
> instead?

Sure renamed in v10.

>
>> obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
>> obj-$(CONFIG_QCOM_APR) += apr.o
>> obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
>> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
> [..]
>> +static int qcom_soc_sleep_stats_probe(struct platform_device *pdev)
>> +{
>> + struct resource *res;
>> + 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;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res)
>> + return PTR_ERR(res);
>
> You no longer use this "res".

Thanks for catching this, removed in v10.

>
>> +
>> + reg = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
>> + if (!reg)
>
> IS_ERR()

Updated in v10 to use IS_ERR().

>
>> + 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->num_records);
>> +
>> + platform_set_drvdata(pdev, root);
>> +
>> + return 0;
>> +}
>> +
>> +static int qcom_soc_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 = {
>> + .num_records = 2,
>> + .appended_stats_avail = true,
>> +};
>> +
>> +static const struct stats_config rpmh_data = {
>> + .num_records = 3,
>> + .appended_stats_avail = false,
>> +};
>> +
>> +static const struct of_device_id qcom_soc_sleep_stats_table[] = {
>> + { .compatible = "qcom,rpm-sleep-stats", .data = &rpm_data },
>> + { .compatible = "qcom,rpmh-sleep-stats", .data = &rpmh_data },
>> + { }
>> +};
>
> MODULE_DEVICE_TABLE(of, qcom_soc_sleep_stats_table);
>
> Otherwise the module doesn't load automatically.
>
> Regards,
> Bjorn

Added MODULE_DEVICE_TABLE in v10.

Thanks,
Maulik

>
>> +
>> +static struct platform_driver soc_sleep_stats = {
>> + .probe = qcom_soc_sleep_stats_probe,
>> + .remove = qcom_soc_sleep_stats_remove,
>> + .driver = {
>> + .name = "soc_sleep_stats",
>> + .of_match_table = qcom_soc_sleep_stats_table,
>> + },
>> +};
>> +module_platform_driver(soc_sleep_stats);
>> +
>> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) SoC 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
>>

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