Introduce SoC sleep stats driver

From: Mahesh Sivasubramanian <[email protected]>

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 [email protected]

Signed-off-by Mahesh Sivasubramanian <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

Signed-off-by Maulik Shah <[email protected]>

Reviewed-by Rob Herring <[email protected]>

Reviewed-by Bjorn Andersson <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

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

Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml new file +36 -0

new file mode 100644
index 0000000..5213daf
@@ -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 <[email protected]>
+
+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
+  - |

Quoted 65 lines · Maulik Shah

From: Mahesh Sivasubramanian <[email protected]>

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 [email protected]

Signed-off-by Mahesh Sivasubramanian <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

Signed-off-by Maulik Shah <[email protected]>

Reviewed-by Rob Herring <[email protected]>

Reviewed-by Bjorn Andersson <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

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

Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml new file +41 -0

new file mode 100644
index 0000000..5213daf
@@ -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 <[email protected]>
+
+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

Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml line 46

Quoted 5 lines

Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml new file +5 -0

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

Quoted 66 lines · Stephan Gerhold

Quoted 65 lines · Maulik Shah

From: Mahesh Sivasubramanian <[email protected]>

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 [email protected]

Signed-off-by Mahesh Sivasubramanian <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

Signed-off-by Maulik Shah <[email protected]>

Reviewed-by Rob Herring <[email protected]>

Reviewed-by Bjorn Andersson <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

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

Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml new file +41 -0

new file mode 100644
index 0000000..5213daf
@@ -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 <[email protected]>
+
+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

Quoted 8 lines

Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml line 46

Quoted 5 lines

Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml new file +5 -0

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

Quoted 9 lines

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.

Quoted 4 lines

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.

Quoted 112 lines · Maulik Shah

Quoted 66 lines · Stephan Gerhold

Quoted 65 lines · Maulik Shah

From: Mahesh Sivasubramanian <[email protected]>

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 [email protected]

Signed-off-by Mahesh Sivasubramanian <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

Signed-off-by Maulik Shah <[email protected]>

Reviewed-by Rob Herring <[email protected]>

Reviewed-by Bjorn Andersson <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

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

Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml new file +41 -0

new file mode 100644
index 0000000..5213daf
@@ -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 <[email protected]>
+
+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

Quoted 8 lines

Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml line 46

Quoted 5 lines

Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml new file +5 -0

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

Quoted 9 lines

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

Quoted 8 lines

         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!

Quoted 8 lines

Quoted 4 lines

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

Quoted 133 lines · Stephan Gerhold

Quoted 112 lines · Maulik Shah

Quoted 66 lines · Stephan Gerhold

Quoted 65 lines · Maulik Shah

From: Mahesh Sivasubramanian <[email protected]>

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 [email protected]

Signed-off-by Mahesh Sivasubramanian <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

Signed-off-by Maulik Shah <[email protected]>

Reviewed-by Rob Herring <[email protected]>

Reviewed-by Bjorn Andersson <[email protected]>

Reviewed-by Stephen Boyd <[email protected]>

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

Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml new file +41 -0

new file mode 100644
index 0000000..5213daf
@@ -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 <[email protected]>
+
+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

Quoted 8 lines

Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml line 46

Quoted 5 lines

Documentation/devicetree/bindings/soc/qcom/qcom-sleep-stats.yaml new file +5 -0

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

Quoted 9 lines

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

Quoted 8 lines

         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!

Quoted 8 lines

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 10 lines

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.

Quoted 8 lines

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.

Quoted 3 lines

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.

From: Mahesh Sivasubramanian <[email protected]>

Let's add a driver to read the stats from remote processor and export to debugfs.

The driver creates "qcom_sleep_stats" directory in debugfs and adds files for various low power mode available. Below is sample output with command

cat /sys/kernel/debug/qcom_sleep_stats/ddr
count = 0
Last Entered At = 0
Last Exited At = 0
Accumulated Duration = 0

Signed-off-by Mahesh Sivasubramanian <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

[mkshah: add subsystem sleep stats, create one file for each stat]

Signed-off-by Maulik Shah <[email protected]>

---
 drivers/soc/qcom/Kconfig            |  10 ++
 drivers/soc/qcom/Makefile           |   1 +
 drivers/soc/qcom/qcom_sleep_stats.c | 259 ++++++++++++++++++++++++++++++++++++
 3 files changed, 270 insertions(+)
 create mode 100644 drivers/soc/qcom/qcom_sleep_stats.c

