[RFC PATCH 0/2] clk: qcom: provide alternative 'parked' RCG

[RFC PATCH 0/2] clk: qcom: provide alternative 'parked' RCG

[Dmitry Baryshkov]

Implement an alternative for the clk_rcg2_shared_ops, which also
implements a proper is_enabled callback. Note, to use
clk_rcg2_parked_ops one must remove XO (safe source) from the
parent_data and parent_map.

Dmitry Baryshkov (2):
  clk: qcom: implement RCG2 'parked' clock support
  clk: qcom: dispcc-sm8250: switch to clk_rcg2_parked_ops

 drivers/clk/qcom/clk-rcg.h       |  1 +
 drivers/clk/qcom/clk-rcg2.c      | 34 ++++++++++++++++++++++++++++++++
 drivers/clk/qcom/dispcc-sm8250.c | 10 +++-------
 3 files changed, 38 insertions(+), 7 deletions(-)

-- 
2.39.2

[RFC PATCH 1/2] clk: qcom: implement RCG2 'parked' clock support

[Dmitry Baryshkov]

clk_rcg2_shared_ops implements support for the case of the RCG which
must not be completely turned off. However its design has one major
drawback: it doesn't allow us to properly implement the is_enabled
callback, which causes different kinds of misbehaviour from the CCF.

Follow the idea behind clk_regmap_phy_mux_ops and implement the new
clk_rcg2_parked_ops. It also targets the clocks which must not be fully
switched off (and shared most of the implementation with
clk_rcg2_shared_ops). The major difference is that it requires that the
parent map doesn't conain the safe (parked) clock source. Instead if the
CFG_REG register points to the safe source, the clock is considered to
be disabled.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-rcg.h  |  1 +
 drivers/clk/qcom/clk-rcg2.c | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
index e6d84c8c7989..9fbbf1251564 100644
--- a/drivers/clk/qcom/clk-rcg.h
+++ b/drivers/clk/qcom/clk-rcg.h
@@ -176,6 +176,7 @@ extern const struct clk_ops clk_byte2_ops;
 extern const struct clk_ops clk_pixel_ops;
 extern const struct clk_ops clk_gfx3d_ops;
 extern const struct clk_ops clk_rcg2_shared_ops;
+extern const struct clk_ops clk_rcg2_parked_ops;
 extern const struct clk_ops clk_dp_ops;
 
 struct clk_rcg_dfs_data {
diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index 5183c74b074f..3f52abf0025e 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -5,6 +5,7 @@
 
 #include <linux/kernel.h>
 #include <linux/bitops.h>
+#include <linux/bitfield.h>
 #include <linux/err.h>
 #include <linux/bug.h>
 #include <linux/export.h>
@@ -1150,6 +1151,39 @@ const struct clk_ops clk_rcg2_shared_ops = {
 };
 EXPORT_SYMBOL_GPL(clk_rcg2_shared_ops);
 
+static int clk_rcg2_park_is_enabled(struct clk_hw *hw)
+{
+	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
+	u32 cfg;
+	int ret;
+
+	if (!clk_rcg2_is_enabled(hw))
+		return false;
+
+	ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &cfg);
+	if (ret)
+		return ret;
+
+	return FIELD_GET(CFG_SRC_SEL_MASK, cfg) != rcg->safe_src_index;
+}
+
+/*
+ * Unlike clk_rcg2_shared_ops, the safe_src_index aka XO must NOT be present in
+ * parent_map. This allows us to implement proper is_enabled callback.
+ */
+const struct clk_ops clk_rcg2_parked_ops = {
+	.is_enabled = clk_rcg2_park_is_enabled,
+	.enable = clk_rcg2_shared_enable,
+	.disable = clk_rcg2_shared_disable,
+	.get_parent = clk_rcg2_shared_get_parent,
+	.set_parent = clk_rcg2_shared_set_parent,
+	.recalc_rate = clk_rcg2_shared_recalc_rate,
+	.determine_rate = clk_rcg2_determine_rate,
+	.set_rate = clk_rcg2_shared_set_rate,
+	.set_rate_and_parent = clk_rcg2_shared_set_rate_and_parent,
+};
+EXPORT_SYMBOL_GPL(clk_rcg2_parked_ops);
+
 /* Common APIs to be used for DFS based RCGR */
 static void clk_rcg2_dfs_populate_freq(struct clk_hw *hw, unsigned int l,
 				       struct freq_tbl *f)
-- 
2.39.2

[RFC PATCH 2/2] clk: qcom: dispcc-sm8250: switch to clk_rcg2_parked_ops

[Dmitry Baryshkov]

Switch MDP, AHB and ROT clocks to the clk_rcg2_parked_ops so that the
CCF can properly determine if the clock is enabled or disabled.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/dispcc-sm8250.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/qcom/dispcc-sm8250.c b/drivers/clk/qcom/dispcc-sm8250.c
index e17bb8b543b5..229d32aee431 100644
--- a/drivers/clk/qcom/dispcc-sm8250.c
+++ b/drivers/clk/qcom/dispcc-sm8250.c
@@ -144,12 +144,10 @@ static const struct clk_parent_data disp_cc_parent_data_2[] = {
 };
 
 static const struct parent_map disp_cc_parent_map_3[] = {
-	{ P_BI_TCXO, 0 },
 	{ P_DISP_CC_PLL1_OUT_MAIN, 4 },
 };
 
 static const struct clk_parent_data disp_cc_parent_data_3[] = {
-	{ .fw_name = "bi_tcxo" },
 	{ .hw = &disp_cc_pll1.clkr.hw },
 };
 
