[PATCH 1/3] cmd: avoid duplicate weak flush_dcache_all()

[Heinrich Schuchardt]

On 19.06.24 15:19, Ilias Apalodimas wrote:
> On Wed, 19 Jun 2024 at 16:05, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
>>
>> On Wed, 19 Jun 2024 at 15:36, Heinrich Schuchardt
>> <heinrich.schuchardt at canonical.com> wrote:
>>>
>>> On 19.06.24 14:23, Ilias Apalodimas wrote:
>>>> On Sun, 16 Jun 2024 at 20:31, Heinrich Schuchardt
>>>> <heinrich.schuchardt at canonical.com> wrote:
>>>>>
>>>>> If we have multiple weak implementations of functions, the linker might
>>>>> choose any of these. ARM and RISC-V already provide a weak implementation
>>>>> of flush_dcache_all().
>>>>>
>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>>>>> ---
>>>>>    cmd/cache.c | 3 +++
>>>>>    1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/cmd/cache.c b/cmd/cache.c
>>>>> index 0254ff17f9b..16fa0f7c652 100644
>>>>> --- a/cmd/cache.c
>>>>> +++ b/cmd/cache.c
>>>>> @@ -52,11 +52,14 @@ static int do_icache(struct cmd_tbl *cmdtp, int flag, int argc,
>>>>>           return 0;
>>>>>    }
>>>>>
>>>>> +/* ARM and RISC-V define a weak flush_dcache_all() themselves. */
>>>>> +#if !defined(CONFIG_ARM) && !defined(CONFIG_RISCV)
>>>>>    void __weak flush_dcache_all(void)
>>>>>    {
>>>>>           puts("No arch specific flush_dcache_all available!\n");
>>>>>           /* please define arch specific flush_dcache_all */
>>>>>    }
>>>>
>>>> Aren't we supposed to add a single __weak function so the linker can
>>>> replace it? IOW why is the declaration for Arm/riscv a weak one?
>>>
>>> Some sub-architectures override the architecture specific weak
>>> implementation, e.g.
>>>
>>> arch/riscv/cpu/andes/cache.c:44:
>>> void flush_dcache_all(void)
>>>
>>> arch/arm/cpu/arm926ejs/cache.c:17:
>>> void flush_dcache_all(void)
>>
>> Ok, this does fix a problem, but afaict this is a band-aid and the
>> cache management is a mess overall -- e.g invalidate_icache_all() will
>> suffer from the same issue if a sub-architecture decides to define its
>> own in the future.
>>
>> Would it be less bad to define
>> static inline __weak flush_dcache_all(void)
>> {
>> }
>> static inline __weak flush_icache_all(void)
>> {
>> }
> 
> ugh, this but without the inline!
> 
> Thanks
> /Ilias

GCC does not allow both a weak and a strong implementation in the same 
module:

   CC      board/sandbox/sandbox.o
arch/sandbox/cpu/cache.c:15:6: error: redefinition of 
‘invalidate_icache_all’
    15 | void invalidate_icache_all(void)
       |      ^~~~~~~~~~~~~~~~~~~~~
In file included from arch/sandbox/cpu/cache.c:6:
include/cpu_func.h:74:13: note: previous definition of 
‘invalidate_icache_all’ with type ‘void(void)’
    74 | void __weak invalidate_icache_all(void)
       |             ^~~~~~~~~~~~~~~~~~~~~

Best regards

Heinrich

>> in include/cpu_func.h instead of a random cmd file?
>> We can only define those if !CONFIG_ARM && !!CONFIG_RISCV and add a
>> comment on why.
>> It's kind of putting lipstick on a pig, but at least we'll have them
>> gathered in a single header file and know what we have to fix.
>>
>> Cheers
>> /Ilias
>>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>>
>>>> Thanks
>>>> /Ilias
>>>>> +#endif
>>>>>
>>>>>    static int do_dcache(struct cmd_tbl *cmdtp, int flag, int argc,
>>>>>                        char *const argv[])
>>>>> --
>>>>> 2.43.0
>>>>>
>>>