arm: vf610: add NAND flash support

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 <[email protected]>

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

drivers/mtd/nand/fsl_nfc.c new file +0 -0

new file mode 100644
index 0000000..df2c8be
@@ -0,0 +1,676 @@

drivers/mtd/nand/fsl_nfc.c new file +11 -0

+	uint               column;
+	int                spareonly;
+	int                page;
+	/* Status and ID are in alternate locations. */
+	int                alt_buf;
+#define ALT_BUF_ID   1
+#define ALT_BUF_STAT 2
+	struct clk        *clk;
+};
+
+#define mtd_to_nfc(_mtd) (struct fsl_nfc *)((struct nand_chip

*)_mtd->priv)->priv;

drivers/mtd/nand/fsl_nfc.c new file +4 -0

+	ecc_status = __raw_readl(nfc->regs + ECC_SRAM_ADDR * 8 + 4);
+	ecc_count = ecc_status & ECC_ERR_COUNT;
+	if (!(ecc_status & ECC_STATUS_MASK))
+		return ecc_count;

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 <[email protected]>

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

drivers/mtd/nand/Makefile +0 -0

index bf1312a..85cb0dd 100644

drivers/mtd/nand/Makefile +15 -0

+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)
+{

drivers/mtd/nand/Makefile +2 -0

+int board_nand_init(struct nand_chip *chip)
+{

Quoted 11 lines

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 <[email protected]>

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

drivers/mtd/nand/Makefile obj-$(CONFIG_NAND_KB9202) += kb9202_nand.o · line 55

Quoted 10 lines

drivers/mtd/nand/Makefile +1 -0

index bf1312a..85cb0dd 100644
@@ -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.

Quoted 21 lines

drivers/mtd/nand/Makefile +21 -0

+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);

drivers/mtd/nand/Makefile

Quoted 4 lines

drivers/mtd/nand/Makefile +4 -0

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

Quoted 2 lines

drivers/mtd/nand/Makefile +2 -0

+int board_nand_init(struct nand_chip *chip)
+{

[snip]

drivers/mtd/nand/Makefile

Quoted 6 lines

drivers/mtd/nand/Makefile +6 -0

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

Am 2014-08-12 00:33, schrieb Scott Wood:

Quoted 12 lines · Stefan Agner

Quoted 11 lines

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 <[email protected]>

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

Quoted 15 lines

drivers/mtd/nand/Makefile obj-$(CONFIG_NAND_KB9202) += kb9202_nand.o · line 55

Quoted 10 lines

drivers/mtd/nand/Makefile +1 -0

index bf1312a..85cb0dd 100644
@@ -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).

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.

Quoted 21 lines

Quoted 21 lines

drivers/mtd/nand/Makefile +21 -0

+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);

Quoted 9 lines

drivers/mtd/nand/Makefile

Quoted 4 lines

drivers/mtd/nand/Makefile +4 -0

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

Quoted 14 lines

Quoted 2 lines

drivers/mtd/nand/Makefile +2 -0

