From mboxrd@z Thu Jan  1 00:00:00 1970
From: Florian Fainelli <florian@openwrt.org>
Date: Sat, 11 Dec 2010 18:08:36 +0100
Subject: [U-Boot] [U-Boot,
	2/2] NAND: add support for reading ONFI page table
In-Reply-To: <20101211001513.GA12252@udp111988uds.am.freescale.net>
References: <1292019402-25433-2-git-send-email-florian@openwrt.org>
	<20101211001513.GA12252@udp111988uds.am.freescale.net>
Message-ID: <201012111808.36967.florian@openwrt.org>
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 Saturday 11 December 2010 01:15:13 Scott Wood wrote:
> On Fri, Dec 10, 2010 at 12:16:42PM -0000, Florian Fainelli wrote:
> > From: Florian Fainelli <florian@openwrt.org>
> > 
> > This patch adds support for reading an ONFI page parameter from a NAND
> > device supporting it. If this is the case, struct nand_chip onfi_version
> > member contains the supported ONFI version, 0 otherwise.
> > 
> > This allows NAND drivers past nand_scan_ident to set the best timings for
> > the NAND chip.
> 
> Wrap changelogs around 72 characters, as git log indents them a bit.
> 
> > Signed-off-by: Florian Fainelli <florian@openwrt.org>
> > 
> > ---
> > drivers/mtd/nand/nand_base.c |  129
> > ++++++++++++++++++++++++++++++++++-------
> > 
> >  include/linux/mtd/nand.h     |   68 ++++++++++++++++++++++
> >  2 files changed, 175 insertions(+), 22 deletions(-)
> 
> Tested on what I assume is a non-ONFI chip, no breakage.
> 
> Please wrap this in a CONFIG option, so that we don't increase the
> image size of boards that don't need to support this.

Makes sense, I will do this

> 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 21cc5a3..a3a0507 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -2406,15 +2406,94 @@ static void nand_set_defaults(struct nand_chip
> > *chip, int busw)
> > 
> >  		chip->controller = &chip->hwcontrol;
> >  
> >  }
> > 
> > +static u16 onfi_crc16(u16 crc, u8 const *p, size_t len)
> > +{
> > +	int i;
> > +
> > +	while (len--) {
> > +		crc ^= *p++ << 8;
> > +		for (i = 0; i < 8; i++)
> > +			crc = (crc << 1) ^ ((crc & 0x8000) ? 0x8005 : 0);
> > +	}
> > +
> > +	return crc;
> > +}
> > +
> > +/*
> > + * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0
> > otherwise + */
> > +static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip
> > *chip, +								int busw)
> > +{
> 
> Keep lines under 80 columns.
> 
> > +
> > +	/* try ONFI for unknow chip or LP */
> 
> "unknown".
> 
> > +	chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
> > +	if (chip->read_byte(mtd) != 'O' || chip->read_byte(mtd) != 'N' ||
> > +		chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I')
> > +		return 0;
> 
> I'm not sure how this comment matches the code -- it looks like it's
> checking for an explicit ONFI ID.  I don't see anything about unknown
> chips or large pages.

It does not actually, I did not think about checking it when porting this from Linux.

> 
> > +	/* check version */
> > +	val = le16_to_cpu(p->revision);
> > +	if (val == 1 || val > (1 << 4)) {
> > +		printk(KERN_INFO "%s: unsupported ONFI version: %d\n",
> > +								__func__, val);
> > +		return 0;
> > +	}
> 
> Ideally I'd like to see continuation lines lined up nicely with
> the start of arguments on the previous line, but at least
> don't tab it all the way over to the right edge of the screen.
> 
> >  	chip->chipsize = (uint64_t)type->chipsize << 20;
> > 
> > +	chip->onfi_version = 0;
> > +
> > +	ret = nand_flash_detect_onfi(mtd, chip, busw);
> > +	if (ret)
> > +		goto ident_done;
> 
> Move the non-ONFI code out into its own function, instead of using
> goto.
> 
> > +	__le32 blocks_per_lun;
> > +	u8 lun_count;
> > +	u8 addr_cycles;
> > +	u8 bits_per_cell;
> > +	__le16 bb_per_lun;
> > +	__le16 block_endurance;
> > +	u8 guaranteed_good_blocks;
> > +	__le16 guaranteed_block_endurance;
> 
> [snip]
> 
> > +} __attribute__((packed));
> 
> Sigh.  Someone on the standards body needs to learn about alignment.
> 
> -Scott
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot