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

[Biju Das <biju.das.jz@xxxxxxxxxxxxxx>]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v2 3/9] clk: renesas: rzg2l: Add DSI divider clk
> support
> 
> Hi Biju,
> 
> 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>
> > ---
> > V1->V2:
> >  * Replaced round_rate with determine_rate
> >  * Update rate variable during set_rate
> >  * Added get_vclk_parent_rate helper function
> >  * Replaced pll5_params with mux_dsi_div_params for dsi div values.
> 
> Thanks for the update!
> 
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> 
> > +static unsigned long rzg2l_cpg_get_vclk_parent_rate(struct clk_hw *hw,
> > +                                                   unsigned long
> > +rate) {
> > +       unsigned long parent_rate;
> 
> 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;
> > +       struct rzg2l_pll5_param params;
> 
> Reverse Xmas tree order?
OK.
> 
> > +
> > +       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.

Regards,
Biju

> 
> > +       writel(CPG_PL5_SDIV_DIV_DSI_A_WEN | CPG_PL5_SDIV_DIV_DSI_B_WEN |
> > +              (priv->mux_dsi_div_params.dsi_div_a << 0) |
> > +              (priv->mux_dsi_div_params.dsi_div_b << 8),
> > +              priv->base + CPG_PL5_SDIV);
> > +
> > +       return 0;
> > +}
> 
> 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