Patchwork [v3,13/28] mtd: nand: pxa3xx: Add bad block handling

login
register
mail settings
Submitter Ezequiel Garcia
Date Nov. 5, 2013, 12:55 p.m.
Message ID <1383656135-8627-14-git-send-email-ezequiel.garcia@free-electrons.com>
Download mbox | patch
Permalink /patch/288525/
State Superseded
Headers show

Comments

Ezequiel Garcia - Nov. 5, 2013, 12:55 p.m.
Add support for flash-based bad block table using Marvell's
custom in-flash bad block table layout. The support is enabled
a 'flash_bbt' platform data or device tree parameter.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c                | 33 +++++++++++++++++++++++++++
 include/linux/platform_data/mtd-nand-pxa3xx.h |  3 +++
 2 files changed, 36 insertions(+)
Brian Norris - Nov. 5, 2013, 6:23 p.m.
Hi Ezequiel,

I wrote up some comments on your v2 series while on a plane Sunday, but
I didn't make time to send them out until now. Oh well.

On Tue, Nov 05, 2013 at 09:55:20AM -0300, Ezequiel Garcia wrote:
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -1128,6 +1152,14 @@ KEEP_CONFIG:
>  
>  	if (nand_scan_ident(mtd, 1, def))
>  		return -ENODEV;
> +
> +	if (pdata->flash_bbt) {
> +		chip->bbt_options |= NAND_BBT_USE_FLASH |
> +				     NAND_BBT_NO_OOB_BBM;

You're using NAND_BBT_NO_OOB_BBM? So you are unable to write bad block
markers to flash at all? Is this related to your independent patch for
trying to scan BBM from the data area? Could you instead write a
nand_chip.block_markbad() callback routine that would program BBM to the
appropriate data area?

Or, if you really want to avoid programming new BBMs, then you should
probably describe this decision in the patch description more clearly.

> +		chip->bbt_td = &bbt_main_descr;
> +		chip->bbt_md = &bbt_mirror_descr;
> +	}
> +
>  	/* calculate addressing information */
>  	if (mtd->writesize >= 2048)
>  		host->col_addr_cycles = 2;
> @@ -1323,6 +1355,7 @@ static int pxa3xx_nand_probe_dt(struct platform_device *pdev)
>  	if (of_get_property(np, "marvell,nand-keep-config", NULL))
>  		pdata->keep_config = 1;
>  	of_property_read_u32(np, "num-cs", &pdata->num_cs);
> +	pdata->flash_bbt = of_get_nand_on_flash_bbt(np);

Now that you're using the "nand-on-flash-bbt" property, you should
document it in Documentation/devicetree/bindings/mtd/pxa3xx-nand.txt
like the other drivers do. (It's already documented generically in
Documentation/.../nand.txt, but I think it's good practice to explicitly
note which drivers support the binding, since nand_base doesn't do this
generically for all NAND drivers.)

>  
>  	pdev->dev.platform_data = pdata;
>  

Brian
Ezequiel Garcia - Nov. 5, 2013, 11:40 p.m.
On Tue, Nov 05, 2013 at 10:23:01AM -0800, Brian Norris wrote:
> Hi Ezequiel,
> 
> I wrote up some comments on your v2 series while on a plane Sunday, but
> I didn't make time to send them out until now. Oh well.
> 

No problem. I just wanted to push a new version with the minor fixes
from Huang to prevent from stalling.

> On Tue, Nov 05, 2013 at 09:55:20AM -0300, Ezequiel Garcia wrote:
> > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > @@ -1128,6 +1152,14 @@ KEEP_CONFIG:
> >  
> >  	if (nand_scan_ident(mtd, 1, def))
> >  		return -ENODEV;
> > +
> > +	if (pdata->flash_bbt) {
> > +		chip->bbt_options |= NAND_BBT_USE_FLASH |
> > +				     NAND_BBT_NO_OOB_BBM;
> 
> You're using NAND_BBT_NO_OOB_BBM? So you are unable to write bad block
> markers to flash at all? Is this related to your independent patch for
> trying to scan BBM from the data area?

Yes.

> Could you instead write a
> nand_chip.block_markbad() callback routine that would program BBM to the
> appropriate data area?
> 

No :-)

> Or, if you really want to avoid programming new BBMs, then you should
> probably describe this decision in the patch description more clearly.
> 

Right.

I'll have to describe a bunch of stuff about the controller so this
NO_OOB_BBM makes sense. Please bare with me and keep reading :)

The central issue and the main difficulty is the "splitted"
data/oob/data/oob way of regarding a page.

This is intrinsic to the hardware and we must learn to deal with it.
So, let's suppose we have 4K pages, and the manufacturer marks a block at
offset 4096 (the 'x' is offset 4096).