@@ -166,13 +164,11 @@ static const struct clk_parent_data disp_cc_parent_data_4[] = {
 };
 
 static const struct parent_map disp_cc_parent_map_5[] = {
-	{ P_BI_TCXO, 0 },
 	{ P_DISP_CC_PLL0_OUT_MAIN, 1 },
 	{ P_DISP_CC_PLL1_OUT_MAIN, 4 },
 };
 
 static const struct clk_parent_data disp_cc_parent_data_5[] = {
-	{ .fw_name = "bi_tcxo" },
 	{ .hw = &disp_cc_pll0.clkr.hw },
 	{ .hw = &disp_cc_pll1.clkr.hw },
 };
@@ -219,7 +215,7 @@ static struct clk_rcg2 disp_cc_mdss_ahb_clk_src = {
 		.parent_data = disp_cc_parent_data_3,
 		.num_parents = ARRAY_SIZE(disp_cc_parent_data_3),
 		.flags = CLK_SET_RATE_PARENT,
-		.ops = &clk_rcg2_shared_ops,
+		.ops = &clk_rcg2_parked_ops,
 	},
 };
 
@@ -565,7 +561,7 @@ static struct clk_rcg2 disp_cc_mdss_mdp_clk_src = {
 		.parent_data = disp_cc_parent_data_5,
 		.num_parents = ARRAY_SIZE(disp_cc_parent_data_5),
 		.flags = CLK_SET_RATE_PARENT,
-		.ops = &clk_rcg2_shared_ops,
+		.ops = &clk_rcg2_parked_ops,
 	},
 };
 
@@ -617,7 +613,7 @@ static struct clk_rcg2 disp_cc_mdss_rot_clk_src = {
 		.parent_data = disp_cc_parent_data_5,
 		.num_parents = ARRAY_SIZE(disp_cc_parent_data_5),
 		.flags = CLK_SET_RATE_PARENT,
-		.ops = &clk_rcg2_shared_ops,
+		.ops = &clk_rcg2_parked_ops,
 	},
 };
 
-- 
2.39.2

Re: [RFC PATCH 1/2] clk: qcom: implement RCG2 'parked' clock support

