diff mbox series

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

Message ID 20230115195312.1477845-2-linus.walleij@linaro.org
State New
Headers show
Series Backport BRCMNAND changes from Linux | expand

Commit Message

Linus Walleij Jan. 15, 2023, 7:52 p.m. UTC
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(-)

Comments

William Zhang Jan. 26, 2023, 1:02 a.m. UTC | #1
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)
>
Linus Walleij Jan. 26, 2023, 8:43 a.m. UTC | #2
On Thu, Jan 26, 2023 at 2:02 AM William Zhang
<william.zhang@broadcom.com> wrote:

> 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")

Argh yeah I see. Let's hold this series off then.
It was worth a try!

> 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.

No I can't do that I do not have a big enough experience with NAND
flash and no testbed for it either.

> 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.

OK no they seemed to be mostly improvements of the algorithm so we
can certainly live without.

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

Actually not, I think, it's one of the other patches I sent, the one enabling
BCH-1 by reading the proper ECC properties from the device tree.
That made it finally work.

The iproc NAND driver I sent should also work pretty much as-is, nothing
depends on these backports.

Yours,
Linus Walleij
William Zhang Jan. 26, 2023, 5:39 p.m. UTC | #3
On 01/26/2023 12:43 AM, Linus Walleij wrote:
> On Thu, Jan 26, 2023 at 2:02 AM William Zhang
> <william.zhang@broadcom.com> wrote:
> 
>> 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")
> 
> Argh yeah I see. Let's hold this series off then.
> It was worth a try!
> 
>> 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.
> 
> No I can't do that I do not have a big enough experience with NAND
> flash and no testbed for it either.
> 
>> 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.
> 
> OK no they seemed to be mostly improvements of the algorithm so we
> can certainly live without.
> 
>> Was the bug you were hunting down in the code related to this patch?
> 
> Actually not, I think, it's one of the other patches I sent, the one enabling
> BCH-1 by reading the proper ECC properties from the device tree.
> That made it finally work.
 >
> The iproc NAND driver I sent should also work pretty much as-is, nothing
> depends on these backports.
>
Yes the patches that are not relate to the ooblayout should still be 
good to have.

> Yours,
> Linus Walleij
>
Michael Nazzareno Trimarchi Feb. 3, 2023, 8:48 a.m. UTC | #4
Hi William

On Thu, Jan 26, 2023 at 6:39 PM William Zhang
<william.zhang@broadcom.com> wrote:
>
>
>
> On 01/26/2023 12:43 AM, Linus Walleij wrote:
> > On Thu, Jan 26, 2023 at 2:02 AM William Zhang
> > <william.zhang@broadcom.com> wrote:
> >

Can you add your review-by?

Michael

> >> 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")
> >
> > Argh yeah I see. Let's hold this series off then.
> > It was worth a try!
> >
> >> 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.
> >
> > No I can't do that I do not have a big enough experience with NAND
> > flash and no testbed for it either.
> >
> >> 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.
> >
> > OK no they seemed to be mostly improvements of the algorithm so we
> > can certainly live without.
> >
> >> Was the bug you were hunting down in the code related to this patch?
> >
> > Actually not, I think, it's one of the other patches I sent, the one enabling
> > BCH-1 by reading the proper ECC properties from the device tree.
> > That made it finally work.
>  >
> > The iproc NAND driver I sent should also work pretty much as-is, nothing
> > depends on these backports.
> >
> Yes the patches that are not relate to the ooblayout should still be
> good to have.
>
> > Yours,
> > Linus Walleij
> >
Linus Walleij Feb. 3, 2023, 11:10 a.m. UTC | #5
On Fri, Feb 3, 2023 at 9:48 AM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:
> On Thu, Jan 26, 2023 at 6:39 PM William Zhang
> <william.zhang@broadcom.com> wrote:
> >
> >
> >
> > On 01/26/2023 12:43 AM, Linus Walleij wrote:
> > > On Thu, Jan 26, 2023 at 2:02 AM William Zhang
> > > <william.zhang@broadcom.com> wrote:
> > >
>
> Can you add your review-by?

I think maybe I need to rebase the series and take out all the changes
that does not relate to patch 1 that changes the way we handle the
OOB layout so William can test the result?

Yours,
Linus Walleij
William Zhang Feb. 3, 2023, 5:23 p.m. UTC | #6
Hi Linus and Michael,

On 02/03/2023 03:10 AM, Linus Walleij wrote:
> On Fri, Feb 3, 2023 at 9:48 AM Michael Nazzareno Trimarchi
> <michael@amarulasolutions.com> wrote:
>> On Thu, Jan 26, 2023 at 6:39 PM William Zhang
>> <william.zhang@broadcom.com> wrote:
>>>
>>>
>>>
>>> On 01/26/2023 12:43 AM, Linus Walleij wrote:
>>>> On Thu, Jan 26, 2023 at 2:02 AM William Zhang
>>>> <william.zhang@broadcom.com> wrote:
>>>>
>>
>> Can you add your review-by?
> 
> I think maybe I need to rebase the series and take out all the changes
> that does not relate to patch 1 that changes the way we handle the
> OOB layout so William can test the result?
> 
Yeah I think that is a good idea and I can add my review-by and test-by 
tag if everything is good.

> Yours,
> Linus Walleij
>
Michael Nazzareno Trimarchi Feb. 11, 2023, 9:45 a.m. UTC | #7
Hi Linus

On Fri, Feb 3, 2023 at 6:23 PM William Zhang <william.zhang@broadcom.com> wrote:
>
> Hi Linus and Michael,
>
> On 02/03/2023 03:10 AM, Linus Walleij wrote:
> > On Fri, Feb 3, 2023 at 9:48 AM Michael Nazzareno Trimarchi
> > <michael@amarulasolutions.com> wrote:
> >> On Thu, Jan 26, 2023 at 6:39 PM William Zhang
> >> <william.zhang@broadcom.com> wrote:
> >>>
> >>>
> >>>
> >>> On 01/26/2023 12:43 AM, Linus Walleij wrote:
> >>>> On Thu, Jan 26, 2023 at 2:02 AM William Zhang
> >>>> <william.zhang@broadcom.com> wrote:
> >>>>
> >>
> >> Can you add your review-by?
> >
> > I think maybe I need to rebase the series and take out all the changes
> > that does not relate to patch 1 that changes the way we handle the
> > OOB layout so William can test the result?
> >
> Yeah I think that is a good idea and I can add my review-by and test-by
> tag if everything is good.
>
> > Yours,
> > Linus Walleij
> >

Can you repost your patchset?

Thanks
Michael
diff mbox series

Patch

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)