-----------------------------------------------
|                    Data           |x  OOB   |
-----------------------------------------------

When this same page is 'viewed' by the driver, and because of the
splitted layout, the data and OOB regions are now at different
locations. It would be something like this:

-----------------------------------------------
|      Data      | OOB |      Data   x  | OOB |
-----------------------------------------------

The offset *in the data region* depends in the controller configuration,
but considering we have a 32B and 30B ECC, the calculation would give:

 2048 + 2048 - 32 - 30 = 4034.

So, if I use nanddump to dump a page, I would have to look at offset
4034 to find the factory bad block marker.

For this reason, why need to use a customize bad block scanning.

In addition, this means under regular usage we will write such position
(since it belongs to the data region) and every used block is likely
to be marked as bad.

So, there's no point in marking a block as bad, because good blocks
are *also* mark as bad. We need to rely in the bad block table, and only
perform the scan in on the very first time (when the device is unused).

We're aware this sounds kind of crappy since we'll get completely screwed
in case the bad block table is somehow lost or corrupted, but we don't
care about such case.

Still, I'd like to know:

1. Do you think the bad block table could be corrupted or is this not
likely to ever happen?

2. Do you have any ideas to 'avoid' writing to the marker? or maybe to
otherwise scan the factory markers the first time, but then use some
other position for the kernel in-flash BB marker?

> > +		chip->bbt_td = &bbt_main_descr;
> > +		chip->bbt_md = &bbt_mirror_descr;
> > +	}
> > +
> >  	/* calculate addressing information */
> >  	if (mtd->writesize >= 2048)
> >  		host->col_addr_cycles = 2;
> > @@ -1323,6 +1355,7 @@ static int pxa3xx_nand_probe_dt(struct platform_device *pdev)
> >  	if (of_get_property(np, "marvell,nand-keep-config", NULL))
> >  		pdata->keep_config = 1;
> >  	of_property_read_u32(np, "num-cs", &pdata->num_cs);
> > +	pdata->flash_bbt = of_get_nand_on_flash_bbt(np);
> 
> Now that you're using the "nand-on-flash-bbt" property, you should
> document it in Documentation/devicetree/bindings/mtd/pxa3xx-nand.txt
> like the other drivers do. (It's already documented generically in
> Documentation/.../nand.txt, but I think it's good practice to explicitly
> note which drivers support the binding, since nand_base doesn't do this
> generically for all NAND drivers.)
>

Yeah, good idea.

Thanks,
Brian Norris - Nov. 6, 2013, 1:36 a.m.
On Tue, Nov 5, 2013 at 3:40 PM, Ezequiel Garcia
<ezequiel.garcia@free-electrons.com> wrote:
> On Tue, Nov 05, 2013 at 10:23:01AM -0800, Brian Norris wrote:
>> On Tue, Nov 05, 2013 at 09:55:20AM -0300, Ezequiel Garcia wrote:
>> > --- a/drivers/mtd/nand/pxa3xx_nand.c
>> > +++ b/drivers/mtd/nand/pxa3xx_nand.c
>> > @@ -1128,6 +1152,14 @@ KEEP_CONFIG:
>> >
>> >     if (nand_scan_ident(mtd, 1, def))
>> >             return -ENODEV;
>> > +
>> > +   if (pdata->flash_bbt) {
>> > +           chip->bbt_options |= NAND_BBT_USE_FLASH |
>> > +                                NAND_BBT_NO_OOB_BBM;
>>
>> You're using NAND_BBT_NO_OOB_BBM? So you are unable to write bad block
>> markers to flash at all? Is this related to your independent patch for
>> trying to scan BBM from the data area?
>
> Yes.
>
>> Could you instead write a
>> nand_chip.block_markbad() callback routine that would program BBM to the
>> appropriate data area?
>
> No :-)

Well given the reset of your comments, I guess you could write an
empty one (or one with a BUG() or WARN()?).

>> Or, if you really want to avoid programming new BBMs, then you should
>> probably describe this decision in the patch description more clearly.
>>
>
> Right.
>
> I'll have to describe a bunch of stuff about the controller so this
> NO_OOB_BBM makes sense. Please bare with me and keep reading :)

[snip nice description; I did read it!]

> So, there's no point in marking a block as bad, because good blocks
> are *also* mark as bad. We need to rely in the bad block table, and only
> perform the scan in on the very first time (when the device is unused).

Right. I didn't quite think through this whole process.

I think a short (few lines) comment in the code to describe the
justification for using NAND_BBT_NO_OOB_BBM is in order for v4. And
maybe include a bit of this in the commit message as well.

