[PATCH 0/2] Introduce SoC sleep stats driver

[PATCH 0/2] Introduce SoC sleep stats driver

[Maulik Shah]

Qualcomm Technologies Inc's (QTI) 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 backbone rails - Cx and
Mx and the oscillator clock, XO.

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

This series adds a driver to read the stats produced by remote processor and
exports using sysfs.

Maulik Shah (2):
  dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs
  drivers: qcom: Add SoC sleep stats driver

 .../bindings/soc/qcom/soc-sleep-stats.txt     |  36 +++
 drivers/soc/qcom/Kconfig                      |   9 +
 drivers/soc/qcom/Makefile                     |   1 +
 drivers/soc/qcom/soc_sleep_stats.c            | 249 ++++++++++++++++++
 4 files changed, 295 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt
 create mode 100644 drivers/soc/qcom/soc_sleep_stats.c

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

[PATCH 1/2] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs

[Maulik Shah]

Add device binding documentation for Qualcomm Technology Inc's (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>
---
 .../bindings/soc/qcom/soc-sleep-stats.txt     | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt

diff --git a/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt
new file mode 100644
index 000000000000..ee40687ded34
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt
@@ -0,0 +1,36 @@
+* SoC Sleep Stats
+
+Always On Processor/Resource Power Manager maintains statistics of the SoC
+sleep modes involving lowering or powering down of the backbone rails - Cx
+and Mx and the oscillator clock, XO.
+
+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.
+SoC Sleep Stats driver provides sysfs interface to display this information.
+
+PROPERTIES
+
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: Should be "qcom,rpmh-sleep-stats" or "qcom,rpm-sleep-stats".
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: The base address on the Always On Processor or Resource Power
+		    Manager from where the stats are read.
+
+EXAMPLE 1:
+
+	rpmh_sleep_stats: soc-sleep-stats@c3f0000 {
+		compatible = "qcom,rpmh-sleep-stats";
+		reg = <0 0xc3f0000 0 0x400>;
+	};
+
+EXAMPLE 2:
+
+	rpm_sleep_stats: soc-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 the Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 1/2] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs

[Stephen Boyd]

Quoting Maulik Shah (2019-08-07 23:12:27)
> Add device binding documentation for Qualcomm Technology Inc's (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>

Your SoB chain is odd. The author is Mahesh? Otherwise, use the
Co-Developed-by tag.

> ---
>  .../bindings/soc/qcom/soc-sleep-stats.txt     | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt
> new file mode 100644
> index 000000000000..ee40687ded34
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt
> @@ -0,0 +1,36 @@
> +* SoC Sleep Stats
> +
> +Always On Processor/Resource Power Manager maintains statistics of the SoC
> +sleep modes involving lowering or powering down of the backbone rails - Cx

What is a 'backbone' rail?

> +and Mx and the oscillator clock, XO.

Drop the comma? XO is the 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.
> +SoC Sleep Stats driver provides sysfs interface to display this information.

Can this document be YAML? Then it can be validated.

> +
> +PROPERTIES
> +
> +- compatible:
> +       Usage: required
> +       Value type: <string>
> +       Definition: Should be "qcom,rpmh-sleep-stats" or "qcom,rpm-sleep-stats".
> +
> +- reg:
> +       Usage: required
> +       Value type: <prop-encoded-array>
> +       Definition: The base address on the Always On Processor or Resource Power
> +                   Manager from where the stats are read.
> +
> +EXAMPLE 1:
> +
> +       rpmh_sleep_stats: soc-sleep-stats@c3f0000 {
> +               compatible = "qcom,rpmh-sleep-stats";
> +               reg = <0 0xc3f0000 0 0x400>;

Is this memory region in DDR? Or some specific IMEM location? I wonder
if it would be better to just have a pointer from the RPM node to this
memory region and then populate some stats if so.

> +       };
> +

Re: [PATCH 1/2] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs

[Maulik Shah]

