Date:   Wed, 31 Jul 2019 20:34:31 +0000
From:   Paul Burton <paul.burton@...s.com>
To:     Zhou Yanjie <zhouyanjie@...o.com>
CC:     "linux-mips@...r.kernel.org" <linux-mips@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "ralf@...ux-mips.org" <ralf@...ux-mips.org>,
        "paul@...pouillou.net" <paul@...pouillou.net>,
        "jhogan@...nel.org" <jhogan@...nel.org>,
        "malat@...ian.org" <malat@...ian.org>,
        "chenhc@...ote.com" <chenhc@...ote.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "allison@...utok.net" <allison@...utok.net>,
        "syq@...ian.org" <syq@...ian.org>,
        "jiaxun.yang@...goat.com" <jiaxun.yang@...goat.com>
Subject: Re: [PATCH v2] MIPS: Ingenic: Fix bugs when detecting X1000's
 parameters.

Hi Zhou,

On Wed, Jul 31, 2019 at 12:39:03PM +0800, Zhou Yanjie wrote:
> 1.fix bugs when detecting L2 cache sets value.
> 2.fix bugs when detecting L2 cache ways value.
> 3.fix bugs when calculate bogoMips and loops_per_jiffy.

This should be split into 2 patches - one that fixes the L2 cache
detection problems, and possibly one that fixes the bogomips/lpj issue.

> Signed-off-by: Zhou Yanjie <zhouyanjie@...o.com>
> ---
>  arch/mips/include/asm/mipsregs.h |  1 +
>  arch/mips/kernel/cpu-probe.c     |  7 +++++++
>  arch/mips/mm/sc-mips.c           | 18 +++++++++++++++---
>  3 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/mips/include/asm/mipsregs.h b/arch/mips/include/asm/mipsregs.h
> index 1e6966e..01e0fcb 100644
> --- a/arch/mips/include/asm/mipsregs.h
> +++ b/arch/mips/include/asm/mipsregs.h
> @@ -2813,6 +2813,7 @@ __BUILD_SET_C0(status)
>  __BUILD_SET_C0(cause)
>  __BUILD_SET_C0(config)
>  __BUILD_SET_C0(config5)
> +__BUILD_SET_C0(config7)
>  __BUILD_SET_C0(intcontrol)
>  __BUILD_SET_C0(intctl)
>  __BUILD_SET_C0(srsmap)
> diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c
> index eb527a1..547c9a0 100644
> --- a/arch/mips/kernel/cpu-probe.c
> +++ b/arch/mips/kernel/cpu-probe.c
> @@ -1964,6 +1964,13 @@ static inline void cpu_probe_ingenic(struct cpuinfo_mips *c, unsigned int cpu)
>  		c->cputype = CPU_XBURST;
>  		c->writecombine = _CACHE_UNCACHED_ACCELERATED;
>  		__cpu_name[cpu] = "Ingenic XBurst";
> +		/*
> +		 * config7 bit 4 is used to control a low-power mode in
> +		 * XBurst architecture. This mode may cause errors in the
> +		 * calculation of bogomips and loops_per_jiffy, set config7
> +		 * bit 4 to disable this feature to prevent that.
> +		 */
> +		set_c0_config7(BIT(4));

I happen to know what this bit does - see this old patch for an
explanation:

  https://github.com/paulburton/linux/commit/0d72377bd615d00e99733adc0d37e6a2373fcde7

In short it disables a loop optimization in the CPU that is supposed to
special case loops & prevent them from relying upon the BTB.
Unfortunately that loop optimization negatively affects short loops,
such as in __delay(), and Ingenic's vendor kernels have generally set
this bit to disable it.

Note though that bogomips is bogus, so changing the bogomips value is
really not good justification for the patch at all (which is why I've so
far not bothered upstreaming the patch linked above). The best
justification I can think of is that Ingenic set the bit in their
downstream kernels, which presumably means it's beneficial overall (or
just that someone cares too much about bogomips).

In any case, one thing I don't know for sure is which CPU versions are
affected. I don't believe this affected older devices like the JZ4740,
and my copy of the XBurst1 CPU Core Programming Manual documents the bit
as reserved. Given that you're seeing the X1000 is affected, and I know
the JZ4780 is affected, that covers at least 2 different PRIDs so we
can't just check for that.

Hopefully writing to the bit is just a no-op on older systems if it is
actually reserved, but it'd be great if we could test that.

At the very least we should define the bit in asm/mipsregs.h & properly
document what it does - using BIT(4) here may be a little nicer than
(1<<4), but it's still just a magic number. I don't mind if you want to
fix your patch to do that, or one of us can resurrect mine which has
that information already.

>  		break;
>  	default:
>  		panic("Unknown Ingenic Processor ID!");
> diff --git a/arch/mips/mm/sc-mips.c b/arch/mips/mm/sc-mips.c
> index 9385ddb..ed953d4 100644
> --- a/arch/mips/mm/sc-mips.c
> +++ b/arch/mips/mm/sc-mips.c
> @@ -215,6 +215,14 @@ static inline int __init mips_sc_probe(void)
>  	else
>  		return 0;
>  
> +	/*
> +	 * According to config2 it would be 512-sets, but that is contradicted
> +	 * by all documentation.
> +	 */
> +	if (current_cpu_type() == CPU_XBURST &&
> +				mips_machtype == MACH_INGENIC_X1000)
> +		c->scache.sets = 256;
> +
>  	tmp = (config2 >> 0) & 0x0f;
>  	if (tmp <= 7)
>  		c->scache.ways = tmp + 1;
> @@ -225,9 +233,13 @@ static inline int __init mips_sc_probe(void)
>  	 * According to config2 it would be 5-ways, but that is contradicted
>  	 * by all documentation.
>  	 */
> -	if (current_cpu_type() == CPU_XBURST &&
> -				mips_machtype == MACH_INGENIC_JZ4770)
> -		c->scache.ways = 4;
> +	if (current_cpu_type() == CPU_XBURST) {
> +		switch (mips_machtype) {
> +		case MACH_INGENIC_JZ4770:
> +		case MACH_INGENIC_X1000:
> +			c->scache.ways = 4;
> +		}
> +	}
>  
>  	c->scache.waysize = c->scache.sets * c->scache.linesz;
>  	c->scache.waybit = __ffs(c->scache.waysize);

Given that we need to fix up both sets & ways, I think this would be
cleaner if both were done in the switch statement here after we've read
the values from Config2. ie. something like:

	if (current_cpu_type() == CPU_XBURST) {
		switch (mips_machtype) {
		case MACH_INGENIC_JZ4770:
			c->scache.ways = 4;
			break;

		case MACH_INGENIC_X1000:
			c->scache.sets = 256;
			c->scache.ways = 4;
			break;
		}
	}

With appropriate comments added for each machine/SoC.

Thanks,
    Paul

Powered by blists - more mailing lists