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

[Tom Rini]

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120917/725562d3/attachment.pgp>