[U-Boot] [PATCH 0/7] Switch rockchip firefly to using tiny-printf

[U-Boot] [PATCH 0/7] Switch rockchip firefly to using tiny-printf

[Sjoerd Simons]

The Rockchip rk3288 SPL was always too close to the 32k limit, either
needing gcc 5 or a patched gcc (with some constant string GC fixes) to
actually stay (just) below 32k. With recent changes, it unfortunatly
went over with common gcc versions.

This serie switches the firefly SPL to use tiny-printf instead of the
printf from vsprint, saving around 1800 bytes in the final binary to
bring it under the limit with a bit more margin again.



Sjoerd Simons (7):
  spl: use panic_str instead of panic
  spl: mmc: Explicitly init mmc struct
  lib/tiny-printf.c: Implement vprintf
  lib: Split panic functions out of vsprintf.c
  lib: split out strtoxxxx functions out of vsprintf.c
  mmc: mmc: Don't use sprintf when using tiny-printf
  rockchip: firefly: Use tiny-printf

 common/spl/spl.c                 |   2 +-
 common/spl/spl_mmc.c             |   2 +-
 configs/firefly-rk3288_defconfig |   1 +
 drivers/mmc/mmc.c                |   4 +-
 lib/Makefile                     |   6 +-
 lib/panic.c                      |  45 +++++++++
 lib/strto.c                      | 174 +++++++++++++++++++++++++++++++++++
 lib/tiny-printf.c                |  18 +++-
 lib/vsprintf.c                   | 193 ---------------------------------------
 9 files changed, 241 insertions(+), 204 deletions(-)
 create mode 100644 lib/panic.c
 create mode 100644 lib/strto.c

-- 
2.6.2

[U-Boot] [PATCH 1/7] spl: use panic_str instead of panic

[Sjoerd Simons]

For a simple static string, use panic_str() which prevents calling
printf needlessly.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---

 common/spl/spl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c
index 7a393dc..6e6dee7 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -452,7 +452,7 @@ ulong spl_relocate_stack_gd(void)
 #ifdef CONFIG_SPL_SYS_MALLOC_SIMPLE
 	if (CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN) {
 		if (!(gd->flags & GD_FLG_SPL_INIT))
-			panic("spl_init must be called before heap reloc");
+			panic_str("spl_init must be called before heap reloc");
 
 		ptr -= CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN;
 		gd->malloc_base = ptr;
-- 
2.6.2

[U-Boot] [PATCH 1/7] spl: use panic_str instead of panic

[Simon Glass]

On 4 December 2015 at 15:27, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> For a simple static string, use panic_str() which prevents calling
> printf needlessly.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> ---
>
>  common/spl/spl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

[U-Boot] [PATCH 1/7] spl: use panic_str instead of panic

[Simon Glass]

On 6 December 2015 at 09:45, Simon Glass <sjg@chromium.org> wrote:
>
> On 4 December 2015 at 15:27, Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
> > For a simple static string, use panic_str() which prevents calling
> > printf needlessly.
> >
> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> > ---
> >
> >  common/spl/spl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>

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

[U-Boot] [PATCH 1/7] spl: use panic_str instead of panic

[Simon Glass]

Applied to u-boot-rockchip, thanks!

[U-Boot] [PATCH 2/7] spl: mmc: Explicitly init mmc struct

[Sjoerd Simons]

gcc fails to work out that the mmc variable will only ever be used if it
has been initialized by spl_mmc_get_device_index and throws the
following error:

common/spl/spl_mmc.c: In function ?spl_mmc_load_image?:
common/spl/spl_mmc.c:31:24: warning: ?mmc? may be used uninitialized in this function [-Wmaybe-uninitialized]
  count = mmc->block_dev.block_read(0, sector, 1, header);
                        ^
common/spl/spl_mmc.c:251:14: note: ?mmc? was declared here
  struct mmc *mmc;

Prevent this by explicitly initializing the variable.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---

 common/spl/spl_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index b3c2c64..43748d0 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -248,7 +248,7 @@ int spl_mmc_do_fs_boot(struct mmc *mmc)
 
 int spl_mmc_load_image(u32 boot_device)
 {
-	struct mmc *mmc;
+	struct mmc *mmc = NULL;
 	u32 boot_mode;
 	int err = 0;
 	__maybe_unused int part;
-- 
2.6.2

[U-Boot] [PATCH 2/7] spl: mmc: Explicitly init mmc struct

[Simon Glass]

Hi Sjoerd,

On 4 December 2015 at 15:27, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> gcc fails to work out that the mmc variable will only ever be used if it
> has been initialized by spl_mmc_get_device_index and throws the
> following error:
>
> common/spl/spl_mmc.c: In function ?spl_mmc_load_image?:
> common/spl/spl_mmc.c:31:24: warning: ?mmc? may be used uninitialized in this function [-Wmaybe-uninitialized]
>   count = mmc->block_dev.block_read(0, sector, 1, header);
>                         ^
> common/spl/spl_mmc.c:251:14: note: ?mmc? was declared here
>   struct mmc *mmc;
>
> Prevent this by explicitly initializing the variable.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> ---

See:

http://patchwork.ozlabs.org/patch/551653/

Regards,
Simon

[U-Boot] [PATCH 3/7] lib/tiny-printf.c: Implement vprintf

[Sjoerd Simons]

Implement both printf and vprintf for a bit more flexibility, e.g.
allows the panic() function to work with tiny-printf.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---

 lib/tiny-printf.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
index 6766a8f..403b134 100644
--- a/lib/tiny-printf.c
+++ b/lib/tiny-printf.c
@@ -40,17 +40,14 @@ static void div_out(unsigned int *num, unsigned int div)
 		out_dgt(dgt);
 }
 
-int printf(const char *fmt, ...)
+int vprintf(const char *fmt, va_list va)
 {
-	va_list va;
 	char ch;
 	char *p;
 	unsigned int num;
 	char buf[12];
 	unsigned int div;
 
-	va_start(va, fmt);
-
 	while ((ch = *(fmt++))) {
 		if (ch != '%') {
 			putc(ch);
@@ -117,6 +114,17 @@ int printf(const char *fmt, ...)
 	}
 
 abort:
-	va_end(va);
 	return 0;
 }
+
+int printf(const char *fmt, ...)
+{
+	va_list va;
+	int ret;
+
+	va_start(va, fmt);
+	ret = vprintf(fmt, va);
+	va_end(va);
+
+	return ret;
+}
-- 
2.6.2

[U-Boot] [PATCH 3/7] lib/tiny-printf.c: Implement vprintf

[Simon Glass]

On 4 December 2015 at 15:27, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> Implement both printf and vprintf for a bit more flexibility, e.g.
> allows the panic() function to work with tiny-printf.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> ---
>
>  lib/tiny-printf.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)

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

[U-Boot] [PATCH 3/7] lib/tiny-printf.c: Implement vprintf

[Simon Glass]

Applied to u-boot-rockchip, thanks!

[U-Boot] [PATCH 4/7] lib: Split panic functions out of vsprintf.c

[Sjoerd Simons]

To allow panic and panic_str to still be used when using tiny-printf,
split them out into their own file which gets build regardless of what
printf implementation is used.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---

 lib/Makefile   |  6 +++---
 lib/panic.c    | 45 +++++++++++++++++++++++++++++++++++++++++++++
 lib/vsprintf.c | 29 -----------------------------
 3 files changed, 48 insertions(+), 32 deletions(-)
 create mode 100644 lib/panic.c

diff --git a/lib/Makefile b/lib/Makefile
index 1f1ff6f..ae84833 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -85,13 +85,13 @@ obj-$(CONFIG_LIB_RAND) += rand.o
 ifdef CONFIG_SPL_BUILD
 # SPL U-Boot may use full-printf, tiny-printf or none at all
 ifdef CONFIG_USE_TINY_PRINTF
-obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o
+obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o
 else
-obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o
+obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o panic.o
 endif
 else
 # Main U-Boot always uses the full printf support
-obj-y += vsprintf.o
+obj-y += vsprintf.o panic.o
 endif
 
 subdir-ccflags-$(CONFIG_CC_OPTIMIZE_LIBS_FOR_SPEED) += -O2
diff --git a/lib/panic.c b/lib/panic.c
new file mode 100644
index 0000000..e2b8b74
--- /dev/null
+++ b/lib/panic.c
@@ -0,0 +1,45 @@
+/*
+ *  linux/lib/vsprintf.c
+ *
+ *  Copyright (C) 1991, 1992  Linus Torvalds
+ */
+
+/* vsprintf.c -- Lars Wirzenius & Linus Torvalds. */
+/*
+ * Wirzenius wrote this portably, Torvalds fucked it up :-)
+ */
+
+#include <common.h>
+#if !defined(CONFIG_PANIC_HANG)
+#include <command.h>
+#endif
+
+static void panic_finish(void) __attribute__ ((noreturn));
+
+static void panic_finish(void)
+{
+	putc('\n');
+#if defined(CONFIG_PANIC_HANG)
+	hang();
+#else
+	udelay(100000);	/* allow messages to go out */
+	do_reset(NULL, 0, 0, NULL);
+#endif
+	while (1)
+		;
+}
+
+void panic_str(const char *str)
+{
+	puts(str);
+	panic_finish();
+}
+
+void panic(const char *fmt, ...)
+{
+	va_list args;
+	va_start(args, fmt);
+	vprintf(fmt, args);
+	va_end(args);
+	panic_finish();
+}
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index dd8380b..bf5fd01 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -897,35 +897,6 @@ int vprintf(const char *fmt, va_list args)
 	return i;
 }
 
