Re: [PATCH 4/4] MIPS: loongson64: fix FTLB configuration

[Huang Pei <huangpei@loongson.cn>]

On Tue, Nov 30, 2021 at 11:34:48AM +0100, Thomas Bogendoerfer wrote:
> On Fri, Nov 26, 2021 at 11:13:19AM +0800, Huang Pei wrote:
> > On Thu, Nov 25, 2021 at 04:55:28PM +0100, Thomas Bogendoerfer wrote:
> > > On Thu, Nov 25, 2021 at 06:59:49PM +0800, Huang Pei wrote:
> > > > It turns out that 'decode_configs' -> 'set_ftlb_enable' is called under
> > > > c->cputype unset, which leaves FTLB disabled on BOTH 3A2000 and 3A3000
> > > > 
> > > > Fix it by calling "decode_configs" after c->cputype is initialized
> > > > 
> > > > Fixes: da1bd29742b1 ("MIPS: Loongson64: Probe CPU features via CPUCFG")
> > > > Signed-off-by: Huang Pei <huangpei@loongson.cn>
> > > > ---
> > > >  arch/mips/kernel/cpu-probe.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c
> > > > index ac0e2cfc6d57..24a529c6c4be 100644
> > > > --- a/arch/mips/kernel/cpu-probe.c
> > > > +++ b/arch/mips/kernel/cpu-probe.c
> > > > @@ -1734,8 +1734,6 @@ static inline void decode_cpucfg(struct cpuinfo_mips *c)
> > > >  
> > > >  static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu)
> > > >  {
> > > > -	decode_configs(c);
> > > > -
> > > >  	/* All Loongson processors covered here define ExcCode 16 as GSExc. */
> > > >  	c->options |= MIPS_CPU_GSEXCEX;
> > > >  
> > > > @@ -1796,6 +1794,8 @@ static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu)
> > > >  		panic("Unknown Loongson Processor ID!");
> > > >  		break;
> > > >  	}
> > > > +
> > > > +	decode_configs(c);
> > > >  }
> > > >  #else
> > > >  static inline void cpu_probe_loongson(struct cpuinfo_mips *c, unsigned int cpu) { }
> > > > -- 
> > > > 2.20.1
> > > 
> > > applied to mips-fixes.
> > > 
> > > Thomas.
> > > 
> > > -- 
> > > Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> > > good idea.                                                [ RFC1925, 2.3 ]
> > Hi, Thomas,
> > 
> > What about PATCH 1/4, without it, kernel/trace/ring_buffer.i using
> > local_add_return, like this 
> > --------------------------------------------------------------------------------
> >     __asm__ __volatile__(
> > 	"     .set    push                                    \n"
> > 	"     .set    ""arch=r4000""                  \n"
> > 	".if (( 0x00 ) != -1) && ( (1 << 31) ); .set push; .set mips64r2;.rept 1; sync 
> > 	0x00; .endr; .set pop; .else; ; .endif" "\n"
> > 	"1:" "lld     " "%1, %2               # local_add_return\n"
> > 	"     addu    %0, %1, %3				\n"
> > 	"scd " "%0, %2						\n"
> > 	"     beqz    %0, 1b					\n"
> > 	"     addu    %0, %1, %3				\n"
> > 	"     .set    pop
> > 	\n"
> > 	: "=&r" (result), "=&r" (temp), "=m" (l->a.counter)
> > 	: "Ir" (i), "m" (l->a.counter)
> > 	: "memory");
> > } else if (1) {
> >   unsigned long temp;
> > 
> >     __asm__ __volatile__(
> > 	    "     .set    push                                    \n"
> > 	    "     .set    ""arch=r4000""                  \n"
> > 	    ".if (( 0x00 ) != -1) && ( (1 << 31) ); .set push; .set
> > 	    mips64r2; .rept 1; sync 0x00; .endr; .set pop; .else; ;
> > 	    .endif" "                    \n"
> > 	    "1:" "lld     " "%1, %2               # local_add_return
> > 	    \n"
> > 	    "     addu    %0, %1, %3                              \n"
> > 	    "scd " "%0, %2
> > 	    \n"
> > 	    "     beqz    %0, 1b
> > 	    \n"
> > 	    "     addu    %0, %1, %3
> > 	    \n"
> > 	    "     .set    pop
> > 	    \n"
> > 	    : "=&r" (result), "=&r" (temp), "=m"
> > 	    (l->a.counter)
> > 	    : "Ir" (i), "m" (l->a.counter)
> > 	    : "memory");
> > 
> > --------------------------------------------------------------------------------
> > it is wrong here, "lld" + "addu"
> 
> it fixes something, but I didn't see the impact from the commit message.
> Because there is no Fixes tag, I haven't applied it tomips-fixes.
> 
Sorry, this bug is introduced by 7232311ef14c274d88871212a07557f18f4140d1
> And
> 
> +#elif MIPS_ISA_REV >= 6
> +# define SC_BEQZ	beqzc
> 
> why are you doing this ?
This copies from arch/mips/include/asm/llsc.h, I had another patch which
remove llsc.h. I do not want add that patch into bugfix;

> > and PATCH 2, any comment?
> 
> parameter page already contains the virtual address of the page to
> flush, so there is nothing to fix. I'm not 100% about the cache
> nature of all TX39 core, so leaving the check for cpu_has_dc_aliases
> in place is the safer bet. And this platform is nearly dead, so I'm
> sure sure whether this sort of cosemtics is needed.
> 
Based on the comment, I can see TX39 want to bypass TLB by Indexed Cache
Operation,  because with KSEG0 address, Indexed Cache Operation does not 
pass through TLB; But there are two limitation:

+. KSEG0 only cover first 512MB physical address, so do not support 
High Memory, if not using KSEG0 address ,then it passes through TLB, which
is not intended;

+. Indexed Cache Operation can overkill cache line with same cache index
from different way, but it can not kill Cache Alias, since Cache Alias
actually has different cache index;

From datasheet of TX39xx, I know the R3900 core of TX39 series has only
4KB direct-mapped Icache and 2KB two-way Dcache, so it has no Cache 
Alias under 4KB page size, and I can assume it does not support memory
beyond 512MB based on Kconfig of TX39, AKA no High Memory;

Above all, I think it is OK to both use KSEG0 address and remove cpu_has_dc_alias 

> Thomas.
> 
> -- 
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]