[U-Boot] [PATCH 9/9] arch/arm/include/asm/arch-omap5/clocks.h: Fix GCC 4.2 warnings

[R, Sricharan]

Hi Tom,
  I agree on this. This was a bug.
  My gcc was on version 4.4.4 and did not see any warnings here.

Thanks,
 Sricharan

On Mon, Dec 5, 2011 at 8:38 PM, Tom Rini <trini at ti.com> wrote:

> On 12/04/2011 06:59 AM, Anatolij Gustschin wrote:
> > On Sun, 4 Dec 2011 12:30:40 +0100
> > Marek Vasut <marek.vasut at gmail.com> wrote:
> >
> >>> Fix:
> >>> clocks.c: In function 'setup_post_dividers':
> >>> clocks.c:175: warning: comparison is always true due to limited range
> of
> >>> data type
> >>> clocks.c:177: warning: comparison is always true due to limited range
> of
> >>> data type
> >>> clocks.c:179: warning: comparison is always true due to limited range
> of
> >>> data type
> >>> clocks.c:181: warning: comparison is always true due to limited range
> of
> >>> data type
> >>> clocks.c:183: warning: comparison is always true due to limited range
> of
> >>> data type
> >>> clocks.c:185: warning: comparison is always true due to limited range
> of
> >>> data type
> >>> clocks.c:187: warning: comparison is always true due to limited range
> of
> >>> data type
> >>> clocks.c:189: warning: comparison is always true due to limited range
> of
> >>> data type
> >>>
> >>> Signed-off-by: Anatolij Gustschin <agust at denx.de>
> >>> Cc: sricharan <r.sricharan at ti.com>
> >>> Cc: Tom Rini <trini at ti.com>
> >>> ---
> >>> Some notes:
> >>>
> >>>  - GCC v4.5.1 didn't warn here
> >>>  - GCC v4.6.1 seems to have a bug and can't compile this code:
> >>>    clocks.c: In function 'enable_non_essential_clocks':
> >>>    clocks.c:349:13: internal compiler error: in decode_addr_const, at
> >>> varasm.c:2632
> >>>
> >>>  arch/arm/include/asm/arch-omap5/clocks.h |   16 ++++++++--------
> >>>  1 files changed, 8 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/arch/arm/include/asm/arch-omap5/clocks.h
> >>> b/arch/arm/include/asm/arch-omap5/clocks.h index fa99f65..d0e6dd6
> 100644
> >>> --- a/arch/arm/include/asm/arch-omap5/clocks.h
> >>> +++ b/arch/arm/include/asm/arch-omap5/clocks.h
> >>> @@ -686,14 +686,14 @@ struct dpll_regs {
> >>>  struct dpll_params {
> >>>     u32 m;
> >>>     u32 n;
> >>> -   u8 m2;
> >>> -   u8 m3;
> >>> -   u8 h11;
> >>> -   u8 h12;
> >>> -   u8 h13;
> >>> -   u8 h14;
> >>> -   u8 h22;
> >>> -   u8 h23;
> >>> +   s8 m2;
> >>> +   s8 m3;
> >>> +   s8 h11;
> >>> +   s8 h12;
> >>> +   s8 h13;
> >>> +   s8 h14;
> >>> +   s8 h22;
> >>> +   s8 h23;
> >>>  };
> >>>
> >>>  extern struct omap5_prcm_regs *const prcm;
> >>
> >> Make clock registers a signed type? whoa
> >
> > No, we don't make registers a signed type. This is parameters structure
> > for some parameter tables containing -1 as an indicator that the
> > parameter shouldn't be written to the register. Using unsigned type
> > for structure field results in parameter value 255:
> >
> > static const struct dpll_params per_dpll_params_768mhz[NUM_SYS_CLKS] = {
> >         {32, 0, 4, 3, 6, 4, -1, 2, -1, -1},             /* 12 MHz   */
> >         {-1, -1, -1, -1, -1, -1, -1, -1, -1, -1},       /* 13 MHz   */
> >         {160, 6, 4, 3, 6, 4, -1, 2, -1, -1},            /* 16.8 MHz */
> >         {20, 0, 4, 3, 6, 4, -1, 2, -1, -1},             /* 19.2 MHz */
> >         {192, 12, 4, 3, 6, 4, -1, 2, -1, -1},           /* 26 MHz   */
> >         {-1, -1, -1, -1, -1, -1, -1, -1, -1, -1},       /* 27 MHz   */
> >         {10, 0, 4, 3, 6, 4, -1, 2, -1, -1}              /* 38.4 MHz */
> > };
> >
> > The code then checks:
> >
> > void setup_post_dividers(u32 *const base, const struct dpll_params
> *params)
> > {
> >         struct dpll_regs *const dpll_regs = (struct dpll_regs *)base;
> >
> >         /* Setup post-dividers */
> >         if (params->m2 >= 0)
> >                 writel(params->m2, &dpll_regs->cm_div_m2_dpll);
> >         if (params->m3 >= 0)
> >                 writel(params->m3, &dpll_regs->cm_div_m3_dpll);
> >         if (params->h11 >= 0)
> >                 writel(params->h11, &dpll_regs->cm_div_h11_dpll);
> >         if (params->h12 >= 0)
> >                 writel(params->h12, &dpll_regs->cm_div_h12_dpll);
> >         if (params->h13 >= 0)
> >                 writel(params->h13, &dpll_regs->cm_div_h13_dpll);
> >         if (params->h14 >= 0)
> >                 writel(params->h14, &dpll_regs->cm_div_h14_dpll);
> >         if (params->h22 >= 0)
> >                 writel(params->h22, &dpll_regs->cm_div_h22_dpll);
> >         if (params->h23 >= 0)
> >                 writel(params->h23, &dpll_regs->cm_div_h23_dpll);
> > }
> >
> > The result is that the registers will always be written to, since
> > the comparison is always true. This is apparently not intended in
> > the code.
> >
> > The actual registers structure 'struct dpll_regs' uses unsigned type.
> >
> > This sneaked in in the commit 2e5ba489 adding omap5 clock support.
> > The similar parameter structure for omap4 used signed type for the
> > fields in question.
> >
> > Newer gcc doesn't warn here unless -Wextra option is used.
>
> Sricharan, my examination, this analysis is correct, can you confirm
> that omap5 is supposed to work like omap4 in this case?  Thanks.
>
> --
> Tom
>