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

[Quanyang Wang]

Hi Maxime,

On 10/10/22 16:49, Maxime Ripard wrote:
> Hi,
> 
> On Sun, Oct 02, 2022 at 10:17:24AM +0800, Quanyang Wang wrote:
>> 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,
> 
> I guess it would be s/should/must/, but yeah, we agree.
> 
>> but can take a rate outside of boundaries as input.
> 
> I'm not sure what that would change though?
> 
>> 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.
> 
> Sounds good to me indeed.
> 
>> 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,
> 
> I'm a bit confused now.
> 
> If I understand your clock topology, you have a PLL, and then a divider
> of 8, and want the final clock to be running at 200MHz?
> 
> If so, the divider should call its parent round/determine_rate with 200
> * 8 MHz = 1600MHz, which is is still inside the boundaries of 1.5-3.0GHz
> and won't be affected?
> 
> Why should the child be affected by the parent boundaries, or the other
> way aroundSorry, I didn't explain the problem clearly.

As below is the vpll clk topology in /sys/kernel/debug/clk/clk_summary 
when reverted "clk: Always clamp the rounded rate".
  clk_name				MHz
  pss_ref_clk                          33333333
     vpll_post_src                     33333333
     vpll_pre_src                      33333333
        vpll_int                       1599999984
           vpll_half                   799999992
              vpll_int_mux             799999992
                 vpll                  799999992
                    dp_video_ref_mux    799999992
                       dp_video_ref_div1    99999999
                          dp_video_ref_div2  99999999
                             dp_video_ref    99999999

When call clk_set_rate(dp_video_ref, 100MHz), there is a 
clk_calc_new_rates request passed from bottom (dp_video_ref) to top 
(vpll_int), every clk will calculate its clk_rate and its 
best_parent_rate. vpll_half will calculate its clk rate is 100MHz and 
its parent clk vpll_int should be 200MHz since vpll_half is a half 
divider. But vpll_int ranges from 1.5GHz~3GHz, so it call 
zynqmp_pll_round_rate to calculate a new rate 1.6GHz for itself via 
200MHz * 8 because 8 is the smallest integer:

zynqmp_pll_round_rate:
        if (200MHz < 1.5GHz) {
                mult = DIV_ROUND_UP(1.5GHz, 200MHz);
                rate = rate * mult;

The question is that implementation of round_rate callbacks for some clk 
drivers have abilities to round a out-of-range rate req to a 
inside-range rate:

clk_core_determine_round_nolock:
         rate(1.6GHz) = core->ops->round_rate("vpll_int", req->rate(200MHz),
                           &req->best_parent_rate);

But the commit 948fb096 ("clk: Always clamp the rounded rate") breaks 
this like:
clk_core_determine_round_nolock:
	req->rate (1.5GHz) = clamp(req->rate (200MHz), req->min_rate (1.5GHz), 
req->max_rate (3GHz));
	......
	rate(1.5GHz) = core->ops->round_rate("vpll_int", req->rate(1.5GHz),
                           &req->best_parent_rate);


Thanks,
Quanyang
> Maxime