On Fri, 27 Oct 2023 at 01:56, Bjorn Andersson <quic_bjorande@xxxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
