[U-Boot] [RFC PATCH] SPL: replace ctype implementation with tiny version

[U-Boot] [RFC PATCH] SPL: replace ctype implementation with tiny version

[Andre Przywara]

The ctype implementation (isdigit() & friends) works with an array of
256 Bytes - one for each character. This is pretty big in SPL terms,
so let's replace this "bloated" implementation with a tiny version
using C statements. This only implements the functions that the SPL
requires and confines this change only to an actual SPL build.
Saves about 200 Bytes from the SPL code size.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
Hi,

some people voiced concerns about running out of SPL code space when
adding new features. In this particular case this was an issue when
looking at the SPL FIT extension series[1].
This patch here on top of this series saves more space than the SPL FIT
series consumed, so I trade this as a bait to people wrestling with this
problem ;-)

Cheers,
Andre.

[1] http://lists.denx.de/pipermail/u-boot/2017-January/278772.html

 include/linux/tiny_ctype.h | 12 ++++++++++++
 lib/Makefile               |  2 ++
 lib/strto.c                |  4 ++++
 3 files changed, 18 insertions(+)
 create mode 100644 include/linux/tiny_ctype.h

diff --git a/include/linux/tiny_ctype.h b/include/linux/tiny_ctype.h
new file mode 100644
index 0000000..4910412
--- /dev/null
+++ b/include/linux/tiny_ctype.h
@@ -0,0 +1,12 @@
+#ifndef _LINUX_CTYPE_H
+#define _LINUX_CTYPE_H
+
+#define isdigit(c)	(((c) >= '0') && ((c) <= '9'))
+#define isxdigit(c)	(isdigit(c) || \
+			 ((c) >= 'A' && (c) <= 'F') || \
+			 ((c) >= 'a' && (c) <= 'f'))
+#define islower(c)	((((c) >= 'a') && ((c) <= 'z')) || ((c) >= 223))
+
+#define toupper(c)	((((c) >= 'a') && ((c) <= 'z')) ? (c) - 32 : (c))
+
+#endif
diff --git a/lib/Makefile b/lib/Makefile
index 23e9f1e..15385fd 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -66,7 +66,9 @@ obj-y += display_options.o
 CFLAGS_display_options.o := $(if $(BUILD_TAG),-DBUILD_TAG='"$(BUILD_TAG)"')
 obj-$(CONFIG_BCH) += bch.o
 obj-y += crc32.o
+ifndef CONFIG_SPL_BUILD
 obj-y += ctype.o
+endif
 obj-y += div64.o
 obj-y += hang.o
 obj-y += linux_compat.o
diff --git a/lib/strto.c b/lib/strto.c
index e93a4f5..9e9ba75 100644
--- a/lib/strto.c
+++ b/lib/strto.c
@@ -11,7 +11,11 @@
 
 #include <common.h>
 #include <errno.h>
+#ifdef CONFIG_SPL_BUILD
+#include <linux/tiny_ctype.h>
+#else
 #include <linux/ctype.h>
+#endif
 
 unsigned long simple_strtoul(const char *cp, char **endp,
 				unsigned int base)
-- 
2.8.2

[U-Boot] [RFC PATCH] SPL: replace ctype implementation with tiny version

[Tom Rini]

On Fri, Jan 20, 2017 at 10:33:28PM +0000, Andre Przywara wrote:
> The ctype implementation (isdigit() & friends) works with an array of
> 256 Bytes - one for each character. This is pretty big in SPL terms,
> so let's replace this "bloated" implementation with a tiny version
> using C statements. This only implements the functions that the SPL
> requires and confines this change only to an actual SPL build.
> Saves about 200 Bytes from the SPL code size.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> Hi,
> 
> some people voiced concerns about running out of SPL code space when
> adding new features. In this particular case this was an issue when
> looking at the SPL FIT extension series[1].
> This patch here on top of this series saves more space than the SPL FIT
> series consumed, so I trade this as a bait to people wrestling with this
> problem ;-)

So.. where might this fail?  Also, this doesn't look like it comes from
the kernel exactly, so "linux/tiny_ctype.h" doesn't seem right (and, is
missing the license boilerplate).  It does bear some resemblance to the
kernel ctype header so it would of course be correct to attribute that
in the boilerplate.

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

[U-Boot] [RFC PATCH] SPL: replace ctype implementation with tiny version

[Andrew F. Davis]

On 01/20/2017 04:33 PM, Andre Przywara wrote:
> The ctype implementation (isdigit() & friends) works with an array of
> 256 Bytes - one for each character. This is pretty big in SPL terms,
> so let's replace this "bloated" implementation with a tiny version
> using C statements. This only implements the functions that the SPL
> requires and confines this change only to an actual SPL build.
> Saves about 200 Bytes from the SPL code size.
> 

Saves us a very needed 256 bytes on AM335x!

Acked-by: Andrew F. Davis <afd@ti.com>

Thanks,
Andrew

> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> Hi,
> 
> some people voiced concerns about running out of SPL code space when
> adding new features. In this particular case this was an issue when
> looking at the SPL FIT extension series[1].
> This patch here on top of this series saves more space than the SPL FIT
> series consumed, so I trade this as a bait to people wrestling with this
> problem ;-)
> 
> Cheers,
> Andre.
> 
> [1] http://lists.denx.de/pipermail/u-boot/2017-January/278772.html
> 
>  include/linux/tiny_ctype.h | 12 ++++++++++++
>  lib/Makefile               |  2 ++
>  lib/strto.c                |  4 ++++
>  3 files changed, 18 insertions(+)
>  create mode 100644 include/linux/tiny_ctype.h
> 
> diff --git a/include/linux/tiny_ctype.h b/include/linux/tiny_ctype.h
> new file mode 100644
> index 0000000..4910412
> --- /dev/null
> +++ b/include/linux/tiny_ctype.h
> @@ -0,0 +1,12 @@
> +#ifndef _LINUX_CTYPE_H
> +#define _LINUX_CTYPE_H
> +
> +#define isdigit(c)	(((c) >= '0') && ((c) <= '9'))
> +#define isxdigit(c)	(isdigit(c) || \
> +			 ((c) >= 'A' && (c) <= 'F') || \
> +			 ((c) >= 'a' && (c) <= 'f'))
> +#define islower(c)	((((c) >= 'a') && ((c) <= 'z')) || ((c) >= 223))
> +
> +#define toupper(c)	((((c) >= 'a') && ((c) <= 'z')) ? (c) - 32 : (c))
> +
> +#endif
> diff --git a/lib/Makefile b/lib/Makefile
> index 23e9f1e..15385fd 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -66,7 +66,9 @@ obj-y += display_options.o
>  CFLAGS_display_options.o := $(if $(BUILD_TAG),-DBUILD_TAG='"$(BUILD_TAG)"')
>  obj-$(CONFIG_BCH) += bch.o
>  obj-y += crc32.o
> +ifndef CONFIG_SPL_BUILD
>  obj-y += ctype.o
> +endif
>  obj-y += div64.o
>  obj-y += hang.o
>  obj-y += linux_compat.o
> diff --git a/lib/strto.c b/lib/strto.c
> index e93a4f5..9e9ba75 100644
> --- a/lib/strto.c
> +++ b/lib/strto.c
> @@ -11,7 +11,11 @@
>  
>  #include <common.h>
>  #include <errno.h>
> +#ifdef CONFIG_SPL_BUILD
> +#include <linux/tiny_ctype.h>
> +#else
>  #include <linux/ctype.h>
> +#endif
>  
>  unsigned long simple_strtoul(const char *cp, char **endp,
>  				unsigned int base)
> 

[U-Boot] [RFC PATCH] SPL: replace ctype implementation with tiny version

[Andrew F. Davis]

