On Mon, 18 Apr 2016 17:32:49 +0300
Roger Quadros [off-list ref] wrote:

quoted

Boris,

On 30/03/16 19:14, Boris Brezillon wrote:

quoted

Implementing the mtd_ooblayout_ops interface is the new way of exposing
ECC/OOB layout to MTD users.

Signed-off-by: Boris Brezillon <redacted>
---
 drivers/mtd/nand/omap2.c | 194 +++++++++++++++++++++++++++--------------------
 1 file changed, 113 insertions(+), 81 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 4ebf16b..bca154a 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c

@@ -169,8 +169,6 @@ struct omap_nand_info {
 	u_char				*buf;
 	int					buf_len;
 	struct gpmc_nand_regs		reg;
-	/* generated at runtime depending on ECC algorithm and layout selected */
-	struct nand_ecclayout		oobinfo;
 	/* fields specific for BCHx_HW ECC scheme */
 	struct device			*elm_dev;
 	struct device_node		*of_node;

@@ -1639,19 +1637,108 @@ static bool omap2_nand_ecc_check(struct omap_nand_info *info,
 	return true;
 }
 
+static int omap_ooblayout_ecc(struct mtd_info *mtd, int section,
+			      struct mtd_oob_region *oobregion)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	int off = chip->options & NAND_BUSWIDTH_16 ?
+		  BADBLOCK_MARKER_LENGTH : 1;

IMO "off = 1" is valid only for OMAP_ECC_HAM1_CODE_HW and 8-bit NAND.
For all other layouts it is always set to BADBLOCK_MARKER_LENGTH.

Indeed.

[...]

quoted

quoted

-	/* all OOB bytes from oobfree->offset till end off OOB are free */
-	ecclayout->oobfree->length = mtd->oobsize - ecclayout->oobfree->offset;
 	/* check if NAND device's OOB is enough to store ECC signatures */
-	if (mtd->oobsize < (ecclayout->eccbytes + BADBLOCK_MARKER_LENGTH)) {
+	min_oobbytes = (oobbytes_per_step *
+			(mtd->writesize / nand_chip->ecc.size)) +
+		       (nand_chip->options & NAND_BUSWIDTH_16 ?
+			BADBLOCK_MARKER_LENGTH : 1);

would it affect this as well?

And here as well.

I've generated a patch (see below) fixing those problems.

quoted

quoted

+	if (mtd->oobsize < min_oobbytes) {
 		dev_err(&info->pdev->dev,
 			"not enough OOB bytes required = %d, available=%d\n",
-			ecclayout->eccbytes, mtd->oobsize);
+			min_oobbytes, mtd->oobsize);
 		err = -EINVAL;
 		goto return_error;
 	}

I will need to test this change with all possible configurations.
The number of configurations on OMAP is a bit overwhelming :(.

Sorry about that, but at least now I have someone who can test it :).

Thanks,

Boris

--->8---

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 9b96f56..07d4039 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c

@@ -1640,9 +1640,13 @@ static bool omap2_nand_ecc_check(struct omap_nand_info *info,
 static int omap_ooblayout_ecc(struct mtd_info *mtd, int section,
 			      struct mtd_oob_region *oobregion)
 {
-	struct nand_chip *chip = mtd_to_nand(mtd);
-	int off = chip->options & NAND_BUSWIDTH_16 ?
-		  BADBLOCK_MARKER_LENGTH : 1;
+	struct omap_nand_info *info = mtd_to_omap(mtd);
+	struct nand_chip *chip = &info->nand;
+	int off = BADBLOCK_MARKER_LENGTH;
+
+	if (info->ecc_opt == OMAP_ECC_HAM1_CODE_HW &&
+	    !(chip->options & NAND_BUSWIDTH_16))
+		off = 1;
 
 	if (section)
 		return -ERANGE;

@@ -1656,9 +1660,13 @@ static int omap_ooblayout_ecc(struct mtd_info *mtd, int section,
 static int omap_ooblayout_free(struct mtd_info *mtd, int section,
 			       struct mtd_oob_region *oobregion)
 {
-	struct nand_chip *chip = mtd_to_nand(mtd);
-	int off = chip->options & NAND_BUSWIDTH_16 ?
-		  BADBLOCK_MARKER_LENGTH : 1;
+	struct omap_nand_info *info = mtd_to_omap(mtd);
+	struct nand_chip *chip = &info->nand;
+	int off = BADBLOCK_MARKER_LENGTH;
+
+	if (info->ecc_opt == OMAP_ECC_HAM1_CODE_HW &&
+	    !(chip->options & NAND_BUSWIDTH_16))
+		off = 1;
 
 	if (section)
 		return -ERANGE;

@@ -1682,8 +1690,7 @@ static int omap_sw_ooblayout_ecc(struct mtd_info *mtd, int section,
 				 struct mtd_oob_region *oobregion)
 {
 	struct nand_chip *chip = mtd_to_nand(mtd);
-	int off = chip->options & NAND_BUSWIDTH_16 ?
-		  BADBLOCK_MARKER_LENGTH : 1;
+	int off = BADBLOCK_MARKER_LENGTH;
 
 	if (section >= chip->ecc.steps)
 		return -ERANGE;

@@ -1702,8 +1709,7 @@ static int omap_sw_ooblayout_free(struct mtd_info *mtd, int section,
 				  struct mtd_oob_region *oobregion)
 {
 	struct nand_chip *chip = mtd_to_nand(mtd);
-	int off = chip->options & NAND_BUSWIDTH_16 ?
-		  BADBLOCK_MARKER_LENGTH : 1;
+	int off = BADBLOCK_MARKER_LENGTH;
 
 	if (section)
 		return -ERANGE;

@@ -1737,7 +1743,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 	dma_cap_mask_t			mask;
 	unsigned			sig;
 	struct resource			*res;
-	int				min_oobbytes;
+	int				min_oobbytes = BADBLOCK_MARKER_LENGTH;
 	int				oobbytes_per_step;
 
 	pdata = dev_get_platdata(&pdev->dev);

@@ -1921,6 +1927,9 @@ static int omap_nand_probe(struct platform_device *pdev)
 		nand_chip->ecc.correct          = omap_correct_data;
 		mtd_set_ooblayout(mtd, &omap_ooblayout_ops);
 		oobbytes_per_step		= nand_chip->ecc.bytes;
+
+		if (nand_chip->options & NAND_BUSWIDTH_16)
+			min_oobbytes		= 1;

 		break;
 
 	case OMAP_ECC_BCH4_CODE_HW_DETECTION_SW:

@@ -2038,10 +2047,8 @@ static int omap_nand_probe(struct platform_device *pdev)
 	}
 
 	/* check if NAND device's OOB is enough to store ECC signatures */
-	min_oobbytes = (oobbytes_per_step *
-			(mtd->writesize / nand_chip->ecc.size)) +
-		       (nand_chip->options & NAND_BUSWIDTH_16 ?
-			BADBLOCK_MARKER_LENGTH : 1);
+	min_oobbytes += (oobbytes_per_step *
+			 (mtd->writesize / nand_chip->ecc.size));
 	if (mtd->oobsize < min_oobbytes) {
 		dev_err(&info->pdev->dev,
 			"not enough OOB bytes required = %d, available=%d\n",