[Bryan O'Donoghue]

On 04/10/2023 01:31, Dmitry Baryshkov wrote:
> clk_rcg2_shared_ops implements support for the case of the RCG which
> must not be completely turned off. However its design has one major
> drawback: it doesn't allow us to properly implement the is_enabled
> callback, which causes different kinds of misbehaviour from the CCF.
> 
> Follow the idea behind clk_regmap_phy_mux_ops and implement the new
> clk_rcg2_parked_ops. It also targets the clocks which must not be fully
> switched off (and shared most of the implementation with
> clk_rcg2_shared_ops). The major difference is that it requires that the
> parent map doesn't conain the safe (parked) clock source. Instead if the
> CFG_REG register points to the safe source, the clock is considered to
> be disabled.

Why not have a new bit in .flags ?

Instead of lying about the clock being off, mark the clock as "parked", 
or "safe parked" or whatever term we choose for it ?

I feel 'disabled' should mean disabled and 'enabled' should me enabled 
when I read a value from debugfs and if we are parking a clock it should 
have a clear means of being flagged as a clock that should be parked.

An example. I recently inherited some autogenerated code for camcc on 
sc8280xp.

One of the clocks is marked as CLK_IS_CRITICAL by the autogen code 
meaning "never switch off" but CLK_IS_CRITICAL stops the camcc driver 
from doing runtime pm_ops to power collapse.

The solution I have is to remove CLK_IS_CRITICAL and to hard-code in 
probe() the clock to always on.

But if we had a CLK_DISABLE_SAFE_PARK flag - then not just for rcg but 
for branch clocks we could differentiate away from hard-coded always on 
in probe...

?

---
bod

Re: [RFC PATCH 1/2] clk: qcom: implement RCG2 'parked' clock support

[Dmitry Baryshkov]

On Wed, 4 Oct 2023 at 12:27, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> On 04/10/2023 01:31, Dmitry Baryshkov wrote:
> > clk_rcg2_shared_ops implements support for the case of the RCG which
> > must not be completely turned off. However its design has one major
> > drawback: it doesn't allow us to properly implement the is_enabled
> > callback, which causes different kinds of misbehaviour from the CCF.
> >
> > Follow the idea behind clk_regmap_phy_mux_ops and implement the new
> > clk_rcg2_parked_ops. It also targets the clocks which must not be fully
> > switched off (and shared most of the implementation with
> > clk_rcg2_shared_ops). The major difference is that it requires that the
> > parent map doesn't conain the safe (parked) clock source. Instead if the
> > CFG_REG register points to the safe source, the clock is considered to
> > be disabled.
>
> Why not have a new bit in .flags ?
>
> Instead of lying about the clock being off, mark the clock as "parked",
> or "safe parked" or whatever term we choose for it ?

The main problem with adding flags doesn't fully scale. From the CCF
perspective, what should be the difference between parked and disabled
clocks? How should it treat the parked one?

From my point of view, the 'parked' clock is as good as 'disabled',
because the end devices do not work with the corresponding clock being
sourced from XO.
We already have a similar implementation for the PCIe PIPE clocks,
which reinterpret physical 'XO' source as a 'disabled' state to
facilitate disabling the clock before turning the genpd off.

>
> I feel 'disabled' should mean disabled and 'enabled' should me enabled
> when I read a value from debugfs and if we are parking a clock it should
> have a clear means of being flagged as a clock that should be parked.
>
> An example. I recently inherited some autogenerated code for camcc on
> sc8280xp.
>
> One of the clocks is marked as CLK_IS_CRITICAL by the autogen code
> meaning "never switch off" but CLK_IS_CRITICAL stops the camcc driver
> from doing runtime pm_ops to power collapse.
>
> The solution I have is to remove CLK_IS_CRITICAL and to hard-code in
> probe() the clock to always on.
>
> But if we had a CLK_DISABLE_SAFE_PARK flag - then not just for rcg but
> for branch clocks we could differentiate away from hard-coded always on
> in probe...
>
> ?
>
> ---
> bod



--
With best wishes
Dmitry

Re: [RFC PATCH 1/2] clk: qcom: implement RCG2 'parked' clock support

[Bryan O'Donoghue]

On 04/10/2023 13:08, Dmitry Baryshkov wrote:
> On Wed, 4 Oct 2023 at 12:27, Bryan O'Donoghue
> <bryan.odonoghue@linaro.org> wrote:
>>
>> On 04/10/2023 01:31, Dmitry Baryshkov wrote:
>>> clk_rcg2_shared_ops implements support for the case of the RCG which
>>> must not be completely turned off. However its design has one major
>>> drawback: it doesn't allow us to properly implement the is_enabled
>>> callback, which causes different kinds of misbehaviour from the CCF.
>>>
>>> Follow the idea behind clk_regmap_phy_mux_ops and implement the new
>>> clk_rcg2_parked_ops. It also targets the clocks which must not be fully
>>> switched off (and shared most of the implementation with
>>> clk_rcg2_shared_ops). The major difference is that it requires that the
>>> parent map doesn't conain the safe (parked) clock source. Instead if the
>>> CFG_REG register points to the safe source, the clock is considered to
>>> be disabled.
>>
>> Why not have a new bit in .flags ?
>>
>> Instead of lying about the clock being off, mark the clock as "parked",
>> or "safe parked" or whatever term we choose for it ?
> 
> The main problem with adding flags doesn't fully scale. From the CCF
> perspective, what should be the difference between parked and disabled
> clocks? How should it treat the parked one?

Exactly the same as a disabled clock, except you get a "parked" instead 
of a "disabled" when looking up its state and you don't have to

-	{ .fw_name = "bi_tcxo" },

Also you can then flag for branch2 clocks the same thing - so parking 
would be done at a higher level in the CCF.

---
bod

Re: [RFC PATCH 1/2] clk: qcom: implement RCG2 'parked' clock support

[Dmitry Baryshkov]

On 04/10/2023 15:52, Bryan O'Donoghue wrote:
> On 04/10/2023 13:08, Dmitry Baryshkov wrote:
>> On Wed, 4 Oct 2023 at 12:27, Bryan O'Donoghue
>> <bryan.odonoghue@linaro.org> wrote:
>>>
>>> On 04/10/2023 01:31, Dmitry Baryshkov wrote:
>>>> clk_rcg2_shared_ops implements support for the case of the RCG which
>>>> must not be completely turned off. However its design has one major
>>>> drawback: it doesn't allow us to properly implement the is_enabled
>>>> callback, which causes different kinds of misbehaviour from the CCF.
>>>>
>>>> Follow the idea behind clk_regmap_phy_mux_ops and implement the new
>>>> clk_rcg2_parked_ops. It also targets the clocks which must not be fully
>>>> switched off (and shared most of the implementation with
>>>> clk_rcg2_shared_ops). The major difference is that it requires that the
>>>> parent map doesn't conain the safe (parked) clock source. Instead if 
>>>> the
>>>> CFG_REG register points to the safe source, the clock is considered to
>>>> be disabled.
>>>
>>> Why not have a new bit in .flags ?
>>>
>>> Instead of lying about the clock being off, mark the clock as "parked",
>>> or "safe parked" or whatever term we choose for it ?
>>
>> The main problem with adding flags doesn't fully scale. From the CCF
>> perspective, what should be the difference between parked and disabled
>> clocks? How should it treat the parked one?
> 
> Exactly the same as a disabled clock, except you get a "parked" instead 
> of a "disabled" when looking up its state and you don't have to
> 
> -    { .fw_name = "bi_tcxo" },
> 
> Also you can then flag for branch2 clocks the same thing - so parking 
> would be done at a higher level in the CCF.

Without this removal there is no easy way to identify if the clock is 
parked to XO or if it is reparented to that clock.


-- 
With best wishes
Dmitry

Re: [RFC PATCH 1/2] clk: qcom: implement RCG2 'parked' clock support

[Konrad Dybcio]

On 4.10.2023 02:31, Dmitry Baryshkov wrote:
> clk_rcg2_shared_ops implements support for the case of the RCG which
> must not be completely turned off. However its design has one major
> drawback: it doesn't allow us to properly implement the is_enabled
> callback, which causes different kinds of misbehaviour from the CCF.
> 
> Follow the idea behind clk_regmap_phy_mux_ops and implement the new
> clk_rcg2_parked_ops. It also targets the clocks which must not be fully
> switched off (and shared most of the implementation with
> clk_rcg2_shared_ops). The major difference is that it requires that the
> parent map doesn't conain the safe (parked) clock source. Instead if the
> CFG_REG register points to the safe source, the clock is considered to
> be disabled.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Would the intention here be to replace all usages of _shared_?

Konrad

Re: [RFC PATCH 1/2] clk: qcom: implement RCG2 'parked' clock support

[Konrad Dybcio]

On 4.10.2023 14:52, Bryan O'Donoghue wrote:
> On 04/10/2023 13:08, Dmitry Baryshkov wrote:
>> On Wed, 4 Oct 2023 at 12:27, Bryan O'Donoghue
>> <bryan.odonoghue@linaro.org> wrote:
>>>
>>> On 04/10/2023 01:31, Dmitry Baryshkov wrote:
>>>> clk_rcg2_shared_ops implements support for the case of the RCG which
>>>> must not be completely turned off. However its design has one major
>>>> drawback: it doesn't allow us to properly implement the is_enabled
>>>> callback, which causes different kinds of misbehaviour from the CCF.
>>>>
>>>> Follow the idea behind clk_regmap_phy_mux_ops and implement the new
>>>> clk_rcg2_parked_ops. It also targets the clocks which must not be fully
>>>> switched off (and shared most of the implementation with
>>>> clk_rcg2_shared_ops). The major difference is that it requires that the
>>>> parent map doesn't conain the safe (parked) clock source. Instead if the
>>>> CFG_REG register points to the safe source, the clock is considered to
>>>> be disabled.
>>>
>>> Why not have a new bit in .flags ?
>>>
>>> Instead of lying about the clock being off, mark the clock as "parked",
>>> or "safe parked" or whatever term we choose for it ?
>>
>> The main problem with adding flags doesn't fully scale. From the CCF
>> perspective, what should be the difference between parked and disabled
>> clocks? How should it treat the parked one?
> 
> Exactly the same as a disabled clock, except you get a "parked" instead of a "disabled" when looking up its state and you don't have to
The thing is that currently there's only the notion of "enabled"
or "not enabled".. Introducing a third state here would be the
jump from boolean to quantum logic!

I think that abstracting this information away from Linux is not
an issue.. These clocks "can't be any more off", or the SoC will
explode badly and Linux will be unusable..

Think of it like CPUs with a hypervisor, you shut them down, but
the physical number crunchers on the host CPU may not actually
get cut off from their power source, but there's no reason for
the VM to know that. That's probably what happens on our little
virtualized snapdragons anyway..

Konrad

Re: [RFC PATCH 1/2] clk: qcom: implement RCG2 'parked' clock support

[Bryan O'Donoghue]

On 07/10/2023 00:45, Konrad Dybcio wrote:
> On 4.10.2023 14:52, Bryan O'Donoghue wrote:
>> On 04/10/2023 13:08, Dmitry Baryshkov wrote:
>>> On Wed, 4 Oct 2023 at 12:27, Bryan O'Donoghue
>>> <bryan.odonoghue@linaro.org> wrote:
>>>>
>>>> On 04/10/2023 01:31, Dmitry Baryshkov wrote:
>>>>> clk_rcg2_shared_ops implements support for the case of the RCG which
>>>>> must not be completely turned off. However its design has one major
>>>>> drawback: it doesn't allow us to properly implement the is_enabled
>>>>> callback, which causes different kinds of misbehaviour from the CCF.
>>>>>
>>>>> Follow the idea behind clk_regmap_phy_mux_ops and implement the new
>>>>> clk_rcg2_parked_ops. It also targets the clocks which must not be fully
>>>>> switched off (and shared most of the implementation with
>>>>> clk_rcg2_shared_ops). The major difference is that it requires that the
>>>>> parent map doesn't conain the safe (parked) clock source. Instead if the
>>>>> CFG_REG register points to the safe source, the clock is considered to
>>>>> be disabled.
>>>>
>>>> Why not have a new bit in .flags ?
>>>>
>>>> Instead of lying about the clock being off, mark the clock as "parked",
>>>> or "safe parked" or whatever term we choose for it ?
>>>
>>> The main problem with adding flags doesn't fully scale. From the CCF
>>> perspective, what should be the difference between parked and disabled
>>> clocks? How should it treat the parked one?
>>
>> Exactly the same as a disabled clock, except you get a "parked" instead of a "disabled" when looking up its state and you don't have to
> The thing is that currently there's only the notion of "enabled"
> or "not enabled".. Introducing a third state here would be the
> jump from boolean to quantum logic!
> 
> I think that abstracting this information away from Linux is not
> an issue.. These clocks "can't be any more off", or the SoC will
> explode badly and Linux will be unusable..
> 
> Think of it like CPUs with a hypervisor, you shut them down, but
> the physical number crunchers on the host CPU may not actually
> get cut off from their power source, but there's no reason for
> the VM to know that. That's probably what happens on our little
> virtualized snapdragons anyway..
> 
> Konrad

So not a state but a flag.

1. The clock tree we declare _should_ be a fair and complete description
    of the hardware clock tree.

2. If we remove XO from some trees with the only indication of
    differentiation being the callback you bind the tree to you need
    someone reading the code both know about parking, derive that
    information from reading the callback, which means you require that
    person to read the code in detail and understand it.

    That's alot of tribal knowledge we are storing up there.

3. A different approach is to add a new CLK_DISABLE_PARKS_TO_XO - or
    whatever name makes sense.

    a) The clock tree declared in the gcc, camcc, dispcc, videocc or
       is correct and aligned with the documentation and silicon.
       Right away this avoids patches sent to 'fixup' incomplete trees.

    b) When you look at a clock struct clk_branch_gcc.clk.hw.init.flags
       there is a big dirty CLK_DOES_THIS_THING flag which doesn't
       require a developer to have tribal knowledge about how we've
       hacked up clock parking.

My basic point here is the declaration of a parked clock should be 
obvious, easy to understand and not lend itself to "helpful" patches to 
"fix" the clock tree.

Also consider precedent. When you want to quickly get your clock 
controller up and running - you generally open existing upstream stuff 
to clone and own as much as possible. A BIT_DIRTY_FLAG transmits more 
information than a small callback with esoteric logic buried inside of 
the disable path.

I agree with your point on a new state but similarly I think the 
callback buries too much information away. IMO the top level clock 
declaration - rather like the DT should as closely as possible declare 
an accurate clock tree.

If we need to do special stuff to an individual tree, then CLK_FLAG it. 
Are qcom clocks really the only clocks in the world that need to park to 
XO on the disable path ?

Alternatively continue on with the callback but make the name more 
instructive not "park" since we are dealing with people who have English 
as a second language, third language. English is my first language but 
still a "parked" clock means little to me except that like you and 
Dmitry I work with qcom stuff so I understand it.

"disable_park_xo->clk_disable" or something - even still I think 
removing XO from the clock tree is asking for trouble.

Start from the principle that gcc/camcc/dispcc clock trees should be 
complete and work from there.

That's my 0.02 anyway.

---
bod

Re: [RFC PATCH 1/2] clk: qcom: implement RCG2 'parked' clock support

[Dmitry Baryshkov]

On 07/10/2023 13:08, Bryan O'Donoghue wrote:
> On 07/10/2023 00:45, Konrad Dybcio wrote:
>> On 4.10.2023 14:52, Bryan O'Donoghue wrote:
>>> On 04/10/2023 13:08, Dmitry Baryshkov wrote:
>>>> On Wed, 4 Oct 2023 at 12:27, Bryan O'Donoghue
>>>> <bryan.odonoghue@linaro.org> wrote:
>>>>>
>>>>> On 04/10/2023 01:31, Dmitry Baryshkov wrote:
>>>>>> clk_rcg2_shared_ops implements support for the case of the RCG which
>>>>>> must not be completely turned off. However its design has one major
>>>>>> drawback: it doesn't allow us to properly implement the is_enabled
>>>>>> callback, which causes different kinds of misbehaviour from the CCF.
>>>>>>
>>>>>> Follow the idea behind clk_regmap_phy_mux_ops and implement the new
>>>>>> clk_rcg2_parked_ops. It also targets the clocks which must not be 
>>>>>> fully
>>>>>> switched off (and shared most of the implementation with
>>>>>> clk_rcg2_shared_ops). The major difference is that it requires 
>>>>>> that the
>>>>>> parent map doesn't conain the safe (parked) clock source. Instead 
>>>>>> if the
>>>>>> CFG_REG register points to the safe source, the clock is 
>>>>>> considered to
>>>>>> be disabled.
>>>>>
>>>>> Why not have a new bit in .flags ?
>>>>>
>>>>> Instead of lying about the clock being off, mark the clock as 
>>>>> "parked",
>>>>> or "safe parked" or whatever term we choose for it ?
>>>>
>>>> The main problem with adding flags doesn't fully scale. From the CCF
>>>> perspective, what should be the difference between parked and disabled
>>>> clocks? How should it treat the parked one?
>>>
>>> Exactly the same as a disabled clock, except you get a "parked" 
>>> instead of a "disabled" when looking up its state and you don't have to
>> The thing is that currently there's only the notion of "enabled"
>> or "not enabled".. Introducing a third state here would be the
>> jump from boolean to quantum logic!
>>
>> I think that abstracting this information away from Linux is not
>> an issue.. These clocks "can't be any more off", or the SoC will
>> explode badly and Linux will be unusable..
>>
>> Think of it like CPUs with a hypervisor, you shut them down, but
>> the physical number crunchers on the host CPU may not actually
>> get cut off from their power source, but there's no reason for
>> the VM to know that. That's probably what happens on our little
>> virtualized snapdragons anyway..
>>
>> Konrad
> 
> So not a state but a flag.
> 
> 1. The clock tree we declare _should_ be a fair and complete description
>     of the hardware clock tree.

Yes and no. We already have clocks not present in the tree for different 
reasons: being handled by the RPM(h), being critical to the platform 
integrity, being useless for Linux, etc.

> 
> 2. If we remove XO from some trees with the only indication of
>     differentiation being the callback you bind the tree to you need
>     someone reading the code both know about parking, derive that
>     information from reading the callback, which means you require that
>     person to read the code in detail and understand it.
> 
>     That's alot of tribal knowledge we are storing up there.

I think adding a huge comment should help. Because otherwise you sound 
like 'we should not expect kernel developers to read the code', which is 
not true.

> 
> 3. A different approach is to add a new CLK_DISABLE_PARKS_TO_XO - or
>     whatever name makes sense.
> 
>     a) The clock tree declared in the gcc, camcc, dispcc, videocc or
>        is correct and aligned with the documentation and silicon.
>        Right away this avoids patches sent to 'fixup' incomplete trees.
> 
>     b) When you look at a clock struct clk_branch_gcc.clk.hw.init.flags
>        there is a big dirty CLK_DOES_THIS_THING flag which doesn't
>        require a developer to have tribal knowledge about how we've
>        hacked up clock parking.

But the problem is that this flag is not generic at all. I think we will 
be damned and prosecuted if we try adding anything about PARK_TO_XO to 
<linux/clk-provider.h>.

And also there is always a question on the state integrity: if the clock 
is parented with the XO on the driver bootstrap, should CCF treat it as 
'parked' or as 'enabled, clocked by XO'?

> 
> My basic point here is the declaration of a parked clock should be 
> obvious, easy to understand and not lend itself to "helpful" patches to 
> "fix" the clock tree.

We already tried doing that... And it failed. For the PHY PIPE clocks we 
ended up doing exactly the same thing, because it simplified the code 
_a_lot_.

> Also consider precedent. When you want to quickly get your clock 
> controller up and running - you generally open existing upstream stuff 
> to clone and own as much as possible. A BIT_DIRTY_FLAG transmits more 
> information than a small callback with esoteric logic buried inside of 
> the disable path.
> 
> I agree with your point on a new state but similarly I think the 
> callback buries too much information away. IMO the top level clock 
> declaration - rather like the DT should as closely as possible declare 
> an accurate clock tree.
> 
> If we need to do special stuff to an individual tree, then CLK_FLAG it. 
> Are qcom clocks really the only clocks in the world that need to park to 
> XO on the disable path ?
> 
> Alternatively continue on with the callback but make the name more 
> instructive not "park" since we are dealing with people who have English 
> as a second language, third language. English is my first language but 
> still a "parked" clock means little to me except that like you and 
> Dmitry I work with qcom stuff so I understand it.
> 
> "disable_park_xo->clk_disable" or something - even still I think 
> removing XO from the clock tree is asking for trouble.

