Skip to contentSkip to conversation

nand: brcmnand: improve hamming oob layout

PATCH Superseded Álvaro Fernández Rojas 3 participants 20 messages May 4, 2020 – May 24, 2020

Subsystems (5)
Stable branch Media (V4L/DVB) Memory technology devices (MTD) NAND flash +1 more subsystems
lore
Share & tools
lore

Álvaro Fernández Rojas

author
#0 lore raw

PATCH v3 1/2 nand: brcmnand: improve hamming oob layout

The current code generates 8 oob sections:
S1 1-5
ECC 6-8
S2 9-15
S3 16-21
ECC 22-24
S4 25-31
S5 32-37
ECC 38-40
S6 41-47
S7 48-53
ECC 54-56
S8 57-63

Change it by merging continuous sections:
S1 1-5
ECC 6-8
S2 9-21
ECC 22-24
S3 25-37
ECC 38-40
S4 41-53
ECC 54-56
S5 57-63

Signed-off-by Álvaro Fernández Rojas <[email protected]>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 28 +++++++++---------------
 1 file changed, 10 insertions(+), 18 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +10 -18
index e4e3ceeac38f..1bba309c7684 100644
@@ -1100,29 +1100,21 @@ static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section,
 	struct brcmnand_cfg *cfg = &host->hwcfg;
 	int sas = cfg->spare_area_size << cfg->sector_size_1k;
 	int sectors = cfg->page_size / (512 << cfg->sector_size_1k);
+	u32 next;
 
-	if (section >= sectors * 2)
+	if (section > sectors)
 		return -ERANGE;
 
-	oobregion->offset = (section / 2) * sas;
+	next = (section * sas);
+	if (section < sectors)
+		next += 6;
 
-	if (section & 1) {
-		oobregion->offset += 9;
-		oobregion->length = 7;
-	} else {
-		oobregion->length = 6;
+	if (section)
+		oobregion->offset = ((section - 1) * sas) + 9;
+	else
+		oobregion->offset = 1; /* BBI */
 
-		/* First sector of each page may have BBI */
-		if (!section) {
-			/*
-			 * Small-page NAND use byte 6 for BBI while large-page
-			 * NAND use byte 0.
-			 */
-			if (cfg->page_size > 512)
-				oobregion->offset++;
-			oobregion->length--;
-		}
-	}
+	oobregion->length = next - oobregion->offset;
 
 	return 0;
 }
Signature · 1 line
2.26.2
author
#1 lore raw

PATCH v3 2/2 nand: brcmnand: fix BBI in hamming oob layout

Small Page NAND uses byte 6 for BBI and Large Page NAND uses first 2 bytes.

Signed-off-by Álvaro Fernández Rojas <[email protected]>
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +11 -3
index 1bba309c7684..59c3241f4ea5 100644
@@ -1109,10 +1109,18 @@ static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section,
 	if (section < sectors)
 		next += 6;
 
-	if (section)
+	if (section) {
 		oobregion->offset = ((section - 1) * sas) + 9;
-	else
-		oobregion->offset = 1; /* BBI */
+	} else {
+		if (cfg->page_size == 512) {
+			/* small page uses byte 6 for BBI */
+			oobregion->offset = 0;
+			next--;
+		} else {
+			/* large page uses first 2 bytes for BBI */
+			oobregion->offset = 2;
+		}
+	}
 
 	oobregion->length = next - oobregion->offset;
Signature · 1 line
2.26.2
author
#2 lore raw

PATCH v2 1/2 nand: brcmnand: improve hamming oob layout

In reply to Álvaro Fernández Rojas

The current code generates 8 oob sections:
S1 1-5
ECC 6-8
S2 9-15
S3 16-21
ECC 22-24
S4 25-31
S5 32-37
ECC 38-40
S6 41-47
S7 48-53
ECC 54-56
S8 57-63

Change it by merging continuous sections:
S1 1-5
ECC 6-8
S2 9-21
ECC 22-24
S3 25-37
ECC 38-40
S4 41-53
ECC 54-56
S5 57-63

Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v2: keep original comment and fix correctly skip byte 6 for small-page nand

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 34 +++++++++++++-----------
 1 file changed, 18 insertions(+), 16 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +18 -16
index e4e3ceeac38f..767343e0e6e7 100644
@@ -1100,30 +1100,32 @@ static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section,
 	struct brcmnand_cfg *cfg = &host->hwcfg;
 	int sas = cfg->spare_area_size << cfg->sector_size_1k;
 	int sectors = cfg->page_size / (512 << cfg->sector_size_1k);
+	u32 next;
 
-	if (section >= sectors * 2)
+	if (section > sectors)
 		return -ERANGE;
 
-	oobregion->offset = (section / 2) * sas;
+	next = (section * sas);
+	if (section < sectors)
+		next += 6;
 
