[RFC] "indirect" DCR access (40x, BookE)

[RFC] "indirect" DCR access (40x, BookE)

[Eugene Surovegin]

Hello all!

As some people know using DCR is sometimes PITA ;-).
The reason is simple, DCR number is encoded in instruction itself, so
one cannot write code like:

	for (i = 0; i < LAST_REG; ++i)
		mtdcr(DCR_XXX_BASE + i, 0);

Because of this limitation we have to use a lot of ugly #ifdefs and/or
explicit switch/if statements when accessing DCR from device drivers
(see for example include/asm-ppc/ppc405-dma.h,
drivers/net/ibm_emac/ibm_ocp_mal.c (2.4 tree), etc)

There is a simple solution for this problem (I'm sure everybody
thought of this one, but just didn't have time to code it :).

This approach trades space (16K of code) for convenience.

Here is the short snippet which demonstrates the idea (full patch is
quite big and can be found at http://kernel.ebshome.net/dcr-2.6.diff):

#define DCR_ACCESS_PROLOG(table) \
        rlwinm  r3,r3,4,18,27;   \
        lis     r5,table@h;      \
        ori     r5,r5,table@l;   \
        add     r3,r3,r5;        \
        mtctr   r3;              \
        bctr

_GLOBAL(__mfdcr)
        DCR_ACCESS_PROLOG(__mfdcr_table)

_GLOBAL(__mtdcr)
        DCR_ACCESS_PROLOG(__mtdcr_table)

__mfdcr_table:
        mfdcr  r3,0; blr
__mtdcr_table:
        mtdcr  0,r4; blr
        mfdcr  r3,1; blr
        mtdcr  1,r4; blr
        mfdcr  r3,2; blr
        mtdcr  2,r4; blr
        mfdcr  r3,3; blr
	....

I grouped reading & writing of the same DCR together to make these
functions more cache friendly. I'm not sure about the names, though
:).

This patch is against 2.6, I can easily make for 2.4 as well if there
is an interest.

Please note, I'm not proposing _removing_ current "mfdcr/mtdcr"
macros, they have their use. These "indirect" versions can be used in
some drivers to get more clean and readable code.

Comments, suggestions are welcome.

Thanks,
Eugene

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: [RFC] "indirect" DCR access (40x, BookE)

[Dan Malek]

Eugene Surovegin wrote:

> Comments, suggestions are welcome.

I think you should just write it as self modifying code :-)
Write the instruction with the DCR number and just execute it.


	-- Dan


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: [RFC] "indirect" DCR access (40x, BookE)

[Eugene Surovegin]

On Thu, Mar 11, 2004 at 09:46:59PM -0500, Dan Malek wrote:
>
> >Comments, suggestions are welcome.
>
> I think you should just write it as self modifying code :-)
> Write the instruction with the DCR number and just execute it.

And deal with locking and icache/dcache coherency ?

No, thanks :)

Eugene.

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: [RFC] "indirect" DCR access (40x, BookE)

[Stephen Williams]

Eugene Surovegin ebs-at-ebshome.net |PPC Linux Embedded| wrote:
> On Thu, Mar 11, 2004 at 09:46:59PM -0500, Dan Malek wrote:
>
>>>Comments, suggestions are welcome.
>>
>>I think you should just write it as self modifying code :-)
>>Write the instruction with the DCR number and just execute it.
>
>
> And deal with locking and icache/dcache coherency ?
>
> No, thanks :)


Actually, I recall that there is a code fixup mechanism that
is invoked early in kernel init that does exactly that: it
manages some machine specific differences by editing the code
in place in a safe way. My memory is not good enough to point
to the right place in the code, though, or to explain how it
works.

--
Steve Williams                "The woods are lovely, dark and deep.
steve at XXXXXXXXXX           But I have promises to keep,
http://www.XXXXXXXXXX         and lines to code before I sleep,
http://www.picturel.com       And lines to code before I sleep."


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: [RFC] "indirect" DCR access (40x, BookE)

[Eugene Surovegin]

