[PATCH 1/2] nand: imx6: Changed default NAND clock

[PATCH 1/2] nand: imx6: Changed default NAND clock

[Christian Hemp]

From: Daniel Schultz <d.schultz@phytec.de>

The Barebox recognized false bad erase blocks while booting from a
Spansion NAND (1). This error occurred due a to high clock. The
Kernel sets the default NAND clock to 22Mhz. So, to fix this error and
to be more identical with the Kernel, the Barebox should be too.

1: nand: NAND device: Manufacturer ID: 0x01, Chip ID: 0xd3 (AMD/Spansion
S34ML08G2), 1024MiB, page size: 2048, OOB size: 128

Signed-off-by: Daniel Schultz <d.schultz@phytec.de>
Tested-by: Stefan Lengfeld <s.lengfeld@phytec.de>
Signed-off-by: Christian Hemp <c.hemp@phytec.de>
---
 drivers/mtd/nand/nand_mxs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_mxs.c b/drivers/mtd/nand/nand_mxs.c
index 01aa063..fe955e8 100644
--- a/drivers/mtd/nand/nand_mxs.c
+++ b/drivers/mtd/nand/nand_mxs.c
@@ -2145,7 +2145,7 @@ static int mxs_nand_probe(struct device_d *dev)
 
 	if (mxs_nand_is_imx6(nand_info)) {
 		clk_disable(nand_info->clk);
-		clk_set_rate(nand_info->clk, 96000000);
+		clk_set_rate(nand_info->clk, 22000000);
 		clk_enable(nand_info->clk);
 		nand_info->dma_channel_base = 0;
 	} else {
-- 
1.9.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] mtd: nand: nand_mxs: Fix readtotal calculation

[Christian Hemp]

The calculation of readtotal must be bit alligend. If not the bch core
finds bit flips in every page, because readtotal is too small.
This bug was mostly introduced since commit "51061a9 mtd: nand: nand_mxs:
Add subpage read support".

Tested with:
nand: NAND device: Manufacturer ID: 0x01, Chip ID: 0xd3 (AMD/Spansion
S34ML08G2), 1024MiB, page size: 2048, OOB size: 128

nand: NAND device: Manufacturer ID: 0x2c, Chip ID: 0xdc (Micron
MT29F4G08ABADAWP), 512MiB, page size: 2048, OOB size: 64

nand: NAND device: Manufacturer ID: 0xec, Chip ID: 0xd3 (Samsung NAND
1GiB 3,3V 8-bit), 1024MiB, page size: 2048, OOB size: 64

Signed-off-by: Christian Hemp <c.hemp@phytec.de>
Signed-off-by: Stefan Lengfeld <s.lengfeld@phytec.de>
---
 drivers/mtd/nand/nand_mxs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/nand_mxs.c b/drivers/mtd/nand/nand_mxs.c
index fe955e8..cba0bee 100644
--- a/drivers/mtd/nand/nand_mxs.c
+++ b/drivers/mtd/nand/nand_mxs.c
@@ -728,15 +728,15 @@ static int __mxs_nand_ecc_read_page(struct mtd_info *mtd, struct nand_chip *nand
 	uint32_t corrected = 0, failed = 0;
 	uint8_t	*status;
 	unsigned int  max_bitflips = 0;
-	int i, ret, readtotal, nchunks, eccstrength, ecc_parity_size;
+	int i, ret, readtotal, nchunks, eccstrength;
 
 	eccstrength = mxs_nand_get_ecc_strength(mtd->writesize, mtd->oobsize);
 
 	readlen = roundup(readlen, MXS_NAND_CHUNK_DATA_CHUNK_SIZE);
 	nchunks = mxs_nand_ecc_chunk_cnt(readlen);
-	ecc_parity_size = 13 * eccstrength / 8;
-	readtotal = MXS_NAND_METADATA_SIZE +
-		(MXS_NAND_CHUNK_DATA_CHUNK_SIZE + ecc_parity_size) * nchunks;
+	readtotal =  MXS_NAND_METADATA_SIZE;
+	readtotal += MXS_NAND_CHUNK_DATA_CHUNK_SIZE * nchunks;
+	readtotal += DIV_ROUND_UP(13 * eccstrength * nchunks, 8);
 
 	mxs_nand_config_bch(mtd, readtotal);
 
-- 
1.9.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] mtd: nand: nand_mxs: Fix readtotal calculation

