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