diff mbox series

[v6,2/4] soc: qcom: Add SoC sleep stats driver

Message ID 1612448508-9179-3-git-send-email-mkshah@codeaurora.org (mailing list archive)
State Superseded
Headers show
Series Introduce SoC sleep stats driver | expand

Commit Message

Maulik Shah Feb. 4, 2021, 2:21 p.m. UTC 
From: Mahesh Sivasubramanian <msivasub@codeaurora.org>

Let's add a driver to read the stats from remote processor and
export to debugfs.

The driver creates "qcom_sleep_stats" directory in debugfs and
adds files for various low power mode available. Below is sample
output with command

cat /sys/kernel/debug/qcom_sleep_stats/ddr
count = 0
Last Entered At = 0
Last Exited At = 0
Accumulated Duration = 0

Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
[mkshah: add subsystem sleep stats, create one file for each stat]
Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
 drivers/soc/qcom/Kconfig           |  10 ++
 drivers/soc/qcom/Makefile          |   1 +
 drivers/soc/qcom/soc_sleep_stats.c | 258 +++++++++++++++++++++++++++++++++++++
 3 files changed, 269 insertions(+)
 create mode 100644 drivers/soc/qcom/soc_sleep_stats.c

Comments

kernel test robot Feb. 7, 2021, 10:25 a.m. UTC | #1 
Hi Maulik,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on arm64/for-next/core linux/master linus/master v5.11-rc6 next-20210125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Maulik-Shah/Introduce-SoC-sleep-stats-driver/20210204-223344
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: h8300-randconfig-r013-20210207 (attached as .config)
compiler: h8300-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/60e8c659b52a5c1ac2b1a91d02df00ddecdccba8
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Maulik-Shah/Introduce-SoC-sleep-stats-driver/20210204-223344
        git checkout 60e8c659b52a5c1ac2b1a91d02df00ddecdccba8
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=h8300 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/soc/qcom/soc_sleep_stats.c: In function 'soc_sleep_stats_show':
>> drivers/soc/qcom/soc_sleep_stats.c:113:25: error: implicit declaration of function 'readq'; did you mean 'readl'? [-Werror=implicit-function-declaration]
     113 |  stat.last_entered_at = readq(reg + LAST_ENTERED_AT_ADDR);
         |                         ^~~~~
         |                         readl
   cc1: some warnings being treated as errors


vim +113 drivers/soc/qcom/soc_sleep_stats.c

   105	
   106	static int soc_sleep_stats_show(struct seq_file *s, void *d)
   107	{
   108		struct stats_prv_data *prv_data = s->private;
   109		void __iomem *reg = prv_data->reg;
   110		struct sleep_stats stat;
   111	
   112		stat.count = readl(reg + COUNT_ADDR);
 > 113		stat.last_entered_at = readq(reg + LAST_ENTERED_AT_ADDR);
   114		stat.last_exited_at = readq(reg + LAST_EXITED_AT_ADDR);
   115		stat.accumulated = readq(reg + ACCUMULATED_ADDR);
   116	
   117		print_sleep_stats(s, &stat);
   118	
   119		if (prv_data->appended_stats_avail) {
   120			struct appended_stats app_stat;
   121	
   122			app_stat.client_votes = readl(reg + CLIENT_VOTES_ADDR);
   123			seq_printf(s, "Client_votes = %#x\n", app_stat.client_votes);
   124		}
   125	
   126		return 0;
   127	}
   128	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Stephen Boyd Feb. 9, 2021, 12:42 a.m. UTC | #2 
Quoting Maulik Shah (2021-02-04 06:21:46)
> From: Mahesh Sivasubramanian <msivasub@codeaurora.org>
> 
> Let's add a driver to read the stats from remote processor and
> export to debugfs.
> 
> The driver creates "qcom_sleep_stats" directory in debugfs and
> adds files for various low power mode available. Below is sample
> output with command
> 
> cat /sys/kernel/debug/qcom_sleep_stats/ddr

The ddr subsystem isn't listed below in subsystems though. Can the
example be updated to reflect what is supported? Or can we gain the ddr
subsystem?

> count = 0
> Last Entered At = 0
> Last Exited At = 0
> Accumulated Duration = 0
> 
> Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> [mkshah: add subsystem sleep stats, create one file for each stat]
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> ---
>  drivers/soc/qcom/Kconfig           |  10 ++
>  drivers/soc/qcom/Makefile          |   1 +
>  drivers/soc/qcom/soc_sleep_stats.c | 258 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 269 insertions(+)
>  create mode 100644 drivers/soc/qcom/soc_sleep_stats.c
> 
> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
> new file mode 100644
> index 0000000..66df638
> --- /dev/null
> +++ b/drivers/soc/qcom/soc_sleep_stats.c
> @@ -0,0 +1,258 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2011-2021, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/debugfs.h>

Any chance to get off of debugfs and expose this in sysfs instead?

> +#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 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
> +
> +#define STAT_OFFSET(record_no, type) (((record_no)*(sizeof(struct sleep_stats))) + (type))
> +#define APPENDED_STAT_OFFSET(record_no) ((record_no)*(sizeof(struct appended_stats)))
> +
> +struct subsystem_data {
> +       const char *name;
> +       u32 smem_item;
> +       u32 pid;
> +};
> +
> +static const struct subsystem_data subsystems[] = {
> +       { "modem", 605, 1 },
> +       { "adsp", 606, 2 },
> +       { "cdsp", 607, 5 },
> +       { "slpi", 608, 3 },
> +       { "gpu", 609, 0 },
> +       { "display", 610, 0 },
> +       { "adsp_island", 613, 2 },
> +       { "slpi_island", 613, 3 },
> +};
> +
> +struct stats_config {
> +       u32 offset_addr;
> +       u32 num_records;

size_t?

> +       bool appended_stats_avail;
> +};
> +
> +struct stats_prv_data {
> +       bool appended_stats_avail;
> +       void __iomem *reg;
> +};
> +
> +struct sleep_stats {
> +       u32 stat_type;
> +       u32 count;
> +       u64 last_entered_at;
> +       u64 last_exited_at;
> +       u64 accumulated;
> +};
> +
> +struct appended_stats {
> +       u32 client_votes;
> +       u32 reserved[3];
> +};
> +
> +static void print_sleep_stats(struct seq_file *s, const struct sleep_stats *stat)
> +{
> +       u64 accumulated = stat->accumulated;
> +       /*
> +        * If a subsystem is in sleep when reading the sleep stats adjust
> +        * the accumulated sleep duration to show actual sleep time.
> +        */
> +       if (stat->last_entered_at > stat->last_exited_at)
> +               accumulated += arch_timer_read_counter()
> +                              - stat->last_entered_at;
> +
> +       seq_printf(s, "Count = %u\n", stat->count);
> +       seq_printf(s, "Last Entered At = %llu\n", stat->last_entered_at);
> +       seq_printf(s, "Last Exited At = %llu\n", stat->last_exited_at);
> +       seq_printf(s, "Accumulated Duration = %llu\n", accumulated);
> +}
> +
> +static int subsystem_sleep_stats_show(struct seq_file *s, void *d)
> +{
> +       struct subsystem_data *subsystem = s->private;
> +       struct sleep_stats *stat;
> +
> +       /*
> +        * Saving this pointer during probe may not help in cases like
> +        * subsystem restart, beside not each subsystem is a remote processor

s/beside/besides/
s/each/every/

> +        * for e.g display for which we can get start and stop notification

for example
s/notification/notification./

> +        *
> +        * Lookup smem pointer each time to keep it simple.
> +        */
> +       stat = qcom_smem_get(subsystem->pid, subsystem->smem_item, NULL);
> +       if (IS_ERR(stat))
> +               return PTR_ERR(stat);
> +
> +       print_sleep_stats(s, stat);
> +
> +       return 0;
> +}
> +
> +static int soc_sleep_stats_show(struct seq_file *s, void *d)
> +{
> +       struct stats_prv_data *prv_data = s->private;
> +       void __iomem *reg = prv_data->reg;
> +       struct sleep_stats stat;
> +
> +       stat.count = readl(reg + COUNT_ADDR);
> +       stat.last_entered_at = readq(reg + LAST_ENTERED_AT_ADDR);
> +       stat.last_exited_at = readq(reg + LAST_EXITED_AT_ADDR);
> +       stat.accumulated = readq(reg + ACCUMULATED_ADDR);
> +
> +       print_sleep_stats(s, &stat);
> +
> +       if (prv_data->appended_stats_avail) {
> +               struct appended_stats app_stat;
> +
> +               app_stat.client_votes = readl(reg + CLIENT_VOTES_ADDR);
> +               seq_printf(s, "Client_votes = %#x\n", app_stat.client_votes);
> +       }
> +
> +       return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(soc_sleep_stats);
> +DEFINE_SHOW_ATTRIBUTE(subsystem_sleep_stats);
> +
> +static struct dentry *create_debugfs_entries(void __iomem *reg,

Can this be soc_sleep_stats_*()? Or inlined at the call site?
create_debugfs_entries() is too generic.

> +                                            struct stats_prv_data *prv_data,
> +                                            u32 num_records)
> +{
> +       struct dentry *root;
> +       struct sleep_stats *stat;
> +       char stat_type[sizeof(u32)] = {0};
> +       u32 offset, type;
> +       int i;
> +
> +       root = debugfs_create_dir("qcom_sleep_stats", NULL);
> +
> +       for (i = 0; i < num_records; i++) {
> +               offset = STAT_OFFSET(i, STAT_TYPE_ADDR);
> +
> +               if (prv_data[i].appended_stats_avail)
> +                       offset += APPENDED_STAT_OFFSET(i);
> +
> +               prv_data[i].reg = reg + offset;
> +
> +               type = readl(prv_data[i].reg);
> +               memcpy_fromio(stat_type, &type, sizeof(u32));

type isn't an __iomem pointer though. Did you compile this with sparse?
Use 'make C=2'

Also, this is really bad design. The mmio register contains a physical
address of where to read from? It's not an offset from the register or
something like that? If an IOMMU is rewriting physical addresses this
driver is screwed and will try to map some other physical address that
could be completely different based on what the IOMMU decides that
IPA maps to physically.

> +               strim(stat_type);
> +               debugfs_create_file(stat_type, 0400, root,
> +                                   &prv_data[i],
> +                                   &soc_sleep_stats_fops);
> +       }
> +
> +       for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
> +               stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item,
> +                                    NULL);
> +               if (IS_ERR(stat))
> +                       continue;
> +
> +               debugfs_create_file(subsystems[i].name, 0400, root,
> +                                   (void *)&subsystems[i],

cast to void shouldn't be needed. Please drop it.

> +                                   &subsystem_sleep_stats_fops);
> +       }
> +
> +       return root;
> +}
> +
> +static int soc_sleep_stats_probe(struct platform_device *pdev)
> +{
> +       struct resource *res;
> +       void __iomem *reg;
> +       void __iomem *offset_addr;
> +       phys_addr_t stats_base;
> +       resource_size_t stats_size;
> +       struct dentry *root;
> +       const struct stats_config *config;
> +       struct stats_prv_data *prv_data;
> +       int i;
> +
> +       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 = ioremap(res->start + config->offset_addr, sizeof(u32));
> +       if (IS_ERR(offset_addr))
> +               return PTR_ERR(offset_addr);
> +
> +       stats_base = res->start | readl_relaxed(offset_addr);
> +       stats_size = resource_size(res);
> +       iounmap(offset_addr);
> +
> +       reg = devm_ioremap(&pdev->dev, stats_base, stats_size);
> +       if (!reg)
> +               return -ENOMEM;
> +
> +       prv_data = devm_kcalloc(&pdev->dev, config->num_records,
> +                               sizeof(struct stats_prv_data), GFP_KERNEL);

sizeof(*prv_data) please so we don't have to think about the type
matching the pointer storage.

> +       if (!prv_data)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < config->num_records; i++)
> +               prv_data[i].appended_stats_avail = config->appended_stats_avail;
> +
> +       root = create_debugfs_entries(reg, prv_data, config->num_records);
> +       platform_set_drvdata(pdev, root);
> +
> +       return 0;
> +}
> +
> +static int soc_sleep_stats_remove(struct platform_device *pdev)
> +{
> +       struct dentry *root = platform_get_drvdata(pdev);
> +
> +       debugfs_remove_recursive(root);
> +
> +       return 0;
> +}
> +
Maulik Shah April 6, 2021, 6:40 a.m. UTC | #3 
Hi,

