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

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

[Maulik Shah]

Changes in v10:
- Address Bjorn's comments from v10
- Add a case for RPM based targets dynamic offset in driver
- Update commit messages to use qcom sleep stats instead of soc sleep stats
- Drop individual target dtsi changes for sc7180 and sc7280
- Add single change to enable sleep stats for RPMh based targets
- Add single change to enable sleep stats for RPM based targets

Changes in v10:
- Updated device node name to use memory instead of aop_msgram
- Remove Lina's email from maintainers 
- Rename driver to qcom_sleep_stats. Update makefile, Kconfig accordingly
- Address Bjorn's comments from v9

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

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

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

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

- Address guenter's comments to add depends on QCOM_SMEM

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

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

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

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

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

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

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

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

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

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

Mahesh Sivasubramanian (2):
  dt-bindings: Introduce QCOM Sleep stats bindings
  soc: qcom: Add Sleep stats driver

Maulik Shah (3):
  arm64: defconfig: Enable Sleep stats driver
  arm64: dts: qcom: Enable RPMh Sleep stats
  arm64: dts: qcom: Enable RPM Sleep stats

 .../bindings/soc/qcom/qcom-sleep-stats.yaml        |  47 ++++
 arch/arm64/boot/dts/qcom/msm8996.dtsi              |   5 +
 arch/arm64/boot/dts/qcom/msm8998.dtsi              |   5 +
 arch/arm64/boot/dts/qcom/qcs404.dtsi               |   5 +
 arch/arm64/boot/dts/qcom/sc7180.dtsi               |   7 +-
 arch/arm64/boot/dts/qcom/sc7280.dtsi               |   7 +-
 arch/arm64/boot/dts/qcom/sdm630.dtsi               |   5 +
 arch/arm64/boot/dts/qcom/sm6125.dtsi               |   5 +
 arch/arm64/boot/dts/qcom/sm8150.dtsi               |   7 +-
 arch/arm64/boot/dts/qcom/sm8250.dtsi               |   7 +-
 arch/arm64/boot/dts/qcom/sm8350.dtsi               |   7 +-
 arch/arm64/configs/defconfig                       |   1 +
 drivers/soc/qcom/Kconfig                           |  10 +
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/qcom_sleep_stats.c                | 259 +++++++++++++++++++++
 15 files changed, 373 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
 create mode 100644 drivers/soc/qcom/qcom_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 v11 1/5] dt-bindings: Introduce QCOM Sleep stats bindings

[Maulik Shah]

From: Mahesh Sivasubramanian <msivasub@codeaurora.org>

Add device binding documentation for Qualcomm Technologies, Inc. (QTI)
Sleep stats driver. The driver is used for displaying 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/qcom-sleep-stats.yaml        | 47 ++++++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
new file mode 100644
index 0000000..5213daf
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
@@ -0,0 +1,47 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/qcom/qcom-sleep-stats.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. (QTI) Sleep stats bindings
+
+maintainers:
+  - Maulik Shah <mkshah@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
+  - |
+    sram@c3f0000 {
+      compatible = "qcom,rpmh-sleep-stats";
+      reg = <0x0c3f0000 0x400>;
+    };
+  # Example of rpm sleep stats
+  - |
+    sram@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 v11 2/5] soc: qcom: Add 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/qcom_sleep_stats.c | 259 ++++++++++++++++++++++++++++++++++++
 3 files changed, 270 insertions(+)
 create mode 100644 drivers/soc/qcom/qcom_sleep_stats.c

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index abfef20..c67fc00 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -182,6 +182,16 @@ config QCOM_SMSM
 	  Say yes here to support the Qualcomm Shared Memory State Machine.
 	  The state machine is represented by bits in shared memory.
 
+config QCOM_SLEEP_STATS
+	tristate "Qualcomm Technologies, Inc. (QTI) Sleep stats driver"
+	depends on (ARCH_QCOM && DEBUG_FS) || COMPILE_TEST
+	depends on QCOM_SMEM
+	help
+	  Qualcomm Technologies, Inc. (QTI) Sleep stats driver to read
+	  the shared memory exported by the remote processor related to
+	  various SoC level low power modes statistics and export to debugfs
+	  interface.
+
 config QCOM_SOCINFO
 	tristate "Qualcomm socinfo driver"
 	depends on QCOM_SMEM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 24514c7..ad84f7b 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_QCOM_SMEM) +=	smem.o
 obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
 obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
+obj-$(CONFIG_QCOM_SLEEP_STATS)	+= qcom_sleep_stats.o
 obj-$(CONFIG_QCOM_SOCINFO)	+= socinfo.o
 obj-$(CONFIG_QCOM_SPM)		+= spm.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
