[U-Boot] [RFC PATCH] lib/tiny-printf.c: Add tiny printf function for space limited environments
Stefan Roese
sr at denx.de
Mon Nov 16 11:29:02 CET 2015
Hi Simon,
On 14.11.2015 03:04, Simon Glass wrote:
> Hi Stefan,
>
> On 11 November 2015 at 07:25, Stefan Roese <sr at denx.de> wrote:
>> This patch adds a small printf() version that supports all basic formats.
>> Its intented to be used in U-Boot SPL versions on platforms with very
>> limited internal RAM sizes.
>>
>> To enable it, just define CONFIG_USE_TINY_PRINTF in your defconfig. This
>> will result in the SPL using this tiny function and the main U-Boot
>> still using the full-blown printf() function.
>>
>> This code was copied from:
>> http://www.sparetimelabs.com/printfrevisited
>> With mostly only coding style related changes so that its checkpatch
>> clean.
>>
>> The size reduction is about 2.5KiB. Here a comparison for the db-88f6820-gp
>> (Marvell A38x) SPL:
>>
>> Without this patch:
>> 108075 13298 2824 124197 1e525 ./spl/u-boot-spl
>>
>> With this patch:
>> 105398 13298 2852 121548 1dacc ./spl/u-boot-spl
>
> Great!
Thanks. BTW, you might have noticed that I still used MAKEALL
for this size outputs here. I somehow failed to get buildman
generate these size statistic outputs here. Sometimes buildman
starts with the compilation, sometimes it only shows a summary,
not really the one that I expected it to show. Here a short
log of some experiments:
[stefan at stefan-work u-boot (tiny-printf-v2-2015-11-16)]$ ./tools/buildman/buildman db-mv784mp-gp
boards.cfg is up to date. Nothing to do.
Building current source for 1 boards (1 thread, 8 jobs per thread)
1 0 0 /1 db-mv784mp-gp
[stefan at stefan-work u-boot (tiny-printf-v2-2015-11-16)]$ ./tools/buildman/buildman db-mv784mp-gp -s
boards.cfg is up to date. Nothing to do.
Summary of current source for 1 boards (1 thread, 8 jobs per thread)
(no errors to report)
[stefan at stefan-work u-boot (tiny-printf-v2-2015-11-16)]$ ./tools/buildman/buildman db-mv784mp-gp -s -c 2
boards.cfg is up to date. Nothing to do.
Summary of current source for 1 boards (1 thread, 8 jobs per thread)
(no errors to report)
[stefan at stefan-work u-boot (tiny-printf-v2-2015-11-16)]$ ./tools/buildman/buildman db-mv784mp-gp -s -c 5
boards.cfg is up to date. Nothing to do.
Summary of current source for 1 boards (1 thread, 8 jobs per thread)
(no errors to report)
[stefan at stefan-work u-boot (tiny-printf-v2-2015-11-16)]$ ./tools/buildman/buildman db-mv784mp-gp -sS -c 5
boards.cfg is up to date. Nothing to do.
Summary of current source for 1 boards (1 thread, 8 jobs per thread)
(no errors to report)
[stefan at stefan-work u-boot (tiny-printf-v2-2015-11-16)]$ ./tools/buildman/buildman mvebu -sS -c 5
boards.cfg is up to date. Nothing to do.
Summary of current source for 4 boards (4 threads, 2 jobs per thread)
arm: + clearfog
[stefan at stefan-work u-boot (tiny-printf-v2-2015-11-16)]$ ./tools/buildman/buildman mvebu -sS -c 10
boards.cfg is up to date. Nothing to do.
Summary of current source for 4 boards (4 threads, 2 jobs per thread)
arm: + clearfog
I'm most likely missing something. Perhaps using a branch name,
not sure. I also experimented with using -b <branch> and get
here:
$ ./tools/buildman/buildman -b tiny-printf-v2-2015-11-16 mvebu
Cannot find a suitable upstream for branch 'tiny-printf-v2-2015-11-16'
which makes perfect sense. As this is absolute work-in-progess
and I really don't want to push this into some upstream
repo at this point.
I've also noticed the comments in the README about this branch
usage (e.g. 'git branch --set-upstream-to upstream/master'). But
I assume that there has to be the possibility to use buildman
without pushing such working branches upstream.
So please let me know what I'm missing here. Thanks!
Some more comments below.
>>
>> Note:
>> To make it possible to compile tiny-printf.c instead of vsprintf.c when
>> CONFIG_USE_TINY_PRINTF is defined, the functions printf() and vprintf() are
>> moved from common/console.c into vsprintf.c in this patch.
>>
>> Signed-off-by: Stefan Roese <sr at denx.de>
>> Cc: Simon Glass <sjg at chromium.org>
>> Cc: Hans de Goede <hdegoede at redhat.com>
>> Cc: Tom Rini <trini at konsulko.com>
>> ---
>> common/console.c | 39 ----------------
>> lib/Kconfig | 8 ++++
>> lib/Makefile | 7 ++-
>> lib/tiny-printf.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> lib/vsprintf.c | 39 ++++++++++++++++
>> 5 files changed, 183 insertions(+), 40 deletions(-)
>> create mode 100644 lib/tiny-printf.c
>>
>> diff --git a/common/console.c b/common/console.c
>> index ace206c..2bfd59f 100644
>> --- a/common/console.c
>> +++ b/common/console.c
>> @@ -535,45 +535,6 @@ void puts(const char *s)
>> }
>> }
>>
>> -int printf(const char *fmt, ...)
>> -{
>> - va_list args;
>> - uint i;
>> - char printbuffer[CONFIG_SYS_PBSIZE];
>> -
>> - va_start(args, fmt);
>> -
>> - /* For this to work, printbuffer must be larger than
>> - * anything we ever want to print.
>> - */
>> - i = vscnprintf(printbuffer, sizeof(printbuffer), fmt, args);
>> - va_end(args);
>> -
>> - /* Print the string */
>> - puts(printbuffer);
>> - return i;
>> -}
>> -
>> -int vprintf(const char *fmt, va_list args)
>> -{
>> - uint i;
>> - char printbuffer[CONFIG_SYS_PBSIZE];
>> -
>> -#if defined(CONFIG_PRE_CONSOLE_BUFFER) && !defined(CONFIG_SANDBOX)
>> - if (!gd->have_console)
>> - return 0;
>> -#endif
>> -
>> - /* For this to work, printbuffer must be larger than
>> - * anything we ever want to print.
>> - */
>> - i = vscnprintf(printbuffer, sizeof(printbuffer), fmt, args);
>> -
>> - /* Print the string */
>> - puts(printbuffer);
>> - return i;
>> -}
>> -
>> /* test if ctrl-c was pressed */
>> static int ctrlc_disabled = 0; /* see disable_ctrl() */
>> static int ctrlc_was_pressed = 0;
>> diff --git a/lib/Kconfig b/lib/Kconfig
>> index 30e84ed..faf3de3 100644
>> --- a/lib/Kconfig
>> +++ b/lib/Kconfig
>> @@ -36,6 +36,14 @@ config SYS_VSNPRINTF
>> Thumb-2, about 420 bytes). Enable this option for safety when
>> using sprintf() with data you do not control.
>>
>> +config USE_TINY_PRINTF
>> + bool "Enable tiny printf() version"
>> + help
>> + This option enables a tiny, stripped down printf version.
>> + This should only be used in space limited environments,
>> + like SPL versions with hard memory limits. This version
>> + reduces the code size by about 2.5KiB on armv7.
>> +
>> config REGEX
>> bool "Enable regular expression support"
>> default y if NET
>> diff --git a/lib/Makefile b/lib/Makefile
>> index 3eecefa..54b6555 100644
>> --- a/lib/Makefile
>> +++ b/lib/Makefile
>> @@ -79,7 +79,12 @@ obj-y += string.o
>> obj-y += time.o
>> obj-$(CONFIG_TRACE) += trace.o
>> obj-$(CONFIG_LIB_UUID) += uuid.o
>> -obj-y += vsprintf.o
>> obj-$(CONFIG_LIB_RAND) += rand.o
>>
>> +ifeq ($(CONFIG_SPL_BUILD)$(CONFIG_USE_TINY_PRINTF),yy)
>> +obj-y += tiny-printf.o
>> +else
>> +obj-y += vsprintf.o
>> +endif
>> +
>> subdir-ccflags-$(CONFIG_CC_OPTIMIZE_LIBS_FOR_SPEED) += -O2
>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
>> new file mode 100644
>> index 0000000..d743a36
>> --- /dev/null
>> +++ b/lib/tiny-printf.c
>> @@ -0,0 +1,130 @@
>> +/*
>> + * Tiny printf version for SPL
>> + *
>> + * Copied from:
>> + * http://www.sparetimelabs.com/printfrevisited/printfrevisited.php
>> + *
>> + * Copyright (C) 2004,2008 Kustaa Nyholm
>> + *
>> + * SPDX-License-Identifier: LGPL-2.1+
>> + */
>> +
>> +#include <common.h>
>> +#include <stdarg.h>
>> +#include <serial.h>
>> +
>> +static char *bf;
>> +static char buf[12];
>> +static unsigned int num;
>> +static char uc;
>> +static char zs;
>
> Do we need all these variables? Perhaps some of them could become
> parameters instead?
As mentioned in the commit text, this patch just includes the
original code as published on the link above. With only some
coding style related changes.
I'll take a closer look at the functional changes later and will
perhaps send a follow-up patch for this.
>> +
>> +static void out(char c)
>> +{
>> + *bf++ = c;
>> +}
>> +
>> +static void out_dgt(char dgt)
>> +{
>> + out(dgt + (dgt < 10 ? '0' : (uc ? 'A' : 'a') - 10));
>> + zs = 1;
>> +}
>> +
>> +static void div_out(unsigned int div)
>> +{
>> + unsigned char dgt = 0;
>> +
>> + num &= 0xffff; /* just for testing the code with 32 bit ints */
>> + while (num >= div) {
>> + num -= div;
>> + dgt++;
>> + }
>> +
>> + if (zs || dgt > 0)
>> + out_dgt(dgt);
>> +}
>> +
>> +int printf(const char *fmt, ...)
>> +{
>> + va_list va;
>> + char ch;
>> + char *p;
>> +
>> + va_start(va, fmt);
>> +
>> + while ((ch = *(fmt++))) {
>> + if (ch != '%') {
>> + putc(ch);
>> + } else {
>> + char lz = 0;
>> + char w = 0;
>> +
>> + ch = *(fmt++);
>> + if (ch == '0') {
>> + ch = *(fmt++);
>> + lz = 1;
>> + }
>> +
>> + if (ch >= '0' && ch <= '9') {
>> + w = 0;
>> + while (ch >= '0' && ch <= '9') {
>> + w = (((w << 2) + w) << 1) + ch - '0';
>
> Doesn't the compiler do the right thing if you say:
>
> w = (w * 10) + ch - '0'
I'll test this.
>> + ch = *fmt++;
>> + }
>> + }
>> + bf = buf;
>> + p = bf;
>> + zs = 0;
>> +
>> + switch (ch) {
>> + case 0:
>> + goto abort;
>> + case 'u':
>> + case 'd':
>> + num = va_arg(va, unsigned int);
>> + if (ch == 'd' && (int)num < 0) {
>> + num = -(int)num;
>> + out('-');
>> + }
>> + div_out(10000);
>
> Does this mean it cannot output numbers larger than 99999?
Yes. Seems that the original version only supports numbers
up to 0xffff. I'll change this also in a follow-up patch.
>> + div_out(1000);
>> + div_out(100);
>> + div_out(10);
>> + out_dgt(num);
>> + break;
>> + case 'x':
>> + case 'X':
>> + uc = ch == 'X';
>> + num = va_arg(va, unsigned int);
>> + div_out(0x1000);
>
> How about:
>
> for (div = 0x1000; div; div >>= 4)
> div_out(div)
Yes.
>> + div_out(0x100);
>> + div_out(0x10);
>> + out_dgt(num);
>> + break;
>> + case 'c':
>> + out((char)(va_arg(va, int)));
>> + break;
>> + case 's':
>> + p = va_arg(va, char*);
>> + break;
>> + case '%':
>> + out('%');
>> + default:
>> + break;
>> + }
>> +
>> + *bf = 0;
>> + bf = p;
>> + while (*bf++ && w > 0)
>> + w--;
>
> I wonder if the digits could be written to the buffer in reverse
> order, thus allowing something like this for the decimal case:
>
> for (div = 10; div <= 10000; div *= 10)
> div_out(div)
>
>> + while (w-- > 0)
>> + putc(lz ? '0' : ' ');
>> + while ((ch = *p++))
>
> and here you could work backwards.
I'll also take a look at this.
>> + putc(ch);
>> + }
>> + }
>> +
>> +abort:
>> + va_end(va);
>> + return 0;
>> +}
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index 4c82837..8cc5b38 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -861,6 +861,45 @@ int sprintf(char *buf, const char *fmt, ...)
>> return i;
>> }
>>
>> +int printf(const char *fmt, ...)
>> +{
>> + va_list args;
>> + uint i;
>> + char printbuffer[CONFIG_SYS_PBSIZE];
>> +
>> + va_start(args, fmt);
>> +
>> + /* For this to work, printbuffer must be larger than
>> + * anything we ever want to print.
>> + */
>> + i = vscnprintf(printbuffer, sizeof(printbuffer), fmt, args);
>> + va_end(args);
>> +
>> + /* Print the string */
>> + puts(printbuffer);
>> + return i;
>> +}
>> +
>> +int vprintf(const char *fmt, va_list args)
>> +{
>> + uint i;
>> + char printbuffer[CONFIG_SYS_PBSIZE];
>> +
>> +#if defined(CONFIG_PRE_CONSOLE_BUFFER) && !defined(CONFIG_SANDBOX)
>> + if (!gd->have_console)
>> + return 0;
>> +#endif
>
> Hmm, that code in the #if should be removed. I did it for printf() but
> forgot vprintf(). If you have time you could add a patch for this.
Yes, will do.
>> +
>> + /* For this to work, printbuffer must be larger than
>
> /*
> * For this to work...
> */
Its just a copy of the original code. But I can clean this up as
well while moving, yes.
Thanks,
Stefan
More information about the U-Boot
mailing list