Patchwork [1/2] nand: nand_bbt: Export nand_update_bbt

login
register
mail settings
Submitter Artem Bityutskiy
Date Aug. 24, 2012, 6:41 a.m.
Message ID <1345790488.2848.252.camel@sauron.fi.intel.com>
Download mbox | patch
Permalink /patch/179771/
State New
Headers show

Comments

Artem Bityutskiy - Aug. 24, 2012, 6:41 a.m.
On Thu, 2012-08-23 at 11:36 -0400, Huang Shijie wrote:
> On Thu, Aug 23, 2012 at 11:08 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Sat, 2012-07-28 at 19:29 -0300, Fabio Estevam wrote:
> >> From: Fabio Estevam <fabio.estevam@freescale.com>
> >>
> >> When building MTD_NAND_GPMI_NAND as module, the following error shows up:
> >>
> >> ERROR: "nand_update_bbt" [drivers/mtd/nand/gpmi-nand/gpmi_nand.ko] undefined!
> >>
> >> Export nand_update_bbt to fix it
> >>
> >> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> >
> > Why this driver redefined ->block_markbad() at all, it is not supposed
> For hardware reason, in mx23, the bad block mark is stored in the
> first byte of the nand page;
> in mx28/mx50/mx6q, the bad block mark is stored in the first byte of the OOB.
> 
> That's why the driver redefined the ->block_markbad().

The default function seem to do the same as your function does. You
select where you keep your OOB using chip options, and the default
function does the right thing, no?

From 5cdff78da6dde1e2eef472dcbbe7ca8f7fd64061 Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Date: Fri, 24 Aug 2012 09:26:29 +0300
Subject: [PATCH] gpmi-nand: use the default implementation of block_markbad

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   46 --------------------------------
 1 file changed, 46 deletions(-)
Huang Shijie - Aug. 24, 2012, 10:35 a.m.
于 2012年08月24日 14:41, Artem Bityutskiy 写道:
> The default function seem to do the same as your function does. You
> select where you keep your OOB using chip options, and the default
> function does the right thing, no?
The key issue is that the mx23 should copies the bad block mark into the 
"first byte of the nand page",
while the other chips does do this.

I don't think the default function do the same thing. You see:
   nand_default_block_markbad() --> nand_do_write_oob() --> 
chip->ecc.write_oob() -->gpmi_ecc_write_oob()

The gpmi_ecc_write_oob() does nothing,but return -EPERM.

Huang Shijie
Huang Shijie - Aug. 24, 2012, 10:35 a.m.
On Fri, Aug 24, 2012 at 6:35 PM, Huang Shijie <b32955@freescale.com> wrote:
> 于 2012年08月24日 14:41, Artem Bityutskiy 写道:
>
>> The default function seem to do the same as your function does. You
>> select where you keep your OOB using chip options, and the default
>> function does the right thing, no?
>
> The key issue is that the mx23 should copies the bad block mark into the
> "first byte of the nand page",
> while the other chips does do this.

s/does/does not/

>
> I don't think the default function do the same thing. You see:
>   nand_default_block_markbad() --> nand_do_write_oob() -->
> chip->ecc.write_oob() -->gpmi_ecc_write_oob()
>
> The gpmi_ecc_write_oob() does nothing,but return -EPERM.
>
> Huang Shijie
>
>
>
>
Brian Norris - Aug. 24, 2012, 10:57 p.m.
Hi,

From Artem: "Why this driver redefined ->block_markbad() at all, it is
not supposed
to do this. We should fix the driver instead."

Why is ->block_markbad() a function pointer, then, if the driver is
not allowed to redefine it? Admittedly, it often doesn't make sense to
override it; plus, if you override ->block_markbad(), you probably
want to also override ->block_bad(). But nand_base.c doesn't really
have good infrastructure for that.

So, by the current, somewhat messy and incomplete state of the
nand_base + nand_bbt code, it seems to be "supported" to override
->block_markbad() if you really need to. But some more work could be
done to improve this.

On Fri, Aug 24, 2012 at 3:35 AM, Huang Shijie <b32955@freescale.com> wrote:
> 于 2012年08月24日 14:41, Artem Bityutskiy 写道:
>
>> The default function seem to do the same as your function does. You
>> select where you keep your OOB using chip options, and the default
>> function does the right thing, no?
>
> I don't think the default function do the same thing. You see:
>   nand_default_block_markbad() --> nand_do_write_oob() -->
> chip->ecc.write_oob() -->gpmi_ecc_write_oob()
>
> The gpmi_ecc_write_oob() does nothing,but return -EPERM.

Yeah, that's kind of strange. So you never have room in OOB even for
bad block markers? How do you identify factory-marked bad blocks?
Wouldn't they be indistinguishable from your ECC area? Or do you have
free OOB space in which you could actually implement write_oob()
properly?

None of my comments are really a disagreement with your patch. Your
driver has a strange way of dealing with ECC + bad block markers, and
assuming you figured out your swap_block_mark code safely (I didn't
really study this) then I think it's OK ("supported") to provide your
own ->block_markbad() function.

Brian
Huang Shijie - Aug. 26, 2012, 1:05 p.m.
On Fri, Aug 24, 2012 at 6:57 PM, Brian Norris
<computersforpeace@gmail.com> wrote:

>>> The default function seem to do the same as your function does. You
>>> select where you keep your OOB using chip options, and the default
>>> function does the right thing, no?
>>
>> I don't think the default function do the same thing. You see:
>>   nand_default_block_markbad() --> nand_do_write_oob() -->
>> chip->ecc.write_oob() -->gpmi_ecc_write_oob()
>>
>> The gpmi_ecc_write_oob() does nothing,but return -EPERM.
>
> Yeah, that's kind of strange. So you never have room in OOB even fo

yes. The different ROMs of mx23/mx28/mx50/mx6q make it strange.

In mx28/mx50/mx6q, the first byte of OOB is used to store the bad
block marker. These chips's ROMs support the so-called `swap` feature which can
swaps the bad block marker to a safe place.

But the mx23's ROM does not support the swap feature, so it really has
no room in OOB to
store the bad block marker. When the mx23 first scan a nand chip, the
factory-marked
 bad block marker is copied to the first byte of the NAND page. When
the chip boots from
the nand, the ROM of mx23 will  read the first byte of the NAND page
to check whether this block is a bad block. Why copy to the first
byte? the mx23 use the first 10 byte of the nand page as a so-called
"metadata".

> bad block markers? How do you identify factory-marked bad blocks?
> Wouldn't they be indistinguishable from your ECC area? Or do you have
> free OOB space in which you could actually implement write_oob()
> properly?

The original old gpmi-nand driver does implement the write_oob() which
only allow the
block_markbad() run through. In another word, if the write_oob() is
called by the block_markbad, it will
grant the operation. All the other operations are denied. In order to
achieve this target, the old
code used an ugly hack code, it hooked the mtd->block_markbad(), such as:
         .....................................................
               mtd->hooked_block_markbad = mtd->block_markbad();
               mtd->block_markbad =              gpmi_block_markbad();

               nand->ecc.write_oob = mil_ecc_write_oob;
         .....................................................

             gpmi_block_markbad()
          {
               this->marking_a_block_bad = true;
              mtd->hooked_block_markbad();
              this->marking_a_block_bad = false;
           }

        mil_ecc_write_oob()
        {
            if (!this->marking_a_block_bad)
                    return;
            ..............................................
        }

     As i described above, the mil_ecc_write_oob() will write 0 to the
first byte of the OOB in mx28/mx50/mx6q;
the mil_ecc_write_oob() will write 0 to the first byte of the nand page in mx23.

For you see, the implementation of the write_oob() is too ugly. I
fininally found if i implement the nand->block_markbad(), the code
will very tidy and clean. The current code realizes the same feature
as
the old code, but the current code is very simple.

what's more is that the nand->block_markbad is `REPLACEABLE`. so i do
not break any rule. :)


I hope my poor english describes this issue clearly.

thanks
Huang Shijie

Patch

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 8c0d2f0..8e193fb 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1179,51 +1179,6 @@  gpmi_ecc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int page)
 	return -EPERM;
 }
 
-static int gpmi_block_markbad(struct mtd_info *mtd, loff_t ofs)
-{
-	struct nand_chip *chip = mtd->priv;
-	struct gpmi_nand_data *this = chip->priv;
-	int block, ret = 0;
-	uint8_t *block_mark;
-	int column, page, status, chipnr;
-
-	/* Get block number */
-	block = (int)(ofs >> chip->bbt_erase_shift);
-	if (chip->bbt)
-		chip->bbt[block >> 2] |= 0x01 << ((block & 0x03) << 1);
-
-	/* Do we have a flash based bad block table ? */
-	if (chip->bbt_options & NAND_BBT_USE_FLASH)
-		ret = nand_update_bbt(mtd, ofs);
-	else {
-		chipnr = (int)(ofs >> chip->chip_shift);
-		chip->select_chip(mtd, chipnr);
-
-		column = this->swap_block_mark ? mtd->writesize : 0;
-
-		/* Write the block mark. */
-		block_mark = this->data_buffer_dma;
-		block_mark[0] = 0; /* bad block marker */
-
-		/* Shift to get page */
-		page = (int)(ofs >> chip->page_shift);
-
-		chip->cmdfunc(mtd, NAND_CMD_SEQIN, column, page);
-		chip->write_buf(mtd, block_mark, 1);
-		chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
-
-		status = chip->waitfunc(mtd, chip);
-		if (status & NAND_STATUS_FAIL)
-			ret = -EIO;
-
-		chip->select_chip(mtd, -1);
-	}
-	if (!ret)
-		mtd->ecc_stats.badblocks++;
-
-	return ret;
-}
-
 static int nand_boot_set_geometry(struct gpmi_nand_data *this)
 {
 	struct boot_rom_geometry *geometry = &this->rom_geometry;
@@ -1562,7 +1517,6 @@  static int __devinit gpmi_nfc_init(struct gpmi_nand_data *this)
 	chip->ecc.write_oob	= gpmi_ecc_write_oob;
 	chip->scan_bbt		= gpmi_scan_bbt;
 	chip->badblock_pattern	= &gpmi_bbt_descr;
-	chip->block_markbad	= gpmi_block_markbad;
 	chip->options		|= NAND_NO_SUBPAGE_WRITE;
 	chip->ecc.mode		= NAND_ECC_HW;
 	chip->ecc.size		= 1;