Skip to contentSkip to conversation

Switch rockchip firefly to using tiny-printf

PATCH New Sjoerd Simons 5 participants 32 messages Dec 4, 2015 – Dec 14, 2015

Subsystems (3)
Library code MMC, SD and SDIO Vsprintf
lore
Share & tools
lore

Sjoerd Simons

author
#0 lore raw

cover Switch rockchip firefly to using tiny-printf

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
Signature · 1 line
2.6.2
author
#1 lore raw

PATCH 1/7 spl: use panic_str instead of panic

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

Signed-off-by Sjoerd Simons <[email protected]>

---

 common/spl/spl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
common/spl/spl.c +1 -1
index 7a393dc..6e6dee7 100644
@@ -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;
Signature · 1 line
2.6.2
#3 lore raw

PATCH 1/7 spl: use panic_str instead of panic

Quoted 6 lines · Simon Glass
Quoted 3 lines · Sjoerd Simons

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

Quoted 7 lines
Quoted 5 lines
Signed-off-by Sjoerd Simons <[email protected]>

---

 common/spl/spl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by Simon Glass <[email protected]>
Acked-by Simon Glass <[email protected]>

Tested on firefly:

Tested-by Simon Glass <[email protected]>
#4 lore raw

PATCH 1/7 spl: use panic_str instead of panic

Applied to u-boot-rockchip, thanks!

author
#5 lore raw

PATCH 2/7 spl: mmc: Explicitly init mmc struct

In reply to 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 <[email protected]>

---

 common/spl/spl_mmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
common/spl/spl_mmc.c +1 -1
index b3c2c64..43748d0 100644
@@ -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;
Signature · 1 line
2.6.2
#6 lore raw

PATCH 2/7 spl: mmc: Explicitly init mmc struct

Hi Sjoerd,

Quoted 8 lines · 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);
                        ^
Quoted 7 lines

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 <[email protected]>

---

See:

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

Regards,
Simon

author
#7 lore raw

PATCH 3/7 lib/tiny-printf.c: Implement vprintf

In reply to 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 <[email protected]>

---

 lib/tiny-printf.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)
lib/tiny-printf.c +13 -5
index 6766a8f..403b134 100644
@@ -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;
+}
Signature · 1 line
2.6.2
#9 lore raw

PATCH 3/7 lib/tiny-printf.c: Implement vprintf

Applied to u-boot-rockchip, thanks!

author
#10 lore raw

PATCH 4/7 lib: Split panic functions out of vsprintf.c

In reply to 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 <[email protected]>

---

 lib/Makefile   |  6 +++---
 lib/panic.c    | 45 +++++++++++++++++++++++++++++++++++++++++++++
 lib/vsprintf.c | 29 -----------------------------
 3 files changed, 48 insertions(+), 32 deletions(-)
 create mode 100644 lib/panic.c
lib/Makefile +3 -3
index 1f1ff6f..ae84833 100644
@@ -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
lib/panic.c new file +45 -0

45 lines not on this page · raw message

lib/vsprintf.c +0 -29
index dd8380b..bf5fd01 100644
@@ -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)
Signature · 1 line
2.6.2
#11 lore raw

PATCH 4/7 lib: Split panic functions out of vsprintf.c

Hi Sjoerd,

Quoted 4 lines · 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.

Quoted 8 lines
Signed-off-by Sjoerd Simons <[email protected]>

---

 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 <[email protected]>
Quoted 10 lines
lib/Makefile +0 -1
index 1f1ff6f..ae84833 100644
@@ -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

lib/Makefile obj-$(CONFIG_LIB_RAND) += rand.o · line 95

Quoted 10 lines
lib/Makefile +3 -2
+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

lib/panic.c line 2

Quoted 10 lines
lib/Makefile +0 -0
 subdir-ccflags-$(CONFIG_CC_OPTIMIZE_LIBS_FOR_SPEED) += -O2
