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

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

Hello.

On 06/05/2013 08:36 PM, David Daney wrote:

>
>>
>>> The ISA exception bit selects whether exceptions are taken in classic
>>> MIPS or microMIPS mode. This bit is Config3.ISAOnExc and is bit 16. It
>>> It was improperly defined as bits 16 and 17. Fortunately, bit 17 is
>>> read-only and did not effect microMIPS operation. However, detecting
>>> a classic or microMIPS kernel when examining the /proc/cpuinfo file,
>>> the result always showed a microMIPS kernel.
>
> I don't see anything in the patch that would make Classic CPUs be 
> misidentified.  Is the change log still accurate?
>
> ...
>
>>> Signed-off-by: Steven J. Hill <Steven.Hill@imgtec.com>
>>> ---
>>>   arch/mips/include/asm/mipsregs.h |    2 +-
>>>   arch/mips/kernel/cpu-probe.c     |    7 ++-----
>>>   arch/mips/mti-malta/malta-init.c |    7 +++++++
>>>   arch/mips/mti-sead3/sead3-init.c |    7 +++++++
>>>   4 files changed, 17 insertions(+), 6 deletions(-)
>>
>> [...]
>>> diff --git a/arch/mips/mti-malta/malta-init.c
>>> b/arch/mips/mti-malta/malta-init.c
>>> index ff8caff..3598f1d 100644
> [...]
>>> --- a/arch/mips/mti-sead3/sead3-init.c
>>> +++ b/arch/mips/mti-sead3/sead3-init.c
>>> @@ -130,6 +130,13 @@ static void __init mips_ejtag_setup(void)
>>>
>>>   void __init prom_init(void)
>>>   {
>>> +#ifdef CONFIG_CPU_MICROMIPS
>>> +    unsigned int config3 = read_c0_config3();
>>> +
>>> +    if (config3 & MIPS_CONF3_ISA)
>>> +        write_c0_config3(config3 | MIPS_CONF3_ISA_OE);
>>> +#endif
>>> +
>>
>>     I see it's repeated twice and enclosed in #ifdef... Couldn't you
>> factor it out in some sort of inline function and put into some header:
>>
>> #ifdef CONFIG_CPU_MICROMIPS
>> static inline void mips_set_config3_isa_oe(void)
>
>
> I don't have a strong opinion about factoring it out like this, but we 
> do, let's give it a better name.  Something like 
> enable_micromips_exception_mode() or similar.  That way we know what 
> it does.

     Yes, the name was only a (bad) example, since I didn't really 
understand what the code was doing.

>
>> {
>>      unsigned int config3 = read_c0_config3();
>>
>>      if (config3 & MIPS_CONF3_ISA)
>>          write_c0_config3(config3 | MIPS_CONF3_ISA_OE);
>> }
>> #else
>> static inline void mips_set_config3_isa_oe(void) {}
>> #endif
>>

WBR, Sergei