diff mbox series

[3/4] interconnect: qcom: icc-rpm: Make simple functions return void

Message ID 20240326-topic-rpm_icc_qos_cleanup-v1-3-357e736792be@linaro.org (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series Rework rpm-icc QoS settings | expand

Commit Message

Konrad Dybcio March 26, 2024, 7:42 p.m. UTC 
Register accesses can't just randomly fail. Change the return type of
functions that only do that to void.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/icc-rpm.c | 110 +++++++++++++++---------------------
 1 file changed, 47 insertions(+), 63 deletions(-)

Comments

Dmitry Baryshkov March 26, 2024, 8:14 p.m. UTC | #1 
On Tue, 26 Mar 2024 at 21:43, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> Register accesses can't just randomly fail. Change the return type of
> functions that only do that to void.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/interconnect/qcom/icc-rpm.c | 110 +++++++++++++++---------------------
>  1 file changed, 47 insertions(+), 63 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Mike Tipton May 8, 2024, 1:57 a.m. UTC | #2 
On Tue, Mar 26, 2024 at 08:42:34PM +0100, Konrad Dybcio wrote:
> Register accesses can't just randomly fail. Change the return type of
> functions that only do that to void.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/interconnect/qcom/icc-rpm.c | 110 +++++++++++++++---------------------
>  1 file changed, 47 insertions(+), 63 deletions(-)
> 

[...]

> @@ -115,48 +111,42 @@ static int qcom_icc_set_bimc_qos(struct icc_node *src)
>  	 * only if we are NOT in Bypass Mode.
>  	 */
>  	if (mode != NOC_QOS_MODE_BYPASS) {
> -		for (i = 3; i >= 0; i--) {
> -			rc = qcom_icc_bimc_set_qos_health(qp,
> -							  &qn->qos, i);
> -			if (rc)
> -				return rc;
> -		}
> +		qcom_icc_bimc_set_qos_health(qp, &qn->qos, 3);
> +		qcom_icc_bimc_set_qos_health(qp, &qn->qos, 2);
> +		qcom_icc_bimc_set_qos_health(qp, &qn->qos, 1);
> +		qcom_icc_bimc_set_qos_health(qp, &qn->qos, 0);

Not sure I see the point of unrolling this loop. With the error checking
removed, the loop could be reduced to just two lines, which is shorter
than the unrolled version and isn't really any more complicated:

	for (i = 3; i >= 0; i--)
		qcom_icc_bimc_set_qos_health(qp, &qn->qos, i);

Unrolling the loop is also slightly out-of-scope for this patch that
claims to just change the return types.

Otherwise, patch looks good to me.
diff mbox series

Patch

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index a8ed435f696c..0169de588a46 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -50,30 +50,27 @@ 
 
 #define ICC_BUS_CLK_MIN_RATE		19200ULL /* kHz */
 
-static int qcom_icc_set_qnoc_qos(struct icc_node *src)
+static void qcom_icc_set_qnoc_qos(struct icc_node *src)
 {
 	struct icc_provider *provider = src->provider;
 	struct qcom_icc_provider *qp = to_qcom_provider(provider);
 	struct qcom_icc_node *qn = src->data;
 	struct qcom_icc_qos *qos = &qn->qos;
-	int rc;
-
-	rc = regmap_update_bits(qp->regmap,
-			qp->qos_offset + QNOC_QOS_MCTL_LOWn_ADDR(qos->qos_port),
-			QNOC_QOS_MCTL_DFLT_PRIO_MASK,
-			qos->areq_prio << QNOC_QOS_MCTL_DFLT_PRIO_SHIFT);
-	if (rc)
-		return rc;
-
-	return regmap_update_bits(qp->regmap,
-			qp->qos_offset + QNOC_QOS_MCTL_LOWn_ADDR(qos->qos_port),
-			QNOC_QOS_MCTL_URGFWD_EN_MASK,
-			!!qos->urg_fwd_en << QNOC_QOS_MCTL_URGFWD_EN_SHIFT);
+
+	regmap_update_bits(qp->regmap,
+			   qp->qos_offset + QNOC_QOS_MCTL_LOWn_ADDR(qos->qos_port),
+			   QNOC_QOS_MCTL_DFLT_PRIO_MASK,
+			   qos->areq_prio << QNOC_QOS_MCTL_DFLT_PRIO_SHIFT);
+
+	regmap_update_bits(qp->regmap,
+			   qp->qos_offset + QNOC_QOS_MCTL_LOWn_ADDR(qos->qos_port),
+			   QNOC_QOS_MCTL_URGFWD_EN_MASK,
+			   !!qos->urg_fwd_en << QNOC_QOS_MCTL_URGFWD_EN_SHIFT);
 }
 
-static int qcom_icc_bimc_set_qos_health(struct qcom_icc_provider *qp,
-					struct qcom_icc_qos *qos,
-					int regnum)
+static void qcom_icc_bimc_set_qos_health(struct qcom_icc_provider *qp,
+					 struct qcom_icc_qos *qos,
+					 int regnum)
 {
 	u32 val;
 	u32 mask;
@@ -90,19 +87,18 @@  static int qcom_icc_bimc_set_qos_health(struct qcom_icc_provider *qp,
 		mask |= M_BKE_HEALTH_CFG_LIMITCMDS_MASK;
 	}
 
-	return regmap_update_bits(qp->regmap,
-				  qp->qos_offset + M_BKE_HEALTH_CFG_ADDR(regnum, qos->qos_port),
-				  mask, val);
+	regmap_update_bits(qp->regmap,
+			   qp->qos_offset + M_BKE_HEALTH_CFG_ADDR(regnum, qos->qos_port),
+			   mask, val);
 }
 
-static int qcom_icc_set_bimc_qos(struct icc_node *src)
+static void qcom_icc_set_bimc_qos(struct icc_node *src)
 {
 	struct qcom_icc_provider *qp;
 	struct qcom_icc_node *qn;
 	struct icc_provider *provider;
 	u32 mode = NOC_QOS_MODE_BYPASS;
 	u32 val = 0;
-	int i, rc = 0;
 
 	qn = src->data;
 	provider = src->provider;
@@ -115,48 +111,42 @@  static int qcom_icc_set_bimc_qos(struct icc_node *src)
 	 * only if we are NOT in Bypass Mode.
 	 */
 	if (mode != NOC_QOS_MODE_BYPASS) {
-		for (i = 3; i >= 0; i--) {
-			rc = qcom_icc_bimc_set_qos_health(qp,
-							  &qn->qos, i);
-			if (rc)
-				return rc;
-		}
+		qcom_icc_bimc_set_qos_health(qp, &qn->qos, 3);
+		qcom_icc_bimc_set_qos_health(qp, &qn->qos, 2);
+		qcom_icc_bimc_set_qos_health(qp, &qn->qos, 1);
+		qcom_icc_bimc_set_qos_health(qp, &qn->qos, 0);
 
 		/* Set BKE_EN to 1 when Fixed, Regulator or Limiter Mode */
 		val = 1;
 	}
 
-	return regmap_update_bits(qp->regmap,
-				  qp->qos_offset + M_BKE_EN_ADDR(qn->qos.qos_port),
-				  M_BKE_EN_EN_BMASK, val);
+	regmap_update_bits(qp->regmap,
+			   qp->qos_offset + M_BKE_EN_ADDR(qn->qos.qos_port),
+			   M_BKE_EN_EN_BMASK, val);
 }
 
-static int qcom_icc_noc_set_qos_priority(struct qcom_icc_provider *qp,
+static void qcom_icc_noc_set_qos_priority(struct qcom_icc_provider *qp,
 					 struct qcom_icc_qos *qos)
 {
 	u32 val;
-	int rc;
 
 	/* Must be updated one at a time, P1 first, P0 last */
 	val = qos->areq_prio << NOC_QOS_PRIORITY_P1_SHIFT;
-	rc = regmap_update_bits(qp->regmap,
-				qp->qos_offset + NOC_QOS_PRIORITYn_ADDR(qos->qos_port),
-				NOC_QOS_PRIORITY_P1_MASK, val);
-	if (rc)
-		return rc;
-
-	return regmap_update_bits(qp->regmap,
-				  qp->qos_offset + NOC_QOS_PRIORITYn_ADDR(qos->qos_port),
-				  NOC_QOS_PRIORITY_P0_MASK, qos->prio_level);
+	regmap_update_bits(qp->regmap,
+			   qp->qos_offset + NOC_QOS_PRIORITYn_ADDR(qos->qos_port),
+			   NOC_QOS_PRIORITY_P1_MASK, val);
+
+	regmap_update_bits(qp->regmap,
+			   qp->qos_offset + NOC_QOS_PRIORITYn_ADDR(qos->qos_port),
+			   NOC_QOS_PRIORITY_P0_MASK, qos->prio_level);
 }
 
-static int qcom_icc_set_noc_qos(struct icc_node *src)
+static void qcom_icc_set_noc_qos(struct icc_node *src)
 {
 	struct qcom_icc_provider *qp;
 	struct qcom_icc_node *qn;
 	struct icc_provider *provider;
 	u32 mode = NOC_QOS_MODE_BYPASS_VAL;
-	int rc = 0;
 
 	qn = src->data;
 	provider = src->provider;
@@ -166,15 +156,12 @@  static int qcom_icc_set_noc_qos(struct icc_node *src)
 		dev_dbg(src->provider->dev,
 			"NoC QoS: Skipping %s: vote aggregated on parent.\n",
 			qn->name);
-		return 0;
 	}
 
 	if (qn->qos.qos_mode == NOC_QOS_MODE_FIXED) {
 		dev_dbg(src->provider->dev, "NoC QoS: %s: Set Fixed mode\n", qn->name);
 		mode = NOC_QOS_MODE_FIXED_VAL;
-		rc = qcom_icc_noc_set_qos_priority(qp, &qn->qos);
-		if (rc)
-			return rc;
+		qcom_icc_noc_set_qos_priority(qp, &qn->qos);
 	} else if (qn->qos.qos_mode == NOC_QOS_MODE_BYPASS) {
 		dev_dbg(src->provider->dev, "NoC QoS: %s: Set Bypass mode\n", qn->name);
 		mode = NOC_QOS_MODE_BYPASS_VAL;
@@ -182,12 +169,12 @@  static int qcom_icc_set_noc_qos(struct icc_node *src)
 		/* How did we get here? */
 	}
 
-	return regmap_update_bits(qp->regmap,
-				  qp->qos_offset + NOC_QOS_MODEn_ADDR(qn->qos.qos_port),
-				  NOC_QOS_MODEn_MASK, mode);
+	regmap_update_bits(qp->regmap,
+			   qp->qos_offset + NOC_QOS_MODEn_ADDR(qn->qos.qos_port),
+			   NOC_QOS_MODEn_MASK, mode);
 }
 
-static int qcom_icc_qos_set(struct icc_node *node)
+static void qcom_icc_qos_set(struct icc_node *node)
 {
 	struct qcom_icc_provider *qp = to_qcom_provider(node->provider);
 	struct qcom_icc_node *qn = node->data;
@@ -196,11 +183,14 @@  static int qcom_icc_qos_set(struct icc_node *node)
 
 	switch (qp->type) {
 	case QCOM_ICC_BIMC:
-		return qcom_icc_set_bimc_qos(node);
+		qcom_icc_set_bimc_qos(node);
+		break;
 	case QCOM_ICC_QNOC:
-		return qcom_icc_set_qnoc_qos(node);
+		qcom_icc_set_qnoc_qos(node);
+		break;
 	default:
-		return qcom_icc_set_noc_qos(node);
+		qcom_icc_set_noc_qos(node);
+		break;
 	}
 }
 
@@ -586,14 +576,8 @@  int qnoc_probe(struct platform_device *pdev)
 
 		/* Set QoS registers (we only need to do it once, generally) */
 		if (qnodes[i]->qos.ap_owned &&
-		    qnodes[i]->qos.qos_mode != NOC_QOS_MODE_INVALID) {
-			ret = qcom_icc_qos_set(node);
-			if (ret) {
-				clk_bulk_disable_unprepare(qp->num_intf_clks,
-							   qp->intf_clks);
-				goto err_remove_nodes;
-			}
-		}
+		    qnodes[i]->qos.qos_mode != NOC_QOS_MODE_INVALID)
+			qcom_icc_qos_set(node);
 
 		data->nodes[i] = node;
 	}