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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux