[v3,4/4] mtd: mxc_nand: generate nand_ecclayout for 8 bit ECC

Hardware 8 bit ECC requires a different nand_ecclayout. Instead of adding yet
another static struct nand_ecclayout, generate it in code.

Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
v3:
   Rename ecc_8bit_layout to ecc_8bit_layout_4k (Uwe Kleine-König)

v2:
   Initialize eccbytes
---
 drivers/mtd/nand/mxc_nand.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

On Wed, May 13, 2015 at 11:17:39AM +0300, Baruch Siach wrote:
> Hardware 8 bit ECC requires a different nand_ecclayout. Instead of adding yet
> another static struct nand_ecclayout, generate it in code.
> 
> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

On Wed, May 13, 2015 at 11:17:39AM +0300, Baruch Siach wrote:
> Hardware 8 bit ECC requires a different nand_ecclayout. Instead of adding yet
> another static struct nand_ecclayout, generate it in code.

I like the idea of not adding more static declarations. But I think you
have a potential problem below.

> Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
> v3:
>    Rename ecc_8bit_layout to ecc_8bit_layout_4k (Uwe Kleine-König)
> 
> v2:
>    Initialize eccbytes
> ---
>  drivers/mtd/nand/mxc_nand.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index 010be8aa41d4..25759563bf11 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -960,6 +960,23 @@ static int get_eccsize(struct mtd_info *mtd)
>  		return 8;
>  }
>  
> +static void ecc_8bit_layout_4k(struct nand_ecclayout *layout)
> +{
> +	int i, j;
> +
> +	layout->eccbytes = 8*18;
> +	for (i = 0; i < 8; i++)
> +		for (j = 0; j < 18; j++)
> +			layout->eccpos[i*18 + j] = i*26 + j + 7;
> +
> +	layout->oobfree[0].offset = 2;
> +	layout->oobfree[0].length = 4;
> +	for (i = 1; i < 8; i++) {
> +		layout->oobfree[i].offset = i*26;
> +		layout->oobfree[i].length = 7;
> +	}
> +}
> +
>  static void preset_v1(struct mtd_info *mtd)
>  {
>  	struct nand_chip *nand_chip = mtd->priv;
> @@ -1636,8 +1653,11 @@ static int mxcnd_probe(struct platform_device *pdev)
>  
>  	if (mtd->writesize == 2048)
>  		this->ecc.layout = host->devtype_data->ecclayout_2k;
> -	else if (mtd->writesize == 4096)
> +	else if (mtd->writesize == 4096) {
>  		this->ecc.layout = host->devtype_data->ecclayout_4k;
> +		if (get_eccsize(mtd) == 8)
> +			ecc_8bit_layout_4k(this->ecc.layout);

So you're overwriting an existing layout (e.g., nandv2_hw_eccoob_4k).
What if you have more than one NAND chip? You might do better by
dynamically allocating the memory.

> +	}
>  
>  	/*
>  	 * Experimentation shows that i.MX NFC can only handle up to 218 oob

Brian

Hi Brian,

On Wed, May 20, 2015 at 03:41:20PM -0700, Brian Norris wrote:
> On Wed, May 13, 2015 at 11:17:39AM +0300, Baruch Siach wrote:
> > +		if (get_eccsize(mtd) == 8)
> > +			ecc_8bit_layout_4k(this->ecc.layout);
> 
> So you're overwriting an existing layout (e.g., nandv2_hw_eccoob_4k).
> What if you have more than one NAND chip? You might do better by
> dynamically allocating the memory.

It would take a quite a bit more code changes then that to have the mxc_nand 
driver support more than one NAND chip, not to mention the DT binding. As Uwe 
has indicated on a previous version of this series, ecclayout handling in this 
driver could use some cleanup. This patch just fixes bug, trying to break 
anything else while doing so.

Thanks for reviewing, and for applying the rest of this series.

baruch

On Thu, May 21, 2015 at 07:11:28AM +0300, Baruch Siach wrote:
> On Wed, May 20, 2015 at 03:41:20PM -0700, Brian Norris wrote:
> > On Wed, May 13, 2015 at 11:17:39AM +0300, Baruch Siach wrote:
> > > +		if (get_eccsize(mtd) == 8)
> > > +			ecc_8bit_layout_4k(this->ecc.layout);
> > 
> > So you're overwriting an existing layout (e.g., nandv2_hw_eccoob_4k).
> > What if you have more than one NAND chip? You might do better by
> > dynamically allocating the memory.
> 
> It would take a quite a bit more code changes then that to have the mxc_nand 
> driver support more than one NAND chip, not to mention the DT binding. As Uwe 

Right. I guess there's also the case that you have more than one
instance of this controller / driver. But I assume that's pretty
unlikely?

> has indicated on a previous version of this series, ecclayout handling in this 
> driver could use some cleanup. This patch just fixes bug, trying to break 
> anything else while doing so.

Yeah, OK. Then I'll apply this patch anyway, and the rest could be
worked out later if this driver ever supports more cases.

> Thanks for reviewing, and for applying the rest of this series.

Applied, thanks.

Brian