v1 -> v2:
- Use CLK_IS_CRITICAL instead of leaving a clk enable vote, expand macros
to do so
- Fix the keepalive clocks for 8998 & 660 (CNoC -> PNoC, it was
confusingly named cnoc_periph downstream)
- Introduce .determinte_rate to ensure we don't set keepalive clocks'
rates below 19.2 MHz
- Add a (!conditional!) way to test the ultimate goal of all these changes
by essentially enabling unused clk cleanup through a dt property (for
legacy reasons)
v2 was tested on:
- MSM8996 Sony Kagura (can disable unused)
- MSM8998 Sony Maple (can disable unused with OOT icc)
- SM6375 Sony PDX225 (can disable unused with OOT icc)
v1: https://lore.kernel.org/r/[email protected]
This series brings support for a couple of things necessary for the full
system idle on SMD RPM SoCs, namely unused clk shutdown and keepalive
votes (permanent active votes that are required on certain clocks for the
platform to function).
Tested on MSM8996 and SM6375, does not seem to introduce any additional
regressions.
Keepalive clocks for other platforms were gathered by digging in old
downstream kernels, please give them a test.
Signed-off-by: Konrad Dybcio <[email protected]>
---
Konrad Dybcio (11):
dt-bindings: clock: qcom,rpmcc: Add a way to enable unused clock cleanup
clk: qcom: smd-rpm_ Make __DEFINE_CLK_SMD_RPM_BRANCH_PREFIX accept flags
clk: qcom: smd-rpm: Make DEFINE_CLK_SMD_RPM_BRANCH_A accept flags
clk: qcom: smd-rpm: Make BI_TCXO_AO critical
clk: qcom: smd-rpm: Make __DEFINE_CLK_SMD_RPM_PREFIX accept flags
clk: qcom: smd-rpm: Separate out a macro for defining an AO clock
clk: qcom: smd-rpm: Add support for keepalive votes
clk: qcom: smd-rpm: Introduce DEFINE_CLK_SMD_RPM_BUS_KEEPALIVE
clk: qcom: smd-rpm: Hook up PCNoC_0 keep_alive
clk: qcom: smd-rpm: Hook up CNoC_1 and SNoC_2 keep_alive
arm64: dts: qcom: msm8996: Enable rpmcc unused clk disablement
Shawn Guo (3):
clk: qcom: smd-rpm: Add .is_enabled hook
clk: qcom: smd-rpm: Add .is_prepared hook
clk: qcom: smd-rpm: Mark clock enabled in clk_smd_rpm_handoff()
.../devicetree/bindings/clock/qcom,rpmcc.yaml | 6 +
arch/arm64/boot/dts/qcom/msm8996.dtsi | 1 +
drivers/clk/qcom/clk-smd-rpm.c | 133 +++++++++++++++------
3 files changed, 106 insertions(+), 34 deletions(-)
---
base-commit: fc31900c948610e7b5c2f15fb7795832c8325327
change-id: 20230303-topic-rpmcc_sleep-d67aad9f3012
Best regards,
--
Konrad Dybcio <[email protected]>
From: Shawn Guo <[email protected]>
The RPM clock enabling state can be found with 'enabled' in struct
clk_smd_rpm. Add .is_enabled hook so that clk_summary in debugfs can
show a correct enabling state for RPM clocks.
Signed-off-by: Shawn Guo <[email protected]>
[Konrad: rebase]
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/clk/qcom/clk-smd-rpm.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index 198886c1b6c8..ecacfbc4a16c 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -424,18 +424,27 @@ static int clk_smd_rpm_enable_scaling(struct qcom_smd_rpm *rpm)
return 0;
}
+static int clk_smd_rpm_is_enabled(struct clk_hw *hw)
+{
+ struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
+
+ return r->enabled;
+}
+
static const struct clk_ops clk_smd_rpm_ops = {
.prepare = clk_smd_rpm_prepare,
.unprepare = clk_smd_rpm_unprepare,
.set_rate = clk_smd_rpm_set_rate,
.round_rate = clk_smd_rpm_round_rate,
.recalc_rate = clk_smd_rpm_recalc_rate,
+ .is_enabled = clk_smd_rpm_is_enabled,
};
static const struct clk_ops clk_smd_rpm_branch_ops = {
.prepare = clk_smd_rpm_prepare,
.unprepare = clk_smd_rpm_unprepare,
.recalc_rate = clk_smd_rpm_recalc_rate,
+ .is_enabled = clk_smd_rpm_is_enabled,
};
DEFINE_CLK_SMD_RPM_BRANCH_A(bi_tcxo, QCOM_SMD_RPM_MISC_CLK, 0, 19200000);
--
2.39.2
Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all
(or at least most) of the oneline peripherals ask the interconnect
framework to keep their buses online and guarantee enough bandwidth,
we're relying on bootloader defaults to keep the said buses alive through
RPM requests and rate setting on RPM clocks.
Without that in place, the RPM clocks are never enabled in the CCF, which
qualifies them to be cleaned up, since - as far as Linux is concerned -
nobody's using them and they're just wasting power. Doing so will end
tragically, as within miliseconds we'll get *some* access attempt on an
unlocked bus which will cause a platform crash.
On the other hand, if we want to save power and put well-supported
platforms to sleep, we should be shutting off at least some of these
clocks (this time with a clear distinction of which ones are *actually*
not in use, coming from the interconnect driver).
To differentiate between these two cases while not breaking older DTs,
introduce an opt-in property to correctly mark RPM clocks as enabled
after handoff (the initial max freq vote) and hence qualify them for the
common unused clock cleanup.
Signed-off-by: Konrad Dybcio <[email protected]>
---
Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
index 2a95bf8664f9..386153f61971 100644
--- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
@@ -58,6 +58,12 @@ properties:
minItems: 1
maxItems: 2
+ qcom,clk-disable-unused:
+ type: boolean
+ description:
+ Indicates whether unused RPM clocks can be shut down with the common
+ unused clock cleanup. Requires a functional interconnect driver.
+
required:
- compatible
- '#clock-cells'
--
2.39.2
From: Shawn Guo <[email protected]>
The RPM clocks are enabled/disabled through clk framework prepare/unprepare
hooks. Without .is_prepared hook, those unused RPM clocks will not be
disabled by core function clk_unprepare_unused_subtree(), because
clk_core_is_prepared() always returns 0.
Add .is_prepared hook to clk_ops and return the clock prepare (enable)
state, so that those unused RPM clocks can be disabled by clk framework.
Signed-off-by: Shawn Guo <[email protected]>
[Konrad: rebase, don't duplicate the enable func]
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/clk/qcom/clk-smd-rpm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index ecacfbc4a16c..cce7daa97c1e 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -438,6 +438,7 @@ static const struct clk_ops clk_smd_rpm_ops = {
.round_rate = clk_smd_rpm_round_rate,
.recalc_rate = clk_smd_rpm_recalc_rate,
.is_enabled = clk_smd_rpm_is_enabled,
+ .is_prepared = clk_smd_rpm_is_enabled,
};
static const struct clk_ops clk_smd_rpm_branch_ops = {
@@ -445,6 +446,7 @@ static const struct clk_ops clk_smd_rpm_branch_ops = {
.unprepare = clk_smd_rpm_unprepare,
.recalc_rate = clk_smd_rpm_recalc_rate,
.is_enabled = clk_smd_rpm_is_enabled,
+ .is_prepared = clk_smd_rpm_is_enabled,
};
DEFINE_CLK_SMD_RPM_BRANCH_A(bi_tcxo, QCOM_SMD_RPM_MISC_CLK, 0, 19200000);
--
2.39.2
In preparation for supporting keepalive clocks which can never be shut off
(as the platform would fall apart otherwise), make the
__DEFINE_CLK_SMD_RPM_BRANCH_PREFIX macro accept clock flags for the
active-only clock.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/clk/qcom/clk-smd-rpm.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index cce7daa97c1e..72b1f010509b 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -67,7 +67,7 @@
type, r_id, key)
#define __DEFINE_CLK_SMD_RPM_BRANCH_PREFIX(_prefix, _name, _active,\
- type, r_id, r, key) \
+ type, r_id, r, key, ao_flags) \
static struct clk_smd_rpm clk_smd_rpm_##_prefix##_active; \
static struct clk_smd_rpm clk_smd_rpm_##_prefix##_name = { \
.rpm_res_type = (type), \
@@ -102,12 +102,13 @@
.name = "xo_board", \
}, \
.num_parents = 1, \
+ .flags = (ao_flags), \
}, \
}
#define __DEFINE_CLK_SMD_RPM_BRANCH(_name, _active, type, r_id, r, key) \
__DEFINE_CLK_SMD_RPM_BRANCH_PREFIX(/* empty */, \
- _name, _active, type, r_id, r, key)
+ _name, _active, type, r_id, r, key, 0)
#define DEFINE_CLK_SMD_RPM(_name, type, r_id) \
__DEFINE_CLK_SMD_RPM(_name##_clk, _name##_a_clk, \
@@ -126,12 +127,12 @@
#define DEFINE_CLK_SMD_RPM_BRANCH(_name, type, r_id, r) \
__DEFINE_CLK_SMD_RPM_BRANCH_PREFIX(branch_, \
_name##_clk, _name##_a_clk, \
- type, r_id, r, QCOM_RPM_SMD_KEY_ENABLE)
+ type, r_id, r, QCOM_RPM_SMD_KEY_ENABLE, 0)
#define DEFINE_CLK_SMD_RPM_BRANCH_A(_name, type, r_id, r) \
__DEFINE_CLK_SMD_RPM_BRANCH_PREFIX(branch_, \
_name, _name##_a, type, \
- r_id, r, QCOM_RPM_SMD_KEY_ENABLE)
+ r_id, r, QCOM_RPM_SMD_KEY_ENABLE, 0)
#define DEFINE_CLK_SMD_RPM_QDSS(_name, type, r_id) \
__DEFINE_CLK_SMD_RPM(_name##_clk, _name##_a_clk, \
@@ -146,7 +147,7 @@
__DEFINE_CLK_SMD_RPM_BRANCH_PREFIX(_prefix, \
_name, _name##_a, \
QCOM_SMD_RPM_CLK_BUF_A, r_id, r, \
- QCOM_RPM_KEY_SOFTWARE_ENABLE)
+ QCOM_RPM_KEY_SOFTWARE_ENABLE, 0)
#define DEFINE_CLK_SMD_RPM_XO_BUFFER_PINCTRL(_name, r_id, r) \
DEFINE_CLK_SMD_RPM_XO_BUFFER(_name, r_id, r); \
--
2.39.2
In preparation for supporting keepalive clocks which can never be shut off
(as the platform would fall apart otherwise), make the
DEFINE_CLK_SMD_RPM_BRANCH_A macro accept clock flags for the active-only
clock.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/clk/qcom/clk-smd-rpm.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index 72b1f010509b..fec6ae4a8989 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -129,10 +129,10 @@
_name##_clk, _name##_a_clk, \
type, r_id, r, QCOM_RPM_SMD_KEY_ENABLE, 0)
-#define DEFINE_CLK_SMD_RPM_BRANCH_A(_name, type, r_id, r) \
+#define DEFINE_CLK_SMD_RPM_BRANCH_A(_name, type, r_id, r, ao_flags) \
__DEFINE_CLK_SMD_RPM_BRANCH_PREFIX(branch_, \
_name, _name##_a, type, \
- r_id, r, QCOM_RPM_SMD_KEY_ENABLE, 0)
+ r_id, r, QCOM_RPM_SMD_KEY_ENABLE, ao_flags)
#define DEFINE_CLK_SMD_RPM_QDSS(_name, type, r_id) \
__DEFINE_CLK_SMD_RPM(_name##_clk, _name##_a_clk, \
@@ -450,10 +450,10 @@ static const struct clk_ops clk_smd_rpm_branch_ops = {
.is_prepared = clk_smd_rpm_is_enabled,
};
-DEFINE_CLK_SMD_RPM_BRANCH_A(bi_tcxo, QCOM_SMD_RPM_MISC_CLK, 0, 19200000);
+DEFINE_CLK_SMD_RPM_BRANCH_A(bi_tcxo, QCOM_SMD_RPM_MISC_CLK, 0, 19200000, 0);
DEFINE_CLK_SMD_RPM_BRANCH(qdss, QCOM_SMD_RPM_MISC_CLK, 1, 19200000);
DEFINE_CLK_SMD_RPM_QDSS(qdss, QCOM_SMD_RPM_MISC_CLK, 1);
-DEFINE_CLK_SMD_RPM_BRANCH_A(bimc_freq_log, QCOM_SMD_RPM_MISC_CLK, 4, 1);
+DEFINE_CLK_SMD_RPM_BRANCH_A(bimc_freq_log, QCOM_SMD_RPM_MISC_CLK, 4, 1, 0);
DEFINE_CLK_SMD_RPM_BRANCH(mss_cfg_ahb, QCOM_SMD_RPM_MCFG_CLK, 0, 19200000);
--
2.39.2
We should never let go of the active-only XO vote, as otherwise the
RPM may decide that there are no online users and it can be shut down,
resulting in a total, uncontrolled system collapse.
Guarantee this through adding the CLK_IS_CRITICAL flag.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/clk/qcom/clk-smd-rpm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index fec6ae4a8989..9dc779360ada 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -450,7 +450,8 @@ static const struct clk_ops clk_smd_rpm_branch_ops = {
.is_prepared = clk_smd_rpm_is_enabled,
};
-DEFINE_CLK_SMD_RPM_BRANCH_A(bi_tcxo, QCOM_SMD_RPM_MISC_CLK, 0, 19200000, 0);
+/* Disabling BI_TCXO_AO could gate the root clock source of the entire system. */
+DEFINE_CLK_SMD_RPM_BRANCH_A(bi_tcxo, QCOM_SMD_RPM_MISC_CLK, 0, 19200000, CLK_IS_CRITICAL);
DEFINE_CLK_SMD_RPM_BRANCH(qdss, QCOM_SMD_RPM_MISC_CLK, 1, 19200000);
DEFINE_CLK_SMD_RPM_QDSS(qdss, QCOM_SMD_RPM_MISC_CLK, 1);
DEFINE_CLK_SMD_RPM_BRANCH_A(bimc_freq_log, QCOM_SMD_RPM_MISC_CLK, 4, 1, 0);
--
2.39.2
In preparation for supporting keepalive clocks which can never be shut off
(as the platform would fall apart otherwise), make the
__DEFINE_CLK_SMD_RPM_PREFIX macro accept clock flags for the active-only
clock.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/clk/qcom/clk-smd-rpm.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index 9dc779360ada..ddb4268ba2a5 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -26,7 +26,7 @@
#define QCOM_RPM_SCALING_ENABLE_ID 0x2
#define __DEFINE_CLK_SMD_RPM_PREFIX(_prefix, _name, _active, \
- type, r_id, key) \
+ type, r_id, key, ao_flags) \
static struct clk_smd_rpm clk_smd_rpm_##_prefix##_active; \
static struct clk_smd_rpm clk_smd_rpm_##_prefix##_name = { \
.rpm_res_type = (type), \
@@ -58,13 +58,14 @@
.fw_name = "xo", \
.name = "xo_board", \
}, \
+ .flags = (ao_flags), \
.num_parents = 1, \
}, \
}
#define __DEFINE_CLK_SMD_RPM(_name, _active, type, r_id, key) \
__DEFINE_CLK_SMD_RPM_PREFIX(/* empty */, _name, _active, \
- type, r_id, key)
+ type, r_id, key, 0)
#define __DEFINE_CLK_SMD_RPM_BRANCH_PREFIX(_prefix, _name, _active,\
type, r_id, r, key, ao_flags) \
@@ -117,7 +118,7 @@
#define DEFINE_CLK_SMD_RPM_BUS(_name, r_id) \
__DEFINE_CLK_SMD_RPM_PREFIX(bus_##r_id##_, \
_name##_clk, _name##_a_clk, QCOM_SMD_RPM_BUS_CLK, r_id, \
- QCOM_RPM_SMD_KEY_RATE)
+ QCOM_RPM_SMD_KEY_RATE, 0)
#define DEFINE_CLK_SMD_RPM_CLK_SRC(_name, type, r_id) \
__DEFINE_CLK_SMD_RPM( \
--
2.39.2
To declare a keepalive variant of a bus clock, it will be useful to
have a reusable macro which will ease defining a keepalive variant
of an AO clock with an IS_CRITICAL flag. Introduce it.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/clk/qcom/clk-smd-rpm.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index ddb4268ba2a5..eb7781e5c8c1 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -44,6 +44,11 @@
.num_parents = 1, \
}, \
}; \
+ __DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active, type, \
+ r_id, key, ao_flags)
+
+#define __DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active, \
+ type, r_id, key, ao_flags) \
static struct clk_smd_rpm clk_smd_rpm_##_prefix##_active = { \
.rpm_res_type = (type), \
.rpm_clk_id = (r_id), \
--
2.39.2
Some bus clock should always have a minimum (19.2 MHz) vote cast on
them, otherwise the platform will fall apart, hang and reboot.
Add support for specifying which clocks should be kept alive and
always keep a vote on XO_A to make sure the clock tree doesn't
collapse. This removes the need to keep a maximum vote that was
previously guaranteed by clk_smd_rpm_handoff.
This commit is a combination of existing (not-exactly-upstream) work
by Taniya Das, Shawn Guo and myself.
Co-developed-by: Shawn Guo <[email protected]>
Co-developed-by: Taniya Das <[email protected]>
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/clk/qcom/clk-smd-rpm.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index eb7781e5c8c1..d89918f9ae60 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -45,15 +45,17 @@
}, \
}; \
__DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active, type, \
- r_id, key, ao_flags)
+ r_id, key, ao_flags, false)
#define __DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active, \
- type, r_id, key, ao_flags) \
+ type, r_id, key, ao_flags, \
+ _keep_alive) \
static struct clk_smd_rpm clk_smd_rpm_##_prefix##_active = { \
.rpm_res_type = (type), \
.rpm_clk_id = (r_id), \
.active_only = true, \
.rpm_key = (key), \
+ .keep_alive = (_keep_alive), \
.peer = &clk_smd_rpm_##_prefix##_name, \
.rate = INT_MAX, \
.hw.init = &(struct clk_init_data){ \
@@ -170,6 +172,7 @@ struct clk_smd_rpm {
const bool active_only;
bool enabled;
bool branch;
+ bool keep_alive;
struct clk_smd_rpm *peer;
struct clk_hw hw;
unsigned long rate;
@@ -198,11 +201,16 @@ static int clk_smd_rpm_handoff(struct clk_smd_rpm *r)
.value = cpu_to_le32(r->branch ? 1 : INT_MAX),
};
+ /* Set up keepalive clocks with a minimum bus rate */
+ if (r->keep_alive)
+ req.value = cpu_to_le32(19200); /* 19.2 MHz */
+
ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_ACTIVE_STATE,
r->rpm_res_type, r->rpm_clk_id, &req,
sizeof(req));
if (ret)
return ret;
+
ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_SLEEP_STATE,
r->rpm_res_type, r->rpm_clk_id, &req,
sizeof(req));
@@ -438,12 +446,29 @@ static int clk_smd_rpm_is_enabled(struct clk_hw *hw)
return r->enabled;
}
+static int clk_smd_rpm_determine_rate(struct clk_hw *hw,
+ struct clk_rate_request *req)
+{
+ struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
+
+ /*
+ * RPM resolves the rates internally. All we have to do on the kernel
+ * side is ensure that we don't accidentally put down the keepalive
+ * clocks, which could happen if they received a vote below 19.2 MHz.
+ */
+ if (r->keep_alive)
+ req->rate = max(req->rate, 19200000UL);
+
+ return 0;
+}
+
static const struct clk_ops clk_smd_rpm_ops = {
.prepare = clk_smd_rpm_prepare,
.unprepare = clk_smd_rpm_unprepare,
.set_rate = clk_smd_rpm_set_rate,
.round_rate = clk_smd_rpm_round_rate,
.recalc_rate = clk_smd_rpm_recalc_rate,
+ .determine_rate = clk_smd_rpm_determine_rate,
.is_enabled = clk_smd_rpm_is_enabled,
.is_prepared = clk_smd_rpm_is_enabled,
};
--
2.39.2
In preparation for supporting keepalive clocks which can never be shut off
(as the platform would fall apart otherwise), add a macro for defining
such clocks.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/clk/qcom/clk-smd-rpm.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index d89918f9ae60..8e25b3d7d30c 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -127,6 +127,11 @@
_name##_clk, _name##_a_clk, QCOM_SMD_RPM_BUS_CLK, r_id, \
QCOM_RPM_SMD_KEY_RATE, 0)
+#define DEFINE_CLK_SMD_RPM_BUS_KEEP_ALIVE(_name, r_id) \
+ __DEFINE_CLK_SMD_RPM_AO_PREFIX(bus_##r_id##_, \
+ _name##_clk, _name##_a_keep_alive_clk, QCOM_SMD_RPM_BUS_CLK, \
+ r_id, QCOM_RPM_SMD_KEY_RATE, CLK_IS_CRITICAL, true)
+
#define DEFINE_CLK_SMD_RPM_CLK_SRC(_name, type, r_id) \
__DEFINE_CLK_SMD_RPM( \
_name##_clk_src, _name##_a_clk_src, \
--
2.39.2
14 [1] of our 18 supported platforms need an active keepalive vote on
PCNoC_0 so as not to cause havoc on the entire SoC. Guarantee that.
[1] there are 13 changes to driver data, but 8226 reuses 8974.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/clk/qcom/clk-smd-rpm.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index 8e25b3d7d30c..a44b52bd0c83 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -500,6 +500,7 @@ DEFINE_CLK_SMD_RPM(aggre1_noc, QCOM_SMD_RPM_AGGR_CLK, 1);
DEFINE_CLK_SMD_RPM(aggre2_noc, QCOM_SMD_RPM_AGGR_CLK, 2);
DEFINE_CLK_SMD_RPM_BUS(pcnoc, 0);
+DEFINE_CLK_SMD_RPM_BUS_KEEP_ALIVE(pcnoc, 0);
DEFINE_CLK_SMD_RPM_BUS(snoc, 1);
DEFINE_CLK_SMD_RPM_BUS(sysmmnoc, 2);
DEFINE_CLK_SMD_RPM_BUS(cnoc, 2);
@@ -558,7 +559,7 @@ DEFINE_CLK_SMD_RPM_XO_BUFFER(div_clk3, 13, 19200000);
static struct clk_smd_rpm *msm8909_clks[] = {
[RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
- [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
+ [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
[RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
[RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
[RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
@@ -592,7 +593,7 @@ static const struct rpm_smd_clk_desc rpm_clk_msm8909 = {
static struct clk_smd_rpm *msm8916_clks[] = {
[RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
- [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
+ [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
[RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
[RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
[RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
@@ -626,7 +627,7 @@ static struct clk_smd_rpm *msm8936_clks[] = {
[RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
[RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
[RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
- [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
+ [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
[RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
[RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
[RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
@@ -660,7 +661,7 @@ static const struct rpm_smd_clk_desc rpm_clk_msm8936 = {
static struct clk_smd_rpm *msm8974_clks[] = {
[RPM_SMD_PNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
- [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
+ [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
[RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
[RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
[RPM_SMD_CNOC_CLK] = &clk_smd_rpm_bus_2_cnoc_clk,
@@ -712,7 +713,7 @@ static struct clk_smd_rpm *msm8976_clks[] = {
[RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
[RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
[RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
- [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
+ [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
[RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
[RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
[RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
@@ -746,7 +747,7 @@ static struct clk_smd_rpm *msm8992_clks[] = {
[RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
[RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
[RPM_SMD_PNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
- [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
+ [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
[RPM_SMD_OCMEMGX_CLK] = &clk_smd_rpm_ocmemgx_clk,
[RPM_SMD_OCMEMGX_A_CLK] = &clk_smd_rpm_ocmemgx_a_clk,
[RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
@@ -804,7 +805,7 @@ static struct clk_smd_rpm *msm8994_clks[] = {
[RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
[RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
[RPM_SMD_PNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
- [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
+ [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
[RPM_SMD_OCMEMGX_CLK] = &clk_smd_rpm_ocmemgx_clk,
[RPM_SMD_OCMEMGX_A_CLK] = &clk_smd_rpm_ocmemgx_a_clk,
[RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
@@ -864,7 +865,7 @@ static struct clk_smd_rpm *msm8996_clks[] = {
[RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
[RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
[RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
- [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
+ [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
[RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
[RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
[RPM_SMD_CNOC_CLK] = &clk_smd_rpm_bus_2_cnoc_clk,
@@ -918,7 +919,7 @@ static struct clk_smd_rpm *qcs404_clks[] = {
[RPM_SMD_QDSS_CLK] = &clk_smd_rpm_qdss_clk,
[RPM_SMD_QDSS_A_CLK] = &clk_smd_rpm_qdss_a_clk,
[RPM_SMD_PNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
- [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
+ [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
[RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
[RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
[RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
@@ -948,7 +949,7 @@ static struct clk_smd_rpm *msm8998_clks[] = {
[RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
[RPM_SMD_BIMC_A_CLK] = &clk_smd_rpm_bimc_a_clk,
[RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
- [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
+ [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
[RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
[RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
[RPM_SMD_CNOC_CLK] = &clk_smd_rpm_bus_2_cnoc_clk,
@@ -1010,7 +1011,7 @@ static struct clk_smd_rpm *sdm660_clks[] = {
[RPM_SMD_CNOC_CLK] = &clk_smd_rpm_bus_2_cnoc_clk,
[RPM_SMD_CNOC_A_CLK] = &clk_smd_rpm_bus_2_cnoc_a_clk,
[RPM_SMD_CNOC_PERIPH_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
- [RPM_SMD_CNOC_PERIPH_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
+ [RPM_SMD_CNOC_PERIPH_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
[RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
[RPM_SMD_BIMC_A_CLK] = &clk_smd_rpm_bimc_a_clk,
[RPM_SMD_MMSSNOC_AXI_CLK] = &clk_smd_rpm_mmssnoc_axi_rpm_clk,
@@ -1052,7 +1053,7 @@ static struct clk_smd_rpm *mdm9607_clks[] = {
[RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
[RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
[RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
- [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
+ [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
[RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
[RPM_SMD_BIMC_A_CLK] = &clk_smd_rpm_bimc_a_clk,
[RPM_SMD_QPIC_CLK] = &clk_smd_rpm_qpic_clk,
@@ -1074,7 +1075,7 @@ static struct clk_smd_rpm *msm8953_clks[] = {
[RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
[RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
[RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
- [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
+ [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
[RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
[RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
[RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
--
2.39.2
4 of our 18 supported platforms need an active keepalive vote on
CNoC_1 and SNoC_2 so as not to cause havoc on the entire SoC.
Guarantee that.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/clk/qcom/clk-smd-rpm.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index a44b52bd0c83..ef3157fd29d5 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -507,7 +507,9 @@ DEFINE_CLK_SMD_RPM_BUS(cnoc, 2);
DEFINE_CLK_SMD_RPM_BUS(mmssnoc_ahb, 3);
DEFINE_CLK_SMD_RPM_BUS(snoc_periph, 0);
DEFINE_CLK_SMD_RPM_BUS(cnoc, 1);
+DEFINE_CLK_SMD_RPM_BUS_KEEP_ALIVE(cnoc, 1);
DEFINE_CLK_SMD_RPM_BUS(snoc, 2);
+DEFINE_CLK_SMD_RPM_BUS_KEEP_ALIVE(snoc, 2);
DEFINE_CLK_SMD_RPM_BUS(snoc_lpass, 5);
DEFINE_CLK_SMD_RPM(bimc, QCOM_SMD_RPM_MEM_CLK, 0);
@@ -1111,7 +1113,7 @@ static struct clk_smd_rpm *sm6125_clks[] = {
[RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
[RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
[RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_2_snoc_clk,
- [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_2_snoc_a_clk,
+ [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_2_snoc_a_keep_alive_clk,
[RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
[RPM_SMD_BIMC_A_CLK] = &clk_smd_rpm_bimc_a_clk,
[RPM_SMD_QDSS_CLK] = &clk_smd_rpm_branch_qdss_clk,
@@ -1121,7 +1123,7 @@ static struct clk_smd_rpm *sm6125_clks[] = {
[RPM_SMD_RF_CLK2] = &clk_smd_rpm_rf_clk2,
[RPM_SMD_RF_CLK2_A] = &clk_smd_rpm_rf_clk2_a,
[RPM_SMD_CNOC_CLK] = &clk_smd_rpm_bus_1_cnoc_clk,
- [RPM_SMD_CNOC_A_CLK] = &clk_smd_rpm_bus_1_cnoc_a_clk,
+ [RPM_SMD_CNOC_A_CLK] = &clk_smd_rpm_bus_1_cnoc_a_keep_alive_clk,
[RPM_SMD_IPA_CLK] = &clk_smd_rpm_ipa_clk,
[RPM_SMD_IPA_A_CLK] = &clk_smd_rpm_ipa_a_clk,
[RPM_SMD_CE1_CLK] = &clk_smd_rpm_ce1_clk,
@@ -1154,7 +1156,7 @@ static struct clk_smd_rpm *sm6115_clks[] = {
[RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
[RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
[RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_2_snoc_clk,
- [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_2_snoc_a_clk,
+ [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_2_snoc_a_keep_alive_clk,
[RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
[RPM_SMD_BIMC_A_CLK] = &clk_smd_rpm_bimc_a_clk,
[RPM_SMD_QDSS_CLK] = &clk_smd_rpm_branch_qdss_clk,
@@ -1164,7 +1166,7 @@ static struct clk_smd_rpm *sm6115_clks[] = {
[RPM_SMD_RF_CLK2] = &clk_smd_rpm_rf_clk2,
[RPM_SMD_RF_CLK2_A] = &clk_smd_rpm_rf_clk2_a,
[RPM_SMD_CNOC_CLK] = &clk_smd_rpm_bus_1_cnoc_clk,
- [RPM_SMD_CNOC_A_CLK] = &clk_smd_rpm_bus_1_cnoc_a_clk,
+ [RPM_SMD_CNOC_A_CLK] = &clk_smd_rpm_bus_1_cnoc_a_keep_alive_clk,
[RPM_SMD_IPA_CLK] = &clk_smd_rpm_ipa_clk,
[RPM_SMD_IPA_A_CLK] = &clk_smd_rpm_ipa_a_clk,
[RPM_SMD_CE1_CLK] = &clk_smd_rpm_ce1_clk,
@@ -1194,13 +1196,13 @@ static struct clk_smd_rpm *sm6375_clks[] = {
[RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
[RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
[RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_2_snoc_clk,
- [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_2_snoc_a_clk,
+ [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_2_snoc_a_keep_alive_clk,
[RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
[RPM_SMD_BIMC_A_CLK] = &clk_smd_rpm_bimc_a_clk,
[RPM_SMD_QDSS_CLK] = &clk_smd_rpm_branch_qdss_clk,
[RPM_SMD_QDSS_A_CLK] = &clk_smd_rpm_branch_qdss_a_clk,
[RPM_SMD_CNOC_CLK] = &clk_smd_rpm_bus_1_cnoc_clk,
- [RPM_SMD_CNOC_A_CLK] = &clk_smd_rpm_bus_1_cnoc_a_clk,
+ [RPM_SMD_CNOC_A_CLK] = &clk_smd_rpm_bus_1_cnoc_a_keep_alive_clk,
[RPM_SMD_IPA_CLK] = &clk_smd_rpm_ipa_clk,
[RPM_SMD_IPA_A_CLK] = &clk_smd_rpm_ipa_a_clk,
[RPM_SMD_QUP_CLK] = &clk_smd_rpm_qup_clk,
@@ -1231,7 +1233,7 @@ static struct clk_smd_rpm *qcm2290_clks[] = {
[RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
[RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
[RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_2_snoc_clk,
- [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_2_snoc_a_clk,
+ [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_2_snoc_a_keep_alive_clk,
[RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
[RPM_SMD_BIMC_A_CLK] = &clk_smd_rpm_bimc_a_clk,
[RPM_SMD_QDSS_CLK] = &clk_smd_rpm_branch_qdss_clk,
@@ -1241,7 +1243,7 @@ static struct clk_smd_rpm *qcm2290_clks[] = {
[RPM_SMD_RF_CLK3] = &clk_smd_rpm_38m4_rf_clk3,
[RPM_SMD_RF_CLK3_A] = &clk_smd_rpm_38m4_rf_clk3_a,
[RPM_SMD_CNOC_CLK] = &clk_smd_rpm_bus_1_cnoc_clk,
- [RPM_SMD_CNOC_A_CLK] = &clk_smd_rpm_bus_1_cnoc_a_clk,
+ [RPM_SMD_CNOC_A_CLK] = &clk_smd_rpm_bus_1_cnoc_a_keep_alive_clk,
[RPM_SMD_IPA_CLK] = &clk_smd_rpm_ipa_clk,
[RPM_SMD_IPA_A_CLK] = &clk_smd_rpm_ipa_a_clk,
[RPM_SMD_QUP_CLK] = &clk_smd_rpm_qup_clk,
--
2.39.2
MSM8996 is in a good enough state to shut down unused RPM clocks.
Do it!
Signed-off-by: Konrad Dybcio <[email protected]>
---
arch/arm64/boot/dts/qcom/msm8996.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 905678e7175d..59451d87cfbf 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -488,6 +488,7 @@ rpmcc: clock-controller {
#clock-cells = <1>;
clocks = <&xo_board>;
clock-names = "xo";
+ qcom,clk-disable-unused;
};
rpmpd: power-controller {
--
2.39.2
From: Shawn Guo <[email protected]>
The result of clock handoff is that the clock is voted by APSS and
enabled by RPM. So it should be marked as enabled.
This, combined with .is_enabled/prepared will ultimately cause RPM
clocks that were enabled by the bootloader to actually be shut down
if unused. We can only afford to do so if we have a functioning ICC
driver. An immediate thought to test for that can be to provide an
ICC path to the RPMCC node, however that would create a couple of
circular dependencies between GCC/DISPCC/.../RPMCC and ICC.
The next best thing to do without breaking older device trees is to
add an opt-in DTS property. Do just that.
Signed-off-by: Shawn Guo <[email protected]>
[Konrad: make conditional, explain the consequences]
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/clk/qcom/clk-smd-rpm.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index ef3157fd29d5..6736e53e607a 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -197,7 +197,7 @@ struct rpm_smd_clk_desc {
static DEFINE_MUTEX(rpm_smd_clk_lock);
-static int clk_smd_rpm_handoff(struct clk_smd_rpm *r)
+static int clk_smd_rpm_handoff(struct clk_smd_rpm *r, bool disable_unused_clks)
{
int ret;
struct clk_smd_rpm_req req = {
@@ -222,6 +222,10 @@ static int clk_smd_rpm_handoff(struct clk_smd_rpm *r)
if (ret)
return ret;
+ /* Marking clocks enabled here will trigger unused cleanup */
+ if (disable_unused_clks)
+ r->enabled = true;
+
return 0;
}
@@ -1319,6 +1323,7 @@ static int rpm_smd_clk_probe(struct platform_device *pdev)
struct qcom_smd_rpm *rpm;
struct clk_smd_rpm **rpm_smd_clks;
const struct rpm_smd_clk_desc *desc;
+ bool disable_unused_clks;
rpm = dev_get_drvdata(pdev->dev.parent);
if (!rpm) {
@@ -1326,6 +1331,14 @@ static int rpm_smd_clk_probe(struct platform_device *pdev)
return -ENODEV;
}
+ /*
+ * We can only really park unused clocks if we have a sane interconnect
+ * driver. Otherwise, the platform may (and probably will) try accessing
+ * IPs that are hosted on unclocked buses. In an effort not to break
+ * older DTs, make this an opt-in through a DT property.
+ */
+ disable_unused_clks = of_property_read_bool(pdev->dev.of_node, "qcom,clk-disable-unused");
+
desc = of_device_get_match_data(&pdev->dev);
if (!desc)
return -EINVAL;
@@ -1339,7 +1352,7 @@ static int rpm_smd_clk_probe(struct platform_device *pdev)
rpm_smd_clks[i]->rpm = rpm;
- ret = clk_smd_rpm_handoff(rpm_smd_clks[i]);
+ ret = clk_smd_rpm_handoff(rpm_smd_clks[i], disable_unused_clks);
if (ret)
goto err;
}
--
2.39.2
On 08/03/2023 23:35, Konrad Dybcio wrote:
> From: Shawn Guo <[email protected]>
>
> The RPM clock enabling state can be found with 'enabled' in struct
> clk_smd_rpm. Add .is_enabled hook so that clk_summary in debugfs can
> show a correct enabling state for RPM clocks.
>
> Signed-off-by: Shawn Guo <[email protected]>
> [Konrad: rebase]
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/clk/qcom/clk-smd-rpm.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 08/03/2023 23:35, Konrad Dybcio wrote:
> From: Shawn Guo <[email protected]>
>
> The RPM clocks are enabled/disabled through clk framework prepare/unprepare
> hooks. Without .is_prepared hook, those unused RPM clocks will not be
> disabled by core function clk_unprepare_unused_subtree(), because
> clk_core_is_prepared() always returns 0.
>
> Add .is_prepared hook to clk_ops and return the clock prepare (enable)
> state, so that those unused RPM clocks can be disabled by clk framework.
>
> Signed-off-by: Shawn Guo <[email protected]>
> [Konrad: rebase, don't duplicate the enable func]
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/clk/qcom/clk-smd-rpm.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> index ecacfbc4a16c..cce7daa97c1e 100644
> --- a/drivers/clk/qcom/clk-smd-rpm.c
> +++ b/drivers/clk/qcom/clk-smd-rpm.c
> @@ -438,6 +438,7 @@ static const struct clk_ops clk_smd_rpm_ops = {
> .round_rate = clk_smd_rpm_round_rate,
> .recalc_rate = clk_smd_rpm_recalc_rate,
> .is_enabled = clk_smd_rpm_is_enabled,
> + .is_prepared = clk_smd_rpm_is_enabled,
I still suppose that prepared and enabled statuses should be handled
separately. Otherwise CCF might get confused about prepared-but-enabled
clocks. With this patch in place, it will treat them as being not prepared.
> };
>
> static const struct clk_ops clk_smd_rpm_branch_ops = {
> @@ -445,6 +446,7 @@ static const struct clk_ops clk_smd_rpm_branch_ops = {
> .unprepare = clk_smd_rpm_unprepare,
> .recalc_rate = clk_smd_rpm_recalc_rate,
> .is_enabled = clk_smd_rpm_is_enabled,
> + .is_prepared = clk_smd_rpm_is_enabled,
> };
>
> DEFINE_CLK_SMD_RPM_BRANCH_A(bi_tcxo, QCOM_SMD_RPM_MISC_CLK, 0, 19200000);
--
With best wishes
Dmitry
On 08/03/2023 23:35, Konrad Dybcio wrote:
> In preparation for supporting keepalive clocks which can never be shut off
> (as the platform would fall apart otherwise), make the
> __DEFINE_CLK_SMD_RPM_BRANCH_PREFIX macro accept clock flags for the
> active-only clock.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/clk/qcom/clk-smd-rpm.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 08/03/2023 23:35, Konrad Dybcio wrote:
> In preparation for supporting keepalive clocks which can never be shut off
> (as the platform would fall apart otherwise), make the
> DEFINE_CLK_SMD_RPM_BRANCH_A macro accept clock flags for the active-only
> clock.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/clk/qcom/clk-smd-rpm.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 08/03/2023 23:35, Konrad Dybcio wrote:
> We should never let go of the active-only XO vote, as otherwise the
> RPM may decide that there are no online users and it can be shut down,
> resulting in a total, uncontrolled system collapse.
>
> Guarantee this through adding the CLK_IS_CRITICAL flag.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/clk/qcom/clk-smd-rpm.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 08/03/2023 23:35, Konrad Dybcio wrote:
> In preparation for supporting keepalive clocks which can never be shut off
> (as the platform would fall apart otherwise), make the
> __DEFINE_CLK_SMD_RPM_PREFIX macro accept clock flags for the active-only
> clock.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/clk/qcom/clk-smd-rpm.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 08/03/2023 23:35, Konrad Dybcio wrote:
> To declare a keepalive variant of a bus clock, it will be useful to
> have a reusable macro which will ease defining a keepalive variant
> of an AO clock with an IS_CRITICAL flag. Introduce it.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/clk/qcom/clk-smd-rpm.c | 5 +++++
> 1 file changed, 5 insertions(+)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 08/03/2023 23:35, Konrad Dybcio wrote:
> Some bus clock should always have a minimum (19.2 MHz) vote cast on
> them, otherwise the platform will fall apart, hang and reboot.
>
> Add support for specifying which clocks should be kept alive and
> always keep a vote on XO_A to make sure the clock tree doesn't
> collapse. This removes the need to keep a maximum vote that was
> previously guaranteed by clk_smd_rpm_handoff.
>
> This commit is a combination of existing (not-exactly-upstream) work
> by Taniya Das, Shawn Guo and myself.
>
> Co-developed-by: Shawn Guo <[email protected]>
> Co-developed-by: Taniya Das <[email protected]>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/clk/qcom/clk-smd-rpm.c | 29 +++++++++++++++++++++++++++--
> 1 file changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> index eb7781e5c8c1..d89918f9ae60 100644
> --- a/drivers/clk/qcom/clk-smd-rpm.c
> +++ b/drivers/clk/qcom/clk-smd-rpm.c
> @@ -45,15 +45,17 @@
> }, \
> }; \
> __DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active, type, \
> - r_id, key, ao_flags)
> + r_id, key, ao_flags, false)
>
> #define __DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active, \
> - type, r_id, key, ao_flags) \
> + type, r_id, key, ao_flags, \
> + _keep_alive) \
> static struct clk_smd_rpm clk_smd_rpm_##_prefix##_active = { \
> .rpm_res_type = (type), \
> .rpm_clk_id = (r_id), \
> .active_only = true, \
> .rpm_key = (key), \
> + .keep_alive = (_keep_alive), \
> .peer = &clk_smd_rpm_##_prefix##_name, \
> .rate = INT_MAX, \
> .hw.init = &(struct clk_init_data){ \
> @@ -170,6 +172,7 @@ struct clk_smd_rpm {
> const bool active_only;
> bool enabled;
> bool branch;
> + bool keep_alive;
> struct clk_smd_rpm *peer;
> struct clk_hw hw;
> unsigned long rate;
> @@ -198,11 +201,16 @@ static int clk_smd_rpm_handoff(struct clk_smd_rpm *r)
> .value = cpu_to_le32(r->branch ? 1 : INT_MAX),
> };
>
> + /* Set up keepalive clocks with a minimum bus rate */
> + if (r->keep_alive)
> + req.value = cpu_to_le32(19200); /* 19.2 MHz */
Should this be set to cpu_to_le32(max(19200, ...)) ?
> +
> ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_ACTIVE_STATE,
> r->rpm_res_type, r->rpm_clk_id, &req,
> sizeof(req));
> if (ret)
> return ret;
> +
> ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_SLEEP_STATE,
> r->rpm_res_type, r->rpm_clk_id, &req,
> sizeof(req));
> @@ -438,12 +446,29 @@ static int clk_smd_rpm_is_enabled(struct clk_hw *hw)
> return r->enabled;
> }
>
> +static int clk_smd_rpm_determine_rate(struct clk_hw *hw,
> + struct clk_rate_request *req)
> +{
> + struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
> +
> + /*
> + * RPM resolves the rates internally. All we have to do on the kernel
> + * side is ensure that we don't accidentally put down the keepalive
> + * clocks, which could happen if they received a vote below 19.2 MHz.
> + */
> + if (r->keep_alive)
> + req->rate = max(req->rate, 19200000UL);
> +
> + return 0;
> +}
> +
> static const struct clk_ops clk_smd_rpm_ops = {
> .prepare = clk_smd_rpm_prepare,
> .unprepare = clk_smd_rpm_unprepare,
> .set_rate = clk_smd_rpm_set_rate,
> .round_rate = clk_smd_rpm_round_rate,
> .recalc_rate = clk_smd_rpm_recalc_rate,
> + .determine_rate = clk_smd_rpm_determine_rate,
> .is_enabled = clk_smd_rpm_is_enabled,
> .is_prepared = clk_smd_rpm_is_enabled,
> };
>
--
With best wishes
Dmitry
On 9.03.2023 01:54, Dmitry Baryshkov wrote:
> On 08/03/2023 23:35, Konrad Dybcio wrote:
>> Some bus clock should always have a minimum (19.2 MHz) vote cast on
>> them, otherwise the platform will fall apart, hang and reboot.
>>
>> Add support for specifying which clocks should be kept alive and
>> always keep a vote on XO_A to make sure the clock tree doesn't
>> collapse. This removes the need to keep a maximum vote that was
>> previously guaranteed by clk_smd_rpm_handoff.
>>
>> This commit is a combination of existing (not-exactly-upstream) work
>> by Taniya Das, Shawn Guo and myself.
>>
>> Co-developed-by: Shawn Guo <[email protected]>
>> Co-developed-by: Taniya Das <[email protected]>
>> Signed-off-by: Konrad Dybcio <[email protected]>
>> ---
>> drivers/clk/qcom/clk-smd-rpm.c | 29 +++++++++++++++++++++++++++--
>> 1 file changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
>> index eb7781e5c8c1..d89918f9ae60 100644
>> --- a/drivers/clk/qcom/clk-smd-rpm.c
>> +++ b/drivers/clk/qcom/clk-smd-rpm.c
>> @@ -45,15 +45,17 @@
>> }, \
>> }; \
>> __DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active, type, \
>> - r_id, key, ao_flags)
>> + r_id, key, ao_flags, false)
>> #define __DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active, \
>> - type, r_id, key, ao_flags) \
>> + type, r_id, key, ao_flags, \
>> + _keep_alive) \
>> static struct clk_smd_rpm clk_smd_rpm_##_prefix##_active = { \
>> .rpm_res_type = (type), \
>> .rpm_clk_id = (r_id), \
>> .active_only = true, \
>> .rpm_key = (key), \
>> + .keep_alive = (_keep_alive), \
>> .peer = &clk_smd_rpm_##_prefix##_name, \
>> .rate = INT_MAX, \
>> .hw.init = &(struct clk_init_data){ \
>> @@ -170,6 +172,7 @@ struct clk_smd_rpm {
>> const bool active_only;
>> bool enabled;
>> bool branch;
>> + bool keep_alive;
>> struct clk_smd_rpm *peer;
>> struct clk_hw hw;
>> unsigned long rate;
>> @@ -198,11 +201,16 @@ static int clk_smd_rpm_handoff(struct clk_smd_rpm *r)
>> .value = cpu_to_le32(r->branch ? 1 : INT_MAX),
>> };
>> + /* Set up keepalive clocks with a minimum bus rate */
>> + if (r->keep_alive)
>> + req.value = cpu_to_le32(19200); /* 19.2 MHz */
>
>
> Should this be set to cpu_to_le32(max(19200, ...)) ?
I was debating this. Downstream explicitly sets 19.2 Mhz here and
the only regression I can think of is that we'd throttle a bus that
was left on by the bootloader and is (ab)used by us..
But then, it's only an active vote, and we're voting INT_MAX on the
non-active-only one, so that's a non-issue.
So I think 19.2 here is okay as the bare minimum, whatever stupidity
the eventual interconnect driver may entail..
Konrad
>
>> +
>> ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_ACTIVE_STATE,
>> r->rpm_res_type, r->rpm_clk_id, &req,
>> sizeof(req));
>> if (ret)
>> return ret;
>> +
>> ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_SLEEP_STATE,
>> r->rpm_res_type, r->rpm_clk_id, &req,
>> sizeof(req));
>> @@ -438,12 +446,29 @@ static int clk_smd_rpm_is_enabled(struct clk_hw *hw)
>> return r->enabled;
>> }
>> +static int clk_smd_rpm_determine_rate(struct clk_hw *hw,
>> + struct clk_rate_request *req)
>> +{
>> + struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
>> +
>> + /*
>> + * RPM resolves the rates internally. All we have to do on the kernel
>> + * side is ensure that we don't accidentally put down the keepalive
>> + * clocks, which could happen if they received a vote below 19.2 MHz.
>> + */
>> + if (r->keep_alive)
>> + req->rate = max(req->rate, 19200000UL);
>> +
>> + return 0;
>> +}
>> +
>> static const struct clk_ops clk_smd_rpm_ops = {
>> .prepare = clk_smd_rpm_prepare,
>> .unprepare = clk_smd_rpm_unprepare,
>> .set_rate = clk_smd_rpm_set_rate,
>> .round_rate = clk_smd_rpm_round_rate,
>> .recalc_rate = clk_smd_rpm_recalc_rate,
>> + .determine_rate = clk_smd_rpm_determine_rate,
>> .is_enabled = clk_smd_rpm_is_enabled,
>> .is_prepared = clk_smd_rpm_is_enabled,
>> };
>>
>
On 08/03/2023 23:35, Konrad Dybcio wrote:
> In preparation for supporting keepalive clocks which can never be shut off
> (as the platform would fall apart otherwise), add a macro for defining
> such clocks.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/clk/qcom/clk-smd-rpm.c | 5 +++++
> 1 file changed, 5 insertions(+)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 08/03/2023 23:35, Konrad Dybcio wrote:
> 14 [1] of our 18 supported platforms need an active keepalive vote on
> PCNoC_0 so as not to cause havoc on the entire SoC. Guarantee that.
>
> [1] there are 13 changes to driver data, but 8226 reuses 8974.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/clk/qcom/clk-smd-rpm.c | 27 ++++++++++++++-------------
> 1 file changed, 14 insertions(+), 13 deletions(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 08/03/2023 23:35, Konrad Dybcio wrote:
> 4 of our 18 supported platforms need an active keepalive vote on
> CNoC_1 and SNoC_2 so as not to cause havoc on the entire SoC.
> Guarantee that.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/clk/qcom/clk-smd-rpm.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote:
> Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all
> (or at least most) of the oneline peripherals ask the interconnect
> framework to keep their buses online and guarantee enough bandwidth,
> we're relying on bootloader defaults to keep the said buses alive through
> RPM requests and rate setting on RPM clocks.
>
> Without that in place, the RPM clocks are never enabled in the CCF, which
> qualifies them to be cleaned up, since - as far as Linux is concerned -
> nobody's using them and they're just wasting power. Doing so will end
> tragically, as within miliseconds we'll get *some* access attempt on an
> unlocked bus which will cause a platform crash.
>
> On the other hand, if we want to save power and put well-supported
> platforms to sleep, we should be shutting off at least some of these
> clocks (this time with a clear distinction of which ones are *actually*
> not in use, coming from the interconnect driver).
>
> To differentiate between these two cases while not breaking older DTs,
> introduce an opt-in property to correctly mark RPM clocks as enabled
> after handoff (the initial max freq vote) and hence qualify them for the
> common unused clock cleanup.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
> index 2a95bf8664f9..386153f61971 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
> @@ -58,6 +58,12 @@ properties:
> minItems: 1
> maxItems: 2
>
> + qcom,clk-disable-unused:
> + type: boolean
> + description:
> + Indicates whether unused RPM clocks can be shut down with the common
> + unused clock cleanup. Requires a functional interconnect driver.
I don't think this should be QCom specific. Come up with something
common (which will probably have some debate).
Rob
On 16.03.2023 23:58, Rob Herring wrote:
> On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote:
>> Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all
>> (or at least most) of the oneline peripherals ask the interconnect
>> framework to keep their buses online and guarantee enough bandwidth,
>> we're relying on bootloader defaults to keep the said buses alive through
>> RPM requests and rate setting on RPM clocks.
>>
>> Without that in place, the RPM clocks are never enabled in the CCF, which
>> qualifies them to be cleaned up, since - as far as Linux is concerned -
>> nobody's using them and they're just wasting power. Doing so will end
>> tragically, as within miliseconds we'll get *some* access attempt on an
>> unlocked bus which will cause a platform crash.
>>
>> On the other hand, if we want to save power and put well-supported
>> platforms to sleep, we should be shutting off at least some of these
>> clocks (this time with a clear distinction of which ones are *actually*
>> not in use, coming from the interconnect driver).
>>
>> To differentiate between these two cases while not breaking older DTs,
>> introduce an opt-in property to correctly mark RPM clocks as enabled
>> after handoff (the initial max freq vote) and hence qualify them for the
>> common unused clock cleanup.
>>
>> Signed-off-by: Konrad Dybcio <[email protected]>
>> ---
>> Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
>> index 2a95bf8664f9..386153f61971 100644
>> --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
>> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
>> @@ -58,6 +58,12 @@ properties:
>> minItems: 1
>> maxItems: 2
>>
>> + qcom,clk-disable-unused:
>> + type: boolean
>> + description:
>> + Indicates whether unused RPM clocks can be shut down with the common
>> + unused clock cleanup. Requires a functional interconnect driver.
>
> I don't think this should be QCom specific. Come up with something
> common (which will probably have some debate).
Generally the opposite (ignoring unused clocks during the cleanup) is
the thing you need to opt into.
I can however see how (especially with the focus on not breaking things
for older DTs) somebody else may also decide to only allow them to be
cleaned up conditionally (by marking the clocks that were enabled earlier
as enabled in Linux OR not addding clk.flags |= CLK_IGNORE_UNUSED) as we
do here.
Stephen, Rob, would `clk-disable-unused` be a fitting generic property
name for that? Should we also think about `clk-ignore-unused` as a
clock-controller-specific alternative to the CCF-wide clk_ignore_unused
cmdline?
Konrad
>
> Rob
Quoting Konrad Dybcio (2023-03-16 17:31:34)
>
> On 16.03.2023 23:58, Rob Herring wrote:
> > On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote:
> >>
> >> + qcom,clk-disable-unused:
> >> + type: boolean
> >> + description:
> >> + Indicates whether unused RPM clocks can be shut down with the common
> >> + unused clock cleanup. Requires a functional interconnect driver.
> >
> > I don't think this should be QCom specific. Come up with something
> > common (which will probably have some debate).
> Generally the opposite (ignoring unused clocks during the cleanup) is
> the thing you need to opt into.
>
> I can however see how (especially with the focus on not breaking things
> for older DTs) somebody else may also decide to only allow them to be
> cleaned up conditionally (by marking the clocks that were enabled earlier
> as enabled in Linux OR not addding clk.flags |= CLK_IGNORE_UNUSED) as we
> do here.
>
> Stephen, Rob, would `clk-disable-unused` be a fitting generic property
> name for that? Should we also think about `clk-ignore-unused` as a
> clock-controller-specific alternative to the CCF-wide clk_ignore_unused
> cmdline?
>
There are multiple threads on the list about disabling unused clks.
Moving the decision to disable unused clks to a DT property is yet
another approach. I'd rather not do that, because it really isn't
describing the hardware configuration. If anything, I'd expect the
property to be describing which clks are enabled by the firmware and
then leave the decision to disable them because they're unused up to the
software.
On Wed, Mar 08, 2023 at 10:35:18PM +0100, Konrad Dybcio wrote:
> From: Shawn Guo <[email protected]>
>
> The RPM clock enabling state can be found with 'enabled' in struct
> clk_smd_rpm. Add .is_enabled hook so that clk_summary in debugfs can
> show a correct enabling state for RPM clocks.
>
I don't think .is_enabled should be implemented for clocks where the
actual state can't be queried.
E.g. should a clock which is is_enabled = false be unprepared during
disable_unused? It's already disabled...
Regards,
Bjorn
> Signed-off-by: Shawn Guo <[email protected]>
> [Konrad: rebase]
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/clk/qcom/clk-smd-rpm.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> index 198886c1b6c8..ecacfbc4a16c 100644
> --- a/drivers/clk/qcom/clk-smd-rpm.c
> +++ b/drivers/clk/qcom/clk-smd-rpm.c
> @@ -424,18 +424,27 @@ static int clk_smd_rpm_enable_scaling(struct qcom_smd_rpm *rpm)
> return 0;
> }
>
> +static int clk_smd_rpm_is_enabled(struct clk_hw *hw)
> +{
> + struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
> +
> + return r->enabled;
> +}
> +
> static const struct clk_ops clk_smd_rpm_ops = {
> .prepare = clk_smd_rpm_prepare,
> .unprepare = clk_smd_rpm_unprepare,
> .set_rate = clk_smd_rpm_set_rate,
> .round_rate = clk_smd_rpm_round_rate,
> .recalc_rate = clk_smd_rpm_recalc_rate,
> + .is_enabled = clk_smd_rpm_is_enabled,
> };
>
> static const struct clk_ops clk_smd_rpm_branch_ops = {
> .prepare = clk_smd_rpm_prepare,
> .unprepare = clk_smd_rpm_unprepare,
> .recalc_rate = clk_smd_rpm_recalc_rate,
> + .is_enabled = clk_smd_rpm_is_enabled,
> };
>
> DEFINE_CLK_SMD_RPM_BRANCH_A(bi_tcxo, QCOM_SMD_RPM_MISC_CLK, 0, 19200000);
>
> --
> 2.39.2
>
On Wed, Mar 08, 2023 at 10:35:27PM +0100, Konrad Dybcio wrote:
> 14 [1] of our 18 supported platforms need an active keepalive vote on
> PCNoC_0 so as not to cause havoc on the entire SoC. Guarantee that.
>
Given that these are all clocks for peripheral (or peripheral & config)
noc clocks, doesn't this just ensure that the running CPU is able to
reach something on the peripheral bus?
That is, are you encoding a active-only keep-alive interconnect path?
Could we somehow express that using the interconnect driver on top
instead?
Regards,
Bjorn
> [1] there are 13 changes to driver data, but 8226 reuses 8974.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/clk/qcom/clk-smd-rpm.c | 27 ++++++++++++++-------------
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> index 8e25b3d7d30c..a44b52bd0c83 100644
> --- a/drivers/clk/qcom/clk-smd-rpm.c
> +++ b/drivers/clk/qcom/clk-smd-rpm.c
> @@ -500,6 +500,7 @@ DEFINE_CLK_SMD_RPM(aggre1_noc, QCOM_SMD_RPM_AGGR_CLK, 1);
> DEFINE_CLK_SMD_RPM(aggre2_noc, QCOM_SMD_RPM_AGGR_CLK, 2);
>
> DEFINE_CLK_SMD_RPM_BUS(pcnoc, 0);
> +DEFINE_CLK_SMD_RPM_BUS_KEEP_ALIVE(pcnoc, 0);
> DEFINE_CLK_SMD_RPM_BUS(snoc, 1);
> DEFINE_CLK_SMD_RPM_BUS(sysmmnoc, 2);
> DEFINE_CLK_SMD_RPM_BUS(cnoc, 2);
> @@ -558,7 +559,7 @@ DEFINE_CLK_SMD_RPM_XO_BUFFER(div_clk3, 13, 19200000);
>
> static struct clk_smd_rpm *msm8909_clks[] = {
> [RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
> - [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
> + [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
> @@ -592,7 +593,7 @@ static const struct rpm_smd_clk_desc rpm_clk_msm8909 = {
>
> static struct clk_smd_rpm *msm8916_clks[] = {
> [RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
> - [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
> + [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
> @@ -626,7 +627,7 @@ static struct clk_smd_rpm *msm8936_clks[] = {
> [RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
> [RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
> [RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
> - [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
> + [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
> @@ -660,7 +661,7 @@ static const struct rpm_smd_clk_desc rpm_clk_msm8936 = {
>
> static struct clk_smd_rpm *msm8974_clks[] = {
> [RPM_SMD_PNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
> - [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
> + [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
> [RPM_SMD_CNOC_CLK] = &clk_smd_rpm_bus_2_cnoc_clk,
> @@ -712,7 +713,7 @@ static struct clk_smd_rpm *msm8976_clks[] = {
> [RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
> [RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
> [RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
> - [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
> + [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
> @@ -746,7 +747,7 @@ static struct clk_smd_rpm *msm8992_clks[] = {
> [RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
> [RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
> [RPM_SMD_PNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
> - [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
> + [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
> [RPM_SMD_OCMEMGX_CLK] = &clk_smd_rpm_ocmemgx_clk,
> [RPM_SMD_OCMEMGX_A_CLK] = &clk_smd_rpm_ocmemgx_a_clk,
> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
> @@ -804,7 +805,7 @@ static struct clk_smd_rpm *msm8994_clks[] = {
> [RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
> [RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
> [RPM_SMD_PNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
> - [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
> + [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
> [RPM_SMD_OCMEMGX_CLK] = &clk_smd_rpm_ocmemgx_clk,
> [RPM_SMD_OCMEMGX_A_CLK] = &clk_smd_rpm_ocmemgx_a_clk,
> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
> @@ -864,7 +865,7 @@ static struct clk_smd_rpm *msm8996_clks[] = {
> [RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
> [RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
> [RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
> - [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
> + [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
> [RPM_SMD_CNOC_CLK] = &clk_smd_rpm_bus_2_cnoc_clk,
> @@ -918,7 +919,7 @@ static struct clk_smd_rpm *qcs404_clks[] = {
> [RPM_SMD_QDSS_CLK] = &clk_smd_rpm_qdss_clk,
> [RPM_SMD_QDSS_A_CLK] = &clk_smd_rpm_qdss_a_clk,
> [RPM_SMD_PNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
> - [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
> + [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
> @@ -948,7 +949,7 @@ static struct clk_smd_rpm *msm8998_clks[] = {
> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
> [RPM_SMD_BIMC_A_CLK] = &clk_smd_rpm_bimc_a_clk,
> [RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
> - [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
> + [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
> [RPM_SMD_CNOC_CLK] = &clk_smd_rpm_bus_2_cnoc_clk,
> @@ -1010,7 +1011,7 @@ static struct clk_smd_rpm *sdm660_clks[] = {
> [RPM_SMD_CNOC_CLK] = &clk_smd_rpm_bus_2_cnoc_clk,
> [RPM_SMD_CNOC_A_CLK] = &clk_smd_rpm_bus_2_cnoc_a_clk,
> [RPM_SMD_CNOC_PERIPH_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
> - [RPM_SMD_CNOC_PERIPH_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
> + [RPM_SMD_CNOC_PERIPH_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
> [RPM_SMD_BIMC_A_CLK] = &clk_smd_rpm_bimc_a_clk,
> [RPM_SMD_MMSSNOC_AXI_CLK] = &clk_smd_rpm_mmssnoc_axi_rpm_clk,
> @@ -1052,7 +1053,7 @@ static struct clk_smd_rpm *mdm9607_clks[] = {
> [RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
> [RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
> [RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
> - [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
> + [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
> [RPM_SMD_BIMC_A_CLK] = &clk_smd_rpm_bimc_a_clk,
> [RPM_SMD_QPIC_CLK] = &clk_smd_rpm_qpic_clk,
> @@ -1074,7 +1075,7 @@ static struct clk_smd_rpm *msm8953_clks[] = {
> [RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
> [RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
> [RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
> - [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
> + [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
>
> --
> 2.39.2
>
On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote:
> Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all
> (or at least most) of the oneline peripherals ask the interconnect
> framework to keep their buses online and guarantee enough bandwidth,
> we're relying on bootloader defaults to keep the said buses alive through
> RPM requests and rate setting on RPM clocks.
>
> Without that in place, the RPM clocks are never enabled in the CCF, which
> qualifies them to be cleaned up, since - as far as Linux is concerned -
> nobody's using them and they're just wasting power. Doing so will end
> tragically, as within miliseconds we'll get *some* access attempt on an
> unlocked bus which will cause a platform crash.
>
> On the other hand, if we want to save power and put well-supported
> platforms to sleep, we should be shutting off at least some of these
> clocks (this time with a clear distinction of which ones are *actually*
> not in use, coming from the interconnect driver).
>
> To differentiate between these two cases while not breaking older DTs,
> introduce an opt-in property to correctly mark RPM clocks as enabled
> after handoff (the initial max freq vote) and hence qualify them for the
> common unused clock cleanup.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
> index 2a95bf8664f9..386153f61971 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
> @@ -58,6 +58,12 @@ properties:
> minItems: 1
> maxItems: 2
>
> + qcom,clk-disable-unused:
This is a Linux implementation detail, not a description of the
hardware. So it unfortunately doesn't belong here.
> + type: boolean
> + description:
> + Indicates whether unused RPM clocks can be shut down with the common
> + unused clock cleanup. Requires a functional interconnect driver.
This is an interesting aspect though, there's a lot of things that will
break if any one of these building blocks are missing, for any reason.
Regards,
Bjorn
> +
> required:
> - compatible
> - '#clock-cells'
>
> --
> 2.39.2
>
On 22.03.2023 04:19, Bjorn Andersson wrote:
> On Wed, Mar 08, 2023 at 10:35:27PM +0100, Konrad Dybcio wrote:
>> 14 [1] of our 18 supported platforms need an active keepalive vote on
>> PCNoC_0 so as not to cause havoc on the entire SoC. Guarantee that.
>>
>
> Given that these are all clocks for peripheral (or peripheral & config)
> noc clocks, doesn't this just ensure that the running CPU is able to
> reach something on the peripheral bus?
>
> That is, are you encoding a active-only keep-alive interconnect path?
> Could we somehow express that using the interconnect driver on top
> instead?
Qualcomm downstream has a different notion of "keep_alive" on the msm_bus
side, which is basically:
if (bus->keepalive)
active_vote_freq = max(19200000, aggregated_rate);
Considering that they're doing essentially the same thing, I suppose both
periph/config NoC could be enabled there instead.
Konrad
>
> Regards,
> Bjorn
>
>> [1] there are 13 changes to driver data, but 8226 reuses 8974.
>>
>> Signed-off-by: Konrad Dybcio <[email protected]>
>> ---
>> drivers/clk/qcom/clk-smd-rpm.c | 27 ++++++++++++++-------------
>> 1 file changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
>> index 8e25b3d7d30c..a44b52bd0c83 100644
>> --- a/drivers/clk/qcom/clk-smd-rpm.c
>> +++ b/drivers/clk/qcom/clk-smd-rpm.c
>> @@ -500,6 +500,7 @@ DEFINE_CLK_SMD_RPM(aggre1_noc, QCOM_SMD_RPM_AGGR_CLK, 1);
>> DEFINE_CLK_SMD_RPM(aggre2_noc, QCOM_SMD_RPM_AGGR_CLK, 2);
>>
>> DEFINE_CLK_SMD_RPM_BUS(pcnoc, 0);
>> +DEFINE_CLK_SMD_RPM_BUS_KEEP_ALIVE(pcnoc, 0);
>> DEFINE_CLK_SMD_RPM_BUS(snoc, 1);
>> DEFINE_CLK_SMD_RPM_BUS(sysmmnoc, 2);
>> DEFINE_CLK_SMD_RPM_BUS(cnoc, 2);
>> @@ -558,7 +559,7 @@ DEFINE_CLK_SMD_RPM_XO_BUFFER(div_clk3, 13, 19200000);
>>
>> static struct clk_smd_rpm *msm8909_clks[] = {
>> [RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
>> - [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
>> + [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
>> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
>> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
>> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
>> @@ -592,7 +593,7 @@ static const struct rpm_smd_clk_desc rpm_clk_msm8909 = {
>>
>> static struct clk_smd_rpm *msm8916_clks[] = {
>> [RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
>> - [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
>> + [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
>> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
>> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
>> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
>> @@ -626,7 +627,7 @@ static struct clk_smd_rpm *msm8936_clks[] = {
>> [RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
>> [RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
>> [RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
>> - [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
>> + [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
>> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
>> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
>> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
>> @@ -660,7 +661,7 @@ static const struct rpm_smd_clk_desc rpm_clk_msm8936 = {
>>
>> static struct clk_smd_rpm *msm8974_clks[] = {
>> [RPM_SMD_PNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
>> - [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
>> + [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
>> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
>> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
>> [RPM_SMD_CNOC_CLK] = &clk_smd_rpm_bus_2_cnoc_clk,
>> @@ -712,7 +713,7 @@ static struct clk_smd_rpm *msm8976_clks[] = {
>> [RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
>> [RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
>> [RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
>> - [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
>> + [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
>> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
>> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
>> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
>> @@ -746,7 +747,7 @@ static struct clk_smd_rpm *msm8992_clks[] = {
>> [RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
>> [RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
>> [RPM_SMD_PNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
>> - [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
>> + [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
>> [RPM_SMD_OCMEMGX_CLK] = &clk_smd_rpm_ocmemgx_clk,
>> [RPM_SMD_OCMEMGX_A_CLK] = &clk_smd_rpm_ocmemgx_a_clk,
>> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
>> @@ -804,7 +805,7 @@ static struct clk_smd_rpm *msm8994_clks[] = {
>> [RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
>> [RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
>> [RPM_SMD_PNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
>> - [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
>> + [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
>> [RPM_SMD_OCMEMGX_CLK] = &clk_smd_rpm_ocmemgx_clk,
>> [RPM_SMD_OCMEMGX_A_CLK] = &clk_smd_rpm_ocmemgx_a_clk,
>> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
>> @@ -864,7 +865,7 @@ static struct clk_smd_rpm *msm8996_clks[] = {
>> [RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
>> [RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
>> [RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
>> - [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
>> + [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
>> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
>> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
>> [RPM_SMD_CNOC_CLK] = &clk_smd_rpm_bus_2_cnoc_clk,
>> @@ -918,7 +919,7 @@ static struct clk_smd_rpm *qcs404_clks[] = {
>> [RPM_SMD_QDSS_CLK] = &clk_smd_rpm_qdss_clk,
>> [RPM_SMD_QDSS_A_CLK] = &clk_smd_rpm_qdss_a_clk,
>> [RPM_SMD_PNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
>> - [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
>> + [RPM_SMD_PNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
>> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
>> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
>> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
>> @@ -948,7 +949,7 @@ static struct clk_smd_rpm *msm8998_clks[] = {
>> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
>> [RPM_SMD_BIMC_A_CLK] = &clk_smd_rpm_bimc_a_clk,
>> [RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
>> - [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
>> + [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
>> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
>> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
>> [RPM_SMD_CNOC_CLK] = &clk_smd_rpm_bus_2_cnoc_clk,
>> @@ -1010,7 +1011,7 @@ static struct clk_smd_rpm *sdm660_clks[] = {
>> [RPM_SMD_CNOC_CLK] = &clk_smd_rpm_bus_2_cnoc_clk,
>> [RPM_SMD_CNOC_A_CLK] = &clk_smd_rpm_bus_2_cnoc_a_clk,
>> [RPM_SMD_CNOC_PERIPH_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
>> - [RPM_SMD_CNOC_PERIPH_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
>> + [RPM_SMD_CNOC_PERIPH_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
>> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
>> [RPM_SMD_BIMC_A_CLK] = &clk_smd_rpm_bimc_a_clk,
>> [RPM_SMD_MMSSNOC_AXI_CLK] = &clk_smd_rpm_mmssnoc_axi_rpm_clk,
>> @@ -1052,7 +1053,7 @@ static struct clk_smd_rpm *mdm9607_clks[] = {
>> [RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
>> [RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
>> [RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
>> - [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
>> + [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
>> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
>> [RPM_SMD_BIMC_A_CLK] = &clk_smd_rpm_bimc_a_clk,
>> [RPM_SMD_QPIC_CLK] = &clk_smd_rpm_qpic_clk,
>> @@ -1074,7 +1075,7 @@ static struct clk_smd_rpm *msm8953_clks[] = {
>> [RPM_SMD_XO_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo,
>> [RPM_SMD_XO_A_CLK_SRC] = &clk_smd_rpm_branch_bi_tcxo_a,
>> [RPM_SMD_PCNOC_CLK] = &clk_smd_rpm_bus_0_pcnoc_clk,
>> - [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_clk,
>> + [RPM_SMD_PCNOC_A_CLK] = &clk_smd_rpm_bus_0_pcnoc_a_keep_alive_clk,
>> [RPM_SMD_SNOC_CLK] = &clk_smd_rpm_bus_1_snoc_clk,
>> [RPM_SMD_SNOC_A_CLK] = &clk_smd_rpm_bus_1_snoc_a_clk,
>> [RPM_SMD_BIMC_CLK] = &clk_smd_rpm_bimc_clk,
>>
>> --
>> 2.39.2
>>
On 22.03.2023 04:02, Bjorn Andersson wrote:
> On Wed, Mar 08, 2023 at 10:35:18PM +0100, Konrad Dybcio wrote:
>> From: Shawn Guo <[email protected]>
>>
>> The RPM clock enabling state can be found with 'enabled' in struct
>> clk_smd_rpm. Add .is_enabled hook so that clk_summary in debugfs can
>> show a correct enabling state for RPM clocks.
>>
>
> I don't think .is_enabled should be implemented for clocks where the
> actual state can't be queried.
>
> E.g. should a clock which is is_enabled = false be unprepared during
> disable_unused? It's already disabled...
That's true, it sounds silly.
However, I feel like it's the least painful option, as trying to disable
a clock that's already actually disabled (read, in hw+RPM, not Linux)
will not do any harm.
Not adding this (and by extension not making use of any sort of unused
clk cleanup) will prevent the system from hitting low power modes and
SMD RPM is strictly speaking, too dumb to figure out that these clocks
aren't really consumed.
Konrad
>
> Regards,
> Bjorn
>
>> Signed-off-by: Shawn Guo <[email protected]>
>> [Konrad: rebase]
>> Signed-off-by: Konrad Dybcio <[email protected]>
>> ---
>> drivers/clk/qcom/clk-smd-rpm.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
>> index 198886c1b6c8..ecacfbc4a16c 100644
>> --- a/drivers/clk/qcom/clk-smd-rpm.c
>> +++ b/drivers/clk/qcom/clk-smd-rpm.c
>> @@ -424,18 +424,27 @@ static int clk_smd_rpm_enable_scaling(struct qcom_smd_rpm *rpm)
>> return 0;
>> }
>>
>> +static int clk_smd_rpm_is_enabled(struct clk_hw *hw)
>> +{
>> + struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
>> +
>> + return r->enabled;
>> +}
>> +
>> static const struct clk_ops clk_smd_rpm_ops = {
>> .prepare = clk_smd_rpm_prepare,
>> .unprepare = clk_smd_rpm_unprepare,
>> .set_rate = clk_smd_rpm_set_rate,
>> .round_rate = clk_smd_rpm_round_rate,
>> .recalc_rate = clk_smd_rpm_recalc_rate,
>> + .is_enabled = clk_smd_rpm_is_enabled,
>> };
>>
>> static const struct clk_ops clk_smd_rpm_branch_ops = {
>> .prepare = clk_smd_rpm_prepare,
>> .unprepare = clk_smd_rpm_unprepare,
>> .recalc_rate = clk_smd_rpm_recalc_rate,
>> + .is_enabled = clk_smd_rpm_is_enabled,
>> };
>>
>> DEFINE_CLK_SMD_RPM_BRANCH_A(bi_tcxo, QCOM_SMD_RPM_MISC_CLK, 0, 19200000);
>>
>> --
>> 2.39.2
>>
On 17.03.2023 19:20, Stephen Boyd wrote:
> Quoting Konrad Dybcio (2023-03-16 17:31:34)
>>
>> On 16.03.2023 23:58, Rob Herring wrote:
>>> On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote:
>>>>
>>>> + qcom,clk-disable-unused:
>>>> + type: boolean
>>>> + description:
>>>> + Indicates whether unused RPM clocks can be shut down with the common
>>>> + unused clock cleanup. Requires a functional interconnect driver.
>>>
>>> I don't think this should be QCom specific. Come up with something
>>> common (which will probably have some debate).
>> Generally the opposite (ignoring unused clocks during the cleanup) is
>> the thing you need to opt into.
>>
>> I can however see how (especially with the focus on not breaking things
>> for older DTs) somebody else may also decide to only allow them to be
>> cleaned up conditionally (by marking the clocks that were enabled earlier
>> as enabled in Linux OR not addding clk.flags |= CLK_IGNORE_UNUSED) as we
>> do here.
>>
>> Stephen, Rob, would `clk-disable-unused` be a fitting generic property
>> name for that? Should we also think about `clk-ignore-unused` as a
>> clock-controller-specific alternative to the CCF-wide clk_ignore_unused
>> cmdline?
>>
>
> There are multiple threads on the list about disabling unused clks.
> Moving the decision to disable unused clks to a DT property is yet
> another approach. I'd rather not do that, because it really isn't
> describing the hardware configuration. If anything, I'd expect the
> property to be describing which clks are enabled by the firmware and
> then leave the decision to disable them because they're unused up to the
> software.
After some more thinking, I realized that this could be made opt-in
simply with driver_data..
WDYT?
Konrad
On 6.04.2023 16:44, Konrad Dybcio wrote:
>
>
> On 17.03.2023 19:20, Stephen Boyd wrote:
>> Quoting Konrad Dybcio (2023-03-16 17:31:34)
>>>
>>> On 16.03.2023 23:58, Rob Herring wrote:
>>>> On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote:
>>>>>
>>>>> + qcom,clk-disable-unused:
>>>>> + type: boolean
>>>>> + description:
>>>>> + Indicates whether unused RPM clocks can be shut down with the common
>>>>> + unused clock cleanup. Requires a functional interconnect driver.
>>>>
>>>> I don't think this should be QCom specific. Come up with something
>>>> common (which will probably have some debate).
>>> Generally the opposite (ignoring unused clocks during the cleanup) is
>>> the thing you need to opt into.
>>>
>>> I can however see how (especially with the focus on not breaking things
>>> for older DTs) somebody else may also decide to only allow them to be
>>> cleaned up conditionally (by marking the clocks that were enabled earlier
>>> as enabled in Linux OR not addding clk.flags |= CLK_IGNORE_UNUSED) as we
>>> do here.
>>>
>>> Stephen, Rob, would `clk-disable-unused` be a fitting generic property
>>> name for that? Should we also think about `clk-ignore-unused` as a
>>> clock-controller-specific alternative to the CCF-wide clk_ignore_unused
>>> cmdline?
>>>
>>
>> There are multiple threads on the list about disabling unused clks.
>> Moving the decision to disable unused clks to a DT property is yet
>> another approach. I'd rather not do that, because it really isn't
>> describing the hardware configuration. If anything, I'd expect the
>> property to be describing which clks are enabled by the firmware and
>> then leave the decision to disable them because they're unused up to the
>> software.
> After some more thinking, I realized that this could be made opt-in
> simply with driver_data..
>
> WDYT?
..on a re-evaluation, obviously not a great idea.. Old DTBs will not
be happy about that.
Konrad
>
> Konrad
On 7.04.2023 22:17, Konrad Dybcio wrote:
>
>
> On 6.04.2023 16:44, Konrad Dybcio wrote:
>>
>>
>> On 17.03.2023 19:20, Stephen Boyd wrote:
>>> Quoting Konrad Dybcio (2023-03-16 17:31:34)
>>>>
>>>> On 16.03.2023 23:58, Rob Herring wrote:
>>>>> On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote:
>>>>>>
>>>>>> + qcom,clk-disable-unused:
>>>>>> + type: boolean
>>>>>> + description:
>>>>>> + Indicates whether unused RPM clocks can be shut down with the common
>>>>>> + unused clock cleanup. Requires a functional interconnect driver.
>>>>>
>>>>> I don't think this should be QCom specific. Come up with something
>>>>> common (which will probably have some debate).
>>>> Generally the opposite (ignoring unused clocks during the cleanup) is
>>>> the thing you need to opt into.
>>>>
>>>> I can however see how (especially with the focus on not breaking things
>>>> for older DTs) somebody else may also decide to only allow them to be
>>>> cleaned up conditionally (by marking the clocks that were enabled earlier
>>>> as enabled in Linux OR not addding clk.flags |= CLK_IGNORE_UNUSED) as we
>>>> do here.
>>>>
>>>> Stephen, Rob, would `clk-disable-unused` be a fitting generic property
>>>> name for that? Should we also think about `clk-ignore-unused` as a
>>>> clock-controller-specific alternative to the CCF-wide clk_ignore_unused
>>>> cmdline?
>>>>
>>>
>>> There are multiple threads on the list about disabling unused clks.
>>> Moving the decision to disable unused clks to a DT property is yet
>>> another approach. I'd rather not do that, because it really isn't
>>> describing the hardware configuration. If anything, I'd expect the
>>> property to be describing which clks are enabled by the firmware and
>>> then leave the decision to disable them because they're unused up to the
>>> software.
>> After some more thinking, I realized that this could be made opt-in
>> simply with driver_data..
>>
>> WDYT?
> ..on a re-evaluation, obviously not a great idea.. Old DTBs will not
> be happy about that.
Another idea would be to yank out the not-very-useful "qcom,rpmcc"
fallback compatible and present .is_enabled etc. when it's absent..
Directly checking for the interconnect handle to rpmcc is not possible,
as interconnect requires rpmcc.. And then somebody's interconnect
driver may not be "good enough" (like 8996 and pre-6.3 DTs).
Konrad
>
> Konrad
>>
>> Konrad
On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote:
> Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all
> (or at least most) of the oneline peripherals ask the interconnect
> framework to keep their buses online and guarantee enough bandwidth,
> we're relying on bootloader defaults to keep the said buses alive through
> RPM requests and rate setting on RPM clocks.
>
> Without that in place, the RPM clocks are never enabled in the CCF, which
> qualifies them to be cleaned up, since - as far as Linux is concerned -
> nobody's using them and they're just wasting power. Doing so will end
> tragically, as within miliseconds we'll get *some* access attempt on an
> unlocked bus which will cause a platform crash.
>
> On the other hand, if we want to save power and put well-supported
> platforms to sleep, we should be shutting off at least some of these
> clocks (this time with a clear distinction of which ones are *actually*
> not in use, coming from the interconnect driver).
>
> To differentiate between these two cases while not breaking older DTs,
> introduce an opt-in property to correctly mark RPM clocks as enabled
> after handoff (the initial max freq vote) and hence qualify them for the
> common unused clock cleanup.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
> index 2a95bf8664f9..386153f61971 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
> @@ -58,6 +58,12 @@ properties:
> minItems: 1
> maxItems: 2
>
> + qcom,clk-disable-unused:
> + type: boolean
> + description:
> + Indicates whether unused RPM clocks can be shut down with the common
> + unused clock cleanup. Requires a functional interconnect driver.
> +
I'm surprised that Stephen Boyd did not bring up his usual "rant" here
of moving the interconnect clock voting out of rpmcc into the
interconnect drivers (see [1], [2]). :-)
I was a bit "cautious" about it back then but at this point I think it
kind of makes sense. Make sure to read Stephen's detailed explanation in
https://lore.kernel.org/linux-arm-msm/159796605593.334488.8355244657387381953@swboyd.mtv.corp.google.com/
We keep looking for workarounds to prevent the CCF from "messing" with
interconnect-related clocks. But the CCF cannot mess with "clocks" it
does not manage. The RPM interconnect drivers already talk directly to
the RPM in drivers/interconnect/qcom/smd-rpm.c. I think it should be
quite easy to move the QCOM_SMD_RPM_BUS_CLK relates defines over there
and just bypass the CCF entirely.
For backwards compatibility (for platforms without interconnect drivers)
one could either assume that the bootloader bandwidth votes will be
sufficient and just leave those clocks completely alone. Or the
"icc_smd_rpm" platform device could initially make max votes similar to
the rpmcc device. By coincidence the "icc_smd_rpm" platform device is
always created, no matter how the device tree looks or if the platform
actually has an interconnect driver.
Stephan
[1]: https://lore.kernel.org/linux-arm-msm/159796605593.334488.8355244657387381953@swboyd.mtv.corp.google.com/
[2]: https://lore.kernel.org/linux-arm-msm/[email protected]/
Quoting Stephan Gerhold (2023-04-17 12:05:06)
> On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote:
> > Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all
> > (or at least most) of the oneline peripherals ask the interconnect
> > framework to keep their buses online and guarantee enough bandwidth,
> > we're relying on bootloader defaults to keep the said buses alive through
> > RPM requests and rate setting on RPM clocks.
> >
> > Without that in place, the RPM clocks are never enabled in the CCF, which
> > qualifies them to be cleaned up, since - as far as Linux is concerned -
> > nobody's using them and they're just wasting power. Doing so will end
> > tragically, as within miliseconds we'll get *some* access attempt on an
> > unlocked bus which will cause a platform crash.
> >
> > On the other hand, if we want to save power and put well-supported
> > platforms to sleep, we should be shutting off at least some of these
> > clocks (this time with a clear distinction of which ones are *actually*
> > not in use, coming from the interconnect driver).
> >
> > To differentiate between these two cases while not breaking older DTs,
> > introduce an opt-in property to correctly mark RPM clocks as enabled
> > after handoff (the initial max freq vote) and hence qualify them for the
> > common unused clock cleanup.
> >
> > Signed-off-by: Konrad Dybcio <[email protected]>
> > ---
> > Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
> > index 2a95bf8664f9..386153f61971 100644
> > --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
> > +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
> > @@ -58,6 +58,12 @@ properties:
> > minItems: 1
> > maxItems: 2
> >
> > + qcom,clk-disable-unused:
> > + type: boolean
> > + description:
> > + Indicates whether unused RPM clocks can be shut down with the common
> > + unused clock cleanup. Requires a functional interconnect driver.
> > +
>
> I'm surprised that Stephen Boyd did not bring up his usual "rant" here
> of moving the interconnect clock voting out of rpmcc into the
> interconnect drivers (see [1], [2]). :-)
:) I was hoping to get a fix for disabling unused clks during late init
at the same time. Shucks!
>
> I was a bit "cautious" about it back then but at this point I think it
> kind of makes sense. Make sure to read Stephen's detailed explanation in
> https://lore.kernel.org/linux-arm-msm/159796605593.334488.8355244657387381953@swboyd.mtv.corp.google.com/
>
> We keep looking for workarounds to prevent the CCF from "messing" with
> interconnect-related clocks. But the CCF cannot mess with "clocks" it
> does not manage. The RPM interconnect drivers already talk directly to
> the RPM in drivers/interconnect/qcom/smd-rpm.c. I think it should be
> quite easy to move the QCOM_SMD_RPM_BUS_CLK relates defines over there
> and just bypass the CCF entirely.
Please do it!
>
> For backwards compatibility (for platforms without interconnect drivers)
> one could either assume that the bootloader bandwidth votes will be
> sufficient and just leave those clocks completely alone. Or the
> "icc_smd_rpm" platform device could initially make max votes similar to
> the rpmcc device. By coincidence the "icc_smd_rpm" platform device is
> always created, no matter how the device tree looks or if the platform
> actually has an interconnect driver.
>
Yeah that's a good plan. Suspend will be broken or burn a lot of power,
but presumably the new DTB will be used fairly quickly. Or you can
implement something like clkdev for interconnects that lets you hack up
an association between interconnects and consumers for existing DTs and
then drop those lookups months later.
On 18.04.2023 02:19, Stephen Boyd wrote:
> Quoting Stephan Gerhold (2023-04-17 12:05:06)
>> On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote:
>>> Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all
>>> (or at least most) of the oneline peripherals ask the interconnect
>>> framework to keep their buses online and guarantee enough bandwidth,
>>> we're relying on bootloader defaults to keep the said buses alive through
>>> RPM requests and rate setting on RPM clocks.
>>>
>>> Without that in place, the RPM clocks are never enabled in the CCF, which
>>> qualifies them to be cleaned up, since - as far as Linux is concerned -
>>> nobody's using them and they're just wasting power. Doing so will end
>>> tragically, as within miliseconds we'll get *some* access attempt on an
>>> unlocked bus which will cause a platform crash.
>>>
>>> On the other hand, if we want to save power and put well-supported
>>> platforms to sleep, we should be shutting off at least some of these
>>> clocks (this time with a clear distinction of which ones are *actually*
>>> not in use, coming from the interconnect driver).
>>>
>>> To differentiate between these two cases while not breaking older DTs,
>>> introduce an opt-in property to correctly mark RPM clocks as enabled
>>> after handoff (the initial max freq vote) and hence qualify them for the
>>> common unused clock cleanup.
>>>
>>> Signed-off-by: Konrad Dybcio <[email protected]>
>>> ---
>>> Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
>>> index 2a95bf8664f9..386153f61971 100644
>>> --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
>>> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
>>> @@ -58,6 +58,12 @@ properties:
>>> minItems: 1
>>> maxItems: 2
>>>
>>> + qcom,clk-disable-unused:
>>> + type: boolean
>>> + description:
>>> + Indicates whether unused RPM clocks can be shut down with the common
>>> + unused clock cleanup. Requires a functional interconnect driver.
>>> +
>>
>> I'm surprised that Stephen Boyd did not bring up his usual "rant" here
>> of moving the interconnect clock voting out of rpmcc into the
>> interconnect drivers (see [1], [2]). :-)
>
> :) I was hoping to get a fix for disabling unused clks during late init
> at the same time. Shucks!
>
>>
>> I was a bit "cautious" about it back then but at this point I think it
>> kind of makes sense. Make sure to read Stephen's detailed explanation in
>> https://lore.kernel.org/linux-arm-msm/159796605593.334488.8355244657387381953@swboyd.mtv.corp.google.com/
>>
>> We keep looking for workarounds to prevent the CCF from "messing" with
>> interconnect-related clocks. But the CCF cannot mess with "clocks" it
>> does not manage. The RPM interconnect drivers already talk directly to
>> the RPM in drivers/interconnect/qcom/smd-rpm.c. I think it should be
>> quite easy to move the QCOM_SMD_RPM_BUS_CLK relates defines over there
>> and just bypass the CCF entirely.
>
> Please do it!
Okay, that's a plan..
>
>>
>> For backwards compatibility (for platforms without interconnect drivers)
>> one could either assume that the bootloader bandwidth votes will be
>> sufficient and just leave those clocks completely alone. Or the
>> "icc_smd_rpm" platform device could initially make max votes similar to
>> the rpmcc device. By coincidence the "icc_smd_rpm" platform device is
>> always created, no matter how the device tree looks or if the platform
>> actually has an interconnect driver.
>>
>
> Yeah that's a good plan. Suspend will be broken or burn a lot of power,
(that's what happens as of today, so sgtm!)
> but presumably the new DTB will be used fairly quickly. Or you can
> implement something like clkdev for interconnects that lets you hack up
> an association between interconnects and consumers for existing DTs and
> then drop those lookups months later.
Uh.. let's not.. Let's just contain it in the interconnect driver.
The buses will be at bearable frequencies coming from the bootloader
(as RPM, storage etc. are enabled) and boosting them at icc_rpm_smd
probe sounds sane.
Konrad
On 18.04.2023 12:33, Konrad Dybcio wrote:
>
>
> On 18.04.2023 02:19, Stephen Boyd wrote:
>> Quoting Stephan Gerhold (2023-04-17 12:05:06)
>>> On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote:
>>>> Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all
>>>> (or at least most) of the oneline peripherals ask the interconnect
>>>> framework to keep their buses online and guarantee enough bandwidth,
>>>> we're relying on bootloader defaults to keep the said buses alive through
>>>> RPM requests and rate setting on RPM clocks.
>>>>
>>>> Without that in place, the RPM clocks are never enabled in the CCF, which
>>>> qualifies them to be cleaned up, since - as far as Linux is concerned -
>>>> nobody's using them and they're just wasting power. Doing so will end
>>>> tragically, as within miliseconds we'll get *some* access attempt on an
>>>> unlocked bus which will cause a platform crash.
>>>>
>>>> On the other hand, if we want to save power and put well-supported
>>>> platforms to sleep, we should be shutting off at least some of these
>>>> clocks (this time with a clear distinction of which ones are *actually*
>>>> not in use, coming from the interconnect driver).
>>>>
>>>> To differentiate between these two cases while not breaking older DTs,
>>>> introduce an opt-in property to correctly mark RPM clocks as enabled
>>>> after handoff (the initial max freq vote) and hence qualify them for the
>>>> common unused clock cleanup.
>>>>
>>>> Signed-off-by: Konrad Dybcio <[email protected]>
>>>> ---
>>>> Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++
>>>> 1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
>>>> index 2a95bf8664f9..386153f61971 100644
>>>> --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
>>>> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
>>>> @@ -58,6 +58,12 @@ properties:
>>>> minItems: 1
>>>> maxItems: 2
>>>>
>>>> + qcom,clk-disable-unused:
>>>> + type: boolean
>>>> + description:
>>>> + Indicates whether unused RPM clocks can be shut down with the common
>>>> + unused clock cleanup. Requires a functional interconnect driver.
>>>> +
>>>
>>> I'm surprised that Stephen Boyd did not bring up his usual "rant" here
>>> of moving the interconnect clock voting out of rpmcc into the
>>> interconnect drivers (see [1], [2]). :-)
>>
>> :) I was hoping to get a fix for disabling unused clks during late init
>> at the same time. Shucks!
>>
>>>
>>> I was a bit "cautious" about it back then but at this point I think it
>>> kind of makes sense. Make sure to read Stephen's detailed explanation in
>>> https://lore.kernel.org/linux-arm-msm/159796605593.334488.8355244657387381953@swboyd.mtv.corp.google.com/
>>>
>>> We keep looking for workarounds to prevent the CCF from "messing" with
>>> interconnect-related clocks. But the CCF cannot mess with "clocks" it
>>> does not manage. The RPM interconnect drivers already talk directly to
>>> the RPM in drivers/interconnect/qcom/smd-rpm.c. I think it should be
>>> quite easy to move the QCOM_SMD_RPM_BUS_CLK relates defines over there
>>> and just bypass the CCF entirely.
>>
>> Please do it!
> Okay, that's a plan..
>
>>
>>>
>>> For backwards compatibility (for platforms without interconnect drivers)
>>> one could either assume that the bootloader bandwidth votes will be
>>> sufficient and just leave those clocks completely alone. Or the
>>> "icc_smd_rpm" platform device could initially make max votes similar to
>>> the rpmcc device. By coincidence the "icc_smd_rpm" platform device is
>>> always created, no matter how the device tree looks or if the platform
>>> actually has an interconnect driver.
>>>
>>
>> Yeah that's a good plan. Suspend will be broken or burn a lot of power,
> (that's what happens as of today, so sgtm!)
>
>> but presumably the new DTB will be used fairly quickly. Or you can
>> implement something like clkdev for interconnects that lets you hack up
>> an association between interconnects and consumers for existing DTs and
>> then drop those lookups months later.
> Uh.. let's not.. Let's just contain it in the interconnect driver.
>
> The buses will be at bearable frequencies coming from the bootloader
> (as RPM, storage etc. are enabled) and boosting them at icc_rpm_smd
> probe sounds sane.
What should we do about the non-bus RPM clocks though? I don't fancy
IPA_CLK running 24/7.. And Stephan Gerhold was able to achieve VDD_MIN
on msm8909 with these clocks shut down (albeit with a very basic dt setup)!
Taking into account the old interconnect-enabled DTs, some of the
clocks would need to be on so that the QoS writes can succeed
(e.g. the MAS_IPA endpoint needs IPA_CLK), it gets complicated again..
I suppose something like this would work-ish:
0. remove clock handles as they're now contained within icc and
use them as a "legacy marker"
1. add:
if (qp->bus_clocks)
// skip qos writes
This will:
- let us add is_enabled so that all RPM clocks bar XO_A will be cleaned up
- save massively on code complexity
at the cost of retroactively removing features (QoS settings) for people
with old DTs and new kernels (don't tell Torvalds!)
This DTB ABI stuff really gets in the way sometimes :/ We're only now
fixing up U-Boot to be able to use upstream Linux DTs and other than
that I think only OpenBSD uses it with 8280.. Wish we could get rid of
all old junk once and then establish immutability but oh well..
Konrad
>
> Konrad
On Wed, Apr 19, 2023 at 01:31:01PM +0200, Konrad Dybcio wrote:
> What should we do about the non-bus RPM clocks though? I don't fancy
> IPA_CLK running 24/7.. And Stephan Gerhold was able to achieve VDD_MIN
> on msm8909 with these clocks shut down (albeit with a very basic dt setup)!
>
> Taking into account the old interconnect-enabled DTs, some of the
> clocks would need to be on so that the QoS writes can succeed
> (e.g. the MAS_IPA endpoint needs IPA_CLK), it gets complicated again..
>
I guess MSM8996 is the only platform affected by this? sdm630.dtsi seems
to list the clock already in the a2noc and all others don't seem to have
an interconnect driver yet.
This will be subjective and someone will surely disagree but...
IMO forcing all RPM clocks on during boot and keeping them enabled is
not part of the DT ABI. If you don't describe the hardware correctly and
are missing necessary clocks in the description (like the IPA_CLK on the
interconnect node) then your DT is wrong and should be fixed.
I would see this a bit like typical optimizing C compilers nowadays. If
you write correct code it can optimize, e.g. drop unnecessary function
calls. But if you write incorrect code with undefined behavior it's not
the fault of the compiler if you run into trouble. The code must be
fixed.
The DT bindings don't specify that unused resources (clocks, ...) stay
"magically" active. They specify that that the resources you reference
are available. As such, I would say the OS is free to optimize here and
turn off unused resources.
The more important point IMO is not breaking all platforms without
interconnect drivers. This goes beyond just adding a missing clock to
the DT, you need to write the driver first. But having the max vote in
icc_smd_rpm (somehow) should hopefully take care of that.
> I suppose something like this would work-ish:
>
> 0. remove clock handles as they're now contained within icc and
> use them as a "legacy marker"
> 1. add:
> if (qp->bus_clocks)
> // skip qos writes
Maybe you can just check if all necessary clocks for QOS are there or
not? I don't think it's a problem to skip it on broken DTs. I think it
would be even fine to refuse loading the interconnect driver completely
and just have the standard max vote (as long as that results in a
booting system).
>
> This will:
> - let us add is_enabled so that all RPM clocks bar XO_A will be cleaned up
> - save massively on code complexity
>
+1
> at the cost of retroactively removing features (QoS settings) for people
> with old DTs and new kernels (don't tell Torvalds!)
>
I doubt anyone will notice :p
> This DTB ABI stuff really gets in the way sometimes :/ We're only now
> fixing up U-Boot to be able to use upstream Linux DTs and other than
> that I think only OpenBSD uses it with 8280.. Wish we could get rid of
> all old junk once and then establish immutability but oh well..
Nice, thanks a lot for working on addressing the Qualcomm DT mess in
U-Boot. I've been meaning to work this myself for a long time but never
found the time to start... :')
Thanks,
Stephan
On 19.04.2023 16:00, Stephan Gerhold wrote:
> On Wed, Apr 19, 2023 at 01:31:01PM +0200, Konrad Dybcio wrote:
>> What should we do about the non-bus RPM clocks though? I don't fancy
>> IPA_CLK running 24/7.. And Stephan Gerhold was able to achieve VDD_MIN
>> on msm8909 with these clocks shut down (albeit with a very basic dt setup)!
>>
>> Taking into account the old interconnect-enabled DTs, some of the
>> clocks would need to be on so that the QoS writes can succeed
>> (e.g. the MAS_IPA endpoint needs IPA_CLK), it gets complicated again..
>>
>
> I guess MSM8996 is the only platform affected by this? sdm630.dtsi seems
> to list the clock already in the a2noc and all others don't seem to have
> an interconnect driver yet.
>
> This will be subjective and someone will surely disagree but...
>
> IMO forcing all RPM clocks on during boot and keeping them enabled is
> not part of the DT ABI. If you don't describe the hardware correctly and
> are missing necessary clocks in the description (like the IPA_CLK on the
> interconnect node) then your DT is wrong and should be fixed.
>
> I would see this a bit like typical optimizing C compilers nowadays. If
> you write correct code it can optimize, e.g. drop unnecessary function
> calls. But if you write incorrect code with undefined behavior it's not
> the fault of the compiler if you run into trouble. The code must be
> fixed.
>
> The DT bindings don't specify that unused resources (clocks, ...) stay
> "magically" active. They specify that that the resources you reference
> are available. As such, I would say the OS is free to optimize here and
> turn off unused resources.
>
> The more important point IMO is not breaking all platforms without
> interconnect drivers. This goes beyond just adding a missing clock to
> the DT, you need to write the driver first. But having the max vote in
> icc_smd_rpm (somehow) should hopefully take care of that.
Hm, interesting argument.
Krzysztof, Bjorn, what's your stance on this?
We *need* to add unused cleanup to rpmcc for feature completion and
there's no good way of discerning whether it's safe to do so..
Doing so will make clk_ignore_unused necessary to boot with legacy DTs.
Stephan argues the DTs were incomplete from the start and the breakage
is only a result of us previously abusing what's essentially undefined
behavior.. I think I second this, but it is *a* breakage so I want to
know your opinion.
FWIW the same happens when we have simple-framebuffer enabled and then
introduce dispcc on a given platform without adding the clocks under
the simplefb node and we've not been frowning upon that too much, so I'd
be willing to give it a pass if you're okay with it..
Not caring about this would make things far, far easier really..
Konrad
>
>> I suppose something like this would work-ish:
>>
>> 0. remove clock handles as they're now contained within icc and
>> use them as a "legacy marker"
>> 1. add:
>> if (qp->bus_clocks)
>> // skip qos writes
>
> Maybe you can just check if all necessary clocks for QOS are there or
> not? I don't think it's a problem to skip it on broken DTs. I think it
> would be even fine to refuse loading the interconnect driver completely
> and just have the standard max vote (as long as that results in a
> booting system).
>
>>
>> This will:
>> - let us add is_enabled so that all RPM clocks bar XO_A will be cleaned up
>> - save massively on code complexity
>>
>
> +1
>
>> at the cost of retroactively removing features (QoS settings) for people
>> with old DTs and new kernels (don't tell Torvalds!)
>>
>
> I doubt anyone will notice :p
>
>> This DTB ABI stuff really gets in the way sometimes :/ We're only now
>> fixing up U-Boot to be able to use upstream Linux DTs and other than
>> that I think only OpenBSD uses it with 8280.. Wish we could get rid of
>> all old junk once and then establish immutability but oh well..
>
> Nice, thanks a lot for working on addressing the Qualcomm DT mess in
> U-Boot. I've been meaning to work this myself for a long time but never
> found the time to start... :')
>
> Thanks,
> Stephan
On 8.03.2023 22:35, Konrad Dybcio wrote:
> v1 -> v2:
> - Use CLK_IS_CRITICAL instead of leaving a clk enable vote, expand macros
> to do so
> - Fix the keepalive clocks for 8998 & 660 (CNoC -> PNoC, it was
> confusingly named cnoc_periph downstream)
> - Introduce .determinte_rate to ensure we don't set keepalive clocks'
> rates below 19.2 MHz
> - Add a (!conditional!) way to test the ultimate goal of all these changes
> by essentially enabling unused clk cleanup through a dt property (for
> legacy reasons)
>
> v2 was tested on:
>
> - MSM8996 Sony Kagura (can disable unused)
> - MSM8998 Sony Maple (can disable unused with OOT icc)
> - SM6375 Sony PDX225 (can disable unused with OOT icc)
>
> v1: https://lore.kernel.org/r/[email protected]
>
> This series brings support for a couple of things necessary for the full
> system idle on SMD RPM SoCs, namely unused clk shutdown and keepalive
> votes (permanent active votes that are required on certain clocks for the
> platform to function).
>
> Tested on MSM8996 and SM6375, does not seem to introduce any additional
> regressions.
>
> Keepalive clocks for other platforms were gathered by digging in old
> downstream kernels, please give them a test.
I have an implementation of rpmcc-within-icc ready(ish) locally. Turns out
some SoCs need a keepalive (19.2MHz, active-only) vote on clocks that
are NOT governed by interconnect.. So before we can disable clocks,
both will need to be implemented.. ugh... I was hoping we could avoid
having it in rpmcc..
Konrad
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> Konrad Dybcio (11):
> dt-bindings: clock: qcom,rpmcc: Add a way to enable unused clock cleanup
> clk: qcom: smd-rpm_ Make __DEFINE_CLK_SMD_RPM_BRANCH_PREFIX accept flags
> clk: qcom: smd-rpm: Make DEFINE_CLK_SMD_RPM_BRANCH_A accept flags
> clk: qcom: smd-rpm: Make BI_TCXO_AO critical
> clk: qcom: smd-rpm: Make __DEFINE_CLK_SMD_RPM_PREFIX accept flags
> clk: qcom: smd-rpm: Separate out a macro for defining an AO clock
> clk: qcom: smd-rpm: Add support for keepalive votes
> clk: qcom: smd-rpm: Introduce DEFINE_CLK_SMD_RPM_BUS_KEEPALIVE
> clk: qcom: smd-rpm: Hook up PCNoC_0 keep_alive
> clk: qcom: smd-rpm: Hook up CNoC_1 and SNoC_2 keep_alive
> arm64: dts: qcom: msm8996: Enable rpmcc unused clk disablement
>
> Shawn Guo (3):
> clk: qcom: smd-rpm: Add .is_enabled hook
> clk: qcom: smd-rpm: Add .is_prepared hook
> clk: qcom: smd-rpm: Mark clock enabled in clk_smd_rpm_handoff()
>
> .../devicetree/bindings/clock/qcom,rpmcc.yaml | 6 +
> arch/arm64/boot/dts/qcom/msm8996.dtsi | 1 +
> drivers/clk/qcom/clk-smd-rpm.c | 133 +++++++++++++++------
> 3 files changed, 106 insertions(+), 34 deletions(-)
> ---
> base-commit: fc31900c948610e7b5c2f15fb7795832c8325327
> change-id: 20230303-topic-rpmcc_sleep-d67aad9f3012
>
> Best regards,
On Thu, Apr 20, 2023 at 03:50:16AM +0200, Konrad Dybcio wrote:
> On 8.03.2023 22:35, Konrad Dybcio wrote:
> > Keepalive clocks for other platforms were gathered by digging in old
> > downstream kernels, please give them a test.
> I have an implementation of rpmcc-within-icc ready(ish) locally. Turns out
> some SoCs need a keepalive (19.2MHz, active-only) vote on clocks that
> are NOT governed by interconnect.. So before we can disable clocks,
> both will need to be implemented.. ugh... I was hoping we could avoid
> having it in rpmcc..
>
Can you give an example? Which clocks are affected on which SoC?
Thanks,
Stephan
On Wed, Mar 08, 2023 at 10:35:17PM +0100, Konrad Dybcio wrote:
> Disabling RPMCC clocks can be a bit touchy. If we can't guarantee all
> (or at least most) of the oneline peripherals ask the interconnect
> framework to keep their buses online and guarantee enough bandwidth,
> we're relying on bootloader defaults to keep the said buses alive through
> RPM requests and rate setting on RPM clocks.
>
> Without that in place, the RPM clocks are never enabled in the CCF, which
> qualifies them to be cleaned up, since - as far as Linux is concerned -
> nobody's using them and they're just wasting power. Doing so will end
> tragically, as within miliseconds we'll get *some* access attempt on an
> unlocked bus which will cause a platform crash.
>
> On the other hand, if we want to save power and put well-supported
> platforms to sleep, we should be shutting off at least some of these
> clocks (this time with a clear distinction of which ones are *actually*
> not in use, coming from the interconnect driver).
>
> To differentiate between these two cases while not breaking older DTs,
> introduce an opt-in property to correctly mark RPM clocks as enabled
> after handoff (the initial max freq vote) and hence qualify them for the
> common unused clock cleanup.
>
My 2 cents here...
First, this property doesn't belong in DT at all as it is OS specific handling.
This leaves us with the option of using a cmdline or module params for rmpcc.
But we already have one (clk_ignore_unused), so the platforms making use of old
DTB's should use that instead.
And that get's rid of the debate that when you start disabling rpmcc clocks, old
platforms will break. I don't see a valid point to keep the old platforms alive
since their DTB (firmware) is broken already. So either they have to fix the DTB
or use a cmdline option.
- Mani
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
> index 2a95bf8664f9..386153f61971 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml
> @@ -58,6 +58,12 @@ properties:
> minItems: 1
> maxItems: 2
>
> + qcom,clk-disable-unused:
> + type: boolean
> + description:
> + Indicates whether unused RPM clocks can be shut down with the common
> + unused clock cleanup. Requires a functional interconnect driver.
> +
> required:
> - compatible
> - '#clock-cells'
>
> --
> 2.39.2
>
--
மணிவண்ணன் சதாசிவம்
On 20.04.2023 09:56, Stephan Gerhold wrote:
> On Thu, Apr 20, 2023 at 03:50:16AM +0200, Konrad Dybcio wrote:
>> On 8.03.2023 22:35, Konrad Dybcio wrote:
>>> Keepalive clocks for other platforms were gathered by digging in old
>>> downstream kernels, please give them a test.
>> I have an implementation of rpmcc-within-icc ready(ish) locally. Turns out
>> some SoCs need a keepalive (19.2MHz, active-only) vote on clocks that
>> are NOT governed by interconnect.. So before we can disable clocks,
>> both will need to be implemented.. ugh... I was hoping we could avoid
>> having it in rpmcc..
>>
>
> Can you give an example? Which clocks are affected on which SoC?
msm8998/sdm660 and PNoC
Konrad
>
> Thanks,
> Stephan
On Thu, Apr 20, 2023 at 11:36:24AM +0200, Konrad Dybcio wrote:
> On 20.04.2023 09:56, Stephan Gerhold wrote:
> > On Thu, Apr 20, 2023 at 03:50:16AM +0200, Konrad Dybcio wrote:
> >> On 8.03.2023 22:35, Konrad Dybcio wrote:
> >>> Keepalive clocks for other platforms were gathered by digging in old
> >>> downstream kernels, please give them a test.
> >> I have an implementation of rpmcc-within-icc ready(ish) locally. Turns out
> >> some SoCs need a keepalive (19.2MHz, active-only) vote on clocks that
> >> are NOT governed by interconnect.. So before we can disable clocks,
> >> both will need to be implemented.. ugh... I was hoping we could avoid
> >> having it in rpmcc..
> > Can you give an example? Which clocks are affected on which SoC?
> msm8998/sdm660 and PNoC
I don't see a PNoC for 8998/660, do you mean the "cnoc_periph_clk"
downstream? Like the other NoCs it seems to be a RPM_BUS_CLK_TYPE, which
means it does fit best into interconnect in my opinion. From a quick
grep I don't see any usage of it in msm-4.4 downstream other than the
active-only keepalive vote. So maybe you could just send that vote once
in icc_rpm_smd and then ignore that clock (don't expose it at all)?
Thanks,
Stephan
On 20.04.2023 12:04, Stephan Gerhold wrote:
> On Thu, Apr 20, 2023 at 11:36:24AM +0200, Konrad Dybcio wrote:
>> On 20.04.2023 09:56, Stephan Gerhold wrote:
>>> On Thu, Apr 20, 2023 at 03:50:16AM +0200, Konrad Dybcio wrote:
>>>> On 8.03.2023 22:35, Konrad Dybcio wrote:
>>>>> Keepalive clocks for other platforms were gathered by digging in old
>>>>> downstream kernels, please give them a test.
>>>> I have an implementation of rpmcc-within-icc ready(ish) locally. Turns out
>>>> some SoCs need a keepalive (19.2MHz, active-only) vote on clocks that
>>>> are NOT governed by interconnect.. So before we can disable clocks,
>>>> both will need to be implemented.. ugh... I was hoping we could avoid
>>>> having it in rpmcc..
>>> Can you give an example? Which clocks are affected on which SoC?
>> msm8998/sdm660 and PNoC
>
> I don't see a PNoC for 8998/660, do you mean the "cnoc_periph_clk"
It's the same, but Qualcomm kept changing the name every kernel
release, so that's why we have 50 defines for the same thing
upstream :(
> downstream? Like the other NoCs it seems to be a RPM_BUS_CLK_TYPE, which
> means it does fit best into interconnect in my opinion. From a quick
> grep I don't see any usage of it in msm-4.4 downstream other than the
> active-only keepalive vote. So maybe you could just send that vote once
> in icc_rpm_smd and then ignore that clock (don't expose it at all)?
Hm, perhaps that does sound like a good idea! As far as I understand,
it's governed internally.. Older SoCs had a separate PNoC fabric
exposed.
Konrad
>
> Thanks,
> Stephan
On 8.03.2023 22:35, Konrad Dybcio wrote:
> v1 -> v2:
> - Use CLK_IS_CRITICAL instead of leaving a clk enable vote, expand macros
> to do so
> - Fix the keepalive clocks for 8998 & 660 (CNoC -> PNoC, it was
> confusingly named cnoc_periph downstream)
> - Introduce .determinte_rate to ensure we don't set keepalive clocks'
> rates below 19.2 MHz
> - Add a (!conditional!) way to test the ultimate goal of all these changes
> by essentially enabling unused clk cleanup through a dt property (for
> legacy reasons)
>
> v2 was tested on:
>
> - MSM8996 Sony Kagura (can disable unused)
> - MSM8998 Sony Maple (can disable unused with OOT icc)
> - SM6375 Sony PDX225 (can disable unused with OOT icc)
>
> v1: https://lore.kernel.org/r/[email protected]
>
> This series brings support for a couple of things necessary for the full
> system idle on SMD RPM SoCs, namely unused clk shutdown and keepalive
> votes (permanent active votes that are required on certain clocks for the
> platform to function).
>
> Tested on MSM8996 and SM6375, does not seem to introduce any additional
> regressions.
>
> Keepalive clocks for other platforms were gathered by digging in old
> downstream kernels, please give them a test.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> Konrad Dybcio (11):
> dt-bindings: clock: qcom,rpmcc: Add a way to enable unused clock cleanup
> clk: qcom: smd-rpm_ Make __DEFINE_CLK_SMD_RPM_BRANCH_PREFIX accept flags
> clk: qcom: smd-rpm: Make DEFINE_CLK_SMD_RPM_BRANCH_A accept flags
> clk: qcom: smd-rpm: Make BI_TCXO_AO critical
Stephen, parallel to all of the discussions, would you be willing to
take patches 4-6 as they are? XO_A being critical is something that
won't hurt without the rest.
Konrad
> clk: qcom: smd-rpm: Make __DEFINE_CLK_SMD_RPM_PREFIX accept flags
> clk: qcom: smd-rpm: Separate out a macro for defining an AO clock
> clk: qcom: smd-rpm: Add support for keepalive votes
> clk: qcom: smd-rpm: Introduce DEFINE_CLK_SMD_RPM_BUS_KEEPALIVE
> clk: qcom: smd-rpm: Hook up PCNoC_0 keep_alive
> clk: qcom: smd-rpm: Hook up CNoC_1 and SNoC_2 keep_alive
> arm64: dts: qcom: msm8996: Enable rpmcc unused clk disablement
>
> Shawn Guo (3):
> clk: qcom: smd-rpm: Add .is_enabled hook
> clk: qcom: smd-rpm: Add .is_prepared hook
> clk: qcom: smd-rpm: Mark clock enabled in clk_smd_rpm_handoff()
>
> .../devicetree/bindings/clock/qcom,rpmcc.yaml | 6 +
> arch/arm64/boot/dts/qcom/msm8996.dtsi | 1 +
> drivers/clk/qcom/clk-smd-rpm.c | 133 +++++++++++++++------
> 3 files changed, 106 insertions(+), 34 deletions(-)
> ---
> base-commit: fc31900c948610e7b5c2f15fb7795832c8325327
> change-id: 20230303-topic-rpmcc_sleep-d67aad9f3012
>
> Best regards,
Quoting Konrad Dybcio (2023-04-20 08:57:53)
>
>
> > Konrad Dybcio (11):
> > dt-bindings: clock: qcom,rpmcc: Add a way to enable unused clock cleanup
>
> > clk: qcom: smd-rpm_ Make __DEFINE_CLK_SMD_RPM_BRANCH_PREFIX accept flags
> > clk: qcom: smd-rpm: Make DEFINE_CLK_SMD_RPM_BRANCH_A accept flags
> > clk: qcom: smd-rpm: Make BI_TCXO_AO critical
> Stephen, parallel to all of the discussions, would you be willing to
> take patches 4-6 as they are? XO_A being critical is something that
> won't hurt without the rest.
Sure, can you resend just those in a series?
On 4/25/23 20:35, Stephen Boyd wrote:
> Quoting Konrad Dybcio (2023-04-20 08:57:53)
>>
>>> Konrad Dybcio (11):
>>> dt-bindings: clock: qcom,rpmcc: Add a way to enable unused clock cleanup
>>> clk: qcom: smd-rpm_ Make __DEFINE_CLK_SMD_RPM_BRANCH_PREFIX accept flags
>>> clk: qcom: smd-rpm: Make DEFINE_CLK_SMD_RPM_BRANCH_A accept flags
>>> clk: qcom: smd-rpm: Make BI_TCXO_AO critical
>> Stephen, parallel to all of the discussions, would you be willing to
>> take patches 4-6 as they are? XO_A being critical is something that
>> won't hurt without the rest.
> Sure, can you resend just those in a series?
Thanks, I'll do that after Connect!
Konrad