* [PATCH 0/2] Introduce SoC sleep stats driver @ 2019-08-08 6:12 Maulik Shah 2019-08-08 6:12 ` [PATCH 1/2] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs Maulik Shah 2019-08-08 6:12 ` [PATCH 2/2] drivers: qcom: Add SoC sleep stats driver Maulik Shah 0 siblings, 2 replies; 7+ messages in thread From: Maulik Shah @ 2019-08-08 6:12 UTC (permalink / raw) To: andy.gross, david.brown, linux-arm-msm Cc: linux-kernel, linux-pm, bjorn.andersson, evgreen, dianders, swboyd, rnayak, ilina, lsrao, mkshah 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs 2019-08-08 6:12 [PATCH 0/2] Introduce SoC sleep stats driver Maulik Shah @ 2019-08-08 6:12 ` Maulik Shah 2019-08-08 16:20 ` Stephen Boyd 2019-08-08 6:12 ` [PATCH 2/2] drivers: qcom: Add SoC sleep stats driver Maulik Shah 1 sibling, 1 reply; 7+ messages in thread From: Maulik Shah @ 2019-08-08 6:12 UTC (permalink / raw) To: andy.gross, david.brown, linux-arm-msm Cc: linux-kernel, linux-pm, bjorn.andersson, evgreen, dianders, swboyd, rnayak, ilina, lsrao, mkshah, devicetree, Mahesh Sivasubramanian 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. ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs 2019-08-08 6:12 ` [PATCH 1/2] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs Maulik Shah @ 2019-08-08 16:20 ` Stephen Boyd 2020-02-21 5:57 ` Maulik Shah 0 siblings, 1 reply; 7+ messages in thread From: Stephen Boyd @ 2019-08-08 16:20 UTC (permalink / raw) To: Maulik Shah, andy.gross, david.brown, linux-arm-msm Cc: linux-kernel, linux-pm, bjorn.andersson, evgreen, dianders, rnayak, ilina, lsrao, mkshah, devicetree, Mahesh Sivasubramanian 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. > + }; > + ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs 2019-08-08 16:20 ` Stephen Boyd @ 2020-02-21 5:57 ` Maulik Shah 0 siblings, 0 replies; 7+ messages in thread From: Maulik Shah @ 2020-02-21 5:57 UTC (permalink / raw) To: Stephen Boyd, andy.gross, david.brown, linux-arm-msm Cc: linux-kernel, linux-pm, bjorn.andersson, evgreen, dianders, rnayak, ilina, lsrao, devicetree, Mahesh Sivasubramanian 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] drivers: qcom: Add SoC sleep stats driver 2019-08-08 6:12 [PATCH 0/2] Introduce SoC sleep stats driver Maulik Shah 2019-08-08 6:12 ` [PATCH 1/2] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs Maulik Shah @ 2019-08-08 6:12 ` Maulik Shah 2019-08-08 18:40 ` Stephen Boyd 1 sibling, 1 reply; 7+ messages in thread From: Maulik Shah @ 2019-08-08 6:12 UTC (permalink / raw) To: andy.gross, david.brown, linux-arm-msm Cc: linux-kernel, linux-pm, bjorn.andersson, evgreen, dianders, swboyd, rnayak, ilina, lsrao, mkshah, Mahesh Sivasubramanian 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. ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drivers: qcom: Add SoC sleep stats driver 2019-08-08 6:12 ` [PATCH 2/2] drivers: qcom: Add SoC sleep stats driver Maulik Shah @ 2019-08-08 18:40 ` Stephen Boyd 2020-02-21 6:04 ` Maulik Shah 0 siblings, 1 reply; 7+ messages in thread From: Stephen Boyd @ 2019-08-08 18:40 UTC (permalink / raw) To: Maulik Shah, andy.gross, david.brown, linux-arm-msm Cc: linux-kernel, linux-pm, bjorn.andersson, evgreen, dianders, rnayak, ilina, lsrao, mkshah, Mahesh Sivasubramanian, Mark Rutland, Lorenzo Pieralisi 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; > +} > + ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] drivers: qcom: Add SoC sleep stats driver 2019-08-08 18:40 ` Stephen Boyd @ 2020-02-21 6:04 ` Maulik Shah 0 siblings, 0 replies; 7+ messages in thread From: Maulik Shah @ 2020-02-21 6:04 UTC (permalink / raw) To: Stephen Boyd, andy.gross, david.brown, linux-arm-msm Cc: linux-kernel, linux-pm, bjorn.andersson, evgreen, dianders, rnayak, ilina, lsrao, Mahesh Sivasubramanian, Mark Rutland, Lorenzo Pieralisi 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-02-21 6:05 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-08 6:12 [PATCH 0/2] Introduce SoC sleep stats driver Maulik Shah 2019-08-08 6:12 ` [PATCH 1/2] dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs Maulik Shah 2019-08-08 16:20 ` Stephen Boyd 2020-02-21 5:57 ` Maulik Shah 2019-08-08 6:12 ` [PATCH 2/2] drivers: qcom: Add SoC sleep stats driver Maulik Shah 2019-08-08 18:40 ` Stephen Boyd 2020-02-21 6:04 ` Maulik Shah
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).