[PATCH v2 01/15] clk: renesas: rzv2h-cpg: Add support for DSI clocks
Fabrizio Castro
fabrizio.castro.jz at renesas.com
Thu Apr 17 15:12:06 UTC 2025
Hi Geert,
Thanks for your feedback!
> From: Geert Uytterhoeven <geert at linux-m68k.org>
> Sent: 16 April 2025 10:27
> Subject: Re: [PATCH v2 01/15] clk: renesas: rzv2h-cpg: Add support for DSI clocks
>
> Hi Prabhakar, Fabrizio,
>
> Thanks for your patch!
>
> On Tue, 8 Apr 2025 at 22:09, Prabhakar <prabhakar.csengg at gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj at bp.renesas.com>
> >
> > Add support for PLLDSI and PLLDSI divider clocks.
> >
> > The `renesas-rzv2h-dsi.h` header file is added to share the PLL divider
> > algorithm between the CPG and DSI drivers.
>
> Please explain here why the DSI driver needs access to this algorithm.
Perhaps something like the below could fully explain the reason, although
it's an eyeful:
"The PLL found inside the DSI IP is very similar to the PLLDSI found in the
CPG IP block, although the limits for some of the parameters are different.
Also, the PLLDSI is followed by a programmable divider, whereas there is no
such thing in the DSI PLL IP.
The limits for the PLL found within the DSI IP are:
1 <= PLL_P <= 4
64 <= PLL_M <= 1023
0 <= PLL_S <= 5
−32768 <= PLL_K <= 32767
The limits for PLLDSI (found in CPG) are:
1 <= PLL_P <= 4
64 <= PLL_M <= 533
0 <= PLL_S <= 6
−32768 <= PLL_K <= 32767
The limits for the PLLDSI related divider are:
CSDIV = 1/(2 + 2 * n), with n=0,...,15
Header file `renesas-rzv2h-dsi.h` is added so that both the CPG and DSI drivers
can reuse exactly the same data structures and algorithm, although
they'll drive rzv2h_dsi_get_pll_parameters_values() with different limits.
While the CPG driver only needs visibility of the limits for the PLLDSI,
the DSI driver is going to need visibility of the limits for both PLLDSI and
for the PLL inside the DSI IP.
The DSI driver requires a resolution higher than Hz (which is what the clock
subsystem currently supports), namely: mHz. This is vital to allow the DSI driver
to keep the error between the calculated value of HSFREQ and the generated value
of HSFREQ below a certain threshold.
The difficulty in achieving a small error is down to the accuracy of the VCLK
representation.
Since the clock subsystem only allows for Hz, a 1/2 Hz error on the representation of
VCLK (down to the selection of frequencies that cannot be precisely achieved and
related rounding errors) may lead to a very big error in the calculation
of HSFREQ, which uses the below formula:
HSFREQ = (VCLK * bpp) / num_lanes
In the worst case scenario (1 lane and 24 bpp), a 1/2 Hz error on the representation
of VCLK will lead to an error of 12Hz(!) on the calculation of HSFREQ, leading
to a non working video output.
By granting the DSI driver access to the PLL calculations and PLLDSI (CPG) limits,
the DSI driver can work out the best solution for VCLK independently from the CPG
driver, and it can set VCLK accordingly (knowing that the CPG driver will
use exactly the same parameters determined by the DSI driver, as it will be using
the same input frequency and the same algorithm for the calculations).
For convenience, macro RZV2H_CPG_PLL_DSI_LIMITS() is added to avoid replicating
the declaration of the PLLDSI limits and therefore it can be used in both the
CPG driver and in the DSI driver.
Make use of the data structures and algorithm defined in header file
`renesas-rzv2h-dsi.h` to add PLLDSI support, including its divider.
Since we need to make sure that the DSI driver knows exactly which frequency
the PLLDSI + divider combo is going to generate, the CPG driver is instructed
to calculate the parameters for the PLLDSI + divider combo (by calling into
rzv2h_dsi_get_pll_parameters_values()) only from
rzv2h_cpg_plldsi_div_determine_rate(). rzv2h_cpg_plldsi_set_rate() will simply
reuse the pre-calculated parameter values."
What do you think? Too much?
Cheers,
Fab
>
> > Co-developed-by: Fabrizio Castro <fabrizio.castro.jz at renesas.com>
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz at renesas.com>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj at bp.renesas.com>
>
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
>
> > @@ -196,6 +225,253 @@ static int rzv2h_cpg_pll_clk_enable(struct clk_hw *hw)
> > return ret;
> > }
> >
> > +static unsigned long rzv2h_cpg_plldsi_div_recalc_rate(struct clk_hw *hw,
> > + unsigned long parent_rate)
> > +{
> > + struct rzv2h_plldsi_div_clk *dsi_div = to_plldsi_div_clk(hw);
> > + struct rzv2h_cpg_priv *priv = dsi_div->priv;
> > + struct ddiv ddiv = dsi_div->ddiv;
> > + u32 div;
> > +
> > + div = readl(priv->base + ddiv.offset);
> > + div >>= ddiv.shift;
> > + div &= ((2 << ddiv.width) - 1);
>
> Shouldn't that "2" be "1"?
> GENMASK(ddiv.width - 1, 0), or even better: clk_div_mask(ddiv.width).
>
> > +
> > + div = dsi_div->dtable[div].div;
> > +
> > + return DIV_ROUND_CLOSEST_ULL(parent_rate, div);
> > +}
> > +
> > +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;
> > + unsigned long long rate_mhz;
>
> u64?
> Please use "millihz" instead of "mhz" everywhere, so it becomes very
> clear this is really "mHz" and not "MHz".
>
> > +
> > + /*
> > + * 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_mhz = req->rate * MILLI * 1ULL;
> > + if (rate_mhz == dsi_dividers->error_mhz + dsi_dividers->freq_mhz)
> > + goto exit_determine_rate;
> > +
> > + if (!rzv2h_dsi_get_pll_parameters_values(priv->dsi_limits,
> > + dsi_dividers, rate_mhz)) {
> > + 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;
> > +
> > + return 0;
> > +};
> > +
> > +static int rzv2h_cpg_plldsi_div_set_rate(struct clk_hw *hw,
> > + unsigned long rate,
> > + unsigned long parent_rate)
> > +{
> > + 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;
> > + struct ddiv ddiv = dsi_div->ddiv;
> > + const struct clk_div_table *clkt;
> > + u32 reg, shift, div;
> > +
> > + div = dsi_dividers->csdiv;
> > + for (clkt = dsi_div->dtable; clkt->div; clkt++) {
> > + if (clkt->div == div)
> > + break;
> > + }
> > +
> > + if (!clkt->div && !clkt->val)
> > + return -EINVAL;
>
> No need to check clkt->dev.
>
> > +
> > + shift = ddiv.shift;
> > + reg = readl(priv->base + ddiv.offset);
> > + reg &= ~(GENMASK(shift + ddiv.width, shift));
> > +
> > + writel(reg | (clkt->val << shift) |
> > + DDIV_DIVCTL_WEN(shift), priv->base + ddiv.offset);
> > +
> > + return 0;
>
> This function is very similar to the existing rzv2h_ddiv_set_rate().
> If you can't re-use it as-is, please consider factoring out the common
> part, or at least follow the same style of RMW-operation.
>
> > +};
>
>
> > +static long rzv2h_cpg_plldsi_round_rate(struct clk_hw *hw,
> > + unsigned long rate,
> > + unsigned long *parent_rate)
> > +{
> > + return clamp(rate, 25000000UL, 375000000UL);
>
> This only rounds rates outside the range from 25 to 375 MHz.
> What about rates between 25 and 375 MHz?
>
> > +}
> > +
> > +static unsigned long rzv2h_cpg_plldsi_clk_recalc_rate(struct clk_hw *hw,
> > + unsigned long parent_rate)
> > +{
> > + struct pll_clk *pll_clk = to_pll(hw);
> > + struct rzv2h_cpg_priv *priv = pll_clk->priv;
> > + unsigned int val1, val2;
> > + u64 rate;
> > +
> > + val1 = readl(priv->base + CPG_PLL_CLK1(pll_clk->pll.offset));
> > + val2 = readl(priv->base + CPG_PLL_CLK2(pll_clk->pll.offset));
> > +
> > + rate = mul_u64_u32_shr(parent_rate, (CPG_PLL_CLK1_MDIV(val1) << 16) +
> > + CPG_PLL_CLK1_KDIV(val1), 16 + CPG_PLL_CLK2_SDIV(val2));
> > +
> > + return DIV_ROUND_CLOSEST_ULL(rate, CPG_PLL_CLK1_PDIV(val1));
>
> Can't you just reuse the existing rzv2h_cpg_pll_clk_recalc_rate()?
>
> > +}
> > +
> > +static int rzv2h_cpg_plldsi_set_rate(struct clk_hw *hw,
> > + unsigned long rate,
> > + unsigned long parent_rate)
> > +{
> > + struct pll_clk *pll_clk = to_pll(hw);
> > + struct rzv2h_cpg_priv *priv = pll_clk->priv;
> > + struct rzv2h_plldsi_parameters *dsi_dividers;
> > + struct pll pll = pll_clk->pll;
> > + u16 offset = pll.offset;
> > + u32 val;
> > + int ret;
> > +
> > + /* Put PLL into standby mode */
> > + writel(CPG_PLL_STBY_RESETB_WEN, priv->base + CPG_PLL_STBY(offset));
> > + ret = readl_poll_timeout_atomic(priv->base + CPG_PLL_MON(offset),
> > + val, !(val & CPG_PLL_MON_LOCK),
> > + 100, 2000);
> > + if (ret) {
> > + dev_err(priv->dev, "Failed to put PLLDSI into standby mode");
> > + return ret;
> > + }
> > +
> > + dsi_dividers = &priv->plldsi_div_parameters;
> > + /* Output clock setting 1 */
> > + writel((dsi_dividers->k << 16) | (dsi_dividers->m << 6) | (dsi_dividers->p),
> > + priv->base + CPG_PLL_CLK1(offset));
> > +
> > + /* Output clock setting 2 */
> > + val = readl(priv->base + CPG_PLL_CLK2(offset));
> > + writel((val & ~GENMASK(2, 0)) | dsi_dividers->s,
> > + priv->base + CPG_PLL_CLK2(offset));
> > +
> > + /* Put PLL to normal mode */
> > + writel(CPG_PLL_STBY_RESETB_WEN | CPG_PLL_STBY_RESETB,
> > + priv->base + CPG_PLL_STBY(offset));
> > +
> > + /* PLL normal mode transition, output clock stability check */
> > + ret = readl_poll_timeout_atomic(priv->base + CPG_PLL_MON(offset),
> > + val, (val & CPG_PLL_MON_LOCK),
> > + 100, 2000);
> > + if (ret) {
> > + dev_err(priv->dev, "Failed to put PLLDSI into normal mode");
> > + return ret;
> > + }
> > +
> > + return 0;
>
> This function could be reused for non-DSI PLLs?
>
> > +};
> > +
> > +static const struct clk_ops rzv2h_cpg_plldsi_ops = {
> > + .recalc_rate = rzv2h_cpg_plldsi_clk_recalc_rate,
> > + .round_rate = rzv2h_cpg_plldsi_round_rate,
> > + .set_rate = rzv2h_cpg_plldsi_set_rate,
> > +};
> > +
> > +static struct clk * __init
> > +rzv2h_cpg_plldsi_clk_register(const struct cpg_core_clk *core,
> > + struct rzv2h_cpg_priv *priv)
> > +{
> > + void __iomem *base = priv->base;
> > + struct device *dev = priv->dev;
> > + struct clk_init_data init;
> > + const struct clk *parent;
> > + const char *parent_name;
> > + struct pll_clk *pll_clk;
> > + int ret;
> > +
> > + parent = priv->clks[core->parent];
> > + if (IS_ERR(parent))
> > + return ERR_CAST(parent);
> > +
> > + pll_clk = devm_kzalloc(dev, sizeof(*pll_clk), GFP_KERNEL);
> > + if (!pll_clk)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + parent_name = __clk_get_name(parent);
> > + init.name = core->name;
> > + init.ops = &rzv2h_cpg_plldsi_ops;
> > + init.flags = 0;
> > + init.parent_names = &parent_name;
> > + init.num_parents = 1;
> > +
> > + pll_clk->hw.init = &init;
> > + pll_clk->pll = core->cfg.pll;
> > + pll_clk->base = base;
> > + pll_clk->priv = priv;
> > +
> > + /* Disable SSC and turn on PLL clock when init */
> > + writel(CPG_PLL_STBY_RESETB_WEN | CPG_PLL_STBY_RESETB |
> > + CPG_PLL_STBY_SSCGEN_WEN, base + CPG_PLL_STBY(pll_clk->pll.offset));
>
> Apart from the three lines above, this function does the same as the
> existing rzv2h_cpg_pll_clk_register(). Merge/reuse?
>
> > +
> > + ret = devm_clk_hw_register(dev, &pll_clk->hw);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + return pll_clk->hw.clk;
> > +}
> > +
> > static unsigned long rzv2h_cpg_pll_clk_recalc_rate(struct clk_hw *hw,
> > unsigned long parent_rate)
> > {
>
> > --- /dev/null
> > +++ b/include/linux/clk/renesas-rzv2h-dsi.h
> > @@ -0,0 +1,207 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Renesas RZ/V2H(P) DSI CPG helper
> > + *
> > + * Copyright (C) 2025 Renesas Electronics Corp.
> > + */
>
> Missing include guard.
>
> > +
> > +#include <linux/limits.h>
> > +#include <linux/math.h>
> > +#include <linux/math64.h>
> > +#include <linux/units.h>
> > +
> > +#define OSC_CLK_IN_MEGA (24 * MEGA)
> > +
> > +struct rzv2h_plldsi_div_limits {
>
> This structure looks applicable to all RZ/V2H PLLs, so perhaps drop the
> "dsi" part from the name?
>
> > + struct {
> > + u64 min;
> > + u64 max;
> > + } fvco;
> > +
> > + struct {
> > + u16 min;
> > + u16 max;
> > + } m;
> > +
> > + struct {
> > + u8 min;
> > + u8 max;
> > + } p;
> > +
> > + struct {
> > + u8 min;
> > + u8 max;
> > + } s;
> > +
> > + struct {
> > + s16 min;
> > + s16 max;
> > + } k;
> > +
> > + struct {
> > + u8 min;
> > + u8 max;
> > + } csdiv;
> > +};
> > +
> > +struct rzv2h_plldsi_parameters {
> > + u64 freq_mhz;
> > + s64 error_mhz;
> > + u16 m;
> > + s16 k;
> > + u8 csdiv;
> > + u8 p;
> > + u8 s;
> > +};
> > +
> > +#define RZV2H_CPG_PLL_DSI_LIMITS(name) \
> > + static const struct rzv2h_plldsi_div_limits (name) = { \
> > + .m = { .min = 64, .max = 533 }, \
> > + .p = { .min = 1, .max = 4 }, \
> > + .s = { .min = 0, .max = 6 }, \
> > + .k = { .min = -32768, .max = 32767 }, \
> > + .csdiv = { .min = 2, .max = 32 }, \
> > + .fvco = { .min = 1600 * MEGA, .max = 3200 * MEGA } \
> > + } \
> > +
> > +/**
> > + * rzv2h_dsi_get_pll_parameters_values - Finds the best combination of PLL parameters
> > + * and divider value for a given frequency.
> > + *
> > + * @limits: Pointer to the structure containing the limits for the PLL parameters and
> > + * divider values
> > + * @pars: Pointer to the structure where the best calculated PLL parameters and divider
> > + * values will be stored
> > + * @freq: Target output frequency (in mHz)
> > + *
> > + * This function calculates the best set of PLL parameters (M, K, P, S) and divider
> > + * value (CSDIV) to achieve the desired frequency.
> > + * There is no direct formula to calculate the PLL parameters and the divider value,
> > + * as it's an open system of equations, therefore this function uses an iterative
> > + * approach to determine the best solution. The best solution is one that minimizes
> > + * the error (desired frequency - actual frequency).
> > + *
> > + * Return: true if a valid set of divider values is found, false otherwise.
> > + */
> > +static __maybe_unused bool
> > +rzv2h_dsi_get_pll_parameters_values(const struct rzv2h_plldsi_div_limits *limits,
> > + struct rzv2h_plldsi_parameters *pars,
> > + u64 freq_mhz)
> > +{
>
> I haven't reviewed the heavy calculations yet.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
More information about the dri-devel
mailing list