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

[José Miguel Gonçalves]

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