clk_rcg2_disable_parks_to_tcxo_ops ? Slightly ugly but I'm fine with that.

> 
> Start from the principle that gcc/camcc/dispcc clock trees should be 
> complete and work from there.
> 
> That's my 0.02 anyway.
> 

-- 
With best wishes
Dmitry

Re: [RFC PATCH 1/2] clk: qcom: implement RCG2 'parked' clock support

[Konrad Dybcio]

On 10/7/23 01:43, Konrad Dybcio wrote:
> On 4.10.2023 02:31, Dmitry Baryshkov wrote:
>> clk_rcg2_shared_ops implements support for the case of the RCG which
>> must not be completely turned off. However its design has one major
>> drawback: it doesn't allow us to properly implement the is_enabled
>> callback, which causes different kinds of misbehaviour from the CCF.
>>
>> Follow the idea behind clk_regmap_phy_mux_ops and implement the new
>> clk_rcg2_parked_ops. It also targets the clocks which must not be fully
>> switched off (and shared most of the implementation with
>> clk_rcg2_shared_ops). The major difference is that it requires that the
>> parent map doesn't conain the safe (parked) clock source. Instead if the
>> CFG_REG register points to the safe source, the clock is considered to
>> be disabled.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
> Would the intention here be to replace all usages of _shared_?
?

Konrad

Re: [RFC PATCH 1/2] clk: qcom: implement RCG2 'parked' clock support

[Dmitry Baryshkov]

On Thu, 26 Oct 2023 at 21:57, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
>
>
> On 10/7/23 01:43, Konrad Dybcio wrote:
> > On 4.10.2023 02:31, Dmitry Baryshkov wrote:
> >> clk_rcg2_shared_ops implements support for the case of the RCG which
> >> must not be completely turned off. However its design has one major
> >> drawback: it doesn't allow us to properly implement the is_enabled
> >> callback, which causes different kinds of misbehaviour from the CCF.
> >>
> >> Follow the idea behind clk_regmap_phy_mux_ops and implement the new
> >> clk_rcg2_parked_ops. It also targets the clocks which must not be fully
> >> switched off (and shared most of the implementation with
> >> clk_rcg2_shared_ops). The major difference is that it requires that the
> >> parent map doesn't conain the safe (parked) clock source. Instead if the
> >> CFG_REG register points to the safe source, the clock is considered to
> >> be disabled.
> >>
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> ---
> > Would the intention here be to replace all usages of _shared_?

Yes

-- 
With best wishes
Dmitry

Re: [RFC PATCH 1/2] clk: qcom: implement RCG2 'parked' clock support

[Konrad Dybcio]

On 10/26/23 22:47, Dmitry Baryshkov wrote:
> On Thu, 26 Oct 2023 at 21:57, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>>
>>
>> On 10/7/23 01:43, Konrad Dybcio wrote:
>>> On 4.10.2023 02:31, Dmitry Baryshkov wrote:
>>>> clk_rcg2_shared_ops implements support for the case of the RCG which
>>>> must not be completely turned off. However its design has one major
>>>> drawback: it doesn't allow us to properly implement the is_enabled
>>>> callback, which causes different kinds of misbehaviour from the CCF.
>>>>
>>>> Follow the idea behind clk_regmap_phy_mux_ops and implement the new
>>>> clk_rcg2_parked_ops. It also targets the clocks which must not be fully
>>>> switched off (and shared most of the implementation with
>>>> clk_rcg2_shared_ops). The major difference is that it requires that the
>>>> parent map doesn't conain the safe (parked) clock source. Instead if the
>>>> CFG_REG register points to the safe source, the clock is considered to
>>>> be disabled.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>> Would the intention here be to replace all usages of _shared_?
> 
> Yes
Then I suppose an immediate followup question would be: "why
introduce new ops instead of replacing the existing ones in the
patchset?".

