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

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

[Maulik Shah]

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

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

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

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

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

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

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

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

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

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

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

 .../bindings/soc/qcom/soc-sleep-stats.yaml         |  46 ++++
 arch/arm64/boot/dts/qcom/sc7180.dtsi               |   7 +-
 arch/arm64/configs/defconfig                       |   1 +
 drivers/soc/qcom/Kconfig                           |   9 +
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/soc_sleep_stats.c                 | 244 +++++++++++++++++++++
 6 files changed, 307 insertions(+), 1 deletion(-)
 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 v5 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 0000000..d0c751d
--- /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 v5 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 | 244 +++++++++++++++++++++++++++++++++++++
 3 files changed, 254 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 0000000..0db7c3d
--- /dev/null
+++ b/drivers/soc/qcom/soc_sleep_stats.c
@@ -0,0 +1,244 @@
+// 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;
+};
+
+struct stats_prv_data {
+	const struct stats_config *config;
+	void __iomem *reg;
+};
+
+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)
+{
+	struct stats_prv_data *prv_data = s->private;
+	void __iomem *reg = prv_data->reg;
+	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 (prv_data->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 stats_prv_data *prv_data)
+{
+	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 < prv_data[0].config->num_records; i++) {
+		offset = STAT_TYPE_ADDR + (i * sizeof(struct sleep_stats));
+
+		if (prv_data[0].config->appended_stats_avail)
+			offset += i * sizeof(struct appended_stats);
+
+		prv_data[i].reg = reg + offset;
+
+		type = readl(prv_data[i].reg);
+		memcpy(stat_type, &type, sizeof(u32));
+		debugfs_create_file(stat_type, 0444, root,
+				    &prv_data[i],
+				    &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;
+	const struct stats_config *config;
+	struct stats_prv_data *prv_data;
+	int i;
+
+	config = device_get_match_data(&pdev->dev);
+	if (!config)
+		return -ENODEV;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return PTR_ERR(res);
+
+	offset_addr = ioremap(res->start + config->offset_addr, sizeof(u32));
+	if (IS_ERR(offset_addr))
+		return PTR_ERR(offset_addr);
+
+	stats_base = res->start | readl_relaxed(offset_addr);
+	stats_size = resource_size(res);
+	iounmap(offset_addr);
+
+	reg = devm_ioremap(&pdev->dev, stats_base, stats_size);
+	if (!reg)
+		return -ENOMEM;
+
+	prv_data = devm_kzalloc(&pdev->dev, config->num_records *
+				sizeof(struct stats_prv_data), GFP_KERNEL);
+	if (!prv_data)
+		return -ENOMEM;
+
+	for (i = 0; i < config->num_records; i++)
+		prv_data[i].config = config;
+
+	root = create_debugfs_entries(reg, prv_data);
+	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 v5 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..0eb6695 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 v5 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 5f15901..6228177 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -794,6 +794,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 v5 1/4] dt-bindings: Introduce SoC sleep stats bindings

[Stephen Boyd]

Quoting Maulik Shah (2020-03-17 21:29:15)
> From: Mahesh Sivasubramanian <msivasub@codeaurora.org>
> 
> Add device binding documentation for Qualcomm Technologies, Inc. (QTI)
> SoC sleep stats driver. The driver is used for displaying SoC sleep
> statistic maintained by Always On Processor or Resource Power Manager.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---

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

Two nits below.

> diff --git a/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml
> new file mode 100644
> index 0000000..d0c751d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml
> @@ -0,0 +1,46 @@
[...]
> +
> +examples:
> +  # Example of rpmh sleep stats
> +  - |
> +    rpmh-sleep-stats@c3f0000 {

I would think 'memory' would be a more appropriate node name, but OK.

> +      compatible = "qcom,rpmh-sleep-stats";
> +      reg = <0 0xc3f0000 0 0x400>;

Please add a leading 0 to the address to pad it out to 8 digits.

> +    };

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

[Stephen Boyd]

Quoting Maulik Shah (2020-03-17 21:29:16)
> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
> new file mode 100644
> index 0000000..0db7c3d
> --- /dev/null
> +++ b/drivers/soc/qcom/soc_sleep_stats.c
> @@ -0,0 +1,244 @@
> +// 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[] = {

This can be const.

> +       { "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;
> +};
> +
> +struct stats_prv_data {
> +       const struct stats_config *config;

Can we have 'bool has_appended_stats' here instead?

> +       void __iomem *reg;
> +};
> +
> +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)

Make stat const please.

> +{
> +       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()

This assumes that arch_timer_read_counter() is returning the physical
counter? Is accumulated duration useful for anything? I don't like that
we're relying on the arch timer code to return a certain counter value
that may or may not be the same as what is written into smem.

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

We already get this during probe in create_debugfs_entries() (which is
too generic of a name by the way). Why can't we stash that pointer away
so that it comes through the 'd' parameter to this function?

> +       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)
> +{
> +       struct stats_prv_data *prv_data = s->private;
> +       void __iomem *reg = prv_data->reg;
> +       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);

Ok I see. The same stat info is in SMEM and also in some IO memory
location? So we have to read one from an iomem region and one from smem?
Please make subsystem_sleep_stats_show() take a 'struct
smem_sleep_stats' through 'd' that has the __le32 and __le64 markings
and then do the byte swaps into a stack local struct sleep_stats that
print_sleep_stats() can use.

> +
> +       if (prv_data->config->appended_stats_avail) {
> +               struct appended_stats app_stat;

Just use a u32 for this. The struct is not useful here.

> +
> +               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 stats_prv_data *prv_data)

I'd prefer this was just inlined into probe.

> +{
> +       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 < prv_data[0].config->num_records; i++) {

Pass config as another argument instead of attaching it to prv_data
please.

> +               offset = STAT_TYPE_ADDR + (i * sizeof(struct sleep_stats));
> +
> +               if (prv_data[0].config->appended_stats_avail)
> +                       offset += i * sizeof(struct appended_stats);

I was imagining a macro like, SLEEP_STATn(i, has_appended) that did all the
math to figure out the offset.

> +
> +               prv_data[i].reg = reg + offset;
> +
> +               type = readl(prv_data[i].reg);
> +               memcpy(stat_type, &type, sizeof(u32));

Is it a 4-byte ascii register that may or may not be NUL terminated? We
should use __raw_readl() then so we don't do an endian swap. Using
memcpy_fromio() does this all for you.

> +               debugfs_create_file(stat_type, 0444, root,

Maybe just 0400? Not sure why everyone in the world needs to read this.

> +                                   &prv_data[i],
> +                                   &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;
> +       const struct stats_config *config;
> +       struct stats_prv_data *prv_data;
> +       int i;
> +
> +       config = device_get_match_data(&pdev->dev);
> +       if (!config)
> +               return -ENODEV;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res)
> +               return PTR_ERR(res);
> +
> +       offset_addr = ioremap(res->start + config->offset_addr, sizeof(u32));
> +       if (IS_ERR(offset_addr))
> +               return PTR_ERR(offset_addr);
> +
> +       stats_base = res->start | readl_relaxed(offset_addr);

I still don't get it, but whatever. I highly doubt the lower bits are
anything besides 0 so a logical OR vs addition is not different.

> +       stats_size = resource_size(res);
> +       iounmap(offset_addr);
> +
> +       reg = devm_ioremap(&pdev->dev, stats_base, stats_size);
> +       if (!reg)
> +               return -ENOMEM;
> +
> +       prv_data = devm_kzalloc(&pdev->dev, config->num_records *
> +                               sizeof(struct stats_prv_data), GFP_KERNEL);

We have devm_kcalloc() for array allocations.

> +       if (!prv_data)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < config->num_records; i++)
> +               prv_data[i].config = config;
> +
> +       root = create_debugfs_entries(reg, prv_data);
> +       platform_set_drvdata(pdev, root);
> +
> +       return 0;
> +}

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

