Re: [PATCH 00/53] icc-rpmh multi-RSC voting groundwork

[Konrad Dybcio]

On 15.09.2023 18:05, Mike Tipton wrote:
> On Fri, Sep 15, 2023 at 03:43:27PM +0200, Konrad Dybcio wrote:
>> On 14.09.2023 04:32, Mike Tipton wrote:
>>> On Wed, Sep 13, 2023 at 10:31:49AM +0200, Konrad Dybcio wrote:
>>>>> The applicable voters should likely be defined in the target-specific
>>>>> headers, rather than the common qcom,icc.h. The bit range used for them
>>>>> could be common, but each target may only support a small subset of the
>>>>> total set of possible voters across all targets.
>>>> I'm not sure how client drivers would then choose the
>>>> correct path other than
>>>>
>>>> switch (soc) {
>>>> case 8450:
>>>> 	tag = QCOM_ICC_TAG_VOTER_8450_HLOS;
>>>> 	break;
>>>> case 8550:
>>>> 	tag = QCOM_ICC_TAG_VOTER_8550_HLOS;
>>>> 	break;
>>>> ...
>>>> }
>>>>
>>>> which would be unacceptable.
>>>
>>> The same general way it's handled for the endpoint bindings, which are
>>> already target-specific. 
>>>
>>> Any client drivers hardcoding the endpoint bindings in their driver
>>> would have to include the appropriate, target-specific binding header
>>> (e.g. qcom,sm8550-rpmh.h). That would only be possible if their driver
>>> file is itself target-specific. Otherwise, it would have to pull the
>>> endpoint bindings from devicetree. Or just use the recommended
>>> of_icc_get() and let devicetree do everything for them. Same for the
>>> target-specific voter tag bindings.
>>>
>>> Clients can also specify their tags in devicetree. They don't actually
>>> have to call icc_set_tag() directly. For example:
>>>
>>>     #include <dt-bindings/interconnect/qcom,sm8450.h>
>>>
>>>     interconnects = <&mmss_noc MASTER_MDP QCOM_ICC_TAG_VOTER_DISP
>>>                      &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_VOTER_DISP>;
>>>
>>> Then when they call of_icc_get() for this path it'll automatically have
>>> QCOM_ICC_TAG_VOTER_DISP set for them.
>> I think I'd skew towards the "define everything in the DT" approach.
>>
>> One thing that makes me uneasy to go on with this approach is the
>> question whether there is a case in which we would want to switch
>> from e.g. voting through DISP to voting through APPS (or similar)
>> from within a single device.
> 
> It shouldn't be common. But it could be done fairly simply by listing
> paths for each different voter in the dt properties. 
> 
>     interconnects = <&mmss_noc MASTER_MDP QCOM_ICC_TAG_VOTER_APPS
>                      &mc_virt SLAVE_EBI1  QCOM_ICC_TAG_VOTER_APPS>,
>                     <&mmss_noc MASTER_MDP QCOM_ICC_TAG_VOTER_DISP
>                      &mc_virt SLAVE_EBI1  QCOM_ICC_TAG_VOTER_DISP>,
>     interconnect-names = "path-apps-voter",
>                          "path-disp-voter";
Eeeeeh, I don't know.. this almost sounds like a patch-up solution
to a problem that doesn't quite yet exist.

I debated introducing a third interconnect cell for this, but I am
not sure the added complexity is worth it.


Having a global set of RSC-bound tags would be a "nice" and tidy
solution.. Maybe we could even allocate like 24 bits to these, as
I don't think you'll be introducing new buckets (or at least I hope
you won't!).

24 is an obscene amount of RSCs to have, even counting virtual
channels, so unless you folks have some dark plans to make all
pieces of hardware powered completely separately from each other,
I suppose I could ask for a pinky-promise to not exceed that
number, ever :D

Konrad