| Message ID | 1345790488.2848.252.camel@sauron.fi.intel.com |
|---|---|
| State | New, archived |
| Headers | show |
于 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
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 > > > >
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
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
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;