[Maulik Shah]

Hi,

On 3/19/2020 2:59 AM, Stephen Boyd wrote:
> Quoting Maulik Shah (2020-03-17 21:29:15)
>> From: Mahesh Sivasubramanian <msivasub@codeaurora.org>
>>
>> Add device binding documentation for Qualcomm Technologies, Inc. (QTI)
>> SoC sleep stats driver. The driver is used for displaying SoC sleep
>> statistic maintained by Always On Processor or Resource Power Manager.
>>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> ---
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Thanks Stephen.
>
> Two nits below.
>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml
>> new file mode 100644
>> index 0000000..d0c751d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml
>> @@ -0,0 +1,46 @@
> [...]
>> +
>> +examples:
>> +  # Example of rpmh sleep stats
>> +  - |
>> +    rpmh-sleep-stats@c3f0000 {
> I would think 'memory' would be a more appropriate node name, but OK.
>
>> +      compatible = "qcom,rpmh-sleep-stats";
>> +      reg = <0 0xc3f0000 0 0x400>;
> Please add a leading 0 to the address to pad it out to 8 digits.
I will address this in v6.
>
>> +    };
Thanks,
Maulik

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

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

[Maulik Shah]

Hi,

On 3/19/2020 5:55 AM, Stephen Boyd wrote:
> Quoting Maulik Shah (2020-03-17 21:29:16)
>> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
>> new file mode 100644
>> index 0000000..0db7c3d
>> --- /dev/null
>> +++ b/drivers/soc/qcom/soc_sleep_stats.c
>> @@ -0,0 +1,244 @@
>> +// 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[] = {
> This can be const.
we are passing each element of this in debugfs_create_file as (void * data)
making this const is saying error passing const as void *.
>> +       { "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;
>> +};
>> +
>> +struct stats_prv_data {
>> +       const struct stats_config *config;
> Can we have 'bool has_appended_stats' here instead?
any reason to move? this is per compatible config where rpm based target have appended stats available.
>
>> +       void __iomem *reg;
>> +};
>> +
>> +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)
> Make stat const please.
Sure,  i will address this.
>
>> +{
>> +       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()
> This assumes that arch_timer_read_counter() is returning the physical
> counter? Is accumulated duration useful for anything? I don't like that
> we're relying on the arch timer code to return a certain counter value
> that may or may not be the same as what is written into smem.

we are not comparing it with what is written into smem. The idea here is to adjust the accumulated sleep length to reflect close/real value.

When a subsystem goes to sleep, it updates entry time and then stays in sleep.
Exit time and accumulated duration is updated when subsystem comes out of low power mode.

Now if Subsystem updated entry time and went in sleep, while printing accumulated duration
will keep giving older value.

If read it at interval of say every 10 seconds, if subsystem never comes out during this.
Even after 10 seconds, it gives older accumulated duration, while we want to get
printed as close to real value. so its updated here.

Now when it comes out of sleep, it always prints value given from subsystem.

>
>> +                              - 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);
> We already get this during probe in create_debugfs_entries() (which is
> too generic of a name by the way). 
I will update the name.
> Why can't we stash that pointer away
> so that it comes through the 'd' parameter to this function?
i think you are missing a subsystem restart case, in that pointer may be updated to new value.
so we can not just save it and re-use it every time, we don't know when subsystem restart happens and we now need new pointer.
>
>> +       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)
>> +{
>> +       struct stats_prv_data *prv_data = s->private;
>> +       void __iomem *reg = prv_data->reg;
>> +       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);
> Ok I see. The same stat info is in SMEM and also in some IO memory
> location? So we have to read one from an iomem region and one from smem?
Yes.
> Please make subsystem_sleep_stats_show() take a 'struct
> smem_sleep_stats' through 'd' that has the __le32 and __le64 markings
> and then do the byte swaps into a stack local struct sleep_stats that
> print_sleep_stats() can use.