diff --git a/drivers/soc/qcom/qcom_sleep_stats.c b/drivers/soc/qcom/qcom_sleep_stats.c
new file mode 100644
index 0000000..2368279
--- /dev/null
+++ b/drivers/soc/qcom/qcom_sleep_stats.c
@@ -0,0 +1,259 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2011-2021, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/seq_file.h>
+
+#include <linux/soc/qcom/smem.h>
+#include <clocksource/arm_arch_timer.h>
+
+#define RPM_DYNAMIC_ADDR	0x14
+#define RPM_DYNAMIC_ADDR_MASK	0xFFFF
+
+#define STAT_TYPE_OFFSET	0x0
+#define COUNT_OFFSET		0x4
+#define LAST_ENTERED_AT_OFFSET	0x8
+#define LAST_EXITED_AT_OFFSET	0x10
+#define ACCUMULATED_OFFSET	0x18
+#define CLIENT_VOTES_OFFSET	0x1c
+
+struct subsystem_data {
+	const char *name;
+	u32 smem_item;
+	u32 pid;
+};
+
+static const struct subsystem_data subsystems[] = {
+	{ "modem", 605, 1 },
+	{ "wpss", 605, 13 },
+	{ "adsp", 606, 2 },
+	{ "cdsp", 607, 5 },
+	{ "slpi", 608, 3 },
+	{ "gpu", 609, 0 },
+	{ "display", 610, 0 },
+	{ "adsp_island", 613, 2 },
+	{ "slpi_island", 613, 3 },
+};
+
+struct stats_config {
+	size_t stats_offset;
+	size_t num_records;
+	bool appended_stats_avail;
+	bool dynamic_offset;
+};
+
+struct stats_data {
+	bool appended_stats_avail;
+	void __iomem *base;
+};
+
+struct sleep_stats {
+	u32 stat_type;
+	u32 count;
+	u64 last_entered_at;
+	u64 last_exited_at;
+	u64 accumulated;
+};
+
+struct appended_stats {
+	u32 client_votes;
+	u32 reserved[3];
+};
+
+static void qcom_print_stats(struct seq_file *s, const struct sleep_stats *stat)
+{
+	u64 accumulated = stat->accumulated;
+	/*
+	 * If a subsystem is in sleep when reading the sleep stats adjust
+	 * the accumulated sleep duration to show actual sleep time.
+	 */
+	if (stat->last_entered_at > stat->last_exited_at)
+		accumulated += arch_timer_read_counter() - stat->last_entered_at;
+
+	seq_printf(s, "Count: %u\n", stat->count);
+	seq_printf(s, "Last Entered At: %llu\n", stat->last_entered_at);
+	seq_printf(s, "Last Exited At: %llu\n", stat->last_exited_at);
+	seq_printf(s, "Accumulated Duration: %llu\n", accumulated);
+}
+
+static int qcom_subsystem_sleep_stats_show(struct seq_file *s, void *unused)
+{
+	struct subsystem_data *subsystem = s->private;
+	struct sleep_stats *stat;
+
+	/* Items are allocated lazily, so lookup pointer each time */
+	stat = qcom_smem_get(subsystem->pid, subsystem->smem_item, NULL);
+	if (IS_ERR(stat))
+		return -EIO;
+
+	qcom_print_stats(s, stat);
+
+	return 0;
+}
+
+static int qcom_soc_sleep_stats_show(struct seq_file *s, void *unused)
+{
+	struct stats_data *d = s->private;
+	void __iomem *reg = d->base;
+	struct sleep_stats stat;
+
+	memcpy_fromio(&stat, reg, sizeof(stat));
+	qcom_print_stats(s, &stat);
+
+	if (d->appended_stats_avail) {
+		struct appended_stats votes;
+
+		memcpy_fromio(&votes, reg + CLIENT_VOTES_OFFSET, sizeof(votes));
+		seq_printf(s, "Client Votes: %#x\n", votes.client_votes);
+	}
+
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(qcom_soc_sleep_stats);
+DEFINE_SHOW_ATTRIBUTE(qcom_subsystem_sleep_stats);
+
+static void qcom_create_soc_sleep_stat_files(struct dentry *root, void __iomem *reg,
+					     struct stats_data *d,
+					     const struct stats_config *config)
+{
+	char stat_type[sizeof(u32) + 1] = {0};
+	size_t stats_offset = config->stats_offset;
+	u32 offset = 0, type;
+	int i, j;
+
+	/*
+	 * On RPM targets, stats offset location is dynamic and changes from target
+	 * to target and sometimes from build to build for same target.
+	 *
+	 * In such cases the dynamic address is present at 0x14 offset from base
+	 * address in devicetree. The last 16bits indicates the stats_offset.
+	 */
+	if (config->dynamic_offset) {
+		stats_offset = readl(reg + RPM_DYNAMIC_ADDR);
+		stats_offset &= RPM_DYNAMIC_ADDR_MASK;
+	}
+
+	for (i = 0; i < config->num_records; i++) {
+		d[i].base = reg + offset + stats_offset;
+
+		/*
+		 * Read the low power mode name and create debugfs file for it.
+		 * The names read could be of below,
+		 * (may change depending on low power mode supported).
+		 * For rpmh-sleep-stats: "aosd", "cxsd" and "ddr".
+		 * For rpm-sleep-stats: "vmin" and "vlow".
+		 */
+		type = readl(d[i].base);
+		for (j = 0; j < sizeof(u32); j++) {
+			stat_type[j] = type & 0xff;
+			type = type >> 8;
+		}
+		strim(stat_type);
+		debugfs_create_file(stat_type, 0400, root, &d[i],
+				    &qcom_soc_sleep_stats_fops);
+
+		offset += sizeof(struct sleep_stats);
+		if (d[i].appended_stats_avail)
+			offset += sizeof(struct appended_stats);
+	}
+}
+
+static void qcom_create_subsystem_stat_files(struct dentry *root)
+{
+	const struct sleep_stats *stat;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
+		stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item, NULL);
+		if (IS_ERR(stat))
+			continue;
+
+		debugfs_create_file(subsystems[i].name, 0400, root, (void *)&subsystems[i],
+				    &qcom_subsystem_sleep_stats_fops);
+	}
+}
+
+static int qcom_sleep_stats_probe(struct platform_device *pdev)
+{
+	void __iomem *reg;
+	struct dentry *root;
+	const struct stats_config *config;
+	struct stats_data *d;
+	int i;
+
+	config = device_get_match_data(&pdev->dev);
+	if (!config)
+		return -ENODEV;
+
+	reg = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
+	if (IS_ERR(reg))
+		return -ENOMEM;
+
+	d = devm_kcalloc(&pdev->dev, config->num_records,
+			 sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return -ENOMEM;
+
+	for (i = 0; i < config->num_records; i++)
+		d[i].appended_stats_avail = config->appended_stats_avail;
+
+	root = debugfs_create_dir("qcom_sleep_stats", NULL);
+
+	qcom_create_subsystem_stat_files(root);
+	qcom_create_soc_sleep_stat_files(root, reg, d, config);
+
+	platform_set_drvdata(pdev, root);
+
+	return 0;
+}
+
+static int qcom_sleep_stats_remove(struct platform_device *pdev)
+{
+	struct dentry *root = platform_get_drvdata(pdev);
+
+	debugfs_remove_recursive(root);
+
+	return 0;
+}
+
+static const struct stats_config rpm_data = {
+	.stats_offset = 0,
+	.num_records = 2,
+	.appended_stats_avail = true,
+	.dynamic_offset = true,
+};
+
+static const struct stats_config rpmh_data = {
+	.stats_offset = 0x48,
+	.num_records = 3,
+	.appended_stats_avail = false,
+	.dynamic_offset = false,
+};
+
+static const struct of_device_id qcom_sleep_stats_table[] = {
+	{ .compatible = "qcom,rpm-sleep-stats", .data = &rpm_data },
+	{ .compatible = "qcom,rpmh-sleep-stats", .data = &rpmh_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, qcom_sleep_stats_table);
+
+static struct platform_driver qcom_sleep_stats = {
+	.probe = qcom_sleep_stats_probe,
+	.remove = qcom_sleep_stats_remove,
+	.driver = {
+		.name = "qcom_sleep_stats",
+		.of_match_table = qcom_sleep_stats_table,
+	},
+};
+module_platform_driver(qcom_sleep_stats);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. (QTI) Sleep Stats driver");
+MODULE_LICENSE("GPL v2");
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v11 3/5] arm64: defconfig: Enable Sleep stats driver

[Maulik Shah]

Enable 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>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index da988a5..1d151a9 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1043,6 +1043,7 @@ CONFIG_QCOM_SMEM=y
 CONFIG_QCOM_SMD_RPM=y
 CONFIG_QCOM_SMP2P=y
 CONFIG_QCOM_SMSM=y
+CONFIG_QCOM_SLEEP_STATS=m
 CONFIG_QCOM_SOCINFO=m
 CONFIG_QCOM_APR=m
 CONFIG_ARCH_R8A774A1=y
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v11 4/5] arm64: dts: qcom: Enable RPMh Sleep stats

[Maulik Shah]

Add device node for Sleep stats driver which provides various
low power mode stats on sc7180, sc7280, sm8150, sm8250 and sm8350.

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/sc7180.dtsi | 7 ++++++-
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 7 ++++++-
 arch/arm64/boot/dts/qcom/sm8150.dtsi | 7 ++++++-
 arch/arm64/boot/dts/qcom/sm8250.dtsi | 7 ++++++-
 arch/arm64/boot/dts/qcom/sm8350.dtsi | 7 ++++++-
 5 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index f10217c..e849810 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -3219,13 +3219,18 @@
 
 		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>;
 
 			#clock-cells = <0>;
 		};
 
+		sram@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>,
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 39635da..b21c55a 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -2637,7 +2637,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>;
@@ -2647,6 +2647,11 @@
 			#clock-cells = <0>;
 		};
 
+		sram@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>,
diff --git a/arch/arm64/boot/dts/qcom/sm8150.dtsi b/arch/arm64/boot/dts/qcom/sm8150.dtsi
index 8a03569..bde3776 100644
--- a/arch/arm64/boot/dts/qcom/sm8150.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8150.dtsi
@@ -3170,13 +3170,18 @@
 
 		aoss_qmp: power-controller@c300000 {
 			compatible = "qcom,sm8150-aoss-qmp";
-			reg = <0x0 0x0c300000 0x0 0x100000>;
+			reg = <0x0 0x0c300000 0x0 0x400>;
 			interrupts = <GIC_SPI 389 IRQ_TYPE_EDGE_RISING>;
 			mboxes = <&apss_shared 0>;
 
 			#clock-cells = <0>;
 		};
 