On 2/9/2021 6:12 AM, Stephen Boyd wrote:
> Quoting Maulik Shah (2021-02-04 06:21:46)
>> From: Mahesh Sivasubramanian <msivasub@codeaurora.org>
>>
>> Let's add a driver to read the stats from remote processor and
>> export to debugfs.
>>
>> The driver creates "qcom_sleep_stats" directory in debugfs and
>> adds files for various low power mode available. Below is sample
>> output with command
>>
>> cat /sys/kernel/debug/qcom_sleep_stats/ddr
> The ddr subsystem isn't listed below in subsystems though. Can the
> example be updated to reflect what is supported? Or can we gain the ddr
> subsystem?

I have added comment in the driver on how we get the "ddr" subsystem in v7.

>
>> count = 0
>> Last Entered At = 0
>> Last Exited At = 0
>> Accumulated Duration = 0
>>
>> Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> [mkshah: add subsystem sleep stats, create one file for each stat]
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>> ---
>>   drivers/soc/qcom/Kconfig           |  10 ++
>>   drivers/soc/qcom/Makefile          |   1 +
>>   drivers/soc/qcom/soc_sleep_stats.c | 258 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 269 insertions(+)
>>   create mode 100644 drivers/soc/qcom/soc_sleep_stats.c
>>
>> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
>> new file mode 100644
>> index 0000000..66df638
>> --- /dev/null
>> +++ b/drivers/soc/qcom/soc_sleep_stats.c
>> @@ -0,0 +1,258 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2011-2021, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/debugfs.h>
> Any chance to get off of debugfs and expose this in sysfs instead?