Konrad

Re: [RFC PATCH 1/2] clk: qcom: implement RCG2 'parked' clock support

[Dmitry Baryshkov]

On Thu, 26 Oct 2023 at 23:49, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
>
>
> On 10/26/23 22:47, Dmitry Baryshkov wrote:
> > On Thu, 26 Oct 2023 at 21:57, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>
> >>
> >>
> >> On 10/7/23 01:43, Konrad Dybcio wrote:
> >>> On 4.10.2023 02:31, Dmitry Baryshkov wrote:
> >>>> clk_rcg2_shared_ops implements support for the case of the RCG which
> >>>> must not be completely turned off. However its design has one major
> >>>> drawback: it doesn't allow us to properly implement the is_enabled
> >>>> callback, which causes different kinds of misbehaviour from the CCF.
> >>>>
> >>>> Follow the idea behind clk_regmap_phy_mux_ops and implement the new
> >>>> clk_rcg2_parked_ops. It also targets the clocks which must not be fully
> >>>> switched off (and shared most of the implementation with
> >>>> clk_rcg2_shared_ops). The major difference is that it requires that the
> >>>> parent map doesn't conain the safe (parked) clock source. Instead if the
> >>>> CFG_REG register points to the safe source, the clock is considered to
> >>>> be disabled.
> >>>>
> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>> ---
> >>> Would the intention here be to replace all usages of _shared_?
> >
> > Yes
> Then I suppose an immediate followup question would be: "why
> introduce new ops instead of replacing the existing ones in the
> patchset?".

Because using this ops requires doing more than just replacing the
name. it also requires dropping the XO source from the parent maps. So
I'd prefer to perform this migration on a driver-by-driver basis.
Otherwise it might be very easy to introduce a mistake somewhere.

-- 
With best wishes
Dmitry

Re: [RFC PATCH 1/2] clk: qcom: implement RCG2 'parked' clock support

[Bjorn Andersson]

On Wed, Oct 04, 2023 at 03:31:24AM +0300, Dmitry Baryshkov wrote:
> clk_rcg2_shared_ops implements support for the case of the RCG which
> must not be completely turned off. However its design has one major
> drawback: it doesn't allow us to properly implement the is_enabled
> callback, which causes different kinds of misbehaviour from the CCF.

I have some behaviors in mind when reading this, others might not.
Please give some specific behavior(s) here.

Thanks,
Bjorn

> 
> Follow the idea behind clk_regmap_phy_mux_ops and implement the new
> clk_rcg2_parked_ops. It also targets the clocks which must not be fully
> switched off (and shared most of the implementation with
> clk_rcg2_shared_ops). The major difference is that it requires that the
> parent map doesn't conain the safe (parked) clock source. Instead if the
> CFG_REG register points to the safe source, the clock is considered to
> be disabled.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/clk-rcg.h  |  1 +
>  drivers/clk/qcom/clk-rcg2.c | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
> index e6d84c8c7989..9fbbf1251564 100644
> --- a/drivers/clk/qcom/clk-rcg.h
> +++ b/drivers/clk/qcom/clk-rcg.h
> @@ -176,6 +176,7 @@ extern const struct clk_ops clk_byte2_ops;
>  extern const struct clk_ops clk_pixel_ops;
>  extern const struct clk_ops clk_gfx3d_ops;
>  extern const struct clk_ops clk_rcg2_shared_ops;
> +extern const struct clk_ops clk_rcg2_parked_ops;
>  extern const struct clk_ops clk_dp_ops;
>  
>  struct clk_rcg_dfs_data {
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index 5183c74b074f..3f52abf0025e 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -5,6 +5,7 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/bitops.h>
> +#include <linux/bitfield.h>
>  #include <linux/err.h>
>  #include <linux/bug.h>
>  #include <linux/export.h>
> @@ -1150,6 +1151,39 @@ const struct clk_ops clk_rcg2_shared_ops = {
>  };
>  EXPORT_SYMBOL_GPL(clk_rcg2_shared_ops);
>  
> +static int clk_rcg2_park_is_enabled(struct clk_hw *hw)
> +{
> +	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> +	u32 cfg;
> +	int ret;
> +
> +	if (!clk_rcg2_is_enabled(hw))
> +		return false;
> +
> +	ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &cfg);
> +	if (ret)
> +		return ret;
> +
> +	return FIELD_GET(CFG_SRC_SEL_MASK, cfg) != rcg->safe_src_index;
> +}
> +
> +/*
> + * Unlike clk_rcg2_shared_ops, the safe_src_index aka XO must NOT be present in
> + * parent_map. This allows us to implement proper is_enabled callback.
> + */
> +const struct clk_ops clk_rcg2_parked_ops = {
> +	.is_enabled = clk_rcg2_park_is_enabled,
> +	.enable = clk_rcg2_shared_enable,
> +	.disable = clk_rcg2_shared_disable,
> +	.get_parent = clk_rcg2_shared_get_parent,
> +	.set_parent = clk_rcg2_shared_set_parent,
> +	.recalc_rate = clk_rcg2_shared_recalc_rate,
> +	.determine_rate = clk_rcg2_determine_rate,
> +	.set_rate = clk_rcg2_shared_set_rate,
> +	.set_rate_and_parent = clk_rcg2_shared_set_rate_and_parent,
> +};
> +EXPORT_SYMBOL_GPL(clk_rcg2_parked_ops);
> +
>  /* Common APIs to be used for DFS based RCGR */
>  static void clk_rcg2_dfs_populate_freq(struct clk_hw *hw, unsigned int l,
>  				       struct freq_tbl *f)
> -- 
> 2.39.2
> 

