[U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver

[Scott Wood]

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.goncalves at 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