Hi Dmitry,

On Mon, Jun 19, 2023 at 02:38:59AM +0300, Dmitry Osipenko wrote:
> 05.05.2023 14:26, Maxime Ripard пишет:
> > The Tegra super clocks implements a mux with a set_parent hook, but
> > doesn't provide a determine_rate implementation.
> > 
> > This is a bit odd, since set_parent() is there to, as its name implies,
> > change the parent of a clock. However, the most likely candidate to
> > trigger that parent change is a call to clk_set_rate(), with
> > determine_rate() figuring out which parent is the best suited for a
> > given rate.
> > 
> > The other trigger would be a call to clk_set_parent(), but it's far less
> > used, and it doesn't look like there's any obvious user for that clock.
> > 
> > So, the set_parent hook is effectively unused, possibly because of an
> > oversight. However, it could also be an explicit decision by the
> > original author to avoid any reparenting but through an explicit call to
> > clk_set_parent().
> > 
> > The driver does implement round_rate() though, which means that we can
> > change the rate of the clock, but we will never get to change the
> > parent.
> > 
> > However, It's hard to tell whether it's been done on purpose or not.
> > 
> > Since we'll start mandating a determine_rate() implementation, let's
> > convert the round_rate() implementation to a determine_rate(), which
> > will also make the current behavior explicit. And if it was an
> > oversight, the clock behaviour can be adjusted later on.
> > 
> > Cc: Jonathan Hunter <jonathanh@nvidia.com>
> > Cc: Peter De Schrijver <pdeschrijver@nvidia.com>
> > Cc: Prashant Gaikwad <pgaikwad@nvidia.com>
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Cc: linux-tegra@vger.kernel.org
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/clk/tegra/clk-super.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/clk/tegra/clk-super.c b/drivers/clk/tegra/clk-super.c
> > index 3f3a7a203c5f..7ec47942720c 100644
> > --- a/drivers/clk/tegra/clk-super.c
> > +++ b/drivers/clk/tegra/clk-super.c
> > @@ -142,15 +142,22 @@ static const struct clk_ops tegra_clk_super_mux_ops = {
> >  	.restore_context = clk_super_mux_restore_context,
> >  };
> >  
> > -static long clk_super_round_rate(struct clk_hw *hw, unsigned long rate,
> > -				 unsigned long *parent_rate)
> > +static int clk_super_determine_rate(struct clk_hw *hw,
> > +				    struct clk_rate_request *req)
> >  {
> >  	struct tegra_clk_super_mux *super = to_clk_super_mux(hw);
> >  	struct clk_hw *div_hw = &super->frac_div.hw;
> > +	unsigned long rate;
> >  
> >  	__clk_hw_set_clk(div_hw, hw);
> >  
> > -	return super->div_ops->round_rate(div_hw, rate, parent_rate);
> > +	rate = super->div_ops->round_rate(div_hw, req->rate,
> > +					  &req->best_parent_rate);
> > +	if (rate < 0)
> > +		return rate;
> > +
> > +	req->rate = rate;
> > +	return 0;
> >  }
> >  
> >  static unsigned long clk_super_recalc_rate(struct clk_hw *hw,
> > @@ -193,7 +200,7 @@ const struct clk_ops tegra_clk_super_ops = {
> >  	.get_parent = clk_super_get_parent,
> >  	.set_parent = clk_super_set_parent,
> >  	.set_rate = clk_super_set_rate,
> > -	.round_rate = clk_super_round_rate,
> > +	.determine_rate = clk_super_determine_rate,
> >  	.recalc_rate = clk_super_recalc_rate,
> >  	.restore_context = clk_super_restore_context,
> >  };
> > 
> 
> Tegra30 doesn't boot anymore with this change. Best would be to keep the
> old behaviour for both sclk and periph tegra clocks.

I took a closer look at the patch and can't find anything different to
what the core is doing if there's a round_rate implementation:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L1459

Also, it's not clear to me how that driver is used. It looks like
div_ops is always supposed to be set, and super clocks are registered
with either tegra_clk_register_super_clk() or tegra_clk_register_super_mux()

tegra_clk_register_super_clk() sets the div_ops pointer to
tegra_clk_super_ops, but tegra30 doesn't seem to call it.

tegra_clk_register_super_mux() doesn't set the div_ops pointer, but is
used by tegra30, so I would assume that it's the broken one. But I'm
confused, since div_ops doesn't seem to be set anywhere?

Maxime