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

["sunyeal.hong" <sunyeal.hong@samsung.com>]

Hello Dan,

> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@linaro.org>
> Sent: Saturday, July 20, 2024 3:48 AM
> To: oe-kbuild@lists.linux.dev; Sunyeal Hong <sunyeal.hong@samsung.com>;
> Krzysztof Kozlowski <krzk@kernel.org>; Sylwester Nawrocki
> <s.nawrocki@samsung.com>; Chanwoo Choi <cw00.choi@samsung.com>; Alim
> Akhtar <alim.akhtar@samsung.com>; Michael Turquette
> <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; Rob Herring
> <robh@kernel.org>; Conor Dooley <conor+dt@kernel.org>
> Cc: lkp@intel.com; oe-kbuild-all@lists.linux.dev; linux-samsung-
> soc@vger.kernel.org; linux-clk@vger.kernel.org;
devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> Sunyeal Hong <sunyeal.hong@samsung.com>
> 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/202407200028.5AADGhmj-lkp@intel.com/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 <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202407200028.5AADGhmj-lkp@intel.com/
> 
> 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.