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

[Bill Pringlemeir]

>>>>> On 14 Aug 2014, stefan at agner.ch wrote:

>>>>> 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.

>>> On Wed, 2014-08-13 at 22:32, Scott Wood wrote:

>>>> raw_writel() is itself something that should only be used for
>>>> hand-optimized sections.  For non-performance-critical code you
>>>> should use normal writel() 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]

>>>> 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...

>> On 18 Aug 2014, stefan at agner.ch wrote:
>>> 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]

> Am 2014-08-18 18:38, schrieb Bill Pringlemeir:

>> 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?

On 19 Aug 2014, stefan at agner.ch wrote:

> 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]

> 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.