On 01/22/2017 03:58 PM, Andrew F. Davis wrote:
> On 01/20/2017 04:33 PM, Andre Przywara wrote:
>> The ctype implementation (isdigit() & friends) works with an array of
>> 256 Bytes - one for each character. This is pretty big in SPL terms,
>> so let's replace this "bloated" implementation with a tiny version
>> using C statements. This only implements the functions that the SPL
>> requires and confines this change only to an actual SPL build.
>> Saves about 200 Bytes from the SPL code size.
>>
> 
> Saves us a very needed 256 bytes on AM335x!
> 
> Acked-by: Andrew F. Davis <afd@ti.com>
> 
> Thanks,
> Andrew
> 
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>> Hi,
>>
>> some people voiced concerns about running out of SPL code space when
>> adding new features. In this particular case this was an issue when
>> looking at the SPL FIT extension series[1].
>> This patch here on top of this series saves more space than the SPL FIT
>> series consumed, so I trade this as a bait to people wrestling with this
>> problem ;-)
>>
>> Cheers,
>> Andre.
>>
>> [1] http://lists.denx.de/pipermail/u-boot/2017-January/278772.html
>>
>>  include/linux/tiny_ctype.h | 12 ++++++++++++
>>  lib/Makefile               |  2 ++
>>  lib/strto.c                |  4 ++++
>>  3 files changed, 18 insertions(+)
>>  create mode 100644 include/linux/tiny_ctype.h
>>
>> diff --git a/include/linux/tiny_ctype.h b/include/linux/tiny_ctype.h
>> new file mode 100644
>> index 0000000..4910412
>> --- /dev/null
>> +++ b/include/linux/tiny_ctype.h
>> @@ -0,0 +1,12 @@
>> +#ifndef _LINUX_CTYPE_H
>> +#define _LINUX_CTYPE_H
>> +
>> +#define isdigit(c)	(((c) >= '0') && ((c) <= '9'))
>> +#define isxdigit(c)	(isdigit(c) || \
>> +			 ((c) >= 'A' && (c) <= 'F') || \
>> +			 ((c) >= 'a' && (c) <= 'f'))
>> +#define islower(c)	((((c) >= 'a') && ((c) <= 'z')) || ((c) >= 223))
>> +
>> +#define toupper(c)	((((c) >= 'a') && ((c) <= 'z')) ? (c) - 32 : (c))
>> +
>> +#endif
>> diff --git a/lib/Makefile b/lib/Makefile
>> index 23e9f1e..15385fd 100644
>> --- a/lib/Makefile
>> +++ b/lib/Makefile
>> @@ -66,7 +66,9 @@ obj-y += display_options.o
>>  CFLAGS_display_options.o := $(if $(BUILD_TAG),-DBUILD_TAG='"$(BUILD_TAG)"')
>>  obj-$(CONFIG_BCH) += bch.o
>>  obj-y += crc32.o
>> +ifndef CONFIG_SPL_BUILD
>>  obj-y += ctype.o
>> +endif

One minor note, this causes build failures for code that uses the
functions not defined here (or not including this header). Keeping the
same space saving technique, we could avoid those changes by moving the
SPL_BUILD check to inside ctype.h, then define a set of macros that
don't use the _ctype table. The linker would drop the table if it is not
needed without needing to have the Makefile change.

Andrew

>>  obj-y += div64.o
>>  obj-y += hang.o
>>  obj-y += linux_compat.o
>> diff --git a/lib/strto.c b/lib/strto.c
>> index e93a4f5..9e9ba75 100644
>> --- a/lib/strto.c
>> +++ b/lib/strto.c
>> @@ -11,7 +11,11 @@
>>  
>>  #include <common.h>
>>  #include <errno.h>
>> +#ifdef CONFIG_SPL_BUILD
>> +#include <linux/tiny_ctype.h>
>> +#else
>>  #include <linux/ctype.h>
>> +#endif
>>  
>>  unsigned long simple_strtoul(const char *cp, char **endp,
>>  				unsigned int base)
>>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 

