On 10/19/2016 09:39 AM, 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 <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!
I believe there can be[1] SoCs where the architectural by-set/way
instructions are entirely sufficient to flush the dcache. In those
cases, the default no-op implementation of the hook is appropriate.
[1] and likely are; not every existing ARMv8 U-Boot port implements the
current __asm_flush_l3_cache hook, and hopefully they're all correct and
working.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
