Re: [PATCH 2/2] MIPS: Ingenic: Disable abandoned HPTLB function.

[Zhou Yanjie]

Hi Paul,

On 2019å11æ17æ 19:49, Paul Cercueil wrote:
> Hi Zhou,
>
>
> Le sam., nov. 16, 2019 at 18:11, Zhou Yanjie <zhouyanjie@xxxxxxxx> a 
> Ãcrit :
> > Hi Paul,
> >
> > On 2019å11æ16æ 05:37, Paul Burton wrote:
> > > Hi Zhou,
> > >
> > > On Thu, Oct 24, 2019 at 05:29:01PM +0800, Zhou Yanjie wrote:
> > > > JZ4760/JZ4770/JZ4775/X1000/X1500 has an abandoned huge page
> > > > tlb, write 0xa9000000 to cp0 config5 sel4 to disable this
> > > > function to prevent getting stuck.
> > > Can you describe how we "get stuck"?
> >
> > When the kernel is started, it will be stuck in the "Run /init as 
> > init process"
> > according to the log information. After using the debug probe, it is 
> > found
> > that tlbmiss occurred when the run init was started, and entered the 
> > infinite
> > loop in the "tlb-funcs.S".
> >
> > > What actually goes wrong on the
> > > affected CPUs? Do they misinterpret EntryLo values? Which bits do they
> > > misinterpret?
> >
> > According to Ingenic's explanation, this is because the 
> > JZ4760/JZ4770/JZ4775/X1000
> > use the same core (both belong to PRID_COMP_INGENIC_D1). This core is 
> > not fully
> > implemented in VTLB at design time, but only implements the 4K page 
> > mode.
>
> Actually hugepages work fine on all Ingenic SoCs I tested with, from 
> JZ4740 upwards, with the VTLB, so this is incorrect.

It may be that I have misunderstood their explanation. I will check this 
with Ingenic
again tomorrow. However, one thing is certain: these chips default to 
HPTLB mode
after power-on, which will cause the kernel to be stuck (tested on 
JZ4770/JZ4775/X1000).
Then need to shutdown HPTLB mode and use VTLB to start normally.

> > Support for larger pages was implemented by a component called HPTLB 
> > that
> > they designed themselves, but this component was later discarded, so 
> > write
> > 0xa9000000 to cp0 register5 sel4 to turn off HPTLB mode and return to 
> > VTLB
> > mode. The actual test also shows that the kernel will no longer be 
> > stuck in
> > the "Run / init as init process" after shutting down the HPTLB mode, 
> > and can
> > boot to the shell normally.
>
> That's good info, please consider adding that in the comment and in 
> the commit message, and maybe also change the last sentence to reflect 
> what's actually going on with the infinite loop after the tlbmiss.

OK, I will add them to the v3's comment and commit message.

Thanks and best regardsï

> Cheers,
> -Paul
>
>
> > > > Confirmed by Ingenic,
> > > > this operation will not adversely affect processors
> > > > without HPTLB function.
> > > >
> > > > Signed-off-by: Zhou Yanjie <zhouyanjie@xxxxxxxx>
> > > > ---
> > > >   arch/mips/kernel/cpu-probe.c | 16 ++++++++++++++--
> > > >   1 file changed, 14 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/mips/kernel/cpu-probe.c 
> > > > b/arch/mips/kernel/cpu-probe.c
> > > > index 16033a4..cfebf8c 100644
> > > > --- a/arch/mips/kernel/cpu-probe.c
> > > > +++ b/arch/mips/kernel/cpu-probe.c
> > > > @@ -1966,11 +1966,23 @@ static inline void cpu_probe_ingenic(struct 
> > > > cpuinfo_mips *c, unsigned int cpu)
> > > >       }
> > > >         /*
> > > > -     * The config0 register in the Xburst CPUs with a processor ID of
> > > > +     * The config0 register in the XBurst CPUs with a processor ID of
> > > > +     * PRID_COMP_INGENIC_D1 has an abandoned huge page tlb, write
> > > > +     * 0xa9000000 to cp0 config5 sel4 to disable this function to
> > > Saying "config5" suggests $16 sel 5 to me - Config5 is after all an
> > > architecturally defined register & it's not this one. It'd be better to
> > > say "cop0 register 5 sel 4".
> >
> > Sure, I'll change it in v2.
> >
> > > > +     * prevent getting stuck.
> > > > +     */
> > > > +    if ((c->processor_id & PRID_COMP_MASK) == PRID_COMP_INGENIC_D1) {
> > > > +        __asm__ (
> > > > +            "li    $2, 0xa9000000 \n\t"
> > > > +            "mtc0  $2, $5, 4      \n\t"
> > > > +            "nop                  \n\t"
> > > > +            ::"r"(2));
> > > I'd prefer that you add #defines to asm/mipsregs.h to provide a
> > > write_c0_X() function where X is replaced with whatever the name of 
> > > this
> > > register is, and preferably also #define macros describing the fields
> > > present in the register. Writing a magic number isn't ideal.
> >
> > Sure, I'll change it in v2.
> >
> > > > +    /*
> > > > +     * The config0 register in the XBurst CPUs with a processor ID of
> > > >        * PRID_COMP_INGENIC_D0 report themselves as MIPS32r2 
> > > > compatible,
> > > >        * but they don't actually support this ISA.
> > > >        */
> > > > -    if ((c->processor_id & PRID_COMP_MASK) == PRID_COMP_INGENIC_D0)
> > > > +    } else if ((c->processor_id & PRID_COMP_MASK) == 
> > > > PRID_COMP_INGENIC_D0)
> > > It might be cleaner to use a switch statement rather than writing out
> > > the & PRID_COMP_MASK condition twice?
> >
> > Sure, I'll change it in v2.
> >
> > Thanks and best regards!
> >
> > > Thanks,
> > >      Paul