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

José Miguel Gonçalves
2012-09-17T17:08:10Z

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