The V1 series was using sysfs (/sys/power/) path to display the stats 
which was later moved to debugfs.

any reason to expose in sysfs again? the informantion in stats driver is 
largely for debug purpose only.

>> +#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 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
>> +
>> +#define STAT_OFFSET(record_no, type) (((record_no)*(sizeof(struct sleep_stats))) + (type))
>> +#define APPENDED_STAT_OFFSET(record_no) ((record_no)*(sizeof(struct appended_stats)))
>> +
>> +struct subsystem_data {
>> +       const char *name;
>> +       u32 smem_item;
>> +       u32 pid;
>> +};
>> +
>> +static const struct subsystem_data subsystems[] = {
>> +       { "modem", 605, 1 },
>> +       { "adsp", 606, 2 },
>> +       { "cdsp", 607, 5 },
>> +       { "slpi", 608, 3 },
>> +       { "gpu", 609, 0 },
>> +       { "display", 610, 0 },
>> +       { "adsp_island", 613, 2 },
>> +       { "slpi_island", 613, 3 },
>> +};
>> +
>> +struct stats_config {
>> +       u32 offset_addr;
>> +       u32 num_records;
> size_t?
u32 will be smaller/num_records will not exceed more than 3 as of now.
>
>> +       bool appended_stats_avail;
>> +};
>> +
>> +struct stats_prv_data {
>> +       bool appended_stats_avail;
>> +       void __iomem *reg;
>> +};
>> +
>> +struct sleep_stats {
>> +       u32 stat_type;
>> +       u32 count;
>> +       u64 last_entered_at;
>> +       u64 last_exited_at;
>> +       u64 accumulated;
>> +};
>> +
>> +struct appended_stats {
>> +       u32 client_votes;
>> +       u32 reserved[3];
>> +};
>> +
>> +static void print_sleep_stats(struct seq_file *s, const struct sleep_stats *stat)
>> +{
>> +       u64 accumulated = stat->accumulated;
>> +       /*
>> +        * If a subsystem is in sleep when reading the sleep stats adjust
>> +        * the accumulated sleep duration to show actual sleep time.
>> +        */
>> +       if (stat->last_entered_at > stat->last_exited_at)
>> +               accumulated += arch_timer_read_counter()
>> +                              - stat->last_entered_at;
>> +
>> +       seq_printf(s, "Count = %u\n", stat->count);
>> +       seq_printf(s, "Last Entered At = %llu\n", stat->last_entered_at);
>> +       seq_printf(s, "Last Exited At = %llu\n", stat->last_exited_at);
>> +       seq_printf(s, "Accumulated Duration = %llu\n", accumulated);
>> +}
>> +
>> +static int subsystem_sleep_stats_show(struct seq_file *s, void *d)
>> +{
>> +       struct subsystem_data *subsystem = s->private;
>> +       struct sleep_stats *stat;
>> +
>> +       /*
>> +        * Saving this pointer during probe may not help in cases like
>> +        * subsystem restart, beside not each subsystem is a remote processor
> s/beside/besides/
> s/each/every/
Fixed in v7.
>
>> +        * for e.g display for which we can get start and stop notification
> for example
> s/notification/notification./
Fixed in v7.
>
>> +        *
>> +        * Lookup smem pointer each time to keep it simple.
>> +        */
>> +       stat = qcom_smem_get(subsystem->pid, subsystem->smem_item, NULL);
>> +       if (IS_ERR(stat))
>> +               return PTR_ERR(stat);
>> +
>> +       print_sleep_stats(s, stat);
>> +
>> +       return 0;
>> +}
>> +
>> +static int soc_sleep_stats_show(struct seq_file *s, void *d)
>> +{
>> +       struct stats_prv_data *prv_data = s->private;
>> +       void __iomem *reg = prv_data->reg;
>> +       struct sleep_stats stat;
>> +
>> +       stat.count = readl(reg + COUNT_ADDR);
>> +       stat.last_entered_at = readq(reg + LAST_ENTERED_AT_ADDR);
>> +       stat.last_exited_at = readq(reg + LAST_EXITED_AT_ADDR);
>> +       stat.accumulated = readq(reg + ACCUMULATED_ADDR);
>> +
>> +       print_sleep_stats(s, &stat);
>> +
>> +       if (prv_data->appended_stats_avail) {
>> +               struct appended_stats app_stat;
>> +
>> +               app_stat.client_votes = readl(reg + CLIENT_VOTES_ADDR);
>> +               seq_printf(s, "Client_votes = %#x\n", app_stat.client_votes);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +DEFINE_SHOW_ATTRIBUTE(soc_sleep_stats);
>> +DEFINE_SHOW_ATTRIBUTE(subsystem_sleep_stats);
>> +
>> +static struct dentry *create_debugfs_entries(void __iomem *reg,
> Can this be soc_sleep_stats_*()? Or inlined at the call site?
> create_debugfs_entries() is too generic.
Renamed with soc_sleep_stats_*() in v7.
>
>> +                                            struct stats_prv_data *prv_data,
>> +                                            u32 num_records)
>> +{
>> +       struct dentry *root;
>> +       struct sleep_stats *stat;
>> +       char stat_type[sizeof(u32)] = {0};
>> +       u32 offset, type;
>> +       int i;
>> +
>> +       root = debugfs_create_dir("qcom_sleep_stats", NULL);
>> +
>> +       for (i = 0; i < num_records; i++) {
>> +               offset = STAT_OFFSET(i, STAT_TYPE_ADDR);
>> +
>> +               if (prv_data[i].appended_stats_avail)
>> +                       offset += APPENDED_STAT_OFFSET(i);
>> +
>> +               prv_data[i].reg = reg + offset;
>> +
>> +               type = readl(prv_data[i].reg);
>> +               memcpy_fromio(stat_type, &type, sizeof(u32));
> type isn't an __iomem pointer though. Did you compile this with sparse?
> Use 'make C=2'
>
> Also, this is really bad design. The mmio register contains a physical
> address of where to read from? It's not an offset from the register or
> something like that? If an IOMMU is rewriting physical addresses this
> driver is screwed and will try to map some other physical address that
> could be completely different based on what the IOMMU decides that
> IPA maps to physically.

dropped use of memcpy_fromio() in v7, instead using local loop to figure 
out the low power mode name.

>
>> +               strim(stat_type);
>> +               debugfs_create_file(stat_type, 0400, root,
>> +                                   &prv_data[i],
>> +                                   &soc_sleep_stats_fops);
>> +       }
>> +
>> +       for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
>> +               stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item,
>> +                                    NULL);
>> +               if (IS_ERR(stat))
>> +                       continue;
>> +
>> +               debugfs_create_file(subsystems[i].name, 0400, root,
>> +                                   (void *)&subsystems[i],
> cast to void shouldn't be needed. Please drop it.

This seems required, without cast, gcc gives below error

./include/linux/debugfs.h:69:16: note: expected ‘void *’ but argument is 
of type ‘const struct subsystem_data *’

>
>> +                                   &subsystem_sleep_stats_fops);
>> +       }
>> +
>> +       return root;
>> +}
>> +
>> +static int soc_sleep_stats_probe(struct platform_device *pdev)
>> +{
>> +       struct resource *res;
>> +       void __iomem *reg;
>> +       void __iomem *offset_addr;
>> +       phys_addr_t stats_base;
>> +       resource_size_t stats_size;
>> +       struct dentry *root;
>> +       const struct stats_config *config;
>> +       struct stats_prv_data *prv_data;
>> +       int i;
>> +
>> +       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 = ioremap(res->start + config->offset_addr, sizeof(u32));
>> +       if (IS_ERR(offset_addr))
>> +               return PTR_ERR(offset_addr);
>> +
>> +       stats_base = res->start | readl_relaxed(offset_addr);
>> +       stats_size = resource_size(res);
>> +       iounmap(offset_addr);
>> +
>> +       reg = devm_ioremap(&pdev->dev, stats_base, stats_size);
>> +       if (!reg)
>> +               return -ENOMEM;
>> +
>> +       prv_data = devm_kcalloc(&pdev->dev, config->num_records,
>> +                               sizeof(struct stats_prv_data), GFP_KERNEL);
> sizeof(*prv_data) please so we don't have to think about the type
> matching the pointer storage.

Updated in v7.

Thanks,
Maulik
>
>> +       if (!prv_data)
>> +               return -ENOMEM;
>> +
>> +       for (i = 0; i < config->num_records; i++)
>> +               prv_data[i].appended_stats_avail = config->appended_stats_avail;
>> +
>> +       root = create_debugfs_entries(reg, prv_data, config->num_records);
>> +       platform_set_drvdata(pdev, root);
>> +
>> +       return 0;
>> +}
>> +
>> +static int soc_sleep_stats_remove(struct platform_device *pdev)
>> +{
>> +       struct dentry *root = platform_get_drvdata(pdev);
>> +
>> +       debugfs_remove_recursive(root);
>> +
>> +       return 0;
>> +}
>> +
diff mbox series

