Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

[Paul Cercueil]

Hi Maxime,

Le ven. 4 nov. 2022 à 14:18:13 +0100, Maxime Ripard 
<maxime@cerno.tech> 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.


> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/clk/ingenic/cgu.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
> index 1f7ba30f5a1b..0c9c8344ad11 100644
> --- a/drivers/clk/ingenic/cgu.c
> +++ b/drivers/clk/ingenic/cgu.c
> @@ -491,22 +491,23 @@ ingenic_clk_calc_div(struct clk_hw *hw,
>  	return div;
>  }
> 
> -static long
> -ingenic_clk_round_rate(struct clk_hw *hw, unsigned long req_rate,
> -		       unsigned long *parent_rate)
> +static int ingenic_clk_determine_rate(struct clk_hw *hw,
> +				      struct clk_rate_request *req)
>  {
>  	struct ingenic_clk *ingenic_clk = to_ingenic_clk(hw);
>  	const struct ingenic_cgu_clk_info *clk_info = 
> to_clk_info(ingenic_clk);
>  	unsigned int div = 1;
> 
>  	if (clk_info->type & CGU_CLK_DIV)
> -		div = ingenic_clk_calc_div(hw, clk_info, *parent_rate, req_rate);
> +		div = ingenic_clk_calc_div(hw, clk_info, req->best_parent_rate,
> +					   req->rate);

Sorry but I'm not sure that this works.

You replace the "parent_rate" with the "best_parent_rate", and that 
means you only check the requested rate vs. the parent with the highest 
frequency, and not vs. the actual parent that will be used.

Cheers,
-Paul

>  	else if (clk_info->type & CGU_CLK_FIXDIV)
>  		div = clk_info->fixdiv.div;
>  	else if (clk_hw_can_set_rate_parent(hw))
> -		*parent_rate = req_rate;
> +		req->best_parent_rate = req->rate;
> 
> -	return DIV_ROUND_UP(*parent_rate, div);
> +	req->rate = DIV_ROUND_UP(req->best_parent_rate, div);
> +	return 0;
>  }
> 
>  static inline int ingenic_clk_check_stable(struct ingenic_cgu *cgu,
> @@ -626,7 +627,7 @@ static const struct clk_ops ingenic_clk_ops = {
>  	.set_parent = ingenic_clk_set_parent,
> 
>  	.recalc_rate = ingenic_clk_recalc_rate,
> -	.round_rate = ingenic_clk_round_rate,
> +	.determine_rate = ingenic_clk_determine_rate,
>  	.set_rate = ingenic_clk_set_rate,
> 
>  	.enable = ingenic_clk_enable,
> 
> --
> b4 0.11.0-dev-99e3a

Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4344 bytes --]

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@cerno.tech> 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.

> 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/clk/ingenic/cgu.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
> > index 1f7ba30f5a1b..0c9c8344ad11 100644
> > --- a/drivers/clk/ingenic/cgu.c
> > +++ b/drivers/clk/ingenic/cgu.c
> > @@ -491,22 +491,23 @@ ingenic_clk_calc_div(struct clk_hw *hw,
> >  	return div;
> >  }
> > 
> > -static long
> > -ingenic_clk_round_rate(struct clk_hw *hw, unsigned long req_rate,
> > -		       unsigned long *parent_rate)
> > +static int ingenic_clk_determine_rate(struct clk_hw *hw,
> > +				      struct clk_rate_request *req)
> >  {
> >  	struct ingenic_clk *ingenic_clk = to_ingenic_clk(hw);
> >  	const struct ingenic_cgu_clk_info *clk_info =
> > to_clk_info(ingenic_clk);
> >  	unsigned int div = 1;
> > 
> >  	if (clk_info->type & CGU_CLK_DIV)
> > -		div = ingenic_clk_calc_div(hw, clk_info, *parent_rate, req_rate);
> > +		div = ingenic_clk_calc_div(hw, clk_info, req->best_parent_rate,
> > +					   req->rate);
> 
> Sorry but I'm not sure that this works.
> 
> You replace the "parent_rate" with the "best_parent_rate", and that means
> you only check the requested rate vs. the parent with the highest frequency,
> and not vs. the actual parent that will be used.

best_parent_rate is initialized to the current parent rate, not the
parent with the highest frequency:
https://elixir.bootlin.com/linux/v6.1-rc3/source/drivers/clk/clk.c#L1471

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

[Aidan MacDonald]

Maxime Ripard <maxime@cerno.tech> writes:

> 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@cerno.tech> 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.
>

Ideally there should be a way for drivers and the device tree to
say, "clock X must be driven by clock Y", but the clock framework
would be allowed to re-parent clocks freely as long as it doesn't
violate any DT or driver constraints.

That way allowing reparenting doesn't need to be an all-or-nothing
thing, and it doesn't need to be decided at the clock driver level
with special flags.

Regards,
Aidan

>> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>> > ---
>> >  drivers/clk/ingenic/cgu.c | 15 ++++++++-------
>> >  1 file changed, 8 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
>> > index 1f7ba30f5a1b..0c9c8344ad11 100644
>> > --- a/drivers/clk/ingenic/cgu.c
>> > +++ b/drivers/clk/ingenic/cgu.c
>> > @@ -491,22 +491,23 @@ ingenic_clk_calc_div(struct clk_hw *hw,
>> >  	return div;
>> >  }
>> >
>> > -static long
>> > -ingenic_clk_round_rate(struct clk_hw *hw, unsigned long req_rate,
>> > -		       unsigned long *parent_rate)
>> > +static int ingenic_clk_determine_rate(struct clk_hw *hw,
>> > +				      struct clk_rate_request *req)
>> >  {
>> >  	struct ingenic_clk *ingenic_clk = to_ingenic_clk(hw);
>> >  	const struct ingenic_cgu_clk_info *clk_info =
>> > to_clk_info(ingenic_clk);
>> >  	unsigned int div = 1;
>> >
>> >  	if (clk_info->type & CGU_CLK_DIV)
>> > -		div = ingenic_clk_calc_div(hw, clk_info, *parent_rate, req_rate);
>> > +		div = ingenic_clk_calc_div(hw, clk_info, req->best_parent_rate,
>> > +					   req->rate);
>>
>> Sorry but I'm not sure that this works.
>>
>> You replace the "parent_rate" with the "best_parent_rate", and that means
>> you only check the requested rate vs. the parent with the highest frequency,
>> and not vs. the actual parent that will be used.
>
> best_parent_rate is initialized to the current parent rate, not the
> parent with the highest frequency:
> https://elixir.bootlin.com/linux/v6.1-rc3/source/drivers/clk/clk.c#L1471
>
> Maxime

Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

[Maxime Ripard]

Hi,

On Fri, Nov 04, 2022 at 05:35:29PM +0000, Aidan MacDonald wrote:
> 
> Maxime Ripard <maxime@cerno.tech> writes:
> 
> > 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@cerno.tech> 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.
>
> Ideally there should be a way for drivers and the device tree to
> say, "clock X must be driven by clock Y", but the clock framework
> would be allowed to re-parent clocks freely as long as it doesn't
> violate any DT or driver constraints.

I'm not really sure what you mean there, sorry. Isn't it what
assigned-clock-parents/clk_set_parent() at probe, plus a determine_rate
implementation that would affect best_parent_hw would already provide?

> That way allowing reparenting doesn't need to be an all-or-nothing
> thing, and it doesn't need to be decided at the clock driver level
> with special flags.

Like I said, the default implementation is already working to what you
suggested if I understood properly. However, this has never been tested
for any of the drivers in that series so I don't want to introduce (and
debug ;)) regressions in all those drivers that were not setting any
constraint but never actually tested their reparenting code.

So that series is strictly equivalent to what you had before, it's just
explicit now.

If you find that some other decision make sense for your driver in
particular cases, feel free to change it. I barely know most of these
platforms, so I won't be able to make that decision (and test it)
unfortunately.

Maxime

Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

[Aidan MacDonald]

Maxime Ripard <maxime@cerno.tech> writes:

> Hi,
>
> On Fri, Nov 04, 2022 at 05:35:29PM +0000, Aidan MacDonald wrote:
>>
>> Maxime Ripard <maxime@cerno.tech> writes:
>>
>> > 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@cerno.tech> 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.
>>
>> Ideally there should be a way for drivers and the device tree to
>> say, "clock X must be driven by clock Y", but the clock framework
>> would be allowed to re-parent clocks freely as long as it doesn't
>> violate any DT or driver constraints.
>
> I'm not really sure what you mean there, sorry. Isn't it what
> assigned-clock-parents/clk_set_parent() at probe, plus a determine_rate
> implementation that would affect best_parent_hw would already provide?

Assigning the parent clock in the DT works once, at boot, but going off
what you wrote in the commit message, if the clock driver has a
.determine_rate() implementation that *can* reparent clocks then it
probably *will* reparent them, and the DT assignment will be lost.

What I'm suggesting is a runtime constraint that the clock subsystem
would enforce, and actively prevent drivers from changing the parent.
Either explicitly with clk_set_parent() or due to .determine_rate().

That way you could write a .determine_rate() implementation that *can*
select a better parent, but if the DT applies a constraint to fix the
clock to a particular parent, the clock subsystem will force that parent
to be used so you can be sure the clock is never reparented by accident.

>> That way allowing reparenting doesn't need to be an all-or-nothing
>> thing, and it doesn't need to be decided at the clock driver level
>> with special flags.
>
> Like I said, the default implementation is already working to what you
> suggested if I understood properly. However, this has never been tested
> for any of the drivers in that series so I don't want to introduce (and
> debug ;)) regressions in all those drivers that were not setting any
> constraint but never actually tested their reparenting code.
>
> So that series is strictly equivalent to what you had before, it's just
> explicit now.
>
> If you find that some other decision make sense for your driver in
> particular cases, feel free to change it. I barely know most of these
> platforms, so I won't be able to make that decision (and test it)
> unfortunately.
>
> Maxime

That's OK, I didn't review the patch, I'm just making a general
suggestion. :)

Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

[Maxime Ripard]

