On 06/28/2012 03:47 PM, Rafael Beims wrote:
> Freescale FCM controller has a 2K size limitation of buffer RAM. In order
> to support the Nand flash chip with pagesize larger than 2K bytes,
> we read/write 2k data repeatedly by issuing FIR_OP_RB/FIR_OP_WB and save
> them to a large buffer.
> Because of this, the in flash layout of the oob is different from the
> default for 4096kiB page sizes. Therefore, we need to migrate the
> factory bad block markers from the original position to the new layout.
>
> Signed-off-by: Shengzhou Liu <shengzhou....@freescale.com>
> Signed-off-by: Liu Shuo <b35...@freescale.com>
> Signed-off-by: Rafael Beims <rafael.be...@datacom.ind.br>
> ---
> Changes in v2:
> - Added check to disallow the migration code to run in devices with
> page size <= 2048
>
> drivers/mtd/nand/fsl_elbc_nand.c | 447
> +++++++++++++++++++++++++++++++++++---
> 1 files changed, 419 insertions(+), 28 deletions(-)
Thanks for working on this! I've been meaning to for a while, but have
been busy with other things.
> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c
> b/drivers/mtd/nand/fsl_elbc_nand.c
> index 9076ad4..3b6bb1d 100644
> --- a/drivers/mtd/nand/fsl_elbc_nand.c
> +++ b/drivers/mtd/nand/fsl_elbc_nand.c
> @@ -76,15 +76,17 @@ struct fsl_elbc_ctrl {
>
> /* device info */
> fsl_lbc_t *regs;
> - u8 __iomem *addr; /* Address of assigned FCM buffer */
> - unsigned int page; /* Last page written to / read from */
> - unsigned int read_bytes; /* Number of bytes read during command */
> - unsigned int column; /* Saved column from SEQIN */
> - unsigned int index; /* Pointer to next byte to 'read' */
> - unsigned int status; /* status read from LTESR after last op */
> - unsigned int mdr; /* UPM/FCM Data Register value */
> - unsigned int use_mdr; /* Non zero if the MDR is to be set */
> - unsigned int oob; /* Non zero if operating on OOB data */
> + u8 __iomem *addr; /* Address of assigned FCM buffer */
> + unsigned int page; /* Last page written to / read from */
> + unsigned int read_bytes; /* Number of bytes read during command */
> + unsigned int column; /* Saved column from SEQIN */
> + unsigned int index; /* Pointer to next byte to 'read' */
> + unsigned int status; /* status read from LTESR after last op */
> + unsigned int mdr; /* UPM/FCM Data Register value */
> + unsigned int use_mdr; /* Non zero if the MDR is to be set */
> + unsigned int oob; /* Non zero if operating on OOB data */
> + char *buffer; /* Just used when pagesize is greater */
> + /* than FCM RAM 2K limitation */
> };
>
> /* These map to the positions used by the FCM hardware ECC generator */
> @@ -131,6 +133,15 @@ static struct nand_bbt_descr largepage_memorybased = {
> .pattern = scan_ff_pattern,
> };
>
> +static u8 migrated_pattern[] = { 0xde, 0xad, 0xde, 0xad };
Let's generate a proper random number here -- or at least a meaningful
ASCII string. Things like the above are too common and invite magic
number collision.
> +static int fsl_elbc_migrate_badblocks(struct mtd_info *mtd,
> + struct nand_bbt_descr *bd)
> +{
> + struct nand_chip *this = mtd->priv;
> + int len, numblocks, i;
> + int startblock;
> + loff_t from;
> + uint8_t *newoob, *buf;
> +
> + struct fsl_elbc_mtd *priv = this->priv;
> + struct fsl_elbc_ctrl *ctrl = priv->ctrl;
> +
> + int num_subpages = mtd->writesize / 2048-1;
> + len = mtd->writesize + mtd->oobsize;
> + numblocks = this->chipsize >> (this->phys_erase_shift - 1);
> + startblock = 0;
> + from = 0;
> +
> + newoob = vmalloc(mtd->oobsize);
Even if this were Linux, oobsize should never be big enough to need vmalloc.
> + memset(newoob, 0xff, mtd->writesize+mtd->oobsize);
You're writing beyond the end of that buffer.
> + for (i = startblock; i < numblocks; i += 2) {
> + int page = (from >> this->page_shift) & this->pagemask;
> + fsl_elbc_cmdfunc(mtd, NAND_CMD_READ0, 0, page);
> +
> + /* As we are already using the hack to read the bytes
> + * from NAND, the original badblock marker is offset
> + * from its original location in the internal buffer.
> + * This is because the hack reads 2048 + 64 and already
> + * positions the spare in the correct region
> + * (after the 4096 offset)
> + */
> + uint8_t *badblock_pattern = (ctrl->buffer+
> + mtd->writesize-(num_subpages*64))+bd->offs;
Spaces around operators.
I think that should be (num_subpages - 1) * 64.
> + if (fsl_elbc_check_pattern(mtd, badblock_pattern, bd)) {
> + printf("Badblock found at %08x, migrating...\n", page);
> + memcpy(newoob, badblock_pattern , bd->len);
No space before ,
> +static int fsl_elbc_write_migration_marker(struct mtd_info *mtd,
> + uint8_t *buf, int len, struct nand_bbt_descr *bd)
> +{
> + struct nand_chip *this = mtd->priv;
> + struct nand_bbt_descr *td = this->bbt_td;
> + int startblock;
> + int dir, i;
> + int blocks_to_write = 2;
> +
> + /* Start below maximum bbt */
> + startblock = (mtd->size >> this->phys_erase_shift) - td->maxblocks;
> + dir = -1;
If you want startblock below the bbt area, I think you're off by one.
> + for (i = 0; i < bd->maxblocks && blocks_to_write != 0; i++) {
> + int actblock = startblock + dir*i;
> + loff_t offs = (loff_t)actblock << this->phys_erase_shift;
> + int page = (offs >> this->page_shift) & this->pagemask;
> +
> + /* Avoid badblocks writing the marker... */
> + if (!fsl_elbc_scan_read_raw_oob(mtd, buf, page, mtd->writesize,
> + &largepage_memorybased)) {
> +
> + /* We are reusing this buffer, reset it */
> + memset(buf, 0xff, len);
> + memcpy(buf+bd->offs, bd->pattern, bd->len);
> +
> + /* Mark the block as bad the same way that
> + * it's done for the bbt. This should avoid
> + * this block being overwritten
> + */
> + memset(buf+this->badblockpos, 0x02, 1);
> +
> + fsl_elbc_cmdfunc(mtd, NAND_CMD_SEQIN,
> + mtd->writesize, page);
> + fsl_elbc_write_buf(mtd, buf, mtd->oobsize);
> + fsl_elbc_cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> + blocks_to_write--;
> + printf("Wrote migration marker to offset: %x\n", page);
> + }
Why are we writing the marker twice?
> +static int fsl_elbc_scan_bbt(struct mtd_info *mtd)
> +{
> + struct nand_chip *this = mtd->priv;
> + struct nand_bbt_descr *bd = &largepage_migrated;
> + struct nand_bbt_descr *td = this->bbt_td;
> + int len;
> + int startblock, block, dir;
> + uint8_t *buf;
> + int migrate = 0;
> +
> + if (mtd->writesize > 2048) {
> + /* Start below maximum bbt */
> + startblock = (mtd->size >> this->phys_erase_shift) -
> td->maxblocks;
Again, off by one.
> + dir = -1;
dir never seems to get set to anything else, so why a variable?
I think we should start with the last block, and search backwards until
we find it, or until we exceed some reasonable limit, such as
td->maxblocks * 2 (Is that really enough? How many contiguous bad
blocks can we see?). Actually writing a joint bbt/migration marker
(which was requested in earlier discussion to avoid wasting an erase
block, but I don't want it to be mandatory) can come later, but we want
to recognize it now.
> + /* Allocate a temporary buffer for one eraseblock incl. oob */
> + len = (1 << this->phys_erase_shift);
> + len += (len >> this->page_shift) * mtd->oobsize;
> + buf = vmalloc(len);
This code isn't going to be shared with Linux, so just malloc().
Likewise printf(...) instead of printk(KERN_ERR ...).
BTW, should mention at least in the changelog that this is partially
derived from code in nand_bbt.c. Also, using this code means we need to
remove the "or later" from fsl_elbc_nand.c, because nand_bbt.c is GPL v2
only. Wolfgang probably won't be pleased. It might be better to just
write it from scratch -- I'm not sure how much it really helps to be
mimicking the bbt code here (without actually being able to share it),
versus straightforwardly coding this specific task.
> + if (!buf) {
> + printk(KERN_ERR "fsl_elbc_nand: Out of memory\n");
> + kfree(this->bbt);
> + this->bbt = NULL;
> + return -ENOMEM;
> + }
Why are we freeing the bbt here? When did we allocate it?
> + for (block = 0; block < td->maxblocks; block++) {
> + int actblock = startblock + dir * block;
> + loff_t offs = (loff_t)actblock <<
> this->phys_erase_shift;
> + int page = (offs >> this->page_shift) & this->pagemask;
> +
> + migrate = fsl_elbc_scan_read_raw_oob(mtd, buf, page,
> + mtd->writesize, &largepage_migrated);
> +
> + /* We found the migration marker, get out of here */
> + if (migrate == 0)
> + break;
> + }
> +
> + if (migrate) {
> + printf("Moving factory marked badblocks to new oob\n");
> + fsl_elbc_migrate_badblocks(mtd, &largepage_memorybased);
> + fsl_elbc_write_migration_marker(mtd, buf, len,
> + &largepage_migrated);
> + }
> +
> + vfree(buf);
> + }
> + /* Now that we checked and possibly migrated badblock
> + * markers, continue with default bbt scanning */
/*
* U-Boot multiline comment style
* is like this.
*/
Again, thanks for working on this. To properly test it I need to get
raw reads/writes working properly with eLBC, though. Once I do that,
I'll fix this patch up (unless you want to do a v3 before then).
-Scott
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
