From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?windows-1252?Q?Jos=E9_Miguel_Gon=E7alves?= Date: Mon, 17 Sep 2012 19:34:47 +0100 Subject: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver In-Reply-To: <20120917182720.GE16252@bill-the-cat> 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> <20120917165757.GA16252@bill-the-cat> <505758FA.2090805@inov.pt> <5057643E.20806@ti.com> <5057667C.6050604@inov.pt> <20120917182720.GE16252@bill-the-cat> Message-ID: <50576D47.3080001@inov.pt> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 17-09-2012 19:27, Tom Rini wrote: > 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 >>>>>>>>>> >>>>>>>>> [...] >>>>>>>>> >>>>>>>>>> +#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 :) >>>> 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 , 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 > Are macros OK to remove printf() and to replace putc()/puts() by serial_putc()/serial_puts() in the SPL? Shouldn?t we be using inline functions instead?