Message-ID: <e32f880e-977b-f4a8-0125-523f60fa352c@codeaurora.org>
Date:   Fri, 6 Mar 2020 12:05:34 +0530
From:   Maulik Shah <mkshah@...eaurora.org>
To:     Stephen Boyd <swboyd@...omium.org>, bjorn.andersson@...aro.org,
        evgreen@...omium.org, mka@...omium.org
Cc:     linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        agross@...nel.org, dianders@...omium.org, rnayak@...eaurora.org,
        ilina@...eaurora.org, lsrao@...eaurora.org,
        Mahesh Sivasubramanian <msivasub@...eaurora.org>
Subject: Re: [PATCH v2 2/4] soc: qcom: Add SoC sleep stats driver


On 2/29/2020 2:40 AM, Stephen Boyd wrote:
> Quoting Maulik Shah (2020-02-21 00:49:44)
>> From: Mahesh Sivasubramanian <msivasub@...eaurora.org>
>>
>> Lets's add a driver to read the the stats from remote processor
> Let's
Done.
>> 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 <msivasub@...eaurora.org>
>> Signed-off-by: Lina Iyer <ilina@...eaurora.org>
>> Signed-off-by: Maulik Shah <mkshah@...eaurora.org>
>> ---
>>  drivers/soc/qcom/Kconfig           |  10 ++
>>  drivers/soc/qcom/Makefile          |   1 +
>>  drivers/soc/qcom/soc_sleep_stats.c | 279 +++++++++++++++++++++++++++++++++++++
>>  3 files changed, 290 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..4d36654 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -185,6 +185,16 @@ 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
> Can you add || COMPILE_TEST?
Done.
>> +       depends on DEBUG_FS
>> +       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/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
>> new file mode 100644
>> index 00000000..bf38bb5
>> --- /dev/null
>> +++ b/drivers/soc/qcom/soc_sleep_stats.c
>> @@ -0,0 +1,279 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2011-2020, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/uaccess.h>
> Why?
will remove.
>> +#include <linux/debugfs.h>
>> +#include <linux/seq_file.h>
>> +#include <linux/stddef.h>
> Why?
stddef.h was included for offsetof() usage.
from below comments removed offsetof usage in entire driver and also this header.
>> +
>> +#include <linux/soc/qcom/smem.h>
>> +#include <clocksource/arm_arch_timer.h>
>> +
>> +#define NAME_LENGTH    5
>> +
>> +enum subsystem_item_id {
>> +       MODEM = 605,
>> +       ADSP,
>> +       CDSP,
>> +       SLPI,
>> +       GPU,
>> +       DISPLAY,
>> +};
>> +
>> +enum subsystem_pid {
>> +       PID_APSS = 0,
>> +       PID_MODEM = 1,
>> +       PID_ADSP = 2,
>> +       PID_SLPI = 3,
>> +       PID_CDSP = 5,
>> +       PID_GPU = PID_APSS,
>> +       PID_DISPLAY = PID_APSS,
> I still don't understand this enums. Why not make them a #define set
> with known values?
Done.
>> +};
>> +
>> +struct subsystem_data {
>> +       char *name;
> Can it be const char *?
Done.
>> +       enum subsystem_item_id item_id;
>> +       enum subsystem_pid pid;
>> +};
>> +
>> +static struct subsystem_data subsystems[] = {
>> +       { "modem", MODEM, PID_MODEM },
>> +       { "adsp", ADSP, PID_ADSP },
>> +       { "cdsp", CDSP, PID_CDSP },
>> +       { "slpi", SLPI, PID_SLPI },
>> +       { "gpu", GPU, PID_GPU },
>> +       { "display", DISPLAY, PID_DISPLAY },
>> +};
>> +
>> +struct stats_config {
>> +       u32 offset_addr;
> Looks ok but probably can just be unsigned int?
Done.
>> +       u32 num_records;
> Why not size_t? Is 32-bit width important?
32-bit width not important.
>> +       bool appended_stats_avail;
>> +};
>> +
>> +static const struct stats_config *config;
> This shouldn't be necessary. Can the config be passed into the debugfs
> file ops private data?

No we are already passing address in debugfs file ops private data from where to start reading stats.

will require to keep this global.

