Message-ID: <1e533194-7047-8342-b426-f607fddbfaa3@linaro.org>
Date:   Tue, 10 May 2022 15:09:20 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Viresh Kumar <viresh.kumar@...aro.org>,
        Stephen Boyd <sboyd@...nel.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Michael Turquette <mturquette@...libre.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        Alim Akhtar <alim.akhtar@...sung.com>,
        Avri Altman <avri.altman@....com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Taniya Das <tdas@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, linux-scsi@...r.kernel.org
Subject: Re: [RFC PATCH v2 4/6] PM: opp: allow control of multiple clocks

On 10/05/2022 06:40, Viresh Kumar wrote:
> On 09-05-22, 12:38, Krzysztof Kozlowski wrote:
>> On 25/04/2022 09:27, Viresh Kumar wrote:
>>> This is tricky as the OPP core can't really assume the order in which the clocks
>>> needs to be programmed. We had the same problem with multiple regulators and the
>>> same is left for drivers to do via the custom-api.
>>>
>>> Either we can take the same route here, and let platforms add their own OPP
>>> drivers which can handle this, Or hide this all behind a basic device clock's
>>> driver, which you get with clk_get(dev, NULL).
>>
>> For my use case, the order of scaling will be the same as in previous
>> implementation, because UFS drivers just got bunch of clocks with
>> freq-table-hz property and were scaling in DT order.
>>
>> If drivers need something better, they can always provide custom-opp
>> thus replacing my method. My implementation here does not restrict them.
>>
>> For the drivers where the order does not matter, why forcing each driver
>> to provide its own implementation of clock scaling? Isn't shared generic
>> PM OPP code a way to remove code duplication?
> 
> Code duplication is a good argument and I am in favor of avoiding it,
> but nevertheless this shouldn't be something which platforms can pick
> by mistake, just because they didn't go through core code. In other
> words, this shouldn't be the default behavior of the core.
> 
> If we want, core can provide a helper to get rid of the duplication
> though, but the user explicitly needs to use it.

OK, that sounds like a solution.

> 
>>>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>>>
>>>> +static int _generic_set_opp_clks_only(struct device *dev,
>>>> +				      struct opp_table *opp_table,
>>>> +				      struct dev_pm_opp *opp)
>>>> +{
>>>> +	int i, ret;
>>>> +
>>>> +	if (!opp_table->clks)
>>>> +		return 0;
>>>> +
>>>> +	for (i = 0; i < opp_table->clk_count; i++) {
>>>> +		if (opp->rates[i]) {
>>>
>>> This should mean that we can disable that clock and it isn't required.
>>
>> No, it does not mean that. The DT might provide several clocks which
>> only some are important for frequency scaling. All others just need to
>> be enabled.
>>
>> Maybe you prefer to skip getting such clocks in PM OPP?
> 
> They shouldn't reach the OPP core then. What will the OPP core do if a
> clock has a value for one OPP and not the other ?

That would be the same mistake as providing one voltage as 0 or with
something outside of a spec (but still within regulators min/max).
Mistakes in DTS create undesirable behavior and this part is no different.

However I understand your point - since the driver provides the list of
clocks to OPP, it should not provide ones which are irrelevant.

> 
>>>> @@ -969,8 +1008,8 @@ static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
>>>
>>> I think this routine breaks as soon as we add support for multiple clocks.
>>> clks[0]'s frequency can be same for multiple OPPs and this won't get you the
>>> right OPP then.
>>
>> I don't think so and this was raised also by Stephen - only the first
>> clock is considered the one used for all PM OPP frequency operations,
>> like get ceil/floor.
> 
> IMHO, this is broken by design. I can easily see that someone wants to
> have few variants of all other frequencies for the same frequency of
> the so called "main" clock, i.e. multiple OPPs with same "main" freq
> value.  I don't think we can mark the clocks "main" or otherwise as
> easily for every platform.
> 
> Stephen, any inputs on this ?

In such case, matching opps by frequency would be a quite different API.
The drivers can use now:
https://github.com/krzk/linux/commit/ebc31798494fcc66389ae409dce6d9489c16156a#diff-b6370444c32afa2e55d9b6150f355ba6f4d20c5ed5da5399ea8295d323de8267R1200

If you assume that this frequency can be used for multiple OPPs, then
the API should be different. Something like:
int dev_pm_opp_set_rate(struct device *dev, unsigned long *target_freqs,
                        size_t num_freqs);

Finding right opp for given frequencies would be also quite much more
complicated task. Not a simple ceil/floor search by one frequency.

I don't need that use-case and my implementation does not prevent anyone
from implementing it in the future. IOW, why developing now complex
solution which no one currently needs? If anyone needs such scaling by
multiple-frequencies, the PM OPP can be reworked/extended/improved again.

Additionally let me point also that my implementation targets not a
specific one driver, but actually entire subsystem of drivers - all UFS
drivers.

> 
>> The assumption (which might need better documentation) is that first
>> clock frequency is the main one:
>> 1. It is still in opp->rate field, so it is used everywhere when OPPs
>> are compared/checked for rates.
>> 1. Usually is used also in opp-table nodes names.
>>
>> The logical explanation is that devices has some main operating
>> frequency, e.g. the core clock, and this determines the performance. In
>> the same time such device might not be able to scale this one core clock
>> independently from others, therefore this set of patches.
> 
> I understand what you are saying, but I can feel that it will break or
> will force bad bug-fixes into the core at a later point of time.
> 
> I think it would be better to take it slowly and see how it goes. Lets
> first add support for the OPP core to parse and store this data and
> then we can add support to use it, or at least do all this in separate
> patches so they are easier to review/apply.

Sure, I'll split the patch to smaller chunks.

Best regards,
Krzysztof

Powered by blists - more mailing lists