Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <151790dd-02e5-a1f5-aab5-360f39e21c57@quicinc.com>
Date:   Thu, 19 Jan 2023 14:18:12 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH] interconnect: Skip call into provider if initial bw is
 zero
Content-Language: en-US
To:     Bryan O'Donoghue <bryan.odonoghue@linaro.org>, <djakov@kernel.org>
CC:     <quic_mdtipton@quicinc.com>, <quic_okukatla@quicinc.com>,
        <linux-pm@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <1673647679-15216-1-git-send-email-quic_viveka@quicinc.com>
 <83a7bfed-3b16-3d01-b1b2-f197252bd0b1@linaro.org>
 <5e1f37ba-494a-19d2-e412-7631508ab142@linaro.org>
From:   Vivek Aknurwar <quic_viveka@quicinc.com>
In-Reply-To: <5e1f37ba-494a-19d2-e412-7631508ab142@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Bryan,
Thanks for taking time to review the patch.

On 1/13/2023 5:40 PM, Bryan O'Donoghue wrote:
> On 14/01/2023 01:24, Bryan O'Donoghue wrote:
>> On 13/01/2023 22:07, Vivek Aknurwar wrote:
>>> Currently framework sets bw even when init bw requirements are zero 
>>> during
>>> provider registration, thus resulting bulk of set bw to hw.
>>> Avoid this behaviour by skipping provider set bw calls if init bw is 
>>> zero.
>>>
>>> Signed-off-by: Vivek Aknurwar <quic_viveka@quicinc.com>
>>> ---
>>>   drivers/interconnect/core.c | 17 ++++++++++-------
>>>   1 file changed, 10 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
>>> index 25debde..43ed595 100644
>>> --- a/drivers/interconnect/core.c
>>> +++ b/drivers/interconnect/core.c
>>> @@ -977,14 +977,17 @@ void icc_node_add(struct icc_node *node, struct 
>>> icc_provider *provider)
>>>       node->avg_bw = node->init_avg;
>>>       node->peak_bw = node->init_peak;
>>> -    if (provider->pre_aggregate)
>>> -        provider->pre_aggregate(node);
>>> -
>>> -    if (provider->aggregate)
>>> -        provider->aggregate(node, 0, node->init_avg, node->init_peak,
>>> -                    &node->avg_bw, &node->peak_bw);
>>> +    if (node->avg_bw || node->peak_bw) {
>>> +        if (provider->pre_aggregate)
>>> +            provider->pre_aggregate(node);
>>> +
>>> +        if (provider->aggregate)
>>> +            provider->aggregate(node, 0, node->init_avg, 
>>> node->init_peak,
>>> +                        &node->avg_bw, &node->peak_bw);
>>> +        if (provider->set)
>>> +            provider->set(node, node);
>>> +    }
>>> -    provider->set(node, node);
>>>       node->avg_bw = 0;
>>>       node->peak_bw = 0;
>>
>> I have the same comment/question for this patch that I had for the 
>> qcom arch specific version of it. This patch seems to be doing at a 
>> higher level what the patch below was doing at a lower level.
>>
>> https://lore.kernel.org/lkml/1039a507-c4cd-e92f-dc29-1e2169ce5078@linaro.org/T/#m0c90588d0d1e2ab88c39be8f5f3a8f0b61396349
>>
>> what happens to earlier silicon - qcom silicon which previously made 
>> explicit zero requests ?

This patch is to optimize and avoid all those bw 0 requests on each node 
addition during probe (which results in rpmh remote calls) for upcoming 
targets.

>>
>> https://lore.kernel.org/lkml/1039a507-c4cd-e92f-dc29-1e2169ce5078@linaro.org/T/#m589e8280de470e038249bb362634221771d845dd
>>
>> https://lkml.org/lkml/2023/1/3/1232
>>
>> Isn't it a better idea to let lower layer drivers differentiate what 
>> they do ?

AFAIU lower layer driver can/should not differentiate between normal 
flow calls vs made as a result from probe/initialization of driver. 
Hence even bw 0 request is honored as like client in general wish to 
vote 0 as in an normal use case.

>>
>> For example on pre 5.4 qcom kernel silicon we might choose to set the 
>> value to zero "because that's what the reference code did" but on 
>> newer silicon we might opt to skip the zero configuration ?
>>
>> I'm happy to be shown the error of my ways but, absent testing to 
>> *show* it doesn't impact existing legacy silicon, I think we should be 
>> wary of this change.
>>
>> ---
>> bod
> 
> Oh, and what is the effect on Samsung and i.MX silicon interconnect 
> providers of skipping the zero set ?

If interconnect providers are trying to clear bw votes coming from 
boot-loader then best place to clear those is in sync-state call back.

> 
> ---
> bod