Re: [PATCH 01/14] mtd: nand: brcm: switch to mtd_ooblayout_ops

Hi Linus,

Unfortunately the u-boot nand base code still uses nand_ecclayout structure because it was based on old kernel nand driver. Your change cause bugcheck in the nand_scan_tail at line 4978 when mtd->oobsize is not one of the default size (i.e. some large nand with BCH-8 ecc requirement and has 224 bytes oobsize per 4K page) because ecc->layout is never set. Also certainly any data built based on nand_cclayout like mtd->oobavail will not be correct.

I actually converted back to nand_ecclayout structure from mtd_ooblayout with this fix to solve the above issues. Fixes: e365de90517b ("drivers: nand: brcmnand: fix nand_chip ecc layout structure")

I can see your point to get the latest from the brcmnand linux kernel driver but this requires updating the u-boot nand base driver to use mtd_ooblayout as well and all others nand controller drivers too. I am not sure if this is something you want to tackle right now.

As far as I can see, all other oob/ecc layout setting patches you back ported in this series are not in the original brcmstb_choose_ecc_layout code you replaced. So I am not worried about that we must switch to mtd_ooblayout_ops at this point. If indeed there is bug in brcmstb_choose_ecc_layout, we can port and convert the fix to nand_ecclayout structure from kernel code.

Was the bug you were hunting down in the code related to this patch?

Thanks, William

On 01/15/2023 11:52 AM, Linus Walleij wrote:

From: Boris Brezillon boris.brezillon@free-electrons.com

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

Signed-off-by: Boris Brezillon boris.brezillon@free-electrons.com [Ported to U-Boot from the Linux kernel] Signed-off-by: Linus Walleij linus.walleij@linaro.org

---

drivers/mtd/nand/raw/brcmnand/brcmnand.c | 260 ++++++++++++++--------- 1 file changed, 156 insertions(+), 104 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c index 74c9348f7fc4..8ea33e861354 100644 --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c @@ -894,131 +894,183 @@ static inline bool is_hamming_ecc(struct brcmnand_controller *ctrl, }

/*

• • Returns a nand_ecclayout strucutre for the given layout/configuration.
• • Returns NULL on failure.

• • Set mtd->ooblayout to the appropriate mtd_ooblayout_ops given
• • the layout/configuration.
• • Returns -ERRCODE on failure.
  */

-static struct nand_ecclayout *brcmnand_create_layout(int ecc_level,

• 				     struct brcmnand_host *host)
  

+static int brcmnand_hamming_ooblayout_ecc(struct mtd_info *mtd, int section,

• 			  struct mtd_oob_region *oobregion)
  

  {
• struct nand_chip *chip = mtd_to_nand(mtd);
• struct brcmnand_host *host = nand_get_controller_data(chip); struct brcmnand_cfg *cfg = &host->hwcfg;

• int i, j;
• struct nand_ecclayout *layout;
• int req;
• int sectors;
• int sas;
• int idx1, idx2;

-#ifndef __UBOOT__

• layout = devm_kzalloc(&host->pdev->dev, sizeof(*layout), GFP_KERNEL);

-#else

• layout = devm_kzalloc(host->pdev, sizeof(*layout), GFP_KERNEL);

-#endif

• if (!layout)

• return NULL;
  

• sectors = cfg->page_size / (512 << cfg->sector_size_1k);
• sas = cfg->spare_area_size << cfg->sector_size_1k;
• /* Hamming */
• if (is_hamming_ecc(host->ctrl, cfg)) {

• for (i = 0, idx1 = 0, idx2 = 0; i < sectors; i++) {
  

• 	/* First sector of each page may have BBI */
  

• 	if (i == 0) {
  

• 		layout->oobfree[idx2].offset = i * sas + 1;
  

• 		/* Small-page NAND use byte 6 for BBI */
  

• 		if (cfg->page_size == 512)
  

• 			layout->oobfree[idx2].offset--;
  

• 		layout->oobfree[idx2].length = 5;
  

• 	} else {
  

• 		layout->oobfree[idx2].offset = i * sas;
  

• 		layout->oobfree[idx2].length = 6;
  

• 	}
  

• 	idx2++;
  

• 	layout->eccpos[idx1++] = i * sas + 6;
  

• 	layout->eccpos[idx1++] = i * sas + 7;
  

• 	layout->eccpos[idx1++] = i * sas + 8;
  

• 	layout->oobfree[idx2].offset = i * sas + 9;
  

• 	layout->oobfree[idx2].length = 7;
  

• 	idx2++;
  

• 	/* Leave zero-terminated entry for OOBFREE */
  

• 	if (idx1 >= MTD_MAX_ECCPOS_ENTRIES_LARGE ||
  

• 	    idx2 >= MTD_MAX_OOBFREE_ENTRIES_LARGE - 1)
  

• 		break;
  

• }
  

• int sas = cfg->spare_area_size << cfg->sector_size_1k;
• int sectors = cfg->page_size / (512 << cfg->sector_size_1k);

• return layout;
  

• }

• if (section >= sectors)

• return -ERANGE;
  

• oobregion->offset = (section * sas) + 6;
• oobregion->length = 3;

• /*

• * CONTROLLER_VERSION:
  

• *   < v5.0: ECC_REQ = ceil(BCH_T * 13/8)
  

• *  >= v5.0: ECC_REQ = ceil(BCH_T * 14/8)
  

• * But we will just be conservative.
  

• */
  

• req = DIV_ROUND_UP(ecc_level * 14, 8);
• if (req >= sas) {

• dev_err(host->pdev,
  

• 	"error: ECC too large for OOB (ECC bytes %d, spare sector %d)\n",
  

• 	req, sas);
  

• return NULL;
  

• }

• return 0;

+}

+static int brcmnand_hamming_ooblayout_free(struct mtd_info *mtd, int section,

• 			   struct mtd_oob_region *oobregion)
  

+{

• struct nand_chip *chip = mtd_to_nand(mtd);
• struct brcmnand_host *host = nand_get_controller_data(chip);
• 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);
• if (section >= sectors * 2)

• return -ERANGE;
  

• oobregion->offset = (section / 2) * sas;

• layout->eccbytes = req * sectors;
• for (i = 0, idx1 = 0, idx2 = 0; i < sectors; i++) {

• for (j = sas - req; j < sas && idx1 <
  

• 		MTD_MAX_ECCPOS_ENTRIES_LARGE; j++, idx1++)
  

• 	layout->eccpos[idx1] = i * sas + j;
  

• if (section & 1) {

• oobregion->offset += 9;
  

• oobregion->length = 7;
  

• } else {

• oobregion->length = 6;
  
  

  /* First sector of each page may have BBI */

• if (i == 0) {
  

• 	if (cfg->page_size == 512 && (sas - req >= 6)) {
  

• 		/* Small-page NAND use byte 6 for BBI */
  

• 		layout->oobfree[idx2].offset = 0;
  

• 		layout->oobfree[idx2].length = 5;
  

• 		idx2++;
  

• 		if (sas - req > 6) {
  

• 			layout->oobfree[idx2].offset = 6;
  

• 			layout->oobfree[idx2].length =
  

• 				sas - req - 6;
  

• 			idx2++;
  

• 		}
  

• 	} else if (sas > req + 1) {
  

• 		layout->oobfree[idx2].offset = i * sas + 1;
  

• 		layout->oobfree[idx2].length = sas - req - 1;
  

• 		idx2++;
  

• 	}
  

• } else if (sas > req) {
  

• 	layout->oobfree[idx2].offset = i * sas;
  

• 	layout->oobfree[idx2].length = sas - req;
  

• 	idx2++;
  

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

  }

• /* Leave zero-terminated entry for OOBFREE */
  

• if (idx1 >= MTD_MAX_ECCPOS_ENTRIES_LARGE ||
  

•     idx2 >= MTD_MAX_OOBFREE_ENTRIES_LARGE - 1)
  

• 	break;
  

  }

• return layout;

• return 0; }

-static struct nand_ecclayout *brcmstb_choose_ecc_layout(

• struct brcmnand_host *host)
  

+static const struct mtd_ooblayout_ops brcmnand_hamming_ooblayout_ops = {

• .ecc = brcmnand_hamming_ooblayout_ecc,
• .rfree = brcmnand_hamming_ooblayout_free,

+};

+static int brcmnand_bch_ooblayout_ecc(struct mtd_info *mtd, int section,

• 		      struct mtd_oob_region *oobregion)
  

+{

• struct nand_chip *chip = mtd_to_nand(mtd);
• struct brcmnand_host *host = nand_get_controller_data(chip);
• 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);
• if (section >= sectors)

• return -ERANGE;
  

• oobregion->offset = (section * (sas + 1)) - chip->ecc.bytes;
• oobregion->length = chip->ecc.bytes;
• return 0;

+}

+static int brcmnand_bch_ooblayout_free_lp(struct mtd_info *mtd, int section,

• 			  struct mtd_oob_region *oobregion)
  

+{

• struct nand_chip *chip = mtd_to_nand(mtd);
• struct brcmnand_host *host = nand_get_controller_data(chip);
• 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);
• if (section >= sectors)

• return -ERANGE;
  

• if (sas <= chip->ecc.bytes)

• return 0;
  

• oobregion->offset = section * sas;
• oobregion->length = sas - chip->ecc.bytes;
• if (!section) {

• oobregion->offset++;
  

• oobregion->length--;
  

• }
• return 0;

+}

+static int brcmnand_bch_ooblayout_free_sp(struct mtd_info *mtd, int section,

• 			  struct mtd_oob_region *oobregion)
  

+{

• struct nand_chip *chip = mtd_to_nand(mtd);
• struct brcmnand_host *host = nand_get_controller_data(chip);
• struct brcmnand_cfg *cfg = &host->hwcfg;
• int sas = cfg->spare_area_size << cfg->sector_size_1k;
• if (section > 1 || sas - chip->ecc.bytes < 6 ||

•    (section && sas - chip->ecc.bytes == 6))
  

• return -ERANGE;
  

• if (!section) {

• oobregion->offset = 0;
  

• oobregion->length = 5;
  

• } else {

• oobregion->offset = 6;
  

• oobregion->length = sas - chip->ecc.bytes - 6;
  

• }
• return 0;

+}

+static const struct mtd_ooblayout_ops brcmnand_bch_lp_ooblayout_ops = {

• .ecc = brcmnand_bch_ooblayout_ecc,
• .rfree = brcmnand_bch_ooblayout_free_lp,

+};

+static const struct mtd_ooblayout_ops brcmnand_bch_sp_ooblayout_ops = {

• .ecc = brcmnand_bch_ooblayout_ecc,
• .rfree = brcmnand_bch_ooblayout_free_sp,

+};

+static int brcmstb_choose_ecc_layout(struct brcmnand_host *host) {

• struct nand_ecclayout *layout; struct brcmnand_cfg *p = &host->hwcfg;

• struct mtd_info *mtd = nand_to_mtd(&host->chip);

• struct nand_ecc_ctrl *ecc = &host->chip.ecc; unsigned int ecc_level = p->ecc_level;

• int sas = p->spare_area_size << p->sector_size_1k;

• int sectors = p->page_size / (512 << p->sector_size_1k);

  if (p->sector_size_1k) ecc_level <<= 1;

• layout = brcmnand_create_layout(ecc_level, host);
• if (!layout) {

• if (is_hamming_ecc(host->ctrl, p)) {

• ecc->bytes = 3 * sectors;
  

• mtd_set_ooblayout(mtd, &brcmnand_hamming_ooblayout_ops);
  

• return 0;
  

• }
• /*

• * CONTROLLER_VERSION:
  

• *   < v5.0: ECC_REQ = ceil(BCH_T * 13/8)
  

• *  >= v5.0: ECC_REQ = ceil(BCH_T * 14/8)
  

• * But we will just be conservative.
  

• */
  

• ecc->bytes = DIV_ROUND_UP(ecc_level * 14, 8);
• if (p->page_size == 512)

• mtd_set_ooblayout(mtd, &brcmnand_bch_sp_ooblayout_ops);
  

• else

• mtd_set_ooblayout(mtd, &brcmnand_bch_lp_ooblayout_ops);
  

• if (ecc->bytes >= sas) { dev_err(host->pdev,

• 	"no proper ecc_layout for this NAND cfg\n");
  

• return NULL;
  

• 	"error: ECC too large for OOB (ECC bytes %d, spare sector %d)\n",
  

• 	ecc->bytes, sas);
  

• return -EINVAL;
  

  }

• return layout;

• return 0; }

  static void brcmnand_wp(struct mtd_info *mtd, int wp)

@@ -2329,9 +2381,9 @@ static int brcmnand_init_cs(struct brcmnand_host *host, ofnode dn) /* only use our internal HW threshold */ mtd->bitflip_threshold = 1;

• chip->ecc.layout = brcmstb_choose_ecc_layout(host);
• if (!chip->ecc.layout)

• return -ENXIO;
  

• ret = brcmstb_choose_ecc_layout(host);

• if (ret)

• return ret;
  
  

  ret = nand_scan_tail(mtd); if (ret)