From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Mon, 11 Aug 2014 17:33:46 -0500
Subject: [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
In-Reply-To: <4ea124ab817db6e3dee2067aadd6db14643990f5.1407312577.git.stefan@agner.ch>
References: <cover.1407312577.git.stefan@agner.ch>
	<4ea124ab817db6e3dee2067aadd6db14643990f5.1407312577.git.stefan@agner.ch>
Message-ID: <1407796426.7427.100.camel@snotra.buserror.net>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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@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.

> +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.

> +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.

-Scott