[U-Boot] [RFC PATCH] SPL: replace ctype implementation with tiny version

[André Przywara]

On 22/01/17 16:09, Tom Rini wrote:
> On Fri, Jan 20, 2017 at 10:33:28PM +0000, Andre Przywara wrote:
>> The ctype implementation (isdigit() & friends) works with an array of
>> 256 Bytes - one for each character. This is pretty big in SPL terms,
>> so let's replace this "bloated" implementation with a tiny version
>> using C statements. This only implements the functions that the SPL
>> requires and confines this change only to an actual SPL build.
>> Saves about 200 Bytes from the SPL code size.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>> Hi,
>>
>> some people voiced concerns about running out of SPL code space when
>> adding new features. In this particular case this was an issue when
>> looking at the SPL FIT extension series[1].
>> This patch here on top of this series saves more space than the SPL FIT
>> series consumed, so I trade this as a bait to people wrestling with this
>> problem ;-)
> 
> So.. where might this fail?

What do you mean by that?

> Also, this doesn't look like it comes from
> the kernel exactly, so "linux/tiny_ctype.h" doesn't seem right (and, is
> missing the license boilerplate).  It does bear some resemblance to the
> kernel ctype header so it would of course be correct to attribute that
> in the boilerplate.

Actually I quickly hacked that up while looking at the existing map,
trying to convert it into statements. It was mostly an experiment to see
if that gives me something (hence the RFC tag).

If the general idea is acceptable, I can surely make this pretty.

Cheers,
Andre.

[U-Boot] [RFC PATCH] SPL: replace ctype implementation with tiny version

[Tom Rini]

On Sun, Jan 22, 2017 at 11:30:04PM +0000, Andr? Przywara wrote:
> On 22/01/17 16:09, Tom Rini wrote:
> > On Fri, Jan 20, 2017 at 10:33:28PM +0000, Andre Przywara wrote:
> >> The ctype implementation (isdigit() & friends) works with an array of
> >> 256 Bytes - one for each character. This is pretty big in SPL terms,
> >> so let's replace this "bloated" implementation with a tiny version
> >> using C statements. This only implements the functions that the SPL
> >> requires and confines this change only to an actual SPL build.
> >> Saves about 200 Bytes from the SPL code size.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >> ---
> >> Hi,
> >>
> >> some people voiced concerns about running out of SPL code space when
> >> adding new features. In this particular case this was an issue when
> >> looking at the SPL FIT extension series[1].
> >> This patch here on top of this series saves more space than the SPL FIT
> >> series consumed, so I trade this as a bait to people wrestling with this
> >> problem ;-)
> > 
> > So.. where might this fail?
> 
> What do you mean by that?
> 
> > Also, this doesn't look like it comes from
> > the kernel exactly, so "linux/tiny_ctype.h" doesn't seem right (and, is
> > missing the license boilerplate).  It does bear some resemblance to the
> > kernel ctype header so it would of course be correct to attribute that
> > in the boilerplate.
> 
> Actually I quickly hacked that up while looking at the existing map,
> trying to convert it into statements. It was mostly an experiment to see
> if that gives me something (hence the RFC tag).
> 
> If the general idea is acceptable, I can surely make this pretty.

Let me answer both parts at once.  If this is as functional in all
cases, why not just always do this instead?

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

[U-Boot] [U-Boot, RFC] SPL: replace ctype implementation with tiny version

[Tom Rini]

On Fri, Jan 20, 2017 at 10:33:28PM +0000, Andre Przywara wrote:

