[U-Boot] [PATCH v2] tiny-printf: Add support for %p format

Add support for %p, %pa[p], %pM, %pm and %pI4 formats to tiny-printf. %pM and %pI4 are widely used by SPL networking stack and is required if networking support is desired in SPL. %p, %pa and %pap are mostly used by debug prints and hence supported only when DEBUG is enabled.
Before this patch: $ size spl/u-boot-spl text data bss dec hex filename 99325 4899 218584 322808 4ecf8 spl/u-boot-spl
After this patch (with CONFIG_SPL_NET_SUPPORT): $ size spl/u-boot-spl text data bss dec hex filename 99666 4899 218584 323149 4ee4d spl/u-boot-spl
So, this patch adds ~350 bytes to code size.
If CONFIG_SPL_NET_SUPPORT is not enabled, this adds ~25 bytes.
If CONFIG_USE_TINY_PRINTF is disabled then: $ size spl/u-boot-spl text data bss dec hex filename 101116 4899 218584 324599 4f3f7 spl/u-boot-spl
So, there is still ~1.4K space saved even with support for %pM/%pI4.
Compiler used is to build is: arm-linux-gnueabihf-gcc (Linaro GCC 6.2-2016.11) 6.2.1 20161016
Signed-off-by: Vignesh R vigneshr@ti.com ---
Changes wrt RFC: * support %p[ap] only under DEBUG * Add comparsion data w/o tiny printf to commit message.
lib/tiny-printf.c | 154 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 154 insertions(+)
diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c index 6def8f98aa41..0b04813dc206 100644 --- a/lib/tiny-printf.c +++ b/lib/tiny-printf.c @@ -12,6 +12,7 @@ #include <common.h> #include <stdarg.h> #include <serial.h> +#include <linux/ctype.h>
struct printf_info { char *bf; /* Digit buffer */ @@ -52,6 +53,154 @@ static void div_out(struct printf_info *info, unsigned long *num, out_dgt(info, dgt); }
+#ifdef CONFIG_SPL_NET_SUPPORT +static void string(struct printf_info *info, char *s) +{ + char ch; + + while ((ch = *s++)) + out(info, ch); +} + +static const char hex_asc[] = "0123456789abcdef"; +#define hex_asc_lo(x) hex_asc[((x) & 0x0f)] +#define hex_asc_hi(x) hex_asc[((x) & 0xf0) >> 4] + +static inline char *pack_hex_byte(char *buf, u8 byte) +{ + *buf++ = hex_asc_hi(byte); + *buf++ = hex_asc_lo(byte); + return buf; +} + +static void mac_address_string(struct printf_info *info, u8 *addr, + bool separator) +{ + /* (6 * 2 hex digits), 5 colons and trailing zero */ + char mac_addr[6 * 3]; + char *p = mac_addr; + int i; + + for (i = 0; i < 6; i++) { + p = pack_hex_byte(p, addr[i]); + if (separator && i != 5) + *p++ = ':'; + } + *p = '\0'; + + string(info, mac_addr); +} + +static char *put_dec_trunc(char *buf, unsigned int q) +{ + unsigned int d3, d2, d1, d0; + d1 = (q >> 4) & 0xf; + d2 = (q >> 8) & 0xf; + d3 = (q >> 12); + + d0 = 6 * (d3 + d2 + d1) + (q & 0xf); + q = (d0 * 0xcd) >> 11; + d0 = d0 - 10 * q; + *buf++ = d0 + '0'; /* least significant digit */ + d1 = q + 9 * d3 + 5 * d2 + d1; + if (d1 != 0) { + q = (d1 * 0xcd) >> 11; + d1 = d1 - 10 * q; + *buf++ = d1 + '0'; /* next digit */ + + d2 = q + 2 * d2; + if ((d2 != 0) || (d3 != 0)) { + q = (d2 * 0xd) >> 7; + d2 = d2 - 10 * q; + *buf++ = d2 + '0'; /* next digit */ + + d3 = q + 4 * d3; + if (d3 != 0) { + q = (d3 * 0xcd) >> 11; + d3 = d3 - 10 * q; + *buf++ = d3 + '0'; /* next digit */ + if (q != 0) + *buf++ = q + '0'; /* most sign. digit */ + } + } + } + return buf; +} + +static void ip4_addr_string(struct printf_info *info, u8 *addr) +{ + /* (4 * 3 decimal digits), 3 dots and trailing zero */ + char ip4_addr[4 * 4]; + char temp[3]; /* hold each IP quad in reverse order */ + char *p = ip4_addr; + int i, digits; + + for (i = 0; i < 4; i++) { + digits = put_dec_trunc(temp, addr[i]) - temp; + /* reverse the digits in the quad */ + while (digits--) + *p++ = temp[digits]; + if (i != 3) + *p++ = '.'; + } + *p = '\0'; + + string(info, ip4_addr); +} +#endif + +/* + * Show a '%p' thing. A kernel extension is that the '%p' is followed + * by an extra set of characters that are extended format + * specifiers. + * + * Right now we handle: + * + * - 'M' For a 6-byte MAC address, it prints the address in the + * usual colon-separated hex notation. + * - 'm' Same as above except there is no colon-separator. + * - 'I4'for IPv4 addresses printed in the usual way (dot-separated + * decimal). + */ + +static void pointer(struct printf_info *info, const char *fmt, void *ptr) +{ +#ifdef DEBUG + unsigned long num = (uintptr_t)ptr; + unsigned long div; +#endif + + switch (*fmt) { +#ifdef DEBUG + case 'a': + + switch (fmt[1]) { + case 'p': + default: + num = *(phys_addr_t *)ptr; + break; + } + break; +#endif +#ifdef CONFIG_SPL_NET_SUPPORT + case 'm': + return mac_address_string(info, ptr, false); + case 'M': + return mac_address_string(info, ptr, true); + case 'I': + if (fmt[1] == '4') + return ip4_addr_string(info, ptr); +#endif + default: + break; + } +#ifdef DEBUG + div = 1UL << (sizeof(long) * 8 - 4); + for (; div; div /= 0x10) + div_out(info, &num, div); +#endif +} + static int _vprintf(struct printf_info *info, const char *fmt, va_list va) { char ch; @@ -144,6 +293,11 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va) case 's': p = va_arg(va, char*); break; + case 'p': + pointer(info, fmt, va_arg(va, void *)); + while (isalnum(fmt[0])) + fmt++; + break; case '%': out(info, '%'); default:

On Mon, Apr 10, 2017 at 12:23:22PM +0530, Vignesh R wrote:
Add support for %p, %pa[p], %pM, %pm and %pI4 formats to tiny-printf. %pM and %pI4 are widely used by SPL networking stack and is required if networking support is desired in SPL. %p, %pa and %pap are mostly used by debug prints and hence supported only when DEBUG is enabled.
Before this patch: $ size spl/u-boot-spl text data bss dec hex filename 99325 4899 218584 322808 4ecf8 spl/u-boot-spl
After this patch (with CONFIG_SPL_NET_SUPPORT): $ size spl/u-boot-spl text data bss dec hex filename 99666 4899 218584 323149 4ee4d spl/u-boot-spl
So, this patch adds ~350 bytes to code size.
If CONFIG_SPL_NET_SUPPORT is not enabled, this adds ~25 bytes.
If CONFIG_USE_TINY_PRINTF is disabled then: $ size spl/u-boot-spl text data bss dec hex filename 101116 4899 218584 324599 4f3f7 spl/u-boot-spl
So, there is still ~1.4K space saved even with support for %pM/%pI4.
Compiler used is to build is: arm-linux-gnueabihf-gcc (Linaro GCC 6.2-2016.11) 6.2.1 20161016
Signed-off-by: Vignesh R vigneshr@ti.com
Reviewed-by: Tom Rini trini@konsulko.com

On 10 April 2017 at 00:53, Vignesh R vigneshr@ti.com wrote:
Add support for %p, %pa[p], %pM, %pm and %pI4 formats to tiny-printf. %pM and %pI4 are widely used by SPL networking stack and is required if networking support is desired in SPL. %p, %pa and %pap are mostly used by debug prints and hence supported only when DEBUG is enabled.
Before this patch: $ size spl/u-boot-spl text data bss dec hex filename 99325 4899 218584 322808 4ecf8 spl/u-boot-spl
After this patch (with CONFIG_SPL_NET_SUPPORT): $ size spl/u-boot-spl text data bss dec hex filename 99666 4899 218584 323149 4ee4d spl/u-boot-spl
So, this patch adds ~350 bytes to code size.
If CONFIG_SPL_NET_SUPPORT is not enabled, this adds ~25 bytes.
If CONFIG_USE_TINY_PRINTF is disabled then: $ size spl/u-boot-spl text data bss dec hex filename 101116 4899 218584 324599 4f3f7 spl/u-boot-spl
So, there is still ~1.4K space saved even with support for %pM/%pI4.
Compiler used is to build is: arm-linux-gnueabihf-gcc (Linaro GCC 6.2-2016.11) 6.2.1 20161016
Signed-off-by: Vignesh R vigneshr@ti.com
Changes wrt RFC:
- support %p[ap] only under DEBUG
- Add comparsion data w/o tiny printf to commit message.
lib/tiny-printf.c | 154 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 154 insertions(+)
diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c index 6def8f98aa41..0b04813dc206 100644 --- a/lib/tiny-printf.c +++ b/lib/tiny-printf.c @@ -12,6 +12,7 @@ #include <common.h> #include <stdarg.h> #include <serial.h> +#include <linux/ctype.h>
struct printf_info { char *bf; /* Digit buffer */ @@ -52,6 +53,154 @@ static void div_out(struct printf_info *info, unsigned long *num, out_dgt(info, dgt); }
+#ifdef CONFIG_SPL_NET_SUPPORT +static void string(struct printf_info *info, char *s) +{
char ch;
while ((ch = *s++))
out(info, ch);
+}
+static const char hex_asc[] = "0123456789abcdef"; +#define hex_asc_lo(x) hex_asc[((x) & 0x0f)] +#define hex_asc_hi(x) hex_asc[((x) & 0xf0) >> 4]
+static inline char *pack_hex_byte(char *buf, u8 byte) +{
*buf++ = hex_asc_hi(byte);
*buf++ = hex_asc_lo(byte);
return buf;
+}
+static void mac_address_string(struct printf_info *info, u8 *addr,
bool separator)
+{
/* (6 * 2 hex digits), 5 colons and trailing zero */
char mac_addr[6 * 3];
char *p = mac_addr;
int i;
for (i = 0; i < 6; i++) {
p = pack_hex_byte(p, addr[i]);
if (separator && i != 5)
*p++ = ':';
}
*p = '\0';
string(info, mac_addr);
+}
+static char *put_dec_trunc(char *buf, unsigned int q) +{
unsigned int d3, d2, d1, d0;
d1 = (q >> 4) & 0xf;
d2 = (q >> 8) & 0xf;
d3 = (q >> 12);
d0 = 6 * (d3 + d2 + d1) + (q & 0xf);
q = (d0 * 0xcd) >> 11;
d0 = d0 - 10 * q;
*buf++ = d0 + '0'; /* least significant digit */
d1 = q + 9 * d3 + 5 * d2 + d1;
if (d1 != 0) {
q = (d1 * 0xcd) >> 11;
d1 = d1 - 10 * q;
*buf++ = d1 + '0'; /* next digit */
d2 = q + 2 * d2;
if ((d2 != 0) || (d3 != 0)) {
q = (d2 * 0xd) >> 7;
d2 = d2 - 10 * q;
*buf++ = d2 + '0'; /* next digit */
d3 = q + 4 * d3;
if (d3 != 0) {
q = (d3 * 0xcd) >> 11;
d3 = d3 - 10 * q;
*buf++ = d3 + '0'; /* next digit */
if (q != 0)
*buf++ = q + '0'; /* most sign. digit */
OMG not the nicest code!
}
}
}
return buf;
+}
+static void ip4_addr_string(struct printf_info *info, u8 *addr) +{
/* (4 * 3 decimal digits), 3 dots and trailing zero */
char ip4_addr[4 * 4];
char temp[3]; /* hold each IP quad in reverse order */
char *p = ip4_addr;
int i, digits;
for (i = 0; i < 4; i++) {
digits = put_dec_trunc(temp, addr[i]) - temp;
/* reverse the digits in the quad */
while (digits--)
*p++ = temp[digits];
if (i != 3)
*p++ = '.';
}
*p = '\0';
string(info, ip4_addr);
+} +#endif
+/*
- Show a '%p' thing. A kernel extension is that the '%p' is followed
- by an extra set of characters that are extended format
- specifiers.
- Right now we handle:
- 'M' For a 6-byte MAC address, it prints the address in the
usual colon-separated hex notation.
- 'm' Same as above except there is no colon-separator.
- 'I4'for IPv4 addresses printed in the usual way (dot-separated
decimal).
- */
+static void pointer(struct printf_info *info, const char *fmt, void *ptr) +{ +#ifdef DEBUG
What is the #ifdef DEBUG for? It may not be enabled globally so I don't think you can do this. You probably need this code always.
Having said that, at some point it would be nice to have a global debug framework, which understood how to enable/disable debugging at build-time or run-time.
Regards, Simon

On Tue, Apr 11, 2017 at 07:56:06AM -0600, Simon Glass wrote:
On 10 April 2017 at 00:53, Vignesh R vigneshr@ti.com wrote:
Add support for %p, %pa[p], %pM, %pm and %pI4 formats to tiny-printf. %pM and %pI4 are widely used by SPL networking stack and is required if networking support is desired in SPL. %p, %pa and %pap are mostly used by debug prints and hence supported only when DEBUG is enabled.
[snip]
+static void pointer(struct printf_info *info, const char *fmt, void *ptr) +{ +#ifdef DEBUG
What is the #ifdef DEBUG for? It may not be enabled globally so I don't think you can do this. You probably need this code always.
So, %p/%pa/%pa[p] are used in debug() prints, which only matter when DEBUG is set. Doing it this way means we globally bloat by (I think I snipped out..) 25 bytes? instead of ~250 bytes. And since we're in tiny-printf, which we use when every byte counts, I'm happier about only bloating by 25 bytes here.

Hi Tom,
On 11 April 2017 at 08:00, Tom Rini trini@konsulko.com wrote:
On Tue, Apr 11, 2017 at 07:56:06AM -0600, Simon Glass wrote:
On 10 April 2017 at 00:53, Vignesh R vigneshr@ti.com wrote:
Add support for %p, %pa[p], %pM, %pm and %pI4 formats to tiny-printf. %pM and %pI4 are widely used by SPL networking stack and is required if networking support is desired in SPL. %p, %pa and %pap are mostly used by debug prints and hence supported only when DEBUG is enabled.
[snip]
+static void pointer(struct printf_info *info, const char *fmt, void *ptr) +{ +#ifdef DEBUG
What is the #ifdef DEBUG for? It may not be enabled globally so I don't think you can do this. You probably need this code always.
So, %p/%pa/%pa[p] are used in debug() prints, which only matter when DEBUG is set. Doing it this way means we globally bloat by (I think I snipped out..) 25 bytes? instead of ~250 bytes. And since we're in tiny-printf, which we use when every byte counts, I'm happier about only bloating by 25 bytes here.
What I mean is that typically DEBUG is enabled file by file. So if I want to output something I need to enable DEBUG in this file as well? That seems confusing to me. Perhaps it needs another CONFIG option?
Regards, Simon

On Tue, Apr 11, 2017 at 08:03:07AM -0600, Simon Glass wrote:
Hi Tom,
On 11 April 2017 at 08:00, Tom Rini trini@konsulko.com wrote:
On Tue, Apr 11, 2017 at 07:56:06AM -0600, Simon Glass wrote:
On 10 April 2017 at 00:53, Vignesh R vigneshr@ti.com wrote:
Add support for %p, %pa[p], %pM, %pm and %pI4 formats to tiny-printf. %pM and %pI4 are widely used by SPL networking stack and is required if networking support is desired in SPL. %p, %pa and %pap are mostly used by debug prints and hence supported only when DEBUG is enabled.
[snip]
+static void pointer(struct printf_info *info, const char *fmt, void *ptr) +{ +#ifdef DEBUG
What is the #ifdef DEBUG for? It may not be enabled globally so I don't think you can do this. You probably need this code always.
So, %p/%pa/%pa[p] are used in debug() prints, which only matter when DEBUG is set. Doing it this way means we globally bloat by (I think I snipped out..) 25 bytes? instead of ~250 bytes. And since we're in tiny-printf, which we use when every byte counts, I'm happier about only bloating by 25 bytes here.
What I mean is that typically DEBUG is enabled file by file. So if I want to output something I need to enable DEBUG in this file as well? That seems confusing to me. Perhaps it needs another CONFIG option?
I usually end up whacking DEBUG into common.h myself, so I hadn't thought about it that way. Long-term, yeah, we should think about how to handle debug stuff as there's times you want everything on and times you just want a little bit on, and we're talking about the we-need-space case here too.

On 11 April 2017 at 08:13, Tom Rini trini@konsulko.com wrote:
On Tue, Apr 11, 2017 at 08:03:07AM -0600, Simon Glass wrote:
Hi Tom,
On 11 April 2017 at 08:00, Tom Rini trini@konsulko.com wrote:
On Tue, Apr 11, 2017 at 07:56:06AM -0600, Simon Glass wrote:
On 10 April 2017 at 00:53, Vignesh R vigneshr@ti.com wrote:
Add support for %p, %pa[p], %pM, %pm and %pI4 formats to tiny-printf. %pM and %pI4 are widely used by SPL networking stack and is required if networking support is desired in SPL. %p, %pa and %pap are mostly used by debug prints and hence supported only when DEBUG is enabled.
[snip]
+static void pointer(struct printf_info *info, const char *fmt, void *ptr) +{ +#ifdef DEBUG
What is the #ifdef DEBUG for? It may not be enabled globally so I don't think you can do this. You probably need this code always.
So, %p/%pa/%pa[p] are used in debug() prints, which only matter when DEBUG is set. Doing it this way means we globally bloat by (I think I snipped out..) 25 bytes? instead of ~250 bytes. And since we're in tiny-printf, which we use when every byte counts, I'm happier about only bloating by 25 bytes here.
What I mean is that typically DEBUG is enabled file by file. So if I want to output something I need to enable DEBUG in this file as well? That seems confusing to me. Perhaps it needs another CONFIG option?
I usually end up whacking DEBUG into common.h myself, so I hadn't thought about it that way. Long-term, yeah, we should think about how to handle debug stuff as there's times you want everything on and times you just want a little bit on, and we're talking about the we-need-space case here too.
OK. Would be a nice little project for someone :-)
Reviewed-by: Simon Glass sjg@chromium.org

On Mon, Apr 10, 2017 at 12:23:22PM +0530, Vignesh R wrote:
Add support for %p, %pa[p], %pM, %pm and %pI4 formats to tiny-printf. %pM and %pI4 are widely used by SPL networking stack and is required if networking support is desired in SPL. %p, %pa and %pap are mostly used by debug prints and hence supported only when DEBUG is enabled.
Before this patch: $ size spl/u-boot-spl text data bss dec hex filename 99325 4899 218584 322808 4ecf8 spl/u-boot-spl
After this patch (with CONFIG_SPL_NET_SUPPORT): $ size spl/u-boot-spl text data bss dec hex filename 99666 4899 218584 323149 4ee4d spl/u-boot-spl
So, this patch adds ~350 bytes to code size.
If CONFIG_SPL_NET_SUPPORT is not enabled, this adds ~25 bytes.
If CONFIG_USE_TINY_PRINTF is disabled then: $ size spl/u-boot-spl text data bss dec hex filename 101116 4899 218584 324599 4f3f7 spl/u-boot-spl
So, there is still ~1.4K space saved even with support for %pM/%pI4.
Compiler used is to build is: arm-linux-gnueabihf-gcc (Linaro GCC 6.2-2016.11) 6.2.1 20161016
Signed-off-by: Vignesh R vigneshr@ti.com Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
participants (3)
-
Simon Glass
-
Tom Rini
-
Vignesh R