On Mon, Nov 07, 2022 at 08:57:22PM +0000, Aidan MacDonald wrote:
> 
> Maxime Ripard <maxime@cerno.tech> writes:
> 
> > Hi,
> >
> > On Fri, Nov 04, 2022 at 05:35:29PM +0000, Aidan MacDonald wrote:
> >>
> >> Maxime Ripard <maxime@cerno.tech> writes:
> >>
> >> > 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@cerno.tech> 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.
> >>
> >> Ideally there should be a way for drivers and the device tree to
> >> say, "clock X must be driven by clock Y", but the clock framework
> >> would be allowed to re-parent clocks freely as long as it doesn't
> >> violate any DT or driver constraints.
> >
> > I'm not really sure what you mean there, sorry. Isn't it what
> > assigned-clock-parents/clk_set_parent() at probe, plus a determine_rate
> > implementation that would affect best_parent_hw would already provide?
> 
> Assigning the parent clock in the DT works once, at boot, but going off
> what you wrote in the commit message, if the clock driver has a
> .determine_rate() implementation that *can* reparent clocks then it
> probably *will* reparent them, and the DT assignment will be lost.

Yes, indeed, but assigned-clock-parents never provided any sort of
guarantee on whether or not the clock was allowed to reparent or not.
It's just a one-off thing, right before probe, and a clk_set_parent()
call at probe will override that just fine.

Just like assigned-clock-rates isn't permanent.

> What I'm suggesting is a runtime constraint that the clock subsystem
> would enforce, and actively prevent drivers from changing the parent.
> Either explicitly with clk_set_parent() or due to .determine_rate().
> 
> That way you could write a .determine_rate() implementation that *can*
> select a better parent, but if the DT applies a constraint to fix the
> clock to a particular parent, the clock subsystem will force that parent
> to be used so you can be sure the clock is never reparented by accident.

Yeah, that sounds like a good idea, and CLK_SET_RATE_NO_REPARENT isn't
too far off from this, it's just ignored by clk_set_parent() for now. I
guess we could rename CLK_SET_RATE_NO_REPARENT to CLK_NO_REPARENT, make
clk_set_parent handle it, and set that flag whenever
assigned-clock-parents is set on a clock.

It's out of scope for this series though, and I certainly don't want to
deal with all the regressions it might create :)

Maxime

Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

[Aidan MacDonald]

Stephen Boyd <sboyd@kernel.org> writes:

> Quoting Maxime Ripard (2022-11-09 03:00:45)
>> On Mon, Nov 07, 2022 at 08:57:22PM +0000, Aidan MacDonald wrote:
>> >
>> > Maxime Ripard <maxime@cerno.tech> writes:
>> >
>> > > Hi,
>> > >
>> > > On Fri, Nov 04, 2022 at 05:35:29PM +0000, Aidan MacDonald wrote:
>> >
>> > Assigning the parent clock in the DT works once, at boot, but going off
>> > what you wrote in the commit message, if the clock driver has a
>> > .determine_rate() implementation that *can* reparent clocks then it
>> > probably *will* reparent them, and the DT assignment will be lost.
>>
>> Yes, indeed, but assigned-clock-parents never provided any sort of
>> guarantee on whether or not the clock was allowed to reparent or not.
>> It's just a one-off thing, right before probe, and a clk_set_parent()
>> call at probe will override that just fine.
>>
>> Just like assigned-clock-rates isn't permanent.
>>
>> > What I'm suggesting is a runtime constraint that the clock subsystem
>> > would enforce, and actively prevent drivers from changing the parent.
>> > Either explicitly with clk_set_parent() or due to .determine_rate().
>> >
>> > That way you could write a .determine_rate() implementation that *can*
>> > select a better parent, but if the DT applies a constraint to fix the
>> > clock to a particular parent, the clock subsystem will force that parent
>> > to be used so you can be sure the clock is never reparented by accident.
>>
>> Yeah, that sounds like a good idea, and CLK_SET_RATE_NO_REPARENT isn't
>> too far off from this, it's just ignored by clk_set_parent() for now. I
>> guess we could rename CLK_SET_RATE_NO_REPARENT to CLK_NO_REPARENT, make
>> clk_set_parent handle it, and set that flag whenever
>> assigned-clock-parents is set on a clock.
>>
>> It's out of scope for this series though, and I certainly don't want to
>> deal with all the regressions it might create :)
>>
>
> This sounds like a new dt binding that says the assigned parent should
> never change. It sounds sort of like gpio hogs. A clock-hogs binding?

Ideally we want the clock driver to be able to reparent clocks freely
to get the best rate. But we also need some control over that to stop
consumers from being reparented in undesired ways. Eg. you might want
to make sure the GPU gets its own PLL so it can be reclocked easily,
and putting another device on the GPU's PLL could prevent that.

The only way to achieve this today is (1) never do any reparenting in
the clock driver; and (2) use assigned-clock-parents in the DT to set
up the entire clock tree manually.

Maxime said that (2) is basically wrong -- if assigned-clock-parents
provides no guarantee on what the OS does "after boot" then the OS is
pretty much free to ignore it.

My suggestion: add a per-clock bitmap to keep track of which parents
are allowed. Any operation that would select a parent clock not on the
whitelist should fail. Automatic reparenting should only select from
clocks on the whitelist. And we need new DT bindings for controlling
the whitelist, for example:

    clock-parents-0 = <&clk1>, <&pll_c>;
    clock-parents-1 = <&clk2>, <&pll_a>, <&pll_b>;

This means that clk1 can only have pll_c as a parent, while clk2 can
have pll_a or pll_b as parents. By default every clock will be able
to use any parent, so a list is only needed if the machine needs a
more restrictive policy.

assigned-clock-parents should disable automatic reparenting, but allow
explicit clk_set_parent(). This will allow clock drivers to start doing
reparenting without breaking old DTs.

Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4343 bytes --]

Hi,

On Thu, Mar 23, 2023 at 03:35:30PM +0000, Aidan MacDonald wrote:
> 
> Stephen Boyd <sboyd@kernel.org> writes:
> 
> > Quoting Maxime Ripard (2022-11-09 03:00:45)
> >> On Mon, Nov 07, 2022 at 08:57:22PM +0000, Aidan MacDonald wrote:
> >> >
> >> > Maxime Ripard <maxime@cerno.tech> writes:
> >> >
> >> > > Hi,
> >> > >
> >> > > On Fri, Nov 04, 2022 at 05:35:29PM +0000, Aidan MacDonald wrote:
> >> >
> >> > Assigning the parent clock in the DT works once, at boot, but going off
> >> > what you wrote in the commit message, if the clock driver has a
> >> > .determine_rate() implementation that *can* reparent clocks then it
> >> > probably *will* reparent them, and the DT assignment will be lost.
> >>
> >> Yes, indeed, but assigned-clock-parents never provided any sort of
> >> guarantee on whether or not the clock was allowed to reparent or not.
> >> It's just a one-off thing, right before probe, and a clk_set_parent()
> >> call at probe will override that just fine.
> >>
> >> Just like assigned-clock-rates isn't permanent.
> >>
> >> > What I'm suggesting is a runtime constraint that the clock subsystem
> >> > would enforce, and actively prevent drivers from changing the parent.
> >> > Either explicitly with clk_set_parent() or due to .determine_rate().
> >> >
> >> > That way you could write a .determine_rate() implementation that *can*
> >> > select a better parent, but if the DT applies a constraint to fix the
> >> > clock to a particular parent, the clock subsystem will force that parent
> >> > to be used so you can be sure the clock is never reparented by accident.
> >>
> >> Yeah, that sounds like a good idea, and CLK_SET_RATE_NO_REPARENT isn't
> >> too far off from this, it's just ignored by clk_set_parent() for now. I
> >> guess we could rename CLK_SET_RATE_NO_REPARENT to CLK_NO_REPARENT, make
> >> clk_set_parent handle it, and set that flag whenever
> >> assigned-clock-parents is set on a clock.
> >>
> >> It's out of scope for this series though, and I certainly don't want to
> >> deal with all the regressions it might create :)
> >>
> >
> > This sounds like a new dt binding that says the assigned parent should
> > never change. It sounds sort of like gpio hogs. A clock-hogs binding?
> 
> Ideally we want the clock driver to be able to reparent clocks freely
> to get the best rate. But we also need some control over that to stop
> consumers from being reparented in undesired ways. Eg. you might want
> to make sure the GPU gets its own PLL so it can be reclocked easily,
> and putting another device on the GPU's PLL could prevent that.
> 
> The only way to achieve this today is (1) never do any reparenting in
> the clock driver; and (2) use assigned-clock-parents in the DT to set
> up the entire clock tree manually.
> 
> Maxime said that (2) is basically wrong -- if assigned-clock-parents
> provides no guarantee on what the OS does "after boot" then the OS is
> pretty much free to ignore it.

I didn't really say it's wrong, just that it never provided the
guarantee you expect it to provide. I can't really say whether it's an
issue or not on your platform.

It's mostly unrelated to this series though, none of these patches
affect that behavior in one way or the other.

> My suggestion: add a per-clock bitmap to keep track of which parents
> are allowed. Any operation that would select a parent clock not on the
> whitelist should fail. Automatic reparenting should only select from
> clocks on the whitelist. And we need new DT bindings for controlling
> the whitelist, for example:
> 
>     clock-parents-0 = <&clk1>, <&pll_c>;
>     clock-parents-1 = <&clk2>, <&pll_a>, <&pll_b>;
> 
> This means that clk1 can only have pll_c as a parent, while clk2 can
> have pll_a or pll_b as parents. By default every clock will be able
> to use any parent, so a list is only needed if the machine needs a
> more restrictive policy.
> 
> assigned-clock-parents should disable automatic reparenting, but allow
> explicit clk_set_parent(). This will allow clock drivers to start doing
> reparenting without breaking old DTs.

I'm generally not a fan of putting all these policies in the device
tree. Do you have an example where it wouldn't be possible to do exactly
this from the driver itself?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

[Aidan MacDonald]

Maxime Ripard <maxime@cerno.tech> writes:

> On Thu, Mar 23, 2023 at 03:35:30PM +0000, Aidan MacDonald wrote:
>>
>> Stephen Boyd <sboyd@kernel.org> writes:
>>
>> > Quoting Maxime Ripard (2022-11-09 03:00:45)
>> >> On Mon, Nov 07, 2022 at 08:57:22PM +0000, Aidan MacDonald wrote:
>> >> >
>> >> > Maxime Ripard <maxime@cerno.tech> writes:
>> >> >
>> >> > > Hi,
>> >> > >
>> >> > > On Fri, Nov 04, 2022 at 05:35:29PM +0000, Aidan MacDonald wrote:
>> >> >
>> >> > Assigning the parent clock in the DT works once, at boot, but going off
>> >> > what you wrote in the commit message, if the clock driver has a
>> >> > .determine_rate() implementation that *can* reparent clocks then it
>> >> > probably *will* reparent them, and the DT assignment will be lost.
>> >>
>> >> Yes, indeed, but assigned-clock-parents never provided any sort of
>> >> guarantee on whether or not the clock was allowed to reparent or not.
>> >> It's just a one-off thing, right before probe, and a clk_set_parent()
>> >> call at probe will override that just fine.
>> >>
>> >> Just like assigned-clock-rates isn't permanent.
>> >>
>> >> > What I'm suggesting is a runtime constraint that the clock subsystem
>> >> > would enforce, and actively prevent drivers from changing the parent.
>> >> > Either explicitly with clk_set_parent() or due to .determine_rate().
>> >> >
>> >> > That way you could write a .determine_rate() implementation that *can*
>> >> > select a better parent, but if the DT applies a constraint to fix the
>> >> > clock to a particular parent, the clock subsystem will force that parent
>> >> > to be used so you can be sure the clock is never reparented by accident.
>> >>
>> >> Yeah, that sounds like a good idea, and CLK_SET_RATE_NO_REPARENT isn't
>> >> too far off from this, it's just ignored by clk_set_parent() for now. I
>> >> guess we could rename CLK_SET_RATE_NO_REPARENT to CLK_NO_REPARENT, make
>> >> clk_set_parent handle it, and set that flag whenever
>> >> assigned-clock-parents is set on a clock.
>> >>
>> >> It's out of scope for this series though, and I certainly don't want to
>> >> deal with all the regressions it might create :)
>> >>
>> >
>> > This sounds like a new dt binding that says the assigned parent should
>> > never change. It sounds sort of like gpio hogs. A clock-hogs binding?
>>
>> Ideally we want the clock driver to be able to reparent clocks freely
>> to get the best rate. But we also need some control over that to stop
>> consumers from being reparented in undesired ways. Eg. you might want
>> to make sure the GPU gets its own PLL so it can be reclocked easily,
>> and putting another device on the GPU's PLL could prevent that.
>>
>> The only way to achieve this today is (1) never do any reparenting in
>> the clock driver; and (2) use assigned-clock-parents in the DT to set
>> up the entire clock tree manually.
>>
>> Maxime said that (2) is basically wrong -- if assigned-clock-parents
>> provides no guarantee on what the OS does "after boot" then the OS is
>> pretty much free to ignore it.
>
> I didn't really say it's wrong, just that it never provided the
> guarantee you expect it to provide. I can't really say whether it's an
> issue or not on your platform.
>
> It's mostly unrelated to this series though, none of these patches
> affect that behavior in one way or the other.

I know. Sorry for derailing your patch :(

>> My suggestion: add a per-clock bitmap to keep track of which parents
>> are allowed. Any operation that would select a parent clock not on the
>> whitelist should fail. Automatic reparenting should only select from
>> clocks on the whitelist. And we need new DT bindings for controlling
>> the whitelist, for example:
>>
>>     clock-parents-0 = <&clk1>, <&pll_c>;
>>     clock-parents-1 = <&clk2>, <&pll_a>, <&pll_b>;
>>
>> This means that clk1 can only have pll_c as a parent, while clk2 can
>> have pll_a or pll_b as parents. By default every clock will be able
>> to use any parent, so a list is only needed if the machine needs a
>> more restrictive policy.
>>
>> assigned-clock-parents should disable automatic reparenting, but allow
>> explicit clk_set_parent(). This will allow clock drivers to start doing
>> reparenting without breaking old DTs.
>
> I'm generally not a fan of putting all these policies in the device
> tree. Do you have an example where it wouldn't be possible to do exactly
> this from the driver itself?
>
> Maxime

I'm confused. What's implicit in the example is clk1 and clk2 might
have *other* possible choices of parent clock and the device tree is
limiting what the OS is allowed to choose.

Why would you put such arbitrary limitations into the driver? They
would be different from machine to machine, unless the clock tree is
so simple there is only *one* meaningful way to configure it. Most
SoCs are complicated enough that there will be tradeoffs depending
on what peripherals you are using (typically a single machine will
not use *every* peripheral device provided by the SoC).

Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

[Maxime Ripard]

On Fri, Mar 24, 2023 at 08:58:48PM +0000, Aidan MacDonald wrote:
> >> My suggestion: add a per-clock bitmap to keep track of which parents
> >> are allowed. Any operation that would select a parent clock not on the
> >> whitelist should fail. Automatic reparenting should only select from
> >> clocks on the whitelist. And we need new DT bindings for controlling
> >> the whitelist, for example:
> >>
> >>     clock-parents-0 = <&clk1>, <&pll_c>;
> >>     clock-parents-1 = <&clk2>, <&pll_a>, <&pll_b>;
> >>
> >> This means that clk1 can only have pll_c as a parent, while clk2 can
> >> have pll_a or pll_b as parents. By default every clock will be able
> >> to use any parent, so a list is only needed if the machine needs a
> >> more restrictive policy.
> >>
> >> assigned-clock-parents should disable automatic reparenting, but allow
> >> explicit clk_set_parent(). This will allow clock drivers to start doing
> >> reparenting without breaking old DTs.
> >
> > I'm generally not a fan of putting all these policies in the device
> > tree. Do you have an example where it wouldn't be possible to do exactly
> > this from the driver itself?
> 
> I'm confused. What's implicit in the example is clk1 and clk2 might
> have *other* possible choices of parent clock and the device tree is
> limiting what the OS is allowed to choose.
>
> Why would you put such arbitrary limitations into the driver?

Why would we put such arbitrary limitations in the firmware? As this
entire thread can attest, people are already using the device tree to
work around the limitations of the Linux driver, or reduce the
features of Linux because they can rely on the device tree. Either
way, it's linked to the state of the Linux driver, and any other OS or
Linux version could very well implement something more dynamic.

> They would be different from machine to machine, unless the clock
> tree is so simple there is only *one* meaningful way to configure
> it.

If we look at the device trees we have in-tree, most of the users of
assigned-clocks are the same from one board to another.

> Most SoCs are complicated enough that there will be tradeoffs
> depending on what peripherals you are using (typically a single
> machine will not use *every* peripheral device provided by the SoC).

We already have APIs to lock parents or rates on a given clock from
the consumer. It's far superior (feature-wise) than what the device
tree will ever offer because it's code, and it depends on the usage
already since an unused driver won't probe.

Maxime

Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

[Paul Cercueil]

Hi Maxime,

Le lundi 27 mars 2023 à 21:24 +0200, Maxime Ripard a écrit :
> On Fri, Mar 24, 2023 at 08:58:48PM +0000, Aidan MacDonald wrote:
> > > > My suggestion: add a per-clock bitmap to keep track of which
> > > > parents
> > > > are allowed. Any operation that would select a parent clock not
> > > > on the
> > > > whitelist should fail. Automatic reparenting should only select
> > > > from
> > > > clocks on the whitelist. And we need new DT bindings for
> > > > controlling
> > > > the whitelist, for example:
> > > > 
> > > >     clock-parents-0 = <&clk1>, <&pll_c>;
> > > >     clock-parents-1 = <&clk2>, <&pll_a>, <&pll_b>;
> > > > 
> > > > This means that clk1 can only have pll_c as a parent, while
> > > > clk2 can
> > > > have pll_a or pll_b as parents. By default every clock will be
> > > > able
> > > > to use any parent, so a list is only needed if the machine
> > > > needs a
> > > > more restrictive policy.
> > > > 
> > > > assigned-clock-parents should disable automatic reparenting,
> > > > but allow
> > > > explicit clk_set_parent(). This will allow clock drivers to
> > > > start doing
> > > > reparenting without breaking old DTs.
> > > 
> > > I'm generally not a fan of putting all these policies in the
> > > device
> > > tree. Do you have an example where it wouldn't be possible to do
> > > exactly
> > > this from the driver itself?
> > 
> > I'm confused. What's implicit in the example is clk1 and clk2 might
> > have *other* possible choices of parent clock and the device tree
> > is
> > limiting what the OS is allowed to choose.
> > 
> > Why would you put such arbitrary limitations into the driver?
> 
> Why would we put such arbitrary limitations in the firmware? As this
> entire thread can attest, people are already using the device tree to
> work around the limitations of the Linux driver, or reduce the
> features of Linux because they can rely on the device tree. Either
> way, it's linked to the state of the Linux driver, and any other OS
> or
> Linux version could very well implement something more dynamic.

Probably because if we have to choose between setting policy in the
kernel or in the firmware, it is arguably better to set it in the
firmware.

Especially when talking about clocks, as the firmware is already the
one programming the boot clocks.

Cheers,
-Paul

> > They would be different from machine to machine, unless the clock
> > tree is so simple there is only *one* meaningful way to configure
> > it.
> 
> If we look at the device trees we have in-tree, most of the users of
> assigned-clocks are the same from one board to another.
> 
> > Most SoCs are complicated enough that there will be tradeoffs
> > depending on what peripherals you are using (typically a single
> > machine will not use *every* peripheral device provided by the
> > SoC).
> 
> We already have APIs to lock parents or rates on a given clock from
> the consumer. It's far superior (feature-wise) than what the device
> tree will ever offer because it's code, and it depends on the usage
> already since an unused driver won't probe.
> 
> Maxime

Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3038 bytes --]

On Wed, Apr 05, 2023 at 02:57:26PM +0200, Paul Cercueil wrote:
> Le lundi 27 mars 2023 à 21:24 +0200, Maxime Ripard a écrit :
> > On Fri, Mar 24, 2023 at 08:58:48PM +0000, Aidan MacDonald wrote:
> > > > > My suggestion: add a per-clock bitmap to keep track of which
> > > > > parents
> > > > > are allowed. Any operation that would select a parent clock not
> > > > > on the
> > > > > whitelist should fail. Automatic reparenting should only select
> > > > > from
> > > > > clocks on the whitelist. And we need new DT bindings for
> > > > > controlling
> > > > > the whitelist, for example:
> > > > > 
> > > > >     clock-parents-0 = <&clk1>, <&pll_c>;
> > > > >     clock-parents-1 = <&clk2>, <&pll_a>, <&pll_b>;
> > > > > 
> > > > > This means that clk1 can only have pll_c as a parent, while
> > > > > clk2 can
> > > > > have pll_a or pll_b as parents. By default every clock will be
> > > > > able
> > > > > to use any parent, so a list is only needed if the machine
> > > > > needs a
> > > > > more restrictive policy.
> > > > > 
> > > > > assigned-clock-parents should disable automatic reparenting,
> > > > > but allow
> > > > > explicit clk_set_parent(). This will allow clock drivers to
> > > > > start doing
> > > > > reparenting without breaking old DTs.
> > > > 
> > > > I'm generally not a fan of putting all these policies in the
> > > > device
> > > > tree. Do you have an example where it wouldn't be possible to do
> > > > exactly
> > > > this from the driver itself?
> > > 
> > > I'm confused. What's implicit in the example is clk1 and clk2 might
> > > have *other* possible choices of parent clock and the device tree
> > > is
> > > limiting what the OS is allowed to choose.
> > > 
> > > Why would you put such arbitrary limitations into the driver?
> > 
> > Why would we put such arbitrary limitations in the firmware? As this
> > entire thread can attest, people are already using the device tree to
> > work around the limitations of the Linux driver, or reduce the
> > features of Linux because they can rely on the device tree. Either
> > way, it's linked to the state of the Linux driver, and any other OS
> > or
> > Linux version could very well implement something more dynamic.
> 
> Probably because if we have to choose between setting policy in the
> kernel or in the firmware, it is arguably better to set it in the
> firmware.

I have a very different view on this I guess. Firmware is (most of the
time) hard to update, and the policy depend on the state of support of a
given OS so it's likely to evolve. The kernel is the best place to me to
put that kind of policy. Why do you think differently?

> Especially when talking about clocks, as the firmware is already the
> one programming the boot clocks.

I'm not sure what your point is there. I don't think I ever saw a
firmware getting the clocks right for every possible scenario on a given
platform. And if it was indeed the case, then we wouldn't even a kernel
driver.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

[Paul Cercueil]

Le mercredi 05 avril 2023 à 16:50 +0200, Maxime Ripard a écrit :
> On Wed, Apr 05, 2023 at 02:57:26PM +0200, Paul Cercueil wrote:
> > Le lundi 27 mars 2023 à 21:24 +0200, Maxime Ripard a écrit :
> > > On Fri, Mar 24, 2023 at 08:58:48PM +0000, Aidan MacDonald wrote:
> > > > > > My suggestion: add a per-clock bitmap to keep track of
> > > > > > which
> > > > > > parents
> > > > > > are allowed. Any operation that would select a parent clock
> > > > > > not
> > > > > > on the
> > > > > > whitelist should fail. Automatic reparenting should only
> > > > > > select
> > > > > > from
> > > > > > clocks on the whitelist. And we need new DT bindings for
> > > > > > controlling
> > > > > > the whitelist, for example:
> > > > > > 
> > > > > >     clock-parents-0 = <&clk1>, <&pll_c>;
> > > > > >     clock-parents-1 = <&clk2>, <&pll_a>, <&pll_b>;
> > > > > > 
> > > > > > This means that clk1 can only have pll_c as a parent, while
> > > > > > clk2 can
> > > > > > have pll_a or pll_b as parents. By default every clock will
> > > > > > be
> > > > > > able
> > > > > > to use any parent, so a list is only needed if the machine
> > > > > > needs a
> > > > > > more restrictive policy.
> > > > > > 
> > > > > > assigned-clock-parents should disable automatic
> > > > > > reparenting,
> > > > > > but allow
> > > > > > explicit clk_set_parent(). This will allow clock drivers to
> > > > > > start doing
> > > > > > reparenting without breaking old DTs.
> > > > > 
> > > > > I'm generally not a fan of putting all these policies in the
> > > > > device
> > > > > tree. Do you have an example where it wouldn't be possible to
> > > > > do
> > > > > exactly
> > > > > this from the driver itself?
> > > > 
> > > > I'm confused. What's implicit in the example is clk1 and clk2
> > > > might
> > > > have *other* possible choices of parent clock and the device
> > > > tree
> > > > is
> > > > limiting what the OS is allowed to choose.
> > > > 
> > > > Why would you put such arbitrary limitations into the driver?
> > > 
> > > Why would we put such arbitrary limitations in the firmware? As
> > > this
> > > entire thread can attest, people are already using the device
> > > tree to
> > > work around the limitations of the Linux driver, or reduce the
> > > features of Linux because they can rely on the device tree.
> > > Either
> > > way, it's linked to the state of the Linux driver, and any other
> > > OS
> > > or
> > > Linux version could very well implement something more dynamic.
> > 
> > Probably because if we have to choose between setting policy in the
> > kernel or in the firmware, it is arguably better to set it in the
> > firmware.
> 
> I have a very different view on this I guess. Firmware is (most of
> the
> time) hard to update, and the policy depend on the state of support
> of a
> given OS so it's likely to evolve. The kernel is the best place to me
> to
> put that kind of policy. Why do you think differently?

Because the clocks configuration can be board-specific. And you don't
really want board-specific stuff in the driver.

If we take the Ingenic JZ4770 SoC as example, on one board we parent
everything we can to the PLL1 clock and set it to 432 MHz (the least
common multiple). Then the PLL0 (which drives the DDR and CPU clocks)
can be updated to overclock/underclock the CPU and RAM.

So should that be hardcoded in the driver? Well, for a different board,
for which overclock is not really needed, it's better to parent
everything to PLL0 so that PLL1 can be shut down to save power. So what
policy should be hardcoded in the driver?

> 
> > Especially when talking about clocks, as the firmware is already
> > the
> > one programming the boot clocks.
> 
> I'm not sure what your point is there. I don't think I ever saw a
> firmware getting the clocks right for every possible scenario on a
> given
> platform. And if it was indeed the case, then we wouldn't even a
> kernel
> driver.

My point is that there is already policy in how the firmware sets up
the hardware; and most often than not, the kernel driver won't change
that (e.g. you're probably not going to touch the DDR clock). It's
better to have all policy in the firmware then, instead of having some
in the firmware, and some in the kernel driver.

Cheers,
-Paul

Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

[Paul Cercueil]

Hi Maxime,

Le ven. 4 nov. 2022 à 15:59:46 +0100, Maxime Ripard 
<maxime@cerno.tech> 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@cerno.tech> 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... The fact that I 
don't want the MMC parented to the PLLs, doesn't mean that it's an 
invalid configuration per se.

Cheers,
-Paul

>> 
>>  > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>>  > ---
>>  >  drivers/clk/ingenic/cgu.c | 15 ++++++++-------
>>  >  1 file changed, 8 insertions(+), 7 deletions(-)
>>  >
>>  > diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
>>  > index 1f7ba30f5a1b..0c9c8344ad11 100644
>>  > --- a/drivers/clk/ingenic/cgu.c
>>  > +++ b/drivers/clk/ingenic/cgu.c
>>  > @@ -491,22 +491,23 @@ ingenic_clk_calc_div(struct clk_hw *hw,
>>  >  	return div;
>>  >  }
>>  >
>>  > -static long
>>  > -ingenic_clk_round_rate(struct clk_hw *hw, unsigned long req_rate,
>>  > -		       unsigned long *parent_rate)
>>  > +static int ingenic_clk_determine_rate(struct clk_hw *hw,
>>  > +				      struct clk_rate_request *req)
>>  >  {
>>  >  	struct ingenic_clk *ingenic_clk = to_ingenic_clk(hw);
>>  >  	const struct ingenic_cgu_clk_info *clk_info =
>>  > to_clk_info(ingenic_clk);
>>  >  	unsigned int div = 1;
>>  >
>>  >  	if (clk_info->type & CGU_CLK_DIV)
>>  > -		div = ingenic_clk_calc_div(hw, clk_info, *parent_rate, 
>> req_rate);
>>  > +		div = ingenic_clk_calc_div(hw, clk_info, req->best_parent_rate,
>>  > +					   req->rate);
>> 
>>  Sorry but I'm not sure that this works.
>> 
>>  You replace the "parent_rate" with the "best_parent_rate", and that 
>> means
>>  you only check the requested rate vs. the parent with the highest 
>> frequency,
>>  and not vs. the actual parent that will be used.
> 
> best_parent_rate is initialized to the current parent rate, not the
> parent with the highest frequency:
> https://elixir.bootlin.com/linux/v6.1-rc3/source/drivers/clk/clk.c#L1471
> 
> Maxime

Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

[Maxime Ripard]

Hi Paul,

On Sat, Nov 05, 2022 at 10:33:54AM +0000, Paul Cercueil wrote:
> Hi Maxime,
> 
> Le ven. 4 nov. 2022 à 15:59:46 +0100, Maxime Ripard <maxime@cerno.tech> 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@cerno.tech> 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...

I'm not sure why you're bringing policies into that discussion. There's
plenty of policy in the driver already, and the current code doesn't do
something that the old wasn't doing (implicitly).

And there's plenty of policies in drivers in general. Whether you limit
the rate or not, whether you allow reparenting or not, even the
CLK_SET_RATE_NO_REPARENT flag mentioned above is a policy decision set
by drivers.

> The fact that I don't want the MMC parented to the PLLs, doesn't mean
> that it's an invalid configuration per se.

Sure, and that's another policy :)

Maxime

Re: [PATCH v2 56/65] clk: ingenic: cgu: Switch to determine_rate

[Paul Cercueil]

Hi Maxime,

Le mer. 9 nov. 2022 à 11:53:01 +0100, Maxime Ripard 
<maxime@cerno.tech> a écrit :
> Hi Paul,
> 
> On Sat, Nov 05, 2022 at 10:33:54AM +0000, Paul Cercueil wrote:
>>  Hi Maxime,
>> 
>>  Le ven. 4 nov. 2022 à 15:59:46 +0100, Maxime Ripard 
>> <maxime@cerno.tech> 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@cerno.tech> 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...
> 
> I'm not sure why you're bringing policies into that discussion. 
> There's
> plenty of policy in the driver already, and the current code doesn't 
> do
> something that the old wasn't doing (implicitly).

Yes, I was just talking about the CLK_SET_RATE_NO_REPARENT flag adding 
policy. The fact that there's plenty of policy in the driver already is 
not an argument for adding some more.

> And there's plenty of policies in drivers in general. Whether you 
> limit
> the rate or not, whether you allow reparenting or not, even the
> CLK_SET_RATE_NO_REPARENT flag mentioned above is a policy decision set
> by drivers.

Allowing reparenting and not limiting the rates is not a policy, it's 
just following what the hardware allows you to do. The absence of 
policy means that the driver allows you to configure the hardware in 
any way you might want to.

Limiting rates, forbidding reparenting, that's policy, and it doesn't 
belong in a driver.

You can argue that choosing not to reparent on rate change is a policy, 
and it is. That's why we need a way to enforce these policies outside 
the driver.

>>  The fact that I don't want the MMC parented to the PLLs, doesn't 
>> mean
>>  that it's an invalid configuration per se.
> 
> Sure, and that's another policy :)

A policy that is not enforced by the driver.

Going back to the patch itself... I am fine with the change, although 
the patch description should probably be updated. We have .set_parent 
callbacks to configure clocks from DT, there's nothing more to it.

Cheers,
-Paul

Re: [PATCH v2 43/65] ASoC: tlv320aic32x4: Add a determine_rate hook

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1313 bytes --]

On Fri, Nov 04, 2022 at 02:18:00PM +0100, Maxime Ripard wrote:

> 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 latter case would be equivalent to setting the flag
> CLK_SET_RATE_NO_REPARENT, together with setting our determine_rate hook
> to __clk_mux_determine_rate(). Indeed, if no determine_rate
> implementation is provided, clk_round_rate() (through
> clk_core_round_rate_nolock()) will call itself on the parent if
> CLK_SET_RATE_PARENT is set, and will not change the clock rate
> otherwise. __clk_mux_determine_rate() has the exact same behavior when
> CLK_SET_RATE_NO_REPARENT is set.

> And if it was an oversight, then we are at least explicit about our
> behavior now and it can be further refined down the line.

Given that the current approach involves patching every single user to
set a default implementation it feels like it might be more
straightforward to just have the clock API use that implementation if
none is defined - as you say there's already a flag to indicate the
unusual case where there's a solid reason to prevent reparenting.  It
feels like the resulting API is more straightforward.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 43/65] ASoC: tlv320aic32x4: Add a determine_rate hook

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]

