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.
> 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.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
