[PATCH v4 0/4] Introduce SoC sleep stats driver

[PATCH v4 0/4] Introduce SoC sleep stats driver

[Maulik Shah]

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 for Qualcomm SoCs
  soc: qcom: Add SoC sleep stats driver

Maulik Shah (2):
  arm64: dts: qcom: sc7180: Enable soc sleep stats
  arm64: defconfig: Enable SoC sleep stats driver for Qualcomm

 .../bindings/soc/qcom/soc-sleep-stats.yaml         |  47 ++++
 arch/arm64/boot/dts/qcom/sc7180.dtsi               |   5 +
 arch/arm64/configs/defconfig                       |   1 +
 drivers/soc/qcom/Kconfig                           |  10 +
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/soc_sleep_stats.c                 | 279 +++++++++++++++++++++
 6 files changed, 343 insertions(+)
 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 v4 1/4] 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>
---
 .../bindings/soc/qcom/soc-sleep-stats.yaml         | 46 ++++++++++++++++++++++
 1 file changed, 46 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 00000000..7c29c61
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml
@@ -0,0 +1,46 @@
+# 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
+
+examples:
+  # Example of rpmh sleep stats
+  - |
+    rpmh_sleep_stats@c3f0000 {
+      compatible = "qcom,rpmh-sleep-stats";
+      reg = <0 0xc3f0000 0 0x400>;
+    };
+  # Example of rpm sleep stats
+  - |
+    rpm_sleep_stats@4690000 {
+      compatible = "qcom,rpm-sleep-stats";
+      reg = <0 0x04690000 0 0x400>;
+    };
+...
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v4 2/4] soc: qcom: Add SoC sleep stats driver

[Maulik Shah]

From: Mahesh Sivasubramanian <msivasub@codeaurora.org>

Let's add a driver to read the 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           |   9 ++
 drivers/soc/qcom/Makefile          |   1 +
 drivers/soc/qcom/soc_sleep_stats.c | 226 +++++++++++++++++++++++++++++++++++++
 3 files changed, 236 insertions(+)
 create mode 100644 drivers/soc/qcom/soc_sleep_stats.c

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index d0a73e7..08bc0df3 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -185,6 +185,15 @@ config QCOM_SOCINFO
 	 Say yes here to support the Qualcomm socinfo driver, providing
 	 information about the SoC to user space.
 
+config QCOM_SOC_SLEEP_STATS
+	tristate "Qualcomm Technologies, Inc. (QTI) SoC sleep stats driver"
+	depends on ARCH_QCOM && DEBUG_FS || COMPILE_TEST
+	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 9fb35c8..e6bd052 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 00000000..323593d
--- /dev/null
+++ b/drivers/soc/qcom/soc_sleep_stats.c
@@ -0,0 +1,226 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2011-2020, 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_ADDR		0x0
+#define COUNT_ADDR		0x4
+#define LAST_ENTERED_AT_ADDR	0x8
+#define LAST_EXITED_AT_ADDR	0x10
+#define ACCUMULATED_ADDR	0x18
+#define CLIENT_VOTES_ADDR	0x1c
+
+struct subsystem_data {
+	const char *name;
+	u32 smem_item;
+	u32 pid;
+};
+
+static struct subsystem_data subsystems[] = {
+	{ "modem", 605, 1 },
+	{ "adsp", 606, 2 },
+	{ "cdsp", 607, 5 },
+	{ "slpi", 608, 3 },
+	{ "gpu", 609, 0 },
+	{ "display", 610, 0 },
+};
+
+struct stats_config {
+	unsigned int offset_addr;
+	unsigned int num_records;
+	bool appended_stats_avail;
+};
+
+static const struct stats_config *config;
+
+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 print_sleep_stats(struct seq_file *s, 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 subsystem_sleep_stats_show(struct seq_file *s, void *d)
+{
+	struct subsystem_data *subsystem = s->private;
+	struct sleep_stats *stat;
+
+	stat = qcom_smem_get(subsystem->pid, subsystem->smem_item, NULL);
+	if (IS_ERR(stat))
+		return PTR_ERR(stat);
+
+	print_sleep_stats(s, stat);
+
+	return 0;
+}
+
+static int soc_sleep_stats_show(struct seq_file *s, void *d)
+{
+	void __iomem *reg = s->private;
+	struct sleep_stats stat;
+
+	stat.count = readl(reg + COUNT_ADDR);
+	stat.last_entered_at = readq(reg + LAST_ENTERED_AT_ADDR);
+	stat.last_exited_at = readq(reg + LAST_EXITED_AT_ADDR);
+	stat.accumulated = readq(reg + ACCUMULATED_ADDR);
+
+	print_sleep_stats(s, &stat);
+
+	if (config->appended_stats_avail) {
+		struct appended_stats app_stat;
+
+		app_stat.client_votes = readl(reg + CLIENT_VOTES_ADDR);
+		seq_printf(s, "Client_votes = %#x\n", app_stat.client_votes);
+	}
+
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(soc_sleep_stats);
+DEFINE_SHOW_ATTRIBUTE(subsystem_sleep_stats);
+
+static struct dentry *create_debugfs_entries(void __iomem *reg)
+{
+	struct dentry *root;
+	struct sleep_stats *stat;
+	char stat_type[sizeof(u32) + 1] = {0};
+	u32 offset, type;
+	int i;
+
+	root = debugfs_create_dir("qcom_sleep_stats", NULL);
+
+	for (i = 0; i < config->num_records; i++) {
+		offset = STAT_TYPE_ADDR + (i * sizeof(struct sleep_stats));
+
+		if (config->appended_stats_avail)
+			offset += i * sizeof(struct appended_stats);
+
+		type = readl(reg + offset);
+		memcpy(stat_type, &type, sizeof(u32));
+		debugfs_create_file(stat_type, 0444, root,
+				    reg + offset,
+				    &soc_sleep_stats_fops);
+	}
+
+	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, 0444, root,
+				    &subsystems[i],
+				    &subsystem_sleep_stats_fops);
+	}
+
+	return root;
+}
+
+static int 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;
+
+	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;
+
+	root = create_debugfs_entries(reg);
+	platform_set_drvdata(pdev, root);
+
+	return 0;
+}
+
+static int 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 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 = soc_sleep_stats_probe,
+	.remove = soc_sleep_stats_remove,
+	.driver = {
+		.name = "soc_sleep_stats",
+		.of_match_table = 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");
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v4 3/4] 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 253274d..b5b0f3f 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -1610,7 +1610,7 @@
 
 		aoss_qmp: qmp@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>;
 
