[PATCH v3 0/2] Introduce RPM Master stats

[PATCH v3 0/2] Introduce RPM Master stats

[Konrad Dybcio]

v2 -> v3:
- rename rpm-master-stats.yaml to qcom,rpm-master-stats.yaml

v2: https://lore.kernel.org/r/20230405-topic-master_stats-v2-0-51c304ecb610@linaro.org

v1 -> v2:
- Drop the `-` in /properties/compatible to make our entry be of the
  correct type [1/2]
- Change %s to %d for printing out the iterator [2/2]

v1: https://lore.kernel.org/r/20230405-topic-master_stats-v1-0-1b1fa2739953@linaro.org

The RPM MSG ram includes per-subsystem low-power mode entry/exit/
residence/etc. statistics which are very useful for trying to debug
what I'd call "SoC insomnia", or IOW the plaftorm refusing to drop
the voltage rails to a minimum and gate the non-critical clocks.

This series adds a very short and simple driver to query that data
and expose it through debugfs.

The base used for writing this driver is:
https://github.com/sonyxperiadev/kernel/blob/aosp/LA.UM.9.14.r1/drivers/soc/qcom/rpm_master_stat.c

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Konrad Dybcio (2):
      dt-bindings: soc: qcom: Add RPM Master stats
      soc: qcom: Introduce RPM master stats driver

 .../bindings/soc/qcom/qcom,rpm-master-stats.yaml   |  53 +++++++
 drivers/soc/qcom/Kconfig                           |  11 ++
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/rpm_master_stats.c                | 160 +++++++++++++++++++++
 4 files changed, 225 insertions(+)
---
base-commit: e3342532ecd39bbd9c2ab5b9001cec1589bc37e9
change-id: 20230405-topic-master_stats-ba201a9af93d

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@linaro.org>

[PATCH v3 1/2] dt-bindings: soc: qcom: Add RPM Master stats

[Konrad Dybcio]

The RPM MSG RAM contains per-RPM-master (e.g. APPS, ADSP etc.) sleep
statistics. They let one assess which core is actively preventing the
system from entering a true low-power mode.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 .../bindings/soc/qcom/qcom,rpm-master-stats.yaml   | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,rpm-master-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,rpm-master-stats.yaml
new file mode 100644
index 000000000000..d7e58cbd3344
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,rpm-master-stats.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/qcom/qcom,rpm-master-stats.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies, Inc. (QTI) RPM Master Stats
+
+maintainers:
+  - Konrad Dybcio <konrad.dybcio@linaro.org>
+
+description:
+  Per-RPM-Master (e.g. APSS, ADSP, etc.) sleep statistics.
+
+properties:
+  compatible:
+    const: qcom,rpm-master-stats
+
+  qcom,rpm-msg-ram:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: Phandle to an RPM MSG RAM slice containing the master stats
+    minItems: 1
+    maxItems: 5
+
+  qcom,master-names:
+    $ref: /schemas/types.yaml#/definitions/string-array
+    description: RPM Master name
+    minItems: 1
+    maxItems: 5
+
+required:
+  - compatible
+  - qcom,rpm-msg-ram
+  - qcom,master-names
+
+additionalProperties: false
+
+examples:
+  - |
+    stats {
+      compatible = "qcom,rpm-master-stats";
+      qcom,rpm-msg-ram = <&apss_master_stats>,
+                         <&mpss_master_stats>,
+                         <&adsp_master_stats>,
+                         <&cdsp_master_stats>,
+                         <&tz_master_stats>;
+      qcom,master-names = "APSS",
+                          "MPSS",
+                          "ADSP",
+                          "CDSP",
+                          "TZ";
+    };
+...

-- 
2.40.0

Re: [PATCH v3 1/2] dt-bindings: soc: qcom: Add RPM Master stats

[Krzysztof Kozlowski]

On 14/04/2023 13:37, Konrad Dybcio wrote:
> The RPM MSG RAM contains per-RPM-master (e.g. APPS, ADSP etc.) sleep
> statistics. They let one assess which core is actively preventing the
> system from entering a true low-power mode.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  .../bindings/soc/qcom/qcom,rpm-master-stats.yaml   | 53 ++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,rpm-master-stats.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,rpm-master-stats.yaml
> new file mode 100644
> index 000000000000..d7e58cbd3344
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,rpm-master-stats.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/qcom/qcom,rpm-master-stats.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies, Inc. (QTI) RPM Master Stats
> +
> +maintainers:
> +  - Konrad Dybcio <konrad.dybcio@linaro.org>
> +
> +description:
> +  Per-RPM-Master (e.g. APSS, ADSP, etc.) sleep statistics.

Explain what is RPM-Master and what do you mean by "sleep".

> +
> +properties:
> +  compatible:
> +    const: qcom,rpm-master-stats
> +
> +  qcom,rpm-msg-ram:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: Phandle to an RPM MSG RAM slice containing the master stats
> +    minItems: 1
> +    maxItems: 5
> +
> +  qcom,master-names:
> +    $ref: /schemas/types.yaml#/definitions/string-array
> +    description: RPM Master name

There is a relation between this and qcom,rpm-msg-ram which you do not
describe. It's not just RPM master name...

Best regards,
Krzysztof

[PATCH v3 2/2] soc: qcom: Introduce RPM master stats driver

[Konrad Dybcio]

Introduce a driver to query and expose detailed, per-subsystem (as opposed
to the existing qcom_stats driver which exposes SoC-wide data) about low
power mode states of a given RPM master. That includes the APSS (ARM),
MPSS (modem) and other remote cores, depending on the platform
configuration.

This is a vastly cleaned up and restructured version of a similar
driver found in msm-5.4.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/soc/qcom/Kconfig            |  11 +++
 drivers/soc/qcom/Makefile           |   1 +
 drivers/soc/qcom/rpm_master_stats.c | 160 ++++++++++++++++++++++++++++++++++++
 3 files changed, 172 insertions(+)

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index a491718f8064..e597799e8121 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -135,6 +135,17 @@ config QCOM_RMTFS_MEM
 
 	  Say y here if you intend to boot the modem remoteproc.
 
+config QCOM_RPM_MASTER_STATS
+	tristate "Qualcomm RPM Master stats"
+	depends on ARCH_QCOM || COMPILE_TEST
+	help
+	  The RPM Master sleep stats driver provides detailed per-subsystem
+	  sleep/wake data, read from the RPM message RAM. It can be used to
+	  assess whether all the low-power modes available are entered as
+	  expected or to check which part of the SoC prevents it from sleeping.
+
+	  Say y here if you intend to debug or monitor platform sleep.
+
 config QCOM_RPMH
 	tristate "Qualcomm RPM-Hardened (RPMH) Communication"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 0f43a88b4894..7349371fdea1 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_QCOM_QMI_HELPERS)	+= qmi_helpers.o
 qmi_helpers-y	+= qmi_encdec.o qmi_interface.o
 obj-$(CONFIG_QCOM_RAMP_CTRL)	+= ramp_controller.o
 obj-$(CONFIG_QCOM_RMTFS_MEM)	+= rmtfs_mem.o
+obj-$(CONFIG_QCOM_RPM_MASTER_STATS)	+= rpm_master_stats.o
 obj-$(CONFIG_QCOM_RPMH)		+= qcom_rpmh.o
 qcom_rpmh-y			+= rpmh-rsc.o
 qcom_rpmh-y			+= rpmh.o
diff --git a/drivers/soc/qcom/rpm_master_stats.c b/drivers/soc/qcom/rpm_master_stats.c
new file mode 100644
index 000000000000..73080c92bf89
--- /dev/null
+++ b/drivers/soc/qcom/rpm_master_stats.c
@@ -0,0 +1,160 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2012-2020, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023, Linaro Limited
+ *
+ * This driver supports what is known as "Master Stats v2", which seems to be
+ * the only version which has ever shipped, all the way from 2013 to 2023.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+struct master_stats_data {
+	void __iomem *base;
+	const char *label;
+};
+
+struct rpm_master_stats {
+	uint32_t active_cores;
+	uint32_t num_shutdowns;
+	uint64_t shutdown_req;
+	uint64_t wakeup_idx;
+	uint64_t bringup_req;
+	uint64_t bringup_ack;
+	uint32_t wakeup_reason; /* 0 = "rude wakeup", 1 = scheduled wakeup */
+	uint32_t last_sleep_trans_dur;
+	uint32_t last_wake_trans_dur;
+
+	/* Per-subsystem (*not necessarily* SoC-wide) XO shutdown stats */
+	uint32_t xo_count;
+	uint64_t xo_last_enter;
+	uint64_t last_exit;
+	uint64_t xo_total_dur;
+};
+
+static int master_stats_show(struct seq_file *s, void *unused)
+{
+	struct master_stats_data *d = s->private;
+	struct rpm_master_stats stat;
+
+	memcpy_fromio(&stat, d->base, sizeof(stat));
+
+	seq_printf(s, "%s:\n", d->label);
+
+	seq_printf(s, "\tLast shutdown @ %llu\n", stat.shutdown_req);
+	seq_printf(s, "\tLast bringup req @ %llu\n", stat.bringup_req);
+	seq_printf(s, "\tLast bringup ack @ %llu\n", stat.bringup_ack);
+	seq_printf(s, "\tLast wakeup idx: %llu\n", stat.wakeup_idx);
+	seq_printf(s, "\tLast XO shutdown enter @ %llu\n", stat.xo_last_enter);
+	seq_printf(s, "\tLast XO shutdown exit @ %llu\n", stat.last_exit);
+	seq_printf(s, "\tXO total duration: %llu\n", stat.xo_total_dur);
+	seq_printf(s, "\tLast sleep transition duration: %u\n", stat.last_sleep_trans_dur);
+	seq_printf(s, "\tLast wake transition duration: %u\n", stat.last_wake_trans_dur);
+	seq_printf(s, "\tXO shutdown count: %u\n", stat.xo_count);
+	seq_printf(s, "\tWakeup reason: 0x%x\n", stat.wakeup_reason);
+	seq_printf(s, "\tShutdown count: %u\n", stat.num_shutdowns);
+	seq_printf(s, "\tActive cores bitmask: 0x%x\n", stat.active_cores);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(master_stats);
+
+static int master_stats_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *msgram_np;
+	struct master_stats_data *d;
+	struct dentry *dent, *root;
+	struct resource res;
+	int count, i, ret;
+
+	count = of_property_count_strings(dev->of_node, "qcom,master-names");
+	if (count < 0)
+		return count;
+
+	d = devm_kzalloc(dev, count * sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return -ENOMEM;
+
+	root = debugfs_create_dir("rpm_master_stats", NULL);
+	platform_set_drvdata(pdev, root);
+
+	for (i = 0; i < count; i++) {
+		msgram_np = of_parse_phandle(dev->of_node, "qcom,rpm-msg-ram", i);
+		if (!msgram_np) {
+			debugfs_remove_recursive(root);
+			return dev_err_probe(dev, -EINVAL,
+					     "Couldn't parse MSG RAM phandle idx %d", i);
+		}
+
+		/*
+		 * Purposefully skip devm_platform helpers as we're using a
+		 * shared resource.
+		 */
+		ret = of_address_to_resource(msgram_np, 0, &res);
+		if (ret < 0) {
+			debugfs_remove_recursive(root);
+			return ret;
+		}
+
+		d[i].base = devm_ioremap(dev, res.start, resource_size(&res));
+		if (IS_ERR(d[i].base)) {
+			debugfs_remove_recursive(root);
+			return dev_err_probe(dev, -EINVAL,
+					     "Could not map the MSG RAM slice idx %d!\n", i);
+		}
+
+		ret = of_property_read_string_index(dev->of_node, "qcom,master-names", i,
+						    &d[i].label);
+		if (ret < 0) {
+			debugfs_remove_recursive(root);
+			return dev_err_probe(dev, ret,
+					     "Could not read name idx %d!\n", i);
+		}
+
+		/*
+		 * Generally it's not advised to fail on debugfs errors, but this
+		 * driver's only job is exposing data therein.
+		 */
+		dent = debugfs_create_file(d[i].label, 0444, root,
+					   &d[i], &master_stats_fops);
+		if (!dent) {
+			debugfs_remove_recursive(root);
+			return -EINVAL;
+		}
+	}
+
+	device_set_pm_not_required(dev);
+
+	return 0;
+}
+
+static void master_stats_remove(struct platform_device *pdev)
+{
+	struct dentry *root = platform_get_drvdata(pdev);
+
+	debugfs_remove_recursive(root);
+}
+
+static const struct of_device_id rpm_master_table[] = {
+	{ .compatible = "qcom,rpm-master-stats" },
+	{ },
+};
+
+static struct platform_driver master_stats_driver = {
+	.probe = master_stats_probe,
+	.remove_new = master_stats_remove,
+	.driver = {
+		.name = "rpm_master_stats",
+		.of_match_table = rpm_master_table,
+	},
+};
+module_platform_driver(master_stats_driver);
+
+MODULE_DESCRIPTION("RPM Master Statistics driver");
+MODULE_LICENSE("GPL");

-- 
2.40.0

Re: [PATCH v3 2/2] soc: qcom: Introduce RPM master stats driver

[Manivannan Sadhasivam]

On Fri, Apr 14, 2023 at 01:37:18PM +0200, Konrad Dybcio wrote:
> Introduce a driver to query and expose detailed, per-subsystem (as opposed
> to the existing qcom_stats driver which exposes SoC-wide data) about low
> power mode states of a given RPM master. That includes the APSS (ARM),
> MPSS (modem) and other remote cores, depending on the platform
> configuration.
> 
> This is a vastly cleaned up and restructured version of a similar
> driver found in msm-5.4.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/soc/qcom/Kconfig            |  11 +++
>  drivers/soc/qcom/Makefile           |   1 +
>  drivers/soc/qcom/rpm_master_stats.c | 160 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 172 insertions(+)
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index a491718f8064..e597799e8121 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -135,6 +135,17 @@ config QCOM_RMTFS_MEM
>  
>  	  Say y here if you intend to boot the modem remoteproc.
>  
> +config QCOM_RPM_MASTER_STATS
> +	tristate "Qualcomm RPM Master stats"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	help
> +	  The RPM Master sleep stats driver provides detailed per-subsystem
> +	  sleep/wake data, read from the RPM message RAM. It can be used to
> +	  assess whether all the low-power modes available are entered as
> +	  expected or to check which part of the SoC prevents it from sleeping.
> +
> +	  Say y here if you intend to debug or monitor platform sleep.
> +
>  config QCOM_RPMH
>  	tristate "Qualcomm RPM-Hardened (RPMH) Communication"
>  	depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 0f43a88b4894..7349371fdea1 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_QCOM_QMI_HELPERS)	+= qmi_helpers.o
>  qmi_helpers-y	+= qmi_encdec.o qmi_interface.o
>  obj-$(CONFIG_QCOM_RAMP_CTRL)	+= ramp_controller.o
>  obj-$(CONFIG_QCOM_RMTFS_MEM)	+= rmtfs_mem.o
> +obj-$(CONFIG_QCOM_RPM_MASTER_STATS)	+= rpm_master_stats.o
>  obj-$(CONFIG_QCOM_RPMH)		+= qcom_rpmh.o
>  qcom_rpmh-y			+= rpmh-rsc.o
>  qcom_rpmh-y			+= rpmh.o
> diff --git a/drivers/soc/qcom/rpm_master_stats.c b/drivers/soc/qcom/rpm_master_stats.c
> new file mode 100644
> index 000000000000..73080c92bf89
> --- /dev/null
> +++ b/drivers/soc/qcom/rpm_master_stats.c
> @@ -0,0 +1,160 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2012-2020, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023, Linaro Limited
> + *
> + * This driver supports what is known as "Master Stats v2", which seems to be
> + * the only version which has ever shipped, all the way from 2013 to 2023.

It'd better to mention "Qualcomm downstream" in the somewhere above comment to
make it clear for users.

> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +
> +struct master_stats_data {
> +	void __iomem *base;
> +	const char *label;
> +};
> +
> +struct rpm_master_stats {
> +	uint32_t active_cores;
> +	uint32_t num_shutdowns;
> +	uint64_t shutdown_req;
> +	uint64_t wakeup_idx;
> +	uint64_t bringup_req;
> +	uint64_t bringup_ack;
> +	uint32_t wakeup_reason; /* 0 = "rude wakeup", 1 = scheduled wakeup */
> +	uint32_t last_sleep_trans_dur;
> +	uint32_t last_wake_trans_dur;
> +
> +	/* Per-subsystem (*not necessarily* SoC-wide) XO shutdown stats */
> +	uint32_t xo_count;
> +	uint64_t xo_last_enter;
> +	uint64_t last_exit;
> +	uint64_t xo_total_dur;

Why can't you use u64, u32?

Also, sort these members as below:

u64
u32

> +};
> +
> +static int master_stats_show(struct seq_file *s, void *unused)
> +{
> +	struct master_stats_data *d = s->private;
> +	struct rpm_master_stats stat;
> +
> +	memcpy_fromio(&stat, d->base, sizeof(stat));
> +
> +	seq_printf(s, "%s:\n", d->label);
> +
> +	seq_printf(s, "\tLast shutdown @ %llu\n", stat.shutdown_req);
> +	seq_printf(s, "\tLast bringup req @ %llu\n", stat.bringup_req);
> +	seq_printf(s, "\tLast bringup ack @ %llu\n", stat.bringup_ack);
> +	seq_printf(s, "\tLast wakeup idx: %llu\n", stat.wakeup_idx);
> +	seq_printf(s, "\tLast XO shutdown enter @ %llu\n", stat.xo_last_enter);
> +	seq_printf(s, "\tLast XO shutdown exit @ %llu\n", stat.last_exit);
> +	seq_printf(s, "\tXO total duration: %llu\n", stat.xo_total_dur);
> +	seq_printf(s, "\tLast sleep transition duration: %u\n", stat.last_sleep_trans_dur);
> +	seq_printf(s, "\tLast wake transition duration: %u\n", stat.last_wake_trans_dur);
> +	seq_printf(s, "\tXO shutdown count: %u\n", stat.xo_count);
> +	seq_printf(s, "\tWakeup reason: 0x%x\n", stat.wakeup_reason);
> +	seq_printf(s, "\tShutdown count: %u\n", stat.num_shutdowns);
> +	seq_printf(s, "\tActive cores bitmask: 0x%x\n", stat.active_cores);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(master_stats);
> +
> +static int master_stats_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *msgram_np;
> +	struct master_stats_data *d;

"d" is not a good choice for a local variable (might be OK above but definitely
not here)

> +	struct dentry *dent, *root;
> +	struct resource res;
> +	int count, i, ret;
> +
> +	count = of_property_count_strings(dev->of_node, "qcom,master-names");
> +	if (count < 0)
> +		return count;
> +
> +	d = devm_kzalloc(dev, count * sizeof(*d), GFP_KERNEL);
> +	if (!d)
> +		return -ENOMEM;
> +
> +	root = debugfs_create_dir("rpm_master_stats", NULL);

It's better to use "qcom_rpm_master_stats".

> +	platform_set_drvdata(pdev, root);
> +
> +	for (i = 0; i < count; i++) {
> +		msgram_np = of_parse_phandle(dev->of_node, "qcom,rpm-msg-ram", i);
> +		if (!msgram_np) {
> +			debugfs_remove_recursive(root);
> +			return dev_err_probe(dev, -EINVAL,

-ENODEV

> +					     "Couldn't parse MSG RAM phandle idx %d", i);
> +		}
> +
> +		/*
> +		 * Purposefully skip devm_platform helpers as we're using a
> +		 * shared resource.
> +		 */
> +		ret = of_address_to_resource(msgram_np, 0, &res);

Missing of_node_put() here due to of_parse_phandle().

> +		if (ret < 0) {
> +			debugfs_remove_recursive(root);
> +			return ret;
> +		}
> +
> +		d[i].base = devm_ioremap(dev, res.start, resource_size(&res));
> +		if (IS_ERR(d[i].base)) {
> +			debugfs_remove_recursive(root);
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Could not map the MSG RAM slice idx %d!\n", i);
> +		}
> +
> +		ret = of_property_read_string_index(dev->of_node, "qcom,master-names", i,
> +						    &d[i].label);
> +		if (ret < 0) {
> +			debugfs_remove_recursive(root);
> +			return dev_err_probe(dev, ret,
> +					     "Could not read name idx %d!\n", i);
> +		}
> +
> +		/*
> +		 * Generally it's not advised to fail on debugfs errors, but this
> +		 * driver's only job is exposing data therein.
> +		 */
> +		dent = debugfs_create_file(d[i].label, 0444, root,
> +					   &d[i], &master_stats_fops);
> +		if (!dent) {

Don't check for NULL, instead use IS_ERR() if you really care about error
checking here.

> +			debugfs_remove_recursive(root);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	device_set_pm_not_required(dev);
> +
> +	return 0;
> +}
> +
> +static void master_stats_remove(struct platform_device *pdev)
> +{
> +	struct dentry *root = platform_get_drvdata(pdev);
> +
> +	debugfs_remove_recursive(root);
> +}
> +
> +static const struct of_device_id rpm_master_table[] = {
> +	{ .compatible = "qcom,rpm-master-stats" },
> +	{ },
> +};
> +
> +static struct platform_driver master_stats_driver = {
> +	.probe = master_stats_probe,
> +	.remove_new = master_stats_remove,
> +	.driver = {
> +		.name = "rpm_master_stats",
> +		.of_match_table = rpm_master_table,
> +	},
> +};
> +module_platform_driver(master_stats_driver);
> +
> +MODULE_DESCRIPTION("RPM Master Statistics driver");

Qualcomm RPM Master Statistics driver

- Mani

> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.40.0
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 2/2] soc: qcom: Introduce RPM master stats driver

[Konrad Dybcio]

On 16.04.2023 16:20, Manivannan Sadhasivam wrote:
> On Fri, Apr 14, 2023 at 01:37:18PM +0200, Konrad Dybcio wrote:
>> Introduce a driver to query and expose detailed, per-subsystem (as opposed
>> to the existing qcom_stats driver which exposes SoC-wide data) about low
>> power mode states of a given RPM master. That includes the APSS (ARM),
>> MPSS (modem) and other remote cores, depending on the platform
>> configuration.
>>
>> This is a vastly cleaned up and restructured version of a similar
>> driver found in msm-5.4.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  drivers/soc/qcom/Kconfig            |  11 +++
>>  drivers/soc/qcom/Makefile           |   1 +
>>  drivers/soc/qcom/rpm_master_stats.c | 160 ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 172 insertions(+)
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index a491718f8064..e597799e8121 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -135,6 +135,17 @@ config QCOM_RMTFS_MEM
>>  
>>  	  Say y here if you intend to boot the modem remoteproc.
>>  
>> +config QCOM_RPM_MASTER_STATS
>> +	tristate "Qualcomm RPM Master stats"
>> +	depends on ARCH_QCOM || COMPILE_TEST
>> +	help
>> +	  The RPM Master sleep stats driver provides detailed per-subsystem
>> +	  sleep/wake data, read from the RPM message RAM. It can be used to
>> +	  assess whether all the low-power modes available are entered as
>> +	  expected or to check which part of the SoC prevents it from sleeping.
>> +
>> +	  Say y here if you intend to debug or monitor platform sleep.
>> +
>>  config QCOM_RPMH
>>  	tristate "Qualcomm RPM-Hardened (RPMH) Communication"
>>  	depends on ARCH_QCOM || COMPILE_TEST
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 0f43a88b4894..7349371fdea1 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_QCOM_QMI_HELPERS)	+= qmi_helpers.o
>>  qmi_helpers-y	+= qmi_encdec.o qmi_interface.o
>>  obj-$(CONFIG_QCOM_RAMP_CTRL)	+= ramp_controller.o
>>  obj-$(CONFIG_QCOM_RMTFS_MEM)	+= rmtfs_mem.o
>> +obj-$(CONFIG_QCOM_RPM_MASTER_STATS)	+= rpm_master_stats.o
>>  obj-$(CONFIG_QCOM_RPMH)		+= qcom_rpmh.o
>>  qcom_rpmh-y			+= rpmh-rsc.o
>>  qcom_rpmh-y			+= rpmh.o
>> diff --git a/drivers/soc/qcom/rpm_master_stats.c b/drivers/soc/qcom/rpm_master_stats.c
>> new file mode 100644
>> index 000000000000..73080c92bf89
>> --- /dev/null
>> +++ b/drivers/soc/qcom/rpm_master_stats.c
>> @@ -0,0 +1,160 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2012-2020, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2023, Linaro Limited
>> + *
>> + * This driver supports what is known as "Master Stats v2", which seems to be
>> + * the only version which has ever shipped, all the way from 2013 to 2023.
> 
> It'd better to mention "Qualcomm downstream" in the somewhere above comment to
> make it clear for users.
Ack

> 
>> + */
>> +
>> +#include <linux/debugfs.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +
>> +struct master_stats_data {
>> +	void __iomem *base;
>> +	const char *label;
>> +};
>> +
>> +struct rpm_master_stats {
>> +	uint32_t active_cores;
>> +	uint32_t num_shutdowns;
>> +	uint64_t shutdown_req;
>> +	uint64_t wakeup_idx;
>> +	uint64_t bringup_req;
>> +	uint64_t bringup_ack;
>> +	uint32_t wakeup_reason; /* 0 = "rude wakeup", 1 = scheduled wakeup */
>> +	uint32_t last_sleep_trans_dur;
>> +	uint32_t last_wake_trans_dur;
>> +
>> +	/* Per-subsystem (*not necessarily* SoC-wide) XO shutdown stats */
>> +	uint32_t xo_count;
>> +	uint64_t xo_last_enter;
>> +	uint64_t last_exit;
>> +	uint64_t xo_total_dur;
> 
> Why can't you use u64, u32?
Brain derp!

> 
> Also, sort these members as below:
> 
> u64
> u32
No, it's the way this data is structured in the
memory and we copy it as a whole.

> 
>> +};
>> +
>> +static int master_stats_show(struct seq_file *s, void *unused)
>> +{
>> +	struct master_stats_data *d = s->private;
>> +	struct rpm_master_stats stat;
>> +
>> +	memcpy_fromio(&stat, d->base, sizeof(stat));
>> +
>> +	seq_printf(s, "%s:\n", d->label);
>> +
>> +	seq_printf(s, "\tLast shutdown @ %llu\n", stat.shutdown_req);
>> +	seq_printf(s, "\tLast bringup req @ %llu\n", stat.bringup_req);
>> +	seq_printf(s, "\tLast bringup ack @ %llu\n", stat.bringup_ack);
>> +	seq_printf(s, "\tLast wakeup idx: %llu\n", stat.wakeup_idx);
>> +	seq_printf(s, "\tLast XO shutdown enter @ %llu\n", stat.xo_last_enter);
>> +	seq_printf(s, "\tLast XO shutdown exit @ %llu\n", stat.last_exit);
>> +	seq_printf(s, "\tXO total duration: %llu\n", stat.xo_total_dur);
>> +	seq_printf(s, "\tLast sleep transition duration: %u\n", stat.last_sleep_trans_dur);
>> +	seq_printf(s, "\tLast wake transition duration: %u\n", stat.last_wake_trans_dur);
>> +	seq_printf(s, "\tXO shutdown count: %u\n", stat.xo_count);
>> +	seq_printf(s, "\tWakeup reason: 0x%x\n", stat.wakeup_reason);
>> +	seq_printf(s, "\tShutdown count: %u\n", stat.num_shutdowns);
>> +	seq_printf(s, "\tActive cores bitmask: 0x%x\n", stat.active_cores);
>> +
>> +	return 0;
>> +}
>> +DEFINE_SHOW_ATTRIBUTE(master_stats);
>> +
>> +static int master_stats_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *msgram_np;
>> +	struct master_stats_data *d;
> 
> "d" is not a good choice for a local variable (might be OK above but definitely
> not here)
Sure, I can change it.

