From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Wed, 19 Oct 2016 09:59:02 -0600 Subject: [U-Boot] [PATCH 1/2] armv8: add hooks for all cache-wide operations In-Reply-To: <20161019153920.GP18591@bill-the-cat> References: <2eb98062-bf22-2a49-ea13-701f3dd65c4e@wwwdotorg.org> <3b4e7932-ea89-5c5f-9fb1-6ad64e5466b5@wwwdotorg.org> <8dcb9b7d-926d-dbc0-3748-6ea6efc21adb@wwwdotorg.org> <20161019153920.GP18591@bill-the-cat> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Tom, On 19 October 2016 at 09:39, Tom Rini 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 wrote: > > >>On 10/18/2016 05:08 PM, Simon Glass wrote: > > >>> > > >>>Hi Stephen, > > >>> > > >>>On 18 October 2016 at 16:54, Stephen Warren wrote: > > >>>> > > >>>>On 10/18/2016 01:56 PM, Simon Glass wrote: > > >>>>> > > >>>>> > > >>>>>Hi Stephen, > > >>>>> > > >>>>>On 18 October 2016 at 13:10, Stephen Warren > > >>>>>wrote: > > >>>>>> > > >>>>>> > > >>>>>>On 10/18/2016 01:03 PM, Simon Glass wrote: > > >>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>>>>Hi Stephen, > > >>>>>>> > > >>>>>>>On 18 October 2016 at 12:58, Stephen Warren > > >>>>>>>wrote: > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> > > >>>>>>>>On 10/18/2016 10:23 AM, Simon Glass wrote: > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>>>Hi Stephen, > > >>>>>>>>> > > >>>>>>>>>On 17 October 2016 at 15:35, Stephen Warren > > >>>>>>>>>wrote: > > >>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>>From: Stephen Warren > > >>>>>>>>>> > > >>>>>>>>>>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 > > >>>>>>>>>>--- > > >>>>>>>>>> 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 :-) Regards, Simon