Hi Mark,

On Fri, Nov 04, 2022 at 03:44:53PM +0000, Mark Brown wrote:
> On Fri, Nov 04, 2022 at 02:18:00PM +0100, Maxime Ripard wrote:
> 
> > 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 latter case would be equivalent to setting the flag
> > CLK_SET_RATE_NO_REPARENT, together with setting our determine_rate hook
> > to __clk_mux_determine_rate(). Indeed, if no determine_rate
> > implementation is provided, clk_round_rate() (through
> > clk_core_round_rate_nolock()) will call itself on the parent if
> > CLK_SET_RATE_PARENT is set, and will not change the clock rate
> > otherwise. __clk_mux_determine_rate() has the exact same behavior when
> > CLK_SET_RATE_NO_REPARENT is set.
> 
> > And if it was an oversight, then we are at least explicit about our
> > behavior now and it can be further refined down the line.
> 
> Given that the current approach involves patching every single user to
> set a default implementation it feels like it might be more
> straightforward to just have the clock API use that implementation if
> none is defined - as you say there's already a flag to indicate the
> unusual case where there's a solid reason to prevent reparenting.  It
> feels like the resulting API is more straightforward.

That would be another solution indeed. The thing is, most places where
determine_rate is missing seems to be oversight, and the flag is missing
as well.

Just filling determine_rate if it's missing with
__clk_mux_determine_rate will possibly pick different parents, and I'm
fairly certain that this have never been tested on most platforms, and
will be completely broken. And I don't really want to play a game of
whack-a-mole adding that flag everywhere it turns out it's broken.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 43/65] ASoC: tlv320aic32x4: Add a determine_rate hook

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

On Fri, Nov 04, 2022 at 04:51:23PM +0100, Maxime Ripard wrote:

> Just filling determine_rate if it's missing with
> __clk_mux_determine_rate will possibly pick different parents, and I'm
> fairly certain that this have never been tested on most platforms, and
> will be completely broken. And I don't really want to play a game of
> whack-a-mole adding that flag everywhere it turns out it's broken.

Well, hopefully everyone for whom it's an issue currently will be
objecting to this version of the change anyway so we'll either know
where to set the flag or we'll get the whack-a-mole with the series
being merged?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 43/65] ASoC: tlv320aic32x4: Add a determine_rate hook

[Maxime Ripard]

Hi Mark,

On Fri, Nov 04, 2022 at 03:59:53PM +0000, Mark Brown wrote:
> On Fri, Nov 04, 2022 at 04:51:23PM +0100, Maxime Ripard wrote:
> 
> > Just filling determine_rate if it's missing with
> > __clk_mux_determine_rate will possibly pick different parents, and I'm
> > fairly certain that this have never been tested on most platforms, and
> > will be completely broken. And I don't really want to play a game of
> > whack-a-mole adding that flag everywhere it turns out it's broken.
> 
> Well, hopefully everyone for whom it's an issue currently will be
> objecting to this version of the change anyway so we'll either know
> where to set the flag or we'll get the whack-a-mole with the series
> being merged?

I'm sorry, I'm not sure what you mean here. The only issue to fix at the
moment is that determine_rate and set_parent aren't coupled, and it led
to issues due to oversight.

I initially added a warning but Stephen wanted to fix all users in that
case and make that an error instead.

If I filled __clk_mux_determine_rate into clocks that weren't using it
before, I would change their behavior. With that flag set, on all users
I add __clk_mux_determine_rate to, the behavior is the same than what we
previously had, so the risk of regressions is minimal, and everything
should keep going like it was?

Maxime

Re: [PATCH v2 43/65] ASoC: tlv320aic32x4: Add a determine_rate hook

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1418 bytes --]

On Mon, Nov 07, 2022 at 09:43:22AM +0100, Maxime Ripard wrote:
> On Fri, Nov 04, 2022 at 03:59:53PM +0000, Mark Brown wrote:

> > Well, hopefully everyone for whom it's an issue currently will be
> > objecting to this version of the change anyway so we'll either know
> > where to set the flag or we'll get the whack-a-mole with the series
> > being merged?

> I'm sorry, I'm not sure what you mean here. The only issue to fix at the
> moment is that determine_rate and set_parent aren't coupled, and it led
> to issues due to oversight.

> I initially added a warning but Stephen wanted to fix all users in that
> case and make that an error instead.

My suggestion is that instead of doing either of these things it'd be
quicker and less error prone to just fix the core to provide the default
implementation if nothing more specific is provided.  Any issues that
causes would already be present with your current series.

> If I filled __clk_mux_determine_rate into clocks that weren't using it
> before, I would change their behavior. With that flag set, on all users
> I add __clk_mux_determine_rate to, the behavior is the same than what we
> previously had, so the risk of regressions is minimal, and everything
> should keep going like it was?

The series does fill in __clk_mux_determine_rate for everything though -
if it was just assumed by default the only thing that'd be needed would
be adding the flag.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 43/65] ASoC: tlv320aic32x4: Add a determine_rate hook

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2064 bytes --]

On Mon, Nov 07, 2022 at 12:06:07PM +0000, Mark Brown wrote:
> On Mon, Nov 07, 2022 at 09:43:22AM +0100, Maxime Ripard wrote:
> > On Fri, Nov 04, 2022 at 03:59:53PM +0000, Mark Brown wrote:
> 
> > > Well, hopefully everyone for whom it's an issue currently will be
> > > objecting to this version of the change anyway so we'll either know
> > > where to set the flag or we'll get the whack-a-mole with the series
> > > being merged?
> 
> > I'm sorry, I'm not sure what you mean here. The only issue to fix at the
> > moment is that determine_rate and set_parent aren't coupled, and it led
> > to issues due to oversight.
> 
> > I initially added a warning but Stephen wanted to fix all users in that
> > case and make that an error instead.
> 
> My suggestion is that instead of doing either of these things it'd be
> quicker and less error prone to just fix the core to provide the default
> implementation if nothing more specific is provided.  Any issues that
> causes would already be present with your current series.
> 
> > If I filled __clk_mux_determine_rate into clocks that weren't using it
> > before, I would change their behavior. With that flag set, on all users
> > I add __clk_mux_determine_rate to, the behavior is the same than what we
> > previously had, so the risk of regressions is minimal, and everything
> > should keep going like it was?
> 
> The series does fill in __clk_mux_determine_rate for everything though -
> if it was just assumed by default the only thing that'd be needed would
> be adding the flag.

The behavior assumed by default was equivalent to
__clk_mux_determine_rate + CLK_SET_RATE_NO_REPARENT. We could indeed set
both if determine_rate is missing in the core, but that's unprecedented
in the clock framework so I think we'll want Stephen to comment here :)

It's also replacing one implicit behavior by another. The point of this
series was to raise awareness on that particular point, so I'm not sure
it actually fixes things. We'll see what Stephen thinks about it.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 43/65] ASoC: tlv320aic32x4: Add a determine_rate hook

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]

On Mon, Nov 07, 2022 at 04:26:03PM +0100, Maxime Ripard wrote:
> On Mon, Nov 07, 2022 at 12:06:07PM +0000, Mark Brown wrote:
> > On Mon, Nov 07, 2022 at 09:43:22AM +0100, Maxime Ripard wrote:

> > The series does fill in __clk_mux_determine_rate for everything though -
> > if it was just assumed by default the only thing that'd be needed would
> > be adding the flag.

> The behavior assumed by default was equivalent to
> __clk_mux_determine_rate + CLK_SET_RATE_NO_REPARENT. We could indeed set
> both if determine_rate is missing in the core, but that's unprecedented
> in the clock framework so I think we'll want Stephen to comment here :)

> It's also replacing one implicit behavior by another. The point of this
> series was to raise awareness on that particular point, so I'm not sure
> it actually fixes things. We'll see what Stephen thinks about it.

We could also just set the operation and still require the flag to be
specified.  I'm a little surprised to learn that it's something you
might want to override, never mind that the API didn't have a default -
it feels like a bit of a landmine that this is the case and is probably
why there's so many cases to fix up.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 43/65] ASoC: tlv320aic32x4: Add a determine_rate hook

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3071 bytes --]

Hi Stephen,

On Wed, Mar 22, 2023 at 04:31:04PM -0700, Stephen Boyd wrote:
> > It's also replacing one implicit behavior by another. The point of this
> > series was to raise awareness on that particular point, so I'm not sure
> > it actually fixes things. We'll see what Stephen thinks about it.
> > 
> 
> Right. A decade ago (!) when determine_rate() was introduced we
> introduced CLK_SET_RATE_NO_REPARENT and set it on each mux user (see
> commit  819c1de344c5 ("clk: add CLK_SET_RATE_NO_REPARENT flag")). This
> way driver behavior wouldn't change and the status quo would be
> maintained, i.e. that clk_set_rate() on a mux wouldn't change parents.
> We didn't enforce that determine_rate exists if the set_parent() op
> existed at the same time though. Probably an oversight.
> 
> Most of the replies to this series have been "DT is setting the parent",
> which makes me believe that there are 'assigned-clock-parents' being
> used.

Yes, that's my understanding as well.

