Re: [PATCH v2 6/7] clk: samsung: clk-pll: Add support for pll_1418x

[Krzysztof Kozlowski <krzk@kernel.org>]

On 06/08/2024 14:11, David Virag wrote:
> pll1418x is used in Exynos7885 SoC for USB PHY clock.
> Operation-wise it is very similar to pll0822x, except that MDIV is only
> 9 bits wide instead of 10, and we use the CON1 register in the PLL
> macro's "con" parameter instead of CON3 like this:
> 
> 	PLL(pll_1418x, CLK_FOUT_USB_PLL, "fout_usb_pll", "oscclk",
> 	    PLL_LOCKTIME_PLL_USB, PLL_CON0_PLL_USB,
> 	    pll_usb_rate_table),
> 
> Technically the PLL should work fine with pll0822x code if the PLL
> tables are correct, but it's more "correct" to actually update the mask.
> 
> Signed-off-by: David Virag <virag.david003@gmail.com>
> ---
>  drivers/clk/samsung/clk-pll.c | 20 ++++++++++++++++----
>  drivers/clk/samsung/clk-pll.h |  1 +
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
> index 4be879ab917e..c61a2810737c 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c
> @@ -430,6 +430,9 @@ static const struct clk_ops samsung_pll36xx_clk_min_ops = {
>  #define PLL0822X_LOCK_STAT_SHIFT	(29)
>  #define PLL0822X_ENABLE_SHIFT		(31)
>  
> +/* PLL1418x is similar to PLL0822x, except that MDIV is one bit smaller */
> +#define PLL1418X_MDIV_MASK		(0x1FF)
> +
>  static unsigned long samsung_pll0822x_recalc_rate(struct clk_hw *hw,
>  						  unsigned long parent_rate)
>  {
> @@ -438,7 +441,10 @@ static unsigned long samsung_pll0822x_recalc_rate(struct clk_hw *hw,
>  	u64 fvco = parent_rate;
>  
>  	pll_con3 = readl_relaxed(pll->con_reg);
> -	mdiv = (pll_con3 >> PLL0822X_MDIV_SHIFT) & PLL0822X_MDIV_MASK;
> +	if (pll->type != pll_1418x)
> +		mdiv = (pll_con3 >> PLL0822X_MDIV_SHIFT) & PLL0822X_MDIV_MASK;
> +	else
> +		mdiv = (pll_con3 >> PLL0822X_MDIV_SHIFT) & PLL1418X_MDIV_MASK;
>  	pdiv = (pll_con3 >> PLL0822X_PDIV_SHIFT) & PLL0822X_PDIV_MASK;
>  	sdiv = (pll_con3 >> PLL0822X_SDIV_SHIFT) & PLL0822X_SDIV_MASK;
>  
> @@ -468,9 +474,14 @@ static int samsung_pll0822x_set_rate(struct clk_hw *hw, unsigned long drate,
>  
>  	/* Change PLL PMS values */
>  	pll_con3 = readl_relaxed(pll->con_reg);
> -	pll_con3 &= ~((PLL0822X_MDIV_MASK << PLL0822X_MDIV_SHIFT) |
> -			(PLL0822X_PDIV_MASK << PLL0822X_PDIV_SHIFT) |
> -			(PLL0822X_SDIV_MASK << PLL0822X_SDIV_SHIFT));
> +	if (pll->type != pll_1418x)
> +		pll_con3 &= ~((PLL0822X_MDIV_MASK << PLL0822X_MDIV_SHIFT) |
> +				(PLL0822X_PDIV_MASK << PLL0822X_PDIV_SHIFT) |
> +				(PLL0822X_SDIV_MASK << PLL0822X_SDIV_SHIFT));

I think this part could be simpler. If I read it correctly, only mdiv
mask is different, so create local variable mdiv_mask and set it in
if-else block accordingly to the type. Then here you have
non-conditional code:
pll_con3 &= ~((mdiv_mask << PLL0822X_MDIV_SHIFT) |



Best regards,
Krzysztof