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

Scott Wood
2014-08-11T22:33:46Z

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