+		sram@c3f0000 {
+			compatible = "qcom,rpmh-sleep-stats";
+			reg = <0 0x0c3f0000 0 0x400>;
+		};
+
 		tsens0: thermal-sensor@c263000 {
 			compatible = "qcom,sm8150-tsens", "qcom,tsens-v2";
 			reg = <0 0x0c263000 0 0x1ff>, /* TM */
diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 2796b27..a815101 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -2899,7 +2899,7 @@
 
 		aoss_qmp: power-controller@c300000 {
 			compatible = "qcom,sm8250-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>;
@@ -2909,6 +2909,11 @@
 			#clock-cells = <0>;
 		};
 
+		sram@c3f0000 {
+			compatible = "qcom,rpmh-sleep-stats";
+			reg = <0 0x0c3f0000 0 0x400>;
+		};
+
 		spmi_bus: spmi@c440000 {
 			compatible = "qcom,spmi-pmic-arb";
 			reg = <0x0 0x0c440000 0x0 0x0001100>,
diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi b/arch/arm64/boot/dts/qcom/sm8350.dtsi
index 6c83cd5..778a4b6 100644
--- a/arch/arm64/boot/dts/qcom/sm8350.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi
@@ -788,7 +788,7 @@
 
 		aoss_qmp: power-controller@c300000 {
 			compatible = "qcom,sm8350-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>;
 			mboxes = <&ipcc IPCC_CLIENT_AOP IPCC_MPROC_SIGNAL_GLINK_QMP>;
@@ -796,6 +796,11 @@
 			#clock-cells = <0>;
 		};
 
+		sram@c3f0000 {
+			compatible = "qcom,rpmh-sleep-stats";
+			reg = <0 0x0c3f0000 0 0x400>;
+		};
+
 		spmi_bus: spmi@c440000 {
 			compatible = "qcom,spmi-pmic-arb";
 			reg = <0x0 0xc440000 0x0 0x1100>,
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH v11 5/5] arm64: dts: qcom: Enable RPM Sleep stats

[Maulik Shah]

Add device node for Sleep stats driver which provides various
low power mode stats on msm8996, msm8998, qcs404, sdm630 and
sm6125.

Cc: devicetree@vger.kernel.org
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/msm8996.dtsi | 5 +++++
 arch/arm64/boot/dts/qcom/msm8998.dtsi | 5 +++++
 arch/arm64/boot/dts/qcom/qcs404.dtsi  | 5 +++++
 arch/arm64/boot/dts/qcom/sdm630.dtsi  | 5 +++++
 arch/arm64/boot/dts/qcom/sm6125.dtsi  | 5 +++++
 5 files changed, 25 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index eb3ec5f..d647382 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -1523,6 +1523,11 @@
 			};
 		};
 
+		sram@290000 {
+			compatible = "qcom,rpm-sleep-stats";
+			reg = <0x00290000 0x400>;
+		};
+
 		spmi_bus: qcom,spmi@400f000 {
 			compatible = "qcom,spmi-pmic-arb";
 			reg = <0x0400f000 0x1000>,
diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index 3c1f133..895806e 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -2022,6 +2022,11 @@
 			};
 		};
 
+		sram@290000 {
+			compatible = "qcom,rpm-sleep-stats";
+			reg = <0x00290000 0x400>;
+		};
+
 		spmi_bus: spmi@800f000 {
 			compatible = "qcom,spmi-pmic-arb";
 			reg =	<0x0800f000 0x1000>,
diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
index 339790b..45e6eae 100644
--- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
@@ -719,6 +719,11 @@
 			reg = <0x01937000 0x25000>;
 		};
 
+		sram@290000 {
+			compatible = "qcom,rpm-sleep-stats";
+			reg = <0x00290000 0x400>;
+		};
+
 		spmi_bus: spmi@200f000 {
 			compatible = "qcom,spmi-pmic-arb";
 			reg = <0x0200f000 0x001000>,
diff --git a/arch/arm64/boot/dts/qcom/sdm630.dtsi b/arch/arm64/boot/dts/qcom/sdm630.dtsi
index 9c7f87e..4044a5b 100644
--- a/arch/arm64/boot/dts/qcom/sdm630.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm630.dtsi
@@ -1176,6 +1176,11 @@
 			status = "disabled";
 		};
 
+		sram@290000 {
+			compatible = "qcom,rpm-sleep-stats";
+			reg = <0x00290000 0x400>;
+		};
+
 		spmi_bus: spmi@800f000 {
 			compatible = "qcom,spmi-pmic-arb";
 			reg =	<0x0800f000 0x1000>,
diff --git a/arch/arm64/boot/dts/qcom/sm6125.dtsi b/arch/arm64/boot/dts/qcom/sm6125.dtsi
index c2317dd..c90491a3 100644
--- a/arch/arm64/boot/dts/qcom/sm6125.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm6125.dtsi
@@ -460,6 +460,11 @@
 			};
 		};
 
+		sram@4690000 {
+			compatible = "qcom,rpm-sleep-stats";
+			reg = <0x04690000 0x400>;
+		};
+
 		spmi_bus: spmi@1c40000 {
 			compatible = "qcom,spmi-pmic-arb";
 			reg = <0x01c40000 0x1100>,
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH v11 2/5] soc: qcom: Add Sleep stats driver

[Stephan Gerhold]

Hi Maulik,