-static void panic_finish(void) __attribute__ ((noreturn));
-
-static void panic_finish(void)
-{
-	putc('\n');
-#if defined(CONFIG_PANIC_HANG)
-	hang();
-#else
-	udelay(100000);	/* allow messages to go out */
-	do_reset(NULL, 0, 0, NULL);
-#endif
-	while (1)
-		;
-}
-
-void panic_str(const char *str)
-{
-	puts(str);
-	panic_finish();
-}
-
-void panic(const char *fmt, ...)
-{
-	va_list args;
-	va_start(args, fmt);
-	vprintf(fmt, args);
-	va_end(args);
-	panic_finish();
-}
 
 void __assert_fail(const char *assertion, const char *file, unsigned line,
 		   const char *function)
-- 
2.6.2

[U-Boot] [PATCH 4/7] lib: Split panic functions out of vsprintf.c

[Simon Glass]

Hi Sjoerd,

On 4 December 2015 at 15:27, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> To allow panic and panic_str to still be used when using tiny-printf,
> split them out into their own file which gets build regardless of what
> printf implementation is used.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> ---
>
>  lib/Makefile   |  6 +++---
>  lib/panic.c    | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  lib/vsprintf.c | 29 -----------------------------
>  3 files changed, 48 insertions(+), 32 deletions(-)
>  create mode 100644 lib/panic.c

Tested on firefly:
Tested-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/lib/Makefile b/lib/Makefile
> index 1f1ff6f..ae84833 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -85,13 +85,13 @@ obj-$(CONFIG_LIB_RAND) += rand.o
>  ifdef CONFIG_SPL_BUILD
>  # SPL U-Boot may use full-printf, tiny-printf or none at all
>  ifdef CONFIG_USE_TINY_PRINTF
> -obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o
> +obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o
>  else
> -obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o
> +obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o panic.o
>  endif
>  else
>  # Main U-Boot always uses the full printf support
> -obj-y += vsprintf.o
> +obj-y += vsprintf.o panic.o
>  endif

Why not just add this outside all the ifdef stuff:

obj-y += panic.o

>
>  subdir-ccflags-$(CONFIG_CC_OPTIMIZE_LIBS_FOR_SPEED) += -O2
> diff --git a/lib/panic.c b/lib/panic.c
> new file mode 100644
> index 0000000..e2b8b74
> --- /dev/null
> +++ b/lib/panic.c
> @@ -0,0 +1,45 @@
> +/*
> + *  linux/lib/vsprintf.c

nit: can you please drop this line or fix it?

> + *
> + *  Copyright (C) 1991, 1992  Linus Torvalds
> + */
> +
> +/* vsprintf.c -- Lars Wirzenius & Linus Torvalds. */
> +/*
> + * Wirzenius wrote this portably, Torvalds fucked it up :-)
> + */

Did any of this code actually come from Linux? If not perhaps invent
your own copyright?

> +
> +#include <common.h>
> +#if !defined(CONFIG_PANIC_HANG)
> +#include <command.h>
> +#endif
> +
> +static void panic_finish(void) __attribute__ ((noreturn));
> +
> +static void panic_finish(void)
> +{
> +       putc('\n');
> +#if defined(CONFIG_PANIC_HANG)
> +       hang();
> +#else
> +       udelay(100000); /* allow messages to go out */
> +       do_reset(NULL, 0, 0, NULL);
> +#endif
> +       while (1)
> +               ;
> +}
> +
> +void panic_str(const char *str)
> +{
> +       puts(str);
> +       panic_finish();
> +}
> +
> +void panic(const char *fmt, ...)
> +{
> +       va_list args;
> +       va_start(args, fmt);
> +       vprintf(fmt, args);
> +       va_end(args);
> +       panic_finish();
> +}
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index dd8380b..bf5fd01 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -897,35 +897,6 @@ int vprintf(const char *fmt, va_list args)
>         return i;
>  }
>
> -static void panic_finish(void) __attribute__ ((noreturn));
> -
> -static void panic_finish(void)
> -{
> -       putc('\n');
> -#if defined(CONFIG_PANIC_HANG)
> -       hang();
> -#else
> -       udelay(100000); /* allow messages to go out */
> -       do_reset(NULL, 0, 0, NULL);
> -#endif
> -       while (1)
> -               ;
> -}
> -
> -void panic_str(const char *str)
> -{
> -       puts(str);
> -       panic_finish();
> -}
> -
> -void panic(const char *fmt, ...)
> -{
> -       va_list args;
> -       va_start(args, fmt);
> -       vprintf(fmt, args);
> -       va_end(args);
> -       panic_finish();
> -}
>
>  void __assert_fail(const char *assertion, const char *file, unsigned line,
>                    const char *function)
> --
> 2.6.2
>

