Re: [PATCH v3 2/8] drm/msm/dsi: 28nm 8960 PHY: Get ref clock from the DT

[Matthias Kaehlcke <mka@chromium.org>]

On Tue, Dec 04, 2018 at 08:44:00AM -0800, Stephen Boyd wrote:
> Quoting Matthias Kaehlcke (2018-11-30 16:52:48)
> > Get the ref clock of the PHY from the device tree instead of
> > hardcoding its name and rate. Use default values if the ref
> > clock is not specified.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > Changes in v3:
> > - use default name and rate if the ref clock is not specified
> >   in the DT
> > - store vco_ref_clk_name instead of vco_ref_clk
> > - fixed check for EPROBE_DEFER
> > - renamed VCO_REF_CLK_RATE to VCO_REF_CLK_DEFAULT_RATE
> > 
> > Changes in v2:
> > - patch added to the series
> > ---
> >  .../gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c   | 28 +++++++++++++++----
> >  1 file changed, 23 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c
> > index 49008451085b8..3af678d3317f6 100644
> > --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c
> > +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c
> > @@ -47,9 +47,9 @@
> >  
> >  #define NUM_PROVIDED_CLKS      2
> >  
> > -#define VCO_REF_CLK_RATE       27000000
> > -#define VCO_MIN_RATE           600000000
> > -#define VCO_MAX_RATE           1200000000
> > +#define VCO_REF_CLK_DEFAULT_RATE       27000000
> > +#define VCO_MIN_RATE                   600000000
> > +#define VCO_MAX_RATE                   1200000000
> >  
> >  #define DSI_BYTE_PLL_CLK       0
> >  #define DSI_PIXEL_PLL_CLK      1
> > @@ -75,6 +75,8 @@ struct dsi_pll_28nm {
> >         struct platform_device *pdev;
> >         void __iomem *mmio;
> >  
> > +       const char *vco_ref_clk_name;
> 
> Can this be passed around during clk registration so we don't have to
> store it away in the structure?

makes sense, will do

> > +
> >         /* custom byte clock divider */
> >         struct clk_bytediv *bytediv;
> >  
> > @@ -125,7 +127,10 @@ static int dsi_pll_28nm_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> >         DBG("rate=%lu, parent's=%lu", rate, parent_rate);
> >  
> >         temp = rate / 10;
> > -       val = VCO_REF_CLK_RATE / 10;
> > +       if (parent_rate)
> > +               val = parent_rate / 10;
> > +       else
> > +               val = VCO_REF_CLK_DEFAULT_RATE / 10;
> 
> Is the clk not properly hooked up to a parent sometimes so parent_rate
> is 0? That sounds odd given the fact that it used to be 'pxo' and that
> has always existed on the system as 27 MHz. So I'd remove this and just
> use parent_rate all the time.

I wondered about this, but since I don't have hardware for testing I
kept the previous hardcoded rate. If we know for sure that 'pxo'
always exists it should indeed be fine to use the parent rate.