-	if (section & 1) {
-		oobregion->offset += 9;
-		oobregion->length = 7;
+	if (section) {
+		oobregion->offset = ((section - 1) * sas) + 9;
 	} else {
-		oobregion->length = 6;
-
-		/* First sector of each page may have BBI */
-		if (!section) {
-			/*
-			 * Small-page NAND use byte 6 for BBI while large-page
-			 * NAND use byte 0.
-			 */
-			if (cfg->page_size > 512)
-				oobregion->offset++;
-			oobregion->length--;
+		/*
+		 * Small-page NAND use byte 6 for BBI while large-page
+		 * NAND use byte 0.
+		 */
+		if (cfg->page_size > 512) {
+			oobregion->offset = 1;
+		} else {
+			oobregion->offset = 0;
+			next--;
 		}
 	}
 
+	oobregion->length = next - oobregion->offset;
+
 	return 0;
 }
Signature · 1 line
2.26.2
author
#3 lore raw

PATCH v2 2/2 nand: brcmnand: fix hamming oob layout

First 2 bytes are used in large-page nand.

Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v2: extend original comment

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +2 -2
index 767343e0e6e7..0a1d76fde37b 100644
@@ -1114,10 +1114,10 @@ static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section,
 	} else {
 		/*
 		 * Small-page NAND use byte 6 for BBI while large-page
-		 * NAND use byte 0.
+		 * NAND use bytes 0 and 1.
 		 */
 		if (cfg->page_size > 512) {
-			oobregion->offset = 1;
+			oobregion->offset = 2;
 		} else {
 			oobregion->offset = 0;
 			next--;
Signature · 1 line
2.26.2
maintainer
#4 lore raw
In reply to Álvaro Fernández Rojas

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Mon, 4 May 2020 20:59:44 +0200:

Quoted 75 lines

The current code generates 8 oob sections:
S1 1-5
ECC 6-8
S2 9-15
S3 16-21
ECC 22-24
S4 25-31
S5 32-37
ECC 38-40
S6 41-47
S7 48-53
ECC 54-56
S8 57-63

Change it by merging continuous sections:
S1 1-5
ECC 6-8
S2 9-21
ECC 22-24
S3 25-37
ECC 38-40
S4 41-53
ECC 54-56
S5 57-63

Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v2: keep original comment and fix correctly skip byte 6 for small-page nand

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 34 +++++++++++++-----------
 1 file changed, 18 insertions(+), 16 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +14 -16
index e4e3ceeac38f..767343e0e6e7 100644
@@ -1100,30 +1100,32 @@ static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section,
 	struct brcmnand_cfg *cfg = &host->hwcfg;
 	int sas = cfg->spare_area_size << cfg->sector_size_1k;
 	int sectors = cfg->page_size / (512 << cfg->sector_size_1k);
+	u32 next;
 
-	if (section >= sectors * 2)
+	if (section > sectors)
 		return -ERANGE;
 
-	oobregion->offset = (section / 2) * sas;
+	next = (section * sas);
+	if (section < sectors)
+		next += 6;
 
-	if (section & 1) {
-		oobregion->offset += 9;
-		oobregion->length = 7;
+	if (section) {
+		oobregion->offset = ((section - 1) * sas) + 9;
 	} else {
-		oobregion->length = 6;
-
-		/* First sector of each page may have BBI */
-		if (!section) {
-			/*
-			 * Small-page NAND use byte 6 for BBI while large-page
-			 * NAND use byte 0.
-			 */
-			if (cfg->page_size > 512)
-				oobregion->offset++;
-			oobregion->length--;
+		/*
+		 * Small-page NAND use byte 6 for BBI while large-page
+		 * NAND use byte 0.
+		 */
+		if (cfg->page_size > 512) {
+			oobregion->offset = 1;
+		} else {

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section, · line 1131

Quoted 10 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +4 -0
+			oobregion->offset = 0;
+			next--;
 		}
 	}
 
+	oobregion->length = next - oobregion->offset;
+
 	return 0;
 }

I'm fine with the two patches but could you please invert the order and add Fixes: + Cc: stable tags on both? on "fix hamming oob layout" please? This way the fix is valid on older versions of the driver an can be backported easily.

Thanks,
Miquèl

author
#5 lore raw

v3 cover mtd: rawnand: brcmnand: improve hamming oob layout

In reply to Álvaro Fernández Rojas

These patches improve the OOB hamming layout by reducing the number of oob sections and correctly

v3: invert patch order.
v2: extend original comment and correctly skip byte 6 for small-page.

Álvaro Fernández Rojas (2):
mtd: rawnand: brcmnand: fix hamming oob layout
mtd: rawnand: brcmnand: improve hamming oob layout

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 34 +++++++++++++-----------
 1 file changed, 18 insertions(+), 16 deletions(-)
Signature · 1 line
2.26.2
author
#6 lore raw

PATCH v3 1/2 mtd: rawnand: brcmnand: fix hamming oob layout

First 2 bytes are used in large-page nand.

Fixes ef5eeea6e911 ("mtd: nand: brcm: switch to mtd_ooblayout_ops")
Cc [email protected]
Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v3: invert patch order
v2: extend original comment

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +7 -4
index e4e3ceeac38f..1c1070111ebc 100644
@@ -1116,11 +1116,14 @@ static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section,
 		if (!section) {
 			/*
 			 * Small-page NAND use byte 6 for BBI while large-page
-			 * NAND use byte 0.
+			 * NAND use bytes 0 and 1.
 			 */
-			if (cfg->page_size > 512)
-				oobregion->offset++;
-			oobregion->length--;
+			if (cfg->page_size > 512) {
+				oobregion->offset += 2;
+				oobregion->length -= 2;
+			} else {
+				oobregion->length--;
+			}
 		}
 	}
Signature · 1 line
2.26.2
maintainer
#7 lore raw

Re: [PATCH v3 1/2] mtd: rawnand: brcmnand: fix hamming oob layout

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag fixing commit: ef5eeea6e911 ("mtd: nand: brcm: switch to mtd_ooblayout_ops").

The bot has tested the following trees: v5.6.13, v5.4.41, v4.19.123, v4.14.180, v4.9.223.

v5.6.13: Build OK!
v5.4.41: Build OK!
v4.19.123: Build OK!
v4.14.180: Failed to apply! Possible dependencies:
Unable to calculate

v4.9.223: Failed to apply! Possible dependencies:
Unable to calculate

NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

Signature · 2 lines
Thanks
Sasha
author
#8 lore raw

PATCH v3 2/2 mtd: rawnand: brcmnand: improve hamming oob layout

In reply to Álvaro Fernández Rojas

The current code generates 8 oob sections:
S1 1-5
ECC 6-8
S2 9-15
S3 16-21
ECC 22-24
S4 25-31
S5 32-37
ECC 38-40
S6 41-47
S7 48-53
ECC 54-56
S8 57-63

Change it by merging continuous sections:
S1 1-5
ECC 6-8
S2 9-21
ECC 22-24
S3 25-37
ECC 38-40
S4 41-53
ECC 54-56
S5 57-63

Fixes ef5eeea6e911 ("mtd: nand: brcm: switch to mtd_ooblayout_ops")
Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v3: invert patch order
v2: keep original comment and fix correctly skip byte 6 for small-page nand

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 37 ++++++++++++------------
 1 file changed, 18 insertions(+), 19 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +18 -19
index 1c1070111ebc..0a1d76fde37b 100644
@@ -1100,33 +1100,32 @@ static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section,
 	struct brcmnand_cfg *cfg = &host->hwcfg;
 	int sas = cfg->spare_area_size << cfg->sector_size_1k;
 	int sectors = cfg->page_size / (512 << cfg->sector_size_1k);
+	u32 next;
 
-	if (section >= sectors * 2)
+	if (section > sectors)
 		return -ERANGE;
 
-	oobregion->offset = (section / 2) * sas;
+	next = (section * sas);
+	if (section < sectors)
+		next += 6;
 
-	if (section & 1) {
-		oobregion->offset += 9;
-		oobregion->length = 7;
+	if (section) {
+		oobregion->offset = ((section - 1) * sas) + 9;
 	} else {
-		oobregion->length = 6;
-
-		/* First sector of each page may have BBI */
-		if (!section) {
-			/*
-			 * Small-page NAND use byte 6 for BBI while large-page
-			 * NAND use bytes 0 and 1.
-			 */
-			if (cfg->page_size > 512) {
-				oobregion->offset += 2;
-				oobregion->length -= 2;
-			} else {
-				oobregion->length--;
-			}
+		/*
+		 * Small-page NAND use byte 6 for BBI while large-page
+		 * NAND use bytes 0 and 1.
+		 */
+		if (cfg->page_size > 512) {
+			oobregion->offset = 2;
+		} else {
+			oobregion->offset = 0;
+			next--;
 		}
 	}
 
+	oobregion->length = next - oobregion->offset;
+
 	return 0;
 }
Signature · 1 line
2.26.2
maintainer
#9 lore raw

Re: [PATCH v3 2/2] mtd: rawnand: brcmnand: improve hamming oob layout

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020 08:00:23 +0200:

Quoted 25 lines

The current code generates 8 oob sections:
S1 1-5
ECC 6-8
S2 9-15
S3 16-21
ECC 22-24
S4 25-31
S5 32-37
ECC 38-40
S6 41-47
S7 48-53
ECC 54-56
S8 57-63

Change it by merging continuous sections:
S1 1-5
ECC 6-8
S2 9-21
ECC 22-24
S3 25-37
ECC 38-40
S4 41-53
ECC 54-56
S5 57-63

Quoted 1 line
Fixes ef5eeea6e911 ("mtd: nand: brcm: switch to mtd_ooblayout_ops")

Sorry for leading you the wrong way, actually this patch does not deserve a Fixes tag.

Quoted 46 lines
Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v3: invert patch order
v2: keep original comment and fix correctly skip byte 6 for small-page nand

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 37 ++++++++++++------------
 1 file changed, 18 insertions(+), 19 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +7 -18
index 1c1070111ebc..0a1d76fde37b 100644
@@ -1100,33 +1100,32 @@ static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section,
 	struct brcmnand_cfg *cfg = &host->hwcfg;
 	int sas = cfg->spare_area_size << cfg->sector_size_1k;
 	int sectors = cfg->page_size / (512 << cfg->sector_size_1k);
+	u32 next;
 
-	if (section >= sectors * 2)
+	if (section > sectors)
 		return -ERANGE;
 
-	oobregion->offset = (section / 2) * sas;
+	next = (section * sas);
+	if (section < sectors)
+		next += 6;
 
-	if (section & 1) {
-		oobregion->offset += 9;
-		oobregion->length = 7;
+	if (section) {
+		oobregion->offset = ((section - 1) * sas) + 9;
 	} else {
-		oobregion->length = 6;
-
-		/* First sector of each page may have BBI */
-		if (!section) {
-			/*
-			 * Small-page NAND use byte 6 for BBI while large-page
-			 * NAND use bytes 0 and 1.
-			 */
-			if (cfg->page_size > 512) {
-				oobregion->offset += 2;
-				oobregion->length -= 2;
-			} else {
-				oobregion->length--;

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section, · line 1123

Quoted 10 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +9 -1
-			}
+		/*
+		 * Small-page NAND use byte 6 for BBI while large-page
+		 * NAND use bytes 0 and 1.
+		 */
+		if (cfg->page_size > 512) {
+			oobregion->offset = 2;
+		} else {
+			oobregion->offset = 0;
+			next--;

This next-- seems very strange, can you explain?

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section, · line 1131

Quoted 8 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +2 -0
 		}
 	}
 
+	oobregion->length = next - oobregion->offset;
+
 	return 0;
 }

Thanks,
Miquèl

author
#10 lore raw

Re: [PATCH v3 2/2] mtd: rawnand: brcmnand: improve hamming oob layout

Hi Miquel,

I also had a hard time understanding your email. It was quite misleading.

Quoted 32 lines · Miquel Raynal

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020 08:00:23 +0200:

Quoted 25 lines

The current code generates 8 oob sections:
S1 1-5
ECC 6-8
S2 9-15
S3 16-21
ECC 22-24
S4 25-31
S5 32-37
ECC 38-40
S6 41-47
S7 48-53
ECC 54-56
S8 57-63

Change it by merging continuous sections:
S1 1-5
ECC 6-8
S2 9-21
ECC 22-24
S3 25-37
ECC 38-40
S4 41-53
ECC 54-56
S5 57-63

Quoted 4 lines
Quoted 1 line
Fixes ef5eeea6e911 ("mtd: nand: brcm: switch to mtd_ooblayout_ops")

Sorry for leading you the wrong way, actually this patch does not deserve a Fixes tag.

Do I need to resend this again?
Looks like no matter what I do it’s always wrong...

Quoted 51 lines
Quoted 51 lines
Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v3: invert patch order
v2: keep original comment and fix correctly skip byte 6 for small-page nand

drivers/mtd/nand/raw/brcmnand/brcmnand.c | 37 ++++++++++++------------

1 file changed, 18 insertions(+), 19 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +10 -19
index 1c1070111ebc..0a1d76fde37b 100644
@@ -1100,33 +1100,32 @@ static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section,
 	struct brcmnand_cfg *cfg = &host->hwcfg;
 	int sas = cfg->spare_area_size << cfg->sector_size_1k;
 	int sectors = cfg->page_size / (512 << cfg->sector_size_1k);
+	u32 next;
 
-	if (section >= sectors * 2)
+	if (section > sectors)
 		return -ERANGE;
 
-	oobregion->offset = (section / 2) * sas;
+	next = (section * sas);
+	if (section < sectors)
+		next += 6;
 
-	if (section & 1) {
-		oobregion->offset += 9;
-		oobregion->length = 7;
+	if (section) {
+		oobregion->offset = ((section - 1) * sas) + 9;
 	} else {
-		oobregion->length = 6;
-
-		/* First sector of each page may have BBI */
-		if (!section) {
-			/*
-			 * Small-page NAND use byte 6 for BBI while large-page
-			 * NAND use bytes 0 and 1.
-			 */
-			if (cfg->page_size > 512) {
-				oobregion->offset += 2;
-				oobregion->length -= 2;
-			} else {
-				oobregion->length--;
-			}
+		/*
+		 * Small-page NAND use byte 6 for BBI while large-page
+		 * NAND use bytes 0 and 1.
Quoted 8 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section, · line 1123

Quoted 6 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +6 -0
+		 */
+		if (cfg->page_size > 512) {
+			oobregion->offset = 2;
+		} else {
+			oobregion->offset = 0;
+			next--;

This next-- seems very strange, can you explain?

In this case next will be 6 (which is the first ECC byte).
However, for small page NANDs byte 5 is reserved for BBT, so we want next to be 5 only in this case.

Quoted 4 lines
Quoted 4 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +0 -0
 		}
 	}
Quoted 9 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section, · line 1131

Quoted 5 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +2 -0
+	oobregion->length = next - oobregion->offset;
+
 	return 0;
 }

Thanks,
Miquèl

Regards,
Álvaro.

maintainer
#11 lore raw

Re: [PATCH v3 2/2] mtd: rawnand: brcmnand: improve hamming oob layout

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020 09:12:10 +0200:

Quoted 37 lines

Hi Miquel,

I also had a hard time understanding your email. It was quite misleading.

Quoted 32 lines · Miquel Raynal

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020 08:00:23 +0200:

Quoted 25 lines

The current code generates 8 oob sections:
S1 1-5
ECC 6-8
S2 9-15
S3 16-21
ECC 22-24
S4 25-31
S5 32-37
ECC 38-40
S6 41-47
S7 48-53
ECC 54-56
S8 57-63

Change it by merging continuous sections:
S1 1-5
ECC 6-8
S2 9-21
ECC 22-24
S3 25-37
ECC 38-40
S4 41-53
ECC 54-56
S5 57-63

Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Fixes ef5eeea6e911 ("mtd: nand: brcm: switch to mtd_ooblayout_ops")

Sorry for leading you the wrong way, actually this patch does not deserve a Fixes tag.

Do I need to resend this again?
Looks like no matter what I do it’s always wrong...

Please don't give up! It is normal to work back and forth with the community. I need the patch to be clear and bug-free so I ask you to make changes and ask questions, that's how it works. But all your patches are enhancing this driver so please keep posting!

Quoted 61 lines
Quoted 52 lines
Quoted 52 lines
Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v3: invert patch order
v2: keep original comment and fix correctly skip byte 6 for small-page nand

drivers/mtd/nand/raw/brcmnand/brcmnand.c | 37 ++++++++++++------------

1 file changed, 18 insertions(+), 19 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +10 -19
index 1c1070111ebc..0a1d76fde37b 100644
@@ -1100,33 +1100,32 @@ static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section,
 	struct brcmnand_cfg *cfg = &host->hwcfg;
 	int sas = cfg->spare_area_size << cfg->sector_size_1k;
 	int sectors = cfg->page_size / (512 << cfg->sector_size_1k);
+	u32 next;
 
-	if (section >= sectors * 2)
+	if (section > sectors)
 		return -ERANGE;
 
-	oobregion->offset = (section / 2) * sas;
+	next = (section * sas);
+	if (section < sectors)
+		next += 6;
 
-	if (section & 1) {
-		oobregion->offset += 9;
-		oobregion->length = 7;
+	if (section) {
+		oobregion->offset = ((section - 1) * sas) + 9;
 	} else {
-		oobregion->length = 6;
-
-		/* First sector of each page may have BBI */
-		if (!section) {
-			/*
-			 * Small-page NAND use byte 6 for BBI while large-page
-			 * NAND use bytes 0 and 1.
-			 */
-			if (cfg->page_size > 512) {
-				oobregion->offset += 2;
-				oobregion->length -= 2;
-			} else {
-				oobregion->length--;
-			}
+		/*
+		 * Small-page NAND use byte 6 for BBI while large-page
+		 * NAND use bytes 0 and 1.
Quoted 8 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section, · line 1123

Quoted 6 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +6 -0
+		 */
+		if (cfg->page_size > 512) {
+			oobregion->offset = 2;
+		} else {
+			oobregion->offset = 0;
+			next--;

This next-- seems very strange, can you explain?

Quoted 2 lines

In this case next will be 6 (which is the first ECC byte).
However, for small page NANDs byte 5 is reserved for BBT, so we want next to be 5 only in this case.

That's clear, please add a comment there then.

Quoted 15 lines
Quoted 5 lines
Quoted 5 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +0 -0
 		}
 	}
Quoted 9 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section, · line 1131

Quoted 5 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +2 -0
+	oobregion->length = next - oobregion->offset;
+
 	return 0;
 }

Thanks,
Miquèl

Quoted 2 lines

Regards,
Álvaro.

Thanks,
Miquèl

author
#12 lore raw

Re: [PATCH v3 2/2] mtd: rawnand: brcmnand: improve hamming oob layout

Hi Miquèl,

Quoted 118 lines · Miquel Raynal

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020 09:12:10 +0200:

Quoted 37 lines

Hi Miquel,

I also had a hard time understanding your email. It was quite misleading.

Quoted 32 lines · Miquel Raynal

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020 08:00:23 +0200:

Quoted 25 lines

The current code generates 8 oob sections:
S1 1-5
ECC 6-8
S2 9-15
S3 16-21
ECC 22-24
S4 25-31
S5 32-37
ECC 38-40
S6 41-47
S7 48-53
ECC 54-56
S8 57-63

Change it by merging continuous sections:
S1 1-5
ECC 6-8
S2 9-21
ECC 22-24
S3 25-37
ECC 38-40
S4 41-53
ECC 54-56
S5 57-63

Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Fixes ef5eeea6e911 ("mtd: nand: brcm: switch to mtd_ooblayout_ops")

Sorry for leading you the wrong way, actually this patch does not deserve a Fixes tag.

Do I need to resend this again?
Looks like no matter what I do it’s always wrong...

Please don't give up! It is normal to work back and forth with the community. I need the patch to be clear and bug-free so I ask you to make changes and ask questions, that's how it works. But all your patches are enhancing this driver so please keep posting!

Quoted 61 lines
Quoted 60 lines
Quoted 48 lines
Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v3: invert patch order
v2: keep original comment and fix correctly skip byte 6 for small-page nand

drivers/mtd/nand/raw/brcmnand/brcmnand.c | 37 ++++++++++++------------

1 file changed, 18 insertions(+), 19 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +7 -18
index 1c1070111ebc..0a1d76fde37b 100644
@@ -1100,33 +1100,32 @@ static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section,
 	struct brcmnand_cfg *cfg = &host->hwcfg;
 	int sas = cfg->spare_area_size << cfg->sector_size_1k;
 	int sectors = cfg->page_size / (512 << cfg->sector_size_1k);
+	u32 next;
 
-	if (section >= sectors * 2)
+	if (section > sectors)
 		return -ERANGE;
 
-	oobregion->offset = (section / 2) * sas;
+	next = (section * sas);
+	if (section < sectors)
+		next += 6;
 
-	if (section & 1) {
-		oobregion->offset += 9;
-		oobregion->length = 7;
+	if (section) {
+		oobregion->offset = ((section - 1) * sas) + 9;
 	} else {
-		oobregion->length = 6;
-
-		/* First sector of each page may have BBI */
-		if (!section) {
-			/*
-			 * Small-page NAND use byte 6 for BBI while large-page
-			 * NAND use bytes 0 and 1.
-			 */
-			if (cfg->page_size > 512) {
-				oobregion->offset += 2;
-				oobregion->length -= 2;
-			} else {
-				oobregion->length--;

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section, · line 1123

Quoted 10 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +9 -1
-			}
+		/*
+		 * Small-page NAND use byte 6 for BBI while large-page
+		 * NAND use bytes 0 and 1.
+		 */
+		if (cfg->page_size > 512) {
+			oobregion->offset = 2;
+		} else {
+			oobregion->offset = 0;
+			next--;

This next-- seems very strange, can you explain?

Quoted 4 lines
Quoted 2 lines

In this case next will be 6 (which is the first ECC byte).
However, for small page NANDs byte 5 is reserved for BBT, so we want next to be 5 only in this case.

That's clear, please add a comment there then.

Isn’t “Small-page NAND use byte 6 for BBI while large-page NAND use bytes 0 and 1.” enough? Do we really need a specific comment before next--?

Quoted 16 lines
Quoted 16 lines
Quoted 15 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section, · line 1131

Quoted 11 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +2 -0
 		}
 	}
 
+	oobregion->length = next - oobregion->offset;
+
 	return 0;
 }

Thanks,
Miquèl

Quoted 7 lines
Quoted 2 lines

Regards,
Álvaro.

Thanks,
Miquèl

Regards,
Álvaro.

maintainer
#13 lore raw

Re: [PATCH v3 2/2] mtd: rawnand: brcmnand: improve hamming oob layout

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020 09:26:23 +0200:

Quoted 120 lines

Hi Miquèl,

Quoted 118 lines · Miquel Raynal

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020 09:12:10 +0200:

Quoted 37 lines

Hi Miquel,

I also had a hard time understanding your email. It was quite misleading.

Quoted 32 lines · Miquel Raynal

Hi Álvaro,

Álvaro Fernández Rojas <[email protected]> wrote on Tue, 12 May 2020 08:00:23 +0200:

Quoted 25 lines

The current code generates 8 oob sections:
S1 1-5
ECC 6-8
S2 9-15
S3 16-21
ECC 22-24
S4 25-31
S5 32-37
ECC 38-40
S6 41-47
S7 48-53
ECC 54-56
S8 57-63

Change it by merging continuous sections:
S1 1-5
ECC 6-8
S2 9-21
ECC 22-24
S3 25-37
ECC 38-40
S4 41-53
ECC 54-56
S5 57-63

Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Fixes ef5eeea6e911 ("mtd: nand: brcm: switch to mtd_ooblayout_ops")

Sorry for leading you the wrong way, actually this patch does not deserve a Fixes tag.

Do I need to resend this again?
Looks like no matter what I do it’s always wrong...

Please don't give up! It is normal to work back and forth with the community. I need the patch to be clear and bug-free so I ask you to make changes and ask questions, that's how it works. But all your patches are enhancing this driver so please keep posting!

Quoted 61 lines
Quoted 60 lines
Quoted 48 lines
Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v3: invert patch order
v2: keep original comment and fix correctly skip byte 6 for small-page nand

drivers/mtd/nand/raw/brcmnand/brcmnand.c | 37 ++++++++++++------------

1 file changed, 18 insertions(+), 19 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +7 -18
index 1c1070111ebc..0a1d76fde37b 100644
@@ -1100,33 +1100,32 @@ static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section,
 	struct brcmnand_cfg *cfg = &host->hwcfg;
 	int sas = cfg->spare_area_size << cfg->sector_size_1k;
 	int sectors = cfg->page_size / (512 << cfg->sector_size_1k);
+	u32 next;
 
-	if (section >= sectors * 2)
+	if (section > sectors)
 		return -ERANGE;
 
-	oobregion->offset = (section / 2) * sas;
+	next = (section * sas);
+	if (section < sectors)
+		next += 6;
 
-	if (section & 1) {
-		oobregion->offset += 9;
-		oobregion->length = 7;
+	if (section) {
+		oobregion->offset = ((section - 1) * sas) + 9;
 	} else {
-		oobregion->length = 6;
-
-		/* First sector of each page may have BBI */
-		if (!section) {
-			/*
-			 * Small-page NAND use byte 6 for BBI while large-page
-			 * NAND use bytes 0 and 1.
-			 */
-			if (cfg->page_size > 512) {
-				oobregion->offset += 2;
-				oobregion->length -= 2;
-			} else {
-				oobregion->length--;

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section, · line 1123

Quoted 10 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +9 -1
-			}
+		/*
+		 * Small-page NAND use byte 6 for BBI while large-page
+		 * NAND use bytes 0 and 1.
+		 */
+		if (cfg->page_size > 512) {
+			oobregion->offset = 2;
+		} else {
+			oobregion->offset = 0;
+			next--;

This next-- seems very strange, can you explain?

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

In this case next will be 6 (which is the first ECC byte).
However, for small page NANDs byte 5 is reserved for BBT, so we want next to be 5 only in this case.

That's clear, please add a comment there then.

Isn’t “Small-page NAND use byte 6 for BBI while large-page NAND use bytes 0 and 1.” enough? Do we really need a specific comment before next--?

Given the time it took me to understand these lines, I'd say : "no" :)

Just give more information in your main comment, explaining than in one case the reserved bytes are at the beginning (enlarging the offset) while in the other case it is at the end, so reducing the section.

Quoted 22 lines
Quoted 22 lines
Quoted 19 lines
Quoted 7 lines
Quoted 7 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +0 -0
 		}
 	}
Quoted 9 lines

drivers/mtd/nand/raw/brcmnand/brcmnand.c static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section, · line 1131

Quoted 5 lines
drivers/mtd/nand/raw/brcmnand/brcmnand.c +2 -0
+	oobregion->length = next - oobregion->offset;
+
 	return 0;
 }

Thanks,
Miquèl

Regards,
Álvaro.

Quoted 6 lines
Quoted 2 lines

Thanks,
Miquèl

Regards,
Álvaro.

Thanks,
Miquèl

author
#14 lore raw

v4 cover mtd: rawnand: brcmnand: improve hamming oob layout

In reply to Álvaro Fernández Rojas

These patches improve the OOB hamming layout by reducing the number of oob sections and correctly reserving first two bytes for large page NANDs.

v4: clarify small/large pages comment.
v3: invert patch order.
v2: extend original comment and correctly skip byte 6 for small-page.

Álvaro Fernández Rojas (2):
mtd: rawnand: brcmnand: fix hamming oob layout
mtd: rawnand: brcmnand: improve hamming oob layout

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 34 +++++++++++++-----------
 1 file changed, 18 insertions(+), 16 deletions(-)
Signature · 1 line
2.26.2
author
#15 lore raw

PATCH v4 1/2 mtd: rawnand: brcmnand: fix hamming oob layout

First 2 bytes are used in large-page nand.

Fixes ef5eeea6e911 ("mtd: nand: brcm: switch to mtd_ooblayout_ops")
Cc [email protected]
Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v4: no changes
v3: invert patch order
v2: extend original comment

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +7 -4
index e4e3ceeac38f..1c1070111ebc 100644
@@ -1116,11 +1116,14 @@ static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section,
 		if (!section) {
 			/*
 			 * Small-page NAND use byte 6 for BBI while large-page
-			 * NAND use byte 0.
+			 * NAND use bytes 0 and 1.
 			 */
-			if (cfg->page_size > 512)
-				oobregion->offset++;
-			oobregion->length--;
+			if (cfg->page_size > 512) {
+				oobregion->offset += 2;
+				oobregion->length -= 2;
+			} else {
+				oobregion->length--;
+			}
 		}
 	}
Signature · 1 line
2.26.2
maintainer
#16 lore raw

Re: [PATCH v4 1/2] mtd: rawnand: brcmnand: fix hamming oob layout

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag fixing commit: ef5eeea6e911 ("mtd: nand: brcm: switch to mtd_ooblayout_ops").

The bot has tested the following trees: v5.6.13, v5.4.41, v4.19.123, v4.14.180, v4.9.223.

v5.6.13: Build OK!
v5.4.41: Build OK!
v4.19.123: Build OK!
v4.14.180: Failed to apply! Possible dependencies:
Unable to calculate

v4.9.223: Failed to apply! Possible dependencies:
Unable to calculate

NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

Signature · 2 lines
Thanks
Sasha
maintainer Applied
#17 lore raw

Re: [PATCH v4 1/2] mtd: rawnand: brcmnand: fix hamming oob layout

In reply to Álvaro Fernández Rojas
Quoted 5 lines · On Tue, 2020-05-12 at 07:57:32 UTC, =?utf-8?q?=C3=81lvaro_Fern=C3=A1ndez_Rojas?…

First 2 bytes are used in large-page nand.

Fixes ef5eeea6e911 ("mtd: nand: brcm: switch to mtd_ooblayout_ops")
Cc [email protected]
Signed-off-by Álvaro Fernández Rojas <[email protected]>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

author
#18 lore raw

PATCH v4 2/2 mtd: rawnand: brcmnand: improve hamming oob layout

In reply to Álvaro Fernández Rojas

The current code generates 8 oob sections:
S1 1-5
ECC 6-8
S2 9-15
S3 16-21
ECC 22-24
S4 25-31
S5 32-37
ECC 38-40
S6 41-47
S7 48-53
ECC 54-56
S8 57-63

Change it by merging continuous sections:
S1 1-5
ECC 6-8
S2 9-21
ECC 22-24
S3 25-37
ECC 38-40
S4 41-53
ECC 54-56
S5 57-63

Signed-off-by Álvaro Fernández Rojas <[email protected]>

---
v4: clarify small/large pages comment
v3: invert patch order
v2: keep original comment and fix correctly skip byte 6 for small-page nand

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 35 +++++++++++-------------
 1 file changed, 16 insertions(+), 19 deletions(-)
drivers/mtd/nand/raw/brcmnand/brcmnand.c +16 -19
index 1c1070111ebc..6292fac6cc4f 100644
@@ -1100,33 +1100,30 @@ static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section,
 	struct brcmnand_cfg *cfg = &host->hwcfg;
 	int sas = cfg->spare_area_size << cfg->sector_size_1k;
 	int sectors = cfg->page_size / (512 << cfg->sector_size_1k);
+	u32 next;
 
-	if (section >= sectors * 2)
+	if (section > sectors)
 		return -ERANGE;
 
-	oobregion->offset = (section / 2) * sas;
+	next = (section * sas);
+	if (section < sectors)
+		next += 6;
 
-	if (section & 1) {
-		oobregion->offset += 9;
-		oobregion->length = 7;
+	if (section) {
+		oobregion->offset = ((section - 1) * sas) + 9;
 	} else {
-		oobregion->length = 6;
-
-		/* First sector of each page may have BBI */
-		if (!section) {
-			/*
-			 * Small-page NAND use byte 6 for BBI while large-page
-			 * NAND use bytes 0 and 1.
-			 */
-			if (cfg->page_size > 512) {
-				oobregion->offset += 2;
-				oobregion->length -= 2;
-			} else {
-				oobregion->length--;
-			}
+		if (cfg->page_size > 512) {
+			/* Large page NAND uses first 2 bytes for BBI */
+			oobregion->offset = 2;
+		} else {
+			/* Small page NAND uses last byte before ECC for BBI */
+			oobregion->offset = 0;
+			next--;
 		}
 	}
 
+	oobregion->length = next - oobregion->offset;
+
 	return 0;
 }
Signature · 1 line
2.26.2
maintainer Applied
#19 lore raw

Re: [PATCH v4 2/2] mtd: rawnand: brcmnand: improve hamming oob layout

Quoted 25 lines · On Tue, 2020-05-12 at 07:57:33 UTC, =?utf-8?q?=C3=81lvaro_Fern=C3=A1ndez_Rojas?…

The current code generates 8 oob sections:
S1 1-5
ECC 6-8
S2 9-15
S3 16-21
ECC 22-24
S4 25-31
S5 32-37
ECC 38-40
S6 41-47
S7 48-53
ECC 54-56
S8 57-63

Change it by merging continuous sections:
S1 1-5
ECC 6-8
S2 9-21
ECC 22-24
S3 25-37
ECC 38-40
S4 41-53
ECC 54-56
S5 57-63

Quoted 1 line
Signed-off-by Álvaro Fernández Rojas <[email protected]>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel