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

[Tom Rini]

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

On 09/17/12 11:34, José Miguel Gonçalves wrote:
> On 17-09-2012 19:27, Tom Rini wrote:
>> On Mon, Sep 17, 2012 at 07:05:48PM +0100, Jos? Miguel Gon?alves
>> wrote:
>>> On 17-09-2012 18:56, Tom Rini wrote:
>>>> -----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 :)
>>> The solution that I sorted out on the current SPL framework was
>>> to add this:
>>> 
>>> #ifdef CONFIG_SPL_BUILD #define printf(arg...) do {} while (0) 
>>> #ifdef CONFIG_SPL_SERIAL_SUPPORT #define puts(arg)
>>> serial_puts(arg) #endif #endif
>>> 
>>> on a CPU specific header. Marek told me to not use macros, but
>>> to use inline functions instead, but has I told earlier on this
>>> thread, I am unable to do that. Suggestions for doing this in a
>>> better way are welcome...
>> It's gotta go in <common.h>, and something like /* Big comment
>> what / why */ #if !defined(CONFIG_SPL_BUILD) || \ 
>> (CONFIG_SPL_BUILD && CONFIG_SPL_PRINTF_SUPPORT) void putc(...); 
>> void puts(...); int printf(....); #else #define putc(c)
>> serial_putc(c) #define puts(s) serial_puts(s) #define
>> printf(arg...) do {} while (0) #endif
>> 
> 
> Are macros OK to remove printf() and to replace putc()/puts() by 
> serial_putc()/serial_puts() in the SPL? Shouldn’t we be using
> inline functions instead?

As Scott pointed out, inline won't remove the string constants from
the binary so it will still be possibly too large, depending on all of
the code in question.  And note the above isn't 100% right, we need a
test for SERIAL_SUPPORT in there too.

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

iQIcBAEBAgAGBQJQV3JcAAoJENk4IS6UOR1WMe0P/RlvfaE4J6ZxdIAIy77XmqGt
KKTEnJV+dTu8/a9jrMZFmZ/lGNPNrujLlphNUgGZr2W1+lriHuCgAZx1ObsMXhx9
XGRn05gshUuUtTWRCpOy3Nzo9jPF/LYrWttBiwDfOPWZ6+6G3zGPIXV3T9HHEFMM
ti0c0zCBMu1ci5RoQg4Zb6CJqJR/giBrBzegQZlL4x8t9p/GR03MSxdVPHx+NoQ7
wTuom8Q2yE5o7xDlx+zjoGCemNAVae7HXggqyqNpjU0nDb/lBg1HcvVt6IuORx9t
U7u4yskgXom3RYetrKYdYPEE3yufONY3BkTEdHvS0lLE/dc7S8JKBLdAa0cCYEPG
PlzJtQa98cZgcWG2PCx9TT3S16FEs+xSu3yN/S5OsvD3LwmPwls45n0rVZf1qUTL
jHQ5NnFPC/xppZupRs4cr19ED8xs2YGKO7iHc3auPEz7rA6XuLGpMteFtBVM55+a
3JuZzzTMxgvP/i86VI6l+ClSLHXUHcVWPLE2bvmAvj3R2sRe6NaM8ldtWP6Wh0Hw
Ncp4mGLaCh7VkdAo9Uunnf+y0m15BoDNH5fgK0eNErYPAzH34AROTbIrTsl6JLmQ
+x1sYL3XLmw7z+nyd0a/IfcBjdDGeGZhaHItkCrIj5hOp937yZIEhtP8q8s93qxj
bURH7UFvYhsiUAp0NG4h
=Xg9g
-----END PGP SIGNATURE-----