> We're aware this sounds kind of crappy since we'll get completely screwed
> in case the bad block table is somehow lost or corrupted, but we don't
> care about such case.
>
> Still, I'd like to know:
>
> 1. Do you think the bad block table could be corrupted or is this not
> likely to ever happen?

Yes, it can be. But no, I don't think it's likely. There are very few,
rare instances where we have to modify the BBT (and thereby make it
susceptible to corruption), and those instances have some level of
robustness to them. Of course, they still have room for improvement.
(I suppose there could as be corruption due to read disturb; but this
is also handled now, by scrubbing the affected BBT blocks that return
-EUCLEAN, refreshing them with clean data.)

Personally, I've experienced "corruption" primarily when I have boards
where I change the ECC configuration; then the BBT scan sees -EBADMSG
and has to rebuild the table.

> 2. Do you have any ideas to 'avoid' writing to the marker? or maybe to
> otherwise scan the factory markers the first time, but then use some
> other position for the kernel in-flash BB marker?

Hmm, not really. We've kind of co-opted the idea of the factory bad
block marker as a secondary, distributed bad block table in the
kernel. It's not really the expected use case, and it breaks in cases
like yours, since we don't support a third form of bad block
marker/table for post-initial-scan markers. I think the best solution
in this case is just to rely solely on the BBT after the first scan.

Brian

Patch

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index e198c94..9a5913d 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -24,6 +24,7 @@ 
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_mtd.h>
 
 #if defined(CONFIG_ARCH_PXA) || defined(CONFIG_ARCH_MMP)
 #define ARCH_HAS_DMA
@@ -245,6 +246,29 @@  static struct pxa3xx_nand_flash builtin_flash_types[] = {
 { "256MiB 16-bit", 0xba20,  64, 2048, 16, 16, 2048, &timing[3] },
 };
 
+static u8 bbt_pattern[] = {'M', 'V', 'B', 'b', 't', '0' };
+static u8 bbt_mirror_pattern[] = {'1', 't', 'b', 'B', 'V', 'M' };
+
+static struct nand_bbt_descr bbt_main_descr = {
+	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
+		| NAND_BBT_2BIT | NAND_BBT_VERSION,
+	.offs =	8,
+	.len = 6,
+	.veroffs = 14,
+	.maxblocks = 8,		/* Last 8 blocks in each chip */
+	.pattern = bbt_pattern
+};
+
+static struct nand_bbt_descr bbt_mirror_descr = {
+	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
+		| NAND_BBT_2BIT | NAND_BBT_VERSION,
+	.offs =	8,
+	.len = 6,
+	.veroffs = 14,
+	.maxblocks = 8,		/* Last 8 blocks in each chip */
+	.pattern = bbt_mirror_pattern
+};
+
 /* Define a default flash type setting serve as flash detecting only */
 #define DEFAULT_FLASH_TYPE (&builtin_flash_types[0])
 
@@ -1128,6 +1152,14 @@  KEEP_CONFIG:
 
 	if (nand_scan_ident(mtd, 1, def))
 		return -ENODEV;
+
+	if (pdata->flash_bbt) {
+		chip->bbt_options |= NAND_BBT_USE_FLASH |
+				     NAND_BBT_NO_OOB_BBM;
+		chip->bbt_td = &bbt_main_descr;
+		chip->bbt_md = &bbt_mirror_descr;
+	}
+
 	/* calculate addressing information */
 	if (mtd->writesize >= 2048)
 		host->col_addr_cycles = 2;
@@ -1323,6 +1355,7 @@  static int pxa3xx_nand_probe_dt(struct platform_device *pdev)
 	if (of_get_property(np, "marvell,nand-keep-config", NULL))
 		pdata->keep_config = 1;
 	of_property_read_u32(np, "num-cs", &pdata->num_cs);
+	pdata->flash_bbt = of_get_nand_on_flash_bbt(np);
 
 	pdev->dev.platform_data = pdata;
 
diff --git a/include/linux/platform_data/mtd-nand-pxa3xx.h b/include/linux/platform_data/mtd-nand-pxa3xx.h
index ffb8019..a941471 100644
--- a/include/linux/platform_data/mtd-nand-pxa3xx.h
+++ b/include/linux/platform_data/mtd-nand-pxa3xx.h
@@ -55,6 +55,9 @@  struct pxa3xx_nand_platform_data {
 	/* indicate how many chip selects will be used */
 	int	num_cs;
 
+	/* use an flash-based bad block table */
+	bool	flash_bbt;
+
 	const struct mtd_partition		*parts[NUM_CHIP_SELECT];
 	unsigned int				nr_parts[NUM_CHIP_SELECT];