From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Jos=E9_Miguel_Gon=E7alves?= Date: Mon, 17 Sep 2012 18:08:10 +0100 Subject: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver In-Reply-To: <20120917165757.GA16252@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> Message-ID: <505758FA.2090805@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 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). >