Hi Geert,
On Wed, May 28, 2025 at 8:09 AM Geert Uytterhoeven <ge...@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, 27 May 2025 at 23:51, Lad, Prabhakar <prabhakar.cse...@gmail.com>
> wrote:
> > On Fri, May 23, 2025 at 3:45 PM Geert Uytterhoeven <ge...@linux-m68k.org>
> > wrote:
> > > On Mon, 12 May 2025 at 20:43, Prabhakar <prabhakar.cse...@gmail.com>
> > > wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad...@bp.renesas.com>
> > > >
> > > > Add support for PLLDSI and PLLDSI divider clocks.
> > > >
> > > > Introduce the `renesas-rzv2h-dsi.h` header to centralize and share
> > > > PLLDSI-related data structures, limits, and algorithms between the
> > > > RZ/V2H
> > > > CPG and DSI drivers.
> > > >
> > > > The DSI PLL is functionally similar to the CPG's PLLDSI, but has
> > > > slightly
> > > > different parameter limits and omits the programmable divider present in
> > > > CPG. To ensure precise frequency calculations-especially for
> > > > milliHz-level
> > > > accuracy needed by the DSI driver-the shared algorithm allows both
> > > > drivers
> > > > to compute PLL parameters consistently using the same logic and input
> > > > clock.
> > > >
> > > > Co-developed-by: Fabrizio Castro <fabrizio.castro...@renesas.com>
> > > > Signed-off-by: Fabrizio Castro <fabrizio.castro...@renesas.com>
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad...@bp.renesas.com>
>
> > > > +static int rzv2h_cpg_plldsi_div_determine_rate(struct clk_hw *hw,
> > > > + struct clk_rate_request
> > > > *req)
> > > > +{
> > > > + struct rzv2h_plldsi_div_clk *dsi_div = to_plldsi_div_clk(hw);
> > > > + struct rzv2h_cpg_priv *priv = dsi_div->priv;
> > > > + struct rzv2h_plldsi_parameters *dsi_dividers =
> > > > &priv->plldsi_div_parameters;
> > > > + u64 rate_millihz;
> > > > +
> > > > + /*
> > > > + * Adjust the requested clock rate (`req->rate`) to ensure it
> > > > falls within
> > > > + * the supported range of 5.44 MHz to 187.5 MHz.
> > > > + */
> > > > + req->rate = clamp(req->rate, 5440000UL, 187500000UL);
> > > > +
> > > > + rate_millihz = mul_u32_u32(req->rate, MILLI);
> > > > + if (rate_millihz == dsi_dividers->error_millihz +
> > > > dsi_dividers->freq_millihz)
> > > > + goto exit_determine_rate;
> > > > +
> > > > + if (!rzv2h_dsi_get_pll_parameters_values(priv->dsi_limits,
> > > > + dsi_dividers,
> > > > rate_millihz)) {
> > > > + dev_err(priv->dev,
> > > > + "failed to determine rate for req->rate: %lu\n",
> > > > + req->rate);
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > +exit_determine_rate:
> > > > + req->best_parent_rate = req->rate * dsi_dividers->csdiv;
> > >
> > > Shouldn't this also update req->rate with the actual rate?
> > >
> > > req->rate = DIV_ROUND_CLOSEST_ULL(dsi_dividers->freq_millihz, MILLI);
> > >
> > Agreed, I will update it.
>
> I think not updating req->rate may cause clk_get_rate() to return
> an incorrect value (can error_millihz > 1000?). Any chance this fix
> can simplify the clock handling in the DSI driver?
>
Yes, error_millihz can be greater than 1000, as result the DSI driver
does check this (>= 500) and proceeds to try the next one.
Cheers,
Prabhaar