@@ -1618,6 +1618,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

[PATCH v4 4/4] 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 a8de3d3..2dd543b 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -793,6 +793,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_ARCH_R8A774A1=y
 CONFIG_ARCH_R8A774B1=y
 CONFIG_ARCH_R8A774C0=y
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v4 1/4] dt-bindings: Introduce SoC sleep stats bindings

[Stephen Boyd]

Quoting Maulik Shah (2020-03-09 04:14:14)
> 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>
> ---
>  .../bindings/soc/qcom/soc-sleep-stats.yaml         | 46 ++++++++++++++++++++++
>  1 file changed, 46 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 00000000..7c29c61
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml
> @@ -0,0 +1,46 @@
> +# 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
> +
> +examples:
> +  # Example of rpmh sleep stats
> +  - |
> +    rpmh_sleep_stats@c3f0000 {
> +      compatible = "qcom,rpmh-sleep-stats";
> +      reg = <0 0xc3f0000 0 0x400>;
> +    };
> +  # Example of rpm sleep stats
> +  - |
> +    rpm_sleep_stats@4690000 {

Node names don't have underscores. It really feels like we should be able
to get away with not having this device node at all. Why can't we have
the rpm message ram be a node that covers the entire range and then have
that either create a platform device for debugfs stats or just have it
register the stat information from whatever driver attaches to that
node?

Carving this up into multiple nodes and making compatible strings
doesn't seem very useful here because we're essentially making device
nodes in DT for logical software components that exist in the rpm
message ram.

Re: [PATCH v4 1/4] dt-bindings: Introduce SoC sleep stats bindings

[Bjorn Andersson]

On Mon 09 Mar 11:23 PDT 2020, Stephen Boyd wrote:

> Quoting Maulik Shah (2020-03-09 04:14:14)
> > 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>
> > ---
> >  .../bindings/soc/qcom/soc-sleep-stats.yaml         | 46 ++++++++++++++++++++++
> >  1 file changed, 46 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 00000000..7c29c61
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml
> > @@ -0,0 +1,46 @@
> > +# 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
> > +
> > +examples:
> > +  # Example of rpmh sleep stats
> > +  - |
> > +    rpmh_sleep_stats@c3f0000 {
> > +      compatible = "qcom,rpmh-sleep-stats";
> > +      reg = <0 0xc3f0000 0 0x400>;
> > +    };
> > +  # Example of rpm sleep stats
> > +  - |
> > +    rpm_sleep_stats@4690000 {
> 
> Node names don't have underscores. It really feels like we should be able
> to get away with not having this device node at all. Why can't we have
> the rpm message ram be a node that covers the entire range and then have
> that either create a platform device for debugfs stats or just have it
> register the stat information from whatever driver attaches to that
> node?
> 
> Carving this up into multiple nodes and making compatible strings
> doesn't seem very useful here because we're essentially making device
> nodes in DT for logical software components that exist in the rpm
> message ram.

It's been a while since I discussed this with Lina, but iirc I opted for
the model you suggest and we concluded that it wouldn't fit with the RPM
case.

And given that, for reasons unknown to me, msgram isn't a single region,
but a set of adjacent memory regions, this does seem to represent
hardware better.

Regards,
Bjorn

Re: [PATCH v4 1/4] dt-bindings: Introduce SoC sleep stats bindings

[Stephen Boyd]

Quoting Bjorn Andersson (2020-03-09 11:51:20)
> On Mon 09 Mar 11:23 PDT 2020, Stephen Boyd wrote:
> 
> > Quoting Maulik Shah (2020-03-09 04:14:14)
> > > 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>
> > > ---
> > >  .../bindings/soc/qcom/soc-sleep-stats.yaml         | 46 ++++++++++++++++++++++
> > >  1 file changed, 46 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 00000000..7c29c61
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml
> > > @@ -0,0 +1,46 @@
> > > +# 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
> > > +
> > > +examples:
> > > +  # Example of rpmh sleep stats
> > > +  - |
> > > +    rpmh_sleep_stats@c3f0000 {
> > > +      compatible = "qcom,rpmh-sleep-stats";
> > > +      reg = <0 0xc3f0000 0 0x400>;
> > > +    };
> > > +  # Example of rpm sleep stats
> > > +  - |
> > > +    rpm_sleep_stats@4690000 {
> > 
> > Node names don't have underscores. It really feels like we should be able
> > to get away with not having this device node at all. Why can't we have
> > the rpm message ram be a node that covers the entire range and then have
> > that either create a platform device for debugfs stats or just have it
> > register the stat information from whatever driver attaches to that
> > node?
> > 
> > Carving this up into multiple nodes and making compatible strings
> > doesn't seem very useful here because we're essentially making device
> > nodes in DT for logical software components that exist in the rpm
> > message ram.
> 
> It's been a while since I discussed this with Lina, but iirc I opted for
> the model you suggest and we concluded that it wouldn't fit with the RPM
> case.
> 
> And given that, for reasons unknown to me, msgram isn't a single region,
> but a set of adjacent memory regions, this does seem to represent
> hardware better.
> 

I guess there's message ram and code ram or something like that? Maybe
that's the problem? Either way it sounds like the node name needs to be
fixed to have dashes and then this is fine to keep. Describing memory
like this in DT just makes me sad.

Re: [PATCH v4 1/4] dt-bindings: Introduce SoC sleep stats bindings

[Maulik Shah]

On 3/10/2020 12:31 AM, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2020-03-09 11:51:20)
>> On Mon 09 Mar 11:23 PDT 2020, Stephen Boyd wrote:
>>
>>> Quoting Maulik Shah (2020-03-09 04:14:14)
>>>> 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>
>>>> ---
>>>>  .../bindings/soc/qcom/soc-sleep-stats.yaml         | 46 ++++++++++++++++++++++
>>>>  1 file changed, 46 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 00000000..7c29c61
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml
>>>> @@ -0,0 +1,46 @@
>>>> +# 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
>>>> +
>>>> +examples:
>>>> +  # Example of rpmh sleep stats
>>>> +  - |
>>>> +    rpmh_sleep_stats@c3f0000 {
>>>> +      compatible = "qcom,rpmh-sleep-stats";
>>>> +      reg = <0 0xc3f0000 0 0x400>;
>>>> +    };
>>>> +  # Example of rpm sleep stats
>>>> +  - |
>>>> +    rpm_sleep_stats@4690000 {
>>> Node names don't have underscores. It really feels like we should be able
>>> to get away with not having this device node at all. Why can't we have
>>> the rpm message ram be a node that covers the entire range and then have
>>> that either create a platform device for debugfs stats or just have it
>>> register the stat information from whatever driver attaches to that
>>> node?
>>>
>>> Carving this up into multiple nodes and making compatible strings
>>> doesn't seem very useful here because we're essentially making device
>>> nodes in DT for logical software components that exist in the rpm
>>> message ram.
>> It's been a while since I discussed this with Lina, but iirc I opted for
>> the model you suggest and we concluded that it wouldn't fit with the RPM
>> case.
>>
>> And given that, for reasons unknown to me, msgram isn't a single region,
>> but a set of adjacent memory regions, this does seem to represent
>> hardware better.
>>
> I guess there's message ram and code ram or something like that? Maybe
> that's the problem? Either way it sounds like the node name needs to be
> fixed to have dashes and then this is fine to keep. Describing memory
> like this in DT just makes me sad.
Hi,

I will spin v5 with fixing node name.

Thanks,
Maulik

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