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