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.
-Scott
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
