Re: [PATCH] clk: zynqmp: pll: Fix divider calculation to avoid out-of-range rate

[Quanyang Wang <quanyang.wang@windriver.com>]

Hi Maxime,

On 10/1/22 18:40, Maxime Ripard wrote:
> Hi
> 
> On Fri, Sep 30, 2022 at 05:05:01PM -0700, Stephen Boyd wrote:
>> +Maxime
>>
>> Quoting Quanyang Wang (2022-09-28 18:05:10)
>>> Hi Laurent,
>>>
>>> I have sent a patch as below to fix this issue which set rate failed and
>>> it's in linux-next repo now.
>>>
>>> https://lore.kernel.org/linux-arm-kernel/20220826142030.213805-1-quanyang.wang@windriver.com/T/
>>>
> 
> It looks to me that the fundamental issue is that, in some situations,
> the round_rate implementation can return a rate outside of the
> boundaries enforced on a clock.
In my limited view, the round_rate callbacks should return a rate within 
boundaries as output, but can take a rate outside of boundaries as input.

Take Xilinx Zynqmp for instance, VPLL's rate range is 1.5GHz~3GHz. A 
consumer dp_video_ref wants a 200MHz rate, its request walks upward 
through multiplexers and dividers then reaches to VPLL, VPLL receives 
this 200MHz request and call  zynqmp_pll_round_rate to "round" this 
out-of-range rate 200MHz to 1600MHz via multiplying by 8. 
zynqmp_pll_round_rate returns 1600MHz and clk subsystem will call 
determine callbacks to configure dividers correctly to make sure that 
dp_video_ref can get an exact rate 200MHz.

But the commit 948fb0969eae8 ("clk: Always clamp the rounded rate") adds

req->rate = clamp(req->rate, req->min_rate, req->max_rate);

before

rate = core->ops->round_rate(core->hw, req->rate,&req->best_parent_rate);

This results that .round_rate callbacks lose functionality since they 
have no chance to pick up a precise rate but only a boundary rate.
Still for Xilinx Zynqmp, the 200MHz rate request to PLL will be set to 
1500MHz by clamp function and then zynqmp_pll_round_rate does nothing, 
dp_video_ref will finally receive a rate which is 1500MHz/8 = 187.5MHz.
The rate gap (200MHz-187.5MHz) happens.

There is no doubt that round_rate should return a valid rate as output. 
But is it necessary that forces the input of round_rate callbacks to be 
within boundaries?

Thanks,
Quanyang
> 
> I think that's the current behaviour (that was there prior to my
> patches) to reject any rate outside of the boundaries in
> clk_calc_new_rates() makes it clear that it's not something we should
> allow.
> 
> I'm a bit two-minded on this though. All the failures of that test I've
> seen actually turned out to be bugs, so I guess it's useful, but it's
> also true that for rounding errors it's a bit overkill. We could also
> relax that check and warn instead of failing.
> 
>>> As for the frequency gap between the requested rate and the actual, it's
>>> because of the commit:
>>>
>>> commit 948fb0969eae8
>>> Author: Maxime Ripard <maxime@cerno.tech>
>>> Date:   Fri Feb 25 15:35:26 2022 +0100
>>>
>>>       clk: Always clamp the rounded rate
>>>
>>> And I haven't figured out how to fix it.
> 
> Again, it boils down on whether or not we should allow a rate outside of
> boundaries. If we don't and if the clock can't do better, then yeah, the
> rate difference is fairly big but we can't do better.
> 
>> Maxime has some more patches to fix this and they're in linux-next.
>> Maybe those fix this problem?
> 
> I don't think they will fix it. However, depending on the outcome of
> that discussion I can send more fixes your way :)
> 
> Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel