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