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@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
>
> 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