[Sascha Hauer]

On Thu, Oct 20, 2016 at 03:40:07PM +0200, Christian Hemp wrote:
> The calculation of readtotal must be bit alligend. If not the bch core
> finds bit flips in every page, because readtotal is too small.
> This bug was mostly introduced since commit "51061a9 mtd: nand: nand_mxs:
> Add subpage read support".

Is this somehow related to:

http://lists.infradead.org/pipermail/linux-mtd/2016-June/068243.html

Or is this another topic?

Sascha

> 
> Tested with:
> nand: NAND device: Manufacturer ID: 0x01, Chip ID: 0xd3 (AMD/Spansion
> S34ML08G2), 1024MiB, page size: 2048, OOB size: 128
> 
> nand: NAND device: Manufacturer ID: 0x2c, Chip ID: 0xdc (Micron
> MT29F4G08ABADAWP), 512MiB, page size: 2048, OOB size: 64
> 
> nand: NAND device: Manufacturer ID: 0xec, Chip ID: 0xd3 (Samsung NAND
> 1GiB 3,3V 8-bit), 1024MiB, page size: 2048, OOB size: 64
> 
> Signed-off-by: Christian Hemp <c.hemp@phytec.de>
> Signed-off-by: Stefan Lengfeld <s.lengfeld@phytec.de>
> ---
>  drivers/mtd/nand/nand_mxs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_mxs.c b/drivers/mtd/nand/nand_mxs.c
> index fe955e8..cba0bee 100644
> --- a/drivers/mtd/nand/nand_mxs.c
> +++ b/drivers/mtd/nand/nand_mxs.c
> @@ -728,15 +728,15 @@ static int __mxs_nand_ecc_read_page(struct mtd_info *mtd, struct nand_chip *nand
>  	uint32_t corrected = 0, failed = 0;
>  	uint8_t	*status;
>  	unsigned int  max_bitflips = 0;
> -	int i, ret, readtotal, nchunks, eccstrength, ecc_parity_size;
> +	int i, ret, readtotal, nchunks, eccstrength;
>  
>  	eccstrength = mxs_nand_get_ecc_strength(mtd->writesize, mtd->oobsize);
>  
>  	readlen = roundup(readlen, MXS_NAND_CHUNK_DATA_CHUNK_SIZE);
>  	nchunks = mxs_nand_ecc_chunk_cnt(readlen);
> -	ecc_parity_size = 13 * eccstrength / 8;
> -	readtotal = MXS_NAND_METADATA_SIZE +
> -		(MXS_NAND_CHUNK_DATA_CHUNK_SIZE + ecc_parity_size) * nchunks;
> +	readtotal =  MXS_NAND_METADATA_SIZE;
> +	readtotal += MXS_NAND_CHUNK_DATA_CHUNK_SIZE * nchunks;
> +	readtotal += DIV_ROUND_UP(13 * eccstrength * nchunks, 8);
>  
>  	mxs_nand_config_bch(mtd, readtotal);
>  
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/2] mtd: nand: nand_mxs: Fix readtotal calculation

[Stefan Lengfeld]

Hi Sascha,

On Fri, Oct 21, 2016 at 09:18:08AM +0200, Sascha Hauer wrote:
> On Thu, Oct 20, 2016 at 03:40:07PM +0200, Christian Hemp wrote:
> > The calculation of readtotal must be bit alligend. If not the bch core
> > finds bit flips in every page, because readtotal is too small.
> > This bug was mostly introduced since commit "51061a9 mtd: nand: nand_mxs:
> > Add subpage read support".
> 
> Is this somehow related to:
> 
> http://lists.infradead.org/pipermail/linux-mtd/2016-June/068243.html
> 
> Or is this another topic?
> 
> Sascha

Yes, this patch fixes the same wrong assumption as in the patch above.  In the
above patch on the kernel list the assumption is that every block of
chunk+eccbits is byte aligned on the flash. So if the eccbits are not divisible
by 8, there are unused bits in the last byte of every block of chunk+eccbits.
That's not the case. In fact the second, third,... chunk will not be byte
aligned on the flash anymore. The blocks are bit shifted.

    AFAIR, the GPMI/ECC engine operates at the bit level (which is a pain
    to deal with BTW), and is only requiring a byte alignment on the total
    number of ECC bits. So here, DIV_ROUND_UP(18 * 13 * 4, 8) = 117, which
    fits in the 118 bytes (128 bytes - 10 bytes of 'metadata').
    [Source: http://lists.infradead.org/pipermail/linux-mtd/2016-June/068244.html]


In the patch below the line

> > -	ecc_parity_size = 13 * eccstrength / 8;

is the culprit. If the eccbits per chunk are not divisible by 8, a rounding
error occurs. Since C integer division truncates the fractional part, the byte
size in 'ecc_parity_size' underestimates the real size of the eccbits on the
flash.

The correct solution is to calculate all sizes in bits and then round up to the
next byte. So the code only overestimates the required read size.

I can only agree to Boris statement:

    AFAIR, the GPMI/ECC engine operates at the bit level (which is a pain
    to deal with BTW),

Dealing with bit aligned stuff is really awful.

Mit freundlichen Grüßen / Kind regards,
	Stefan Lengfeld

> 
> > 
> > Tested with:
> > nand: NAND device: Manufacturer ID: 0x01, Chip ID: 0xd3 (AMD/Spansion
> > S34ML08G2), 1024MiB, page size: 2048, OOB size: 128
> > 
> > nand: NAND device: Manufacturer ID: 0x2c, Chip ID: 0xdc (Micron
> > MT29F4G08ABADAWP), 512MiB, page size: 2048, OOB size: 64
> > 
> > nand: NAND device: Manufacturer ID: 0xec, Chip ID: 0xd3 (Samsung NAND
> > 1GiB 3,3V 8-bit), 1024MiB, page size: 2048, OOB size: 64
> > 
> > Signed-off-by: Christian Hemp <c.hemp@phytec.de>
> > Signed-off-by: Stefan Lengfeld <s.lengfeld@phytec.de>
> > ---
> >  drivers/mtd/nand/nand_mxs.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_mxs.c b/drivers/mtd/nand/nand_mxs.c
> > index fe955e8..cba0bee 100644
> > --- a/drivers/mtd/nand/nand_mxs.c
> > +++ b/drivers/mtd/nand/nand_mxs.c
> > @@ -728,15 +728,15 @@ static int __mxs_nand_ecc_read_page(struct mtd_info *mtd, struct nand_chip *nand
> >  	uint32_t corrected = 0, failed = 0;
> >  	uint8_t	*status;
> >  	unsigned int  max_bitflips = 0;
> > -	int i, ret, readtotal, nchunks, eccstrength, ecc_parity_size;
> > +	int i, ret, readtotal, nchunks, eccstrength;
> >  
> >  	eccstrength = mxs_nand_get_ecc_strength(mtd->writesize, mtd->oobsize);
> >  
> >  	readlen = roundup(readlen, MXS_NAND_CHUNK_DATA_CHUNK_SIZE);
> >  	nchunks = mxs_nand_ecc_chunk_cnt(readlen);
> > -	ecc_parity_size = 13 * eccstrength / 8;
> > -	readtotal = MXS_NAND_METADATA_SIZE +
> > -		(MXS_NAND_CHUNK_DATA_CHUNK_SIZE + ecc_parity_size) * nchunks;
> > +	readtotal =  MXS_NAND_METADATA_SIZE;
> > +	readtotal += MXS_NAND_CHUNK_DATA_CHUNK_SIZE * nchunks;
> > +	readtotal += DIV_ROUND_UP(13 * eccstrength * nchunks, 8);
> >  
> >  	mxs_nand_config_bch(mtd, readtotal);
> >  
> > -- 
> > 1.9.1
> > 
> > 
> > _______________________________________________
> > barebox mailing list
> > barebox@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/barebox
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] nand: imx6: Changed default NAND clock

[Sascha Hauer]

On Thu, Oct 20, 2016 at 03:40:06PM +0200, Christian Hemp wrote:
> From: Daniel Schultz <d.schultz@phytec.de>
> 
> The Barebox recognized false bad erase blocks while booting from a
> Spansion NAND (1). This error occurred due a to high clock. The
> Kernel sets the default NAND clock to 22Mhz. So, to fix this error and
> to be more identical with the Kernel, the Barebox should be too.

The timing is changed afterwards to a faster mode and the clock is
changed again. Does that work for you properly?

Sascha

> 
> 1: nand: NAND device: Manufacturer ID: 0x01, Chip ID: 0xd3 (AMD/Spansion
> S34ML08G2), 1024MiB, page size: 2048, OOB size: 128
> 
> Signed-off-by: Daniel Schultz <d.schultz@phytec.de>
> Tested-by: Stefan Lengfeld <s.lengfeld@phytec.de>
> Signed-off-by: Christian Hemp <c.hemp@phytec.de>
> ---
>  drivers/mtd/nand/nand_mxs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/nand_mxs.c b/drivers/mtd/nand/nand_mxs.c
> index 01aa063..fe955e8 100644
> --- a/drivers/mtd/nand/nand_mxs.c
> +++ b/drivers/mtd/nand/nand_mxs.c
> @@ -2145,7 +2145,7 @@ static int mxs_nand_probe(struct device_d *dev)
>  
>  	if (mxs_nand_is_imx6(nand_info)) {
>  		clk_disable(nand_info->clk);
> -		clk_set_rate(nand_info->clk, 96000000);
> +		clk_set_rate(nand_info->clk, 22000000);
>  		clk_enable(nand_info->clk);
>  		nand_info->dma_channel_base = 0;
>  	} else {
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] nand: imx6: Changed default NAND clock

[Stefan Lengfeld]

Hi Sascha,

On Fri, Oct 21, 2016 at 09:11:47AM +0200, Sascha Hauer wrote:
> On Thu, Oct 20, 2016 at 03:40:06PM +0200, Christian Hemp wrote:
> > From: Daniel Schultz <d.schultz@phytec.de>
> > 
> > The Barebox recognized false bad erase blocks while booting from a
> > Spansion NAND (1). This error occurred due a to high clock. The
> > Kernel sets the default NAND clock to 22Mhz. So, to fix this error and
> > to be more identical with the Kernel, the Barebox should be too.
> 
> The timing is changed afterwards to a faster mode and the clock is
> changed again. Does that work for you properly?
> 
> Sascha

Yes, if the NAND chip supports EDO mode, the clock is changed to 80/100Mhz
correctly. Here is the driver output with additional debug messages:

    nand: NAND device: Manufacturer ID: 0x2c, Chip ID: 0xdc (Micron MT29F4G08ABADAWP), 512MiB, page size: 2048, OOB size: 64
    mxs_nand_setup_timing: mode 5
    mxs_nand_setup_timing: mxs_nand_compute_edo_timing
    mxs_nand_setup_timing: clock is 96000000

The patch fixes the case, when the NAND chip does not support EDO mode.
Instead of function 'mxs_nand_compute_edo_timing' the function
'mxs_nand_compute_hardware_timing' is called here.

        mode = mxs_nand_enable_edo_mode(info);
        if (mode >= 0)
                mxs_nand_compute_edo_timing(info, &hw, mode);
        else
                mxs_nand_compute_hardware_timing(info, &hw);

Since 'mxs_nand_compute_hardware_timing' does not change the clock, the clock
is too fast for the NAND chip.

Here is the output of two different NAND chips without EDO mode. The patch to
set the clock to 22Mhz was already applied:

    nand: NAND device: Manufacturer ID: 0x01, Chip ID: 0xd3 (AMD/Spansion S34ML08G2), 1024MiB, page size: 2048, OOB size: 128
    mxs_nand_setup_timing: mode -22
    mxs_nand_setup_timing: mxs_nand_compute_hardware_timing
    mxs_nand_setup_timing: clock is 21818181
    
    nand: NAND device: Manufacturer ID: 0xec, Chip ID: 0xd3 (Samsung NAND 1GiB 3,3V 8-bit), 1024MiB, page size: 2048, OOB size: 64
    mxs_nand_setup_timing: mode -2
    mxs_nand_setup_timing: mxs_nand_compute_hardware_timing
    mxs_nand_setup_timing: clock is 21818181

Mit freundlichen Grüßen / Kind regards,
	Stefan Lengfeld

> 
> > 
> > 1: nand: NAND device: Manufacturer ID: 0x01, Chip ID: 0xd3 (AMD/Spansion
> > S34ML08G2), 1024MiB, page size: 2048, OOB size: 128
> > 
> > Signed-off-by: Daniel Schultz <d.schultz@phytec.de>
> > Tested-by: Stefan Lengfeld <s.lengfeld@phytec.de>
> > Signed-off-by: Christian Hemp <c.hemp@phytec.de>
> > ---
> >  drivers/mtd/nand/nand_mxs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_mxs.c b/drivers/mtd/nand/nand_mxs.c
> > index 01aa063..fe955e8 100644
> > --- a/drivers/mtd/nand/nand_mxs.c
> > +++ b/drivers/mtd/nand/nand_mxs.c
> > @@ -2145,7 +2145,7 @@ static int mxs_nand_probe(struct device_d *dev)
> >  
> >  	if (mxs_nand_is_imx6(nand_info)) {
> >  		clk_disable(nand_info->clk);
> > -		clk_set_rate(nand_info->clk, 96000000);
> > +		clk_set_rate(nand_info->clk, 22000000);
> >  		clk_enable(nand_info->clk);
> >  		nand_info->dma_channel_base = 0;
> >  	} else {
> > -- 
> > 1.9.1
> > 
> > 
> > _______________________________________________
> > barebox mailing list
> > barebox@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/barebox
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/2] nand: imx6: Changed default NAND clock

[Sascha Hauer]

On Thu, Oct 20, 2016 at 03:40:06PM +0200, Christian Hemp wrote:
> From: Daniel Schultz <d.schultz@phytec.de>
> 
> The Barebox recognized false bad erase blocks while booting from a
> Spansion NAND (1). This error occurred due a to high clock. The
> Kernel sets the default NAND clock to 22Mhz. So, to fix this error and
> to be more identical with the Kernel, the Barebox should be too.
> 
> 1: nand: NAND device: Manufacturer ID: 0x01, Chip ID: 0xd3 (AMD/Spansion
> S34ML08G2), 1024MiB, page size: 2048, OOB size: 128

Applied both, thanks

Sascha

> 
> Signed-off-by: Daniel Schultz <d.schultz@phytec.de>
> Tested-by: Stefan Lengfeld <s.lengfeld@phytec.de>
> Signed-off-by: Christian Hemp <c.hemp@phytec.de>
> ---
>  drivers/mtd/nand/nand_mxs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/nand_mxs.c b/drivers/mtd/nand/nand_mxs.c
> index 01aa063..fe955e8 100644
> --- a/drivers/mtd/nand/nand_mxs.c
> +++ b/drivers/mtd/nand/nand_mxs.c
> @@ -2145,7 +2145,7 @@ static int mxs_nand_probe(struct device_d *dev)
>  
>  	if (mxs_nand_is_imx6(nand_info)) {
>  		clk_disable(nand_info->clk);
> -		clk_set_rate(nand_info->clk, 96000000);
> +		clk_set_rate(nand_info->clk, 22000000);
>  		clk_enable(nand_info->clk);
>  		nand_info->dma_channel_base = 0;
>  	} else {
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox