Re: [PATCH V3 2/8] soc: qcom: geni: Support for ICC voting

From: Akash Asthana <akashast@codeaurora.org>
To: Georgi Djakov <georgi.djakov@linaro.org>,
	Evan Green <evgreen@chromium.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Andy Gross <agross@kernel.org>,
	wsa@the-dreams.de, Mark Brown <broonie@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Rob Herring <robh+dt@kernel.org>,
	linux-i2c@vger.kernel.org, linux-spi@vger.kernel.org,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>, Stephen Boyd <swboyd@chromium.org>,
	Manu Gautam <mgautam@codeaurora.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	linux-serial@vger.kernel.org,
	Matthias Kaehlcke <mka@chromium.org>,
	Doug Anderson <dianders@chromium.org>
Subject: Re: [PATCH V3 2/8] soc: qcom: geni: Support for ICC voting
Date: Wed, 8 Apr 2020 16:43:30 +0530	[thread overview]
Message-ID: <6f738ea2-d50c-3524-798c-b60e982f2c25@codeaurora.org> (raw)
In-Reply-To: <866a5cac-9f05-703e-8c3c-168d8f219c4d@linaro.org>

Hi Georgi, Bjorn, Evan,

On 4/7/2020 3:28 PM, Georgi Djakov wrote:
> Hi,
>
> On 4/7/20 09:46, Akash Asthana wrote:
>> Hi Bjorn, Evan,
>>
>>>> Given that these two functions only switch the bandwidth request between
>>>> some value and 0, I really think we should carry a "bool enabled" on the
>>>> path and replace these two functions with
>>>> icc_bulk_enable()/icc_bulk_disable().
>> So, if above is implementation "bool enabled" on path can be used directly in
>> aggregation of ICC votes on particular node without using icc_set_bw call, if
>> yes then I am not aware how? or we'll be using icc_set_bw API indirectly inside
>> icc_bulk APIs?
> If there is a repeated pattern to switch between some bandwidth value and zero,
> it really makes sense to introduce such functions in the framework core. I think
> that this might be very useful especially for suspend and resume cases.
> Something like icc_{enable,disable}(struct icc_path *path) functions and also
> the bulk versions, that will flag the path as disabled, re-aggregate and do
> icc_set_bw().

This appears to be a non-trivial change to ICC core, as my understanding 
of ICC core is limited as of now hence, I am not very clear of the 
implementation of icc_bulk APIs.

Will it be okay if I keep geni_icc_vote_on/off API as 
is@https://patchwork.kernel.org/patch/11467511/ for now and later will 
switch to icc_bulk once it's introduced in ICC core.

Regards,

Akash

>>>> The added benefit of this would be that you call icc_set_bw() instead of
>>>> changing the geni_icc_path->{avg_bw,peak_bw} and don't need to keep
>>>> track of them here.
>> Ok IIUC, we need to call icc_set_bw() from GENI driver only if we change (avg_bw
>> | peak_bw)?
> Yes, exactly.
>
> Thanks,
> Georgi
>
>> Regards,
>>
>> Akash
>>
>>> Yes yes! I had the same thought here [1].
>>>
>>> Georgi, what do you think?
>>> -Evan
>>>
>>> [1]
>>> https://lore.kernel.org/linux-arm-msm/CAE=gft58QsgTCUHMHKJhcM9ZxAeMiY16CrbNv2HaTCRqwtmt7A@mail.gmail.com/
>>>
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project