LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Stephen Boyd <swboyd@chromium.org>
To: Maulik Shah <mkshah@codeaurora.org>,
	bjorn.andersson@linaro.org, evgreen@chromium.org,
	mka@chromium.org
Cc: linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	agross@kernel.org, dianders@chromium.org, rnayak@codeaurora.org,
	ilina@codeaurora.org, lsrao@codeaurora.org,
	Mahesh Sivasubramanian <msivasub@codeaurora.org>,
	Maulik Shah <mkshah@codeaurora.org>
Subject: Re: [PATCH v5 2/4] soc: qcom: Add SoC sleep stats driver
Date: Wed, 18 Mar 2020 17:25:40 -0700	[thread overview]
Message-ID: <158457754092.152100.9555786468515303757@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <1584505758-21037-3-git-send-email-mkshah@codeaurora.org>

Quoting Maulik Shah (2020-03-17 21:29:16)
> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
> new file mode 100644
> index 0000000..0db7c3d
> --- /dev/null
> +++ b/drivers/soc/qcom/soc_sleep_stats.c
> @@ -0,0 +1,244 @@
> +// 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 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 smem_item;
> +       u32 pid;
> +};
> +
> +static struct subsystem_data subsystems[] = {

This can be const.

> +       { "modem", 605, 1 },
> +       { "adsp", 606, 2 },
> +       { "cdsp", 607, 5 },
> +       { "slpi", 608, 3 },
> +       { "gpu", 609, 0 },
> +       { "display", 610, 0 },
> +};
> +
> +struct stats_config {
> +       unsigned int offset_addr;
> +       unsigned int num_records;
> +       bool appended_stats_avail;
> +};
> +
> +struct stats_prv_data {
> +       const struct stats_config *config;

Can we have 'bool has_appended_stats' here instead?

> +       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, struct sleep_stats *stat)

Make stat const please.

> +{
> +       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()

This assumes that arch_timer_read_counter() is returning the physical
counter? Is accumulated duration useful for anything? I don't like that
we're relying on the arch timer code to return a certain counter value
that may or may not be the same as what is written into smem.

> +                              - 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;
> +
> +       stat = qcom_smem_get(subsystem->pid, subsystem->smem_item, NULL);

We already get this during probe in create_debugfs_entries() (which is
too generic of a name by the way). Why can't we stash that pointer away
so that it comes through the 'd' parameter to this function?

> +       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);

Ok I see. The same stat info is in SMEM and also in some IO memory
location? So we have to read one from an iomem region and one from smem?
Please make subsystem_sleep_stats_show() take a 'struct
smem_sleep_stats' through 'd' that has the __le32 and __le64 markings
and then do the byte swaps into a stack local struct sleep_stats that
print_sleep_stats() can use.

> +
> +       if (prv_data->config->appended_stats_avail) {
> +               struct appended_stats app_stat;

Just use a u32 for this. The struct is not useful here.

> +
> +               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)

I'd prefer this was just inlined into probe.

> +{
> +       struct dentry *root;
> +       struct sleep_stats *stat;
> +       char stat_type[sizeof(u32) + 1] = {0};
> +       u32 offset, type;
> +       int i;
> +
> +       root = debugfs_create_dir("qcom_sleep_stats", NULL);
> +
> +       for (i = 0; i < prv_data[0].config->num_records; i++) {

Pass config as another argument instead of attaching it to prv_data
please.

> +               offset = STAT_TYPE_ADDR + (i * sizeof(struct sleep_stats));
> +
> +               if (prv_data[0].config->appended_stats_avail)
> +                       offset += i * sizeof(struct appended_stats);

I was imagining a macro like, SLEEP_STATn(i, has_appended) that did all the
math to figure out the offset.

> +
> +               prv_data[i].reg = reg + offset;
> +
> +               type = readl(prv_data[i].reg);
> +               memcpy(stat_type, &type, sizeof(u32));

Is it a 4-byte ascii register that may or may not be NUL terminated? We
should use __raw_readl() then so we don't do an endian swap. Using
memcpy_fromio() does this all for you.

> +               debugfs_create_file(stat_type, 0444, root,

Maybe just 0400? Not sure why everyone in the world needs to read this.

> +                                   &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, 0444, root,
> +                                   &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);

I still don't get it, but whatever. I highly doubt the lower bits are
anything besides 0 so a logical OR vs addition is not different.

> +       stats_size = resource_size(res);
> +       iounmap(offset_addr);
> +
> +       reg = devm_ioremap(&pdev->dev, stats_base, stats_size);
> +       if (!reg)
> +               return -ENOMEM;
> +
> +       prv_data = devm_kzalloc(&pdev->dev, config->num_records *
> +                               sizeof(struct stats_prv_data), GFP_KERNEL);

We have devm_kcalloc() for array allocations.

> +       if (!prv_data)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < config->num_records; i++)
> +               prv_data[i].config = config;
> +
> +       root = create_debugfs_entries(reg, prv_data);
> +       platform_set_drvdata(pdev, root);
> +
> +       return 0;
> +}

  reply	other threads:[~2020-03-19  0:25 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-18  4:29 [PATCH v5 0/4] Introduce " Maulik Shah
2020-03-18  4:29 ` [PATCH v5 1/4] dt-bindings: Introduce SoC sleep stats bindings Maulik Shah
2020-03-18 21:29   ` Stephen Boyd
2020-03-19 11:35     ` Maulik Shah
2020-03-18  4:29 ` [PATCH v5 2/4] soc: qcom: Add SoC sleep stats driver Maulik Shah
2020-03-19  0:25   ` Stephen Boyd [this message]
2020-03-19 15:14     ` Maulik Shah
2020-03-19 22:39       ` Stephen Boyd
2020-08-22 15:45   ` Guenter Roeck
2020-03-18  4:29 ` [PATCH v5 3/4] arm64: dts: qcom: sc7180: Enable SoC sleep stats Maulik Shah
2020-03-18  4:29 ` [PATCH v5 4/4] arm64: defconfig: Enable SoC sleep stats driver Maulik Shah

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=158457754092.152100.9555786468515303757@swboyd.mtv.corp.google.com \
    --to=swboyd@chromium.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=dianders@chromium.org \
    --cc=evgreen@chromium.org \
    --cc=ilina@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lsrao@codeaurora.org \
    --cc=mka@chromium.org \
    --cc=mkshah@codeaurora.org \
    --cc=msivasub@codeaurora.org \
    --cc=rnayak@codeaurora.org \
    --subject='Re: [PATCH v5 2/4] soc: qcom: Add SoC sleep stats driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).