Message-ID: <CAHCN7xLo1RgkUPTdTpCtKjnT7w=gPDCfe_ZEbKj0joVvLG02Dw@mail.gmail.com>
Date: Thu, 5 Sep 2024 19:57:36 -0500
From: Adam Ford <aford173@...il.com>
To: Dominique Martinet <dominique.martinet@...ark-techno.com>
Cc: linux-phy@...ts.infradead.org, linux-imx@....com, festevam@...il.com, 
	frieder.schrempf@...tron.de, aford@...conembedded.com, Sandor.yu@....com, 
	Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>, 
	Marco Felsch <m.felsch@...gutronix.de>, Lucas Stach <l.stach@...gutronix.de>, 
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V6 4/5] phy: freescale: fsl-samsung-hdmi: Use closest divider

On Thu, Sep 5, 2024 at 7:26 PM Dominique Martinet
<dominique.martinet@...ark-techno.com> wrote:
>
>
> (sorry I meant to send this yesterday but I'm being forced to adjust my
> mail pipeline with work and gmail and it didn't go out -- trying
> again. Sorry if it actually did go through. Hopefully I didn't misfire
> anything else yesterday...)
>
> Adam Ford wrote on Wed, Sep 04, 2024 at 06:30:32PM -0500:
> > Currently, if the clock values cannot be set to the exact rate,
> > the round_rate and set_rate functions use the closest value found in
> > the look-up-table.  In preparation of removing values from the LUT
> > that can be calculated evenly with the integer calculator, it's
> > necessary to ensure to check both the look-up-table and the integer
> > divider clock values to get the closest values to the requested
> > value.  It does this by measuring the difference between the
> > requested clock value and the closest value in both integer divider
> > calucator and the fractional clock look-up-table.
> >
> > Which ever has the smallest difference between them is returned as
> > the cloesest rate.
> >
> > Signed-off-by: Adam Ford <aford173@...il.com>
> > Signed-off-by: Dominique Martinet <dominique.martinet@...ark-techno.com>
>
> b4 (or whatever you're using) probably picked that up from the patch I
> included in my reply to this patch, this sob should go away.
>
For each iteration, I grabbed the patches from patchwork which
contained any s-o-b messages, if present.  I didn't add anything
manually.
>
>
> > diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > index 4b13e386e5ba..9a21dbbf1a82 100644
> > --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > @@ -547,6 +547,16 @@ static unsigned long phy_clk_recalc_rate(struct clk_hw *hw,
> >       return phy->cur_cfg->pixclk;
> >  }
> >
> > +static u32 fsl_samsung_hdmi_phy_get_closest_rate(unsigned long rate,
> > +                                              u32 int_div_clk, u32 frac_div_clk)
> > +{
> > +     /* The int_div_clk may be greater than rate, so cast it and use ABS */
> > +     if (abs((long)rate - (long)int_div_clk) < (rate - frac_div_clk))
>
> I still think `rate - frac_div_clk` might not always hold in the future
> (because there is no intrinsic reason we'd pick the smaller end in case
> of inexact match and a future improvement might change this to the
> closest value as well), so I'll argue again for having both use abs(),
> but at least there's only one place to update if that changes in the
> future now so hopefully whoever does this will notice...

I can add the ABS on the fractional divider.  I left it out on purpose
since the LUT table always return a value equal or less, so the extra
ABS seemed like busy work.  However, I can see the argument for being
consistent.

>
> > +             return int_div_clk;
> > +
> > +     return frac_div_clk;
> > +}
> > +
> >  static long phy_clk_round_rate(struct clk_hw *hw,
> >                              unsigned long rate, unsigned long *parent_rate)
> >  {
> > @@ -563,6 +573,7 @@ static long phy_clk_round_rate(struct clk_hw *hw,
> >       for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
> >               if (phy_pll_cfg[i].pixclk <= rate)
> >                       break;
> > +
>
> (unrelated)

I don't understand what you're asking here.

>
> >       /* If the rate is an exact match, return it now */
> >       if (rate == phy_pll_cfg[i].pixclk)
> >               return phy_pll_cfg[i].pixclk;
> > @@ -579,8 +590,7 @@ static long phy_clk_round_rate(struct clk_hw *hw,
> >       if (int_div_clk == rate)
> >               return int_div_clk;
> >
> > -     /* Fall back to the closest value in the LUT */
> > -     return phy_pll_cfg[i].pixclk;
> > +     return fsl_samsung_hdmi_phy_get_closest_rate(rate, int_div_clk, phy_pll_cfg[i].pixclk);
> >  }
> >
> >  static int phy_clk_set_rate(struct clk_hw *hw,
> > @@ -594,27 +604,37 @@ static int phy_clk_set_rate(struct clk_hw *hw,
> >
> >       /* If the integer divider works, just use it */
>
> I found this comment a bit confusing given the current flow as of this
> patch. Might make more sense immediately before the if?
>

This code evolved with each iteration, but I didn't necessarily
reorganize the comments.  I can rearrange them.
>
> >       int_div_clk = fsl_samsung_hdmi_phy_find_pms(rate * 5, &p, &m, &s) / 5;
> > +     calculated_phy_pll_cfg.pixclk = int_div_clk;
> > +     calculated_phy_pll_cfg.pll_div_regs[0] = FIELD_PREP(REG01_PMS_P_MASK, p);
> > +     calculated_phy_pll_cfg.pll_div_regs[1] = m;
> > +     calculated_phy_pll_cfg.pll_div_regs[2] = FIELD_PREP(REG03_PMS_S_MASK, s-1);
> > +     phy->cur_cfg = &calculated_phy_pll_cfg;
> >       if (int_div_clk == rate) {
> >               dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using integer divider\n");
> > -             calculated_phy_pll_cfg.pixclk = int_div_clk;
> > -             calculated_phy_pll_cfg.pll_div_regs[0] = FIELD_PREP(REG01_PMS_P_MASK, p);
> > -             calculated_phy_pll_cfg.pll_div_regs[1] = m;
> > -             calculated_phy_pll_cfg.pll_div_regs[2] = FIELD_PREP(REG03_PMS_S_MASK, s-1);
> > -             /* pll_div_regs 3-6 are fixed and pre-defined already */
>
> nitpick: might want to keep the above comment?

ok.
>
> > -             phy->cur_cfg  = &calculated_phy_pll_cfg;
> > +             goto done;
> >       } else {
> >               /* Otherwise, search the LUT */
> > -             dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");
> > -             for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
> > -                     if (phy_pll_cfg[i].pixclk <= rate)
> > +             for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--) {
> > +                     if (phy_pll_cfg[i].pixclk == rate) {
> > +                             dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");
>
> nitpick: might make sense to print what was picked in case of inexact
> match as well, but these are dbg warning so probably fine either way.

I can add the actual values returned.

>
>
> overall I find the flow of this function hard to read; it's a bit ugly
> flow-wise but jumping in the clock comparison 'if' might help trim this?
> (and if we're going out of our way to factor out the diff, maybe the lut
> lookup could be as well)
>
> But I'm probably just being overcritical here, it's fine as is if you
> pefer your version, just writing down this as an illustration of what I
> meant with the above sentence as I'm not sure I was clear -- I'll be
> just as happy to consider this series done so we can do more interesting
> things :P

Now I am a bit more confused, because above I got the impression you
were withdrawing your s-o-b, but now it sounds like you want to move
it forward.

It sounded like Frieder was making some progress on understanding a
little more about the fractional divider.

>
> {
>     u32 int_div_clk, frac_div_clk;
>     int i;
>     u16 m;
>     u8 p, s;
>
>     // (I haven't given up on that *5 to move inside this function...)

I wanted to keep the PMS calculator returning the real clock value
since the calculations are based on equation in the ref manual, Fpll =
Fref * M / (P*S)
This way, the calling function can determine if it needs to be
multiplied by 5.  I haven't fully determined how the fractional
calculator determines what frequency it wants for a target frequency,
and using the values for P, M and S from the fractional divider
doesn't seem to always yield 5x like they did for the table entries
using the integer divider.

I am hoping someone from NXP can elaborate, or give us some clues on
how to get better fractional divider values.

>     int_div_clk = fsl_samsung_hdmi_phy_find_pms(rate, &p, &m, &s);
>     if (int_div_clk == rate)
>         goto use_int_clk;
>
>     frac_div_clk = fsl_samsung_hdmi_phy_find_lut(rate, &i);
>     // (not convinced that check actually brings much, but it's not like
>     // it hurts either)
>     if (frac_div_clk == rate)
>         goto use_frac_clk;
>
>     if (fsl_samsung_hdmi_phy_get_closest_rate(rate, int_div_clk,
>                                               frac_div_clk) == int_div_clk) {
> use_int_clk:
>         dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using integer divider\n");
>         calculated_phy_pll_cfg.pixclk = int_div_clk;
>         calculated_phy_pll_cfg.pll_div_regs[0] = FIELD_PREP(REG01_PMS_P_MASK, p);
>         calculated_phy_pll_cfg.pll_div_regs[1] = m;
>         calculated_phy_pll_cfg.pll_div_regs[2] = FIELD_PREP(REG03_PMS_S_MASK, s-1);
>         /* pll_div_regs 3-6 are fixed and pre-defined already */
>         phy->cur_cfg  = &calculated_phy_pll_cfg;
>     } else {
> use_frac_clk:
>         dev_dbg(phy->dev, "fsl_samsung_hdmi_phy: using fractional divider\n");
>         phy->cur_cfg = &phy_pll_cfg[i];
>     }
>     return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);
> }
>
> --
> Dominique

Powered by blists - more mailing lists