Hi!
v7 -> v8:
- Rebase (dropping has_bus_pd, picked patches from v7)
- Clean up the QoS-setting functions [3/8]
- Only set the QoS registers once [4/8] - Georgi talked to some
Qualcomm folks and we concluded that it's "good enough" as they
should:tm: persist until a full reboot on "almost all" platforms
- Move the intf clock enabling/disabling to correspond with /\ [5/8]
- DO NOT switch to associating an interface clock with a given node
(instead of a provider), as it makes little sense with the changes
with [4/8] and the new iteration of [5/8]
v7: https://lore.kernel.org/r/[email protected]
v6 -> v7 changelog:
- Rebase on Johan's recent patches
Link to v6: https://lore.kernel.org/r/[email protected]
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/[email protected]/
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/[email protected]/
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/[email protected]/
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/[email protected]/T/#m056692bea71d4c272968d5e07afbd9eb07a88123
[2] https://lore.kernel.org/linux-arm-msm/[email protected]/
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 <[email protected]>
---
Konrad Dybcio (8):
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: Drop unused parameters
interconnect: qcom: rpm: Set QoS registers only once
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 | 110 +++++++++++++++++++-----------------
drivers/interconnect/qcom/icc-rpm.h | 22 ++++++--
drivers/interconnect/qcom/msm8996.c | 35 +++++++-----
drivers/interconnect/qcom/sdm660.c | 17 +++---
4 files changed, 102 insertions(+), 82 deletions(-)
---
base-commit: e134c93f788fb93fd6a3ec3af9af850a2048c7e6
change-id: 20230228-topic-qos-5435cac88d89
Best regards,
--
Konrad Dybcio <[email protected]>
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 <[email protected]>
---
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 5341fa169dbf..ee3d09a6850e 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -440,9 +440,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 22bdb1e4bb12..689300a2fd4e 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;
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 14efd2761b7a..21f998cd19f0 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),
.regmap_cfg = &msm8996_a0noc_regmap_config
};
@@ -1865,8 +1865,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
};
@@ -2004,8 +2004,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.40.0
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 <[email protected]>
---
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 ee3d09a6850e..8dce3dcff8da 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -379,7 +379,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
@@ -464,7 +464,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;
@@ -494,11 +494,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;
@@ -551,7 +551,7 @@ int qnoc_probe(struct platform_device *pdev)
icc_provider_deregister(provider);
err_remove_nodes:
icc_nodes_remove(provider);
- clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
+ clk_bulk_disable_unprepare(qp->num_bus_clks, qp->bus_clks);
return ret;
}
@@ -563,7 +563,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 689300a2fd4e..2436777820a4 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.40.0
The QoS registers are (or according to Qualcomm folks, on most
platforms) persistent until a full chip reboot. Move the QoS-setting
functions to the probe function so that we don't needlessly do it over
and over again.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/interconnect/qcom/icc-rpm.c | 50 ++++++++++++++++---------------------
1 file changed, 21 insertions(+), 29 deletions(-)
diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index 7d62c0bf2722..d79e508cb717 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -204,30 +204,33 @@ static int qcom_icc_qos_set(struct icc_node *node)
}
}
-static int qcom_icc_rpm_set(int mas_rpm_id, int slv_rpm_id, u64 sum_bw)
+static int qcom_icc_rpm_set(struct qcom_icc_node *qn, u64 sum_bw)
{
int ret = 0;
- if (mas_rpm_id != -1) {
+ if (qn->qos.ap_owned)
+ return 0;
+
+ if (qn->mas_rpm_id != -1) {
ret = qcom_icc_rpm_smd_send(QCOM_SMD_RPM_ACTIVE_STATE,
RPM_BUS_MASTER_REQ,
- mas_rpm_id,
+ qn->mas_rpm_id,
sum_bw);
if (ret) {
pr_err("qcom_icc_rpm_smd_send mas %d error %d\n",
- mas_rpm_id, ret);
+ qn->mas_rpm_id, ret);
return ret;
}
}
- if (slv_rpm_id != -1) {
+ if (qn->slv_rpm_id != -1) {
ret = qcom_icc_rpm_smd_send(QCOM_SMD_RPM_ACTIVE_STATE,
RPM_BUS_SLAVE_REQ,
- slv_rpm_id,
+ qn->slv_rpm_id,
sum_bw);
if (ret) {
pr_err("qcom_icc_rpm_smd_send slv %d error %d\n",
- slv_rpm_id, ret);
+ qn->slv_rpm_id, ret);
return ret;
}
}
@@ -235,26 +238,6 @@ static int qcom_icc_rpm_set(int mas_rpm_id, int slv_rpm_id, u64 sum_bw)
return ret;
}
-static int __qcom_icc_set(struct icc_node *n, struct qcom_icc_node *qn,
- u64 sum_bw)
-{
- int ret;
-
- if (!qn->qos.ap_owned) {
- /* send bandwidth request message to the RPM processor */
- ret = qcom_icc_rpm_set(qn->mas_rpm_id, qn->slv_rpm_id, sum_bw);
- if (ret)
- return ret;
- } else if (qn->qos.qos_mode != NOC_QOS_MODE_INVALID) {
- /* set bandwidth directly from the AP */
- ret = qcom_icc_qos_set(n, sum_bw);
- if (ret)
- return ret;
- }
-
- return 0;
-}
-
/**
* qcom_icc_pre_bw_aggregate - cleans up values before re-aggregate requests
* @node: icc node to operate on
@@ -370,11 +353,12 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
sum_bw = icc_units_to_bps(max_agg_avg);
- ret = __qcom_icc_set(src, src_qn, sum_bw);
+ ret = qcom_icc_rpm_set(src_qn, sum_bw);
if (ret)
return ret;
+
if (dst_qn) {
- ret = __qcom_icc_set(dst, dst_qn, sum_bw);
+ ret = qcom_icc_rpm_set(dst_qn, sum_bw);
if (ret)
return ret;
}
@@ -528,6 +512,14 @@ int qnoc_probe(struct platform_device *pdev)
for (j = 0; j < qnodes[i]->num_links; j++)
icc_link_create(node, qnodes[i]->links[j]);
+ /* Set QoS registers (we only need to do it once, generally) */
+ if (qnodes[i]->qos.ap_owned &&
+ qnodes[i]->qos.qos_mode != NOC_QOS_MODE_INVALID) {
+ ret = qcom_icc_qos_set(node);
+ if (ret)
+ return ret;
+ }
+
data->nodes[i] = node;
}
data->num_nodes = num_nodes;
--
2.40.0
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 <[email protected]>
---
drivers/interconnect/qcom/icc-rpm.c | 40 ++++++++++++++++++++++++++++---------
drivers/interconnect/qcom/icc-rpm.h | 14 +++++++++++--
drivers/interconnect/qcom/msm8996.c | 22 +++++++++-----------
drivers/interconnect/qcom/sdm660.c | 16 ++++++---------
4 files changed, 58 insertions(+), 34 deletions(-)
diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index d79e508cb717..419b2122bebd 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -424,21 +424,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),
@@ -446,6 +445,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;
@@ -486,6 +497,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;
+
provider = &qp->provider;
provider->dev = dev;
provider->set = qcom_icc_set;
@@ -496,6 +511,11 @@ int qnoc_probe(struct platform_device *pdev)
icc_provider_init(provider);
+ /* 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;
+
for (i = 0; i < num_nodes; i++) {
size_t j;
@@ -524,6 +544,8 @@ int qnoc_probe(struct platform_device *pdev)
}
data->num_nodes = num_nodes;
+ clk_bulk_disable_unprepare(qp->num_intf_clks, qp->intf_clks);
+
ret = icc_provider_register(provider);
if (ret)
goto err_remove_nodes;
diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
index 2436777820a4..b9b63860042f 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;
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 21f998cd19f0..9aedfc8de4bf 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),
.regmap_cfg = &msm8996_a0noc_regmap_config
};
@@ -1865,8 +1861,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
};
@@ -2004,8 +2000,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.40.0
The QoS-setting functions do not care about the bandwidth values passed.
Drop them.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/interconnect/qcom/icc-rpm.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
index 8dce3dcff8da..7d62c0bf2722 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -50,7 +50,7 @@
#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)
+static int qcom_icc_set_qnoc_qos(struct icc_node *src)
{
struct icc_provider *provider = src->provider;
struct qcom_icc_provider *qp = to_qcom_provider(provider);
@@ -95,7 +95,7 @@ static int qcom_icc_bimc_set_qos_health(struct qcom_icc_provider *qp,
mask, val);
}
-static int qcom_icc_set_bimc_qos(struct icc_node *src, u64 max_bw)
+static int qcom_icc_set_bimc_qos(struct icc_node *src)
{
struct qcom_icc_provider *qp;
struct qcom_icc_node *qn;
@@ -150,7 +150,7 @@ static int qcom_icc_noc_set_qos_priority(struct qcom_icc_provider *qp,
NOC_QOS_PRIORITY_P0_MASK, qos->prio_level);
}
-static int qcom_icc_set_noc_qos(struct icc_node *src, u64 max_bw)
+static int qcom_icc_set_noc_qos(struct icc_node *src)
{
struct qcom_icc_provider *qp;
struct qcom_icc_node *qn;
@@ -187,7 +187,7 @@ static int qcom_icc_set_noc_qos(struct icc_node *src, u64 max_bw)
NOC_QOS_MODEn_MASK, mode);
}
-static int qcom_icc_qos_set(struct icc_node *node, u64 sum_bw)
+static int qcom_icc_qos_set(struct icc_node *node)
{
struct qcom_icc_provider *qp = to_qcom_provider(node->provider);
struct qcom_icc_node *qn = node->data;
@@ -196,11 +196,11 @@ static int qcom_icc_qos_set(struct icc_node *node, u64 sum_bw)
switch (qp->type) {
case QCOM_ICC_BIMC:
- return qcom_icc_set_bimc_qos(node, sum_bw);
+ return qcom_icc_set_bimc_qos(node);
case QCOM_ICC_QNOC:
- return qcom_icc_set_qnoc_qos(node, sum_bw);
+ return qcom_icc_set_qnoc_qos(node);
default:
- return qcom_icc_set_noc_qos(node, sum_bw);
+ return qcom_icc_set_noc_qos(node);
}
}
--
2.40.0
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 <[email protected]>
---
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 dc9959a87df2..20340fb62fe6 100644
--- a/drivers/interconnect/qcom/msm8996.c
+++ b/drivers/interconnect/qcom/msm8996.c
@@ -2108,7 +2108,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 <[email protected]>");
MODULE_DESCRIPTION("Qualcomm MSM8996 NoC driver");
--
2.40.0
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 <[email protected]>
---
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 419b2122bebd..2298eb019534 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -449,17 +449,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 b9b63860042f..ee705edf19dd 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,9 +100,9 @@ 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 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 9aedfc8de4bf..dc9959a87df2 100644
--- a/drivers/interconnect/qcom/msm8996.c
+++ b/drivers/interconnect/qcom/msm8996.c
@@ -1819,6 +1819,7 @@ static const struct qcom_icc_desc msm8996_a0noc = {
.num_nodes = ARRAY_SIZE(a0noc_nodes),
.intf_clocks = a0noc_intf_clocks,
.num_intf_clocks = ARRAY_SIZE(a0noc_intf_clocks),
+ .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.40.0
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 <[email protected]>
---
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 2298eb019534..8c1beae13860 100644
--- a/drivers/interconnect/qcom/icc-rpm.c
+++ b/drivers/interconnect/qcom/icc-rpm.c
@@ -481,7 +481,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.40.0
On 7.04.2023 22:14, Konrad Dybcio wrote:
> Hi!
Bump!
Konrad
>
> v7 -> v8:
> - Rebase (dropping has_bus_pd, picked patches from v7)
>
> - Clean up the QoS-setting functions [3/8]
>
> - Only set the QoS registers once [4/8] - Georgi talked to some
> Qualcomm folks and we concluded that it's "good enough" as they
> should:tm: persist until a full reboot on "almost all" platforms
>
> - Move the intf clock enabling/disabling to correspond with /\ [5/8]
>
> - DO NOT switch to associating an interface clock with a given node
> (instead of a provider), as it makes little sense with the changes
> with [4/8] and the new iteration of [5/8]
>
> v7: https://lore.kernel.org/r/[email protected]
>
> v6 -> v7 changelog:
> - Rebase on Johan's recent patches
>
> Link to v6: https://lore.kernel.org/r/[email protected]
>
> 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/[email protected]/
>
> 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/[email protected]/
>
> 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/[email protected]/
>
> 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/[email protected]/T/#m056692bea71d4c272968d5e07afbd9eb07a88123
> [2] https://lore.kernel.org/linux-arm-msm/[email protected]/
>
> 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 <[email protected]>
> ---
> Konrad Dybcio (8):
> 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: Drop unused parameters
> interconnect: qcom: rpm: Set QoS registers only once
> 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 | 110 +++++++++++++++++++-----------------
> drivers/interconnect/qcom/icc-rpm.h | 22 ++++++--
> drivers/interconnect/qcom/msm8996.c | 35 +++++++-----
> drivers/interconnect/qcom/sdm660.c | 17 +++---
> 4 files changed, 102 insertions(+), 82 deletions(-)
> ---
> base-commit: e134c93f788fb93fd6a3ec3af9af850a2048c7e6
> change-id: 20230228-topic-qos-5435cac88d89
>
> Best regards,
Hi Konrad,
Thanks for re-spinning the patches!
On 7.04.23 23:14, 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.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/interconnect/qcom/icc-rpm.c | 40 ++++++++++++++++++++++++++++---------
> drivers/interconnect/qcom/icc-rpm.h | 14 +++++++++++--
> drivers/interconnect/qcom/msm8996.c | 22 +++++++++-----------
> drivers/interconnect/qcom/sdm660.c | 16 ++++++---------
> 4 files changed, 58 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index d79e508cb717..419b2122bebd 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -424,21 +424,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),
> @@ -446,6 +445,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];
Sadly, this is introducing a superfluous compiler warning, which is a bit annoying.
Could you please add some initialization to silence it and re-send just this patch.
drivers/interconnect/qcom/icc-rpm.c: In function ‘qnoc_probe’:
drivers/interconnect/qcom/icc-rpm.c:450:28: warning: ‘cds’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
450 | qp->intf_clks[i].id = cds[i];
| ~~~^~~
BR,
Georgi
On 18.05.2023 17:59, Georgi Djakov wrote:
> Hi Konrad,
> Thanks for re-spinning the patches!
>
> On 7.04.23 23:14, 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.
>>
>> Signed-off-by: Konrad Dybcio <[email protected]>
>> ---
>> drivers/interconnect/qcom/icc-rpm.c | 40 ++++++++++++++++++++++++++++---------
>> drivers/interconnect/qcom/icc-rpm.h | 14 +++++++++++--
>> drivers/interconnect/qcom/msm8996.c | 22 +++++++++-----------
>> drivers/interconnect/qcom/sdm660.c | 16 ++++++---------
>> 4 files changed, 58 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
>> index d79e508cb717..419b2122bebd 100644
>> --- a/drivers/interconnect/qcom/icc-rpm.c
>> +++ b/drivers/interconnect/qcom/icc-rpm.c
>> @@ -424,21 +424,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),
>> @@ -446,6 +445,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];
>
> Sadly, this is introducing a superfluous compiler warning, which is a bit annoying.
> Could you please add some initialization to silence it and re-send just this patch.
Ugh clang doesn't show it.. thanks
Konrad
>
> drivers/interconnect/qcom/icc-rpm.c: In function ‘qnoc_probe’:
> drivers/interconnect/qcom/icc-rpm.c:450:28: warning: ‘cds’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> 450 | qp->intf_clks[i].id = cds[i];
> | ~~~^~~
>
> BR,
> Georgi