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

Scott Wood
2012-09-17T17:03:37Z

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