[v2,01/11] clk: qcom: ipq8074: drop the CLK_SET_RATE_PARENT flag from PLL clocks

GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based
on the request from dependent clocks. Doing so will result in the
unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL
clocks.

Cc: stable@vger.kernel.org
Fixes: b8e7e519625f ("clk: qcom: ipq8074: add remaining PLL’s")
Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
---
Changes in V2:
	- Include the stable mailing list
	- Keep the CLK_SET_RATE_PARENT in UBI32 PLL, looks like these
	  PLL rates can be changed. So don't drop the flag.
---
 drivers/clk/qcom/gcc-ipq8074.c | 6 ------
 1 file changed, 6 deletions(-)

On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
> GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based
> on the request from dependent clocks. Doing so will result in the
> unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL
> clocks.
> 
> Cc: stable@vger.kernel.org
> Fixes: b8e7e519625f ("clk: qcom: ipq8074: add remaining PLL’s")
> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
> ---
Stephen, do you think there should be some sort of error
or at least warning thrown when SET_RATE_PARENT is used with
RO ops?

Konrad

Quoting Konrad Dybcio (2023-09-15 05:19:56)
> On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
> > GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based
> > on the request from dependent clocks. Doing so will result in the
> > unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL
> > clocks.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: b8e7e519625f ("clk: qcom: ipq8074: add remaining PLL’s")
> > Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
> > ---
> Stephen, do you think there should be some sort of error
> or at least warning thrown when SET_RATE_PARENT is used with
> RO ops?
> 

Sure? How would that be implemented?

On 10/19/23 02:16, Stephen Boyd wrote:
> Quoting Konrad Dybcio (2023-09-15 05:19:56)
>> On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
>>> GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based
>>> on the request from dependent clocks. Doing so will result in the
>>> unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL
>>> clocks.
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: b8e7e519625f ("clk: qcom: ipq8074: add remaining PLL’s")
>>> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
>>> ---
>> Stephen, do you think there should be some sort of error
>> or at least warning thrown when SET_RATE_PARENT is used with
>> RO ops?
>>
> 
> Sure? How would that be implemented?
drivers/clk/clk.c : static void clk_change_rate()

if (!skip_set_rate && core->ops->set_rate)
	core->ops->set_rate(core->hw, core->new_rate, best_parent_rate);

->

if (!skip_set_rate) {
	if (core->ops->set_rate)
		core->ops->set_rate(core->hw, core->new_rate,
				    best_parent_rate);
	else
		pr_err("bad idea");
}

Konrad

Quoting Konrad Dybcio (2023-10-19 04:22:33)
> 
> 
> On 10/19/23 02:16, Stephen Boyd wrote:
> > Quoting Konrad Dybcio (2023-09-15 05:19:56)
> >> On 14.09.2023 08:59, Kathiravan Thirumoorthy wrote:
> >>> GPLL, NSS crypto PLL clock rates are fixed and shouldn't be scaled based
> >>> on the request from dependent clocks. Doing so will result in the
> >>> unexpected behaviour. So drop the CLK_SET_RATE_PARENT flag from the PLL
> >>> clocks.
> >>>
> >>> Cc: stable@vger.kernel.org
> >>> Fixes: b8e7e519625f ("clk: qcom: ipq8074: add remaining PLL’s")
> >>> Signed-off-by: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
> >>> ---
> >> Stephen, do you think there should be some sort of error
> >> or at least warning thrown when SET_RATE_PARENT is used with
> >> RO ops?
> >>
> > 
> > Sure? How would that be implemented?
> drivers/clk/clk.c : static void clk_change_rate()
> 
> if (!skip_set_rate && core->ops->set_rate)
>         core->ops->set_rate(core->hw, core->new_rate, best_parent_rate);
> 
> ->
> 
> if (!skip_set_rate) {
>         if (core->ops->set_rate)
>                 core->ops->set_rate(core->hw, core->new_rate,
>                                     best_parent_rate);
>         else
>                 pr_err("bad idea");
> }
> 

CLK_SET_RATE_PARENT means that "calling clk_set_rate() on this clk will
propagate up to the parent". Changing the rate of the parent could
change the rate of this clk to be the same frequency as the parent if
this clk doesn't have a set_rate clk op, or it could be that this clk
has a fixed divider so during the determine_rate() callback it
calculated what rate the parent should be to achieve the requested rate
in clk_set_rate().

It really matters what determine_rate() returns for a clk and if after
changing rates that rate is actually achieved. I suppose if the
determine_rate() callback returns some rate, and then we recalc rates
and notice that the rate determined earlier doesn't match we're
concerned. So far in the last decade we've never cared about this though
and I'm hesitant to start adding that check. I believe some qcom clk
drivers take a shortcut and round the rate in frequency tables so
whatever is returned in determine_rate() doesn't match what
recalc_rate() calculates.

It would be interesting to get rid of the CLK_SET_RATE_PARENT check in
clk_calc_new_rates() and simply always call clk_calc_new_rates() on the
parent if the parent->rate doesn't match what determine_rate thought it
should be. The framework currently calls the rounding clk op for a clk
and gets back the parent rate that the clk requires to achieve that rate
and then it blindly trusts that the parent rate is going to be achieved.
If the CLK_SET_RATE_PARENT flag is set it calls clk_calc_new_rates()
recursively on the parent, but then it doesn't check that the parent
rate is what was requested. That's mostly there to figure out if the
parent also needs to change rate, i.e. calculating the 'top' clk in a
rate change. Note that this also calls determine_rate again on the
parent, once from the child clk's determine_rate clk op and once from
the framework.

I wouldn't be surprised if some driver is relying on this behavior where
the rate isn't checked after being set. Maybe when we extend struct
clk_rate_request to have a linked list that allows a clk to build up a
chain of rate requests we can also enforce more things like matching
rates on recalc. Then any drivers that are relying on this behavior will
have to opt in to a different method of changing rates and notice that
things aren't working.