[U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
Stefan Agner
stefan at agner.ch
Wed Aug 13 10:13:07 CEST 2014
Am 2014-08-13 00:58, schrieb Bill Pringlemeir:
> 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 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.
>
> 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.
>
I agree, we should take the same name. But so far, nobody expressed a
preference in the MTD mailing list. Probably before settling with a
name, we should write that to the MTD mailing list as well so we could
change the name in case somebody does not agree.
Since the Kinetis K70 chip is not really suited for Linux I guess naming
it according to this chip is not really a good option.
I'm not entirely happy with vf610_nfc, but I guess it's the best we
have. Any objections/other ideas?
>>>>> +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.
>
I like the nfc_read/(nfc_set/nfc_clear)/nfc_write approach more then
having nfc_set doing the nfc_read/nfc_write hidden and rely on compiler
optimization. Additionally, this really shows that the programmer _want_
this register written in one step.
I will try to alter the driver again to use this kind of access to see
how it looks in reality.
>>>>> +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.
More information about the U-Boot
mailing list