[PATCH v2 00/10] Fix up icc clock rate calculation on some platforms

[PATCH v2 00/10] Fix up icc clock rate calculation on some platforms

[Konrad Dybcio]

Certain platforms require that some buses (or individual nodes) make
some additional changes to the clock rate formula, throwing in some
magic, Qualcomm-defined coefficients, to account for "inefficiencies".

Add the framework for it and utilize it on a couple SoCs.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Changes in v2:
- Use the (arguably less favourable but necessary for precission) 100/x
  instead of x/100 for ib coefficient, update values in consequent
  patches to reflect that
- Rename "_percent" to "_coeff" because of /\
- Add the necessary code to support per-node clocks
- Add the necessary code to support per-node coefficients
- Hook up the CPUSS<->GNoC clock on QCM2290
- Update EBI node on QCM2290
- Link to v1: https://lore.kernel.org/r/20230726-topic-icc_coeff-v1-0-31616960818c@linaro.org

---
Konrad Dybcio (10):
      interconnect: qcom: icc-rpm: Add AB/IB calculations coefficients
      interconnect: qcom: icc-rpm: Separate out clock rate calulcations
      interconnect: qcom: icc-rpm: Let nodes drive their own bus clock
      interconnect: qcom: icc-rpm: Check for node-specific rate coefficients
      interconnect: qcom: qcm2290: Hook up MAS_APPS_PROC's bus clock
      interconnect: qcom: qcm2290: Set AB coefficients
      interconnect: qcom: qcm2290: Update EBI channel configuration
      interconnect: qcom: sdm660: Set AB/IB coefficients
      interconnect: qcom: msm8996: Set AB/IB coefficients
      clk: qcom: smd-rpm: Move CPUSS_GNoC clock to interconnect

 drivers/clk/qcom/clk-smd-rpm.c             | 16 ++++--
 drivers/interconnect/qcom/icc-rpm-clocks.c |  6 ++
 drivers/interconnect/qcom/icc-rpm.c        | 92 ++++++++++++++++++++++++------
 drivers/interconnect/qcom/icc-rpm.h        | 15 +++++
 drivers/interconnect/qcom/msm8996.c        |  8 ++-
 drivers/interconnect/qcom/qcm2290.c        |  9 ++-
 drivers/interconnect/qcom/sdm660.c         |  4 ++
 7 files changed, 124 insertions(+), 26 deletions(-)
---
base-commit: ec89391563792edd11d138a853901bce76d11f44
change-id: 20230726-topic-icc_coeff-b053d5409b9f

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@linaro.org>

[PATCH v2 01/10] interconnect: qcom: icc-rpm: Add AB/IB calculations coefficients

[Konrad Dybcio]

Presumably due to the hardware being so complex, some nodes (or busses)
have different (usually higher) requirements for bandwidth than what
the usual calculations would suggest.

Looking at the available downstream files, it seems like AB values are
adjusted per-bus and IB values are adjusted per-node.
With that in mind, introduce percentage-based coefficient struct members
and use them in the calculations.

One thing to note is that the IB coefficient is inverse (100/ib_percent)
which feels a bit backwards, but it's necessary for precision..

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/icc-rpm.c | 14 +++++++++++---
 drivers/interconnect/qcom/icc-rpm.h |  6 ++++++
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index 2c16917ba1fd..a837d20af79e 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -298,7 +298,8 @@ static int qcom_icc_bw_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
  */
 static void qcom_icc_bus_aggregate(struct icc_provider *provider, u64 *agg_clk_rate)
 {
-	u64 agg_avg_rate, agg_rate;
+	struct qcom_icc_provider *qp = to_qcom_provider(provider);
+	u64 agg_avg_rate, agg_peak_rate, agg_rate;
 	struct qcom_icc_node *qn;
 	struct icc_node *node;
 	int i;
@@ -315,8 +316,15 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider, u64 *agg_clk_r
 			else
 				agg_avg_rate = qn->sum_avg[i];
 
-			agg_rate = max_t(u64, agg_avg_rate, qn->max_peak[i]);
-			do_div(agg_rate, qn->buswidth);
+			if (qp->ab_coeff)
+				agg_avg_rate = mult_frac(qp->ab_coeff, agg_avg_rate, 100);
+
+			if (qp->ib_coeff)
+				agg_peak_rate = mult_frac(100, qn->max_peak[i], qp->ib_coeff);
+			else
+				agg_peak_rate = qn->max_peak[i];
+
+			agg_rate = max_t(u64, agg_avg_rate, agg_peak_rate);
 
 			agg_clk_rate[i] = max_t(u64, agg_clk_rate[i], agg_rate);
 		}
diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
index eed3451af3e6..5e7d6a4fd2f3 100644
--- a/drivers/interconnect/qcom/icc-rpm.h
+++ b/drivers/interconnect/qcom/icc-rpm.h
@@ -44,6 +44,8 @@ struct rpm_clk_resource {
  * @type: the ICC provider type
  * @regmap: regmap for QoS registers read/write access
  * @qos_offset: offset to QoS registers
+ * @ab_coeff: a percentage-based coefficient for compensating the AB calculations
+ * @ib_coeff: an inverse-percentage-based coefficient for compensating the IB calculations
  * @bus_clk_rate: bus clock rate in Hz
  * @bus_clk_desc: a pointer to a rpm_clk_resource description of bus clocks
  * @bus_clk: a pointer to a HLOS-owned bus clock
@@ -57,6 +59,8 @@ struct qcom_icc_provider {
 	enum qcom_icc_type type;
 	struct regmap *regmap;
 	unsigned int qos_offset;
+	u16 ab_coeff;
+	u16 ib_coeff;
 	u32 bus_clk_rate[QCOM_SMD_RPM_STATE_NUM];
 	const struct rpm_clk_resource *bus_clk_desc;
 	struct clk *bus_clk;
@@ -123,6 +127,8 @@ struct qcom_icc_desc {
 	enum qcom_icc_type type;
 	const struct regmap_config *regmap_cfg;
 	unsigned int qos_offset;
+	u16 ab_coeff;
+	u16 ib_coeff;
 };
 
 /* Valid for all bus types */

-- 
2.41.0

Re: [PATCH v2 01/10] interconnect: qcom: icc-rpm: Add AB/IB calculations coefficients

[Stephan Gerhold]

On Mon, Jul 31, 2023 at 12:52:17PM +0200, Konrad Dybcio wrote:
> Presumably due to the hardware being so complex, some nodes (or busses)
> have different (usually higher) requirements for bandwidth than what
> the usual calculations would suggest.
> 
> Looking at the available downstream files, it seems like AB values are
> adjusted per-bus and IB values are adjusted per-node.
> With that in mind, introduce percentage-based coefficient struct members
> and use them in the calculations.
> 
> One thing to note is that the IB coefficient is inverse (100/ib_percent)
> which feels a bit backwards, but it's necessary for precision..
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/interconnect/qcom/icc-rpm.c | 14 +++++++++++---
>  drivers/interconnect/qcom/icc-rpm.h |  6 ++++++
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index 2c16917ba1fd..a837d20af79e 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -298,7 +298,8 @@ static int qcom_icc_bw_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
>   */
>  static void qcom_icc_bus_aggregate(struct icc_provider *provider, u64 *agg_clk_rate)
>  {
> -	u64 agg_avg_rate, agg_rate;
> +	struct qcom_icc_provider *qp = to_qcom_provider(provider);
> +	u64 agg_avg_rate, agg_peak_rate, agg_rate;
>  	struct qcom_icc_node *qn;
>  	struct icc_node *node;
>  	int i;
> @@ -315,8 +316,15 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider, u64 *agg_clk_r
>  			else
>  				agg_avg_rate = qn->sum_avg[i];
>  
> -			agg_rate = max_t(u64, agg_avg_rate, qn->max_peak[i]);
> -			do_div(agg_rate, qn->buswidth);
> +			if (qp->ab_coeff)
> +				agg_avg_rate = mult_frac(qp->ab_coeff, agg_avg_rate, 100);

agg_avg_rate * (qp->ab_coeff / 100) would feel more logical to me (even
if it should be the same), i.e.

	agg_avg_rate = mult_frac(agg_avg_rate, qp->ab_coeff, 100);

Not sure why you swapped them.

> +
> +			if (qp->ib_coeff)
> +				agg_peak_rate = mult_frac(100, qn->max_peak[i], qp->ib_coeff);

	agg_peak_rate = mult_frac(qn->max_peak[i], 100, qp->ib_coeff);

Anyway, looks like you need to avoid mult_frac anyway for ARM32 compat :/

arm-none-eabi-ld: drivers/interconnect/qcom/icc-rpm.o: in function `qcom_icc_calc_rate':
drivers/interconnect/qcom/icc-rpm.c:310: undefined reference to `__aeabi_uldivmod'
arm-none-eabi-ld: drivers/interconnect/qcom/icc-rpm.c:312: undefined reference to `__aeabi_uldivmod'

Thanks,
Stephan

[PATCH v2 02/10] interconnect: qcom: icc-rpm: Separate out clock rate calulcations

[Konrad Dybcio]

In preparation for also setting per-node clock rates, separate out the
logic that computes it.

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

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index a837d20af79e..f1d8ed354718 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -291,6 +291,29 @@ static int qcom_icc_bw_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
 	return 0;
 }
 
+static u64 qcom_icc_calc_rate(struct qcom_icc_provider *qp, struct qcom_icc_node *qn, int ctx)
+{
+	u64 agg_avg_rate, agg_peak_rate, agg_rate;
+
+	if (qn->channels)
+		agg_avg_rate = div_u64(qn->sum_avg[ctx], qn->channels);
+	else
+		agg_avg_rate = qn->sum_avg[ctx];
+
+	/* Check if the node has a specific coefficient first*/
+	if (qp->ab_coeff)
+		agg_avg_rate = mult_frac(qp->ab_coeff, agg_avg_rate, 100);
+
+	if (qp->ib_coeff)
+		agg_peak_rate = mult_frac(100, qn->max_peak[ctx], qp->ib_coeff);
+	else
+		agg_peak_rate = qn->max_peak[ctx];
+
+	agg_rate = max_t(u64, agg_avg_rate, agg_peak_rate);
+
+	return div_u64(agg_rate, qn->buswidth);
+}
+
 /**
  * qcom_icc_bus_aggregate - calculate bus clock rates by traversing all nodes
  * @provider: generic interconnect provider
@@ -299,10 +322,9 @@ static int qcom_icc_bw_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
 static void qcom_icc_bus_aggregate(struct icc_provider *provider, u64 *agg_clk_rate)
 {
 	struct qcom_icc_provider *qp = to_qcom_provider(provider);
-	u64 agg_avg_rate, agg_peak_rate, agg_rate;
 	struct qcom_icc_node *qn;
 	struct icc_node *node;
-	int i;
+	int ctx;
 
 	/*
 	 * Iterate nodes on the provider, aggregate bandwidth requests for
@@ -310,23 +332,9 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider, u64 *agg_clk_r
 	 */
 	list_for_each_entry(node, &provider->nodes, node_list) {
 		qn = node->data;
-		for (i = 0; i < QCOM_SMD_RPM_STATE_NUM; i++) {
-			if (qn->channels)
-				agg_avg_rate = div_u64(qn->sum_avg[i], qn->channels);
-			else
-				agg_avg_rate = qn->sum_avg[i];
-
-			if (qp->ab_coeff)
-				agg_avg_rate = mult_frac(qp->ab_coeff, agg_avg_rate, 100);
-
-			if (qp->ib_coeff)
-				agg_peak_rate = mult_frac(100, qn->max_peak[i], qp->ib_coeff);
-			else
-				agg_peak_rate = qn->max_peak[i];
-
-			agg_rate = max_t(u64, agg_avg_rate, agg_peak_rate);
-
-			agg_clk_rate[i] = max_t(u64, agg_clk_rate[i], agg_rate);
+		for (ctx = 0; ctx < QCOM_SMD_RPM_STATE_NUM; ctx++) {
+			agg_clk_rate[ctx] = max_t(u64, agg_clk_rate[ctx],
+						  qcom_icc_calc_rate(qp, qn, ctx));
 		}
 	}
 }

-- 
2.41.0

[PATCH v2 03/10] interconnect: qcom: icc-rpm: Let nodes drive their own bus clock

[Konrad Dybcio]

If this hardware couldn't get messier, some nodes are supposed to drive
their own bus clock.. Presumably to connect to some intermediate
interface between the node itself and the bus it's (supposed to be)
connected to.

Expand the node struct with the necessary data and hook up the
allocations & calculations.

To save on memory (not very many nodes have their own clocks), allocate
a pointer to an array instead of allocating an array within
qcom_icc_node.

Note that the node-specific AB/IB coefficients contribute (by design)
to both the node-level and the bus-level aggregation.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/icc-rpm.c | 48 +++++++++++++++++++++++++++++++------
 drivers/interconnect/qcom/icc-rpm.h |  2 ++
 2 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index f1d8ed354718..f0e575c95b49 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -411,6 +411,33 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
 		qp->bus_clk_rate[QCOM_SMD_RPM_SLEEP_STATE] = sleep_rate;
 	}
 
+	/* Handle the node-specific clock */
+	if (!src_qn->bus_clk_desc)
+		return 0;
+
+	active_rate = qcom_icc_calc_rate(qp, src_qn, QCOM_SMD_RPM_ACTIVE_STATE);
+	sleep_rate = qcom_icc_calc_rate(qp, src_qn, QCOM_SMD_RPM_SLEEP_STATE);
+
+	if (active_rate != src_qn->bus_clk_rate[QCOM_SMD_RPM_ACTIVE_STATE]) {
+		ret = qcom_icc_rpm_set_bus_rate(src_qn->bus_clk_desc, QCOM_SMD_RPM_ACTIVE_STATE,
+						active_rate);
+		if (ret)
+			return ret;
+
+		/* Cache the rate after we've successfully committed it to RPM */
+		src_qn->bus_clk_rate[QCOM_SMD_RPM_ACTIVE_STATE] = active_rate;
+	}
+
+	if (sleep_rate != src_qn->bus_clk_rate[QCOM_SMD_RPM_SLEEP_STATE]) {
+		ret = qcom_icc_rpm_set_bus_rate(src_qn->bus_clk_desc, QCOM_SMD_RPM_SLEEP_STATE,
+						sleep_rate);
+		if (ret)
+			return ret;
+
+		/* Cache the rate after we've successfully committed it to RPM */
+		src_qn->bus_clk_rate[QCOM_SMD_RPM_SLEEP_STATE] = sleep_rate;
+	}
+
 	return 0;
 }
 
@@ -531,24 +558,31 @@ int qnoc_probe(struct platform_device *pdev)
 		return ret;
 
 	for (i = 0; i < num_nodes; i++) {
+		struct qcom_icc_node *qn = qnodes[i];
 		size_t j;
 
-		node = icc_node_create(qnodes[i]->id);
+		if (qn->bus_clk_desc) {
+			qn->bus_clk_rate = devm_kcalloc(dev, QCOM_SMD_RPM_STATE_NUM,
+							sizeof(*qn->bus_clk_rate),
+							GFP_KERNEL);
+		}
+
+		node = icc_node_create(qn->id);
 		if (IS_ERR(node)) {
 			ret = PTR_ERR(node);
 			goto err_remove_nodes;
 		}
 
-		node->name = qnodes[i]->name;
-		node->data = qnodes[i];
+		node->name = qn->name;
+		node->data = qn;
 		icc_node_add(node, provider);
 
-		for (j = 0; j < qnodes[i]->num_links; j++)
-			icc_link_create(node, qnodes[i]->links[j]);
+		for (j = 0; j < qn->num_links; j++)
+			icc_link_create(node, qn->links[j]);
 
 		/* 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) {
+		if (qn->qos.ap_owned &&
+		    qn->qos.qos_mode != NOC_QOS_MODE_INVALID) {
 			ret = qcom_icc_qos_set(node);
 			if (ret)
 				return ret;
diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
index 5e7d6a4fd2f3..835b83cfb548 100644
--- a/drivers/interconnect/qcom/icc-rpm.h
+++ b/drivers/interconnect/qcom/icc-rpm.h
@@ -97,6 +97,7 @@ struct qcom_icc_qos {
  * @num_links: the total number of @links
  * @channels: number of channels at this node (e.g. DDR channels)
  * @buswidth: width of the interconnect between a node and the bus (bytes)
+ * @bus_clk_desc: a pointer to a rpm_clk_resource description of bus clocks
  * @sum_avg: current sum aggregate value of all avg bw requests
  * @max_peak: current max aggregate value of all peak bw requests
  * @mas_rpm_id:	RPM id for devices that are bus masters
@@ -110,6 +111,7 @@ struct qcom_icc_node {
 	u16 num_links;
 	u16 channels;
 	u16 buswidth;
+	const struct rpm_clk_resource *bus_clk_desc;
 	u64 sum_avg[QCOM_SMD_RPM_STATE_NUM];
 	u64 max_peak[QCOM_SMD_RPM_STATE_NUM];
 	int mas_rpm_id;

-- 
2.41.0

Re: [PATCH v2 03/10] interconnect: qcom: icc-rpm: Let nodes drive their own bus clock

[Stephan Gerhold]

On Mon, Jul 31, 2023 at 12:52:19PM +0200, Konrad Dybcio wrote:
> If this hardware couldn't get messier, some nodes are supposed to drive
> their own bus clock.. Presumably to connect to some intermediate
> interface between the node itself and the bus it's (supposed to be)
> connected to.
> 
> Expand the node struct with the necessary data and hook up the
> allocations & calculations.
> 
> To save on memory (not very many nodes have their own clocks), allocate
> a pointer to an array instead of allocating an array within
> qcom_icc_node.
> 

Only on ARM32 though. On ARM64 you waste extra memory:

u32 bus_clk_rate[QCOM_SMD_RPM_STATE_NUM];
sizeof(bus_clk_rate) = QCOM_SMD_RPM_STATE_NUM * sizeof(bus_clk_rate[0])
                     = 2 * 4
                     = 8

u32 *bus_clk_rate;
sizeof(bus_clk_rate) = sizeof(ptr)
                     = 8 (for ARM64)
                       + 2 * 4 + malloc overhead
                         for each node with bus_clk_desc

which is > 8 from above.

I'm not quite convinced this optimization is worth it.

Thanks,
Stephan

Re: [PATCH v2 03/10] interconnect: qcom: icc-rpm: Let nodes drive their own bus clock

[Konrad Dybcio]

On 1.08.2023 12:58, Stephan Gerhold wrote:
> On Mon, Jul 31, 2023 at 12:52:19PM +0200, Konrad Dybcio wrote:
>> If this hardware couldn't get messier, some nodes are supposed to drive
>> their own bus clock.. Presumably to connect to some intermediate
>> interface between the node itself and the bus it's (supposed to be)
>> connected to.
>>
>> Expand the node struct with the necessary data and hook up the
>> allocations & calculations.
>>
>> To save on memory (not very many nodes have their own clocks), allocate
>> a pointer to an array instead of allocating an array within
>> qcom_icc_node.
>>
> 
> Only on ARM32 though. On ARM64 you waste extra memory:
> 
> u32 bus_clk_rate[QCOM_SMD_RPM_STATE_NUM];
> sizeof(bus_clk_rate) = QCOM_SMD_RPM_STATE_NUM * sizeof(bus_clk_rate[0])
>                      = 2 * 4
>                      = 8
> 
> u32 *bus_clk_rate;
> sizeof(bus_clk_rate) = sizeof(ptr)
>                      = 8 (for ARM64)
>                        + 2 * 4 + malloc overhead
>                          for each node with bus_clk_desc
> 
> which is > 8 from above.
> 
> I'm not quite convinced this optimization is worth it.
Right, u32 is not the same size as *u32, brain fart :D

Konrad

[PATCH v2 04/10] interconnect: qcom: icc-rpm: Check for node-specific rate coefficients

[Konrad Dybcio]

Some nodes may have different coefficients than the general values for
bus they're attached to. Check for that and use them if present.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/icc-rpm.c | 10 +++++++---
 drivers/interconnect/qcom/icc-rpm.h |  6 ++++++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index f0e575c95b49..91eb428385f6 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -300,11 +300,15 @@ static u64 qcom_icc_calc_rate(struct qcom_icc_provider *qp, struct qcom_icc_node
 	else
 		agg_avg_rate = qn->sum_avg[ctx];
 
-	/* Check if the node has a specific coefficient first*/
-	if (qp->ab_coeff)
+	/* Check if the node has a specific coefficient first */
+	if (qn->ab_coeff)
+		agg_avg_rate = mult_frac(qn->ab_coeff, agg_avg_rate, 100);
+	else if (qp->ab_coeff)
 		agg_avg_rate = mult_frac(qp->ab_coeff, agg_avg_rate, 100);
 
-	if (qp->ib_coeff)
+	if (qn->ab_coeff)
+		agg_peak_rate = mult_frac(100, qn->max_peak[ctx], qn->ib_coeff);
+	else if (qp->ib_coeff)
 		agg_peak_rate = mult_frac(100, qn->max_peak[ctx], qp->ib_coeff);
 	else
 		agg_peak_rate = qn->max_peak[ctx];
diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
index 835b83cfb548..1a26a7b82166 100644
--- a/drivers/interconnect/qcom/icc-rpm.h
+++ b/drivers/interconnect/qcom/icc-rpm.h
@@ -103,6 +103,9 @@ struct qcom_icc_qos {
  * @mas_rpm_id:	RPM id for devices that are bus masters
  * @slv_rpm_id:	RPM id for devices that are bus slaves
  * @qos: NoC QoS setting parameters
+ * @ab_coeff: a percentage-based coefficient for compensating the AB calculations
+ * @ib_coeff: an inverse-percentage-based coefficient for compensating the IB calculations
+ * @bus_clk_rate: a pointer to an array containing bus clock rates in Hz
  */
 struct qcom_icc_node {
 	unsigned char *name;
@@ -117,6 +120,9 @@ struct qcom_icc_node {
 	int mas_rpm_id;
 	int slv_rpm_id;
 	struct qcom_icc_qos qos;
+	u16 ab_coeff;
+	u16 ib_coeff;
+	u32 *bus_clk_rate;
 };
 
 struct qcom_icc_desc {

-- 
2.41.0

Re: [PATCH v2 04/10] interconnect: qcom: icc-rpm: Check for node-specific rate coefficients

[Stephan Gerhold]

On Mon, Jul 31, 2023 at 12:52:20PM +0200, Konrad Dybcio wrote:
> Some nodes may have different coefficients than the general values for
> bus they're attached to. Check for that and use them if present.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/interconnect/qcom/icc-rpm.c | 10 +++++++---
>  drivers/interconnect/qcom/icc-rpm.h |  6 ++++++
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index f0e575c95b49..91eb428385f6 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -300,11 +300,15 @@ static u64 qcom_icc_calc_rate(struct qcom_icc_provider *qp, struct qcom_icc_node
>  	else
>  		agg_avg_rate = qn->sum_avg[ctx];
>  
> -	/* Check if the node has a specific coefficient first*/
> -	if (qp->ab_coeff)
> +	/* Check if the node has a specific coefficient first */
> +	if (qn->ab_coeff)
> +		agg_avg_rate = mult_frac(qn->ab_coeff, agg_avg_rate, 100);
> +	else if (qp->ab_coeff)
>  		agg_avg_rate = mult_frac(qp->ab_coeff, agg_avg_rate, 100);
>  
> -	if (qp->ib_coeff)
> +	if (qn->ab_coeff)
> +		agg_peak_rate = mult_frac(100, qn->max_peak[ctx], qn->ib_coeff);
> +	else if (qp->ib_coeff)
>  		agg_peak_rate = mult_frac(100, qn->max_peak[ctx], qp->ib_coeff);
>  	else
>  		agg_peak_rate = qn->max_peak[ctx];

Code/data size and likely performance would be slightly better if you
only add ab_coeff/ib_coeff to the node and not the provider. This is
slightly inconvenient because you need to duplicate the same value on
a lot of nodes, but the per-node memory is reserved anyway. You might
as well use I would say.

> diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
> index 835b83cfb548..1a26a7b82166 100644
> --- a/drivers/interconnect/qcom/icc-rpm.h
> +++ b/drivers/interconnect/qcom/icc-rpm.h
> @@ -103,6 +103,9 @@ struct qcom_icc_qos {
>   * @mas_rpm_id:	RPM id for devices that are bus masters
>   * @slv_rpm_id:	RPM id for devices that are bus slaves
>   * @qos: NoC QoS setting parameters
> + * @ab_coeff: a percentage-based coefficient for compensating the AB calculations
> + * @ib_coeff: an inverse-percentage-based coefficient for compensating the IB calculations
> + * @bus_clk_rate: a pointer to an array containing bus clock rates in Hz
>   */
>  struct qcom_icc_node {
>  	unsigned char *name;
> @@ -117,6 +120,9 @@ struct qcom_icc_node {
>  	int mas_rpm_id;
>  	int slv_rpm_id;
>  	struct qcom_icc_qos qos;
> +	u16 ab_coeff;
> +	u16 ib_coeff;
> +	u32 *bus_clk_rate;

bus_clk_rate should be in previous patch :)

Thanks,
Stephan

[PATCH v2 05/10] interconnect: qcom: qcm2290: Hook up MAS_APPS_PROC's bus clock

[Konrad Dybcio]

This single node has its own clock which seems to be responsible for
transactions between CPUSS (CPU + some stuff) and the GNOC.

Define it and hook it up.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/icc-rpm-clocks.c | 6 ++++++
 drivers/interconnect/qcom/icc-rpm.h        | 1 +
 drivers/interconnect/qcom/qcm2290.c        | 3 +++
 3 files changed, 10 insertions(+)

diff --git a/drivers/interconnect/qcom/icc-rpm-clocks.c b/drivers/interconnect/qcom/icc-rpm-clocks.c
index 63c82a91bbc7..ac1677de7dfd 100644
--- a/drivers/interconnect/qcom/icc-rpm-clocks.c
+++ b/drivers/interconnect/qcom/icc-rpm-clocks.c
@@ -25,6 +25,12 @@ const struct rpm_clk_resource bimc_clk = {
 };
 EXPORT_SYMBOL_GPL(bimc_clk);
 
+const struct rpm_clk_resource mem_1_clk = {
+	.resource_type = QCOM_SMD_RPM_MEM_CLK,
+	.clock_id = 1,
+};
+EXPORT_SYMBOL_GPL(mem_1_clk);
+
 const struct rpm_clk_resource bus_0_clk = {
 	.resource_type = QCOM_SMD_RPM_BUS_CLK,
 	.clock_id = 0,
diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
index 1a26a7b82166..dc8e37c0a263 100644
--- a/drivers/interconnect/qcom/icc-rpm.h
+++ b/drivers/interconnect/qcom/icc-rpm.h
@@ -152,6 +152,7 @@ extern const struct rpm_clk_resource bimc_clk;
 extern const struct rpm_clk_resource bus_0_clk;
 extern const struct rpm_clk_resource bus_1_clk;
 extern const struct rpm_clk_resource bus_2_clk;
+extern const struct rpm_clk_resource mem_1_clk;
 extern const struct rpm_clk_resource mmaxi_0_clk;
 extern const struct rpm_clk_resource mmaxi_1_clk;
 extern const struct rpm_clk_resource qup_clk;
diff --git a/drivers/interconnect/qcom/qcm2290.c b/drivers/interconnect/qcom/qcm2290.c
index 3c3b24264a5b..52a6eb32e832 100644
--- a/drivers/interconnect/qcom/qcm2290.c
+++ b/drivers/interconnect/qcom/qcm2290.c
@@ -112,6 +112,9 @@ static struct qcom_icc_node mas_appss_proc = {
 	.qos.qos_mode = NOC_QOS_MODE_FIXED,
 	.qos.prio_level = 0,
 	.qos.areq_prio = 0,
+	.bus_clk_desc = &mem_1_clk,
+	.ab_coeff = 159,
+	.ib_coeff = 96,
 	.mas_rpm_id = 0,
 	.slv_rpm_id = -1,
 	.num_links = ARRAY_SIZE(mas_appss_proc_links),

-- 
2.41.0

[PATCH v2 06/10] interconnect: qcom: qcm2290: Set AB coefficients

[Konrad Dybcio]

Some buses need additional manual adjustments atop the usual
calculations. Fill in the missing coefficients.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/qcm2290.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/interconnect/qcom/qcm2290.c b/drivers/interconnect/qcom/qcm2290.c
index 52a6eb32e832..42fa01c66e73 100644
--- a/drivers/interconnect/qcom/qcm2290.c
+++ b/drivers/interconnect/qcom/qcm2290.c
@@ -1201,6 +1201,7 @@ static const struct qcom_icc_desc qcm2290_bimc = {
 	.regmap_cfg = &qcm2290_bimc_regmap_config,
 	/* M_REG_BASE() in vendor msm_bus_bimc_adhoc driver */
 	.qos_offset = 0x8000,
+	.ab_coeff = 153,
 };
 
 static struct qcom_icc_node * const qcm2290_cnoc_nodes[] = {
@@ -1327,6 +1328,7 @@ static const struct qcom_icc_desc qcm2290_mmnrt_virt = {
 	.bus_clk_desc = &mmaxi_0_clk,
 	.regmap_cfg = &qcm2290_snoc_regmap_config,
 	.qos_offset = 0x15000,
+	.ab_coeff = 142,
 };
 
 static struct qcom_icc_node * const qcm2290_mmrt_virt_nodes[] = {
@@ -1342,6 +1344,7 @@ static const struct qcom_icc_desc qcm2290_mmrt_virt = {
 	.bus_clk_desc = &mmaxi_1_clk,
 	.regmap_cfg = &qcm2290_snoc_regmap_config,
 	.qos_offset = 0x15000,
+	.ab_coeff = 139,
 };
 
 static const struct of_device_id qcm2290_noc_of_match[] = {

-- 
2.41.0

[PATCH v2 07/10] interconnect: qcom: qcm2290: Update EBI channel configuration

[Konrad Dybcio]

QCM2290 can support two memory configurations: single-channel, 32-bit
wide LPDDR3 @ up to 933MHz (bus clock) or dual-channel, 16-bit wide
LPDDR4X @ up to 1804 MHz. The interconnect driver in its current form
seems to gravitate towards the first one, however there are no LPDDR3-
equipped boards upstream and we still don't have a great way to discern
the DDR generations on the kernel side.

To make DDR scaling possible on the only currently-supported 2290
board, stick with the LPDDR4X config by default. The side effect on any
potential LPDDR3 board would be that the requested bus clock rate is
too high (but still capped to the firmware-configured FMAX).

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/qcm2290.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/interconnect/qcom/qcm2290.c b/drivers/interconnect/qcom/qcm2290.c
index 42fa01c66e73..3bd7ad67c569 100644
--- a/drivers/interconnect/qcom/qcm2290.c
+++ b/drivers/interconnect/qcom/qcm2290.c
@@ -678,7 +678,8 @@ static struct qcom_icc_node mas_gfx3d = {
 static struct qcom_icc_node slv_ebi1 = {
 	.name = "slv_ebi1",
 	.id = QCM2290_SLAVE_EBI1,
-	.buswidth = 8,
+	.buswidth = 4,
+	.channels = 2,
 	.mas_rpm_id = -1,
 	.slv_rpm_id = 0,
 };

-- 
2.41.0

[PATCH v2 08/10] interconnect: qcom: sdm660: Set AB/IB coefficients

[Konrad Dybcio]

Some buses and nodes need additional manual adjustments atop the usual
calculations. Fill in the missing coefficients.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/sdm660.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/interconnect/qcom/sdm660.c b/drivers/interconnect/qcom/sdm660.c
index 36962f7bd7bb..7392bebba334 100644
--- a/drivers/interconnect/qcom/sdm660.c
+++ b/drivers/interconnect/qcom/sdm660.c
@@ -602,6 +602,7 @@ static struct qcom_icc_node mas_mdp_p0 = {
 	.name = "mas_mdp_p0",
 	.id = SDM660_MASTER_MDP_P0,
 	.buswidth = 16,
+	.ib_coeff = 50,
 	.mas_rpm_id = 8,
 	.slv_rpm_id = -1,
 	.qos.ap_owned = true,
@@ -621,6 +622,7 @@ static struct qcom_icc_node mas_mdp_p1 = {
 	.name = "mas_mdp_p1",
 	.id = SDM660_MASTER_MDP_P1,
 	.buswidth = 16,
+	.ib_coeff = 50,
 	.mas_rpm_id = 61,
 	.slv_rpm_id = -1,
 	.qos.ap_owned = true,
@@ -1540,6 +1542,7 @@ static const struct qcom_icc_desc sdm660_bimc = {
 	.num_nodes = ARRAY_SIZE(sdm660_bimc_nodes),
 	.bus_clk_desc = &bimc_clk,
 	.regmap_cfg = &sdm660_bimc_regmap_config,
+	.ab_coeff = 153,
 };
 
 static struct qcom_icc_node * const sdm660_cnoc_nodes[] = {
@@ -1659,6 +1662,7 @@ static const struct qcom_icc_desc sdm660_mnoc = {
 	.intf_clocks = mm_intf_clocks,
 	.num_intf_clocks = ARRAY_SIZE(mm_intf_clocks),
 	.regmap_cfg = &sdm660_mnoc_regmap_config,
+	.ab_coeff = 153,
 };
 
 static struct qcom_icc_node * const sdm660_snoc_nodes[] = {

-- 
2.41.0

[PATCH v2 09/10] interconnect: qcom: msm8996: Set AB/IB coefficients

[Konrad Dybcio]

Some buses and nodes need additional manual adjustments atop the usual
calculations. Fill in the missing coefficients.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/msm8996.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
index 88683dfa468f..b73566c9b21f 100644
--- a/drivers/interconnect/qcom/msm8996.c
+++ b/drivers/interconnect/qcom/msm8996.c
@@ -448,6 +448,7 @@ static struct qcom_icc_node mas_mdp_p0 = {
 	.name = "mas_mdp_p0",
 	.id = MSM8996_MASTER_MDP_PORT0,
 	.buswidth = 32,
+	.ib_coeff = 25,
 	.mas_rpm_id = 8,
 	.slv_rpm_id = -1,
 	.qos.ap_owned = true,
@@ -463,6 +464,7 @@ static struct qcom_icc_node mas_mdp_p1 = {
 	.name = "mas_mdp_p1",
 	.id = MSM8996_MASTER_MDP_PORT1,
 	.buswidth = 32,
+	.ib_coeff = 25,
 	.mas_rpm_id = 61,
 	.slv_rpm_id = -1,
 	.qos.ap_owned = true,
@@ -1889,7 +1891,8 @@ static const struct qcom_icc_desc msm8996_bimc = {
 	.nodes = bimc_nodes,
 	.num_nodes = ARRAY_SIZE(bimc_nodes),
 	.bus_clk_desc = &bimc_clk,
-	.regmap_cfg = &msm8996_bimc_regmap_config
+	.regmap_cfg = &msm8996_bimc_regmap_config,
+	.ab_coeff = 154,
 };
 
 static struct qcom_icc_node * const cnoc_nodes[] = {
@@ -2004,7 +2007,8 @@ static const struct qcom_icc_desc msm8996_mnoc = {
 	.bus_clk_desc = &mmaxi_0_clk,
 	.intf_clocks = mm_intf_clocks,
 	.num_intf_clocks = ARRAY_SIZE(mm_intf_clocks),
-	.regmap_cfg = &msm8996_mnoc_regmap_config
+	.regmap_cfg = &msm8996_mnoc_regmap_config,
+	.ab_coeff = 154,
 };
 
 static struct qcom_icc_node * const pnoc_nodes[] = {

-- 
2.41.0

[PATCH v2 10/10] clk: qcom: smd-rpm: Move CPUSS_GNoC clock to interconnect

[Konrad Dybcio]

As it turns out, it's yet another interconnect bus clock. Move it
there.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/clk/qcom/clk-smd-rpm.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index 9b5411932594..4a23f6d3eddd 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -567,6 +567,16 @@ static const struct clk_smd_rpm *sm_qnoc_icc_clks[] = {
 	&clk_smd_rpm_bus_2_snoc_clk,
 };
 
+static const struct clk_smd_rpm *qcm2290_icc_clks[] = {
+	&clk_smd_rpm_bimc_clk,
+	&clk_smd_rpm_bus_1_cnoc_clk,
+	&clk_smd_rpm_mmnrt_clk,
+	&clk_smd_rpm_mmrt_clk,
+	&clk_smd_rpm_qup_clk,
+	&clk_smd_rpm_bus_2_snoc_clk,
+	&clk_smd_rpm_cpuss_gnoc_clk,
+};
+
 static struct clk_smd_rpm *msm8909_clks[] = {
 	[RPM_SMD_QPIC_CLK]		= &clk_smd_rpm_qpic_clk,
 	[RPM_SMD_QPIC_CLK_A]		= &clk_smd_rpm_qpic_a_clk,
@@ -1182,15 +1192,13 @@ static struct clk_smd_rpm *qcm2290_clks[] = {
 	[RPM_SMD_PKA_A_CLK] = &clk_smd_rpm_pka_a_clk,
 	[RPM_SMD_BIMC_GPU_CLK] = &clk_smd_rpm_bimc_gpu_clk,
 	[RPM_SMD_BIMC_GPU_A_CLK] = &clk_smd_rpm_bimc_gpu_a_clk,
-	[RPM_SMD_CPUSS_GNOC_CLK] = &clk_smd_rpm_cpuss_gnoc_clk,
-	[RPM_SMD_CPUSS_GNOC_A_CLK] = &clk_smd_rpm_cpuss_gnoc_a_clk,
 };
 
 static const struct rpm_smd_clk_desc rpm_clk_qcm2290 = {
 	.clks = qcm2290_clks,
 	.num_clks = ARRAY_SIZE(qcm2290_clks),
-	.icc_clks = sm_qnoc_icc_clks,
-	.num_icc_clks = ARRAY_SIZE(sm_qnoc_icc_clks)
+	.icc_clks = qcm2290_icc_clks,
+	.num_icc_clks = ARRAY_SIZE(qcm2290_icc_clks)
 };
 
 static const struct of_device_id rpm_smd_clk_match_table[] = {

-- 
2.41.0

Re: [PATCH v2 00/10] Fix up icc clock rate calculation on some platforms

[Georgi Djakov]

Hi Konrad,

On 31.07.23 13:52, Konrad Dybcio wrote:
> Certain platforms require that some buses (or individual nodes) make
> some additional changes to the clock rate formula, throwing in some
> magic, Qualcomm-defined coefficients, to account for "inefficiencies".

Maybe some links to the downstream code would help to better check and
understand this. Adding also Odelu in case he has any comments on the
patches.

Thanks,
Georgi

> Add the framework for it and utilize it on a couple SoCs.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
> Changes in v2:
> - Use the (arguably less favourable but necessary for precission) 100/x
>    instead of x/100 for ib coefficient, update values in consequent
>    patches to reflect that
> - Rename "_percent" to "_coeff" because of /\
> - Add the necessary code to support per-node clocks
> - Add the necessary code to support per-node coefficients
> - Hook up the CPUSS<->GNoC clock on QCM2290
> - Update EBI node on QCM2290
> - Link to v1: https://lore.kernel.org/r/20230726-topic-icc_coeff-v1-0-31616960818c@linaro.org
> 
> ---
> Konrad Dybcio (10):
>        interconnect: qcom: icc-rpm: Add AB/IB calculations coefficients
>        interconnect: qcom: icc-rpm: Separate out clock rate calulcations
>        interconnect: qcom: icc-rpm: Let nodes drive their own bus clock
>        interconnect: qcom: icc-rpm: Check for node-specific rate coefficients
>        interconnect: qcom: qcm2290: Hook up MAS_APPS_PROC's bus clock
>        interconnect: qcom: qcm2290: Set AB coefficients
>        interconnect: qcom: qcm2290: Update EBI channel configuration
>        interconnect: qcom: sdm660: Set AB/IB coefficients
>        interconnect: qcom: msm8996: Set AB/IB coefficients
>        clk: qcom: smd-rpm: Move CPUSS_GNoC clock to interconnect
> 
>   drivers/clk/qcom/clk-smd-rpm.c             | 16 ++++--
>   drivers/interconnect/qcom/icc-rpm-clocks.c |  6 ++
>   drivers/interconnect/qcom/icc-rpm.c        | 92 ++++++++++++++++++++++++------
>   drivers/interconnect/qcom/icc-rpm.h        | 15 +++++
>   drivers/interconnect/qcom/msm8996.c        |  8 ++-
>   drivers/interconnect/qcom/qcm2290.c        |  9 ++-
>   drivers/interconnect/qcom/sdm660.c         |  4 ++
>   7 files changed, 124 insertions(+), 26 deletions(-)
> ---
> base-commit: ec89391563792edd11d138a853901bce76d11f44
> change-id: 20230726-topic-icc_coeff-b053d5409b9f
> 
> Best regards,