On 8/8/2019 9:50 PM, Stephen Boyd wrote:
> Quoting Maulik Shah (2019-08-07 23:12:27)
>> Add device binding documentation for Qualcomm Technology Inc's (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>
> Your SoB chain is odd. The author is Mahesh? Otherwise, use the
> Co-Developed-by tag.
corrected in v2.
>> ---
>>   .../bindings/soc/qcom/soc-sleep-stats.txt     | 36 +++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt
>> new file mode 100644
>> index 000000000000..ee40687ded34
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt
>> @@ -0,0 +1,36 @@
>> +* SoC Sleep Stats
>> +
>> +Always On Processor/Resource Power Manager maintains statistics of the SoC
>> +sleep modes involving lowering or powering down of the backbone rails - Cx
> What is a 'backbone' rail?
done.
>
>> +and Mx and the oscillator clock, XO.
> Drop the comma? XO is the oscillator clock.
done.
>
>> +
>> +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.
>> +SoC Sleep Stats driver provides sysfs interface to display this information.
> Can this document be YAML? Then it can be validated.
converted to YAML in v2.
>
>> +
>> +PROPERTIES
>> +
>> +- compatible:
>> +       Usage: required
>> +       Value type: <string>
>> +       Definition: Should be "qcom,rpmh-sleep-stats" or "qcom,rpm-sleep-stats".
>> +
>> +- reg:
>> +       Usage: required
>> +       Value type: <prop-encoded-array>
>> +       Definition: The base address on the Always On Processor or Resource Power
>> +                   Manager from where the stats are read.
>> +
>> +EXAMPLE 1:
>> +
>> +       rpmh_sleep_stats: soc-sleep-stats@c3f0000 {
>> +               compatible = "qcom,rpmh-sleep-stats";
>> +               reg = <0 0xc3f0000 0 0x400>;
> Is this memory region in DDR? Or some specific IMEM location? I wonder
> if it would be better to just have a pointer from the RPM node to this
> memory region and then populate some stats if so.
Not a DDR.
>
>> +       };
>> +

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

[PATCH 2/2] drivers: qcom: Add SoC sleep stats driver

[Maulik Shah]

Qualcomm Technologies Inc's (QTI) chipsets support SoC level
low power modes. Statistics for SoC sleep stats are produced
by remote processor.

Lets's add a driver to read the shared memory exported by the
remote processor and export to sysfs.

Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
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 | 249 +++++++++++++++++++++++++++++
 3 files changed, 259 insertions(+)
 create mode 100644 drivers/soc/qcom/soc_sleep_stats.c

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 880cf0290962..7aac24430e99 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -163,6 +163,15 @@ config QCOM_SMSM
 	  Say yes here to support the Qualcomm Shared Memory State Machine.
 	  The state machine is represented by bits in shared memory.
 
+config QCOM_SOC_SLEEP_STATS
+	tristate "Qualcomm Technologies Inc. (QTI) SoC sleep stats driver"
+	depends on ARCH_QCOM
+	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 sysfs
+	  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 ffe519b0cb66..1530e0e73075 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_QCOM_SMEM) +=	smem.o
 obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
 obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
+obj-$(CONFIG_QCOM_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-slice.o
diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
new file mode 100644
index 000000000000..5b95d68512ec
--- /dev/null
+++ b/drivers/soc/qcom/soc_sleep_stats.c
@@ -0,0 +1,249 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Copyright (c) 2011-2019, The Linux Foundation. All rights reserved.
+ */
+
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#define ARCH_TIMER_FREQ	19200000
+
+struct stats_config {
+	u32 offset_addr;
+	u32 num_records;
+	bool appended_stats_avail;
+};
+
+struct soc_sleep_stats_data {
+	phys_addr_t stats_base;
+	resource_size_t stats_size;
+	const struct stats_config *config;
+	struct kobject *kobj;
+	struct kobj_attribute ka;
+	struct mutex lock;
+};
+
+struct entry {
+	__le32 stat_type;
+	__le32 count;
+	__le64 last_entered_at;
+	__le64 last_exited_at;
+	__le64 accumulated;
+};
+
+struct appended_entry {
+	__le32 client_votes;
+	__le32 reserved[3];
+};
+
+struct stats_entry {
+	struct entry entry;
+	struct appended_entry appended_entry;
+};
+
+static inline u64 get_time_in_sec(u64 counter)
+{
+	do_div(counter, ARCH_TIMER_FREQ);
+
+	return counter;
+}
+
+static inline ssize_t append_data_to_buf(char *buf, int length,
+					 struct stats_entry *data)
+{
+	char stat_type[5] = {0};
+
+	memcpy(stat_type, &data->entry.stat_type, sizeof(u32));
+
+	return scnprintf(buf, length,
+			 "%s\n"
+			 "\tCount                    :%u\n"
+			 "\tLast Entered At(sec)     :%llu\n"
+			 "\tLast Exited At(sec)      :%llu\n"
+			 "\tAccumulated Duration(sec):%llu\n"
+			 "\tClient Votes             :0x%x\n\n",
+			 stat_type, data->entry.count,
+			 data->entry.last_entered_at,
+			 data->entry.last_exited_at,
+			 data->entry.accumulated,
+			 data->appended_entry.client_votes);
+}
+
+static ssize_t stats_show(struct kobject *obj, struct kobj_attribute *attr,
+			  char *buf)
+{
+	void __iomem *reg;
+	int i;
+	uint32_t offset;
+	ssize_t length = 0, op_length;
+	struct stats_entry data;
+	struct entry *e = &data.entry;
+	struct appended_entry *ae = &data.appended_entry;
+	struct soc_sleep_stats_data *drv = container_of(attr,
+					   struct soc_sleep_stats_data, ka);
+
+	mutex_lock(&drv->lock);
+	reg = ioremap_nocache(drv->stats_base, drv->stats_size);
+	if (!reg) {
+		pr_err("io remap failed\n");
+		mutex_unlock(&drv->lock);
+		return length;
+	}
+
+	for (i = 0; i < drv->config->num_records; i++) {
+		offset = offsetof(struct entry, stat_type);
+		e->stat_type = le32_to_cpu(readl_relaxed(reg + offset));
+
+		offset = offsetof(struct entry, count);
+		e->count = le32_to_cpu(readl_relaxed(reg + offset));
+
+		offset = offsetof(struct entry, last_entered_at);
+		e->last_entered_at = le64_to_cpu(readq_relaxed(reg + offset));
+
+		offset = offsetof(struct entry, last_exited_at);
+		e->last_exited_at = le64_to_cpu(readq_relaxed(reg + offset));
+
+		offset = offsetof(struct entry, last_exited_at);
+		e->accumulated = le64_to_cpu(readq_relaxed(reg + offset));
+
+		e->last_entered_at = get_time_in_sec(e->last_entered_at);
+		e->last_exited_at = get_time_in_sec(e->last_exited_at);
+		e->accumulated = get_time_in_sec(e->accumulated);
+
+		reg += sizeof(struct entry);
+
+		if (drv->config->appended_stats_avail) {
+			offset = offsetof(struct appended_entry, client_votes);
+			ae->client_votes = le32_to_cpu(readl_relaxed(reg +
+								     offset));
+
+			reg += sizeof(struct appended_entry);
+		} else
+			ae->client_votes = 0;
+
+		op_length = append_data_to_buf(buf + length, PAGE_SIZE - length,
+					       &data);
+		if (op_length >= PAGE_SIZE - length)
+			goto exit;
+
+		length += op_length;
+	}
+exit:
+	iounmap(reg);
+	mutex_unlock(&drv->lock);
+	return length;
+}
+
+static int soc_sleep_stats_create_sysfs(struct platform_device *pdev,
+					struct soc_sleep_stats_data *drv)
+{
+	int ret = -ENOMEM;
+
+	drv->kobj = kobject_create_and_add("soc_sleep", power_kobj);
+	if (!drv->kobj)
+		goto fail;
+
+	sysfs_attr_init(drv->ka.attr);
+	drv->ka.attr.mode = 0444;
+	drv->ka.attr.name = "stats";
+	drv->ka.show = stats_show;
+
+	ret = sysfs_create_file(drv->kobj, &drv->ka.attr);
+	if (ret)
+		goto fail;
+
+	platform_set_drvdata(pdev, drv);
+fail:
+	return ret;
+}
+
+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 int soc_sleep_stats_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	struct soc_sleep_stats_data *drv;
+	struct resource *res;
+	void __iomem *offset_addr;
+	int ret;
+
+	drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return -ENOMEM;
+
+	match = of_match_node(soc_sleep_stats_table, pdev->dev.of_node);
+	if (!match)
+		return -ENODEV;
+
+	drv->config = match->data;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return PTR_ERR(res);
+
+	offset_addr = ioremap_nocache(res->start + drv->config->offset_addr,
+				      sizeof(u32));
+	if (IS_ERR(offset_addr))
+		return PTR_ERR(offset_addr);
+
+	drv->stats_base = res->start | readl_relaxed(offset_addr);
+	drv->stats_size = resource_size(res);
+	iounmap(offset_addr);
+	mutex_init(&drv->lock);
+
+	ret = soc_sleep_stats_create_sysfs(pdev, drv);
+	if (ret)
+		pr_info("Failed to create sysfs interface\n");
+
+	return ret;
+}
+
+static int soc_sleep_stats_remove(struct platform_device *pdev)
+{
+	struct soc_sleep_stats_data *drv = platform_get_drvdata(pdev);
+
+	sysfs_remove_file(drv->kobj, &drv->ka.attr);
+	kobject_put(drv->kobj);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+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. SoC sleep stats driver");
+MODULE_LICENSE("GPL v2");
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 2/2] drivers: qcom: Add SoC sleep stats driver

