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

[Ilias Apalodimas]

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