Patch

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 79b568f..e80b63a 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -190,6 +190,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 && DEBUG_FS || COMPILE_TEST
+	depends on QCOM_SMEM
+	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 ad675a6..5f30d74 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 0000000..66df638
--- /dev/null
+++ b/drivers/soc/qcom/soc_sleep_stats.c
@@ -0,0 +1,258 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2011-2021, 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 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
+
+#define STAT_OFFSET(record_no, type) (((record_no)*(sizeof(struct sleep_stats))) + (type))
+#define APPENDED_STAT_OFFSET(record_no) ((record_no)*(sizeof(struct appended_stats)))
+
+struct subsystem_data {
+	const char *name;
+	u32 smem_item;
+	u32 pid;
+};
+
+static const struct subsystem_data subsystems[] = {
+	{ "modem", 605, 1 },
+	{ "adsp", 606, 2 },
+	{ "cdsp", 607, 5 },
+	{ "slpi", 608, 3 },
+	{ "gpu", 609, 0 },
+	{ "display", 610, 0 },
+	{ "adsp_island", 613, 2 },
+	{ "slpi_island", 613, 3 },
+};
+
+struct stats_config {
+	u32 offset_addr;
+	u32 num_records;
+	bool appended_stats_avail;
+};
+
+struct stats_prv_data {
+	bool appended_stats_avail;
+	void __iomem *reg;
+};
+
+struct sleep_stats {
+	u32 stat_type;
+	u32 count;
+	u64 last_entered_at;
+	u64 last_exited_at;
+	u64 accumulated;
+};
+
+struct appended_stats {
+	u32 client_votes;
+	u32 reserved[3];
+};
+
+static void print_sleep_stats(struct seq_file *s, const struct sleep_stats *stat)
+{
+	u64 accumulated = stat->accumulated;
+	/*
+	 * If a subsystem is in sleep when reading the sleep stats adjust
+	 * the accumulated sleep duration to show actual sleep time.
+	 */
+	if (stat->last_entered_at > stat->last_exited_at)
+		accumulated += arch_timer_read_counter()
+			       - stat->last_entered_at;
+
+	seq_printf(s, "Count = %u\n", stat->count);
+	seq_printf(s, "Last Entered At = %llu\n", stat->last_entered_at);
+	seq_printf(s, "Last Exited At = %llu\n", stat->last_exited_at);
+	seq_printf(s, "Accumulated Duration = %llu\n", accumulated);
+}
+
+static int subsystem_sleep_stats_show(struct seq_file *s, void *d)
+{
+	struct subsystem_data *subsystem = s->private;
+	struct sleep_stats *stat;
+
+	/*
+	 * Saving this pointer during probe may not help in cases like
+	 * subsystem restart, beside not each subsystem is a remote processor
+	 * for e.g display for which we can get start and stop notification
+	 *
+	 * Lookup smem pointer each time to keep it simple.
+	 */
+	stat = qcom_smem_get(subsystem->pid, subsystem->smem_item, NULL);
+	if (IS_ERR(stat))
+		return PTR_ERR(stat);
+
+	print_sleep_stats(s, stat);
+
+	return 0;
+}
+
+static int soc_sleep_stats_show(struct seq_file *s, void *d)
+{
+	struct stats_prv_data *prv_data = s->private;
+	void __iomem *reg = prv_data->reg;
+	struct sleep_stats stat;
+
+	stat.count = readl(reg + COUNT_ADDR);
+	stat.last_entered_at = readq(reg + LAST_ENTERED_AT_ADDR);
+	stat.last_exited_at = readq(reg + LAST_EXITED_AT_ADDR);
+	stat.accumulated = readq(reg + ACCUMULATED_ADDR);
+
+	print_sleep_stats(s, &stat);
+
+	if (prv_data->appended_stats_avail) {
+		struct appended_stats app_stat;
+
+		app_stat.client_votes = readl(reg + CLIENT_VOTES_ADDR);
+		seq_printf(s, "Client_votes = %#x\n", app_stat.client_votes);
+	}
+
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(soc_sleep_stats);
+DEFINE_SHOW_ATTRIBUTE(subsystem_sleep_stats);
+
+static struct dentry *create_debugfs_entries(void __iomem *reg,
+					     struct stats_prv_data *prv_data,
+					     u32 num_records)
+{
+	struct dentry *root;
+	struct sleep_stats *stat;
+	char stat_type[sizeof(u32)] = {0};
+	u32 offset, type;
+	int i;
+
+	root = debugfs_create_dir("qcom_sleep_stats", NULL);
+
+	for (i = 0; i < num_records; i++) {
+		offset = STAT_OFFSET(i, STAT_TYPE_ADDR);
+
+		if (prv_data[i].appended_stats_avail)
+			offset += APPENDED_STAT_OFFSET(i);
+
+		prv_data[i].reg = reg + offset;
+
+		type = readl(prv_data[i].reg);
+		memcpy_fromio(stat_type, &type, sizeof(u32));
+		strim(stat_type);
+		debugfs_create_file(stat_type, 0400, root,
+				    &prv_data[i],
+				    &soc_sleep_stats_fops);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
+		stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item,
+				     NULL);
+		if (IS_ERR(stat))
+			continue;
+
+		debugfs_create_file(subsystems[i].name, 0400, root,
+				    (void *)&subsystems[i],
+				    &subsystem_sleep_stats_fops);
+	}
+
+	return root;
+}
+
+static int soc_sleep_stats_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	void __iomem *reg;
+	void __iomem *offset_addr;
+	phys_addr_t stats_base;
+	resource_size_t stats_size;
+	struct dentry *root;
+	const struct stats_config *config;
+	struct stats_prv_data *prv_data;
+	int i;
+
+	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 = ioremap(res->start + config->offset_addr, sizeof(u32));
+	if (IS_ERR(offset_addr))
+		return PTR_ERR(offset_addr);
+
+	stats_base = res->start | readl_relaxed(offset_addr);
+	stats_size = resource_size(res);
+	iounmap(offset_addr);
+
+	reg = devm_ioremap(&pdev->dev, stats_base, stats_size);
+	if (!reg)
+		return -ENOMEM;
+
+	prv_data = devm_kcalloc(&pdev->dev, config->num_records,
+				sizeof(struct stats_prv_data), GFP_KERNEL);
+	if (!prv_data)
+		return -ENOMEM;
+
+	for (i = 0; i < config->num_records; i++)
+		prv_data[i].appended_stats_avail = config->appended_stats_avail;
+
+	root = create_debugfs_entries(reg, prv_data, config->num_records);
+	platform_set_drvdata(pdev, root);
+
+	return 0;
+}
+
+static int soc_sleep_stats_remove(struct platform_device *pdev)
+{
+	struct dentry *root = platform_get_drvdata(pdev);
+
+	debugfs_remove_recursive(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");