Re: Re: [PATCH v13 2/8] mtd: rawnand: rockchip: NFC drivers for RK3308, RK2928 and others

From: 赵仪峰
Date: Sun Nov 01 2020 - 22:48:23 EST

Next message: Andi Kleen: "Re: [PATCH v2 0/4] perf: Fix perf_event_attr::exclusive rotation"
Previous message: santosh.shilimkar@xxxxxxxxxx: "Re: [PATCH v2 00/11] firmware/soc: ti_sci, ringacc/inta: Preparation for AM64 DMA support"
Next in thread: Miquel Raynal: "Re: [PATCH v13 2/8] mtd: rawnand: rockchip: NFC drivers for RK3308, RK2928 and others"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Johan,

void nand_deselect_target(struct nand_chip *chip)
{
if (chip->legacy.select_chip)
chip->legacy.select_chip(chip, -1);

chip->cur_cs = -1;
}

I need add the code below and it work.

chip->legacy.select_chip = rk_nfc_select_chip;

But I found almost all nandc drivers do not add this code. Is there any other way to implement it？

--------------

>Hi Yifeng,
>
>In some functions you deselect the chips.
>The MTD frame work has a functions for that and also keeps track of its
>select status, so I think that you shouldn't poke with that yourself and
>should therefore remove the deselections from your driver.
>
>/**
> * nand_deselect_target() - Deselect the currently selected target
> * @chip: NAND chip object
> *
> * Deselect the currently selected NAND target. The result of operations
> * executed on @chip after the target has been deselected is undefined.
> */
>void nand_deselect_target(struct nand_chip *chip)
>{
> if (chip->legacy.select_chip)
> chip->legacy.select_chip(chip, -1);
>
> chip->cur_cs = -1;
>}
>EXPORT_SYMBOL_GPL(nand_deselect_target);
>
>
>On 10/31/20 12:58 PM, Johan Jonker wrote:
>
>[..]
>
>> On 10/28/20 10:53 AM, Yifeng Zhao wrote:
>
>[..]
>
>>> +
>>> +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);
>>> + struct nand_ecc_ctrl *ecc = &chip->ecc;
>>> + int ret = 0;
>>> + u32 i;
>>> +
>>
>> /*
>> * Normal timing and ECC layout size setup is already done in
>> * the rk_nfc_select_chip() function.
>> */
>>
>> How about the ECC layout size setup for a boot block?
>>
>>> + if (!buf)
>>> + memset(nfc->buffer, 0xff, mtd->writesize + mtd->oobsize);
>>> +> + for (i = 0; i < ecc->steps; i++) {
>>> + /* Copy data to nfc buffer. */
>>> + if (buf)
>>> + memcpy(rk_nfc_data_ptr(chip, i),
>>> +        rk_nfc_buf_to_data_ptr(chip, buf, i),
>>> +        ecc->size);
>>
>>> + /*
>>> + * The first four bytes of OOB are reserved for the
>>> + * boot ROM. In some debugging cases, such as with a
>>> + * read, erase and write back test these 4 bytes stored
>>> + * in OOB also need to be written back.
>>> + */
>>
>>
>> /*
>> * The first four bytes of OOB are reserved for the
>> * boot ROM. In some debugging cases, such as with a
>> * read, erase and write back test these 4 bytes stored
>> * in OOB also need to be written back.
>> *
>> * The function nand_block_bad detects bad blocks like:
>> *
>> * bad = chip->oob_poi[chip->badblockpos];
>> *
>> * chip->badblockpos == 0 for a large page NAND Flash,
>> * so chip->oob_poi[0] is the bad block mask (BBM).
>> *
>> * The OOB data layout on the NFC is:
>> *
>> *   PA0 PA1 PA2 PA3 | BBM OOB1 OOB2 OOB3 | ...
>> *
>> * or
>> *
>> * 0xFF 0xFF 0xFF 0xFF | BBM OOB1 OOB2 OOB3 | ...
>> *
>> * The code here just swaps the first 4 bytes with the last
>> * 4 bytes without losing any data.
>> *
>> * The chip->oob_poi data layout:
>> *
>> *   BBM OOB1 OOB2 OOB3 |......| PA0 PA1 PA2 PA3
>> *
>> * The rk_nfc_ooblayout_free() function already has reserved
>> * these 4 bytes with:
>> *
>> * oob_region->offset = NFC_SYS_DATA_SIZE + 2;
>> */
>>
>>
>>> + if (!i)
>>> + memcpy(rk_nfc_oob_ptr(chip, i),
>>> +        rk_nfc_buf_to_oob_ptr(chip, ecc->steps - 1),
>>> +        NFC_SYS_DATA_SIZE);
>>> + else
>>> + memcpy(rk_nfc_oob_ptr(chip, i),
>>> +        rk_nfc_buf_to_oob_ptr(chip, i - 1),
>>> +        NFC_SYS_DATA_SIZE);
>>> + /* Copy ECC data to the NFC buffer. */
>>> + memcpy(rk_nfc_oob_ptr(chip, i) + NFC_SYS_DATA_SIZE,
>>> +        rk_nfc_buf_to_oob_ecc_ptr(chip, i),
>>> +        ecc->bytes);
>>> + }
>>> +
>>> + nand_prog_page_begin_op(chip, page, 0, NULL, 0);
>>> + rk_nfc_write_buf(nfc, buf, mtd->writesize + mtd->oobsize);
>>> + ret = nand_prog_page_end_op(chip);
>>> +
>
>>> + /*
>>> + * Deselect the currently selected target after the ops is done
>>> + * to reduce the power consumption.
>>> + */
>>> + rk_nfc_select_chip(chip, -1);
>>
>> Does the MTD framework always select again?
>
>Remove.
>Do not assume that the MTD framework or user space knows that you have
>deselected the chip.
>
>>
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int rk_nfc_write_oob(struct nand_chip *chip, int page)
>>> +{
>>> + return rk_nfc_write_page_raw(chip, NULL, 1, page);
>>> +}
>>> +
>>> +static int rk_nfc_write_page_hwecc(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);
>>> + struct rk_nfc_nand_chip *rknand = rk_nfc_to_rknand(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);
>>> +
>>> + memcpy(nfc->page_buf, buf, mtd->writesize);
>>> +
>>> + /*
>>> + * The first blocks (4, 8 or 16 depending on the device) are used
>>> + * by the boot ROM and the first 32 bits of OOB need to link to
>>> + * the next page address in the same block. We can't directly copy
>>> + * OOB data from the MTD framework, because this page address
>>> + * conflicts for example with the bad block marker (BBM),
>>> + * so we shift all OOB data including the BBM with 4 byte positions.
>>> + * As a consequence the OOB size available to the MTD framework is
>>> + * also reduced with 4 bytes.
>>> + *
>>
>>> + *    PA0 PA1 PA2 PA3 | BBM OOB1 OOB2 OOB3 | ...
>>
>>
>> *    PA0 PA1 PA2 PA3 | BBM OOB1 OOB2 OOB3 | ...
>>
>> keep layouts aligned
>>
>>> + *
>>> + * If a NAND is not a boot medium or the page is not a boot block,
>>> + * the first 4 bytes are left untouched by writing 0xFF to them.
>>> + *
>>> + *   0xFF 0xFF 0xFF 0xFF | BBM OOB1 OOB2 OOB3 | ...
>>> + *
>>> + * Configure the ECC algorithm supported by the boot ROM.
>>> + */
>>> + if ((page < pages_per_blk * rknand->boot_blks) &&
>>
>> if ((page < (pages_per_blk * rknand->boot_blks)) &&
>>
>>> +     (chip->options & NAND_IS_BOOT_MEDIUM)) {
>>> + boot_rom_mode = 1;
>>> + if (rknand->boot_ecc != ecc->strength)
>>> + rk_nfc_hw_ecc_setup(chip, ecc,
>>> +     rknand->boot_ecc);
>>> + }
>>> +
>>> + for (i = 0; i < ecc->steps; i++) {
>>> + 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 (!i && boot_rom_mode)
>>> + reg = (page & (pages_per_blk - 1)) * 4;
>>> +
>>> + if (nfc->cfg->type == NFC_V9)
>>> + nfc->oob_buf[i] = reg;
>>> + else
>>> + nfc->oob_buf[i * (oob_step / 4)] = 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 && rknand->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");
>>> + }
>>> +
>>
>>> + if (ret)
>>> + return ret;
>>
>> remove, always deselect
>>
>> if (!ret) {
>>
>>> +
>>> + ret = nand_prog_page_end_op(chip);
>>
>> }
>>
>
>>> +
>>> + /*
>>> + * Deselect the currently selected target after the ops is done
>>> + * to reduce the power consumption.
>>> + */
>>> + rk_nfc_select_chip(chip, -1);
>>
>> Does the MTD framework always select again?
>
>Remove.
>Do not assume that the MTD framework or user space knows that you have
>deselected the chip.
>
>>
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +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);
>>> + struct nand_ecc_ctrl *ecc = &chip->ecc;
>>> + int i;
>>> +
>>
>> /*
>> * Normal timing and ECC layout size setup is already done in
>> * the rk_nfc_select_chip() function.
>> */
>>
>> How about the ECC layout size setup for a boot block?
>>
>>> + nand_read_page_op(chip, page, 0, NULL, 0);
>>> + rk_nfc_read_buf(nfc, nfc->buffer, mtd->writesize + mtd->oobsize);
>>> +
>
>>> + /*
>>> + * Deselect the currently selected target after the ops is done
>>> + * to reduce the power consumption.
>>> + */
>>> + rk_nfc_select_chip(chip, -1);
>
>Remove.
>Do not assume that the MTD framework or user space knows that you have
>deselected the chip.
>
>>> +
>>> + for (i = 0; i < ecc->steps; i++) {
>>> + /*
>>> + * The first four bytes of OOB are reserved for the
>>> + * boot ROM. In some debugging cases, such as with a read,
>>> + * erase and write back test, these 4 bytes also must be
>>> + * saved somewhere, otherwise this information will be
>>> + * lost during a write back.
>>> + */
>>> + if (!i)
>>> + memcpy(rk_nfc_buf_to_oob_ptr(chip, ecc->steps - 1),
>>> +        rk_nfc_oob_ptr(chip, i),
>>> +        NFC_SYS_DATA_SIZE);
>>> + else
>>> + memcpy(rk_nfc_buf_to_oob_ptr(chip, i - 1),
>>> +        rk_nfc_oob_ptr(chip, i),
>>> +        NFC_SYS_DATA_SIZE);
>>> + /* Copy ECC data from the NFC buffer. */
>>> + memcpy(rk_nfc_buf_to_oob_ecc_ptr(chip, i),
>>> +        rk_nfc_oob_ptr(chip, i) + NFC_SYS_DATA_SIZE,
>>> +        ecc->bytes);
>>> + /* Copy data from the NFC buffer. */
>>> + if (buf)
>>> + memcpy(rk_nfc_buf_to_data_ptr(chip, buf, i),
>>> +        rk_nfc_data_ptr(chip, i),
>>> +        ecc->size);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int rk_nfc_read_oob(struct nand_chip *chip, int page)
>>> +{
>>> + return rk_nfc_read_page_raw(chip, NULL, 1, page);
>>> +}
>>> +
>>> +static int rk_nfc_read_page_hwecc(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);
>>> + struct rk_nfc_nand_chip *rknand = rk_nfc_to_rknand(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 ret = 0, i, cnt, boot_rom_mode = 0;
>>
>>> + int bitflips = 0, bch_st;
>>> + u8 *oob;
>>> + u32 tmp;
>>> +
>>> + nand_read_page_op(chip, page, 0, NULL, 0);
>>> +
>>> + 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 blocks (4, 8 or 16 depending on the device)
>>> + * are used by the boot ROM.
>>> + * Configure the ECC algorithm supported by the boot ROM.
>>> + */
>>
>>> + if ((page < pages_per_blk * rknand->boot_blks) &&
>>
>>> + if ((page < (pages_per_blk * rknand->boot_blks)) &&
>>
>>> +     (chip->options & NAND_IS_BOOT_MEDIUM)) {
>>> + boot_rom_mode = 1;
>>> + if (rknand->boot_ecc != ecc->strength)
>>> + rk_nfc_hw_ecc_setup(chip, ecc,
>>> +     rknand->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);
>>
>> add empty line
>>
>>> + 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;
>>
>> ret = -EIO;
>>
>> return only "0" or official error codes
>>
>>> + dev_err(nfc->dev,
>>> + "read: wait transfer done timeout.\n");
>>> + goto out;
>>> + }
>>> +
>>> + for (i = 1; i < ecc->steps; i++) {
>>> + oob = chip->oob_poi + (i - 1) * NFC_SYS_DATA_SIZE;
>>> + if (nfc->cfg->type == NFC_V9)
>>> + tmp = nfc->oob_buf[i];
>>> + else
>>> + tmp = nfc->oob_buf[i * (oob_step / 4)];
>>> + *oob++ = (u8)tmp;
>>> + *oob++ = (u8)(tmp >> 8);
>>> + *oob++ = (u8)(tmp >> 16);
>>> + *oob++ = (u8)(tmp >> 24);
>>> + }
>>> +
>>> + 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 = 0;
>>
>> max_bitflips = -1;
>>
>> use max_bitflips only for the error warning, not as return value
>>
>>> + } else {
>>
>>> + ret = ECC_ERR_CNT(bch_st, nfc->cfg->ecc0);
>>
>> use ret only with "0" or official error codes, use cnt instead
>>
>> cnt = ECC_ERR_CNT(bch_st, nfc->cfg->ecc0);
>>
>>> + mtd->ecc_stats.corrected += ret;
>> mtd->ecc_stats.corrected += cnt;
>>
>>> + bitflips = max_t(u32, bitflips, ret);
>>
>> bitflips = max_t(u32, bitflips, cnt);
>>
>>> +
>>> + ret = ECC_ERR_CNT(bch_st, nfc->cfg->ecc1);
>>
>> cnt = ECC_ERR_CNT(bch_st, nfc->cfg->ecc1);
>>
>>> + mtd->ecc_stats.corrected += ret;
>>
>> mtd->ecc_stats.corrected += cnt;
>>
>>> + bitflips = max_t(u32, bitflips, ret);
>>
>> bitflips = max_t(u32, bitflips, cnt);
>>> + }
>>> + }
>>> +out:
>>> + memcpy(buf, nfc->page_buf, mtd->writesize);
>>> +
>>
>>> + if (boot_rom_mode && rknand->boot_ecc != ecc->strength)
>>> + rk_nfc_hw_ecc_setup(chip, ecc, ecc->strength);
>>> +
>>
>>> + if (bitflips > ecc->strength)
>>
>> if (bitflips == -1) {
>> ret = -EIO;
>>
>>> + dev_err(nfc->dev, "read page: %x ecc error!\n", page);
>>
>> }
>>
>
>>> +
>>> + /*
>>> + * Deselect the currently selected target after the ops is done
>>> + * to reduce the power consumption.
>>> + */
>>> + rk_nfc_select_chip(chip, -1);
>
>
>Remove.
>Do not assume that the MTD framework or user space knows that you have
>deselected the chip.
>
>>> +
>>
>>> + return bitflips;
>>
>> return ret;
>>
>> Return only "0" or official error codes
>> If you want to do a "bad block scan" function in user space analyse/use
>> "mtd->ecc_stats" instead.
>>
>>> +}
>>> +
>
>
>

Next message: Andi Kleen: "Re: [PATCH v2 0/4] perf: Fix perf_event_attr::exclusive rotation"
Previous message: santosh.shilimkar@xxxxxxxxxx: "Re: [PATCH v2 00/11] firmware/soc: ti_sci, ringacc/inta: Preparation for AM64 DMA support"
Next in thread: Miquel Raynal: "Re: [PATCH v13 2/8] mtd: rawnand: rockchip: NFC drivers for RK3308, RK2928 and others"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]