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@xxxxxxxxxx>
> > Cc: Peter De Schrijver <pdeschrijver@xxxxxxxxxx>
> > Cc: Prashant Gaikwad <pgaikwad@xxxxxxxxxx>
> > Cc: Thierry Reding <thierry.reding@xxxxxxxxx>
> > Cc: linux-tegra@xxxxxxxxxxxxxxx
> > Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> > ---
> > 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
Attachment:
signature.asc
Description: PGP signature
