[U-Boot] [PATCH v1 1/1] lib/display_options: Fix print_freq

[U-Boot] [PATCH v1 1/1] lib/display_options: Fix print_freq

[Suriyan Ramasami]

Build without CONFIG_SPL_SERIAL_SUPPORT does not print the cpu freq.
I have seen this in the odroid U3 board, where on boot one sees this:
CPU:   Exynos4412 @  GHz
instead of:
CPU:   Exynos4412 @ 1 GHz

I am assuming that this change was done to get rid of compiler
warnings related to unused variables when building with
CONFIG_SPL_SERIAL_SUPPORT not being defined in an SPL build.

Signed-off-by: Suriyan Ramasami <suriyan.r@gmail.com>
---
 lib/display_options.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/lib/display_options.c b/lib/display_options.c
index df134cd..80316a4 100644
--- a/lib/display_options.c
+++ b/lib/display_options.c
@@ -26,9 +26,7 @@ int display_options (void)
 void print_freq(uint64_t freq, const char *s)
 {
 	unsigned long m = 0;
-#if defined(CONFIG_SPL_SERIAL_SUPPORT)
 	unsigned long n;
-#endif
 	uint32_t f;
 	static const char names[] = {'G', 'M', 'K'};
 	unsigned long d = 1e9;
@@ -48,9 +46,7 @@ void print_freq(uint64_t freq, const char *s)
 	}
 
 	f = do_div(freq, d);
-#if defined(CONFIG_SPL_SERIAL_SUPPORT)
 	n = freq;
-#endif
 
 	/* If there's a remainder, show the first few digits */
 	if (f) {
@@ -63,9 +59,7 @@ void print_freq(uint64_t freq, const char *s)
 			m = (m / 10) + (m % 100 >= 50);
 	}
 
-#if defined(CONFIG_SPL_SERIAL_SUPPORT)
 	printf("%lu", n);
-#endif
 	if (m)
 		printf(".%ld", m);
 	printf(" %cHz%s", c, s);
-- 
1.9.1

[U-Boot] [PATCH v1 1/1] lib/display_options: Fix print_freq

[Simon Glass]

Hi,

On 18 August 2015 at 10:25, Suriyan Ramasami <suriyan.r@gmail.com> wrote:
> Build without CONFIG_SPL_SERIAL_SUPPORT does not print the cpu freq.
> I have seen this in the odroid U3 board, where on boot one sees this:
> CPU:   Exynos4412 @  GHz
> instead of:
> CPU:   Exynos4412 @ 1 GHz
>
> I am assuming that this change was done to get rid of compiler
> warnings related to unused variables when building with
> CONFIG_SPL_SERIAL_SUPPORT not being defined in an SPL build.
>
> Signed-off-by: Suriyan Ramasami <suriyan.r@gmail.com>
> ---
>  lib/display_options.c | 6 ------
>  1 file changed, 6 deletions(-)

Acked-by: Simon Glass <sjg@chromium.org>

That's strange. Your patch looks correct to me.

Regards,
Simon

[U-Boot] [PATCH v1 1/1] lib/display_options: Fix print_freq

[Heiko Schocher]

Hello Simon, Suriyan Ramasami,

Am 22.08.2015 um 02:36 schrieb Simon Glass:
> Hi,
>
> On 18 August 2015 at 10:25, Suriyan Ramasami <suriyan.r@gmail.com> wrote:
>> Build without CONFIG_SPL_SERIAL_SUPPORT does not print the cpu freq.
>> I have seen this in the odroid U3 board, where on boot one sees this:
>> CPU:   Exynos4412 @  GHz
>> instead of:
>> CPU:   Exynos4412 @ 1 GHz
>>
>> I am assuming that this change was done to get rid of compiler
>> warnings related to unused variables when building with
>> CONFIG_SPL_SERIAL_SUPPORT not being defined in an SPL build.
>>
>> Signed-off-by: Suriyan Ramasami <suriyan.r@gmail.com>
>> ---
>>   lib/display_options.c | 6 ------
>>   1 file changed, 6 deletions(-)
>
> Acked-by: Simon Glass <sjg@chromium.org>
>
> That's strange. Your patch looks correct to me.

Yes, strange, how this slipped me ...

This patch leads in the following compiler warning for the smartweb
board:

   CC      spl/lib/display_options.o
/home/hs/zug/u-boot/lib/display_options.c: In function 'print_freq':
/home/hs/zug/u-boot/lib/display_options.c:29:16: warning: variable 'n' set but not used 
[-Wunused-but-set-variable]

@Suriyan Ramasami:
Could you add to your patch the following:

diff --git a/lib/display_options.c b/lib/display_options.c
index 80316a4..a4a5032 100644
--- a/lib/display_options.c
+++ b/lib/display_options.c
@@ -23,6 +23,8 @@ int display_options (void)
         return 0;
  }

+#if !defined(CONFIG_SPL_BUILD) || \
+    defined(CONFIG_SPL_SERIAL_SUPPORT)
  void print_freq(uint64_t freq, const char *s)
  {
         unsigned long m = 0;
@@ -185,3 +187,4 @@ int print_buffer(ulong addr, const void *data, uint width, uint count,

         return 0;
  }
+#endif

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH v1 1/1] lib/display_options: Fix print_freq

