[PATCH v7 0/9] The great interconnecification fixation

[PATCH v7 0/9] The great interconnecification fixation

[Konrad Dybcio]

Hi!

v6 -> v7 changelog:
- Rebase on Johan's recent patches

Link to v6: https://lore.kernel.org/r/20230228-topic-qos-v6-0-3c37a349656f@linaro.org

v5 -> v6 changelog:
- Completely rewrite the commit message of [1/9], I realized that there
  was actually no issue with the present upstream setups and the only
  drivers suffering from ghost votes were.. my own OOT drivers..
  As a consequence of that, all fixes tags were dropped and the patch
  has been kept, since it was deemed useful for newer SoCs that don't
  distinguish ap_owned nodes.

- Change the number of allowed bus_clocks from (0-2 in the previous
  revision, 0-inf in the current upstream state) to {0, 2}. Scaling is
  only possible with a pair of wake-sleep clocks, but some providers
  don't do scaling at all (see 8996 A0NoC, 660 GNoC). Drop the cheeky
  -1 / 0 / >0 checks from the previous revision. [7/9]

- bus_clocks are now forced to be named "bus", "bus_a", as there is no
  need for variance here - we don't do scaling on non-SMD RPM bus clocks.
  [7/9]

- The interface clocks are now only turned on when the associated bus
  is running at a non-zero frequency [6/9] instead of being always on
  and leaking power

Tested on MSM8996 Kagura, SM6375 PDX225 (OOT), MSM8998 Maple (OOT)

Link to v5: https://lore.kernel.org/linux-arm-msm/20230217-topic-icc-fixes-v5-v5-0-c9a550f9fdb9@linaro.org/

v4 -> v5 changelog:
- Previously the "Always set QoS params on QNoC" contained part of what
  should have been included in "make QoS INVALID default".. (very bad)
  Fix it!

- Drop negative offset and keep_alive, they will be resubmitted with new
  icc driver submissions

- use b4 this time.. hopefully the series gets to everybody now

Link to v4: https://lore.kernel.org/linux-arm-msm/20230214143720.2416762-1-konrad.dybcio@linaro.org/

v3 -> v4 changelog:
- Drop "Always set QoS params on QNoC", it only causes issues.. this
  can be investigated another day, as it's not necessary for operation

- Drop "Add a way to always set QoS registers", same as /\

- Add a way (and use it) to have no bus_clocks (the ones we set rate on),
  as at least msm8996 has a bus (A0NoC) that doesn't have any and does
  all the scaling through RPM requests

- Promote 8996 icc to core_initcall

- Introduce keep_alive (see patch [11/12]) (important!, will be used by at least 6375)

- Allow negative QoS offsets in preparation for introducing 8998 icc [12/12]

Link to v3: https://lore.kernel.org/linux-arm-msm/20230116132152.405535-1-konrad.dybcio@linaro.org/

v2 -> v3 changelog:
- Drop "Don't set QoS params before non-zero bw is requested"

- Rebase on next

- [1/9] ("..make QoS INVALID default.."): remove unused define for
  MODE_INVALID_VAL

- Pick up tags

v1 -> v2 changelog:
- reorder "make QoS INVALID default", makes more sense to have it
  before "Always set QoS params on QNoC"

- Limit ap_owned-independent QoS setting to QNoC only

- Add new patches for handling the 8996-and-friends clocks situation
  and optional BIMC regardless-of-ap_owned QoS programming

[1] https://lore.kernel.org/linux-arm-msm/14e06574-f95e-8960-0243-8c95a1c294e9@linaro.org/T/#m056692bea71d4c272968d5e07afbd9eb07a88123
[2] https://lore.kernel.org/linux-arm-msm/20230110132202.956619-1-konrad.dybcio@linaro.org/

This series grew quite a bit bigger than the previous [1] attempt, so
I decided to also add a cover letter.

Link to v2: [2]

It addresses a few things that were not quite right:

- Setting QoS params before a "real" (non-zero) bandwidth request
  makes little sense (since there's no data supposed to flow through
  the bus, why would the QoS matter) and (at least newer) downstream
  prevents that from happening. Do the same in Patch 1.

- QNoC type buses expect to always have their QoS registers set as long
  as there's a non-INVALID QoS mode set; ap_owned is not really a thing
  on these anymore, Patch 3 handles that.

- The recent MSM8996 boot fix was done quickly and not quite properly,
  leading to possibly setting the aggregate bus rate on "normal"
  hardware interface clocks; this series handles that by limiting the
  number of bus_clocks to 2 (which is the maximum that makes sense,
  anyway) and handling the rest as "intf_clocks", which are required
  to access the   hardware at the other end. Patches 5-8 take care of
  that and Patch 10 reverts the _optional moniker in clk_get_ to make
  sure we always have the bus scaling clocks, as they're well, kind
  of important ;)

- Similarly to QNoC, BIMC on "newer" (which can be loosely approximated
  by "new enough" == "has only BIMC and QNoC hosts") SoCs expects to
  always receive QoS programming, whereas BIMC on "older" SoCs cries
  like a wild boar and crashes the platform when trying to do so
  unconditionally. Patch 9 adds a way to take care of that for newer
  SoCs (like SM6375)

- QoS mode INVALID was assumed by developers before to be the default
  ("I didn't specify any QoS settings, so the driver can't assume I
  did.. right? right!?" - wrong, partial struct initialization led to
  0 being set and 0 corresponded to QoS mode FIXED). Make it so, as
  that's the logical choice. This allows the "Always set QoS params
  on QNoC" patch to work without setting tons of what-should-
  -obviously-be-the-default values everywhere, as well as fixes older
  drivers that set ap_owned = true but left the QoS mode field unset.
  Patch 2 cleans that up.

- Some nodes are physically connected over more than one channel
  (usually DDR or other high-throughput paths). Patch 4 allows that
  to be reflected in calculations. This will be required for at least
  MSM8998 and SM6375 (which will be submitted soon after this lands)

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Konrad Dybcio (9):
      interconnect: qcom: rpm: make QoS INVALID default
      interconnect: qcom: rpm: Add support for specifying channel num
      interconnect: qcom: Sort kerneldoc entries
      interconnect: qcom: rpm: Rename icc desc clocks to bus_blocks
      interconnect: qcom: rpm: Rename icc provider num_clocks to num_bus_clocks
      interconnect: qcom: rpm: Handle interface clocks
      interconnect: qcom: icc-rpm: Enforce 2 or 0 bus clocks
      interconnect: qcom: rpm: Don't use clk_get_optional for bus clocks anymore
      interconnect: qcom: msm8996: Promote to core_initcall

 drivers/interconnect/qcom/icc-rpm.c | 89 +++++++++++++++++++++++++------------
 drivers/interconnect/qcom/icc-rpm.h | 38 +++++++++++-----
 drivers/interconnect/qcom/msm8996.c | 35 +++++++++------
 drivers/interconnect/qcom/sdm660.c  | 17 +++----
 4 files changed, 115 insertions(+), 64 deletions(-)
---
base-commit: fc31900c948610e7b5c2f15fb7795832c8325327
change-id: 20230228-topic-qos-5435cac88d89

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

[PATCH v7 1/9] interconnect: qcom: rpm: make QoS INVALID default

[Konrad Dybcio]

Currently NOC_QOS_MODE_FIXED is defined as 0x0 which makes it the
default option (partial struct initialization). The default option
however should be NOC_QOS_MODE_INVALID.

That results in bogus QoS configurations being sent for port 0 (which
is used for the DRAM endpoint on BIMC, for example) coming from all nodes
with .qos.ap_owned = true and uninitialized .qos.qos_mode. It's also an
issue for newer SoCs where all nodes are treated as if they were ap_owned,
but not all of them have QoS configuration.

The NOC_QOS_MODEs are defined as preprocessor constants and are not used
anywhere outside qcom_icc_set_noc_qos(), which is easily worked around.
Separate the desc->type values from the values sent to msmbus in the
aforementioned function. Make the former an enum for better mainainability.

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

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index 4d0997b210f7..35fd75ae70e3 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -48,6 +48,9 @@
 #define NOC_QOS_MODEn_ADDR(n)		(0xc + (n * 0x1000))
 #define NOC_QOS_MODEn_MASK		0x3
 
+#define NOC_QOS_MODE_FIXED_VAL		0x0
+#define NOC_QOS_MODE_BYPASS_VAL		0x2
+
 static int qcom_icc_set_qnoc_qos(struct icc_node *src, u64 max_bw)
 {
 	struct icc_provider *provider = src->provider;
@@ -153,7 +156,7 @@ static int qcom_icc_set_noc_qos(struct icc_node *src, u64 max_bw)
 	struct qcom_icc_provider *qp;
 	struct qcom_icc_node *qn;
 	struct icc_provider *provider;
-	u32 mode = NOC_QOS_MODE_BYPASS;
+	u32 mode = NOC_QOS_MODE_BYPASS_VAL;
 	int rc = 0;
 
 	qn = src->data;
@@ -167,18 +170,17 @@ static int qcom_icc_set_noc_qos(struct icc_node *src, u64 max_bw)
 		return 0;
 	}
 
