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 <tr...@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.va...@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 <ag...@denx.de>
> >>> Cc: sricharan <r.sricha...@ti.com>
> >>> Cc: Tom Rini <tr...@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
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
