Re: [PATCH v6] MIPS: micromips: Fix improper definition of ISA exception bit.

[David Daney <ddaney.cavm@gmail.com>]

On 06/05/2013 12:49 PM, Steven J. Hill wrote:
> The ISA exception bit selects whether exceptions are taken in classic
> or microMIPS mode. This bit is Config3.ISAOnExc and was improperly
> defined as bits 16 and 17 instead of just bit 16. A new function was
> added so that platforms could set this bit when running a kernel
> compiled with only microMIPS instructions.
>
> Signed-off-by: Steven J. Hill <Steven.Hill@imgtec.com>
> ---
> Changes from v5:
> * Make 'set_micromips_exception_mode' function to always be called.

Version 5 was better in my opinion...


>
>   arch/mips/include/asm/mipsregs.h |   18 +++++++++++++++++-
>   arch/mips/kernel/cpu-probe.c     |    3 ---
>   arch/mips/kernel/traps.c         |    5 +++++
>   3 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/arch/mips/include/asm/mipsregs.h b/arch/mips/include/asm/mipsregs.h
> index 87e6207..cc0f5d7 100644
> --- a/arch/mips/include/asm/mipsregs.h
> +++ b/arch/mips/include/asm/mipsregs.h
> @@ -596,7 +596,7 @@
>   #define MIPS_CONF3_RXI		(_ULCAST_(1) << 12)
>   #define MIPS_CONF3_ULRI		(_ULCAST_(1) << 13)
>   #define MIPS_CONF3_ISA		(_ULCAST_(3) << 14)
> -#define MIPS_CONF3_ISA_OE	(_ULCAST_(3) << 16)
> +#define MIPS_CONF3_ISA_OE	(_ULCAST_(1) << 16)
>   #define MIPS_CONF3_VZ		(_ULCAST_(1) << 23)
>
>   #define MIPS_CONF4_MMUSIZEEXT	(_ULCAST_(255) << 0)
> @@ -1161,6 +1161,22 @@ do {									\
>   #define write_c0_brcm_sleepcount(val)	__write_32bit_c0_register($22, 7, val)
>
>   /*
> + * Set exceptions to be taken in microMIPS mode only, otherwise
> + * set for classic exceptions.
> + */
> +static inline void set_micromips_exception_mode(void)
> +{
> +	unsigned int config3 = read_c0_config3();
> +
> +#ifdef CONFIG_CPU_MICROMIPS
> +	if (config3 & MIPS_CONF3_ISA)
> +		write_c0_config3(config3 | MIPS_CONF3_ISA_OE);
> +	else
> +#endif
> +		write_c0_config3(config3 & ~MIPS_CONF3_ISA_OE);

You can only manipulate this bit if you know microMIPS is supported.  So 
I think you should either not touch it for the non-microMIPS case, or 
make the write conditional on the presence of microMIPS support in the CPU.

David Daney


> +}
> +
> +/*