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
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
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
cat /sys/kernel/debug/soc_sleep_stats/ddr
count = 0
Last Entered At = 0
Last Exited At = 0
Accumulated Duration = 0
Signed-off-by: Mahesh Sivasubramanian <[email protected]>
Signed-off-by: Lina Iyer <[email protected]>
Signed-off-by: Maulik Shah <[email protected]>
---
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
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index d0a73e7..08bc0df3 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -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
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 9fb35c8..e6bd052 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -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
diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
new file mode 100644
index 00000000..79a14d2
--- /dev/null
+++ b/drivers/soc/qcom/soc_sleep_stats.c
@@ -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>
+
+#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
+
+#define PID_APSS 0
+#define PID_MODEM 1
+#define PID_ADSP 2
+#define PID_SLPI 3
+#define PID_CDSP 5
+
+#define NAME_LENGTH 5
+
+#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;
+ 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 {
+ unsigned int offset_addr;
+ unsigned int num_records;
+ bool appended_stats_avail;
+};
+
+static const struct stats_config *config;
+
+struct soc_sleep_stats_data {
+ phys_addr_t stats_base;
+ resource_size_t stats_size;
+ void __iomem *reg;
+ struct dentry *root;
+};
+
+struct entry {
+ 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);
+ 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);
+
+ 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);
+ 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);
+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};
+ 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));
+ 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);
+
+ 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;
+
+ 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));
+ 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);
+ 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,
+};
+
+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 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");
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
Add 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.
Cc: [email protected]
Signed-off-by: Maulik Shah <[email protected]>
---
arch/arm64/boot/dts/qcom/sc7180.dtsi | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 253274d..b5b0f3f 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -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>,
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
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]>
---
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index a8de3d3..2dd543b 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -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
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
On Thu 05 Mar 23:23 PST 2020, Maulik Shah wrote:
> 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?
> cat /sys/kernel/debug/soc_sleep_stats/ddr
> count = 0
> Last Entered At = 0
> Last Exited At = 0
> Accumulated Duration = 0
>
> Signed-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.
> 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
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index d0a73e7..08bc0df3 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -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
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 9fb35c8..e6bd052 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -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
> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
> new file mode 100644
> index 00000000..79a14d2
> --- /dev/null
> +++ b/drivers/soc/qcom/soc_sleep_stats.c
> @@ -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>
> +
> +#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.
> +
> +#define PID_APSS 0
> +#define PID_MODEM 1
> +#define PID_ADSP 2
> +#define PID_SLPI 3
> +#define PID_CDSP 5
Ditto.
> +
> +#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.
> +
> +#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"
> + 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 {
> + 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.
> +
> +struct soc_sleep_stats_data {
> + phys_addr_t stats_base;
This can be a local variable in soc_sleep_stats_probe()
> + resource_size_t stats_size;
Ditto.
> + void __iomem *reg;
> + struct dentry *root;
> +};
> +
> +struct entry {
Can you please give this struct a slightly less generic name?
> + 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);
> + 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.
> +
> + 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.
> + 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);
> +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.
> + 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".
> + 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.
> +
> + 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;
> +
> + 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.
> + 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...
> + 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,
> +};
> +
> +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
> + { }
> +};
> +
> +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");
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
On Thu 05 Mar 23:23 PST 2020, Maulik Shah wrote:
> Add 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.
>
> Cc: [email protected]
> Signed-off-by: Maulik Shah <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sc7180.dtsi | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index 253274d..b5b0f3f 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -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>,
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
On Thu 05 Mar 23:23 PST 2020, Maulik Shah wrote:
> 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]>
Reviewed-by: Bjorn Andersson <[email protected]>
> ---
> arch/arm64/configs/defconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index a8de3d3..2dd543b 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -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
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
On 3/7/2020 12:12 PM, Bjorn Andersson wrote:
> On Thu 05 Mar 23:23 PST 2020, Maulik Shah wrote:
>
>> 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?
Done. Updated to create "qcom_sleep_stats" directory.
>> cat /sys/kernel/debug/soc_sleep_stats/ddr
>> count = 0
>> Last Entered At = 0
>> Last Exited At = 0
>> Accumulated Duration = 0
>>
>> Signed-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.
>
>> 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
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index d0a73e7..08bc0df3 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -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
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 9fb35c8..e6bd052 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -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
>> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
>> new file mode 100644
>> index 00000000..79a14d2
>> --- /dev/null
>> +++ b/drivers/soc/qcom/soc_sleep_stats.c
>> @@ -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>
>> +
>> +#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.
>
>> +
>> +#define PID_APSS 0
>> +#define PID_MODEM 1
>> +#define PID_ADSP 2
>> +#define PID_SLPI 3
>> +#define PID_CDSP 5
> Ditto.
Done.
>
>> +
>> +#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.
Done.
>> +
>> +#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"
Done.
>> + 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 {
>> + 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.
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.
>> +
>> +struct soc_sleep_stats_data {
>> + phys_addr_t stats_base;
> This can be a local variable in soc_sleep_stats_probe()
>
>> + resource_size_t stats_size;
> Ditto.
Done.
>
>> + void __iomem *reg;
>> + struct dentry *root;
>> +};
>> +
>> +struct entry {
> Can you please give this struct a slightly less generic name?
Done.
>
>> + 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);
>> + 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.
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.
>
>> +
>> + 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.
>
>> + 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);
>> +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.
Done.
>> + 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".
Done.
>
>> + 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.
>> +
>> + 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;
>> +
>> + 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.
>
>> + 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.
>
>> + 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,
>> +};
>> +
>> +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
Done.
Thanks,
Maulik
>
>> + { }
>> +};
>> +
>> +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");
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>> of Code Aurora Forum, hosted by The Linux Foundation
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Quoting Maulik Shah (2020-03-09 03:58:27)
>
> On 3/7/2020 12:12 PM, Bjorn Andersson wrote:
> > On Thu 05 Mar 23:23 PST 2020, Maulik Shah wrote:
> >> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
> >> new file mode 100644
> >> index 00000000..79a14d2
> >> --- /dev/null
> >> +++ b/drivers/soc/qcom/soc_sleep_stats.c
> >> @@ -0,0 +1,248 @@
[...]
> >> + 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 {
> >> + 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.
>
> 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.
On 3/9/2020 11:49 PM, Stephen Boyd wrote:
> Quoting Maulik Shah (2020-03-09 03:58:27)
>> On 3/7/2020 12:12 PM, Bjorn Andersson wrote:
>>> On Thu 05 Mar 23:23 PST 2020, Maulik Shah wrote:
>>>> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
>>>> new file mode 100644
>>>> index 00000000..79a14d2
>>>> --- /dev/null
>>>> +++ b/drivers/soc/qcom/soc_sleep_stats.c
>>>> @@ -0,0 +1,248 @@
> [...]
>>>> + 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 {
>>>> + 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.
>> 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.
>
> 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
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation