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

[Tom Rini]

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

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