On Thu, Mar 11, 2004 at 08:44:09PM -0800, Stephen Williams wrote:
> >>I think you should just write it as self modifying code :-)
> >>Write the instruction with the DCR number and just execute it.
> >
> >
> >And deal with locking and icache/dcache coherency ?
> >
> >No, thanks :)
>
>
> Actually, I recall that there is a code fixup mechanism that
> is invoked early in kernel init that does exactly that: it
> manages some machine specific differences by editing the code
> in place in a safe way.

Yes, you are correct, but this is done only once during startup and
nobody cares how fast it is. BTW, there is no locking issues at this
stage.

I'm not saying that it's impossible :). It's just not very efficient
to do such stuff on run-time (lock a spinlock, change memory, dcache
flush, icache invalidate, isync...)

Eugene.

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: [RFC] "indirect" DCR access (40x, BookE)

[Kumar Gala]

Look in u-boot.  They have some user commands that allow
reading/writing dcr's.  These commands are written with self modifying
code, I would recommend that we reuse that.

Also, it begs the question of should this be extended to SPRs, not that
I have come across a case with SPRs that I need to iterate over a large
list.

- kumar

On Mar 11, 2004, at 10:54 PM, Eugene Surovegin wrote:

>
> On Thu, Mar 11, 2004 at 08:44:09PM -0800, Stephen Williams wrote:
>>>> I think you should just write it as self modifying code :-)
>>>> Write the instruction with the DCR number and just execute it.
>>>
>>>
>>> And deal with locking and icache/dcache coherency ?
>>>
>>> No, thanks :)
>>
>>
>> Actually, I recall that there is a code fixup mechanism that
>> is invoked early in kernel init that does exactly that: it
>> manages some machine specific differences by editing the code
>> in place in a safe way.
>
> Yes, you are correct, but this is done only once during startup and
> nobody cares how fast it is. BTW, there is no locking issues at this
> stage.
>
> I'm not saying that it's impossible :). It's just not very efficient
> to do such stuff on run-time (lock a spinlock, change memory, dcache
> flush, icache invalidate, isync...)
>
> Eugene.
>


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

RE: [RFC] "indirect" DCR access (40x, BookE)

[Chuck Meade]

Before you guys go too far with this, just keep in mind that there are
efforts under way to have Linux XIP -- not running from RAM.  You will
need to have any self-modifying code in a writable region obviously.
What is not obvious is that you may need to preallocate a region of RAM
to do this in the case of XIP.

Chuck

> -----Original Message-----
> From: owner-linuxppc-embedded@lists.linuxppc.org
> [mailto:owner-linuxppc-embedded@lists.linuxppc.org]On Behalf Of Kumar
> Gala
> Sent: Friday, March 12, 2004 9:25 AM
> To: Eugene Surovegin
> Cc: linuxppc-embedded@lists.linuxppc.org; Stephen Williams
> Subject: Re: [RFC] "indirect" DCR access (40x, BookE)
>
>
>
> Look in u-boot.  They have some user commands that allow
> reading/writing dcr's.  These commands are written with self modifying
> code, I would recommend that we reuse that.
>
> Also, it begs the question of should this be extended to SPRs, not that
> I have come across a case with SPRs that I need to iterate over a large
> list.
>
> - kumar
>
> On Mar 11, 2004, at 10:54 PM, Eugene Surovegin wrote:
>
> >
> > On Thu, Mar 11, 2004 at 08:44:09PM -0800, Stephen Williams wrote:
> >>>> I think you should just write it as self modifying code :-)
> >>>> Write the instruction with the DCR number and just execute it.
> >>>
> >>>
> >>> And deal with locking and icache/dcache coherency ?
> >>>
> >>> No, thanks :)
> >>
> >>
> >> Actually, I recall that there is a code fixup mechanism that
> >> is invoked early in kernel init that does exactly that: it
> >> manages some machine specific differences by editing the code
> >> in place in a safe way.
> >
> > Yes, you are correct, but this is done only once during startup and
> > nobody cares how fast it is. BTW, there is no locking issues at this
> > stage.
> >
> > I'm not saying that it's impossible :). It's just not very efficient
> > to do such stuff on run-time (lock a spinlock, change memory, dcache
> > flush, icache invalidate, isync...)
> >
> > Eugene.
> >
>
>
>
>

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: [RFC] "indirect" DCR access (40x, BookE)

[Eugene Surovegin]

On Fri, Mar 12, 2004 at 08:25:23AM -0600, Kumar Gala wrote:
>
> Look in u-boot.  They have some user commands that allow
> reading/writing dcr's.  These commands are written with self modifying
> code, I would recommend that we reuse that.
>

I would not.

Kumar, I'm aware of code in u-boot :).
Did you read this code comment ?

It clearly states that is not efficient.
It may be OK for a boot loader, because nobody cares much about it
performance.

Let me repeat my point. I _do_ know how to write self-modifying code,
I don't _like_ this approach mostly because it's not very efficient.

Eugene.

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: [RFC] "indirect" DCR access (40x, BookE)

[Dan Malek]

Kumar Gala wrote:

> Look in u-boot.  They have some user commands that allow
> reading/writing dcr's.

> Also, it begs the question of should this be extended to SPRs,

This discussion continues to point out that DCR implementations
are poor for programming.  Back in the days of 16-bit address
spaces, using DCRs or alternate I/O spaces was reasonable.  Decades
ago Motorola, among others, proved larger address spaces and
memory mapped I/O was a preferable programming method.  I hope
chip designers are listening........we like memory mapped I/O,
and with greater than 32-bit addressing on many processors, there is
plenty of memory to map :-)

The SPRs are for different purposes.  These are logical extensions
to specific processor functions.  They are used in specific places
in the software, I've never experienced a need (except in the case of
the 403 that can't decide which spr to use for the timebase) that I
needed to have some code use different SPRs based on some input.
I don't see a need to provide a general access SPR function.

Of course, I thought of yet another method :-)  We could emulate
memory mapped I/O with DCRs.  We would reserve some VM space, access
to this would be trapped, and the offset would represent the DCR number.
Decode the GPR number from the load or store, and run the DCR move.



	-- Dan


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: [RFC] "indirect" DCR access (40x, BookE)

[Benjamin Herrenschmidt]

On Sat, 2004-03-13 at 01:25, Kumar Gala wrote:
> Look in u-boot.  They have some user commands that allow
> reading/writing dcr's.  These commands are written with self modifying
> code, I would recommend that we reuse that.
>
> Also, it begs the question of should this be extended to SPRs, not that
> I have come across a case with SPRs that I need to iterate over a large
> list.

That would way too slow for DCRs used in drivers for example,

I much prefer Eugene's idea.

Ben.


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: [RFC] "indirect" DCR access (40x, BookE)

[Eugene Surovegin]

On Fri, Mar 19, 2004 at 03:00:53PM +1100, Benjamin Herrenschmidt wrote:
> I much prefer Eugene's idea.

During discussion with Ben on IRC he suggested that having two
different DCR access wasn't a good idea and better if we use GCC
__builtin_constant_p extension to decide what version to call.

New patch version can be found http://kernel.ebshome.net/dcr-2.6-2.diff

Here is only _new_ part for review:

===== include/asm-ppc/reg_booke.h 1.5 vs edited =====
--- 1.5/include/asm-ppc/reg_booke.h     Fri Feb 13 07:24:55 2004
+++ edited/include/asm-ppc/reg_booke.h  Mon Mar 22 17:44:02 2004
@@ -10,20 +10,33 @@
 #define __ASM_PPC_REG_BOOKE_H__

 #ifndef __ASSEMBLY__
+#include <linux/compiler.h>
+
 /* Device Control Registers */
