Re: [PATCH v2] MIPS: Add basic R5900 support

["Maciej W. Rozycki" <macro@imgtec.com>]

Hi Fredrik,

 Thank you for the updated patch.

On Sat, 2 Sep 2017, Fredrik Noring wrote:

> Signed-off-by: Fredrik Noring <noring@nocrew.org>

 Please add at least a terse description of what the change actually does.

> Here is revised patch. I've added arch/mips/mm/c-r4k.c and I'm unsure about
> 
> 	c->options |= MIPS_CPU_CACHE_CDEX_P | MIPS_CPU_PREFETCH;
> 
> but similar architectures have MIPS_CPU_CACHE_CDEX_P and R5900 has a PREF
> instruction.

 I'm assuming the R5900 is like the TX79 here.

 If adding the MIPS_CPU_PREFETCH flag, you also need to update 
`set_prefetch_parameters' (in arch/mips/mm/page.c) accordingly to have a 
case for the R5900 as it does not support the Pref_LoadStreamed and 
Pref_PrepareForStore operations the default case requires.  As this is an 
optimisation only I think the whole PREF support for the R5900 will best 
be deferred to a separate later patch.

 As to the MIPS_CPU_CACHE_CDEX_P, the manual is clear that the CPU does 
not support the Create Dirty Exclusive (Create_Dirty_Excl_D ak 0x0d) 
operation, and furthermore all the cache ops use encodings different from 
what the usual are.  So you'll have to refactor all the cache handling to 
take this into account.

> As indicated in the comment, it's not entirely clear why
> 
> 	if (c->dcache.waysize > PAGE_SIZE)
> 		c->dcache.flags |= MIPS_CACHE_ALIASES;
> 
> is necessary.

 Again, the manual is clear here:

"C790 Programming Note:

   Overlapping of the cache index bit range and PFN bit range causes the 
   "cache aliasing problem".  C790 does not have any hardware mechanisms 
   to detect the cache aliasing.  It is programmer's responsibility to 
   avoid the cache aliasing.  When a physical page is mapped on the 
   different virtual pages, VPN[13:12] have to be same in both virtual 
   address.  The conservative way to avoid this is that VPN[13:12] == 
   PFN[13:12] whenever a page is mapped."

so you need the flag indeed -- original R4000/R4400 hardware used a 
Virtual Coherency Exception Data/Instruction (VCED/VCEI) mechanism for 
alias resolution and some newer MIPS processor implementations have logic 
in hardware for that.  Obviously the TX79 (and presumably the R5900 as 
well) has neither.

> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 2828ecde133d..aec56966484b 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -1708,6 +1708,15 @@ config CPU_BMIPS
>  	help
>  	  Support for BMIPS32/3300/4350/4380 and BMIPS5000 processors.
>  
> +config CPU_R5900
> +	bool "R5900"
> +	depends on SYS_HAS_CPU_R5900
> +	select CPU_SUPPORTS_32BIT_KERNEL
> +	select CPU_SUPPORTS_64BIT_KERNEL

 I think it will make sense to defer 64-bit support until the oddities 
around it have been sorted out, see below.  So I suggest removing 
CPU_SUPPORTS_64BIT_KERNEL at first, and then suddenly you don't need to 
worry about stuff we know that will be broken until a further update.

> +	select IRQ_MIPS_CPU
> +	help
> +	  MIPS Technologies R5900 processor (Emotion Engine in Sony Playstation 2).

 Not Toshiba rather than MIPS Technologies?

> diff --git a/arch/mips/Makefile b/arch/mips/Makefile
> index 02a1787c888c..e8e2805a05c4 100644
> --- a/arch/mips/Makefile
> +++ b/arch/mips/Makefile
> @@ -171,6 +171,8 @@ cflags-$(CONFIG_CPU_R5432)	+= $(call cc-option,-march=r5400,-march=r5000) \
>  			-Wa,--trap
>  cflags-$(CONFIG_CPU_R5500)	+= $(call cc-option,-march=r5500,-march=r5000) \
>  			-Wa,--trap
> +cflags-$(CONFIG_CPU_R5900)	+= -march=r5900 -mtune=r5900 \
> +			-Wa,--trap -mno-llsc

 First, `-mtune=' defaults to whatever has been used with `-march=', so 