On Thu, Oct 07, 2021 at 03:27:26PM +0530, 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/qcom_sleep_stats.c | 259 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 270 insertions(+)
>  create mode 100644 drivers/soc/qcom/qcom_sleep_stats.c
> 
> [...]
> +
> +static int qcom_subsystem_sleep_stats_show(struct seq_file *s, void *unused)
> +{
> +	struct subsystem_data *subsystem = s->private;
> +	struct sleep_stats *stat;
> +
> +	/* Items are allocated lazily, so lookup pointer each time */
> +	stat = qcom_smem_get(subsystem->pid, subsystem->smem_item, NULL);
> +	if (IS_ERR(stat))
> +		return -EIO;
> +
> [...]
> +
> +static void qcom_create_subsystem_stat_files(struct dentry *root)
> +{
> +	const struct sleep_stats *stat;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
> +		stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item, NULL);
> +		if (IS_ERR(stat))
> +			continue;
> +
> +		debugfs_create_file(subsystems[i].name, 0400, root, (void *)&subsystems[i],
> +				    &qcom_subsystem_sleep_stats_fops);

This causes WARNINGs on MSM8996 and MSM8916:

[    0.503054] ------------[ cut here ]------------
[    0.503100] WARNING: CPU: 1 PID: 1 at drivers/soc/qcom/smem.c:587 qcom_smem_get+0x184/0x1b0
[    0.503184] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.15.0-rc4+ #378
[    0.503218] Hardware name: Xiaomi Mi Note 2 (DT)
[    0.503278] pc : qcom_smem_get+0x184/0x1b0
[    0.503307] lr : qcom_sleep_stats_probe+0xfc/0x20
[    0.503875] Call trace:
[    0.503896]  qcom_smem_get+0x184/0x1b0
[    0.503925]  qcom_sleep_stats_probe+0xfc/0x270

AFAICT from downstream the smem subsystem information is only read in
the rpmh_master_stat.c driver, should this be specific to RPMh?

There is a rpm_master_stat.c too but that looks quite different,
so I guess the approach is different with RPM?

Two more (unrelated) issues here:

  1. This will silently not register anything if SMEM probes after the
     qcom-sleep-stats driver (qcom_smem_get() will return -EPROBE_DEFER)
     and you will just skip registering the debugfs files.

  2. In qcom_subsystem_sleep_stats_show() you say
     /* Items are allocated lazily, so lookup pointer each time */

     But, if the lookup fails here you don't register the debugfs file
     at all. Does this work if the subsystem is started after this driver?

Thanks,
Stephan

Re: [PATCH v11 1/5] dt-bindings: Introduce QCOM Sleep stats bindings

[Stephan Gerhold]

On Thu, Oct 07, 2021 at 03:27:25PM +0530, Maulik Shah wrote:
> From: Mahesh Sivasubramanian <msivasub@codeaurora.org>
> 
> Add device binding documentation for Qualcomm Technologies, Inc. (QTI)
> Sleep stats driver. The driver is used for displaying 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/qcom-sleep-stats.yaml        | 47 ++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
> new file mode 100644
> index 0000000..5213daf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
> @@ -0,0 +1,47 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/qcom/qcom-sleep-stats.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies, Inc. (QTI) Sleep stats bindings
> +
> +maintainers:
> +  - Maulik Shah <mkshah@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
> +  - |
> +    sram@c3f0000 {
> +      compatible = "qcom,rpmh-sleep-stats";
> +      reg = <0x0c3f0000 0x400>;
> +    };
> +  # Example of rpm sleep stats
> +  - |
> +    sram@4690000 {
> +      compatible = "qcom,rpm-sleep-stats";
> +      reg = <0x04690000 0x400>;
> +    };

Does this region really only contain "rpm-*sleep*-stats"? AFAICT this is
really a more generic memory region where various offsets are read from.

These are all the offsets in msm8998-pm.dtsi downstream [1]:
  ...9000c: rpm-rail-stats offset
  ...90014: rpm-sleep-stats offset (RPM_DYNAMIC_ADDR in your driver)
  ...90018: rpm-log offset
  ...9001c: "RPM FREE HEAP SPACE"

How would you set up any of the other drivers if the entire region
is declared as "rpm-sleep-stats"?

Perhaps this region should have a more generic name that represents what
it actually is and not only one of the information it contains, similar
to "qcom,rpm-msg-ram"?

Thanks,
Stephan

[1]: https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998-pm.dtsi?h=LA.UM.8.4.1.c25#n271

Re: [PATCH v11 1/5] dt-bindings: Introduce QCOM Sleep stats bindings

[Maulik Shah]

Hi Stephan,

On 10/8/2021 1:20 AM, Stephan Gerhold wrote:
> On Thu, Oct 07, 2021 at 03:27:25PM +0530, Maulik Shah wrote:
>> From: Mahesh Sivasubramanian <msivasub@codeaurora.org>
>>
>> Add device binding documentation for Qualcomm Technologies, Inc. (QTI)
>> Sleep stats driver. The driver is used for displaying 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/qcom-sleep-stats.yaml        | 47 ++++++++++++++++++++++
>>   1 file changed, 47 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
>> new file mode 100644
>> index 0000000..5213daf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
>> @@ -0,0 +1,47 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/soc/qcom/qcom-sleep-stats.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Technologies, Inc. (QTI) Sleep stats bindings
>> +
>> +maintainers:
>> +  - Maulik Shah <mkshah@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
>> +  - |
>> +    sram@c3f0000 {
>> +      compatible = "qcom,rpmh-sleep-stats";
>> +      reg = <0x0c3f0000 0x400>;
>> +    };
>> +  # Example of rpm sleep stats
>> +  - |
>> +    sram@4690000 {
>> +      compatible = "qcom,rpm-sleep-stats";
>> +      reg = <0x04690000 0x400>;
>> +    };
> 
> Does this region really only contain "rpm-*sleep*-stats"? AFAICT this is
> really a more generic memory region where various offsets are read from.

Right the area contains all kind of sleep stats/logs.

> 
> These are all the offsets in msm8998-pm.dtsi downstream [1]:
>    ...9000c: rpm-rail-stats offset
>    ...90014: rpm-sleep-stats offset (RPM_DYNAMIC_ADDR in your driver)
>    ...90018: rpm-log offset
>    ...9001c: "RPM FREE HEAP SPACE"
> 
> How would you set up any of the other drivers if the entire region
> is declared as "rpm-sleep-stats"?

We don't need to setup other drivers.
The idea is to have single stats driver (qcom_sleep_stats) that can be 
enahanced to to read other stats also on both RPM/ PRMH targets.

Today this driver reads only sleep stats from offset 0x90014.
if in future say, we want to read rpm-rail-stats from offset 0x9000c 
then it can be added in the existing driver.

In the rpm data, add one more entry to indicate reading rpm-rail-stats,
something like below and add new compatible flag for the target, may be
"qcom,rpm-sleep-stats-legacy" and should work like,

#define RPM_RAIL_STATS_ADDR 0xc

static const struct stats_config rpm_data = {
         .stats_offset = 0,
         .num_records = 2,
         .appended_stats_avail = true,
         .dynamic_offset = true,
         .rpm_rail_stats = true,
};

and check in driver probe like

         if (config->rpm_rail_stats) {
	//create rpm rail stats related files.
         }

Similarly for other rpm-logs / any other stats can be added in same driver.

Hope this clarifies.

> 
> Perhaps this region should have a more generic name that represents what
> it actually is and not only one of the information it contains, similar
> to "qcom,rpm-msg-ram"?

sram looks good to me. Actually on RPM targets its RPM data ram and RPMH 
targets its msgram. To keep the name common sram is used here.

Thanks,
Maulik

> 
> Thanks,
> Stephan
> 
> [1]: https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998-pm.dtsi?h=LA.UM.8.4.1.c25#n271
> 

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

Re: [PATCH v11 2/5] soc: qcom: Add Sleep stats driver

[Maulik Shah]

Hi,

On 10/7/2021 11:59 PM, Stephan Gerhold wrote:
> Hi Maulik,
> 
> On Thu, Oct 07, 2021 at 03:27:26PM +0530, Maulik Shah wrote:
>> From: Mahesh Sivasubramanian <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/qcom_sleep_stats.c | 259 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 270 insertions(+)
>>   create mode 100644 drivers/soc/qcom/qcom_sleep_stats.c
>>
>> [...]
>> +
>> +static int qcom_subsystem_sleep_stats_show(struct seq_file *s, void *unused)
>> +{
>> +	struct subsystem_data *subsystem = s->private;
>> +	struct sleep_stats *stat;
>> +
>> +	/* Items are allocated lazily, so lookup pointer each time */
>> +	stat = qcom_smem_get(subsystem->pid, subsystem->smem_item, NULL);
>> +	if (IS_ERR(stat))
>> +		return -EIO;
>> +
>> [...]
>> +
>> +static void qcom_create_subsystem_stat_files(struct dentry *root)
>> +{
>> +	const struct sleep_stats *stat;
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
>> +		stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item, NULL);
>> +		if (IS_ERR(stat))
>> +			continue;
>> +
>> +		debugfs_create_file(subsystems[i].name, 0400, root, (void *)&subsystems[i],
>> +				    &qcom_subsystem_sleep_stats_fops);
> 
> This causes WARNINGs on MSM8996 and MSM8916:
> 
> [    0.503054] ------------[ cut here ]------------
> [    0.503100] WARNING: CPU: 1 PID: 1 at drivers/soc/qcom/smem.c:587 qcom_smem_get+0x184/0x1b0
> [    0.503184] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.15.0-rc4+ #378
> [    0.503218] Hardware name: Xiaomi Mi Note 2 (DT)
> [    0.503278] pc : qcom_smem_get+0x184/0x1b0
> [    0.503307] lr : qcom_sleep_stats_probe+0xfc/0x20
> [    0.503875] Call trace:
> [    0.503896]  qcom_smem_get+0x184/0x1b0
> [    0.503925]  qcom_sleep_stats_probe+0xfc/0x270
> 
> AFAICT from downstream the smem subsystem information is only read in
> the rpmh_master_stat.c driver, should this be specific to RPMh?

Thanks for checking this on MSM8996. Probably it doesnot have SMEM items 
allocated so causes WARNINGs.

Keeping subsystem stats info in SMEM is not limited to only RPMH 
targets. Downstream has some RPM targets which also uses SMEM to store
subsystem stats so the driver is kept generic.

> 
> There is a rpm_master_stat.c too but that looks quite different,
> so I guess the approach is different with RPM?

Right. on existing upstream RPM targets i can skip to create/get SMEM 
items since
they are not guranteed to be present and one should continue to use 
rpm_master_stats.c to get subsystem stats. (this uses entirely different 
data structure for sleep stats and are not part of RPM data ram/SMEM and 
are deprecated in downstream).

> 
> Two more (unrelated) issues here:
> 
>    1. This will silently not register anything if SMEM probes after the
>       qcom-sleep-stats driver (qcom_smem_get() will return -EPROBE_DEFER)
>       and you will just skip registering the debugfs files.

I think module loading internally takes care of this.
we're making a direct function call into the qcom_smem driver, so we
already have a hard dependency on qcom_smem.ko being loaded.

> 
>    2. In qcom_subsystem_sleep_stats_show() you say
>       /* Items are allocated lazily, so lookup pointer each time */
> 
>       But, if the lookup fails here you don't register the debugfs file
>       at all. Does this work if the subsystem is started after this driver?

Good catch. if the subsystem starts after this driver is loaded, the 
look up fails during probe and we don't create debugfs file for the 
subsystem.

one need to unload/load the driver again after sometime in bootup so by 
that time all the subsytems (modem, adsp, cdsp, etc) are up and we 
create debugfs file for them.

we have downstream fix for this to create the debugfs files irrespective 
of look up fails or not. i have plan to add it once the base driver gets 
merged.

Thanks,
Maulik

> 
> Thanks,
> Stephan
> 

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

Re: [PATCH v11 2/5] soc: qcom: Add Sleep stats driver

[Stephan Gerhold]

On Fri, Oct 08, 2021 at 02:45:20PM +0530, Maulik Shah wrote:
> On 10/7/2021 11:59 PM, Stephan Gerhold wrote:
> > On Thu, Oct 07, 2021 at 03:27:26PM +0530, Maulik Shah wrote:
> > > From: Mahesh Sivasubramanian <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/qcom_sleep_stats.c | 259 ++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 270 insertions(+)
> > >   create mode 100644 drivers/soc/qcom/qcom_sleep_stats.c
> > > 
> > > [...]
> > > +
> > > +static int qcom_subsystem_sleep_stats_show(struct seq_file *s, void *unused)
> > > +{
> > > +	struct subsystem_data *subsystem = s->private;
> > > +	struct sleep_stats *stat;
> > > +
> > > +	/* Items are allocated lazily, so lookup pointer each time */
> > > +	stat = qcom_smem_get(subsystem->pid, subsystem->smem_item, NULL);
> > > +	if (IS_ERR(stat))
> > > +		return -EIO;
> > > +
> > > [...]
> > > +
> > > +static void qcom_create_subsystem_stat_files(struct dentry *root)
> > > +{
> > > +	const struct sleep_stats *stat;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
> > > +		stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item, NULL);
> > > +		if (IS_ERR(stat))
> > > +			continue;
> > > +
> > > +		debugfs_create_file(subsystems[i].name, 0400, root, (void *)&subsystems[i],
> > > +				    &qcom_subsystem_sleep_stats_fops);
> > 
> > This causes WARNINGs on MSM8996 and MSM8916:
> > 
> > [    0.503054] ------------[ cut here ]------------
> > [    0.503100] WARNING: CPU: 1 PID: 1 at drivers/soc/qcom/smem.c:587 qcom_smem_get+0x184/0x1b0
> > [    0.503184] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.15.0-rc4+ #378
> > [    0.503218] Hardware name: Xiaomi Mi Note 2 (DT)
> > [    0.503278] pc : qcom_smem_get+0x184/0x1b0
> > [    0.503307] lr : qcom_sleep_stats_probe+0xfc/0x20
> > [    0.503875] Call trace:
> > [    0.503896]  qcom_smem_get+0x184/0x1b0
> > [    0.503925]  qcom_sleep_stats_probe+0xfc/0x270
> > 
> > AFAICT from downstream the smem subsystem information is only read in
> > the rpmh_master_stat.c driver, should this be specific to RPMh?
> 
> Thanks for checking this on MSM8996. Probably it doesnot have SMEM items
> allocated so causes WARNINGs.
> 
> Keeping subsystem stats info in SMEM is not limited to only RPMH targets.
> Downstream has some RPM targets which also uses SMEM to store
> subsystem stats so the driver is kept generic.
> 

Thanks for clarifying. To be honest, I'm not sure if the WARN_ON() in
smem is very useful since this isn't really fundamentally different as
if the entry is not allocated. But at the end all that matters is that
there are no warnings when loading this driver on older targets.

> > 
> > There is a rpm_master_stat.c too but that looks quite different,
> > so I guess the approach is different with RPM?
> 
> Right. on existing upstream RPM targets i can skip to create/get SMEM items
> since
> they are not guranteed to be present and one should continue to use
> rpm_master_stats.c to get subsystem stats. (this uses entirely different
> data structure for sleep stats and are not part of RPM data ram/SMEM and are
> deprecated in downstream).
> 
> > 
> > Two more (unrelated) issues here:
> > 
> >    1. This will silently not register anything if SMEM probes after the
> >       qcom-sleep-stats driver (qcom_smem_get() will return -EPROBE_DEFER)
> >       and you will just skip registering the debugfs files.
> 
> I think module loading internally takes care of this.
> we're making a direct function call into the qcom_smem driver, so we
> already have a hard dependency on qcom_smem.ko being loaded.
> 

The driver can also be built-in and in this case this is not guaranteed.
This actually happened to me when I tried this on MSM8916: I did not get
the WARNINGs because all the qcom_smem_get() just returned -EPROBE_DEFER.
qcom_smem was then probed after qcom_sleep_stats. Of course, the smem
items are not present there anyway so it did not matter.

> > 
> >    2. In qcom_subsystem_sleep_stats_show() you say
> >       /* Items are allocated lazily, so lookup pointer each time */
> > 
> >       But, if the lookup fails here you don't register the debugfs file
> >       at all. Does this work if the subsystem is started after this driver?
> 
> Good catch. if the subsystem starts after this driver is loaded, the look up
> fails during probe and we don't create debugfs file for the subsystem.
> 
> one need to unload/load the driver again after sometime in bootup so by that
> time all the subsytems (modem, adsp, cdsp, etc) are up and we create debugfs
> file for them.
> 
> we have downstream fix for this to create the debugfs files irrespective of
> look up fails or not. i have plan to add it once the base driver gets
> merged.
> 

OK, I don't seem to have any recent Qualcomm platform that actually has
subsystem stats in SMEM so it doesn't bother me. I just wanted to
mention it for reference. :)

Thanks!
Stephan

Re: [PATCH v11 1/5] dt-bindings: Introduce QCOM Sleep stats bindings

[Stephan Gerhold]

On Fri, Oct 08, 2021 at 02:20:24PM +0530, Maulik Shah wrote:
> On 10/8/2021 1:20 AM, Stephan Gerhold wrote:
> > On Thu, Oct 07, 2021 at 03:27:25PM +0530, Maulik Shah wrote:
> > > From: Mahesh Sivasubramanian <msivasub@codeaurora.org>
> > > 
> > > Add device binding documentation for Qualcomm Technologies, Inc. (QTI)
> > > Sleep stats driver. The driver is used for displaying 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/qcom-sleep-stats.yaml        | 47 ++++++++++++++++++++++
> > >   1 file changed, 47 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
> > > new file mode 100644
> > > index 0000000..5213daf
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
> > > @@ -0,0 +1,47 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/soc/qcom/qcom-sleep-stats.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Qualcomm Technologies, Inc. (QTI) Sleep stats bindings
> > > +
> > > +maintainers:
> > > +  - Maulik Shah <mkshah@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
> > > +  - |
> > > +    sram@c3f0000 {
> > > +      compatible = "qcom,rpmh-sleep-stats";
> > > +      reg = <0x0c3f0000 0x400>;
> > > +    };
> > > +  # Example of rpm sleep stats
> > > +  - |
> > > +    sram@4690000 {
> > > +      compatible = "qcom,rpm-sleep-stats";
> > > +      reg = <0x04690000 0x400>;
> > > +    };
> > 
> > Does this region really only contain "rpm-*sleep*-stats"? AFAICT this is
> > really a more generic memory region where various offsets are read from.
> 
> Right the area contains all kind of sleep stats/logs.
> 
> > 
> > These are all the offsets in msm8998-pm.dtsi downstream [1]:
> >    ...9000c: rpm-rail-stats offset
> >    ...90014: rpm-sleep-stats offset (RPM_DYNAMIC_ADDR in your driver)
> >    ...90018: rpm-log offset
> >    ...9001c: "RPM FREE HEAP SPACE"
> > 
> > How would you set up any of the other drivers if the entire region
> > is declared as "rpm-sleep-stats"?
> 
> We don't need to setup other drivers.
> The idea is to have single stats driver (qcom_sleep_stats) that can be
> enahanced to to read other stats also on both RPM/ PRMH targets.
> 
> Today this driver reads only sleep stats from offset 0x90014.
> if in future say, we want to read rpm-rail-stats from offset 0x9000c then it
> can be added in the existing driver.
> 
> In the rpm data, add one more entry to indicate reading rpm-rail-stats,
> something like below and add new compatible flag for the target, may be
> "qcom,rpm-sleep-stats-legacy" and should work like,
> 
> #define RPM_RAIL_STATS_ADDR 0xc
> 
> static const struct stats_config rpm_data = {
>         .stats_offset = 0,
>         .num_records = 2,
>         .appended_stats_avail = true,
>         .dynamic_offset = true,
>         .rpm_rail_stats = true,
> };
> 
> and check in driver probe like
> 
>         if (config->rpm_rail_stats) {
> 	//create rpm rail stats related files.
>         }
> 
> Similarly for other rpm-logs / any other stats can be added in same driver.
> 
> Hope this clarifies.
> 

Having the same driver for all this sounds good, thanks for clarifying!

> > 
> > Perhaps this region should have a more generic name that represents what
> > it actually is and not only one of the information it contains, similar
> > to "qcom,rpm-msg-ram"?
> 
> sram looks good to me. Actually on RPM targets its RPM data ram and RPMH
> targets its msgram. To keep the name common sram is used here.
> 

The node name (sram@...) is fine. I was actually referring to the
"compatible". Given what you have written above there is more in that
region than just "qcom,rpm-*sleep*-stats". There are also logs and other
stats there. Perhaps "qcom,rpm(h)-stats" (without -sleep-) would already
be more clear.

Or, my other suggestion was to not make any assumption about particular
contents of the data RAM in the device tree. The compatible could also
be called simply "qcom,rpm-data-ram" (just like "qcom,rpm-msg-ram") and
the assumption what it contains (e.g. sleep stats) could be made in your
existing driver.

This is probably a matter of preference at the end and won't make a big
difference but I just wanted to mention it.

For example, to make your driver work on MSM8916 I had to declare a much
larger memory region (e.g. 0x10000 vs 0x400):

		sram@290000 {
			compatible = "qcom,rpm-sleep-stats";
			reg = <0x00290000 0x10000>;
		};

This is because RPM_DYNAMIC_ADDR contains the offset 0xdba0 instead of
e.g. 0x020c on newer SoCs and the memory region must be large enough to
cover the offset.

It just feels a bit weird for me to describe such a large region as only
containing "qcom,rpm-*sleep*-stats". "qcom,rpm-data-ram" might better
describe it what it really is and then your driver can decide what to do
with it.

But, as I said, this is just naming which is always subjective. :)

Thanks!
Stephan

Re: [PATCH v11 1/5] dt-bindings: Introduce QCOM Sleep stats bindings

[Maulik Shah]

Hi Stephan,

On 10/8/2021 4:29 PM, Stephan Gerhold wrote:
> On Fri, Oct 08, 2021 at 02:20:24PM +0530, Maulik Shah wrote:
>> On 10/8/2021 1:20 AM, Stephan Gerhold wrote:
>>> On Thu, Oct 07, 2021 at 03:27:25PM +0530, Maulik Shah wrote:
>>>> From: Mahesh Sivasubramanian <msivasub@codeaurora.org>
>>>>
>>>> Add device binding documentation for Qualcomm Technologies, Inc. (QTI)
>>>> Sleep stats driver. The driver is used for displaying 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/qcom-sleep-stats.yaml        | 47 ++++++++++++++++++++++
>>>>    1 file changed, 47 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
>>>> new file mode 100644
>>>> index 0000000..5213daf
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
>>>> @@ -0,0 +1,47 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/soc/qcom/qcom-sleep-stats.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Qualcomm Technologies, Inc. (QTI) Sleep stats bindings
>>>> +
>>>> +maintainers:
>>>> +  - Maulik Shah <mkshah@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
>>>> +  - |
>>>> +    sram@c3f0000 {
>>>> +      compatible = "qcom,rpmh-sleep-stats";
>>>> +      reg = <0x0c3f0000 0x400>;
>>>> +    };
>>>> +  # Example of rpm sleep stats
>>>> +  - |
>>>> +    sram@4690000 {
>>>> +      compatible = "qcom,rpm-sleep-stats";
>>>> +      reg = <0x04690000 0x400>;
>>>> +    };
>>>
>>> Does this region really only contain "rpm-*sleep*-stats"? AFAICT this is
>>> really a more generic memory region where various offsets are read from.
>>
>> Right the area contains all kind of sleep stats/logs.
>>
>>>
>>> These are all the offsets in msm8998-pm.dtsi downstream [1]:
>>>     ...9000c: rpm-rail-stats offset
>>>     ...90014: rpm-sleep-stats offset (RPM_DYNAMIC_ADDR in your driver)
>>>     ...90018: rpm-log offset
>>>     ...9001c: "RPM FREE HEAP SPACE"
>>>
>>> How would you set up any of the other drivers if the entire region
>>> is declared as "rpm-sleep-stats"?
>>
>> We don't need to setup other drivers.
>> The idea is to have single stats driver (qcom_sleep_stats) that can be
>> enahanced to to read other stats also on both RPM/ PRMH targets.
>>
>> Today this driver reads only sleep stats from offset 0x90014.
>> if in future say, we want to read rpm-rail-stats from offset 0x9000c then it
>> can be added in the existing driver.
>>
>> In the rpm data, add one more entry to indicate reading rpm-rail-stats,
>> something like below and add new compatible flag for the target, may be
>> "qcom,rpm-sleep-stats-legacy" and should work like,
>>
>> #define RPM_RAIL_STATS_ADDR 0xc
>>
>> static const struct stats_config rpm_data = {
>>          .stats_offset = 0,
>>          .num_records = 2,
>>          .appended_stats_avail = true,
>>          .dynamic_offset = true,
>>          .rpm_rail_stats = true,
>> };
>>
>> and check in driver probe like
>>
>>          if (config->rpm_rail_stats) {
>> 	//create rpm rail stats related files.
>>          }
>>
>> Similarly for other rpm-logs / any other stats can be added in same driver.
>>
>> Hope this clarifies.
>>
> 
> Having the same driver for all this sounds good, thanks for clarifying!
> 
>>>
>>> Perhaps this region should have a more generic name that represents what
>>> it actually is and not only one of the information it contains, similar
>>> to "qcom,rpm-msg-ram"?
>>
>> sram looks good to me. Actually on RPM targets its RPM data ram and RPMH
>> targets its msgram. To keep the name common sram is used here.
>>
> 
> The node name (sram@...) is fine. I was actually referring to the
> "compatible". Given what you have written above there is more in that
> region than just "qcom,rpm-*sleep*-stats". There are also logs and other
> stats there. Perhaps "qcom,rpm(h)-stats" (without -sleep-) would already
> be more clear.

ok updated in v12 to name it qcom,rpm(h)-stats. similarly driver name 
also updated from qcom_sleep_stats to qcom_stats.

> 
> Or, my other suggestion was to not make any assumption about particular
> contents of the data RAM in the device tree. The compatible could also
> be called simply "qcom,rpm-data-ram" (just like "qcom,rpm-msg-ram") and
> the assumption what it contains (e.g. sleep stats) could be made in your
> existing driver.
> 
> This is probably a matter of preference at the end and won't make a big
> difference but I just wanted to mention it.
> 
> For example, to make your driver work on MSM8916 I had to declare a much
> larger memory region (e.g. 0x10000 vs 0x400):
> 
> 		sram@290000 {
> 			compatible = "qcom,rpm-sleep-stats";
> 			reg = <0x00290000 0x10000>;
> 		};
> 

the driver can not be as is enabled for MSM8916. Actually on MSM8916 
(even though its RPM target) it uses fixed offset 0xdba0 from the base 
address in the devicetree.

> This is because RPM_DYNAMIC_ADDR contains the offset 0xdba0 instead of
> e.g. 0x020c on newer SoCs and the memory region must be large enough to
> cover the offset.

On all other RPM targets followed by MSM8916 the address for soc sleep 
stats is present at offset 0x14 (RPM_DYNAMIC_ADDR ) from the base 
address but not on MSM8916. so MSM8916 is a special case to enable for 
for RPM stats

To enable this for MSM8916, dynamic_offset need to be false and 
stats_offset need to be given directly as 0xdba0, something similar as 
RPMH targets.

static const struct stats_config rpm_legacy_data = {
         .stats_offset = 0xdba0,
         .num_records = 2,
         .appended_stats_avail = true,
         .dynamic_offset = false,
         .subsystem_stats_in_smem = false,
};

{ .compatible = "qcom,rpm-legacy-stats", .data = &rpm_legacy_data },

and then add

                 sram@290000 {
                         compatible = "qcom,rpm-legacy-stats";
                         reg = <0x00290000 0x10000>;
                 };

which should work fine.

I donot have MSM8916 to validate this but when this driver boots up it 
should create "vlow" and "vmin" files at /sys/kernel/debug/qcom_sleep_stats/
if that is done, it means you are reading stats from correct address 
since the file name is read from the memory itself.

Thanks,
Maulik

> 
> It just feels a bit weird for me to describe such a large region as only
> containing "qcom,rpm-*sleep*-stats". "qcom,rpm-data-ram" might better
> describe it what it really is and then your driver can decide what to do
> with it.
> 
> But, as I said, this is just naming which is always subjective. :)
> 
> Thanks!
> Stephan
> 

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

