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

From: Stephen Boyd <sboyd@kernel.org>
To: Vinod Koul <vkoul@kernel.org>
Cc: linux-arm-msm@vger.kernel.org,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Deepak Katragadda <dkatraga@codeaurora.org>,
	Andy Gross <agross@kernel.org>,
	David Brown <david.brown@linaro.org>,
	Michael Turquette <mturquette@baylibre.com>,
	linux-clk@vger.kernel.org, Taniya Das <tdas@codeaurora.org>,
	Vinod Koul <vkoul@kernel.org>
Subject: Re: [PATCH v3 2/3] clk: qcom: clk-alpha-pll: Add support for Trion PLLs
Date: Thu, 27 Jun 2019 14:50:26 -0700	[thread overview]
Message-ID: <20190627215027.757982063F@mail.kernel.org> (raw)
In-Reply-To: <20190625063140.17106-3-vkoul@kernel.org>

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....

>  #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?

> +        */
> +       ret = regmap_read(regmap, PLL_MODE(pll), &val);
> +       if (ret)
> +               return ret;
> +
> +       if (val & PLL_VOTE_FSM_ENA)
> +               return 0;
> +
> +       div = DIV_ROUND_UP_ULL(parent_rate, rate);
> +       for (i = 0; i < pll->num_post_div; i++) {
> +               if (pll->post_div_table[i].div == div) {
> +                       val = pll->post_div_table[i].val;
> +                       break;
> +               }
> +       }
> +
> +       return regmap_update_bits(regmap, PLL_USER_CTL(pll),
> +                                 PLL_POST_DIV_MASK(pll) << PLL_POST_DIV_SHIFT,
> +                                 val << PLL_POST_DIV_SHIFT);
> +}