lib/panic.c new file +2 -0
new file mode 100644
index 0000000..e2b8b74
@@ -0,0 +1,45 @@
+/*
+ *  linux/lib/vsprintf.c

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

lib/panic.c line 10

Quoted 8 lines
lib/panic.c new file +8 -0
+ *
+ *  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?

Quoted 76 lines
lib/panic.c new file +35 -0
+
+#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();
+}
lib/vsprintf.c +0 -30
index dd8380b..bf5fd01 100644
@@ -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)
--
Quoted 2 lines

2.6.2

Regards,
Simon

author
#12 lore raw

PATCH 4/7 lib: Split panic functions out of vsprintf.c

Quoted 22 lines · Simon Glass
Quoted 18 lines
lib/Makefile +0 -0
index 1f1ff6f..ae84833 100644
@@ -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

lib/Makefile +1 -1
-obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o
+obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o

?else

lib/Makefile +1 -1
-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

lib/Makefile +1 -1
-obj-y += vsprintf.o
+obj-y += vsprintf.o panic.o
Quoted 1 line

?endif

Why not just add this outside all the ifdef stuff:

Quoted 1 line

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 ?

Quoted 11 lines

lib/panic.c line 2

Quoted 9 lines

?subdir-ccflags-$(CONFIG_CC_OPTIMIZE_LIBS_FOR_SPEED) += -O2

lib/panic.c new file +2 -0
new file mode 100644
index 0000000..e2b8b74
@@ -0,0 +1,45 @@
+/*
+ *??linux/lib/vsprintf.c

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

Sure it's pointless anyway

Quoted 11 lines

lib/panic.c line 10

Quoted 8 lines
lib/panic.c new file +8 -0
+ *
+ *??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 :)

Signature · 2 lines
Sjoerd Simons
Collabora Ltd.
#13 lore raw

PATCH 4/7 lib: Split panic functions out of vsprintf.c

Hi Sjoerd,

Quoted 24 lines · Sjoerd Simons
Quoted 22 lines · Simon Glass
Quoted 9 lines
lib/Makefile +0 -1
index 1f1ff6f..ae84833 100644
@@ -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

lib/Makefile obj-$(CONFIG_LIB_RAND) += rand.o · line 95

Quoted 10 lines
lib/Makefile +3 -2
+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:

Quoted 5 lines
Quoted 1 line

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?

Quoted 49 lines
Quoted 12 lines

lib/panic.c line 2

Quoted 10 lines
lib/Makefile +0 -0
 subdir-ccflags-$(CONFIG_CC_OPTIMIZE_LIBS_FOR_SPEED) += -O2
lib/panic.c new file +2 -0
new file mode 100644
index 0000000..e2b8b74
@@ -0,0 +1,45 @@
+/*
+ *  linux/lib/vsprintf.c

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

Sure it's pointless anyway

Quoted 11 lines

lib/panic.c line 10

Quoted 8 lines
lib/panic.c new file +8 -0
+ *
+ *  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 :)

Quoted 3 lines

--
Sjoerd Simons
Collabora Ltd.

Regards,
Simon

#14 lore raw

PATCH 4/7 lib: Split panic functions out of vsprintf.c

Quoted 49 lines · Simon Glass

Hi Sjoerd,

Quoted 24 lines · Sjoerd Simons
Quoted 22 lines · Simon Glass
Quoted 9 lines
lib/Makefile +0 -1
index 1f1ff6f..ae84833 100644
@@ -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

lib/Makefile obj-$(CONFIG_LIB_RAND) += rand.o · line 95

Quoted 10 lines
lib/Makefile +3 -2
+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:

Quoted 5 lines
Quoted 1 line

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

Quoted 1 line

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

The dependency on CONFIG_SPL_SERIAL_SUPPORT.

-Scott

#15 lore raw

PATCH 4/7 lib: Split panic functions out of vsprintf.c

Hi Scott,

Quoted 50 lines · Scott Wood
Quoted 49 lines · Simon Glass

Hi Sjoerd,

Quoted 24 lines · Sjoerd Simons
Quoted 22 lines · Simon Glass
Quoted 9 lines
lib/Makefile +0 -1
index 1f1ff6f..ae84833 100644
@@ -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

lib/Makefile obj-$(CONFIG_LIB_RAND) += rand.o · line 95

Quoted 10 lines
lib/Makefile +3 -2
+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:

Quoted 5 lines
Quoted 1 line

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

Quoted 3 lines
Quoted 1 line

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

The dependency on CONFIG_SPL_SERIAL_SUPPORT.

OK, so how about this:

Quoted 2 lines
Quoted 2 lines

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

Quoted 1 line
Quoted 1 line

ifdef CONFIG_USE_TINY_PRINTF

obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o

Quoted 1 line
Quoted 1 line

else

obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o

Quoted 5 lines
Quoted 5 lines

endif
else
# Main U-Boot always uses the full printf support
obj-y += vsprintf.o panic.o strto.o
endif

Regards,
Simon

#16 lore raw

PATCH 4/7 lib: Split panic functions out of vsprintf.c

Quoted 62 lines · Simon Glass

Hi Scott,

Quoted 50 lines · Scott Wood
Quoted 49 lines · Simon Glass

Hi Sjoerd,

Quoted 24 lines · Sjoerd Simons
Quoted 22 lines · Simon Glass
Quoted 9 lines
lib/Makefile +0 -1
index 1f1ff6f..ae84833 100644
@@ -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

lib/Makefile obj-$(CONFIG_LIB_RAND) += rand.o · line 95

Quoted 10 lines
lib/Makefile +3 -2
+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:

Quoted 5 lines
Quoted 1 line

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

Quoted 3 lines
Quoted 1 line

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

The dependency on CONFIG_SPL_SERIAL_SUPPORT.

OK, so how about this:

Quoted 2 lines
Quoted 2 lines

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

Quoted 11 lines

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

Quoted 1 line
Quoted 1 line

ifdef CONFIG_USE_TINY_PRINTF

obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o

Quoted 1 line
Quoted 1 line

else

obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o

Quoted 5 lines
Quoted 5 lines

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!

author
#17 lore raw

PATCH 4/7 lib: Split panic functions out of vsprintf.c

Quoted 25 lines · Simon Glass
Quoted 12 lines · Simon Glass

Hi Scott,

Quoted 3 lines · Scott Wood
Quoted 2 lines · Simon Glass

Hi Sjoerd,
?

OK, so how about this:

Quoted 2 lines
Quoted 2 lines

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

Quoted 11 lines

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

Quoted 1 line
Quoted 1 line

ifdef CONFIG_USE_TINY_PRINTF

obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o

Quoted 1 line
Quoted 1 line

else

obj-$(CONFIG_SPL_SERIAL_SUPPORT) += vsprintf.o

Quoted 5 lines
Quoted 5 lines

endif
else
# Main U-Boot always uses the full printf support
obj-y += vsprintf.o panic.o strto.o
endif

Quoted 3 lines

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).

Signature · 2 lines
Sjoerd Simons
Collabora Ltd.
author
#18 lore raw

PATCH 5/7 lib: split out strtoxxxx functions out of vsprintf.c

In reply to 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 <[email protected]>

---

 lib/Makefile   |   6 +-
 lib/strto.c    | 174 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/vsprintf.c | 164 -----------------------------------------------------
 3 files changed, 177 insertions(+), 167 deletions(-)
 create mode 100644 lib/strto.c
lib/Makefile +3 -3
index ae84833..dd36f25 100644
@@ -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
lib/strto.c new file +174 -0

174 lines not on this page · raw message

lib/vsprintf.c +0 -164

176 lines not on this page · raw message

Signature · 1 line
2.6.2
#19 lore raw

PATCH 5/7 lib: split out strtoxxxx functions out of vsprintf.c

Hi Sjoerd,

Quoted 4 lines · 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.

Quoted 9 lines
Signed-off-by Sjoerd Simons <[email protected]>

---

 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 <[email protected]>

Tested on firefly:

Tested-by Simon Glass <[email protected]>
Quoted 9 lines
lib/Makefile +0 -1
index ae84833..dd36f25 100644
@@ -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

lib/Makefile obj-$(CONFIG_LIB_RAND) += rand.o · line 95

Quoted 10 lines
lib/Makefile +3 -2
+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

Quoted 8 lines
lib/Makefile +0 -0
 subdir-ccflags-$(CONFIG_CC_OPTIMIZE_LIBS_FOR_SPEED) += -O2
lib/strto.c new file +0 -0
new file mode 100644
index 0000000..a6c0157

lib/strto.c line 10

Quoted 10 lines
lib/strto.c new file +10 -0
@@ -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!

lib/strto.c line 15

Quoted 5 lines
lib/strto.c new file +5 -0
+
+#include <common.h>
+#include <errno.h>
+#include <linux/ctype.h>
+

[snip]

Regards,
Simon

author
#20 lore raw

PATCH 5/7 lib: split out strtoxxxx functions out of vsprintf.c

Quoted 48 lines · Simon Glass

Hi Sjoerd,

Quoted 9 lines · 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 <[email protected]>

---

Quoted 8 lines

?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 <[email protected]>

Tested on firefly:

Tested-by Simon Glass <[email protected]>
Quoted 18 lines
lib/Makefile +0 -0
index ae84833..dd36f25 100644
@@ -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

lib/Makefile +1 -1
-obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o
+obj-$(CONFIG_SPL_SERIAL_SUPPORT) += tiny-printf.o panic.o strto.o

?else

lib/Makefile +1 -1
-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

lib/Makefile +1 -1
-obj-y += vsprintf.o panic.o
+obj-y += vsprintf.o panic.o strto.o
Quoted 1 line

?endif

Can you just add this outside all the ifdef stuff:

Quoted 1 line

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.

Quoted 12 lines
Quoted 12 lines

?subdir-ccflags-$(CONFIG_CC_OPTIMIZE_LIBS_FOR_SPEED) += -O2

lib/strto.c new file +5 -0
new file mode 100644
index 0000000..a6c0157
@@ -0,0 +1,174 @@
+/*
+ *??linux/lib/vsprintf.c
+ *
+ *??Copyright (C) 1991, 1992??Linus Torvalds
+ */
Quoted 8 lines

