| Message ID | 20170410065322.17131-1-vigneshr@ti.com |
|---|---|
| State | Accepted |
| Commit | cdce1f762029619e80061833b153cdbf3d0a0860 |
| Delegated to: | Tom Rini |
| Headers | show |
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!
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:
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(+)