Introduce SoC sleep stats driver
cover Introduce SoC sleep stats driver
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
Signature · 1 line
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
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.
--- .../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
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt new file +36 -0
@@ -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>; + };
Signature · 1 line
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
Quoting Maulik Shah (2019-08-07 23:12:27)
Quoted 8 lines
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.
Signed-off-by Mahesh Sivasubramanian <[email protected]>Signed-off-by Lina Iyer <[email protected]>Signed-off-by Maulik Shah <[email protected]>
Your SoB chain is odd. The author is Mahesh? Otherwise, use the Co-Developed-by tag.
Quoted 11 lines
--- .../bindings/soc/qcom/soc-sleep-stats.txt | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txtDocumentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt new file +0
-0
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt line 4
Quoted 4 lines
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt new file +4
-0@@ -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?
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt line 5
Quoted 1 line
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt new file +1
-0+and Mx and the oscillator clock, XO.
Drop the comma? XO is the oscillator clock.
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt line 9
Quoted 4 lines
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt new file +4
-0+ +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.
Quoted 9 lines
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt new file +9
-0+ +PROPERTIES + +- compatible: + Usage: required + Value type: <string> + Definition: Should be "qcom,rpmh-sleep-stats" or "qcom,rpm-sleep-stats". + +- reg:
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt line 28
Quoted 10 lines
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt new file +10
-0+ 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.
Quoted 2 lines
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt new file +2
-0+ }; +
Re: [PATCH 1/2] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs
Quoted 11 lines · Stephen Boyd
Quoting Maulik Shah (2019-08-07 23:12:27)
Quoted 8 lines
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.
Signed-off-by Mahesh Sivasubramanian <[email protected]>Signed-off-by Lina Iyer <[email protected]>Signed-off-by Maulik Shah <[email protected]>Your SoB chain is odd. The author is Mahesh? Otherwise, use the Co-Developed-by tag.
corrected in v2.
Quoted 5 lines
Quoted 5 lines
--- .../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
Quoted 11 lines
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt line 4
Quoted 10 lines
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt new file +4
-0@@ -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 - CxWhat is a 'backbone' rail?
done.
Quoted 3 lines
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt line 5
Quoted 2 lines
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt new file +1
-0+and Mx and the oscillator clock, XO.Drop the comma? XO is the oscillator clock.
done.
Quoted 6 lines
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt line 9
Quoted 5 lines
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt new file +4
-0+ +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.
Quoted 15 lines
Quoted 15 lines
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt new file +14
-0+ +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. +
Quoted 8 lines
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt line 28
Quoted 5 lines
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt new file +5
-0+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.
Quoted 3 lines
Quoted 3 lines
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.txt new file +2
-0+ }; +
Signature · 1 line
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
In reply to Maulik ShahQualcomm 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.
--- 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
drivers/soc/qcom/Kconfig +9 -0
@@ -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
drivers/soc/qcom/Makefile +1 -0
@@ -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
drivers/soc/qcom/soc_sleep_stats.c new file +249 -0
249 lines not on this page · raw message
Signature · 1 line
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
Quoting Maulik Shah (2019-08-07 23:12:28)
Quoted 10 lines
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 <[email protected]>Signed-off-by Lina Iyer <[email protected]>Signed-off-by Maulik Shah <[email protected]>
SoB chain is weird here too.
Quoted 4 lines
--- 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.
Quoted 6 lines
drivers/soc/qcom/Kconfig +0
-0
drivers/soc/qcom/Kconfig config QCOM_SMSM · line 171
Quoted 9 lines
drivers/soc/qcom/Kconfig +6
-0@@ -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.
Quoted 19 lines
drivers/soc/qcom/Kconfig +3
-0+ 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_TESTdrivers/soc/qcom/soc_sleep_stats.c new file +7
-0@@ -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__
drivers/soc/qcom/soc_sleep_stats.c line 17
Quoted 10 lines
drivers/soc/qcom/soc_sleep_stats.c new file +10
-0+ +#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?
drivers/soc/qcom/soc_sleep_stats.c line 19
Quoted 2 lines
drivers/soc/qcom/soc_sleep_stats.c new file +2
-0+ +#define ARCH_TIMER_FREQ 19200000
Can't this come through clk APIs? Or is this the ARM architected timer freqeuency?
Quoted 37 lines
drivers/soc/qcom/soc_sleep_stats.c new file +37
-0+ +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 );
drivers/soc/qcom/soc_sleep_stats.c line 66
Quoted 10 lines
drivers/soc/qcom/soc_sleep_stats.c new file +10
-0+ +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?
Quoted 22 lines
drivers/soc/qcom/soc_sleep_stats.c new file +22
-0+ +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;
drivers/soc/qcom/soc_sleep_stats.c line 98
Quoted 10 lines
drivers/soc/qcom/soc_sleep_stats.c new file +10
-0+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?
Quoted 24 lines
drivers/soc/qcom/soc_sleep_stats.c new file +24
-0+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); +
drivers/soc/qcom/soc_sleep_stats.c line 132
Quoted 10 lines
drivers/soc/qcom/soc_sleep_stats.c new file +10
-0+ 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.
Quoted 12 lines
drivers/soc/qcom/soc_sleep_stats.c new file +12
-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;
drivers/soc/qcom/soc_sleep_stats.c line 154
Quoted 10 lines
drivers/soc/qcom/soc_sleep_stats.c new file +10
-0+} + +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?
drivers/soc/qcom/soc_sleep_stats.c line 163
Quoted 9 lines
drivers/soc/qcom/soc_sleep_stats.c new file +9
-0+ +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()?
drivers/soc/qcom/soc_sleep_stats.c line 165
Quoted 2 lines
drivers/soc/qcom/soc_sleep_stats.c new file +2
-0+ +platform_set_drvdata (pdev, drv);
Do this platform_set_drvdata in probe?
Quoted 29 lines
drivers/soc/qcom/soc_sleep_stats.c new file +29
-0+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;
drivers/soc/qcom/soc_sleep_stats.c line 204
Quoted 10 lines
drivers/soc/qcom/soc_sleep_stats.c new file +10
-0+ + 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()?
drivers/soc/qcom/soc_sleep_stats.c line 211
Quoted 7 lines
drivers/soc/qcom/soc_sleep_stats.c new file +7
-0+ + 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()?
drivers/soc/qcom/soc_sleep_stats.c line 218
Quoted 7 lines
drivers/soc/qcom/soc_sleep_stats.c new file +7
-0+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?
drivers/soc/qcom/soc_sleep_stats.c line 222
Quoted 4 lines
drivers/soc/qcom/soc_sleep_stats.c new file +4
-0+ + ret =soc_sleep_stats_create_sysfs (pdev, drv); +if (ret) +pr_info ("Failed to create sysfs interface\n" );
Not pr_err()? Or dev_err()?
drivers/soc/qcom/soc_sleep_stats.c line 233
Quoted 11 lines
drivers/soc/qcom/soc_sleep_stats.c new file +11
-0+ +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.
Quoted 4 lines
drivers/soc/qcom/soc_sleep_stats.c new file +4
-0+ +return 0 ; +} +
Re: [PATCH 2/2] drivers: qcom: Add SoC sleep stats driver
Quoted 5 lines · Stephen Boyd
Quoting Maulik Shah (2019-08-07 23:12:28)
Quoted 4 lines
Qualcomm Technologies Inc's (QTI) chipsets support SoC level low power modes. Statistics for SoC sleep stats are produced by remote processor.
Quoted 7 lines
Quoted 6 lines
Lets's add a driver to read the shared memory exported by the remote processor and export to sysfs.
Signed-off-by Mahesh Sivasubramanian <[email protected]>Signed-off-by Lina Iyer <[email protected]>Signed-off-by Maulik Shah <[email protected]>SoB chain is weird here too.
corrected in v2.
Quoted 7 lines
Quoted 5 lines
--- 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.
Quoted 11 lines
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.
Quoted 9 lines
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. Theclient_votespart 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.
Quoted 8 lines
Quoted 8 lines
drivers/soc/qcom/Kconfig +0
-0@@ -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.
Quoted 7 lines
drivers/soc/qcom/Kconfig config QCOM_SMSM · line 171
Quoted 6 lines
drivers/soc/qcom/Kconfig +6
-0+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 toShared memory sounds like DDR.
Not a DDR.
Quoted 23 lines
Quoted 23 lines
drivers/soc/qcom/Kconfig +3
-0+ 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_TESTdrivers/soc/qcom/soc_sleep_stats.c new file +10
-0@@ -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>
Quoted 8 lines
drivers/soc/qcom/soc_sleep_stats.c line 17
Quoted 7 lines
drivers/soc/qcom/soc_sleep_stats.c new file +7
-0+#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.
Quoted 5 lines
drivers/soc/qcom/soc_sleep_stats.c line 19
Quoted 3 lines
drivers/soc/qcom/soc_sleep_stats.c new file +2
-0+ +#define ARCH_TIMER_FREQ 19200000Can't this come through clk APIs? Or is this the ARM architected timer freqeuency?
Updated to use clk APIs
Quoted 40 lines
Quoted 40 lines
drivers/soc/qcom/soc_sleep_stats.c new file +40
-0+ +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; +}
Quoted 8 lines
drivers/soc/qcom/soc_sleep_stats.c line 66
Quoted 7 lines
drivers/soc/qcom/soc_sleep_stats.c new file +7
-0+ +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.
Quoted 28 lines
Quoted 28 lines
drivers/soc/qcom/soc_sleep_stats.c new file +27
-0+ +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);
Quoted 8 lines
drivers/soc/qcom/soc_sleep_stats.c line 98
Quoted 5 lines
drivers/soc/qcom/soc_sleep_stats.c new file +5
-0+ +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.
Quoted 29 lines
Quoted 29 lines
drivers/soc/qcom/soc_sleep_stats.c new file +28
-0+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);
Quoted 8 lines
drivers/soc/qcom/soc_sleep_stats.c line 132
Quoted 6 lines
drivers/soc/qcom/soc_sleep_stats.c new file +6
-0+ 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.
Quoted 16 lines
Quoted 16 lines
drivers/soc/qcom/soc_sleep_stats.c new file +16
-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)
Quoted 8 lines
drivers/soc/qcom/soc_sleep_stats.c line 154
Quoted 6 lines
drivers/soc/qcom/soc_sleep_stats.c new file +6
-0+{ +int ret = -ENOMEM ; + + drv->kobj =kobject_create_and_add ("soc_sleep" , power_kobj); +if (!drv->kobj) + goto fail;Just return
-ENOMEMhere. It is really weird to make kobjects directly like this. How is userspace expected to use this?
Done. using debugfs.
Quoted 11 lines
drivers/soc/qcom/soc_sleep_stats.c line 163
Quoted 9 lines
drivers/soc/qcom/soc_sleep_stats.c new file +9
-0+ +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()?
Quoted 3 lines
drivers/soc/qcom/soc_sleep_stats.c line 165
Quoted 2 lines
drivers/soc/qcom/soc_sleep_stats.c new file +2
-0+ +platform_set_drvdata (pdev, drv);Do this
platform_set_drvdatain probe?
done.
Quoted 32 lines
Quoted 32 lines
drivers/soc/qcom/soc_sleep_stats.c new file +32
-0+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)
Quoted 8 lines
drivers/soc/qcom/soc_sleep_stats.c line 204
Quoted 7 lines
drivers/soc/qcom/soc_sleep_stats.c new file +7
-0+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.
Quoted 9 lines
drivers/soc/qcom/soc_sleep_stats.c line 211
Quoted 8 lines
drivers/soc/qcom/soc_sleep_stats.c new file +7
-0+ + 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.
Quoted 9 lines
drivers/soc/qcom/soc_sleep_stats.c line 218
Quoted 8 lines
drivers/soc/qcom/soc_sleep_stats.c new file +7
-0+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.
Quoted 6 lines
drivers/soc/qcom/soc_sleep_stats.c line 222
Quoted 5 lines
drivers/soc/qcom/soc_sleep_stats.c new file +4
-0+ + ret =soc_sleep_stats_create_sysfs (pdev, drv); +if (ret) +pr_info ("Failed to create sysfs interface\n" );Not
pr_err()? Ordev_err()?
done.
Quoted 4 lines
Quoted 4 lines
drivers/soc/qcom/soc_sleep_stats.c new file +4
-0+ +return ret; +} +
Quoted 8 lines
drivers/soc/qcom/soc_sleep_stats.c line 233
Quoted 7 lines
drivers/soc/qcom/soc_sleep_stats.c new file +7
-0+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.
Quoted 5 lines
Quoted 5 lines
drivers/soc/qcom/soc_sleep_stats.c new file +4
-0+ +return 0 ; +} +
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation