RE: [PATCH v3 2/2] clk: scmi: add set/get_parent support

Peng Fan
2023-10-03T09:46:40Z

Hi Cristian,

> Subject: Re: [PATCH v3 2/2] clk: scmi: add set/get_parent support
>
> On Sun, Oct 01, 2023 at 12:38:44PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > SCMI v3.2 adds set/get parent clock commands, so update the clk driver
> > to support them.
> >
>
> Hi,
>
> a few notes down below.
>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> > drivers/clk/clk-scmi.c | 50
> > +++++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 49 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index
> > 2e1337b511eb..5aaca674830f 100644
> > --- a/drivers/clk/clk-scmi.c
> > +++ b/drivers/clk/clk-scmi.c
> > @@ -24,6 +24,7 @@ struct scmi_clk {
> > struct clk_hw hw;
> > const struct scmi_clock_info *info;
> > const struct scmi_protocol_handle *ph;
> > + struct clk_parent_data *parent_data;
> > };
> >
> > #define to_scmi_clk(clk) container_of(clk, struct scmi_clk, hw) @@
> > -78,6 +79,35 @@ static int scmi_clk_set_rate(struct clk_hw *hw, unsigned
> long rate,
> > return scmi_proto_clk_ops->rate_set(clk->ph, clk->id, rate); }
> >
> > +static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index) {
> > + struct scmi_clk *clk = to_scmi_clk(hw);
> > +
> > + return scmi_proto_clk_ops->parent_set(clk->ph, clk->id,
> > +parent_index); }
> > +
> > +static u8 scmi_clk_get_parent(struct clk_hw *hw) {
> > + struct scmi_clk *clk = to_scmi_clk(hw);
> > + u32 parent_id;
> > + int ret;
> > +
> > + ret = scmi_proto_clk_ops->parent_get(clk->ph, clk->id, &parent_id);
> > + if (ret)
> > + return 0;
> > +
> > + return parent_id;
> > +}
> > +
>
> While testing using CLK Debugfs with CLOCK_ALLOW_WRITE_DEBUGFS 1 I
> noticed that I can correctly change the clk_parent and then read back the
> clk_possible_parents, BUT if I read clk_parent right after boot (OR after
> loading the clk-scmi module) I cannot get back any value from debugfs even
> though I can see the correct SCMI messages being exchanged from the traces.
>
> My guess was that, while scmi_clk_set_parent is invoked by the CLK core with
> a parent_index that has been remapped by the core to the SCMI clock
> domain ID, this is not done by scmi_clk_get_parent() so you are returning to
> the clock framework as parent_id the raw SCMI clock domain id as returned
> by the platform instead of the clk parent id used by the core.
>
> This does not happen after you issue at first a reparent because in that case
> on the following read of clk_parent the CLK framework returns the last value
> you have set that it had cached previously.
>
> This fixes for me the issue:
>
> ---8<----
>
> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index
> 5aaca674830f..fd47232d4021 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -89,14 +89,21 @@ static int scmi_clk_set_parent(struct clk_hw *hw, u8
> parent_index) static u8 scmi_clk_get_parent(struct clk_hw *hw) {
> struct scmi_clk *clk = to_scmi_clk(hw);
> - u32 parent_id;
> + u32 parent_id, p_idx;
> int ret;
>
> ret = scmi_proto_clk_ops->parent_get(clk->ph, clk->id, &parent_id);
> if (ret)
> return 0;
>
> - return parent_id;
> + for (p_idx = 0; p_idx < clk->info->num_parents; p_idx++)
> + if (clk->parent_data[p_idx].index == parent_id)
> + break;
> +
> + if (p_idx == clk->info->num_parents)
> + return 0;
> +
> + return p_idx;
> }
>

You are right. Thanks for doing the fix.

> ----8<-----
>
> Not sure if there is a clever way to do it.
>
> Aside from this, another inherent issue is that you cannot really return an
> error from .get_parent() so if the SCMI get_parent ops should fail (ex. timeout)
> you return 0... (and me too in the above fix) but this is due to the CLK
> framework callback definition itself.

Yes. Right. I will include your fix and do a test, then out v4, should be soon.

Thanks,
Peng.

>
> Thanks,
> Cristian