Re: [PATCH v2 07/16] LoongArch: lib: General routines

[Yao Zi <me@ziyao.cc>]

On Thu, Jul 02, 2026 at 11:28:08AM +0100, Simon Glass wrote:
> Hi Yao,
> 
> On 2026-07-01T11:17:53, Yao Zi <me@ziyao.cc> wrote:
> > LoongArch: lib: General routines
> >
> > Add some common library routines for the architecture.
> >
> > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> > Signed-off-by: Yao Zi <me@ziyao.cc>
> >
> > arch/loongarch/include/asm/cache.h       |  10 ++
> >  arch/loongarch/include/asm/global_data.h |   7 +
> >  arch/loongarch/lib/Makefile              |   7 +
> >  arch/loongarch/lib/asm-offsets.c         |  66 ++++++++++
> >  arch/loongarch/lib/boot.c                |  14 ++
> >  arch/loongarch/lib/cache.c               | 213 +++++++++++++++++++++++++++++++
> >  arch/loongarch/lib/reset.c               |  14 ++
> >  arch/loongarch/lib/setjmp.S              |  52 ++++++++
> >  8 files changed, 383 insertions(+)
> 
> > Add some common library routines for the architecture.

...

> > diff --git a/arch/loongarch/lib/cache.c b/arch/loongarch/lib/cache.c
> > @@ -0,0 +1,213 @@
> > +static inline void flush_cache_line_index(unsigned int index,
> > +                                       unsigned long addr)
> > +{
> > +#define do_flush(index)                                                      \
> > +     case index:                                                     \
> > +             cache_op(FIELD_PREP(CACHE_OP, CACHE_INDEX_INVWB) |      \
> > +                      FIELD_PREP(CACHE_INDEX, index),                \
> > +                      index);                                        \
> > +             break;
> > +
> > +     switch (index) {
> > +             do_flush(0);
> > +             do_flush(1);
> > +             do_flush(2);
> > +             do_flush(3);
> > +             do_flush(4);
> > +             do_flush(5);
> > +     }
> > +
> > +#undef do_flush
> > +}
> 
> addr is silently dropped. Inside do_flush() the token index
> resolves to the macro parameter (0..5, the cache identifier), so
> cache_op() always gets the small constant as its address operand.
> The way/set offset that flush_dcache_level_all() computes never
> reaches the CACOP instruction, so this hammers way 0 / set 0 of
> the selected cache repeatedly instead of walking it. Please pass
> addr through (rename the macro parameter to avoid the shadow), and
> confirm with a real by-index flush test.

Oops, yes. Thanks for pointing out.

> > diff --git a/arch/loongarch/lib/cache.c b/arch/loongarch/lib/cache.c
> > @@ -0,0 +1,213 @@
> > +void probe_caches(void)
> > +{
> > +     unsigned int level = 0, index = 0;
> > +     u32 cfg = read_cpucfg(LOONGARCH_CPUCFG16);
> > +
> > +     if (cfg & CPUCFG16_L1_IUPRE) {
> > +             if (cfg & CPUCFG16_L1_IUUNIFY) {
> > +                     gd->arch.dcache_index[level] = index;
> > +                     populate_dcache_properties(cfg, level++,
> > +                                                LOONGARCH_CPUCFG17);
> > +             }
> > +
> > +             index++;
> > +     }
> > +
> > +     if (cfg & CPUCFG16_L1_DPRE) {
> > +             gd->arch.dcache_index[level] = index++;
> > +             populate_dcache_properties(cfg, level++, LOONGARCH_CPUCFG18);
> > +     }
> > +
> > +     cfg >>= 3;
> > +
> > +     for (; level < 3; level++) {
> 
> Please use CACHE_MAX_LEVEL rather than the bare 3.

Ok.

> populate_dcache_properties() also sets dcache_inclusive from cfg
> whenever level != 0, but on the L1D path cfg is still CPUCFG16 -
> the L1 inclusive bit describes L1I vs L1D, not L1D vs L2, so this
> looks incorrect. Please double-check.

The numbering logic for caches used by CACOP are quite confusing, and I
think it deserves some comments in the file. For CACOP instructions, it
indexes caches in the following order,

- L1I or L1 unified
- L1D
- L2I or L2 unified
- L2D
- L3I or L3 unified
- L3D

The index only counts caches present on the system, e.g., for a system
with L1I, L1D, L2 unified and L3 unified, they're respectively indexed
through 0-3 instead of 0, 1, 2, 4.

Furthermore, current architecture definition provides no cpucfg
register for L2D/L3D information L3D, so the current code assumes L2
and L3, if present, must be unified or instruction-only.

Back in the logic, L1_IUPRE means either L1I or L1 unified is present,
and "index" is increased in this case, but "level" is only increased if
L1_IUUNIFY is set, which means L1 is unified and L1_DPRE must not be
set. So populate_dcache_properties() would never be fired to probe
L1D with level = 1.

I'll add some comments on this in v3.

...

> Simon

Best regards,
Yao Zi