> 
>> +	struct dentry *dent, *root;
>> +	struct resource res;
>> +	int count, i, ret;
>> +
>> +	count = of_property_count_strings(dev->of_node, "qcom,master-names");
>> +	if (count < 0)
>> +		return count;
>> +
>> +	d = devm_kzalloc(dev, count * sizeof(*d), GFP_KERNEL);
>> +	if (!d)
>> +		return -ENOMEM;
>> +
>> +	root = debugfs_create_dir("rpm_master_stats", NULL);
> 
> It's better to use "qcom_rpm_master_stats".
Ack

> 
>> +	platform_set_drvdata(pdev, root);
>> +
>> +	for (i = 0; i < count; i++) {
>> +		msgram_np = of_parse_phandle(dev->of_node, "qcom,rpm-msg-ram", i);
>> +		if (!msgram_np) {
>> +			debugfs_remove_recursive(root);
>> +			return dev_err_probe(dev, -EINVAL,
> 
> -ENODEV
Ack

> 
>> +					     "Couldn't parse MSG RAM phandle idx %d", i);
>> +		}
>> +
>> +		/*
>> +		 * Purposefully skip devm_platform helpers as we're using a
>> +		 * shared resource.
>> +		 */
>> +		ret = of_address_to_resource(msgram_np, 0, &res);
> 
> Missing of_node_put() here due to of_parse_phandle().
Ack

> 
>> +		if (ret < 0) {
>> +			debugfs_remove_recursive(root);
>> +			return ret;
>> +		}
>> +
>> +		d[i].base = devm_ioremap(dev, res.start, resource_size(&res));
>> +		if (IS_ERR(d[i].base)) {
>> +			debugfs_remove_recursive(root);
>> +			return dev_err_probe(dev, -EINVAL,
>> +					     "Could not map the MSG RAM slice idx %d!\n", i);
>> +		}
>> +
>> +		ret = of_property_read_string_index(dev->of_node, "qcom,master-names", i,
>> +						    &d[i].label);
>> +		if (ret < 0) {
>> +			debugfs_remove_recursive(root);
>> +			return dev_err_probe(dev, ret,
>> +					     "Could not read name idx %d!\n", i);
>> +		}
>> +
>> +		/*
>> +		 * Generally it's not advised to fail on debugfs errors, but this
>> +		 * driver's only job is exposing data therein.
>> +		 */
>> +		dent = debugfs_create_file(d[i].label, 0444, root,
>> +					   &d[i], &master_stats_fops);
>> +		if (!dent) {
> 
> Don't check for NULL, instead use IS_ERR() if you really care about error
> checking here.
"This function will return a pointer to a dentry if
it succeeds. This pointer must be passed to the
debugfs_remove function when the file is to be removed
(no automatic cleanup happens if your module is unloaded,
you are responsible here.) If an error occurs, NULL will
be returned."

> 
>> +			debugfs_remove_recursive(root);
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	device_set_pm_not_required(dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static void master_stats_remove(struct platform_device *pdev)
>> +{
>> +	struct dentry *root = platform_get_drvdata(pdev);
>> +
>> +	debugfs_remove_recursive(root);
>> +}
>> +
>> +static const struct of_device_id rpm_master_table[] = {
>> +	{ .compatible = "qcom,rpm-master-stats" },
>> +	{ },
>> +};
>> +
>> +static struct platform_driver master_stats_driver = {
>> +	.probe = master_stats_probe,
>> +	.remove_new = master_stats_remove,
>> +	.driver = {
>> +		.name = "rpm_master_stats",
>> +		.of_match_table = rpm_master_table,
>> +	},
>> +};
>> +module_platform_driver(master_stats_driver);
>> +
>> +MODULE_DESCRIPTION("RPM Master Statistics driver");
> 
> Qualcomm RPM Master Statistics driver
Ack

Konrad
> 
> - Mani
> 
>> +MODULE_LICENSE("GPL");
>>
>> -- 
>> 2.40.0
>>
> 

Re: [PATCH v3 2/2] soc: qcom: Introduce RPM master stats driver

[Manivannan Sadhasivam]

On Mon, Apr 17, 2023 at 09:14:39AM +0200, Konrad Dybcio wrote:
> 
> 
> On 16.04.2023 16:20, Manivannan Sadhasivam wrote:
> > On Fri, Apr 14, 2023 at 01:37:18PM +0200, Konrad Dybcio wrote:
> >> Introduce a driver to query and expose detailed, per-subsystem (as opposed
> >> to the existing qcom_stats driver which exposes SoC-wide data) about low
> >> power mode states of a given RPM master. That includes the APSS (ARM),
> >> MPSS (modem) and other remote cores, depending on the platform
> >> configuration.
> >>
> >> This is a vastly cleaned up and restructured version of a similar
> >> driver found in msm-5.4.
> >>
> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >> ---
> >>  drivers/soc/qcom/Kconfig            |  11 +++
> >>  drivers/soc/qcom/Makefile           |   1 +
> >>  drivers/soc/qcom/rpm_master_stats.c | 160 ++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 172 insertions(+)
> >>
> >> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> >> index a491718f8064..e597799e8121 100644
> >> --- a/drivers/soc/qcom/Kconfig
> >> +++ b/drivers/soc/qcom/Kconfig
> >> @@ -135,6 +135,17 @@ config QCOM_RMTFS_MEM
> >>  
> >>  	  Say y here if you intend to boot the modem remoteproc.
> >>  
> >> +config QCOM_RPM_MASTER_STATS
> >> +	tristate "Qualcomm RPM Master stats"
> >> +	depends on ARCH_QCOM || COMPILE_TEST
> >> +	help
> >> +	  The RPM Master sleep stats driver provides detailed per-subsystem
> >> +	  sleep/wake data, read from the RPM message RAM. It can be used to
> >> +	  assess whether all the low-power modes available are entered as
> >> +	  expected or to check which part of the SoC prevents it from sleeping.
> >> +
> >> +	  Say y here if you intend to debug or monitor platform sleep.
> >> +
> >>  config QCOM_RPMH
> >>  	tristate "Qualcomm RPM-Hardened (RPMH) Communication"
> >>  	depends on ARCH_QCOM || COMPILE_TEST
> >> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> >> index 0f43a88b4894..7349371fdea1 100644
> >> --- a/drivers/soc/qcom/Makefile
> >> +++ b/drivers/soc/qcom/Makefile
> >> @@ -14,6 +14,7 @@ obj-$(CONFIG_QCOM_QMI_HELPERS)	+= qmi_helpers.o
> >>  qmi_helpers-y	+= qmi_encdec.o qmi_interface.o
> >>  obj-$(CONFIG_QCOM_RAMP_CTRL)	+= ramp_controller.o
> >>  obj-$(CONFIG_QCOM_RMTFS_MEM)	+= rmtfs_mem.o
> >> +obj-$(CONFIG_QCOM_RPM_MASTER_STATS)	+= rpm_master_stats.o
> >>  obj-$(CONFIG_QCOM_RPMH)		+= qcom_rpmh.o
> >>  qcom_rpmh-y			+= rpmh-rsc.o
> >>  qcom_rpmh-y			+= rpmh.o
> >> diff --git a/drivers/soc/qcom/rpm_master_stats.c b/drivers/soc/qcom/rpm_master_stats.c
> >> new file mode 100644
> >> index 000000000000..73080c92bf89
> >> --- /dev/null
> >> +++ b/drivers/soc/qcom/rpm_master_stats.c
> >> @@ -0,0 +1,160 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Copyright (c) 2012-2020, The Linux Foundation. All rights reserved.
> >> + * Copyright (c) 2023, Linaro Limited
> >> + *
> >> + * This driver supports what is known as "Master Stats v2", which seems to be
> >> + * the only version which has ever shipped, all the way from 2013 to 2023.
> > 
> > It'd better to mention "Qualcomm downstream" in the somewhere above comment to
> > make it clear for users.
> Ack
> 
> > 
> >> + */
> >> +
> >> +#include <linux/debugfs.h>
> >> +#include <linux/io.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_address.h>
> >> +#include <linux/platform_device.h>
> >> +
> >> +struct master_stats_data {
> >> +	void __iomem *base;
> >> +	const char *label;
> >> +};
> >> +
> >> +struct rpm_master_stats {
> >> +	uint32_t active_cores;
> >> +	uint32_t num_shutdowns;
> >> +	uint64_t shutdown_req;
> >> +	uint64_t wakeup_idx;
> >> +	uint64_t bringup_req;
> >> +	uint64_t bringup_ack;
> >> +	uint32_t wakeup_reason; /* 0 = "rude wakeup", 1 = scheduled wakeup */
> >> +	uint32_t last_sleep_trans_dur;
> >> +	uint32_t last_wake_trans_dur;
> >> +
> >> +	/* Per-subsystem (*not necessarily* SoC-wide) XO shutdown stats */
> >> +	uint32_t xo_count;
> >> +	uint64_t xo_last_enter;
> >> +	uint64_t last_exit;
> >> +	uint64_t xo_total_dur;
> > 
> > Why can't you use u64, u32?
> Brain derp!
> 
> > 
> > Also, sort these members as below:
> > 
> > u64
> > u32
> No, it's the way this data is structured in the
> memory and we copy it as a whole.
> 

Ok, in that case you'd need __packed.

> > 
> >> +};
> >> +

[...]

> >> +		/*
> >> +		 * Generally it's not advised to fail on debugfs errors, but this
> >> +		 * driver's only job is exposing data therein.
> >> +		 */
> >> +		dent = debugfs_create_file(d[i].label, 0444, root,
> >> +					   &d[i], &master_stats_fops);
> >> +		if (!dent) {
> > 
> > Don't check for NULL, instead use IS_ERR() if you really care about error
> > checking here.
> "This function will return a pointer to a dentry if
> it succeeds. This pointer must be passed to the
> debugfs_remove function when the file is to be removed
> (no automatic cleanup happens if your module is unloaded,
> you are responsible here.) If an error occurs, NULL will
> be returned."

This seems to be the old comment. Take a look at the updated one in mainline:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/debugfs/inode.c#n468

Greg changed the semantics of the debugfs APIs a while back but the kernel doc
was updated recently:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/debugfs/inode.c?id=d3002468cb5d5da11e22145c9af32facd5c34352

- Mani

> 
> > 
> >> +			debugfs_remove_recursive(root);
> >> +			return -EINVAL;
> >> +		}
> >> +	}
> >> +
> >> +	device_set_pm_not_required(dev);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void master_stats_remove(struct platform_device *pdev)
> >> +{
> >> +	struct dentry *root = platform_get_drvdata(pdev);
> >> +
> >> +	debugfs_remove_recursive(root);
> >> +}
> >> +
> >> +static const struct of_device_id rpm_master_table[] = {
> >> +	{ .compatible = "qcom,rpm-master-stats" },
> >> +	{ },
> >> +};
> >> +
> >> +static struct platform_driver master_stats_driver = {
> >> +	.probe = master_stats_probe,
> >> +	.remove_new = master_stats_remove,
> >> +	.driver = {
> >> +		.name = "rpm_master_stats",
> >> +		.of_match_table = rpm_master_table,
> >> +	},
> >> +};
> >> +module_platform_driver(master_stats_driver);
> >> +
> >> +MODULE_DESCRIPTION("RPM Master Statistics driver");
> > 
> > Qualcomm RPM Master Statistics driver
> Ack
> 
> Konrad
> > 
> > - Mani
> > 
> >> +MODULE_LICENSE("GPL");
> >>
> >> -- 
> >> 2.40.0
> >>
> > 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 2/2] soc: qcom: Introduce RPM master stats driver

[Konrad Dybcio]

On 17.04.2023 10:20, Manivannan Sadhasivam wrote:
> On Mon, Apr 17, 2023 at 09:14:39AM +0200, Konrad Dybcio wrote:
>>
>>
>> On 16.04.2023 16:20, Manivannan Sadhasivam wrote:
>>> On Fri, Apr 14, 2023 at 01:37:18PM +0200, Konrad Dybcio wrote:
>>>> Introduce a driver to query and expose detailed, per-subsystem (as opposed
>>>> to the existing qcom_stats driver which exposes SoC-wide data) about low
>>>> power mode states of a given RPM master. That includes the APSS (ARM),
>>>> MPSS (modem) and other remote cores, depending on the platform
>>>> configuration.
>>>>
>>>> This is a vastly cleaned up and restructured version of a similar
>>>> driver found in msm-5.4.
>>>>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>  drivers/soc/qcom/Kconfig            |  11 +++
>>>>  drivers/soc/qcom/Makefile           |   1 +
>>>>  drivers/soc/qcom/rpm_master_stats.c | 160 ++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 172 insertions(+)
>>>>
>>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>>>> index a491718f8064..e597799e8121 100644
>>>> --- a/drivers/soc/qcom/Kconfig
>>>> +++ b/drivers/soc/qcom/Kconfig
>>>> @@ -135,6 +135,17 @@ config QCOM_RMTFS_MEM
>>>>  
>>>>  	  Say y here if you intend to boot the modem remoteproc.
>>>>  
>>>> +config QCOM_RPM_MASTER_STATS
>>>> +	tristate "Qualcomm RPM Master stats"
>>>> +	depends on ARCH_QCOM || COMPILE_TEST
>>>> +	help
>>>> +	  The RPM Master sleep stats driver provides detailed per-subsystem
>>>> +	  sleep/wake data, read from the RPM message RAM. It can be used to
>>>> +	  assess whether all the low-power modes available are entered as
>>>> +	  expected or to check which part of the SoC prevents it from sleeping.
>>>> +
>>>> +	  Say y here if you intend to debug or monitor platform sleep.
>>>> +
>>>>  config QCOM_RPMH
>>>>  	tristate "Qualcomm RPM-Hardened (RPMH) Communication"
>>>>  	depends on ARCH_QCOM || COMPILE_TEST
>>>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>>>> index 0f43a88b4894..7349371fdea1 100644
>>>> --- a/drivers/soc/qcom/Makefile
>>>> +++ b/drivers/soc/qcom/Makefile
>>>> @@ -14,6 +14,7 @@ obj-$(CONFIG_QCOM_QMI_HELPERS)	+= qmi_helpers.o
>>>>  qmi_helpers-y	+= qmi_encdec.o qmi_interface.o
>>>>  obj-$(CONFIG_QCOM_RAMP_CTRL)	+= ramp_controller.o
>>>>  obj-$(CONFIG_QCOM_RMTFS_MEM)	+= rmtfs_mem.o
>>>> +obj-$(CONFIG_QCOM_RPM_MASTER_STATS)	+= rpm_master_stats.o
>>>>  obj-$(CONFIG_QCOM_RPMH)		+= qcom_rpmh.o
>>>>  qcom_rpmh-y			+= rpmh-rsc.o
>>>>  qcom_rpmh-y			+= rpmh.o
>>>> diff --git a/drivers/soc/qcom/rpm_master_stats.c b/drivers/soc/qcom/rpm_master_stats.c
>>>> new file mode 100644
>>>> index 000000000000..73080c92bf89
>>>> --- /dev/null
>>>> +++ b/drivers/soc/qcom/rpm_master_stats.c
>>>> @@ -0,0 +1,160 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Copyright (c) 2012-2020, The Linux Foundation. All rights reserved.
>>>> + * Copyright (c) 2023, Linaro Limited
>>>> + *
>>>> + * This driver supports what is known as "Master Stats v2", which seems to be
>>>> + * the only version which has ever shipped, all the way from 2013 to 2023.
>>>
>>> It'd better to mention "Qualcomm downstream" in the somewhere above comment to
>>> make it clear for users.
>> Ack
>>
>>>
>>>> + */
>>>> +
>>>> +#include <linux/debugfs.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/platform_device.h>
>>>> +
>>>> +struct master_stats_data {
>>>> +	void __iomem *base;
>>>> +	const char *label;
>>>> +};
>>>> +
>>>> +struct rpm_master_stats {
>>>> +	uint32_t active_cores;
>>>> +	uint32_t num_shutdowns;
>>>> +	uint64_t shutdown_req;
>>>> +	uint64_t wakeup_idx;
>>>> +	uint64_t bringup_req;
>>>> +	uint64_t bringup_ack;
>>>> +	uint32_t wakeup_reason; /* 0 = "rude wakeup", 1 = scheduled wakeup */
>>>> +	uint32_t last_sleep_trans_dur;
>>>> +	uint32_t last_wake_trans_dur;
>>>> +
>>>> +	/* Per-subsystem (*not necessarily* SoC-wide) XO shutdown stats */
>>>> +	uint32_t xo_count;
>>>> +	uint64_t xo_last_enter;
>>>> +	uint64_t last_exit;
>>>> +	uint64_t xo_total_dur;
>>>
>>> Why can't you use u64, u32?
>> Brain derp!
>>
>>>
>>> Also, sort these members as below:
>>>
>>> u64
>>> u32
>> No, it's the way this data is structured in the
>> memory and we copy it as a whole.
>>
> 
> Ok, in that case you'd need __packed.
> 
>>>
>>>> +};
>>>> +
> 
> [...]
> 
>>>> +		/*
>>>> +		 * Generally it's not advised to fail on debugfs errors, but this
>>>> +		 * driver's only job is exposing data therein.
>>>> +		 */
>>>> +		dent = debugfs_create_file(d[i].label, 0444, root,
>>>> +					   &d[i], &master_stats_fops);
>>>> +		if (!dent) {
>>>
>>> Don't check for NULL, instead use IS_ERR() if you really care about error
>>> checking here.
>> "This function will return a pointer to a dentry if
>> it succeeds. This pointer must be passed to the
>> debugfs_remove function when the file is to be removed
>> (no automatic cleanup happens if your module is unloaded,
>> you are responsible here.) If an error occurs, NULL will
>> be returned."
> 
> This seems to be the old comment. Take a look at the updated one in mainline:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/debugfs/inode.c#n468
> 
> Greg changed the semantics of the debugfs APIs a while back but the kernel doc
> was updated recently:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/debugfs/inode.c?id=d3002468cb5d5da11e22145c9af32facd5c34352
Okay, ack to both. I'll fix all that and resend!

Konrad
> 
> - Mani
> 
>>
>>>
>>>> +			debugfs_remove_recursive(root);
>>>> +			return -EINVAL;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	device_set_pm_not_required(dev);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static void master_stats_remove(struct platform_device *pdev)
>>>> +{
>>>> +	struct dentry *root = platform_get_drvdata(pdev);
>>>> +
>>>> +	debugfs_remove_recursive(root);
>>>> +}
>>>> +
>>>> +static const struct of_device_id rpm_master_table[] = {
>>>> +	{ .compatible = "qcom,rpm-master-stats" },
>>>> +	{ },
>>>> +};
>>>> +
>>>> +static struct platform_driver master_stats_driver = {
>>>> +	.probe = master_stats_probe,
>>>> +	.remove_new = master_stats_remove,
>>>> +	.driver = {
>>>> +		.name = "rpm_master_stats",
>>>> +		.of_match_table = rpm_master_table,
>>>> +	},
>>>> +};
>>>> +module_platform_driver(master_stats_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("RPM Master Statistics driver");
>>>
>>> Qualcomm RPM Master Statistics driver
>> Ack
>>
>> Konrad
>>>
>>> - Mani
>>>
>>>> +MODULE_LICENSE("GPL");
>>>>
>>>> -- 
>>>> 2.40.0
>>>>
>>>
>