This serie contains various fixes for the Rockchip NAND controller
driver that showed up while testing boot block writing.
Fixed are:
Always copy hwecc PA data to/from oob_poi buffer in order to be able
to read/write the various boot block layouts.
Add option to safely probe the driver on a NAND with unknown data layout.
Fix default timing.
Fix oobfree layout.
Add missing chip ID.
Changed V2:
Add tag
Add manufacturer ops
Reword
Johan Jonker (4):
mtd: nand: raw: rockchip-nand-controller: copy hwecc PA data to
oob_poi buffer
mtd: nand: raw: rockchip-nand-controller: add skipbbt option
mtd: nand: raw: rockchip-nand-controller: fix oobfree offset and
description
mtd: nand: raw: add basic sandisk manufacturer ops
Paweł Jarosz (1):
mtd: nand: add support for the Sandisk SDTNQGAMA chip
drivers/mtd/nand/raw/Makefile | 1 +
drivers/mtd/nand/raw/internals.h | 1 +
drivers/mtd/nand/raw/nand_ids.c | 5 +-
drivers/mtd/nand/raw/nand_sandisk.c | 26 ++++++++++
.../mtd/nand/raw/rockchip-nand-controller.c | 52 ++++++++++++-------
5 files changed, 64 insertions(+), 21 deletions(-)
create mode 100644 drivers/mtd/nand/raw/nand_sandisk.c
--
2.30.2
Sandisk SDTNQGAMA is a 8GB size, 3.3V 8 bit chip with 16KB page size,
1KB write size and 40 bit ecc support
Co-developed-by: Paweł Jarosz <[email protected]>
Signed-off-by: Paweł Jarosz <[email protected]>
Signed-off-by: Johan Jonker <[email protected]>
---
drivers/mtd/nand/raw/nand_ids.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mtd/nand/raw/nand_ids.c b/drivers/mtd/nand/raw/nand_ids.c
index 1a89ed796..650351c62 100644
--- a/drivers/mtd/nand/raw/nand_ids.c
+++ b/drivers/mtd/nand/raw/nand_ids.c
@@ -44,6 +44,9 @@ struct nand_flash_dev nand_flash_ids[] = {
{"TC58NVG6D2 64G 3.3V 8-bit",
{ .id = {0x98, 0xde, 0x94, 0x82, 0x76, 0x56, 0x04, 0x20} },
SZ_8K, SZ_8K, SZ_2M, 0, 8, 640, NAND_ECC_INFO(40, SZ_1K) },
+ {"SDTNQGAMA 64G 3.3V 8-bit",
+ { .id = {0x45, 0xde, 0x94, 0x93, 0x76, 0x57} },
+ SZ_16K, SZ_8K, SZ_4M, 0, 6, 1280, NAND_ECC_INFO(40, SZ_1K) },
{"SDTNRGAMA 64G 3.3V 8-bit",
{ .id = {0x45, 0xde, 0x94, 0x93, 0x76, 0x50} },
SZ_16K, SZ_8K, SZ_4M, 0, 6, 1280, NAND_ECC_INFO(40, SZ_1K) },
--
2.30.2
The MTD framework reserves 1 or 2 bytes for the bad block marker
depending on the bus size. The rockchip-nand-controller driver
currently only supports a 8 bit bus, but reserves standard 2 bytes
for the BBM. The first free OOB byte is therefore OOB2 at offset 2.
Page address(PA) bytes are moved to the last 4 positions before
ECC. Update the description for Linux.
Signed-off-by: Johan Jonker <[email protected]>
---
drivers/mtd/nand/raw/rockchip-nand-controller.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
index 31d8c7a87..fcda4c760 100644
--- a/drivers/mtd/nand/raw/rockchip-nand-controller.c
+++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
@@ -566,9 +566,10 @@ static int rk_nfc_write_page_raw(struct nand_chip *chip, const u8 *buf,
* BBM OOB1 OOB2 OOB3 |......| PA0 PA1 PA2 PA3
*
* The rk_nfc_ooblayout_free() function already has reserved
- * these 4 bytes with:
+ * these 4 bytes together with 2 bytes for BBM
+ * by reducing it's length:
*
- * oob_region->offset = NFC_SYS_DATA_SIZE + 2;
+ * oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
*/
if (!i)
memcpy(rk_nfc_oob_ptr(chip, i),
@@ -945,12 +946,8 @@ static int rk_nfc_ooblayout_free(struct mtd_info *mtd, int section,
if (section)
return -ERANGE;
- /*
- * The beginning of the OOB area stores the reserved data for the NFC,
- * the size of the reserved data is NFC_SYS_DATA_SIZE bytes.
- */
oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
- oob_region->offset = NFC_SYS_DATA_SIZE + 2;
+ oob_region->offset = 2;
return 0;
}
--
2.30.2
Rockchip boot blocks are written per 4 x 512 byte sectors per page.
Each page must have a page address (PA) pointer in OOB to the next page.
Pages are written in a pattern depending on the NAND chip ID.
This logic used to build a page pattern table is not fully disclosed and
is not easy to fit in the MTD framework.
The formula in rk_nfc_write_page_hwecc() function is not correct.
Make hwecc and raw behavior identical.
Generate boot block page address and pattern for hwecc in user space
and copy PA data to/from the last 4 bytes in the
chip->oob_poi data layout.
Signed-off-by: Johan Jonker <[email protected]>
---
.../mtd/nand/raw/rockchip-nand-controller.c | 34 ++++++++++++-------
1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
index 2312e2736..cafccc324 100644
--- a/drivers/mtd/nand/raw/rockchip-nand-controller.c
+++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
@@ -597,7 +597,7 @@ static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
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;
+ u32 tmp;
u8 *oob;
nand_prog_page_begin_op(chip, page, 0, NULL, 0);
@@ -624,6 +624,13 @@ static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
*
* 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
+ *
* Configure the ECC algorithm supported by the boot ROM.
*/
if ((page < (pages_per_blk * rknand->boot_blks)) &&
@@ -634,21 +641,17 @@ static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
}
for (i = 0; i < ecc->steps; i++) {
- if (!i) {
- reg = 0xFFFFFFFF;
- } else {
+ if (!i)
+ oob = chip->oob_poi + (ecc->steps - 1) * NFC_SYS_DATA_SIZE;
+ 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;
+ tmp = oob[0] | oob[1] << 8 | oob[2] << 16 | oob[3] << 24;
if (nfc->cfg->type == NFC_V9)
- nfc->oob_buf[i] = reg;
+ nfc->oob_buf[i] = tmp;
else
- nfc->oob_buf[i * (oob_step / 4)] = reg;
+ nfc->oob_buf[i * (oob_step / 4)] = tmp;
}
dma_data = dma_map_single(nfc->dev, (void *)nfc->page_buf,
@@ -811,12 +814,17 @@ static int rk_nfc_read_page_hwecc(struct nand_chip *chip, u8 *buf, int oob_on,
goto timeout_err;
}
- for (i = 1; i < ecc->steps; i++) {
- oob = chip->oob_poi + (i - 1) * NFC_SYS_DATA_SIZE;
+ for (i = 0; i < ecc->steps; i++) {
+ if (!i)
+ oob = chip->oob_poi + (ecc->steps - 1) * NFC_SYS_DATA_SIZE;
+ else
+ 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);
--
2.30.2
Hi Johan,
[email protected] wrote on Mon, 12 Jun 2023 17:03:18 +0200:
> The MTD framework reserves 1 or 2 bytes for the bad block marker
> depending on the bus size. The rockchip-nand-controller driver
> currently only supports a 8 bit bus, but reserves standard 2 bytes
> for the BBM.
We always reserve 2 bytes, no?
> The first free OOB byte is therefore OOB2 at offset 2.
> Page address(PA) bytes are moved to the last 4 positions before
> ECC. Update the description for Linux.
The description should just be:
Move Page Address (PA) bytes to the last 4 positions before ECC.
And then you should justify why this is needed. Also, this would break
all existing jffs2 users, right?
>
> Signed-off-by: Johan Jonker <[email protected]>
> ---
> drivers/mtd/nand/raw/rockchip-nand-controller.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
> index 31d8c7a87..fcda4c760 100644
> --- a/drivers/mtd/nand/raw/rockchip-nand-controller.c
> +++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
> @@ -566,9 +566,10 @@ static int rk_nfc_write_page_raw(struct nand_chip *chip, const u8 *buf,
> * BBM OOB1 OOB2 OOB3 |......| PA0 PA1 PA2 PA3
> *
> * The rk_nfc_ooblayout_free() function already has reserved
> - * these 4 bytes with:
> + * these 4 bytes together with 2 bytes for BBM
> + * by reducing it's length:
> *
> - * oob_region->offset = NFC_SYS_DATA_SIZE + 2;
> + * oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
> */
> if (!i)
> memcpy(rk_nfc_oob_ptr(chip, i),
> @@ -945,12 +946,8 @@ static int rk_nfc_ooblayout_free(struct mtd_info *mtd, int section,
> if (section)
> return -ERANGE;
>
> - /*
> - * The beginning of the OOB area stores the reserved data for the NFC,
> - * the size of the reserved data is NFC_SYS_DATA_SIZE bytes.
> - */
> oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
> - oob_region->offset = NFC_SYS_DATA_SIZE + 2;
> + oob_region->offset = 2;
>
> return 0;
> }
> --
> 2.30.2
>
Thanks,
Miquèl
Hi Johan,
[email protected] wrote on Mon, 12 Jun 2023 17:02:40 +0200:
> Rockchip boot blocks are written per 4 x 512 byte sectors per page.
> Each page must have a page address (PA) pointer in OOB to the next page.
> Pages are written in a pattern depending on the NAND chip ID.
> This logic used to build a page pattern table is not fully disclosed and
> is not easy to fit in the MTD framework.
> The formula in rk_nfc_write_page_hwecc() function is not correct.
> Make hwecc and raw behavior identical.
> Generate boot block page address and pattern for hwecc in user space
> and copy PA data to/from the last 4 bytes in the
> chip->oob_poi data layout.
>
> Signed-off-by: Johan Jonker <[email protected]>
> ---
> .../mtd/nand/raw/rockchip-nand-controller.c | 34 ++++++++++++-------
> 1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
> index 2312e2736..cafccc324 100644
> --- a/drivers/mtd/nand/raw/rockchip-nand-controller.c
> +++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
> @@ -597,7 +597,7 @@ static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
> 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;
> + u32 tmp;
> u8 *oob;
>
> nand_prog_page_begin_op(chip, page, 0, NULL, 0);
> @@ -624,6 +624,13 @@ static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
> *
> * 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.
Maybe you don't loose any data, but you basically break all existing
jffs2 users, right? Is this page address only useful on your rk SoC or
are all the SoCs using the same logic?
I think it would be best to flag where this is required and avoid a
massive incompatible change like this one (and the previous one). BTW,
any reason not to merge the two first patches? It seems like the series
would not be bisectable between the two first commits.
Patches 4 and 5 look good as they are not directly related, I'll queue
them, you can avoid re-sending them.
> + *
> + * The chip->oob_poi data layout:
> + *
> + * BBM OOB1 OOB2 OOB3 |......| PA0 PA1 PA2 PA3
> + *
> * Configure the ECC algorithm supported by the boot ROM.
> */
> if ((page < (pages_per_blk * rknand->boot_blks)) &&
> @@ -634,21 +641,17 @@ static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
> }
>
> for (i = 0; i < ecc->steps; i++) {
> - if (!i) {
> - reg = 0xFFFFFFFF;
> - } else {
> + if (!i)
> + oob = chip->oob_poi + (ecc->steps - 1) * NFC_SYS_DATA_SIZE;
> + 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;
> + tmp = oob[0] | oob[1] << 8 | oob[2] << 16 | oob[3] << 24;
>
> if (nfc->cfg->type == NFC_V9)
> - nfc->oob_buf[i] = reg;
> + nfc->oob_buf[i] = tmp;
> else
> - nfc->oob_buf[i * (oob_step / 4)] = reg;
> + nfc->oob_buf[i * (oob_step / 4)] = tmp;
> }
>
> dma_data = dma_map_single(nfc->dev, (void *)nfc->page_buf,
> @@ -811,12 +814,17 @@ static int rk_nfc_read_page_hwecc(struct nand_chip *chip, u8 *buf, int oob_on,
> goto timeout_err;
> }
>
> - for (i = 1; i < ecc->steps; i++) {
> - oob = chip->oob_poi + (i - 1) * NFC_SYS_DATA_SIZE;
> + for (i = 0; i < ecc->steps; i++) {
> + if (!i)
> + oob = chip->oob_poi + (ecc->steps - 1) * NFC_SYS_DATA_SIZE;
> + else
> + 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);
> --
> 2.30.2
>
Thanks,
Miquèl
On 6/12/23 19:26, Miquel Raynal wrote:
> Hi Johan,
>
> [email protected] wrote on Mon, 12 Jun 2023 17:03:18 +0200:
>
>> The MTD framework reserves 1 or 2 bytes for the bad block marker
>> depending on the bus size. The rockchip-nand-controller driver
>> currently only supports a 8 bit bus, but reserves standard 2 bytes
>> for the BBM.
>
> We always reserve 2 bytes, no?
Not always used, but for consistency/simplicity the author assumes/reserves 2 bytes.
>
>> The first free OOB byte is therefore OOB2 at offset 2.
>> Page address(PA) bytes are moved to the last 4 positions before
>> ECC. Update the description for Linux.
>
> The description should just be:
>
> Move Page Address (PA) bytes to the last 4 positions before ECC.
Space is already reserved, but overwritten.
>
> And then you should justify why this is needed. Also, this would break
> all existing jffs2 users, right?
Hi Miquel,
From your comments it seems that the chip->oob_poi buffer layout is still not clear to you.
Hope that this text below helps.
If existing jffs2 users of free OOB are writing they are corrupting our PA data in RAW mode.
So that must be fixed. Please advise how we split pre and post change users.
(With a Module parameter like skipbbt renamed to "user_mode" = 0 offset 6, "user_mode" = 1 offset 2)
Copying PA data in both RAW and HW mode has already reserved space in the layout.
Let me know if I can help to get forward here.
Johan
===
Given:
Rockchip rk3066 MK808 with NAND:
nand: Hynix H27UCG8T2ATR-BC 64G 3.3V 8-bit
nand: 8192 MiB, MLC, erase size: 2048 KiB, page size: 8192, OOB size: 640
===
Calulations:
#define NFC_SYS_DATA_SIZE (4) /* 4 bytes sys data in oob pre 1024 data.*/
So per step only 4 bytes of OOB can be read.
===
The NFC can read/write in 1024 data bytes per step.
To read/write a full page it needs 8 steps.
chip->ecc.size = 1024;
chip->ecc.steps = mtd->writesize / chip->ecc.size;
= 8192 / 1024
= 8 steps
===
The total size of usefull OOB before ECC:
rknand->metadata_size = NFC_SYS_DATA_SIZE * ecc->steps;
= 4 * 8
= 32
===
Wrong free OOB offset starts at OOB6:
oob_region->offset = NFC_SYS_DATA_SIZE + 2;
= 4 + 2
= 6
With a free OOB offset of 6 and a length of 26 ==> 6 + 26 = 32 we corrupt the PA address starting at offset 28.
New offset OOB2:
oob_region->offset = 2;
The full range of free runs from OOB2 till/including OOB27.
===
The last 4 bytes of metadata are reserved for this Page Address(PA) for the bootrom.
Currently only in use in RAW mode.
The current PA calculation needed to write boot blocks for all Rockchip SoCs is however useless.
The pattern of where the next page is written depends on the chip ID.
As the MTD framework doesn't pass this chip ID in it's data structures,
we must calculate that in userspace.
Therefore both RAW and HW mode must pass the PA bytes.
===
The NFC hardware is capable for a 16 bit bus, but not implemented yet.
Reserved are standard 2 bits for the BBM for a consistantency by the original author.
===
chip->oob_poi buffer layout for 8 steps:
BBM0 BBM1 OOB2 OOB3 | OOB4 OOB5 OOB6 OOB7
OOB8 OOB9 OOB10 OOB11 | OOB12 OOB13 OOB15 OOB15
OOB16 OOB17 OOB18 OOB19 | OOB20 OOB21 OOB22 OOB23
OOB24 OOB25 OOB26 OOB27 | PA0 PA1 PA2 PA3
ECC0 ECC1 ECC2 ECC3 | ... ... ... ...
===
rk_nfc_ooblayout_free:
oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
= 32 - 4 - 2
= 26
oob_region->offset = 2;
Free OOB should start at OOB2 to not overwrite PA data.
===
rk_nfc_ooblayout_ecc:
oob_region->length = mtd->oobsize - rknand->metadata_size;
= 640 - 32
= 608
oob_region->offset = rknand->metadata_size;
= 32
ECC data starts at offset 32.
===
>
>>
>> Signed-off-by: Johan Jonker <[email protected]>
>> ---
>> drivers/mtd/nand/raw/rockchip-nand-controller.c | 11 ++++-------
>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c b/drivers/mtd/nand/raw/rockchip-nand-controller.c
>> index 31d8c7a87..fcda4c760 100644
>> --- a/drivers/mtd/nand/raw/rockchip-nand-controller.c
>> +++ b/drivers/mtd/nand/raw/rockchip-nand-controller.c
>> @@ -566,9 +566,10 @@ static int rk_nfc_write_page_raw(struct nand_chip *chip, const u8 *buf,
>> * BBM OOB1 OOB2 OOB3 |......| PA0 PA1 PA2 PA3
>> *
>> * The rk_nfc_ooblayout_free() function already has reserved
>> - * these 4 bytes with:
>> + * these 4 bytes together with 2 bytes for BBM
>> + * by reducing it's length:
>> *
>> - * oob_region->offset = NFC_SYS_DATA_SIZE + 2;
>> + * oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
>> */
>> if (!i)
>> memcpy(rk_nfc_oob_ptr(chip, i),
>> @@ -945,12 +946,8 @@ static int rk_nfc_ooblayout_free(struct mtd_info *mtd, int section,
>> if (section)
>> return -ERANGE;
>>
>> - /*
>> - * The beginning of the OOB area stores the reserved data for the NFC,
>> - * the size of the reserved data is NFC_SYS_DATA_SIZE bytes.
>> - */
>> oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
>> - oob_region->offset = NFC_SYS_DATA_SIZE + 2;
>> + oob_region->offset = 2;
>>
>> return 0;
>> }
>> --
>> 2.30.2
>>
>
>
> Thanks,
> Miquèl
Hi Johan,
[email protected] wrote on Wed, 14 Jun 2023 11:23:44 +0200:
> On 6/12/23 19:26, Miquel Raynal wrote:
> > Hi Johan,
> >
> > [email protected] wrote on Mon, 12 Jun 2023 17:03:18 +0200:
> >
> >> The MTD framework reserves 1 or 2 bytes for the bad block marker
> >> depending on the bus size. The rockchip-nand-controller driver
> >> currently only supports a 8 bit bus, but reserves standard 2 bytes
> >> for the BBM.
> >
> > We always reserve 2 bytes, no?
>
> Not always used, but for consistency/simplicity the author assumes/reserves 2 bytes.
It's kind of an implicit rule in the raw NAND subsystem. It's not an
author choice.
> >> The first free OOB byte is therefore OOB2 at offset 2.
> >> Page address(PA) bytes are moved to the last 4 positions before
> >> ECC. Update the description for Linux.
> >
> > The description should just be:
> >
>
> > Move Page Address (PA) bytes to the last 4 positions before ECC.
>
> Space is already reserved, but overwritten.
Well, I don't know, but I'm quoting your commit log "Page address(PA)
bytes are moved to the last 4 positions before ECC" and if this
sentence is right, I am proposing another way to say this which sounds
more declarative.
>
> >
> > And then you should justify why this is needed. Also, this would
> > break all existing jffs2 users, right?
>
> Hi Miquel,
>
> From your comments it seems that the chip->oob_poi buffer layout is
> still not clear to you. Hope that this text below helps.
> If existing jffs2 users of free OOB are writing
They are, it's the first thing that jjfs2 does: writing cleanmarkers in
the free area.
> they are corrupting
> our PA data in RAW mode. So that must be fixed.
I did not yet understand whether corrupting the PA data was an absolute
mistake or if it was specific to a given range of ROM codes. But let's
assume it must be fixed.
> Please advise how we
> split pre and post change users.
If you change the layout, you break users. There is no question here.
But if you do that, we need:
- a crystal clear explanation of why this is needed
- to say it clearly: this change breaks existing jffs2 users
> (With a Module parameter like
> skipbbt renamed to "user_mode" = 0 offset 6, "user_mode" = 1 offset
I know the cafe driver does that, it is awful IMHO.
> 2) Copying PA data in both RAW and HW mode has already reserved space
> in the layout. Let me know if I can help to get forward here.
>
> Johan
>
> ===
>
> Given:
>
> Rockchip rk3066 MK808 with NAND:
> nand: Hynix H27UCG8T2ATR-BC 64G 3.3V 8-bit
> nand: 8192 MiB, MLC, erase size: 2048 KiB, page size: 8192, OOB size:
> 640
>
> ===
>
> Calulations:
>
> #define NFC_SYS_DATA_SIZE (4) /* 4 bytes sys data in
> oob pre 1024 data.*/
>
> So per step only 4 bytes of OOB can be read.
I think I get what you mean but the above sentence is wrong. You can
always read the full OOB in raw mode. And in general you can as well in
host ECC mode. Then what users do with the OOB information is
orthogonal. However, if they don't want their data to be smashed, they
can request the information about which bytes are free to be used
(typically what jffs2 does, while ubi does not care about OOB). The oob
layout helpers can then restrain the advertised free area to only share
bytes which are not used by the PA.
>
> ===
>
> The NFC can read/write in 1024 data bytes per step.
> To read/write a full page it needs 8 steps.
>
> chip->ecc.size = 1024;
> chip->ecc.steps = mtd->writesize / chip->ecc.size;
> = 8192 / 1024
> = 8 steps
> ===
>
> The total size of usefull OOB before ECC:
>
> rknand->metadata_size = NFC_SYS_DATA_SIZE * ecc->steps;
> = 4 * 8
> = 32
> ===
>
> Wrong free OOB offset starts at OOB6:
> oob_region->offset = NFC_SYS_DATA_SIZE + 2;
> = 4 + 2
> = 6
>
> With a free OOB offset of 6 and a length of 26 ==> 6 + 26 = 32 we
> corrupt the PA address starting at offset 28.
>
> New offset OOB2:
> oob_region->offset = 2;
>
> The full range of free runs from OOB2 till/including OOB27.
> ===
>
> The last 4 bytes of metadata are reserved for this Page Address(PA)
> for the bootrom. Currently only in use in RAW mode.
I'm not sure to understand what "currently on ly in use in raw mode".
In raw mode, the user can overwrite the whole OOB area, it is the user
input what should be written in each and every byte.
In ECC mode the ECC engine will smash some of this data to write its
own ECC bytes.
> The current PA calculation needed to write boot blocks for all
> Rockchip SoCs is however useless. The pattern of where the next page
> is written depends on the chip ID. As the MTD framework doesn't pass
> this chip ID in it's data structures, we must calculate that in
> userspace.
yes, I agree the right approach if you need to write these is to
perform raw OOB writes with values calculated manually.
> Therefore both RAW and HW mode must pass the PA bytes.
Yes, no problem with that.
> ===
>
> The NFC hardware is capable for a 16 bit bus, but not implemented yet.
> Reserved are standard 2 bits for the BBM for a consistantency by the
> original author.
>
> ===
>
> chip->oob_poi buffer layout for 8 steps:
>
> BBM0 BBM1 OOB2 OOB3 | OOB4 OOB5 OOB6 OOB7
>
> OOB8 OOB9 OOB10 OOB11 | OOB12 OOB13 OOB15 OOB15
> OOB16 OOB17 OOB18 OOB19 | OOB20 OOB21 OOB22 OOB23
>
> OOB24 OOB25 OOB26 OOB27 | PA0 PA1 PA2 PA3
>
> ECC0 ECC1 ECC2 ECC3 | ... ... ... ...
Yes.
>
> ===
>
> rk_nfc_ooblayout_free:
> oob_region->length = rknand->metadata_size - NFC_SYS_DATA_SIZE - 2;
> = 32 - 4 - 2
> = 26
>
> oob_region->offset = 2;
>
> Free OOB should start at OOB2 to not overwrite PA data.
Yes.
>
> ===
>
> rk_nfc_ooblayout_ecc:
> oob_region->length = mtd->oobsize - rknand->metadata_size;
> = 640 - 32
> = 608
> oob_region->offset = rknand->metadata_size;
> = 32
>
> ECC data starts at offset 32.
Yes.
>
> ===
>
>
> >
> >>
> >> Signed-off-by: Johan Jonker <[email protected]>
> >> ---
> >> drivers/mtd/nand/raw/rockchip-nand-controller.c | 11 ++++-------
> >> 1 file changed, 4 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/rockchip-nand-controller.c
> >> b/drivers/mtd/nand/raw/rockchip-nand-controller.c index
> >> 31d8c7a87..fcda4c760 100644 ---
> >> a/drivers/mtd/nand/raw/rockchip-nand-controller.c +++
> >> b/drivers/mtd/nand/raw/rockchip-nand-controller.c @@ -566,9
> >> +566,10 @@ static int rk_nfc_write_page_raw(struct nand_chip
> >> *chip, const u8 *buf,
> >> * BBM OOB1 OOB2 OOB3 |......| PA0 PA1 PA2
> >> PA3 *
> >> * The rk_nfc_ooblayout_free() function already
> >> has reserved
> >> - * these 4 bytes with:
> >> + * these 4 bytes together with 2 bytes for BBM
> >> + * by reducing it's length:
> >> *
> >> - * oob_region->offset = NFC_SYS_DATA_SIZE + 2;
> >> + * oob_region->length = rknand->metadata_size -
> >> NFC_SYS_DATA_SIZE - 2; */
> >> if (!i)
> >> memcpy(rk_nfc_oob_ptr(chip, i),
> >> @@ -945,12 +946,8 @@ static int rk_nfc_ooblayout_free(struct
> >> mtd_info *mtd, int section, if (section)
> >> return -ERANGE;
> >>
> >> - /*
> >> - * The beginning of the OOB area stores the reserved data
> >> for the NFC,
> >> - * the size of the reserved data is NFC_SYS_DATA_SIZE
> >> bytes.
> >> - */
> >> oob_region->length = rknand->metadata_size -
> >> NFC_SYS_DATA_SIZE - 2;
> >> - oob_region->offset = NFC_SYS_DATA_SIZE + 2;
> >> + oob_region->offset = 2;
> >>
> >> return 0;
> >> }
> >> --
> >> 2.30.2
> >>
> >
> >
> > Thanks,
> > Miquèl
Thanks,
Miquèl