[...]

drivers/soc/qcom/qcom_sleep_stats.c new file +14 -0

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

Hi Maulik,

Quoted 42 lines · Maulik Shah

From: Mahesh Sivasubramanian <[email protected]>

Let's add a driver to read the stats from remote processor and export to debugfs.

The driver creates "qcom_sleep_stats" directory in debugfs and adds files for various low power mode available. Below is sample output with command

cat /sys/kernel/debug/qcom_sleep_stats/ddr
count = 0
Last Entered At = 0
Last Exited At = 0
Accumulated Duration = 0

Signed-off-by Mahesh Sivasubramanian <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

[mkshah: add subsystem sleep stats, create one file for each stat]

Signed-off-by Maulik Shah <[email protected]>

---
  drivers/soc/qcom/Kconfig            |  10 ++
  drivers/soc/qcom/Makefile           |   1 +
  drivers/soc/qcom/qcom_sleep_stats.c | 259 ++++++++++++++++++++++++++++++++++++
  3 files changed, 270 insertions(+)

create mode 100644 drivers/soc/qcom/qcom_sleep_stats.c

[...]

drivers/soc/qcom/qcom_sleep_stats.c new file +14 -0

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

drivers/soc/qcom/qcom_sleep_stats.c

Quoted 10 lines

drivers/soc/qcom/qcom_sleep_stats.c new file +10 -0

+	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

Quoted 66 lines · Stephan Gerhold

Quoted 42 lines · Maulik Shah

From: Mahesh Sivasubramanian <[email protected]>

Let's add a driver to read the stats from remote processor and export to debugfs.

The driver creates "qcom_sleep_stats" directory in debugfs and adds files for various low power mode available. Below is sample output with command

cat /sys/kernel/debug/qcom_sleep_stats/ddr
count = 0
Last Entered At = 0
Last Exited At = 0
Accumulated Duration = 0

Signed-off-by Mahesh Sivasubramanian <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

[mkshah: add subsystem sleep stats, create one file for each stat]

Signed-off-by Maulik Shah <[email protected]>

---
  drivers/soc/qcom/Kconfig            |  10 ++
  drivers/soc/qcom/Makefile           |   1 +
  drivers/soc/qcom/qcom_sleep_stats.c | 259 ++++++++++++++++++++++++++++++++++++
  3 files changed, 270 insertions(+)

create mode 100644 drivers/soc/qcom/qcom_sleep_stats.c

[...]

drivers/soc/qcom/qcom_sleep_stats.c new file +14 -0

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

drivers/soc/qcom/qcom_sleep_stats.c

Quoted 10 lines

drivers/soc/qcom/qcom_sleep_stats.c new file +10 -0

+	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

Quoted 2 lines

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.

Quoted 3 lines

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

Quoted 3 lines

Two more (unrelated) issues here:

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 70 lines · Maulik Shah

Quoted 66 lines · Stephan Gerhold

Quoted 42 lines · Maulik Shah

From: Mahesh Sivasubramanian <[email protected]>

Let's add a driver to read the stats from remote processor and export to debugfs.

The driver creates "qcom_sleep_stats" directory in debugfs and adds files for various low power mode available. Below is sample output with command

cat /sys/kernel/debug/qcom_sleep_stats/ddr
count = 0
Last Entered At = 0
Last Exited At = 0
Accumulated Duration = 0

Signed-off-by Mahesh Sivasubramanian <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

[mkshah: add subsystem sleep stats, create one file for each stat]

Signed-off-by Maulik Shah <[email protected]>

---
   drivers/soc/qcom/Kconfig            |  10 ++
   drivers/soc/qcom/Makefile           |   1 +
   drivers/soc/qcom/qcom_sleep_stats.c | 259 ++++++++++++++++++++++++++++++++++++
   3 files changed, 270 insertions(+)

create mode 100644 drivers/soc/qcom/qcom_sleep_stats.c

[...]

drivers/soc/qcom/qcom_sleep_stats.c new file +14 -0

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

drivers/soc/qcom/qcom_sleep_stats.c

Quoted 10 lines

drivers/soc/qcom/qcom_sleep_stats.c new file +10 -0

+	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

Quoted 2 lines

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

Quoted 7 lines

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.

Quoted 14 lines

Quoted 3 lines

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

Quoted 3 lines

Two more (unrelated) issues here:

Quoted 8 lines

Quoted 3 lines

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.

Quoted 15 lines

Quoted 7 lines

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.

Quoted 4 lines

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