Regards,
Simon

[U-Boot] [PATCH 4/7] lib: Split panic functions out of vsprintf.c

[Sjoerd Simons]

On Mon, 2015-12-07 at 17:39 -0700, Simon Glass wrote:
> > diff --git a/lib/Makefile b/lib/Makefile
> > index 1f1ff6f..ae84833 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -85,13 +85,13 @@ obj-$(CONFIG_LIB_RAND) += rand.o
> > ?ifdef CONFIG_SPL_BUILD
> > ?# SPL U-Boot may use full-printf, tiny-printf or none at all
> > ?ifdef CONFIG_USE_TINY_PRINTF
> > -obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o
> > +obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o
> > ?else
> > -obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o
> > +obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o panic.o
> > ?endif
> > ?else
> > ?# Main U-Boot always uses the full printf support
> > -obj-y += vsprintf.o
> > +obj-y += vsprintf.o panic.o
> > ?endif
> 
> Why not just add this outside all the ifdef stuff:
> 
> obj-y += panic.o

Just keeping the old behaviour, that code was not build for SPL builds
without serial support before. Do you see a benefit of just always
building it ?

> > ?subdir-ccflags-$(CONFIG_CC_OPTIMIZE_LIBS_FOR_SPEED) += -O2
> > diff --git a/lib/panic.c b/lib/panic.c
> > new file mode 100644
> > index 0000000..e2b8b74
> > --- /dev/null
> > +++ b/lib/panic.c
> > @@ -0,0 +1,45 @@
> > +/*
> > + *??linux/lib/vsprintf.c
> 
> nit: can you please drop this line or fix it?

Sure it's pointless anyway

> > + *
> > + *??Copyright (C) 1991, 1992??Linus Torvalds
> > + */
> > +
> > +/* vsprintf.c -- Lars Wirzenius & Linus Torvalds. */
> > +/*
> > + * Wirzenius wrote this portably, Torvalds fucked it up :-)
> > + */
> 
> Did any of this code actually come from Linux? If not perhaps invent
> your own copyright?

I actually went back and checked, vsprintf.c was imported
in?de180e6daa529dc78668c99bdf17a9cdd440782d which is a helpful commit
with the name "Initial revisions".?

Most of the code in vsprintf.c is likely to just come from from linux
afaik (see lib/vsprintf.c in the linux source) especial in the initial
linux git repository. Unfortunate unless you actually want to go
trawling back through pre-git linux versions to work out how similar
the were at the branching point.

The panic functions don't appear in git versions of linux, but may or
may not be there in pre-git versions.

In this case I just took the simple/conservative path and copied the
copyright header (assuming correctness which seems reasonable enough)
when splitting things up. I'd be fine with adjusting the copyright
header _if_ there was more (easily) available historical data about the
authors. Given that doesn't seem to be the case, I would prefer to keep
the copyright as-is unless someone wants to take the time to do some
code archaeology :)


-- 
Sjoerd Simons
Collabora Ltd.

[U-Boot] [PATCH 4/7] lib: Split panic functions out of vsprintf.c

[Simon Glass]

Hi Sjoerd,

On 8 December 2015 at 00:27, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
>
> On Mon, 2015-12-07 at 17:39 -0700, Simon Glass wrote:
> > > diff --git a/lib/Makefile b/lib/Makefile
> > > index 1f1ff6f..ae84833 100644
> > > --- a/lib/Makefile
> > > +++ b/lib/Makefile
> > > @@ -85,13 +85,13 @@ obj-$(CONFIG_LIB_RAND) += rand.o
> > >  ifdef CONFIG_SPL_BUILD
> > >  # SPL U-Boot may use full-printf, tiny-printf or none at all
> > >  ifdef CONFIG_USE_TINY_PRINTF
> > > -obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o
> > > +obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o
> > >  else
> > > -obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o
> > > +obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o panic.o
> > >  endif
> > >  else
> > >  # Main U-Boot always uses the full printf support
> > > -obj-y += vsprintf.o
> > > +obj-y += vsprintf.o panic.o
> > >  endif
> >
> > Why not just add this outside all the ifdef stuff:
> >
> > obj-y += panic.o
>
> Just keeping the old behaviour, that code was not build for SPL builds
> without serial support before. Do you see a benefit of just always
> building it ?

I cannot see a case where you don't build it:

ifdef CONFIG_SPL_BUILD
# SPL U-Boot may use full-printf, tiny-printf or none at all
ifdef CONFIG_USE_TINY_PRINTF
obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o strto.o
else
obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o panic.o strto.o
endif
else
# Main U-Boot always uses the full printf support
obj-y += vsprintf.o panic.o strto.o
endif


Every case has panic.o and strto.o. What am I missing?

>
> > >  subdir-ccflags-$(CONFIG_CC_OPTIMIZE_LIBS_FOR_SPEED) += -O2
> > > diff --git a/lib/panic.c b/lib/panic.c
> > > new file mode 100644
> > > index 0000000..e2b8b74
> > > --- /dev/null
> > > +++ b/lib/panic.c
> > > @@ -0,0 +1,45 @@
> > > +/*
> > > + *  linux/lib/vsprintf.c
> >
> > nit: can you please drop this line or fix it?
>
> Sure it's pointless anyway
>
> > > + *
> > > + *  Copyright (C) 1991, 1992  Linus Torvalds
> > > + */
> > > +
> > > +/* vsprintf.c -- Lars Wirzenius & Linus Torvalds. */
> > > +/*
> > > + * Wirzenius wrote this portably, Torvalds fucked it up :-)
> > > + */
> >
> > Did any of this code actually come from Linux? If not perhaps invent
> > your own copyright?
>
> I actually went back and checked, vsprintf.c was imported
> in de180e6daa529dc78668c99bdf17a9cdd440782d which is a helpful commit
> with the name "Initial revisions".
>
> Most of the code in vsprintf.c is likely to just come from from linux
> afaik (see lib/vsprintf.c in the linux source) especial in the initial
> linux git repository. Unfortunate unless you actually want to go
> trawling back through pre-git linux versions to work out how similar
> the were at the branching point.
>
> The panic functions don't appear in git versions of linux, but may or
> may not be there in pre-git versions.
>
> In this case I just took the simple/conservative path and copied the
> copyright header (assuming correctness which seems reasonable enough)
> when splitting things up. I'd be fine with adjusting the copyright
> header _if_ there was more (easily) available historical data about the
> authors. Given that doesn't seem to be the case, I would prefer to keep
> the copyright as-is unless someone wants to take the time to do some
> code archaeology :)
>
>
> --
> Sjoerd Simons
> Collabora Ltd.