-	if (qn->qos.qos_mode != NOC_QOS_MODE_INVALID)
-		mode = qn->qos.qos_mode;
-
-	if (mode == NOC_QOS_MODE_FIXED) {
-		dev_dbg(src->provider->dev, "NoC QoS: %s: Set Fixed mode\n",
-			qn->name);
+	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;
-	} else if (mode == NOC_QOS_MODE_BYPASS) {
-		dev_dbg(src->provider->dev, "NoC QoS: %s: Set Bypass mode\n",
-			qn->name);
+	} 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;
+	} else {
+		/* How did we get here? */
 	}
 
 	return regmap_update_bits(qp->regmap,
@@ -244,7 +246,7 @@ 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) {
+	} else if (qn->qos.qos_mode != NOC_QOS_MODE_INVALID) {
 		/* set bandwidth directly from the AP */
 		ret = qcom_icc_qos_set(n, sum_bw);
 		if (ret)
diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
index a49af844ab13..8ba1918d7997 100644
--- a/drivers/interconnect/qcom/icc-rpm.h
+++ b/drivers/interconnect/qcom/icc-rpm.h
@@ -97,10 +97,12 @@ struct qcom_icc_desc {
 	unsigned int qos_offset;
 };
 
-/* Valid for both NoC and BIMC */
-#define NOC_QOS_MODE_INVALID		-1
-#define NOC_QOS_MODE_FIXED		0x0
-#define NOC_QOS_MODE_BYPASS		0x2
+/* Valid for all bus types */
+enum qos_mode {
+	NOC_QOS_MODE_INVALID = 0,
+	NOC_QOS_MODE_FIXED,
+	NOC_QOS_MODE_BYPASS,
+};
 
 int qnoc_probe(struct platform_device *pdev);
 int qnoc_remove(struct platform_device *pdev);

-- 
2.39.2

[PATCH v7 2/9] interconnect: qcom: rpm: Add support for specifying channel num

[Konrad Dybcio]

Some nodes, like EBI0 (DDR) or L3/LLCC, may be connected over more than
one channel. This should be taken into account in bandwidth calcualtion,
as we're supposed to feed msmbus with the per-channel bandwidth. Add
support for specifying that and use it during bandwidth aggregation.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/icc-rpm.c | 7 ++++++-
 drivers/interconnect/qcom/icc-rpm.h | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index 35fd75ae70e3..27c4c6497994 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -317,6 +317,7 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider,
 {
 	struct icc_node *node;
 	struct qcom_icc_node *qn;
+	u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
 	int i;
 
 	/* Initialise aggregate values */
@@ -334,7 +335,11 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider,
 	list_for_each_entry(node, &provider->nodes, node_list) {
 		qn = node->data;
 		for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
-			agg_avg[i] += qn->sum_avg[i];
+			if (qn->channels)
+				sum_avg[i] = div_u64(qn->sum_avg[i], qn->channels);
+			else
+				sum_avg[i] = qn->sum_avg[i];
+			agg_avg[i] += sum_avg[i];
 			agg_peak[i] = max_t(u64, agg_peak[i], qn->max_peak[i]);
 		}
 	}
diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
index 8ba1918d7997..8aed5400afda 100644
--- a/drivers/interconnect/qcom/icc-rpm.h
+++ b/drivers/interconnect/qcom/icc-rpm.h
@@ -66,6 +66,7 @@ struct qcom_icc_qos {
  * @id: a unique node identifier
  * @links: an array of nodes where we can go next while traversing
  * @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)
  * @sum_avg: current sum aggregate value of all avg bw requests
  * @max_peak: current max aggregate value of all peak bw requests
@@ -78,6 +79,7 @@ struct qcom_icc_node {
 	u16 id;
 	const u16 *links;
 	u16 num_links;
+	u16 channels;
 	u16 buswidth;
 	u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
 	u64 max_peak[QCOM_ICC_NUM_BUCKETS];

-- 
2.39.2

[PATCH v7 3/9] interconnect: qcom: Sort kerneldoc entries

[Konrad Dybcio]

Sort the kerneldoc entries the same way the struct members are
sorted.

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

diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
index 8aed5400afda..21f440beda86 100644
--- a/drivers/interconnect/qcom/icc-rpm.h
+++ b/drivers/interconnect/qcom/icc-rpm.h
@@ -23,12 +23,12 @@ enum qcom_icc_type {
 /**
  * struct qcom_icc_provider - Qualcomm specific interconnect provider
  * @provider: generic interconnect provider
- * @bus_clks: the clk_bulk_data table of bus clocks
  * @num_clks: the total number of clk_bulk_data entries
  * @type: the ICC provider type
- * @qos_offset: offset to QoS registers
  * @regmap: regmap for QoS registers read/write access
+ * @qos_offset: offset to QoS registers
  * @bus_clk_rate: bus clock rate in Hz
+ * @bus_clks: the clk_bulk_data table of bus clocks
  */
 struct qcom_icc_provider {
 	struct icc_provider provider;

-- 
2.39.2

[PATCH v7 4/9] interconnect: qcom: rpm: Rename icc desc clocks to bus_blocks

[Konrad Dybcio]

Rename the "clocks" (and _names) fields of qcom_icc_desc to
"bus_clocks" in preparation for introducing handling of clocks that
need to be enabled but not voted on with aggregate frequency.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/icc-rpm.c |  6 +++---
 drivers/interconnect/qcom/icc-rpm.h |  4 ++--
 drivers/interconnect/qcom/msm8996.c | 12 ++++++------
 drivers/interconnect/qcom/sdm660.c  |  8 ++++----
 4 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index 27c4c6497994..62c48fb13fbf 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -441,9 +441,9 @@ int qnoc_probe(struct platform_device *pdev)
 	qnodes = desc->nodes;
 	num_nodes = desc->num_nodes;
 
-	if (desc->num_clocks) {
-		cds = desc->clocks;
-		cd_num = desc->num_clocks;
+	if (desc->num_bus_clocks) {
+		cds = desc->bus_clocks;
+		cd_num = desc->num_bus_clocks;
 	} else {
 		cds = bus_clocks;
 		cd_num = ARRAY_SIZE(bus_clocks);
diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
index 21f440beda86..d6b4c56bf02c 100644
--- a/drivers/interconnect/qcom/icc-rpm.h
+++ b/drivers/interconnect/qcom/icc-rpm.h
@@ -91,8 +91,8 @@ struct qcom_icc_node {
 struct qcom_icc_desc {
 	struct qcom_icc_node * const *nodes;
 	size_t num_nodes;
-	const char * const *clocks;
-	size_t num_clocks;
+	const char * const *bus_clocks;
+	size_t num_bus_clocks;
 	bool has_bus_pd;
 	enum qcom_icc_type type;
 	const struct regmap_config *regmap_cfg;
diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
index 25a1a32bc611..69fc50a6fa5c 100644
--- a/drivers/interconnect/qcom/msm8996.c
+++ b/drivers/interconnect/qcom/msm8996.c
@@ -1821,8 +1821,8 @@ static const struct qcom_icc_desc msm8996_a0noc = {
 	.type = QCOM_ICC_NOC,
 	.nodes = a0noc_nodes,
 	.num_nodes = ARRAY_SIZE(a0noc_nodes),
-	.clocks = bus_a0noc_clocks,
-	.num_clocks = ARRAY_SIZE(bus_a0noc_clocks),
+	.bus_clocks = bus_a0noc_clocks,
+	.num_bus_clocks = ARRAY_SIZE(bus_a0noc_clocks),
 	.has_bus_pd = true,
 	.regmap_cfg = &msm8996_a0noc_regmap_config
 };
@@ -1866,8 +1866,8 @@ static const struct qcom_icc_desc msm8996_a2noc = {
 	.type = QCOM_ICC_NOC,
 	.nodes = a2noc_nodes,
 	.num_nodes = ARRAY_SIZE(a2noc_nodes),
-	.clocks = bus_a2noc_clocks,
-	.num_clocks = ARRAY_SIZE(bus_a2noc_clocks),
+	.bus_clocks = bus_a2noc_clocks,
+	.num_bus_clocks = ARRAY_SIZE(bus_a2noc_clocks),
 	.regmap_cfg = &msm8996_a2noc_regmap_config
 };
 
@@ -2005,8 +2005,8 @@ static const struct qcom_icc_desc msm8996_mnoc = {
 	.type = QCOM_ICC_NOC,
 	.nodes = mnoc_nodes,
 	.num_nodes = ARRAY_SIZE(mnoc_nodes),
-	.clocks = bus_mm_clocks,
-	.num_clocks = ARRAY_SIZE(bus_mm_clocks),
+	.bus_clocks = bus_mm_clocks,
+	.num_bus_clocks = ARRAY_SIZE(bus_mm_clocks),
 	.regmap_cfg = &msm8996_mnoc_regmap_config
 };
 
diff --git a/drivers/interconnect/qcom/sdm660.c b/drivers/interconnect/qcom/sdm660.c
index 8d879b0bcabc..a22ba821efbf 100644
--- a/drivers/interconnect/qcom/sdm660.c
+++ b/drivers/interconnect/qcom/sdm660.c
@@ -1516,8 +1516,8 @@ static const struct qcom_icc_desc sdm660_a2noc = {
 	.type = QCOM_ICC_NOC,
 	.nodes = sdm660_a2noc_nodes,
 	.num_nodes = ARRAY_SIZE(sdm660_a2noc_nodes),
-	.clocks = bus_a2noc_clocks,
-	.num_clocks = ARRAY_SIZE(bus_a2noc_clocks),
+	.bus_clocks = bus_a2noc_clocks,
+	.num_bus_clocks = ARRAY_SIZE(bus_a2noc_clocks),
 	.regmap_cfg = &sdm660_a2noc_regmap_config,
 };
 
@@ -1659,8 +1659,8 @@ static const struct qcom_icc_desc sdm660_mnoc = {
 	.type = QCOM_ICC_NOC,
 	.nodes = sdm660_mnoc_nodes,
 	.num_nodes = ARRAY_SIZE(sdm660_mnoc_nodes),
-	.clocks = bus_mm_clocks,
-	.num_clocks = ARRAY_SIZE(bus_mm_clocks),
+	.bus_clocks = bus_mm_clocks,
+	.num_bus_clocks = ARRAY_SIZE(bus_mm_clocks),
 	.regmap_cfg = &sdm660_mnoc_regmap_config,
 };
 

-- 
2.39.2

[PATCH v7 5/9] interconnect: qcom: rpm: Rename icc provider num_clocks to num_bus_clocks

[Konrad Dybcio]

In preparation for handling non-scaling clocks that we still have to
enable, rename num_clocks to more descriptive num_bus_clocks.

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

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index 62c48fb13fbf..b52f788d8f3d 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -380,7 +380,7 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
 			return ret;
 	}
 
-	for (i = 0; i < qp->num_clks; i++) {
+	for (i = 0; i < qp->num_bus_clks; i++) {
 		/*
 		 * Use WAKE bucket for active clock, otherwise, use SLEEP bucket
 		 * for other clocks.  If a platform doesn't set interconnect
@@ -465,7 +465,7 @@ int qnoc_probe(struct platform_device *pdev)
 
 	for (i = 0; i < cd_num; i++)
 		qp->bus_clks[i].id = cds[i];
-	qp->num_clks = cd_num;
+	qp->num_bus_clks = cd_num;
 
 	qp->type = desc->type;
 	qp->qos_offset = desc->qos_offset;
@@ -495,11 +495,11 @@ int qnoc_probe(struct platform_device *pdev)
 	}
 
 regmap_done:
-	ret = devm_clk_bulk_get_optional(dev, qp->num_clks, qp->bus_clks);
+	ret = devm_clk_bulk_get_optional(dev, qp->num_bus_clks, qp->bus_clks);
 	if (ret)
 		return ret;
 
-	ret = clk_bulk_prepare_enable(qp->num_clks, qp->bus_clks);
+	ret = clk_bulk_prepare_enable(qp->num_bus_clks, qp->bus_clks);
 	if (ret)
 		return ret;
 
@@ -558,7 +558,7 @@ int qnoc_probe(struct platform_device *pdev)
 err_remove_nodes:
 	icc_nodes_remove(provider);
 err_disable_clks:
-	clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
+	clk_bulk_disable_unprepare(qp->num_bus_clks, qp->bus_clks);
 
 	return ret;
 }
@@ -570,7 +570,7 @@ int qnoc_remove(struct platform_device *pdev)
 
 	icc_provider_deregister(&qp->provider);
 	icc_nodes_remove(&qp->provider);
-	clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
+	clk_bulk_disable_unprepare(qp->num_bus_clks, qp->bus_clks);
 
 	return 0;
 }
diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
index d6b4c56bf02c..d4401f35f6d2 100644
--- a/drivers/interconnect/qcom/icc-rpm.h
+++ b/drivers/interconnect/qcom/icc-rpm.h
@@ -23,7 +23,7 @@ enum qcom_icc_type {
 /**
  * struct qcom_icc_provider - Qualcomm specific interconnect provider
  * @provider: generic interconnect provider
- * @num_clks: the total number of clk_bulk_data entries
+ * @num_bus_clks: the total number of bus_clks clk_bulk_data entries
  * @type: the ICC provider type
  * @regmap: regmap for QoS registers read/write access
  * @qos_offset: offset to QoS registers
@@ -32,7 +32,7 @@ enum qcom_icc_type {
  */
 struct qcom_icc_provider {
 	struct icc_provider provider;
-	int num_clks;
+	int num_bus_clks;
 	enum qcom_icc_type type;
 	struct regmap *regmap;
 	unsigned int qos_offset;

-- 
2.39.2

[PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Konrad Dybcio]

Some (but not all) providers (or their specific nodes) require
specific clocks to be turned on before they can be accessed. Failure
to ensure that results in a seemingly random system crash (which
would usually happen at boot with the interconnect driver built-in),
resulting in the platform not booting up properly.

Limit the number of bus_clocks to 2 (which is the maximum that SMD
RPM interconnect supports anyway) and handle non-scaling clocks
separately. Update MSM8996 and SDM660 drivers to make sure they do
not regress with this change.

This unfortunately has to be done in one patch to prevent either
compile errors or broken bisect.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/icc-rpm.c | 52 ++++++++++++++++++++++++++++++-------
 drivers/interconnect/qcom/icc-rpm.h | 14 ++++++++--
 drivers/interconnect/qcom/msm8996.c | 22 +++++++---------
 drivers/interconnect/qcom/sdm660.c  | 16 +++++-------
 4 files changed, 70 insertions(+), 34 deletions(-)

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index b52f788d8f3d..ca932ed720fb 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -369,6 +369,17 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
 
 	qcom_icc_bus_aggregate(provider, agg_avg, agg_peak, &max_agg_avg);
 
+	/* If we're powering on the bus, ensure the necessary clocks are on */
+	if (unlikely(!qp->is_on)) {
+		if (agg_peak[0] || agg_peak[1] || max_agg_avg) {
+			/* If this fails, bus accesses will crash the platform! */
+			ret = clk_bulk_prepare_enable(qp->num_intf_clks, qp->intf_clks);
+			if (ret)
+				return ret;
+			qp->is_on = true;
+		}
+	}
+
 	sum_bw = icc_units_to_bps(max_agg_avg);
 
 	ret = __qcom_icc_set(src, src_qn, sum_bw);
@@ -409,6 +420,14 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
 		qp->bus_clk_rate[i] = rate;
 	}
 
+	/* Turn off the interface clocks if the bus was shut down so as not to leak power */
+	if (!qp->bus_clk_rate[0] && !qp->bus_clk_rate[1]) {
+		if (!agg_peak[0] && !agg_peak[1] && !max_agg_avg) {
+			clk_bulk_disable_unprepare(qp->num_intf_clks, qp->intf_clks);
+			qp->is_on = false;
+		}
+	}
+
 	return 0;
 }
 
@@ -441,21 +460,20 @@ int qnoc_probe(struct platform_device *pdev)
 	qnodes = desc->nodes;
 	num_nodes = desc->num_nodes;
 
-	if (desc->num_bus_clocks) {
-		cds = desc->bus_clocks;
-		cd_num = desc->num_bus_clocks;
+	if (desc->num_intf_clocks) {
+		cds = desc->intf_clocks;
+		cd_num = desc->num_intf_clocks;
 	} else {
-		cds = bus_clocks;
-		cd_num = ARRAY_SIZE(bus_clocks);
+		/* 0 intf clocks is perfectly fine */
+		cd_num = 0;
 	}
 
-	qp = devm_kzalloc(dev, struct_size(qp, bus_clks, cd_num), GFP_KERNEL);
+	qp = devm_kzalloc(dev, sizeof(*qp), GFP_KERNEL);
 	if (!qp)
 		return -ENOMEM;
 
-	qp->bus_clk_rate = devm_kcalloc(dev, cd_num, sizeof(*qp->bus_clk_rate),
-					GFP_KERNEL);
-	if (!qp->bus_clk_rate)
+	qp->intf_clks = devm_kzalloc(dev, sizeof(qp->intf_clks), GFP_KERNEL);
+	if (!qp->intf_clks)
 		return -ENOMEM;
 
 	data = devm_kzalloc(dev, struct_size(data, nodes, num_nodes),
@@ -463,6 +481,18 @@ int qnoc_probe(struct platform_device *pdev)
 	if (!data)
 		return -ENOMEM;
 
+	qp->num_intf_clks = cd_num;
+	for (i = 0; i < cd_num; i++)
+		qp->intf_clks[i].id = cds[i];
+
+	if (desc->num_bus_clocks) {
+		cds = desc->bus_clocks;
+		cd_num = desc->num_bus_clocks;
+	} else {
+		cds = bus_clocks;
+		cd_num = ARRAY_SIZE(bus_clocks);
+	}
+
 	for (i = 0; i < cd_num; i++)
 		qp->bus_clks[i].id = cds[i];
 	qp->num_bus_clks = cd_num;
@@ -503,6 +533,10 @@ int qnoc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = devm_clk_bulk_get(dev, qp->num_intf_clks, qp->intf_clks);
+	if (ret)
+		return ret;
+
 	if (desc->has_bus_pd) {
 		ret = dev_pm_domain_attach(dev, true);
 		goto err_disable_clks;
diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
index d4401f35f6d2..a4ef45b4a9e0 100644
--- a/drivers/interconnect/qcom/icc-rpm.h
+++ b/drivers/interconnect/qcom/icc-rpm.h
@@ -20,24 +20,32 @@ enum qcom_icc_type {
 	QCOM_ICC_QNOC,
 };
 
+#define NUM_BUS_CLKS	2
+
 /**
  * struct qcom_icc_provider - Qualcomm specific interconnect provider
  * @provider: generic interconnect provider
  * @num_bus_clks: the total number of bus_clks clk_bulk_data entries
+ * @num_intf_clks: the total number of intf_clks clk_bulk_data entries
  * @type: the ICC provider type
  * @regmap: regmap for QoS registers read/write access
  * @qos_offset: offset to QoS registers
  * @bus_clk_rate: bus clock rate in Hz
  * @bus_clks: the clk_bulk_data table of bus clocks
+ * @intf_clks: a clk_bulk_data array of interface clocks
+ * @is_on: whether the bus is powered on
  */
 struct qcom_icc_provider {
 	struct icc_provider provider;
 	int num_bus_clks;
+	int num_intf_clks;
 	enum qcom_icc_type type;
 	struct regmap *regmap;
 	unsigned int qos_offset;
-	u64 *bus_clk_rate;
-	struct clk_bulk_data bus_clks[];
+	u64 bus_clk_rate[NUM_BUS_CLKS];
+	struct clk_bulk_data bus_clks[NUM_BUS_CLKS];
+	struct clk_bulk_data *intf_clks;
+	bool is_on;
 };
 
 /**
@@ -93,6 +101,8 @@ struct qcom_icc_desc {
 	size_t num_nodes;
 	const char * const *bus_clocks;
 	size_t num_bus_clocks;
+	const char * const *intf_clocks;
+	size_t num_intf_clocks;
 	bool has_bus_pd;
 	enum qcom_icc_type type;
 	const struct regmap_config *regmap_cfg;
diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
index 69fc50a6fa5c..1a5e0ad36cc4 100644
--- a/drivers/interconnect/qcom/msm8996.c
+++ b/drivers/interconnect/qcom/msm8996.c
@@ -21,21 +21,17 @@
 #include "smd-rpm.h"
 #include "msm8996.h"
 
-static const char * const bus_mm_clocks[] = {
-	"bus",
-	"bus_a",
+static const char * const mm_intf_clocks[] = {
 	"iface"
 };
 
-static const char * const bus_a0noc_clocks[] = {
+static const char * const a0noc_intf_clocks[] = {
 	"aggre0_snoc_axi",
 	"aggre0_cnoc_ahb",
 	"aggre0_noc_mpu_cfg"
 };
 
-static const char * const bus_a2noc_clocks[] = {
-	"bus",
-	"bus_a",
+static const char * const a2noc_intf_clocks[] = {
 	"aggre2_ufs_axi",
 	"ufs_axi"
 };
@@ -1821,8 +1817,8 @@ static const struct qcom_icc_desc msm8996_a0noc = {
 	.type = QCOM_ICC_NOC,
 	.nodes = a0noc_nodes,
 	.num_nodes = ARRAY_SIZE(a0noc_nodes),
-	.bus_clocks = bus_a0noc_clocks,
-	.num_bus_clocks = ARRAY_SIZE(bus_a0noc_clocks),
+	.intf_clocks = a0noc_intf_clocks,
+	.num_intf_clocks = ARRAY_SIZE(a0noc_intf_clocks),
 	.has_bus_pd = true,
 	.regmap_cfg = &msm8996_a0noc_regmap_config
 };
@@ -1866,8 +1862,8 @@ static const struct qcom_icc_desc msm8996_a2noc = {
 	.type = QCOM_ICC_NOC,
 	.nodes = a2noc_nodes,
 	.num_nodes = ARRAY_SIZE(a2noc_nodes),
-	.bus_clocks = bus_a2noc_clocks,
-	.num_bus_clocks = ARRAY_SIZE(bus_a2noc_clocks),
+	.intf_clocks = a2noc_intf_clocks,
+	.num_intf_clocks = ARRAY_SIZE(a2noc_intf_clocks),
 	.regmap_cfg = &msm8996_a2noc_regmap_config
 };
 
@@ -2005,8 +2001,8 @@ static const struct qcom_icc_desc msm8996_mnoc = {
 	.type = QCOM_ICC_NOC,
 	.nodes = mnoc_nodes,
 	.num_nodes = ARRAY_SIZE(mnoc_nodes),
-	.bus_clocks = bus_mm_clocks,
-	.num_bus_clocks = ARRAY_SIZE(bus_mm_clocks),
+	.intf_clocks = mm_intf_clocks,
+	.num_intf_clocks = ARRAY_SIZE(mm_intf_clocks),
 	.regmap_cfg = &msm8996_mnoc_regmap_config
 };
 
diff --git a/drivers/interconnect/qcom/sdm660.c b/drivers/interconnect/qcom/sdm660.c
index a22ba821efbf..0e8a96f4ce90 100644
--- a/drivers/interconnect/qcom/sdm660.c
+++ b/drivers/interconnect/qcom/sdm660.c
@@ -127,15 +127,11 @@ enum {
 	SDM660_SNOC,
 };
 
-static const char * const bus_mm_clocks[] = {
-	"bus",
-	"bus_a",
+static const char * const mm_intf_clocks[] = {
 	"iface",
 };
 
-static const char * const bus_a2noc_clocks[] = {
-	"bus",
-	"bus_a",
+static const char * const a2noc_intf_clocks[] = {
 	"ipa",
 	"ufs_axi",
 	"aggre2_ufs_axi",
@@ -1516,8 +1512,8 @@ static const struct qcom_icc_desc sdm660_a2noc = {
 	.type = QCOM_ICC_NOC,
 	.nodes = sdm660_a2noc_nodes,
 	.num_nodes = ARRAY_SIZE(sdm660_a2noc_nodes),
-	.bus_clocks = bus_a2noc_clocks,
-	.num_bus_clocks = ARRAY_SIZE(bus_a2noc_clocks),
+	.intf_clocks = a2noc_intf_clocks,
+	.num_intf_clocks = ARRAY_SIZE(a2noc_intf_clocks),
 	.regmap_cfg = &sdm660_a2noc_regmap_config,
 };
 
@@ -1659,8 +1655,8 @@ static const struct qcom_icc_desc sdm660_mnoc = {
 	.type = QCOM_ICC_NOC,
 	.nodes = sdm660_mnoc_nodes,
 	.num_nodes = ARRAY_SIZE(sdm660_mnoc_nodes),
-	.bus_clocks = bus_mm_clocks,
-	.num_bus_clocks = ARRAY_SIZE(bus_mm_clocks),
+	.intf_clocks = mm_intf_clocks,
+	.num_intf_clocks = ARRAY_SIZE(mm_intf_clocks),
 	.regmap_cfg = &sdm660_mnoc_regmap_config,
 };
 

-- 
2.39.2

[PATCH v7 7/9] interconnect: qcom: icc-rpm: Enforce 2 or 0 bus clocks

[Konrad Dybcio]

For SMD RPM bus scaling to work, we need a pair of sleep-wake clocks.
The variable number of them we previously supported was only a hack
to keep the clocks required for QoS register access, but now that
these are separated, we can leave bus_clks to the actual bus clocks.

In cases where there is no actual bus scaling (such as A0NoC on MSM8996
and GNoC on SDM660 where the HLOS is only supposed to program the QoS
registers and the bus is either static or controlled remotely), allow
for no clock scaling with a boolean property.

Remove all the code related to allowing an arbitrary number of bus_clks,
replace the number by BUS_CLK_MAX (= 2) and guard the bus clock paths
to ensure they are not taken on non-scaling buses.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/icc-rpm.c | 14 +++-----------
 drivers/interconnect/qcom/icc-rpm.h |  4 ++--
 drivers/interconnect/qcom/msm8996.c |  1 +
 drivers/interconnect/qcom/sdm660.c  |  1 +
 4 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index ca932ed720fb..31d069433343 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -485,17 +485,9 @@ int qnoc_probe(struct platform_device *pdev)
 	for (i = 0; i < cd_num; i++)
 		qp->intf_clks[i].id = cds[i];
 
-	if (desc->num_bus_clocks) {
-		cds = desc->bus_clocks;
-		cd_num = desc->num_bus_clocks;
-	} else {
-		cds = bus_clocks;
-		cd_num = ARRAY_SIZE(bus_clocks);
-	}
-
-	for (i = 0; i < cd_num; i++)
-		qp->bus_clks[i].id = cds[i];
-	qp->num_bus_clks = cd_num;
+	qp->num_bus_clks = desc->no_clk_scaling ? 0 : NUM_BUS_CLKS;
+	for (i = 0; i < qp->num_bus_clks; i++)
+		qp->bus_clks[i].id = bus_clocks[i];
 
 	qp->type = desc->type;
 	qp->qos_offset = desc->qos_offset;
diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
index a4ef45b4a9e0..3cddff44b4ef 100644
--- a/drivers/interconnect/qcom/icc-rpm.h
+++ b/drivers/interconnect/qcom/icc-rpm.h
@@ -25,7 +25,7 @@ enum qcom_icc_type {
 /**
  * struct qcom_icc_provider - Qualcomm specific interconnect provider
  * @provider: generic interconnect provider
- * @num_bus_clks: the total number of bus_clks clk_bulk_data entries
+ * @num_bus_clks: the total number of bus_clks clk_bulk_data entries (0 or 2)
  * @num_intf_clks: the total number of intf_clks clk_bulk_data entries
  * @type: the ICC provider type
  * @regmap: regmap for QoS registers read/write access
@@ -100,10 +100,10 @@ struct qcom_icc_desc {
 	struct qcom_icc_node * const *nodes;
 	size_t num_nodes;
 	const char * const *bus_clocks;
-	size_t num_bus_clocks;
 	const char * const *intf_clocks;
 	size_t num_intf_clocks;
 	bool has_bus_pd;
+	bool no_clk_scaling;
 	enum qcom_icc_type type;
 	const struct regmap_config *regmap_cfg;
 	unsigned int qos_offset;
diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
index 1a5e0ad36cc4..347fe59ec293 100644
--- a/drivers/interconnect/qcom/msm8996.c
+++ b/drivers/interconnect/qcom/msm8996.c
@@ -1820,6 +1820,7 @@ static const struct qcom_icc_desc msm8996_a0noc = {
 	.intf_clocks = a0noc_intf_clocks,
 	.num_intf_clocks = ARRAY_SIZE(a0noc_intf_clocks),
 	.has_bus_pd = true,
+	.no_clk_scaling = true,
 	.regmap_cfg = &msm8996_a0noc_regmap_config
 };
 
diff --git a/drivers/interconnect/qcom/sdm660.c b/drivers/interconnect/qcom/sdm660.c
index 0e8a96f4ce90..7ffaf70d62d3 100644
--- a/drivers/interconnect/qcom/sdm660.c
+++ b/drivers/interconnect/qcom/sdm660.c
@@ -1616,6 +1616,7 @@ static const struct qcom_icc_desc sdm660_gnoc = {
 	.nodes = sdm660_gnoc_nodes,
 	.num_nodes = ARRAY_SIZE(sdm660_gnoc_nodes),
 	.regmap_cfg = &sdm660_gnoc_regmap_config,
+	.no_clk_scaling = true,
 };
 
 static struct qcom_icc_node * const sdm660_mnoc_nodes[] = {

-- 
2.39.2

[PATCH v7 8/9] interconnect: qcom: rpm: Don't use clk_get_optional for bus clocks anymore

[Konrad Dybcio]

Commit dd42ec8ea5b9 ("interconnect: qcom: rpm: Use _optional func for provider clocks")
relaxed the requirements around probing bus clocks. This was a decent
solution for making sure MSM8996 would still boot with old DTs, but
now that there's a proper fix in place that both old and new DTs
will be happy about, revert back to the safer variant of the
function.

Fixes: dd42ec8ea5b9 ("interconnect: qcom: rpm: Use _optional func for provider clocks")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/icc-rpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index 31d069433343..554fb27de4be 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -517,7 +517,7 @@ int qnoc_probe(struct platform_device *pdev)
 	}
 
 regmap_done:
-	ret = devm_clk_bulk_get_optional(dev, qp->num_bus_clks, qp->bus_clks);
+	ret = devm_clk_bulk_get(dev, qp->num_bus_clks, qp->bus_clks);
 	if (ret)
 		return ret;
 

-- 
2.39.2

[PATCH v7 9/9] interconnect: qcom: msm8996: Promote to core_initcall

[Konrad Dybcio]

The interconnect driver is (or soon will be) vital to many other
devices, as it's not a given that the bootloader will set up enough
bandwidth for us or that the values we come into are reasonable.

Promote the driver to core_initcall to ensure the consumers (i.e.
most "meaningful" parts of the SoC) can probe without deferrals.

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

diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
index 347fe59ec293..1eb51ed18b0b 100644
--- a/drivers/interconnect/qcom/msm8996.c
+++ b/drivers/interconnect/qcom/msm8996.c
@@ -2109,7 +2109,17 @@ static struct platform_driver qnoc_driver = {
 		.sync_state = icc_sync_state,
 	}
 };
-module_platform_driver(qnoc_driver);
+static int __init qnoc_driver_init(void)
+{
+	return platform_driver_register(&qnoc_driver);
+}
+core_initcall(qnoc_driver_init);
+
+static void __exit qnoc_driver_exit(void)
+{
+	platform_driver_unregister(&qnoc_driver);
+}
+module_exit(qnoc_driver_exit);
 
 MODULE_AUTHOR("Yassine Oudjana <y.oudjana@protonmail.com>");
 MODULE_DESCRIPTION("Qualcomm MSM8996 NoC driver");

-- 
2.39.2

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Bryan O'Donoghue]

On 08/03/2023 21:40, Konrad Dybcio wrote:
> Some (but not all) providers (or their specific nodes) require
> specific clocks to be turned on before they can be accessed. Failure
> to ensure that results in a seemingly random system crash (which
> would usually happen at boot with the interconnect driver built-in),
> resulting in the platform not booting up properly.

Can you give an example of which clocks on which SoC's ?

Is the intention of this patch to subsequently go through *.dts *.dtsi 
and start to remove assigned-clocks ?

Are we saying that currently there ought to be assigned-clocks for some 
of these NoC declarations ?

---
bod

Re: [PATCH v7 9/9] interconnect: qcom: msm8996: Promote to core_initcall

[Bryan O'Donoghue]

On 08/03/2023 21:40, Konrad Dybcio wrote:
> The interconnect driver is (or soon will be) vital to many other
> devices, as it's not a given that the bootloader will set up enough
> bandwidth for us or that the values we come into are reasonable.
> 
> Promote the driver to core_initcall to ensure the consumers (i.e.
> most "meaningful" parts of the SoC) can probe without deferrals.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/interconnect/qcom/msm8996.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
> index 347fe59ec293..1eb51ed18b0b 100644
> --- a/drivers/interconnect/qcom/msm8996.c
> +++ b/drivers/interconnect/qcom/msm8996.c
> @@ -2109,7 +2109,17 @@ static struct platform_driver qnoc_driver = {
>   		.sync_state = icc_sync_state,
>   	}
>   };
> -module_platform_driver(qnoc_driver);
> +static int __init qnoc_driver_init(void)
> +{
> +	return platform_driver_register(&qnoc_driver);
> +}
> +core_initcall(qnoc_driver_init);
> +
> +static void __exit qnoc_driver_exit(void)
> +{
> +	platform_driver_unregister(&qnoc_driver);
> +}
> +module_exit(qnoc_driver_exit);
>   
>   MODULE_AUTHOR("Yassine Oudjana <y.oudjana@protonmail.com>");
>   MODULE_DESCRIPTION("Qualcomm MSM8996 NoC driver");
> 

Its probably the right-thing-to-do to have interconnects probe earlier 
but, then, why restrict that to 8996 only ?

---
bod

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Konrad Dybcio]

On 10.03.2023 15:21, Bryan O'Donoghue wrote:
> On 08/03/2023 21:40, Konrad Dybcio wrote:
>> Some (but not all) providers (or their specific nodes) require
>> specific clocks to be turned on before they can be accessed. Failure
>> to ensure that results in a seemingly random system crash (which
>> would usually happen at boot with the interconnect driver built-in),
>> resulting in the platform not booting up properly.
> 
> Can you give an example of which clocks on which SoC's ?
See for example 67fb53745e0b

This was a clock documented downstream under the node-qos clocks here:

https://github.com/sonyxperiadev/kernel/blob/aosp/LA.UM.5.7.r1/arch/arm/boot/dts/qcom/msm8996-bus.dtsi#L102-L109

but there are occasions where such clocks are undocumented and downstream
skips them because it relies on them being on by miracle, such as the case
of MASTER_IPA and the IPA rpmcc clock on msm8998. Downstream has no
sync_state, so they would only set the QoS registers when the relevant
hardware was online, so the clocks were on already.

> 
> Is the intention of this patch to subsequently go through *.dts *.dtsi and start to remove assigned-clocks ?
> 
> Are we saying that currently there ought to be assigned-clocks for some of these NoC declarations ?
Not really, assigned-clocks are used for static ratesetting, see
for example dwc3 nodes where we need it to be fast enough for
HS/SS operation at all times (though that should have prooobably
been handled in the driver but it's a separate topic), I don't
think any of them were used to combat what this commit tries to.

Konrad
> 
> ---
> bod

Re: [PATCH v7 9/9] interconnect: qcom: msm8996: Promote to core_initcall

[Konrad Dybcio]

On 10.03.2023 15:23, Bryan O'Donoghue wrote:
> On 08/03/2023 21:40, Konrad Dybcio wrote:
>> The interconnect driver is (or soon will be) vital to many other
>> devices, as it's not a given that the bootloader will set up enough
>> bandwidth for us or that the values we come into are reasonable.
>>
>> Promote the driver to core_initcall to ensure the consumers (i.e.
>> most "meaningful" parts of the SoC) can probe without deferrals.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/interconnect/qcom/msm8996.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
>> index 347fe59ec293..1eb51ed18b0b 100644
>> --- a/drivers/interconnect/qcom/msm8996.c
>> +++ b/drivers/interconnect/qcom/msm8996.c
>> @@ -2109,7 +2109,17 @@ static struct platform_driver qnoc_driver = {
>>           .sync_state = icc_sync_state,
>>       }
>>   };
>> -module_platform_driver(qnoc_driver);
>> +static int __init qnoc_driver_init(void)
>> +{
>> +    return platform_driver_register(&qnoc_driver);
>> +}
>> +core_initcall(qnoc_driver_init);
>> +
>> +static void __exit qnoc_driver_exit(void)
>> +{
>> +    platform_driver_unregister(&qnoc_driver);
>> +}
>> +module_exit(qnoc_driver_exit);
>>     MODULE_AUTHOR("Yassine Oudjana <y.oudjana@protonmail.com>");
>>   MODULE_DESCRIPTION("Qualcomm MSM8996 NoC driver");
>>
> 
> Its probably the right-thing-to-do to have interconnects probe earlier but, then, why restrict that to 8996 only ?
To be honest with you, this one caught my attention and it was the
one I tested things on.. But yeah, they should all be probing ASAP.

Konrad
> 
> ---
> bod

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Bryan O'Donoghue]

On 10/03/2023 14:26, Konrad Dybcio wrote:
> 
> 
> On 10.03.2023 15:21, Bryan O'Donoghue wrote:
>> On 08/03/2023 21:40, Konrad Dybcio wrote:
>>> Some (but not all) providers (or their specific nodes) require
>>> specific clocks to be turned on before they can be accessed. Failure
>>> to ensure that results in a seemingly random system crash (which
>>> would usually happen at boot with the interconnect driver built-in),
>>> resulting in the platform not booting up properly.
>>
>> Can you give an example of which clocks on which SoC's ?
> See for example 67fb53745e0b
> 
> This was a clock documented downstream under the node-qos clocks here:
> 
> https://github.com/sonyxperiadev/kernel/blob/aosp/LA.UM.5.7.r1/arch/arm/boot/dts/qcom/msm8996-bus.dtsi#L102-L109
> 
> but there are occasions where such clocks are undocumented and downstream
> skips them because it relies on them being on by miracle, such as the case
> of MASTER_IPA and the IPA rpmcc clock on msm8998. Downstream has no
> sync_state, so they would only set the QoS registers when the relevant
> hardware was online, so the clocks were on already.

What switched the clocks on ? Presumably LK.

Is this a symptom of using a bootloader other than LK ? If you use the 
same bootloader, then why hasn't the bootloader/LK already set it up on 
your platform ?

>>
>> Is the intention of this patch to subsequently go through *.dts *.dtsi and start to remove assigned-clocks ?
>>
>> Are we saying that currently there ought to be assigned-clocks for some of these NoC declarations ?
> Not really, assigned-clocks are used for static ratesetting, see
> for example dwc3 nodes where we need it to be fast enough for
> HS/SS operation at all times (though that should have prooobably
> been handled in the driver but it's a separate topic), I don't
> think any of them were used to combat what this commit tries to.

I think you could use assigned-clocks for that ..

So its not part of your series but then presumably you have a follow-on 
patch for the 8998 dts that points your ->intf_clks at these then ?

clocks = <&clock_gcc clk_aggre2_noc_clk>,
          <&clock_gcc clk_gcc_ufs_axi_clk>,
          <&clock_gcc clk_gcc_aggre2_ufs_axi_clk>;

It seems like the right thing to do..

Still not clear why these clocks are off.. your bootchain ?

---
bod

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Konrad Dybcio]

On 10.03.2023 17:47, Bryan O'Donoghue wrote:
> On 10/03/2023 14:26, Konrad Dybcio wrote:
>>
>>
>> On 10.03.2023 15:21, Bryan O'Donoghue wrote:
>>> On 08/03/2023 21:40, Konrad Dybcio wrote:
>>>> Some (but not all) providers (or their specific nodes) require
>>>> specific clocks to be turned on before they can be accessed. Failure
>>>> to ensure that results in a seemingly random system crash (which
>>>> would usually happen at boot with the interconnect driver built-in),
>>>> resulting in the platform not booting up properly.
>>>
>>> Can you give an example of which clocks on which SoC's ?
>> See for example 67fb53745e0b
>>
>> This was a clock documented downstream under the node-qos clocks here:
>>
>> https://github.com/sonyxperiadev/kernel/blob/aosp/LA.UM.5.7.r1/arch/arm/boot/dts/qcom/msm8996-bus.dtsi#L102-L109
>>
>> but there are occasions where such clocks are undocumented and downstream
>> skips them because it relies on them being on by miracle, such as the case
>> of MASTER_IPA and the IPA rpmcc clock on msm8998. Downstream has no
>> sync_state, so they would only set the QoS registers when the relevant
>> hardware was online, so the clocks were on already.
> 
> What switched the clocks on ? Presumably LK.
> 
> Is this a symptom of using a bootloader other than LK ? If you use the same bootloader, then why hasn't the bootloader/LK already set it up on your platform ?
XBL* in this case (qcom fully switched to UEFI with 8998, I'm not
using anything other to what came on the device)

Setting up these dependencies happens all throughout the boot
chain: after the SoC starts up, BIMC/PNoC/CNoC/SNoC(/AnNoC) are
set up by some early firmware so that DDR, USB, UFS/eMMC, crypto
engines etc. can be reachable. It's *that* level of deep..

Then, they are not shut down at all, leaving USB etc. accessible
by default.

> 
>>>
>>> Is the intention of this patch to subsequently go through *.dts *.dtsi and start to remove assigned-clocks ?
>>>
>>> Are we saying that currently there ought to be assigned-clocks for some of these NoC declarations ?
>> Not really, assigned-clocks are used for static ratesetting, see
>> for example dwc3 nodes where we need it to be fast enough for
>> HS/SS operation at all times (though that should have prooobably
>> been handled in the driver but it's a separate topic), I don't
>> think any of them were used to combat what this commit tries to.
> 
> I think you could use assigned-clocks for that ..
> 
> So its not part of your series but then presumably you have a follow-on patch for the 8998 dts that points your ->intf_clks at these then ?
> 
> clocks = <&clock_gcc clk_aggre2_noc_clk>,
>          <&clock_gcc clk_gcc_ufs_axi_clk>,
>          <&clock_gcc clk_gcc_aggre2_ufs_axi_clk>;
> 
> It seems like the right thing to do..
Why so? We're passing all clock references to clocks=<> and we handle
them separately. This is akin to treating the "core" clock differently
to the "iface" clock on some IP block, except on a wider scale.

> 
> Still not clear why these clocks are off.. your bootchain ?
Generally the issue is that icc sync_states goes over *all the possible
interconnect paths on the entire SoC*. For QoS register access, you need
to be able to interface them. Some of the hardware blocks live on a
separate sort of "island". That' part of why clocks such as
GCC_CFG_NOC_USB3_PRIM_AXI_CLK exist. They're responsible for clocking
the CNoC<->USB connection, which in the bigger picture looks more or less
like:


    1     2-3            2-3    3-4            3-4    4-5
USB<->CNoC<->CNoC_to_SNoC<->SNoC<->SNoC_to_BIMC<->BIMC<->APSS

where:

1 = GCC_CFG_NOC_USB3_PRIM_AXI_CLK
2 = RPM CNOC CLK
3 = RPM SNOC CLK
4 = RPM BIMC CLK
5 = (usually internally managed) HMSS / GNOC CLK

or something along these lines, the *NoC names may be in the wrong order
but this is the general picture.

On msm-4.x there is no such thing as sync_state. The votes are only
cast from within the IP-block drivers themselves, using data gathered from
qcom,msmbus-blahblah and msmbus calls from within the driver. That way,
downstream ensures there's never unclocked QoS register access.

After writing this entire email, I got an idea that we could consider not
accessing these QoS registers from within sync_state (e.g. use sth like
if(is_sync_state_done))..

That would lead to this patch being mostly
irrelevant (IF AND ONLY IF all the necessary clocks were handled by the
end device drivers AND clk/icc voting was done in correct order - which
as we can tell from the sad 8996 example, is not always the case).

Not guaranteeing it (like this patch does) would make it worse from the
standpoint of representing the hardware needs properly, but it could
surely save some headaches. To an extent.

Konrad
> 
> ---
> bod

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Bryan O'Donoghue]

On 10/03/2023 18:05, Konrad Dybcio wrote:
>> I think you could use assigned-clocks for that ..
>>
>> So its not part of your series but then presumably you have a follow-on patch for the 8998 dts that points your ->intf_clks at these then ?
>>
>> clocks = <&clock_gcc clk_aggre2_noc_clk>,
>>           <&clock_gcc clk_gcc_ufs_axi_clk>,
>>           <&clock_gcc clk_gcc_aggre2_ufs_axi_clk>;
>>
>> It seems like the right thing to do..
> Why so? We're passing all clock references to clocks=<> and we handle
> them separately. This is akin to treating the "core" clock differently
> to the "iface" clock on some IP block, except on a wider scale.

Eh, that was a question, not a statement. I mean to ask if your 
intf_clks are intended to be populated with some/all of the above 
additional gcc references ?

>> Still not clear why these clocks are off.. your bootchain ?
> Generally the issue is that icc sync_states goes over *all the possible
> interconnect paths on the entire SoC*. For QoS register access, you need
> to be able to interface them. Some of the hardware blocks live on a
> separate sort of "island". That' part of why clocks such as
> GCC_CFG_NOC_USB3_PRIM_AXI_CLK exist. They're responsible for clocking
> the CNoC<->USB connection, which in the bigger picture looks more or less
> like:
> 
> 
>      1     2-3            2-3    3-4            3-4    4-5
> USB<->CNoC<->CNoC_to_SNoC<->SNoC<->SNoC_to_BIMC<->BIMC<->APSS
> 
> where:
> 
> 1 = GCC_CFG_NOC_USB3_PRIM_AXI_CLK
> 2 = RPM CNOC CLK
> 3 = RPM SNOC CLK
> 4 = RPM BIMC CLK
> 5 = (usually internally managed) HMSS / GNOC CLK
> 
> or something along these lines, the *NoC names may be in the wrong order
> but this is the general picture.
> 
> On msm-4.x there is no such thing as sync_state. The votes are only
> cast from within the IP-block drivers themselves, using data gathered from
> qcom,msmbus-blahblah and msmbus calls from within the driver. That way,
> downstream ensures there's never unclocked QoS register access.
> 
> After writing this entire email, I got an idea that we could consider not
> accessing these QoS registers from within sync_state (e.g. use sth like
> if(is_sync_state_done))..
> 
> That would lead to this patch being mostly
> irrelevant (IF AND ONLY IF all the necessary clocks were handled by the
> end device drivers AND clk/icc voting was done in correct order - which
> as we can tell from the sad 8996 example, is not always the case).
> 
> Not guaranteeing it (like this patch does) would make it worse from the
> standpoint of representing the hardware needs properly, but it could
> surely save some headaches. To an extent.

Hmm.

If I have understood you correctly above, then for some of the NoC QoS 
entries we basically need additional clocks, which is separate to the 
clocks the controller bus and bus_a clocks.

We don't see the problem all the time because of sync_state, so your 
question is should we push the clocks to the devices. Based on the dtsi 
you gave as an example, my €0.02 would say no, those are clearly 
additional clock dependencies for NoC.

Going by the name, you'd expect the UFS controller could own these two 
clocks

"clk-gcc-ufs-axi-clk",
"clk-aggre2-ufs-axi-clk-no-rate"

but even then by the same logic the NoC should own 
"clk-aggre2-noc-clk-no-rate" I wouldn't much fancy splitting them apart.

So - I'd say the commit log doesn't really explain to me what we have 
discussed here.

Suggest rewording it a little bit "non-scaling clock" is accurate but 
for me on the first read doesn't really tell me that these are 
node-specific clock dependencies or that there is an expectation that 
the intf_clks should be tied to node-specific clocks.

So two asks

- Update the commit log and potentially the structure comments
- Can you split the devm_kzalloc() into a seperate patch ?
   I don't see why this needs to be done but if it does need to be
   done it could as far as I read it be done before this patch

---
bod

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Konrad Dybcio]

On 11.03.2023 01:16, Bryan O'Donoghue wrote:
> On 10/03/2023 18:05, Konrad Dybcio wrote:
>>> I think you could use assigned-clocks for that ..
>>>
>>> So its not part of your series but then presumably you have a follow-on patch for the 8998 dts that points your ->intf_clks at these then ?
>>>
>>> clocks = <&clock_gcc clk_aggre2_noc_clk>,
>>>           <&clock_gcc clk_gcc_ufs_axi_clk>,
>>>           <&clock_gcc clk_gcc_aggre2_ufs_axi_clk>;
>>>
>>> It seems like the right thing to do..
>> Why so? We're passing all clock references to clocks=<> and we handle
>> them separately. This is akin to treating the "core" clock differently
>> to the "iface" clock on some IP block, except on a wider scale.
> 
> Eh, that was a question, not a statement. I mean to ask if your intf_clks are intended to be populated with some/all of the above additional gcc references ?
Argh sorry, I'm not great at reading lately..

Yes that would be the plan. All of these ones from downstream plus more
that we discover as we go (if we choose to go this route)

And the discovery process looks like:

1. boot with "disabling clk %s" debug prints added
2. do something that triggers a crash (e.g. wait for an IP block
   to enter its suspend routine)
3. crash horribly
4. try to recover the last disabled clock's name or analyze WCGW
5. try resolving the clock dependency
6. goto 1 until you don't crash anymore

> 
>>> Still not clear why these clocks are off.. your bootchain ?
>> Generally the issue is that icc sync_states goes over *all the possible
>> interconnect paths on the entire SoC*. For QoS register access, you need
>> to be able to interface them. Some of the hardware blocks live on a
>> separate sort of "island". That' part of why clocks such as
>> GCC_CFG_NOC_USB3_PRIM_AXI_CLK exist. They're responsible for clocking
>> the CNoC<->USB connection, which in the bigger picture looks more or less
>> like:
>>
>>
>>      1     2-3            2-3    3-4            3-4    4-5
>> USB<->CNoC<->CNoC_to_SNoC<->SNoC<->SNoC_to_BIMC<->BIMC<->APSS
>>
>> where:
>>
>> 1 = GCC_CFG_NOC_USB3_PRIM_AXI_CLK
>> 2 = RPM CNOC CLK
>> 3 = RPM SNOC CLK
>> 4 = RPM BIMC CLK
>> 5 = (usually internally managed) HMSS / GNOC CLK
>>
>> or something along these lines, the *NoC names may be in the wrong order
>> but this is the general picture.
>>
>> On msm-4.x there is no such thing as sync_state. The votes are only
>> cast from within the IP-block drivers themselves, using data gathered from
>> qcom,msmbus-blahblah and msmbus calls from within the driver. That way,
>> downstream ensures there's never unclocked QoS register access.
>>
>> After writing this entire email, I got an idea that we could consider not
>> accessing these QoS registers from within sync_state (e.g. use sth like
>> if(is_sync_state_done))..
>>
>> That would lead to this patch being mostly
>> irrelevant (IF AND ONLY IF all the necessary clocks were handled by the
>> end device drivers AND clk/icc voting was done in correct order - which
>> as we can tell from the sad 8996 example, is not always the case).
>>
>> Not guaranteeing it (like this patch does) would make it worse from the
>> standpoint of representing the hardware needs properly, but it could
>> surely save some headaches. To an extent.
> 
> Hmm.
> 
> If I have understood you correctly above, then for some of the NoC QoS entries we basically need additional clocks, which is separate to the clocks the controller bus and bus_a clocks.
Yes.

(Technically we need them for all of the nodes, but for example we're not
going to poke at DDR registers when the DDR is shut off)

> 
> We don't see the problem all the time because of sync_state,
No, actually it's sync_state (+ improper sequencing of turning on clocks
and regulators/power domains vs setting ICC BW in device drivers) that
causes this. Downstream piggybacks off of the devices being already
set up and ready to go before setting BW (and by extension QoS).

so your question is should we push the clocks to the devices.
Yes, it's either this patch and blindly finding the required clocks

or skipping QoS on sync_state (I haven't tested whether it'd enough
yet, FWIW) with this patch and less finding required clocks (as we
could partially piggyback off of the clocks being enabled before we
cast ICC votes as part of resume/init sequence of a driver)

or being like downstream and (almost) only relying on the client devices

Based on the dtsi you gave as an example, my €0.02 would say no, those are clearly additional clock dependencies for NoC.
Yes, definitely. The question is how should be go about handling them
that would:

- scale for all the IP blocks and nodes
- not be overly complex
- preferably doesn't just rely on trial-and-error and educated guesses
- be hard to mess up on the programmer's side

> 
> Going by the name, you'd expect the UFS controller could own these two clocks
> 
> "clk-gcc-ufs-axi-clk",
> "clk-aggre2-ufs-axi-clk-no-rate"
> 
> but even then by the same logic the NoC should own "clk-aggre2-noc-clk-no-rate" I wouldn't much fancy splitting them apart.
Yeah, it seems to be a common pattern: whichever on-die block is mentioned
first in the name (or first after the clock controller's prefix), actually
"owns" the clock

> 
> So - I'd say the commit log doesn't really explain to me what we have discussed here.
> 
> Suggest rewording it a little bit "non-scaling clock" is accurate but for me on the first read doesn't really tell me that these are node-specific clock dependencies or that there is an expectation that the intf_clks should be tied to node-specific clocks.
> 
> So two asks
> 
> - Update the commit log and potentially the structure comments
I'm probably just very biased because I authored these commits, but I can't
see which part is not clear.. Could I (and this is not passive-aggressive or
anything) ask for a pointer there?

> - Can you split the devm_kzalloc() into a seperate patch ?
>   I don't see why this needs to be done but if it does need to be
>   done it could as far as I read it be done before this patch
bus_clks[] is no longer a trailing empty array (a.k.a "struct hack"),
so we shouldn't be using struct_size anymore. This sort of needs to
be done together, as e.g. 8996 (without this commit) expects >2 bus
clocks which are in reality these "interface clocks", or "node access
dependencies", so splitting that out would introduce unnecessary churn
or break bisecting.

Konrad
> 
> ---
> bod

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Bryan O'Donoghue]

On 11/03/2023 00:54, Konrad Dybcio wrote:
>> - Update the commit log and potentially the structure comments
> I'm probably just very biased because I authored these commits, but I can't
> see which part is not clear.. Could I (and this is not passive-aggressive or
> anything) ask for a pointer there?
> 

I mean to say "non scaling clocks" isn't an indicator IMO of the fact 
that these are QoS node specific clocks.

Right now the interconnect model is predicated on bus and bus_a but, 
you've found that on some SoCs we have node-specific clocks too.

:g/non\ scaling/s//non-scaling\ node-specific/g

would do or "QoS node-specific" the fact the clocks don't scale is 
incidental the dependency though is that IMO at least these are 
additional node-specific clocks we need to enable.

---
bod

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Konrad Dybcio]

On 11.03.2023 13:11, Bryan O'Donoghue wrote:
> On 11/03/2023 00:54, Konrad Dybcio wrote:
>>> - Update the commit log and potentially the structure comments
>> I'm probably just very biased because I authored these commits, but I can't
>> see which part is not clear.. Could I (and this is not passive-aggressive or
>> anything) ask for a pointer there?
>>
> 
> I mean to say "non scaling clocks" isn't an indicator IMO of the fact that these are QoS node specific clocks.
> 
> Right now the interconnect model is predicated on bus and bus_a but, you've found that on some SoCs we have node-specific clocks too.
> 
> :g/non\ scaling/s//non-scaling\ node-specific/g
> 
> would do or "QoS node-specific" the fact the clocks don't scale is incidental the dependency though is that IMO at least these are additional node-specific clocks we need to enable.
Okay that makes sense. Thanks.

Konrad
> 
> ---
> bod

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Dmitry Baryshkov]

On Sat, 11 Mar 2023 at 14:11, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> On 11/03/2023 00:54, Konrad Dybcio wrote:
> >> - Update the commit log and potentially the structure comments
> > I'm probably just very biased because I authored these commits, but I can't
> > see which part is not clear.. Could I (and this is not passive-aggressive or
> > anything) ask for a pointer there?
> >
>
> I mean to say "non scaling clocks" isn't an indicator IMO of the fact
> that these are QoS node specific clocks.
>
> Right now the interconnect model is predicated on bus and bus_a but,
> you've found that on some SoCs we have node-specific clocks too.
>
> :g/non\ scaling/s//non-scaling\ node-specific/g
>
> would do or "QoS node-specific" the fact the clocks don't scale is
> incidental the dependency though is that IMO at least these are
> additional node-specific clocks we need to enable.

This looks somewhat close to what we have observed in the patches for
ipq9574 platform. It doesn't have a scaling interconnect (in other
words, no bus clocks), but some devices have clocks driving the NIU
(Network Interface Units) which connect the device to NoC.

>
> ---
> bod




--
With best wishes
Dmitry

Re: [PATCH v7 1/9] interconnect: qcom: rpm: make QoS INVALID default

[Dmitry Baryshkov]

On 08/03/2023 23:40, Konrad Dybcio wrote:
> Currently NOC_QOS_MODE_FIXED is defined as 0x0 which makes it the
> default option (partial struct initialization). The default option
> however should be NOC_QOS_MODE_INVALID.
> 
> That results in bogus QoS configurations being sent for port 0 (which
> is used for the DRAM endpoint on BIMC, for example) coming from all nodes
> with .qos.ap_owned = true and uninitialized .qos.qos_mode. It's also an
> issue for newer SoCs where all nodes are treated as if they were ap_owned,
> but not all of them have QoS configuration.
> 
> The NOC_QOS_MODEs are defined as preprocessor constants and are not used
> anywhere outside qcom_icc_set_noc_qos(), which is easily worked around.
> Separate the desc->type values from the values sent to msmbus in the
> aforementioned function. Make the former an enum for better mainainability.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v7 2/9] interconnect: qcom: rpm: Add support for specifying channel num

[Dmitry Baryshkov]

On 08/03/2023 23:40, Konrad Dybcio wrote:
> Some nodes, like EBI0 (DDR) or L3/LLCC, may be connected over more than
> one channel. This should be taken into account in bandwidth calcualtion,
> as we're supposed to feed msmbus with the per-channel bandwidth. Add
> support for specifying that and use it during bandwidth aggregation.
> 
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/interconnect/qcom/icc-rpm.c | 7 ++++++-
>   drivers/interconnect/qcom/icc-rpm.h | 2 ++
>   2 files changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v7 3/9] interconnect: qcom: Sort kerneldoc entries

[Dmitry Baryshkov]

On 08/03/2023 23:40, Konrad Dybcio wrote:
> Sort the kerneldoc entries the same way the struct members are
> sorted.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/interconnect/qcom/icc-rpm.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Dmitry Baryshkov]

On 08/03/2023 23:40, Konrad Dybcio wrote:
> Some (but not all) providers (or their specific nodes) require
> specific clocks to be turned on before they can be accessed. Failure
> to ensure that results in a seemingly random system crash (which
> would usually happen at boot with the interconnect driver built-in),
> resulting in the platform not booting up properly.
> 
> Limit the number of bus_clocks to 2 (which is the maximum that SMD
> RPM interconnect supports anyway) and handle non-scaling clocks
> separately. Update MSM8996 and SDM660 drivers to make sure they do
> not regress with this change.
> 
> This unfortunately has to be done in one patch to prevent either
> compile errors or broken bisect.

Can we determine somehow if the intf clocks are required for the whole 
NoC or just for a single node? I don't think that clocks like a0noc_ufs 
are requiered to be up for the whole aggre_noc. Instead they probably 
have to be enabled only when UFS makes use of the NoC (in other words 
when is has voted for the bandwidth).

> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/interconnect/qcom/icc-rpm.c | 52 ++++++++++++++++++++++++++++++-------
>   drivers/interconnect/qcom/icc-rpm.h | 14 ++++++++--
>   drivers/interconnect/qcom/msm8996.c | 22 +++++++---------
>   drivers/interconnect/qcom/sdm660.c  | 16 +++++-------
>   4 files changed, 70 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index b52f788d8f3d..ca932ed720fb 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -369,6 +369,17 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>   
>   	qcom_icc_bus_aggregate(provider, agg_avg, agg_peak, &max_agg_avg);
>   
> +	/* If we're powering on the bus, ensure the necessary clocks are on */
> +	if (unlikely(!qp->is_on)) {
> +		if (agg_peak[0] || agg_peak[1] || max_agg_avg) {
> +			/* If this fails, bus accesses will crash the platform! */
> +			ret = clk_bulk_prepare_enable(qp->num_intf_clks, qp->intf_clks);
> +			if (ret)
> +				return ret;
> +			qp->is_on = true;
> +		}
> +	}
> +
>   	sum_bw = icc_units_to_bps(max_agg_avg);
>   
>   	ret = __qcom_icc_set(src, src_qn, sum_bw);
> @@ -409,6 +420,14 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>   		qp->bus_clk_rate[i] = rate;
>   	}
>   
> +	/* Turn off the interface clocks if the bus was shut down so as not to leak power */
> +	if (!qp->bus_clk_rate[0] && !qp->bus_clk_rate[1]) {
> +		if (!agg_peak[0] && !agg_peak[1] && !max_agg_avg) {
> +			clk_bulk_disable_unprepare(qp->num_intf_clks, qp->intf_clks);
> +			qp->is_on = false;
> +		}
> +	}
> +
>   	return 0;
>   }
>   
> @@ -441,21 +460,20 @@ int qnoc_probe(struct platform_device *pdev)
>   	qnodes = desc->nodes;
>   	num_nodes = desc->num_nodes;
>   
> -	if (desc->num_bus_clocks) {
> -		cds = desc->bus_clocks;
> -		cd_num = desc->num_bus_clocks;
> +	if (desc->num_intf_clocks) {
> +		cds = desc->intf_clocks;
> +		cd_num = desc->num_intf_clocks;
>   	} else {
> -		cds = bus_clocks;
> -		cd_num = ARRAY_SIZE(bus_clocks);
> +		/* 0 intf clocks is perfectly fine */
> +		cd_num = 0;
>   	}
>   
> -	qp = devm_kzalloc(dev, struct_size(qp, bus_clks, cd_num), GFP_KERNEL);
> +	qp = devm_kzalloc(dev, sizeof(*qp), GFP_KERNEL);
>   	if (!qp)
>   		return -ENOMEM;
>   
> -	qp->bus_clk_rate = devm_kcalloc(dev, cd_num, sizeof(*qp->bus_clk_rate),
> -					GFP_KERNEL);
> -	if (!qp->bus_clk_rate)
> +	qp->intf_clks = devm_kzalloc(dev, sizeof(qp->intf_clks), GFP_KERNEL);
> +	if (!qp->intf_clks)
>   		return -ENOMEM;
>   
>   	data = devm_kzalloc(dev, struct_size(data, nodes, num_nodes),
> @@ -463,6 +481,18 @@ int qnoc_probe(struct platform_device *pdev)
>   	if (!data)
>   		return -ENOMEM;
>   
> +	qp->num_intf_clks = cd_num;
> +	for (i = 0; i < cd_num; i++)
> +		qp->intf_clks[i].id = cds[i];
> +
> +	if (desc->num_bus_clocks) {
> +		cds = desc->bus_clocks;
> +		cd_num = desc->num_bus_clocks;
> +	} else {
> +		cds = bus_clocks;
> +		cd_num = ARRAY_SIZE(bus_clocks);
> +	}
> +
>   	for (i = 0; i < cd_num; i++)
>   		qp->bus_clks[i].id = cds[i];
>   	qp->num_bus_clks = cd_num;
> @@ -503,6 +533,10 @@ int qnoc_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>   
> +	ret = devm_clk_bulk_get(dev, qp->num_intf_clks, qp->intf_clks);
> +	if (ret)
> +		return ret;
> +
>   	if (desc->has_bus_pd) {
>   		ret = dev_pm_domain_attach(dev, true);
>   		goto err_disable_clks;
> diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
> index d4401f35f6d2..a4ef45b4a9e0 100644
> --- a/drivers/interconnect/qcom/icc-rpm.h
> +++ b/drivers/interconnect/qcom/icc-rpm.h
> @@ -20,24 +20,32 @@ enum qcom_icc_type {
>   	QCOM_ICC_QNOC,
>   };
>   
> +#define NUM_BUS_CLKS	2
> +
>   /**
>    * struct qcom_icc_provider - Qualcomm specific interconnect provider
>    * @provider: generic interconnect provider
>    * @num_bus_clks: the total number of bus_clks clk_bulk_data entries
> + * @num_intf_clks: the total number of intf_clks clk_bulk_data entries
>    * @type: the ICC provider type
>    * @regmap: regmap for QoS registers read/write access
>    * @qos_offset: offset to QoS registers
>    * @bus_clk_rate: bus clock rate in Hz
>    * @bus_clks: the clk_bulk_data table of bus clocks
> + * @intf_clks: a clk_bulk_data array of interface clocks
> + * @is_on: whether the bus is powered on
>    */
>   struct qcom_icc_provider {
>   	struct icc_provider provider;
>   	int num_bus_clks;
> +	int num_intf_clks;
>   	enum qcom_icc_type type;
>   	struct regmap *regmap;
>   	unsigned int qos_offset;
> -	u64 *bus_clk_rate;
> -	struct clk_bulk_data bus_clks[];
> +	u64 bus_clk_rate[NUM_BUS_CLKS];
> +	struct clk_bulk_data bus_clks[NUM_BUS_CLKS];
> +	struct clk_bulk_data *intf_clks;
> +	bool is_on;
>   };
>   
>   /**
> @@ -93,6 +101,8 @@ struct qcom_icc_desc {
>   	size_t num_nodes;
>   	const char * const *bus_clocks;
>   	size_t num_bus_clocks;
> +	const char * const *intf_clocks;
> +	size_t num_intf_clocks;
>   	bool has_bus_pd;
>   	enum qcom_icc_type type;
>   	const struct regmap_config *regmap_cfg;
> diff --git a/drivers/interconnect/qcom/msm8996.c b/drivers/interconnect/qcom/msm8996.c
> index 69fc50a6fa5c..1a5e0ad36cc4 100644
> --- a/drivers/interconnect/qcom/msm8996.c
> +++ b/drivers/interconnect/qcom/msm8996.c
> @@ -21,21 +21,17 @@
>   #include "smd-rpm.h"
>   #include "msm8996.h"
>   
> -static const char * const bus_mm_clocks[] = {
> -	"bus",
> -	"bus_a",
> +static const char * const mm_intf_clocks[] = {
>   	"iface"
>   };
>   
> -static const char * const bus_a0noc_clocks[] = {
> +static const char * const a0noc_intf_clocks[] = {
>   	"aggre0_snoc_axi",
>   	"aggre0_cnoc_ahb",
>   	"aggre0_noc_mpu_cfg"
>   };
>   
> -static const char * const bus_a2noc_clocks[] = {
> -	"bus",
> -	"bus_a",
> +static const char * const a2noc_intf_clocks[] = {
>   	"aggre2_ufs_axi",
>   	"ufs_axi"
>   };
> @@ -1821,8 +1817,8 @@ static const struct qcom_icc_desc msm8996_a0noc = {
>   	.type = QCOM_ICC_NOC,
>   	.nodes = a0noc_nodes,
>   	.num_nodes = ARRAY_SIZE(a0noc_nodes),
> -	.bus_clocks = bus_a0noc_clocks,
> -	.num_bus_clocks = ARRAY_SIZE(bus_a0noc_clocks),
> +	.intf_clocks = a0noc_intf_clocks,
> +	.num_intf_clocks = ARRAY_SIZE(a0noc_intf_clocks),
>   	.has_bus_pd = true,
>   	.regmap_cfg = &msm8996_a0noc_regmap_config
>   };
> @@ -1866,8 +1862,8 @@ static const struct qcom_icc_desc msm8996_a2noc = {
>   	.type = QCOM_ICC_NOC,
>   	.nodes = a2noc_nodes,
>   	.num_nodes = ARRAY_SIZE(a2noc_nodes),
> -	.bus_clocks = bus_a2noc_clocks,
> -	.num_bus_clocks = ARRAY_SIZE(bus_a2noc_clocks),
> +	.intf_clocks = a2noc_intf_clocks,
> +	.num_intf_clocks = ARRAY_SIZE(a2noc_intf_clocks),
>   	.regmap_cfg = &msm8996_a2noc_regmap_config
>   };
>   
> @@ -2005,8 +2001,8 @@ static const struct qcom_icc_desc msm8996_mnoc = {
>   	.type = QCOM_ICC_NOC,
>   	.nodes = mnoc_nodes,
>   	.num_nodes = ARRAY_SIZE(mnoc_nodes),
> -	.bus_clocks = bus_mm_clocks,
> -	.num_bus_clocks = ARRAY_SIZE(bus_mm_clocks),
> +	.intf_clocks = mm_intf_clocks,
> +	.num_intf_clocks = ARRAY_SIZE(mm_intf_clocks),
>   	.regmap_cfg = &msm8996_mnoc_regmap_config
>   };
>   
> diff --git a/drivers/interconnect/qcom/sdm660.c b/drivers/interconnect/qcom/sdm660.c
> index a22ba821efbf..0e8a96f4ce90 100644
> --- a/drivers/interconnect/qcom/sdm660.c
> +++ b/drivers/interconnect/qcom/sdm660.c
> @@ -127,15 +127,11 @@ enum {
>   	SDM660_SNOC,
>   };
>   
> -static const char * const bus_mm_clocks[] = {
> -	"bus",
> -	"bus_a",
> +static const char * const mm_intf_clocks[] = {
>   	"iface",
>   };
>   
> -static const char * const bus_a2noc_clocks[] = {
> -	"bus",
> -	"bus_a",
> +static const char * const a2noc_intf_clocks[] = {
>   	"ipa",
>   	"ufs_axi",
>   	"aggre2_ufs_axi",
> @@ -1516,8 +1512,8 @@ static const struct qcom_icc_desc sdm660_a2noc = {
>   	.type = QCOM_ICC_NOC,
>   	.nodes = sdm660_a2noc_nodes,
>   	.num_nodes = ARRAY_SIZE(sdm660_a2noc_nodes),
> -	.bus_clocks = bus_a2noc_clocks,
> -	.num_bus_clocks = ARRAY_SIZE(bus_a2noc_clocks),
> +	.intf_clocks = a2noc_intf_clocks,
> +	.num_intf_clocks = ARRAY_SIZE(a2noc_intf_clocks),
>   	.regmap_cfg = &sdm660_a2noc_regmap_config,
>   };
>   
> @@ -1659,8 +1655,8 @@ static const struct qcom_icc_desc sdm660_mnoc = {
>   	.type = QCOM_ICC_NOC,
>   	.nodes = sdm660_mnoc_nodes,
>   	.num_nodes = ARRAY_SIZE(sdm660_mnoc_nodes),
> -	.bus_clocks = bus_mm_clocks,
> -	.num_bus_clocks = ARRAY_SIZE(bus_mm_clocks),
> +	.intf_clocks = mm_intf_clocks,
> +	.num_intf_clocks = ARRAY_SIZE(mm_intf_clocks),
>   	.regmap_cfg = &sdm660_mnoc_regmap_config,
>   };
>   
> 

-- 
With best wishes
Dmitry

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Bryan O'Donoghue]

On 11/03/2023 14:01, Dmitry Baryshkov wrote:
>>
>> Limit the number of bus_clocks to 2 (which is the maximum that SMD
>> RPM interconnect supports anyway) and handle non-scaling clocks
>> separately. Update MSM8996 and SDM660 drivers to make sure they do
>> not regress with this change.
>>
>> This unfortunately has to be done in one patch to prevent either
>> compile errors or broken bisect.
> 
> Can we determine somehow if the intf clocks are required for the whole 
> NoC or just for a single node? I don't think that clocks like a0noc_ufs 
> are requiered to be up for the whole aggre_noc. Instead they probably 
> have to be enabled only when UFS makes use of the NoC (in other words 
> when is has voted for the bandwidth).

Its probably worthwhile experimenting to see if the *ufs*_clk can/should 
be added to the UFS device list of clocks.

I hadn't realised we were talking about enabling the intf clocks always 
but, actually that is what we are talking about isn't it ?

It seems pretty unlikely that other devices would depend on clocks named 
*ufs*

OTOH "clk-aggre2-noc-clk-no-rate" may be used across different nodes, 
seems like a pretty generic name, though still suspicious it is bundled 
with UFS, likely it is only required for UFS ?!?

Konrad can you try:

1. Moving the intf_clks/QoS clocks be contained to UFS alone ?
2. If that doesn't work just the *ufs* clocks be put there with
2a. "clk-aggre2-noc-clk-no-rate" alone added to the intf_clk list

It seems wrong to be enabling ufs related NoC clocks unless we are 
actually switching on UFS.

---
bod

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Dmitry Baryshkov]

On Sat, 11 Mar 2023 at 16:29, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> On 11/03/2023 14:01, Dmitry Baryshkov wrote:
> >>
> >> Limit the number of bus_clocks to 2 (which is the maximum that SMD
> >> RPM interconnect supports anyway) and handle non-scaling clocks
> >> separately. Update MSM8996 and SDM660 drivers to make sure they do
> >> not regress with this change.
> >>
> >> This unfortunately has to be done in one patch to prevent either
> >> compile errors or broken bisect.
> >
> > Can we determine somehow if the intf clocks are required for the whole
> > NoC or just for a single node? I don't think that clocks like a0noc_ufs
> > are requiered to be up for the whole aggre_noc. Instead they probably
> > have to be enabled only when UFS makes use of the NoC (in other words
> > when is has voted for the bandwidth).
>
> Its probably worthwhile experimenting to see if the *ufs*_clk can/should
> be added to the UFS device list of clocks.

While we were doing this for some of the clocks (PCIe and USB, if I'm
not mistaken), I think that generally this is not fully correct. In my
opinion it should be in the interconnect driver, who turns
corresponding clocks on and off. These clocks correspond to the SoC
topology, rather than the end-device.

>
> I hadn't realised we were talking about enabling the intf clocks always
> but, actually that is what we are talking about isn't it ?
>
> It seems pretty unlikely that other devices would depend on clocks named
> *ufs*
>
> OTOH "clk-aggre2-noc-clk-no-rate" may be used across different nodes,
> seems like a pretty generic name, though still suspicious it is bundled
> with UFS, likely it is only required for UFS ?!?
>
> Konrad can you try:
>
> 1. Moving the intf_clks/QoS clocks be contained to UFS alone ?
> 2. If that doesn't work just the *ufs* clocks be put there with
> 2a. "clk-aggre2-noc-clk-no-rate" alone added to the intf_clk list
>
> It seems wrong to be enabling ufs related NoC clocks unless we are
> actually switching on UFS.

-- 
With best wishes
Dmitry

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Bryan O'Donoghue]

On 11/03/2023 14:35, Dmitry Baryshkov wrote:
>> Its probably worthwhile experimenting to see if the*ufs*_clk can/should
>> be added to the UFS device list of clocks.
> While we were doing this for some of the clocks (PCIe and USB, if I'm
> not mistaken), I think that generally this is not fully correct. In my
> opinion it should be in the interconnect driver, who turns
> corresponding clocks on and off. These clocks correspond to the SoC
> topology, rather than the end-device.
> 

True enough, they are interconnect clocks.

The question is how to only turn them on when the device that depends on 
them wants them.

---
bod

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Dmitry Baryshkov]

On 11/03/2023 16:38, Bryan O'Donoghue wrote:
> On 11/03/2023 14:35, Dmitry Baryshkov wrote:
>>> Its probably worthwhile experimenting to see if the*ufs*_clk can/should
>>> be added to the UFS device list of clocks.
>> While we were doing this for some of the clocks (PCIe and USB, if I'm
>> not mistaken), I think that generally this is not fully correct. In my
>> opinion it should be in the interconnect driver, who turns
>> corresponding clocks on and off. These clocks correspond to the SoC
>> topology, rather than the end-device.
>>
> 
> True enough, they are interconnect clocks.
> 
> The question is how to only turn them on when the device that depends on 
> them wants them.

I think we can turn them on an off from qcom_icc_set(). Each node can 
list required clocks.

-- 
With best wishes
Dmitry

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Georgi Djakov]

Hi Konrad,

Thank you for working on this and sorry about jumping a bit late into
the discussion.

On 8.03.23 23:40, Konrad Dybcio wrote:
> Some (but not all) providers (or their specific nodes) require
> specific clocks to be turned on before they can be accessed. Failure
> to ensure that results in a seemingly random system crash (which
> would usually happen at boot with the interconnect driver built-in),
> resulting in the platform not booting up properly.

These "interface" clocks seem to be used only to program QoS for the
respective ip block (eg ufs). So if we don't program QoS, there should
be no crashes, right?

I believe that in downstream they defer setting QoS until the first
non-zero bandwidth request because of drivers that probe asynchronously
or there is some firmware booting involved (IPA maybe). And bad stuff
might happen if we touch the clock while the firmware is still booting.
So setting the QoS on the first non-zero bandwidth request might not be
a bad idea. Such nodes should probably be also excluded from sync_state
by implementing get_bw() to return 0 bandwidth.

BR,
Georgi

> 
> Limit the number of bus_clocks to 2 (which is the maximum that SMD
> RPM interconnect supports anyway) and handle non-scaling clocks
> separately. Update MSM8996 and SDM660 drivers to make sure they do
> not regress with this change.
> 
> This unfortunately has to be done in one patch to prevent either
> compile errors or broken bisect.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/interconnect/qcom/icc-rpm.c | 52 ++++++++++++++++++++++++++++++-------
>   drivers/interconnect/qcom/icc-rpm.h | 14 ++++++++--
>   drivers/interconnect/qcom/msm8996.c | 22 +++++++---------
>   drivers/interconnect/qcom/sdm660.c  | 16 +++++-------
>   4 files changed, 70 insertions(+), 34 deletions(-)
> 
[..]

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Georgi Djakov]

Hi,

On 11.03.23 17:26, Dmitry Baryshkov wrote:
> On 11/03/2023 16:38, Bryan O'Donoghue wrote:
>> On 11/03/2023 14:35, Dmitry Baryshkov wrote:
>>>> Its probably worthwhile experimenting to see if the*ufs*_clk can/should
>>>> be added to the UFS device list of clocks.
>>> While we were doing this for some of the clocks (PCIe and USB, if I'm
>>> not mistaken), I think that generally this is not fully correct. In my
>>> opinion it should be in the interconnect driver, who turns
>>> corresponding clocks on and off. These clocks correspond to the SoC
>>> topology, rather than the end-device.
>>>
>>
>> True enough, they are interconnect clocks.
>>
>> The question is how to only turn them on when the device that depends on them wants them.
> 
> I think we can turn them on an off from qcom_icc_set(). Each node can list required clocks.
> 

Yes, this is a bit weird, but looks like these are the interface clocks
required for programming the qos box of the respective peripheral and
nothing else. Maybe we can even configure QoS just once (eg. on the first
bandwidth request) and not every time we call qcom_icc_set().

BR,
Georgi

Re: [PATCH v7 2/9] interconnect: qcom: rpm: Add support for specifying channel num

[Georgi Djakov]

Hi Konrad,

Thanks for the patch!

On 8.03.23 23:40, Konrad Dybcio wrote:
> Some nodes, like EBI0 (DDR) or L3/LLCC, may be connected over more than
> one channel. This should be taken into account in bandwidth calcualtion,
> as we're supposed to feed msmbus with the per-channel bandwidth. Add
> support for specifying that and use it during bandwidth aggregation.
> 

This looks good, but do you have any follow-up patch to use this and set
the channels in some driver?

BR,
Georgi

> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/interconnect/qcom/icc-rpm.c | 7 ++++++-
>   drivers/interconnect/qcom/icc-rpm.h | 2 ++
>   2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index 35fd75ae70e3..27c4c6497994 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -317,6 +317,7 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider,
>   {
>   	struct icc_node *node;
>   	struct qcom_icc_node *qn;
> +	u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
>   	int i;
>   
>   	/* Initialise aggregate values */
> @@ -334,7 +335,11 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider,
>   	list_for_each_entry(node, &provider->nodes, node_list) {
>   		qn = node->data;
>   		for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
> -			agg_avg[i] += qn->sum_avg[i];
> +			if (qn->channels)
> +				sum_avg[i] = div_u64(qn->sum_avg[i], qn->channels);
> +			else
> +				sum_avg[i] = qn->sum_avg[i];
> +			agg_avg[i] += sum_avg[i];
>   			agg_peak[i] = max_t(u64, agg_peak[i], qn->max_peak[i]);
>   		}
>   	}
> diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
> index 8ba1918d7997..8aed5400afda 100644
> --- a/drivers/interconnect/qcom/icc-rpm.h
> +++ b/drivers/interconnect/qcom/icc-rpm.h
> @@ -66,6 +66,7 @@ struct qcom_icc_qos {
>    * @id: a unique node identifier
>    * @links: an array of nodes where we can go next while traversing
>    * @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)
>    * @sum_avg: current sum aggregate value of all avg bw requests
>    * @max_peak: current max aggregate value of all peak bw requests
> @@ -78,6 +79,7 @@ struct qcom_icc_node {
>   	u16 id;
>   	const u16 *links;
>   	u16 num_links;
> +	u16 channels;
>   	u16 buswidth;
>   	u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
>   	u64 max_peak[QCOM_ICC_NUM_BUCKETS];
> 

Re: [PATCH v7 2/9] interconnect: qcom: rpm: Add support for specifying channel num

[Konrad Dybcio]

On 21.03.2023 15:06, Georgi Djakov wrote:
> Hi Konrad,
> 
> Thanks for the patch!
> 
> On 8.03.23 23:40, Konrad Dybcio wrote:
>> Some nodes, like EBI0 (DDR) or L3/LLCC, may be connected over more than
>> one channel. This should be taken into account in bandwidth calcualtion,
>> as we're supposed to feed msmbus with the per-channel bandwidth. Add
>> support for specifying that and use it during bandwidth aggregation.
>>
> 
> This looks good, but do you have any follow-up patch to use this and set
> the channels in some driver?
Yes, I have a couple of OOT drivers that are gonna make use of it.
TBF it should have been sent separately from the QoS mess, but I
don't think it's much of an issue to take it as-is.

The aforementioned OOT drivers for MSM8998 and SM6375 will be
submitted after we reach a consensus on how we want to ensure
that each node is guaranteed to have its clocks enabled before
access, among some other minor things.

Konrad
> 
> BR,
> Georgi
> 
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/interconnect/qcom/icc-rpm.c | 7 ++++++-
>>   drivers/interconnect/qcom/icc-rpm.h | 2 ++
>>   2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
>> index 35fd75ae70e3..27c4c6497994 100644
>> --- a/drivers/interconnect/qcom/icc-rpm.c
>> +++ b/drivers/interconnect/qcom/icc-rpm.c
>> @@ -317,6 +317,7 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider,
>>   {
>>       struct icc_node *node;
>>       struct qcom_icc_node *qn;
>> +    u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
>>       int i;
>>         /* Initialise aggregate values */
>> @@ -334,7 +335,11 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider,
>>       list_for_each_entry(node, &provider->nodes, node_list) {
>>           qn = node->data;
>>           for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
>> -            agg_avg[i] += qn->sum_avg[i];
>> +            if (qn->channels)
>> +                sum_avg[i] = div_u64(qn->sum_avg[i], qn->channels);
>> +            else
>> +                sum_avg[i] = qn->sum_avg[i];
>> +            agg_avg[i] += sum_avg[i];
>>               agg_peak[i] = max_t(u64, agg_peak[i], qn->max_peak[i]);
>>           }
>>       }
>> diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
>> index 8ba1918d7997..8aed5400afda 100644
>> --- a/drivers/interconnect/qcom/icc-rpm.h
>> +++ b/drivers/interconnect/qcom/icc-rpm.h
>> @@ -66,6 +66,7 @@ struct qcom_icc_qos {
>>    * @id: a unique node identifier
>>    * @links: an array of nodes where we can go next while traversing
>>    * @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)
>>    * @sum_avg: current sum aggregate value of all avg bw requests
>>    * @max_peak: current max aggregate value of all peak bw requests
>> @@ -78,6 +79,7 @@ struct qcom_icc_node {
>>       u16 id;
>>       const u16 *links;
>>       u16 num_links;
>> +    u16 channels;
>>       u16 buswidth;
>>       u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
>>       u64 max_peak[QCOM_ICC_NUM_BUCKETS];
>>
> 

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Konrad Dybcio]

On 21.03.2023 14:58, Georgi Djakov wrote:
> Hi,
> 
> On 11.03.23 17:26, Dmitry Baryshkov wrote:
>> On 11/03/2023 16:38, Bryan O'Donoghue wrote:
>>> On 11/03/2023 14:35, Dmitry Baryshkov wrote:
>>>>> Its probably worthwhile experimenting to see if the*ufs*_clk can/should
>>>>> be added to the UFS device list of clocks.
>>>> While we were doing this for some of the clocks (PCIe and USB, if I'm
>>>> not mistaken), I think that generally this is not fully correct. In my
>>>> opinion it should be in the interconnect driver, who turns
>>>> corresponding clocks on and off. These clocks correspond to the SoC
>>>> topology, rather than the end-device.
>>>>
>>>
>>> True enough, they are interconnect clocks.
>>>
>>> The question is how to only turn them on when the device that depends on them wants them.
>>
>> I think we can turn them on an off from qcom_icc_set(). Each node can list required clocks.
>>
> 
> Yes, this is a bit weird, but looks like these are the interface clocks
> required for programming the qos box of the respective peripheral and
> nothing else. Maybe we can even configure QoS just once (eg. on the first
> bandwidth request) and not every time we call qcom_icc_set().
Would that persist a full bus reset - if we e.g. shut down MMNoC 
after the display stack is turned off in preparation for a power
collapse, would we have to reprogram it?

Another thing is, do we know "how persistent" the QoS settings are?
What could reset them? Would a bandwidth request for a node that
belongs to the same path do so?

Konrad
> 
> BR,
> Georgi

Re: [PATCH v7 2/9] interconnect: qcom: rpm: Add support for specifying channel num

[Georgi Djakov]

On 21.03.23 16:09, Konrad Dybcio wrote:
> 
> On 21.03.2023 15:06, Georgi Djakov wrote:
>> Hi Konrad,
>>
>> Thanks for the patch!
>>
>> On 8.03.23 23:40, Konrad Dybcio wrote:
>>> Some nodes, like EBI0 (DDR) or L3/LLCC, may be connected over more than
>>> one channel. This should be taken into account in bandwidth calcualtion,
>>> as we're supposed to feed msmbus with the per-channel bandwidth. Add
>>> support for specifying that and use it during bandwidth aggregation.
>>>
>>
>> This looks good, but do you have any follow-up patch to use this and set
>> the channels in some driver?
> Yes, I have a couple of OOT drivers that are gonna make use of it.
> TBF it should have been sent separately from the QoS mess, but I
> don't think it's much of an issue to take it as-is.
> 
> The aforementioned OOT drivers for MSM8998 and SM6375 will be
> submitted after we reach a consensus on how we want to ensure
> that each node is guaranteed to have its clocks enabled before
> access, among some other minor things.

Yes, these QoS clocks are confusing. Maybe you can even submit them
without configuring any QoS stuff in first place? Does enabling QoS
actually show any benefits on these devices?

Thanks,
Georgi

> Konrad
>>
>> BR,
>> Georgi
>>
>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>    drivers/interconnect/qcom/icc-rpm.c | 7 ++++++-
>>>    drivers/interconnect/qcom/icc-rpm.h | 2 ++
>>>    2 files changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
>>> index 35fd75ae70e3..27c4c6497994 100644
>>> --- a/drivers/interconnect/qcom/icc-rpm.c
>>> +++ b/drivers/interconnect/qcom/icc-rpm.c
>>> @@ -317,6 +317,7 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider,
>>>    {
>>>        struct icc_node *node;
>>>        struct qcom_icc_node *qn;
>>> +    u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
>>>        int i;
>>>          /* Initialise aggregate values */
>>> @@ -334,7 +335,11 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider,
>>>        list_for_each_entry(node, &provider->nodes, node_list) {
>>>            qn = node->data;
>>>            for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
>>> -            agg_avg[i] += qn->sum_avg[i];
>>> +            if (qn->channels)
>>> +                sum_avg[i] = div_u64(qn->sum_avg[i], qn->channels);
>>> +            else
>>> +                sum_avg[i] = qn->sum_avg[i];
>>> +            agg_avg[i] += sum_avg[i];
>>>                agg_peak[i] = max_t(u64, agg_peak[i], qn->max_peak[i]);
>>>            }
>>>        }
>>> diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
>>> index 8ba1918d7997..8aed5400afda 100644
>>> --- a/drivers/interconnect/qcom/icc-rpm.h
>>> +++ b/drivers/interconnect/qcom/icc-rpm.h
>>> @@ -66,6 +66,7 @@ struct qcom_icc_qos {
>>>     * @id: a unique node identifier
>>>     * @links: an array of nodes where we can go next while traversing
>>>     * @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)
>>>     * @sum_avg: current sum aggregate value of all avg bw requests
>>>     * @max_peak: current max aggregate value of all peak bw requests
>>> @@ -78,6 +79,7 @@ struct qcom_icc_node {
>>>        u16 id;
>>>        const u16 *links;
>>>        u16 num_links;
>>> +    u16 channels;
>>>        u16 buswidth;
>>>        u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
>>>        u64 max_peak[QCOM_ICC_NUM_BUCKETS];
>>>
>>

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Konrad Dybcio]

On 21.03.2023 14:56, Georgi Djakov wrote:
> Hi Konrad,
> 
> Thank you for working on this and sorry about jumping a bit late into
> the discussion.
> 
> On 8.03.23 23:40, Konrad Dybcio wrote:
>> Some (but not all) providers (or their specific nodes) require
>> specific clocks to be turned on before they can be accessed. Failure
>> to ensure that results in a seemingly random system crash (which
>> would usually happen at boot with the interconnect driver built-in),
>> resulting in the platform not booting up properly.
> 
> These "interface" clocks seem to be used only to program QoS for the
> respective ip block (eg ufs). So if we don't program QoS, there should
> be no crashes, right?
Correct.

> 
> I believe that in downstream they defer setting QoS until the first
> non-zero bandwidth request because of drivers that probe asynchronously
> or there is some firmware booting involved (IPA maybe). 
Hmm.. that would make sense.

> And bad stuff
> might happen if we touch the clock while the firmware is still booting.
> So setting the QoS on the first non-zero bandwidth request might not be
> a bad idea.
Sounds like a plan, I don't think setting QoS parameters on buses
that are not in use does anything (citation needed).

Such nodes should probably be also excluded from sync_state
> by implementing get_bw() to return 0 bandwidth.
I agree, setting the QoS parameters in sync_state (so, without
their dependencies fully met) sounds like a recipe for disaster,
while getting rid of QoS in sync_state at all would leave the nodes
that are not explicitly referenced in the device tree unconfigured.


Konrad

> 
> BR,
> Georgi
> 
>>
>> Limit the number of bus_clocks to 2 (which is the maximum that SMD
>> RPM interconnect supports anyway) and handle non-scaling clocks
>> separately. Update MSM8996 and SDM660 drivers to make sure they do
>> not regress with this change.
>>
>> This unfortunately has to be done in one patch to prevent either
>> compile errors or broken bisect.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/interconnect/qcom/icc-rpm.c | 52 ++++++++++++++++++++++++++++++-------
>>   drivers/interconnect/qcom/icc-rpm.h | 14 ++++++++--
>>   drivers/interconnect/qcom/msm8996.c | 22 +++++++---------
>>   drivers/interconnect/qcom/sdm660.c  | 16 +++++-------
>>   4 files changed, 70 insertions(+), 34 deletions(-)
>>
> [..]
> 

Re: [PATCH v7 2/9] interconnect: qcom: rpm: Add support for specifying channel num

[Konrad Dybcio]

On 21.03.2023 15:21, Georgi Djakov wrote:
> On 21.03.23 16:09, Konrad Dybcio wrote:
>>
>> On 21.03.2023 15:06, Georgi Djakov wrote:
>>> Hi Konrad,
>>>
>>> Thanks for the patch!
>>>
>>> On 8.03.23 23:40, Konrad Dybcio wrote:
>>>> Some nodes, like EBI0 (DDR) or L3/LLCC, may be connected over more than
>>>> one channel. This should be taken into account in bandwidth calcualtion,
>>>> as we're supposed to feed msmbus with the per-channel bandwidth. Add
>>>> support for specifying that and use it during bandwidth aggregation.
>>>>
>>>
>>> This looks good, but do you have any follow-up patch to use this and set
>>> the channels in some driver?
>> Yes, I have a couple of OOT drivers that are gonna make use of it.
>> TBF it should have been sent separately from the QoS mess, but I
>> don't think it's much of an issue to take it as-is.
>>
>> The aforementioned OOT drivers for MSM8998 and SM6375 will be
>> submitted after we reach a consensus on how we want to ensure
>> that each node is guaranteed to have its clocks enabled before
>> access, among some other minor things.
> 
> Yes, these QoS clocks are confusing. Maybe you can even submit them
> without configuring any QoS stuff in first place? Does enabling QoS
> actually show any benefits on these devices?
Haven't tested that thoroughly to be honest. But I'll try to get
some numbers.

Konrad
> 
> Thanks,
> Georgi
> 
>> Konrad
>>>
>>> BR,
>>> Georgi
>>>
>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>    drivers/interconnect/qcom/icc-rpm.c | 7 ++++++-
>>>>    drivers/interconnect/qcom/icc-rpm.h | 2 ++
>>>>    2 files changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
>>>> index 35fd75ae70e3..27c4c6497994 100644
>>>> --- a/drivers/interconnect/qcom/icc-rpm.c
>>>> +++ b/drivers/interconnect/qcom/icc-rpm.c
>>>> @@ -317,6 +317,7 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider,
>>>>    {
>>>>        struct icc_node *node;
>>>>        struct qcom_icc_node *qn;
>>>> +    u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
>>>>        int i;
>>>>          /* Initialise aggregate values */
>>>> @@ -334,7 +335,11 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider,
>>>>        list_for_each_entry(node, &provider->nodes, node_list) {
>>>>            qn = node->data;
>>>>            for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
>>>> -            agg_avg[i] += qn->sum_avg[i];
>>>> +            if (qn->channels)
>>>> +                sum_avg[i] = div_u64(qn->sum_avg[i], qn->channels);
>>>> +            else
>>>> +                sum_avg[i] = qn->sum_avg[i];
>>>> +            agg_avg[i] += sum_avg[i];
>>>>                agg_peak[i] = max_t(u64, agg_peak[i], qn->max_peak[i]);
>>>>            }
>>>>        }
>>>> diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
>>>> index 8ba1918d7997..8aed5400afda 100644
>>>> --- a/drivers/interconnect/qcom/icc-rpm.h
>>>> +++ b/drivers/interconnect/qcom/icc-rpm.h
>>>> @@ -66,6 +66,7 @@ struct qcom_icc_qos {
>>>>     * @id: a unique node identifier
>>>>     * @links: an array of nodes where we can go next while traversing
>>>>     * @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)
>>>>     * @sum_avg: current sum aggregate value of all avg bw requests
>>>>     * @max_peak: current max aggregate value of all peak bw requests
>>>> @@ -78,6 +79,7 @@ struct qcom_icc_node {
>>>>        u16 id;
>>>>        const u16 *links;
>>>>        u16 num_links;
>>>> +    u16 channels;
>>>>        u16 buswidth;
>>>>        u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
>>>>        u64 max_peak[QCOM_ICC_NUM_BUCKETS];
>>>>
>>>
> 

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Georgi Djakov]

On 21.03.23 16:11, Konrad Dybcio wrote:
> 
> 
> On 21.03.2023 14:58, Georgi Djakov wrote:
>> Hi,
>>
>> On 11.03.23 17:26, Dmitry Baryshkov wrote:
>>> On 11/03/2023 16:38, Bryan O'Donoghue wrote:
>>>> On 11/03/2023 14:35, Dmitry Baryshkov wrote:
>>>>>> Its probably worthwhile experimenting to see if the*ufs*_clk can/should
>>>>>> be added to the UFS device list of clocks.
>>>>> While we were doing this for some of the clocks (PCIe and USB, if I'm
>>>>> not mistaken), I think that generally this is not fully correct. In my
>>>>> opinion it should be in the interconnect driver, who turns
>>>>> corresponding clocks on and off. These clocks correspond to the SoC
>>>>> topology, rather than the end-device.
>>>>>
>>>>
>>>> True enough, they are interconnect clocks.
>>>>
>>>> The question is how to only turn them on when the device that depends on them wants them.
>>>
>>> I think we can turn them on an off from qcom_icc_set(). Each node can list required clocks.
>>>
>>
>> Yes, this is a bit weird, but looks like these are the interface clocks
>> required for programming the qos box of the respective peripheral and
>> nothing else. Maybe we can even configure QoS just once (eg. on the first
>> bandwidth request) and not every time we call qcom_icc_set().
> Would that persist a full bus reset - if we e.g. shut down MMNoC
> after the display stack is turned off in preparation for a power
> collapse, would we have to reprogram it?
> 
> Another thing is, do we know "how persistent" the QoS settings are?
> What could reset them? Would a bandwidth request for a node that
> belongs to the same path do so?

That's a good question. From what i recall, i expect them to persist until
you reset the board. Probably we can verify it with an experiment by reading
them back, but let me check if i can find some info.

Thanks,
Georgi

Re: [PATCH v7 2/9] interconnect: qcom: rpm: Add support for specifying channel num

[Georgi Djakov]

On 21.03.23 16:23, Konrad Dybcio wrote:
> 
> 
> On 21.03.2023 15:21, Georgi Djakov wrote:
>> On 21.03.23 16:09, Konrad Dybcio wrote:
>>>
>>> On 21.03.2023 15:06, Georgi Djakov wrote:
>>>> Hi Konrad,
>>>>
>>>> Thanks for the patch!
>>>>
>>>> On 8.03.23 23:40, Konrad Dybcio wrote:
>>>>> Some nodes, like EBI0 (DDR) or L3/LLCC, may be connected over more than
>>>>> one channel. This should be taken into account in bandwidth calcualtion,
>>>>> as we're supposed to feed msmbus with the per-channel bandwidth. Add
>>>>> support for specifying that and use it during bandwidth aggregation.
>>>>>
>>>>
>>>> This looks good, but do you have any follow-up patch to use this and set
>>>> the channels in some driver?
>>> Yes, I have a couple of OOT drivers that are gonna make use of it.
>>> TBF it should have been sent separately from the QoS mess, but I
>>> don't think it's much of an issue to take it as-is.
>>>
>>> The aforementioned OOT drivers for MSM8998 and SM6375 will be
>>> submitted after we reach a consensus on how we want to ensure
>>> that each node is guaranteed to have its clocks enabled before
>>> access, among some other minor things.
>>
>> Yes, these QoS clocks are confusing. Maybe you can even submit them
>> without configuring any QoS stuff in first place? Does enabling QoS
>> actually show any benefits on these devices?
> Haven't tested that thoroughly to be honest. But I'll try to get
> some numbers.

I expect this to have impact only on some latency sensitive stuff like
modem or when there is heavy traffic flows. Maybe we can start without
QoS first and then add it on top as a next step?

BR,
Georgi

> 
> Konrad
>>
>> Thanks,
>> Georgi
>>
>>> Konrad
>>>>
>>>> BR,
>>>> Georgi
>>>>
>>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>> ---
>>>>>     drivers/interconnect/qcom/icc-rpm.c | 7 ++++++-
>>>>>     drivers/interconnect/qcom/icc-rpm.h | 2 ++
>>>>>     2 files changed, 8 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
>>>>> index 35fd75ae70e3..27c4c6497994 100644
>>>>> --- a/drivers/interconnect/qcom/icc-rpm.c
>>>>> +++ b/drivers/interconnect/qcom/icc-rpm.c
>>>>> @@ -317,6 +317,7 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider,
>>>>>     {
>>>>>         struct icc_node *node;
>>>>>         struct qcom_icc_node *qn;
>>>>> +    u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
>>>>>         int i;
>>>>>           /* Initialise aggregate values */
>>>>> @@ -334,7 +335,11 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider,
>>>>>         list_for_each_entry(node, &provider->nodes, node_list) {
>>>>>             qn = node->data;
>>>>>             for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
>>>>> -            agg_avg[i] += qn->sum_avg[i];
>>>>> +            if (qn->channels)
>>>>> +                sum_avg[i] = div_u64(qn->sum_avg[i], qn->channels);
>>>>> +            else
>>>>> +                sum_avg[i] = qn->sum_avg[i];
>>>>> +            agg_avg[i] += sum_avg[i];
>>>>>                 agg_peak[i] = max_t(u64, agg_peak[i], qn->max_peak[i]);
>>>>>             }
>>>>>         }
>>>>> diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
>>>>> index 8ba1918d7997..8aed5400afda 100644
>>>>> --- a/drivers/interconnect/qcom/icc-rpm.h
>>>>> +++ b/drivers/interconnect/qcom/icc-rpm.h
>>>>> @@ -66,6 +66,7 @@ struct qcom_icc_qos {
>>>>>      * @id: a unique node identifier
>>>>>      * @links: an array of nodes where we can go next while traversing
>>>>>      * @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)
>>>>>      * @sum_avg: current sum aggregate value of all avg bw requests
>>>>>      * @max_peak: current max aggregate value of all peak bw requests
>>>>> @@ -78,6 +79,7 @@ struct qcom_icc_node {
>>>>>         u16 id;
>>>>>         const u16 *links;
>>>>>         u16 num_links;
>>>>> +    u16 channels;
>>>>>         u16 buswidth;
>>>>>         u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
>>>>>         u64 max_peak[QCOM_ICC_NUM_BUCKETS];
>>>>>
>>>>
>>

Re: [PATCH v7 2/9] interconnect: qcom: rpm: Add support for specifying channel num

[Konrad Dybcio]

On 21.03.2023 15:49, Georgi Djakov wrote:
> On 21.03.23 16:23, Konrad Dybcio wrote:
>>
>>
>> On 21.03.2023 15:21, Georgi Djakov wrote:
>>> On 21.03.23 16:09, Konrad Dybcio wrote:
>>>>
>>>> On 21.03.2023 15:06, Georgi Djakov wrote:
>>>>> Hi Konrad,
>>>>>
>>>>> Thanks for the patch!
>>>>>
>>>>> On 8.03.23 23:40, Konrad Dybcio wrote:
>>>>>> Some nodes, like EBI0 (DDR) or L3/LLCC, may be connected over more than
>>>>>> one channel. This should be taken into account in bandwidth calcualtion,
>>>>>> as we're supposed to feed msmbus with the per-channel bandwidth. Add
>>>>>> support for specifying that and use it during bandwidth aggregation.
>>>>>>
>>>>>
>>>>> This looks good, but do you have any follow-up patch to use this and set
>>>>> the channels in some driver?
>>>> Yes, I have a couple of OOT drivers that are gonna make use of it.
>>>> TBF it should have been sent separately from the QoS mess, but I
>>>> don't think it's much of an issue to take it as-is.
>>>>
>>>> The aforementioned OOT drivers for MSM8998 and SM6375 will be
>>>> submitted after we reach a consensus on how we want to ensure
>>>> that each node is guaranteed to have its clocks enabled before
>>>> access, among some other minor things.
>>>
>>> Yes, these QoS clocks are confusing. Maybe you can even submit them
>>> without configuring any QoS stuff in first place? Does enabling QoS
>>> actually show any benefits on these devices?
>> Haven't tested that thoroughly to be honest. But I'll try to get
>> some numbers.
> 
> I expect this to have impact only on some latency sensitive stuff like
> modem or when there is heavy traffic flows. Maybe we can start without
> QoS first and then add it on top as a next step?
I only now remembered why I didn't do that.. Adding QoS at a later time
will break older DTs, as with QoS we need to pass some clocks to the driver.

Konrad
> 
> BR,
> Georgi
> 
>>
>> Konrad
>>>
>>> Thanks,
>>> Georgi
>>>
>>>> Konrad
>>>>>
>>>>> BR,
>>>>> Georgi
>>>>>
>>>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>> ---
>>>>>>     drivers/interconnect/qcom/icc-rpm.c | 7 ++++++-
>>>>>>     drivers/interconnect/qcom/icc-rpm.h | 2 ++
>>>>>>     2 files changed, 8 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
>>>>>> index 35fd75ae70e3..27c4c6497994 100644
>>>>>> --- a/drivers/interconnect/qcom/icc-rpm.c
>>>>>> +++ b/drivers/interconnect/qcom/icc-rpm.c
>>>>>> @@ -317,6 +317,7 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider,
>>>>>>     {
>>>>>>         struct icc_node *node;
>>>>>>         struct qcom_icc_node *qn;
>>>>>> +    u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
>>>>>>         int i;
>>>>>>           /* Initialise aggregate values */
>>>>>> @@ -334,7 +335,11 @@ static void qcom_icc_bus_aggregate(struct icc_provider *provider,
>>>>>>         list_for_each_entry(node, &provider->nodes, node_list) {
>>>>>>             qn = node->data;
>>>>>>             for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
>>>>>> -            agg_avg[i] += qn->sum_avg[i];
>>>>>> +            if (qn->channels)
>>>>>> +                sum_avg[i] = div_u64(qn->sum_avg[i], qn->channels);
>>>>>> +            else
>>>>>> +                sum_avg[i] = qn->sum_avg[i];
>>>>>> +            agg_avg[i] += sum_avg[i];
>>>>>>                 agg_peak[i] = max_t(u64, agg_peak[i], qn->max_peak[i]);
>>>>>>             }
>>>>>>         }
>>>>>> diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
>>>>>> index 8ba1918d7997..8aed5400afda 100644
>>>>>> --- a/drivers/interconnect/qcom/icc-rpm.h
>>>>>> +++ b/drivers/interconnect/qcom/icc-rpm.h
>>>>>> @@ -66,6 +66,7 @@ struct qcom_icc_qos {
>>>>>>      * @id: a unique node identifier
>>>>>>      * @links: an array of nodes where we can go next while traversing
>>>>>>      * @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)
>>>>>>      * @sum_avg: current sum aggregate value of all avg bw requests
>>>>>>      * @max_peak: current max aggregate value of all peak bw requests
>>>>>> @@ -78,6 +79,7 @@ struct qcom_icc_node {
>>>>>>         u16 id;
>>>>>>         const u16 *links;
>>>>>>         u16 num_links;
>>>>>> +    u16 channels;
>>>>>>         u16 buswidth;
>>>>>>         u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
>>>>>>         u64 max_peak[QCOM_ICC_NUM_BUCKETS];
>>>>>>
>>>>>
>>>
> 

Re: [PATCH v7 6/9] interconnect: qcom: rpm: Handle interface clocks

[Georgi Djakov]

On 21.03.23 16:38, Georgi Djakov wrote:
> On 21.03.23 16:11, Konrad Dybcio wrote:
>>
>>
>> On 21.03.2023 14:58, Georgi Djakov wrote:
>>> Hi,
>>>
>>> On 11.03.23 17:26, Dmitry Baryshkov wrote:
>>>> On 11/03/2023 16:38, Bryan O'Donoghue wrote:
>>>>> On 11/03/2023 14:35, Dmitry Baryshkov wrote:
>>>>>>> Its probably worthwhile experimenting to see if the*ufs*_clk can/should
>>>>>>> be added to the UFS device list of clocks.
>>>>>> While we were doing this for some of the clocks (PCIe and USB, if I'm
>>>>>> not mistaken), I think that generally this is not fully correct. In my
>>>>>> opinion it should be in the interconnect driver, who turns
>>>>>> corresponding clocks on and off. These clocks correspond to the SoC
>>>>>> topology, rather than the end-device.
>>>>>>
>>>>>
>>>>> True enough, they are interconnect clocks.
>>>>>
>>>>> The question is how to only turn them on when the device that depends on them wants them.
>>>>
>>>> I think we can turn them on an off from qcom_icc_set(). Each node can list required clocks.
>>>>
>>>
>>> Yes, this is a bit weird, but looks like these are the interface clocks
>>> required for programming the qos box of the respective peripheral and
>>> nothing else. Maybe we can even configure QoS just once (eg. on the first
>>> bandwidth request) and not every time we call qcom_icc_set().
>> Would that persist a full bus reset - if we e.g. shut down MMNoC
>> after the display stack is turned off in preparation for a power
>> collapse, would we have to reprogram it?
>>
>> Another thing is, do we know "how persistent" the QoS settings are?
>> What could reset them? Would a bandwidth request for a node that
>> belongs to the same path do so?
> 
> That's a good question. From what i recall, i expect them to persist until
> you reset the board. Probably we can verify it with an experiment by reading
> them back, but let me check if i can find some info.
> 

This seems to be hardware specific and there is no general answer. It depends
on where the reset line for the NIU comes from. It could be from the primary
chip reset in most cases, but it could be also within the power domain of the
associated core.

Thanks,
Georgi