> The clk_set_parent() path is valid for those cases. Probably nobody
> cares about determine_rate because they don't set rates on these clks.
> Some drivers even explicitly left out determine_rate()/round_rate()
> because they didn't want to have some other clk round up to the mux
> and change the parent.
> 
> Eventually we want drivers to migrate to determine_rate op so we can get
> rid of the round_rate op and save a pointer (we're so greedy). It's been
> 10 years though, and that hasn't been done. Sigh! I can see value in
> this series from the angle of migrating, but adding a determine_rate op
> when there isn't a round_rate op makes it hard to reason about. What if
> something copies the clk_ops or sets a different flag? Now we've just
> added parent changing support to clk_set_rate(). What if the clk has
> CLK_SET_RATE_PARENT flag set? Now we're going to ask the parent clk to
> change rate. Fun bugs.
> 
> TL;DR: If the set_parent op exists but determine_rate/round_rate doesn't
> then the clk is a mux that doesn't want to support clk_set_rate(). Make
> a new mux function that's the contents of the CLK_SET_RATE_NO_REPARENT
> branch in clk_mux_determine_rate_flags() and call that directly from the
> clk_ops so it is clear what's happening,
> clk_hw_mux_same_parent_determine_rate() or something with a better name.
> Otherwise migrate the explicit determine_rate op to this new function
> and don't set the flag.
> 
> It may be possible to entirely remove the CLK_SET_RATE_NO_REPARENT flag
> with this design, if the determine_rate clk_op can call the inner
> wrapper function instead of __clk_mux_determine_rate*() (those
> underscores are awful, we should just prefix them with clk_hw_mux_*()
> and live happier). That should be another patch series.

Sorry but it's not really clear to me what you expect in the v2 of this
series (if you even expect one). It looks that you don't like the
assignment-if-missing idea Mark suggested, but should I just
rebase/resend or did you expect something else?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 21/65] clk: davinci: da8xx-cfgchip: Add a determine_rate hook

[David Lechner]

On 11/4/22 8:17 AM, Maxime Ripard wrote:
> The Davinci DA8xxx cfgchip mux clock 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 parent is defined in the device tree and is not expected to change
at runtime, so if I am understanding the patch correctly, setting the
CLK_SET_RATE_NO_REPARENT flag seems correct.

> 
> The latter case would be equivalent to setting the flag
> CLK_SET_RATE_NO_REPARENT, together with setting our determine_rate hook
> to __clk_mux_determine_rate(). Indeed, if no determine_rate
> implementation is provided, clk_round_rate() (through
> clk_core_round_rate_nolock()) will call itself on the parent if
> CLK_SET_RATE_PARENT is set, and will not change the clock rate
> otherwise. __clk_mux_determine_rate() has the exact same behavior when
> CLK_SET_RATE_NO_REPARENT is set.
> 
> And if it was an oversight, then we are at least explicit about our
> behavior now and it can be further refined down the line.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/clk/davinci/da8xx-cfgchip.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/davinci/da8xx-cfgchip.c b/drivers/clk/davinci/da8xx-cfgchip.c
> index 4103d605e804..c04276bc4051 100644
> --- a/drivers/clk/davinci/da8xx-cfgchip.c
> +++ b/drivers/clk/davinci/da8xx-cfgchip.c
> @@ -229,6 +229,7 @@ static u8 da8xx_cfgchip_mux_clk_get_parent(struct clk_hw *hw)
>   }
>   
>   static const struct clk_ops da8xx_cfgchip_mux_clk_ops = {
> +	.determine_rate	= __clk_mux_determine_rate,
>   	.set_parent	= da8xx_cfgchip_mux_clk_set_parent,
>   	.get_parent	= da8xx_cfgchip_mux_clk_get_parent,
>   };
> @@ -251,7 +252,7 @@ da8xx_cfgchip_mux_clk_register(struct device *dev,
>   	init.ops = &da8xx_cfgchip_mux_clk_ops;
>   	init.parent_names = parent_names;
>   	init.num_parents = 2;
> -	init.flags = 0;
> +	init.flags = CLK_SET_RATE_NO_REPARENT;
>   
>   	mux->hw.init = &init;
>   	mux->regmap = regmap;
> 

Re: [PATCH v2 21/65] clk: davinci: da8xx-cfgchip: Add a determine_rate hook

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]

Hi David,

On Fri, Nov 04, 2022 at 11:45:17AM -0500, David Lechner wrote:
> On 11/4/22 8:17 AM, Maxime Ripard wrote:
> > The Davinci DA8xxx cfgchip mux clock 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 parent is defined in the device tree and is not expected to change
> at runtime, so if I am understanding the patch correctly, setting the
> CLK_SET_RATE_NO_REPARENT flag seems correct.

Is that an acked-by/reviewed-by?

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 21/65] clk: davinci: da8xx-cfgchip: Add a determine_rate hook

[David Lechner]

On 11/7/22 6:06 AM, Maxime Ripard wrote:
> Hi David,
> 
> On Fri, Nov 04, 2022 at 11:45:17AM -0500, David Lechner wrote:
>> On 11/4/22 8:17 AM, Maxime Ripard wrote:
>>> The Davinci DA8xxx cfgchip mux clock 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 parent is defined in the device tree and is not expected to change
>> at runtime, so if I am understanding the patch correctly, setting the
>> CLK_SET_RATE_NO_REPARENT flag seems correct.
> 
> Is that an acked-by/reviewed-by?
> 
> Thanks!
> Maxime

The commit message could be updated to be more certain now, but sure...

Acked-by: David Lechner <david@lechnology.com>

Re: [PATCH v2 22/65] clk: davinci: da8xx-cfgchip: Add a determine_rate hook

[David Lechner]

On 11/4/22 8:17 AM, Maxime Ripard wrote:
> The Davinci DA8xxx cfgchip "clk48" clock 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 latter case would be equivalent to setting the flag
> CLK_SET_RATE_NO_REPARENT, together with setting our determine_rate hook
> to __clk_mux_determine_rate(). Indeed, if no determine_rate
> implementation is provided, clk_round_rate() (through
> clk_core_round_rate_nolock()) will call itself on the parent if
> CLK_SET_RATE_PARENT is set, and will not change the clock rate
> otherwise. __clk_mux_determine_rate() has the exact same behavior when
> CLK_SET_RATE_NO_REPARENT is set.
> 
> And if it was an oversight, then we are at least explicit about our
> behavior now and it can be further refined down the line.

The parent is defined in the device tree and is not expected to change
at runtime, so if I am understanding the patch correctly, setting the
CLK_SET_RATE_NO_REPARENT flag seems correct.

> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/clk/davinci/da8xx-cfgchip.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/clk/davinci/da8xx-cfgchip.c b/drivers/clk/davinci/da8xx-cfgchip.c
> index c04276bc4051..4c1cc59bba53 100644
> --- a/drivers/clk/davinci/da8xx-cfgchip.c
> +++ b/drivers/clk/davinci/da8xx-cfgchip.c
> @@ -565,6 +565,7 @@ static u8 da8xx_usb1_clk48_get_parent(struct clk_hw *hw)
>   }
>   
>   static const struct clk_ops da8xx_usb1_clk48_ops = {
> +	.determine_rate	= __clk_mux_determine_rate,
>   	.set_parent	= da8xx_usb1_clk48_set_parent,
>   	.get_parent	= da8xx_usb1_clk48_get_parent,
>   };
> @@ -589,6 +590,7 @@ da8xx_cfgchip_register_usb1_clk48(struct device *dev,
>   
>   	init.name = "usb1_clk48";
>   	init.ops = &da8xx_usb1_clk48_ops;
> +	init.flags = CLK_SET_RATE_NO_REPARENT;
>   	init.parent_names = parent_names;
>   	init.num_parents = 2;
>   
> 

Re: [PATCH v2 54/65] clk: da8xx: clk48: Switch to determine_rate

[David Lechner]

On 11/4/22 8:18 AM, Maxime Ripard wrote:
> The TI DA8xx USB0 clk48 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.

I think this one should be the same as the clk:davinci changes and
not allow re-parenting. Since this is a USB 48MHz PHY clock, a rate
change will never be requested.

> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/clk/davinci/da8xx-cfgchip.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/davinci/da8xx-cfgchip.c b/drivers/clk/davinci/da8xx-cfgchip.c
> index 4c1cc59bba53..f60c97091818 100644
> --- a/drivers/clk/davinci/da8xx-cfgchip.c
> +++ b/drivers/clk/davinci/da8xx-cfgchip.c
> @@ -462,10 +462,12 @@ static unsigned long da8xx_usb0_clk48_recalc_rate(struct clk_hw *hw,
>   	return 48000000;
>   }
>   
> -static long da8xx_usb0_clk48_round_rate(struct clk_hw *hw, unsigned long rate,
> -					unsigned long *parent_rate)
> +static int da8xx_usb0_clk48_determine_rate(struct clk_hw *hw,
> +					   struct clk_rate_request *req)
>   {
> -	return 48000000;
> +	req->rate = 48000000;
> +
> +	return 0;
>   }
>   
>   static int da8xx_usb0_clk48_set_parent(struct clk_hw *hw, u8 index)
> @@ -494,7 +496,7 @@ static const struct clk_ops da8xx_usb0_clk48_ops = {
>   	.disable	= da8xx_usb0_clk48_disable,
>   	.is_enabled	= da8xx_usb0_clk48_is_enabled,
>   	.recalc_rate	= da8xx_usb0_clk48_recalc_rate,
> -	.round_rate	= da8xx_usb0_clk48_round_rate,
> +	.determine_rate	= da8xx_usb0_clk48_determine_rate,
>   	.set_parent	= da8xx_usb0_clk48_set_parent,
>   	.get_parent	= da8xx_usb0_clk48_get_parent,
>   };
> 

Re: [PATCH v2 54/65] clk: da8xx: clk48: Switch to determine_rate

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1972 bytes --]

Hi David,

On Fri, Nov 04, 2022 at 11:49:34AM -0500, David Lechner wrote:
> On 11/4/22 8:18 AM, Maxime Ripard wrote:
> > The TI DA8xx USB0 clk48 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.
> 
> I think this one should be the same as the clk:davinci changes and
> not allow re-parenting. Since this is a USB 48MHz PHY clock, a rate
> change will never be requested.

I'm not sure, it doesn't seem to be the same clock, it's not doing the
same thing (this one will always force the same rate, the others let the
rate change), and we're not doing the same refactoring (this one had a
round_rate implementation, the other one doesn't)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 28/65] clk: renesas: r9a06g032: Add a determine_rate hook

[Geert Uytterhoeven]

CC Gareth

