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

[Tom Rini <trini@ti.com>]

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@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).

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120917/1a9e0e7a/attachment.pgp>