On 21/08/2014 23:15, Bill Pringlemeir wrote:
>
>>>>>> On 14 Aug 2014, ste...@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, ste...@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, ste...@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: sba...@denx.de
=====================================================================
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
