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

[Rajendra Nayak <rnayak@codeaurora.org>]

On 7/27/2020 11:23 AM, Rajendra Nayak wrote:
> 
> 
> On 7/24/2020 7:39 PM, Stanimir Varbanov wrote:
>> Hi,
>>
>> On 7/23/20 9:06 PM, Stanimir Varbanov wrote:
>>> Hi Rajendra,
>>>
>>> After applying 2,3 and 4/5 patches on linaro-integration v5.8-rc2 I see
>>> below messages on db845:
>>>
>>> qcom-venus aa00000.video-codec: dev_pm_opp_set_rate: failed to find
>>> current OPP for freq 533000097 (-34)
>>>
>>> ^^^ This one is new.
>>>
>>> qcom_rpmh TCS Busy, retrying RPMH message send: addr=0x30000
>>>
>>> ^^^ and this message is annoying, can we make it pr_debug in rpmh?
>>>
>>> On 7/23/20 2:26 PM, Rajendra Nayak wrote:
>>>> Add the OPP tables in order to be able to vote on the performance state of
>>>> a power-domain.
>>>>
>>>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>>>> ---
>>>>   arch/arm64/boot/dts/qcom/sdm845.dtsi | 40 ++++++++++++++++++++++++++++++++++--
>>>>   1 file changed, 38 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>>> index e506793..5ca2265 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>>> @@ -3631,8 +3631,10 @@
>>>>               interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>;
>>>>               power-domains = <&videocc VENUS_GDSC>,
>>>>                       <&videocc VCODEC0_GDSC>,
>>>> -                    <&videocc VCODEC1_GDSC>;
>>>> -            power-domain-names = "venus", "vcodec0", "vcodec1";
>>>> +                    <&videocc VCODEC1_GDSC>,
>>>> +                    <&rpmhpd SDM845_CX>;
>>>> +            power-domain-names = "venus", "vcodec0", "vcodec1", "cx";
>>>> +            operating-points-v2 = <&venus_opp_table>;
>>>>               clocks = <&videocc VIDEO_CC_VENUS_CTL_CORE_CLK>,
>>>>                    <&videocc VIDEO_CC_VENUS_AHB_CLK>,
>>>>                    <&videocc VIDEO_CC_VENUS_CTL_AXI_CLK>,
>>>> @@ -3654,6 +3656,40 @@
>>>>               video-core1 {
>>>>                   compatible = "venus-encoder";
>>>>               };
>>>> +
>>>> +            venus_opp_table: venus-opp-table {
>>>> +                compatible = "operating-points-v2";
>>>> +
>>>> +                opp-100000000 {
>>>> +                    opp-hz = /bits/ 64 <100000000>;
>>>> +                    required-opps = <&rpmhpd_opp_min_svs>;
>>>> +                };
>>>> +
>>>> +                opp-200000000 {
>>>> +                    opp-hz = /bits/ 64 <200000000>;
>>>> +                    required-opps = <&rpmhpd_opp_low_svs>;
>>>> +                };
>>>> +
>>>> +                opp-320000000 {
>>>> +                    opp-hz = /bits/ 64 <320000000>;
>>>> +                    required-opps = <&rpmhpd_opp_svs>;
>>>> +                };
>>>> +
>>>> +                opp-380000000 {
>>>> +                    opp-hz = /bits/ 64 <380000000>;
>>>> +                    required-opps = <&rpmhpd_opp_svs_l1>;
>>>> +                };
>>>> +
>>>> +                opp-444000000 {
>>>> +                    opp-hz = /bits/ 64 <444000000>;
>>>> +                    required-opps = <&rpmhpd_opp_nom>;
>>>> +                };
>>>> +
>>>> +                opp-533000000 {
>>>> +                    opp-hz = /bits/ 64 <533000000>;
>>
>> 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
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 would expect a clk_round_rate() when called with 533000097 to return
> a 533000000.
> 
> Taniya, Do you know why?
> 
>>
>> If I change to opp-hz = /bits/ 64 <533000097> the error disappear.
>>
>> I guess we have to revisit m/n and/or pre-divider for this freq when the
>> source pll is P_VIDEO_PLL0_OUT_MAIN PLL?
>>
>>>> +                    required-opps = <&rpmhpd_opp_turbo>;
>>>> +                };
>>>> +            };
>>>>           };
>>>>           videocc: clock-controller@ab00000 {
>>>>
>>>
>>
> 

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