[PATCH v8 0/5] Introduce SoC sleep stats driver

[PATCH v8 0/5] Introduce SoC sleep stats driver

[Maulik Shah]

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                 | 255 +++++++++++++++++++++
 7 files changed, 327 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

[PATCH v8 1/5] dt-bindings: Introduce SoC sleep stats bindings

[Maulik Shah]

From: Mahesh Sivasubramanian <msivasub@codeaurora.org>

Add device binding documentation for Qualcomm Technologies, Inc. (QTI)
SoC sleep stats driver. The driver is used for displaying SoC sleep
statistic maintained by Always On Processor or Resource Power Manager.

Cc: devicetree@vger.kernel.org
Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
 .../bindings/soc/qcom/soc-sleep-stats.yaml         | 48 ++++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml

diff --git a/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml
new file mode 100644
index 0000000..9078c4f
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml
@@ -0,0 +1,48 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/qcom/soc-sleep-stats.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. (QTI) SoC sleep stats bindings
+
+maintainers:
+  - Maulik Shah <mkshah@codeaurora.org>
+  - Lina Iyer <ilina@codeaurora.org>
+
+description:
+  Always On Processor/Resource Power Manager maintains statistics of the SoC
+  sleep modes involving powering down of the rails and oscillator clock.
+
+  Statistics includes SoC sleep mode type, number of times low power mode were
+  entered, time of last entry, time of last exit and accumulated sleep duration.
+
+properties:
+  compatible:
+    enum:
+      - qcom,rpmh-sleep-stats
+      - qcom,rpm-sleep-stats
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  # Example of rpmh sleep stats
+  - |
+    rpmh-sleep-stats@c3f0000 {
+      compatible = "qcom,rpmh-sleep-stats";
+      reg = <0x0c3f0000 0x400>;
+    };
+  # Example of rpm sleep stats
+  - |
+    rpm-sleep-stats@4690000 {
+      compatible = "qcom,rpm-sleep-stats";
+      reg = <0x04690000 0x400>;
+    };
+...
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v8 2/5] soc: qcom: Add SoC sleep stats driver

[Maulik Shah]

From: Mahesh Sivasubramanian <msivasub@codeaurora.org>

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 <msivasub@codeaurora.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
[mkshah: add subsystem sleep stats, create one file for each stat]
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/soc/qcom/Kconfig           |  10 ++
 drivers/soc/qcom/Makefile          |   1 +
 drivers/soc/qcom/soc_sleep_stats.c | 255 +++++++++++++++++++++++++++++++++++++
 3 files changed, 266 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..a8396f9
--- /dev/null
+++ b/drivers/soc/qcom/soc_sleep_stats.c
@@ -0,0 +1,255 @@
+// 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 PTR_ERR(stat);
+
+	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)
+{
+	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;
+	void __iomem *offset_addr;
+	phys_addr_t stats_base;
+	resource_size_t stats_size;
+	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);
+
+	offset_addr = ioremap(res->start + config->offset_addr, sizeof(u32));
+	if (IS_ERR(offset_addr))
+		return PTR_ERR(offset_addr);
+
+	stats_base = res->start | readl_relaxed(offset_addr);
+	stats_size = resource_size(res);
+	iounmap(offset_addr);
+
+	reg = devm_ioremap(&pdev->dev, stats_base, stats_size);
+	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 = {
+	.offset_addr = 0x14,
+	.num_records = 2,
+	.appended_stats_avail = true,
+};
+
+static const struct stats_config rpmh_data = {
+	.offset_addr = 0x4,
+	.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_driver = {
+	.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_driver);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) SoC Sleep Stats driver");
+MODULE_LICENSE("GPL v2");
+MODULE_SOFTDEP("pre: smem");
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v8 2/5] soc: qcom: Add SoC sleep stats driver

[Stephen Boyd]

Quoting Maulik Shah (2021-05-21 04:26:08)
> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
> new file mode 100644
> index 0000000..a8396f9
> --- /dev/null
> +++ b/drivers/soc/qcom/soc_sleep_stats.c
> @@ -0,0 +1,255 @@
> +// 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;

Is this coming from memory directly? So we need a bunch of __le32 and
__le64 markings and then the endian swaps?

> +};
> +
> +struct appended_stats {
> +       u32 client_votes;
> +       u32 reserved[3];

This one too? It's stored in smem?

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

Doesn't this read the virtual count? Which could be very different than
the physical count that the subsystems see and then write into smem? I
suppose we need some sort of "physical count" reading API to match up
the timestamps that are there but that is sort of hard given that the
physical count isn't always intended to be read by the kernel. Maybe we
should split off an MMIO architected timer to be used by this code that
uses the same clocksource that the other subsystems are using?

> +
> +       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 PTR_ERR(stat);

Can this return EPROBE_DEFER to userspace? That would be pretty weird.
Maybe we should convert the return value to -EIO regardless of the error.

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

What do we do if there are two same named low power modes? Maybe they
should be directories instead with pretty name and values inside of
them.

> +                * 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)
> +{
> +       struct sleep_stats *stat;

const? The pointer isn't used though so is this even relevant?

> +       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],

Do we need to cast to void pointer? Is it unused? If it's unused then
pass NULL?

> +                                   &qcom_subsystem_sleep_stats_fops);
> +       }
> +}
> +
> +static int qcom_soc_sleep_stats_probe(struct platform_device *pdev)
> +{
> +       struct resource *res;
> +       void __iomem *reg;
> +       void __iomem *offset_addr;
> +       phys_addr_t stats_base;
> +       resource_size_t stats_size;
> +       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);
> +
> +       offset_addr = ioremap(res->start + config->offset_addr, sizeof(u32));

Why can't we devm_platform_get_and_ioremap_resource()?

> +       if (IS_ERR(offset_addr))
> +               return PTR_ERR(offset_addr);
> +
> +       stats_base = res->start | readl_relaxed(offset_addr);
> +       stats_size = resource_size(res);
> +       iounmap(offset_addr);

Oh because of unmap? Can we add a resource for stats_base/size to the
system? I'd really like to see what address that is at. I think it is
actually within the same region of this "pointer" that is assigned in
DT, but I honestly don't know why we wouldn't just hardcode the address
in DT to begin with instead of doing this map, find the real address,
and then map that. Is this really changing that often? Probably it
changes a few times during development but then it settles down and we
can just hardcode it in DT? At least please make a resource and reserve
it so we know that the region isn't being used by other kernel drivers.

> +
> +       reg = devm_ioremap(&pdev->dev, stats_base, stats_size);
> +       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 = {
> +       .offset_addr = 0x14,
> +       .num_records = 2,
> +       .appended_stats_avail = true,
> +};
> +
> +static const struct stats_config rpmh_data = {
> +       .offset_addr = 0x4,
> +       .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_driver = {

A driver seems pretty heavyweight for a thing that sticks stuff into
debugfs. It's not "driving" anything.

> +       .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_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) SoC Sleep Stats driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_SOFTDEP("pre: smem");

Re: [PATCH v8 2/5] soc: qcom: Add SoC sleep stats driver

[Maulik Shah]

Hi,

On 5/27/2021 5:15 AM, Stephen Boyd wrote:
> Quoting Maulik Shah (2021-05-21 04:26:08)
>> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
>> new file mode 100644
>> index 0000000..a8396f9
>> --- /dev/null
>> +++ b/drivers/soc/qcom/soc_sleep_stats.c
>> @@ -0,0 +1,255 @@
>> +// 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;
> Is this coming from memory directly? So we need a bunch of __le32 and
> __le64 markings and then the endian swaps?
Its stored in AOP (RPMH)/ RPM msgram for SoC sleep stats and individual 
subsystem stats stored in SMEM.
>
>> +};
>> +
>> +struct appended_stats {
>> +       u32 client_votes;
>> +       u32 reserved[3];
> This one too? It's stored in smem?
Its only stored in RPM msgram. RPMH and SMEM do not have it.
>
>> +};
>> +
>> +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;
> Doesn't this read the virtual count? Which could be very different than
> the physical count that the subsystems see and then write into smem? I
> suppose we need some sort of "physical count" reading API to match up
> the timestamps that are there but that is sort of hard given that the
> physical count isn't always intended to be read by the kernel. Maybe we
> should split off an MMIO architected timer to be used by this code that
> uses the same clocksource that the other subsystems are using?
The timer used is same across all subsystems. Beside it try to give more 
accurate information by modifying
total sleep length if subsytem is staying in sleep.
>
>> +
>> +       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 PTR_ERR(stat);
> Can this return EPROBE_DEFER to userspace? That would be pretty weird.
> Maybe we should convert the return value to -EIO regardless of the error.
Ok. Updated in v9 to 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.
> What do we do if there are two same named low power modes? Maybe they
> should be directories instead with pretty name and values inside of
> them.

