Re: [PATCH v2 3/9] clk: renesas: rzg2l: Add DSI divider clk support

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Biju,

On Fri, Apr 29, 2022 at 11:50 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > Subject: Re: [PATCH v2 3/9] clk: renesas: rzg2l: Add DSI divider clk
> > support

> > On Wed, Apr 27, 2022 at 11:48 AM 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 combining DSIDIVA and
> > > DSIDIVB .
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>

> > > +
> > > +       parent_rate = rzg2l_cpg_get_foutpostdiv_rate(&params, rate);
> > > +
> > > +       if (priv->mux_dsi_div_params.clksrc)
> > > +               parent_rate /= 2;
> > > +
> > > +       return parent_rate;
> > > +}
> > > +
> > > +static int rzg2l_cpg_dsi_div_determine_rate(struct clk_hw *hw,
> > > +                                           struct clk_rate_request
> > > +*req) {
> > > +       req->best_parent_rate = rzg2l_cpg_get_vclk_parent_rate(hw,
> > > +req->rate);
> > > +
> > > +       return 0;
> >
> > So any value of req->rate passed is supported, and req->rate never needs
> > to be updated?
>
> Yes, We can add a check for freq greater than 148.5MHz and if it is greater,
> Assign req->rate to 148.5MHz.
>
> >
> > > +}
> > > +
> > > +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;
> > > +
> > > +       /*
> > > +        * MUX -->DIV_DSI_{A,B} -->M3 -->VCLK
> > > +        *
> > > +        * Based on the dot clock, the DSI divider clock sets the
> > divider value,
> > > +        * calculates the pll parameters for generating FOUTPOSTDIV and
> > the clk
> > > +        * source for the MUX and propagates that info to the parents.
> > > +        */
> > > +
> > > +       if (!rate)
> > > +               return -EINVAL;
> > > +
> > > +       dsi_div->rate = rate;
> >
> > So any non-zero value of rate is supported?
>
> Currently yes, But If we add a check for > 148.5Mhz in determine_rate, it will allow only upto 148.5Mhz.

You still need that check here, too, as there is no guarantee users
will call .set_rate() with the rounded parameters obtained from
.determine_rate().

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