[PATCH] clk: zynqmp: pll: Fix divider calculation to avoid out-of-range rate
Maxime Ripard
maxime at cerno.tech
Tue Oct 11 05:27:59 PDT 2022
Hi,
On Tue, Oct 11, 2022 at 11:11:31AM +0800, Quanyang Wang wrote:
> On 10/10/22 20:49, Maxime Ripard wrote:
> > 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.
>
> Oh, I think I see what's wrong. It's because the children clocks of vpll_int
> have the wrong rate range. The commit 948fb0969eae8 makes sense and my
> understanding was wrong.
>
> The clk vpll_int sets the rate range by the function:
> clk_hw_set_rate_range(hw, 1.5GHz, 3.0GHz);
> But the function clk_hw_set_rate_range just set the rate range for vpll_int,
> not cascade the limitation for its children clks. This results that the its
> children clks still ranges 0~ULONG_MAX.
Yeah, indeed the range isn't propagated to the children yet. This wasn't
the case before my series either, so I wouldn't consider it a
regression.
> When a 100MHz rate request is raised from the bottom "dp_video_ref",
> every clk in the tree doesn't modify it and just pass it to its parent
> clk since 100MHz is in its rate range 0~ULONG_MAX.
From a current-state point of view, that makes sense.
> Then vpll_int receive a 200MHz rate request from vpll_half which will
> set itself to be 100MHz.
Still makes sense :)
> But in fact, vpll_half rate range should be 1.5GHz/2 ~ 3GHz/2. The
> same to the clk dp_video_ref_mux. And the divider dp_video_ref_div1
> should range from 1.5GHz/2/63 ~ 3GHz/2/1.
I still don't get where exactly the 8 multiplier is enforced? Is it in
vpll_int? If so, then why is it reporting a rate of 1.5-3.0 GHz, whereas
its child clock that is supposed to halve the rate has 100MHz?
> In my limited view, there is a relation between clocks that is
> muxes/dividers should inherit the limitation from its parent clk. But now it
> seems that the rate ranges are isolating between clks. Is there a way that
> can re-set the rate range for children clks when its parent clk re-set the
> rate range?
We can't really do that in the framework, because we can't really know
the capabilities of the clocks at a generic level using any of the
clocks operations. Indeed some clocks won't affect the rate ("pure"
gate, muxes), and some will.
Figuring out the rate range for those that don't affect it is trivial,
but for those who do you can't know how it affects the rate. Plus, PLL
will typically have a narrower range of operation than the extremum you
can achieve by using the larger divider and smaller multiplier for
example.
So I think you can't really expect an obvious relationship between the
parent range and the child range. So it should be reported by the
driver, like you did, by calling clk_hw_set_rate_range().
Maxime
More information about the linux-arm-kernel
mailing list