Re: [1/4] interconnect: qcom: icc-rpmh: Add QoS config support

[Odelu Kukatla <quic_okukatla@quicinc.com>]

On 2/22/2024 6:00 PM, Konrad Dybcio wrote:
> 
> 
> On 2/21/24 18:57, Odelu Kukatla wrote:
>>
>>
>> On 2/5/2024 5:51 PM, Konrad Dybcio wrote:
>>> On 5.02.2024 12:57, Odelu Kukatla (QUIC) wrote:
>>>> -----Original Message-----
>>>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> Sent: Monday, January 22, 2024 8:55 PM
>>>> To: Odelu Kukatla (QUIC) <quic_okukatla@quicinc.com>; georgi.djakov@linaro.org; Bjorn Andersson <andersson@kernel.org>; Georgi Djakov <djakov@kernel.org>; linux-arm-msm@vger.kernel.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
>>>> Subject: Re: [1/4] interconnect: qcom: icc-rpmh: Add QoS config support
> 
> Why is this email taken private?
> 
> [...]
> 
I am adding previous list back for now, will add "to list" from "./scripts/get_maintainer.pl" in V3.
>>>
>>
>> Yeah, you are right. !synced_state holds all BCMs enabled. But clock voting is still required.
>> But qcom_icc_rpmh_configure_qos() can be invoked from out side of qcom_icc_rpmh_probe() in QuickBoot mode where *_probe does not happen.
>> so irrespective of !synced_state votes, better to keep it separate from sync_state() and place votes on required BCMs during QoS time and remove them after that.
> 
> What on earth is quickboot mode?
> Grepping for it on linux-next, there's precisely 0 results.
> 
> If it's some downstream sw invention, it has no right to affect
> the code submitted upstream - "if it's not on the list, it doesn't
> exist".
> 

Quickboot is not there yet in upstream. I will clean the code related to BCM voting and move the required clock voting to qcom_icc_rpmh_configure_qos().

>>
>>>>> + * @prio: priority value assigned to requests on the node
>>>>> + * @urg_fwd: if set, master priority is used for requests.
>>>>
>>>> "master priority" meaning "this req goes before anyone else", or "use the icc provider [master]'s priority value"?
>>>>
>>>>> + * @prio_fwd_disable: if set, master priority is ignored and NOCs default priority is used.
>>>>
>>>> NoC's
>>>>
>>>> This sounds like !(prio || urg_fwd)? Surely it must do something more useful?
>>>
>>> This remains unanswered
>>>
>>> Konrad
>>
>> @prio_fwd_disable:
>> 1 -> FIXED MODE; upstream tnx-inband-priority is ignored; value @pio is used for transaction.
>> 0 -> BYPASS MODE; max of (tnx-inband-priority, @prio).
> 
> So, this could be basically called "force_priority_val"
> 
> 
>> @urg_fwd:
>> 1 -> Urgency promotion messages issued by the connected master IP remain unchanged as they transit the NIU and the QNoC QoS generator.
>> 0 -> IP-generated messages are discarded, and messages generated by QoS generators are used instead.
> 
> So the interconnect endpoints can effectively modify the priority if
> urg_fwd is enabled.. Interesting.. "ignore_ep_urgency_req"?
> 

Names were chosen closer to bit fields in QoS registers. I will keep the names same and update the documentation with clear explanation.

> Both of these should also be better documented, the kerneldoc you
> submitted explains nothing unless this what I believe to be excerpt
> from the documentation is present to support it.
> 
> Konrad

Thanks,
Odelu