Re: [RFC PATCH 1/2] clk: qcom: implement RCG2 'parked' clock support

[Dmitry Baryshkov]

On Fri, 27 Oct 2023 at 01:56, Bjorn Andersson <quic_bjorande@quicinc.com> wrote:
>
> On Wed, Oct 04, 2023 at 03:31:24AM +0300, Dmitry Baryshkov wrote:
> > clk_rcg2_shared_ops implements support for the case of the RCG which
> > must not be completely turned off. However its design has one major
> > drawback: it doesn't allow us to properly implement the is_enabled
> > callback, which causes different kinds of misbehaviour from the CCF.
>
> I have some behaviors in mind when reading this, others might not.
> Please give some specific behavior(s) here.

Bjorn (and other interested parties). At this RFC stage it would be
really nice to check whether the patch idea is worth the trouble and
if it fixes the issue.

>
> Thanks,
> Bjorn
>
> >
> > Follow the idea behind clk_regmap_phy_mux_ops and implement the new
> > clk_rcg2_parked_ops. It also targets the clocks which must not be fully
> > switched off (and shared most of the implementation with
> > clk_rcg2_shared_ops). The major difference is that it requires that the
> > parent map doesn't conain the safe (parked) clock source. Instead if the
> > CFG_REG register points to the safe source, the clock is considered to
> > be disabled.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/clk/qcom/clk-rcg.h  |  1 +
> >  drivers/clk/qcom/clk-rcg2.c | 34 ++++++++++++++++++++++++++++++++++
> >  2 files changed, 35 insertions(+)
> >
> > diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
> > index e6d84c8c7989..9fbbf1251564 100644
> > --- a/drivers/clk/qcom/clk-rcg.h
> > +++ b/drivers/clk/qcom/clk-rcg.h
> > @@ -176,6 +176,7 @@ extern const struct clk_ops clk_byte2_ops;
> >  extern const struct clk_ops clk_pixel_ops;
> >  extern const struct clk_ops clk_gfx3d_ops;
> >  extern const struct clk_ops clk_rcg2_shared_ops;
> > +extern const struct clk_ops clk_rcg2_parked_ops;
> >  extern const struct clk_ops clk_dp_ops;
> >
> >  struct clk_rcg_dfs_data {
> > diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> > index 5183c74b074f..3f52abf0025e 100644
> > --- a/drivers/clk/qcom/clk-rcg2.c
> > +++ b/drivers/clk/qcom/clk-rcg2.c
> > @@ -5,6 +5,7 @@
> >
> >  #include <linux/kernel.h>
> >  #include <linux/bitops.h>
> > +#include <linux/bitfield.h>
> >  #include <linux/err.h>
> >  #include <linux/bug.h>
> >  #include <linux/export.h>
> > @@ -1150,6 +1151,39 @@ const struct clk_ops clk_rcg2_shared_ops = {
> >  };
> >  EXPORT_SYMBOL_GPL(clk_rcg2_shared_ops);
> >
> > +static int clk_rcg2_park_is_enabled(struct clk_hw *hw)
> > +{
> > +     struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> > +     u32 cfg;
> > +     int ret;
> > +
> > +     if (!clk_rcg2_is_enabled(hw))
> > +             return false;
> > +
> > +     ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &cfg);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return FIELD_GET(CFG_SRC_SEL_MASK, cfg) != rcg->safe_src_index;
> > +}
> > +
> > +/*
> > + * Unlike clk_rcg2_shared_ops, the safe_src_index aka XO must NOT be present in
> > + * parent_map. This allows us to implement proper is_enabled callback.
> > + */
> > +const struct clk_ops clk_rcg2_parked_ops = {
> > +     .is_enabled = clk_rcg2_park_is_enabled,
> > +     .enable = clk_rcg2_shared_enable,
> > +     .disable = clk_rcg2_shared_disable,
> > +     .get_parent = clk_rcg2_shared_get_parent,
> > +     .set_parent = clk_rcg2_shared_set_parent,
> > +     .recalc_rate = clk_rcg2_shared_recalc_rate,
> > +     .determine_rate = clk_rcg2_determine_rate,
> > +     .set_rate = clk_rcg2_shared_set_rate,
> > +     .set_rate_and_parent = clk_rcg2_shared_set_rate_and_parent,
> > +};
> > +EXPORT_SYMBOL_GPL(clk_rcg2_parked_ops);
> > +
> >  /* Common APIs to be used for DFS based RCGR */
> >  static void clk_rcg2_dfs_populate_freq(struct clk_hw *hw, unsigned int l,
> >                                      struct freq_tbl *f)
> > --
> > 2.39.2
> >



-- 
With best wishes
Dmitry