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

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/17/12 10:08, José Miguel Gonçalves wrote:
> On 17-09-2012 17:57, 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 :)
> 
> That's exactly what I've got in mind when I talked about a future 
> expansion! Being able to boot also from an SD card. With only 8KB
> for .text and .data, I can not use printfs in the SPL for this
> platform (at least with the present printf support for SPL).
> 
>> - 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).

OK, so please take a stab at option two, on top of my SPL series,
keeping in mind what Scott has said (which makes sense) because
otherwise you'll be changing a lot of MMC files too to drop out printf :)

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQV2Q+AAoJENk4IS6UOR1W0VMP/1ToNzW5XmNApUGYIZi1d779
hJ6MieZoiTHOCnrHiRMMMAfOnPNnt+63TozmXWkhNhlPVRCs1Irx8nCpzMabjevr
ZENjnewtKsvgBsICak5rSQLbfyQBUG8tL3iBMPnYwyNhDf8CgED6rNCnxhV8Lr7j
o0gELaNHPRD7bJwglXo3TN0BxNtTrww3uSArSYh4WMVaOP908Mk7p7y8qEVSvNeh
BrdbVZK1oPmhlke9EUXfCQYqn/JdJ7mtdW1q5PdST7GFtnLZmqpj+FuOEwN3OioA
DE6J461Aqr95mGVUnUr7PxglytL6zLKJcE8YpIhu9nL6r9QRg0wm4HIKr3uTKLl5
WBs4ziJsLtm5IAZ7xg2FOsPrCkrAiL3bVQg0+7xVc1cVzKIGmtMR6oGVS+nI38Q6
lzmz0AQSuobeLkiP4+tL8C7kFkwMcj9I5LByN968ZMvbTftecIsdgYSkluOdGU5w
dwKPjplU4t8yy6d1TXbh0Xdw7q8cBZ3bjqbAKi3uo9BpEHgCDOwHp7oOTuJTDB/7
C6WxXHdQFHh7m0hxf1zkawOeh+oqd5MHAxjlckQ/zmg5UsmNWDA6RmmYgWrOBw9m
jCvN/lhe1soBnxaz2byUKwEkPIDwmBl+JgtSL7DMZ7bLfS59daXiaMNac2JPstG3
j0lBvj7SCVnQrE6SJxxa
=71I1
-----END PGP SIGNATURE-----