Re: [PATCH] MIPS: Tidy up CP0.Config6 bits definition

From: WANG Xuerui <kernel@xen0n.name>
To: Huacai Chen <chenhc@lemote.com>,
	Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: linux-mips@vger.kernel.org, Fuxin Zhang <zhangfx@lemote.com>,
	Zhangjin Wu <wuzhangjin@gmail.com>,
	Huacai Chen <chenhuacai@gmail.com>,
	Jiaxun Yang <jiaxun.yang@flygoat.com>
Subject: Re: [PATCH] MIPS: Tidy up CP0.Config6 bits definition
Date: Sat, 23 May 2020 12:42:56 +0800	[thread overview]
Message-ID: <7e1a0ef8-6feb-3440-9e30-45cd912194a3@xen0n.name> (raw)
In-Reply-To: <1590207940-20157-1-git-send-email-chenhc@lemote.com>

Hi Huacai,

On 5/23/20 12:25 PM, Huacai Chen wrote:
> CP0.Config6 is a Vendor-defined register whose bits definitions are
> different from one to another. Recently, Xuerui's Loongson-3 patch and
> Serge's P5600 patch make the definitions inconsistency and unclear. To
> make life easy, this patch tidy the definition up.
>
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> ---
>   arch/mips/include/asm/mipsregs.h | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/arch/mips/include/asm/mipsregs.h b/arch/mips/include/asm/mipsregs.h
> index fe6293f..e89eeb9 100644
> --- a/arch/mips/include/asm/mipsregs.h
> +++ b/arch/mips/include/asm/mipsregs.h
> @@ -690,6 +690,12 @@
>   #define MIPS_CONF6_JRCD		(_ULCAST_(1) << 0)
>   /* MIPSr6 extensions enable */
>   #define MIPS_CONF6_R6		(_ULCAST_(1) << 2)
> +/* Loongson-3 internal timer bit */
> +#define MIPS_CONF6_INTIMER	(_ULCAST_(1) << 6)
> +/* Loongson-3 external timer bit */
> +#define MIPS_CONF6_EXTIMER	(_ULCAST_(1) << 7)

These two are not present before, maybe split into two patches?

Also, actually this register is called GSConfig in Loongson's manuals; 
the register bears no resemblance to the actual Config6 on P5600 and the 
likes, it just happens to occupy the same position. So maintaining the 
sorting order actually hurts readability and maintainability IMO.

Maybe reflect this (sad or not, but things happen) truth through naming, 
and group the Loongson bits together?

> +/* Loongson-3 SFB on/off bit */
> +#define MIPS_CONF6_SFBEN	(_ULCAST_(1) << 8)
This bit is called "STFill" in Loongson 3A3000/3B3000 User Manual Volume 
2. Is Loongson renaming things between releases?
>   /* IFU Performance Control */
>   #define MIPS_CONF6_IFUPERFCTL	(_ULCAST_(3) << 10)
>   #define MIPS_CONF6_SYND		(_ULCAST_(1) << 13)
> @@ -697,16 +703,16 @@
>   #define MIPS_CONF6_SPCD		(_ULCAST_(1) << 14)
>   /* proAptiv FTLB on/off bit */
>   #define MIPS_CONF6_FTLBEN	(_ULCAST_(1) << 15)
> +/* Loongson-3's LL on exclusive cacheline */
> +#define MIPS_CONF6_LLEXC	(_ULCAST_(1) << 16)
> +/* Loongson-3's SC has a random delay */
> +#define MIPS_CONF6_SCRAND	(_ULCAST_(1) << 17)
>   /* Disable load/store bonding */
>   #define MIPS_CONF6_DLSB		(_ULCAST_(1) << 21)
>   /* Loongson-3 FTLB on/off bit */
>   #define MIPS_CONF6_FTLBDIS	(_ULCAST_(1) << 22)
>   /* FTLB probability bits */
>   #define MIPS_CONF6_FTLBP_SHIFT	(16)
> -/* Loongson-3 feature bits */
> -#define MIPS_CONF6_LOONGSON_SCRAND	(_ULCAST_(1) << 17)
> -#define MIPS_CONF6_LOONGSON_LLEXC	(_ULCAST_(1) << 16)
> -#define MIPS_CONF6_LOONGSON_STFILL	(_ULCAST_(1) << 8)
>   
>   #define MIPS_CONF7_WII		(_ULCAST_(1) << 31)
>   