[v2,3/4] clk: samsung: clk-pll: Add support for pll_531x

Commit Message

Sunyeal Hong July 7, 2024, 11:13 p.m. UTC

pll531x PLL is used in Exynos Auto v920 SoC for shared pll.
pll531x: Integer/fractional PLL with mid frequency FVCO (800 to 3120 MHz)

PLL531x
FOUT = (MDIV + F/2^32-F[31]) * FIN/(PDIV x 2^SDIV)

Signed-off-by: Sunyeal Hong <[e-mail]>
---
 drivers/clk/samsung/clk-pll.c | 45 +++++++++++++++++++++++++++++++++++
 drivers/clk/samsung/clk-pll.h |  1 +
 2 files changed, 46 insertions(+)

Comments

Alim Akhtar July 8, 2024, 11:58 a.m. UTC | #1

Hello Sunyeal,

> -----Original Message-----
> From: Sunyeal Hong <[e-mail]>
> Sent: Monday, July 8, 2024 4:44 AM
> To: Krzysztof Kozlowski <[e-mail]>; Sylwester Nawrocki
> <[e-mail]>; Chanwoo Choi <[e-mail]>;
> Alim Akhtar <[e-mail]>; Michael Turquette
> <[e-mail]>; Stephen Boyd <[e-mail]>; Rob
> Herring <[e-mail]>; Conor Dooley <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; Sunyeal Hong <[e-mail]>
> Subject: [PATCH v2 3/4] clk: samsung: clk-pll: Add support for pll_531x
> 
> pll531x PLL is used in Exynos Auto v920 SoC for shared pll.
> pll531x: Integer/fractional PLL with mid frequency FVCO (800 to 3120 MHz)
> 
> PLL531x
> FOUT = (MDIV + F/2^32-F[31]) * FIN/(PDIV x 2^SDIV)
> 
Any reason for not mentioning equation for integer PLL? 

> Signed-off-by: Sunyeal Hong <[e-mail]>
> ---
Anyway, LGTM,

Reviewed-by: Alim Akhtar <[e-mail]>

>  drivers/clk/samsung/clk-pll.c | 45
> +++++++++++++++++++++++++++++++++++
>  drivers/clk/samsung/clk-pll.h |  1 +
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c index
> 4be879ab917e..b3bcef074ab7 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c
> @@ -1261,6 +1261,48 @@ static const struct clk_ops
> samsung_pll2650xx_clk_min_ops = {
>  	.recalc_rate = samsung_pll2650xx_recalc_rate,  };
> 
> +/*
> + * PLL531X Clock Type
> + */
> +/* Maximum lock time can be 500 * PDIV cycles */
> +#define PLL531X_LOCK_FACTOR		(500)
> +#define PLL531X_MDIV_MASK		(0x3FF)
> +#define PLL531X_PDIV_MASK		(0x3F)
> +#define PLL531X_SDIV_MASK		(0x7)
> +#define PLL531X_FDIV_MASK		(0xFFFF)
> +#define PLL531X_MDIV_SHIFT		(16)
> +#define PLL531X_PDIV_SHIFT		(8)
> +#define PLL531X_SDIV_SHIFT		(0)
> +
> +static unsigned long samsung_pll531x_recalc_rate(struct clk_hw *hw,
> +						 unsigned long parent_rate)
> +{
> +	struct samsung_clk_pll *pll = to_clk_pll(hw);
> +	u32 mdiv, pdiv, sdiv, pll_con0, pll_con8;
> +	s32 fdiv;
> +	u64 fout = parent_rate;
> +
> +	pll_con0 = readl_relaxed(pll->con_reg);
> +	pll_con8 = readl_relaxed(pll->con_reg + 20);
> +	mdiv = (pll_con0 >> PLL531X_MDIV_SHIFT) & PLL531X_MDIV_MASK;
> +	pdiv = (pll_con0 >> PLL531X_PDIV_SHIFT) & PLL531X_PDIV_MASK;
> +	sdiv = (pll_con0 >> PLL531X_SDIV_SHIFT) & PLL531X_SDIV_MASK;
> +	fdiv = (s32)(pll_con8 & PLL531X_FDIV_MASK);
> +
> +	if (fdiv >> 31)
> +		mdiv--;
> +
> +	fout *= ((u64)mdiv << 24) + (fdiv >> 8);
> +	do_div(fout, (pdiv << sdiv));
> +	fout >>= 24;
> +
> +	return (unsigned long)fout;
> +}
> +
> +static const struct clk_ops samsung_pll531x_clk_ops = {
> +	.recalc_rate = samsung_pll531x_recalc_rate, };
> +
>  static void __init _samsung_clk_register_pll(struct samsung_clk_provider
> *ctx,
>  				const struct samsung_pll_clock *pll_clk)  {
> @@ -1394,6 +1436,9 @@ static void __init _samsung_clk_register_pll(struct
> samsung_clk_provider *ctx,
>  		else
>  			init.ops = &samsung_pll2650xx_clk_ops;
>  		break;
> +	case pll_531x:
> +		init.ops = &samsung_pll531x_clk_ops;
> +		break;
>  	default:
>  		pr_warn("%s: Unknown pll type for pll clk %s\n",
>  			__func__, pll_clk->name);
> diff --git a/drivers/clk/samsung/clk-pll.h b/drivers/clk/samsung/clk-pll.h
> index ffd3d52c0dec..ce9d6f21f993 100644
> --- a/drivers/clk/samsung/clk-pll.h
> +++ b/drivers/clk/samsung/clk-pll.h
> @@ -41,6 +41,7 @@ enum samsung_pll_type {
>  	pll_0516x,
>  	pll_0517x,
>  	pll_0518x,
> +	pll_531x,
>  };
> 
>  #define PLL_RATE(_fin, _m, _p, _s, _k, _ks) \
> --
> 2.45.2

Sunyeal Hong July 10, 2024, 2:20 a.m. UTC | #2

Hello Alim,

> -----Original Message-----
> From: Alim Akhtar <[e-mail]>
> Sent: Monday, July 8, 2024 8:58 PM
> To: 'Sunyeal Hong' <[e-mail]>; 'Krzysztof Kozlowski'
> <[e-mail]>; 'Sylwester Nawrocki' <[e-mail]>; 'Chanwoo
> Choi' <[e-mail]>; 'Michael Turquette'
> <[e-mail]>; 'Stephen Boyd' <[e-mail]>; 'Rob
> Herring' <[e-mail]>; 'Conor Dooley' <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: RE: [PATCH v2 3/4] clk: samsung: clk-pll: Add support for
> pll_531x
> 
> Hello Sunyeal,
> 
> > -----Original Message-----
> > From: Sunyeal Hong <[e-mail]>
> > Sent: Monday, July 8, 2024 4:44 AM
> > To: Krzysztof Kozlowski <[e-mail]>; Sylwester Nawrocki
> > <[e-mail]>; Chanwoo Choi <[e-mail]>; Alim
> > Akhtar <[e-mail]>; Michael Turquette
> > <[e-mail]>; Stephen Boyd <[e-mail]>; Rob
> > Herring <[e-mail]>; Conor Dooley <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > linux- [e-mail]; Sunyeal Hong <[e-mail]>
> > Subject: [PATCH v2 3/4] clk: samsung: clk-pll: Add support for
> > pll_531x
> >
> > pll531x PLL is used in Exynos Auto v920 SoC for shared pll.
> > pll531x: Integer/fractional PLL with mid frequency FVCO (800 to 3120
> > MHz)
> >
> > PLL531x
> > FOUT = (MDIV + F/2^32-F[31]) * FIN/(PDIV x 2^SDIV)
> >
> Any reason for not mentioning equation for integer PLL?
> 
If the F value is 0, it operates as an integer PLL. 
> > Signed-off-by: Sunyeal Hong <[e-mail]>
> > ---
> Anyway, LGTM,
> 
> Reviewed-by: Alim Akhtar <[e-mail]>
> 
> >  drivers/clk/samsung/clk-pll.c | 45
> > +++++++++++++++++++++++++++++++++++
> >  drivers/clk/samsung/clk-pll.h |  1 +
> >  2 files changed, 46 insertions(+)
> >
> > diff --git a/drivers/clk/samsung/clk-pll.c
> > b/drivers/clk/samsung/clk-pll.c index
> > 4be879ab917e..b3bcef074ab7 100644
> > --- a/drivers/clk/samsung/clk-pll.c
> > +++ b/drivers/clk/samsung/clk-pll.c
> > @@ -1261,6 +1261,48 @@ static const struct clk_ops
> > samsung_pll2650xx_clk_min_ops = {
> >  	.recalc_rate = samsung_pll2650xx_recalc_rate,  };
> >
> > +/*
> > + * PLL531X Clock Type
> > + */
> > +/* Maximum lock time can be 500 * PDIV cycles */
> > +#define PLL531X_LOCK_FACTOR		(500)
> > +#define PLL531X_MDIV_MASK		(0x3FF)
> > +#define PLL531X_PDIV_MASK		(0x3F)
> > +#define PLL531X_SDIV_MASK		(0x7)
> > +#define PLL531X_FDIV_MASK		(0xFFFF)
> > +#define PLL531X_MDIV_SHIFT		(16)
> > +#define PLL531X_PDIV_SHIFT		(8)
> > +#define PLL531X_SDIV_SHIFT		(0)
> > +
> > +static unsigned long samsung_pll531x_recalc_rate(struct clk_hw *hw,
> > +						 unsigned long parent_rate)
> > +{
> > +	struct samsung_clk_pll *pll = to_clk_pll(hw);
> > +	u32 mdiv, pdiv, sdiv, pll_con0, pll_con8;
> > +	s32 fdiv;
> > +	u64 fout = parent_rate;
> > +
> > +	pll_con0 = readl_relaxed(pll->con_reg);
> > +	pll_con8 = readl_relaxed(pll->con_reg + 20);
> > +	mdiv = (pll_con0 >> PLL531X_MDIV_SHIFT) & PLL531X_MDIV_MASK;
> > +	pdiv = (pll_con0 >> PLL531X_PDIV_SHIFT) & PLL531X_PDIV_MASK;
> > +	sdiv = (pll_con0 >> PLL531X_SDIV_SHIFT) & PLL531X_SDIV_MASK;
> > +	fdiv = (s32)(pll_con8 & PLL531X_FDIV_MASK);
> > +
> > +	if (fdiv >> 31)
> > +		mdiv--;
> > +
> > +	fout *= ((u64)mdiv << 24) + (fdiv >> 8);
> > +	do_div(fout, (pdiv << sdiv));
> > +	fout >>= 24;
> > +
> > +	return (unsigned long)fout;
> > +}
> > +
> > +static const struct clk_ops samsung_pll531x_clk_ops = {
> > +	.recalc_rate = samsung_pll531x_recalc_rate, };
> > +
> >  static void __init _samsung_clk_register_pll(struct
> > samsung_clk_provider *ctx,
> >  				const struct samsung_pll_clock *pll_clk)  { @@ -
> 1394,6 +1436,9 @@
> > static void __init _samsung_clk_register_pll(struct
> > samsung_clk_provider *ctx,
> >  		else
> >  			init.ops = &samsung_pll2650xx_clk_ops;
> >  		break;
> > +	case pll_531x:
> > +		init.ops = &samsung_pll531x_clk_ops;
> > +		break;
> >  	default:
> >  		pr_warn("%s: Unknown pll type for pll clk %s\n",
> >  			__func__, pll_clk->name);
> > diff --git a/drivers/clk/samsung/clk-pll.h
> > b/drivers/clk/samsung/clk-pll.h index ffd3d52c0dec..ce9d6f21f993
> > 100644
> > --- a/drivers/clk/samsung/clk-pll.h
> > +++ b/drivers/clk/samsung/clk-pll.h
> > @@ -41,6 +41,7 @@ enum samsung_pll_type {
> >  	pll_0516x,
> >  	pll_0517x,
> >  	pll_0518x,
> > +	pll_531x,
> >  };
> >
> >  #define PLL_RATE(_fin, _m, _p, _s, _k, _ks) \
> > --
> > 2.45.2
>

Alim Akhtar July 10, 2024, 2:34 a.m. UTC | #3

Hello Sunyeal,

> -----Original Message-----
> From: sunyeal.hong <[e-mail]>
> Sent: Wednesday, July 10, 2024 7:50 AM
> To: 'Alim Akhtar' <[e-mail]>; 'Krzysztof Kozlowski'
> <[e-mail]>; 'Sylwester Nawrocki' <[e-mail]>;
> 'Chanwoo Choi' <[e-mail]>; 'Michael Turquette'
> <[e-mail]>; 'Stephen Boyd' <[e-mail]>; 'Rob
> Herring' <[e-mail]>; 'Conor Dooley' <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: RE: [PATCH v2 3/4] clk: samsung: clk-pll: Add support for pll_531x
> 
> Hello Alim,
> 
> > -----Original Message-----
> > From: Alim Akhtar <[e-mail]>
> > Sent: Monday, July 8, 2024 8:58 PM
> > To: 'Sunyeal Hong' <[e-mail]>; 'Krzysztof Kozlowski'
> > <[e-mail]>; 'Sylwester Nawrocki' <[e-mail]>;
> > 'Chanwoo Choi' <[e-mail]>; 'Michael Turquette'
> > <[e-mail]>; 'Stephen Boyd' <[e-mail]>; 'Rob
> > Herring' <[e-mail]>; 'Conor Dooley' <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > linux- [e-mail]
> > Subject: RE: [PATCH v2 3/4] clk: samsung: clk-pll: Add support for
> > pll_531x
> >
> > Hello Sunyeal,
> >
> > > -----Original Message-----
> > > From: Sunyeal Hong <[e-mail]>
> > > Sent: Monday, July 8, 2024 4:44 AM
> > > To: Krzysztof Kozlowski <[e-mail]>; Sylwester Nawrocki
> > > <[e-mail]>; Chanwoo Choi
> <[e-mail]>; Alim
> > > Akhtar <[e-mail]>; Michael Turquette
> > > <[e-mail]>; Stephen Boyd <[e-mail]>; Rob
> > > Herring <[e-mail]>; Conor Dooley <[e-mail]>
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > linux- [e-mail]; Sunyeal Hong
> > > <[e-mail]>
> > > Subject: [PATCH v2 3/4] clk: samsung: clk-pll: Add support for
> > > pll_531x
> > >
> > > pll531x PLL is used in Exynos Auto v920 SoC for shared pll.
> > > pll531x: Integer/fractional PLL with mid frequency FVCO (800 to 3120
> > > MHz)
> > >
> > > PLL531x
> > > FOUT = (MDIV + F/2^32-F[31]) * FIN/(PDIV x 2^SDIV)
> > >
> > Any reason for not mentioning equation for integer PLL?
> >
> If the F value is 0, it operates as an integer PLL.
Thanks for clarification, it is good to mention the same in the commit message. 

[snip]
> > > --
> > > 2.45.2
> >
>

Sunyeal Hong July 10, 2024, 7 a.m. UTC | #4

Hello Alim,

> -----Original Message-----
> From: Alim Akhtar <[e-mail]>
> Sent: Wednesday, July 10, 2024 11:35 AM
> To: 'sunyeal.hong' <[e-mail]>; 'Krzysztof Kozlowski'
> <[e-mail]>; 'Sylwester Nawrocki' <[e-mail]>; 'Chanwoo
> Choi' <[e-mail]>; 'Michael Turquette'
> <[e-mail]>; 'Stephen Boyd' <[e-mail]>; 'Rob
> Herring' <[e-mail]>; 'Conor Dooley' <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: RE: [PATCH v2 3/4] clk: samsung: clk-pll: Add support for
> pll_531x
> 
> Hello Sunyeal,
> 
> > -----Original Message-----
> > From: sunyeal.hong <[e-mail]>
> > Sent: Wednesday, July 10, 2024 7:50 AM
> > To: 'Alim Akhtar' <[e-mail]>; 'Krzysztof Kozlowski'
> > <[e-mail]>; 'Sylwester Nawrocki' <[e-mail]>;
> > 'Chanwoo Choi' <[e-mail]>; 'Michael Turquette'
> > <[e-mail]>; 'Stephen Boyd' <[e-mail]>; 'Rob
> > Herring' <[e-mail]>; 'Conor Dooley' <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > linux- [e-mail]
> > Subject: RE: [PATCH v2 3/4] clk: samsung: clk-pll: Add support for
> > pll_531x
> >
> > Hello Alim,
> >
> > > -----Original Message-----
> > > From: Alim Akhtar <[e-mail]>
> > > Sent: Monday, July 8, 2024 8:58 PM
> > > To: 'Sunyeal Hong' <[e-mail]>; 'Krzysztof Kozlowski'
> > > <[e-mail]>; 'Sylwester Nawrocki' <[e-mail]>;
> > > 'Chanwoo Choi' <[e-mail]>; 'Michael Turquette'
> > > <[e-mail]>; 'Stephen Boyd' <[e-mail]>; 'Rob
> > > Herring' <[e-mail]>; 'Conor Dooley' <[e-mail]>
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > linux- [e-mail]
> > > Subject: RE: [PATCH v2 3/4] clk: samsung: clk-pll: Add support for
> > > pll_531x
> > >
> > > Hello Sunyeal,
> > >
> > > > -----Original Message-----
> > > > From: Sunyeal Hong <[e-mail]>
> > > > Sent: Monday, July 8, 2024 4:44 AM
> > > > To: Krzysztof Kozlowski <[e-mail]>; Sylwester Nawrocki
> > > > <[e-mail]>; Chanwoo Choi
> > <[e-mail]>; Alim
> > > > Akhtar <[e-mail]>; Michael Turquette
> > > > <[e-mail]>; Stephen Boyd <[e-mail]>; Rob
> > > > Herring <[e-mail]>; Conor Dooley <[e-mail]>
> > > > Cc: [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail];
> > > > linux- [e-mail]; Sunyeal Hong
> > > > <[e-mail]>
> > > > Subject: [PATCH v2 3/4] clk: samsung: clk-pll: Add support for
> > > > pll_531x
> > > >
> > > > pll531x PLL is used in Exynos Auto v920 SoC for shared pll.
> > > > pll531x: Integer/fractional PLL with mid frequency FVCO (800 to
> > > > 3120
> > > > MHz)
> > > >
> > > > PLL531x
> > > > FOUT = (MDIV + F/2^32-F[31]) * FIN/(PDIV x 2^SDIV)
> > > >
> > > Any reason for not mentioning equation for integer PLL?
> > >
> > If the F value is 0, it operates as an integer PLL.
> Thanks for clarification, it is good to mention the same in the commit
> message.
> 
Okay. I will update comment for integer PLL description.
> [snip]
> > > > --
> > > > 2.45.2
> > >
> >
> 

Thanks,
Sunyeal Hong

Dan Carpenter July 19, 2024, 6:48 p.m. UTC | #5

Hi Sunyeal,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sunyeal-Hong/dt-bindings-clock-add-Exynos-Auto-v920-SoC-CMU-bindings/20240708-072150
base:   https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux.git for-next
patch link:    https://lore.kernel.org/r/20240707231331.3433340-4-sunyeal.hong%40samsung.com
patch subject: [PATCH v2 3/4] clk: samsung: clk-pll: Add support for pll_531x
config: arc-randconfig-r071-20240719 (https://download.01.org/0day-ci/archive/20240720/[e-mail]/config)
compiler: arc-elf-gcc (GCC) 13.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[e-mail]>
| Reported-by: Dan Carpenter <[e-mail]>
| Closes: https://lore.kernel.org/r/[e-mail]/

smatch warnings:
drivers/clk/samsung/clk-pll.c:1292 samsung_pll531x_recalc_rate() warn: mask and shift to zero: expr='fdiv >> 31'

vim +1292 drivers/clk/samsung/clk-pll.c

5c788df7a25de7 Sunyeal Hong 2024-07-08  1277  static unsigned long samsung_pll531x_recalc_rate(struct clk_hw *hw,
5c788df7a25de7 Sunyeal Hong 2024-07-08  1278  						 unsigned long parent_rate)
5c788df7a25de7 Sunyeal Hong 2024-07-08  1279  {
5c788df7a25de7 Sunyeal Hong 2024-07-08  1280  	struct samsung_clk_pll *pll = to_clk_pll(hw);
5c788df7a25de7 Sunyeal Hong 2024-07-08  1281  	u32 mdiv, pdiv, sdiv, pll_con0, pll_con8;
5c788df7a25de7 Sunyeal Hong 2024-07-08  1282  	s32 fdiv;
5c788df7a25de7 Sunyeal Hong 2024-07-08  1283  	u64 fout = parent_rate;
5c788df7a25de7 Sunyeal Hong 2024-07-08  1284  
5c788df7a25de7 Sunyeal Hong 2024-07-08  1285  	pll_con0 = readl_relaxed(pll->con_reg);
5c788df7a25de7 Sunyeal Hong 2024-07-08  1286  	pll_con8 = readl_relaxed(pll->con_reg + 20);
5c788df7a25de7 Sunyeal Hong 2024-07-08  1287  	mdiv = (pll_con0 >> PLL531X_MDIV_SHIFT) & PLL531X_MDIV_MASK;
5c788df7a25de7 Sunyeal Hong 2024-07-08  1288  	pdiv = (pll_con0 >> PLL531X_PDIV_SHIFT) & PLL531X_PDIV_MASK;
5c788df7a25de7 Sunyeal Hong 2024-07-08  1289  	sdiv = (pll_con0 >> PLL531X_SDIV_SHIFT) & PLL531X_SDIV_MASK;
5c788df7a25de7 Sunyeal Hong 2024-07-08  1290  	fdiv = (s32)(pll_con8 & PLL531X_FDIV_MASK);

PLL531X_FDIV_MASK is 0xffff.  Was this supposed to be a cast to s16
instead of s32?  Why is fdiv signed?  Shifting negative values is
undefined in C.

5c788df7a25de7 Sunyeal Hong 2024-07-08  1291  
5c788df7a25de7 Sunyeal Hong 2024-07-08 @1292  	if (fdiv >> 31)

It's really unclear what's happening here.  If I had to guess, I'd say
that this was testing to see if fdiv was negative.

5c788df7a25de7 Sunyeal Hong 2024-07-08  1293  		mdiv--;
5c788df7a25de7 Sunyeal Hong 2024-07-08  1294  
5c788df7a25de7 Sunyeal Hong 2024-07-08  1295  	fout *= ((u64)mdiv << 24) + (fdiv >> 8);
                                                                             ^^^^^^^^^
More shifting.

5c788df7a25de7 Sunyeal Hong 2024-07-08  1296  	do_div(fout, (pdiv << sdiv));
5c788df7a25de7 Sunyeal Hong 2024-07-08  1297  	fout >>= 24;
5c788df7a25de7 Sunyeal Hong 2024-07-08  1298  
5c788df7a25de7 Sunyeal Hong 2024-07-08  1299  	return (unsigned long)fout;
5c788df7a25de7 Sunyeal Hong 2024-07-08  1300  }

Sunyeal Hong July 22, 2024, 10:27 p.m. UTC | #6

Hello Dan,

> -----Original Message-----
> From: Dan Carpenter <[e-mail]>
> Sent: Saturday, July 20, 2024 3:48 AM
> To: [e-mail]; Sunyeal Hong <[e-mail]>;
> Krzysztof Kozlowski <[e-mail]>; Sylwester Nawrocki
> <[e-mail]>; Chanwoo Choi <[e-mail]>; Alim
> Akhtar <[e-mail]>; Michael Turquette
> <[e-mail]>; Stephen Boyd <[e-mail]>; Rob Herring
> <[e-mail]>; Conor Dooley <[e-mail]>
> Cc: [e-mail]; [e-mail]; linux-samsung-
> [e-mail]; [e-mail];
[e-mail];
> [e-mail]; [e-mail];
> Sunyeal Hong <[e-mail]>
> Subject: Re: [PATCH v2 3/4] clk: samsung: clk-pll: Add support for
> pll_531x
> 
> Hi Sunyeal,
> 
> kernel test robot noticed the following build warnings:
> 
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://protect2.fireeye.com/v1/url?k=ccd9a91d-93426779-ccd82252-
> 000babda0201-b5083e850ec85e2b&q=1&e=dc8f0621-d4ce-45e5-8254-
> 9a5da18037d5&u=https%3A%2F%2Fgithub.com%2Fintel-lab-
> lkp%2Flinux%2Fcommits%2FSunyeal-Hong%2Fdt-bindings-clock-add-Exynos-Auto-
> v920-SoC-CMU-bindings%2F20240708-072150
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux.git
> for-next
> patch link:    https://lore.kernel.org/r/20240707231331.3433340-4-
> sunyeal.hong%40samsung.com
> patch subject: [PATCH v2 3/4] clk: samsung: clk-pll: Add support for
> pll_531x
> config: arc-randconfig-r071-20240719 (https://download.01.org/0day-
> ci/archive/20240720/[e-mail]/config)
> compiler: arc-elf-gcc (GCC) 13.2.0
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new
> version of the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <[e-mail]>
> | Reported-by: Dan Carpenter <[e-mail]>
> | Closes: https://lore.kernel.org/r/[e-mail]/
> 
> smatch warnings:
> drivers/clk/samsung/clk-pll.c:1292 samsung_pll531x_recalc_rate() warn:
> mask and shift to zero: expr='fdiv >> 31'
> 
> vim +1292 drivers/clk/samsung/clk-pll.c
> 
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1277  static unsigned long
> samsung_pll531x_recalc_rate(struct clk_hw *hw,
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1278
> 	 unsigned long parent_rate)
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1279  {
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1280  	struct
samsung_clk_pll *pll
> = to_clk_pll(hw);
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1281  	u32 mdiv, pdiv,
sdiv,
> pll_con0, pll_con8;
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1282  	s32 fdiv;
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1283  	u64 fout =
parent_rate;
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1284
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1285  	pll_con0 =
> readl_relaxed(pll->con_reg);
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1286  	pll_con8 =
> readl_relaxed(pll->con_reg + 20);
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1287  	mdiv = (pll_con0 >>
> PLL531X_MDIV_SHIFT) & PLL531X_MDIV_MASK;
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1288  	pdiv = (pll_con0 >>
> PLL531X_PDIV_SHIFT) & PLL531X_PDIV_MASK;
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1289  	sdiv = (pll_con0 >>
> PLL531X_SDIV_SHIFT) & PLL531X_SDIV_MASK;
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1290  	fdiv =
(s32)(pll_con8 &
> PLL531X_FDIV_MASK);
> 
> PLL531X_FDIV_MASK is 0xffff.  Was this supposed to be a cast to s16
> instead of s32?  Why is fdiv signed?  Shifting negative values is
undefined
> in C.
> 
As you reviewed, I will change fdiv to u32 type and modify it to use a mask
of 0xffffffff.
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1291
> 5c788df7a25de7 Sunyeal Hong 2024-07-08 @1292  	if (fdiv >> 31)
> 
> It's really unclear what's happening here.  If I had to guess, I'd say
> that this was testing to see if fdiv was negative.
> 
the bit of fdiv 31 is a bit that can check negative numbers. In this case,
the mdiv is first subtracting 1. Please refer to the formula of commit
description.
FOUT = (MDIV + F/2^32-F[31]) x FIN/(PDIV x 2^SDIV) for fractional PLL
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1293  		mdiv--;
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1294
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1295  	fout *= ((u64)mdiv
<< 24) +
> (fdiv >> 8);
>
^^^^^^^^^ More
> shifting.
> 
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1296  	do_div(fout, (pdiv
<<
> sdiv));
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1297  	fout >>= 24;
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1298
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1299  	return (unsigned
long)fout;
> 5c788df7a25de7 Sunyeal Hong 2024-07-08  1300  }
> 
> --
> 0-DAY CI Kernel Test Service
> https://protect2.fireeye.com/v1/url?k=8c19bb62-d3827506-8c18302d-
> 000babda0201-2f20ef1ee86cc8ae&q=1&e=dc8f0621-d4ce-45e5-8254-
> 9a5da18037d5&u=https%3A%2F%2Fgithub.com%2Fintel%2Flkp-tests%2Fwiki
> 

I will fix the patch and update again.

Thanks,
Sunyeal Hong.

Patch

diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index 4be879ab917e..b3bcef074ab7 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -1261,6 +1261,48 @@  static const struct clk_ops samsung_pll2650xx_clk_min_ops = {
 	.recalc_rate = samsung_pll2650xx_recalc_rate,
 };
 
+/*
+ * PLL531X Clock Type
+ */
+/* Maximum lock time can be 500 * PDIV cycles */
+#define PLL531X_LOCK_FACTOR		(500)
+#define PLL531X_MDIV_MASK		(0x3FF)
+#define PLL531X_PDIV_MASK		(0x3F)
+#define PLL531X_SDIV_MASK		(0x7)
+#define PLL531X_FDIV_MASK		(0xFFFF)
+#define PLL531X_MDIV_SHIFT		(16)
+#define PLL531X_PDIV_SHIFT		(8)
+#define PLL531X_SDIV_SHIFT		(0)
+
+static unsigned long samsung_pll531x_recalc_rate(struct clk_hw *hw,
+						 unsigned long parent_rate)
+{
+	struct samsung_clk_pll *pll = to_clk_pll(hw);
+	u32 mdiv, pdiv, sdiv, pll_con0, pll_con8;
+	s32 fdiv;
+	u64 fout = parent_rate;
+
+	pll_con0 = readl_relaxed(pll->con_reg);
+	pll_con8 = readl_relaxed(pll->con_reg + 20);
+	mdiv = (pll_con0 >> PLL531X_MDIV_SHIFT) & PLL531X_MDIV_MASK;
+	pdiv = (pll_con0 >> PLL531X_PDIV_SHIFT) & PLL531X_PDIV_MASK;
+	sdiv = (pll_con0 >> PLL531X_SDIV_SHIFT) & PLL531X_SDIV_MASK;
+	fdiv = (s32)(pll_con8 & PLL531X_FDIV_MASK);
+
+	if (fdiv >> 31)
+		mdiv--;
+
+	fout *= ((u64)mdiv << 24) + (fdiv >> 8);
+	do_div(fout, (pdiv << sdiv));
+	fout >>= 24;
+
+	return (unsigned long)fout;
+}
+
+static const struct clk_ops samsung_pll531x_clk_ops = {
+	.recalc_rate = samsung_pll531x_recalc_rate,
+};
+
 static void __init _samsung_clk_register_pll(struct samsung_clk_provider *ctx,
 				const struct samsung_pll_clock *pll_clk)
 {
@@ -1394,6 +1436,9 @@  static void __init _samsung_clk_register_pll(struct samsung_clk_provider *ctx,
 		else
 			init.ops = &samsung_pll2650xx_clk_ops;
 		break;
+	case pll_531x:
+		init.ops = &samsung_pll531x_clk_ops;
+		break;
 	default:
 		pr_warn("%s: Unknown pll type for pll clk %s\n",
 			__func__, pll_clk->name);
diff --git a/drivers/clk/samsung/clk-pll.h b/drivers/clk/samsung/clk-pll.h
index ffd3d52c0dec..ce9d6f21f993 100644
--- a/drivers/clk/samsung/clk-pll.h
+++ b/drivers/clk/samsung/clk-pll.h
@@ -41,6 +41,7 @@  enum samsung_pll_type {
 	pll_0516x,
 	pll_0517x,
 	pll_0518x,
+	pll_531x,
 };
 
 #define PLL_RATE(_fin, _m, _p, _s, _k, _ks) \