Let me check this, i don't see every user of qcom_smem_get() doing byte swaps.

passing this through 'd' may not be required even if we want to do endian conversion.

'd' is alreaedy occupied to pass the subsystem_data structure item.

>
>> +
>> +       if (prv_data->config->appended_stats_avail) {
>> +               struct appended_stats app_stat;
> Just use a u32 for this. The struct is not useful here.
Okay. i will address this.
>
>> +
>> +               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 stats_prv_data *prv_data)
> I'd prefer this was just inlined into probe.
I would like to keep it same way.
>
>> +{
>> +       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 < prv_data[0].config->num_records; i++) {
> Pass config as another argument instead of attaching it to prv_data
> please.
Okay i will address this.
>
>> +               offset = STAT_TYPE_ADDR + (i * sizeof(struct sleep_stats));
>> +
>> +               if (prv_data[0].config->appended_stats_avail)
>> +                       offset += i * sizeof(struct appended_stats);
> I was imagining a macro like, SLEEP_STATn(i, has_appended) that did all the
> math to figure out the offset.
Okay i will update driver to use macro.
>
>> +
>> +               prv_data[i].reg = reg + offset;
>> +
>> +               type = readl(prv_data[i].reg);
>> +               memcpy(stat_type, &type, sizeof(u32));
> Is it a 4-byte ascii register that may or may not be NUL terminated? We
> should use __raw_readl() then so we don't do an endian swap. Using
> memcpy_fromio() does this all for you.

memcpy_fromio() seems not copying properly with u32 or u32+1 size.  seems it need align to 8 byte.

so when i pass u64 size it seems working fine. i will change this to use memcpy_fromio with sizeof(u64).

>
>> +               debugfs_create_file(stat_type, 0444, root,
> Maybe just 0400? Not sure why everyone in the world needs to read this.
Okay i will address this.
>
>> +                                   &prv_data[i],
>> +                                   &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;
>> +       const struct stats_config *config;
>> +       struct stats_prv_data *prv_data;
>> +       int i;
>> +
>> +       config = device_get_match_data(&pdev->dev);
>> +       if (!config)
>> +               return -ENODEV;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       if (!res)
>> +               return PTR_ERR(res);
>> +
>> +       offset_addr = ioremap(res->start + config->offset_addr, sizeof(u32));
>> +       if (IS_ERR(offset_addr))
>> +               return PTR_ERR(offset_addr);
>> +
>> +       stats_base = res->start | readl_relaxed(offset_addr);
> I still don't get it, but whatever. I highly doubt the lower bits are
> anything besides 0 so a logical OR vs addition is not different.
its different and need to do logical OR.
>
>> +       stats_size = resource_size(res);
>> +       iounmap(offset_addr);
>> +
>> +       reg = devm_ioremap(&pdev->dev, stats_base, stats_size);
>> +       if (!reg)
>> +               return -ENOMEM;
>> +
>> +       prv_data = devm_kzalloc(&pdev->dev, config->num_records *
>> +                               sizeof(struct stats_prv_data), GFP_KERNEL);
> We have devm_kcalloc() for array allocations.
Okay i will address this.
>
>> +       if (!prv_data)
>> +               return -ENOMEM;
>> +
>> +       for (i = 0; i < config->num_records; i++)
>> +               prv_data[i].config = config;
>> +
>> +       root = create_debugfs_entries(reg, prv_data);
>> +       platform_set_drvdata(pdev, root);
>> +
>> +       return 0;
>> +}

Thanks,

Maulik

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

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

[Stephen Boyd]

Quoting Maulik Shah (2020-03-19 08:14:03)
> On 3/19/2020 5:55 AM, Stephen Boyd wrote:
> > Quoting Maulik Shah (2020-03-17 21:29:16)
> >> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
> >> new file mode 100644
> >> index 0000000..0db7c3d
> >> --- /dev/null
> >> +++ b/drivers/soc/qcom/soc_sleep_stats.c
> >> @@ -0,0 +1,244 @@
[..]
> >> +
> >> +static struct subsystem_data subsystems[] = {
> > This can be const.
> we are passing each element of this in debugfs_create_file as (void * data)
> making this const is saying error passing const as void *.

I think we can cast it and have a comment. This way the structure can be
placed in the RO section of memory.

> >> +       { "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;
> >> +};
> >> +
> >> +struct stats_prv_data {
> >> +       const struct stats_config *config;
> > Can we have 'bool has_appended_stats' here instead?
> any reason to move? this is per compatible config where rpm based target have appended stats available.

It avoids one indirection to figure out something and makes the code
clearer. prv_data doesn't have any use for config otherwise.

> >
> >> +{
> >> +       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()
> > This assumes that arch_timer_read_counter() is returning the physical
> > counter? Is accumulated duration useful for anything? I don't like that
> > we're relying on the arch timer code to return a certain counter value
> > that may or may not be the same as what is written into smem.
> 
> we are not comparing it with what is written into smem. The idea here is to adjust the accumulated sleep length to reflect close/real value.
> 
> When a subsystem goes to sleep, it updates entry time and then stays in sleep.
> Exit time and accumulated duration is updated when subsystem comes out of low power mode.

Who is updating the accumulated time? This debugfs code? Or the
subsystems themselves?

> 
> Now if Subsystem updated entry time and went in sleep, while printing accumulated duration
> will keep giving older value.
> 
> If read it at interval of say every 10 seconds, if subsystem never comes out during this.
> Even after 10 seconds, it gives older accumulated duration, while we want to get
> printed as close to real value. so its updated here.
> 
> Now when it comes out of sleep, it always prints value given from subsystem.

Thanks, I understand the motivation.

> 
> >
> >> +                              - 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);
> > We already get this during probe in create_debugfs_entries() (which is
> > too generic of a name by the way). 
> I will update the name.
> > Why can't we stash that pointer away
> > so that it comes through the 'd' parameter to this function?
> i think you are missing a subsystem restart case, in that pointer may be updated to new value.
> so we can not just save it and re-use it every time, we don't know when subsystem restart happens and we now need new pointer.

Yes I'm missing subsystem restart. A comment would certainly help. If
this is the case, maybe this should be libified and each subsystem
driver should call some add/remove function in here so that we can
refetch the smem pointer. Doing the lookup each time isn't great.

> >
> >> +
> >> +               prv_data[i].reg = reg + offset;
> >> +
> >> +               type = readl(prv_data[i].reg);
> >> +               memcpy(stat_type, &type, sizeof(u32));
> > Is it a 4-byte ascii register that may or may not be NUL terminated? We
> > should use __raw_readl() then so we don't do an endian swap. Using
> > memcpy_fromio() does this all for you.
> 
> memcpy_fromio() seems not copying properly with u32 or u32+1 size.  seems it need align to 8 byte.
> 
> so when i pass u64 size it seems working fine. i will change this to use memcpy_fromio with sizeof(u64).

What is the failure? Do you need to read 4 bytes at a time? On arm64
memcpy_fromio() looks to do byte size reads if the size isn't 8 bytes.
Maybe just use __raw_readl() then if it is only 4 bytes and you need a
word accessor.

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

[Guenter Roeck]

On Wed, Mar 18, 2020 at 09:59:16AM +0530, Maulik Shah wrote:
> 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 | 244 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 254 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

	depends on QCOM_SMEM

Otherwise causes compile error if QCOM_SMEM=n.

Guenter

> +	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 0000000..0db7c3d
> --- /dev/null
> +++ b/drivers/soc/qcom/soc_sleep_stats.c
> @@ -0,0 +1,244 @@
> +// 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;
> +};
> +
> +struct stats_prv_data {
> +	const struct stats_config *config;
> +	void __iomem *reg;
> +};
> +
> +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)
> +{
> +	struct stats_prv_data *prv_data = s->private;
> +	void __iomem *reg = prv_data->reg;
> +	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 (prv_data->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 stats_prv_data *prv_data)
> +{
> +	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 < prv_data[0].config->num_records; i++) {
> +		offset = STAT_TYPE_ADDR + (i * sizeof(struct sleep_stats));
> +
> +		if (prv_data[0].config->appended_stats_avail)
> +			offset += i * sizeof(struct appended_stats);
> +
> +		prv_data[i].reg = reg + offset;
> +
> +		type = readl(prv_data[i].reg);
> +		memcpy(stat_type, &type, sizeof(u32));
> +		debugfs_create_file(stat_type, 0444, root,
> +				    &prv_data[i],
> +				    &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;
> +	const struct stats_config *config;
> +	struct stats_prv_data *prv_data;
> +	int i;
> +
> +	config = device_get_match_data(&pdev->dev);
> +	if (!config)
> +		return -ENODEV;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return PTR_ERR(res);
> +
> +	offset_addr = ioremap(res->start + config->offset_addr, sizeof(u32));
> +	if (IS_ERR(offset_addr))
> +		return PTR_ERR(offset_addr);
> +
> +	stats_base = res->start | readl_relaxed(offset_addr);
> +	stats_size = resource_size(res);
> +	iounmap(offset_addr);
> +
> +	reg = devm_ioremap(&pdev->dev, stats_base, stats_size);
> +	if (!reg)
> +		return -ENOMEM;
> +
> +	prv_data = devm_kzalloc(&pdev->dev, config->num_records *
> +				sizeof(struct stats_prv_data), GFP_KERNEL);
> +	if (!prv_data)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < config->num_records; i++)
> +		prv_data[i].config = config;
> +
> +	root = create_debugfs_entries(reg, prv_data);
> +	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");