From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Sat, 2 May 2015 02:48:29 +0200
Subject: [U-Boot] [U-Boot,05/10] mtd: nand: s3c: Add S3C2440 specifics
In-Reply-To: <20141127022053.GA26896@home.buserror.net>
References: <1413045778-5690-5-git-send-email-marex@denx.de>
 <20141127022053.GA26896@home.buserror.net>
Message-ID: <201505020248.29430.marex@denx.de>
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 Thursday, November 27, 2014 at 03:20:53 AM, Scott Wood wrote:
> 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.

No, Samsung has moved on.

> >  #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...

Where exactly ?

> >  		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

So what do you suggest ?

> > -	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.

If that's the case, then please apply.