Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH 3/9] clk: renesas: rzg2l: Add DSI divider clk support
>
> Hi Biju,
>
> On Fri, Apr 22, 2022 at 1:15 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> wrote:
> > > Subject: Re: [PATCH 3/9] clk: renesas: rzg2l: Add DSI divider clk
> > > support 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>
>
> > > > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > > > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > > > +static long rzg2l_cpg_dsi_div_round_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;
> > > > +
> > > > + 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;
> > > > + 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}
>
> There is no guarantee that .set_rate() is called right after
> .determine_rate() with the exact same parameters, or that it is called at
> all. Modifying priv->pll5_params prematurely may affect the other clocks
> in hard to debug ways. So please modify priv->pll5_params only when
> actually setting the clock rate.
OK will do this change in next version.
>
> > I have added there to avoid duplication.
>
> You can use a helper that takes a pointer to a struct rzg2l_pll5_param,
> calculates the values, and fills them in.
> .{determine,round rate}() would call it with a pointer to a local
> variable.
> .set_rate() would call it with a pointer to &priv->pll5_params.
OK. Will send these changes in next version
Cheers,
Biju
