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

[Stephen Warren]

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 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.