Message-ID: <113b50f8-35f6-73fc-4fc9-302262927c5e@quicinc.com>
Date:   Fri, 23 Jun 2023 13:07:10 -0700
From:   Mike Tipton <quic_mdtipton@...cinc.com>
To:     Bjorn Andersson <quic_bjorande@...cinc.com>,
        Konrad Dybcio <konrad.dybcio@...aro.org>
CC:     <neil.armstrong@...aro.org>, Andy Gross <agross@...nel.org>,
        "Bjorn Andersson" <andersson@...nel.org>,
        Georgi Djakov <djakov@...nel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-pm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 0/4] interconnect: qcom: rpmh: sm8550: mask to send as
 vote

On 6/23/2023 11:58 AM, Bjorn Andersson wrote:
> On Fri, Jun 23, 2023 at 03:58:09PM +0200, Konrad Dybcio wrote:
>> On 23.06.2023 14:50, neil.armstrong@...aro.org wrote:
>>> On the SM8550 SoC, some nodes requires a specific bit mark
>>> instead of a bandwidth when voting.
>>>
>>> Add an enable_mask variable to be used instead of bandwidth.
>>>
>>> Signed-off-by: Neil Armstrong <neil.armstrong@...aro.org>
>>> ---
>> After reviewing this patchset and taking a peek at older downstream,
>> it looks like ACV should be using 0x8 bmask on *all RPMh SoCs*.
>>
>> It's worth noting however, that 8350's downstream (the first msm
>> kernel using the icc framework) did not incorporate that change.
>> Not sure if intentionally or not. Probably not. Might be worth to
>> poke Qcom to backport it in such case. If 8350 is still supported.
>> Probably not.
>>
> 
> Your observation is correct.

Mostly correct. Historically it's always been 0x8, but it's not 
guaranteed. And it will be different on some upcoming SoCs.

> 
> But, note further that command db reports ACV to have data-width of 0,
> resulting in the numerator, and thereby vote_x and vote_y always being
> 0.
> 
> This is downstream worked around by:
> https://git.codelinaro.org/clo/la/kernel/msm-5.15/-/commit/4d2818084015df1e05274ebcc5a0d21e6d256f93
> 
> Which should cause vote_x and vote_y to be non-zero. However without
> this series (and enable_mask defined for ACV on all platforms) the votes
> placed in the BCM would then be garbage...
> 
> 
> 
> That said, unless I'm missing something the math involved here is
> unnecessary.For BCMs with enable_mask, if for any node sum_avg[bucket]
> or max_peak[bucket] is non-zero then the calculated vote_x and vote_y
> comes out non-zero and we write the mask, otherwise 0.

You're not missing anything. The full aggregation logic isn't necessary 
for BCMs with an enable_mask. It was just a bit simpler to implement 
this way. And the extra time spent in the aggregation logic should be 
minimal. But, it could certainly be rewritten to have an entirely 
separate, simpler "aggregation" loop than the full BCMs.

> 
> Rewritten to avoid all the unnecessary multiplication and divisions, we
> wouldn't care about the unit or width and thereby don't need above
> referenced patch.

Yeah, the patch shouldn't be necessary anymore in that case. Though 
keeping it would protect us against div-by-zero in case of something 
unexpected in cmd_db.

> 
> 
> A further tangent here is that a BCM with enable_mask != BIT(0) but
> keepalive set, a 0-bandwidth vote in AMC would result in an invalid
> (undefined?) BCM value being written out in the snippet below the loop.

True, though in practice it should never be a problem. Currently, there 
are only two use cases for enable_mask -- "on/off" BCMs and ACV. The 
enable_mask for on/off BCMs is always 0x1. The only time enable_mask != 
0x1 is for ACV, but keepalive should never be set for ACV. I agree this 
is a bit of a logical hole, though. And could break in the future for 
as-yet undefined usage of enable_mask.

> 
>> Check out these snippets:
>>
>> https://git.codelinaro.org/clo/la/kernel/msm-4.19/-/blob/LA.UM.10.2.1.c25/drivers/soc/qcom/msm_bus/msm_bus_arb_rpmh.c#L556-567
>>
>> https://git.codelinaro.org/clo/la/kernel/msm-4.19/-/blob/LA.UM.10.2.1.c25/drivers/soc/qcom/msm_bus/msm_bus_arb_rpmh.c#L475-495
>>
>> Notice how acv is never updated beyond effectively setting =0 or =bmask,
>> perhaps Qualcomm never implemented something else..
>>
>> Since this series is fine as-is, I'd be happy to see an incremental one.
>> Reported-by would be cool as well :D
> 
> I agree, let's get this merged, backported to stable, and then fix ACV
> handling in a follow up commit (which doesn't necessarily need to hit
> stable).
> 
> You should have a Jira card for this one already, but I don't mind
> sharing the Reported-by with you ;)
> 
> Regards,
> Bjorn
> 
>>
>> Konrad
>>> Changes in v2:
>>> - Took downstream patch for patch 1
>>> - Added konrad's reviewed tag
>>> - Added changes for sm8450 and sa8775p
>>> - Link to v1: https://lore.kernel.org/r/20230619-topic-sm8550-upstream-interconnect-mask-vote-v1-0-66663c0aa592@linaro.org
>>>
>>> ---
>>> Mike Tipton (1):
>>>        interconnect: qcom: Add support for mask-based BCMs
>>>
>>> Neil Armstrong (3):
>>>        interconnect: qcom: sm8450: add enable_mask for bcm nodes
>>>        interconnect: qcom: sm8550: add enable_mask for bcm nodes
>>>        interconnect: qcom: sa8775p: add enable_mask for bcm nodes
>>>
>>>   drivers/interconnect/qcom/bcm-voter.c |  5 +++++
>>>   drivers/interconnect/qcom/icc-rpmh.h  |  2 ++
>>>   drivers/interconnect/qcom/sa8775p.c   |  1 +
>>>   drivers/interconnect/qcom/sm8450.c    |  9 +++++++++
>>>   drivers/interconnect/qcom/sm8550.c    | 17 +++++++++++++++++
>>>   5 files changed, 34 insertions(+)
>>> ---
>>> base-commit: 47045630bc409ce6606d97b790895210dd1d517d
>>> change-id: 20230619-topic-sm8550-upstream-interconnect-mask-vote-96aa20355158
>>>
>>> Best regards,

Powered by blists - more mailing lists