+int board_nand_init(struct nand_chip *chip)
+{

[snip]

drivers/mtd/nand/Makefile

Quoted 6 lines

drivers/mtd/nand/Makefile +6 -0

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

Quoted 37 lines · Stefan Agner

Am 2014-08-12 00:33, schrieb Scott Wood:

Quoted 13 lines · Stefan Agner

Quoted 12 lines

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 <[email protected]>

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

Quoted 15 lines

Quoted 10 lines

drivers/mtd/nand/Makefile +0 -0

index bf1312a..85cb0dd 100644
@@ -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.

Quoted 4 lines

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.

Quoted 13 lines

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

Quoted 4 lines

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.

Quoted 29 lines

Quoted 29 lines

Quoted 15 lines

drivers/mtd/nand/Makefile +15 -0

+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)
+{

drivers/mtd/nand/Makefile

Quoted 10 lines

drivers/mtd/nand/Makefile +10 -0

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

Quoted 1 line

Ok, will do.

Sorry, the above should say "always be using I/O accesors", not always raw ones.

Quoted 15 lines

Quoted 14 lines

Quoted 2 lines

drivers/mtd/nand/Makefile +2 -0

+int board_nand_init(struct nand_chip *chip)
+{

[snip]

drivers/mtd/nand/Makefile

Quoted 6 lines

drivers/mtd/nand/Makefile +6 -0

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

Quoted 3 lines

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.

Quoted 40 lines · Stefan Agner

Quoted 37 lines

Am 2014-08-12 00:33, schrieb Scott Wood:

Quoted 13 lines · Stefan Agner

Quoted 12 lines

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 <[email protected]>

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

Quoted 15 lines

Quoted 10 lines

drivers/mtd/nand/Makefile +0 -0

index bf1312a..85cb0dd 100644
@@ -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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 13 lines

Quoted 13 lines

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

Quoted 6 lines

Quoted 4 lines

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.

Quoted 30 lines

Quoted 29 lines

Quoted 29 lines

Quoted 15 lines

drivers/mtd/nand/Makefile +15 -0

+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)
+{

drivers/mtd/nand/Makefile

Quoted 10 lines

drivers/mtd/nand/Makefile +10 -0

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

Quoted 4 lines

Quoted 1 line

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.

Quoted 21 lines

Quoted 15 lines

Quoted 14 lines

Quoted 2 lines

drivers/mtd/nand/Makefile +2 -0

+int board_nand_init(struct nand_chip *chip)
+{

[snip]

drivers/mtd/nand/Makefile

Quoted 6 lines

drivers/mtd/nand/Makefile +6 -0

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

Quoted 3 lines

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.

Quoted 30 lines

Quoted 29 lines

Quoted 29 lines

Quoted 15 lines

(quoted diff) +15 -0

+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)
+{

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Quoted 4 lines

Quoted 1 line

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.

Am 2014-08-13 00:58, schrieb Bill Pringlemeir:
[snip]

Quoted 74 lines

Quoted 30 lines

Quoted 29 lines

Quoted 29 lines

Quoted 15 lines

(quoted diff) +15 -0

+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)
+{

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Quoted 4 lines

Quoted 1 line

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);

Quoted 6 lines

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 just applied this change:

drivers/mtd/nand/fsl_nfc.c +2 -14

index df2c8be..01c010f 100644
@@ -191,26 +191,14 @@ 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);
+	return __raw_readl(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;
+	__raw_writel(val, nfc->regs + reg);
 }
 

static void nfc_set(struct mtd_info *mtd, uint reg, u32 bits)

And did some performance measurement:

=> nand read ${loadaddr} 0x0 0x2000000

===> With if/else
NAND read: device 0 offset 0x0, size 0x2000000
33554432 bytes read in 8494 ms: OK (3.8 MiB/s)

===> With __raw_readl only...
NAND read: device 0 offset 0x0, size 0x2000000
33554432 bytes read in 8184 ms: OK (3.9 MiB/s)

=> nand write ${loadaddr} rootfs-ubi 0x2000000

===> With if/else
NAND write: device 0 offset 0xa00000, size 0x2000000 33554432 bytes written in 18200 ms: OK (1.8 MiB/s)

===> With __raw_readl only...
NAND write: device 0 offset 0xa00000, size 0x2000000 33554432 bytes written in 15095 ms: OK (2.1 MiB/s)

These values are reproducible. I guess by removing the conditional branch in the nfc_read/nfc_write functions we even gain performance.

IMHO we should use the raw_writel only and "hand optimize" for functions which are used often. For the initialization/configuration functions, there is little value to save some register access.

Quoted 143 lines

Am 2014-08-13 00:58, schrieb Bill Pringlemeir:
[snip]

Quoted 74 lines

Quoted 30 lines

Quoted 29 lines

Quoted 29 lines

Quoted 15 lines

(quoted diff) +15 -0

+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)
+{

Review

Quoted 10 lines

(quoted diff) +10 -0

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

Quoted 4 lines

Quoted 1 line

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);

Quoted 6 lines

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 just applied this change:

drivers/mtd/nand/fsl_nfc.c +2 -14

index df2c8be..01c010f 100644
@@ -191,26 +191,14 @@ 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);
+	return __raw_readl(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;
+	__raw_writel(val, nfc->regs + reg);
 }
 

static void nfc_set(struct mtd_info *mtd, uint reg, u32 bits)

And did some performance measurement:

=> nand read ${loadaddr} 0x0 0x2000000

===> With if/else
NAND read: device 0 offset 0x0, size 0x2000000
33554432 bytes read in 8494 ms: OK (3.8 MiB/s)

===> With __raw_readl only...
NAND read: device 0 offset 0x0, size 0x2000000
33554432 bytes read in 8184 ms: OK (3.9 MiB/s)

=> nand write ${loadaddr} rootfs-ubi 0x2000000

===> With if/else
NAND write: device 0 offset 0xa00000, size 0x2000000 33554432 bytes written in 18200 ms: OK (1.8 MiB/s)

