From: Quanyang Wang <quanyang.wang@windriver.com>
To: Maxime Ripard <maxime@cerno.tech>
Cc: Stephen Boyd <sboyd@kernel.org>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Michal Simek <michal.simek@xilinx.com>,
Rajan Vaja <rajan.vaja@xilinx.com>
Subject: Re: [PATCH] clk: zynqmp: pll: Fix divider calculation to avoid out-of-range rate
Date: Tue, 11 Oct 2022 11:11:31 +0800 [thread overview]
Message-ID: <3b05a90d-a3f0-1df7-e2f9-d3f44cdf36a8@windriver.com> (raw)
In-Reply-To: <20221010124915.25pl5jyxtaepimhd@houat>
Hi Maxime,
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. 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. Then vpll_int receive a 200MHz rate request
from vpll_half which will set itself to be 100MHz. 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.
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?
Thanks,
Quanyang
>
> Maxime
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-10-11 3:13 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-28 20:16 [PATCH] clk: zynqmp: pll: Fix divider calculation to avoid out-of-range rate Laurent Pinchart
2022-09-29 1:05 ` Quanyang Wang
2022-10-01 0:05 ` Stephen Boyd
2022-10-01 10:40 ` Maxime Ripard
2022-10-02 2:17 ` Quanyang Wang
2022-10-03 0:06 ` Laurent Pinchart
2022-10-10 8:49 ` Maxime Ripard
2022-10-10 12:12 ` Quanyang Wang
2022-10-10 12:49 ` Maxime Ripard
2022-10-11 3:11 ` Quanyang Wang [this message]
2022-10-11 12:27 ` Maxime Ripard
2022-10-02 23:45 ` Laurent Pinchart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3b05a90d-a3f0-1df7-e2f9-d3f44cdf36a8@windriver.com \
--to=quanyang.wang@windriver.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=maxime@cerno.tech \
--cc=michal.simek@xilinx.com \
--cc=rajan.vaja@xilinx.com \
--cc=sboyd@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).