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?

Maxime