lib/strto.c line 10

Quoted 5 lines
lib/strto.c new file +5 -0
+
+/* 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

Quoted 10 lines

lib/strto.c line 15

Quoted 5 lines
lib/strto.c new file +5 -0
+
+#include <common.h>
+#include <errno.h>
+#include <linux/ctype.h>
+

[snip]

Regards,
Simon

Signature · 2 lines
Sjoerd Simons
Collabora Ltd.
#21 lore raw

PATCH 5/7 lib: split out strtoxxxx functions out of vsprintf.c

Applied to u-boot-rockchip, thanks!

author
#22 lore raw

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

In reply to 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 <[email protected]>

---

 drivers/mmc/mmc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/mmc/mmc.c +3 -1
index 2a58702..3a34028 100644
@@ -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);
Signature · 1 line
2.6.2
#23 lore raw

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

Quoted 8 lines · 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 <[email protected]>

---

 drivers/mmc/mmc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Acked-by Simon Glass <[email protected]>

Tested on firefly:

Tested-by Simon Glass <[email protected]>

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

- Simon

author
#24 lore raw

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

Quoted 9 lines · Simon Glass
Quoted 7 lines · 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 <[email protected]>

---

Quoted 8 lines
Quoted 2 lines

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

Acked-by Simon Glass <[email protected]>

Tested on firefly:

Tested-by Simon Glass <[email protected]>

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) :)

Signature · 2 lines
Sjoerd Simons
Collabora Ltd.
#25 lore raw

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

Applied to u-boot-rockchip, thanks!

author
#26 lore raw

PATCH 7/7 rockchip: firefly: Use tiny-printf

In reply to 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 <[email protected]>

---

 configs/firefly-rk3288_defconfig | 1 +
 1 file changed, 1 insertion(+)
configs/firefly-rk3288_defconfig +1 -0
index b645af5..be4ca88 100644
@@ -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
Signature · 1 line
2.6.2
#27 lore raw

PATCH 7/7 rockchip: firefly: Use tiny-printf

Quoted 10 lines · 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 <[email protected]>

---

 configs/firefly-rk3288_defconfig | 1 +
 1 file changed, 1 insertion(+)
Acked-by Simon Glass <[email protected]>

Tested on firefly:

Tested-by Simon Glass <[email protected]>
#28 lore raw

PATCH 7/7 rockchip: firefly: Use tiny-printf

Applied to u-boot-rockchip, thanks!

#29 lore raw

cover Switch rockchip firefly to using tiny-printf

In reply to Sjoerd Simons
Quoted 9 lines · 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.

Thanks for working on this. Whole series:

Reviewed-by Stefan Roese <[email protected]>

Thanks,
Stefan

#30 lore raw

cover Switch rockchip firefly to using tiny-printf

Hi Tom,

Quoted 8 lines · Stefan Roese
Quoted 7 lines · 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.

Quoted 8 lines
Quoted 3 lines

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 <[email protected]>

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

#31 lore raw

cover Switch rockchip firefly to using tiny-printf

Quoted 16 lines · Simon Glass

Hi Tom,

Quoted 13 lines · Stefan Roese
Quoted 10 lines · 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.

Quoted 8 lines
Quoted 3 lines

Thanks for working on this. Whole series:

Reviewed-by Stefan Roese <[email protected]>

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!

Signature · 8 lines
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>