linux-kernel - Re: [PATCH V4 3/5] phy: freescale: fsl-samsung-hdmi: Support dynamic integer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHCN7xJs1AefLJ1=0FfOs=A2B-gE5sC_VSVS6aGVhnSPOKDuxg@mail.gmail.com>
Date: Wed, 4 Sep 2024 05:37:00 -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>, 
	Lucas Stach <l.stach@...gutronix.de>, Marco Felsch <m.felsch@...gutronix.de>, 
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V4 3/5] phy: freescale: fsl-samsung-hdmi: Support dynamic integer

On Wed, Sep 4, 2024 at 3:28 AM Dominique Martinet
<dominique.martinet@...ark-techno.com> wrote:
>
> Adam Ford wrote on Mon, Sep 02, 2024 at 08:30:45PM -0500:
> > There is currently a look-up table for a variety of resolutions.
> > Since the phy has the ability to dynamically calculate the values
> > necessary to use the intger divider which should allow more
> > resolutions without having to update the look-up-table.
> >
> > If the lookup table cannot find an exact match, fall back to the
> > dynamic calculator of the integer divider.
> >
> > Previously, the value of P was hard-coded to 1, this required an
> > update to the phy_pll_cfg table to add in the extra value into the
> > table, so if the value of P is calculated to be something else
> > by the PMS calculator, the calculated_phy_pll_cfg structure
> > can be used instead without having to keep track of which method
> > was used.
> >
> > Signed-off-by: Adam Ford <aford173@...il.com>
>
> I've rechecked this series with abs() added in the later patch and this

Oh shoot, I totally missed your request for the abs patch.
Sorry about that.  Do you want me to do a V6?

adam

> looks fine; all modes I tried properly synced up with my monitor.
> (except one but I don't see set_rate() being called for it so it's
> something else)
>
> (On a semi-unrelated note on my backport I get a "PLL failed to lock"
> message for the first sync only, but everything seems to work regardless
> even if there is no further set_rate(), so I'll pretend I didn't see
> that... the old code just has a 20ms wait without any check so it's not
> like it was any better... anyway that's unrelated to this serie)
>
> I'm also confident enough set_rate() won't be called in parallel with
> different rates for my device so I'm fine with the new global, letting
> others complain if that's a problem for them.
>
>
> So, feel free to add this to all 5 patches:
> Test-by: Dominique Martinet <dominique.martinet@...ark-techno.com>
>
> Just one style nitpick:
>
> > @@ -453,29 +541,70 @@ static unsigned long phy_clk_recalc_rate(struct clk_hw *hw,
> >  static long phy_clk_round_rate(struct clk_hw *hw,
> >                              unsigned long rate, unsigned long *parent_rate)
> >  {
> > +     u32 int_div_clk;
> >       int i;
> > +     u16 m;
> > +     u8 p, s;
> > +
> > +     /* If the clock is out of range return error instead of searching */
> > +     if (rate > 297000000 || rate < 22250000)
> > +             return -EINVAL;
> >
> > +     /* Check the look-up table */
> >       for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
> >               if (phy_pll_cfg[i].pixclk <= rate)
> > -                     return phy_pll_cfg[i].pixclk;
> > +                     break;
> > +     /* If the rate is an exact match, return it now */
> > +     if (rate == phy_pll_cfg[i].pixclk)
> > +             return phy_pll_cfg[i].pixclk;
> > +
> > +     /*
> > +      * The math on the lookup table shows the PMS math yields a
> > +      * frequency 5 x pixclk.
> > +      * When we check the integer divider against the desired rate,
> > +      * multiply the rate x 5 and then divide the outcome by 5.
> > +      */
> > +     int_div_clk = fsl_samsung_hdmi_phy_find_pms(rate * 5, &p, &m, &s) / 5;
>
> I still think it makes more sense to move the * 5, / 5 and comment
> inside fsl_samsung_hdmi_phy_find_pms -- the other caller doesn't have
> such the comment so it might look odd depending on where one started
> looking.
>
> --
> Dominique