Quoting Dmitry Baryshkov (2023-10-03 18:23:07)
> 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@xxxxxxxxxx>
> ---
> drivers/clk/qcom/clk-rcg.h | 1 +
> drivers/clk/qcom/clk-rcg2.c | 56 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 57 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..fc75e2bc2d70 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,61 @@ const struct clk_ops clk_rcg2_shared_ops = {
> };
> EXPORT_SYMBOL_GPL(clk_rcg2_shared_ops);
>
> +static int clk_rcg2_parked_is_enabled(struct clk_hw *hw)
> +{
> + struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> + u32 cmd, cfg;
> + int ret;
> +
> + ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CMD_REG, &cmd);
> + if (ret)
> + return ret;
> +
> + if ((cmd & CMD_ROOT_EN) == 0)
> + return false;
return 0?
CMD_ROOT_OFF can be 0 and CMD_ROOT_EN can also be 0 at the same time.
When that happens, some branch child clk is enabled and the rcg is
actually enabled. There's a hardware feedback mechanism from the
branches to the rcg so the rcg is guaranteed enabled. I'm trying to say
that this bit is unreliable on its own, so we need to take care here. In
fact, this bit is only used as a software override to make sure the
branches don't turn off the rcg inadvertently.
What if a branch is enabled, but the rcg root_en bit isn't set, and XO
is used? In that case, this will report the clk as disabled when it's
really enabled. That will look confusing to the clk framework because a
child will be enabled without the parent being enabled. Things will
probably still work though, because this only matters during disabling
unused clks.
Maybe it's better to not implement an is_enabled() callback for this clk
and simply call a function to see which parent the hardware is using (XO
or not). Basically don't go through clk_hw_is_enabled() and just call
clk_rcg2_parked_is_enabled() directly wherever the clk_hw API is used.
Then the framework doesn't get confused about enabled children with
disabled parents, but the downside is that the framework doesn't know if
the rcg is enabled. This is most likely fine though because an enabled
rcg doesn't really make a difference. The important thing is knowing
which branches are enabled at the framework level. Furthermore, the
framework doesn't currently handle propagating up the enable state at
boot to parents, so if say we have a child branch that is enabled, the
enable state of the parent _must_ be enabled as well, or the branch is
wedged and the only way to unwedge that is to enable the parent. It's
quite a mess!
Long story short, I question why we need to implement is_enabled() for
this clk. What's the benefit? The branches being off is more important
if we're concerned about saving power. There's the problem of handing
off enable state from when the driver probes, but that's not so easy to
solve given that a branch could be enabled (or a branch could be enabled
that isn't even known to linux). And it also sort of doesn't matter
because we know XO is practically always enabled and what really matters
is making sure the driver can't wedge the RCG by changing the source to
something that isn't enabled if it thinks the RCG is disabled when it is
really enabled.
That's sort of the only rule here, don't write the hardware when the
current parent isn't enabled or the new parent isn't enabled. We don't
know if the rcg is ever enabled, so we can only write the "go bit"
(CMD_UPDATE) when we're 100% certain that the parent (or next parent
when switching) is enabled. XO we know is always enabled, but otherwise
we don't know unless the framework has enabled the clk (and therefore
implicitly enabled the parent). The set_rate op could be called from
either enabled or disabled state, same for the set_parent op. And we
want the other clk APIs to report the state of the clk (like the parent
or rate) even if the hardware hasn't been changed.
> +
> + 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;
> +}
> +
> +static int clk_rcg2_parked_init(struct clk_hw *hw)
> +{
> + struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> + const struct freq_tbl *f = rcg->freq_tbl;
> +
> + regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &rcg->parked_cfg);
I need this part today to fix a stuck clk problem I see on trogdor.lazor
where during registration a call to clk_ops::get_parent() sees the clk
isn't enabled at boot (because there isn't a clk_ops::is_enabled()
function) so clk_rcg2_shared_get_parent() reads the parent from the
'parked_cfg' value, which is zero. If the hardware actually has non-zero
for the parent then the framework will get the wrong parent, which is
what happens on trogdor when the devmode screen is shown. The parent is
the display PLL instead of XO. I haven't dug far enough to understand
why disabling unused clks wedges the branch when we try to enable it
again, but not disabling unused clks fixes the problem or reading the
config register at registration to get the proper parent also fixes it.
I guess the problem is that we're switching the RCG value when we
shouldn't be doing that.
> +
> + if (FIELD_GET(CFG_SRC_SEL_MASK, rcg->parked_cfg) != rcg->safe_src_index)
> + return 0;
> +
> + if (WARN_ON(!f) ||
> + WARN_ON(qcom_find_src_cfg(hw, rcg->parent_map, f->src) == rcg->safe_src_index))
> + return -EINVAL;
> +
> + return __clk_rcg2_configure(rcg, f, &rcg->parked_cfg);
It would be good to have a comment above this like
/*
* Dirty the rcg registers to point at the first frequency table
* entry which is guaranteed to not use the safe_src_index.
* Setting the rate of the clk with rcg registers containing the
* safe_src_index will confuse clk_rcg2_parked_is_enabled() as
* to the enable state and lead to actually changing the rate of
* the clk when it isn't enabled.
*/
> +}
> +
> +/*
> + * 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.
We could also modify clk_ops::set_rate() and clk_ops::determine_rate()
to ignore frequency table entries with the safe_src_index, so that no
driver can change the frequency to be XO. Then XO is still "reserved",
and it still means the clk is disabled when the parent is XO, but we
don't have to change the RCG registers during clk_rcg2_parked_init() to
move off the safe_src/XO parent. We also have to prevent the parent from
being set to XO with clk_set_parent(). That should be doable by failing
the clk_ops::set_parent() op when the parent is XO.
I'd actually prefer that approach if it's workable, so that we don't
dirty the RCG registers during clk registration. I think qcom folks were
unhappy with the rcg registers being dirty for a long time
(CMD_DIRTY_CFG), because the other entity (the gdsc?) was triggering the
rcg switch (CMD_UPDATE) and that was causing the wrong parent to be
used.
I still come back to the why question though. What are we gaining by
implementing is_enabled for this clk?
> + */
> +const struct clk_ops clk_rcg2_parked_ops = {
> + .init = clk_rcg2_parked_init,
> + .is_enabled = clk_rcg2_parked_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);
