Patchwork [U-Boot] mtd: nand: mxs: reset BCH earlier, too, to avoid NAND startup problems

login
register
mail settings
Submitter Wolfram Sang
Date Dec. 5, 2012, 8:48 p.m.
Message ID <1354740527-18798-1-git-send-email-w.sang@pengutronix.de>
Download mbox | patch
Permalink /patch/203981/
State Accepted
Delegated to: Scott Wood
Headers show

Comments

Wolfram Sang - Dec. 5, 2012, 8:48 p.m.
It could happen (1 out of 100 times) that NAND did not start up correctly after
warm rebooting, so we end up with various failures or DMA timed out due to a
stalled BCH. When resetting BCH together with GPMI, the issue could not be
observed anymore (after 10000+ reboots). We probably need the consistent state
already before sending commands to NAND. This behaviour was observed in barebox
and kernel, so I assume it affects U-Boot as well. I chose to keep the extra
reset for BCH when changing the flash layout to be on the safe side.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---

Only compile tested. Test case was to repeatedly reboot into a simple userspace
on a UBI volume. Either the bootloader failed to find its data or the kernel
could not mount the UBI volume once in a while. (Yes, the kernel could not
mount the UBI although it was itself loaded correctly from NAND. So, it is some
set-up issue.) This was nasty to debug, so I thought I let you know...

 drivers/mtd/nand/mxs_nand.c |    3 +++
 1 file changed, 3 insertions(+)
Otavio Salvador - Dec. 5, 2012, 10:28 p.m.
On Wed, Dec 5, 2012 at 6:48 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:

> It could happen (1 out of 100 times) that NAND did not start up correctly
> after
> warm rebooting, so we end up with various failures or DMA timed out due to
> a
> stalled BCH. When resetting BCH together with GPMI, the issue could not be
> observed anymore (after 10000+ reboots). We probably need the consistent
> state
> already before sending commands to NAND. This behaviour was observed in
> barebox
> and kernel, so I assume it affects U-Boot as well. I chose to keep the
> extra
> reset for BCH when changing the flash layout to be on the safe side.
>
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>
> Only compile tested. Test case was to repeatedly reboot into a simple
> userspace
> on a UBI volume. Either the bootloader failed to find its data or the
> kernel
> could not mount the UBI volume once in a while. (Yes, the kernel could not
> mount the UBI although it was itself loaded correctly from NAND. So, it is
> some
> set-up issue.) This was nasty to debug, so I thought I let you know...
>
>  drivers/mtd/nand/mxs_nand.c |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c
> index 4701be8..e38e151 100644
> --- a/drivers/mtd/nand/mxs_nand.c
> +++ b/drivers/mtd/nand/mxs_nand.c
> @@ -1058,6 +1058,8 @@ int mxs_nand_init(struct mxs_nand_info *info)
>  {
>         struct mxs_gpmi_regs *gpmi_regs =
>                 (struct mxs_gpmi_regs *)MXS_GPMI_BASE;
> +       struct mxs_bch_regs *bch_regs =
> +               (struct mxs_bch_regs *)MXS_BCH_BASE;
>         int i = 0, j;
>
>         info->desc = malloc(sizeof(struct mxs_dma_desc *) *
> @@ -1081,6 +1083,7 @@ int mxs_nand_init(struct mxs_nand_info *info)
>
>         /* Reset the GPMI block. */
>         mxs_reset_block(&gpmi_regs->hw_gpmi_ctrl0_reg);
> +       mxs_reset_block(&bch_regs->hw_bch_ctrl_reg);
>

A comment here why this is need would be nice.


>         /*
>          * Choose NAND mode, set IRQ polarity, disable write protection and
> --
> 1.7.10.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Fabio Estevam - Dec. 5, 2012, 11:35 p.m.
Hi Wolfram,

On Wed, Dec 5, 2012 at 6:48 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:

> diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c
> index 4701be8..e38e151 100644
> --- a/drivers/mtd/nand/mxs_nand.c
> +++ b/drivers/mtd/nand/mxs_nand.c
> @@ -1058,6 +1058,8 @@ int mxs_nand_init(struct mxs_nand_info *info)
>  {
>         struct mxs_gpmi_regs *gpmi_regs =
>                 (struct mxs_gpmi_regs *)MXS_GPMI_BASE;
> +       struct mxs_bch_regs *bch_regs =
> +               (struct mxs_bch_regs *)MXS_BCH_BASE;
>         int i = 0, j;
>
>         info->desc = malloc(sizeof(struct mxs_dma_desc *) *
> @@ -1081,6 +1083,7 @@ int mxs_nand_init(struct mxs_nand_info *info)
>
>         /* Reset the GPMI block. */
>         mxs_reset_block(&gpmi_regs->hw_gpmi_ctrl0_reg);
> +       mxs_reset_block(&bch_regs->hw_bch_ctrl_reg);

In your kernel patch you only do the reset for mx23, but here you do
it for mx28.

Which one is correct?

Regards,

Fabio Estevam
Wolfram Sang - Dec. 5, 2012, 11:39 p.m.
On Wed, Dec 05, 2012 at 09:35:26PM -0200, Fabio Estevam wrote:
> Hi Wolfram,
> 
> On Wed, Dec 5, 2012 at 6:48 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> 
> > diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c
> > index 4701be8..e38e151 100644
> > --- a/drivers/mtd/nand/mxs_nand.c
> > +++ b/drivers/mtd/nand/mxs_nand.c
> > @@ -1058,6 +1058,8 @@ int mxs_nand_init(struct mxs_nand_info *info)
> >  {
> >         struct mxs_gpmi_regs *gpmi_regs =
> >                 (struct mxs_gpmi_regs *)MXS_GPMI_BASE;
> > +       struct mxs_bch_regs *bch_regs =
> > +               (struct mxs_bch_regs *)MXS_BCH_BASE;
> >         int i = 0, j;
> >
> >         info->desc = malloc(sizeof(struct mxs_dma_desc *) *
> > @@ -1081,6 +1083,7 @@ int mxs_nand_init(struct mxs_nand_info *info)
> >
> >         /* Reset the GPMI block. */
> >         mxs_reset_block(&gpmi_regs->hw_gpmi_ctrl0_reg);
> > +       mxs_reset_block(&bch_regs->hw_bch_ctrl_reg);
> 
> In your kernel patch you only do the reset for mx23, but here you do
> it for mx28.
> 
> Which one is correct?

Both patches are correct. Check the kernel code again, please,
especially the function arguments.
Fabio Estevam - Dec. 6, 2012, 10:11 a.m.
On Wed, Dec 5, 2012 at 9:39 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:

> Both patches are correct. Check the kernel code again, please,
> especially the function arguments.

Ok, all is clear after reading the comments of the kernel
gpmi_reset_block function.

Regards,

Fabio Estevam
Marek Vasut - Dec. 11, 2012, 2:32 a.m.
Dear Wolfram Sang,

> It could happen (1 out of 100 times) that NAND did not start up correctly
> after warm rebooting, so we end up with various failures or DMA timed out
> due to a stalled BCH. When resetting BCH together with GPMI, the issue
> could not be observed anymore (after 10000+ reboots). We probably need the
> consistent state already before sending commands to NAND. This behaviour
> was observed in barebox and kernel, so I assume it affects U-Boot as well.
> I chose to keep the extra reset for BCH when changing the flash layout to
> be on the safe side.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
[...]

Good, thanks. Ccing Scott to pick this up.

Acked-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut
Scott Wood - Dec. 11, 2012, 11:23 p.m.
On 12/05/2012 02:48:47 PM, Wolfram Sang wrote:
> It could happen (1 out of 100 times) that NAND did not start up  
> correctly after
> warm rebooting, so we end up with various failures or DMA timed out  
> due to a
> stalled BCH. When resetting BCH together with GPMI, the issue could  
> not be
> observed anymore (after 10000+ reboots). We probably need the  
> consistent state
> already before sending commands to NAND. This behaviour was observed  
> in barebox
> and kernel, so I assume it affects U-Boot as well. I chose to keep  
> the extra
> reset for BCH when changing the flash layout to be on the safe side.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---

Applied to u-boot-nand-flash

-Scott

Patch

diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c
index 4701be8..e38e151 100644
--- a/drivers/mtd/nand/mxs_nand.c
+++ b/drivers/mtd/nand/mxs_nand.c
@@ -1058,6 +1058,8 @@  int mxs_nand_init(struct mxs_nand_info *info)
 {
 	struct mxs_gpmi_regs *gpmi_regs =
 		(struct mxs_gpmi_regs *)MXS_GPMI_BASE;
+	struct mxs_bch_regs *bch_regs =
+		(struct mxs_bch_regs *)MXS_BCH_BASE;
 	int i = 0, j;
 
 	info->desc = malloc(sizeof(struct mxs_dma_desc *) *
@@ -1081,6 +1083,7 @@  int mxs_nand_init(struct mxs_nand_info *info)
 
 	/* Reset the GPMI block. */
 	mxs_reset_block(&gpmi_regs->hw_gpmi_ctrl0_reg);
+	mxs_reset_block(&bch_regs->hw_bch_ctrl_reg);
 
 	/*
 	 * Choose NAND mode, set IRQ polarity, disable write protection and