On 13 Aug 2014, stefan@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.