[U-Boot] [PATCH 1/2] armv8: add hooks for all cache-wide operations

[Tom Rini]

On Wed, Oct 19, 2016 at 09:59:02AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On 19 October 2016 at 09:39, Tom Rini <trini at 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 <swarren at wwwdotorg.org> wrote:
> > > >>On 10/18/2016 05:08 PM, Simon Glass wrote:
> > > >>>
> > > >>>Hi Stephen,
> > > >>>
> > > >>>On 18 October 2016 at 16:54, Stephen Warren <swarren at wwwdotorg.org> wrote:
> > > >>>>
> > > >>>>On 10/18/2016 01:56 PM, Simon Glass wrote:
> > > >>>>>
> > > >>>>>
> > > >>>>>Hi Stephen,
> > > >>>>>
> > > >>>>>On 18 October 2016 at 13:10, Stephen Warren <swarren at wwwdotorg.org>
> > > >>>>>wrote:
> > > >>>>>>
> > > >>>>>>
> > > >>>>>>On 10/18/2016 01:03 PM, Simon Glass wrote:
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>>Hi Stephen,
> > > >>>>>>>
> > > >>>>>>>On 18 October 2016 at 12:58, Stephen Warren <swarren at wwwdotorg.org>
> > > >>>>>>>wrote:
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>On 10/18/2016 10:23 AM, Simon Glass wrote:
> > > >>>>>>>>>
> > > >>>>>>>>>
> > > >>>>>>>>>
> > > >>>>>>>>>
> > > >>>>>>>>>Hi Stephen,
> > > >>>>>>>>>
> > > >>>>>>>>>On 17 October 2016 at 15:35, Stephen Warren <swarren at wwwdotorg.org>
> > > >>>>>>>>>wrote:
> > > >>>>>>>>>>
> > > >>>>>>>>>>
> > > >>>>>>>>>>
> > > >>>>>>>>>>
> > > >>>>>>>>>>From: Stephen Warren <swarren at 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 <swarren at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161019/47ff9b7e/attachment.sig>