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