From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Mon, 17 Sep 2012 09:57:57 -0700 Subject: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver In-Reply-To: <505598FF.7010108@inov.pt> References: <1347643742-19966-1-git-send-email-jose.goncalves@inov.pt> <1347643742-19966-10-git-send-email-jose.goncalves@inov.pt> <201209142021.12033.marex@denx.de> <50537B54.2060903@inov.pt> <20120914190120.GL22028@bill-the-cat> <505598FF.7010108@inov.pt> Message-ID: <20120917165757.GA16252@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > >>>[...] > >>> > >>>>+#include > >>>>+#include > >>>>+#include > >>>>+#include > >>>>+#include > >>>>+ > >>>>+#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 :) - 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). -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: