lkml.org 
[lkml]   [2020]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] drivers: qcom: Add SoC sleep stats driver
From
Date

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

yes, maximum name length 5.

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

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

\
 
 \ /
  Last update: 2020-02-21 07:06    [W:0.427 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site