Re: [PATCH] MIPS: Add R16000 detection

[Joshua Kinard <kumba@gentoo.org>]

On 01/20/2015 00:11, Matt Turner wrote:
> On Mon, Jan 19, 2015 at 8:13 PM, Joshua Kinard <kumba@gentoo.org> wrote:
>> 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 .
> 
> Sorry, you're not getting it.
> 
> git grep -B1 CPU_R14000
> 
> Notice how all of the instances of CPU_R14000 are preceded by
> CPU_R12000? That's because CPU_R14000 doesn't do anything differently.
> 
> All you should do to remove CPU_R14000 is to set c->cputype =
> CPU_R12000 in the PRID_IMP_R14000 case in cpu-probe.c.

I see what you're getting at, but I disagree with the reasoning.  The code
reads clearer when it's explicitly stated the way it is, rather than fudging
things and treating an R14K as an R12K for a minor gain of a few cycles.

And since I know there's something "weird" about the R14K right now, one of
those case statements might be needed down the road to do something a little
bit differently for R14K versus R12K and such (maybe in the TLB code, if I can
ever wrap my head around that).

In the end, it's Ralf's call on accepting it.