Re: [PATCH 1/2 v2] mtd/nand: fixup for fmr initialization of Freescale NAND controller

On 12/07/2011 09:36 PM, Liu Shengzhou-B36685 wrote:

quoted

-----Original Message-----
From: Wood Scott-B07421
Sent: Thursday, December 08, 2011 1:17 AM
To: Liu Shengzhou-B36685
Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org; linux-
mtd@lists.infradead.org; dwmw2@infradead.org; Gala Kumar-B11780
Subject: Re: [PATCH 1/2 v2] mtd/nand: fixup for fmr initialization of
Freescale NAND controller

On 12/07/2011 12:30 AM, Liu Shengzhou-B36685 wrote:

quoted

[Shengzhou] This patch doesn't change the way ECCM is handled, it's

still same as before, just make sure CWTO timeout is set to maximum.

It does change it.  It used to use the existing value in FMR, and now it
sets it based on ORn[PGS].

-Scott

[Shengzhou]
  In u-boot:
	#ifdef CONFIG_FSL_ELBC_FMR
           priv->fmr = CONFIG_FSL_ELBC_FMR;
	#else
	     priv->fmr = (15 << FMR_CWTO_SHIFT) | (2 << FMR_AL_SHIFT);
	     or = in_be32(&elbc_ctrl->regs->bank[priv->bank].or);
	     if (or & OR_FCM_PGS)
    		       priv->fmr |= FMR_ECCM;
	#endif

In kernel: It used to be " priv->fmr = in_be32(&lbc->fmr) & FMR_ECCM
", so fmr was always 0x100(or 0,depend on ORn[PGS]), CWTO was
0(timeout was minimum).   In this patch, for not relying on
bootloader, fmr is initialized as what u-boot does, except
FMR_AL_SHIFT is handled in fsl_elbc_chip_init_tail and without
definition of CONFIG_FSL_ELBC_FMR.

  So, it doesn't change it.

You're assuming that the above U-Boot code is always run.  This depends
on whether the NAND driver is enabled in U-Boot.

In the future, though, it might also depend on whether a NAND command is
actually run in U-Boot -- this makes the setting of FMR
non-deterministic between boots, which is worse than a one-time breakage
of an unusual setup (driver not enabled in U-Boot at all).

So it is a change, but I now think it's a change we should make.  The
changelog should mention that this is happening, though.

Do we still need CONFIG_FSL_ELBC_FMR in kernel? 

We do not want such a compile-time constant in the kernel.  Use ORn[PGS]
as the patch currently does.

-Scott