arm: vf610: add NAND flash support
v2 cover arm: vf610: add NAND flash support
This patch set adds NAND Flash Controller (NFC) support for Freescale Vybrid ARM SoCs (vf610).
The driver is based on Bill Pringlemeirs prelineary patch sent in January 2014 to the MTD mailing list:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/226623.html
Changes in v2:
- Renamed the driver from fsl_nfc to vf610_nfc
- Use writel/readl in register access functions
- Optimized some register accesses by read/write value only once in code
- Use CONFIG_SYS_NAND_SELF_INIT and fix board_nand_init implementation
- Removed uncommented code/fixed comments
- Add CONFIG_USE_ARCH_MEMCPY for improved NAND performance
- Use hweight32 for improved count_written_bits performance
- Implement page_read/page_write rather than reuse MTD stacks version
Due to the duplicated initialization (missing SELF_INIT), page size was actually set to 0 which lead count_written_bits to not count the amount of written bits at all.
Performance numbers:
V1 => optimized count_written_bits
Read empty pages: 0.8MiB/s => 1.4MiB/s
V1 => optimized memcpy => optimized page_read/page_write Read full pages: 3.6MiB/s => 7MiB/s => 10.3MiB/s
Stefan Agner (4):
arm: vf610: add NFC pin mux
arm: vf610: add NFC clock support
mtd: nand: add Freescale NFC driver
arm: vf610: add NAND support for vf610twr
arch/arm/include/asm/arch-vf610/crm_regs.h | 14 + arch/arm/include/asm/arch-vf610/imx-regs.h | 1 + arch/arm/include/asm/arch-vf610/iomux-vf610.h | 34 ++ arch/arm/include/asm/imx-common/iomux-v3.h | 4 + board/freescale/vf610twr/vf610twr.c | 47 +- configs/vf610twr_defconfig | 2 +- configs/vf610twr_nand_defconfig | 3 + drivers/mtd/nand/Makefile | 1 + drivers/mtd/nand/vf610_nfc.c | 706 ++++++++++++++++++++++++++ include/configs/vf610twr.h | 46 +- 10 files changed, 853 insertions(+), 5 deletions(-) create mode 100644 configs/vf610twr_nand_defconfig create mode 100644 drivers/mtd/nand/vf610_nfc.c
Signature · 1 line
2.0.4
PATCH v2 1/4 arm: vf610: add NFC pin mux
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.
--- 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(+)
arch/arm/include/asm/arch-vf610/iomux-vf610.h +34 -0
@@ -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),
arch/arm/include/asm/imx-common/iomux-v3.h +4 -0
@@ -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
Signature · 1 line
2.0.4
PATCH v2 2/4 arm: vf610: add NFC clock support
In reply to Stefan AgnerAdd NFC (NAND Flash Controller) clock support and enable them at board initialization time.
--- 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(+)
arch/arm/include/asm/arch-vf610/crm_regs.h +14 -0
@@ -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)
arch/arm/include/asm/arch-vf610/imx-regs.h +1 -0
@@ -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
Signature · 1 line
2.0.4
PATCH v2 3/4 mtd: nand: add Freescale NFC driver
In reply to Stefan AgnerThis adds initial support for Freescale NFC (NAND Flash Controller) found in ARM Vybrid SoC's, Power Architecture MPC5125 and others. However, this driver is only tested on Vybrid.
--- drivers/mtd/nand/Makefile | 1 + drivers/mtd/nand/vf610_nfc.c | 706 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 707 insertions(+) create mode 100644 drivers/mtd/nand/vf610_nfc.c
drivers/mtd/nand/Makefile +1 -0
@@ -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_VF610_NFC) += vf610_nfc.o obj-$(CONFIG_NAND_MXC) += mxc_nand.o obj-$(CONFIG_NAND_MXS) += mxs_nand.o obj-$(CONFIG_NAND_NDFC) += ndfc.o
drivers/mtd/nand/vf610_nfc.c new file +706 -0
706 lines not on this page · raw message
Signature · 1 line
2.0.4
PATCH v2 3/4 mtd: nand: add Freescale NFC driver
Quoted 21 lines · stefan at agner.ch
This adds initial support for Freescale NFC (NAND Flash Controller) found in ARM Vybrid SoC's, Power Architecture MPC5125 and others. However, this driver is only tested on Vybrid.
Signed-off-by Stefan Agner <[email protected]>--- drivers/mtd/nand/Makefile | 1 + drivers/mtd/nand/vf610_nfc.c | 706 ++++++++++++++++++++++++++++++++++++++++++> + 2 files changed, 707 insertions(+) create mode 100644 drivers/mtd/nand/vf610_nfc.cdrivers/mtd/nand/vf610_nfc.c new file +3
-0@@ -0,0 +1,706 @@ +/* +* Copyright 2009-2014 Freescale Semiconductor, Inc. and others +*
[snip]
Quoted 4 lines
drivers/mtd/nand/vf610_nfc.c new file +4
-0+/* Count the number of 0's in buff upto max_bits */ +static inline int count_written_bits (uint8_t *buff,int size,int max_bits) +{ +uint32_t *buff32 = (uint32_t *)buff;
drivers/mtd/nand/vf610_nfc.c line 489
Quoted 10 lines
drivers/mtd/nand/vf610_nfc.c new file +10
-0+int k, written_bits =0 ; + +for (k =0 ; k < (size /4 ); k++) { + written_bits +=hweight32 (~buff32[k]); +if (written_bits > max_bits) +break ; + } + +return written_bits; +}
That is a nice change.
Quoted 8 lines
drivers/mtd/nand/vf610_nfc.c new file +8
-0+static inline int vf610_nfc_correct_data (struct mtd_info *mtd,u_char *dat) +{ +struct vf610_nfc *nfc =mtd_to_nfc (mtd); +u8 ecc_status; +u8 ecc_count; +int flip; + + ecc_status =__raw_readb (nfc->regs +ECC_SRAM_ADDR *8 +ECC_OFFSET );
drivers/mtd/nand/vf610_nfc.c line 508
Quoted 10 lines
drivers/mtd/nand/vf610_nfc.c new file +10
-0+ 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 ;
Sorry, I missed this in version one of the patch. The original had,
< if (flip > ecc_count) {
< nfc->page = -1;
---
Quoted 1 line
if (flip >
ecc_count)
522d508
< }
I can see why you removed this (nfc->page = -1). However, I think that higher layers may want to re-read on an error in case of un-stable bits? It is very little code to ensure a re-read in case of ECC failure. The 2nd physical read may pass whereas the first failed. This path is rare, but maybe important? A higher layer may migrate the data in this case; just as with a corrected bits. But maybe U-Boot will never do this?
drivers/mtd/nand/vf610_nfc.c line 513
Quoted 5 lines
drivers/mtd/nand/vf610_nfc.c new file +5
-0+ +/* Erased page. */ +memset (dat,0xff , nfc->chip.ecc.size); +return 0 ; +}
Regards,
Bill Pringlemeir.
PATCH v2 3/4 mtd: nand: add Freescale NFC driver
In reply to Stefan AgnerQuoted 1 line · Stefan Agner
+#define
DRV_NAME"fsl_nfc"
DRV_NAME doesn't match filename (neither does the patch title), and it doesn't seem all that useful anyway -- the one place that uses it would be better off using __func__.
Quoted 1 line
+static int
vf610_nfc_nand_init(int devnum, u8 *addr)
Why u8? Either use void or u32. Also should have __iomem.
...OK, I see you copied that from the examples I pointed out. I have no idea why those use u8 either. :-(
drivers/mtd/nand/vf610_nfc.c line 582
Quoted 1 line
drivers/mtd/nand/vf610_nfc.c new file +1
-0+ chip->IO_ADDR_R = chip->IO_ADDR_W = nfc->regs = (void __iomem *)addr;
Don't set IO_ADDR_R/IO_ADDR_W if they're not going to be used.
-Scott
PATCH v2 3/4 mtd: nand: add Freescale NFC driver
In reply to Stefan AgnerQuoted 7 lines · stefan at agner.ch
This adds initial support for Freescale NFC (NAND Flash Controller) found in ARM Vybrid SoC's, Power Architecture MPC5125 and others. However, this driver is only tested on Vybrid.
This is only to expand on the nand controller register and SRAM use.
[snip]
Quoted 5 lines
drivers/mtd/nand/vf610_nfc.c new file +0
-0
[snip]
drivers/mtd/nand/vf610_nfc.c line 211
Quoted 13 lines
drivers/mtd/nand/vf610_nfc.c new file +13
-0+static inline u32 vf610_nfc_read (struct mtd_info *mtd,uint reg) +{ +struct vf610_nfc *nfc =mtd_to_nfc (mtd); + +return readl (nfc->regs + reg); +} + +static inline void vf610_nfc_write (struct mtd_info *mtd,uint reg,u32 val) +{ +struct vf610_nfc *nfc =mtd_to_nfc (mtd); + +writel (val, nfc->regs + reg); +}
Ok, we always use readl/writel. This is fine, but a little slower and bigger. I may try a register cache if I resubmit to the Linux MTD as per Scott's suggestion. Especially, this version is good for an incremental patch.
I think these are in 'arch/arm/include/asm/io.h' of U-Boot.
#define dmb() __asm__ __volatile__ ("" : : : "memory")
#define __iormb() dmb()
#define __iowmb() dmb()#define readl(c) ({ u32 __v = __arch_getl(c); __iormb(); __v; })
#define writel(v,c) ({ u32 __v = v; __iowmb(); __arch_putl(__v,c); __v; })
Currently, these look like compiler barriers to me. Fine so far.
Quoted 23 lines
drivers/mtd/nand/vf610_nfc.c new file +23
-0+ +static inline void vf610_nfc_set (struct mtd_info *mtd,uint reg,u32 bits) +{ +vf610_nfc_write (mtd, reg,vf610_nfc_read (mtd, reg) | bits); +} + +static inline void vf610_nfc_clear (struct mtd_info *mtd,uint reg,u32 bits) +{ +vf610_nfc_write (mtd, reg,vf610_nfc_read (mtd, reg) & ~bits); +} + +static inline void vf610_nfc_set_field (struct mtd_info *mtd,u32 reg, +u32 mask,u32 shift,u32 val) +{ +vf610_nfc_write (mtd, reg, + (vf610_nfc_read (mtd, reg) & (~mask)) | val << shift); +} + +/* Clear flags for upcoming command */ +static inline void vf610_nfc_clear_status (struct mtd_info *mtd) +{ +u32 tmp =vf610_nfc_read (mtd,NFC_IRQ_STATUS ); + tmp |=CMD_DONE_CLEAR_BIT |IDLE_CLEAR_BIT ;
drivers/mtd/nand/vf610_nfc.c line 244
Quoted 10 lines
drivers/mtd/nand/vf610_nfc.c new file +10
-0+vf610_nfc_write (mtd,NFC_IRQ_STATUS , tmp); +} + +/* Wait for complete operation */ +static inline void vf610_nfc_done (struct mtd_info *mtd) +{ +uint start; + +vf610_nfc_set (mtd,NFC_FLASH_CMD2 ,START_BIT ); +barrier ();
This barrier() is not needed then. The vf610_nfc_set() should have done it twice already, plus everything is volatile.
[snip]
drivers/mtd/nand/vf610_nfc.c line 386
Quoted 8 lines
drivers/mtd/nand/vf610_nfc.c new file +8
-0+static inline void vf610_nfc_read_spare (struct mtd_info *mtd,void *buf, +int len) +{ +struct vf610_nfc *nfc =mtd_to_nfc (mtd); + + len =min (mtd->oobsize, (uint )len); +if (len >0 ) +memcpy (buf, nfc->regs + mtd->writesize, len);
Notice the 'memcpy(.. nfc->regs);'...
Quoted 7 lines
drivers/mtd/nand/vf610_nfc.c new file +7
-0+} + +/* Read data from NFC buffers */ +static void vf610_nfc_read_buf (struct mtd_info *mtd,u_char *buf,int len) +{ +struct vf610_nfc *nfc =mtd_to_nfc (mtd); +uint c = nfc->column;
drivers/mtd/nand/vf610_nfc.c line 403
Quoted 10 lines
drivers/mtd/nand/vf610_nfc.c new file +10
-0+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);
Another 'memcpy(.. nfc->regs);'...
Quoted 18 lines
drivers/mtd/nand/vf610_nfc.c new file +18
-0+else +if (nfc->alt_buf &ALT_BUF_ID ) + *buf =vf610_nfc_get_id (mtd, c); +else + *buf =vf610_nfc_get_status (mtd); + + buf += l; + len -= l; + } + +/* Handle spare area access */ +if (len) { + nfc->column += len; +vf610_nfc_read_spare (mtd, buf, len); + } +} + +/* Write data to NFC buffers */
drivers/mtd/nand/vf610_nfc.c line 431
Quoted 10 lines
drivers/mtd/nand/vf610_nfc.c new file +10
-0+static void vf610_nfc_write_buf (struct mtd_info *mtd,const u_char *buf, +int len) +{ +struct vf610_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);
Another 'memcpy(.. nfc->regs);'...
[snip]
These memcpy's are the same 'bus' interface as the registers. We should be just as worried about this SRAM buffer memory as the memory mapped registers, shouldn't we? Is a barrier() before reading and a barrier() after writing fine for U-Boot? Personally, I think they are safe as only the 'vf610_nfc_set(mtd, NFC_FLASH_CMD2, START_BIT)' needs some care. Maybe a comment is fine? It seems the Vybrid is safe for different access sizes, but it is possible that some other CPU might not be able to access this memory via 32/16/8 bit accesses and 'memcpy()' may not be appropriate. It seems that 'natural' size of the NFC controller itself is 32bits and the CPU interface does lane masking. Ie, boot mode documentation talks about remapping 'sram_physical_addr[13:3] = {cpu_addr[11:3],cpu_addr[13:12]}' saying that bits 2,1 are not used (hopefully one based numbers). This is just my guess...
The VF6xx page has a documentation tab,
http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=VF6xx
There is an app note, AN4947 'Understanding Vybrid Architecture', which describes some timing details for the AHB bus (where this flash controller is connected). Pg21 Table 7 of that document gives some measurements. The QSPI is a similar peripheral on the AHB. The first and second lines give accesses of 4408 and subsequent accesses are 2770 Cortex-A5 clocks. Normal SDRAM is 258 and 8 clocks. Ie, it is quite important in places to minimize accesses and try to make them sequential.
However, it looks like most U-Boot NAND drivers use the memcpy()? With
the exceptions of fsl_elbc_nand.c, fsl_ifc_nand.c, and mpc5121_nfc.c. Maybe it doesn't matter...
Fwiw,
Bill Pringlemeir.
PATCH v2 3/4 mtd: nand: add Freescale NFC driver
Am 2014-08-14 23:12, schrieb Bill Pringlemeir:
Quoted 28 lines
Quoted 5 lines · stefan at agner.ch
This adds initial support for Freescale NFC (NAND Flash Controller) found in ARM Vybrid SoC's, Power Architecture MPC5125 and others. However, this driver is only tested on Vybrid.
This is only to expand on the nand controller register and SRAM use.
[snip]
Quoted 5 lines
drivers/mtd/nand/vf610_nfc.c new file +0
-0[snip]
Quoted 10 lines
drivers/mtd/nand/vf610_nfc.c new file +10
-0+static inline u32 vf610_nfc_read (struct mtd_info *mtd,uint reg) +{ +struct vf610_nfc *nfc =mtd_to_nfc (mtd); + +return readl (nfc->regs + reg); +} + +static inline void vf610_nfc_write (struct mtd_info *mtd,uint reg,u32 val) +{ +struct vf610_nfc *nfc =mtd_to_nfc (mtd);
Quoted 8 lines
drivers/mtd/nand/vf610_nfc.c line 211
Quoted 3 lines
drivers/mtd/nand/vf610_nfc.c new file +3
-0+ +writel (val, nfc->regs + reg); +}Ok, we always use readl/writel. This is fine, but a little slower and bigger. I may try a register cache if I resubmit to the Linux MTD as per Scott's suggestion. Especially, this version is good for an incremental patch.
I measured the difference and get 1MB/s Full pages, readl/writel:
NAND read: device 0 offset 0x200000, size 0x800000
8388608 bytes read in 772 ms (10.4 MiB/s): OK
Full pages, __raw_readl/__raw_writel
NAND read: device 0 offset 0x200000, size 0x800000
8388608 bytes read in 696 ms (11.5 MiB/s): OK
Ok, this is actually quite a lot. Especially since I already optimized the C code (by not using the helper functions like nfc_set/nfc_clear in vf610_nfc_send_command), one would think there is now almost no optimization potential. I looked into the disassembled code and could narrow down the issue. Due to the memory barriers, all offsets were calculated on each register access (nfc base to reg base, and add reg offset), multiple instances of:
20: e59cc120 ldr ip, [ip, #288] ; 0x120
24: e59cc134 ldr ip, [ip, #308] ; 0x134
I optimized the code again and calculate the offsets manually and access __raw_readl/__raw_writel rather then vf610_nfc_read/write in the vf610_nfc_send_command(s) function, I get the full speed again:
NAND read: device 0 offset 0x200000, size 0x800000
8388608 bytes read in 687 ms (11.6 MiB/s): OK
Quoted 40 lines
I think these are in '
arch/arm/include/asm/io.h' of U-Boot.#define dmb() __asm__ __volatile__ ("" : : : "memory") #define __iormb() dmb() #define __iowmb() dmb()#define readl(c) ({ u32
__v=__arch_getl(c);__iormb();__v; })
#definewritel(v,c)({ u32__v= v;__iowmb();__arch_putl(__v,c);__v; })Currently, these look like compiler barriers to me. Fine so far.
Quoted 28 lines
drivers/mtd/nand/vf610_nfc.c new file +28
-0+ +static inline void vf610_nfc_set (struct mtd_info *mtd,uint reg,u32 bits) +{ +vf610_nfc_write (mtd, reg,vf610_nfc_read (mtd, reg) | bits); +} + +static inline void vf610_nfc_clear (struct mtd_info *mtd,uint reg,u32 bits) +{ +vf610_nfc_write (mtd, reg,vf610_nfc_read (mtd, reg) & ~bits); +} + +static inline void vf610_nfc_set_field (struct mtd_info *mtd,u32 reg, +u32 mask,u32 shift,u32 val) +{ +vf610_nfc_write (mtd, reg, + (vf610_nfc_read (mtd, reg) & (~mask)) | val << shift); +} + +/* Clear flags for upcoming command */ +static inline void vf610_nfc_clear_status (struct mtd_info *mtd) +{ +u32 tmp =vf610_nfc_read (mtd,NFC_IRQ_STATUS ); + tmp |=CMD_DONE_CLEAR_BIT |IDLE_CLEAR_BIT ; +vf610_nfc_write (mtd,NFC_IRQ_STATUS , tmp); +} + +/* Wait for complete operation */ +static inline void vf610_nfc_done (struct mtd_info *mtd)
Quoted 9 lines
drivers/mtd/nand/vf610_nfc.c line 244
Quoted 5 lines
drivers/mtd/nand/vf610_nfc.c new file +5
-0+{ +uint start; + +vf610_nfc_set (mtd,NFC_FLASH_CMD2 ,START_BIT ); +barrier ();This
barrier()is not needed then. Thevf610_nfc_set()should have done it twice already, plus everything is volatile.
Agreed, this is not needed any more.
Quoted 64 lines
[snip]
drivers/mtd/nand/vf610_nfc.c line 386
Quoted 8 lines
drivers/mtd/nand/vf610_nfc.c new file +8
-0+static inline void vf610_nfc_read_spare (struct mtd_info *mtd,void *buf, +int len) +{ +struct vf610_nfc *nfc =mtd_to_nfc (mtd); + + len =min (mtd->oobsize, (uint )len); +if (len >0 ) +memcpy (buf, nfc->regs + mtd->writesize, len);Notice the 'memcpy(..
nfc->regs);'...Quoted 7 lines
drivers/mtd/nand/vf610_nfc.c new file +7
-0+} + +/* Read data from NFC buffers */ +static void vf610_nfc_read_buf (struct mtd_info *mtd,u_char *buf,int len) +{ +struct vf610_nfc *nfc =mtd_to_nfc (mtd); +uint c = nfc->column;drivers/mtd/nand/vf610_nfc.c line 403
Quoted 10 lines
drivers/mtd/nand/vf610_nfc.c new file +10
-0+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);Another 'memcpy(..
nfc->regs);'...Quoted 18 lines
drivers/mtd/nand/vf610_nfc.c new file +18
-0+else +if (nfc->alt_buf &ALT_BUF_ID ) + *buf =vf610_nfc_get_id (mtd, c); +else + *buf =vf610_nfc_get_status (mtd); + + buf += l; + len -= l; + } + +/* Handle spare area access */ +if (len) { + nfc->column += len; +vf610_nfc_read_spare (mtd, buf, len); + } +} + +/* Write data to NFC buffers */ drivers/mtd/nand/vf610_nfc.c line 431
Quoted 10 lines
drivers/mtd/nand/vf610_nfc.c new file +10
-0+static void vf610_nfc_write_buf (struct mtd_info *mtd,const u_char *buf, +int len) +{ +struct vf610_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);Another 'memcpy(..
nfc->regs);'...
Quoted 7 lines
[snip]
These memcpy's are the same 'bus' interface as the registers. We should be just as worried about this SRAM buffer memory as the memory mapped registers, shouldn't we? Is a
barrier()before reading and abarrier()after writing fine for U-Boot? Personally, I think they are safe as only the 'vf610_nfc_set(mtd, NFC_FLASH_CMD2, START_BIT)' needs some
I also think that that this is the only place a barrier is really needed. However, as Scott stated:
Quoted 4 lines · Scott Wood
raw_writel()is itself something that should only be used for hand-optimized sections. For non-performance-critical code you should use normalwritel()so that you don't need to worry about manually adding I/O barriers.
The reason I choosed readl/writel instead of the raw variants is to preserve align with other drivers...
Quoted 9 lines
care. Maybe a comment is fine? It seems the Vybrid is safe for different access sizes, but it is possible that some other CPU might not be able to access this memory via 32/16/8 bit accesses and '
memcpy()' may not be appropriate. It seems that 'natural' size of the NFC controller itself is 32bits and the CPU interface does lane masking. Ie, boot mode documentation talks about remapping 'sram_physical_addr[13:3] = {cpu_addr[11:3],cpu_addr[13:12]}' saying that bits 2,1 are not used (hopefully one based numbers). This is just my guess...
What assumptions do you make how memcpy accesses memory? This latest patch now uses the optimized versions from the kernel... Maybe they even try to access 64-bit width (the NIC interconnect supports 64-bit access)
Quoted 4 lines
The VF6xx page has a documentation tab,
http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=VF6xx
Quoted 8 lines
There is an app note, AN4947 'Understanding Vybrid Architecture', which describes some timing details for the AHB bus (where this flash controller is connected). Pg21 Table 7 of that document gives some measurements. The QSPI is a similar peripheral on the AHB. The first and second lines give accesses of 4408 and subsequent accesses are 2770 Cortex-A5 clocks. Normal SDRAM is 258 and 8 clocks. Ie, it is quite important in places to minimize accesses and try to make them sequential.
We also have caches, hence I don't think the access will take that long. And on the other side, the SRAM is much faster.
Quoted 4 lines
However, it looks like most U-Boot NAND drivers use the
memcpy()? With
the exceptions offsl_elbc_nand.c,fsl_ifc_nand.c, andmpc5121_nfc.c. Maybe it doesn't matter...
--
Stefan
PATCH v2 3/4 mtd: nand: add Freescale NFC driver
Quoted 44 lines · stefan at agner.ch
Am 2014-08-14 23:12, schrieb Bill Pringlemeir:
Quoted 16 lines
Quoted 5 lines · stefan at agner.ch
This adds initial support for Freescale NFC (NAND Flash Controller) found in ARM Vybrid SoC's, Power Architecture MPC5125 and others. However, this driver is only tested on Vybrid.
This is only to expand on the nand controller register and SRAM use.
[snip]
Quoted 3 lines
(quoted diff) +0
-0
b/drivers/mtd/nand/vf610_nfc.cnew file mode 100644 index 0000000..3150ac1 ---/dev/null+++b/drivers/mtd/nand/vf610_nfc.c[snip]
Quoted 6 lines
Quoted 1 line
+static inline u32
vf610_nfc_read(struct mtd_info *mtd, uint reg)+{Ok, we always use readl/writel. This is fine, but a little slower and bigger. I may try a register cache if I resubmit to the Linux MTD as per Scott's suggestion. Especially, this version is good for an incremental patch.
I measured the difference and get 1MB/s Full pages, readl/writel:
NAND read: device 0 offset0x200000, size0x8000008388608 bytes read in 772 ms (10.4 MiB/s): OKFull pages, __raw_readl/__raw_writel
NAND read: device 0 offset0x200000, size0x8000008388608 bytes read in 696 ms (11.5 MiB/s): OKOk, this is actually quite a lot. Especially since I already optimized the C code (by not using the helper functions like nfc_set/nfc_clear in
vf610_nfc_send_command), one would think there is now almost no optimization potential. I looked into the disassembled code and could narrow down the issue. Due to the memory barriers, all offsets were calculated on each register access (nfc base to reg base, and add reg offset), multiple instances of:20:
e59cc120ldr ip, [ip, #288] ;0x120
24:e59cc134ldr ip, [ip, #308] ;0x134
Quoted 6 lines
I optimized the code again and calculate the offsets manually and access __raw_readl/__raw_writel rather then vf610_nfc_read/write in the
vf610_nfc_send_command(s)function, I get the full speed again:NAND read: device 0 offset
0x200000, size0x8000008388608 bytes read in 687 ms (11.6 MiB/s): OK
I think what you have is fine. The 10BM/s versus 11MB/s is not insignificant, but it is not a huge difference. I expect that the driver is already better than the others; especially the Imx-25 was only 7.7MB/s read and 4.6Mb/s write from Linux mtd tests. Although the end user might like to wait 10% less for an image to load.
Also, probably a lot of people care about code size. This is not so much a case for U-Boot as it is usually machine specific and doesn't support several SOCs afaik.
However, the more specific you make the optimization to the platform, the less likely it is to extend well. We also wish to have this work well with different gcc versions and CPUs (PowerPC, etc). The 'readl/writel' handicap the compiler. Although they are more likely to work with a wide variety of buses.
[snip]
Quoted 19 lines · Scott Wood
Quoted 4 lines
raw_writel()is itself something that should only be used for hand-optimized sections. For non-performance-critical code you should use normalwritel()so that you don't need to worry about manually adding I/O barriers.The reason I choosed readl/writel instead of the raw variants is to preserve align with other drivers...
Quoted 9 lines
care. Maybe a comment is fine? It seems the Vybrid is safe for different access sizes, but it is possible that some other CPU might not be able to access this memory via 32/16/8 bit accesses and '
memcpy()' may not be appropriate. It seems that 'natural' size of the NFC controller itself is 32bits and the CPU interface does lane masking. Ie, boot mode documentation talks about remapping 'sram_physical_addr[13:3] = {cpu_addr[11:3],cpu_addr[13:12]}' saying that bits 2,1 are not used (hopefully one based numbers). This is just my guess...
Quoted 4 lines
What assumptions do you make how memcpy accesses memory? This latest patch now uses the optimized versions from the kernel... Maybe they even try to access 64-bit width (the NIC interconnect supports 64-bit access)
The memcpy() itself could use anything. 64bits is possible on AXI/NIC. The 'PBRIDGE' is 64bit, but I think the AIPS/IPS (apparently AIPS means 'AHB-lite to IPS) are 32bit. At least that is the case on the Imx25 which has a different AIPS version. I assumed the 'memcpy()' was using 32bits but this certainly isn't explicit in the code.
The majority of the register banks are non-volatile with this controller. Instead of running multiple NAND programming sequences, the controller runs them all for us. Most registers are are mainly like SRAM.
My only point is that the SRAM buffers use the same interface as the main Nand controller register banks. So using 'readl/writel' for the register, but not the SRAM buffers seems inconsistent.
So to address this inconsistency, I was thinking that we should at least have a comment?
/* BUS: This assumes the BUS is 32 bit accessible. If you are porting
to other systems, this may not be the case.
*/
memcpy(nfc->regs + NFC_MAIN_AREA(0) + c, buf, l);
Or we could implement our own version of memcpy that did 32bit aligned transfers with a similar comment. In theory, we need a barrier after the memcpy(), in case anyone modified the code to touch the NFC_FLASH_CMD2's START_BIT directly after the memcpy() or custom function. But all the paranoia adds some code and has potential to slow things down.
Doing a barrier after every single byte read as the ARM Linux's memcpy_fromio() does will surely make significant performance differences. Instead of being double the Imx25, it was half. A user waiting 400% longer won't be too happy.
Fwiw,
Bill Pringlemeir.
PATCH v2 3/4 mtd: nand: add Freescale NFC driver
Am 2014-08-18 18:38, schrieb Bill Pringlemeir:
Quoted 110 lines · stefan at agner.ch
Quoted 45 lines
Am 2014-08-14 23:12, schrieb Bill Pringlemeir:
Quoted 16 lines
Quoted 5 lines · stefan at agner.ch
This adds initial support for Freescale NFC (NAND Flash Controller) found in ARM Vybrid SoC's, Power Architecture MPC5125 and others. However, this driver is only tested on Vybrid.
This is only to expand on the nand controller register and SRAM use.
[snip]
Quoted 3 lines
(quoted diff) +0
-0
b/drivers/mtd/nand/vf610_nfc.cnew file mode 100644 index 0000000..3150ac1 ---/dev/null+++b/drivers/mtd/nand/vf610_nfc.c[snip]
Quoted 6 lines
Quoted 1 line
+static inline u32
vf610_nfc_read(struct mtd_info *mtd, uint reg)+{Ok, we always use readl/writel. This is fine, but a little slower and bigger. I may try a register cache if I resubmit to the Linux MTD as per Scott's suggestion. Especially, this version is good for an incremental patch.
I measured the difference and get 1MB/s Full pages, readl/writel:
NAND read: device 0 offset0x200000, size0x8000008388608 bytes read in 772 ms (10.4 MiB/s): OKFull pages, __raw_readl/__raw_writel
NAND read: device 0 offset0x200000, size0x8000008388608 bytes read in 696 ms (11.5 MiB/s): OKOk, this is actually quite a lot. Especially since I already optimized the C code (by not using the helper functions like nfc_set/nfc_clear in
vf610_nfc_send_command), one would think there is now almost no optimization potential. I looked into the disassembled code and could narrow down the issue. Due to the memory barriers, all offsets were calculated on each register access (nfc base to reg base, and add reg offset), multiple instances of:20:
e59cc120ldr ip, [ip, #288] ;0x120
24:e59cc134ldr ip, [ip, #308] ;0x134Quoted 6 lines
I optimized the code again and calculate the offsets manually and access __raw_readl/__raw_writel rather then vf610_nfc_read/write in the
vf610_nfc_send_command(s)function, I get the full speed again:NAND read: device 0 offset
0x200000, size0x8000008388608 bytes read in 687 ms (11.6 MiB/s): OKI think what you have is fine. The 10BM/s versus 11MB/s is not insignificant, but it is not a huge difference. I expect that the driver is already better than the others; especially the Imx-25 was only 7.7MB/s read and 4.6Mb/s write from Linux mtd tests. Although the end user might like to wait 10% less for an image to load.
Also, probably a lot of people care about code size. This is not so much a case for U-Boot as it is usually machine specific and doesn't support several SOCs afaik.
However, the more specific you make the optimization to the platform, the less likely it is to extend well. We also wish to have this work well with different gcc versions and CPUs (PowerPC, etc). The 'readl/writel' handicap the compiler. Although they are more likely to work with a wide variety of buses.
[snip]
Quoted 19 lines · Scott Wood
Quoted 4 lines
raw_writel()is itself something that should only be used for hand-optimized sections. For non-performance-critical code you should use normalwritel()so that you don't need to worry about manually adding I/O barriers.The reason I choosed readl/writel instead of the raw variants is to preserve align with other drivers...
Quoted 9 lines
care. Maybe a comment is fine? It seems the Vybrid is safe for different access sizes, but it is possible that some other CPU might not be able to access this memory via 32/16/8 bit accesses and '
memcpy()' may not be appropriate. It seems that 'natural' size of the NFC controller itself is 32bits and the CPU interface does lane masking. Ie, boot mode documentation talks about remapping 'sram_physical_addr[13:3] = {cpu_addr[11:3],cpu_addr[13:12]}' saying that bits 2,1 are not used (hopefully one based numbers). This is just my guess...Quoted 4 lines
What assumptions do you make how memcpy accesses memory? This latest patch now uses the optimized versions from the kernel... Maybe they even try to access 64-bit width (the NIC interconnect supports 64-bit access)
The
memcpy()itself could use anything. 64bits is possible on AXI/NIC. The 'PBRIDGE' is 64bit, but I think the AIPS/IPS (apparently AIPS means 'AHB-lite to IPS) are 32bit. At least that is the case on the Imx25 which has a different AIPS version. I assumed the 'memcpy()' was using 32bits but this certainly isn't explicit in the code.The majority of the register banks are non-volatile with this controller. Instead of running multiple NAND programming sequences, the controller runs them all for us. Most registers are are mainly like SRAM.
My only point is that the SRAM buffers use the same interface as the main Nand controller register banks. So using 'readl/writel' for the register, but not the SRAM buffers seems inconsistent.
Quoted 8 lines
So to address this inconsistency, I was thinking that we should at least have a comment?
/* BUS: This assumes the BUS is 32 bit accessible. If you are porting
to other systems, this may not be the case.
*/
memcpy(nfc->regs + NFC_MAIN_AREA(0) + c, buf, l);
IMHO, we just treat this as if its memory and I guess this is fine for a buffer. memcpy knows how to copy data, and takes care if the architecture needs aligned access when reading 32-bit width, or similar requirements. We do not know whether memcpy really uses 32-bit accesses, hence this comment might even be wrong. In a short test, I could also access the buffer in byte/word length (tested using md.b/md.w).
Also, I assume this just works for a different architecture too. If not, the one using this driver the first time on a different architecture would see this pretty quickly I guess :-)
Quoted 6 lines
Or we could implement our own version of memcpy that did 32bit aligned transfers with a similar comment. In theory, we need a barrier after the
memcpy(), in case anyone modified the code to touch the NFC_FLASH_CMD2'sSTART_BITdirectly after thememcpy()or custom function. But all the paranoia adds some code and has potential to slow things down.
Well that would be a reordering accross multiple function reads, and many instructions right now. I don't think that this is a valid case to introduce a barrier.
Quoted 4 lines
Doing a barrier after every single byte read as the ARM Linux's
memcpy_fromio()does will surely make significant performance differences. Instead of being double the Imx25, it was half. A user waiting 400% longer won't be too happy.
There are also patches floating around which just use memcpy:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-June/173195.html
In our case, a barrier just after the memcpy would be sufficient.
Somehow your comment and my latest patch revision crossed each other. Could you post your comment on the latest revision in case you are fine with it?
--
Stefan
PATCH v3 3/4 mtd: nand: add Freescale vf610_nfc driver
Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines · stefan at agner.ch
This adds initial support for Freescale NFC (NAND Flash Controller) found in ARM Vybrid SoC's, Power Architecture MPC5125 and others. However, this driver is only tested on Vybrid.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines · Scott Wood
Quoted 5 lines
raw_writel()is itself something that should only be used for hand-optimized sections. For non-performance-critical code you should use normalwritel()so that you don't need to worry about manually adding I/O barriers.Am 2014-08-14 23:12, schrieb Bill Pringlemeir:
[regarding memcpy() in the driver]
Quoted 15 lines
Quoted 15 lines
Quoted 9 lines
Quoted 9 lines
Maybe a comment is fine? It seems the Vybrid is safe for different access sizes, but it is possible that some other CPU might not be able to access this memory via 32/16/8 bit accesses and '
memcpy()' may not be appropriate. It seems that 'natural' size of the NFC controller itself is 32bits and the CPU interface does lane masking. Ie, boot mode documentation talks about remapping 'sram_physical_addr[13:3] = {cpu_addr[11:3],cpu_addr[13:12]}' saying that bits 2,1 are not used (hopefully one based numbers). This is just my guess...Quoted 4 lines · stefan at agner.ch
What assumptions do you make how memcpy accesses memory? This latest patch now uses the optimized versions from the kernel... Maybe they even try to access 64-bit width (the NIC interconnect supports 64-bit access)
[snip]
Quoted 8 lines
Am 2014-08-18 18:38, schrieb Bill Pringlemeir:
Quoted 6 lines
My only point is that the SRAM buffers use the same interface as the main Nand controller register banks. So using 'readl/writel' for the register, but not the SRAM buffers seems inconsistent.
So to address this inconsistency, I was thinking that we should at least have a comment?
Quoted 11 lines · stefan at agner.ch
IMHO, we just treat this as if its memory and I guess this is fine for a buffer. memcpy knows how to copy data, and takes care if the architecture needs aligned access when reading 32-bit width, or similar requirements. We do not know whether memcpy really uses 32-bit accesses, hence this comment might even be wrong. In a short test, I could also access the buffer in byte/word length (tested using md.b/md.w).
Also, I assume this just works for a different architecture too. If not, the one using this driver the first time on a different architecture would see this pretty quickly I guess :-)
[snip]
Quoted 1 line
In our case, a barrier just after the memcpy would be sufficient.
I would suggest you make a 'vf610_nfc_memcpy()' [or even from/to variants if you are pendantic] which can be a wrapper function of just 'memcpy'. Just the like the readl/writel wrappers this will collect the BUS accesses into one place. So they are documented for people porting the code. Trying to accommodate some future insane hardware hookup seems futile beyond this?
Otherwise, I will add an 'Ack' or 'Reviewed-By' from me if you like. I am sorry, I don't know what if anything is appropriate.
Thanks,
Bill Pringlemeir.
PATCH v3 3/4 mtd: nand: add Freescale vf610_nfc driver
Quoted 70 lines · Bill Pringlemeir
Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines · stefan at agner.ch
This adds initial support for Freescale NFC (NAND Flash Controller) found in ARM Vybrid SoC's, Power Architecture MPC5125 and others. However, this driver is only tested on Vybrid.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines · Scott Wood
Quoted 5 lines
raw_writel()is itself something that should only be used for hand-optimized sections. For non-performance-critical code you should use normalwritel()so that you don't need to worry about manually adding I/O barriers.Am 2014-08-14 23:12, schrieb Bill Pringlemeir:
[regarding
memcpy()in the driver]Quoted 15 lines
Quoted 15 lines
Quoted 9 lines
Quoted 9 lines
Maybe a comment is fine? It seems the Vybrid is safe for different access sizes, but it is possible that some other CPU might not be able to access this memory via 32/16/8 bit accesses and '
memcpy()' may not be appropriate. It seems that 'natural' size of the NFC controller itself is 32bits and the CPU interface does lane masking. Ie, boot mode documentation talks about remapping 'sram_physical_addr[13:3] = {cpu_addr[11:3],cpu_addr[13:12]}' saying that bits 2,1 are not used (hopefully one based numbers). This is just my guess...Quoted 4 lines · stefan at agner.ch
What assumptions do you make how memcpy accesses memory? This latest patch now uses the optimized versions from the kernel... Maybe they even try to access 64-bit width (the NIC interconnect supports 64-bit access)
[snip]
Quoted 8 lines
Am 2014-08-18 18:38, schrieb Bill Pringlemeir:
Quoted 6 lines
My only point is that the SRAM buffers use the same interface as the main Nand controller register banks. So using 'readl/writel' for the register, but not the SRAM buffers seems inconsistent.
So to address this inconsistency, I was thinking that we should at least have a comment?
Quoted 11 lines · stefan at agner.ch
IMHO, we just treat this as if its memory and I guess this is fine for a buffer. memcpy knows how to copy data, and takes care if the architecture needs aligned access when reading 32-bit width, or similar requirements. We do not know whether memcpy really uses 32-bit accesses, hence this comment might even be wrong. In a short test, I could also access the buffer in byte/word length (tested using md.b/md.w).
Also, I assume this just works for a different architecture too. If not, the one using this driver the first time on a different architecture would see this pretty quickly I guess :-)
[snip]
Quoted 1 line
In our case, a barrier just after the memcpy would be sufficient.
I would suggest you make a '
vf610_nfc_memcpy()' [or even from/to variants if you are pendantic] which can be a wrapper function of just 'memcpy'. Just the like the readl/writel wrappers this will collect the BUS accesses into one place. So they are documented for people porting the code. Trying to accommodate some future insane hardware hookup seems futile beyond this?
Quoted 2 lines
Otherwise, I will add an 'Ack' or 'Reviewed-By' from me if you like. I am sorry, I don't know what if anything is appropriate.
Both are appropraite. IMHO you are an author and you checked the code after Stefan's porting: ACK should be the best choice,.
Best regards,
Stefano Babic
Signature · 5 lines
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de =====================================================================
PATCH v2 4/4 arm: vf610: add NAND support for vf610twr
In reply to Stefan AgnerThis 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).
--- board/freescale/vf610twr/vf610twr.c | 47 ++++++++++++++++++++++++++++++++++--- configs/vf610twr_defconfig | 2 +- configs/vf610twr_nand_defconfig | 3 +++ include/configs/vf610twr.h | 46 +++++++++++++++++++++++++++++++++++- 4 files changed, 93 insertions(+), 5 deletions(-) create mode 100644 configs/vf610twr_nand_defconfig
board/freescale/vf610twr/vf610twr.c +44 -3
@@ -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_VF610_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_VF610_NFC +setup_iomux_nfc (); +#endif return 0 ; }
configs/vf610twr_defconfig +1 -1
@@ -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
configs/vf610twr_nand_defconfig new file +3 -0
@@ -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
include/configs/vf610twr.h +45 -1
68 lines not on this page · raw message
Signature · 1 line
2.0.4