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

[Peng Fan]

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@xxxxxxx>
> >
> > 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@xxxxxxx>
> > ---
> >  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