* [U-Boot] [PATCH v1 1/1] lib/display_options: Fix print_freq @ 2015-08-18 16:25 Suriyan Ramasami 2015-08-22 0:36 ` Simon Glass 0 siblings, 1 reply; 6+ messages in thread From: Suriyan Ramasami @ 2015-08-18 16:25 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v1 1/1] lib/display_options: Fix print_freq 2015-08-18 16:25 [U-Boot] [PATCH v1 1/1] lib/display_options: Fix print_freq Suriyan Ramasami @ 2015-08-22 0:36 ` Simon Glass 2015-08-24 5:37 ` Heiko Schocher 0 siblings, 1 reply; 6+ messages in thread From: Simon Glass @ 2015-08-22 0:36 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v1 1/1] lib/display_options: Fix print_freq 2015-08-22 0:36 ` Simon Glass @ 2015-08-24 5:37 ` Heiko Schocher 2015-08-24 22:14 ` Suriyan Ramasami 0 siblings, 1 reply; 6+ messages in thread From: Heiko Schocher @ 2015-08-24 5:37 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v1 1/1] lib/display_options: Fix print_freq 2015-08-24 5:37 ` Heiko Schocher @ 2015-08-24 22:14 ` Suriyan Ramasami 2015-08-25 4:40 ` Heiko Schocher 2015-08-25 5:04 ` Simon Glass 0 siblings, 2 replies; 6+ messages in thread From: Suriyan Ramasami @ 2015-08-24 22:14 UTC (permalink / raw) To: u-boot 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v1 1/1] lib/display_options: Fix print_freq 2015-08-24 22:14 ` Suriyan Ramasami @ 2015-08-25 4:40 ` Heiko Schocher 2015-08-25 5:04 ` Simon Glass 1 sibling, 0 replies; 6+ messages in thread From: Heiko Schocher @ 2015-08-25 4:40 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v1 1/1] lib/display_options: Fix print_freq 2015-08-24 22:14 ` Suriyan Ramasami 2015-08-25 4:40 ` Heiko Schocher @ 2015-08-25 5:04 ` Simon Glass 1 sibling, 0 replies; 6+ messages in thread From: Simon Glass @ 2015-08-25 5:04 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-08-25 5:04 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-08-18 16:25 [U-Boot] [PATCH v1 1/1] lib/display_options: Fix print_freq Suriyan Ramasami 2015-08-22 0:36 ` Simon Glass 2015-08-24 5:37 ` Heiko Schocher 2015-08-24 22:14 ` Suriyan Ramasami 2015-08-25 4:40 ` Heiko Schocher 2015-08-25 5:04 ` Simon Glass
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox