[PATCH] ARM: OMAP2+: Fix onenand initialization to avoid filesystem corruption

[PATCH] ARM: OMAP2+: Fix onenand initialization to avoid filesystem corruption

[Ivaylo Dimitrov]

Commit <e7b11dc7b77bfce0a351230a5feeadc1d0bba997> ("ARM: OMAP2+: Fix
onenand rate detection to avoid filesystem corruption") partially fixed
onenand configuration when GPMC module is reset. Finish the job by also
providing the correct values in ONENAND_REG_SYS_CFG1 register.

Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
---
 arch/arm/mach-omap2/gpmc-onenand.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
index 7b76ce0..8633c70 100644
--- a/arch/arm/mach-omap2/gpmc-onenand.c
+++ b/arch/arm/mach-omap2/gpmc-onenand.c
@@ -101,10 +101,8 @@ static void omap2_onenand_set_async_mode(void __iomem *onenand_base)
 
 static void set_onenand_cfg(void __iomem *onenand_base)
 {
-	u32 reg;
+	u32 reg = ONENAND_SYS_CFG1_RDY | ONENAND_SYS_CFG1_INT;
 
-	reg = readw(onenand_base + ONENAND_REG_SYS_CFG1);
-	reg &= ~((0x7 << ONENAND_SYS_CFG1_BRL_SHIFT) | (0x7 << 9));
 	reg |=	(latency << ONENAND_SYS_CFG1_BRL_SHIFT) |
 		ONENAND_SYS_CFG1_BL_16;
 	if (onenand_flags & ONENAND_FLAG_SYNCREAD)
@@ -123,6 +121,7 @@ static void set_onenand_cfg(void __iomem *onenand_base)
 		reg |= ONENAND_SYS_CFG1_VHF;
 	else
 		reg &= ~ONENAND_SYS_CFG1_VHF;
+
 	writew(reg, onenand_base + ONENAND_REG_SYS_CFG1);
 }
 
@@ -289,6 +288,7 @@ static int omap2_onenand_setup_async(void __iomem *onenand_base)
 		}
 	}
 
+	onenand_async.sync_write = true;
 	omap2_onenand_calc_async_timings(&t);
 
 	ret = gpmc_cs_program_settings(gpmc_onenand_data->cs, &onenand_async);
-- 
1.9.1

[PATCH] ARM: OMAP2+: Fix onenand initialization to avoid filesystem corruption

[Tony Lindgren]

* Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> [160205 06:38]:
> Commit <e7b11dc7b77bfce0a351230a5feeadc1d0bba997> ("ARM: OMAP2+: Fix
> onenand rate detection to avoid filesystem corruption") partially fixed
> onenand configuration when GPMC module is reset. Finish the job by also
> providing the correct values in ONENAND_REG_SYS_CFG1 register.

OK. So probably the INT or RDY polarity made the ECC not work.

Aaro, care to dump out also the nolo configured CFG1 value from
n8x0 and n9(50)?

You can do it by adding something like this to the beginning
of set_onenand_cfg():

pr_info("CFG1: 0x%04x\n", readw(onenand_base + ONENAND_REG_SYS_CFG1));

And presumably the values are the same as on n900. If not, we
should do the legacy file system flash test on all of them
before committing this fix.

Regards,

Tony

> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> ---
>  arch/arm/mach-omap2/gpmc-onenand.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
> index 7b76ce0..8633c70 100644
> --- a/arch/arm/mach-omap2/gpmc-onenand.c
> +++ b/arch/arm/mach-omap2/gpmc-onenand.c
> @@ -101,10 +101,8 @@ static void omap2_onenand_set_async_mode(void __iomem *onenand_base)
>  
>  static void set_onenand_cfg(void __iomem *onenand_base)
>  {
> -	u32 reg;
> +	u32 reg = ONENAND_SYS_CFG1_RDY | ONENAND_SYS_CFG1_INT;
>  
> -	reg = readw(onenand_base + ONENAND_REG_SYS_CFG1);
> -	reg &= ~((0x7 << ONENAND_SYS_CFG1_BRL_SHIFT) | (0x7 << 9));
>  	reg |=	(latency << ONENAND_SYS_CFG1_BRL_SHIFT) |
>  		ONENAND_SYS_CFG1_BL_16;
>  	if (onenand_flags & ONENAND_FLAG_SYNCREAD)
> @@ -123,6 +121,7 @@ static void set_onenand_cfg(void __iomem *onenand_base)
>  		reg |= ONENAND_SYS_CFG1_VHF;
>  	else
>  		reg &= ~ONENAND_SYS_CFG1_VHF;
> +
>  	writew(reg, onenand_base + ONENAND_REG_SYS_CFG1);
>  }
>  
> @@ -289,6 +288,7 @@ static int omap2_onenand_setup_async(void __iomem *onenand_base)
>  		}
>  	}
>  
> +	onenand_async.sync_write = true;
>  	omap2_onenand_calc_async_timings(&t);
>  
>  	ret = gpmc_cs_program_settings(gpmc_onenand_data->cs, &onenand_async);
> -- 
> 1.9.1
> 

[PATCH] ARM: OMAP2+: Fix onenand initialization to avoid filesystem corruption

[Ivaylo Dimitrov]

Hi

On  8.02.2016 21:36, Tony Lindgren wrote:
>
> OK. So probably the INT or RDY polarity made the ECC not work.
>

Well, ECC disable bit (ONENAND_SYS_CFG1_NO_ECC) was set as well, so most 
probably it was the bugger :)

> Aaro, care to dump out also the nolo configured CFG1 value from
> n8x0 and n9(50)?
>
> You can do it by adding something like this to the beginning
> of set_onenand_cfg():
>

Also, do not forget to restore HWMOD_INIT_NO_RESET in gpmc_hwmod in 
question and (maybe) revert e7b11dc7b77bfce0a351230a5feeadc1d0bba997.

Regards,
Ivo

[PATCH] ARM: OMAP2+: Fix onenand initialization to avoid filesystem corruption

[Tony Lindgren]

* Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> [160208 12:15]:
> Hi
> 
> On  8.02.2016 21:36, Tony Lindgren wrote:
> >
> >OK. So probably the INT or RDY polarity made the ECC not work.
> >
> 
> Well, ECC disable bit (ONENAND_SYS_CFG1_NO_ECC) was set as well, so most
> probably it was the bugger :)

Oh OK that explains.

> >Aaro, care to dump out also the nolo configured CFG1 value from
> >n8x0 and n9(50)?
> >
> >You can do it by adding something like this to the beginning
> >of set_onenand_cfg():
> >
> 
> Also, do not forget to restore HWMOD_INIT_NO_RESET in gpmc_hwmod in question
> and (maybe) revert e7b11dc7b77bfce0a351230a5feeadc1d0bba997.

Probably just enabling CONFIG_OMAP_GPMC_DEBUG allows dumping
the GPMC configuration. And it sounds like it's a good idea
to have this patch enabled.

Regards,

Tony

[PATCH] ARM: OMAP2+: Fix onenand initialization to avoid filesystem corruption

[Aaro Koskinen]

Hi,

On Mon, Feb 08, 2016 at 11:36:45AM -0800, Tony Lindgren wrote:
> * Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> [160205 06:38]:
> > Commit <e7b11dc7b77bfce0a351230a5feeadc1d0bba997> ("ARM: OMAP2+: Fix
> > onenand rate detection to avoid filesystem corruption") partially fixed
> > onenand configuration when GPMC module is reset. Finish the job by also
> > providing the correct values in ONENAND_REG_SYS_CFG1 register.
> 
> OK. So probably the INT or RDY polarity made the ECC not work.
> 
> Aaro, care to dump out also the nolo configured CFG1 value from
> n8x0 and n9(50)?
> 
> You can do it by adding something like this to the beginning
> of set_onenand_cfg():
> 
> pr_info("CFG1: 0x%04x\n", readw(onenand_base + ONENAND_REG_SYS_CFG1));
> 
> And presumably the values are the same as on n900. If not, we
> should do the legacy file system flash test on all of them
> before committing this fix.

I got these (with CONFIG_OMAP_GPMC_DEBUG also enabled):

N800:
[    2.803100] CFG1: 0x46c0
[    3.150115] CFG1: 0x06c0

N810:
[    2.802368] CFG1: 0x46c0
[    3.148620] CFG1: 0x06c0

N900:
[    4.965576] CFG1: 0x46c0
[    5.333740] CFG1: 0x06c0

N950:
[    4.217193] CFG1: 0x66c4
[    4.583221] CFG1: 0x06c0

N9:
[    4.212677] CFG1: 0x66c4
[    4.578613] CFG1: 0x06c0

A.

[PATCH] ARM: OMAP2+: Fix onenand initialization to avoid filesystem corruption

[Tony Lindgren]

* Aaro Koskinen <aaro.koskinen@iki.fi> [160210 13:13]:
> Hi,
> 
> On Mon, Feb 08, 2016 at 11:36:45AM -0800, Tony Lindgren wrote:
> > * Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> [160205 06:38]:
> > > Commit <e7b11dc7b77bfce0a351230a5feeadc1d0bba997> ("ARM: OMAP2+: Fix
> > > onenand rate detection to avoid filesystem corruption") partially fixed
> > > onenand configuration when GPMC module is reset. Finish the job by also
> > > providing the correct values in ONENAND_REG_SYS_CFG1 register.
> > 
> > OK. So probably the INT or RDY polarity made the ECC not work.
> > 
> > Aaro, care to dump out also the nolo configured CFG1 value from
> > n8x0 and n9(50)?
> > 
> > You can do it by adding something like this to the beginning
> > of set_onenand_cfg():
> > 
> > pr_info("CFG1: 0x%04x\n", readw(onenand_base + ONENAND_REG_SYS_CFG1));
> > 
> > And presumably the values are the same as on n900. If not, we
> > should do the legacy file system flash test on all of them
> > before committing this fix.
> 
> I got these (with CONFIG_OMAP_GPMC_DEBUG also enabled):
> 
> N800:
> [    2.803100] CFG1: 0x46c0
> [    3.150115] CFG1: 0x06c0
> 
> N810:
> [    2.802368] CFG1: 0x46c0
> [    3.148620] CFG1: 0x06c0
> 
> N900:
> [    4.965576] CFG1: 0x46c0
> [    5.333740] CFG1: 0x06c0

Thanks, so these all should be fine with Ivaylo's patch.

> N950:
> [    4.217193] CFG1: 0x66c4
> [    4.583221] CFG1: 0x06c0
> 
> N9:
> [    4.212677] CFG1: 0x66c4
> [    4.578613] CFG1: 0x06c0

Aaro, care to do one more test on n9(50) like Ivaylo posted
earlier:

1. Flash original maemo rootfs

2. Boot mainline kernel with Ivaylo's patch and mount onenand

3. Boot original maemo kernel and check dmesg there's no
   file corruption

Also.. There's a chance somebody has created a onenand file system
with recent mainline kernels that did the reset and disabled ECC.
So with Ivaylo's patch fixing that, those may not mount properly
any longer. Most likely people just keep their maemo rootfs there
though with the MMC being available.

Regards,

Tony

[PATCH] ARM: OMAP2+: Fix onenand initialization to avoid filesystem corruption

[Ivaylo Dimitrov]

Hi,

On 11.02.2016 02:12, Tony Lindgren wrote:
>
> Also.. There's a chance somebody has created a onenand file system
> with recent mainline kernels that did the reset and disabled ECC.
> So with Ivaylo's patch fixing that, those may not mount properly
> any longer. Most likely people just keep their maemo rootfs there
> though with the MMC being available.

I guess this is possible, but what worries me more is that the longer 
the patch is not pushed, the higher the chance somebody to end-up with 
broken rootfs. Wouldn't it be better to push it, thus preventing that 
happening?

BTW the differences for N9/50 come from ONENAND_SYS_CFG1_HF bit and 
ONENAND_SYS_CFG1_BRL_6 vs ONENAND_SYS_CFG1_BRL_4. Both are changed 
(later in the code) anyway, so I guess it is safe to reset them to 
default values.

Or, maybe the correct fix is to issue RESET command to onenand 
controller after GPMC reset? RESET command is supposed to put all the 
bits to their default values.

Ivo.

[PATCH] ARM: OMAP2+: Fix onenand initialization to avoid filesystem corruption

[Tony Lindgren]

* Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> [160221 10:14]:
> Hi,
> 
> On 11.02.2016 02:12, Tony Lindgren wrote:
> >
> >Also.. There's a chance somebody has created a onenand file system
> >with recent mainline kernels that did the reset and disabled ECC.
> >So with Ivaylo's patch fixing that, those may not mount properly
> >any longer. Most likely people just keep their maemo rootfs there
> >though with the MMC being available.
> 
> I guess this is possible, but what worries me more is that the longer the
> patch is not pushed, the higher the chance somebody to end-up with broken
> rootfs. Wouldn't it be better to push it, thus preventing that happening?

Will apply today into omap-for-v4.5/fixes with Aaro's ack. Note that
Aaro found also two other error causing issues with the onenand
driver, so it seems the onenand driver has been broken for years in
the mainline kernel..

> BTW the differences for N9/50 come from ONENAND_SYS_CFG1_HF bit and
> ONENAND_SYS_CFG1_BRL_6 vs ONENAND_SYS_CFG1_BRL_4. Both are changed (later in
> the code) anyway, so I guess it is safe to reset them to default values.

Yes I think we're better off having things fully configured by the
kernel in the long run.

> Or, maybe the correct fix is to issue RESET command to onenand controller
> after GPMC reset? RESET command is supposed to put all the bits to their
> default values.

Something like that could be added if needed.

Regards,

Tony

[PATCH] ARM: OMAP2+: Fix onenand initialization to avoid filesystem corruption

[Aaro Koskinen]

Hi,

On Fri, Feb 05, 2016 at 04:37:08PM +0200, Ivaylo Dimitrov wrote:
> Commit <e7b11dc7b77bfce0a351230a5feeadc1d0bba997> ("ARM: OMAP2+: Fix
> onenand rate detection to avoid filesystem corruption") partially fixed
> onenand configuration when GPMC module is reset. Finish the job by also
> providing the correct values in ONENAND_REG_SYS_CFG1 register.
> 
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>

I tested this on N950 by creating ubifs file system with stock kernel,
then reading & writing to it with mainline kernel, and then again
verifying the fs is good with the stock kernel. Note that onenand does
not work properly on N950 at the moment unless ONENAND_HAS_CACHE_PROGRAM
is disabled by patching, but that's an issue unrelated to this patch.

Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi>

A.

> ---
>  arch/arm/mach-omap2/gpmc-onenand.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
> index 7b76ce0..8633c70 100644
> --- a/arch/arm/mach-omap2/gpmc-onenand.c
> +++ b/arch/arm/mach-omap2/gpmc-onenand.c
> @@ -101,10 +101,8 @@ static void omap2_onenand_set_async_mode(void __iomem *onenand_base)
>  
>  static void set_onenand_cfg(void __iomem *onenand_base)
>  {
> -	u32 reg;
> +	u32 reg = ONENAND_SYS_CFG1_RDY | ONENAND_SYS_CFG1_INT;
>  
> -	reg = readw(onenand_base + ONENAND_REG_SYS_CFG1);
> -	reg &= ~((0x7 << ONENAND_SYS_CFG1_BRL_SHIFT) | (0x7 << 9));
>  	reg |=	(latency << ONENAND_SYS_CFG1_BRL_SHIFT) |
>  		ONENAND_SYS_CFG1_BL_16;
>  	if (onenand_flags & ONENAND_FLAG_SYNCREAD)
> @@ -123,6 +121,7 @@ static void set_onenand_cfg(void __iomem *onenand_base)
>  		reg |= ONENAND_SYS_CFG1_VHF;
>  	else
>  		reg &= ~ONENAND_SYS_CFG1_VHF;
> +
>  	writew(reg, onenand_base + ONENAND_REG_SYS_CFG1);
>  }
>  
> @@ -289,6 +288,7 @@ static int omap2_onenand_setup_async(void __iomem *onenand_base)
>  		}
>  	}
>  
> +	onenand_async.sync_write = true;
>  	omap2_onenand_calc_async_timings(&t);
>  
>  	ret = gpmc_cs_program_settings(gpmc_onenand_data->cs, &onenand_async);
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html