Hi Biju,
Thank you for the review.
On Fri, Jun 13, 2025 at 6:57 AM Biju Das <biju.das...@bp.renesas.com> wrote:
>
> Hi Prabhakar,
>
> > -----Original Message-----
> > From: Prabhakar <prabhakar.cse...@gmail.com>
> > Sent: 30 May 2025 18:19
> .castro...@renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> > lad...@bp.renesas.com>
> > Subject: [PATCH v6 1/4] clk: renesas: rzv2h-cpg: Add support for DSI clocks
> >
> > 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>
> > ---
> > v5->v6:
> > - Renamed CPG_PLL_STBY_SSCGEN_WEN to CPG_PLL_STBY_SSC_EN_WEN
> > - Updated CPG_PLL_CLK1_DIV_K, CPG_PLL_CLK1_DIV_M, and
> > CPG_PLL_CLK1_DIV_P macros to use GENMASK
> > - Updated req->rate in rzv2h_cpg_plldsi_div_determine_rate()
> > - Dropped the cast in rzv2h_cpg_plldsi_div_set_rate()
> > - Dropped rzv2h_cpg_plldsi_round_rate() and implemented
> > rzv2h_cpg_plldsi_determine_rate() instead
> > - Made use of FIELD_PREP()
> > - Moved CPG_CSDIV1 macro in patch 2/4
> > - Dropped two_pow_s in rzv2h_dsi_get_pll_parameters_values()
> > - Used mul_u32_u32() while calculating output_m and output_k_range
> > - Used div_s64() instead of div64_s64() while calculating
> > pll_k
> > - Used mul_u32_u32() while calculating fvco and fvco checks
> > - Rounded the final output using DIV_U64_ROUND_CLOSEST()
> >
> > v4->v5:
> > - No changes
> >
> > v3->v4:
> > - Corrected parameter name in rzv2h_dsi_get_pll_parameters_values()
> > description freq_millihz
> >
> > v2->v3:
> > - Update the commit message to clarify the purpose of `renesas-rzv2h-dsi.h`
> > header
> > - Used mul_u32_u32() in rzv2h_cpg_plldsi_div_determine_rate()
> > - Replaced *_mhz to *_millihz for clarity
> > - Updated u64->u32 for fvco limits
> > - Initialized the members in declaration order for
> > RZV2H_CPG_PLL_DSI_LIMITS() macro
> > - Used clk_div_mask() in rzv2h_cpg_plldsi_div_recalc_rate()
> > - Replaced `unsigned long long` with u64
> > - Dropped rzv2h_cpg_plldsi_clk_recalc_rate() and reused
> > rzv2h_cpg_pll_clk_recalc_rate() instead
> > - In rzv2h_cpg_plldsi_div_set_rate() followed the same style
> > of RMW-operation as done in the other functions
> > - Renamed rzv2h_cpg_plldsi_set_rate() to rzv2h_cpg_pll_set_rate()
> > - Dropped rzv2h_cpg_plldsi_clk_register() and reused
> > rzv2h_cpg_pll_clk_register() instead
> > - Added a gaurd in renesas-rzv2h-dsi.h header
> >
> > v1->v2:
> > - No changes
> > ---
> > drivers/clk/renesas/rzv2h-cpg.c | 278 +++++++++++++++++++++++++-
> > drivers/clk/renesas/rzv2h-cpg.h | 13 ++
> > include/linux/clk/renesas-rzv2h-dsi.h | 210 +++++++++++++++++++
> > 3 files changed, 492 insertions(+), 9 deletions(-) create mode 100644
> > include/linux/clk/renesas-
> > rzv2h-dsi.h
> >
> > diff --git a/drivers/clk/renesas/rzv2h-cpg.c
> > b/drivers/clk/renesas/rzv2h-cpg.c index
> > 761da3bf77ce..d590f9f47371 100644
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> > @@ -14,9 +14,13 @@
> > #include <linux/bitfield.h>
> > #include <linux/clk.h>
> > #include <linux/clk-provider.h>
> > +#include <linux/clk/renesas-rzv2h-dsi.h>
> > #include <linux/delay.h>
> > #include <linux/init.h>
> > #include <linux/iopoll.h>
> > +#include <linux/math.h>
> > +#include <linux/math64.h>
> > +#include <linux/minmax.h>
> > #include <linux/mod_devicetable.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > @@ -26,6 +30,7 @@
> > #include <linux/refcount.h>
> > #include <linux/reset-controller.h>
> > #include <linux/string_choices.h>
> > +#include <linux/units.h>
> >
> > #include <dt-bindings/clock/renesas-cpg-mssr.h>
> >
> > @@ -48,12 +53,13 @@
> > #define CPG_PLL_STBY(x) ((x))
> > #define CPG_PLL_STBY_RESETB BIT(0)
> > #define CPG_PLL_STBY_RESETB_WEN BIT(16)
> > +#define CPG_PLL_STBY_SSC_EN_WEN BIT(18)
> > #define CPG_PLL_CLK1(x) ((x) + 0x004)
> > -#define CPG_PLL_CLK1_KDIV(x) ((s16)FIELD_GET(GENMASK(31, 16), (x)))
> > -#define CPG_PLL_CLK1_MDIV(x) FIELD_GET(GENMASK(15, 6), (x))
> > -#define CPG_PLL_CLK1_PDIV(x) FIELD_GET(GENMASK(5, 0), (x))
> > +#define CPG_PLL_CLK1_KDIV GENMASK(31, 16)
> > +#define CPG_PLL_CLK1_MDIV GENMASK(15, 6)
> > +#define CPG_PLL_CLK1_PDIV GENMASK(5, 0)
> > #define CPG_PLL_CLK2(x) ((x) + 0x008)
> > -#define CPG_PLL_CLK2_SDIV(x) FIELD_GET(GENMASK(2, 0), (x))
> > +#define CPG_PLL_CLK2_SDIV GENMASK(2, 0)
> > #define CPG_PLL_MON(x) ((x) + 0x010)
> > #define CPG_PLL_MON_RESETB BIT(0)
> > #define CPG_PLL_MON_LOCK BIT(4)
> > @@ -79,6 +85,8 @@
> > * @last_dt_core_clk: ID of the last Core Clock exported to DT
> > * @mstop_count: Array of mstop values
> > * @rcdev: Reset controller entity
> > + * @dsi_limits: PLL DSI parameters limits
> > + * @plldsi_div_parameters: PLL DSI and divider parameters configuration
> > */
> > struct rzv2h_cpg_priv {
> > struct device *dev;
> > @@ -95,6 +103,9 @@ struct rzv2h_cpg_priv {
> > atomic_t *mstop_count;
> >
> > struct reset_controller_dev rcdev;
> > +
> > + const struct rzv2h_pll_div_limits *dsi_limits;
> > + struct rzv2h_plldsi_parameters plldsi_div_parameters;
> > };
> >
> > #define rcdev_to_priv(x) container_of(x, struct rzv2h_cpg_priv, rcdev)
> > @@ -150,6 +161,24 @@ struct ddiv_clk {
> >
> > #define to_ddiv_clock(_div) container_of(_div, struct ddiv_clk, div)
> >
> > +/**
> > + * struct rzv2h_plldsi_div_clk - PLL DSI DDIV clock
> > + *
> > + * @dtable: divider table
> > + * @priv: CPG private data
> > + * @hw: divider clk
> > + * @ddiv: divider configuration
> > + */
> > +struct rzv2h_plldsi_div_clk {
> > + const struct clk_div_table *dtable;
> > + struct rzv2h_cpg_priv *priv;
> > + struct clk_hw hw;
> > + struct ddiv ddiv;
> > +};
> > +
> > +#define to_plldsi_div_clk(_hw) \
> > + container_of(_hw, struct rzv2h_plldsi_div_clk, hw)
> > +
> > static int rzv2h_cpg_pll_clk_is_enabled(struct clk_hw *hw) {
> > struct pll_clk *pll_clk = to_pll(hw);
> > @@ -198,6 +227,214 @@ 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 &= 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;
> > + 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;
>
> I believe this has to go after below assignment.
>
Good catch, agreed.
> As parent_rate = rate * calculated DSI divider value.
>
> req->rate = DIV_ROUND_CLOSEST_ULL(dsi_dividers->freq_millihz, MILLI);
> req->best_parent_rate = req->rate * dsi_dividers->csdiv;
>
>
>
> > + req->rate = DIV_ROUND_CLOSEST_ULL(dsi_dividers->freq_millihz, MILLI);
> > +
> > + 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;
> > + bool div_found = false;
> > + u32 val, shift, div;
> > +
> > + div = dsi_dividers->csdiv;
> > + for (clkt = dsi_div->dtable; clkt->div; clkt++) {
> > + if (clkt->div == div) {
> > + div_found = true;
> > + break;
> > + }
> > + }
> > +
> > + if (!div_found)
> > + return -EINVAL;
>
> This check can be done in determine rate and cache the divider??
>
Ok, I'll drop this check as the divider is already cached. The for
loop above is to determine the val which is used below to program the
registers.
> > +
> > + shift = ddiv.shift;
> > + val = readl(priv->base + ddiv.offset) | DDIV_DIVCTL_WEN(shift);
> > + val &= ~(clk_div_mask(ddiv.width) << shift);
> > + val |= clkt->val << shift;
> > + writel(val, priv->base + ddiv.offset);
> > +
> > + return 0;
> > +};
> > +
> > +static const struct clk_ops rzv2h_cpg_plldsi_div_ops = {
> > + .recalc_rate = rzv2h_cpg_plldsi_div_recalc_rate,
> > + .determine_rate = rzv2h_cpg_plldsi_div_determine_rate,
> > + .set_rate = rzv2h_cpg_plldsi_div_set_rate, };
> > +
> > +static struct clk * __init
> > +rzv2h_cpg_plldsi_div_clk_register(const struct cpg_core_clk *core,
> > + struct rzv2h_cpg_priv *priv)
> > +{
> > + struct rzv2h_plldsi_div_clk *clk_hw_data;
> > + struct clk **clks = priv->clks;
> > + struct clk_init_data init;
> > + const struct clk *parent;
> > + const char *parent_name;
> > + struct clk_hw *clk_hw;
> > + int ret;
> > +
> > + parent = clks[core->parent];
> > + if (IS_ERR(parent))
> > + return ERR_CAST(parent);
> > +
> > + clk_hw_data = devm_kzalloc(priv->dev, sizeof(*clk_hw_data),
> > GFP_KERNEL);
> > + if (!clk_hw_data)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + clk_hw_data->priv = priv;
> > + clk_hw_data->ddiv = core->cfg.ddiv;
> > + clk_hw_data->dtable = core->dtable;
> > +
> > + parent_name = __clk_get_name(parent);
> > + init.name = core->name;
> > + init.ops = &rzv2h_cpg_plldsi_div_ops;
> > + init.flags = core->flag;
> > + init.parent_names = &parent_name;
> > + init.num_parents = 1;
> > +
> > + clk_hw = &clk_hw_data->hw;
> > + clk_hw->init = &init;
> > +
> > + ret = devm_clk_hw_register(priv->dev, clk_hw);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + return clk_hw->clk;
> > +}
> > +
> > +static int rzv2h_cpg_plldsi_determine_rate(struct clk_hw *hw,
> > + struct clk_rate_request *req)
> > +{
> > + struct rzv2h_pll_div_limits dsi_limits;
> > + struct rzv2h_plldsi_parameters dsi_dividers;
> > + struct pll_clk *pll_clk = to_pll(hw);
> > + struct rzv2h_cpg_priv *priv = pll_clk->priv;
> > + u64 rate_millihz;
> > +
> > + memcpy(&dsi_limits, priv->dsi_limits, sizeof(dsi_limits));
> > + dsi_limits.csdiv.min = 1;
> > + dsi_limits.csdiv.max = 1;
> > +
> > + req->rate = clamp(req->rate, 25000000UL, 375000000UL);
>
> I guess this clamping is not required as the child already has clamping
> and max DSI divider is fixed.
>
> rate(Max) = 187500 * 1000 * divider = 187.5 MHz(clamped value) * dsi divider
>
Agreed, I will drop the clamp.
> > +
> > + rate_millihz = mul_u32_u32(req->rate, MILLI);
> > + 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;
> > + }
> > +
> > + req->best_parent_rate = req->rate * dsi_dividers.csdiv;
>
> This is wrong as it will lead to high value for parent as the rate is fixed
> 24MHz.
>
> 24MHz-> PLL_DSI(This clock) -> DSI DIVIDER-> DoT clock
>
Agreed we cannot adjust the best_parent_rate here.
Cheers,
Prabhakar