This is agreed between AOP and Linux kernel so we would not have two or 
more same named low power modes.

This applies to many things like the structure defination which also be 
agreed and AOP/subsytems populates same

structure in MSGRAM / SMEM respectively which kernel can read in same 
format.

>
>> +                * 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)
>> +{
>> +       struct sleep_stats *stat;
> const? The pointer isn't used though so is this even relevant?
Changed to const.
>
>> +       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],
> Do we need to cast to void pointer? Is it unused? If it's unused then
> pass NULL?
It is used. It needs cast too since subsystems array is already a const, 
without cast will compiler will complain discarding const.
>
>> +                                   &qcom_subsystem_sleep_stats_fops);
>> +       }
>> +}
>> +
>> +static int qcom_soc_sleep_stats_probe(struct platform_device *pdev)
>> +{
>> +       struct resource *res;
>> +       void __iomem *reg;
>> +       void __iomem *offset_addr;
>> +       phys_addr_t stats_base;
>> +       resource_size_t stats_size;
>> +       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);
>> +
>> +       offset_addr = ioremap(res->start + config->offset_addr, sizeof(u32));
> Why can't we devm_platform_get_and_ioremap_resource()?
Ok, updated in v9.
>
>> +       if (IS_ERR(offset_addr))
>> +               return PTR_ERR(offset_addr);
>> +
>> +       stats_base = res->start | readl_relaxed(offset_addr);
>> +       stats_size = resource_size(res);
>> +       iounmap(offset_addr);
> Oh because of unmap? Can we add a resource for stats_base/size to the
> system? I'd really like to see what address that is at.
Ok it was developed like this since AOP can populate the stat at 
different address and it gives this address when reading the value at 
offset.
> I think it is
> actually within the same region of this "pointer" that is assigned in
> DT, but I honestly don't know why we wouldn't just hardcode the address
> in DT to begin with instead of doing this map, find the real address,
> and then map that. Is this really changing that often?

yes its in the same region. it generally don't change for single target 
but prone to change for other targets.

I have hardcoded that address it self in DT to start with and removed 
the offset part from driver.

>   Probably it
> changes a few times during development but then it settles down and we
> can just hardcode it in DT? At least please make a resource and reserve
> it so we know that the region isn't being used by other kernel drivers.
>
>> +
>> +       reg = devm_ioremap(&pdev->dev, stats_base, stats_size);
>> +       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 = {
>> +       .offset_addr = 0x14,
>> +       .num_records = 2,
>> +       .appended_stats_avail = true,
>> +};
>> +
>> +static const struct stats_config rpmh_data = {
>> +       .offset_addr = 0x4,
>> +       .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_driver = {
> A driver seems pretty heavyweight for a thing that sticks stuff into
> debugfs. It's not "driving" anything.

Removed the driver from the end.

Thanks,
Maulik

>
>> +       .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_driver);
>> +
>> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) SoC Sleep Stats driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_SOFTDEP("pre: smem");

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

Re: [PATCH v8 2/5] soc: qcom: Add SoC sleep stats driver

[Bjorn Andersson]

On Fri 21 May 06:26 CDT 2021, Maulik Shah wrote:

> From: Mahesh Sivasubramanian <msivasub@codeaurora.org>
> 
> 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 <msivasub@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> [mkshah: add subsystem sleep stats, create one file for each stat]
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
>  drivers/soc/qcom/Kconfig           |  10 ++
>  drivers/soc/qcom/Makefile          |   1 +
>  drivers/soc/qcom/soc_sleep_stats.c | 255 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 266 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..a8396f9
> --- /dev/null
> +++ b/drivers/soc/qcom/soc_sleep_stats.c
> @@ -0,0 +1,255 @@
> +// 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 PTR_ERR(stat);
> +
> +	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)
> +{
> +	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;
> +	void __iomem *offset_addr;
> +	phys_addr_t stats_base;
> +	resource_size_t stats_size;
> +	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);
> +
> +	offset_addr = ioremap(res->start + config->offset_addr, sizeof(u32));
> +	if (IS_ERR(offset_addr))
> +		return PTR_ERR(offset_addr);
> +
> +	stats_base = res->start | readl_relaxed(offset_addr);
> +	stats_size = resource_size(res);
> +	iounmap(offset_addr);
> +
> +	reg = devm_ioremap(&pdev->dev, stats_base, stats_size);
> +	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 = {
> +	.offset_addr = 0x14,
> +	.num_records = 2,
> +	.appended_stats_avail = true,
> +};
> +
> +static const struct stats_config rpmh_data = {
> +	.offset_addr = 0x4,
> +	.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_driver = {
> +	.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_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) SoC Sleep Stats driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_SOFTDEP("pre: smem");

I still think the qcom_smem_* function calls results in a harder
dependency than this softdep.

Regards,
Bjorn

[PATCH v8 3/5] arm64: dts: qcom: sc7180: Enable SoC sleep stats

[Maulik Shah]

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: devicetree@vger.kernel.org
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 arch/arm64/boot/dts/qcom/sc7180.dtsi | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 6228ba2..889d04d 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -3215,7 +3215,7 @@
 
 		aoss_qmp: power-controller@c300000 {
 			compatible = "qcom,sc7180-aoss-qmp";
-			reg = <0 0x0c300000 0 0x100000>;
+			reg = <0 0x0c300000 0 0x400>;
 			interrupts = <GIC_SPI 389 IRQ_TYPE_EDGE_RISING>;
 			mboxes = <&apss_shared 0>;
 
@@ -3223,6 +3223,11 @@
 			#power-domain-cells = <1>;
 		};
 
+		rpmh-sleep-stats@c3f0000 {
+			compatible = "qcom,rpmh-sleep-stats";
+			reg = <0 0x0c3f0000 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

Re: [PATCH v8 3/5] arm64: dts: qcom: sc7180: Enable SoC sleep stats

[Stephen Boyd]

Quoting Maulik Shah (2021-05-21 04:26:09)
> 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: devicetree@vger.kernel.org
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sc7180.dtsi | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index 6228ba2..889d04d 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -3215,7 +3215,7 @@
>
>                 aoss_qmp: power-controller@c300000 {
>                         compatible = "qcom,sc7180-aoss-qmp";
> -                       reg = <0 0x0c300000 0 0x100000>;
> +                       reg = <0 0x0c300000 0 0x400>;
>                         interrupts = <GIC_SPI 389 IRQ_TYPE_EDGE_RISING>;
>                         mboxes = <&apss_shared 0>;
>
> @@ -3223,6 +3223,11 @@
>                         #power-domain-cells = <1>;
>                 };
>
> +               rpmh-sleep-stats@c3f0000 {
> +                       compatible = "qcom,rpmh-sleep-stats";
> +                       reg = <0 0x0c3f0000 0 0x400>;
> +               };
> +

Does this need to be in DT? Can the sc7180-aoss-qmp driver use the
aux-bus and stick the sleep stats device on there?

>                 spmi_bus: spmi@c440000 {
>                         compatible = "qcom,spmi-pmic-arb";
>                         reg = <0 0x0c440000 0 0x1100>,

Re: [PATCH v8 3/5] arm64: dts: qcom: sc7180: Enable SoC sleep stats

[Bjorn Andersson]

On Wed 26 May 18:30 CDT 2021, Stephen Boyd wrote:

> Quoting Maulik Shah (2021-05-21 04:26:09)
> > 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: devicetree@vger.kernel.org
> > Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sc7180.dtsi | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > index 6228ba2..889d04d 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > @@ -3215,7 +3215,7 @@
> >
> >                 aoss_qmp: power-controller@c300000 {
> >                         compatible = "qcom,sc7180-aoss-qmp";
> > -                       reg = <0 0x0c300000 0 0x100000>;
> > +                       reg = <0 0x0c300000 0 0x400>;
> >                         interrupts = <GIC_SPI 389 IRQ_TYPE_EDGE_RISING>;
> >                         mboxes = <&apss_shared 0>;
> >
> > @@ -3223,6 +3223,11 @@
> >                         #power-domain-cells = <1>;
> >                 };
> >
> > +               rpmh-sleep-stats@c3f0000 {
> > +                       compatible = "qcom,rpmh-sleep-stats";
> > +                       reg = <0 0x0c3f0000 0 0x400>;
> > +               };
> > +
> 
> Does this need to be in DT? Can the sc7180-aoss-qmp driver use the
> aux-bus and stick the sleep stats device on there?
> 

The AOSS memory space has N chunks of "message ram", one is used for the
QMP protocol (presumably the APSS specific one), a different one is used
for the sleep stats.

I presume we could have come up with a binding for the entire AOSS/AOP
and then describe (either implicit or explicitly) the QMP and
debug-stats under that.

But we'd also have to come up with the same container-device for the RPM
case.

Regards,
Bjorn

> >                 spmi_bus: spmi@c440000 {
> >                         compatible = "qcom,spmi-pmic-arb";
> >                         reg = <0 0x0c440000 0 0x1100>,

Re: [PATCH v8 3/5] arm64: dts: qcom: sc7180: Enable SoC sleep stats

[Stephen Boyd]

Quoting Bjorn Andersson (2021-05-31 10:57:03)
> On Wed 26 May 18:30 CDT 2021, Stephen Boyd wrote:
>
> > Quoting Maulik Shah (2021-05-21 04:26:09)
> > > 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: devicetree@vger.kernel.org
> > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/sc7180.dtsi | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > index 6228ba2..889d04d 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > @@ -3215,7 +3215,7 @@
> > >
> > >                 aoss_qmp: power-controller@c300000 {
> > >                         compatible = "qcom,sc7180-aoss-qmp";
> > > -                       reg = <0 0x0c300000 0 0x100000>;
> > > +                       reg = <0 0x0c300000 0 0x400>;
> > >                         interrupts = <GIC_SPI 389 IRQ_TYPE_EDGE_RISING>;
> > >                         mboxes = <&apss_shared 0>;
> > >
> > > @@ -3223,6 +3223,11 @@
> > >                         #power-domain-cells = <1>;
> > >                 };
> > >
> > > +               rpmh-sleep-stats@c3f0000 {
> > > +                       compatible = "qcom,rpmh-sleep-stats";
> > > +                       reg = <0 0x0c3f0000 0 0x400>;
> > > +               };
> > > +
> >
> > Does this need to be in DT? Can the sc7180-aoss-qmp driver use the
> > aux-bus and stick the sleep stats device on there?
> >
>
> The AOSS memory space has N chunks of "message ram", one is used for the
> QMP protocol (presumably the APSS specific one), a different one is used
> for the sleep stats.
>
> I presume we could have come up with a binding for the entire AOSS/AOP
> and then describe (either implicit or explicitly) the QMP and
> debug-stats under that.
>
> But we'd also have to come up with the same container-device for the RPM
> case.

Because the rpm node doesn't include this region of memory today? I
still fail to see why we're changing the existing binding and adding a
DT node for this new region that is basically a debug feature.

Re: [PATCH v8 3/5] arm64: dts: qcom: sc7180: Enable SoC sleep stats

[Bjorn Andersson]

On Wed 02 Jun 19:26 CDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-05-31 10:57:03)
> > On Wed 26 May 18:30 CDT 2021, Stephen Boyd wrote:
> >
> > > Quoting Maulik Shah (2021-05-21 04:26:09)
> > > > 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: devicetree@vger.kernel.org
> > > > Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> > > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > ---
> > > >  arch/arm64/boot/dts/qcom/sc7180.dtsi | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > > index 6228ba2..889d04d 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > > > @@ -3215,7 +3215,7 @@
> > > >
> > > >                 aoss_qmp: power-controller@c300000 {
> > > >                         compatible = "qcom,sc7180-aoss-qmp";
> > > > -                       reg = <0 0x0c300000 0 0x100000>;
> > > > +                       reg = <0 0x0c300000 0 0x400>;
> > > >                         interrupts = <GIC_SPI 389 IRQ_TYPE_EDGE_RISING>;
> > > >                         mboxes = <&apss_shared 0>;
> > > >
> > > > @@ -3223,6 +3223,11 @@
> > > >                         #power-domain-cells = <1>;
> > > >                 };
> > > >
> > > > +               rpmh-sleep-stats@c3f0000 {
> > > > +                       compatible = "qcom,rpmh-sleep-stats";
> > > > +                       reg = <0 0x0c3f0000 0 0x400>;
> > > > +               };
> > > > +
> > >
> > > Does this need to be in DT? Can the sc7180-aoss-qmp driver use the
> > > aux-bus and stick the sleep stats device on there?
> > >
> >
> > The AOSS memory space has N chunks of "message ram", one is used for the
> > QMP protocol (presumably the APSS specific one), a different one is used
> > for the sleep stats.
> >
> > I presume we could have come up with a binding for the entire AOSS/AOP
> > and then describe (either implicit or explicitly) the QMP and
> > debug-stats under that.
> >
> > But we'd also have to come up with the same container-device for the RPM
> > case.
> 
> Because the rpm node doesn't include this region of memory today? I
> still fail to see why we're changing the existing binding and adding a
> DT node for this new region that is basically a debug feature.

We're not changing the binding, the memory region for the "AOSS QMP"
thing was never larger than 0x400.

0x100000 is the size of all the AOSS "msg_ram" regions. We don't have
this whole thing described in a binding and we don't have an
implementation for the whole thing.

If we're going for that we'd need to extend the binding to indicate
which of the msg_ram regions are used for APSS QMP and for debug stats
on particular platform (either by compatible, explicit properties or as
some subnodes).


That said, as I looked into my other objection, for the RPM
(non-hardened) case it seems that we're actually describing the RPM
region. So there it would make sense to describe it as such in DT - but
we don't have any other code (that I'm aware of) that would implement
the "qcom,<platform>-rpm".

Regards,
Bjorn

Re: [PATCH v8 3/5] arm64: dts: qcom: sc7180: Enable SoC sleep stats

[Stephen Boyd]

Quoting Bjorn Andersson (2021-06-02 19:44:40)
> On Wed 02 Jun 19:26 CDT 2021, Stephen Boyd wrote:
>
> > Quoting Bjorn Andersson (2021-05-31 10:57:03)
> > > On Wed 26 May 18:30 CDT 2021, Stephen Boyd wrote:
> > >
> > > > Quoting Maulik Shah (2021-05-21 04:26:09)
> > > > > @@ -3223,6 +3223,11 @@
> > > > >                         #power-domain-cells = <1>;
> > > > >                 };
> > > > >
> > > > > +               rpmh-sleep-stats@c3f0000 {
> > > > > +                       compatible = "qcom,rpmh-sleep-stats";
> > > > > +                       reg = <0 0x0c3f0000 0 0x400>;
> > > > > +               };
> > > > > +
> > > >
> > > > Does this need to be in DT? Can the sc7180-aoss-qmp driver use the
> > > > aux-bus and stick the sleep stats device on there?
> > > >
> > >
> > > The AOSS memory space has N chunks of "message ram", one is used for the
> > > QMP protocol (presumably the APSS specific one), a different one is used
> > > for the sleep stats.
> > >
> > > I presume we could have come up with a binding for the entire AOSS/AOP
> > > and then describe (either implicit or explicitly) the QMP and
> > > debug-stats under that.
> > >
> > > But we'd also have to come up with the same container-device for the RPM
> > > case.
> >
> > Because the rpm node doesn't include this region of memory today? I
> > still fail to see why we're changing the existing binding and adding a
> > DT node for this new region that is basically a debug feature.
>
> We're not changing the binding, the memory region for the "AOSS QMP"
> thing was never larger than 0x400.
>
> 0x100000 is the size of all the AOSS "msg_ram" regions. We don't have
> this whole thing described in a binding and we don't have an
> implementation for the whole thing.
>
> If we're going for that we'd need to extend the binding to indicate
> which of the msg_ram regions are used for APSS QMP and for debug stats
> on particular platform (either by compatible, explicit properties or as
> some subnodes).

Fair enough. At the least, can we change the name of the node then to
'sram' or 'ram'? The 'rpmh-sleep-stats' node name is nonsense.

>
>
> That said, as I looked into my other objection, for the RPM
> (non-hardened) case it seems that we're actually describing the RPM
> region. So there it would make sense to describe it as such in DT - but
> we don't have any other code (that I'm aware of) that would implement
> the "qcom,<platform>-rpm".
>