On Fri, Nov 4, 2022 at 2:18 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> The Renesas r9a06g032 bitselect clock 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 latter case would be equivalent to setting the flag
> CLK_SET_RATE_NO_REPARENT, together with setting our determine_rate hook
> to __clk_mux_determine_rate(). Indeed, if no determine_rate
> implementation is provided, clk_round_rate() (through
> clk_core_round_rate_nolock()) will call itself on the parent if
> CLK_SET_RATE_PARENT is set, and will not change the clock rate
> otherwise. __clk_mux_determine_rate() has the exact same behavior when
> CLK_SET_RATE_NO_REPARENT is set.
>
> And if it was an oversight, then we are at least explicit about our
> behavior now and it can be further refined down the line.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/clk/renesas/r9a06g032-clocks.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/renesas/r9a06g032-clocks.c b/drivers/clk/renesas/r9a06g032-clocks.c
> index 983faa5707b9..70c37097ca6e 100644
> --- a/drivers/clk/renesas/r9a06g032-clocks.c
> +++ b/drivers/clk/renesas/r9a06g032-clocks.c
> @@ -773,6 +773,7 @@ static int r9a06g032_clk_mux_set_parent(struct clk_hw *hw, u8 index)
>  }
>
>  static const struct clk_ops clk_bitselect_ops = {
> +       .determine_rate = __clk_mux_determine_rate,
>         .get_parent = r9a06g032_clk_mux_get_parent,
>         .set_parent = r9a06g032_clk_mux_set_parent,
>  };
> @@ -797,7 +798,7 @@ r9a06g032_register_bitsel(struct r9a06g032_priv *clocks,
>
>         init.name = desc->name;
>         init.ops = &clk_bitselect_ops;
> -       init.flags = CLK_SET_RATE_PARENT;
> +       init.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT;
>         init.parent_names = names;
>         init.num_parents = 2;
>
>
> --
> b4 0.11.0-dev-99e3a

Re: [PATCH v2 65/65] clk: Warn if we register a mux without determine_rate

[Charles Keepax]

On Fri, Nov 04, 2022 at 02:18:22PM +0100, Maxime Ripard wrote:
> The determine_rate hook allows to select the proper parent and its rate
> for a given clock configuration. On another hand, set_parent is there to
> change the parent of a mux.
> 
> Some clocks provide a set_parent hook but don't implement
> determine_rate. In such a case, set_parent is pretty much useless since
> the clock framework will always assume the current parent is to be used,
> and we will thus never change it.
> 
> This situation can be solved in two ways:
>   - either we don't need to change the parent, and we thus shouldn't
>     implement set_parent;
>   - or we don't want to change the parent, in this case we should set
>     CLK_SET_RATE_NO_REPARENT;
>   - or we're missing a determine_rate implementation.
> 
> The latter is probably just an oversight from the driver's author, and
> we should thus raise their awareness about the fact that the current
> state of the driver is confusing.
> 
> It's not clear at this point how many drivers are affected though, so
> let's make it a warning instead of an error for now.
> 

Commit message could probably use updated to make the new state
of the chain.

Thanks,
Charles

Re: [PATCH v2 20/65] clk: wm831x: clkout: Add a determine_rate hook

[Charles Keepax]

On Fri, Nov 04, 2022 at 02:17:37PM +0100, Maxime Ripard wrote:
> The WM381x "clkout" clock 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 latter case would be equivalent to setting the flag
> CLK_SET_RATE_NO_REPARENT, together with setting our determine_rate hook
> to __clk_mux_determine_rate(). Indeed, if no determine_rate
> implementation is provided, clk_round_rate() (through
> clk_core_round_rate_nolock()) will call itself on the parent if
> CLK_SET_RATE_PARENT is set, and will not change the clock rate
> otherwise. __clk_mux_determine_rate() has the exact same behavior when
> CLK_SET_RATE_NO_REPARENT is set.
> 
> And if it was an oversight, then we are at least explicit about our
> behavior now and it can be further refined down the line.
> 

Yeah I don't think there would be anything wrong with this clock
changing parents on a rate change, but as you say this can be
refined down the line if someone needs the behaviour. It's an
older part so probably better to stick roughly to the current
behaviour for now.

Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles

Re: [PATCH v2 34/65] clk: ux500: prcmu: Add a determine_rate hook

[Linus Walleij]

On Fri, Nov 4, 2022 at 2:32 PM Maxime Ripard <maxime@cerno.tech> wrote:

> The UX500 PRCMU "clkout" clock 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().

It is actually set up from the device tree, typically like this:

/* clkout1 from ACLK divided by 8 */
clocks = <&clkout_clk DB8500_CLKOUT_1 DB8500_CLKOUT_SRC_ACLK 8>;

So the parent (source) and divisor comes in there.

clk->source and clk->divider is already set up when clk_hw_register() is
called.

So set/get_parent() is never used on clkout.

I think I just added the callbacks for completeness, should we delete them
altogether? The patch is probably fine as-is as well so
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 34/65] clk: ux500: prcmu: Add a determine_rate hook

[Maxime Ripard]

Hi Linus,

On Tue, Nov 08, 2022 at 02:25:04PM +0100, Linus Walleij wrote:
> On Fri, Nov 4, 2022 at 2:32 PM Maxime Ripard <maxime@cerno.tech> wrote:
> 
> > The UX500 PRCMU "clkout" clock 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().
> 
> It is actually set up from the device tree, typically like this:
> 
> /* clkout1 from ACLK divided by 8 */
> clocks = <&clkout_clk DB8500_CLKOUT_1 DB8500_CLKOUT_SRC_ACLK 8>;
> 
> So the parent (source) and divisor comes in there.
> 
> clk->source and clk->divider is already set up when clk_hw_register() is
> called.

I wasn't aware that we had such bindings. AFAIUI, it looks redundant
with assigned-clock-rates and assigned-clock-parents, could we deprecate
it?

> So set/get_parent() is never used on clkout.
> 
> I think I just added the callbacks for completeness, should we delete them
> altogether?

I can't really test any of these platforms, so I'm a bit wary of making
such changes myself. Feel free to send a follow-up if you think it's
needed :)

> The patch is probably fine as-is as well so
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks!
Maxime

Re: [PATCH v2 35/65] clk: ux500: sysctrl: Add a determine_rate hook

[Linus Walleij]

On Fri, Nov 4, 2022 at 2:32 PM Maxime Ripard <maxime@cerno.tech> wrote:

> The UX500 sysctrl "set_parent" clocks implement 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 latter case would be equivalent to setting the flag
> CLK_SET_RATE_NO_REPARENT, together with setting our determine_rate hook
> to __clk_mux_determine_rate(). Indeed, if no determine_rate
> implementation is provided, clk_round_rate() (through
> clk_core_round_rate_nolock()) will call itself on the parent if
> CLK_SET_RATE_PARENT is set, and will not change the clock rate
> otherwise. __clk_mux_determine_rate() has the exact same behavior when
> CLK_SET_RATE_NO_REPARENT is set.
>
> And if it was an oversight, then we are at least explicit about our
> behavior now and it can be further refined down the line.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 35/65] clk: ux500: sysctrl: Add a determine_rate hook

[Ulf Hansson]

On Fri, 4 Nov 2022 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
>
> The UX500 sysctrl "set_parent" clocks implement 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.

If I recall correctly, that is the use case we did target for these
types of clocks. See sound/soc/ux500/ux500_ab85xx.c, for example.

Maybe there are some additional pieces missing from the old down
stream kernel, I don't have full picture, sorry.

Anyway, if I am not wrong, this was about supporting a low-power audio
use case, which requires us to switch the parent clock (to avoid
wasting energy).

>
> 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().

Yes, this was the reason.

As a matter of fact, I don't even recall that re-parenting was
possible through clk_set_rate() when this clock driver was introduced.
But, I might be wrong, it's quite a while ago.

>
> The latter case would be equivalent to setting the flag
> CLK_SET_RATE_NO_REPARENT, together with setting our determine_rate hook
> to __clk_mux_determine_rate(). Indeed, if no determine_rate
> implementation is provided, clk_round_rate() (through
> clk_core_round_rate_nolock()) will call itself on the parent if
> CLK_SET_RATE_PARENT is set, and will not change the clock rate
> otherwise. __clk_mux_determine_rate() has the exact same behavior when
> CLK_SET_RATE_NO_REPARENT is set.
>
> And if it was an oversight, then we are at least explicit about our
> behavior now and it can be further refined down the line.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Seems reasonable to me!

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/clk/ux500/clk-sysctrl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/ux500/clk-sysctrl.c b/drivers/clk/ux500/clk-sysctrl.c
> index 702f2f8b43fa..d36336665b6d 100644
> --- a/drivers/clk/ux500/clk-sysctrl.c
> +++ b/drivers/clk/ux500/clk-sysctrl.c
> @@ -110,6 +110,7 @@ static const struct clk_ops clk_sysctrl_gate_fixed_rate_ops = {
>  };
>
>  static const struct clk_ops clk_sysctrl_set_parent_ops = {
> +       .determine_rate = __clk_mux_determine_rate,
>         .set_parent = clk_sysctrl_set_parent,
>         .get_parent = clk_sysctrl_get_parent,
>  };
> @@ -220,6 +221,7 @@ struct clk *clk_reg_sysctrl_set_parent(struct device *dev,
>                                 unsigned long flags)
>  {
>         return clk_reg_sysctrl(dev, name, parent_names, num_parents,
> -                       reg_sel, reg_mask, reg_bits, 0, 0, flags,
> +                       reg_sel, reg_mask, reg_bits, 0, 0,
> +                       flags | CLK_SET_RATE_NO_REPARENT,
>                         &clk_sysctrl_set_parent_ops);
>  }
>
> --
> b4 0.11.0-dev-99e3a

Re: [PATCH v2 35/65] clk: ux500: sysctrl: Add a determine_rate hook

[Linus Walleij]

On Thu, Nov 10, 2022 at 12:29 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Fri, 4 Nov 2022 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > The UX500 sysctrl "set_parent" clocks implement 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.
>
> If I recall correctly, that is the use case we did target for these
> types of clocks. See sound/soc/ux500/ux500_ab85xx.c, for example.

Hm I am trying to get that driver to work ... from time to time.
It's just that ALSA SoC DT has changed to much that it turns out
into a complete rewrite :/

So in sound/soc/ux500/mop500_ab8500.c
I see this:

        status = clk_set_parent(drvdata->clk_ptr_intclk, clk_ptr);
        if (status)
(...)

and there is elaborate code to switch between "SYSCLK" and
"ULPCLK" (ulta-low power clock). Just like you say... however
a clock named SYSCLK or ULPCLK does not appear in the
code in drivers/clk/ux500 or any DT bindings so... it seems to
be non-working for the time being.

Yours,
Linus Walleij

Re: [PATCH v2 35/65] clk: ux500: sysctrl: Add a determine_rate hook

