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

[Simon Glass]

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? Just call them directly. 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.

Regards,
Simon