Re: [PATCH v3 2/3] clk: qcom: clk-alpha-pll: Add support for Trion PLLs

[Vinod Koul <vkoul@kernel.org>]

On 27-06-19, 14:50, Stephen Boyd wrote:
> Quoting Vinod Koul (2019-06-24 23:31:39)
> > diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> > index 2c6773188761..30210f5c6726 100644
> > --- a/drivers/clk/qcom/clk-alpha-pll.c
> > +++ b/drivers/clk/qcom/clk-alpha-pll.c
> > @@ -32,6 +32,7 @@
> >  # define PLL_LOCK_DET          BIT(31)
> >  
> >  #define PLL_L_VAL(p)           ((p)->offset + (p)->regs[PLL_OFF_L_VAL])
> > +#define PLL_CAL_L_VAL(p)       ((p)->offset + (p)->regs[PLL_OFF_CAL_L_VAL])
> >  #define PLL_ALPHA_VAL(p)       ((p)->offset + (p)->regs[PLL_OFF_ALPHA_VAL])
> >  #define PLL_ALPHA_VAL_U(p)     ((p)->offset + (p)->regs[PLL_OFF_ALPHA_VAL_U])
> >  
> > @@ -44,14 +45,17 @@
> >  # define PLL_VCO_MASK          0x3
> >  
> >  #define PLL_USER_CTL_U(p)      ((p)->offset + (p)->regs[PLL_OFF_USER_CTL_U])
> > +#define PLL_USER_CTL_U1(p)     ((p)->offset + (p)->regs[PLL_OFF_USER_CTL_U1])
> >  
> >  #define PLL_CONFIG_CTL(p)      ((p)->offset + (p)->regs[PLL_OFF_CONFIG_CTL])
> >  #define PLL_CONFIG_CTL_U(p)    ((p)->offset + (p)->regs[PLL_OFF_CONFIG_CTL_U])
> > +#define PLL_CONFIG_CTL_U1(p)   ((p)->offset + (p)->regs[PLL_OFF_CONFIG_CTL_U11])
> 
> This looks like a typo, U11 vs U1. So I don't think this has been
> compile tested....

Not sure how this has happened, I have this in my test br too. Will fix
this

> >  #define PLL_TEST_CTL(p)                ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL])
> >  #define PLL_TEST_CTL_U(p)      ((p)->offset + (p)->regs[PLL_OFF_TEST_CTL_U])
> >  #define PLL_STATUS(p)          ((p)->offset + (p)->regs[PLL_OFF_STATUS])
> >  #define PLL_OPMODE(p)          ((p)->offset + (p)->regs[PLL_OFF_OPMODE])
> >  #define PLL_FRAC(p)            ((p)->offset + (p)->regs[PLL_OFF_FRAC])
> > +#define PLL_CAL_VAL(p)         ((p)->offset + (p)->regs[PLL_OFF_CAL_VAL])
> >  
> >  const u8 clk_alpha_pll_regs[][PLL_OFF_MAX_REGS] = {
> >         [CLK_ALPHA_PLL_TYPE_DEFAULT] =  {
> >  const struct clk_ops clk_alpha_pll_ops = {
> >         .enable = clk_alpha_pll_enable,
> >         .disable = clk_alpha_pll_disable,
> > @@ -1053,6 +1210,77 @@ static unsigned long clk_alpha_pll_postdiv_fabia_recalc_rate(struct clk_hw *hw,
> [...]
> > +
> > +static int
> > +clk_trion_pll_postdiv_set_rate(struct clk_hw *hw, unsigned long rate,
> > +                              unsigned long parent_rate)
> > +{
> > +       struct clk_alpha_pll_postdiv *pll = to_clk_alpha_pll_postdiv(hw);
> > +       struct regmap *regmap = pll->clkr.regmap;
> > +       int i, val = 0, div, ret;
> > +
> > +       /*
> > +        * If the PLL is in FSM mode, then treat the set_rate callback
> > +        * as a no-operation.
> 
> And this is OK? Shouldn't we fail because we can't change to the rate
> that's desired?

Agreed, we should error out.
Also looking at other PLLs I see we check for FSM in clk enabled
and I guess we should do it there for this as well, will move

-- 
~Vinod