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

[Tom Rini]

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