===> With __raw_readl only...
NAND write: device 0 offset 0xa00000, size 0x2000000 33554432 bytes written in 15095 ms: OK (2.1 MiB/s)

Quoted 6 lines

These values are reproducible. I guess by removing the conditional branch in the nfc_read/nfc_write functions we even gain performance.

IMHO we should use the raw_writel only and "hand optimize" for functions which are used often. For the initialization/configuration functions, there is little value to save some register access.

This will indeed depend on the compiler. I guess that the U-Boot 'readl' and 'writel' are the same as Linux? If the compiler doesn't do the analysis to see from the parameters that the 'if/else' is not needed, then it will not reduce the code and this can be larger.

I was using gcc 4.8 when I looked at this. The NFC is an AHB peripheral and takes several clocks to get access to. Did the object size increase/decrease? For Linux and this change with gcc 4.8,

With if/else

$ size drivers/mtd/nand/fsl_nfc.o
   text    data     bss     dec     hex filename
   3300    3652       0    6952    1b28 drivers/mtd/nand/fsl_nfc.o

Only readl/writel,

$ size drivers/mtd/nand/fsl_nfc.o
   text    data     bss     dec     hex filename
   3532    3652       0    7184    1c10 drivers/mtd/nand/fsl_nfc.o

Do you get bigger sizes for the 'readl/writel' only cases? This would indicate the compiler didn't inline/reduce the function calls. For certain, the 'read/set+clear/write' will be smaller and marginally faster with all compilers. If you have a smaller binary with the 'if/else' and the code still performs slower, then something I don't understand is happening.

It is sensible that this will not effect performance that much. The register access is not the main bottle neck. The 'memcpy()' in nfc_read_buf(), nfc_read_spare() and nfc_write_buf() are more critical to performance. Ie, getting the nand data on/off of the NFC controllers internal buffers to main memory is the main bottle neck. Ironically, on the Vybrid, the Cortex-A5 is able to do this faster than the DMA engine as it has much better though-put to the SDRAM.

The buffer transfers were originally 'memcpy_io()' or something which did barriers after each and every memory fetch. By replacing the register access alone, you shouldn't get that much of a performance difference (1/2 to 1/4 through-put).

Funny is, the size is bigger in the first uninlined case... Maybe GCC inlined the function only for some calls, I did not checked that...

With if/else
text data bss dec hex filename
2395 2904 0 5299 14b3 drivers/mtd/nand/fsl_nfc.o

IMHO we should use the raw_writel only and "hand optimize" for functions which are used often. For the initialization/configuration functions, there is little value to save some register access.

Quoted 10 lines · scottwood at freescale.com

Quoted 6 lines · Stefan Agner

Am 2014-08-12 00:33, schrieb Scott Wood:

Quoted 3 lines

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.

Quoted 28 lines · Bill Pringlemeir

Quoted 10 lines · scottwood at freescale.com

Quoted 6 lines · Stefan Agner

Am 2014-08-12 00:33, schrieb Scott Wood:

Quoted 3 lines

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

Quoted 7 lines

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.

If this is performance-critical then it would be best to modify the code to explicitly do one read from I/O (if you can't know in advance what the existing value will be), clear all the bits you're going to clear, then have one explicit write back to the I/O device -- rather than treating it as ordinary memory for the compiler to optimize accesses to.

If nothing else, it makes the code clearer to make I/O accesses explicit, and reduces the likelihood that people see I/O accesses without accessors and go on to do the same in some other less safe circumstance.

Regarding "can't know in advance", I think that some of the register values maybe set by the boot rom. This might make more sense for Linux than U-Boot. However, after the initial configuration, many do need the 'read/modify/write' as there are many bit fields with different functionality.

Quoted 5 lines · Bill Pringlemeir

Regarding "can't know in advance", I think that some of the register values maybe set by the boot rom. This might make more sense for Linux than U-Boot. However, after the initial configuration, many do need the 'read/modify/write' as there are many bit fields with different functionality.

If the register is only modified by software, and not asynchronously by hardware, then you could read the value once when the driver starts, and cache its value to avoid a reportedly expensive I/O access every time you need to modify it.

Add pin mux for NAND Flash Controller (NFC). NAND can be connected using 8 or 16 data lines, this patch adds pin mux entries for all 16 data lines.

Signed-off-by Stefan Agner <[email protected]>

Add NFC (NAND Flash Controller) clock support and enable them at board initialization time.

Signed-off-by Stefan Agner <[email protected]>