>> +
>> +struct soc_sleep_stats_data {
> This struct is only used in probe. Why not just make probe have more
> local variables?
This is allocated in probe only

struct soc_sleep_stats_data *drv;
however we pass drv->reg + offset and drv->root in debugfs_create_file, these are used during show to indicate
from where to start reading stats. also during remove,
it currently only uses drv->root member to clean up debugfs, but lets pass entire drv, if there is need to undo something else in future.
>> +       phys_addr_t stats_base;
>> +       resource_size_t stats_size;
>> +       void __iomem *reg;
>> +       struct dentry *root;
>> +};
>> +
>> +struct entry {
>> +       __le32 stat_type;
>> +       __le32 count;
>> +       __le64 last_entered_at;
>> +       __le64 last_exited_at;
>> +       __le64 accumulated;
> These should just be u32 and u64.
Done.
>> +};
>> +
>> +struct appended_entry {
>> +       __le32 client_votes;
>> +       __le32 reserved[3];
>> +};
> Same, just u32.
Done.
>> +
>> +static u64 get_time_in_sec(u64 counter)
>> +{
>> +       do_div(counter, arch_timer_get_rate());
> I don't think arch_timer_get_rate() is exported as a symbol. Why we need
> to convert this to time? Is the counter not sufficient to understand
> that so many ticks have passed since it entered and exited?

Thanks, yes it is not exported. i think we can display without converting to seconds.

if the need arises to display in seconds, we can add below locally

#define ARCH_TIMER_FREQ 19200000

and then do_div with this value as done in v1.

>> +
>> +       return counter;
>> +}
>> +
>> +static void print_sleep_stats(struct seq_file *s, struct entry *e)
>> +{
>> +       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);
>> +
>> +       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);
> Why is count not capitalized but everything else is?
Done.
>> +}
>> +
>> +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);
> Do we need to look this up each time we read the stats? Why can't we get
> this once in probe or when we create the debugfs file?
Yes, we need to look up each time in shared memory for latest values.
>> +       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;
>> +       uint32_t offset;
>> +       struct entry e;
>> +
>> +       offset = offsetof(struct entry, count);
>> +       e.count = le32_to_cpu(readl_relaxed(reg + offset));
> readl APIs already do endian conversions.
Done.
>> +
>> +       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, accumulated);
>> +       e.accumulated = le64_to_cpu(readq_relaxed(reg + offset));
>> +
>> +       print_sleep_stats(s, &e);
>> +
>> +       if (config->appended_stats_avail) {
>> +               struct appended_entry ae;
>> +
>> +               offset = offsetof(struct appended_entry, client_votes) +
>> +                        sizeof(struct entry);
>> +               ae.client_votes = le32_to_cpu(readl_relaxed(reg + offset));
>> +               seq_printf(s, "Client_votes = 0x%x\n", ae.client_votes);
> Use %#x to avoid having to add 0x prefix. Also, can we replace '=' with
> ':' so that readability is a bit nicer?
Done.
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +DEFINE_SHOW_ATTRIBUTE(soc_sleep_stats);
>> +DEFINE_SHOW_ATTRIBUTE(subsystem_sleep_stats);
>> +
>> +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 drop comma after sentinel. It makes a compiler error appear if
> anything comes after.
Done.
>> +};
> Move this table next to the driver structure please.
Done.
>> +
>> +static int create_debugfs_entries(struct soc_sleep_stats_data *drv)
>> +{
>> +       struct entry *e;
>> +       char stat_type[NAME_LENGTH] = {0};
> Is it a string? Otherwise, seems pretty useless to initialize this to 0
> on the stack.
yes its string.
>> +       uint32_t offset, type;
> Just use u32 instead of uint32_t in any kernel code.
Done
>> +       int i;
>> +
>> +       drv->root = debugfs_create_dir("soc_sleep_stats", NULL);
>> +       if (IS_ERR_OR_NULL(drv->root))
>> +               return PTR_ERR(drv->root);
> This is wrong. Debugfs checks have generally been removed because it's
> not a problem if debugfs fails. When it fails, drv->root will be NULL
> and any debugfs_create() function will ignore it. Since this driver
> depends on DEBUGFS being enabled, -ENODEV won't be returned as an error
> pointer.
Done.
>> +
>> +       for (i = 0; i < config->num_records; i++) {
>> +               offset = offsetof(struct entry, stat_type) +
>> +                        (i * sizeof(struct entry));
> This offset of stuff is sort of complicated. I'd prefer to treat it like
> how we treat most registers with #defines and base + #define sort of
> logic. That is easier to read. It also let's us decide to completely
> reorder struct members and not have to worry that the struct doesn't
> match how memory is laid out. 

 Reordering members should not be done.  qcom_smem_get() API returns
 pointer to "struct entry" type and will need keep order of members as it is.

> Instead we have macros that define the
> offset from some base __iomem pointer.
 
 Okay, i removed offsetof usage from entire driver and using #define
 now.
>> +
>> +               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(uint32_t));
>> +               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);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +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 = of_device_get_match_data(&pdev->dev);
> Use device_get_match_data() to make this be less DT specific.
Done.
>> +       if (!config)
>> +               return -ENODEV;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       if (!res)
>> +               return PTR_ERR(res);
>> +
>> +       offset_addr = ioremap_nocache(res->start + config->offset_addr,
> Why do we need ioremap_nocache()? Can ioremap() work?
Will update it to use devm_ioremap().
>> +                                     sizeof(u32));
>> +       if (IS_ERR(offset_addr))
>> +               return PTR_ERR(offset_addr);
>> +
>> +       drv->stats_base = res->start | readl_relaxed(offset_addr);
> Why | vs. +?

This is how final address need to be formed.

 Thanks,
 Maulik

>> +       drv->stats_size = resource_size(res);
>> +       iounmap(offset_addr);
>> +
>> +       drv->reg = devm_ioremap(&pdev->dev, drv->stats_base, drv->stats_size);
>> +       if (!drv->reg)
>> +               return -ENOMEM;
>> +
>> +       if (create_debugfs_entries(drv))
>> +               return -ENODEV;
>> +
>> +       platform_set_drvdata(pdev, drv);
>> +
>> +       return 0;
>> +}

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

Powered by blists - more mailing lists