[Stephen Boyd]

Quoting Maulik Shah (2019-08-07 23:12:28)
> Qualcomm Technologies Inc's (QTI) chipsets support SoC level
> low power modes. Statistics for SoC sleep stats are produced
> by remote processor.
> 
> Lets's add a driver to read the shared memory exported by the
> remote processor and export to sysfs.
> 
> Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>

SoB chain is weird here too.

> ---
>  drivers/soc/qcom/Kconfig           |   9 ++
>  drivers/soc/qcom/Makefile          |   1 +
>  drivers/soc/qcom/soc_sleep_stats.c | 249 +++++++++++++++++++++++++++++

There should be a Documentation/ABI/ path in this diffstat above because
you're adding sysfs attributes.

There's some similar support in the ARM PSCI spec for extracting
idle/suspend stats, see section 5.21 PSCI_STAT_RESIDENCY/COUNT. Maybe
this code can align with that feature in PSCI? At the least, I hope we
can come up with a generic sysfs ABI that can be used to describe CPU
and system wide power states in a way that userspace can read and
understand how long the device was in these different power states. I
would guess that other architectures like x86 may also want to get
involved in reporting this information in a standard way, so please loop
in some x86 power folks too.

It would be neat if the PSCI feature could be used for this instead of
having a custom SoC driver. Maybe that won't work though because this
works for shipping firmware and/or because of the 'client_votes' thing
which looks like special extra data describing the other subsystems? At
least for some SoCs it may be all they need though, so keeping the PSCI
call in mind would be good when developing the ABI and may be enough for
userspace purposes. The client_votes part may be possible to layer on
top of the PSCI calls anyway, and go into some other file so we can
figure out which remoteproc is holding up suspend or idle states.

> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 880cf0290962..7aac24430e99 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -163,6 +163,15 @@ config QCOM_SMSM
>           Say yes here to support the Qualcomm Shared Memory State Machine.
>           The state machine is represented by bits in shared memory.
>  
> +config QCOM_SOC_SLEEP_STATS
> +       tristate "Qualcomm Technologies Inc. (QTI) SoC sleep stats driver"
> +       depends on ARCH_QCOM
> +       help
> +         Qualcomm Technologies Inc. (QTI) SoC sleep stats driver to read
> +         the shared memory exported by the remote processor related to

Shared memory sounds like DDR.

> +         various SoC level low power modes statistics and export to sysfs
> +         interface.
> +
>  config QCOM_WCNSS_CTRL
>         tristate "Qualcomm WCNSS control driver"
>         depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
> new file mode 100644
> index 000000000000..5b95d68512ec
> --- /dev/null
> +++ b/drivers/soc/qcom/soc_sleep_stats.c
> @@ -0,0 +1,249 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Copyright (c) 2011-2019, The Linux Foundation. All rights reserved.
> + */
> +
> +#define pr_fmt(fmt) "%s: " fmt, __func__
> +
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>

Is this include used?

> +
> +#define ARCH_TIMER_FREQ        19200000

Can't this come through clk APIs? Or is this the ARM architected timer
freqeuency?

> +
> +struct stats_config {
> +       u32 offset_addr;
> +       u32 num_records;
> +       bool appended_stats_avail;
> +};
> +
> +struct soc_sleep_stats_data {
> +       phys_addr_t stats_base;
> +       resource_size_t stats_size;
> +       const struct stats_config *config;
> +       struct kobject *kobj;
> +       struct kobj_attribute ka;
> +       struct mutex lock;
> +};
> +
> +struct entry {
> +       __le32 stat_type;
> +       __le32 count;
> +       __le64 last_entered_at;
> +       __le64 last_exited_at;
> +       __le64 accumulated;
> +};
> +
> +struct appended_entry {
> +       __le32 client_votes;
> +       __le32 reserved[3];
> +};
> +
> +struct stats_entry {
> +       struct entry entry;
> +       struct appended_entry appended_entry;
> +};
> +
> +static inline u64 get_time_in_sec(u64 counter)
> +{
> +       do_div(counter, ARCH_TIMER_FREQ);
> +
> +       return counter;
> +}
> +
> +static inline ssize_t append_data_to_buf(char *buf, int length,
> +                                        struct stats_entry *data)
> +{
> +       char stat_type[5] = {0};
> +
> +       memcpy(stat_type, &data->entry.stat_type, sizeof(u32));

sizeof(u32) != 5. Is this on purpose?

> +
> +       return scnprintf(buf, length,
> +                        "%s\n"
> +                        "\tCount                    :%u\n"
> +                        "\tLast Entered At(sec)     :%llu\n"
> +                        "\tLast Exited At(sec)      :%llu\n"
> +                        "\tAccumulated Duration(sec):%llu\n"
> +                        "\tClient Votes             :0x%x\n\n",
> +                        stat_type, data->entry.count,
> +                        data->entry.last_entered_at,
> +                        data->entry.last_exited_at,
> +                        data->entry.accumulated,
> +                        data->appended_entry.client_votes);
> +}
> +
> +static ssize_t stats_show(struct kobject *obj, struct kobj_attribute *attr,
> +                         char *buf)
> +{
> +       void __iomem *reg;
> +       int i;
> +       uint32_t offset;
> +       ssize_t length = 0, op_length;
> +       struct stats_entry data;
> +       struct entry *e = &data.entry;
> +       struct appended_entry *ae = &data.appended_entry;
> +       struct soc_sleep_stats_data *drv = container_of(attr,
> +                                          struct soc_sleep_stats_data, ka);
> +
> +       mutex_lock(&drv->lock);
> +       reg = ioremap_nocache(drv->stats_base, drv->stats_size);
> +       if (!reg) {
> +               pr_err("io remap failed\n");

This looks like a real bad idea to ioremap each time the stats are
shown. Why not just map once in probe so we don't have to create a
mapping and suffer the overhead involved in that?

> +               mutex_unlock(&drv->lock);
> +               return length;
> +       }
> +
> +       for (i = 0; i < drv->config->num_records; i++) {
> +               offset = offsetof(struct entry, stat_type);
> +               e->stat_type = le32_to_cpu(readl_relaxed(reg + offset));
> +
> +               offset = offsetof(struct entry, count);
> +               e->count = le32_to_cpu(readl_relaxed(reg + offset));
> +
> +               offset = offsetof(struct entry, last_entered_at);
> +               e->last_entered_at = le64_to_cpu(readq_relaxed(reg + offset));
> +
> +               offset = offsetof(struct entry, last_exited_at);
> +               e->last_exited_at = le64_to_cpu(readq_relaxed(reg + offset));
> +
> +               offset = offsetof(struct entry, last_exited_at);
> +               e->accumulated = le64_to_cpu(readq_relaxed(reg + offset));
> +
> +               e->last_entered_at = get_time_in_sec(e->last_entered_at);
> +               e->last_exited_at = get_time_in_sec(e->last_exited_at);
> +               e->accumulated = get_time_in_sec(e->accumulated);
> +
> +               reg += sizeof(struct entry);
> +
> +               if (drv->config->appended_stats_avail) {
> +                       offset = offsetof(struct appended_entry, client_votes);
> +                       ae->client_votes = le32_to_cpu(readl_relaxed(reg +
> +                                                                    offset));
> +
> +                       reg += sizeof(struct appended_entry);
> +               } else
> +                       ae->client_votes = 0;

Please add braces to the else statement when the if statement has
braces.

> +
> +               op_length = append_data_to_buf(buf + length, PAGE_SIZE - length,
> +                                              &data);
> +               if (op_length >= PAGE_SIZE - length)
> +                       goto exit;
> +
> +               length += op_length;
> +       }
> +exit:
> +       iounmap(reg);
> +       mutex_unlock(&drv->lock);
> +       return length;
> +}
> +
> +static int soc_sleep_stats_create_sysfs(struct platform_device *pdev,
> +                                       struct soc_sleep_stats_data *drv)
> +{
> +       int ret = -ENOMEM;
> +
> +       drv->kobj = kobject_create_and_add("soc_sleep", power_kobj);
> +       if (!drv->kobj)
> +               goto fail;

Just return -ENOMEM here. It is really weird to make kobjects directly
like this. How is userspace expected to use this?

> +
> +       sysfs_attr_init(drv->ka.attr);
> +       drv->ka.attr.mode = 0444;
> +       drv->ka.attr.name = "stats";
> +       drv->ka.show = stats_show;
> +
> +       ret = sysfs_create_file(drv->kobj, &drv->ka.attr);
> +       if (ret)
> +               goto fail;

Just return sysfs_create_file()?

> +
> +       platform_set_drvdata(pdev, drv);

Do this platform_set_drvdata in probe?

> +fail:
> +       return ret;
> +}
> +
> +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 int soc_sleep_stats_probe(struct platform_device *pdev)
> +{
> +       const struct of_device_id *match;
> +       struct soc_sleep_stats_data *drv;
> +       struct resource *res;
> +       void __iomem *offset_addr;
> +       int ret;
> +
> +       drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
> +       if (!drv)
> +               return -ENOMEM;
> +
> +       match = of_match_node(soc_sleep_stats_table, pdev->dev.of_node);
> +       if (!match)
> +               return -ENODEV;
> +
> +       drv->config = match->data;

Is this of_device_get_match_data()?

> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res)
> +               return PTR_ERR(res);
> +
> +       offset_addr = ioremap_nocache(res->start + drv->config->offset_addr,
> +                                     sizeof(u32));