> The ctype implementation (isdigit() & friends) works with an array of
> 256 Bytes - one for each character. This is pretty big in SPL terms,
> so let's replace this "bloated" implementation with a tiny version
> using C statements. This only implements the functions that the SPL
> requires and confines this change only to an actual SPL build.
> Saves about 200 Bytes from the SPL code size.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Acked-by: Andrew F. Davis <afd@ti.com>
> ---
> Hi,
> 
> some people voiced concerns about running out of SPL code space when
> adding new features. In this particular case this was an issue when
> looking at the SPL FIT extension series[1].
> This patch here on top of this series saves more space than the SPL FIT
> series consumed, so I trade this as a bait to people wrestling with this
> problem ;-)
> 
> Cheers,
> Andre.
> 
> [1] http://lists.denx.de/pipermail/u-boot/2017-January/278772.html
> 
>  include/linux/tiny_ctype.h | 12 ++++++++++++
>  lib/Makefile               |  2 ++
>  lib/strto.c                |  4 ++++
>  3 files changed, 18 insertions(+)
>  create mode 100644 include/linux/tiny_ctype.h

So, cycling back.  I think this is a good idea, but breaks on say
socfpga_de0_nano_soc currently:
arm:  +   socfpga_de0_nano_soc
+(socfpga_de0_nano_soc) lib/built-in.o: In function `_vprintf':
+(socfpga_de0_nano_soc) lib/tiny-printf.c:315: undefined reference to `_ctype'
+(socfpga_de0_nano_soc) make[2]: *** [spl/u-boot-spl] Error 1
+(socfpga_de0_nano_soc) make[1]: *** [spl/u-boot-spl] Error 2
+(socfpga_de0_nano_soc) make: *** [sub-make] Error 2

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

[U-Boot] [U-Boot, RFC] SPL: replace ctype implementation with tiny version

[Andre Przywara]

Hi,

On 05/06/17 16:01, Tom Rini wrote:
> On Fri, Jan 20, 2017 at 10:33:28PM +0000, Andre Przywara wrote:
> 
>> The ctype implementation (isdigit() & friends) works with an array of
>> 256 Bytes - one for each character. This is pretty big in SPL terms,
>> so let's replace this "bloated" implementation with a tiny version
>> using C statements. This only implements the functions that the SPL
>> requires and confines this change only to an actual SPL build.
>> Saves about 200 Bytes from the SPL code size.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> Acked-by: Andrew F. Davis <afd@ti.com>
>> ---
>> Hi,
>>
>> some people voiced concerns about running out of SPL code space when
>> adding new features. In this particular case this was an issue when
>> looking at the SPL FIT extension series[1].
>> This patch here on top of this series saves more space than the SPL FIT
>> series consumed, so I trade this as a bait to people wrestling with this
>> problem ;-)
>>
>> Cheers,
>> Andre.
>>
>> [1] http://lists.denx.de/pipermail/u-boot/2017-January/278772.html
>>
>>  include/linux/tiny_ctype.h | 12 ++++++++++++
>>  lib/Makefile               |  2 ++
>>  lib/strto.c                |  4 ++++
>>  3 files changed, 18 insertions(+)
>>  create mode 100644 include/linux/tiny_ctype.h
> 
> So, cycling back.  I think this is a good idea, but breaks on say
> socfpga_de0_nano_soc currently:
> arm:  +   socfpga_de0_nano_soc
> +(socfpga_de0_nano_soc) lib/built-in.o: In function `_vprintf':
> +(socfpga_de0_nano_soc) lib/tiny-printf.c:315: undefined reference to `_ctype'
> +(socfpga_de0_nano_soc) make[2]: *** [spl/u-boot-spl] Error 1
> +(socfpga_de0_nano_soc) make[1]: *** [spl/u-boot-spl] Error 2
> +(socfpga_de0_nano_soc) make: *** [sub-make] Error 2

Yeah, I found this version broken too for some buildman targets.
So I came up with some fixes, and IIRC at least this compiled cleanly.
Unfortunately some boards *regressed* in terms of size, apparently
because the ctype functions are now inlined and if a board calls them
several times we end up with multiple copies, offsetting the code size
savings, which were quite substantial for many boards, otherwise.
I tried one more fix, which improved the situation, but still left
boards with bigger code sizes.
At this point I gave up, but I can post what I had so far for others to
pick up.

Cheers,
Andre.