broken RM7000 in CVS ...

broken RM7000 in CVS ...

[Jun Sun]

I saw mips_cpu structure is introduced to the CVS tree.  That is a really good
thing, although many places need to be improved.

An initial try found the following bug.  There are probably more down the
road. :-)

Jun

diff -Nru linux/arch/mips/mm/loadmmu.c.orig linux/arch/mips/mm/loadmmu.c
--- linux/arch/mips/mm/loadmmu.c.orig   Thu Jan 11 19:32:11 2001
+++ linux/arch/mips/mm/loadmmu.c        Thu Jan 11 19:48:06 2001
@@ -59,6 +59,11 @@
                printk("Loading MIPS32 MMU routines.\n");
                ld_mmu_mips32();
 #endif
+#if defined(CONFIG_CPU_RM7000)
+               printk("Loading RM7000 MMU routines.\n");
+               ld_mmu_rm7k();
+#endif
+
        } else switch(mips_cpu.cputype) {
 #ifdef CONFIG_CPU_R3000
        case CPU_R2000:
@@ -74,13 +79,6 @@
        case CPU_R5432:
                printk("Loading R5432 MMU routines.\n");
                ld_mmu_r5432();
-               break;
-#endif
-
-#if defined(CONFIG_CPU_RM7000)
-       case CPU_RM7000:
-               printk("Loading RM7000 MMU routines.\n");
-               ld_mmu_rm7k();
                break;
 #endif

Re: broken RM7000 in CVS ...

[Kevin D. Kissell]

Yes, arguably the mips_cpu structure could also contain
a descriptor of the MMU routines to bind, and it probably
would have if it would have been a simple matter of an
address/length of a vector to copy.  But heck, it could
be a function pointer as well, I suppose.

Anyway, I'm not surprised if there was something off
somewhere in the RM7000 descriptor.  I did a first
cut based on an old copy of the QED spec - I had no
hardware to test it on - and there was no pretention
of putting in full RM7000 support in the kernel when
I did the decriptor, it was more a matter of showing
what a 3-cache CPU would look like!

            Kevin K.

----- Original Message -----
From: "Jun Sun" <jsun@mvista.com>
To: <linux-mips@oss.sgi.com>
Sent: Friday, January 12, 2001 4:54 AM
Subject: broken RM7000 in CVS ...


>
> I saw mips_cpu structure is introduced to the CVS tree.  That is a really
good
> thing, although many places need to be improved.
>
> An initial try found the following bug.  There are probably more down the
> road. :-)
>
> Jun
>
> diff -Nru linux/arch/mips/mm/loadmmu.c.orig linux/arch/mips/mm/loadmmu.c
> --- linux/arch/mips/mm/loadmmu.c.orig   Thu Jan 11 19:32:11 2001
> +++ linux/arch/mips/mm/loadmmu.c        Thu Jan 11 19:48:06 2001
> @@ -59,6 +59,11 @@
>                 printk("Loading MIPS32 MMU routines.\n");
>                 ld_mmu_mips32();
>  #endif
> +#if defined(CONFIG_CPU_RM7000)
> +               printk("Loading RM7000 MMU routines.\n");
> +               ld_mmu_rm7k();
> +#endif
> +
>         } else switch(mips_cpu.cputype) {
>  #ifdef CONFIG_CPU_R3000
>         case CPU_R2000:
> @@ -74,13 +79,6 @@
>         case CPU_R5432:
>                 printk("Loading R5432 MMU routines.\n");
>                 ld_mmu_r5432();
> -               break;
> -#endif
> -
> -#if defined(CONFIG_CPU_RM7000)
> -       case CPU_RM7000:
> -               printk("Loading RM7000 MMU routines.\n");
> -               ld_mmu_rm7k();
>                 break;
>  #endif

Re: broken RM7000 in CVS ...

[Kevin D. Kissell]

Yes, arguably the mips_cpu structure could also contain
a descriptor of the MMU routines to bind, and it probably
would have if it would have been a simple matter of an
address/length of a vector to copy.  But heck, it could
be a function pointer as well, I suppose.

Anyway, I'm not surprised if there was something off
somewhere in the RM7000 descriptor.  I did a first
cut based on an old copy of the QED spec - I had no
hardware to test it on - and there was no pretention
of putting in full RM7000 support in the kernel when
I did the decriptor, it was more a matter of showing
what a 3-cache CPU would look like!

            Kevin K.

----- Original Message -----
From: "Jun Sun" <jsun@mvista.com>
To: <linux-mips@oss.sgi.com>
Sent: Friday, January 12, 2001 4:54 AM
Subject: broken RM7000 in CVS ...


>
> I saw mips_cpu structure is introduced to the CVS tree.  That is a really
good
> thing, although many places need to be improved.
>
> An initial try found the following bug.  There are probably more down the
> road. :-)
>
> Jun
>
> diff -Nru linux/arch/mips/mm/loadmmu.c.orig linux/arch/mips/mm/loadmmu.c
> --- linux/arch/mips/mm/loadmmu.c.orig   Thu Jan 11 19:32:11 2001
> +++ linux/arch/mips/mm/loadmmu.c        Thu Jan 11 19:48:06 2001
> @@ -59,6 +59,11 @@
>                 printk("Loading MIPS32 MMU routines.\n");
>                 ld_mmu_mips32();
>  #endif
> +#if defined(CONFIG_CPU_RM7000)
> +               printk("Loading RM7000 MMU routines.\n");
> +               ld_mmu_rm7k();
> +#endif
> +
>         } else switch(mips_cpu.cputype) {
>  #ifdef CONFIG_CPU_R3000
>         case CPU_R2000:
> @@ -74,13 +79,6 @@
>         case CPU_R5432:
>                 printk("Loading R5432 MMU routines.\n");
>                 ld_mmu_r5432();
> -               break;
> -#endif
> -
> -#if defined(CONFIG_CPU_RM7000)
> -       case CPU_RM7000:
> -               printk("Loading RM7000 MMU routines.\n");
> -               ld_mmu_rm7k();
>                 break;
>  #endif

Re: broken RM7000 in CVS ...

[Jun Sun]

"Kevin D. Kissell" wrote:
> 
> Yes, arguably the mips_cpu structure could also contain
> a descriptor of the MMU routines to bind, and it probably
> would have if it would have been a simple matter of an
> address/length of a vector to copy.  But heck, it could
> be a function pointer as well, I suppose.
> 

I think that is a good idea.  I suggest we have two more pointers in the
mips_cpu strcuture : one to mips_mmu_ops structure, and the other to
setup_exception_vectors() function.

BTW, I have a question about MIPS32 (or 4KC).  Do all MIPS32 CPUs have the
same PRID?  Or all "incarnations" of 4KC have the same PRID?  I suppose MIPS32
CPUs have a more complete config register where you can probe for all the
options.  For others we can use a table-like structure to fill in the options.

Along this line, it probably makes sense to have another pointer to
mips_cpu_config() function, where for MIPS32 it is the standard MIPS32 config
probing function and for most others it is NULL.

Now the mips_cpu_table looks like :

struct mips_cpu mips_cpu_table[]={
	{ PRID_IMP_4KC, mips32_cpu_config},
	{ PRID_IMP_RM7K, null, 0xaaa, {...}}
	.....
};

The cpu_probe() routine will now look like:
{
   read prid register
   find mips_cpu_table[i] with matching PRID.
   mips_cpu = &mips_cpu_table[i];
   if (mips_cpu->mips_cpu_config) mips_cpu->mips_cpu_config();
}

To me this is beautiful. Am I dreaming? :-)

Jun

Re: broken RM7000 in CVS ...

[Justin Carlson]

On Fri, 12 Jan 2001, you wrote:
> I think that is a good idea.  I suggest we have two more pointers in the
> mips_cpu strcuture : one to mips_mmu_ops structure, and the other to
> setup_exception_vectors() function.
> 
> BTW, I have a question about MIPS32 (or 4KC).  Do all MIPS32 CPUs have the
> same PRID?  Or all "incarnations" of 4KC have the same PRID?  

No.  Mips32 defines PRID into 4 partitions: a company id, processor id,
revision id, and company options field.  

>I suppose MIPS32
> CPUs have a more complete config register where you can probe for all the
> options.  For others we can use a table-like structure to fill in the options.

This is sort of true.  Mips32 does do a pretty good job of defining how to
probe for L1 caches and the like, but other things, such as L2 caches, are not
going to be so easily probed.  I think the mmu_ops routines should continue
to be specialized on a per-processor basis, just because they are both quite
performance sensitive and relatively easy to write/maintain. 

> 
> Along this line, it probably makes sense to have another pointer to
> mips_cpu_config() function, where for MIPS32 it is the standard MIPS32 config
> probing function and for most others it is NULL.
> 
> Now the mips_cpu_table looks like :
> 
> struct mips_cpu mips_cpu_table[]={
> 	{ PRID_IMP_4KC, mips32_cpu_config},
> 	{ PRID_IMP_RM7K, null, 0xaaa, {...}}
> 	.....
> };

If I'm understanding your idea correctly, this table would require you to
always compile in all the mmu routines for all processors, just to fill in the
table entries.  Doesn't seem like a particularly good idea to me, even if we
could use generic mips32 routines for most parts.  

> The cpu_probe() routine will now look like:
> {
>    read prid register
>    find mips_cpu_table[i] with matching PRID.
>    mips_cpu = &mips_cpu_table[i];
>    if (mips_cpu->mips_cpu_config) mips_cpu->mips_cpu_config();
> }
> 
> To me this is beautiful. Am I dreaming? :-)

See objections above.

I did submit a patch to Ralf a couple weeks ago to fix cpu_probe so it 
checks PRID according to the mips32 spec, but never heard back, and haven't
seen it show up in CVS.  WIll try again sometime soon, methinks.

-Justin

Re: broken RM7000 in CVS ...

[Jun Sun]

Justin Carlson wrote:
> 
> This is sort of true.  Mips32 does do a pretty good job of defining how to
> probe for L1 caches and the like, but other things, such as L2 caches, are not
> going to be so easily probed.  

My understanding is that we don't have a standard way to probe for external
cache (L2 or L3).  So this problem is not only for MIPS32 cpus.

One possible fix is to have board-specific setup routine fill in the needed
data in the mipc_cpu structure, although I am not sure if that is a little too
late in the startup process.  (I think at least one flush_cache call is made
before we reach board_setup() routine).

Jun

> >
> > Along this line, it probably makes sense to have another pointer to
> > mips_cpu_config() function, where for MIPS32 it is the standard MIPS32 config
> > probing function and for most others it is NULL.
> >
> > Now the mips_cpu_table looks like :
> >
> > struct mips_cpu mips_cpu_table[]={
> >       { PRID_IMP_4KC, mips32_cpu_config},
> >       { PRID_IMP_RM7K, null, 0xaaa, {...}}
> >       .....
> > };
> 
> If I'm understanding your idea correctly, this table would require you to
> always compile in all the mmu routines for all processors, just to fill in the
> table entries.  Doesn't seem like a particularly good idea to me, even if we
> could use generic mips32 routines for most parts.
>

Each table entry can be surrounded by something like #if
defined(CONFIG_CPU_RM7000) and #endif.  That should take care of the problem.

 
Jun

Re: broken RM7000 in CVS ...

[Justin Carlson]

On Fri, 12 Jan 2001, you wrote:
> Justin Carlson wrote:
> > 
> > This is sort of true.  Mips32 does do a pretty good job of defining how to
> > probe for L1 caches and the like, but other things, such as L2 caches, are not
> > going to be so easily probed.  
> 
> My understanding is that we don't have a standard way to probe for external
> cache (L2 or L3).  So this problem is not only for MIPS32 cpus.
> 

It's not specific to mips32 processors, no.  My point was, the mips32 spec
doesn't (and probably shouldn't, IMHO) solve this problem completely.

> > If I'm understanding your idea correctly, this table would require you to
> > always compile in all the mmu routines for all processors, just to fill in the
> > table entries.  Doesn't seem like a particularly good idea to me, even if we
> > could use generic mips32 routines for most parts.
> >
> 
> Each table entry can be surrounded by something like #if
> defined(CONFIG_CPU_RM7000) and #endif.  That should take care of the problem.
> 

Not if you want to have constant-defined offsets into the table.  Which is just
about the only reason to use a table for this...Either:

1)  You've got multiple entries in the table for different cpus, which you're
indexing by some hash of PRID fields.  This requires a full table.  (Or a really
ugly hash function that's adaptive depending on which which cpu support is
compiled in)

2)  You've got a single entry table.  

Unless I'm really misunderstanding what you're proposing...

-Justin

Re: broken RM7000 in CVS ...

[Jun Sun]

Justin Carlson wrote:
> 
> On Fri, 12 Jan 2001, you wrote:
> > > If I'm understanding your idea correctly, this table would require you to
> > > always compile in all the mmu routines for all processors, just to fill in the
> > > table entries.  Doesn't seem like a particularly good idea to me, even if we
> > > could use generic mips32 routines for most parts.
> > >
> >
> > Each table entry can be surrounded by something like #if
> > defined(CONFIG_CPU_RM7000) and #endif.  That should take care of the problem.
> >
> 
> Not if you want to have constant-defined offsets into the table.  Which is just
> about the only reason to use a table for this...Either:
> 

No, I am thinking to have constant-defined offset into the table.  Instead, I
am thinking to do a linear search of the table and find a matching entry based
on the PRID.

Without table, I can see two alternatives, 1) switch/case statement to fill in
the data by statements (which is the current case) or 2) for each CPU
(protected by #ifdef CONFIG_) we define a mips_cpu struct.

I guess I just like table better than switch/case statements.  Table seems
cleaner to me.

I like table over option 2) because it is possible to build a kernel that
supports multiple CPUs.

> 1)  You've got multiple entries in the table for different cpus, which you're
> indexing by some hash of PRID fields.  This requires a full table.  (Or a really
> ugly hash function that's adaptive depending on which which cpu support is
> compiled in)
> 
> 2)  You've got a single entry table.
> 

In practice most tables probably only have single entry (due to the config),
but I guess that is OK.


Jun

Re: broken RM7000 in CVS ...

[Alan Cox]

> My understanding is that we don't have a standard way to probe for external
> cache (L2 or L3).  So this problem is not only for MIPS32 cpus.

Cache is very arch specific. You don't want to know how you find L2 cache
on a MacII for example 8)

Re: broken RM7000 in CVS ...

[Alan Cox]

> My understanding is that we don't have a standard way to probe for external
> cache (L2 or L3).  So this problem is not only for MIPS32 cpus.

Cache is very arch specific. You don't want to know how you find L2 cache
on a MacII for example 8)

Re: broken RM7000 in CVS ...

[Ralf Baechle]

On Fri, Jan 12, 2001 at 11:48:50PM +0000, Alan Cox wrote:

> > My understanding is that we don't have a standard way to probe for external
> > cache (L2 or L3).  So this problem is not only for MIPS32 cpus.
> 
> Cache is very arch specific. You don't want to know how you find L2 cache
> on a MacII for example 8)

Actually the Indy's R4600 / R5000 second level caches also call for the use
of LARTs in a while (1) loop ;-)  Read the generic code had to be changed
in order to support in a sane way.

  Ralf

Re: broken RM7000 in CVS ...

[Ralf Baechle]

On Fri, Jan 12, 2001 at 11:48:50PM +0000, Alan Cox wrote:

> > My understanding is that we don't have a standard way to probe for external
> > cache (L2 or L3).  So this problem is not only for MIPS32 cpus.
> 
> Cache is very arch specific. You don't want to know how you find L2 cache
> on a MacII for example 8)

Actually the Indy's R4600 / R5000 second level caches also call for the use
of LARTs in a while (1) loop ;-)  Read the generic code had to be changed
in order to support in a sane way.

  Ralf

Re: broken RM7000 in CVS ...

[Kevin D. Kissell]

> "Kevin D. Kissell" wrote:
> >
> > Yes, arguably the mips_cpu structure could also contain
> > a descriptor of the MMU routines to bind, and it probably
> > would have if it would have been a simple matter of an
> > address/length of a vector to copy.  But heck, it could
> > be a function pointer as well, I suppose.
> >
>
> I think that is a good idea.  I suggest we have two more pointers in the
> mips_cpu strcuture : one to mips_mmu_ops structure, and the other to
> setup_exception_vectors() function.
>
> BTW, I have a question about MIPS32 (or 4KC).  Do all MIPS32 CPUs have the
> same PRID?

No.

> Or all "incarnations" of 4KC have the same PRID?

The same upper 24 bits, anyway.

> I suppose MIPS32 CPUs have a more complete config register where you can
> probe for all the options.  For others we can use a table-like structure
to fill in
> the options.

And thereby hangs a tale.  MIPS tweaked the Config register, and has
added additional registers "behind" the Config register (a previously
reserved zero field in the mtc0/mfc0 instructions now serves as a
sort of bank select, and most current gnu assemblers recognize
"mfc0 $2, 16, 1", etc.) in MIPS32 to allow MMU and cache configuration,
presence of FPU, etc, to be read at run-time.  This is important for
synthesizable cores like the 4K and 5K families, since the same
basic core (and hence same PrID) can be instantiated with different
cache geometries, etc.  So one of the first things we did was write
code that reads those registers and populates the mips_cpu structure
accordingly.  Unfortunately, we were ahead of our time!  At the time
we needed to ship our Linux kernel tweaks to people outside MIPS,
those parts of the MIPS32 spec were still confidential, so we had
to take them out and make the 4K and 5K revert to using the same
table as everyone else!   I believe we got the all-clear to put the code
back in some time ago, but Carsten and I have been too busy with
other stuff.  Sorry!

> Along this line, it probably makes sense to have another pointer to
> mips_cpu_config() function, where for MIPS32 it is the standard MIPS32
config
> probing function and for most others it is NULL.

It probably wouldn't be NULL.   One would still want a routine
that probes for the size of the caches if the table indicates that
it could be variable for that device.  Etc.
>
> Now the mips_cpu_table looks like :
>
> struct mips_cpu mips_cpu_table[]={
> { PRID_IMP_4KC, mips32_cpu_config},
> { PRID_IMP_RM7K, null, 0xaaa, {...}}
> .....
> };
>
> The cpu_probe() routine will now look like:
> {
>    read prid register
>    find mips_cpu_table[i] with matching PRID.
>    mips_cpu = &mips_cpu_table[i];
>    if (mips_cpu->mips_cpu_config) mips_cpu->mips_cpu_config();

Dispatching through the table to the config routine would
be possible, but unless you want to write a seperate routine
for every possible set of CPU parameters and options, it would
be preferrable (IMHO) to keep the parameters in the table
and to pass the table entry address to the function being
invoked.  That way some smaller number of common functions
could be used.

> }
>
> To me this is beautiful. Am I dreaming? :-)

Not unless you are asleep.  If you're awake, we
call that hallucinating.  ;-)

            Regards,

            Kevin K.

Re: broken RM7000 in CVS ...

[Kevin D. Kissell]

> "Kevin D. Kissell" wrote:
> >
> > Yes, arguably the mips_cpu structure could also contain
> > a descriptor of the MMU routines to bind, and it probably
> > would have if it would have been a simple matter of an
> > address/length of a vector to copy.  But heck, it could
> > be a function pointer as well, I suppose.
> >
>
> I think that is a good idea.  I suggest we have two more pointers in the
> mips_cpu strcuture : one to mips_mmu_ops structure, and the other to
> setup_exception_vectors() function.
>
> BTW, I have a question about MIPS32 (or 4KC).  Do all MIPS32 CPUs have the
> same PRID?

No.

> Or all "incarnations" of 4KC have the same PRID?

The same upper 24 bits, anyway.

> I suppose MIPS32 CPUs have a more complete config register where you can
> probe for all the options.  For others we can use a table-like structure
to fill in
> the options.

And thereby hangs a tale.  MIPS tweaked the Config register, and has
added additional registers "behind" the Config register (a previously
reserved zero field in the mtc0/mfc0 instructions now serves as a
sort of bank select, and most current gnu assemblers recognize
"mfc0 $2, 16, 1", etc.) in MIPS32 to allow MMU and cache configuration,
presence of FPU, etc, to be read at run-time.  This is important for
synthesizable cores like the 4K and 5K families, since the same
basic core (and hence same PrID) can be instantiated with different
cache geometries, etc.  So one of the first things we did was write
code that reads those registers and populates the mips_cpu structure
accordingly.  Unfortunately, we were ahead of our time!  At the time
we needed to ship our Linux kernel tweaks to people outside MIPS,
those parts of the MIPS32 spec were still confidential, so we had
to take them out and make the 4K and 5K revert to using the same
table as everyone else!   I believe we got the all-clear to put the code
back in some time ago, but Carsten and I have been too busy with
other stuff.  Sorry!

> Along this line, it probably makes sense to have another pointer to
> mips_cpu_config() function, where for MIPS32 it is the standard MIPS32
config
> probing function and for most others it is NULL.

It probably wouldn't be NULL.   One would still want a routine
that probes for the size of the caches if the table indicates that
it could be variable for that device.  Etc.
>
> Now the mips_cpu_table looks like :
>
> struct mips_cpu mips_cpu_table[]={
> { PRID_IMP_4KC, mips32_cpu_config},
> { PRID_IMP_RM7K, null, 0xaaa, {...}}
> .....
> };
>
> The cpu_probe() routine will now look like:
> {
>    read prid register
>    find mips_cpu_table[i] with matching PRID.
>    mips_cpu = &mips_cpu_table[i];
>    if (mips_cpu->mips_cpu_config) mips_cpu->mips_cpu_config();

Dispatching through the table to the config routine would
be possible, but unless you want to write a seperate routine
for every possible set of CPU parameters and options, it would
be preferrable (IMHO) to keep the parameters in the table
and to pass the table entry address to the function being
invoked.  That way some smaller number of common functions
could be used.

> }
>
> To me this is beautiful. Am I dreaming? :-)

Not unless you are asleep.  If you're awake, we
call that hallucinating.  ;-)

            Regards,

            Kevin K.

Re: broken RM7000 in CVS ...

[Ralf Baechle]

On Fri, Jan 12, 2001 at 08:42:24PM +0100, Kevin D. Kissell wrote:

> And thereby hangs a tale.  MIPS tweaked the Config register, and has
> added additional registers "behind" the Config register (a previously
> reserved zero field in the mtc0/mfc0 instructions now serves as a
> sort of bank select, and most current gnu assemblers recognize
> "mfc0 $2, 16, 1", etc.) in MIPS32 to allow MMU and cache configuration,

Do you know when the ability to address the additional register banks got
added to gas?  I'd like to get rid of the sucky .word <opcode> thing we're
using right now to address the additional register banks.

  Ralf

Re: broken RM7000 in CVS ...

[Ralf Baechle]

On Fri, Jan 12, 2001 at 08:42:24PM +0100, Kevin D. Kissell wrote:

> And thereby hangs a tale.  MIPS tweaked the Config register, and has
> added additional registers "behind" the Config register (a previously
> reserved zero field in the mtc0/mfc0 instructions now serves as a
> sort of bank select, and most current gnu assemblers recognize
> "mfc0 $2, 16, 1", etc.) in MIPS32 to allow MMU and cache configuration,

Do you know when the ability to address the additional register banks got
added to gas?  I'd like to get rid of the sucky .word <opcode> thing we're
using right now to address the additional register banks.

  Ralf