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

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

On 06/05/2013 01:01 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 06/05/2013 11: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.
>
>      Ahem, isn't that function a material for another patch?


I think you might be going overboard.  The entire patch relates to 
exactly one bit a config register, can't you let it be a single patch?

David Daney

>
>> Signed-off-by: Steven J. Hill <Steven.Hill@imgtec.com>
>> ---
>> Changes from v5:
>> * Make 'set_micromips_exception_mode' function to always be called.
>>
>>   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);
>> +}
>> +
>> +/*
>>    * Macros to access the floating point coprocessor control registers
>>    */
>>   #define read_32bit_cp1_register(source)                    \
> [...]
>> diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
>> index a75ae40..151ed59 100644
>> --- a/arch/mips/kernel/traps.c
>> +++ b/arch/mips/kernel/traps.c
>> @@ -1837,6 +1837,11 @@ void __init trap_init(void)
>>               ebase += (read_c0_ebase() & 0x3ffff000);
>>       }
>> +    /*
>> +     * Set microMIPS exceptions for platforms that support it.
>> +     */
>> +    set_micromips_exception_mode();
>
>      If we have reduced the call sites to 1, do we still need the
> function? :-)
>
> WBR, Sergei
>
>
>
>