Re: [PATCH 2/2] drivers: qcom: Add SoC sleep stats driver

[Maulik Shah]

On 8/9/2019 12:10 AM, Stephen Boyd wrote:
> Quoting Maulik Shah (2019-08-07 23:12:28)
> > Qualcomm Technologies Inc's (QTI) chipsets support SoC level
> > low power modes. Statistics for SoC sleep stats are produced
> > by remote processor.
> >
> > Lets's add a driver to read the shared memory exported by the
> > remote processor and export to sysfs.
> >
> > Signed-off-by: Mahesh Sivasubramanian <msivasub@xxxxxxxxxxxxxx>
> > Signed-off-by: Lina Iyer <ilina@xxxxxxxxxxxxxx>
> > Signed-off-by: Maulik Shah <mkshah@xxxxxxxxxxxxxx>
> SoB chain is weird here too.
corrected in v2.
> > ---
> >   drivers/soc/qcom/Kconfig           |   9 ++
> >   drivers/soc/qcom/Makefile          |   1 +
> >   drivers/soc/qcom/soc_sleep_stats.c | 249 +++++++++++++++++++++++++++++
> There should be a Documentation/ABI/ path in this diffstat above because
> you're adding sysfs attributes.
In v2 using debugfs for stats information.
> There's some similar support in the ARM PSCI spec for extracting
> idle/suspend stats, see section 5.21 PSCI_STAT_RESIDENCY/COUNT. Maybe
> this code can align with that feature in PSCI? At the least, I hope we
> can come up with a generic sysfs ABI that can be used to describe CPU
> and system wide power states in a way that userspace can read and
> understand how long the device was in these different power states. I
> would guess that other architectures like x86 may also want to get
> involved in reporting this information in a standard way, so please loop
> in some x86 power folks too.
>
> It would be neat if the PSCI feature could be used for this instead of
> having a custom SoC driver. Maybe that won't work though because this
> works for shipping firmware and/or because of the 'client_votes' thing
> which looks like special extra data describing the other subsystems? At
> least for some SoCs it may be all they need though, so keeping the PSCI
> call in mind would be good when developing the ABI and may be enough for
> userspace purposes. The client_votes part may be possible to layer on
> top of the PSCI calls anyway, and go into some other file so we can
> figure out which remoteproc is holding up suspend or idle states.
correct PSCI won't work here.
> > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> > index 880cf0290962..7aac24430e99 100644
> > --- a/drivers/soc/qcom/Kconfig
> > +++ b/drivers/soc/qcom/Kconfig
> > @@ -163,6 +163,15 @@ config QCOM_SMSM
> >            Say yes here to support the Qualcomm Shared Memory State Machine.
> >            The state machine is represented by bits in shared memory.
> >   
> > +config QCOM_SOC_SLEEP_STATS
> > +       tristate "Qualcomm Technologies Inc. (QTI) SoC sleep stats driver"
> > +       depends on ARCH_QCOM
> > +       help
> > +         Qualcomm Technologies Inc. (QTI) SoC sleep stats driver to read
> > +         the shared memory exported by the remote processor related to
> Shared memory sounds like DDR.
Not a DDR.
> > +         various SoC level low power modes statistics and export to sysfs
> > +         interface.
> > +
> >   config QCOM_WCNSS_CTRL
> >          tristate "Qualcomm WCNSS control driver"
> >          depends on ARCH_QCOM || COMPILE_TEST
> > diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
> > new file mode 100644
> > index 000000000000..5b95d68512ec
> > --- /dev/null
> > +++ b/drivers/soc/qcom/soc_sleep_stats.c
> > @@ -0,0 +1,249 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +/*
> > + * Copyright (c) 2011-2019, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#define pr_fmt(fmt) "%s: " fmt, __func__
> > +
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> Is this include used?
removed unused headers in v2.
> > +
> > +#define ARCH_TIMER_FREQ        19200000
> Can't this come through clk APIs? Or is this the ARM architected timer
> freqeuency?
Updated to use clk APIs
> > +
> > +struct stats_config {
> > +       u32 offset_addr;
> > +       u32 num_records;
> > +       bool appended_stats_avail;
> > +};
> > +
> > +struct soc_sleep_stats_data {
> > +       phys_addr_t stats_base;
> > +       resource_size_t stats_size;
> > +       const struct stats_config *config;
> > +       struct kobject *kobj;
> > +       struct kobj_attribute ka;
> > +       struct mutex lock;
> > +};
> > +
> > +struct entry {
> > +       __le32 stat_type;
> > +       __le32 count;
> > +       __le64 last_entered_at;
> > +       __le64 last_exited_at;
> > +       __le64 accumulated;
> > +};
> > +
> > +struct appended_entry {
> > +       __le32 client_votes;
> > +       __le32 reserved[3];
> > +};
> > +
> > +struct stats_entry {
> > +       struct entry entry;
> > +       struct appended_entry appended_entry;
> > +};
> > +
> > +static inline u64 get_time_in_sec(u64 counter)
> > +{
> > +       do_div(counter, ARCH_TIMER_FREQ);
> > +
> > +       return counter;
> > +}
> > +
> > +static inline ssize_t append_data_to_buf(char *buf, int length,
> > +                                        struct stats_entry *data)
> > +{
> > +       char stat_type[5] = {0};
> > +
> > +       memcpy(stat_type, &data->entry.stat_type, sizeof(u32));
> sizeof(u32) != 5. Is this on purpose?

yes, maximum name length 5.

> > +
> > +       return scnprintf(buf, length,
> > +                        "%s\n"
> > +                        "\tCount                    :%u\n"
> > +                        "\tLast Entered At(sec)     :%llu\n"
> > +                        "\tLast Exited At(sec)      :%llu\n"
> > +                        "\tAccumulated Duration(sec):%llu\n"
> > +                        "\tClient Votes             :0x%x\n\n",
> > +                        stat_type, data->entry.count,
> > +                        data->entry.last_entered_at,
> > +                        data->entry.last_exited_at,
> > +                        data->entry.accumulated,
> > +                        data->appended_entry.client_votes);
> > +}
> > +
> > +static ssize_t stats_show(struct kobject *obj, struct kobj_attribute *attr,
> > +                         char *buf)
> > +{
> > +       void __iomem *reg;
> > +       int i;
> > +       uint32_t offset;
> > +       ssize_t length = 0, op_length;
> > +       struct stats_entry data;
> > +       struct entry *e = &data.entry;
> > +       struct appended_entry *ae = &data.appended_entry;
> > +       struct soc_sleep_stats_data *drv = container_of(attr,
> > +                                          struct soc_sleep_stats_data, ka);
> > +
> > +       mutex_lock(&drv->lock);
> > +       reg = ioremap_nocache(drv->stats_base, drv->stats_size);
> > +       if (!reg) {
> > +               pr_err("io remap failed\n");
> This looks like a real bad idea to ioremap each time the stats are
> shown. Why not just map once in probe so we don't have to create a
> mapping and suffer the overhead involved in that?
Updated in v2 to ioremap only once in probe.
> > +               mutex_unlock(&drv->lock);
> > +               return length;
> > +       }
> > +
> > +       for (i = 0; i < drv->config->num_records; i++) {
> > +               offset = offsetof(struct entry, stat_type);
> > +               e->stat_type = le32_to_cpu(readl_relaxed(reg + offset));
> > +
> > +               offset = offsetof(struct entry, count);
> > +               e->count = le32_to_cpu(readl_relaxed(reg + offset));
> > +
> > +               offset = offsetof(struct entry, last_entered_at);
> > +               e->last_entered_at = le64_to_cpu(readq_relaxed(reg + offset));
> > +
> > +               offset = offsetof(struct entry, last_exited_at);
> > +               e->last_exited_at = le64_to_cpu(readq_relaxed(reg + offset));
> > +
> > +               offset = offsetof(struct entry, last_exited_at);
> > +               e->accumulated = le64_to_cpu(readq_relaxed(reg + offset));
> > +
> > +               e->last_entered_at = get_time_in_sec(e->last_entered_at);
> > +               e->last_exited_at = get_time_in_sec(e->last_exited_at);
> > +               e->accumulated = get_time_in_sec(e->accumulated);
> > +
> > +               reg += sizeof(struct entry);
> > +
> > +               if (drv->config->appended_stats_avail) {
> > +                       offset = offsetof(struct appended_entry, client_votes);
> > +                       ae->client_votes = le32_to_cpu(readl_relaxed(reg +
> > +                                                                    offset));
> > +
> > +                       reg += sizeof(struct appended_entry);
> > +               } else
> > +                       ae->client_votes = 0;
> Please add braces to the else statement when the if statement has
> braces.
Done.
> > +
> > +               op_length = append_data_to_buf(buf + length, PAGE_SIZE - length,
> > +                                              &data);
> > +               if (op_length >= PAGE_SIZE - length)
> > +                       goto exit;
> > +
> > +               length += op_length;
> > +       }
> > +exit:
> > +       iounmap(reg);
> > +       mutex_unlock(&drv->lock);
> > +       return length;
> > +}
> > +
> > +static int soc_sleep_stats_create_sysfs(struct platform_device *pdev,
> > +                                       struct soc_sleep_stats_data *drv)
> > +{
> > +       int ret = -ENOMEM;
> > +
> > +       drv->kobj = kobject_create_and_add("soc_sleep", power_kobj);
> > +       if (!drv->kobj)
> > +               goto fail;
> Just return -ENOMEM here. It is really weird to make kobjects directly
> like this. How is userspace expected to use this?
Done. using debugfs.
> > +
> > +       sysfs_attr_init(drv->ka.attr);
> > +       drv->ka.attr.mode = 0444;
> > +       drv->ka.attr.name = "stats";
> > +       drv->ka.show = stats_show;
> > +
> > +       ret = sysfs_create_file(drv->kobj, &drv->ka.attr);
> > +       if (ret)
> > +               goto fail;
> Just return sysfs_create_file()?
>
> > +
> > +       platform_set_drvdata(pdev, drv);
> Do this platform_set_drvdata in probe?
done.
> > +fail:
> > +       return ret;
> > +}
> > +
> > +static const struct stats_config rpm_data = {
> > +       .offset_addr = 0x14,
> > +       .num_records = 2,
> > +       .appended_stats_avail = true,
> > +};
> > +
> > +static const struct stats_config rpmh_data = {
> > +       .offset_addr = 0x4,
> > +       .num_records = 3,
> > +       .appended_stats_avail = false,
> > +};
> > +
> > +static const struct of_device_id soc_sleep_stats_table[] = {
> > +       { .compatible = "qcom,rpm-sleep-stats", .data = &rpm_data},
> > +       { .compatible = "qcom,rpmh-sleep-stats", .data = &rpmh_data},
> > +       { },
> > +};
> > +
> > +static int soc_sleep_stats_probe(struct platform_device *pdev)
> > +{
> > +       const struct of_device_id *match;
> > +       struct soc_sleep_stats_data *drv;
> > +       struct resource *res;
> > +       void __iomem *offset_addr;
> > +       int ret;
> > +
> > +       drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
> > +       if (!drv)
> > +               return -ENOMEM;
> > +
> > +       match = of_match_node(soc_sleep_stats_table, pdev->dev.of_node);
> > +       if (!match)
> > +               return -ENODEV;
> > +
> > +       drv->config = match->data;
> Is this of_device_get_match_data()?
done.
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       if (!res)
> > +               return PTR_ERR(res);
> > +
> > +       offset_addr = ioremap_nocache(res->start + drv->config->offset_addr,
> > +                                     sizeof(u32));
> Why not just devm_platform_ioremap_resource()?
this adds drv->config->offset_addr before ioremap.
> > +       if (IS_ERR(offset_addr))
> > +               return PTR_ERR(offset_addr);
> > +
> > +       drv->stats_base = res->start | readl_relaxed(offset_addr);
> > +       drv->stats_size = resource_size(res);
> > +       iounmap(offset_addr);
> > +       mutex_init(&drv->lock);
> Hopefully this lock isn't required?
removed lock.
> > +
> > +       ret = soc_sleep_stats_create_sysfs(pdev, drv);
> > +       if (ret)
> > +               pr_info("Failed to create sysfs interface\n");
> Not pr_err()? Or dev_err()?
done.
> > +
> > +       return ret;
> > +}
> > +
> > +static int soc_sleep_stats_remove(struct platform_device *pdev)
> > +{
> > +       struct soc_sleep_stats_data *drv = platform_get_drvdata(pdev);
> > +
> > +       sysfs_remove_file(drv->kobj, &drv->ka.attr);
> > +       kobject_put(drv->kobj);
> > +       platform_set_drvdata(pdev, NULL);
> This last line isn't necessary. Please remove.
Done.
> > +
> > +       return 0;
> > +}
> > +

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