Re: [PATCH] MIPS: Add R16000 detection

[Joshua Kinard <kumba@gentoo.org>]

On 01/19/2015 21:43, Matt Turner wrote:
> On Mon, Jan 19, 2015 at 4:56 PM, Joshua Kinard <kumba@gentoo.org> wrote:
>> On 01/19/2015 14:34, Matt Turner wrote:
>>> On Sun, Jan 18, 2015 at 5:30 PM, Joshua Kinard <kumba@gentoo.org> wrote:
>>>> diff --git a/arch/mips/kernel/cpu-probe.c b/arch/mips/kernel/cpu-probe.c
>>>> index 5342674..3f334a8 100644
>>>> --- a/arch/mips/kernel/cpu-probe.c
>>>> +++ b/arch/mips/kernel/cpu-probe.c
>>>> @@ -833,8 +833,13 @@ static inline void cpu_probe_legacy(struct cpuinfo_mips *c, unsigned int cpu)
>>>>                 c->tlbsize = 64;
>>>>                 break;
>>>>         case PRID_IMP_R14000:
>>>> -               c->cputype = CPU_R14000;
>>>> -               __cpu_name[cpu] = "R14000";
>>>> +               if (((c->processor_id >> 4) & 0x0f) > 2) {
>>>> +                       c->cputype = CPU_R16000;
>>>> +                       __cpu_name[cpu] = "R16000";
>>>> +               } else {
>>>> +                       c->cputype = CPU_R14000;
>>>> +                       __cpu_name[cpu] = "R14000";
>>>> +               }
>>>
>>> It looks like this is the only hunk that has a functional change, and
>>> that is simply setting __cpu_name[cpu] to "R16000"
>>>
>>> You can do that without adding CPU_R16000 to the enumeration. I don't
>>> see that adding it accomplishes anything.
>>>
>>
>> It mirrors what CPU_R14000 and CPU_R12000 do.  I won't rule out that, down the
>> road, something about the R16K might be different enough from the R14K to
>> require one of these other spots later on, so adding it now isn't going to
>> adversely affect things.
> 
> That's justification for removing CPU_R14000 as well, not adding CPU_R16000.
> 
> Otherwise it's just adding useless code.

R14000 has a different CPU PRId than R12000 or R10000, so the code that sets
the icache/scache linesz wouldn't know to apply to R14K, including the writing
the the FrameMask register in CP0.  Octane and Origin2K/Onyx2 can both use
R14000 CPUs, so this is a bad suggestion, as removing R14000 detection would
render those systems inoperable with those CPUs.  I know, cause I'm the one
that actually sent the R14K patch in 9 years ago w/ commit 44d921b2 .

I'm also for reducing code and all, but this isn't the case in which to do it.
 You're quibbling over the addition of an one new enum item, one new if-else
statement, one extra logical-OR conditional to an existing if statement, and
nine new case statements, some of which only execute once during the CPU
probing portion of boot.

There are likely a lot of better places in the existing code that could use
some optimization or dead code removal.

--J