| Message ID | 1454683028-4193-1-git-send-email-ivo.g.dimitrov.75@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* 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 >
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
* 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
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.
* 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
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.
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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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
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);
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(-)