From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jonathan Larmour <jlarmour@redhat.co.uk>
To: sun409@pchome.com.tw
Cc: Jurica Baricevic <jura@INTESIS.hr>, Ecos <ecos-discuss@sourceware.cygnus.com>
Subject: Re: [ECOS] MIPS R3000 patch for MIPS arch
Date: Thu, 01 Jun 2000 17:22:00 -0000
Message-id: <3936FE39.7F7BA9BD@redhat.co.uk>
References: <000c01bfcbe4$f316c140$6b00a8c0@jura>
X-SW-Source: 2000-06/msg00024.html

Jurica Baricevic wrote:
> 
> Thank you. A few days ago I tested eCos 1.3.1 with modifications very
> similar to yours on an R3041 board and I can say that everything worked
> well. Since 'load delay slot' seemed as the main problem in porting to
> R3000, I really hope that Cygnus could accept your patch.

Me too :-).

> Also, I believe
> that distinguishing between MIPS processors in the mips/arch directory
> should be oriented to MIPS ISA levels (hopefully in next eCos
> version/revision :-) ). 

Why wait :-). It would benefit anonymous CVS users immediately.

> Namely, '#ifdef CYG_HAL_MIPS_R3900' and similar
> statements should be avoided if possible; and something like '#ifdef
> CYG_HAL_MIPS_ISA1' should be put on right places instead.

I agree, and in fact I'd be happy to take this patch with such
improvements. Tim, I think it would be more generic for each variant HAL to
define the ISA level supported in the CDL, e.g. for tx39:

cdl_option CYGHWR_HAL_MIPS_ISA {
    display     "MIPS ISA level supported by CPU"
    calculated  2
}

I believe ISA 2 is correct for the tx39 since it supports branch likely
instructions. For the vr4300 it would be:

cdl_option CYGHWR_HAL_MIPS_ISA {
    display     "MIPS ISA level supported by CPU"
    calculated  3
}

Then at the top of vectors.S you could write:
#if CYGHWR_HAL_MIPS_ISA == 1
#define LWNOP nop
#else
#define LWNOP
#endif

Then instead of adding the explicit nops in vectors.S for all ISAs, use
this LWNOP macro, which means the (admittedly small) overhead will only
happen for CPUs with ISA 1.

Also you don't need to add "sun409@pchome.com.tw added" every nop line :-).
Perhaps just say "MIPS ISA 1 lw delay slot"

Similarly, for your change to hal_cpu_eret in arch.inc, we can instead
have:

        # Return from exception.
#ifdef CYGHWR_HAL_MIPS_ISA == 1
       .macro  hal_cpu_eret pc,sr
       mtc0    \sr,status                      # Load status register
       nop
       jr      \pc                             # jump back to interrupted
code
       rfe                                     # restore state (delay
slot)    
       .endm
#elif CYGHWR_HAL_MIPS_ISA == 2
        .macro  hal_cpu_eret pc,sr
        mtc0    \sr,status                      # Load status register
        nop
        nop
        nop
        sync                                    # settle things down
        jr      \pc                             # jump back to interrupted
code
        rfe                                     # restore state (delay
slot)
        .endm
#else   # ISA >= 3
        .macro  hal_cpu_eret pc,sr
        .set mips3
[etc.]

If you can make these changes, I'll check it in.

> I have a question with regard to the patch:
> Patch in 'packages/hal/mips/arch/current/include/hal_intr.h' (macro
> HAL_RESTORE_INTERRUPTS) looks as a bug fix for all MIPS ISA levels. Am I
> right?

Yes. Although it doesn't happen to cause any problems in the existing code
because the macro is only used in a limited way. But obviously the fix is
welcome.

Jifl
-- 
Red Hat, 35 Cambridge Place, Cambridge, UK. CB2 1NS  Tel: +44 (1223) 728762
"Plan to be spontaneous tomorrow."  ||  These opinions are all my own fault