re: clk: samsung: add infrastructure to register cpu clocks

re: clk: samsung: add infrastructure to register cpu clocks

[Dan Carpenter]

Hello Thomas Abraham,

The patch ddeac8d968d4: "clk: samsung: add infrastructure to register
cpu clocks" from Apr 3, 2015, leads to the following static checker
warning:

	drivers/clk/samsung/clk-cpu.c:164 exynos_cpuclk_pre_rate_change()
	warn: test_bit() takes a bit number

drivers/clk/samsung/clk-cpu.c
   158          /*
   159           * For the selected PLL clock frequency, get the pre-defined divider
   160           * values. If the clock for sclk_hpm is not sourced from apll, then
   161           * the values for DIV_COPY and DIV_HPM dividers need not be set.
   162           */
   163          div0 = cfg_data->div0;
   164          if (test_bit(CLK_CPU_HAS_DIV1, &cpuclk->flags)) {
                             ^^^^^^^^^^^^^^^^
CLK_CPU_HAS_DIV1 is (1 << 0).  We sometimes used it correctly as a mask
and sometimes incorrectly (like here) as a bit number.

   165                  div1 = cfg_data->div1;
   166                  if (readl(base + E4210_SRC_CPU) & E4210_MUX_HPM_MASK)
   167                          div1 = readl(base + E4210_DIV_CPU1) &
   168                                  (E4210_DIV1_HPM_MASK | E4210_DIV1_COPY_MASK);
   169          }


regards,
dan carpenter

Re: clk: samsung: add infrastructure to register cpu clocks

[Krzysztof Kozlowski]

2015-06-26 18:54 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> Hello Thomas Abraham,
>
> The patch ddeac8d968d4: "clk: samsung: add infrastructure to register
> cpu clocks" from Apr 3, 2015, leads to the following static checker
> warning:
>
>         drivers/clk/samsung/clk-cpu.c:164 exynos_cpuclk_pre_rate_change()
>         warn: test_bit() takes a bit number
>
> drivers/clk/samsung/clk-cpu.c
>    158          /*
>    159           * For the selected PLL clock frequency, get the pre-defined divider
>    160           * values. If the clock for sclk_hpm is not sourced from apll, then
>    161           * the values for DIV_COPY and DIV_HPM dividers need not be set.
>    162           */
>    163          div0 = cfg_data->div0;
>    164          if (test_bit(CLK_CPU_HAS_DIV1, &cpuclk->flags)) {
>                              ^^^^^^^^^^^^^^^^
> CLK_CPU_HAS_DIV1 is (1 << 0).  We sometimes used it correctly as a mask
> and sometimes incorrectly (like here) as a bit number.
>
>    165                  div1 = cfg_data->div1;
>    166                  if (readl(base + E4210_SRC_CPU) & E4210_MUX_HPM_MASK)
>    167                          div1 = readl(base + E4210_DIV_CPU1) &
>    168                                  (E4210_DIV1_HPM_MASK | E4210_DIV1_COPY_MASK);
>    169          }

+Cc Bartlomiej

Although original author is Thomas but he abandoned this patch and
this was actually Bartlomiej's work mostly. Bart, care to look at it?

Best regards,
Krzysztof

Re: clk: samsung: add infrastructure to register cpu clocks

[Bartlomiej Zolnierkiewicz]

Hi,

On Friday, June 26, 2015 07:03:25 PM Krzysztof Kozlowski wrote:
> 2015-06-26 18:54 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> > Hello Thomas Abraham,
> >
> > The patch ddeac8d968d4: "clk: samsung: add infrastructure to register
> > cpu clocks" from Apr 3, 2015, leads to the following static checker
> > warning:
> >
> >         drivers/clk/samsung/clk-cpu.c:164 exynos_cpuclk_pre_rate_change()
> >         warn: test_bit() takes a bit number
> >
> > drivers/clk/samsung/clk-cpu.c
> >    158          /*
> >    159           * For the selected PLL clock frequency, get the pre-defined divider
> >    160           * values. If the clock for sclk_hpm is not sourced from apll, then
> >    161           * the values for DIV_COPY and DIV_HPM dividers need not be set.
> >    162           */
> >    163          div0 = cfg_data->div0;
> >    164          if (test_bit(CLK_CPU_HAS_DIV1, &cpuclk->flags)) {
> >                              ^^^^^^^^^^^^^^^^
> > CLK_CPU_HAS_DIV1 is (1 << 0).  We sometimes used it correctly as a mask
> > and sometimes incorrectly (like here) as a bit number.

Fortunately the current value of this flag is "1" so test_bit() still
works correctly.  I'll fix it up later anyway.  Thanks for catching it.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> >    165                  div1 = cfg_data->div1;
> >    166                  if (readl(base + E4210_SRC_CPU) & E4210_MUX_HPM_MASK)
> >    167                          div1 = readl(base + E4210_DIV_CPU1) &
> >    168                                  (E4210_DIV1_HPM_MASK | E4210_DIV1_COPY_MASK);
> >    169          }
> 
> +Cc Bartlomiej
> 
> Although original author is Thomas but he abandoned this patch and
> this was actually Bartlomiej's work mostly. Bart, care to look at it?
> 
> Best regards,
> Krzysztof

Re: clk: samsung: add infrastructure to register cpu clocks

[Dan Carpenter]

On Fri, Jun 26, 2015 at 12:20:35PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Friday, June 26, 2015 07:03:25 PM Krzysztof Kozlowski wrote:
> > 2015-06-26 18:54 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> > > Hello Thomas Abraham,
> > >
> > > The patch ddeac8d968d4: "clk: samsung: add infrastructure to register
> > > cpu clocks" from Apr 3, 2015, leads to the following static checker
> > > warning:
> > >
> > >         drivers/clk/samsung/clk-cpu.c:164 exynos_cpuclk_pre_rate_change()
> > >         warn: test_bit() takes a bit number
> > >
> > > drivers/clk/samsung/clk-cpu.c
> > >    158          /*
> > >    159           * For the selected PLL clock frequency, get the pre-defined divider
> > >    160           * values. If the clock for sclk_hpm is not sourced from apll, then
> > >    161           * the values for DIV_COPY and DIV_HPM dividers need not be set.
> > >    162           */
> > >    163          div0 = cfg_data->div0;
> > >    164          if (test_bit(CLK_CPU_HAS_DIV1, &cpuclk->flags)) {
> > >                              ^^^^^^^^^^^^^^^^
> > > CLK_CPU_HAS_DIV1 is (1 << 0).  We sometimes used it correctly as a mask
> > > and sometimes incorrectly (like here) as a bit number.
> 
> Fortunately the current value of this flag is "1" so test_bit() still
> works correctly.  I'll fix it up later anyway.  Thanks for catching it.

I don't think it works, although I may have misread.  We set BIT(0) in
exynos4_clk_init() but we test BIT(1) here.

regards,
dan carpenter

Re: clk: samsung: add infrastructure to register cpu clocks

[Bartlomiej Zolnierkiewicz]

On Friday, June 26, 2015 01:47:32 PM Dan Carpenter wrote:
> On Fri, Jun 26, 2015 at 12:20:35PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > 
> > Hi,
> > 
> > On Friday, June 26, 2015 07:03:25 PM Krzysztof Kozlowski wrote:
> > > 2015-06-26 18:54 GMT+09:00 Dan Carpenter <dan.carpenter@oracle.com>:
> > > > Hello Thomas Abraham,
> > > >
> > > > The patch ddeac8d968d4: "clk: samsung: add infrastructure to register
> > > > cpu clocks" from Apr 3, 2015, leads to the following static checker
> > > > warning:
> > > >
> > > >         drivers/clk/samsung/clk-cpu.c:164 exynos_cpuclk_pre_rate_change()
> > > >         warn: test_bit() takes a bit number
> > > >
> > > > drivers/clk/samsung/clk-cpu.c
> > > >    158          /*
> > > >    159           * For the selected PLL clock frequency, get the pre-defined divider
> > > >    160           * values. If the clock for sclk_hpm is not sourced from apll, then
> > > >    161           * the values for DIV_COPY and DIV_HPM dividers need not be set.
> > > >    162           */
> > > >    163          div0 = cfg_data->div0;
> > > >    164          if (test_bit(CLK_CPU_HAS_DIV1, &cpuclk->flags)) {
> > > >                              ^^^^^^^^^^^^^^^^
> > > > CLK_CPU_HAS_DIV1 is (1 << 0).  We sometimes used it correctly as a mask
> > > > and sometimes incorrectly (like here) as a bit number.
> > 
> > Fortunately the current value of this flag is "1" so test_bit() still
> > works correctly.  I'll fix it up later anyway.  Thanks for catching it.
> 
> I don't think it works, although I may have misread.  We set BIT(0) in
> exynos4_clk_init() but we test BIT(1) here.

Ah, indeed.  It works because we are always also setting BIT(1) in
exynos4_clk_init().

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics