Re: [PATCH v4 4/5] arm64: dts: sdm845: Add OPP tables and power-domains for venus

From: Rajendra Nayak <rnayak@codeaurora.org>
To: Stephen Boyd <sboyd@kernel.org>, Viresh Kumar <viresh.kumar@linaro.org>
Cc: Stanimir Varbanov <stanimir.varbanov@linaro.org>,
	robh+dt@kernel.org, agross@kernel.org,
	bjorn.andersson@linaro.org, linux-arm-msm@vger.kernel.org,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, mka@chromium.org,
	Taniya Das <tdas@codeaurora.org>
Subject: Re: [PATCH v4 4/5] arm64: dts: sdm845: Add OPP tables and power-domains for venus
Date: Tue, 28 Jul 2020 09:47:28 +0530	[thread overview]
Message-ID: <e08e9822-7c0c-29d7-67b2-245af66b623a@codeaurora.org> (raw)
In-Reply-To: <159589753282.1360974.11628682178494669632@swboyd.mtv.corp.google.com>

On 7/28/2020 6:22 AM, Stephen Boyd wrote:
> Quoting Viresh Kumar (2020-07-27 08:38:06)
>> On 27-07-20, 17:38, Rajendra Nayak wrote:
>>> On 7/27/2020 11:23 AM, Rajendra Nayak wrote:
>>>> On 7/24/2020 7:39 PM, Stanimir Varbanov wrote:
>>>>>>> +
>>>>>>> +                opp-533000000 {
>>>>>>> +                    opp-hz = /bits/ 64 <533000000>;
>>
>> Is this the highest OPP in table ?
>>
>>>>> Actually it comes from videocc, where ftbl_video_cc_venus_clk_src
>>>>> defines 533000000 but the real calculated freq is 533000097.
>>>>
>>>> I still don't quite understand why the videocc driver returns this
>>>> frequency despite this not being in the freq table.
>>>
>>> Ok, so I see the same issue on sc7180 also. clk_round_rate() does seem to
>>> return whats in the freq table, but clk_set_rate() goes ahead and sets it
> 
> I'm happy to see clk_round_rate() return the actual rate that would be
> achieved and not just the rate that is in the frequency tables. Would
> that fix the problem? 

It would, but only if I also update the OPP table to have 533000097
instead of 533000000 (which I guess is needed anyway)
If this is the actual frequency that's achievable, then perhaps even the clock
freq table should have this? 533000097 and not 533000000?
That way clk_round_rate() would return the actual rate that's achieved and
we don't need any extra math. Isn't that the reason these freq tables exist
anyway.

> It may be that we need to make clk_round_rate() do
> some more math on qcom platforms and actually figure out what the rate
> is going to be instead of blindly trust the frequency that has been set
> in the tables.
> 
>>> to 533000097. Subsequently when we try to set a different OPP, it fails to
>>> find the 'current' OPP entry for 533000097. This sounds like an issue with the OPP
>>> framework? Should we not fall back to the highest OPP as the current OPP?
>>>
>>> Stephen/Viresh, any thoughts?
>>
>> I think we (in all frameworks generally) try to set a frequency <=
>> target frequency and so there may be a problem if the frequency is
>> larger than highest supported. IOW, you need to fix tables a bit.
>>
> 
> Rounding is annoying for sure.
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation