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

[Bill Pringlemeir <bpringlemeir@nbsps.com>]

On 12 Aug 2014, scottwood at freescale.com wrote:

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

I am familiar with the mxc_nand on the imx.  The register set might look
similar (ie, the register names) but when you look in depth at the bits
and their function, it is pretty clear it is different.  Some people
inside Freescale have said that this is a completely different IP.

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

Here we talked about the name,

 http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/251698.html

It is also present on a Kinetis K70 chip.

What should be important is that we use the same name in both U-boot
and Linux?  Won't it be confusing to call it something different?  I
really don't care what it is called as long as it is consistent.

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

This is probably because of me.  There are lines like this for
configuration,

        /* Set configuration register. */
        nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_ADDR_AUTO_INCR_BIT);
        nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_BUFNO_AUTO_INCR_BIT);
        nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_BOOT_MODE_BIT);
        nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_DMA_REQ_BIT);
        nfc_set(mtd, NFC_FLASH_CONFIG, CONFIG_FAST_FLASH_BIT);

If you use some sort of 'volatile', you are saying to the compiler that
these lines are *not*,

       ldr r0, [r1, #CONFIG]    # read previous value
       ldr r2, =~MASK
       orr r0, #FAST_FLASH_BIT  # set one bit.
       and r0, r2               # clear all bits at once.
       str r0, [r1, #CONFIG]    # write it back.

Instead it will change into five different load/modify/stores.  The
memory itself is just like SRAM except a few registers that are actually
volatile; ie changed via the hardware.

Particularly bad are the memcpy_io() on the ARM.  Using these results
in about 1/2 to 1/4 of the performance of the drivers through-put on the
Vybrid.  I can see that using accessors is good, but just replacing this
with 'writel' will result in significantly more code and slower
throughput.

If people insist on this, then something like,

      val = nfc_read(mtd, NFC_REG);
      val = nfc_clear(val, CONFIG_ADDR_AUTO_INCR_BIT);
      val = nfc_clear(val, CONFIG_BUFNO_AUTO_INCR_BIT);
      val = nfc_clear(val, CONFIG_BOOT_MODE_BIT);
      val = nfc_clear(val, CONFIG_DMA_REQ_BIT);
      val = nfc_set(val, CONFIG_FAST_FLASH_BIT);
      nfc_write(mtd, NFC_REG, val);

would result in the same code with 'nfc_read()' and 'nfc_write()' using
the I/O accessors.  I found this more difficult to read for the higher
level functions.  Instead some some standard macros for setting and
clearing bits could be used.  The original driver was using 'nfc_set()'
and 'nfc_clear()'.  Maybe they should just go.

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