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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Quanyang,

On Thu, Sep 29, 2022 at 09:05:10AM +0800, Quanyang Wang wrote:
> 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/

I should have searched the mailing list better before sending a patch,
sorry.

I've tested your patch, and it fixes the problem too. The resulting
pixel frequency is even more off though:

[   66.741024] zynqmp-dpsub fd4a0000.display: requested pixel rate: 297000000 actual rate: 249999998

But that's a separate issue.

> As for the frequency gap between the requested rate and the actual, it's 
> because of the commit:
> 
> commit 948fb0969eae8
> Author: Maxime Ripard <maxime@cerno.tech>
> Date:   Fri Feb 25 15:35:26 2022 +0100
> 
>      clk: Always clamp the rounded rate
> 
> And I haven't figured out how to fix it.
> 
> Thanks,
> 
> Quanyang
> 
> On 9/29/22 04:16, Laurent Pinchart wrote:
> > When calculating the divider in zynqmp_pll_round_rate() and
> > zynqmp_pll_set_rate(), the division rounding error may result in an
> > output frequency that is slightly outside of the PLL output range if the
> > requested range is close to the low or high limit. As a result, the
> > limits check in clk_calc_new_rates() would fail, and clk_set_rate()
> > would return an error, as seen in the zynqmp-dpsub driver:
> >
> > [   13.672309] zynqmp-dpsub fd4a0000.display: failed to set pixel clock rate to 297000000 (-22)
> >
> > Fix this by adjusting the divider. The rate calculation then succeeds
> > for zynqmp-dpsub:
> >
> > [   13.554849] zynqmp-dpsub fd4a0000.display: requested pixel rate: 297000000 actual rate: 255555553
> >
> > The resulting PLL configuration, however, is not optimal, as the rate
> > error is 14%. The hardware can do much better, but CCF doesn't attempt
> > to find the best overall configuration for cascaded clocks. That's a
> > candidate for a separate fix.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   drivers/clk/zynqmp/pll.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/clk/zynqmp/pll.c b/drivers/clk/zynqmp/pll.c
> > index 91a6b4cc910e..be6fa44a21e0 100644
> > --- a/drivers/clk/zynqmp/pll.c
> > +++ b/drivers/clk/zynqmp/pll.c
> > @@ -120,6 +120,10 @@ static long zynqmp_pll_round_rate(struct clk_hw *hw, unsigned long rate,
> >   	}
> >   
> >   	fbdiv = DIV_ROUND_CLOSEST(rate, *prate);
> > +	if (*prate * fbdiv < PS_PLL_VCO_MIN)
> > +		fbdiv++;
> > +	if (*prate * fbdiv > PS_PLL_VCO_MAX)
> > +		fbdiv--;
> >   	fbdiv = clamp_t(u32, fbdiv, PLL_FBDIV_MIN, PLL_FBDIV_MAX);
> >   	return *prate * fbdiv;
> >   }
> > @@ -208,6 +212,10 @@ static int zynqmp_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> >   	}
> >   
> >   	fbdiv = DIV_ROUND_CLOSEST(rate, parent_rate);
> > +	if (parent_rate * fbdiv < PS_PLL_VCO_MIN)
> > +		fbdiv++;
> > +	else if (parent_rate * fbdiv > PS_PLL_VCO_MAX)
> > +		fbdiv--;
> >   	fbdiv = clamp_t(u32, fbdiv, PLL_FBDIV_MIN, PLL_FBDIV_MAX);
> >   	ret = zynqmp_pm_clock_setdivider(clk_id, fbdiv);
> >   	if (ret)
> >
> > base-commit: 1c23f9e627a7b412978b4e852793c5e3c3efc555

-- 
Regards,

Laurent Pinchart