I only half parsed this part. Are you saying that because we don't have
a driver for qcom,<platform>-rpm we shouldn't keep it all within the rpm
node?

Re: [PATCH v8 3/5] arm64: dts: qcom: sc7180: Enable SoC sleep stats

[Bjorn Andersson]

On Fri 04 Jun 16:53 CDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-06-02 19:44:40)
> > On Wed 02 Jun 19:26 CDT 2021, Stephen Boyd wrote:
> >
> > > Quoting Bjorn Andersson (2021-05-31 10:57:03)
> > > > On Wed 26 May 18:30 CDT 2021, Stephen Boyd wrote:
> > > >
> > > > > Quoting Maulik Shah (2021-05-21 04:26:09)
> > > > > > @@ -3223,6 +3223,11 @@
> > > > > >                         #power-domain-cells = <1>;
> > > > > >                 };
> > > > > >
> > > > > > +               rpmh-sleep-stats@c3f0000 {
> > > > > > +                       compatible = "qcom,rpmh-sleep-stats";
> > > > > > +                       reg = <0 0x0c3f0000 0 0x400>;
> > > > > > +               };
> > > > > > +
> > > > >
> > > > > Does this need to be in DT? Can the sc7180-aoss-qmp driver use the
> > > > > aux-bus and stick the sleep stats device on there?
> > > > >
> > > >
> > > > The AOSS memory space has N chunks of "message ram", one is used for the
> > > > QMP protocol (presumably the APSS specific one), a different one is used
> > > > for the sleep stats.
> > > >
> > > > I presume we could have come up with a binding for the entire AOSS/AOP
> > > > and then describe (either implicit or explicitly) the QMP and
> > > > debug-stats under that.
> > > >
> > > > But we'd also have to come up with the same container-device for the RPM
> > > > case.
> > >
> > > Because the rpm node doesn't include this region of memory today? I
> > > still fail to see why we're changing the existing binding and adding a
> > > DT node for this new region that is basically a debug feature.
> >
> > We're not changing the binding, the memory region for the "AOSS QMP"
> > thing was never larger than 0x400.
> >
> > 0x100000 is the size of all the AOSS "msg_ram" regions. We don't have
> > this whole thing described in a binding and we don't have an
> > implementation for the whole thing.
> >
> > If we're going for that we'd need to extend the binding to indicate
> > which of the msg_ram regions are used for APSS QMP and for debug stats
> > on particular platform (either by compatible, explicit properties or as
> > some subnodes).
> 
> Fair enough. At the least, can we change the name of the node then to
> 'sram' or 'ram'? The 'rpmh-sleep-stats' node name is nonsense.
> 

