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

[Richard Sandiford <rdsandiford@googlemail.com>]

Hi Jürgen,

"Jürgen Urban" <JuergenUrban@gmx.de> writes:
> It would be nice when this can be added to the offical binutils. Please
> tell me if this acceptable or there is a problem.

I suppose the main potential barrier is whether the copyright can still
be assigned to the FSF.  I imagine this code has been worked on by
several people over the years.  Is that right?  If so, I think they
would all need to sign a copyright assignment.

Are there still remnants of the original Cygnus implementation, or was
this written from scratch?  There was often talk at Cygnus/Red Hat of
formally submitting the port (along with the tx79), but it never happened
due to lack of time.

I'll hold off a full review until the copyright situation is clearer,
but generally the patch looks good.  There shouldn't be any major
technical obstacles to getting the support in.

Some specific comments:

> The R5900 CPU is used as the main processor in the Sony Playstation 2 (ps2).
> The CPU supports MIPS ISA I, II, III and IV, but not all
> instructions. There are only few instructions which are MIPS IV, so it
> is configured as MIPS III in binutils.
> R5900 supports 32-bit, 64-bit and 128-bit data transfers and registers.
> The address bus is 32-bit. There fore the CPU can run MIPS ABI o32 and
> n32 code. o64 and n64 code would run until an address outside the first
> 4 GByte is addressed (Indeed the PS2 has 32 MByte RAM and the PS2 Tool
> has 128 MByte).

FWIW, o64 is like n32: an ABI for 64-bit targets that uses 32-bit ELF.
By default it has 32-bit pointers and longs, although some people have
used it with -mlong64.  It shouldn't really be used in new code though.

The mips64*-ps2-elf* stanzas in config.bfd include the n64 vectors,
which is good.  So even if o64 and n64 aren't supported at runtime,
we should still be able to assemble o64 and n64 code.  The "n32native"
part of the testsuite patch doesn't look right from that point of view,
although it looks like "!n32native" might actually be skipping based
on something other than the ABI.

> - Support for VU instructions of the COP 2 (VU0 in Macro Mode). This
> includes special register suffixes.

We're running very short of single character operand markers, so for:

+   VU0 Macromode:
+   "+m" vu0 immediate for viaddi (OP_*_VADDI)
+   "+5" vu0 fp reg position 1 (OP_*_VUTREG)
+   "+6" vu0 fp reg position 2 (OP_*_VUSREG)
+   "+7" vu0 fp reg position 3 (OP_*_VUDREG)
+   "+8" vu0 int reg position 1 (OP_*_VUTREG)
+   "+9" vu0 int reg position 2 (OP_*_VUSREG)
+   "+0" vu0 int reg position 3 (OP_*_VURREG)
+   "+d" vu0 fp reg with ftf modifier (OP_*_VUTREG and OP_*_VUFTF)
+   "+e" vu0 fp reg with fsf modifier (OP_*_VUSREG and OP_*_VUFSF)
+   "+f" Immediate operand for vcallms instruction. (OP_*_VUCALLMS)
+   "+i" vu0 I register
+   "+q" vu0 Q register
+   "+r" vu0 R register
+   "#" optional suffix that must match if present
+   "=" dest operant completer, must match previous dest if present
+   "?" dest instruction completer (OP_*_VUDEST)
+   ";" dest instruction completer, must by xyz
+   "_" vu0 ACC register

I think all the operands should have a prefix (i.e. the last 5 too).
Same for the ++/-- formats.

> - Detecting of short loops because of a CPU bug (minimum number of
> instructions needed in a conditional loop).

I haven't looked at this in detail, but I noticed:

+  /* On R5900 short loops need to be fixed by inserting a nop in
+     the branch delay slots.
+     A short loop can be terminated too early.  */
+  if (mips_opts.arch == CPU_R5900
+      /* Check if instruction has a parameter, ignore "j $31". */
+      && (address_expr != NULL)
+      /* Parameter must be 16 bit. */
+      && (*reloc_type == BFD_RELOC_16_PCREL_S2)
+      /* Branch to same segment. */
+      && (S_GET_SEGMENT(address_expr->X_add_symbol) == now_seg)
+      /* Branch to same code fragment. */
+      && (symbol_get_frag(address_expr->X_add_symbol) == frag_now)

Even tight loops can be split over several frags.  The start of the loop
might have been at the very end of the buffer allocated for one frag, or
the loop might contain a macro-derived variant instruction, etc.

> - New parameter "-mhard-ldsd" to support ld and sd instructions in MIPS
> ABI o32. This is needed to compile the Linux kernel with support for 64
> bit registers, because otherwise the instructions ld and sd are replaced
> by lw and sw.

It seems dangerous to ignore HAVE_64BIT_GPRs in this one instance
(and only this one instance).  Could you explain in more detail
in which situations the kernel needs to use LD and SD?

> - The "-mtune=r5900" parameter forces "-march=r5900", because this is
> the only configurable option of GCC to support R5900 as default with
> mips64el-linux-gnu or mipsel-linux-gnu.

FWIW, the defaults for both -march and -mtune can be set when GCC is
configured (using --with-arch and --with-tune respectively).

-mtune must never change the architecture.  I'm afraid that rule
has to win over anything R5900-specific.

Richard