Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH 3/9] clk: renesas: rzg2l: Add DSI divider clk support
>
> Hi Biju,
>
> On Fri, Mar 18, 2022 at 6:51 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> wrote:
> > M3 clock is sourced from DSI Divider (DSIDIVA * DSIDIVB)
> >
> > This patch add support for DSI divider clk by combaining DSIDIVA and
> > DSIDIVB .
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > RFC->V1
> > * Removed LUT and added an equation for computing VCLK.
>
> Thanks for the update!
>
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > @@ -279,6 +282,114 @@ rzg2l_cpg_sd_mux_clk_register(const struct
> cpg_core_clk *core,
> > return clk_hw->clk;
> > }
> >
> > +struct dsi_div_hw_data {
> > + struct clk_hw hw;
> > + u32 conf;
> > + unsigned long rate;
> > + struct rzg2l_cpg_priv *priv;
> > +};
> > +
> > +#define to_dsi_div_hw_data(_hw) container_of(_hw, struct
> dsi_div_hw_data, hw)
> > +
> > +static unsigned long rzg2l_cpg_dsi_div_recalc_rate(struct clk_hw *hw,
> > + unsigned long
> > +parent_rate) {
> > + struct dsi_div_hw_data *dsi_div = to_dsi_div_hw_data(hw);
> > +
> > + return dsi_div->rate;
> > +}
> > +
> > +static long rzg2l_cpg_dsi_div_round_rate(struct clk_hw *hw,
> > + unsigned long rate,
> > + unsigned long *parent_rate)
>
> Please implement the determine_rate() instead.
OK.
>
> > +
>
> Please drop the blank line.
OK.
>
> > +{
> > + struct dsi_div_hw_data *dsi_div = to_dsi_div_hw_data(hw);
> > + struct rzg2l_cpg_priv *priv = dsi_div->priv;
> > +
> > + dsi_div->rate = rate;
> > +
> > + priv->pll5_params.pl5_intin = rate / MEGA;
>
> .round_rate() (and .determine_rate()) is used to check if a rate is
> supported, without actually changing the clock rate. Hence this should not
> operate on priv->pll5_params, but on a local variable.
>
> > + priv->pll5_params.pl5_fracin = ((rate % MEGA) << 24) / MEGA;
>
> While this works fine on 64-bit (RZ/G2L is arm64, so that's OK), "(rate %
> MEGA) << 24" will overflow when compile-testing on 32-bit.
> Taking into account the 64-by-32 division, I think this should be:
>
> div_u64(((u64)rate % MEGA) << 24, MEGA);
OK.
>
> > + priv->pll5_params.pl5_refdiv = 2;
> > + priv->pll5_params.pl5_postdiv1 = 1;
> > + priv->pll5_params.pl5_postdiv2 = 1;
> > + priv->pll5_params.clksrc = 1;
> > + priv->pll5_params.dsi_div_a = 1;
> > + priv->pll5_params.dsi_div_b = 2;
> > +
> > + priv->pll5_params.frequency =
> > + EXTAL_FREQ_IN_MEGA_HZ * MEGA / priv-
> >pll5_params.pl5_refdiv *
> > + ((((priv->pll5_params.pl5_intin << 24) + priv-
> >pll5_params.pl5_fracin)) >> 24) /
> > + (priv->pll5_params.pl5_postdiv1 *
> > + priv->pll5_params.pl5_postdiv2);
> > +
> > + if (priv->pll5_params.clksrc)
> > + priv->pll5_params.frequency /= 2;
> > +
> > + *parent_rate = priv->pll5_params.frequency;
> > +
> > + return dsi_div->rate;
> > +}
> > +
> > +static int rzg2l_cpg_dsi_div_set_rate(struct clk_hw *hw,
> > + unsigned long rate,
> > + unsigned long parent_rate) {
> > + struct dsi_div_hw_data *dsi_div = to_dsi_div_hw_data(hw);
> > + struct rzg2l_cpg_priv *priv = dsi_div->priv;
> > +
>
> You should update priv->pll5_params here, instead of in your
> .round_rate() callback.
I need to find parent_rate based on video clock during {determine,round rate}
I have added there to avoid duplication.
I agree, the following to be set here instead of round_rate() callback.
dsi_div->rate = rate;
Cheers,
Biju
>
> > + writel(CPG_PL5_SDIV_DIV_DSI_A_WEN | CPG_PL5_SDIV_DIV_DSI_B_WEN |
> > + (priv->pll5_params.dsi_div_a << 0) | (priv-
> >pll5_params.dsi_div_b << 8),
> > + priv->base + CPG_PL5_SDIV);
> > +
> > + return 0;
> > +}
>
> The rest LGTM.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
