diff mbox

[v5,39/50] mtd: nand: omap2: switch to mtd_ooblayout_ops

Message ID 57160862.90603@ti.com
State Superseded
Headers show

Commit Message

Roger Quadros April 19, 2016, 10:28 a.m. UTC 
On 18/04/16 18:05, Boris Brezillon wrote:
> On Mon, 18 Apr 2016 17:32:49 +0300

> Roger Quadros <rogerq@ti.com> wrote:

> 

>> Boris,

>>

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

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

>>> ---

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

> 

> [...]

> 

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

> 

>>

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


Shouldn't this have been
		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",

> 


After the above changes BCH with HW ECC worked fine but BCH with SW ECC still failed.
I had to fix it up with the below patch. This is mainly because chip->ecc.steps wasn't
yet initialized before calling nand_bch_init().

After the below patch it worked fine with bch4 (hw & sw), bch8 (hw & sw) and ham1.
I couldn't yet verify bch16 though.

--cheers,
-roger

-- 
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index db22c01..1b1a804 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1692,7 +1692,7 @@  static int omap_sw_ooblayout_ecc(struct mtd_info *mtd, int section,
 	struct nand_chip *chip = mtd_to_nand(mtd);
 	int off = BADBLOCK_MARKER_LENGTH;
 
-	if (section >= chip->ecc.steps)
+	if (section >= mtd->writesize / chip->ecc.size)	/* ecc steps */
 		return -ERANGE;
 
 	/*
@@ -1711,7 +1711,7 @@  static int omap_sw_ooblayout_free(struct mtd_info *mtd, int section,
 	struct nand_chip *chip = mtd_to_nand(mtd);
 	int off = BADBLOCK_MARKER_LENGTH;
 
-	if (section)
+	if (section >= mtd->writesize / chip->ecc.size) /* ecc steps */
 		return -ERANGE;
 
 	/*