Re: [PATCH] Support for MIPS R5900 (Sony Playstation 2)

["Maciej W. Rozycki" <macro at codesourcery dot com>]

On Fri, 4 Jan 2013, nick clifton wrote:

> > Changelog for bfd:
> > 2013-01-03  Juergen Urban<JuergenUrban@gmx.de>
> > 
> > 	* archures.c: Add support for MIPS r5900
> > 	* bfd-in2.h: Add support for MIPS r5900
> > 	* config.bfd: Add support for Sony Playstation 2
> > 	* cpu-mips.c: Add support for MIPS r5900
> > 	* elfxx-mips.c: Add support for MIPS r5900 (extension of r4000)
> > 
> > Changelog for gas:
> > 2013-01-03  Juergen Urban<JuergenUrban@gmx.de>
> > 
> > 	* config/tc-mips.c: Add support for MIPS r5900
> > 	Add M_LQ_AB and M_SQ_AB to support large values for instructions lq
> > and sq.
> > 	* config/tc-mips.c (can_swap_branch_p, get_append_method): Detect some
> > conditional short loops to fix a bug on the r5900 by NOP in the branch delay
> > slot.
> > 	* config/tc-mips.c (M_MUL): Support 3 operands in multu on r5900.
> > 	* config/tc-mips.c (M_TRUNCWS): Support trunc.w.s on r5900 in MIPS ISA
> > I.
> > 	* config/tc-mips.c (s_mipsset): Force 32 bit floating point on r5900.
> > 	* configure.in: Detect CPU type when target string contains r5900
> > (e.g. mips64r5900el-linux-gnu).
> > 
> > 2013-01-03  ragnarok2040
> > 
> > 	* config/tc-mips.c (mips_ip): Check parameter range of instructions
> > mfps and mtps on r5900.
> > 
> > Changelog for include:
> > 2013-01-03  Juergen Urban<JuergenUrban@gmx.de>
> > 
> > 	* elf/mips.h: Add MIPS machine variant number for r5900 which is
> > compatible with old Playstation 2 software.
> > 	* opcode/mips.h: Add support for r5900 instructions including lq and
> > sq.
> > 
> > Changelog for ld:
> > 2013-01-03  Juergen Urban<JuergenUrban@gmx.de>
> > 
> > 	* configure.tgt: Support ELF files for Sony Playstation 2 (for ps2dev
> > and ps2sdk).
> > 	* emulparams/elf32lr5900n32.sh: Create linker script for Sony
> > Playstation 2 ELF files using MIPS ABI n32.
> > 	* emulparams/elf32lr5900.sh: Create linker script for Sony Playstation
> > 2 ELF files using MIPS ABI o32.
> > 	* Makefile.am: Add linker scripts for Sony Playstation 2 ELF files.
> > 
> > Changelog for opcodes:
> > 2013-01-03  ragnarok2040
> > 
> > 	* opcodes/mips-dis.c: Add names for CP0 registers of r5900.
> > 	* opcodes/mips-opc.c: Add M_SQ_AB and M_LQ_AB to support larger range
> > for instructions sq and lq.
> > 
> > Changelog for opcodes:
> > 2013-01-03  Juergen Urban<JuergenUrban@gmx.de>
> > 
> > 	* opcodes/mips-opc.c: Add support for MIPS r5900 CPU.
> > 	Add support for 128 bit MMI (Multimedia Instructions).
> > 	Add support for EE instructions (Emotion Engine).
> > 	Disable unsupported floating point instructions (64 bit and undefined
> > compare operations).
> > 	Enable instructions of MIPS ISA IV which are supported by r5900.
> > 	Disable 64 bit co processor instructions.
> > 	Disable 64 bit multiplication and division instructions.
> > 	Disable instructions for co-processor 2 and 3, because these are not
> > supported (preparation for later VU0 support (Vector Unit)).
> > 	Disable cvt.w.s because this behaves like trunc.w.s and the correct
> > execution can't be ensured on r5900.
> > 	Add trunc.w.s using the opcode encoding of cvt.w.s on r5900. This will
> > confuse less developers and compilers.
> 
> 
> Approved and applied.

 FWIW, I don't like the way mips_opts.micromips has been special-cased for 
CPU_R5900 with this change, e.g.:

-	  if (mips_opts.isa != ISA_MIPS1 || mips_opts.micromips)
+	  if ((mips_opts.isa != ISA_MIPS1 || mips_opts.micromips)
+	      && (mips_opts.arch != CPU_R5900))

The original intent for -mmicromips was to override any specific CPU 
selection -- for simplicity and because there are no vendor extensions to 
the microMIPS instruction set (at least not yet).

 Of course one could say it makes no sense to enable microMIPS assembly 
for processors that do not support it.  I guess this could be discussed, 
but in any case a change to add R5900 support is not a place to change the 
preexisting general arrangements.

 I would therefore appreciate if pieces like the above were rewritten like 
below:

	  if ((mips_opts.isa != ISA_MIPS1 && mips_opts.arch != CPU_R5900)
	      || mips_opts.micromips)

-- that is preserving the previous consistent behaviour (note that there 
is no need for extra parentheses around an equality operation as it takes 
precedence over the logical operation anyway).  I suppose the "ISA_MIPS1 
|| CPU_R5900" part could be macroised too -- it's used in several places 
(e.g. some assertions as well).

 Jürgen, as the author of the modification, would you please have a look 
into it?

 As a side note -- I've noticed elsewhere in this change that the R5900 
supports single-precision FP only.  This is similar to what the 
R4640/R4650 does (up to supporting 32 single registers in the FR mode -- 
is that any different to the R5900?), however up till now we did nothing 
explicit for such a setup, except from respecting the -mfp32 option.  
This change introduces extra bits however, like hardwiring -mfp32 -- 
perhaps they could be generalised in some way to cover the R4640/R4650 as 
well? -- again, for consistency.

  Maciej