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

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1.1: Type: text/plain, Size: 4647 bytes --]

On Mon, Oct 10, 2022 at 08:12:08PM +0800, Quanyang Wang wrote:
> 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 around
>
> Sorry, 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

I couldn't find any of these clocks by grepping in the kernel code, are
they upstream?

> 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

Still, I'm not entirely sure what's going on. If the only divider we
have is vpll_half which halves the rate, and we want 100MHz on
dp_video_ref, then vpll_int should provide 200MHz? Why would we increase
it to 1.6GHz? I get that the range of operating frequencies for vpll_int
is 1.5-3GHz, but I don't understand how we could end up with 100MHz on
dp_video_ref with 1.6GHz for that PLL. Or the other way around, why we
want that * 8 in the first place for vpll_int.

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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