diff mbox series

[v2,3/5] mtd: nand: raw: rockchip-nand-controller: fix oobfree offset and description

Message ID f2cebf54-a16c-c849-a988-bfd98c502748@gmail.com (mailing list archive)
State New, archived
Headers show
Series Fixes for Rockchip NAND controller driver | expand

Commit Message

Johan Jonker June 12, 2023, 3:03 p.m. UTC 
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 <jbx6244@gmail.com>
---
 drivers/mtd/nand/raw/rockchip-nand-controller.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

--
2.30.2

Comments

Miquel Raynal June 12, 2023, 5:26 p.m. UTC | #1 
Hi Johan,

jbx6244@gmail.com 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 <jbx6244@gmail.com>
> ---
>  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
Johan Jonker June 14, 2023, 9:23 a.m. UTC | #2 
On 6/12/23 19:26, Miquel Raynal wrote:
> Hi Johan,
> 
> jbx6244@gmail.com 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 <jbx6244@gmail.com>
>> ---
>>  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
Miquel Raynal June 14, 2023, 4:01 p.m. UTC | #3 
Hi Johan,

jbx6244@gmail.com wrote on Wed, 14 Jun 2023 11:23:44 +0200:

> On 6/12/23 19:26, Miquel Raynal wrote:
> > Hi Johan,
> > 
> > jbx6244@gmail.com 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 <jbx6244@gmail.com>
> >> ---
> >>  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
diff mbox series

Patch

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;
 }