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 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.
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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
