lkml.org 
[lkml]   [2020]   [Jun]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v6 2/8] mtd: rawnand: rockchip: NFC drivers for RK3308, RK2928 and others
    From
    Date
    Hi Yifeng, Miquel,

    Some more comments about swap();

    On 6/9/20 9:40 AM, Yifeng Zhao wrote:

    [..]

    > +static int rk_nfc_ooblayout_free(struct mtd_info *mtd, int section,
    > + struct mtd_oob_region *oob_region)
    > +{
    > + struct nand_chip *chip = mtd_to_nand(mtd);
    > +

    > + if (section >= chip->ecc.steps)
    > + return -ERANGE;

    Given:

    NFC_SYS_DATA_SIZE = 4
    chip->ecc.steps = 8
    section [0..7]

    Total free OOB size advertised to the MTD framework is:

    ecc.steps * NFC_SYS_DATA_SIZE - 1 BBM
    8 * 4 - 1 = 31 bytes

    With link address in OOB byte [0..3] this become:
    31 - 4 = 27 bytes

    Does that give data lost?
    Should we limit the number of free OOB bytes by 4 more to be save?
    Is my calculation correct?

    See further questions about this below.

    > +
    > + if (!section) {
    > + /* The first byte is bad block mask flag. */
    > + oob_region->length = NFC_SYS_DATA_SIZE - 1;
    > + oob_region->offset = 1;
    > + } else {
    > + oob_region->length = NFC_SYS_DATA_SIZE;
    > + oob_region->offset = section * NFC_SYS_DATA_SIZE;
    > + }
    > +
    > + return 0;
    > +}
    > +
    > +static int rk_nfc_ooblayout_ecc(struct mtd_info *mtd, int section,
    > + struct mtd_oob_region *oob_region)
    > +{
    > + struct nand_chip *chip = mtd_to_nand(mtd);
    > +

    > + if (section)
    > + return -ERANGE;

    With the formule above a section > 0 does not alow ECC.

    Just a question about the MTD inner working for Miquel:

    With ooblayout_free using 8 steps and this just 1 does it still generate
    the correct ECC? Does it calculate ECC over 1024B or over 8*1024B ?

    Should we move the text that explains the layout closer to these
    functions and add a little more text to explain why we choose this
    particular layout?

    /*
    * NFC Page Data Layout:
    * 1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc +
    * 1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc +
    * ......
    * NAND Page Data Layout:
    * 1024 * n Data + m Bytes oob
    * Original Bad Block Mask Location:
    * First byte of oob(spare).
    * nand_chip->oob_poi data layout:
    * 4Bytes sys data + .... + 4Bytes sys data + ecc data.
    */

    We expect now ECC data after n steps * 4 OOB bytes,
    but are we still using it with HW ECC or only for raw?

    > +
    > + oob_region->offset = NFC_SYS_DATA_SIZE * chip->ecc.steps;
    > + oob_region->length = mtd->oobsize - oob_region->offset;
    > +
    > + return 0;
    > +}
    > +
    > +static const struct mtd_ooblayout_ops rk_nfc_ooblayout_ops = {
    > + .free = rk_nfc_ooblayout_free,
    > + .ecc = rk_nfc_ooblayout_ecc,
    > +};

    [..]

    > +static int rk_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
    > + const u8 *buf, int page, int raw)
    > +{
    > + struct rk_nfc *nfc = nand_get_controller_data(chip);
    > + struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
    > + struct nand_ecc_ctrl *ecc = &chip->ecc;
    > + int oob_step = (ecc->bytes > 60) ? NFC_MAX_OOB_PER_STEP :
    > + NFC_MIN_OOB_PER_STEP;
    > + int pages_per_blk = mtd->erasesize / mtd->writesize;
    > + int ret = 0, i, boot_rom_mode = 0;
    > + dma_addr_t dma_data, dma_oob;
    > + u32 reg;
    > + u8 *oob;
    > +
    > + nand_prog_page_begin_op(chip, page, 0, NULL, 0);
    > +
    > + if (!raw) {
    > + memcpy(nfc->page_buf, buf, mtd->writesize);
    > + memset(nfc->oob_buf, 0xff, oob_step * ecc->steps);
    > +
    > + /*
    > + * The first 8(some devices are 4 or 16) blocks in use by
    > + * the boot ROM and the first 32 bits of oob need to link
    > + * to the next page address in the same block.
    > + * Config the ECC algorithm supported by the boot ROM.
    > + */
    > + if (page < pages_per_blk * rk_nand->boot_blks &&
    > + chip->options & NAND_IS_BOOT_MEDIUM) {
    > + boot_rom_mode = 1;
    > + if (rk_nand->boot_ecc != ecc->strength)
    > + rk_nfc_hw_ecc_setup(chip, ecc,
    > + rk_nand->boot_ecc);
    > + }
    > +
    > + /*
    > + * Swap the first oob with the seventh oob and bad block
    > + * mask is saved at the seventh oob.
    > + */
    > + swap(chip->oob_poi[0], chip->oob_poi[7]);

    Add more info on why this is swapped.

    LA[0..3] is a link address that the BBM shouldn't over write.
    For Yifeng: Is there an other reason?

    Before swap:

    BBM OOB1 OOB2 OOB3, OOB4 OOB5 OOB6 OOB7, OOB8 ....

    After swap:

    OOB7 OOB1 OOB2 OOB3, OOB4 OOB5 OOB6 BBM , OOB8 ....

    If (!i && boot_rom_mode):

    LA0 LA1 LA2 LA3 , OOB4 OOB5 OOB6 BBM , OOB8 ....

    Read back after swap again:

    BBM LA1 LA2 LA3 , OOB4 OOB5 OOB6 LA0 , OOB8 ....

    Question:
    Are data OOB7 OOB1 OOB2 OOB3 lost now?
    Is this correct?

    #################################################
    Proposal:
    Should we reduce the free OOB size by 4
    and shift everything 4 bytes to recover all bytes?
    Replace the first 4 bytes with 0XFF or LA[0..3].

    Normal:
    0xFF 0XFF 0XFF 0xFF, BBM OOB1 OOB2 OOB3, OOB4

    If (!i && boot_rom_mode):
    LA0 LA1 LA2 LA3 , BBM OOB1 OOB2 OOB3, OOB4

    Question for Miquel and Yifeng:
    Does this work? Could you test?

    > +
    > + for (i = 0; i < ecc->steps; i++) {

    Just a proposel:

    if (!i && boot_rom_mode)
    reg = (page & (pages_per_blk - 1)) * 4;
    else if (!i)
    reg = 0xFFFFFFFF;
    else
    oob = chip->oob_poi + (i-1) * NFC_SYS_DATA_SIZE;
    reg = oob[0] | oob[1] << 8 | oob[2] << 16 |
    oob[3] << 24;

    > +
    > + if (nfc->cfg->type == NFC_V6 ||
    > + nfc->cfg->type == NFC_V8)
    > + nfc->oob_buf[i * oob_step / 4] = reg;
    > + else
    > + nfc->oob_buf[i] = reg;
    > + }
    > +
    > + dma_data = dma_map_single(nfc->dev, (void *)nfc->page_buf,
    > + mtd->writesize, DMA_TO_DEVICE);
    > + dma_oob = dma_map_single(nfc->dev, nfc->oob_buf,
    > + ecc->steps * oob_step,
    > + DMA_TO_DEVICE);
    > +
    > + reinit_completion(&nfc->done);
    > + writel(INT_DMA, nfc->regs + nfc->cfg->int_en_off);
    > +
    > + rk_nfc_xfer_start(nfc, NFC_WRITE, ecc->steps, dma_data,
    > + dma_oob);
    > + ret = wait_for_completion_timeout(&nfc->done,
    > + msecs_to_jiffies(100));
    > + if (!ret)
    > + dev_warn(nfc->dev, "write: wait dma done timeout.\n");
    > + /*
    > + * Whether the DMA transfer is completed or not. The driver
    > + * needs to check the NFC`s status register to see if the data
    > + * transfer was completed.
    > + */
    > + ret = rk_nfc_wait_for_xfer_done(nfc);
    > +
    > + dma_unmap_single(nfc->dev, dma_data, mtd->writesize,
    > + DMA_TO_DEVICE);
    > + dma_unmap_single(nfc->dev, dma_oob, ecc->steps * oob_step,
    > + DMA_TO_DEVICE);
    > +
    > + if (boot_rom_mode && rk_nand->boot_ecc != ecc->strength)
    > + rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
    > +
    > + if (ret) {
    > + ret = -EIO;
    > + dev_err(nfc->dev,
    > + "write: wait transfer done timeout.\n");
    > + }
    > + } else {
    > + rk_nfc_write_buf(chip, buf, mtd->writesize + + mtd->oobsize);

    Remove a '+'

    > + }
    > +
    > + if (ret)
    > + return ret;
    > +
    > + ret = nand_prog_page_end_op(chip);
    > +
    > + /* Deselect the currently selected target. */
    > + rk_nfc_select_chip(chip, -1);
    > +
    > + return ret;
    > +}
    > +
    > +static int rk_nfc_write_page_raw(struct nand_chip *chip, const u8 *buf,
    > + int oob_on, int page)
    > +{
    > + struct mtd_info *mtd = nand_to_mtd(chip);
    > + struct rk_nfc *nfc = nand_get_controller_data(chip);
    > + u32 i;
    > +
    > + memset(nfc->buffer, 0xff, mtd->writesize + mtd->oobsize);
    > + swap(chip->oob_poi[0], chip->oob_poi[7]);
    > + for (i = 0; i < chip->ecc.steps; i++) {
    > + if (buf)
    > + memcpy(rk_data_ptr(chip, i), data_ptr(chip, buf, i),
    > + chip->ecc.size);
    > +
    > + memcpy(rk_oob_ptr(chip, i), oob_ptr(chip, i),
    > + NFC_SYS_DATA_SIZE);
    > + }
    > +
    > + return rk_nfc_write_page(mtd, chip, nfc->buffer, page, 1);
    > +}
    > +
    > +static int rk_nfc_write_oob_std(struct nand_chip *chip, int page)
    > +{
    > + return rk_nfc_write_page_raw(chip, NULL, 1, page);
    > +}
    > +
    > +static int rk_nfc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
    > + u32 data_offs, u32 readlen,
    > + u8 *buf, int page, int raw)
    > +{
    > + struct rk_nfc *nfc = nand_get_controller_data(chip);
    > + struct rk_nfc_nand_chip *rk_nand = to_rk_nand(chip);
    > + struct nand_ecc_ctrl *ecc = &chip->ecc;
    > + int oob_step = (ecc->bytes > 60) ? NFC_MAX_OOB_PER_STEP :
    > + NFC_MIN_OOB_PER_STEP;
    > + int pages_per_blk = mtd->erasesize / mtd->writesize;
    > + dma_addr_t dma_data, dma_oob;
    > + int ret = 0, i, boot_rom_mode = 0;
    > + int bitflips = 0, bch_st;
    > + u8 *oob;
    > + u32 tmp;
    > +
    > + nand_read_page_op(chip, page, 0, NULL, 0);
    > + if (!raw) {
    > + dma_data = dma_map_single(nfc->dev, nfc->page_buf,
    > + mtd->writesize,
    > + DMA_FROM_DEVICE);
    > + dma_oob = dma_map_single(nfc->dev, nfc->oob_buf,
    > + ecc->steps * oob_step,
    > + DMA_FROM_DEVICE);
    > +
    > + /*
    > + * The first 8(some devices are 4 or 16) blocks in use by
    > + * the bootrom.
    > + * Config the ECC algorithm supported by the boot ROM.
    > + */
    > + if (page < pages_per_blk * rk_nand->boot_blks &&
    > + chip->options & NAND_IS_BOOT_MEDIUM) {
    > + boot_rom_mode = 1;
    > + if (rk_nand->boot_ecc != ecc->strength)
    > + rk_nfc_hw_ecc_setup(chip, ecc,
    > + rk_nand->boot_ecc);
    > + }
    > +
    > + reinit_completion(&nfc->done);
    > + writel(INT_DMA, nfc->regs + nfc->cfg->int_en_off);
    > + rk_nfc_xfer_start(nfc, NFC_READ, ecc->steps, dma_data,
    > + dma_oob);
    > + ret = wait_for_completion_timeout(&nfc->done,
    > + msecs_to_jiffies(100));
    > + if (!ret)
    > + dev_warn(nfc->dev, "read: wait dma done timeout.\n");
    > + /*
    > + * Whether the DMA transfer is completed or not. The driver
    > + * needs to check the NFC`s status register to see if the data
    > + * transfer was completed.
    > + */
    > + ret = rk_nfc_wait_for_xfer_done(nfc);
    > + dma_unmap_single(nfc->dev, dma_data, mtd->writesize,
    > + DMA_FROM_DEVICE);
    > + dma_unmap_single(nfc->dev, dma_oob, ecc->steps * oob_step,
    > + DMA_FROM_DEVICE);
    > +
    > + if (ret) {
    > + bitflips = -EIO;
    > + dev_err(nfc->dev,
    > + "read: wait transfer done timeout.\n");
    > + goto out;
    > + }
    > +
    > + for (i = 0; i < ecc->steps; i++) {
    > + oob = chip->oob_poi + i * NFC_SYS_DATA_SIZE;
    > + if (nfc->cfg->type == NFC_V6 ||
    > + nfc->cfg->type == NFC_V8)
    > + tmp = nfc->oob_buf[i * oob_step / 4];
    > + else
    > + tmp = nfc->oob_buf[i];
    > + *oob++ = (u8)tmp;
    > + *oob++ = (u8)(tmp >> 8);
    > + *oob++ = (u8)(tmp >> 16);
    > + *oob++ = (u8)(tmp >> 24);
    > + }
    > +
    > + /*
    > + * Swap the first oob with the seventh oob and bad block
    > + * mask is saved at the seventh oob.
    > + */
    > + swap(chip->oob_poi[0], chip->oob_poi[7]);
    > +
    > + for (i = 0; i < ecc->steps / 2; i++) {
    > + bch_st = readl_relaxed(nfc->regs +
    > + nfc->cfg->bch_st_off + i * 4);
    > + if (bch_st & BIT(nfc->cfg->ecc0.err_flag_bit) ||
    > + bch_st & BIT(nfc->cfg->ecc1.err_flag_bit)) {
    > + mtd->ecc_stats.failed++;
    > + bitflips = -1;
    > + } else {
    > + ret = ECC_ERR_CNT(bch_st, nfc->cfg->ecc0);
    > + mtd->ecc_stats.corrected += ret;
    > + bitflips = max_t(u32, bitflips, ret);
    > +
    > + ret = ECC_ERR_CNT(bch_st, nfc->cfg->ecc1);
    > + mtd->ecc_stats.corrected += ret;
    > + bitflips = max_t(u32, bitflips, ret);
    > + }
    > + }
    > +out:
    > + memcpy(buf, nfc->page_buf, mtd->writesize);
    > +
    > + if (boot_rom_mode && rk_nand->boot_ecc != ecc->strength)
    > + rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
    > +
    > + if (bitflips < 0)
    > + dev_err(nfc->dev, "read page: %x ecc error!\n", page);
    > + } else {
    > + rk_nfc_read_buf(chip, buf, mtd->writesize + mtd->oobsize);
    > + }
    > + /* Deselect the currently selected target. */
    > + rk_nfc_select_chip(chip, -1);
    > +
    > + return bitflips;
    > +}
    > +
    > +static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
    > + int oob_on, int page)
    > +{
    > + return rk_nfc_write_page(nand_to_mtd(chip), chip, buf, page, 0);
    > +}
    > +
    > +static int rk_nfc_read_page_hwecc(struct nand_chip *chip, u8 *p, int oob_on,
    > + int pg)
    > +{
    > + struct mtd_info *mtd = nand_to_mtd(chip);
    > +
    > + return rk_nfc_read_page(mtd, chip, 0, mtd->writesize, p, pg, 0);
    > +}
    > +
    > +static int rk_nfc_read_page_raw(struct nand_chip *chip, u8 *buf, int oob_on,
    > + int page)
    > +{
    > + struct mtd_info *mtd = nand_to_mtd(chip);
    > + struct rk_nfc *nfc = nand_get_controller_data(chip);
    > + int i, ret;
    > +
    > + ret = rk_nfc_read_page(mtd, chip, 0, mtd->writesize, nfc->buffer,
    > + page, 1);
    > + if (ret < 0)
    > + return ret;
    > +
    > + for (i = 0; i < chip->ecc.steps; i++) {
    > + memcpy(oob_ptr(chip, i), rk_oob_ptr(chip, i),
    > + NFC_SYS_DATA_SIZE);
    > +
    > + if (buf)
    > + memcpy(data_ptr(chip, buf, i), rk_data_ptr(chip, i),
    > + chip->ecc.size);
    > + }
    > + swap(chip->oob_poi[0], chip->oob_poi[7]);
    > +
    > + return ret;
    > +}

    [..]

    \
     
     \ /
      Last update: 2020-06-13 15:33    [W:4.720 / U:0.220 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site