[Ulf Hansson]

On Thu, 10 Nov 2022 at 12:39, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Nov 10, 2022 at 12:29 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On Fri, 4 Nov 2022 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > The UX500 sysctrl "set_parent" clocks implement 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.
> >
> > If I recall correctly, that is the use case we did target for these
> > types of clocks. See sound/soc/ux500/ux500_ab85xx.c, for example.
>
> Hm I am trying to get that driver to work ... from time to time.
> It's just that ALSA SoC DT has changed to much that it turns out
> into a complete rewrite :/
>
> So in sound/soc/ux500/mop500_ab8500.c
> I see this:
>
>         status = clk_set_parent(drvdata->clk_ptr_intclk, clk_ptr);
>         if (status)
> (...)
>
> and there is elaborate code to switch between "SYSCLK" and
> "ULPCLK" (ulta-low power clock). Just like you say... however
> a clock named SYSCLK or ULPCLK does not appear in the
> code in drivers/clk/ux500 or any DT bindings so... it seems to
> be non-working for the time being.

It's definitely not working, but the corresponding clocks ("ulpclk",
"intclk", "audioclk", etc) are being registered in ab8500_reg_clks().

What seems to be missing is a DT conversion for these clocks, so they
can be consumed properly. Right?

Kind regards
Uffe

Re: [PATCH v2 35/65] clk: ux500: sysctrl: Add a determine_rate hook

[Linus Walleij]

On Thu, Nov 10, 2022 at 2:05 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Thu, 10 Nov 2022 at 12:39, Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > On Thu, Nov 10, 2022 at 12:29 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > On Fri, 4 Nov 2022 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> > > >
> > > > The UX500 sysctrl "set_parent" clocks implement 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.
> > >
> > > If I recall correctly, that is the use case we did target for these
> > > types of clocks. See sound/soc/ux500/ux500_ab85xx.c, for example.
> >
> > Hm I am trying to get that driver to work ... from time to time.
> > It's just that ALSA SoC DT has changed to much that it turns out
> > into a complete rewrite :/
> >
> > So in sound/soc/ux500/mop500_ab8500.c
> > I see this:
> >
> >         status = clk_set_parent(drvdata->clk_ptr_intclk, clk_ptr);
> >         if (status)
> > (...)
> >
> > and there is elaborate code to switch between "SYSCLK" and
> > "ULPCLK" (ulta-low power clock). Just like you say... however
> > a clock named SYSCLK or ULPCLK does not appear in the
> > code in drivers/clk/ux500 or any DT bindings so... it seems to
> > be non-working for the time being.
>
> It's definitely not working, but the corresponding clocks ("ulpclk",
> "intclk", "audioclk", etc) are being registered in ab8500_reg_clks().
>
> What seems to be missing is a DT conversion for these clocks, so they
> can be consumed properly. Right?

Yeps that and a few more things, I have a scratch rewrite here:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-stericsson.git/log/?h=ux500-audio-rewrite

I remember Lee said he had audio working with the mainline kernel
on Snowball at one point, unfortunately I think that was before we
started with the DT conversions and then we probably broke it.

Yours,
Linus Walleij

Re: [PATCH v2 35/65] clk: ux500: sysctrl: Add a determine_rate hook

[Lee Jones]

On Fri, 11 Nov 2022, Linus Walleij wrote:

> On Thu, Nov 10, 2022 at 2:05 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On Thu, 10 Nov 2022 at 12:39, Linus Walleij <linus.walleij@linaro.org> wrote:
> > >
> > > On Thu, Nov 10, 2022 at 12:29 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > On Fri, 4 Nov 2022 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > >
> > > > > The UX500 sysctrl "set_parent" clocks implement 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.
> > > >
> > > > If I recall correctly, that is the use case we did target for these
> > > > types of clocks. See sound/soc/ux500/ux500_ab85xx.c, for example.
> > >
> > > Hm I am trying to get that driver to work ... from time to time.
> > > It's just that ALSA SoC DT has changed to much that it turns out
> > > into a complete rewrite :/
> > >
> > > So in sound/soc/ux500/mop500_ab8500.c
> > > I see this:
> > >
> > >         status = clk_set_parent(drvdata->clk_ptr_intclk, clk_ptr);
> > >         if (status)
> > > (...)
> > >
> > > and there is elaborate code to switch between "SYSCLK" and
> > > "ULPCLK" (ulta-low power clock). Just like you say... however
> > > a clock named SYSCLK or ULPCLK does not appear in the
> > > code in drivers/clk/ux500 or any DT bindings so... it seems to
> > > be non-working for the time being.
> >
> > It's definitely not working, but the corresponding clocks ("ulpclk",
> > "intclk", "audioclk", etc) are being registered in ab8500_reg_clks().
> >
> > What seems to be missing is a DT conversion for these clocks, so they
> > can be consumed properly. Right?
> 
> Yeps that and a few more things, I have a scratch rewrite here:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-stericsson.git/log/?h=ux500-audio-rewrite
> 
> I remember Lee said he had audio working with the mainline kernel
> on Snowball at one point, unfortunately I think that was before we
> started with the DT conversions and then we probably broke it.

That was also 100 years ago. :)

But yes, it used to work at one point.

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 58/65] clk: sprd: composite: Switch to determine_rate

[Chunyan Zhang]

On Fri, 4 Nov 2022 at 21:33, Maxime Ripard <maxime@cerno.tech> wrote:
>
> The Spreadtrum composite 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.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Acked-by: Chunyan Zhang <zhang.lyra@gmail.com>

Thanks,
Chunyan

> ---
>  drivers/clk/sprd/composite.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clk/sprd/composite.c b/drivers/clk/sprd/composite.c
> index ebb644820b1e..d3a852720c07 100644
> --- a/drivers/clk/sprd/composite.c
> +++ b/drivers/clk/sprd/composite.c
> @@ -9,13 +9,19 @@
>
>  #include "composite.h"
>
> -static long sprd_comp_round_rate(struct clk_hw *hw, unsigned long rate,
> -                               unsigned long *parent_rate)
> +static int sprd_comp_determine_rate(struct clk_hw *hw,
> +                                   struct clk_rate_request *req)
>  {
>         struct sprd_comp *cc = hw_to_sprd_comp(hw);
> +       unsigned long rate;
>
> -       return sprd_div_helper_round_rate(&cc->common, &cc->div,
> -                                        rate, parent_rate);
> +       rate = sprd_div_helper_round_rate(&cc->common, &cc->div,
> +                                         req->rate, &req->best_parent_rate);
> +       if (rate < 0)
> +               return rate;
> +
> +       req->rate = rate;
> +       return 0;
>  }
>
>  static unsigned long sprd_comp_recalc_rate(struct clk_hw *hw,
> @@ -53,7 +59,7 @@ const struct clk_ops sprd_comp_ops = {
>         .get_parent     = sprd_comp_get_parent,
>         .set_parent     = sprd_comp_set_parent,
>
> -       .round_rate     = sprd_comp_round_rate,
> +       .determine_rate = sprd_comp_determine_rate,
>         .recalc_rate    = sprd_comp_recalc_rate,
>         .set_rate       = sprd_comp_set_rate,
>  };
>
> --
> b4 0.11.0-dev-99e3a

Re: [PATCH v2 13/65] clk: lmk04832: clkout: Add a determine_rate hook

[Liam Beguin]

Hi Maxime,

On Fri, Nov 04, 2022 at 02:17:30PM +0100, Maxime Ripard wrote:
> The LKM04832 "CLKOUT" clock 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().

This is correct, the set_parent hook is effectively unused at the
moment. It was implemented as a way for consumers to select the parent
themselves.

The LMK04832 is used in JESD204 applications where devices need a device
clock as well as a sysref clock. Since this is determined by the
hardware layout, a devicetree option is used to select the inital state
of the clkout mux. This is set at the end of lmk04832_register_clkout().

> The latter case would be equivalent to setting the flag
> CLK_SET_RATE_NO_REPARENT, together with setting our determine_rate hook
> to __clk_mux_determine_rate(). Indeed, if no determine_rate
> implementation is provided, clk_round_rate() (through
> clk_core_round_rate_nolock()) will call itself on the parent if
> CLK_SET_RATE_PARENT is set, and will not change the clock rate
> otherwise. __clk_mux_determine_rate() has the exact same behavior when
> CLK_SET_RATE_NO_REPARENT is set.
> 
> And if it was an oversight, then we are at least explicit about our
> behavior now and it can be further refined down the line.
> 
> Since the CLK_SET_RATE_NO_REPARENT flag was already set though, it seems
> unlikely.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Liam Beguin <liambeguin@gmail.com>

Cheers,
Liam

> ---
>  drivers/clk/clk-lmk04832.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/clk/clk-lmk04832.c b/drivers/clk/clk-lmk04832.c
> index f416f8bc2898..f68bb0affdad 100644
> --- a/drivers/clk/clk-lmk04832.c
> +++ b/drivers/clk/clk-lmk04832.c
> @@ -1281,6 +1281,7 @@ static const struct clk_ops lmk04832_clkout_ops = {
>  	.is_enabled = lmk04832_clkout_is_enabled,
>  	.prepare = lmk04832_clkout_prepare,
>  	.unprepare = lmk04832_clkout_unprepare,
> +	.determine_rate = __clk_mux_determine_rate,
>  	.set_parent = lmk04832_clkout_set_parent,
>  	.get_parent = lmk04832_clkout_get_parent,
>  };
> 
> -- 
> b4 0.11.0-dev-99e3a
> 

Re: [PATCH v2 00/65] clk: Make determine_rate mandatory for muxes

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]

Hi Stephen,

On Tue, Mar 21, 2023 at 04:55:03PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-11-04 06:17:17)
> > Hi,
> > 
> > This is a follow-up to a previous series that was printing a warning
> > when a mux has a set_parent implementation but is missing
> > determine_rate().
> > 
> > The rationale is that set_parent() is very likely to be useful when
> > changing the rate, but it's determine_rate() that takes the parenting
> > decision. If we're missing it, then the current parent is always going
> > to be used, and thus set_parent() will not be used. The only exception
> > being a direct call to clk_set_parent(), but those are fairly rare
> > compared to clk_set_rate().
> > 
> > Stephen then asked to promote the warning to an error, and to fix up all
> > the muxes that are in that situation first. So here it is :)
> > 
> > Let me know what you think,
> 
> What's the plan here? Are you going to resend?

It wasn't clear to me whether or not this was something that you wanted,
and I got some pushback on the drivers so I kind of forgot about it.

If you do want it (and it looks like you do), I'll resend it.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]