+void __mtdcr(int reg, unsigned int val);
+unsigned int __mfdcr(int reg);
 #define mfdcr(rn) mfdcr_or_dflt(rn, 0)
 #define mfdcr_or_dflt(rn,default_rval)                                 \
        ({unsigned int rval;                                            \
-       if (rn == 0)                                                    \
+       if (unlikely(rn == 0))                                          \
                rval = default_rval;                                    \
        else                                                            \
-               asm volatile("mfdcr %0," __stringify(rn) : "=r" (rval)); \
+               if (__builtin_constant_p(rn))                           \
+                       asm volatile("mfdcr %0," __stringify(rn)        \
+                                     : "=r" (rval));                   \
+               else                                                    \
+                       rval = __mfdcr(rn);                             \
        rval;})

 #define mtdcr(rn, v)                                                   \
 do {                                                                   \
-       if (rn != 0)                                                    \
-               asm volatile("mtdcr " __stringify(rn) ",%0" : : "r" (v)); \
+       if (likely(rn != 0)){                                           \
+               if (__builtin_constant_p(rn))                           \
+                       asm volatile("mtdcr " __stringify(rn) ",%0"     \
+                                     : : "r" (v));                     \
+               else                                                    \
+                       __mtdcr(rn, v);                                 \
+       }                                                               \
 } while (0)

 /* R/W of indirect DCRs make use of standard naming conventions for DCRs */

Thanks,

Eugene

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Re: [RFC] "indirect" DCR access (40x, BookE)

[Kumar Gala]

Looks good to me.

- kumar

On Mar 22, 2004, at 8:47 PM, Eugene Surovegin wrote:

>
> On Fri, Mar 19, 2004 at 03:00:53PM +1100, Benjamin Herrenschmidt wrote:
>> I much prefer Eugene's idea.
>
> During discussion with Ben on IRC he suggested that having two
> different DCR access wasn't a good idea and better if we use GCC
> __builtin_constant_p extension to decide what version to call.
>
> New patch version can be found http://kernel.ebshome.net/dcr-2.6-2.diff
>
> Here is only _new_ part for review:
>
> ===== include/asm-ppc/reg_booke.h 1.5 vs edited =====
> --- 1.5/include/asm-ppc/reg_booke.h     Fri Feb 13 07:24:55 2004
> +++ edited/include/asm-ppc/reg_booke.h  Mon Mar 22 17:44:02 2004
> @@ -10,20 +10,33 @@
>  #define __ASM_PPC_REG_BOOKE_H__
>
>  #ifndef __ASSEMBLY__
> +#include <linux/compiler.h>
> +
>  /* Device Control Registers */
> +void __mtdcr(int reg, unsigned int val);
> +unsigned int __mfdcr(int reg);
>  #define mfdcr(rn) mfdcr_or_dflt(rn, 0)
>  #define mfdcr_or_dflt(rn,default_rval)
>  \
>         ({unsigned int rval;
>  \
> -       if (rn == 0)
>  \
> +       if (unlikely(rn == 0))
>  \
>                 rval = default_rval;
>  \
>         else
>  \
> -               asm volatile("mfdcr %0," __stringify(rn) : "=r"
> (rval)); \
> +               if (__builtin_constant_p(rn))
>  \
> +                       asm volatile("mfdcr %0," __stringify(rn)
>  \
> +                                     : "=r" (rval));
>  \
> +               else
>  \
> +                       rval = __mfdcr(rn);
>  \
>         rval;})
>
>  #define mtdcr(rn, v)
>  \
>  do {
>  \
> -       if (rn != 0)
>  \
> -               asm volatile("mtdcr " __stringify(rn) ",%0" : : "r"
> (v)); \
> +       if (likely(rn != 0)){
>  \
> +               if (__builtin_constant_p(rn))
>  \
> +                       asm volatile("mtdcr " __stringify(rn) ",%0"
>  \
> +                                     : : "r" (v));
>  \
> +               else
>  \
> +                       __mtdcr(rn, v);
>  \
> +       }
>  \
>  } while (0)
>
>  /* R/W of indirect DCRs make use of standard naming conventions for
> DCRs */
>
> Thanks,
>
> Eugene
>


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/