Introduce SoC sleep stats driver
Subsystems (4)
v3 cover Introduce SoC sleep stats driver
Changes in v3:
- Address stephen's comments from v2 in change 1 and 2.
- Address bjorn's comments from v2 in change 3 and 4.
- Add Rob and bjorn's Reviewed-by on YAML change.
Changes in v2:
- Convert Documentation to YAML.
- Address stephen's comments from v1.
- Use debugfs instead of sysfs.
- Add sc7180 dts changes for sleep stats
- Add defconfig changes to enable driver
- Include subsystem stats from [1] in this single stats driver.
- Address stephen's comments from [1]
- Update cover letter inline to mention [1]
Qualcomm Technologies, Inc. (QTI)'s 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 rails and the oscillator clock.
Additionally multiple subsystems present on SoC like modem, spss, adsp, cdsp maintains their low power mode statistics in shared memory (SMEM).
Statistics includes SoC sleep mode type, number of times LPM entered, time of last entry, exit, and accumulated sleep duration in seconds.
This series adds a driver to read the stats and export to debugfs.
[1] https://lore.kernel.org/patchwork/patch/1149381/
Mahesh Sivasubramanian (2):
dt-bindings: Introduce soc sleep stats bindings for Qualcomm SoCs
soc: qcom: Add SoC sleep stats driver
Maulik Shah (2):
arm64: dts: qcom: sc7180: Enable soc sleep stats
arm64: defconfig: Enable SoC sleep stats driver for Qualcomm
.../bindings/soc/qcom/soc-sleep-stats.yaml | 47 ++++ arch/arm64/boot/dts/qcom/sc7180.dtsi | 5 + arch/arm64/configs/defconfig | 1 + drivers/soc/qcom/Kconfig | 10 + drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/soc_sleep_stats.c | 279 +++++++++++++++++++++ 6 files changed, 343 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml create mode 100644 drivers/soc/qcom/soc_sleep_stats.c
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH v3 1/4 dt-bindings: Introduce SoC sleep stats bindings
From: Mahesh Sivasubramanian <[email protected]>
Add device binding documentation for Qualcomm Technologies, Inc. (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.yaml | 46 ++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml
Documentation/devicetree/bindings/soc/qcom/soc-sleep-stats.yaml new file +46 -0
@@ -0,0 +1,46 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/soc/qcom/soc-sleep-stats.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Technologies, Inc. (QTI) SoC sleep stats bindings + +maintainers: + - Maulik Shah <[email protected]> + - Lina Iyer <[email protected]> + +description: + Always On Processor/Resource Power Manager maintains statistics of the SoC + sleep modes involving powering down of the rails and oscillator clock. + + Statistics includes SoC sleep mode type, number of times low power mode were + entered, time of last entry, time of last exit and accumulated sleep duration. + +properties: + compatible: + enum: + - qcom,rpmh-sleep-stats + - qcom,rpm-sleep-stats + + reg: + maxItems: 1 + +required: + - compatible + - reg + +examples: + # Example of rpmh sleep stats + - | + rpmh_sleep_stats@c3f0000 { + compatible = "qcom,rpmh-sleep-stats"; + reg = <0 0xc3f0000 0 0x400>; + }; + # Example of rpm sleep stats + - | + rpm_sleep_stats@4690000 { + compatible = "qcom,rpm-sleep-stats"; + reg = <0 0x04690000 0 0x400>; + }; +...
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH v3 2/4 soc: qcom: Add SoC sleep stats driver
In reply to Maulik ShahFrom: Mahesh Sivasubramanian <[email protected]>
Let's add a driver to read the the stats from remote processor and export to debugfs.
The driver creates "soc_sleep_stats" directory in debugfs and adds files for various low power mode available. Below is sample output with command
cat /sys/kernel/debug/soc_sleep_stats/ddr
count = 0
Last Entered At = 0
Last Exited At = 0
Accumulated Duration = 0
--- drivers/soc/qcom/Kconfig | 9 ++ drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/soc_sleep_stats.c | 248 +++++++++++++++++++++++++++++++++++++ 3 files changed, 258 insertions(+) create mode 100644 drivers/soc/qcom/soc_sleep_stats.c
drivers/soc/qcom/Kconfig +9 -0
@@ -185,6 +185,15 @@ config QCOM_SOCINFO Say yes here to support the Qualcomm socinfo driver, providing information about the SoC to user space. +config QCOM_SOC_SLEEP_STATS + tristate "Qualcomm Technologies, Inc. (QTI) SoC sleep stats driver" + depends on ARCH_QCOM && DEBUG_FS || COMPILE_TEST + 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 debugfs + interface. + config QCOM_WCNSS_CTRL tristate "Qualcomm WCNSS control driver" depends on ARCH_QCOM || COMPILE_TEST
drivers/soc/qcom/Makefile +1 -0
@@ -20,6 +20,7 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o obj-$(CONFIG_QCOM_SMP2P) += smp2p.o obj-$(CONFIG_QCOM_SMSM) += smsm.o obj-$(CONFIG_QCOM_SOCINFO) += socinfo.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-qcom.o
drivers/soc/qcom/soc_sleep_stats.c new file +248 -0
248 lines not on this page · raw message
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v3 2/4] soc: qcom: Add SoC sleep stats driver
Quoted 9 lines · Maulik Shah
From: Mahesh Sivasubramanian <[email protected]>
Let's add a driver to read the the stats from remote processor and export to debugfs.
The driver creates "
soc_sleep_stats" directory in debugfs and adds files for various low power mode available. Below is sample output with command
Given that this doesn't provide some generic stats for SoCs, how about naming the whole thing qcom_sleep_stats instead?
Quoted 10 lines
cat
/sys/kernel/debug/soc_sleep_stats/ddr
count = 0
Last Entered At = 0
Last Exited At = 0
Accumulated Duration = 0Signed-off-by Mahesh Sivasubramanian <[email protected]>Signed-off-by Lina Iyer <[email protected]>Signed-off-by Maulik Shah <[email protected]>---
Please include a changelog here.
Quoted 56 lines
drivers/soc/qcom/Kconfig | 9 ++ drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/soc_sleep_stats.c | 248 +++++++++++++++++++++++++++++++++++++ 3 files changed, 258 insertions(+) create mode 100644 drivers/soc/qcom/soc_sleep_stats.cdrivers/soc/qcom/Kconfig +9
-0@@ -185,6 +185,15 @@ config QCOM_SOCINFO Say yes here to support the Qualcomm socinfo driver, providing information about the SoC to user space. +config QCOM_SOC_SLEEP_STATS + tristate "Qualcomm Technologies, Inc. (QTI) SoC sleep stats driver" + depends on ARCH_QCOM && DEBUG_FS || COMPILE_TEST + 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 debugfs + interface. + config QCOM_WCNSS_CTRL tristate "Qualcomm WCNSS control driver" depends on ARCH_QCOM || COMPILE_TESTdrivers/soc/qcom/Makefile +1
-0@@ -20,6 +20,7 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o obj-$(CONFIG_QCOM_SMP2P) += smp2p.o obj-$(CONFIG_QCOM_SMSM) += smsm.o obj-$(CONFIG_QCOM_SOCINFO) += socinfo.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-qcom.odrivers/soc/qcom/soc_sleep_stats.c new file +12
-0@@ -0,0 +1,248 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* +* Copyright (c) 2011-2020, The Linux Foundation. All rights reserved. +*/ + +#include <linux/debugfs.h> +#include <linux/device.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/seq_file.h>
drivers/soc/qcom/soc_sleep_stats.c line 22
Quoted 10 lines
drivers/soc/qcom/soc_sleep_stats.c new file +10
-0+ +#include <linux/soc/qcom/smem.h> +#include <clocksource/arm_arch_timer.h> + +#define MODEM_ITEM_ID 605 +#define ADSP_ITEM_ID 606 +#define CDSP_ITEM_ID 607 +#define SLPI_ITEM_ID 608 +#define GPU_ITEM_ID 609 +#define DISPLAY_ITEM_ID 610
I don't think these defines adds any value, just plug in the numbers in the table directly.
drivers/soc/qcom/soc_sleep_stats.c line 28
Quoted 6 lines
drivers/soc/qcom/soc_sleep_stats.c new file +6
-0+ +#define PID_APSS 0 +#define PID_MODEM 1 +#define PID_ADSP 2 +#define PID_SLPI 3 +#define PID_CDSP 5
Ditto.
drivers/soc/qcom/soc_sleep_stats.c line 30
Quoted 2 lines
drivers/soc/qcom/soc_sleep_stats.c new file +2
-0+ +#define NAME_LENGTH 5
As I suggested below, this isn't 5 - it's sizeof(u32) + 1, and as such it doesn't add value to carry it as a define that doesn't fit on the same screen when reading the code below.
drivers/soc/qcom/soc_sleep_stats.c line 41
Quoted 11 lines
drivers/soc/qcom/soc_sleep_stats.c new file +11
-0+ +#define STAT_TYPE_ADDR 0x0 +#define COUNT_ADDR 0x4 +#define LAST_ENTERED_AT_ADDR 0x8 +#define LAST_EXITED_AT_ADDR 0x10 +#define ACCUMULATED_ADDR 0x18 +#define CLIENT_VOTES_ADDR 0x1c + +struct subsystem_data { +const char *name; +u32 item_id;
"smem_item"
Quoted 9 lines
drivers/soc/qcom/soc_sleep_stats.c new file +9
-0+u32 pid; +}; + +static struct subsystem_data subsystems[] = { + {"modem" ,MODEM_ITEM_ID ,PID_MODEM }, + {"adsp" ,ADSP_ITEM_ID ,PID_ADSP }, + {"cdsp" ,CDSP_ITEM_ID ,PID_CDSP }, + {"slpi" ,SLPI_ITEM_ID ,PID_SLPI }, + {"gpu" ,GPU_ITEM_ID ,PID_APSS },
drivers/soc/qcom/soc_sleep_stats.c line 60
Quoted 10 lines
drivers/soc/qcom/soc_sleep_stats.c new file +10
-0+ { "display", DISPLAY_ITEM_ID, PID_APSS }, +}; + +struct stats_config { +unsigned int offset_addr; +unsigned int num_records; +bool appended_stats_avail; +}; + +static const struct stats_config *config;
Add this to soc_sleep_stats_data and pass that as s->private instead of just the reg, to avoid the global variable.
drivers/soc/qcom/soc_sleep_stats.c line 63
Quoted 3 lines
drivers/soc/qcom/soc_sleep_stats.c new file +3
-0+ +struct soc_sleep_stats_data { +phys_addr_t stats_base;
This can be a local variable in soc_sleep_stats_probe()
drivers/soc/qcom/soc_sleep_stats.c line 64
Quoted 1 line
drivers/soc/qcom/soc_sleep_stats.c new file +1
-0+resource_size_t stats_size;
Ditto.
drivers/soc/qcom/soc_sleep_stats.c line 69
Quoted 5 lines
drivers/soc/qcom/soc_sleep_stats.c new file +5
-0+void __iomem *reg; +struct dentry *root; +}; + +struct entry {
Can you please give this struct a slightly less generic name?
Quoted 26 lines
drivers/soc/qcom/soc_sleep_stats.c new file +26
-0+u32 stat_type; +u32 count; +u64 last_entered_at; +u64 last_exited_at; +u64 accumulated; +}; + +struct appended_entry { +u32 client_votes; +u32 reserved[3 ]; +}; + +static void print_sleep_stats (struct seq_file *s,struct entry *e) +{ +seq_printf (s,"Count = %u\n" , e->count); +seq_printf (s,"Last Entered At = %llu\n" , e->last_entered_at); +seq_printf (s,"Last Exited At = %llu\n" , e->last_exited_at); +seq_printf (s,"Accumulated Duration = %llu\n" , e->accumulated); +} + +static int subsystem_sleep_stats_show (struct seq_file *s,void *d) +{ +struct subsystem_data *subsystem = s->private; +struct entry *e; + + e =qcom_smem_get (subsystem->pid, subsystem->item_id,NULL );
drivers/soc/qcom/soc_sleep_stats.c line 105
Quoted 10 lines
drivers/soc/qcom/soc_sleep_stats.c new file +10
-0+if (IS_ERR (e)) +return PTR_ERR (e); + +/* +* If a subsystem is in sleep when reading the sleep stats adjust +* the accumulated sleep duration to show actual sleep time. +*/ +if (e->last_entered_at > e->last_exited_at) + e->accumulated += (arch_timer_read_counter () + - e->last_entered_at);
How is this supposed to work? "e" is a pointer to the actual data in smem, so every time "subsystem_sleep_stats" is shown we'll add the delta since last_entered_at to a persistent variable?
Won't this make the "accumulated" value depend on the number of times you have read the data?
I'm guessing that you're actually looking for:
accumulated + now() - last_entered_at
In which case, making a stack-local copy of the data in e and then doing this addition should provide the result you're looking for.
drivers/soc/qcom/soc_sleep_stats.c line 117
Quoted 12 lines
drivers/soc/qcom/soc_sleep_stats.c new file +12
-0+ +print_sleep_stats (s, e); + +return 0 ; +} + +static int soc_sleep_stats_show (struct seq_file *s,void *d) +{ +void __iomem *reg = s->private; +struct entry e; + + e.count =readl_relaxed (reg +COUNT_ADDR );
No need to "_relax" these reads.
Quoted 13 lines
drivers/soc/qcom/soc_sleep_stats.c new file +13
-0+ e.last_entered_at =readq_relaxed (reg +LAST_ENTERED_AT_ADDR ); + e.last_exited_at =readq_relaxed (reg +LAST_EXITED_AT_ADDR ); + e.accumulated =readq_relaxed (reg +ACCUMULATED_ADDR ); + +print_sleep_stats (s, &e); + +if (config->appended_stats_avail) { +struct appended_entry ae; + + ae.client_votes =readl_relaxed (reg +CLIENT_VOTES_ADDR ); +seq_printf (s,"Client_votes = %#x\n" , ae.client_votes); + } +
drivers/soc/qcom/soc_sleep_stats.c line 140
Quoted 10 lines
drivers/soc/qcom/soc_sleep_stats.c new file +10
-0+return 0 ; +} + +DEFINE_SHOW_ATTRIBUTE (soc_sleep_stats); +DEFINE_SHOW_ATTRIBUTE (subsystem_sleep_stats); + +static void create_debugfs_entries (struct soc_sleep_stats_data *drv) +{ +struct entry *e; +char stat_type[NAME_LENGTH ] = {0 };
NAME_LENGTH is sizeof(u32) + 1 and will never change, so I think the use of a define only serves to obfuscate things.
drivers/soc/qcom/soc_sleep_stats.c line 152
Quoted 12 lines
drivers/soc/qcom/soc_sleep_stats.c new file +12
-0+u32 offset, type; +int i; + + drv->root =debugfs_create_dir ("soc_sleep_stats" ,NULL ); + +for (i =0 ; i < config->num_records; i++) { + offset =STAT_TYPE_ADDR + (i *sizeof (struct entry )); + +if (config->appended_stats_avail) + offset += i *sizeof (struct appended_entry ); + + type =le32_to_cpu (readl_relaxed (drv->reg + offset));
readl_relaxed() is already CPU endian, so this isn't right.
And please drop the "_relaxed".
drivers/soc/qcom/soc_sleep_stats.c line 161
Quoted 9 lines
drivers/soc/qcom/soc_sleep_stats.c new file +9
-0+memcpy (stat_type, &type,sizeof (u32)); +debugfs_create_file (stat_type,0444 , drv->root, + drv->reg + offset, + &soc_sleep_stats_fops); + } + +for (i =0 ; i <ARRAY_SIZE (subsystems); i++) { + e =qcom_smem_get (subsystems[i].pid, subsystems[i].item_id, +NULL );
Feel free to ignore the 80-char "limit", and drop the empty line before the check below.
Quoted 21 lines
drivers/soc/qcom/soc_sleep_stats.c new file +21
-0+ +if (IS_ERR (e)) +continue ; + +debugfs_create_file (subsystems[i].name,0444 , drv->root, + &subsystems[i], + &subsystem_sleep_stats_fops); + } +} + +static int soc_sleep_stats_probe (struct platform_device *pdev) +{ +struct soc_sleep_stats_data *drv; +struct resource *res; +void __iomem *offset_addr; + + drv =devm_kzalloc (&pdev->dev,sizeof (*drv),GFP_KERNEL ); +if (!drv) +return -ENOMEM ; + + config =device_get_match_data (&pdev->dev);
drivers/soc/qcom/soc_sleep_stats.c line 192
Quoted 10 lines
drivers/soc/qcom/soc_sleep_stats.c new file +10
-0+if (!config) +return -ENODEV ; + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!res) +return PTR_ERR (res); + + offset_addr =devm_ioremap (&pdev->dev, + res->start + config->offset_addr, +sizeof (u32));
No need to devm this.
drivers/soc/qcom/soc_sleep_stats.c line 197
Quoted 5 lines
drivers/soc/qcom/soc_sleep_stats.c new file +5
-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);
As stated above, these can be local variables...
Quoted 26 lines
drivers/soc/qcom/soc_sleep_stats.c new file +26
-0+devm_iounmap (&pdev->dev, offset_addr); + + drv->reg =devm_ioremap (&pdev->dev, drv->stats_base, drv->stats_size); +if (!drv->reg) +return -ENOMEM ; + +create_debugfs_entries (drv); +platform_set_drvdata (pdev, drv); + +return 0 ; +} + +static int soc_sleep_stats_remove (struct platform_device *pdev) +{ +struct soc_sleep_stats_data *drv =platform_get_drvdata (pdev); + +debugfs_remove_recursive (drv->root); + +return 0 ; +} + +static const struct stats_config rpm_data = { + .offset_addr =0x14 , + .num_records =2 , + .appended_stats_avail = true, +};
drivers/soc/qcom/soc_sleep_stats.c line 233
Quoted 10 lines
drivers/soc/qcom/soc_sleep_stats.c new file +10
-0+ +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},
Please add an extra space between data and '}' in these two.
Thanks,
Bjorn
Quoted 18 lines
drivers/soc/qcom/soc_sleep_stats.c new file +15
-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. (QTI) SoC Sleep Stats driver" ); +MODULE_LICENSE ("GPL v2" );Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v3 2/4] soc: qcom: Add SoC sleep stats driver
Quoted 7 lines · Bjorn Andersson
Quoted 5 lines · Maulik Shah
From: Mahesh Sivasubramanian <[email protected]>
Let's add a driver to read the the stats from remote processor and export to debugfs.
Quoted 6 lines
Quoted 4 lines
The driver creates "
soc_sleep_stats" directory in debugfs and adds files for various low power mode available. Below is sample output with commandGiven that this doesn't provide some generic stats for SoCs, how about naming the whole thing
qcom_sleep_statsinstead?
Done. Updated to create "qcom_sleep_stats" directory.
Quoted 11 lines
Quoted 10 lines
cat
/sys/kernel/debug/soc_sleep_stats/ddr
count = 0
Last Entered At = 0
Last Exited At = 0
Accumulated Duration = 0Signed-off-by Mahesh Sivasubramanian <[email protected]>Signed-off-by Lina Iyer <[email protected]>Signed-off-by Maulik Shah <[email protected]>---
Please include a changelog here.
Done.
Quoted 70 lines
Quoted 57 lines
drivers/soc/qcom/Kconfig | 9 ++ drivers/soc/qcom/Makefile | 1 + drivers/soc/qcom/soc_sleep_stats.c | 248 +++++++++++++++++++++++++++++++++++++ 3 files changed, 258 insertions(+) create mode 100644 drivers/soc/qcom/soc_sleep_stats.cdrivers/soc/qcom/Kconfig +9
-0@@ -185,6 +185,15 @@ config QCOM_SOCINFO Say yes here to support the Qualcomm socinfo driver, providing information about the SoC to user space. +config QCOM_SOC_SLEEP_STATS + tristate "Qualcomm Technologies, Inc. (QTI) SoC sleep stats driver" + depends on ARCH_QCOM && DEBUG_FS || COMPILE_TEST + 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 debugfs + interface. + config QCOM_WCNSS_CTRL tristate "Qualcomm WCNSS control driver" depends on ARCH_QCOM || COMPILE_TESTdrivers/soc/qcom/Makefile +1
-0@@ -20,6 +20,7 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o obj-$(CONFIG_QCOM_SMP2P) += smp2p.o obj-$(CONFIG_QCOM_SMSM) += smsm.o obj-$(CONFIG_QCOM_SOCINFO) += socinfo.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-qcom.odrivers/soc/qcom/soc_sleep_stats.c new file +12
-0@@ -0,0 +1,248 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* +* Copyright (c) 2011-2020, The Linux Foundation. All rights reserved. +*/ + +#include <linux/debugfs.h> +#include <linux/device.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/seq_file.h> drivers/soc/qcom/soc_sleep_stats.c line 22
Quoted 10 lines
drivers/soc/qcom/soc_sleep_stats.c new file +10
-0+ +#include <linux/soc/qcom/smem.h> +#include <clocksource/arm_arch_timer.h> + +#define MODEM_ITEM_ID 605 +#define ADSP_ITEM_ID 606 +#define CDSP_ITEM_ID 607 +#define SLPI_ITEM_ID 608 +#define GPU_ITEM_ID 609 +#define DISPLAY_ITEM_ID 610I don't think these defines adds any value, just plug in the numbers in the table directly.
Quoted 7 lines
drivers/soc/qcom/soc_sleep_stats.c line 28
Quoted 6 lines
drivers/soc/qcom/soc_sleep_stats.c new file +6
-0+ +#define PID_APSS 0 +#define PID_MODEM 1 +#define PID_ADSP 2 +#define PID_SLPI 3 +#define PID_CDSP 5Ditto.
Done.
Quoted 6 lines
drivers/soc/qcom/soc_sleep_stats.c line 30
Quoted 3 lines
drivers/soc/qcom/soc_sleep_stats.c new file +2
-0+ +#define NAME_LENGTH 5As I suggested below, this isn't 5 - it's sizeof(u32) + 1, and as such it doesn't add value to carry it as a define that doesn't fit on the same screen when reading the code below.
Done.
Quoted 4 lines
Quoted 4 lines
drivers/soc/qcom/soc_sleep_stats.c new file +4
-0+ +#define STAT_TYPE_ADDR 0x0 +#define COUNT_ADDR 0x4 +#define LAST_ENTERED_AT_ADDR 0x8
Quoted 8 lines
drivers/soc/qcom/soc_sleep_stats.c line 41
Quoted 7 lines
drivers/soc/qcom/soc_sleep_stats.c new file +7
-0+#define LAST_EXITED_AT_ADDR 0x10 +#define ACCUMULATED_ADDR 0x18 +#define CLIENT_VOTES_ADDR 0x1c + +struct subsystem_data { +const char *name; +u32 item_id;"
smem_item"
Done.
Quoted 13 lines
Quoted 13 lines
drivers/soc/qcom/soc_sleep_stats.c new file +13
-0+u32 pid; +}; + +static struct subsystem_data subsystems[] = { + {"modem" ,MODEM_ITEM_ID ,PID_MODEM }, + {"adsp" ,ADSP_ITEM_ID ,PID_ADSP }, + {"cdsp" ,CDSP_ITEM_ID ,PID_CDSP }, + {"slpi" ,SLPI_ITEM_ID ,PID_SLPI }, + {"gpu" ,GPU_ITEM_ID ,PID_APSS }, + {"display" ,DISPLAY_ITEM_ID ,PID_APSS }, +}; + +struct stats_config {
Quoted 8 lines
drivers/soc/qcom/soc_sleep_stats.c line 60
Quoted 6 lines
drivers/soc/qcom/soc_sleep_stats.c new file +6
-0+unsigned int offset_addr; +unsigned int num_records; +bool appended_stats_avail; +}; + +static const struct stats_config *config;Add this to
soc_sleep_stats_dataand pass that ass->privateinstead of just the reg, to avoid the global variable.
No, this is required to keep global. we are not passing just reg as s->private, we are passing "reg + offset" which differs for each stat.
Quoted 7 lines
drivers/soc/qcom/soc_sleep_stats.c line 63
Quoted 3 lines
drivers/soc/qcom/soc_sleep_stats.c new file +3
-0+ +struct soc_sleep_stats_data { +phys_addr_t stats_base;This can be a local variable in
soc_sleep_stats_probe()drivers/soc/qcom/soc_sleep_stats.c line 64
Quoted 1 line
drivers/soc/qcom/soc_sleep_stats.c new file +1
-0+resource_size_t stats_size;Ditto.
Done.
Quoted 7 lines
drivers/soc/qcom/soc_sleep_stats.c line 69
Quoted 6 lines
drivers/soc/qcom/soc_sleep_stats.c new file +5
-0+void __iomem *reg; +struct dentry *root; +}; + +struct entry {Can you please give this struct a slightly less generic name?
Done.
Quoted 44 lines
Quoted 27 lines
drivers/soc/qcom/soc_sleep_stats.c new file +26
-0+u32 stat_type; +u32 count; +u64 last_entered_at; +u64 last_exited_at; +u64 accumulated; +}; + +struct appended_entry { +u32 client_votes; +u32 reserved[3 ]; +}; + +static void print_sleep_stats (struct seq_file *s,struct entry *e) +{ +seq_printf (s,"Count = %u\n" , e->count); +seq_printf (s,"Last Entered At = %llu\n" , e->last_entered_at); +seq_printf (s,"Last Exited At = %llu\n" , e->last_exited_at); +seq_printf (s,"Accumulated Duration = %llu\n" , e->accumulated); +} + +static int subsystem_sleep_stats_show (struct seq_file *s,void *d) +{ +struct subsystem_data *subsystem = s->private; +struct entry *e; + + e =qcom_smem_get (subsystem->pid, subsystem->item_id,NULL );drivers/soc/qcom/soc_sleep_stats.c line 105
Quoted 10 lines
drivers/soc/qcom/soc_sleep_stats.c new file +10
-0+if (IS_ERR (e)) +return PTR_ERR (e); + +/* +* If a subsystem is in sleep when reading the sleep stats adjust +* the accumulated sleep duration to show actual sleep time. +*/ +if (e->last_entered_at > e->last_exited_at) + e->accumulated += (arch_timer_read_counter () + - e->last_entered_at);How is this supposed to work? "e" is a pointer to the actual data in smem, so every time "
subsystem_sleep_stats" is shown we'll add the delta sincelast_entered_atto a persistent variable?Won't this make the "accumulated" value depend on the number of times you have read the data?
Quoted 6 lines
I'm guessing that you're actually looking for:
accumulated +
now()-last_entered_atIn which case, making a stack-local copy of the data in e and then doing this addition should provide the result you're looking for.
Although accumulated will updated again when subsystem updates this entry. it will be impacted when subsystem is in sleep and repeated read is done for the stats.
Thanks for pointing this. Done.
Quoted 14 lines
drivers/soc/qcom/soc_sleep_stats.c line 117
Quoted 13 lines
drivers/soc/qcom/soc_sleep_stats.c new file +12
-0+ +print_sleep_stats (s, e); + +return 0 ; +} + +static int soc_sleep_stats_show (struct seq_file *s,void *d) +{ +void __iomem *reg = s->private; +struct entry e; + + e.count =readl_relaxed (reg +COUNT_ADDR );No need to "
_relax" these reads.
Done.
Quoted 18 lines
Quoted 18 lines
drivers/soc/qcom/soc_sleep_stats.c new file +17
-0+ e.last_entered_at =readq_relaxed (reg +LAST_ENTERED_AT_ADDR ); + e.last_exited_at =readq_relaxed (reg +LAST_EXITED_AT_ADDR ); + e.accumulated =readq_relaxed (reg +ACCUMULATED_ADDR ); + +print_sleep_stats (s, &e); + +if (config->appended_stats_avail) { +struct appended_entry ae; + + ae.client_votes =readl_relaxed (reg +CLIENT_VOTES_ADDR ); +seq_printf (s,"Client_votes = %#x\n" , ae.client_votes); + } + +return 0 ; +} + +DEFINE_SHOW_ATTRIBUTE (soc_sleep_stats);
Quoted 8 lines
drivers/soc/qcom/soc_sleep_stats.c line 140
Quoted 6 lines
drivers/soc/qcom/soc_sleep_stats.c new file +6
-0+DEFINE_SHOW_ATTRIBUTE (subsystem_sleep_stats); + +static void create_debugfs_entries (struct soc_sleep_stats_data *drv) +{ +struct entry *e; +char stat_type[NAME_LENGTH ] = {0 };
NAME_LENGTHis sizeof(u32) + 1 and will never change, so I think the use of a define only serves to obfuscate things.
Done.
Quoted 15 lines
drivers/soc/qcom/soc_sleep_stats.c line 152
Quoted 12 lines
drivers/soc/qcom/soc_sleep_stats.c new file +12
-0+u32 offset, type; +int i; + + drv->root =debugfs_create_dir ("soc_sleep_stats" ,NULL ); + +for (i =0 ; i < config->num_records; i++) { + offset =STAT_TYPE_ADDR + (i *sizeof (struct entry )); + +if (config->appended_stats_avail) + offset += i *sizeof (struct appended_entry ); + + type =le32_to_cpu (readl_relaxed (drv->reg + offset));
readl_relaxed()is already CPU endian, so this isn't right.
Quoted 1 line
And please drop the "
_relaxed".
Done.
Quoted 12 lines
drivers/soc/qcom/soc_sleep_stats.c line 161
Quoted 10 lines
drivers/soc/qcom/soc_sleep_stats.c new file +9
-0+memcpy (stat_type, &type,sizeof (u32)); +debugfs_create_file (stat_type,0444 , drv->root, + drv->reg + offset, + &soc_sleep_stats_fops); + } + +for (i =0 ; i <ARRAY_SIZE (subsystems); i++) { + e =qcom_smem_get (subsystems[i].pid, subsystems[i].item_id, +NULL );Feel free to ignore the 80-char "limit", and drop the empty line before the check below.
Done.
Quoted 24 lines
Quoted 24 lines
drivers/soc/qcom/soc_sleep_stats.c new file +24
-0+ +if (IS_ERR (e)) +continue ; + +debugfs_create_file (subsystems[i].name,0444 , drv->root, + &subsystems[i], + &subsystem_sleep_stats_fops); + } +} + +static int soc_sleep_stats_probe (struct platform_device *pdev) +{ +struct soc_sleep_stats_data *drv; +struct resource *res; +void __iomem *offset_addr; + + drv =devm_kzalloc (&pdev->dev,sizeof (*drv),GFP_KERNEL ); +if (!drv) +return -ENOMEM ; + + config =device_get_match_data (&pdev->dev); +if (!config) +return -ENODEV ; +
Quoted 8 lines
drivers/soc/qcom/soc_sleep_stats.c line 192
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 =devm_ioremap (&pdev->dev, + res->start + config->offset_addr, +sizeof (u32));No need to devm this.
Done.
Quoted 7 lines
drivers/soc/qcom/soc_sleep_stats.c line 197
Quoted 6 lines
drivers/soc/qcom/soc_sleep_stats.c new file +5
-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);As stated above, these can be local variables...
Done.
Quoted 39 lines
Quoted 27 lines
drivers/soc/qcom/soc_sleep_stats.c new file +26
-0+devm_iounmap (&pdev->dev, offset_addr); + + drv->reg =devm_ioremap (&pdev->dev, drv->stats_base, drv->stats_size); +if (!drv->reg) +return -ENOMEM ; + +create_debugfs_entries (drv); +platform_set_drvdata (pdev, drv); + +return 0 ; +} + +static int soc_sleep_stats_remove (struct platform_device *pdev) +{ +struct soc_sleep_stats_data *drv =platform_get_drvdata (pdev); + +debugfs_remove_recursive (drv->root); + +return 0 ; +} + +static const struct stats_config rpm_data = { + .offset_addr =0x14 , + .num_records =2 , + .appended_stats_avail = true, +};drivers/soc/qcom/soc_sleep_stats.c line 233
Quoted 10 lines
drivers/soc/qcom/soc_sleep_stats.c new file +10
-0+ +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},Please add an extra space between data and '}' in these two.
Quoted 2 lines
Thanks,
Bjorn
Done.
Thanks,
Maulik
Quoted 19 lines
Quoted 19 lines
drivers/soc/qcom/soc_sleep_stats.c new file +15
-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. (QTI) SoC Sleep Stats driver" ); +MODULE_LICENSE ("GPL v2" );Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v3 2/4] soc: qcom: Add SoC sleep stats driver
Quoting Maulik Shah (2020-03-09 03:58:27)
Quoted 9 lines · Bjorn Andersson
Quoted 7 lines · Maulik Shah
Quoted 6 lines
drivers/soc/qcom/soc_sleep_stats.c new file +0
-0@@ -0,0 +1,248 @@
[...]
Quoted 22 lines
Quoted 13 lines
Quoted 13 lines
drivers/soc/qcom/soc_sleep_stats.c new file +13
-0+u32 pid; +}; + +static struct subsystem_data subsystems[] = { + {"modem" ,MODEM_ITEM_ID ,PID_MODEM }, + {"adsp" ,ADSP_ITEM_ID ,PID_ADSP }, + {"cdsp" ,CDSP_ITEM_ID ,PID_CDSP }, + {"slpi" ,SLPI_ITEM_ID ,PID_SLPI }, + {"gpu" ,GPU_ITEM_ID ,PID_APSS }, + {"display" ,DISPLAY_ITEM_ID ,PID_APSS }, +}; + +struct stats_config {Quoted 8 lines
drivers/soc/qcom/soc_sleep_stats.c line 60
Quoted 6 lines
drivers/soc/qcom/soc_sleep_stats.c new file +6
-0+unsigned int offset_addr; +unsigned int num_records; +bool appended_stats_avail; +}; + +static const struct stats_config *config;Add this to
soc_sleep_stats_dataand pass that ass->privateinstead of just the reg, to avoid the global variable.
Quoted 2 lines
No, this is required to keep global. we are not passing just reg as
s->private, we are passing "reg + offset" which differs for each stat.
Can you please stop sending these review comment replies and then immediately turning around and sending the next revision of the patch series. I doubt that the changes take less than an hour to write and it would be helpful for everyone involved to have constructive discussions about the code if there's ever a response besides "done" or "ok".
In this case it should be possible to get rid of the global 'config'. Make a new container struct to hold the config and offset or figure out what actually needs to be passed into the functions instead and do that when the files are created.
Re: [PATCH v3 2/4] soc: qcom: Add SoC sleep stats driver
Quoted 31 lines · Stephen Boyd
Quoting Maulik Shah (2020-03-09 03:58:27)
Quoted 8 lines · Bjorn Andersson
Quoted 7 lines · Maulik Shah
Quoted 6 lines
drivers/soc/qcom/soc_sleep_stats.c new file +0
-0@@ -0,0 +1,248 @@[...]
Quoted 21 lines
Quoted 21 lines
Quoted 9 lines
drivers/soc/qcom/soc_sleep_stats.c new file +9
-0+u32 pid; +}; + +static struct subsystem_data subsystems[] = { + {"modem" ,MODEM_ITEM_ID ,PID_MODEM }, + {"adsp" ,ADSP_ITEM_ID ,PID_ADSP }, + {"cdsp" ,CDSP_ITEM_ID ,PID_CDSP }, + {"slpi" ,SLPI_ITEM_ID ,PID_SLPI }, + {"gpu" ,GPU_ITEM_ID ,PID_APSS },drivers/soc/qcom/soc_sleep_stats.c line 60
Quoted 10 lines
drivers/soc/qcom/soc_sleep_stats.c new file +10
-0+ { "display", DISPLAY_ITEM_ID, PID_APSS }, +}; + +struct stats_config { +unsigned int offset_addr; +unsigned int num_records; +bool appended_stats_avail; +}; + +static const struct stats_config *config;Add this to
soc_sleep_stats_dataand pass that ass->privateinstead of just the reg, to avoid the global variable.
Quoted 7 lines
Quoted 2 lines
No, this is required to keep global. we are not passing just reg as
s->private, we are passing "reg + offset" which differs for each stat.Can you please stop sending these review comment replies and then immediately turning around and sending the next revision of the patch series. I doubt that the changes take less than an hour to write and it would be helpful for everyone involved to have constructive discussions about the code if there's ever a response besides "done" or "ok".
Sure, i be careful to wait for at least few days before spinning new one, atleast wait till all discussion finish on current version.
Also will try to reply in more details for comments as much as possible.
Quoted 5 lines
In this case it should be possible to get rid of the global 'config'. Make a new container struct to hold the config and offset or figure out what actually needs to be passed into the functions instead and do that when the files are created.
i will address this to get rid of global config.
Thanks,
Maulik
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 v3 3/4 arm64: dts: qcom: sc7180: Enable SoC sleep stats
In reply to Maulik ShahAdd device node for SoC sleep stats driver which provides various low power mode stats.
Also update the reg size of aoss_qmp device to 0x400.
--- arch/arm64/boot/dts/qcom/sc7180.dtsi | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
arch/arm64/boot/dts/qcom/sc7180.dtsi +6 -1
@@ -1610,7 +1610,7 @@ aoss_qmp: qmp@c300000 { compatible = "qcom,sc7180-aoss-qmp"; - reg = <0 0x0c300000 0 0x100000>; + reg = <0 0x0c300000 0 0x400>; interrupts = <GIC_SPI 389 IRQ_TYPE_EDGE_RISING>; mboxes = <&apss_shared 0>;
@@ -1618,6 +1618,11 @@ #power-domain-cells = <1>; }; + rpmh_sleep_stats@c3f0000 { + compatible = "qcom,rpmh-sleep-stats"; + reg = <0 0x0c3f0000 0 0x400>; + }; + spmi_bus: spmi@c440000 { compatible = "qcom,spmi-pmic-arb"; reg = <0 0x0c440000 0 0x1100>,
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v3 3/4] arm64: dts: qcom: sc7180: Enable SoC sleep stats
Quoted 7 lines · Maulik Shah
Add device node for SoC sleep stats driver which provides various low power mode stats.
Also update the reg size of
aoss_qmpdevice to0x400.Signed-off-by Maulik Shah <[email protected]>
Quoted 32 lines
--- arch/arm64/boot/dts/qcom/sc7180.dtsi | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)arch/arm64/boot/dts/qcom/sc7180.dtsi +6
-1@@ -1610,7 +1610,7 @@ aoss_qmp: qmp@c300000 { compatible = "qcom,sc7180-aoss-qmp"; - reg = <0 0x0c300000 0 0x100000>; + reg = <0 0x0c300000 0 0x400>; interrupts = <GIC_SPI 389 IRQ_TYPE_EDGE_RISING>; mboxes = <&apss_shared 0>;@@ -1618,6 +1618,11 @@ #power-domain-cells = <1>; }; + rpmh_sleep_stats@c3f0000 { + compatible = "qcom,rpmh-sleep-stats"; + reg = <0 0x0c3f0000 0 0x400>; + }; + spmi_bus: spmi@c440000 { compatible = "qcom,spmi-pmic-arb"; reg = <0 0x0c440000 0 0x1100>,Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH v3 4/4 arm64: defconfig: Enable SoC sleep stats driver
In reply to Maulik ShahEnable SoC sleep stats driver. The driver gives statistics for various low power modes on Qualcomm Technologies, Inc. (QTI) SoCs.
--- arch/arm64/configs/defconfig | 1 + 1 file changed, 1 insertion(+)
arch/arm64/configs/defconfig +1 -0
@@ -793,6 +793,7 @@ CONFIG_QCOM_SMD_RPM=y CONFIG_QCOM_SMP2P=y CONFIG_QCOM_SMSM=y CONFIG_QCOM_SOCINFO=m +CONFIG_QCOM_SOC_SLEEP_STATS=m CONFIG_ARCH_R8A774A1=y CONFIG_ARCH_R8A774B1=y CONFIG_ARCH_R8A774C0=y
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v3 4/4] arm64: defconfig: Enable SoC sleep stats driver
Quoted 4 lines · Maulik Shah
Enable SoC sleep stats driver. The driver gives statistics for various low power modes on Qualcomm Technologies, Inc. (QTI) SoCs.
Signed-off-by Maulik Shah <[email protected]>
Quoted 19 lines
--- arch/arm64/configs/defconfig | 1 + 1 file changed, 1 insertion(+)arch/arm64/configs/defconfig +1
-0@@ -793,6 +793,7 @@ CONFIG_QCOM_SMD_RPM=y CONFIG_QCOM_SMP2P=y CONFIG_QCOM_SMSM=y CONFIG_QCOM_SOCINFO=m +CONFIG_QCOM_SOC_SLEEP_STATS=m CONFIG_ARCH_R8A774A1=y CONFIG_ARCH_R8A774B1=y CONFIG_ARCH_R8A774C0=ySignature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation