[U-Boot] [PATCH 3/4] mtd: nand: add Freescale NFC driver

[Scott Wood <scottwood@freescale.com>]

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