From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Wed, 26 Nov 2014 20:20:53 -0600
Subject: [U-Boot] [U-Boot,05/10] mtd: nand: s3c: Add S3C2440 specifics
In-Reply-To: <1413045778-5690-5-git-send-email-marex@denx.de>
References: <1413045778-5690-5-git-send-email-marex@denx.de>
Message-ID: <20141127022053.GA26896@home.buserror.net>
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 Sat, Oct 11, 2014 at 06:42:53PM +0200, Marek Vasut wrote:
> +#ifdef CONFIG_S3C2410
>  #define S3C2410_NFCONF_TACLS(x)    ((x)<<8)
>  #define S3C2410_NFCONF_TWRPH0(x)   ((x)<<4)
>  #define S3C2410_NFCONF_TWRPH1(x)   ((x)<<0)
> +#else	/* S3C2412, S3C2440 */
> +#define S3C2410_NFCONF_TACLS(x)    ((x)<<12)
> +#define S3C2410_NFCONF_TWRPH0(x)   ((x)<<8)
> +#define S3C2410_NFCONF_TWRPH1(x)   ((x)<<4)
> +#endif

Is there any chance there will be a third variant?  Perhaps the S3C2440
variant should be explicitly requested with an #error if no variant is
selected.

>  #define S3C2410_ADDR_NALE 4
>  #define S3C2410_ADDR_NCLE 8
> @@ -42,25 +51,30 @@ static void s3c24x0_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>  {
>  	struct nand_chip *chip = mtd->priv;
>  	struct s3c24x0_nand *nand = s3c24x0_get_base_nand();
> +	uint32_t sel_reg, sel_bit;
>  
>  	debug("hwcontrol(): 0x%02x 0x%08x\n", cmd & 0xff, ctrl);
>  
>  	if (ctrl & NAND_CTRL_CHANGE) {
> -		ulong IO_ADDR_W = (ulong)nand;
> +		chip->IO_ADDR_W = &nand->nfconf;
>  
>  		if (!(ctrl & NAND_CLE))
> -			IO_ADDR_W |= S3C2410_ADDR_NCLE;
> +			chip->IO_ADDR_W = &nand->nfaddr;
>  		if (!(ctrl & NAND_ALE))
> -			IO_ADDR_W |= S3C2410_ADDR_NALE;
> +			chip->IO_ADDR_W = &nand->nfcmd;
>  
> -		chip->IO_ADDR_W = (void *)IO_ADDR_W;
> +#ifdef CONFIG_S3C2410
> +		sel_reg = (uint32_t)&nand->nfconf;
> +		sel_bit = S3C2410_NFCONF_nFCE;
> +#else
> +		sel_reg = (uint32_t)&nand->nfcont;
> +		sel_bit = S3C2440_NFCONT_nFCE;
> +#endif

Why are you casting &nand->nfcon[ft] to an integer...

>  		if (ctrl & NAND_NCE)
> -			writel(readl(&nand->nfconf) & ~S3C2410_NFCONF_nFCE,
> -			       &nand->nfconf);
> +			clrbits_le32(sel_reg, sel_bit);
>  		else
> -			writel(readl(&nand->nfconf) | S3C2410_NFCONF_nFCE,
> -			       &nand->nfconf);
> +			setbits_le32(sel_reg, sel_bit);

...only to pass that integer to something that normally accepts a
pointer, which doesn't cause a warning only because of ARM's crappy I/O
accessors that don't do type checking?

At least the code that was there before used ulong for inappropriate
pointer-to-integer casts rather than uint32_t. :-P

> -	writel(readl(&clk_power->clkcon) | (1 << 4), &clk_power->clkcon);
> +	setbits_le32(&clk_power->clkcon, 1 << 4);

This change seems unrelated to adding s3c2440 support.  It'd be clearer
if the {clr,set,clrset}bits_le32() conversion were done separately,
before the s3c2440 stuff.  I'm not insisting that you redo this one, but
next time try to keep cleanup separate.

-Scott