Re: [PATCH 1/4] GPMI-NAND: Define ecc.strength

Re: [PATCH 1/4] GPMI-NAND: Define ecc.strength

[Shawn Guo]

So non-DT probe has the same issue?  Or this is a DT probe only issue?

Copy linux-mtd list and Artem Bityutskiy on this.

-- 
Regards,
Shawn

On Mon, May 21, 2012 at 03:02:17AM +0200, Marek Vasut wrote:
> The ecc.strength wasn't set, resulting in the following crash:
> [    2.550000] kernel BUG at drivers/mtd/nand/nand_base.c:3347!
> [    2.550000] Internal error: Oops - BUG: 0 [#1] ARM
> [    2.550000] Modules linked in:
> [    2.550000] CPU: 0    Not tainted  (3.4.0-rc7-next-20120518-dirty #1211)
> [    2.550000] PC is at nand_scan_tail+0x328/0x650
> [    2.550000] LR is at nand_scan_tail+0x328/0x650
> [    2.550000] pc : [<c020841c>]    lr : [<c020841c>]    psr: 20000013
> [    2.550000] sp : c7443ea8  ip : c7448000  fp : c043dc70
> [    2.550000] r10: 00000000  r9 : c059dd20  r8 : c7526300
> [    2.550000] r7 : 00000001  r6 : c7479c08  r5 : c7526300  r4 : c7526098
> [    2.550000] r3 : c7448000  r2 : 00000000  r1 : 00000001  r0 : 00000034
> [    2.550000] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
> [    2.550000] Control: 0005317f  Table: 40004000  DAC: 00000017
> [    2.550000] Process swapper (pid: 1, stack limit = 0xc7442270)
> [    2.550000] Stack: (0xc7443ea8 to 0xc7444000)
> [    2.550000] 3ea0:                   c7526000 00000000 c7479c08 c02f68e0 c03d50ec c7526000
> [    2.550000] 3ec0: c747c6e0 00000000 c0ae7720 00000004 00000000 00000000 c7479c08 c0ae7720
> [    2.550000] 3ee0: c7479c3c c0593e20 c7572200 c01f6618 c01f6604 c01f55b0 c7479c08 c0593e20
> [    2.550000] 3f00: c7479c3c c059dd20 c7572200 c01f57cc 00000000 c0593e20 c01f5738 c01f3d40
> [    2.550000] 3f20: c74038a8 c74767d0 c0593e20 c04475ec c05901b8 c01f4e18 c03d50e0 c019d438
> [    2.550000] 3f40: c7403800 c0593e20 c04475ec 00000000 c059dd20 c04475f8 c7442000 c01f5a70
> [    2.550000] 3f60: 00000000 c044e98c c04475ec 00000000 c059dd20 c04475f8 c7442000 c043dc7c
> [    2.550000] 3f80: c044e98c c0008824 c03d6c30 c040c4e4 00000006 00000006 c03b0ce4 c0bf54e0
> [    2.550000] 3fa0: c044e98c c04475ec 00000006 c04475f8 0000004c c042a21c 00000000 c042a8b8
> [    2.550000] 3fc0: 00000006 00000006 c042a21c 00000000 00000000 00000000 00000000 c042a7bc
> [    2.550000] 3fe0: c000fab4 00000013 00000000 00000000 00000000 c000fab4 efbbfaa5 eaeafefe
> [    2.550000] [<c020841c>] (nand_scan_tail+0x328/0x650) from [<c02f68e0>] (gpmi_nand_probe+0x43c/0x5a4)
> [    2.550000] [<c02f68e0>] (gpmi_nand_probe+0x43c/0x5a4) from [<c01f6618>] (platform_drv_probe+0x14/0x18)
> [    2.550000] [<c01f6618>] (platform_drv_probe+0x14/0x18) from [<c01f55b0>] (driver_probe_device+0x74/0x1fc)
> [    2.550000] [<c01f55b0>] (driver_probe_device+0x74/0x1fc) from [<c01f57cc>] (__driver_attach+0x94/0x98)
> [    2.550000] [<c01f57cc>] (__driver_attach+0x94/0x98) from [<c01f3d40>] (bus_for_each_dev+0x50/0x80)
> [    2.550000] [<c01f3d40>] (bus_for_each_dev+0x50/0x80) from [<c01f4e18>] (bus_add_driver+0x188/0x25c)
> [    2.550000] [<c01f4e18>] (bus_add_driver+0x188/0x25c) from [<c01f5a70>] (driver_register+0x78/0x138)
> [    2.550000] [<c01f5a70>] (driver_register+0x78/0x138) from [<c043dc7c>] (gpmi_nand_init+0xc/0x30)
> [    2.550000] [<c043dc7c>] (gpmi_nand_init+0xc/0x30) from [<c0008824>] (do_one_initcall+0x108/0x17c)
> [    2.550000] [<c0008824>] (do_one_initcall+0x108/0x17c) from [<c042a8b8>] (kernel_init+0xfc/0x1bc)
> [    2.550000] [<c042a8b8>] (kernel_init+0xfc/0x1bc) from [<c000fab4>] (kernel_thread_exit+0x0/0x8)
> [    2.550000] Code: e3530000 1affff76 e59f02d0 eb03c16a (e7f001f2)
> [    2.830000] ---[ end trace 3f05d4c0039432f1 ]---
> [    2.840000] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Huang Shijie <b32955@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 9b4c109..f5f3d761 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -1436,6 +1436,9 @@ static int gpmi_pre_bbt_scan(struct gpmi_nand_data  *this)
>  	if (ret)
>  		return ret;
>  
> +	/* Adjust the ECC strength according to the chip. */
> +	this->nand.ecc.strength = this->bch_geometry.ecc_strength;
> +
>  	/* NAND boot init, depends on the gpmi_set_geometry(). */
>  	return nand_boot_init(this);
>  }
> @@ -1494,6 +1497,7 @@ static int __devinit gpmi_nfc_init(struct gpmi_nand_data *this)
>  	chip->options		|= NAND_NO_SUBPAGE_WRITE;
>  	chip->ecc.mode		= NAND_ECC_HW;
>  	chip->ecc.size		= 1;
> +	chip->ecc.strength	= 8;
>  	chip->ecc.layout	= &gpmi_hw_ecclayout;
>  
>  	/* Allocate a temporary DMA buffer for reading ID in the nand_scan() */
> -- 
> 1.7.10
> 

Re: [PATCH 1/4] GPMI-NAND: Define ecc.strength

[Marek Vasut]

Dear Shawn Guo,

> So non-DT probe has the same issue?  Or this is a DT probe only issue?

I started using DT, so I'm now fixing everything to work like it used to. Damn 
this DT conversion broke about everything.

> 
> Copy linux-mtd list and Artem Bityutskiy on this.
> 
> > The ecc.strength wasn't set, resulting in the following crash:
> > [    2.550000] kernel BUG at drivers/mtd/nand/nand_base.c:3347!
> > [    2.550000] Internal error: Oops - BUG: 0 [#1] ARM
> > [    2.550000] Modules linked in:
> > [    2.550000] CPU: 0    Not tainted  (3.4.0-rc7-next-20120518-dirty
> > #1211) [    2.550000] PC is at nand_scan_tail+0x328/0x650
> > [    2.550000] LR is at nand_scan_tail+0x328/0x650
> > [    2.550000] pc : [<c020841c>]    lr : [<c020841c>]    psr: 20000013
> > [    2.550000] sp : c7443ea8  ip : c7448000  fp : c043dc70
> > [    2.550000] r10: 00000000  r9 : c059dd20  r8 : c7526300
> > [    2.550000] r7 : 00000001  r6 : c7479c08  r5 : c7526300  r4 : c7526098
> > [    2.550000] r3 : c7448000  r2 : 00000000  r1 : 00000001  r0 : 00000034
> > [    2.550000] Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM 
> > Segment kernel [    2.550000] Control: 0005317f  Table: 40004000  DAC:
> > 00000017 [    2.550000] Process swapper (pid: 1, stack limit =
> > 0xc7442270) [    2.550000] Stack: (0xc7443ea8 to 0xc7444000)
> > [    2.550000] 3ea0:                   c7526000 00000000 c7479c08
> > c02f68e0 c03d50ec c7526000 [    2.550000] 3ec0: c747c6e0 00000000
> > c0ae7720 00000004 00000000 00000000 c7479c08 c0ae7720 [    2.550000]
> > 3ee0: c7479c3c c0593e20 c7572200 c01f6618 c01f6604 c01f55b0 c7479c08
> > c0593e20 [    2.550000] 3f00: c7479c3c c059dd20 c7572200 c01f57cc
> > 00000000 c0593e20 c01f5738 c01f3d40 [    2.550000] 3f20: c74038a8
> > c74767d0 c0593e20 c04475ec c05901b8 c01f4e18 c03d50e0 c019d438 [   
> > 2.550000] 3f40: c7403800 c0593e20 c04475ec 00000000 c059dd20 c04475f8
> > c7442000 c01f5a70 [    2.550000] 3f60: 00000000 c044e98c c04475ec
> > 00000000 c059dd20 c04475f8 c7442000 c043dc7c [    2.550000] 3f80:
> > c044e98c c0008824 c03d6c30 c040c4e4 00000006 00000006 c03b0ce4 c0bf54e0
> > [    2.550000] 3fa0: c044e98c c04475ec 00000006 c04475f8 0000004c
> > c042a21c 00000000 c042a8b8 [    2.550000] 3fc0: 00000006 00000006
> > c042a21c 00000000 00000000 00000000 00000000 c042a7bc [    2.550000]
> > 3fe0: c000fab4 00000013 00000000 00000000 00000000 c000fab4 efbbfaa5
> > eaeafefe [    2.550000] [<c020841c>] (nand_scan_tail+0x328/0x650) from
> > [<c02f68e0>] (gpmi_nand_probe+0x43c/0x5a4) [    2.550000] [<c02f68e0>]
> > (gpmi_nand_probe+0x43c/0x5a4) from [<c01f6618>]
> > (platform_drv_probe+0x14/0x18) [    2.550000] [<c01f6618>]
> > (platform_drv_probe+0x14/0x18) from [<c01f55b0>]
> > (driver_probe_device+0x74/0x1fc) [    2.550000] [<c01f55b0>]
> > (driver_probe_device+0x74/0x1fc) from [<c01f57cc>]
> > (__driver_attach+0x94/0x98) [    2.550000] [<c01f57cc>]
> > (__driver_attach+0x94/0x98) from [<c01f3d40>]
> > (bus_for_each_dev+0x50/0x80) [    2.550000] [<c01f3d40>]
> > (bus_for_each_dev+0x50/0x80) from [<c01f4e18>]
> > (bus_add_driver+0x188/0x25c) [    2.550000] [<c01f4e18>]
> > (bus_add_driver+0x188/0x25c) from [<c01f5a70>]
> > (driver_register+0x78/0x138) [    2.550000] [<c01f5a70>]
> > (driver_register+0x78/0x138) from [<c043dc7c>] (gpmi_nand_init+0xc/0x30)
> > [    2.550000] [<c043dc7c>] (gpmi_nand_init+0xc/0x30) from [<c0008824>]
> > (do_one_initcall+0x108/0x17c) [    2.550000] [<c0008824>]
> > (do_one_initcall+0x108/0x17c) from [<c042a8b8>] (kernel_init+0xfc/0x1bc)
> > [    2.550000] [<c042a8b8>] (kernel_init+0xfc/0x1bc) from [<c000fab4>]
> > (kernel_thread_exit+0x0/0x8) [    2.550000] Code: e3530000 1affff76
> > e59f02d0 eb03c16a (e7f001f2) [    2.830000] ---[ end trace
> > 3f05d4c0039432f1 ]---
> > [    2.840000] Kernel panic - not syncing: Attempted to kill init!
> > exitcode=0x0000000b
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Huang Shijie <b32955@freescale.com>
> > Cc: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Fabio Estevam <fabio.estevam@freescale.com>
> > ---
> > 
> >  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c index 9b4c109..f5f3d761 100644
> > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > @@ -1436,6 +1436,9 @@ static int gpmi_pre_bbt_scan(struct gpmi_nand_data 
> > *this)
> > 
> >  	if (ret)
> >  	
> >  		return ret;
> > 
> > +	/* Adjust the ECC strength according to the chip. */
> > +	this->nand.ecc.strength = this->bch_geometry.ecc_strength;
> > +
> > 
> >  	/* NAND boot init, depends on the gpmi_set_geometry(). */
> >  	return nand_boot_init(this);
> >  
> >  }
> > 
> > @@ -1494,6 +1497,7 @@ static int __devinit gpmi_nfc_init(struct
> > gpmi_nand_data *this)
> > 
> >  	chip->options		|= NAND_NO_SUBPAGE_WRITE;
> >  	chip->ecc.mode		= NAND_ECC_HW;
> >  	chip->ecc.size		= 1;
> > 
> > +	chip->ecc.strength	= 8;
> > 
> >  	chip->ecc.layout	= &gpmi_hw_ecclayout;
> >  	
> >  	/* Allocate a temporary DMA buffer for reading ID in the nand_scan() */

Best regards,
Marek Vasut

Re: [PATCH 1/4] GPMI-NAND: Define ecc.strength

[Shawn Guo]

On Mon, May 21, 2012 at 03:36:41AM +0200, Marek Vasut wrote:
> Dear Shawn Guo,
> 
> > So non-DT probe has the same issue?  Or this is a DT probe only issue?
> 
> I started using DT, so I'm now fixing everything to work like it used to. Damn 
> this DT conversion broke about everything.
> 
So you are saying the issue is introduced by gpmi-nand DT conversion?

-- 
Regards,
Shawn

Re: [PATCH 1/4] GPMI-NAND: Define ecc.strength

[Marek Vasut]

Dear Shawn Guo,

> On Mon, May 21, 2012 at 03:36:41AM +0200, Marek Vasut wrote:
> > Dear Shawn Guo,
> > 
> > > So non-DT probe has the same issue?  Or this is a DT probe only issue?
> > 
> > I started using DT, so I'm now fixing everything to work like it used to.
> > Damn this DT conversion broke about everything.
> 
> So you are saying the issue is introduced by gpmi-nand DT conversion?

I doubt so ... but noone ever could test it without DT bindings as you 
NAKed/stalled every single attempt to add the platform binding code. So this 
code was rotting in the tree, who knows how many bugs are still in it. Now I 
guess it's about time to start squashing the bugs.

Best regards,
Marek Vasut

Re: [PATCH 1/4] GPMI-NAND: Define ecc.strength

[Huang Shijie]

于 2012年05月21日 10:40, Marek Vasut 写道:
> I doubt so ... but noone ever could test it without DT bindings as you
> NAKed/stalled every single attempt to add the platform binding code. So this
> code was rotting in the tree, who knows how many bugs are still in it. Now I
> guess it's about time to start squashing the bugs.
>
I checked the latest l2-mtd code. The bug does exit if the gpmi-nand 
driver can works(actually it can not).

I will test your patch.
Anyway, this patch should be sent to Artem, not the arm-soc.

thanks
Huang Shijie

Re: [PATCH 1/4] GPMI-NAND: Define ecc.strength

[Marek Vasut]

Dear Huang Shijie,

> 于 2012年05月21日 10:40, Marek Vasut 写道:
> > I doubt so ... but noone ever could test it without DT bindings as you
> > NAKed/stalled every single attempt to add the platform binding code. So
> > this code was rotting in the tree, who knows how many bugs are still in
> > it. Now I guess it's about time to start squashing the bugs.
> 
> I checked the latest l2-mtd code. The bug does exit if the gpmi-nand
> driver can works(actually it can not).

There's also the bug where you need to put gpmi@8000c000 into the DTS file 
instead of gpmi-nand@8000c000 ... maybe you should fix it?

Also, there's still the bug with subpage write size, "GPMI-NAND: Wrong ECC size 
in driver" thread in linux-mtd. That thread kind of died, but gpmi-nand is still 
broken in that way, it's depending on loose end of the mtd stack there. Won't 
hurt fixing that too, there was even a fix proposed.

> 
> I will test your patch.
> Anyway, this patch should be sent to Artem, not the arm-soc.

Resent, fixed, sorry for the inconvenience.

> 
> thanks
> Huang Shijie

Best regards,
Marek Vasut

Re: [PATCH 1/4] GPMI-NAND: Define ecc.strength

[Shawn Guo]

On Mon, May 21, 2012 at 04:40:09AM +0200, Marek Vasut wrote:
> > So you are saying the issue is introduced by gpmi-nand DT conversion?
> 
> I doubt so ...

I can be very sure that you haven't looked at the gpmi-nand DT
conversion patches at all.

The bug has been there since day one.  The commit cfbc855b (mtd: nand:
add sanity check of ecc strength to nand_scan_tail()) added a BUG() to
catch it recently.

So please at least spend some time to know the real cause before
pointing your figure to DT conversion.

-- 
Regards,
Shawn

Re: [PATCH 1/4] GPMI-NAND: Define ecc.strength

[Marek Vasut]

Dear Shawn Guo,

> On Mon, May 21, 2012 at 04:40:09AM +0200, Marek Vasut wrote:
> > > So you are saying the issue is introduced by gpmi-nand DT conversion?
> > 
> > I doubt so ...
> 
> I can be very sure that you haven't looked at the gpmi-nand DT
> conversion patches at all.
> 
> The bug has been there since day one.  The commit cfbc855b (mtd: nand:
> add sanity check of ecc strength to nand_scan_tail()) added a BUG() to
> catch it recently.
> 
> So please at least spend some time to know the real cause before
> pointing your figure to DT conversion.

Well ... since noone was really able to test the patch, noone figured out it's 
broken. We can agree on that, right.

And btw. by "I doubt so ..." I means exactly that I doubt the bug was caused by 
the DT conversion.

Best regards,
Marek Vasut

Re: [PATCH 1/4] GPMI-NAND: Define ecc.strength

[Huang Shijie]

Hi Marek:
> On Mon, May 21, 2012 at 03:36:41AM +0200, Marek Vasut wrote:
>> Dear Shawn Guo,
>>
>>> So non-DT probe has the same issue?  Or this is a DT probe only issue?
>> I started using DT, so I'm now fixing everything to work like it used to. Damn
>> this DT conversion broke about everything.
>>
> So you are saying the issue is introduced by gpmi-nand DT conversion?
>
thanks for your work. :)
But the gpmi nand driver should not work right now.

The DT for gpmi-nand is in Artem's l2-mtd tree.
And the device node item in DTS for gpmi is not added now.( I ever 
submitted, but not accepted.)
Also the pinctrl item for gpmi is also not added not.

So I suggest you test it again when the next merge window opens, and all 
the code is merged to kernel.
If the bug still exits, please submit a fix patch then.

By the way, I really did not meet your error when I test the DT patches 
for gpmi.

thanks
Huang Shijie

Re: [PATCH 1/4] GPMI-NAND: Define ecc.strength

[Marek Vasut]

Dear Huang Shijie,

> Hi Marek:
> > On Mon, May 21, 2012 at 03:36:41AM +0200, Marek Vasut wrote:
> >> Dear Shawn Guo,
> >> 
> >>> So non-DT probe has the same issue?  Or this is a DT probe only issue?
> >> 
> >> I started using DT, so I'm now fixing everything to work like it used
> >> to. Damn this DT conversion broke about everything.
> > 
> > So you are saying the issue is introduced by gpmi-nand DT conversion?
> 
> thanks for your work. :)
> But the gpmi nand driver should not work right now.

Why not? I tested, I could at least read stuff from it.

> The DT for gpmi-nand is in Artem's l2-mtd tree.
> And the device node item in DTS for gpmi is not added now.( I ever
> submitted, but not accepted.)

Well I crafted one myself, I need different pinmux settings anyway so I added my 
own pinmux configuration.

> Also the pinctrl item for gpmi is also not added not.
> 
> So I suggest you test it again when the next merge window opens, and all
> the code is merged to kernel.

Please ... Shawn explained that this bug will still be there. So why not get it 
in early to fix bug asap. I'll resend to artem now.

> If the bug still exits, please submit a fix patch then.
> 
> By the way, I really did not meet your error when I test the DT patches
> for gpmi.

Can you post your snippet?

> thanks
> Huang Shijie

Best regards,
Marek Vasut