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

sunyeal.hong
2024-07-22T22:27:30Z

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.