linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Nicola Mazzucato <nicola.mazzucato@arm.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: devicetree@vger.kernel.org, linux-pm@vger.kernel.org,
	vireshk@kernel.org, daniel.lezcano@linaro.org, rjw@rjwysocki.net,
	linux-kernel@vger.kernel.org, robh+dt@kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Sudeep Holla <sudeep.holla@arm.com>,
	chris.redpath@arm.com, Ionela Voinescu <ionela.voinescu@arm.com>,
	morten.rasmussen@arm.com, Lukasz Luba <lukasz.luba@arm.com>
Subject: Re: [PATCH v2 2/2] [RFC] CPUFreq: Add support for cpu-perf-dependencies
Date: Mon, 19 Oct 2020 09:50:33 +0100	[thread overview]
Message-ID: <503af305-77a4-964a-ed17-8df8b4e3a546@arm.com> (raw)
In-Reply-To: <20201014042531.r7iykzygkvmpsqck@vireshk-i7>

Hi Viresh,

thank you for your suggestion on using 'opp-shared'.
I think it could work for most of the cases we explained earlier.

Summarising, there are two parts of this entire proposal:
1) where/how to get the information: now we are focusing on taking advantage of
'opp-shared' within an empty opp table
2) and how/where this information will be consumed

Further details below:

1) a CPUFreq driver that takes the OPPs from firmware, can call
dev_pm_opp_of_get_sharing_cpus like you suggested. When doing so, a provided
cpumaksk will be populated with the corresponding cpus that share the same
(empty) table opp in DT.
All good so far.
The current opp core is not expecting an empty table and therefore some errors
are thrown when this happens.
Since we are now allowing this corner-case, I am presenting below where I think
some minor corrections may be needed:

--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
        struct device_node *required_np, *np;
        int count, i;

        /* Traversing the first OPP node is all we need */
        np = of_get_next_available_child(opp_np, NULL);
        if (!np) {
-               dev_err(dev, "Empty OPP table\n");
+               dev_warn(dev, "Empty OPP table\n");
+
+               /*
+                * With empty table we remove shared_opp. This is to leave the
+                * responsibility to decide which opp are shared to the opp users
+                */
+               opp_table->shared_opp = OPP_TABLE_ACCESS_EXCLUSIVE;
+
                return;
        }

