RE: [RFC 01/28] clk: renesas: rzg2l: Add FOUTPOSTDIV clk support

From: Biju Das <biju.das.jz@bp.renesas.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	linux-clk <linux-clk@vger.kernel.org>,
	Chris Paterson <Chris.Paterson2@renesas.com>,
	Biju Das <biju.das@bp.renesas.com>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
Subject: RE: [RFC 01/28] clk: renesas: rzg2l: Add FOUTPOSTDIV clk support
Date: Fri, 18 Mar 2022 10:21:37 +0000	[thread overview]
Message-ID: <OS0PR01MB5922951AA71AF6A86F3DE34986139@OS0PR01MB5922.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAMuHMdVSQNKyoFEg59u5jBYGww6-EB23=DV93gNeavtMNsms8Q@mail.gmail.com>

Hi Geert,

Thanks for the feedback.

> Subject: Re: [RFC 01/28] clk: renesas: rzg2l: Add FOUTPOSTDIV clk support
> 
> Hi Biju,
> 
> On Wed, Jan 12, 2022 at 6:46 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > PLL5 generates FOUTPOSTDIV clk and is sourced by LCDC/DSI modules.
> > The FOUTPOSTDIV is connected to PLL5_4 MUX. Video clock is sourced
> > from DSI divider which is connected to PLL5_4 MUX.
> >
> > Added 2 LUT's for generating FOUTPOSTDIV, 1 for DSI mode and other for
> > DPI mode as it requires different parameters for generating the video
> > clock. The LUT supports minimal set of frequency used by commonly used
> > resolutions.
> >
> > This patch uses the above LUT to generate the required video clock by
> > matching the frequency value in LUT with FOUTPOSTDIV/DSI_DIV.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > @@ -78,6 +78,8 @@ struct sd_hw_data {
> >   * @last_dt_core_clk: ID of the last Core Clock exported to DT
> >   * @notifiers: Notifier chain to save/restore clock state for system
> resume
> >   * @info: Pointer to platform data
> > + * @pll5_table: Table containing a set of pll5 parameters
> > + * @num_pll5_entries: Number of entries in pll5 table
> >   */
> >  struct rzg2l_cpg_priv {
> >         struct reset_controller_dev rcdev; @@ -93,6 +95,9 @@ struct
> > rzg2l_cpg_priv {
> >
> >         struct raw_notifier_head notifiers;
> >         const struct rzg2l_cpg_info *info;
> > +
> > +       const struct rzg2l_pll5_param *pll5_table;
> > +       unsigned int num_pll5_entries;
> >  };
> >
> >  static void rzg2l_cpg_del_clk_provider(void *data) @@ -266,6 +271,203
> > @@ rzg2l_cpg_sd_mux_clk_register(const struct cpg_core_clk *core,
> >         return clk_hw->clk;
> >  }
> >
> > +struct rzg2l_pll5_param {
> > +       u64     frequency;
> 
> u32 should be sufficient.

OK.

> 
> > +       u64     pl5_refdiv;
> 
> u8? ;-)
> 
> (and move down to avoid gaps)
> 
> > +       u32     pl5_intin;
> 
> u8?
> 
> > +       u32     pl5_fracin;
> > +       u8      pl5_postdiv1;
> > +       u8      pl5_postdiv2;
> > +       u8      dsi_div_a;
> > +       u8      dsi_div_b;
> > +       u8      dsi_div;
> > +       u8      clksrc;
> > +};
> > +
> > +static const struct rzg2l_pll5_param dsi_mode_param[] = {
> > +       {   25175000, 1,  16, 13141593, 1, 1, 2, 3, 16, 0 }, /* VGA
> 25.175MHz */
> > +       {   25200000, 1,  16, 13421773, 1, 1, 2, 3, 16, 0 }, /* VGA
> 25.200MHz */
> > +       {   27000000, 1,  18,        0, 1, 1, 2, 3, 16, 0 }, /*
> 480p/576p 27.000MHz */
> > +       {   27027000, 1,  18,   301990, 1, 1, 2, 3, 16, 0 }, /* 480p
> 27.027MHz */
> > +       {   29605000, 1,  19, 12359216, 1, 1, 2, 3, 16, 0 }, /* WVGA
> 29.605MHz */
> > +       {   40000000, 2,  80,        0, 2, 1, 1, 2,  6, 0 }, /* SVGA
> 40.00MHz */
> > +       {   65000000, 1,  43,  5592405, 1, 1, 2, 3, 16, 0 }, /* XGA
> 65.00MHz */
> > +       {   71000000, 2,  71,        0, 1, 1, 1, 2,  6, 0 }, /* WXGA
> 1280x800 71.0MHz */
> > +       {   74176000, 1,  49,  7560932, 1, 1, 2, 3, 16, 0 }, /* 720p
> 74.176MHz */
> > +       {   74250000, 1,  49,  8388608, 1, 1, 2, 3, 16, 0 }, /* 720p
> 74.25MHz */
> > +       {   85500000, 2,  85,  8388608, 1, 1, 1, 2,  6, 0 }, /* FWXGA
> 1360x768 85.5MHz */
> > +       {   88750000, 2,  88, 12582912, 1, 1, 1, 2,  6, 1 }, /* WXGA+
> 1440x900 88.75MHz */
> > +       {  108000000, 2, 108,        0, 1, 1, 1, 2,  6, 1 }, /* SXGA
> 108MHz */
> > +       {  148500000, 2, 148,  8388608, 1, 1, 1, 2,  6, 1 }, /* 1080p
> 148.5MHz */
> > +       { 3000000000, 1, 125,        0, 1, 1, 0, 0,  0, 0 }, /* 3000 MHz
> */
> 
> 3000000000U, as this is larger than 1^31.
> Why do you need the 3 GHz entry? There are no such high video modes.
> 
> Do you need .dsi_div?
> .dsi_div = (1 << .dsi_div_a) * (.dsi_div_b + 1)

OK, Will change it.

> 
> Personally, I don't like tables, as calculations are more flexible.
> I'd expect the formula to be:

Agreed, Will use the generic equation and remove the LUT for next version.

> 
>     .frequency = extal / .pl5_refdiv *
>                  ((.pl5_intin << 24 + .pl5_fracin) >> 24) /
>                  (.pl5_postdiv1 * .pl5_postdiv * .dsi_div)
> 
> (with extal = 24 MHz)
> 
> However, that gives the wrong results for .pl5_refdiv = 2:
>   - For such entries in the above table, it calculates a frequency
>     that's twice the value in the table,
>   - For such entries in the below table, it calculates a frequency
>     that's half the value in the table.

> 
> Note that some entries (esp. the first one) from the table below give a
> clock rate closer to the expected one the corresponding entries from the
> table above.
> 
> > +};
> > +
> > +static const struct rzg2l_pll5_param dpi_mode_param[] = {
> > +       {   25175000, 1, 102, 13386820, 7, 7, 1, 0, 2, 0 }, /* VGA
> 25.175MHz */
> > +       {   25200000, 1,  73,  8388608, 7, 5, 1, 0, 2, 0 }, /* VGA
> 25.200MHz */
> > +       {   27000000, 1,  78, 12582912, 7, 5, 1, 0, 2, 0 }, /* 480p/576p
> 27.000MHz */
> > +       {   27027000, 1, 110,  6043992, 7, 7, 1, 0, 2, 0 }, /* 480p
> 27.027MHz */
> > +       {   29605000, 1,  88, 13673431, 6, 6, 1, 0, 2, 0 }, /* WVGA
> 29.605MHz */
> > +       {   40000000, 1,  70,        0, 7, 3, 1, 0, 2, 0 }, /* SVGA
> 40.00MHz */
> > +       {   65000000, 1,  81,  4194304, 5, 3, 1, 0, 2, 0 }, /* XGA
> 65.00MHz */
> > +       {   71000000, 2,  71,        0, 6, 2, 1, 0, 2, 0 }, /* WXGA
> 1280x800 71.0MHz */
> > +       {   74176000, 1,  74,  2952790, 6, 2, 1, 0, 2, 0 }, /* 720p
> 74.176MHz */
> > +       {   74250000, 1,  86, 10485760, 7, 2, 1, 0, 2, 0 }, /* 720p
> 74.25MHz */
> > +       {   85500000, 1,  83,  8388608, 6, 2, 1, 0, 2, 0 }, /* WXGA
> 1280x800 83.5MHz   */
> 
> 83500000
> 
> > +       { 3000000000, 1, 125,        0, 1, 1, 0, 0, 0, 0 }, /* 3000 MHz
> */
> > +};
> > +
> > +static int rzg2l_cpg_sipll5_get_index(unsigned long rate,
> > +                                     const struct rzg2l_pll5_param
> *pll5tab,
> > +                                     unsigned int n)
> 
> Perhaps just pass priv instead of pll5tab and n?
> 
> > +{
> > +       unsigned int  i;
> > +
> > +       for (i = 0; i < n; i++) {
> > +               if (pll5tab[i].frequency  == rate / pll5tab[i].dsi_div)
> > +                       break;
> > +       }
> > +
> > +       if (i == n)
> > +               i--;
> > +
> > +       return i;
> > +}
> > +
> > +struct sipll5 {
> > +       struct clk_hw hw;
> > +       unsigned int conf;
> > +       unsigned long rate;
> > +       struct rzg2l_cpg_priv *priv;
> > +};
> > +
> > +#define to_sipll5(_hw) container_of(_hw, struct sipll5, hw)
> > +
> > +static unsigned long rzg2l_cpg_sipll5_get_rate(unsigned long rate,
> > +                                              const struct
> rzg2l_pll5_param *pll5tab,
> > +                                              unsigned int n)
> 
> This function has only a single caller, so perhaps it's better to inline
> it manually?
> 
> > +{
> > +       int index = rzg2l_cpg_sipll5_get_index(rate, pll5tab, n);
> > +
> > +       return pll5tab[index].frequency * pll5tab[index].dsi_div; }
> > +
> > +static unsigned long rzg2l_cpg_sipll5_recalc_rate(struct clk_hw *hw,
> > +                                                 unsigned long
> > +parent_rate) {
> > +       struct sipll5 *sipll5 = to_sipll5(hw);
> > +
> > +       return sipll5->rate;
> > +}
> > +
> > +static long rzg2l_cpg_sipll5_round_rate(struct clk_hw *hw,
> > +                                       unsigned long rate,
> > +                                       unsigned long *parent_rate) {
> > +       struct sipll5 *sipll5 = to_sipll5(hw);
> > +       struct rzg2l_cpg_priv *priv = sipll5->priv;
> > +       const struct rzg2l_pll5_param *pll5tab = priv->pll5_table;
> > +
> > +       sipll5->rate = rzg2l_cpg_sipll5_get_rate(rate, pll5tab, priv-
> >num_pll5_entries);
> > +       return sipll5->rate;
> > +}
> > +
> > +static int rzg2l_cpg_sipll5_set_rate(struct clk_hw *hw,
> > +                                    unsigned long rate,
> > +                                    unsigned long parent_rate) {
> > +       struct sipll5 *sipll5 = to_sipll5(hw);
> > +       struct rzg2l_cpg_priv *priv = sipll5->priv;
> > +       const struct rzg2l_pll5_param *pll5tab = priv->pll5_table;
> > +       u8 id = rzg2l_cpg_sipll5_get_index(rate, pll5tab, priv-
> >num_pll5_entries);
> > +       int ret;
> > +       u32 val;
> > +
> > +       /* Put PLL5 into standby mode */
> > +       writel(0x00050000, priv->base + CPG_SIPLL5_STBY);
> 
> Please add defines for magic numbers

OK.

> 
> > +       ret = readl_poll_timeout(priv->base + CPG_SIPLL5_MON, val,
> > +                                !(val & CPG_SIPLL5_MON_PLL5_LOCK), 100,
> 250000);
> > +       if (ret) {
> > +               dev_err(priv->dev, "failed to release pll5 lock");
> > +               return ret;
> > +       }
> > +
> > +       /* Output clock setting 1 */
> > +       writel(0x01110000 |
> 
> Magic number
OK.
> 
> > +              (pll5tab[id].pl5_postdiv1 << 0) |
> > +              (pll5tab[id].pl5_postdiv2 << 4) |
> > +              (pll5tab[id].pl5_refdiv << 8),
> > +              priv->base + CPG_SIPLL5_CLK1);
> > +       /* Output clock setting, SSCG modulation value setting 3 */
> > +       writel((0 << 0) | (pll5tab[id].pl5_fracin << 8), priv->base +
> CPG_SIPLL5_CLK3);
> > +       /* Output clock setting 4 */
> > +       writel(0x000000ff | (pll5tab[id].pl5_intin << 16), priv->base
> > + + CPG_SIPLL5_CLK4);
> 
> magic number (and more below)

OK.

> 
> > +
> > +       /* SSCG modulation value setting 5 */
> > +       writel((0x16 << 0), priv->base + CPG_SIPLL5_CLK5);
> 
> > +       /* PLL normal mode setting */
> > +       writel(0x00050001, priv->base + CPG_SIPLL5_STBY);
> > +
> > +       /* PLL normal mode transition, output clock stability check */
> > +       ret = readl_poll_timeout(priv->base + CPG_SIPLL5_MON, val,
> > +                                (val & CPG_SIPLL5_MON_PLL5_LOCK), 100,
> 250000);
> > +       if (ret) {
> > +               dev_err(priv->dev, "failed to lock pll5");
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> 
> > @@ -918,6 +1123,18 @@ static int __init rzg2l_cpg_probe(struct
> platform_device *pdev)
> >         priv->num_resets = info->num_resets;
> >         priv->last_dt_core_clk = info->last_dt_core_clk;
> >
> > +       priv->pll5_table = dpi_mode_param;
> > +       priv->num_pll5_entries = ARRAY_SIZE(dpi_mode_param);
> > +       /* Fix me: Selection of the table needs to be overridden by
> either
> > +        * 1) a property in DTS or
> > +        * 2) Detecting DSI/DPI mode from dts or
> > +        * 3) DSI/DPI mode runtime detection
> > +        */
> > +       if (info->pll5_lcdc_dsi_mode) {
> > +               priv->pll5_table = dsi_mode_param;
> > +               priv->num_pll5_entries = ARRAY_SIZE(dsi_mode_param);
> > +       }
> > +
> 
> I'd expect that to be detected at runtime, or derived from what clock
> rate(s) the display driver asks for?

OK. Will Add DSI support for now and when we add RZ/G2UL support
At that time will add runtime check for detecting DSI and DPI.

Cheers,
Biju