Why not just devm_platform_ioremap_resource()?

> +       if (IS_ERR(offset_addr))
> +               return PTR_ERR(offset_addr);
> +
> +       drv->stats_base = res->start | readl_relaxed(offset_addr);
> +       drv->stats_size = resource_size(res);
> +       iounmap(offset_addr);
> +       mutex_init(&drv->lock);

Hopefully this lock isn't required?

> +
> +       ret = soc_sleep_stats_create_sysfs(pdev, drv);
> +       if (ret)
> +               pr_info("Failed to create sysfs interface\n");

Not pr_err()? Or dev_err()?

> +
> +       return ret;
> +}
> +
> +static int soc_sleep_stats_remove(struct platform_device *pdev)
> +{
> +       struct soc_sleep_stats_data *drv = platform_get_drvdata(pdev);
> +
> +       sysfs_remove_file(drv->kobj, &drv->ka.attr);
> +       kobject_put(drv->kobj);
> +       platform_set_drvdata(pdev, NULL);

This last line isn't necessary. Please remove.

> +
> +       return 0;
> +}
> +

Re: [PATCH 2/2] drivers: qcom: Add SoC sleep stats driver

[Maulik Shah]

On 8/9/2019 12:10 AM, Stephen Boyd wrote:
> Quoting Maulik Shah (2019-08-07 23:12:28)
>> Qualcomm Technologies Inc's (QTI) chipsets support SoC level
>> low power modes. Statistics for SoC sleep stats are produced
>> by remote processor.
>>
>> Lets's add a driver to read the shared memory exported by the
>> remote processor and export to sysfs.
>>
>> Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> SoB chain is weird here too.
corrected in v2.
>
>> ---
>>   drivers/soc/qcom/Kconfig           |   9 ++
>>   drivers/soc/qcom/Makefile          |   1 +
>>   drivers/soc/qcom/soc_sleep_stats.c | 249 +++++++++++++++++++++++++++++
> There should be a Documentation/ABI/ path in this diffstat above because
> you're adding sysfs attributes.
In v2 using debugfs for stats information.
>
> There's some similar support in the ARM PSCI spec for extracting
> idle/suspend stats, see section 5.21 PSCI_STAT_RESIDENCY/COUNT. Maybe
> this code can align with that feature in PSCI? At the least, I hope we
> can come up with a generic sysfs ABI that can be used to describe CPU
> and system wide power states in a way that userspace can read and
> understand how long the device was in these different power states. I
> would guess that other architectures like x86 may also want to get
> involved in reporting this information in a standard way, so please loop
> in some x86 power folks too.
>
> It would be neat if the PSCI feature could be used for this instead of
> having a custom SoC driver. Maybe that won't work though because this
> works for shipping firmware and/or because of the 'client_votes' thing
> which looks like special extra data describing the other subsystems? At
> least for some SoCs it may be all they need though, so keeping the PSCI
> call in mind would be good when developing the ABI and may be enough for
> userspace purposes. The client_votes part may be possible to layer on
> top of the PSCI calls anyway, and go into some other file so we can
> figure out which remoteproc is holding up suspend or idle states.
correct PSCI won't work here.
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index 880cf0290962..7aac24430e99 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -163,6 +163,15 @@ config QCOM_SMSM
>>            Say yes here to support the Qualcomm Shared Memory State Machine.
>>            The state machine is represented by bits in shared memory.
>>   
>> +config QCOM_SOC_SLEEP_STATS
>> +       tristate "Qualcomm Technologies Inc. (QTI) SoC sleep stats driver"
>> +       depends on ARCH_QCOM
>> +       help
>> +         Qualcomm Technologies Inc. (QTI) SoC sleep stats driver to read
>> +         the shared memory exported by the remote processor related to
> Shared memory sounds like DDR.
Not a DDR.
>
>> +         various SoC level low power modes statistics and export to sysfs
>> +         interface.
>> +
>>   config QCOM_WCNSS_CTRL
>>          tristate "Qualcomm WCNSS control driver"
>>          depends on ARCH_QCOM || COMPILE_TEST
>> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
>> new file mode 100644
>> index 000000000000..5b95d68512ec
>> --- /dev/null
>> +++ b/drivers/soc/qcom/soc_sleep_stats.c
>> @@ -0,0 +1,249 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +/*
>> + * Copyright (c) 2011-2019, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#define pr_fmt(fmt) "%s: " fmt, __func__
>> +
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/uaccess.h>
> Is this include used?
removed unused headers in v2.
>
>> +
>> +#define ARCH_TIMER_FREQ        19200000
> Can't this come through clk APIs? Or is this the ARM architected timer
> freqeuency?
Updated to use clk APIs
>> +
>> +struct stats_config {
>> +       u32 offset_addr;
>> +       u32 num_records;
>> +       bool appended_stats_avail;
>> +};
>> +
>> +struct soc_sleep_stats_data {
>> +       phys_addr_t stats_base;
>> +       resource_size_t stats_size;
>> +       const struct stats_config *config;
>> +       struct kobject *kobj;
>> +       struct kobj_attribute ka;
>> +       struct mutex lock;
>> +};
>> +
>> +struct entry {
>> +       __le32 stat_type;
>> +       __le32 count;
>> +       __le64 last_entered_at;
>> +       __le64 last_exited_at;
>> +       __le64 accumulated;
>> +};
>> +
>> +struct appended_entry {
>> +       __le32 client_votes;
>> +       __le32 reserved[3];
>> +};
>> +
>> +struct stats_entry {
>> +       struct entry entry;
>> +       struct appended_entry appended_entry;
>> +};
>> +
>> +static inline u64 get_time_in_sec(u64 counter)
>> +{
>> +       do_div(counter, ARCH_TIMER_FREQ);
>> +
>> +       return counter;
>> +}
>> +
>> +static inline ssize_t append_data_to_buf(char *buf, int length,
>> +                                        struct stats_entry *data)
>> +{
>> +       char stat_type[5] = {0};
>> +
>> +       memcpy(stat_type, &data->entry.stat_type, sizeof(u32));
> sizeof(u32) != 5. Is this on purpose?

yes, maximum name length 5.

>
>> +
>> +       return scnprintf(buf, length,
>> +                        "%s\n"
>> +                        "\tCount                    :%u\n"
>> +                        "\tLast Entered At(sec)     :%llu\n"
>> +                        "\tLast Exited At(sec)      :%llu\n"
>> +                        "\tAccumulated Duration(sec):%llu\n"
>> +                        "\tClient Votes             :0x%x\n\n",
>> +                        stat_type, data->entry.count,
>> +                        data->entry.last_entered_at,
>> +                        data->entry.last_exited_at,
>> +                        data->entry.accumulated,
>> +                        data->appended_entry.client_votes);
>> +}
>> +
>> +static ssize_t stats_show(struct kobject *obj, struct kobj_attribute *attr,
>> +                         char *buf)
>> +{
>> +       void __iomem *reg;
>> +       int i;
>> +       uint32_t offset;
>> +       ssize_t length = 0, op_length;
>> +       struct stats_entry data;
>> +       struct entry *e = &data.entry;
>> +       struct appended_entry *ae = &data.appended_entry;
>> +       struct soc_sleep_stats_data *drv = container_of(attr,
>> +                                          struct soc_sleep_stats_data, ka);
>> +
>> +       mutex_lock(&drv->lock);
>> +       reg = ioremap_nocache(drv->stats_base, drv->stats_size);
>> +       if (!reg) {
>> +               pr_err("io remap failed\n");
> This looks like a real bad idea to ioremap each time the stats are
> shown. Why not just map once in probe so we don't have to create a
> mapping and suffer the overhead involved in that?
Updated in v2 to ioremap only once in probe.
>
>> +               mutex_unlock(&drv->lock);
>> +               return length;
>> +       }
>> +
>> +       for (i = 0; i < drv->config->num_records; i++) {
>> +               offset = offsetof(struct entry, stat_type);
>> +               e->stat_type = le32_to_cpu(readl_relaxed(reg + offset));
>> +
>> +               offset = offsetof(struct entry, count);
>> +               e->count = le32_to_cpu(readl_relaxed(reg + offset));
>> +
>> +               offset = offsetof(struct entry, last_entered_at);
>> +               e->last_entered_at = le64_to_cpu(readq_relaxed(reg + offset));
>> +
>> +               offset = offsetof(struct entry, last_exited_at);
>> +               e->last_exited_at = le64_to_cpu(readq_relaxed(reg + offset));
>> +
>> +               offset = offsetof(struct entry, last_exited_at);
>> +               e->accumulated = le64_to_cpu(readq_relaxed(reg + offset));
>> +
>> +               e->last_entered_at = get_time_in_sec(e->last_entered_at);
>> +               e->last_exited_at = get_time_in_sec(e->last_exited_at);
>> +               e->accumulated = get_time_in_sec(e->accumulated);
>> +
>> +               reg += sizeof(struct entry);
>> +
>> +               if (drv->config->appended_stats_avail) {
>> +                       offset = offsetof(struct appended_entry, client_votes);
>> +                       ae->client_votes = le32_to_cpu(readl_relaxed(reg +
>> +                                                                    offset));
>> +
>> +                       reg += sizeof(struct appended_entry);
>> +               } else
>> +                       ae->client_votes = 0;
> Please add braces to the else statement when the if statement has
> braces.
Done.
>> +
>> +               op_length = append_data_to_buf(buf + length, PAGE_SIZE - length,
>> +                                              &data);
>> +               if (op_length >= PAGE_SIZE - length)
>> +                       goto exit;
>> +
>> +               length += op_length;
>> +       }
>> +exit:
>> +       iounmap(reg);
>> +       mutex_unlock(&drv->lock);
>> +       return length;
>> +}
>> +
>> +static int soc_sleep_stats_create_sysfs(struct platform_device *pdev,
>> +                                       struct soc_sleep_stats_data *drv)
>> +{
>> +       int ret = -ENOMEM;
>> +
>> +       drv->kobj = kobject_create_and_add("soc_sleep", power_kobj);
>> +       if (!drv->kobj)
>> +               goto fail;
> Just return -ENOMEM here. It is really weird to make kobjects directly
> like this. How is userspace expected to use this?
Done. using debugfs.
>> +
>> +       sysfs_attr_init(drv->ka.attr);
>> +       drv->ka.attr.mode = 0444;
>> +       drv->ka.attr.name = "stats";
>> +       drv->ka.show = stats_show;
>> +
>> +       ret = sysfs_create_file(drv->kobj, &drv->ka.attr);
>> +       if (ret)
>> +               goto fail;
> Just return sysfs_create_file()?
>
>> +
>> +       platform_set_drvdata(pdev, drv);
> Do this platform_set_drvdata in probe?
done.
>> +fail:
>> +       return ret;
>> +}
>> +
>> +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 int soc_sleep_stats_probe(struct platform_device *pdev)
>> +{
>> +       const struct of_device_id *match;
>> +       struct soc_sleep_stats_data *drv;
>> +       struct resource *res;
>> +       void __iomem *offset_addr;
>> +       int ret;
>> +
>> +       drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
>> +       if (!drv)
>> +               return -ENOMEM;
>> +
>> +       match = of_match_node(soc_sleep_stats_table, pdev->dev.of_node);
>> +       if (!match)
>> +               return -ENODEV;
>> +
>> +       drv->config = match->data;
> Is this of_device_get_match_data()?
done.
>
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       if (!res)
>> +               return PTR_ERR(res);
>> +
>> +       offset_addr = ioremap_nocache(res->start + drv->config->offset_addr,
>> +                                     sizeof(u32));
> Why not just devm_platform_ioremap_resource()?
this adds  drv->config->offset_addr before ioremap.
>
>> +       if (IS_ERR(offset_addr))
>> +               return PTR_ERR(offset_addr);
>> +
>> +       drv->stats_base = res->start | readl_relaxed(offset_addr);
>> +       drv->stats_size = resource_size(res);
>> +       iounmap(offset_addr);
>> +       mutex_init(&drv->lock);
> Hopefully this lock isn't required?
removed lock.
>
>> +
>> +       ret = soc_sleep_stats_create_sysfs(pdev, drv);
>> +       if (ret)
>> +               pr_info("Failed to create sysfs interface\n");
> Not pr_err()? Or dev_err()?
done.
>> +
>> +       return ret;
>> +}
>> +
>> +static int soc_sleep_stats_remove(struct platform_device *pdev)
>> +{
>> +       struct soc_sleep_stats_data *drv = platform_get_drvdata(pdev);
>> +
>> +       sysfs_remove_file(drv->kobj, &drv->ka.attr);
>> +       kobject_put(drv->kobj);
>> +       platform_set_drvdata(pdev, NULL);
> This last line isn't necessary. Please remove.
Done.
>
>> +
>> +       return 0;
>> +}
>> +

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