Message-ID: <158465757824.152100.16700786589009071452@swboyd.mtv.corp.google.com>
Date:   Thu, 19 Mar 2020 15:39:38 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Maulik Shah <mkshah@...eaurora.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 v5 2/4] soc: qcom: Add SoC sleep stats driver

Quoting Maulik Shah (2020-03-19 08:14:03)
> On 3/19/2020 5:55 AM, Stephen Boyd wrote:
> > 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 @@
[..]
> >> +
> >> +static struct subsystem_data subsystems[] = {
> > This can be const.
> we are passing each element of this in debugfs_create_file as (void * data)
> making this const is saying error passing const as void *.

I think we can cast it and have a comment. This way the structure can be
placed in the RO section of memory.

> >> +       { "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?
> any reason to move? this is per compatible config where rpm based target have appended stats available.

It avoids one indirection to figure out something and makes the code
clearer. prv_data doesn't have any use for config otherwise.

> >
> >> +{
> >> +       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.
> 
> we are not comparing it with what is written into smem. The idea here is to adjust the accumulated sleep length to reflect close/real value.
> 
> When a subsystem goes to sleep, it updates entry time and then stays in sleep.
> Exit time and accumulated duration is updated when subsystem comes out of low power mode.

Who is updating the accumulated time? This debugfs code? Or the
subsystems themselves?

> 
> Now if Subsystem updated entry time and went in sleep, while printing accumulated duration
> will keep giving older value.
> 
> If read it at interval of say every 10 seconds, if subsystem never comes out during this.
> Even after 10 seconds, it gives older accumulated duration, while we want to get
> printed as close to real value. so its updated here.
> 
> Now when it comes out of sleep, it always prints value given from subsystem.

Thanks, I understand the motivation.

> 
> >
> >> +                              - 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). 
> I will update the name.
> > Why can't we stash that pointer away
> > so that it comes through the 'd' parameter to this function?
> i think you are missing a subsystem restart case, in that pointer may be updated to new value.
> so we can not just save it and re-use it every time, we don't know when subsystem restart happens and we now need new pointer.

Yes I'm missing subsystem restart. A comment would certainly help. If
this is the case, maybe this should be libified and each subsystem
driver should call some add/remove function in here so that we can
refetch the smem pointer. Doing the lookup each time isn't great.

> >
> >> +
> >> +               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.
> 
> memcpy_fromio() seems not copying properly with u32 or u32+1 size.  seems it need align to 8 byte.
> 
> so when i pass u64 size it seems working fine. i will change this to use memcpy_fromio with sizeof(u64).

What is the failure? Do you need to read 4 bytes at a time? On arm64
memcpy_fromio() looks to do byte size reads if the size isn't 8 bytes.
Maybe just use __raw_readl() then if it is only 4 bytes and you need a
word accessor.

Powered by blists - more mailing lists