* [U-Boot] [PATCH 1/4] arm: vf610: add NFC pin mux
2014-08-06 8:59 [U-Boot] [PATCH 0/4] arm: vf610: add NAND flash support Stefan Agner
@ 2014-08-06 8:59 ` Stefan Agner
2014-08-30 15:14 ` [U-Boot] [U-Boot,1/4] " Tom Rini
2014-08-06 8:59 ` [U-Boot] [PATCH 2/4] arm: vf610: add NFC clock support Stefan Agner
` (2 subsequent siblings)
3 siblings, 1 reply; 22+ messages in thread
From: Stefan Agner @ 2014-08-06 8:59 UTC (permalink / raw)
To: u-boot
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 <stefan@agner.ch>
---
arch/arm/include/asm/arch-vf610/iomux-vf610.h | 34 +++++++++++++++++++++++++++
arch/arm/include/asm/imx-common/iomux-v3.h | 4 ++++
2 files changed, 38 insertions(+)
diff --git a/arch/arm/include/asm/arch-vf610/iomux-vf610.h b/arch/arm/include/asm/arch-vf610/iomux-vf610.h
index a965641..7464da8 100644
--- a/arch/arm/include/asm/arch-vf610/iomux-vf610.h
+++ b/arch/arm/include/asm/arch-vf610/iomux-vf610.h
@@ -19,6 +19,13 @@
#define VF610_DDR_PAD_CTRL PAD_CTL_DSE_25ohm
#define VF610_I2C_PAD_CTRL (PAD_CTL_PUS_47K_UP | PAD_CTL_DSE_50ohm | \
PAD_CTL_SPEED_HIGH | PAD_CTL_OBE_IBE_ENABLE)
+#define VF610_NFC_IO_PAD_CTRL (PAD_CTL_SPEED_MED | PAD_CTL_SRE | \
+ PAD_CTL_DSE_50ohm | PAD_CTL_PUS_47K_UP | \
+ PAD_CTL_OBE_IBE_ENABLE)
+#define VF610_NFC_CN_PAD_CTRL (PAD_CTL_SPEED_MED | PAD_CTL_SRE | \
+ PAD_CTL_DSE_25ohm | PAD_CTL_OBE_ENABLE)
+#define VF610_NFC_RB_PAD_CTRL (PAD_CTL_SPEED_MED | PAD_CTL_SRE | \
+ PAD_CTL_PUS_22K_UP | PAD_CTL_IBE_ENABLE)
#define VF610_QSPI_PAD_CTRL (PAD_CTL_SPEED_HIGH | PAD_CTL_DSE_150ohm | \
PAD_CTL_PUS_22K_UP | PAD_CTL_OBE_IBE_ENABLE)
@@ -56,6 +63,15 @@ enum {
VF610_PAD_PTA29__ESDHC1_DAT3 = IOMUX_PAD(0x004c, 0x004c, 5, __NA_, 0, VF610_SDHC_PAD_CTRL),
VF610_PAD_PTB14__I2C0_SCL = IOMUX_PAD(0x0090, 0x0090, 2, 0x033c, 1, VF610_I2C_PAD_CTRL),
VF610_PAD_PTB15__I2C0_SDA = IOMUX_PAD(0x0094, 0x0094, 2, 0x0340, 1, VF610_I2C_PAD_CTRL),
+ VF610_PAD_PTD31__NF_IO15 = IOMUX_PAD(0x00fc, 0x00fc, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
+ VF610_PAD_PTD30__NF_IO14 = IOMUX_PAD(0x0100, 0x0100, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
+ VF610_PAD_PTD29__NF_IO13 = IOMUX_PAD(0x0104, 0x0104, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
+ VF610_PAD_PTD28__NF_IO12 = IOMUX_PAD(0x0108, 0x0108, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
+ VF610_PAD_PTD27__NF_IO11 = IOMUX_PAD(0x010c, 0x010c, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
+ VF610_PAD_PTD26__NF_IO10 = IOMUX_PAD(0x0110, 0x0110, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
+ VF610_PAD_PTD25__NF_IO9 = IOMUX_PAD(0x0114, 0x0114, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
+ VF610_PAD_PTD24__NF_IO8 = IOMUX_PAD(0x0118, 0x0118, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
+ VF610_PAD_PTD23__NF_IO7 = IOMUX_PAD(0x011c, 0x011c, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
VF610_PAD_PTD0__QSPI0_A_QSCK = IOMUX_PAD(0x013c, 0x013c, 1, __NA_, 0, VF610_QSPI_PAD_CTRL),
VF610_PAD_PTD1__QSPI0_A_CS0 = IOMUX_PAD(0x0140, 0x0140, 1, __NA_, 0, VF610_QSPI_PAD_CTRL),
VF610_PAD_PTD2__QSPI0_A_DATA3 = IOMUX_PAD(0x0144, 0x0144, 1, __NA_, 0, VF610_QSPI_PAD_CTRL),
@@ -68,6 +84,24 @@ enum {
VF610_PAD_PTD10__QSPI0_B_DATA2 = IOMUX_PAD(0x0164, 0x0164, 1, __NA_, 0, VF610_QSPI_PAD_CTRL),
VF610_PAD_PTD11__QSPI0_B_DATA1 = IOMUX_PAD(0x0168, 0x0168, 1, __NA_, 0, VF610_QSPI_PAD_CTRL),
VF610_PAD_PTD12__QSPI0_B_DATA0 = IOMUX_PAD(0x016c, 0x016c, 1, __NA_, 0, VF610_QSPI_PAD_CTRL),
+ VF610_PAD_PTD22__NF_IO6 = IOMUX_PAD(0x0120, 0x0120, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
+ VF610_PAD_PTD21__NF_IO5 = IOMUX_PAD(0x0124, 0x0124, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
+ VF610_PAD_PTD20__NF_IO4 = IOMUX_PAD(0x0128, 0x0128, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
+ VF610_PAD_PTD19__NF_IO3 = IOMUX_PAD(0x012c, 0x012c, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
+ VF610_PAD_PTD18__NF_IO2 = IOMUX_PAD(0x0130, 0x0130, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
+ VF610_PAD_PTD17__NF_IO1 = IOMUX_PAD(0x0134, 0x0134, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
+ VF610_PAD_PTD16__NF_IO0 = IOMUX_PAD(0x0138, 0x0138, 2, __NA_, 0, VF610_NFC_IO_PAD_CTRL),
+ VF610_PAD_PTB24__NF_WE_B = IOMUX_PAD(0x0178, 0x0178, 5, __NA_, 0, VF610_NFC_CN_PAD_CTRL),
+ VF610_PAD_PTB25__NF_CE0_B = IOMUX_PAD(0x017c, 0x017c, 5, __NA_, 0, VF610_NFC_CN_PAD_CTRL),
+
+ VF610_PAD_PTB27__NF_RE_B = IOMUX_PAD(0x0184, 0x0184, 6, __NA_, 0, VF610_NFC_CN_PAD_CTRL),
+
+ VF610_PAD_PTC26__NF_RB_B = IOMUX_PAD(0x018C, 0x018C, 5, __NA_, 0, VF610_NFC_RB_PAD_CTRL),
+
+ VF610_PAD_PTC27__NF_ALE = IOMUX_PAD(0x0190, 0x0190, 6, __NA_, 0, VF610_NFC_CN_PAD_CTRL),
+
+ VF610_PAD_PTC28__NF_CLE = IOMUX_PAD(0x0194, 0x0194, 6, __NA_, 0, VF610_NFC_CN_PAD_CTRL),
+
VF610_PAD_DDR_A15__DDR_A_15 = IOMUX_PAD(0x0220, 0x0220, 0, __NA_, 0, VF610_DDR_PAD_CTRL),
VF610_PAD_DDR_A14__DDR_A_14 = IOMUX_PAD(0x0224, 0x0224, 0, __NA_, 0, VF610_DDR_PAD_CTRL),
VF610_PAD_DDR_A13__DDR_A_13 = IOMUX_PAD(0x0228, 0x0228, 0, __NA_, 0, VF610_DDR_PAD_CTRL),
diff --git a/arch/arm/include/asm/imx-common/iomux-v3.h b/arch/arm/include/asm/imx-common/iomux-v3.h
index e91d4ac..70ee86c 100644
--- a/arch/arm/include/asm/imx-common/iomux-v3.h
+++ b/arch/arm/include/asm/imx-common/iomux-v3.h
@@ -123,6 +123,8 @@ typedef u64 iomux_v3_cfg_t;
#define PAD_CTL_SPEED_MED (1 << 12)
#define PAD_CTL_SPEED_HIGH (3 << 12)
+#define PAD_CTL_SRE (1 << 11)
+
#define PAD_CTL_DSE_150ohm (1 << 6)
#define PAD_CTL_DSE_50ohm (3 << 6)
#define PAD_CTL_DSE_25ohm (6 << 6)
@@ -135,6 +137,8 @@ typedef u64 iomux_v3_cfg_t;
#define PAD_CTL_PUE (1 << 2 | PAD_CTL_PKE)
#define PAD_CTL_OBE_IBE_ENABLE (3 << 0)
+#define PAD_CTL_OBE_ENABLE (1 << 1)
+#define PAD_CTL_IBE_ENABLE (1 << 0)
#else
--
2.0.4
^ permalink raw reply related [flat|nested] 22+ messages in thread* [U-Boot] [PATCH 2/4] arm: vf610: add NFC clock support
2014-08-06 8:59 [U-Boot] [PATCH 0/4] arm: vf610: add NAND flash support Stefan Agner
2014-08-06 8:59 ` [U-Boot] [PATCH 1/4] arm: vf610: add NFC pin mux Stefan Agner
@ 2014-08-06 8:59 ` Stefan Agner
2014-08-30 15:14 ` [U-Boot] [U-Boot,2/4] " Tom Rini
2014-08-06 8:59 ` [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver Stefan Agner
2014-08-06 8:59 ` [U-Boot] [PATCH 4/4] arm: vf610: add NAND support for vf610twr Stefan Agner
3 siblings, 1 reply; 22+ messages in thread
From: Stefan Agner @ 2014-08-06 8:59 UTC (permalink / raw)
To: u-boot
Add NFC (NAND Flash Controller) clock support and enable them
at board initialization time.
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
arch/arm/include/asm/arch-vf610/crm_regs.h | 14 ++++++++++++++
arch/arm/include/asm/arch-vf610/imx-regs.h | 1 +
2 files changed, 15 insertions(+)
diff --git a/arch/arm/include/asm/arch-vf610/crm_regs.h b/arch/arm/include/asm/arch-vf610/crm_regs.h
index 5256624..724682c 100644
--- a/arch/arm/include/asm/arch-vf610/crm_regs.h
+++ b/arch/arm/include/asm/arch-vf610/crm_regs.h
@@ -156,14 +156,27 @@ struct anadig_reg {
#define CCM_CSCMR1_ESDHC1_CLK_SEL_OFFSET 18
#define CCM_CSCMR1_ESDHC1_CLK_SEL_MASK (0x3 << 18)
#define CCM_CSCMR1_ESDHC1_CLK_SEL(v) (((v) & 0x3) << 18)
+#define CCM_CSCMR1_NFC_CLK_SEL_OFFSET 12
+#define CCM_CSCMR1_NFC_CLK_SEL_MASK (0x3 << 12)
+#define CCM_CSCMR1_NFC_CLK_SEL(v) (((v) & 0x3) << 12)
#define CCM_CSCDR1_RMII_CLK_EN (1 << 24)
+#define CCM_CSCDR2_NFC_EN (1 << 9)
+#define CCM_CSCDR2_NFC_FRAC_DIV_EN (1 << 13)
+#define CCM_CSCDR2_NFC_CLK_INV (1 << 14)
+#define CCM_CSCDR2_NFC_FRAC_DIV_OFFSET 4
+#define CCM_CSCDR2_NFC_FRAC_DIV_MASK (0xf << 4)
+#define CCM_CSCDR2_NFC_FRAC_DIV(v) (((v) & 0xf) << 4)
+
#define CCM_CSCDR2_ESDHC1_EN (1 << 29)
#define CCM_CSCDR2_ESDHC1_CLK_DIV_OFFSET 20
#define CCM_CSCDR2_ESDHC1_CLK_DIV_MASK (0xf << 20)
#define CCM_CSCDR2_ESDHC1_CLK_DIV(v) (((v) & 0xf) << 20)
+#define CCM_CSCDR3_NFC_PRE_DIV_OFFSET 13
+#define CCM_CSCDR3_NFC_PRE_DIV_MASK (0x7 << 13)
+#define CCM_CSCDR3_NFC_PRE_DIV(v) (((v) & 0x7) << 13)
#define CCM_CSCDR3_QSPI0_EN (1 << 4)
#define CCM_CSCDR3_QSPI0_DIV(v) ((v) << 3)
#define CCM_CSCDR3_QSPI0_X2_DIV(v) ((v) << 2)
@@ -195,6 +208,7 @@ struct anadig_reg {
#define CCM_CCGR7_SDHC1_CTRL_MASK (0x3 << 4)
#define CCM_CCGR9_FEC0_CTRL_MASK 0x3
#define CCM_CCGR9_FEC1_CTRL_MASK (0x3 << 2)
+#define CCM_CCGR10_NFC_CTRL_MASK 0x3
#define ANADIG_PLL5_CTRL_BYPASS (1 << 16)
#define ANADIG_PLL5_CTRL_ENABLE (1 << 13)
diff --git a/arch/arm/include/asm/arch-vf610/imx-regs.h b/arch/arm/include/asm/arch-vf610/imx-regs.h
index bd6f680..bb00217 100644
--- a/arch/arm/include/asm/arch-vf610/imx-regs.h
+++ b/arch/arm/include/asm/arch-vf610/imx-regs.h
@@ -86,6 +86,7 @@
#define ESDHC1_BASE_ADDR (AIPS1_BASE_ADDR + 0x00032000)
#define ENET_BASE_ADDR (AIPS1_BASE_ADDR + 0x00050000)
#define ENET1_BASE_ADDR (AIPS1_BASE_ADDR + 0x00051000)
+#define NFC_BASE_ADDR (AIPS1_BASE_ADDR + 0x00060000)
#define QSPI0_AMBA_BASE 0x20000000
--
2.0.4
^ permalink raw reply related [flat|nested] 22+ messages in thread* [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
2014-08-06 8:59 [U-Boot] [PATCH 0/4] arm: vf610: add NAND flash support Stefan Agner
2014-08-06 8:59 ` [U-Boot] [PATCH 1/4] arm: vf610: add NFC pin mux Stefan Agner
2014-08-06 8:59 ` [U-Boot] [PATCH 2/4] arm: vf610: add NFC clock support Stefan Agner
@ 2014-08-06 8:59 ` Stefan Agner
2014-08-06 23:01 ` Bill Pringlemeir
2014-08-11 22:33 ` Scott Wood
2014-08-06 8:59 ` [U-Boot] [PATCH 4/4] arm: vf610: add NAND support for vf610twr Stefan Agner
3 siblings, 2 replies; 22+ messages in thread
From: Stefan Agner @ 2014-08-06 8:59 UTC (permalink / raw)
To: u-boot
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
obj-$(CONFIG_NAND_MXS) += mxs_nand.o
obj-$(CONFIG_NAND_NDFC) += ndfc.o
diff --git a/drivers/mtd/nand/fsl_nfc.c b/drivers/mtd/nand/fsl_nfc.c
new file mode 100644
index 0000000..df2c8be
--- /dev/null
+++ b/drivers/mtd/nand/fsl_nfc.c
@@ -0,0 +1,676 @@
+/*
+ * Copyright 2009-2014 Freescale Semiconductor, Inc. and others
+ *
+ * Description: MPC5125, VF610, MCF54418 and Kinetis K70 Nand driver.
+ * Ported to U-Boot by Stefan Agner
+ * Based on RFC driver posted on Kernel Mailing list by Bill Pringlemeir
+ * Jason ported to M54418TWR and MVFA5.
+ * Authors: Stefan Agner <stefan.agner@toradex.com>
+ * Bill Pringlemeir <bpringlemeir@nbsps.com>
+ * Shaohui Xie <b21989@freescale.com>
+ * Jason Jin <Jason.jin@freescale.com>
+ *
+ * Based on original driver mpc5121_nfc.c.
+ *
+ * This is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * Limitations:
+ * - Untested on MPC5125 and M54418.
+ * - DMA not used.
+ * - 2K pages or less.
+ * - Only 2K page w. 64+OOB and hardware ECC.
+ */
+
+#include <common.h>
+#include <malloc.h>
+
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/partitions.h>
+
+#include <nand.h>
+#include <errno.h>
+#include <asm/io.h>
+#include <asm/processor.h>
+
+#define DRV_NAME "fsl_nfc"
+
+/* Register Offsets */
+#define NFC_FLASH_CMD1 0x3F00
+#define NFC_FLASH_CMD2 0x3F04
+#define NFC_COL_ADDR 0x3F08
+#define NFC_ROW_ADDR 0x3F0c
+#define NFC_ROW_ADDR_INC 0x3F14
+#define NFC_FLASH_STATUS1 0x3F18
+#define NFC_FLASH_STATUS2 0x3F1c
+#define NFC_CACHE_SWAP 0x3F28
+#define NFC_SECTOR_SIZE 0x3F2c
+#define NFC_FLASH_CONFIG 0x3F30
+#define NFC_IRQ_STATUS 0x3F38
+
+/* Addresses for NFC MAIN RAM BUFFER areas */
+#define NFC_MAIN_AREA(n) ((n) * 0x1000)
+
+#define PAGE_2K 0x0800
+#define OOB_64 0x0040
+
+/*
+ * NFC_CMD2[CODE] values. See section:
+ * - 31.4.7 Flash Command Code Description, Vybrid manual
+ * - 23.8.6 Flash Command Sequencer, MPC5125 manual
+ *
+ * Briefly these are bitmasks of controller cycles.
+ */
+#define READ_PAGE_CMD_CODE 0x7EE0
+#define PROGRAM_PAGE_CMD_CODE 0x7FC0
+#define ERASE_CMD_CODE 0x4EC0
+#define READ_ID_CMD_CODE 0x4804
+#define RESET_CMD_CODE 0x4040
+#define STATUS_READ_CMD_CODE 0x4068
+
+/* NFC ECC mode define */
+#define ECC_BYPASS 0
+#define ECC_45_BYTE 6
+
+/*** Register Mask and bit definitions */
+
+/* NFC_FLASH_CMD1 Field */
+#define CMD_BYTE2_MASK 0xFF000000
+#define CMD_BYTE2_SHIFT 24
+
+/* NFC_FLASH_CM2 Field */
+#define CMD_BYTE1_MASK 0xFF000000
+#define CMD_BYTE1_SHIFT 24
+#define CMD_CODE_MASK 0x00FFFF00
+#define CMD_CODE_SHIFT 8
+#define BUFNO_MASK 0x00000006
+#define BUFNO_SHIFT 1
+#define START_BIT (1<<0)
+
+/* NFC_COL_ADDR Field */
+#define COL_ADDR_MASK 0x0000FFFF
+#define COL_ADDR_SHIFT 0
+
+/* NFC_ROW_ADDR Field */
+#define ROW_ADDR_MASK 0x00FFFFFF
+#define ROW_ADDR_SHIFT 0
+#define ROW_ADDR_CHIP_SEL_RB_MASK 0xF0000000
+#define ROW_ADDR_CHIP_SEL_RB_SHIFT 28
+#define ROW_ADDR_CHIP_SEL_MASK 0x0F000000
+#define ROW_ADDR_CHIP_SEL_SHIFT 24
+
+/* NFC_FLASH_STATUS2 Field */
+#define STATUS_BYTE1_MASK 0x000000FF
+
+/* NFC_FLASH_CONFIG Field */
+#define CONFIG_ECC_SRAM_ADDR_MASK 0x7FC00000
+#define CONFIG_ECC_SRAM_ADDR_SHIFT 22
+#define CONFIG_ECC_SRAM_REQ_BIT (1<<21)
+#define CONFIG_DMA_REQ_BIT (1<<20)
+#define CONFIG_ECC_MODE_MASK 0x000E0000
+#define CONFIG_ECC_MODE_SHIFT 17
+#define CONFIG_FAST_FLASH_BIT (1<<16)
+#define CONFIG_16BIT (1<<7)
+#define CONFIG_BOOT_MODE_BIT (1<<6)
+#define CONFIG_ADDR_AUTO_INCR_BIT (1<<5)
+#define CONFIG_BUFNO_AUTO_INCR_BIT (1<<4)
+#define CONFIG_PAGE_CNT_MASK 0xF
+#define CONFIG_PAGE_CNT_SHIFT 0
+
+/* NFC_IRQ_STATUS Field */
+#define IDLE_IRQ_BIT (1<<29)
+#define IDLE_EN_BIT (1<<20)
+#define CMD_DONE_CLEAR_BIT (1<<18)
+#define IDLE_CLEAR_BIT (1<<17)
+
+#define NFC_TIMEOUT (1000)
+
+/* ECC status placed@end of buffers. */
+#define ECC_SRAM_ADDR ((PAGE_2K+256-8) >> 3)
+#define ECC_STATUS_MASK 0x80
+#define ECC_ERR_COUNT 0x3F
+
+struct fsl_nfc {
+ struct mtd_info *mtd;
+ struct nand_chip chip;
+ struct device *dev;
+ void __iomem *regs;
+ //wait_queue_head_t irq_waitq;
+ 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;
+
+static u8 bbt_pattern[] = {'B', 'b', 't', '0' };
+static u8 mirror_pattern[] = {'1', 't', 'b', 'B' };
+
+static struct nand_bbt_descr bbt_main_descr = {
+ .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE |
+ NAND_BBT_2BIT | NAND_BBT_VERSION,
+ .offs = 11,
+ .len = 4,
+ .veroffs = 15,
+ .maxblocks = 4,
+ .pattern = bbt_pattern,
+};
+
+static struct nand_bbt_descr bbt_mirror_descr = {
+ .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE |
+ NAND_BBT_2BIT | NAND_BBT_VERSION,
+ .offs = 11,
+ .len = 4,
+ .veroffs = 15,
+ .maxblocks = 4,
+ .pattern = mirror_pattern,
+};
+
+static struct nand_ecclayout nfc_ecc45 = {
+ .eccbytes = 45,
+ .eccpos = {19, 20, 21, 22, 23,
+ 24, 25, 26, 27, 28, 29, 30, 31,
+ 32, 33, 34, 35, 36, 37, 38, 39,
+ 40, 41, 42, 43, 44, 45, 46, 47,
+ 48, 49, 50, 51, 52, 53, 54, 55,
+ 56, 57, 58, 59, 60, 61, 62, 63},
+ .oobfree = {
+ {.offset = 8,
+ .length = 11} }
+};
+
+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;
+}
+
+static void nfc_set(struct mtd_info *mtd, uint reg, u32 bits)
+{
+ nfc_write(mtd, reg, nfc_read(mtd, reg) | bits);
+}
+
+static void nfc_clear(struct mtd_info *mtd, uint reg, u32 bits)
+{
+ nfc_write(mtd, reg, nfc_read(mtd, reg) & ~bits);
+}
+
+static void nfc_set_field(struct mtd_info *mtd, u32 reg,
+ u32 mask, u32 shift, u32 val)
+{
+ nfc_write(mtd, reg, (nfc_read(mtd, reg) & (~mask)) | val << shift);
+}
+
+/* Clear flags for upcoming command */
+static void nfc_clear_status(struct mtd_info *mtd)
+{
+ nfc_set(mtd, NFC_IRQ_STATUS, CMD_DONE_CLEAR_BIT);
+ nfc_set(mtd, NFC_IRQ_STATUS, IDLE_CLEAR_BIT);
+}
+
+/* Wait for complete operation */
+static void nfc_done(struct mtd_info *mtd)
+{
+ uint start;
+
+ nfc_set(mtd, NFC_FLASH_CMD2, START_BIT);
+ barrier();
+
+ start = get_timer(0);
+
+ while (!(nfc_read(mtd, NFC_IRQ_STATUS) & IDLE_IRQ_BIT)) {
+ if (get_timer(start) > NFC_TIMEOUT)
+ printf("Timeout while waiting for BUSY.\n");
+ }
+ nfc_clear_status(mtd);
+}
+
+static u8 nfc_get_id(struct mtd_info *mtd, int col)
+{
+ u32 flash_id;
+
+ if (col < 4) {
+ flash_id = nfc_read(mtd, NFC_FLASH_STATUS1);
+ return (flash_id >> (3-col)*8) & 0xff;
+ } else {
+ flash_id = nfc_read(mtd, NFC_FLASH_STATUS2);
+ return flash_id >> 24;
+ }
+}
+
+static u8 nfc_get_status(struct mtd_info *mtd)
+{
+ return nfc_read(mtd, NFC_FLASH_STATUS2) & STATUS_BYTE1_MASK;
+}
+
+/* Single command */
+static void nfc_send_command(struct mtd_info *mtd, u32 cmd_byte1,
+ u32 cmd_code)
+{
+ nfc_clear_status(mtd);
+
+ nfc_set_field(mtd, NFC_FLASH_CMD2, CMD_BYTE1_MASK,
+ CMD_BYTE1_SHIFT, cmd_byte1);
+
+ nfc_clear(mtd, NFC_FLASH_CMD2, BUFNO_MASK);
+
+ nfc_set_field(mtd, NFC_FLASH_CMD2, CMD_CODE_MASK,
+ CMD_CODE_SHIFT, cmd_code);
+}
+
+/* Two commands */
+static void nfc_send_commands(struct mtd_info *mtd, u32 cmd_byte1,
+ u32 cmd_byte2, u32 cmd_code)
+{
+ nfc_send_command(mtd, cmd_byte1, cmd_code);
+
+ nfc_set_field(mtd, NFC_FLASH_CMD1, CMD_BYTE2_MASK,
+ CMD_BYTE2_SHIFT, cmd_byte2);
+}
+
+static void nfc_addr_cycle(struct mtd_info *mtd, int column, int page)
+{
+ if (column != -1) {
+ struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+ if (nfc->chip.options | NAND_BUSWIDTH_16)
+ column = column/2;
+ nfc_set_field(mtd, NFC_COL_ADDR, COL_ADDR_MASK,
+ COL_ADDR_SHIFT, column);
+ }
+ if (page != -1)
+ nfc_set_field(mtd, NFC_ROW_ADDR, ROW_ADDR_MASK,
+ ROW_ADDR_SHIFT, page);
+}
+
+/* Send command to NAND chip */
+static void nfc_command(struct mtd_info *mtd, unsigned command,
+ int column, int page)
+{
+ struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+
+ nfc->column = max(column, 0);
+ nfc->spareonly = 0;
+ nfc->alt_buf = 0;
+
+ switch (command) {
+ case NAND_CMD_PAGEPROG:
+ nfc->page = -1;
+ nfc_send_commands(mtd, NAND_CMD_SEQIN,
+ command, PROGRAM_PAGE_CMD_CODE);
+ nfc_addr_cycle(mtd, column, page);
+ break;
+
+ case NAND_CMD_RESET:
+ nfc_send_command(mtd, command, RESET_CMD_CODE);
+ break;
+ /*
+ * NFC does not support sub-page reads and writes,
+ * so emulate them using full page transfers.
+ */
+ case NAND_CMD_READOOB:
+ nfc->spareonly = 1;
+ case NAND_CMD_SEQIN: /* Pre-read for partial writes. */
+ case NAND_CMD_READ0:
+ column = 0;
+ /* Already read? */
+ if (nfc->page == page)
+ return;
+ nfc->page = page;
+ nfc_send_commands(mtd, NAND_CMD_READ0,
+ NAND_CMD_READSTART, READ_PAGE_CMD_CODE);
+ nfc_addr_cycle(mtd, column, page);
+ break;
+
+ case NAND_CMD_ERASE1:
+ if (nfc->page == page)
+ nfc->page = -1;
+ nfc_send_commands(mtd, command,
+ NAND_CMD_ERASE2, ERASE_CMD_CODE);
+ nfc_addr_cycle(mtd, column, page);
+ break;
+
+ case NAND_CMD_READID:
+ nfc->alt_buf = ALT_BUF_ID;
+ nfc_send_command(mtd, command, READ_ID_CMD_CODE);
+ break;
+
+ case NAND_CMD_STATUS:
+ nfc->alt_buf = ALT_BUF_STAT;
+ nfc_send_command(mtd, command, STATUS_READ_CMD_CODE);
+ break;
+ default:
+ return;
+ }
+
+ nfc_done(mtd);
+}
+
+static void nfc_read_spare(struct mtd_info *mtd, void *buf, int len)
+{
+ struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+
+ len = min(mtd->oobsize, (uint)len);
+ if (len > 0)
+ memcpy(buf, nfc->regs + mtd->writesize, len);
+}
+
+/* Read data from NFC buffers */
+static void nfc_read_buf(struct mtd_info *mtd, u_char *buf, int len)
+{
+ struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+ uint c = nfc->column;
+ uint l;
+
+ /* Handle main area */
+ if (!nfc->spareonly) {
+
+ l = min((uint)len, mtd->writesize - c);
+ nfc->column += l;
+
+ if (!nfc->alt_buf)
+ memcpy(buf, nfc->regs + NFC_MAIN_AREA(0) + c, l);
+ else
+ if (nfc->alt_buf & ALT_BUF_ID)
+ *buf = nfc_get_id(mtd, c);
+ else
+ *buf = nfc_get_status(mtd);
+ buf += l;
+ len -= l;
+ }
+
+ /* Handle spare area access */
+ if (len) {
+ nfc->column += len;
+ nfc_read_spare(mtd, buf, len);
+ }
+}
+
+/* Write data to NFC buffers */
+static void nfc_write_buf(struct mtd_info *mtd, const u_char *buf, int len)
+{
+ struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+ uint c = nfc->column;
+ uint l;
+
+ l = min((uint)len, mtd->writesize + mtd->oobsize - c);
+ nfc->column += l;
+ memcpy(nfc->regs + NFC_MAIN_AREA(0) + c, buf, l);
+}
+
+/* Read byte from NFC buffers */
+static u8 nfc_read_byte(struct mtd_info *mtd)
+{
+ u8 tmp;
+ nfc_read_buf(mtd, &tmp, sizeof(tmp));
+ return tmp;
+}
+
+/* Read word from NFC buffers */
+static u16 nfc_read_word(struct mtd_info *mtd)
+{
+ u16 tmp;
+ nfc_read_buf(mtd, (u_char *)&tmp, sizeof(tmp));
+ return tmp;
+}
+
+/* If not provided, upper layers apply a fixed delay. */
+static int nfc_dev_ready(struct mtd_info *mtd)
+{
+ /* NFC handles R/B internally; always ready. */
+ return 1;
+}
+
+/*
+ * Vybrid only. MPC5125 has full RB and four CS. Assume boot loader
+ * has set this register for now.
+ */
+static void
+nfc_select_chip(struct mtd_info *mtd, int chip)
+{
+#ifdef CONFIG_VF610
+ nfc_set_field(mtd, NFC_ROW_ADDR, ROW_ADDR_CHIP_SEL_RB_MASK,
+ ROW_ADDR_CHIP_SEL_RB_SHIFT, 1);
+
+ if (chip == 0)
+ nfc_set_field(mtd, NFC_ROW_ADDR, ROW_ADDR_CHIP_SEL_MASK,
+ ROW_ADDR_CHIP_SEL_SHIFT, 1);
+ else if (chip == 1)
+ nfc_set_field(mtd, NFC_ROW_ADDR, ROW_ADDR_CHIP_SEL_MASK,
+ ROW_ADDR_CHIP_SEL_SHIFT, 2);
+ else
+ nfc_clear(mtd, NFC_ROW_ADDR, ROW_ADDR_CHIP_SEL_MASK);
+#endif
+}
+
+/* Count the number of 0's in buff upto max_bits */
+static int count_written_bits(uint8_t *buff, int size, int max_bits)
+{
+ int k, written_bits = 0;
+
+ for (k = 0; k < size; k++) {
+ written_bits += hweight8(~buff[k]);
+ if (written_bits > max_bits)
+ break;
+ }
+
+ return written_bits;
+}
+
+static int nfc_correct_data(struct mtd_info *mtd, u_char *dat,
+ u_char *read_ecc, u_char *calc_ecc)
+{
+ struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+ u32 ecc_status;
+ u8 ecc_count;
+ int flip;
+
+ /*
+ * Errata: ECC status is stored at NFC_CFG[ECCADD] +4 for
+ * little-endian and +7 for big-endian SOC. Access as 32 bits
+ * and use low byte.
+ */
+ 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;
+
+ /* If 'ecc_count' zero or less then buffer is all 0xff or erased. */
+ flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
+
+ /* ECC failed. */
+ if (flip > ecc_count)
+ return -1;
+
+ /* Erased page. */
+ memset(dat, 0xff, nfc->chip.ecc.size);
+ return 0;
+}
+
+static int nfc_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
+ u_char *ecc_code)
+{
+ return 0;
+}
+
+static void nfc_enable_hwecc(struct mtd_info *mtd, int mode)
+{
+}
+
+struct nfc_config {
+ int hardware_ecc;
+ int width;
+ int flash_bbt;
+};
+
+//static int nfc_probe(struct platform_device *pdev)
+int board_nand_init(struct nand_chip *chip)
+{
+ struct fsl_nfc *nfc;
+ struct mtd_info *mtd;
+ uint chips_no = 0;
+ int err = 0;
+ int page_sz;
+ struct nfc_config cfg = {
+ .hardware_ecc = 1,
+#ifdef CONFIG_SYS_NAND_BUSWIDTH_16BIT
+ .width = 16,
+#else
+ .width = 8,
+#endif
+ .flash_bbt = 1,
+ };
+
+ if (chip->IO_ADDR_R == NULL)
+ return -1;
+
+ nfc = malloc(sizeof(*nfc));
+ if (!nfc) {
+ printf(KERN_ERR DRV_NAME ": Memory exhausted!\n");
+ return -ENOMEM;
+ }
+
+ nfc->regs = (void __iomem *)chip->IO_ADDR_R; //CONFIG_SYS_NAND_BASE;
+
+ mtd = &nand_info[chips_no++];
+ mtd->priv = chip;
+ chip->priv = nfc;
+
+ if (cfg.width == 16) {
+ chip->options |= NAND_BUSWIDTH_16;
+ nfc_set(mtd, NFC_FLASH_CONFIG, CONFIG_16BIT);
+ } else {
+ chip->options &= ~NAND_BUSWIDTH_16;
+ nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_16BIT);
+ }
+
+ chip->dev_ready = nfc_dev_ready;
+ chip->cmdfunc = nfc_command;
+ chip->read_byte = nfc_read_byte;
+ chip->read_word = nfc_read_word;
+ chip->read_buf = nfc_read_buf;
+ chip->write_buf = nfc_write_buf;
+ chip->select_chip = nfc_select_chip;
+
+ /* Bad block options. */
+ if (cfg.flash_bbt)
+ chip->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_CREATE;
+
+ /* Default to software ECC until flash ID. */
+ nfc_set_field(mtd, NFC_FLASH_CONFIG,
+ CONFIG_ECC_MODE_MASK,
+ CONFIG_ECC_MODE_SHIFT, ECC_BYPASS);
+
+ chip->bbt_td = &bbt_main_descr;
+ chip->bbt_md = &bbt_mirror_descr;
+
+ page_sz = PAGE_2K + OOB_64;
+ page_sz += cfg.width == 16 ? 1 : 0;
+ nfc_write(mtd, NFC_SECTOR_SIZE, page_sz);
+
+ /* 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);
+
+ /* Enable Idle IRQ */
+ nfc_set(mtd, NFC_IRQ_STATUS, IDLE_EN_BIT);
+
+ /* PAGE_CNT = 1 */
+ nfc_set_field(mtd, NFC_FLASH_CONFIG, CONFIG_PAGE_CNT_MASK,
+ CONFIG_PAGE_CNT_SHIFT, 1);
+
+ /* Set ECC_STATUS offset */
+ nfc_set_field(mtd, NFC_FLASH_CONFIG,
+ CONFIG_ECC_SRAM_ADDR_MASK,
+ CONFIG_ECC_SRAM_ADDR_SHIFT, ECC_SRAM_ADDR);
+
+ /* first scan to find the device and get the page size */
+ if (nand_scan_ident(mtd, CONFIG_SYS_MAX_NAND_DEVICE, NULL)) {
+ err = -ENXIO;
+ goto error;
+ }
+
+ chip->ecc.mode = NAND_ECC_SOFT; /* default */
+
+ page_sz = mtd->writesize + mtd->oobsize;
+
+ /* Single buffer only, max 256 OOB minus ECC status */
+ if (page_sz > PAGE_2K + 256 - 8) {
+ dev_err(nfc->dev, "Unsupported flash size\n");
+ err = -ENXIO;
+ goto error;
+ }
+ page_sz += cfg.width == 16 ? 1 : 0;
+ nfc_write(mtd, NFC_SECTOR_SIZE, page_sz);
+
+ if (cfg.hardware_ecc) {
+ if (mtd->writesize != PAGE_2K && mtd->oobsize < 64) {
+ dev_err(nfc->dev, "Unsupported flash with hwecc\n");
+ err = -ENXIO;
+ goto error;
+ }
+
+ chip->ecc.layout = &nfc_ecc45;
+
+ /* propagate ecc.layout to mtd_info */
+ mtd->ecclayout = chip->ecc.layout;
+ chip->ecc.calculate = nfc_calculate_ecc;
+ chip->ecc.hwctl = nfc_enable_hwecc;
+ chip->ecc.correct = nfc_correct_data;
+ chip->ecc.mode = NAND_ECC_HW;
+
+ chip->ecc.bytes = 45;
+ chip->ecc.size = PAGE_2K;
+ chip->ecc.strength = 24;
+
+ /* set ECC mode to 45 bytes OOB with 24 bits correction */
+ nfc_set_field(mtd, NFC_FLASH_CONFIG,
+ CONFIG_ECC_MODE_MASK,
+ CONFIG_ECC_MODE_SHIFT, ECC_45_BYTE);
+
+ /* Enable ECC_STATUS */
+ nfc_set(mtd, NFC_FLASH_CONFIG, CONFIG_ECC_SRAM_REQ_BIT);
+
+ }
+
+ /* second phase scan */
+ if (nand_scan_tail(mtd)) {
+ err = -ENXIO;
+ goto error;
+ }
+
+ return 0;
+
+error:
+ return err;
+}
--
2.0.4
^ permalink raw reply related [flat|nested] 22+ messages in thread* [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
2014-08-06 8:59 ` [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver Stefan Agner
@ 2014-08-06 23:01 ` Bill Pringlemeir
2014-08-11 22:33 ` Scott Wood
1 sibling, 0 replies; 22+ messages in thread
From: Bill Pringlemeir @ 2014-08-06 23:01 UTC (permalink / raw)
To: u-boot
On 6 Aug 2014, stefan at agner.ch 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/fsl_nfc.c b/drivers/mtd/nand/fsl_nfc.c
> new file mode 100644
> index 0000000..df2c8be
> --- /dev/null
> +++ b/drivers/mtd/nand/fsl_nfc.c
> @@ -0,0 +1,676 @@
[snip]
> +#define NFC_TIMEOUT (1000)
> +
> +/* ECC status placed at end of buffers. */
> +#define ECC_SRAM_ADDR ((PAGE_2K+256-8) >> 3)
> +#define ECC_STATUS_MASK 0x80
> +#define ECC_ERR_COUNT 0x3F
> +
> +struct fsl_nfc {
> + struct mtd_info *mtd;
> + struct nand_chip chip;
> + struct device *dev;
> + void __iomem *regs;
> + //wait_queue_head_t irq_waitq;
I think u-boot doesn't like C++ comments?
> + 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;
[snip]
> +static int nfc_correct_data(struct mtd_info *mtd, u_char *dat,
> + u_char *read_ecc, u_char *calc_ecc)
> +{
> + struct fsl_nfc *nfc = mtd_to_nfc(mtd);
> + u32 ecc_status;
> + u8 ecc_count;
> + int flip;
> +
> + /*
Some extra ws here Ie, '/* ' with a space after the star. Maybe that
is from me?
> + * Errata: ECC status is stored at NFC_CFG[ECCADD] +4 for
> + * little-endian and +7 for big-endian SOC. Access as 32 bits
> + * and use low byte.
> + */
This appears to be documented in the latest Vybrid manual (Rev7).
> + 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;
[snip]
> +static void nfc_enable_hwecc(struct mtd_info *mtd, int mode)
> +{
> +}
> +
> +struct nfc_config {
> + int hardware_ecc;
> + int width;
> + int flash_bbt;
> +};
> +
> +//static int nfc_probe(struct platform_device *pdev)
Another C++ comment.
All minor nits. I am certainly ok with this code being included in
u-boot.
Regards,
Bill Pringlemeir.
^ permalink raw reply [flat|nested] 22+ messages in thread* [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
2014-08-06 8:59 ` [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver Stefan Agner
2014-08-06 23:01 ` Bill Pringlemeir
@ 2014-08-11 22:33 ` Scott Wood
2014-08-12 21:13 ` Stefan Agner
1 sibling, 1 reply; 22+ messages in thread
From: Scott Wood @ 2014-08-11 22:33 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 22+ messages in thread* [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
2014-08-11 22:33 ` Scott Wood
@ 2014-08-12 21:13 ` Stefan Agner
2014-08-12 22:17 ` Scott Wood
0 siblings, 1 reply; 22+ messages in thread
From: Stefan Agner @ 2014-08-12 21:13 UTC (permalink / raw)
To: u-boot
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).
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.
>> +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.
>> +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.
--
Stefan
^ permalink raw reply [flat|nested] 22+ messages in thread* [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
2014-08-12 21:13 ` Stefan Agner
@ 2014-08-12 22:17 ` Scott Wood
2014-08-12 22:58 ` Bill Pringlemeir
0 siblings, 1 reply; 22+ messages in thread
From: Scott Wood @ 2014-08-12 22:17 UTC (permalink / raw)
To: u-boot
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.
> 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
^ permalink raw reply [flat|nested] 22+ messages in thread* [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
2014-08-12 22:17 ` Scott Wood
@ 2014-08-12 22:58 ` Bill Pringlemeir
2014-08-13 8:13 ` Stefan Agner
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: Bill Pringlemeir @ 2014-08-12 22:58 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 22+ messages in thread* [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
2014-08-12 22:58 ` Bill Pringlemeir
@ 2014-08-13 8:13 ` Stefan Agner
2014-08-13 11:20 ` Stefan Agner
2014-08-13 20:41 ` Scott Wood
2 siblings, 0 replies; 22+ messages in thread
From: Stefan Agner @ 2014-08-13 8:13 UTC (permalink / raw)
To: u-boot
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@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.
^ permalink raw reply [flat|nested] 22+ messages in thread* [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
2014-08-12 22:58 ` Bill Pringlemeir
2014-08-13 8:13 ` Stefan Agner
@ 2014-08-13 11:20 ` Stefan Agner
2014-08-13 15:14 ` Bill Pringlemeir
2014-08-13 20:32 ` Scott Wood
2014-08-13 20:41 ` Scott Wood
2 siblings, 2 replies; 22+ messages in thread
From: Stefan Agner @ 2014-08-13 11:20 UTC (permalink / raw)
To: u-boot
Am 2014-08-13 00:58, schrieb Bill Pringlemeir:
[snip]
>>>>> +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 just applied this change:
diff --git a/drivers/mtd/nand/fsl_nfc.c b/drivers/mtd/nand/fsl_nfc.c
index df2c8be..01c010f 100644
--- a/drivers/mtd/nand/fsl_nfc.c
+++ b/drivers/mtd/nand/fsl_nfc.c
@@ -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.
--
Stefan
^ permalink raw reply related [flat|nested] 22+ messages in thread* [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
2014-08-13 11:20 ` Stefan Agner
@ 2014-08-13 15:14 ` Bill Pringlemeir
2014-08-13 16:27 ` Stefan Agner
2014-08-13 20:32 ` Scott Wood
1 sibling, 1 reply; 22+ messages in thread
From: Bill Pringlemeir @ 2014-08-13 15:14 UTC (permalink / raw)
To: u-boot
On 13 Aug 2014, stefan at agner.ch wrote:
> Am 2014-08-13 00:58, schrieb Bill Pringlemeir:
> [snip]
>>>>>> +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 just applied this change:
>
> diff --git a/drivers/mtd/nand/fsl_nfc.c b/drivers/mtd/nand/fsl_nfc.c
> index df2c8be..01c010f 100644
> --- a/drivers/mtd/nand/fsl_nfc.c
> +++ b/drivers/mtd/nand/fsl_nfc.c
> @@ -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.
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).
Regards,
Bill Pringlemeir.
^ permalink raw reply [flat|nested] 22+ messages in thread* [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
2014-08-13 15:14 ` Bill Pringlemeir
@ 2014-08-13 16:27 ` Stefan Agner
2014-08-13 17:11 ` Bill Pringlemeir
0 siblings, 1 reply; 22+ messages in thread
From: Stefan Agner @ 2014-08-13 16:27 UTC (permalink / raw)
To: u-boot
Am 2014-08-13 17:14, schrieb Bill Pringlemeir:
> On 13 Aug 2014, stefan at agner.ch wrote:
>
>> Am 2014-08-13 00:58, schrieb Bill Pringlemeir:
>> [snip]
>>>>>>> +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 just applied this change:
>>
>> diff --git a/drivers/mtd/nand/fsl_nfc.c b/drivers/mtd/nand/fsl_nfc.c
>> index df2c8be..01c010f 100644
>> --- a/drivers/mtd/nand/fsl_nfc.c
>> +++ b/drivers/mtd/nand/fsl_nfc.c
>> @@ -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.
>
> 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.
As far as I understood the RM, we would have multiple buffers? So we
could get the next page while moving the last to main memory (by DMA or
CPU). However, I'm not sure whether such an operation is possible when
using the MTD interface.. This probably makes more sense for the Linux
driver anyway.
> 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).
The inlining is indeed the most prominent factor for this performance
difference. I use a GCC 4.7.4. I also think that U-Boot uses different
optimization flags then the kernel. However, in my setup the compiler
did not inline nfc_read/write default. When removing the if/else, the
inlining was done by GCC 4.7.4
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
${CROSS_COMPILE}objdump -d drivers/mtd/nand/fsl_nfc.o | grep nfc_write
Disassembly of section .text.nfc_write:
00000000 <nfc_write>:
....
Without if/else, in-lined by the compiler...
text data bss dec hex filename
2263 2904 0 5167 142f drivers/mtd/nand/fsl_nfc.o
I then tried explicitly inlining with if/else, I got more or less the
same performance.
My next version will not contain the if/else and access through
writel/readl exclusively. I also explicitly inlined those functions
since this made the measurable difference in performance. Some hand
optimization for the register access of the most important functions
(nfc_send_command(s)) showed a slight improvement. This way we have
easily readable code in initialization functions and get good
performance.
=> nand read ${loadaddr} 0x0 0x2000000
NAND read: device 0 offset 0x0, size 0x2000000
33554432 bytes read in 8032 ms: OK (4 MiB/s)
text data bss dec hex filename
2000 2904 0 4904 1328 drivers/mtd/nand/fsl_nfc.o
--
Stefan
^ permalink raw reply [flat|nested] 22+ messages in thread* [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
2014-08-13 16:27 ` Stefan Agner
@ 2014-08-13 17:11 ` Bill Pringlemeir
0 siblings, 0 replies; 22+ messages in thread
From: Bill Pringlemeir @ 2014-08-13 17:11 UTC (permalink / raw)
To: u-boot
On 13 Aug 2014, stefan at agner.ch wrote:
> 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
This is totally sensible. In some cases, the function epilogue and
prologue are actually bigger than the function body. Also, the implicit
call means that the caller must save some temporary registers (R0-R3).
For the simple nfc_read() and nfc_write(), I would expect the size to
get bigger if they are not inlined. Especially, gcc can recognize that
the same memory location is being operated on and collapse the
accesses.
Anyways, thanks for showing that the previous code was depending too
much on compiler knowledge. Your current plan sounds promising.
Regards,
Bill Pringlemeir.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
2014-08-13 11:20 ` Stefan Agner
2014-08-13 15:14 ` Bill Pringlemeir
@ 2014-08-13 20:32 ` Scott Wood
1 sibling, 0 replies; 22+ messages in thread
From: Scott Wood @ 2014-08-13 20:32 UTC (permalink / raw)
To: u-boot
On Wed, 2014-08-13 at 13:20 +0200, Stefan Agner wrote:
> 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.
raw_writel() is itself something that should only be used for
hand-optimized sections. For non-performance-critical code you should
use normal writel() so that you don't need to worry about manually
adding I/O barriers.
-Scott
^ permalink raw reply [flat|nested] 22+ messages in thread
* [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
2014-08-12 22:58 ` Bill Pringlemeir
2014-08-13 8:13 ` Stefan Agner
2014-08-13 11:20 ` Stefan Agner
@ 2014-08-13 20:41 ` Scott Wood
2014-08-13 21:44 ` Bill Pringlemeir
2 siblings, 1 reply; 22+ messages in thread
From: Scott Wood @ 2014-08-13 20:41 UTC (permalink / raw)
To: u-boot
On Tue, 2014-08-12 at 18:58 -0400, Bill Pringlemeir wrote:
> 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:
> >>> 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.
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.
-Scott
^ permalink raw reply [flat|nested] 22+ messages in thread
* [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
2014-08-13 20:41 ` Scott Wood
@ 2014-08-13 21:44 ` Bill Pringlemeir
2014-08-13 22:54 ` Scott Wood
0 siblings, 1 reply; 22+ messages in thread
From: Bill Pringlemeir @ 2014-08-13 21:44 UTC (permalink / raw)
To: u-boot
On 13 Aug 2014, scottwood at freescale.com wrote:
> On Tue, 2014-08-12 at 18:58 -0400, Bill Pringlemeir wrote:
>> 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:
>>>>> 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.
> 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.
Yes, absolutely. The issue is that the 'nfc_clear()', etc seems more
programmer friendly to me. It was from an original driver. I believe
this is what Stefan means by 'optimized' and not 'raw_readl/raw_writel'
versus 'readl/writel'.
I believe that this is smaller/faster in all cases; not just hot paths
as the code should be smaller. I tried to modify the accessor to leave
the original code as is. The registers are a bunch of bit fields. It
is clear to read them each as being set/cleared on a different lines?
However, the whole group can be set at once at the machine level.
Sometimes the bit fields aren't so related. So, if you are trying to
write the code about what is happening to the NAND flash (Ie cycles to
run) then ganging the NFC controller registers together can make things
a little more obscure. Maybe this is a small price to pay if the
register ordering is more of an issue to people.
The accesses are generally all order independent *when idle*. There is
one bit of the NFC_FLASH_CMD2 register as bit0 or START_BIT in the code.
When it is toggled, the NFC controller starts it's business and then
only a few register can be polled or an interrupt generated. In this
phase, no register changes should take place or at least care should be
taken.
I have only compiler barriers in the driver I submitted to the Linux
MTD. It is possible that the PowerPC or other multi-issue CPUs might
need the iowmb() or otherwise when the 'START_BIT' is toggled. I had
thought of this in the mean time, so thank-you for bringing it up.
Allowing the compiler to re-order the settings of the registers when
idle can let it decide to do all the zeroing at once, etc depending on
what is optimal. The 'read/modify many/write' is a happy medium for me.
Minimizing accesses to the registers is important as these often involve
a slow bus interface and also generate a lot of code for load/store
CPUs.
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.
Thanks,
Bill Pringlemeir.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
2014-08-13 21:44 ` Bill Pringlemeir
@ 2014-08-13 22:54 ` Scott Wood
2014-08-14 14:26 ` Bill Pringlemeir
0 siblings, 1 reply; 22+ messages in thread
From: Scott Wood @ 2014-08-13 22:54 UTC (permalink / raw)
To: u-boot
On Wed, 2014-08-13 at 17:44 -0400, Bill Pringlemeir wrote:
> 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.
-Scott
^ permalink raw reply [flat|nested] 22+ messages in thread
* [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver
2014-08-13 22:54 ` Scott Wood
@ 2014-08-14 14:26 ` Bill Pringlemeir
0 siblings, 0 replies; 22+ messages in thread
From: Bill Pringlemeir @ 2014-08-14 14:26 UTC (permalink / raw)
To: u-boot
On 13 Aug 2014, scottwood at freescale.com wrote:
> On Wed, 2014-08-13 at 17:44 -0400, Bill Pringlemeir wrote:
>> 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.
Yes, that is a good point. The regmap interface could be used in the
Linux kernel. I don't see any infrastructure like that in U-Boot? It
is fairly simple to re-invent as this driver only needs the flat
structure.
Regards,
Bill Pringlemeir.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [U-Boot] [PATCH 4/4] arm: vf610: add NAND support for vf610twr
2014-08-06 8:59 [U-Boot] [PATCH 0/4] arm: vf610: add NAND flash support Stefan Agner
` (2 preceding siblings ...)
2014-08-06 8:59 ` [U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver Stefan Agner
@ 2014-08-06 8:59 ` Stefan Agner
3 siblings, 0 replies; 22+ messages in thread
From: Stefan Agner @ 2014-08-06 8:59 UTC (permalink / raw)
To: u-boot
This adds NAND Flash Controller (NFC) support for the Vybrid tower
system (TWR-VF65GS10). Full 16-Bit bus width is supported. Also an
aditional config vf610twr_nand is introduced which gets the
environment from NAND. However, booting U-Boot from NAND is not
yet possible due to missing boot configuration block (BCB).
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
board/freescale/vf610twr/vf610twr.c | 47 ++++++++++++++++++++++++++++++++++---
configs/vf610twr_defconfig | 2 +-
configs/vf610twr_nand_defconfig | 3 +++
include/configs/vf610twr.h | 43 +++++++++++++++++++++++++++++++++
4 files changed, 91 insertions(+), 4 deletions(-)
create mode 100644 configs/vf610twr_nand_defconfig
diff --git a/board/freescale/vf610twr/vf610twr.c b/board/freescale/vf610twr/vf610twr.c
index 54a9f2c..63ebbf8 100644
--- a/board/freescale/vf610twr/vf610twr.c
+++ b/board/freescale/vf610twr/vf610twr.c
@@ -278,6 +278,39 @@ static void setup_iomux_i2c(void)
imx_iomux_v3_setup_multiple_pads(i2c0_pads, ARRAY_SIZE(i2c0_pads));
}
+#ifdef CONFIG_NAND_FSL_NFC
+static void setup_iomux_nfc(void)
+{
+ static const iomux_v3_cfg_t nfc_pads[] = {
+ VF610_PAD_PTD31__NF_IO15,
+ VF610_PAD_PTD30__NF_IO14,
+ VF610_PAD_PTD29__NF_IO13,
+ VF610_PAD_PTD28__NF_IO12,
+ VF610_PAD_PTD27__NF_IO11,
+ VF610_PAD_PTD26__NF_IO10,
+ VF610_PAD_PTD25__NF_IO9,
+ VF610_PAD_PTD24__NF_IO8,
+ VF610_PAD_PTD23__NF_IO7,
+ VF610_PAD_PTD22__NF_IO6,
+ VF610_PAD_PTD21__NF_IO5,
+ VF610_PAD_PTD20__NF_IO4,
+ VF610_PAD_PTD19__NF_IO3,
+ VF610_PAD_PTD18__NF_IO2,
+ VF610_PAD_PTD17__NF_IO1,
+ VF610_PAD_PTD16__NF_IO0,
+ VF610_PAD_PTB24__NF_WE_B,
+ VF610_PAD_PTB25__NF_CE0_B,
+ VF610_PAD_PTB27__NF_RE_B,
+ VF610_PAD_PTC26__NF_RB_B,
+ VF610_PAD_PTC27__NF_ALE,
+ VF610_PAD_PTC28__NF_CLE
+ };
+
+ imx_iomux_v3_setup_multiple_pads(nfc_pads, ARRAY_SIZE(nfc_pads));
+}
+#endif
+
+
static void setup_iomux_qspi(void)
{
static const iomux_v3_cfg_t qspi0_pads[] = {
@@ -354,6 +387,8 @@ static void clock_init(void)
CCM_CCGR7_SDHC1_CTRL_MASK);
clrsetbits_le32(&ccm->ccgr9, CCM_REG_CTRL_MASK,
CCM_CCGR9_FEC0_CTRL_MASK | CCM_CCGR9_FEC1_CTRL_MASK);
+ clrsetbits_le32(&ccm->ccgr10, CCM_REG_CTRL_MASK,
+ CCM_CCGR10_NFC_CTRL_MASK);
clrsetbits_le32(&anadig->pll2_ctrl, ANADIG_PLL2_CTRL_POWERDOWN,
ANADIG_PLL2_CTRL_ENABLE | ANADIG_PLL2_CTRL_DIV_SELECT);
@@ -373,14 +408,17 @@ static void clock_init(void)
CCM_CACRR_IPG_CLK_DIV(1) | CCM_CACRR_BUS_CLK_DIV(2) |
CCM_CACRR_ARM_CLK_DIV(0));
clrsetbits_le32(&ccm->cscmr1, CCM_REG_CTRL_MASK,
- CCM_CSCMR1_ESDHC1_CLK_SEL(3) | CCM_CSCMR1_QSPI0_CLK_SEL(3));
+ CCM_CSCMR1_ESDHC1_CLK_SEL(3) | CCM_CSCMR1_QSPI0_CLK_SEL(3) |
+ CCM_CSCMR1_NFC_CLK_SEL(0));
clrsetbits_le32(&ccm->cscdr1, CCM_REG_CTRL_MASK,
CCM_CSCDR1_RMII_CLK_EN);
clrsetbits_le32(&ccm->cscdr2, CCM_REG_CTRL_MASK,
- CCM_CSCDR2_ESDHC1_EN | CCM_CSCDR2_ESDHC1_CLK_DIV(0));
+ CCM_CSCDR2_ESDHC1_EN | CCM_CSCDR2_ESDHC1_CLK_DIV(0) |
+ CCM_CSCDR2_NFC_EN);
clrsetbits_le32(&ccm->cscdr3, CCM_REG_CTRL_MASK,
CCM_CSCDR3_QSPI0_EN | CCM_CSCDR3_QSPI0_DIV(1) |
- CCM_CSCDR3_QSPI0_X2_DIV(1) | CCM_CSCDR3_QSPI0_X4_DIV(3));
+ CCM_CSCDR3_QSPI0_X2_DIV(1) | CCM_CSCDR3_QSPI0_X4_DIV(3) |
+ CCM_CSCDR3_NFC_PRE_DIV(5));
clrsetbits_le32(&ccm->cscmr2, CCM_REG_CTRL_MASK,
CCM_CSCMR2_RMII_CLK_SEL(0));
}
@@ -411,6 +449,9 @@ int board_early_init_f(void)
setup_iomux_enet();
setup_iomux_i2c();
setup_iomux_qspi();
+#ifdef CONFIG_NAND_FSL_NFC
+ setup_iomux_nfc();
+#endif
return 0;
}
diff --git a/configs/vf610twr_defconfig b/configs/vf610twr_defconfig
index 10e6432..7de374a 100644
--- a/configs/vf610twr_defconfig
+++ b/configs/vf610twr_defconfig
@@ -1,3 +1,3 @@
-CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/freescale/vf610twr/imximage.cfg"
+CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/freescale/vf610twr/imximage.cfg,ENV_IS_IN_MMC"
CONFIG_ARM=y
CONFIG_TARGET_VF610TWR=y
diff --git a/configs/vf610twr_nand_defconfig b/configs/vf610twr_nand_defconfig
new file mode 100644
index 0000000..e78db26
--- /dev/null
+++ b/configs/vf610twr_nand_defconfig
@@ -0,0 +1,3 @@
+CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/freescale/vf610twr/imximage.cfg,ENV_IS_IN_NAND"
+CONFIG_ARM=y
+CONFIG_TARGET_VF610TWR=y
diff --git a/include/configs/vf610twr.h b/include/configs/vf610twr.h
index 0342550..543f241 100644
--- a/include/configs/vf610twr.h
+++ b/include/configs/vf610twr.h
@@ -14,6 +14,7 @@
#define CONFIG_VF610
+#define CONFIG_SYS_GENERIC_BOARD
#define CONFIG_DISPLAY_CPUINFO
#define CONFIG_DISPLAY_BOARDINFO
@@ -44,6 +45,39 @@
#undef CONFIG_CMD_IMLS
+/* NAND support */
+#define CONFIG_CMD_NAND
+#define CONFIG_CMD_NAND_TRIMFFS
+
+#ifdef CONFIG_CMD_NAND
+#define CONFIG_NAND_FSL_NFC
+#define CONFIG_SYS_NAND_BUSWIDTH_16BIT
+#define CONFIG_SYS_MAX_NAND_DEVICE 1
+#define CONFIG_SYS_NAND_BASE NFC_BASE_ADDR
+
+/* UBI */
+#define CONFIG_CMD_UBI
+#define CONFIG_CMD_UBIFS
+#define CONFIG_CMD_MTDPARTS
+#define CONFIG_RBTREE
+#define CONFIG_LZO
+#define CONFIG_MTD_DEVICE
+#define CONFIG_MTD_PARTITIONS
+
+/* Dynamic MTD partition support */
+#define CONFIG_CMD_MTDPARTS
+#define CONFIG_MTD_PARTITIONS
+#define CONFIG_MTD_DEVICE
+#define MTDIDS_DEFAULT "nand0=fsl_nfc"
+#define MTDPARTS_DEFAULT "mtdparts=fsl_nfc:" \
+ "128k(vf-bcb)ro," \
+ "1408k(u-boot)ro," \
+ "512k(u-boot-env)," \
+ "4m(kernel)," \
+ "512k(fdt)," \
+ "-(rootfs)"
+#endif
+
#define CONFIG_MMC
#define CONFIG_FSL_ESDHC
#define CONFIG_SYS_FSL_ESDHC_ADDR 0
@@ -218,11 +252,20 @@
/* FLASH and environment organization */
#define CONFIG_SYS_NO_FLASH
+#ifdef CONFIG_ENV_IS_IN_MMC
#define CONFIG_ENV_SIZE (8 * 1024)
#define CONFIG_ENV_IS_IN_MMC
#define CONFIG_ENV_OFFSET (12 * 64 * 1024)
#define CONFIG_SYS_MMC_ENV_DEV 0
+#endif
+
+#ifdef CONFIG_ENV_IS_IN_NAND
+#define CONFIG_ENV_SIZE (64 * 2048)
+#define CONFIG_ENV_SECT_SIZE (64 * 2048)
+#define CONFIG_ENV_RANGE (512 * 1024)
+#define CONFIG_ENV_OFFSET 0x180000
+#endif
#define CONFIG_OF_LIBFDT
#define CONFIG_CMD_BOOTZ
--
2.0.4
^ permalink raw reply related [flat|nested] 22+ messages in thread