Yes, "ram" sounds like a better node name for both the qmp and
sleep-stats region - in the RPMH case.

> >
> >
> > That said, as I looked into my other objection, for the RPM
> > (non-hardened) case it seems that we're actually describing the RPM
> > region. So there it would make sense to describe it as such in DT - but
> > we don't have any other code (that I'm aware of) that would implement
> > the "qcom,<platform>-rpm".
> >
> 
> I only half parsed this part. Are you saying that because we don't have
> a driver for qcom,<platform>-rpm we shouldn't keep it all within the rpm
> node?

What I was trying to say is that in the RPM (non-H) case the described
memory region is not a chunk of "ram" (or "sram"), but seems to rather
be the RPM region. So there it seems more reasonable to have a non-debug
compatible, but I don't think we have any other use for it than the
debug-stats...

Regards,
Bjorn

Re: [PATCH v8 3/5] arm64: dts: qcom: sc7180: Enable SoC sleep stats

[Maulik Shah]

Hi,

On 6/6/2021 9:12 AM, Bjorn Andersson wrote:
> Yes, "ram" sounds like a better node name for both the qmp and
> sleep-stats region - in the RPMH case.

Updated to use "aop_msgram" in v9.  I think saying only "ram" may 
further confuse with DDR.

Thanks,
Maulik

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

[PATCH v8 4/5] arm64: defconfig: Enable SoC sleep stats driver

[Maulik Shah]

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 <mkshah@codeaurora.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 45db77d..0c1cad7 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1029,6 +1029,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

Re: [PATCH v8 4/5] arm64: defconfig: Enable SoC sleep stats driver

[Stephen Boyd]

Quoting Maulik Shah (2021-05-21 04:26:10)
> 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 <mkshah@codeaurora.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

[PATCH v8 5/5] arm64: dts: qcom: sc7280: Enable SoC sleep stats

[Maulik Shah]

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: devicetree@vger.kernel.org
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 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 2cc4785..47afca8 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -827,7 +827,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>;
@@ -838,6 +838,11 @@
 			#power-domain-cells = <1>;
 		};
 
+		rpmh-sleep-stats@c3f0000 {
+			compatible = "qcom,rpmh-sleep-stats";
+			reg = <0 0x0c3f0000 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

Re: [PATCH v8 5/5] arm64: dts: qcom: sc7280: Enable SoC sleep stats

[Stephen Boyd]

Quoting Maulik Shah (2021-05-21 04:26:11)
> 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: devicetree@vger.kernel.org
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Same question as in sc7180.

>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 2cc4785..47afca8 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -827,7 +827,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>;
> @@ -838,6 +838,11 @@
>                         #power-domain-cells = <1>;
>                 };
>
> +               rpmh-sleep-stats@c3f0000 {
> +                       compatible = "qcom,rpmh-sleep-stats";
> +                       reg = <0 0x0c3f0000 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
>