Re: [PATCH v11 2/5] soc: qcom: Add Sleep stats driver

[Maulik Shah]

Hi Stephan,

On 10/8/2021 4:04 PM, Stephan Gerhold wrote:
> On Fri, Oct 08, 2021 at 02:45:20PM +0530, Maulik Shah wrote:
>> On 10/7/2021 11:59 PM, Stephan Gerhold wrote:
>>> On Thu, Oct 07, 2021 at 03:27:26PM +0530, Maulik Shah wrote:
>>>> From: Mahesh Sivasubramanian <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/qcom_sleep_stats.c | 259 ++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 270 insertions(+)
>>>>    create mode 100644 drivers/soc/qcom/qcom_sleep_stats.c
>>>>
>>>> [...]
>>>> +
>>>> +static int qcom_subsystem_sleep_stats_show(struct seq_file *s, void *unused)
>>>> +{
>>>> +	struct subsystem_data *subsystem = s->private;
>>>> +	struct sleep_stats *stat;
>>>> +
>>>> +	/* Items are allocated lazily, so lookup pointer each time */
>>>> +	stat = qcom_smem_get(subsystem->pid, subsystem->smem_item, NULL);
>>>> +	if (IS_ERR(stat))
>>>> +		return -EIO;
>>>> +
>>>> [...]
>>>> +
>>>> +static void qcom_create_subsystem_stat_files(struct dentry *root)
>>>> +{
>>>> +	const struct sleep_stats *stat;
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
>>>> +		stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item, NULL);
>>>> +		if (IS_ERR(stat))
>>>> +			continue;
>>>> +
>>>> +		debugfs_create_file(subsystems[i].name, 0400, root, (void *)&subsystems[i],
>>>> +				    &qcom_subsystem_sleep_stats_fops);
>>>
>>> This causes WARNINGs on MSM8996 and MSM8916:
>>>
>>> [    0.503054] ------------[ cut here ]------------
>>> [    0.503100] WARNING: CPU: 1 PID: 1 at drivers/soc/qcom/smem.c:587 qcom_smem_get+0x184/0x1b0
>>> [    0.503184] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.15.0-rc4+ #378
>>> [    0.503218] Hardware name: Xiaomi Mi Note 2 (DT)
>>> [    0.503278] pc : qcom_smem_get+0x184/0x1b0
>>> [    0.503307] lr : qcom_sleep_stats_probe+0xfc/0x20
>>> [    0.503875] Call trace:
>>> [    0.503896]  qcom_smem_get+0x184/0x1b0
>>> [    0.503925]  qcom_sleep_stats_probe+0xfc/0x270
>>>
>>> AFAICT from downstream the smem subsystem information is only read in
>>> the rpmh_master_stat.c driver, should this be specific to RPMh?
>>
>> Thanks for checking this on MSM8996. Probably it doesnot have SMEM items
>> allocated so causes WARNINGs.
>>
>> Keeping subsystem stats info in SMEM is not limited to only RPMH targets.
>> Downstream has some RPM targets which also uses SMEM to store
>> subsystem stats so the driver is kept generic.
>>
> 
> Thanks for clarifying. To be honest, I'm not sure if the WARN_ON() in
> smem is very useful since this isn't really fundamentally different as
> if the entry is not allocated. But at the end all that matters is that
> there are no warnings when loading this driver on older targets.
> 
>>>
>>> There is a rpm_master_stat.c too but that looks quite different,
>>> so I guess the approach is different with RPM?
>>
>> Right. on existing upstream RPM targets i can skip to create/get SMEM items
>> since
>> they are not guranteed to be present and one should continue to use
>> rpm_master_stats.c to get subsystem stats. (this uses entirely different
>> data structure for sleep stats and are not part of RPM data ram/SMEM and are
>> deprecated in downstream).
>>
>>>
>>> Two more (unrelated) issues here:
>>>
>>>     1. This will silently not register anything if SMEM probes after the
>>>        qcom-sleep-stats driver (qcom_smem_get() will return -EPROBE_DEFER)
>>>        and you will just skip registering the debugfs files.
>>
>> I think module loading internally takes care of this.
>> we're making a direct function call into the qcom_smem driver, so we
>> already have a hard dependency on qcom_smem.ko being loaded.
>>
> 
> The driver can also be built-in and in this case this is not guaranteed.
> This actually happened to me when I tried this on MSM8916: I did not get
> the WARNINGs because all the qcom_smem_get() just returned -EPROBE_DEFER.
> qcom_smem was then probed after qcom_sleep_stats. Of course, the smem
> items are not present there anyway so it did not matter.


I see SMEM is in arch init call, Updated in v12 to register driver in 
late_init by the time SMEM would have probed already and should not need 
to handle -EPROBE_DEFER.


> 
>>>
>>>     2. In qcom_subsystem_sleep_stats_show() you say
>>>        /* Items are allocated lazily, so lookup pointer each time */
>>>
>>>        But, if the lookup fails here you don't register the debugfs file
>>>        at all. Does this work if the subsystem is started after this driver?
>>
>> Good catch. if the subsystem starts after this driver is loaded, the look up
>> fails during probe and we don't create debugfs file for the subsystem.
>>
>> one need to unload/load the driver again after sometime in bootup so by that
>> time all the subsytems (modem, adsp, cdsp, etc) are up and we create debugfs
>> file for them.
>>
>> we have downstream fix for this to create the debugfs files irrespective of
>> look up fails or not. i have plan to add it once the base driver gets
>> merged.
>>
> 
> OK, I don't seem to have any recent Qualcomm platform that actually has
> subsystem stats in SMEM so it doesn't bother me. I just wanted to
> mention it for reference. :)

