Re: [RFC V3 3/3] phy: freescale: fsl-samsung-hdmi: Support dynamic integer

[Dominique Martinet]

Adam Ford wrote on Fri, Aug 30, 2024 at 10:04:41AM -0500:
> > So, as far as I'm concerned I'd be happy to move forward with that and
> > will backport this to our tree/remove our kludged values, would "just"
> > need to properly pick the closest value if no exact match instead of
> > always falling back to the table (or just remove the table altogether if
> > we can test a few monitors?)
> 
> I took that step on my local build machine to remove the integer
> divider stuff from the LUT since it can be found automatically.  I
> haven't committed them yet, but I'll likely add it to the series once
> submit it without the RFC.
> 
> I am also likely to update the rounding function to look at the delta
> between the closest LUT clock value and the closest integer clock
> value and use whichever is closest to the desired value.

Sounds good, thanks!

> > I'd change cur_cfg from pointer to the struct itself like this (partial
> > patch that probably won't even apply on your branch:)
> > ----
> > diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > index 9048cdc760c2..d7124604819c 100644
> > --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > @@ -402,7 +402,7 @@ struct fsl_samsung_hdmi_phy {
> >
> >         /* clk provider */
> >         struct clk_hw hw;
> > -       const struct phy_config *cur_cfg;
> > +       struct phy_config cur_cfg;
> >  };
> >
> >  static inline struct fsl_samsung_hdmi_phy *
> > @@ -562,9 +562,9 @@ static int phy_clk_set_rate(struct clk_hw *hw,
> >         if (i < 0)
> >                 return -EINVAL;
> >
> > -       phy->cur_cfg = &phy_pll_cfg[i];
> > +       phy->cur_cfg = phy_pll_cfg[i];
> >
> > -       return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);
> > +       return fsl_samsung_hdmi_phy_configure(phy, &phy->cur_cfg);
> >  }
> >
> >  static const struct clk_ops phy_clk_ops = {
> > ----
> >
> > Then you can just set it directly for calculated values.
> > But conceptually it's the same, just one less indirection.
> 
> I was thinking about that, but I wasn't sure the impact of
> un-const-ing would have on the LUT.
> 
> Using the extra indirection minimizes the impact of changes to the LUT
> and how its handled by the compiler.

The LUT has to stay const, you don't want something modifying it by
mistake; my suggestion makes cur_cfg a copy which does not need to be
const and the assignment (`phy->cur_cfg = phy_pll_cfg[i];`) is
equivalent to a memcpy -- the struct is small enough that the cost of
copying it if the value didn't match is negligible in my opinion.

With that said, I'm not saying you must do this, this just avoids using
a static value which could be problematic if there are multiple parallel
uses of the driver, go ahead with whatever you prefer for v4.
(note my approach doesn't fix the same problem if the driver is
used twice in parallel.. is it possible that e.g. one kernel thread
calls phy_clk_set_rate() while another thread checks something about the
hardware and calls phy_clk_round_rate()? If so neither approach is
correct and we can't easily skip recomputing, and it'd be better to just
have a phy_config struct on the stack instead -- on my imx8mp it takes
4.7us without -O, 0.6us with -O2 to compute; I *think* we can affort to
recompute every time...)

-- 
Dominique