please remove it (I think we used to had both in the old days, but that 
stuff is now gone, so please follow the current rules).  If you feel 
pedantic, then to double-check you may compare binaries built with and w/o 
`-mtune=' to make sure the're the same, however TBH I think it would be a 
waste of time (especially if it turns out that GCC stores its command-line 
options somewhere in the binary produced).

 Second, given that the R5900 has no LL/SC instructions, I would expect 
`-march=r5900' to already imply it, so `-mno-llsc' should not be needed.  
Unless you want to support building with an older compiler, in which case:

cflags-$(CONFIG_CPU_R5900)	+= $(call cc-option,-march=r5900,-march=r4600 -mno-llsc) \
			-Wa,--trap

perhaps?  I.e. does the code you are going to introduce use any of the 
unusual processor-specific instructions, such as DI/EI (which have 
encodings and semantics different from their later MIPSr2 counterparts)?

> diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c
> index 1aba27786bd5..c9431900d11f 100644
> --- a/arch/mips/kernel/cpu-probe.c
> +++ b/arch/mips/kernel/cpu-probe.c
> @@ -1383,6 +1383,17 @@ static inline void cpu_probe_legacy(struct cpuinfo_mips *c, unsigned int cpu)
>  			     MIPS_CPU_WATCH | MIPS_CPU_LLSC;
>  		c->tlbsize = 48;
>  		break;
> +	case PRID_IMP_R5900:
> +		c->cputype = CPU_R5900;
> +		__cpu_name[cpu] = "R5900";
> +		c->isa_level = MIPS_CPU_ISA_III;
> +		c->fpu_msk31 |= FPU_CSR_CONDX;
> +		c->options = MIPS_CPU_TLB | MIPS_CPU_4K_CACHE |
> +			     MIPS_CPU_4KEX | MIPS_CPU_DIVEC |
> +			     MIPS_CPU_FPU | MIPS_CPU_32FPR |
> +			     MIPS_CPU_COUNTER;

 As (MIPS_CPU_TLB | MIPS_CPU_4K_CACHE | MIPS_CPU_4KEX | MIPS_CPU_COUNTER)
can be shortened to R4K_OPTS, please do so.

 More importantly, I think MIPS_CPU_32FPR is not right, as it's defined 
as: "32 dbl. prec. FP registers" whereas the R5900 AFAIK only supports 
single floats, so as far as the layout of the register file is concerned 
it is similar to a MIPS32r1 FPU.

 I do hope the R5900 implements the FPU in a sane way such as the R4650 
does, that is it still implements CP0.Status.FR, to flip between 16 and 32 
single FGRs, and all the double CP1 operations, i.e. LDC1/SDC1, 
DMTC1/DMFC1, and all the double arithmetics, such as MOV.D, ADD.D, etc. 
cause an Unimplemented Operation FPE exception, which we can then emulate.  

 Of course for the kernel as far as the instruction list is concerned only 
the lack of LDC1/SDC1 will matter in that it requires special attention 
for FP context switching.  All the user stuff will be handled 
automagically by our emulator.

 NB I find it is interesting that neither MIPS_CPU_32FPR nor its 
associated `cpu_has_32fpr' predicate is actually used anywhere.  So while 
it serves a CPU feature documentation purpose (for information that can be 
hard to chase sometimes), it actually is not used at the run time.

 Also the processor is unusual in that although it's a legacy architecture 
implementation it does use a distinct vector for the Interrupt exception.  
However its use is hardwired and there is no CP0.Cause.IV bit to control 
it, with its CP0.Cause.23 location fixed at 0.  So I think it would make 
sense to arrange for `configure_exception_vector' not to try flipping it, 
as a microoptimisation, but more importantly to have the code express that 
we know what we're doing here.

 So I think an update along the line of this:

	if (cpu_has_divec) {
		if (cpu_has_mipsmt) {
			/* ... */
		} else if (current_cpu_type() != CPU_R5900) {
			/*
			 * The R5900 has no Cause.IV bit and always uses
			 * the dedicated interrupt exception vector.
			 */
			set_c0_cause(CAUSEF_IV);
		}
	}

would be appropriate as a part of this patch.

 Finally, you'll need an option to indicate this is a processor that only 