@@ int dev_pm_opp_of_find_icc_paths(struct device *dev,
        int ret, i, count, num_paths;
        struct icc_path **paths;

        ret = _bandwidth_supported(dev, opp_table);
-       if (ret <= 0)
+       if (ret == -EINVAL)
+               return 0; /* Empty OPP table is a valid corner-case, let's not
fail */
+       else if (ret <= 0)
                return ret;

The above are not 'strictly' necessary to achieve the intended goal, but they
make clearer that an empty table is now allowed and not an error anymore.
What it is your point of view on this?

In addition, I think it would also be appropriate to update the documentation
(Documentation/devicetree/bindings/opp/opp.txt) to reflect this new case
(required properties etc).
Any different thoughts?

2) Once the driver gets the 'performance dependencies' by
dev_pm_opp_of_get_sharing_cpus(), this information will have to be shared with
EAS, thermal, etc.. The natural way to do so would be to add a new cpumask like
I proposed in this RFC.
I see this as an improvement for the whole subsystem and a scalable choice since
we can unambiguously provide the correct information to whoever needs it, given
that we don't enforce "hw dependencies" for related_cpus.
The changes would be trivial (it's in the original RFC).
On the other hand, we can't unload this h/w detail into related_cpus IMO as we
are dealing with per-cpu systems in this context.
Hope it makes sense?

Once we are pretty much aligned with our ideas, I can run some other tests and
make a V3.

Thank you very much,

Best regards
Nicola

On 10/14/20 5:25 AM, Viresh Kumar wrote:
> On 12-10-20, 18:18, Lukasz Luba wrote:
>> On 10/12/20 5:52 PM, Ionela Voinescu wrote:
>>> On Monday 12 Oct 2020 at 16:49:30 (+0100), Sudeep Holla wrote:
>>>> On Fri, Oct 09, 2020 at 11:09:21AM +0530, Viresh Kumar wrote:
>>>>> - I wonder if we can keep using that instead of creating new bindings
>>>>>    for exact same stuff ? Though the difference here would be that the
>>>>>    OPP may not have any other entries.
>>>>
>>>> Well summarised, sorry for chiming in late. I could have not summarised
>>>> any better. Just saw the big thread and was thinking of summarising.
>>>> If the last point on OPP is possible(i.e. no OPP entries but just use
>>>> it for fetch the information) for $subject patch is trying to achieve,
>>>> then it would be good.
> 
> Under normal circumstances, I wouldn't have suggested empty opp-tables
> for sure but it doesn't seem worth adding another binding to get this
> information out :)
> 
>>>
>>> Just to put in my two pennies worth: using opp-shared (in possibly empty
>>> OPP table) as alternative to cpu-perf-dependencies sounds good enough
>>> to me as well.
>>
>> +1
> 
> Now that (almost) everyone agrees, I don't think we need to make any
> change anywhere, in code or bindings. This should work right now as
> well.  The code should never try to create OPP tables and the core
> will not create one. Your driver (which want to get this information
> out of empty OPP tables) shall call dev_pm_opp_of_get_sharing_cpus(),
> which just parses the DT to get this information out.
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  parent reply	other threads:[~2020-10-19  8:50 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-24  9:53 [PATCH v2 0/2] CPUFreq: Add support for cpu performance dependencies Nicola Mazzucato
2020-09-24  9:53 ` [PATCH v2 1/2] dt-bindings: arm: Add devicetree binding for cpu-performance-dependencies Nicola Mazzucato
2020-10-08 13:42   ` Ionela Voinescu
2020-09-24  9:53 ` [PATCH v2 2/2] [RFC] CPUFreq: Add support for cpu-perf-dependencies Nicola Mazzucato
2020-10-06  7:19   ` Viresh Kumar
2020-10-07 12:58     ` Nicola Mazzucato
2020-10-08 11:02       ` Viresh Kumar
2020-10-08 15:03         ` Ionela Voinescu
2020-10-08 15:57           ` Rafael J. Wysocki
2020-10-08 17:08             ` Ionela Voinescu
2020-10-12 16:06             ` Sudeep Holla
2020-10-08 16:00           ` Nicola Mazzucato
2020-10-09  5:39             ` Viresh Kumar
2020-10-09 11:10               ` Nicola Mazzucato
2020-10-09 11:17                 ` Viresh Kumar
2020-10-09 14:01                 ` Rob Herring
2020-10-09 15:28                   ` Nicola Mazzucato
2020-10-12  4:19                     ` Viresh Kumar
2020-10-12 10:22                   ` Lukasz Luba
2020-10-12 10:50                     ` Rafael J. Wysocki
2020-10-12 11:05                       ` Lukasz Luba
2020-10-12 10:59                     ` Ionela Voinescu
2020-10-12 13:48                       ` Lukasz Luba
2020-10-12 16:30                         ` Ionela Voinescu
2020-10-12 18:19                           ` Lukasz Luba
2020-10-12 22:01                             ` Ionela Voinescu
2020-10-13 11:53                               ` Rafael J. Wysocki
2020-10-13 12:39                                 ` Ionela Voinescu
2020-10-15 15:56                                   ` Rafael J. Wysocki
2020-10-15 18:38                                     ` Ionela Voinescu
2020-10-12 13:59                     ` Rob Herring
2020-10-12 16:02                     ` Sudeep Holla
2020-10-12 15:54                   ` Sudeep Holla
2020-10-12 15:49               ` Sudeep Holla
2020-10-12 16:52                 ` Ionela Voinescu
2020-10-12 17:18                   ` Lukasz Luba
2020-10-14  4:25                     ` Viresh Kumar
2020-10-14  9:11                       ` Lukasz Luba
2020-10-19  8:50                       ` Nicola Mazzucato [this message]
2020-10-19  9:46                         ` Viresh Kumar
2020-10-19 13:36                           ` Nicola Mazzucato
2020-10-20 10:48                             ` Viresh Kumar
2020-10-13 13:53               ` Lukasz Luba
2020-10-14  4:20                 ` Viresh Kumar

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=503af305-77a4-964a-ed17-8df8b4e3a546@arm.com \
    --to=nicola.mazzucato@arm.com \
    --cc=chris.redpath@arm.com \
    --cc=daniel.lezcano@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=ionela.voinescu@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=lukasz.luba@arm.com \
    --cc=morten.rasmussen@arm.com \
    --cc=rjw@rjwysocki.net \
    --cc=robh+dt@kernel.org \
    --cc=sudeep.holla@arm.com \
    --cc=viresh.kumar@linaro.org \
    --cc=vireshk@kernel.org \
    --subject='Re: [PATCH v2 2/2] [RFC] CPUFreq: Add support for cpu-perf-dependencies' \
    /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).