[U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver

[Scott Wood]

On Tue, 2014-08-12 at 23:13 +0200, Stefan Agner wrote:
> Am 2014-08-12 00:33, schrieb Scott Wood:
> > On Wed, 2014-08-06 at 10:59 +0200, Stefan Agner wrote:
> >> This adds initial support for Freescale NFC (NAND Flash Controller).
> >> The IP is used in ARM based Vybrid SoCs as well as on some PowerPC
> >> devices. This driver is only tested on Vybrid.
> >>
> >> Signed-off-by: Stefan Agner <stefan at agner.ch>
> >> ---
> >>  drivers/mtd/nand/Makefile  |   1 +
> >>  drivers/mtd/nand/fsl_nfc.c | 676 +++++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 677 insertions(+)
> >>  create mode 100644 drivers/mtd/nand/fsl_nfc.c
> >>
> >> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> >> index bf1312a..85cb0dd 100644
> >> --- a/drivers/mtd/nand/Makefile
> >> +++ b/drivers/mtd/nand/Makefile
> >> @@ -51,6 +51,7 @@ obj-$(CONFIG_NAND_KB9202) += kb9202_nand.o
> >>  obj-$(CONFIG_NAND_KIRKWOOD) += kirkwood_nand.o
> >>  obj-$(CONFIG_NAND_KMETER1) += kmeter1_nand.o
> >>  obj-$(CONFIG_NAND_MPC5121_NFC) += mpc5121_nfc.o
> >> +obj-$(CONFIG_NAND_FSL_NFC) += fsl_nfc.o
> >>  obj-$(CONFIG_NAND_MXC) += mxc_nand.o
> > 
> > Could you explain how this differs from mpc5121_nfc, mxc_nand, etc?  If
> > it's truly different enough to deserve its own driver, it should at
> > least get a more specific name.
> > 
> 
> I'm not really an expert in NAND devices, but from looking into the
> reference manuals and drivers, I summarize those differences:
> mxc_nand: Supports 3 NAND Flash controller found in i.MX ARM SoCs:
> V1: MX31/MX27, 16 Bit Registers
> V2.1: MX25/MX35, 16 Bit Registers, 
> V3.2: MX51/MX53, 32 Bit Registers, 12 address registers...
> All three have no DMA controller integrated. 
> 
> mpc5121_nfc: Supports the MPC5121 Power Architecture Processor NAND
> flash controller. Big Endian, but other than this, this IP looks very
> similar to the V2.1 NFC above (looking at the registers and the block
> diagram). 

Yes, this is the similarity that prompted me to ask the question...  I
asked it back when those drivers were submitted, but was unable to get
the submitter of each to work together on a common driver.

> fsl_nfc: Supports VF610 (2011), however should be easily portable to
> MPC5125 Power Architecture Processor (2009) and MCF5441X ColdFire V4
> Microprocessor (2009)
> All three share the almost identical Register Layout, similar block
> diagram and have integrated DMA controller.
> 
> IMHO, this IP really deserves a own driver.
>
> Also, from my humble perspective, it might have made more sense to
> integrate mpc5121_nfc into mxc_nand. In return, splitting out mxc_nand
> NFC V3.2 (looking at the ifdefs and quite different register layout).
> Anyway, not part of the topic here..
> 
> Regarding naming: A more specific name makes sense since Freescale calls
> all its Flash Controllers "NFC". I suggest vf610_nfc because that SoC is
> really is making use of the driver today... Looking at release dates,
> mpc5125_nfc would make sense as well.

OK.

> >> +static u32 nfc_read(struct mtd_info *mtd, uint reg)
> >> +{
> >> +	struct fsl_nfc *nfc = mtd_to_nfc(mtd);
> >> +
> >> +	if (reg == NFC_FLASH_STATUS1 ||
> >> +	    reg == NFC_FLASH_STATUS2 ||
> >> +	    reg == NFC_IRQ_STATUS)
> >> +		return __raw_readl(nfc->regs + reg);
> >> +	/* Gang read/writes together for most registers. */
> >> +	else
> >> +		return *(u32 *)(nfc->regs + reg);
> >> +}
> >> +
> >> +static void nfc_write(struct mtd_info *mtd, uint reg, u32 val)
> >> +{
> >> +	struct fsl_nfc *nfc = mtd_to_nfc(mtd);
> >> +
> >> +	if (reg == NFC_FLASH_STATUS1 ||
> >> +	    reg == NFC_FLASH_STATUS2 ||
> >> +	    reg == NFC_IRQ_STATUS)
> >> +		__raw_writel(val, nfc->regs + reg);
> >> +	/* Gang read/writes together for most registers. */
> >> +	else
> >> +		*(u32 *)(nfc->regs + reg) = val;
> >> +}
> > 
> > You should always be using raw I/O accessors.  If the intent is to
> > bypass I/O ordering for certain registers, the raw accessors should
> > already be skipping that.
> > 
> 
> Ok, will do.

Sorry, the above should say "always be using I/O accesors", not always
raw ones.

> >> +int board_nand_init(struct nand_chip *chip)
> >> +{
> > [snip]
> >> +	/* second phase scan */
> >> +	if (nand_scan_tail(mtd)) {
> >> +		err = -ENXIO;
> >> +		goto error;
> >> +	}
> >> +
> > 
> > board_nand_init() should only call nand_scan_tail if
> > CONFIG_SYS_NAND_SELF_INIT is defined -- and if it is defined, then
> > board_nand_init() takes no arguments.
> > 
> 
> Ok, we need the page size during initialization, hence nand_scan_ident
> needs to be in the init code. I remove the argument from board_nand_init
> then.

Look at fsl_elbc_nand.c and fsl_ifc_nand.c for examples of how to use
CONFIG_SYS_NAND_SELF_INIT.

-Scott