Re: [PATCH 2/4] interconnect: qcom: rpm: Set QoS parameters regardless of RPM bw setting

[Konrad Dybcio]

On 4.01.2023 00:43, Bryan O'Donoghue wrote:
> On 03/01/2023 17:30, Konrad Dybcio wrote:
>> QoS parameters and RPM bandwidth requests are wholly separate. Setting one
>> should only depend on the description of the interconnect node and not
>> whether the other is present. If we vote through RPM, QoS parameters
>> should be set so that the bus controller can make better decisions.
> 
> Is that true ?
> 
>> If we don't vote through RPM, QoS parameters should be set regardless,
>> as we're requesting additional bandwidth by setting the interconnect
>> clock rates.
>>
>> The Fixes tag references the commit in which this logic was added, it
>> has since been shuffled around to a different file, but it's the one
>> where it originates from.
>>
>> Fixes: f80a1d414328 ("interconnect: qcom: Add SDM660 interconnect provider driver")
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
>> ---
>>   drivers/interconnect/qcom/icc-rpm.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
>> index 06e0fee547ab..a190a0a839c8 100644
>> --- a/drivers/interconnect/qcom/icc-rpm.c
>> +++ b/drivers/interconnect/qcom/icc-rpm.c
>> @@ -252,8 +252,10 @@ static int __qcom_icc_set(struct icc_node *n, struct qcom_icc_node *qn,
>>           ret = qcom_icc_rpm_set(qn->mas_rpm_id, qn->slv_rpm_id, sum_bw);
>>           if (ret)
>>               return ret;
>> -    } else if (qn->qos.qos_mode != -1) {
>> -        /* set bandwidth directly from the AP */
>> +    }
>> +
>> +    if (qn->qos.qos_mode != NOC_QOS_MODE_INVALID) {
>> +        /* Set QoS params from the AP */
>>           ret = qcom_icc_qos_set(n, sum_bw);
>>           if (ret)
>>               return ret;
> 
> Taking the example of
> 
> static struct qcom_icc_node bimc_snoc_slv = {
>         .name = "bimc_snoc_slv",
>         .id = MSM8939_BIMC_SNOC_SLV,
>         .buswidth = 16,
>         .mas_rpm_id = -1,
>         .slv_rpm_id = 2,
>         .num_links = ARRAY_SIZE(bimc_snoc_slv_links),
>         .links = bimc_snoc_slv_links,
> };
> 
> #define NOC_QOS_MODE_INVALID -1
> ap_owned == false
> qos_mode == NOC_QOS_MODE_FIXED
> 
> 
> if (!qn->qos.ap_owned) {
>     /* bod: this will run */
>     /* send bandwidth request message to the RPM processor */
>     ret = qcom_icc_rpm_set(qn->mas_rpm_id, qn->slv_rpm_id, sum_bw);
>     if (ret)
>         return ret;
> } else if (qn->qos.qos_mode != -1) {
>     /* bod: this will not run */
>     /* set bandwidth directly from the AP */
>     ret = qcom_icc_qos_set(n, sum_bw);
>     if (ret)
>         return ret;
> }
> 
> and your proposed change
> 
> if (!qn->qos.ap_owned) {
>     /* bod: this will run */
>     /* send bandwidth request message to the RPM processor */
>     ret = qcom_icc_rpm_set(qn->mas_rpm_id, qn->slv_rpm_id, sum_bw);
>     if (ret)
>         return ret;
> }
> 
> if (qn->qos.qos_mode != NOC_QOS_MODE_INVALID) {
>     /* bod: this will run */
Also, this will not run with the next patch, perhaps i should
have ordered them differently (or perhaps the issue it solves
should have never been introduced :P).

Konrad
>     /* set bandwidth directly from the AP */
>     ret = qcom_icc_qos_set(n, sum_bw);
>     if (ret)
>         return ret;
> }
> 
> however if we look downstream we have the concept of ap_owned
> 
> https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.BR.1.2.9-00810-8x09.0/drivers/platform/msm/msm_bus/msm_bus_fabric_adhoc.c#L194
> 
> https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.BR.1.2.9-00810-8x09.0/drivers/platform/msm/msm_bus/msm_bus_fabric_adhoc.c#L208
> 
> In simple terms
> if (node_info->ap_owned) {
>     ret = fabdev->noc_ops.set_bw(node_info,
>                                     } else {
>     ret = send_rpm_msg(node_device);
> }
> 
> I agree your code does what it says on the tin but, whats the overall justification to depart from the downstream logic ?
> 
> ---
> bod