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

[Stefano Babic <sbabic@denx.de>]

On 21/08/2014 23:15, Bill Pringlemeir wrote:
> 
>>>>>> 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.

Both are appropraite. IMHO you are an author and you checked the code
after Stefan's porting: ACK should be the best choice,.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================