Ok updated driver to not use SMEM for RPM based targets for existing 
upstream targets (when such target goes upstream in future which uses 
SMEM for subsystem stats we can enable back with new compatible say -v2).

Thanks,
Maulik

> 
> Thanks!
> Stephan
> 

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

Re: [PATCH v11 1/5] dt-bindings: Introduce QCOM Sleep stats bindings

[Stephan Gerhold]

Hi Maulik,

On Wed, Oct 13, 2021 at 11:27:30AM +0530, Maulik Shah wrote:
> On 10/8/2021 4:29 PM, Stephan Gerhold wrote:
> > On Fri, Oct 08, 2021 at 02:20:24PM +0530, Maulik Shah wrote:
> > > On 10/8/2021 1:20 AM, Stephan Gerhold wrote:
> > > > On Thu, Oct 07, 2021 at 03:27:25PM +0530, Maulik Shah wrote:
> > > > > From: Mahesh Sivasubramanian <msivasub@codeaurora.org>
> > > > > 
> > > > > Add device binding documentation for Qualcomm Technologies, Inc. (QTI)
> > > > > Sleep stats driver. The driver is used for displaying 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/qcom-sleep-stats.yaml        | 47 ++++++++++++++++++++++
> > > > >    1 file changed, 47 insertions(+)
> > > > >    create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
> > > > > new file mode 100644
> > > > > index 0000000..5213daf
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml
> > > > > @@ -0,0 +1,47 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/soc/qcom/qcom-sleep-stats.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Qualcomm Technologies, Inc. (QTI) Sleep stats bindings
> > > > > +
> > > > > +maintainers:
> > > > > +  - Maulik Shah <mkshah@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
> > > > > +  - |
> > > > > +    sram@c3f0000 {
> > > > > +      compatible = "qcom,rpmh-sleep-stats";
> > > > > +      reg = <0x0c3f0000 0x400>;
> > > > > +    };
> > > > > +  # Example of rpm sleep stats
> > > > > +  - |
> > > > > +    sram@4690000 {
> > > > > +      compatible = "qcom,rpm-sleep-stats";
> > > > > +      reg = <0x04690000 0x400>;
> > > > > +    };
> > > > 
> > > > Does this region really only contain "rpm-*sleep*-stats"? AFAICT this is
> > > > really a more generic memory region where various offsets are read from.
> > > 
> > > Right the area contains all kind of sleep stats/logs.
> > > 
> > > > 
> > > > These are all the offsets in msm8998-pm.dtsi downstream [1]:
> > > >     ...9000c: rpm-rail-stats offset
> > > >     ...90014: rpm-sleep-stats offset (RPM_DYNAMIC_ADDR in your driver)
> > > >     ...90018: rpm-log offset
> > > >     ...9001c: "RPM FREE HEAP SPACE"
> > > > 
> > > > How would you set up any of the other drivers if the entire region
> > > > is declared as "rpm-sleep-stats"?
> > > 
> > > We don't need to setup other drivers.
> > > The idea is to have single stats driver (qcom_sleep_stats) that can be
> > > enahanced to to read other stats also on both RPM/ PRMH targets.
> > > 
> > > Today this driver reads only sleep stats from offset 0x90014.
> > > if in future say, we want to read rpm-rail-stats from offset 0x9000c then it
> > > can be added in the existing driver.
> > > 
> > > In the rpm data, add one more entry to indicate reading rpm-rail-stats,
> > > something like below and add new compatible flag for the target, may be
> > > "qcom,rpm-sleep-stats-legacy" and should work like,
> > > 
> > > #define RPM_RAIL_STATS_ADDR 0xc
> > > 
> > > static const struct stats_config rpm_data = {
> > >          .stats_offset = 0,
> > >          .num_records = 2,
> > >          .appended_stats_avail = true,
> > >          .dynamic_offset = true,
> > >          .rpm_rail_stats = true,
> > > };
> > > 
> > > and check in driver probe like
> > > 
> > >          if (config->rpm_rail_stats) {
> > > 	//create rpm rail stats related files.
> > >          }
> > > 
> > > Similarly for other rpm-logs / any other stats can be added in same driver.
> > > 
> > > Hope this clarifies.
> > > 
> > 
> > Having the same driver for all this sounds good, thanks for clarifying!
> > 
> > > > 
> > > > Perhaps this region should have a more generic name that represents what
> > > > it actually is and not only one of the information it contains, similar
> > > > to "qcom,rpm-msg-ram"?
> > > 
> > > sram looks good to me. Actually on RPM targets its RPM data ram and RPMH
> > > targets its msgram. To keep the name common sram is used here.
> > > 
> > 
> > The node name (sram@...) is fine. I was actually referring to the
> > "compatible". Given what you have written above there is more in that
> > region than just "qcom,rpm-*sleep*-stats". There are also logs and other
> > stats there. Perhaps "qcom,rpm(h)-stats" (without -sleep-) would already
> > be more clear.
> 
> ok updated in v12 to name it qcom,rpm(h)-stats. similarly driver name also
> updated from qcom_sleep_stats to qcom_stats.
> 

