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

[Bill Pringlemeir <bpringlemeir@nbsps.com>]

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.