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 */
/* 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
