| Message ID | 1571909341-10108-3-git-send-email-zhouyanjie@zoho.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] MIPS: Rename JZRISC to XBurst. | expand |
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"? What actually goes wrong on the affected CPUs? Do they misinterpret EntryLo values? Which bits do they misinterpret? > Confirmed by Ingenic, > this operation will not adversely affect processors > without HPTLB function. > > Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com> > --- > 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". > + * 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. > + /* > + * 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? Thanks, Paul
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. 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. > >> Confirmed by Ingenic, >> this operation will not adversely affect processors >> without HPTLB function. >> >> Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com> >> --- >> 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
Hi Zhou, Le sam., nov. 16, 2019 at 18:11, Zhou Yanjie <zhouyanjie@zoho.com> 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. > 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. Cheers, -Paul > >> >>> Confirmed by Ingenic, >>> this operation will not adversely affect processors >>> without HPTLB function. >>> >>> Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com> >>> --- >>> 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 > > >
Hi Paul, On 2019年11月17日 19:49, Paul Cercueil wrote: > Hi Zhou, > > > Le sam., nov. 16, 2019 at 18:11, Zhou Yanjie <zhouyanjie@zoho.com> 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@zoho.com> >>>> --- >>>> 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 >> >> >> > >
Hi Paul, On 2019年11月17日 19:49, Paul Cercueil wrote: > Hi Zhou, > > > Le sam., nov. 16, 2019 at 18:11, Zhou Yanjie <zhouyanjie@zoho.com> 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. > I once again checked with Ingenic. I was wrong before, and VTLB is fully implemented. HPTLB is a custom mode that is not compatible with the MIPS standard. The two are parallel relationship, not the complementary relationship I have previously understood. After the chip reset, the default is HPTLB mode, so we need to write 0xa9000000 to cp0 register 5 sel 4 to switch back to VTLB mode. Thanks and best regards! > >> 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. > > Cheers, > -Paul > > >> >>> >>>> Confirmed by Ingenic, >>>> this operation will not adversely affect processors >>>> without HPTLB function. >>>> >>>> Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com> >>>> --- >>>> 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 >> >> >> > >
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 + * 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)); + /* + * 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) c->isa_level &= ~MIPS_CPU_ISA_M32R2; }
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. Confirmed by Ingenic, this operation will not adversely affect processors without HPTLB function. Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com> --- arch/mips/kernel/cpu-probe.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)