| Message ID | 1322930775-4767-9-git-send-email-agust@denx.de |
|---|---|
| State | Accepted, archived |
| Delegated to: | Tom Rini |
| Headers | show |
> 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@denx.de> > Cc: sricharan <r.sricharan@ti.com> > Cc: Tom Rini <trini@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
On Sun, 4 Dec 2011 12:30:40 +0100 Marek Vasut <marek.vasut@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@denx.de> > > Cc: sricharan <r.sricharan@ti.com> > > Cc: Tom Rini <trini@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. Thanks, Anatolij
On 12/04/2011 06:59 AM, Anatolij Gustschin wrote: > On Sun, 4 Dec 2011 12:30:40 +0100 > Marek Vasut <marek.vasut@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@denx.de> >>> Cc: sricharan <r.sricharan@ti.com> >>> Cc: Tom Rini <trini@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.
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@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@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@denx.de> > >>> Cc: sricharan <r.sricharan@ti.com> > >>> Cc: Tom Rini <trini@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 >
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;
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@denx.de> Cc: sricharan <r.sricharan@ti.com> Cc: Tom Rini <trini@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(-)