[Suriyan Ramasami]

Hello Heiko/Simon,

On Sun, Aug 23, 2015 at 10:37 PM, Heiko Schocher <hs@denx.de> wrote:

> Hello Simon, Suriyan Ramasami,
>
> Am 22.08.2015 um 02:36 schrieb Simon Glass:
>
>> Hi,
>>
>> On 18 August 2015 at 10:25, Suriyan Ramasami <suriyan.r@gmail.com> wrote:
>>
>>> Build without CONFIG_SPL_SERIAL_SUPPORT does not print the cpu freq.
>>> I have seen this in the odroid U3 board, where on boot one sees this:
>>> CPU:   Exynos4412 @  GHz
>>> instead of:
>>> CPU:   Exynos4412 @ 1 GHz
>>>
>>> I am assuming that this change was done to get rid of compiler
>>> warnings related to unused variables when building with
>>> CONFIG_SPL_SERIAL_SUPPORT not being defined in an SPL build.
>>>
>>> Signed-off-by: Suriyan Ramasami <suriyan.r@gmail.com>
>>> ---
>>>   lib/display_options.c | 6 ------
>>>   1 file changed, 6 deletions(-)
>>>
>>
>> Acked-by: Simon Glass <sjg@chromium.org>
>>
>> That's strange. Your patch looks correct to me.
>>
>
> Yes, strange, how this slipped me ...
>
> This patch leads in the following compiler warning for the smartweb
> board:
>
>   CC      spl/lib/display_options.o
> /home/hs/zug/u-boot/lib/display_options.c: In function 'print_freq':
> /home/hs/zug/u-boot/lib/display_options.c:29:16: warning: variable 'n' set
> but not used [-Wunused-but-set-variable]
>
> @Suriyan Ramasami:
> Could you add to your patch the following:
>
> diff --git a/lib/display_options.c b/lib/display_options.c
> index 80316a4..a4a5032 100644
> --- a/lib/display_options.c
> +++ b/lib/display_options.c
> @@ -23,6 +23,8 @@ int display_options (void)
>         return 0;
>  }
>
> +#if !defined(CONFIG_SPL_BUILD) || \
> +    defined(CONFIG_SPL_SERIAL_SUPPORT)
>  void print_freq(uint64_t freq, const char *s)
>  {
>         unsigned long m = 0;
> @@ -185,3 +187,4 @@ int print_buffer(ulong addr, const void *data, uint
> width, uint count,
>
>         return 0;
>  }
> +#endif
>
> I am trying to understand this a bit. The compiler warning seems to stem
from the fact that printf() is eradicated with CONFIG_SPL_BUILD being set,
and hence results in the 'n being set but not used' situation.
As its just one variable 'n' causing this, would it be more readable if we
do not have the #ifdef you suggested, but rather just have this instead:
1. We get rid of:
unsigned long n;
2. We substitute this:
        n = freq;
        printf("%lu", n);
with:
        printf("%lu", (unsigned long) freq);

Comments and thoughts welcome! Thanks!


> Thanks!
>
> bye,
> Heiko
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>

[U-Boot] [PATCH v1 1/1] lib/display_options: Fix print_freq

[Heiko Schocher]

Hello Suriyan,

Am 25.08.2015 um 00:14 schrieb Suriyan Ramasami:
> Hello Heiko/Simon,
>
> On Sun, Aug 23, 2015 at 10:37 PM, Heiko Schocher <hs at denx.de <mailto:hs@denx.de>> wrote:
>
>     Hello Simon, Suriyan Ramasami,
>
>     Am 22.08.2015 um 02:36 schrieb Simon Glass:
>
>         Hi,
>
>         On 18 August 2015 at 10:25, Suriyan Ramasami <suriyan.r@gmail.com
>         <mailto:suriyan.r@gmail.com>> wrote:
>
>             Build without CONFIG_SPL_SERIAL_SUPPORT does not print the cpu freq.
>             I have seen this in the odroid U3 board, where on boot one sees this:
>             CPU:   Exynos4412 @  GHz
>             instead of:
>             CPU:   Exynos4412 @ 1 GHz
>
>             I am assuming that this change was done to get rid of compiler
>             warnings related to unused variables when building with
>             CONFIG_SPL_SERIAL_SUPPORT not being defined in an SPL build.
>
>             Signed-off-by: Suriyan Ramasami <suriyan.r at gmail.com <mailto:suriyan.r@gmail.com>>
>             ---
>                lib/display_options.c | 6 ------
>                1 file changed, 6 deletions(-)
>
>
>         Acked-by: Simon Glass <sjg at chromium.org <mailto:sjg@chromium.org>>
>
>         That's strange. Your patch looks correct to me.
>
>
>     Yes, strange, how this slipped me ...
>
>     This patch leads in the following compiler warning for the smartweb
>     board:
>
>        CC      spl/lib/display_options.o
>     /home/hs/zug/u-boot/lib/display_options.c: In function 'print_freq':
>     /home/hs/zug/u-boot/lib/display_options.c:29:16: warning: variable 'n' set but not used
>     [-Wunused-but-set-variable]
>
>     @Suriyan Ramasami:
>     Could you add to your patch the following:
>
>     diff --git a/lib/display_options.c b/lib/display_options.c
>     index 80316a4..a4a5032 100644
>     --- a/lib/display_options.c
>     +++ b/lib/display_options.c
>     @@ -23,6 +23,8 @@ int display_options (void)
>              return 0;
>       }
>
>     +#if !defined(CONFIG_SPL_BUILD) || \
>     +    defined(CONFIG_SPL_SERIAL_SUPPORT)
>       void print_freq(uint64_t freq, const char *s)
>       {
>              unsigned long m = 0;
>     @@ -185,3 +187,4 @@ int print_buffer(ulong addr, const void *data, uint width, uint count,
>
>              return 0;
>       }
>     +#endif
>
> I am trying to understand this a bit. The compiler warning seems to stem from the fact that printf()
> is eradicated with CONFIG_SPL_BUILD being set, and hence results in the 'n being set but not used'
> situation.
> As its just one variable 'n' causing this, would it be more readable if we do not have the #ifdef
> you suggested, but rather just have this instead:
> 1. We get rid of:
> unsigned long n;
> 2. We substitute this:
>          n = freq;
>          printf("%lu", n);
> with:
>          printf("%lu", (unsigned long) freq);
>
> Comments and thoughts welcome! Thanks!

Sounds better!

bye,
Heiko
>
>     Thanks!
>
>     bye,
>     Heiko
>     --
>     DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>     HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>
>

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH v1 1/1] lib/display_options: Fix print_freq

