Hi Maxime,
Le ven. 4 nov. 2022 à 15:59:46 +0100, Maxime Ripard
<maxime@xxxxxxxxxx> a
écrit :
> Hi Paul,
>
> On Fri, Nov 04, 2022 at 02:31:20PM +0000, Paul Cercueil wrote:
> > Le ven. 4 nov. 2022 à 14:18:13 +0100, Maxime Ripard
> > <maxime@xxxxxxxxxx> a
> > écrit :
> > > The Ingenic CGU 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.
> >
> > So it's partly on purpose, partly because I didn't know about
> > .determine_rate.
> >
> > There's nothing odd about having a lonely .set_parent
callback; in
> > my case
> > the clocks are parented from the device tree.
> >
> > Having the clocks driver trigger a parent change when
requesting a
> > rate
> > change sounds very dangerous, IMHO. My MMC controller can be
> > parented to the
> > external 48 MHz oscillator, and if the card requests 50 MHz, it
> > could switch
> > to one of the PLLs. That works as long as the PLLs don't change
> > rate, but if
> > one is configured as driving the CPU clock, it becomes messy.
> > The thing is, the clocks driver has no way to know whether or
not
> > it is
> > "safe" to use a designated parent.
> >
> > For that reason, in practice, I never actually want to have a
clock
> > re-parented - it's almost always a bad idea vs. sticking to the
> > parent clock
> > configured in the DTS.
>
> Yeah, and this is totally fine. But we need to be explicit about
it. The
> determine_rate implementation I did in all the patches is an exact
> equivalent to the round_rate one if there was one. We will never
ask to
> change the parent.
>
> Given what you just said, I would suggest to set the
> CLK_SET_RATE_NO_REPARENT flag as well.
But that would introduce policy into the driver...
