| Message ID | 1383656135-8627-14-git-send-email-ezequiel.garcia@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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,
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
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];
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(+)