does 32-bit addressing.  So o32 and n32 binaries will work, but n64 ones 
will not.  Well, not in the general case, as `-msym32' will, but that's a 
matter for the n64 ELF loader to sort out, by checking the VMA ranges 
requested (there's no MSYM32 ELF annotation yet, even though it's been 
discussed over and over again), which it currently does not.  Also for 
64-bit binaries to work correctly LLD/SCD emulation will be required.

 So as I noted above I think it will make sense if you remove 
CPU_SUPPORTS_64BIT_KERNEL above and only include it with the patch that 
adds support for running 64-bit binaries in the 32-bit addressing mode 
(effectively the same as contemporary MIPS architecture's CP0.Status.PX=1
mode) and the complementing unusual CP0.Status.FR handling, along with a 
flag like MIPS_CPU_FR to denote the presence of the CP0.Status.FR bit, but 
not 64-bit FGRs.

 That leaves us with o32 support only, which still I suspect will not 
handle the FPU correctly, due to the lack of LDC1/SDC1 instructions, 
currently expected by our code to exist for any but MIPS I processors, and 
missing odd-numbered FGRs in the CP0.Status.FR=0 mode in the first place.

 So perhaps again let's defer that feature and start with the MIPS_CPU_FPU 
flag removed, and then add it along with proper FPU support in a later 
patch?  That patch will presumably add FP context switching support using 
LWC1/SWC1 and anything else to handle R5900's FPU peculiarities.

 Until that further patch has been applied the R5900 would then operate in 
the fully-emulated FP mode only, i.e. as if `nofpu' has been 
unconditionally selected.

> diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
> index 3fe99cb271a9..0420ce8fb086 100644
> --- a/arch/mips/mm/c-r4k.c
> +++ b/arch/mips/mm/c-r4k.c
> @@ -1192,6 +1192,20 @@ static void probe_pcache(void)
>  		c->options |= MIPS_CPU_CACHE_CDEX_P | MIPS_CPU_PREFETCH;
>  		break;
>  
> +	case CPU_R5900:
> +		icache_size = 1 << (12 + ((config & CONF_IC) >> 9));
> +		c->icache.linesz = 64;
> +		c->icache.ways = 2;
> +		c->icache.waybit = 0;
> +
> +		dcache_size = 1 << (12 + ((config & CONF_DC) >> 6));
> +		c->dcache.linesz = 64;
> +		c->dcache.ways = 2;
> +		c->dcache.waybit = 0;
> +
> +		c->options |= MIPS_CPU_CACHE_CDEX_P | MIPS_CPU_PREFETCH;

 The cache parameters appear correct to me and I have discussed the 
options earlier on.

> @@ -1465,6 +1479,17 @@ static void probe_pcache(void)
>  	case CPU_R16000:
>  		break;
>  
> +	case CPU_R5900:
> +		if (c->icache.waysize > PAGE_SIZE)
> +			c->dcache.flags |= MIPS_CACHE_ALIASES;
> +		/*
> +		 * There seems to be a missing d-cache flush which is fixed
> +		 * with MIPS_CACHE_ALIASES.
> +		 */
> +		if (c->dcache.waysize > PAGE_SIZE)
> +			c->dcache.flags |= MIPS_CACHE_ALIASES;
> +		break;

 Duplicate code here; otherwise OK, as noted above.  You may wish to 
update the comment though.

 Ralf may yet want to chime in, but overall I think that the way to move 
forward with your submission is to:

1. Make the adjustments to this patch I have outlined above; dropping FPU 
   and 64-bit support for the time being in particular.

2. Update cache handlers to use the correct R5900-specific cache op 
   encodings, presumably within the same patch.

3. As Ralf has already reqested, add basic board support for the PS2 
   platform, including essential drivers that are required to boot, e.g. 
   serial, network; fancy stuff can be added gradually later on.

4. Post the whole set of changes collected so far, properly split into 
   functionally self-contained changes, i.e. ones that build and can run
   successfully run on actual hardware, for a reasonable definition of 
   success, e.g. patch #1 is this one, patch #2 is base board setup
   infrastructure, patch #3 is interrupt support, patch #4 is timer 
   support, patch #5 is the serial driver, patch #6 is the network 
   driver, or suchlike.

We can then integrate these to have basic hardware support already working 
and only then continue with more complicated features, especially ones 
such as the FPU and 64-bit support which will require considerable updates 
to generic architecture code.

 NB if CP1.FCSR.FS indeed turns out hardwired to 1, then having this FPU 
hardware handled will be a more complex task, involving adding AT_FPUCW 
support to our MIPS port of the kernel and adjusting glibc appropriately 
before we are able to proceed.  So if this is indeed the case, then I 
think it'll be the most reasonable if we just ignore the issue of 
CP1.FCSR.FS for the time being, and have the emulated FP work with the bit 
flippable.

 Questions, comments?

  Maciej