On Wed, Oct 19, 2016 at 09:59:02AM -0600, Simon Glass wrote: > Hi Tom, > > On 19 October 2016 at 09:39, Tom Rini <tr...@konsulko.com> wrote: > > > > On Wed, Oct 19, 2016 at 09:36:52AM -0600, Stephen Warren wrote: > > > On 10/18/2016 08:41 PM, Simon Glass wrote: > > > >Hi Stephen, > > > > > > > >On 18 October 2016 at 17:33, Stephen Warren <swar...@wwwdotorg.org> > > > >wrote: > > > >>On 10/18/2016 05:08 PM, Simon Glass wrote: > > > >>> > > > >>>Hi Stephen, > > > >>> > > > >>>On 18 October 2016 at 16:54, Stephen Warren <swar...@wwwdotorg.org> > > > >>>wrote: > > > >>>> > > > >>>>On 10/18/2016 01:56 PM, Simon Glass wrote: > > > >>>>> > > > >>>>> > > > >>>>>Hi Stephen, > > > >>>>> > > > >>>>>On 18 October 2016 at 13:10, Stephen Warren <swar...@wwwdotorg.org> > > > >>>>>wrote: > > > >>>>>> > > > >>>>>> > > > >>>>>>On 10/18/2016 01:03 PM, Simon Glass wrote: > > > >>>>>>> > > > >>>>>>> > > > >>>>>>> > > > >>>>>>>Hi Stephen, > > > >>>>>>> > > > >>>>>>>On 18 October 2016 at 12:58, Stephen Warren <swar...@wwwdotorg.org> > > > >>>>>>>wrote: > > > >>>>>>>> > > > >>>>>>>> > > > >>>>>>>> > > > >>>>>>>>On 10/18/2016 10:23 AM, Simon Glass wrote: > > > >>>>>>>>> > > > >>>>>>>>> > > > >>>>>>>>> > > > >>>>>>>>> > > > >>>>>>>>>Hi Stephen, > > > >>>>>>>>> > > > >>>>>>>>>On 17 October 2016 at 15:35, Stephen Warren > > > >>>>>>>>><swar...@wwwdotorg.org> > > > >>>>>>>>>wrote: > > > >>>>>>>>>> > > > >>>>>>>>>> > > > >>>>>>>>>> > > > >>>>>>>>>> > > > >>>>>>>>>>From: Stephen Warren <swar...@nvidia.com> > > > >>>>>>>>>> > > > >>>>>>>>>>SoC-specific logic may be required for all forms of cache-wide > > > >>>>>>>>>>operations; invalidate and flush of both dcache and icache (note > > > >>>>>>>>>>that > > > >>>>>>>>>>only 3 of the 4 possible combinations make sense, since the > > > >>>>>>>>>>icache > > > >>>>>>>>>>never > > > >>>>>>>>>>contains dirty lines). This patch adds an optional hook for all > > > >>>>>>>>>>implemented cache-wide operations, and renames the one existing > > > >>>>>>>>>>hook > > > >>>>>>>>>>to > > > >>>>>>>>>>better represent exactly which operation it is implementing. A > > > >>>>>>>>>>dummy > > > >>>>>>>>>>no-op implementation of each hook is provided. These dummy > > > >>>>>>>>>>implementations are moved into C code, since there's no need to > > > >>>>>>>>>>implement them in assembly. > > > >>>>>>>>>> > > > >>>>>>>>>>Signed-off-by: Stephen Warren <swar...@nvidia.com> > > > >>>>>>>>>>--- > > > >>>>>>>>>> arch/arm/cpu/armv8/cache.S | 6 ------ > > > >>>>>>>>>> arch/arm/cpu/armv8/cache_v8.c | 23 > > > >>>>>>>>>>++++++++++++++++++++--- > > > >>>>>>>>>> arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S | 4 ++-- > > > >>>>>>>>>> arch/arm/include/asm/system.h | 5 ++++- > > > >>>>>>>>>> arch/arm/mach-tegra/tegra186/cache.c | 2 +- > > > >>>>>>>>>> 5 files changed, 27 insertions(+), 13 deletions(-) > > > >>>>>>>>>> > > > >>>>>>>>> > > > >>>>>>>>>I think we should have a proper interface for this stuff rather > > > >>>>>>>>>than > > > >>>>>>>>>weak functions. Maybe we need a linker-list approach, or a cache > > > >>>>>>>>>uclass? > > > >>>>>>>> > > > >>>>>>>> > > > >>>>>>>> > > > >>>>>>>> > > > >>>>>>>> > > > >>>>>>>>What's improper about this interface? Presumably we could argue > > > >>>>>>>>that > > > >>>>>>>>no > > > >>>>>>>>function in the entire of U-Boot be called by symbol name, which > > > >>>>>>>>doesn't > > > >>>>>>>>seem useful. > > > >>>>>>>> > > > >>>>>>>>I'm not sure exactly what you envisage by a linker-list approach. > > > >>>>>>>>Can > > > >>>>>>>>you > > > >>>>>>>>provide some background? I understand how the linker can construct > > > >>>>>>>>list > > > >>>>>>>>of > > > >>>>>>>>objects/implementations/..., but that doesn't seem useful here > > > >>>>>>>>since > > > >>>>>>>>there's > > > >>>>>>>>a known-ahead-of-time single implementation of these functions in > > > >>>>>>>>a > > > >>>>>>>>single > > > >>>>>>>>build of U-Boot. > > > >>>>>>> > > > >>>>>>> > > > >>>>>>> > > > >>>>>>> > > > >>>>>>>Your own commit messages says that this is SoC-specific. I'm > > > >>>>>>>suggesting that we define an interface (which I think you have > > > >>>>>>>already > > > >>>>>>>done with your header file additions), and allow SoCs to implement > > > >>>>>>>it > > > >>>>>>>via a linker list. > > > >>>>>>> > > > >>>>>>>IMO the cache code in U-Boot is starting to get a bit creaky. > > > >>>>>>> > > > >>>>>>>>A cache uclass seems like /massive/ overkill, especially since I'd > > > >>>>>>>>expect > > > >>>>>>>>these very low-level functions to be required well before any > > > >>>>>>>>higher > > > >>>>>>>>level > > > >>>>>>>>code like DM/classes/... to be available. > > > >>>>>>> > > > >>>>>>> > > > >>>>>>> > > > >>>>>>> > > > >>>>>>>DM is available very early. But it's not clear from your patch when > > > >>>>>>>this code is actually called. > > > >>>>>> > > > >>>>>> > > > >>>>>> > > > >>>>>> > > > >>>>>>I believe that weak functions are a perfectly acceptable approach > > > >>>>>>here. > > > >>>>>> > > > >>>>>>Yes, the implementation of these functions is SoC-specific. The > > > >>>>>>Makefiles > > > >>>>>>will pull in the appropriate implementation for that SoC whenever > > > >>>>>>U-Boot > > > >>>>>>is > > > >>>>>>built, just like every other board- or SoC-specific function in the > > > >>>>>>entire > > > >>>>>>of U-Boot. > > > >>>>>> > > > >>>>>>There's no need for linker lists since there is only ever one > > > >>>>>>implementation. > > > >>>>> > > > >>>>> > > > >>>>> > > > >>>>>If there is only ever one implementation, why do you need weak > > > >>>>>functions? > > > >>>> > > > >>>> > > > >>>> > > > >>>>As I explicitly stated above, each SoC can have a different > > > >>>>implementation, > > > >>>>yet only a single implementation is ever needed for a particular > > > >>>>U-Boot > > > >>>>build. > > > >>>> > > > >>>>>Just call them directly. > > > >>>> > > > >>>> > > > >>>> > > > >>>>The code is doing that, both before and after my patch. > > > >>> > > > >>> > > > >>>I mean call them without needing weak functions. > > > >>> > > > >>>> > > > >>>>>I think in fact you mean that > > > >>>>>there can be no implementation (but perhaps an empty one), or one > > > >>>>>implementation. You are effectively using multiple weak functions to > > > >>>>>provide default code. I think it would be better if this were > > > >>>>>explicit. > > > >>>>> > > > >>>>>I still think that the cache functions could do with a rethink. > > > >>>> > > > >>>> > > > >>>> > > > >>>>In my opinion, this patch doesn't change the code structure at all. > > > >>>>There > > > >>>>is > > > >>>>already an interface between the core (L1/L2) cache management code > > > >>>>and > > > >>>>the > > > >>>>SoC-specific cache management code. That interface already uses a weak > > > >>>>function to provide the default no-op implementation. This patch > > > >>>>doesn't > > > >>>>change any of that. All this patch does is fix that existing > > > >>>>interface to > > > >>>>cover all 3 combinations of dcache_flush, dcache_invalidate, and > > > >>>>icache_invalidate, rather than just one of those combinations. It's > > > >>>>more > > > >>>>of > > > >>>>a bug-fix than anything else. > > > >>> > > > >>> > > > >>>Yes I see that. > > > >>> > > > >>>> > > > >>>>If you want to rework this interface sometime, be my guest. However, I > > > >>>>don't > > > >>>>think it's fair to require that someone who simply wants to fix the > > > >>>>existing > > > >>>>code (in a way that is orthogonal to your desired interface > > > >>>>refactoring) > > > >>>>do > > > >>>>that refactoring first, rather than doing it yourself. > > > >>> > > > >>> > > > >>>I understand what you are saying, but isn't that how open source > > > >>>software works? Believe me, I have done my fair share of refactoring > > > >>>:-) > > > >>> > > > >>>At least can you look at not making it any harder to fix up later? The > > > >>>more we pile onto this interface, the harder it will be later. We > > > >>>should aim to make ARMv8 really nice as it is the new thing. > > > >> > > > >> > > > >>I believe moving one or three functions into any replacement scheme will > > > >>have an identical level of difficulty. So, I believe the patch already > > > >>satisfies the "no harder" requirement. > > > > > > > >Well it seems your mind is already made up! > > > > > > Well, I don't believe there are any viable or reasonable > > > alternatives. I'm not prolonging this thread because I find it > > > enjoyable, but because of the lack of alternatives to what this > > > patch does. > > > > > > Doing this via DM doesn't simplify anything or make it more > > > maintainable; it's just using DM for the sake of it. DM is great > > > when it makes life simpler and code more maintainable, but we use it > > > because of those benefits, not just for the sake of it. Using DM > > > adds complexity, and so there has to be a benefit of doing so. I > > > don't believe there is here. > > > > > > Doing this by linker scripts simply doesn't make sense: > > > > > > Any given U-Boot binary will only contain a single implementation of > > > these functions, so there's no need for any kind of runtime lookup, > > > and if there was a runtime lookup, what would be the key and where > > > would it come from? I believe we'd still have some > > > compile-time-seledted function/data to drive the runtime lookup > > > process, and so we'd be in exactly the same situation as we already > > > are, just with more complexity added on top. > > > > > > Using linker scripts to switch between implementations at compile > > > time is much more complex than just letting the linker link stuff > > > together directly. That's what it's good at. Using linker scripts > > > would just add extra complexity without any benefit. We'd still end > > > up with a single implementation in the binary, yet to call it code > > > would have to indirect through the linker-defined table, rather than > > > simply calling the same code by symbol name. Uggh! > > > > > > Note that per the discussions in other forks of this thread, it's > > > likely we'll need to code all these cache operations in assembly to > > > avoid accessing DRAM while turning the cache on/off. This implies to > > > me that we'll need to keep all the cache code extremely simple and > > > direct, without any form of runtime or compile time indirection. > > > > For the record, until / unless we move to the point where we can run > > different SoCs within a single binary, doing this via weak functions > > seems to me to be the correct abstraction. If we know that some SoCs > > will be able to nop these, that is. If all SoCs will need something, we > > should simply omit the default functions. Thanks! > > Is that a goal? I can see it would be useful to be able to build for > multiple SoCs (i.e. avoid having to worry about what board you have) > but we are a way off from that :-)
We're well off from seeing about the reality of that, yes :) -- Tom
signature.asc
Description: Digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
