From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marek.vasut@gmail.com>
Date: Thu, 3 Nov 2011 01:15:35 +0100
Subject: [U-Boot] [PATCH 3/4 V2] OneNAND: Add simple OneNAND SPL
In-Reply-To: <4EB1C737.20702@freescale.com>
References: <1320067393-18822-4-git-send-email-marek.vasut@gmail.com>
	<1320188059-6612-1-git-send-email-marek.vasut@gmail.com>
	<4EB1C737.20702@freescale.com>
Message-ID: <201111030115.35835.marek.vasut@gmail.com>
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 11/01/2011 05:54 PM, Marek Vasut wrote:
> > +static void spl_onenand_get_geometry(struct spl_onenand_data *data)
> > +{
[...]

> > +	/* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
> > +	if (data.pagesize == 2048) {
> > +		total_pages = len / 2048;
> > +		page = offset / 2048;
> > +		total_pages += !!(len & 2047);
> > +	} else if (data.pagesize == 4096) {
> > +		total_pages = len / 4096;
> > +		page = offset / 4096;
> > +		total_pages += !!(len & 4095);
> > +	}
> 
> What's wrong with DIV_ROUND_UP?  It should produce smaller code than
> what you've done here...

It pulls in aeabi_*div* functions, which won't fit into block 0 of Onenand.

> 
> > +	for (; page <= total_pages; page++) {
> > +		block = page / ONENAND_PAGES_PER_BLOCK;
> > +		rpage = page & (ONENAND_PAGES_PER_BLOCK - 1);
> > +		ret = spl_onenand_read_page(block, rpage, addr, data.pagesize);
> > +		if (ret) {
> > +			total_pages++;
> > +			err |= 1;
> > +		} else
> > +			addr += data.pagesize / 4;
> > +	}
> 
> As discussed, please retain the existing block-skipping semantics.  And
> if you do skip a block, that's not an error to be propagated upward (as
> opposed to something like an uncorrectable ECC error).
> 
> If one side of an if statement requires braces, both sides should have
> them.
> 
> > diff --git a/include/onenand_uboot.h b/include/onenand_uboot.h
> > index 92279d5..fcb50ff 100644
> > --- a/include/onenand_uboot.h
> > +++ b/include/onenand_uboot.h
> > @@ -16,6 +16,8 @@
> > 
> >  #include <linux/types.h>
> > 
> > +#ifndef	CONFIG_SPL_BUILD
> > +
> 
> Please use a space rather than a tab after #define, #ifndef, etc.
> 
> >  /* Forward declarations */
> >  struct mtd_info;
> >  struct mtd_oob_ops;
> > 
> > @@ -52,4 +54,10 @@ extern int flexonenand_set_boundary(struct mtd_info
> > *mtd, int die,
> > 
> >  extern void s3c64xx_onenand_init(struct mtd_info *);
> >  extern void s3c64xx_set_width_regs(struct onenand_chip *);
> > 
> > +#else
> > +
> > +int spl_onenand_load_image(uint32_t dst, uint32_t offset, uint32_t len);
> > +
> > +#endif
> 
> Why does this need to be ifdeffed at all?  We normally don't ifdef
> header declarations.
> 
> -Scott