Hi Yifeng,
Don't poke with "ecc->bytes" ones it is set in rk_nfc_ecc_init(). It
will not be noted by the MTD frame work or userspace. I think there's
currently no way to let the user know that a different ECC must be used.
Neither can the user set ECC on the fly.
Example R/W flow:
nand_select_target()
chip->ecc.write_page_raw()
chip->ecc.write_page()
[..]
chip->ecc.read_page_raw()
chip->ecc.read_page()
nand_deselect_target()
A write/read with:
rk_nfc_read_page_hwecc()
rk_nfc_write_page_hwecc()
or
rk_nfc_read_page_raw()
rk_nfc_write_page_raw()
must end up with the same result. If we can't archive that, then we
shouldn't offer RAW mode to the user for now. If Miquel agrees you
should just get the driver ready now without these 2 functions and round
things up.
On 11/2/20 11:21 AM, Yifeng wrote:
> Hi Johan，
>
>
>>> +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 ecc_bytes_backup = ecc->bytes;
>
>>> + 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?
> |The ecc->bytes will different while |||rknand->|boot_ecc is different
> from ecc->strength.|
>
> |How about？
> 1. backup ecc->bytes
> 2. config ecc->bytes with boot_ecc_bytes
> 3. restore ecc->bytes|
>
> | pages_per_blk = mtd->erasesize / mtd->writesize;|
>
> | if ((chip->options & NAND_IS_BOOT_MEDIUM) &&||
> || (page < (pages_per_blk * rknand->boot_blks))) {||
> || boot_rom_blk = 1;||
> || if (rknand->boot_ecc != ecc->strength)||
> || ecc->bytes = rknand->boot_ecc_bytes;||
> || }|
>
>>> + 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);
>>> + }
>
> if (boot_rom_blk && (rknand->boot_ecc != ecc->strength))
> ecc->bytes = ecc_bytes_backup;
>
>>> + 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?
>>
>>> +
>>> + 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 ecc_bytes_backup = ecc->bytes;
>
>>> + 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);
>
> | pages_per_blk = mtd->erasesize / mtd->writesize;|
>
> | if ((chip->options & NAND_IS_BOOT_MEDIUM) &&||
> || (page < (pages_per_blk * rknand->boot_blks))) {||
> || boot_rom_blk = 1;||
> || if (rknand->boot_ecc != ecc->strength)||
> || ecc->bytes = rknand->boot_ecc_bytes;||
> || }|
>
>>> + /*
>>> + * Deselect the currently selected target after the ops is done
>>> + * to reduce the power consumption.
>>> + */
>>> + rk_nfc_select_chip(chip, -1);
>>> +
>>> + 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);
>>> + }
> | if (boot_rom_blk && (rknand->boot_ecc != ecc->strength))||
> || ecc->bytes = ecc_bytes_backup;|
>>> + return 0;
>>> +}
>>> +
> ...
>>> +static int rk_nfc_attach_chip(struct nand_chip *chip)
>>> +{
>>> + struct mtd_info *mtd = nand_to_mtd(chip);
>>> + struct device *dev = mtd->dev.parent;
>>> + 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 new_len, new_oob_len;
>>> + void *buf;
>>> + int ret;
>>> +
>>> + if (chip->options & NAND_BUSWIDTH_16) {
>>> + dev_err(dev, "16 bits bus width not supported");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (ecc->engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST)
>>> + return 0;
>>> +
>>> + ret = rk_nfc_ecc_init(dev, mtd);
>>> + if (ret)
>>> + return ret;
>>> + rknand->spare_per_sector = ecc->bytes + NFC_SYS_DATA_SIZE;
>>> + rknand->metadata_size = NFC_SYS_DATA_SIZE * ecc->steps;
>>> +
> | if (chip->options & NAND_IS_BOOT_MEDIUM)||
> || rknand->boot_ecc_bytes = DIV_ROUND_UP(rknand->boot_ecc * fls(8
> * ecc->size), 8);|
>>> + if (rknand->metadata_size < NFC_SYS_DATA_SIZE + 2) {
>>> + dev_err(dev,
>>> + "Driver needs at least %d bytes of meta data\n",
>>> + NFC_SYS_DATA_SIZE + 2);
>>> + return -EIO;
>>> + }
>>> +
>>> + /* Check buffer first, avoid duplicate alloc buffer. */
>>> + new_len = mtd->writesize + mtd->oobsize;
>>> + if (nfc->buffer && new_len > nfc->buffer_size) {
>>> + buf = krealloc(nfc->buffer, new_len, GFP_KERNEL | GFP_DMA);
>>> + if (!buf)
>>> + return -ENOMEM;
>>> + nfc->buffer = buf;
>>> + nfc->buffer_size = new_len;
>>> + }
>>> +
>>> + new_oob_len = ecc->steps * NFC_MAX_OOB_PER_STEP;
>>> + if (nfc->oob_buf && new_oob_len > nfc->oob_buf_size) {
>>> + buf = krealloc(nfc->oob_buf, new_oob_len,
>>> + GFP_KERNEL | GFP_DMA);
>>> + if (!buf) {
>>> + kfree(nfc->buffer);
>>> + nfc->buffer = NULL;
>>> + return -ENOMEM;
>>> + }
>>> + nfc->oob_buf = buf;
>>> + nfc->oob_buf_size = new_oob_len;
>>> + }
>>> +
>>> + if (!nfc->buffer) {
>>> + nfc->buffer = kzalloc(new_len, GFP_KERNEL | GFP_DMA);
>>> + if (!nfc->buffer)
>>> + return -ENOMEM;
>>> + nfc->buffer_size = new_len;
>>> + }
>>> +
>>> + if (!nfc->oob_buf) {
>>> + nfc->oob_buf = kzalloc(new_oob_len, GFP_KERNEL | GFP_DMA);
>>> + if (!nfc->oob_buf) {
>>> + kfree(nfc->buffer);
>>> + nfc->buffer = NULL;
>>> + return -ENOMEM;
>>> + }
>>> + nfc->oob_buf_size = new_oob_len;
>>> + }
>>> +
>>> + nfc->page_buf = nfc->buffer;
>>> +
>>> + chip->ecc.write_page_raw = rk_nfc_write_page_raw;
>>> + chip->ecc.write_page = rk_nfc_write_page_hwecc;
>>> + chip->ecc.write_oob_raw = rk_nfc_write_oob;
>>> + chip->ecc.write_oob = rk_nfc_write_oob;
>>> +
>>> + chip->ecc.read_page_raw = rk_nfc_read_page_raw;
>>> + chip->ecc.read_page = rk_nfc_read_page_hwecc;
>>> + chip->ecc.read_oob_raw = rk_nfc_read_oob;
>>> + chip->ecc.read_oob = rk_nfc_read_oob;
>>> +
>>> + return 0;
>>> +}
>
> Thanks，
>
> Yifeng
>
>
Hi Johan, Yifeng
Johan Jonker <[email protected]> wrote on Mon, 2 Nov 2020 13:57:56
+0100:
> Hi Yifeng,
>
> Don't poke with "ecc->bytes" ones it is set in rk_nfc_ecc_init(). It
> will not be noted by the MTD frame work or userspace. I think there's
> currently no way to let the user know that a different ECC must be used.
> Neither can the user set ECC on the fly.
>
> Example R/W flow:
>
> nand_select_target()
> chip->ecc.write_page_raw()
> chip->ecc.write_page()
>
> [..]
>
> chip->ecc.read_page_raw()
> chip->ecc.read_page()
> nand_deselect_target()
>
> A write/read with:
>
> rk_nfc_read_page_hwecc()
> rk_nfc_write_page_hwecc()
>
> or
>
> rk_nfc_read_page_raw()
> rk_nfc_write_page_raw()
>
> must end up with the same result. If we can't archive that, then we
> shouldn't offer RAW mode to the user for now. If Miquel agrees you
> should just get the driver ready now without these 2 functions and round
> things up.
What about just not supporting the BootROM area if it was marked
"reserved" by the BRom in the DT?
Raw accessors is really a nice and basic feature that I would like to
have in every new driver.
Thanks,
Miquèl
Hi,
On 11/2/20 2:07 PM, Miquel Raynal wrote:
> Hi Johan, Yifeng
>
> Johan Jonker <[email protected]> wrote on Mon, 2 Nov 2020 13:57:56
> +0100:
>
>> Hi Yifeng,
>>
>> Don't poke with "ecc->bytes" ones it is set in rk_nfc_ecc_init(). It
>> will not be noted by the MTD frame work or userspace. I think there's
>> currently no way to let the user know that a different ECC must be used.
>> Neither can the user set ECC on the fly.
>>
>> Example R/W flow:
>>
>> nand_select_target()
>> chip->ecc.write_page_raw()
>> chip->ecc.write_page()
>>
>> [..]
>>
>> chip->ecc.read_page_raw()
>> chip->ecc.read_page()
>> nand_deselect_target()
>>
>> A write/read with:
>>
>> rk_nfc_read_page_hwecc()
>> rk_nfc_write_page_hwecc()
>>
>> or
>>
>> rk_nfc_read_page_raw()
>> rk_nfc_write_page_raw()
>>
>> must end up with the same result. If we can't archive that, then we
>> shouldn't offer RAW mode to the user for now. If Miquel agrees you
>> should just get the driver ready now without these 2 functions and round
>> things up.
>
> What about just not supporting the BootROM area if it was marked
> "reserved" by the BRom in the DT?
Should we just fill the buffers with '0xff' for boot blocks?
>
> Raw accessors is really a nice and basic feature that I would like to
> have in every new driver.
>
> Thanks,
> Miquèl
>
On 11/2/20 2:11 PM, Johan Jonker wrote:
> Hi,
>
> On 11/2/20 2:07 PM, Miquel Raynal wrote:
>> Hi Johan, Yifeng
>>
>> Johan Jonker <[email protected]> wrote on Mon, 2 Nov 2020 13:57:56
>> +0100:
>>
>>> Hi Yifeng,
>>>
>>> Don't poke with "ecc->bytes" ones it is set in rk_nfc_ecc_init(). It
>>> will not be noted by the MTD frame work or userspace. I think there's
>>> currently no way to let the user know that a different ECC must be used.
>>> Neither can the user set ECC on the fly.
>>>
>>> Example R/W flow:
>>>
>>> nand_select_target()
>>> chip->ecc.write_page_raw()
>>> chip->ecc.write_page()
>>>
>>> [..]
>>>
>>> chip->ecc.read_page_raw()
>>> chip->ecc.read_page()
>>> nand_deselect_target()
>>>
>>> A write/read with:
>>>
>>> rk_nfc_read_page_hwecc()
>>> rk_nfc_write_page_hwecc()
>>>
>>> or
>>>
>>> rk_nfc_read_page_raw()
>>> rk_nfc_write_page_raw()
>>>
>>> must end up with the same result. If we can't archive that, then we
>>> shouldn't offer RAW mode to the user for now. If Miquel agrees you
>>> should just get the driver ready now without these 2 functions and round
>>> things up.
>>
>> What about just not supporting the BootROM area if it was marked
>> "reserved" by the BRom in the DT?
>
> Should we just fill the buffers with '0xff' for boot blocks?
(part 2) ;)
My fault....
Better use:
if ((chip->options & NAND_IS_BOOT_MEDIUM) &&
(page < (pages_per_blk * rknand->boot_blks))) {
return -EIO;
}
>
>>
>> Raw accessors is really a nice and basic feature that I would like to
>> have in every new driver.
>>
>> Thanks,
>> Miquèl
>>
>
Hi Johan,
Johan Jonker <[email protected]> wrote on Mon, 2 Nov 2020 17:31:18
+0100:
> On 11/2/20 2:11 PM, Johan Jonker wrote:
> > Hi,
> >
> > On 11/2/20 2:07 PM, Miquel Raynal wrote:
> >> Hi Johan, Yifeng
> >>
> >> Johan Jonker <[email protected]> wrote on Mon, 2 Nov 2020 13:57:56
> >> +0100:
> >>
> >>> Hi Yifeng,
> >>>
> >>> Don't poke with "ecc->bytes" ones it is set in rk_nfc_ecc_init(). It
> >>> will not be noted by the MTD frame work or userspace. I think there's
> >>> currently no way to let the user know that a different ECC must be used.
> >>> Neither can the user set ECC on the fly.
> >>>
> >>> Example R/W flow:
> >>>
> >>> nand_select_target()
> >>> chip->ecc.write_page_raw()
> >>> chip->ecc.write_page()
> >>>
> >>> [..]
> >>>
> >>> chip->ecc.read_page_raw()
> >>> chip->ecc.read_page()
> >>> nand_deselect_target()
> >>>
> >>> A write/read with:
> >>>
> >>> rk_nfc_read_page_hwecc()
> >>> rk_nfc_write_page_hwecc()
> >>>
> >>> or
> >>>
> >>> rk_nfc_read_page_raw()
> >>> rk_nfc_write_page_raw()
> >>>
> >>> must end up with the same result. If we can't archive that, then we
> >>> shouldn't offer RAW mode to the user for now. If Miquel agrees you
> >>> should just get the driver ready now without these 2 functions and round
> >>> things up.
> >>
> >> What about just not supporting the BootROM area if it was marked
> >> "reserved" by the BRom in the DT?
> >
> > Should we just fill the buffers with '0xff' for boot blocks?
>
> (part 2) ;)
> My fault....
> Better use:
>
> if ((chip->options & NAND_IS_BOOT_MEDIUM) &&
> (page < (pages_per_blk * rknand->boot_blks))) {
>
> return -EIO;
>
> }
Yup, I was about to tell you that I would prefer returning a nice
error, this is fine I guess.
Anyway, I think reading bad block markers is done in raw mode, so if
raw accessors refuse to return valid values for boot blocks, you won't
be able to access it neither with raw nor corrected hooks.
Perhaps refusing the access to the regular page access is ok, but maybe
we should be able to at least read these pages in raw mode
(and move the BBM to its right location). What do you think?
Thanks,
Miquèl
>
>
> >
> >>
> >> Raw accessors is really a nice and basic feature that I would like to
> >> have in every new driver.
> >>
> >> Thanks,
> >> Miquèl
> >>
> >
>
On 11/2/20 6:00 PM, Miquel Raynal wrote:
> Hi Johan,
>
> Johan Jonker <[email protected]> wrote on Mon, 2 Nov 2020 17:31:18
> +0100:
>
>> On 11/2/20 2:11 PM, Johan Jonker wrote:
>>> Hi,
>>>
>>> On 11/2/20 2:07 PM, Miquel Raynal wrote:
>>>> Hi Johan, Yifeng
>>>>
>>>> Johan Jonker <[email protected]> wrote on Mon, 2 Nov 2020 13:57:56
>>>> +0100:
>>>>
>>>>> Hi Yifeng,
>>>>>
>>>>> Don't poke with "ecc->bytes" ones it is set in rk_nfc_ecc_init(). It
>>>>> will not be noted by the MTD frame work or userspace. I think there's
>>>>> currently no way to let the user know that a different ECC must be used.
>>>>> Neither can the user set ECC on the fly.
>>>>>
>>>>> Example R/W flow:
>>>>>
>>>>> nand_select_target()
>>>>> chip->ecc.write_page_raw()
>>>>> chip->ecc.write_page()
>>>>>
>>>>> [..]
>>>>>
>>>>> chip->ecc.read_page_raw()
>>>>> chip->ecc.read_page()
>>>>> nand_deselect_target()
>>>>>
>>>>> A write/read with:
>>>>>
>>>>> rk_nfc_read_page_hwecc()
>>>>> rk_nfc_write_page_hwecc()
>>>>>
>>>>> or
>>>>>
>>>>> rk_nfc_read_page_raw()
>>>>> rk_nfc_write_page_raw()
>>>>>
>>>>> must end up with the same result. If we can't archive that, then we
>>>>> shouldn't offer RAW mode to the user for now. If Miquel agrees you
>>>>> should just get the driver ready now without these 2 functions and round
>>>>> things up.
>>>>
>>>> What about just not supporting the BootROM area if it was marked
>>>> "reserved" by the BRom in the DT?
>>>
>>> Should we just fill the buffers with '0xff' for boot blocks?
>>
>> (part 2) ;)
>> My fault....
>> Better use:
>>
>> if ((chip->options & NAND_IS_BOOT_MEDIUM) &&
>> (page < (pages_per_blk * rknand->boot_blks))) {
>>
>> return -EIO;
>>
>> }
>
> Yup, I was about to tell you that I would prefer returning a nice
> error, this is fine I guess.
>
> Anyway, I think reading bad block markers is done in raw mode, so if
> raw accessors refuse to return valid values for boot blocks, you won't
> be able to access it neither with raw nor corrected hooks.
>
> Perhaps refusing the access to the regular page access is ok, but maybe
> we should be able to at least read these pages in raw mode
> (and move the BBM to its right location). What do you think?
I think that the problem with asymmetric read and write access is that a
user reads data successful, but that it can't write it back after it's
block is erased. You shouldn't give the illusion that the boot ROM
blocks can be accessed in RAW mode. Something with perception...does
user space known that Rockchip's NFC is special...
>
> Thanks,
> Miquèl
>
>>
>>
>>>
>>>>
>>>> Raw accessors is really a nice and basic feature that I would like to
>>>> have in every new driver.
>>>>
>>>> Thanks,
>>>> Miquèl
>>>>
>>>
>>
On 2020/11/3 1:11, Johan Jonker wrote:
>On 11/2/20 6:00 PM, Miquel Raynal wrote:
>> Hi Johan,
>>
>> Johan Jonker <[email protected]> wrote on Mon, 2 Nov 2020 17:31:18
>> +0100:
>>
>>> On 11/2/20 2:11 PM, Johan Jonker wrote:
>>>> Hi,
>>>>
>>>> On 11/2/20 2:07 PM, Miquel Raynal wrote:
>>>>> Hi Johan, Yifeng
>>>>>
>>>>> Johan Jonker <[email protected]> wrote on Mon, 2 Nov 2020 13:57:56
>>>>> +0100:
>>>>>
>>>>>> Hi Yifeng,
>>>>>>
>>>>>> Don't poke with "ecc->bytes" ones it is set in rk_nfc_ecc_init(). It
>>>>>> will not be noted by the MTD frame work or userspace. I think there's
>>>>>> currently no way to let the user know that a different ECC must be used.
>>>>>> Neither can the user set ECC on the fly.
>>>>>>
>>>>>> Example R/W flow:
>>>>>>
>>>>>> nand_select_target()
>>>>>> chip->ecc.write_page_raw()
>>>>>> chip->ecc.write_page()
>>>>>>
>>>>>> [..]
>>>>>>
>>>>>> chip->ecc.read_page_raw()
>>>>>> chip->ecc.read_page()
>>>>>> nand_deselect_target()
>>>>>>
>>>>>> A write/read with:
>>>>>>
>>>>>> rk_nfc_read_page_hwecc()
>>>>>> rk_nfc_write_page_hwecc()
>>>>>>
>>>>>> or
>>>>>>
>>>>>> rk_nfc_read_page_raw()
>>>>>> rk_nfc_write_page_raw()
>>>>>>
>>>>>> must end up with the same result. If we can't archive that, then we
>>>>>> shouldn't offer RAW mode to the user for now. If Miquel agrees you
>>>>>> should just get the driver ready now without these 2 functions and round
>>>>>> things up.
>>>>>
>>>>> What about just not supporting the BootROM area if it was marked
>>>>> "reserved" by the BRom in the DT?
>>>>
>>>> Should we just fill the buffers with '0xff' for boot blocks?
>>>
>>> (part 2) ;)
>>> My fault....
>>> Better use:
>>>
>>> if ((chip->options & NAND_IS_BOOT_MEDIUM) &&
>>> (page < (pages_per_blk * rknand->boot_blks))) {
>>>
>>> return -EIO;
>>>
>>> }
>>
>> Yup, I was about to tell you that I would prefer returning a nice
>> error, this is fine I guess.
>>
>> Anyway, I think reading bad block markers is done in raw mode, so if
>> raw accessors refuse to return valid values for boot blocks, you won't
>> be able to access it neither with raw nor corrected hooks.
>>
>> Perhaps refusing the access to the regular page access is ok, but maybe
>> we should be able to at least read these pages in raw mode
>> (and move the BBM to its right location). What do you think?
>
>I think that the problem with asymmetric read and write access is that a
>user reads data successful, but that it can't write it back after it's
>block is erased. You shouldn't give the illusion that the boot ROM
>blocks can be accessed in RAW mode. Something with perception...does
>user space known that Rockchip's NFC is special...
>
>
Is it possible to return a error when the ECC is different between the boot ROM blocks and other blocks?
If the ECC is the same, and the OOB layout is the same and does not need special treatment.
static int rk_nfc_read_page_raw(struct nand_chip *chip, u8 *buf, int oob_on,int page)
{
...
if ((chip->options & NAND_IS_BOOT_MEDIUM) &&
(page < (pages_per_blk * rknand->boot_blks)) &&
(rknand->boot_ecc != ecc->strength)) {
return -EIO;
}
...
}
About the BBM, the u-boot will create BBT at the first power up.
chip->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
If uboot does not create BBT, then the framework will scan the OOB of all blocks and create it.
I modified the read OOB function (enable ECC) ，and it can read the correct bad block mask.
static int rk_nfc_write_oob(struct nand_chip *chip, int page)
{
return rk_nfc_write_page_hwecc(chip, NULL, 1, page);
}
static int rk_nfc_read_oob(struct nand_chip *chip, int page)
{
return rk_nfc_read_page_hwecc(chip, NULL, 1, page);
}
chip->ecc.read_oob = rk_nfc_read_oob;
chip->ecc.write_oob = rk_nfc_write_oob;
>>
>> Thanks,
>> Miquèl
>>
>>>
>>>
>>>>
>>>>>
>>>>> Raw accessors is really a nice and basic feature that I would like to
>>>>> have in every new driver.
>>>>>
>>>>> Thanks,
>>>>> Miquèl
>>>>>
>>>>
>>>
>
>
>
>
Hi Johan,
On 2020/10/31 19:58, Johan Jonker wrote:
>Hi Yifeng,
...
>> +
>> +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);
>
>}
It cannot return - EIO while ECC fail, refer to the function nand_do_read_ops,
maybe need do read_retry while ECC fail.
I think return 0 is better while ecc fail，as suggested by Miquel.
In other cases, return the actual ECC error bit, because the file system(such as ubifs) needs to
know whether the data is reliable or needs to be refreshed (-EUCLEAN).
int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops)
{
...
return ret_code >= mtd->bitflip_threshold ? -EUCLEAN : 0;
}
>> +
>> + /*
>> + * Deselect the currently selected target after the ops is done
>> + * to reduce the power consumption.
>> + */
>> + rk_nfc_select_chip(chip, -1);
>> +
>
>> + 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.
>
>> +}