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

[Stephen Warren]

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.