On 09/17/12 10:03, Scott Wood wrote: > On 09/17/2012 11:57:57 AM, Tom Rini wrote: >> On Sun, Sep 16, 2012 at 10:16:47AM +0100, Jos? Miguel Gon?alves wrote: >> > On 09/14/2012 08:01 PM, Tom Rini wrote: >> > >On Fri, Sep 14, 2012 at 07:45:40PM +0100, Jos? Miguel Gon?alves wrote: >> > >>On 14-09-2012 19:21, Marek Vasut wrote: >> > >>>Dear Jos? Miguel Gon?alves, >> > >>> >> > >>>>NAND Flash driver with HW ECC for the S3C24XX SoCs. >> > >>>>Currently it only supports SLC NAND chips. >> > >>>> >> > >>>>Signed-off-by: Jos? Miguel Gon?alves <jose.goncal...@inov.pt> >> > >>>[...] >> > >>> >> > >>>>+#include <common.h> >> > >>>>+#include <nand.h> >> > >>>>+#include <asm/io.h> >> > >>>>+#include <asm/arch/s3c24xx_cpu.h> >> > >>>>+#include <asm/errno.h> >> > >>>>+ >> > >>>>+#define MAX_CHIPS 2 >> > >>>>+static int nand_cs[MAX_CHIPS] = { 0, 1 }; >> > >>>>+ >> > >>>>+#ifdef CONFIG_SPL_BUILD >> > >>>>+#define printf(arg...) do {} while (0) >> > >>>This doesn't seem quite right ... >> > >>> >> > >>>1) this should be in CPU directory >> > >>>2) should be enabled only if CONFIG_SPL_SERIAL_SUPPORT is not set >> > >>>3) should be inline function, not a macro >> > >>1) and 3) OK. >> > >>Don't quite understand 2). I want to remove the printfs in the SPL >> > >>build, as it would blown up the internal SoC RAM space available. >> > >>So why add a condition with CONFIG_SPL_SERIAL_SUPPORT? >> > >You've got 8KB, based on the final patch in the series. At least >> in my >> > >SPL series that's still enough to get you printf/puts (I believe >> 4kb was >> > >the cutoff where that had to be dropped). >> > > >> > >> > Barely: >> > >> > $ size u-boot-spl >> > text data bss dec hex filename >> > 3337 8 588 3933 f5d u-boot-spl >> > >> > $ size u-boot-spl-printf >> > text data bss dec hex filename >> > 7968 8 604 8580 2184 u-boot-spl-printf >> > >> > The printf is not so important that justifies exhausting the IRAM >> > space available and preventing any future SPL expansion... >> >> There's two parts to this: >> - What else can you do in a single binary, in theory? Is there boot >> medium detection and you would want to have, for example, NAND and SD >> support in the same binary? I would say memory is meant for using, >> but this is a board maintainer decision and that's you :) >> - We have a define today (CONFIG_SPL_LIBCOMMON_SUPPORT) that toggles >> printf or no printf. If we really need to say yes to >> LIBCOMMON_SUPPORT and no to printf, we need finer grained config >> options and then a do-nothing printf is used for SPL. Doing the >> opt-out driver by driver just punts this problem down the road to the >> next developer and that's not very nice (and adding >> CONFIG_SPL_PRINTF_SUPPORT shouldn't be a big patch, modify a few >> Makefiles, update a bunch of config files, add >> common/spl/dummy_funcs.c and a __weak printf). > > Weak symbols are not OK for configuring printf out of the SPL, as you'll > still have all the format strings and caller code in the binary. It > should be a macro (or an inline function that replaces the standard > printf declaration), but it should be in a system header (not the CPU > directory -- not sure what Marek meant there) and be based on an > appropriate CONFIG symbol.
I'm a little leery of adding #if ... into <common.h> around printf. I'd like to not worry about the branch/return bytes until we really really have to again but yes, the strings are more of a concern since they won't be collected out. Just top of my head thinking above. -- Tom _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot