From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?ISO-8859-1?Q?Jos=E9_Miguel_Gon=E7alves?= <jose.goncalves@inov.pt>
Date: Fri, 14 Sep 2012 19:45:40 +0100
Subject: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
In-Reply-To: <201209142021.12033.marex@denx.de>
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>
Message-ID: <50537B54.2060903@inov.pt>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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?

>
>> +#endif
>> +
>> +static void s3c_nand_select_chip(struct mtd_info *mtd, int chip)
>> +{
>> +	struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
>> +	u_long nfcont;
>> +
>> +	nfcont = readl(&nand->nfcont);
>> +
>> +	switch (chip) {
>> +	case -1:
>> +		nfcont |= NFCONT_NCE1 | NFCONT_NCE0;
>> +		break;
>> +	case 0:
>> +		nfcont &= ~NFCONT_NCE0;
>> +		break;
>> +	case 1:
>> +		nfcont &= ~NFCONT_NCE1;
>> +		break;
>> +	default:
>> +		return;
>> +	}
>> +
>> +	writel(nfcont, &nand->nfcont);
>> +}
>> +
>> +/*
>> + * Hardware specific access to control-lines function
>> + */
>> +static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int
>> ctrl) +{
>> +	struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
>> +	struct nand_chip *this = mtd->priv;
>> +
>> +	if (ctrl & NAND_CTRL_CHANGE) {
>> +		if (ctrl & NAND_CLE)
>> +			this->IO_ADDR_W = &nand->nfcmmd;
>> +		else if (ctrl & NAND_ALE)
>> +			this->IO_ADDR_W = &nand->nfaddr;
>> +		else
>> +			this->IO_ADDR_W = &nand->nfdata;
> Why don't you use local variable here?

Because you need to retain the NAND controller register to use between calls to 
s3c_nand_hwcontrol().
If you call the function without the NAND_CTRL_CHANGE bit set in the parameter 
'ctrl' you must use the register used on the last call on the next access.

>
>> +		if (ctrl & NAND_NCE)
>> +			s3c_nand_select_chip(mtd, *(int *)this->priv);
> Uh, how's this *(int *) supposed to work?
>

*(int *)this->priv contains an integer that is the chip id (0 or 1).

Best regards,
Jos? Gon?alves

Best regards,
Jos? Gon?alves