Regards,
Simon

[U-Boot] [PATCH 4/7] lib: Split panic functions out of vsprintf.c

[Scott Wood]

On Tue, 2015-12-08 at 12:34 -0700, Simon Glass wrote:
> Hi Sjoerd,
> 
> On 8 December 2015 at 00:27, Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
> > 
> > On Mon, 2015-12-07 at 17:39 -0700, Simon Glass wrote:
> > > > diff --git a/lib/Makefile b/lib/Makefile
> > > > index 1f1ff6f..ae84833 100644
> > > > --- a/lib/Makefile
> > > > +++ b/lib/Makefile
> > > > @@ -85,13 +85,13 @@ obj-$(CONFIG_LIB_RAND) += rand.o
> > > >  ifdef CONFIG_SPL_BUILD
> > > >  # SPL U-Boot may use full-printf, tiny-printf or none at all
> > > >  ifdef CONFIG_USE_TINY_PRINTF
> > > > -obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o
> > > > +obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o
> > > >  else
> > > > -obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o
> > > > +obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o panic.o
> > > >  endif
> > > >  else
> > > >  # Main U-Boot always uses the full printf support
> > > > -obj-y += vsprintf.o
> > > > +obj-y += vsprintf.o panic.o
> > > >  endif
> > > 
> > > Why not just add this outside all the ifdef stuff:
> > > 
> > > obj-y += panic.o
> > 
> > Just keeping the old behaviour, that code was not build for SPL builds
> > without serial support before. Do you see a benefit of just always
> > building it ?
> 
> I cannot see a case where you don't build it:
> 
> ifdef CONFIG_SPL_BUILD
> # SPL U-Boot may use full-printf, tiny-printf or none at all
> ifdef CONFIG_USE_TINY_PRINTF
> obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o strto.o
> else
> obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o panic.o strto.o
> endif
> else
> # Main U-Boot always uses the full printf support
> obj-y += vsprintf.o panic.o strto.o
> endif
> 
> 
> Every case has panic.o and strto.o. What am I missing?

The dependency on CONFIG_SPL_SERIAL_SUPPORT.

-Scott

[U-Boot] [PATCH 4/7] lib: Split panic functions out of vsprintf.c

[Simon Glass]

Hi Scott,

On 8 December 2015 at 12:36, Scott Wood <scottwood@freescale.com> wrote:
> On Tue, 2015-12-08 at 12:34 -0700, Simon Glass wrote:
>> Hi Sjoerd,
>>
>> On 8 December 2015 at 00:27, Sjoerd Simons
>> <sjoerd.simons@collabora.co.uk> wrote:
>> >
>> > On Mon, 2015-12-07 at 17:39 -0700, Simon Glass wrote:
>> > > > diff --git a/lib/Makefile b/lib/Makefile
>> > > > index 1f1ff6f..ae84833 100644
>> > > > --- a/lib/Makefile
>> > > > +++ b/lib/Makefile
>> > > > @@ -85,13 +85,13 @@ obj-$(CONFIG_LIB_RAND) += rand.o
>> > > >  ifdef CONFIG_SPL_BUILD
>> > > >  # SPL U-Boot may use full-printf, tiny-printf or none at all
>> > > >  ifdef CONFIG_USE_TINY_PRINTF
>> > > > -obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o
>> > > > +obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o
>> > > >  else
>> > > > -obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o
>> > > > +obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o panic.o
>> > > >  endif
>> > > >  else
>> > > >  # Main U-Boot always uses the full printf support
>> > > > -obj-y += vsprintf.o
>> > > > +obj-y += vsprintf.o panic.o
>> > > >  endif
>> > >
>> > > Why not just add this outside all the ifdef stuff:
>> > >
>> > > obj-y += panic.o
>> >
>> > Just keeping the old behaviour, that code was not build for SPL builds
>> > without serial support before. Do you see a benefit of just always
>> > building it ?
>>
>> I cannot see a case where you don't build it:
>>
>> ifdef CONFIG_SPL_BUILD
>> # SPL U-Boot may use full-printf, tiny-printf or none at all
>> ifdef CONFIG_USE_TINY_PRINTF
>> obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o strto.o
>> else
>> obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o panic.o strto.o
>> endif
>> else
>> # Main U-Boot always uses the full printf support
>> obj-y += vsprintf.o panic.o strto.o
>> endif
>>
>>
>> Every case has panic.o and strto.o. What am I missing?
>
> The dependency on CONFIG_SPL_SERIAL_SUPPORT.

OK, so how about this:

>> ifdef CONFIG_SPL_BUILD
>> # SPL U-Boot may use full-printf, tiny-printf or none at all

obj-$(CONFIG_SPL_SERIAL_SUPPORT) += panic.o strto.o

>> ifdef CONFIG_USE_TINY_PRINTF
obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o
>> else
obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o
>> endif
>> else
>> # Main U-Boot always uses the full printf support
>> obj-y += vsprintf.o panic.o strto.o
>> endif

Regards,
Simon

[U-Boot] [PATCH 4/7] lib: Split panic functions out of vsprintf.c

[Simon Glass]

On 8 December 2015 at 12:38, Simon Glass <sjg@chromium.org> wrote:
> Hi Scott,
>
> On 8 December 2015 at 12:36, Scott Wood <scottwood@freescale.com> wrote:
>> On Tue, 2015-12-08 at 12:34 -0700, Simon Glass wrote:
>>> Hi Sjoerd,
>>>
>>> On 8 December 2015 at 00:27, Sjoerd Simons
>>> <sjoerd.simons@collabora.co.uk> wrote:
>>> >
>>> > On Mon, 2015-12-07 at 17:39 -0700, Simon Glass wrote:
>>> > > > diff --git a/lib/Makefile b/lib/Makefile
>>> > > > index 1f1ff6f..ae84833 100644
>>> > > > --- a/lib/Makefile
>>> > > > +++ b/lib/Makefile
>>> > > > @@ -85,13 +85,13 @@ obj-$(CONFIG_LIB_RAND) += rand.o
>>> > > >  ifdef CONFIG_SPL_BUILD
>>> > > >  # SPL U-Boot may use full-printf, tiny-printf or none at all
>>> > > >  ifdef CONFIG_USE_TINY_PRINTF
>>> > > > -obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o
>>> > > > +obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o
>>> > > >  else
>>> > > > -obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o
>>> > > > +obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o panic.o
>>> > > >  endif
>>> > > >  else
>>> > > >  # Main U-Boot always uses the full printf support
>>> > > > -obj-y += vsprintf.o
>>> > > > +obj-y += vsprintf.o panic.o
>>> > > >  endif
>>> > >
>>> > > Why not just add this outside all the ifdef stuff:
>>> > >
>>> > > obj-y += panic.o
>>> >
>>> > Just keeping the old behaviour, that code was not build for SPL builds
>>> > without serial support before. Do you see a benefit of just always
>>> > building it ?
>>>
>>> I cannot see a case where you don't build it:
>>>
>>> ifdef CONFIG_SPL_BUILD
>>> # SPL U-Boot may use full-printf, tiny-printf or none at all
>>> ifdef CONFIG_USE_TINY_PRINTF
>>> obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o strto.o
>>> else
>>> obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o panic.o strto.o
>>> endif
>>> else
>>> # Main U-Boot always uses the full printf support
>>> obj-y += vsprintf.o panic.o strto.o
>>> endif
>>>
>>>
>>> Every case has panic.o and strto.o. What am I missing?
>>
>> The dependency on CONFIG_SPL_SERIAL_SUPPORT.
>
> OK, so how about this:
>
>>> ifdef CONFIG_SPL_BUILD
>>> # SPL U-Boot may use full-printf, tiny-printf or none at all
>
> obj-$(CONFIG_SPL_SERIAL_SUPPORT) += panic.o strto.o
>
>>> ifdef CONFIG_USE_TINY_PRINTF
> obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o
>>> else
> obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o
>>> endif
>>> else
>>> # Main U-Boot always uses the full printf support
>>> obj-y += vsprintf.o panic.o strto.o
>>> endif

It's just a nit so I'm going to leave it as is for now.

Applied to u-boot-rockchip, thanks!

[U-Boot] [PATCH 4/7] lib: Split panic functions out of vsprintf.c

[Sjoerd Simons]

On Sun, 2015-12-13 at 20:45 -0700, Simon Glass wrote:
> On 8 December 2015 at 12:38, Simon Glass <sjg@chromium.org> wrote:
> > Hi Scott,
> > 
> > On 8 December 2015 at 12:36, Scott Wood <scottwood@freescale.com>
> > wrote:
> > > On Tue, 2015-12-08 at 12:34 -0700, Simon Glass wrote:
> > > > Hi Sjoerd,
> > > >?
> > OK, so how about this:
> > 
> > > > ifdef CONFIG_SPL_BUILD
> > > > # SPL U-Boot may use full-printf, tiny-printf or none at all
> > 
> > obj-$(CONFIG_SPL_SERIAL_SUPPORT) += panic.o strto.o
> > 
> > > > ifdef CONFIG_USE_TINY_PRINTF
> > obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o
> > > > else
> > obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o
> > > > endif
> > > > else
> > > > # Main U-Boot always uses the full printf support
> > > > obj-y += vsprintf.o panic.o strto.o
> > > > endif
> 
> It's just a nit so I'm going to leave it as is for now.
> 
> Applied to u-boot-rockchip, thanks!

Heh, i just did an update series last night to send this morning :)
lovely timing.

I'll drop you some patches for these nits later this week while i still
remember them :) (I agree your suggestion here is nicer).


-- 
Sjoerd Simons
Collabora Ltd.

[U-Boot] [PATCH 5/7] lib: split out strtoxxxx functions out of vsprintf.c

[Sjoerd Simons]

To allow the various string to number conversion functions to be used
when using tiny-printf,split them out into their own file which gets
build regardless of what printf implementation is used.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---

 lib/Makefile   |   6 +-
 lib/strto.c    | 174 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/vsprintf.c | 164 -----------------------------------------------------
 3 files changed, 177 insertions(+), 167 deletions(-)
 create mode 100644 lib/strto.c

diff --git a/lib/Makefile b/lib/Makefile
index ae84833..dd36f25 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -85,13 +85,13 @@ obj-$(CONFIG_LIB_RAND) += rand.o
 ifdef CONFIG_SPL_BUILD
 # SPL U-Boot may use full-printf, tiny-printf or none at all
 ifdef CONFIG_USE_TINY_PRINTF
-obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o
+obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o strto.o
 else
-obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o panic.o
+obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o panic.o strto.o
 endif
 else
 # Main U-Boot always uses the full printf support
-obj-y += vsprintf.o panic.o
+obj-y += vsprintf.o panic.o strto.o
 endif
 
 subdir-ccflags-$(CONFIG_CC_OPTIMIZE_LIBS_FOR_SPEED) += -O2
diff --git a/lib/strto.c b/lib/strto.c
new file mode 100644
index 0000000..a6c0157
--- /dev/null
+++ b/lib/strto.c
@@ -0,0 +1,174 @@
+/*
+ *  linux/lib/vsprintf.c
+ *
+ *  Copyright (C) 1991, 1992  Linus Torvalds
+ */
+
+/* vsprintf.c -- Lars Wirzenius & Linus Torvalds. */
+/*
+ * Wirzenius wrote this portably, Torvalds fucked it up :-)
+ */
+
+#include <common.h>
+#include <errno.h>
+#include <linux/ctype.h>
+
+unsigned long simple_strtoul(const char *cp, char **endp,
+				unsigned int base)
+{
+	unsigned long result = 0;
+	unsigned long value;
+
+	if (*cp == '0') {
+		cp++;
+		if ((*cp == 'x') && isxdigit(cp[1])) {
+			base = 16;
+			cp++;
+		}
+
+		if (!base)
+			base = 8;
+	}
+
+	if (!base)
+		base = 10;
+
+	while (isxdigit(*cp) && (value = isdigit(*cp) ? *cp-'0' : (islower(*cp)
+	    ? toupper(*cp) : *cp)-'A'+10) < base) {
+		result = result*base + value;
+		cp++;
+	}
+
+	if (endp)
+		*endp = (char *)cp;
+
+	return result;
+}
+
+int strict_strtoul(const char *cp, unsigned int base, unsigned long *res)
+{
+	char *tail;
+	unsigned long val;
+	size_t len;
+
+	*res = 0;
+	len = strlen(cp);
+	if (len == 0)
+		return -EINVAL;
+
+	val = simple_strtoul(cp, &tail, base);
+	if (tail == cp)
+		return -EINVAL;
+
+	if ((*tail == '\0') ||
+		((len == (size_t)(tail - cp) + 1) && (*tail == '\n'))) {
+		*res = val;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+long simple_strtol(const char *cp, char **endp, unsigned int base)
+{
+	if (*cp == '-')
+		return -simple_strtoul(cp + 1, endp, base);
+
+	return simple_strtoul(cp, endp, base);
+}
+
+unsigned long ustrtoul(const char *cp, char **endp, unsigned int base)
+{
+	unsigned long result = simple_strtoul(cp, endp, base);
+	switch (**endp) {
+	case 'G':
+		result *= 1024;
+		/* fall through */
+	case 'M':
+		result *= 1024;
+		/* fall through */
+	case 'K':
+	case 'k':
+		result *= 1024;
+		if ((*endp)[1] == 'i') {
+			if ((*endp)[2] == 'B')
+				(*endp) += 3;
+			else
+				(*endp) += 2;
+		}
+	}
+	return result;
+}
+
+unsigned long long ustrtoull(const char *cp, char **endp, unsigned int base)
+{
+	unsigned long long result = simple_strtoull(cp, endp, base);
+	switch (**endp) {
+	case 'G':
+		result *= 1024;
+		/* fall through */
+	case 'M':
+		result *= 1024;
+		/* fall through */
+	case 'K':
+	case 'k':
+		result *= 1024;
+		if ((*endp)[1] == 'i') {
+			if ((*endp)[2] == 'B')
+				(*endp) += 3;
+			else
+				(*endp) += 2;
+		}
+	}
+	return result;
+}
+
+unsigned long long simple_strtoull(const char *cp, char **endp,
+					unsigned int base)
+{
+	unsigned long long result = 0, value;
+
+	if (*cp == '0') {
+		cp++;
+		if ((*cp == 'x') && isxdigit(cp[1])) {
+			base = 16;
+			cp++;
+		}
+
+		if (!base)
+			base = 8;
+	}
+
+	if (!base)
+		base = 10;
+
+	while (isxdigit(*cp) && (value = isdigit(*cp) ? *cp - '0'
+		: (islower(*cp) ? toupper(*cp) : *cp) - 'A' + 10) < base) {
+		result = result * base + value;
+		cp++;
+	}
+
+	if (endp)
+		*endp = (char *) cp;
+
+	return result;
+}
+
+long trailing_strtoln(const char *str, const char *end)
+{
+	const char *p;
+
+	if (!end)
+		end = str + strlen(str);
+	for (p = end - 1; p > str; p--) {
+		if (!isdigit(*p))
+			return simple_strtoul(p + 1, NULL, 10);
+	}
+
+	return -1;
+}
+
+long trailing_strtol(const char *str)
+{
+	return trailing_strtoln(str, NULL);
+}
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index bf5fd01..24167a1 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -15,176 +15,12 @@
 #include <linux/types.h>
 #include <linux/string.h>
 #include <linux/ctype.h>
-#include <errno.h>
 
 #include <common.h>
-#if !defined(CONFIG_PANIC_HANG)
-#include <command.h>
-#endif
 
 #include <div64.h>
 #define noinline __attribute__((noinline))
 
-unsigned long simple_strtoul(const char *cp, char **endp,
-				unsigned int base)
-{
-	unsigned long result = 0;
-	unsigned long value;
-
-	if (*cp == '0') {
-		cp++;
-		if ((*cp == 'x') && isxdigit(cp[1])) {
-			base = 16;
-			cp++;
-		}
-
-		if (!base)
-			base = 8;
-	}
-
-	if (!base)
-		base = 10;
-
-	while (isxdigit(*cp) && (value = isdigit(*cp) ? *cp-'0' : (islower(*cp)
-	    ? toupper(*cp) : *cp)-'A'+10) < base) {
-		result = result*base + value;
-		cp++;
-	}
-
-	if (endp)
-		*endp = (char *)cp;
-
-	return result;
-}
-
-int strict_strtoul(const char *cp, unsigned int base, unsigned long *res)
-{
-	char *tail;
-	unsigned long val;
-	size_t len;
-
-	*res = 0;
-	len = strlen(cp);
-	if (len == 0)
-		return -EINVAL;
-
-	val = simple_strtoul(cp, &tail, base);
-	if (tail == cp)
-		return -EINVAL;
-
-	if ((*tail == '\0') ||
-		((len == (size_t)(tail - cp) + 1) && (*tail == '\n'))) {
-		*res = val;
-		return 0;
-	}
-
-	return -EINVAL;
-}
-
-long simple_strtol(const char *cp, char **endp, unsigned int base)
-{
-	if (*cp == '-')
-		return -simple_strtoul(cp + 1, endp, base);
-
-	return simple_strtoul(cp, endp, base);
-}
-
-unsigned long ustrtoul(const char *cp, char **endp, unsigned int base)
-{
-	unsigned long result = simple_strtoul(cp, endp, base);
-	switch (**endp) {
-	case 'G':
-		result *= 1024;
-		/* fall through */
-	case 'M':
-		result *= 1024;
-		/* fall through */
-	case 'K':
-	case 'k':
-		result *= 1024;
-		if ((*endp)[1] == 'i') {
-			if ((*endp)[2] == 'B')
-				(*endp) += 3;
-			else
-				(*endp) += 2;
-		}
-	}
-	return result;
-}
-
-unsigned long long ustrtoull(const char *cp, char **endp, unsigned int base)
-{
-	unsigned long long result = simple_strtoull(cp, endp, base);
-	switch (**endp) {
-	case 'G':
-		result *= 1024;
-		/* fall through */
-	case 'M':
-		result *= 1024;
-		/* fall through */
-	case 'K':
-	case 'k':
-		result *= 1024;
-		if ((*endp)[1] == 'i') {
-			if ((*endp)[2] == 'B')
-				(*endp) += 3;
-			else
-				(*endp) += 2;
-		}
-	}
-	return result;
-}
-
-unsigned long long simple_strtoull(const char *cp, char **endp,
-					unsigned int base)
-{
-	unsigned long long result = 0, value;
-
-	if (*cp == '0') {
-		cp++;
-		if ((*cp == 'x') && isxdigit(cp[1])) {
-			base = 16;
-			cp++;
-		}
-
-		if (!base)
-			base = 8;
-	}
-
-	if (!base)
-		base = 10;
-
-	while (isxdigit(*cp) && (value = isdigit(*cp) ? *cp - '0'
-		: (islower(*cp) ? toupper(*cp) : *cp) - 'A' + 10) < base) {
-		result = result * base + value;
-		cp++;
-	}
-
-	if (endp)
-		*endp = (char *) cp;
-
-	return result;
-}
-
-long trailing_strtoln(const char *str, const char *end)
-{
-	const char *p;
-
-	if (!end)
-		end = str + strlen(str);
-	for (p = end - 1; p > str; p--) {
-		if (!isdigit(*p))
-			return simple_strtoul(p + 1, NULL, 10);
-	}
-
-	return -1;
-}
-
-long trailing_strtol(const char *str)
-{
-	return trailing_strtoln(str, NULL);
-}
-
 /* we use this so that we can do without the ctype library */
 #define is_digit(c)	((c) >= '0' && (c) <= '9')
 
-- 
2.6.2

[U-Boot] [PATCH 5/7] lib: split out strtoxxxx functions out of vsprintf.c

[Simon Glass]

Hi Sjoerd,

On 4 December 2015 at 15:27, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> To allow the various string to number conversion functions to be used
> when using tiny-printf,split them out into their own file which gets
> build regardless of what printf implementation is used.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> ---
>
>  lib/Makefile   |   6 +-
>  lib/strto.c    | 174 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/vsprintf.c | 164 -----------------------------------------------------
>  3 files changed, 177 insertions(+), 167 deletions(-)
>  create mode 100644 lib/strto.c
>

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

> diff --git a/lib/Makefile b/lib/Makefile
> index ae84833..dd36f25 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -85,13 +85,13 @@ obj-$(CONFIG_LIB_RAND) += rand.o
>  ifdef CONFIG_SPL_BUILD
>  # SPL U-Boot may use full-printf, tiny-printf or none at all
>  ifdef CONFIG_USE_TINY_PRINTF
> -obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o
> +obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o strto.o
>  else
> -obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o panic.o
> +obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o panic.o strto.o
>  endif
>  else
>  # Main U-Boot always uses the full printf support
> -obj-y += vsprintf.o panic.o
> +obj-y += vsprintf.o panic.o strto.o
>  endif

Can you just add this outside all the ifdef stuff:

obj-y += strto.o

>
>  subdir-ccflags-$(CONFIG_CC_OPTIMIZE_LIBS_FOR_SPEED) += -O2
> diff --git a/lib/strto.c b/lib/strto.c
> new file mode 100644
> index 0000000..a6c0157
> --- /dev/null
> +++ b/lib/strto.c
> @@ -0,0 +1,174 @@
> +/*
> + *  linux/lib/vsprintf.c
> + *
> + *  Copyright (C) 1991, 1992  Linus Torvalds
> + */
> +
> +/* vsprintf.c -- Lars Wirzenius & Linus Torvalds. */
> +/*
> + * Wirzenius wrote this portably, Torvalds fucked it up :-)
> + */

Please check this code came from Linux. Probably it did, just want to
make sure. The comment does not inspire confidence that it works!

> +
> +#include <common.h>
> +#include <errno.h>
> +#include <linux/ctype.h>
> +

[snip]

Regards,
Simon

[U-Boot] [PATCH 5/7] lib: split out strtoxxxx functions out of vsprintf.c

[Sjoerd Simons]

On Mon, 2015-12-07 at 17:39 -0700, Simon Glass wrote:
> Hi Sjoerd,
> 
> On 4 December 2015 at 15:27, Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
> > To allow the various string to number conversion functions to be
> > used
> > when using tiny-printf,split them out into their own file which
> > gets
> > build regardless of what printf implementation is used.
> > 
> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> > ---
> > 
> > ?lib/Makefile???|???6 +-
> > ?lib/strto.c????| 174
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > ?lib/vsprintf.c | 164 -------------------------------------------
> > ----------
> > ?3 files changed, 177 insertions(+), 167 deletions(-)
> > ?create mode 100644 lib/strto.c
> > 
> 
> Acked-by: Simon Glass <sjg@chromium.org>
> Tested on firefly:
> Tested-by: Simon Glass <sjg@chromium.org>
> 
> > diff --git a/lib/Makefile b/lib/Makefile
> > index ae84833..dd36f25 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -85,13 +85,13 @@ obj-$(CONFIG_LIB_RAND) += rand.o
> > ?ifdef CONFIG_SPL_BUILD
> > ?# SPL U-Boot may use full-printf, tiny-printf or none at all
> > ?ifdef CONFIG_USE_TINY_PRINTF
> > -obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o
> > +obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o strto.o
> > ?else
> > -obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o panic.o
> > +obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o panic.o strto.o
> > ?endif
> > ?else
> > ?# Main U-Boot always uses the full printf support
> > -obj-y += vsprintf.o panic.o
> > +obj-y += vsprintf.o panic.o strto.o
> > ?endif
> 
> Can you just add this outside all the ifdef stuff:
> 
> obj-y += strto.o

Same as with panic, it wasn't build before on SPLs without serial
support so i kept that behaviour. Though as I don't think any of the
strto functions depending on print functionality always building it may
make sense.

> > ?subdir-ccflags-$(CONFIG_CC_OPTIMIZE_LIBS_FOR_SPEED) += -O2
> > diff --git a/lib/strto.c b/lib/strto.c
> > new file mode 100644
> > index 0000000..a6c0157
> > --- /dev/null
> > +++ b/lib/strto.c
> > @@ -0,0 +1,174 @@
> > +/*
> > + *??linux/lib/vsprintf.c
> > + *
> > + *??Copyright (C) 1991, 1992??Linus Torvalds
> > + */
> > +
> > +/* vsprintf.c -- Lars Wirzenius & Linus Torvalds. */
> > +/*
> > + * Wirzenius wrote this portably, Torvalds fucked it up :-)
> > + */
> 
> Please check this code came from Linux. Probably it did, just want to
> make sure. The comment does not inspire confidence that it works!

See my reply to the previous patch :). the strtoxxx are more clearly
derived from Linux. And yeah, the linux code has the same copyright
header, but apparently it works for them... :p

> > +
> > +#include <common.h>
> > +#include <errno.h>
> > +#include <linux/ctype.h>
> > +
> 
> [snip]
> 
> Regards,
> Simon

-- 
Sjoerd Simons
Collabora Ltd.

[U-Boot] [PATCH 5/7] lib: split out strtoxxxx functions out of vsprintf.c

[Simon Glass]

Applied to u-boot-rockchip, thanks!

[U-Boot] [PATCH 6/7] mmc: mmc: Don't use sprintf when using tiny-printf

[Sjoerd Simons]

There is no sprintf implementation in tiny-printf, so don't try to use
it when tiny-printf if used.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---

 drivers/mmc/mmc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 2a58702..3a34028 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -1469,7 +1469,9 @@ static int mmc_startup(struct mmc *mmc)
 	mmc->block_dev.blksz = mmc->read_bl_len;
 	mmc->block_dev.log2blksz = LOG2(mmc->block_dev.blksz);
 	mmc->block_dev.lba = lldiv(mmc->capacity, mmc->read_bl_len);
-#if !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_LIBCOMMON_SUPPORT)
+#if !defined(CONFIG_SPL_BUILD) || \
+		(defined(CONFIG_SPL_LIBCOMMON_SUPPORT) && \
+		!defined(CONFIG_USE_TINY_PRINTF))
 	sprintf(mmc->block_dev.vendor, "Man %06x Snr %04x%04x",
 		mmc->cid[0] >> 24, (mmc->cid[2] & 0xffff),
 		(mmc->cid[3] >> 16) & 0xffff);
-- 
2.6.2

[U-Boot] [PATCH 6/7] mmc: mmc: Don't use sprintf when using tiny-printf

[Simon Glass]

On 4 December 2015 at 15:27, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> There is no sprintf implementation in tiny-printf, so don't try to use
> it when tiny-printf if used.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> ---
>
>  drivers/mmc/mmc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

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

I can't help thinking that sprintf() would not add a lot more code...

- Simon

[U-Boot] [PATCH 6/7] mmc: mmc: Don't use sprintf when using tiny-printf

[Sjoerd Simons]

On Mon, 2015-12-07 at 17:40 -0700, Simon Glass wrote:
> On 4 December 2015 at 15:27, Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
> > There is no sprintf implementation in tiny-printf, so don't try to
> > use
> > it when tiny-printf if used.
> > 
> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> > ---
> > 
> > ?drivers/mmc/mmc.c | 4 +++-
> > ?1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Acked-by: Simon Glass <sjg@chromium.org>
> Tested on firefly:
> Tested-by: Simon Glass <sjg@chromium.org>
> 
> I can't help thinking that sprintf() would not add a lot more code...

Probably not (it's write to buffer rather then write to output, so
maybe just passing a function pointer for an out function in would just
do the job). More work then I had time for though :/

Though i'm not sure it's worth it though (at least on firefly the spl
won't output those fields) :)

-- 
Sjoerd Simons
Collabora Ltd.

[U-Boot] [PATCH 6/7] mmc: mmc: Don't use sprintf when using tiny-printf

[Simon Glass]

Applied to u-boot-rockchip, thanks!

[U-Boot] [PATCH 7/7] rockchip: firefly: Use tiny-printf

[Sjoerd Simons]

Switch to using tiny-printf for the firefly SPL, this reduces the SPL by
around 1800 bytes bringing it back under the 32k limit for both gcc 4.9
and gcc 5.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>

---

 configs/firefly-rk3288_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/firefly-rk3288_defconfig b/configs/firefly-rk3288_defconfig
index b645af5..be4ca88 100644
--- a/configs/firefly-rk3288_defconfig
+++ b/configs/firefly-rk3288_defconfig
@@ -42,5 +42,6 @@ CONFIG_DEBUG_UART_CLOCK=24000000
 CONFIG_DEBUG_UART_SHIFT=2
 CONFIG_SYS_NS16550=y
 CONFIG_USE_PRIVATE_LIBGCC=y
+CONFIG_USE_TINY_PRINTF=y
 CONFIG_CMD_DHRYSTONE=y
 CONFIG_ERRNO_STR=y
-- 
2.6.2

[U-Boot] [PATCH 7/7] rockchip: firefly: Use tiny-printf

[Simon Glass]

On 4 December 2015 at 15:27, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> Switch to using tiny-printf for the firefly SPL, this reduces the SPL by
> around 1800 bytes bringing it back under the 32k limit for both gcc 4.9
> and gcc 5.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>
> ---
>
>  configs/firefly-rk3288_defconfig | 1 +
>  1 file changed, 1 insertion(+)

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

[U-Boot] [PATCH 7/7] rockchip: firefly: Use tiny-printf

[Simon Glass]

Applied to u-boot-rockchip, thanks!

[U-Boot] [PATCH 0/7] Switch rockchip firefly to using tiny-printf

[Stefan Roese]

On 04.12.2015 23:27, Sjoerd Simons wrote:
>
> The Rockchip rk3288 SPL was always too close to the 32k limit, either
> needing gcc 5 or a patched gcc (with some constant string GC fixes) to
> actually stay (just) below 32k. With recent changes, it unfortunatly
> went over with common gcc versions.
>
> This serie switches the firefly SPL to use tiny-printf instead of the
> printf from vsprint, saving around 1800 bytes in the final binary to
> bring it under the limit with a bit more margin again.

Thanks for working on this. Whole series:

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH 0/7] Switch rockchip firefly to using tiny-printf

[Simon Glass]

Hi Tom,

On 5 December 2015 at 03:00, Stefan Roese <sr@denx.de> wrote:
> On 04.12.2015 23:27, Sjoerd Simons wrote:
>>
>>
>> The Rockchip rk3288 SPL was always too close to the 32k limit, either
>> needing gcc 5 or a patched gcc (with some constant string GC fixes) to
>> actually stay (just) below 32k. With recent changes, it unfortunatly
>> went over with common gcc versions.
>>
>> This serie switches the firefly SPL to use tiny-printf instead of the
>> printf from vsprint, saving around 1800 bytes in the final binary to
>> bring it under the limit with a bit more margin again.
>
>
> Thanks for working on this. Whole series:
>
> Reviewed-by: Stefan Roese <sr@denx.de>

This looks good to me. It does fix Firefly with the bugged gcc.

Tom, would you be OK with me picking this up for this release? I also
need to do a proper review...

Regards,
Simon

[U-Boot] [PATCH 0/7] Switch rockchip firefly to using tiny-printf

[Tom Rini]

On Mon, Dec 07, 2015 at 02:43:24PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On 5 December 2015 at 03:00, Stefan Roese <sr@denx.de> wrote:
> > On 04.12.2015 23:27, Sjoerd Simons wrote:
> >>
> >>
> >> The Rockchip rk3288 SPL was always too close to the 32k limit, either
> >> needing gcc 5 or a patched gcc (with some constant string GC fixes) to
> >> actually stay (just) below 32k. With recent changes, it unfortunatly
> >> went over with common gcc versions.
> >>
> >> This serie switches the firefly SPL to use tiny-printf instead of the
> >> printf from vsprint, saving around 1800 bytes in the final binary to
> >> bring it under the limit with a bit more margin again.
> >
> >
> > Thanks for working on this. Whole series:
> >
> > Reviewed-by: Stefan Roese <sr@denx.de>
> 
> This looks good to me. It does fix Firefly with the bugged gcc.
> 
> Tom, would you be OK with me picking this up for this release? I also
> need to do a proper review...

Yes, that's fine, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151207/5a0eeee9/attachment.sig>