[Simon Glass]

Hi,

On 24 August 2015 at 16:14, Suriyan Ramasami <suriyan.r@gmail.com> wrote:
> Hello Heiko/Simon,
>
> On Sun, Aug 23, 2015 at 10:37 PM, Heiko Schocher <hs@denx.de> wrote:
>>
>> Hello Simon, Suriyan Ramasami,
>>
>> Am 22.08.2015 um 02:36 schrieb Simon Glass:
>>>
>>> Hi,
>>>
>>> On 18 August 2015 at 10:25, Suriyan Ramasami <suriyan.r@gmail.com> wrote:
>>>>
>>>> Build without CONFIG_SPL_SERIAL_SUPPORT does not print the cpu freq.
>>>> I have seen this in the odroid U3 board, where on boot one sees this:
>>>> CPU:   Exynos4412 @  GHz
>>>> instead of:
>>>> CPU:   Exynos4412 @ 1 GHz
>>>>
>>>> I am assuming that this change was done to get rid of compiler
>>>> warnings related to unused variables when building with
>>>> CONFIG_SPL_SERIAL_SUPPORT not being defined in an SPL build.
>>>>
>>>> Signed-off-by: Suriyan Ramasami <suriyan.r@gmail.com>
>>>> ---
>>>>   lib/display_options.c | 6 ------
>>>>   1 file changed, 6 deletions(-)
>>>
>>>
>>> Acked-by: Simon Glass <sjg@chromium.org>
>>>
>>> That's strange. Your patch looks correct to me.
>>
>>
>> Yes, strange, how this slipped me ...
>>
>> This patch leads in the following compiler warning for the smartweb
>> board:
>>
>>   CC      spl/lib/display_options.o
>> /home/hs/zug/u-boot/lib/display_options.c: In function 'print_freq':
>> /home/hs/zug/u-boot/lib/display_options.c:29:16: warning: variable 'n' set
>> but not used [-Wunused-but-set-variable]
>>
>> @Suriyan Ramasami:
>> Could you add to your patch the following:
>>
>> diff --git a/lib/display_options.c b/lib/display_options.c
>> index 80316a4..a4a5032 100644
>> --- a/lib/display_options.c
>> +++ b/lib/display_options.c
>> @@ -23,6 +23,8 @@ int display_options (void)
>>         return 0;
>>  }
>>
>> +#if !defined(CONFIG_SPL_BUILD) || \
>> +    defined(CONFIG_SPL_SERIAL_SUPPORT)
>>  void print_freq(uint64_t freq, const char *s)
>>  {
>>         unsigned long m = 0;
>> @@ -185,3 +187,4 @@ int print_buffer(ulong addr, const void *data, uint
>> width, uint count,
>>
>>         return 0;
>>  }
>> +#endif
>>
> I am trying to understand this a bit. The compiler warning seems to stem
> from the fact that printf() is eradicated with CONFIG_SPL_BUILD being set,
> and hence results in the 'n being set but not used' situation.
> As its just one variable 'n' causing this, would it be more readable if we
> do not have the #ifdef you suggested, but rather just have this instead:
> 1. We get rid of:
> unsigned long n;
> 2. We substitute this:
>         n = freq;
>         printf("%lu", n);
> with:
>         printf("%lu", (unsigned long) freq);
>
> Comments and thoughts welcome! Thanks!
>

Yes you are right - I don't think we need that variable at all.

Regards,
Simon