Thanks!

> > 
> > Or, my other suggestion was to not make any assumption about particular
> > contents of the data RAM in the device tree. The compatible could also
> > be called simply "qcom,rpm-data-ram" (just like "qcom,rpm-msg-ram") and
> > the assumption what it contains (e.g. sleep stats) could be made in your
> > existing driver.
> > 
> > This is probably a matter of preference at the end and won't make a big
> > difference but I just wanted to mention it.
> > 
> > For example, to make your driver work on MSM8916 I had to declare a much
> > larger memory region (e.g. 0x10000 vs 0x400):
> > 
> > 		sram@290000 {
> > 			compatible = "qcom,rpm-sleep-stats";
> > 			reg = <0x00290000 0x10000>;
> > 		};
> > 
> 
> the driver can not be as is enabled for MSM8916. Actually on MSM8916 (even
> though its RPM target) it uses fixed offset 0xdba0 from the base address in
> the devicetree.
> 
> > This is because RPM_DYNAMIC_ADDR contains the offset 0xdba0 instead of
> > e.g. 0x020c on newer SoCs and the memory region must be large enough to
> > cover the offset.
> 
> On all other RPM targets followed by MSM8916 the address for soc sleep stats
> is present at offset 0x14 (RPM_DYNAMIC_ADDR ) from the base address but not
> on MSM8916. so MSM8916 is a special case to enable for for RPM stats
> 

I already tested it on MSM8916 and this is only partially true. :)

It looks like the RPM_DYNAMIC_ADDR was added together with some firmware
update. I tested on several devices and on most of them RPM_DYNAMIC_ADDR
reads 0xdba0 (that is, there are no changes necessary in the driver).

So far I have only found one single device with fairly old firmware
where RPM_DYNAMIC_ADDR reads 0x0.

> To enable this for MSM8916, dynamic_offset need to be false and stats_offset
> need to be given directly as 0xdba0, something similar as RPMH targets.
> 
> static const struct stats_config rpm_legacy_data = {
>         .stats_offset = 0xdba0,
>         .num_records = 2,
>         .appended_stats_avail = true,
>         .dynamic_offset = false,
>         .subsystem_stats_in_smem = false,
> };
> 
> { .compatible = "qcom,rpm-legacy-stats", .data = &rpm_legacy_data },
> 
> and then add
> 
>                 sram@290000 {
>                         compatible = "qcom,rpm-legacy-stats";
>                         reg = <0x00290000 0x10000>;
>                 };
> 
> which should work fine.
> 

But yeah, I guess we might need the fixed offset anyway at some point
for even older targets than MSM8916, so it is probably safer to add it
like this. Will investigate it some more once your patch is accepted.

> I donot have MSM8916 to validate this but when this driver boots up it
> should create "vlow" and "vmin" files at /sys/kernel/debug/qcom_sleep_stats/
> if that is done, it means you are reading stats from correct address since
> the file name is read from the memory itself.
> 

Almost, it's "vmin" and "xosd" ("XO Shutdown" I guess) on MSM8916. :)

Thanks,
Stephan