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

[Adam Ford]

On Wed, Sep 4, 2024 at 3:28 AM Dominique Martinet
<dominique.martinet@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxx>
>
> 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