[U-Boot] [RFC] PPC: post_word_{load/store} - eliminate redundant code

[U-Boot] [RFC] PPC: post_word_{load/store} - eliminate redundant code

[Michael Zaidman]

Hi All,

In order to eliminate redundant code for powerPC architecture when
defining post_word_{load/store} routines I suggest  moving them into
the arch/ppc/lib/commproc.c file rather than adding them to every
arch/ppc/cpu/XXXX.
Also, defining them with weak attribute will permit overriding them
for particular board or CPU.

I'll send the patch later, of course if no one has any objections
against this solution.

Regards,
Michael

[U-Boot] [RFC] PPC: post_word_{load/store} - eliminate redundant code

[Michael Zaidman]

On Mon, Apr 19, 2010 at 7:44 PM, Michael Zaidman
<michael.zaidman@gmail.com> wrote:
> Hi All,
>
> In order to eliminate redundant code for powerPC architecture when
> defining post_word_{load/store} routines I suggest ?moving them into
> the arch/ppc/lib/commproc.c file rather than adding them to every
> arch/ppc/cpu/XXXX.
> Also, defining them with weak attribute will permit overriding them
> for particular board or CPU.
>
> I'll send the patch later, of course if no one has any objections
> against this solution.
>

Please see this patch:
http://lists.denx.de/pipermail/u-boot/2010-April/070319.html

-michael

[U-Boot] [RFC] PPC: post_word_{load/store} - eliminate redundant code

[Wolfgang Denk]

Dear Michael Zaidman,

In message <v2t660c0f821004190944p4a0a060fha1c92dbede09a7bf@mail.gmail.com> you wrote:
> 
> In order to eliminate redundant code for powerPC architecture when
> defining post_word_{load/store} routines I suggest  moving them into
> the arch/ppc/lib/commproc.c file rather than adding them to every
> arch/ppc/cpu/XXXX.

NAK. The commproc.* files are supposed to contain code dealing with
the PowerQUICC Communication Processor Modules. This is in no way the
right place for common code.

Stefan - arch/ppc/cpu/ppc4xx/commproc.c  makes no sense - can we
please clean this up?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The people of Gideon have always believed that life is  sacred.  That
the  love  of  life  is  the  greatest  gift  ... We are incapable of
destroying or interfering with the creation of that which we love  so
deeply -- life in every form from fetus to developed being.
	-- Hodin of Gideon, "The Mark of Gideon", stardate 5423.4

[U-Boot] [RFC] PPC: post_word_{load/store} - eliminate redundant code

[Stefan Roese]

Hi Wolfgang,

On Tuesday 20 April 2010 23:39:09 Wolfgang Denk wrote:
> > In order to eliminate redundant code for powerPC architecture when
> > defining post_word_{load/store} routines I suggest  moving them into
> > the arch/ppc/lib/commproc.c file rather than adding them to every
> > arch/ppc/cpu/XXXX.
> 
> NAK. The commproc.* files are supposed to contain code dealing with
> the PowerQUICC Communication Processor Modules. This is in no way the
> right place for common code.
> 
> Stefan - arch/ppc/cpu/ppc4xx/commproc.c  makes no sense - can we
> please clean this up?

Yes, I'll take a look at it.

Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

[U-Boot] [RFC] PPC: post_word_{load/store} - eliminate redundant code

[Michael Zaidman]

On Wed, Apr 21, 2010 at 12:39 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Michael Zaidman,
>
> In message <v2t660c0f821004190944p4a0a060fha1c92dbede09a7bf@mail.gmail.com> you wrote:
>>
>> In order to eliminate redundant code for powerPC architecture when
>> defining post_word_{load/store} routines I suggest ?moving them into
>> the arch/ppc/lib/commproc.c file rather than adding them to every
>> arch/ppc/cpu/XXXX.
>
> NAK. The commproc.* files are supposed to contain code dealing with
> the PowerQUICC Communication Processor Modules. This is in no way the
> right place for common code.

OK, my wrong interpretation of "comm" as common rather than
communication, sorry.
I see that others have also failed to interpret it correctly...


> Stefan - arch/ppc/cpu/ppc4xx/commproc.c ?makes no sense - can we
> please clean this up?
>
Cleaning this up please take a look on commproc.h also. It serves the
CPM of 8xx only while commproc.c is present under number of PowerQUICC
CPU architectures. Although, ONLY commproc.c of mpc8xx includes the
commproc.h, to me, it looks like inconsistency.

Regards,
Michael

[U-Boot] [RFC] PPC: post_word_{load/store} - eliminate redundant code

[Michael Zaidman]

On Wed, Apr 21, 2010 at 12:39 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Michael Zaidman,
>
> In message <v2t660c0f821004190944p4a0a060fha1c92dbede09a7bf@mail.gmail.com> you wrote:
>>
>> In order to eliminate redundant code for powerPC architecture when
>> defining post_word_{load/store} routines I suggest ?moving them into
>> the arch/ppc/lib/commproc.c file rather than adding them to every
>> arch/ppc/cpu/XXXX.
>
> NAK. The commproc.* files are supposed to contain code dealing with
> the PowerQUICC Communication Processor Modules. This is in no way the
> right place for common code.
>
Meanwhile the  post_word_{load/store} are located in the commproc.c
files for the following architectures:
mpc8260,
mpc85xx,
mpc8xx,
ppc4xx.

Looking for the way to add POST functionality for the mpc83xx family I
realized that the post_word_{load/store} can be generalized for all
ppc architectures.

Could you point please to the file where such kind of things - common
for all CPU families of particular architecture should be located?

Thanks,
Michael

[U-Boot] [RFC] PPC: post_word_{load/store} - eliminate redundant code

[Stefan Roese]

Hi Michael,

On Wednesday 21 April 2010 15:24:15 Michael Zaidman wrote:
> >> In order to eliminate redundant code for powerPC architecture when
> >> defining post_word_{load/store} routines I suggest  moving them into
> >> the arch/ppc/lib/commproc.c file rather than adding them to every
> >> arch/ppc/cpu/XXXX.
> > 
> > NAK. The commproc.* files are supposed to contain code dealing with
> > the PowerQUICC Communication Processor Modules. This is in no way the
> > right place for common code.
> 
> Meanwhile the  post_word_{load/store} are located in the commproc.c
> files for the following architectures:
> mpc8260,
> mpc85xx,
> mpc8xx,
> ppc4xx.

I just started looking into this as well.
 
> Looking for the way to add POST functionality for the mpc83xx family I
> realized that the post_word_{load/store} can be generalized for all
> ppc architectures.

Or a weak default version even for all architectures.
 
> Could you point please to the file where such kind of things - common
> for all CPU families of particular architecture should be located?

If done for all architectures, then "post/post.c" (already exists) or 
"lib/post.c" (new file) comes to my mind.

Any comments on this? Other suggestions?

Thanks.

Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

[U-Boot] [RFC] PPC: post_word_{load/store} - eliminate redundant code

[Wolfgang Denk]

Dear Michael Zaidman,

In message <x2h660c0f821004210624y32636405j7746304a059def4a@mail.gmail.com> you wrote:
>
> Meanwhile the  post_word_{load/store} are located in the commproc.c
> files for the following architectures:
> mpc8260,
> mpc85xx,
> mpc8xx,
> ppc4xx.
> 
> Looking for the way to add POST functionality for the mpc83xx family I
> realized that the post_word_{load/store} can be generalized for all
> ppc architectures.
> 
> Could you point please to the file where such kind of things - common
> for all CPU families of particular architecture should be located?

Actually there are two parts to it:

bootcount_store() and bootcount_load() are needed for the boot
counter, a generic feature; I tend to move these into
arch/powerpc/lib/bootcount.c; the code also needs to be rewritten to
use I/O accessors.

post_word_store() and post_word_load() is architecture specific,
common POST code that unfortunately also gets used by the logbuffer
code. This should be split. Then we would have
arch/powerpc/lib/logbuf.c and post/arch/powerpc/post_io.c or such.

Please also note that there are more files implementing such code -
see for example here:

	board/cm5200/cm5200.c
	board/bf537-stamp/post.c
	board/barco/barco.c
	board/bc3450/bc3450.c
	board/ssv/common/post.c
	board/tqc/tqm5200/tqm5200.c
	board/xes/xpedite1000/xpedite1000.c


Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Without facts, the decision cannot be made logically. You  must  rely
on your human intuition.
	-- Spock, "Assignment: Earth", stardate unknown

[U-Boot] [RFC] PPC: post_word_{load/store} - eliminate redundant code

[Michael Zaidman]

On Wed, Apr 21, 2010 at 4:51 PM, Stefan Roese <sr@denx.de> wrote:
> Hi Michael,
>
> On Wednesday 21 April 2010 15:24:15 Michael Zaidman wrote:
>> >> In order to eliminate redundant code for powerPC architecture when
>> >> defining post_word_{load/store} routines I suggest ?moving them into
>> >> the arch/ppc/lib/commproc.c file rather than adding them to every
>> >> arch/ppc/cpu/XXXX.
>> >
>> > NAK. The commproc.* files are supposed to contain code dealing with
>> > the PowerQUICC Communication Processor Modules. This is in no way the
>> > right place for common code.
>>
>> Meanwhile the ?post_word_{load/store} are located in the commproc.c
>> files for the following architectures:
>> mpc8260,
>> mpc85xx,
>> mpc8xx,
>> ppc4xx.
>
> I just started looking into this as well.
>
>> Looking for the way to add POST functionality for the mpc83xx family I
>> realized that the post_word_{load/store} can be generalized for all
>> ppc architectures.
>
> Or a weak default version even for all architectures.

Ok, but it solve the problem partially - it will pass the compilation
but will not work for some architectures. There are a number of
differences between CPU architectures such as big and little endians,
size of the data bus and so on...

Regards,
Michael

[U-Boot] [RFC] PPC: post_word_{load/store} - eliminate redundant code

[Michael Zaidman]

On Wed, Apr 21, 2010 at 4:59 PM, Wolfgang Denk <wd@denx.de> wrote:
>
> Actually there are two parts to it:
>
> bootcount_store() and bootcount_load() are needed for the boot
> counter, a generic feature; I tend to move these into
> arch/powerpc/lib/bootcount.c; the code also needs to be rewritten to
> use I/O accessors.
>
> post_word_store() and post_word_load() is architecture specific,
> common POST code that unfortunately also gets used by the logbuffer
> code. This should be split. Then we would have
> arch/powerpc/lib/logbuf.c and post/arch/powerpc/post_io.c or such.
>
> Please also note that there are more files implementing such code -
> see for example here:
>
> ? ? ? ?board/cm5200/cm5200.c
> ? ? ? ?board/bf537-stamp/post.c
> ? ? ? ?board/barco/barco.c
> ? ? ? ?board/bc3450/bc3450.c
> ? ? ? ?board/ssv/common/post.c
> ? ? ? ?board/tqc/tqm5200/tqm5200.c
> ? ? ? ?board/xes/xpedite1000/xpedite1000.c
>
Yes, I have seen them also. I actually thought to clean up them but do
it in two phases - first make the post_word accessors to be common per
arch and  define them as weak so it will not break existing code.
Afterwords - eliminate an existing redundant code.

Thanks for the tips.  Please let me know how do you want me to proceed
with the patch?

Regards,
Michael

[U-Boot] [RFC] PPC: post_word_{load/store} - eliminate redundant code

[Wolfgang Denk]

Dear Michael Zaidman,

In message <o2o660c0f821004210729u4b63cd94n4cb40c87088f8edb@mail.gmail.com> you wrote:
> >
> > Actually there are two parts to it:
> >
> > bootcount_store() and bootcount_load() are needed for the boot
> > counter, a generic feature; I tend to move these into
> > arch/powerpc/lib/bootcount.c; the code also needs to be rewritten to
> > use I/O accessors.
> >
> > post_word_store() and post_word_load() is architecture specific,
> > common POST code that unfortunately also gets used by the logbuffer
> > code. This should be split. Then we would have
> > arch/powerpc/lib/logbuf.c and post/arch/powerpc/post_io.c or such.

Looking at the code I wonder why we need post_word_store() and
post_word_load() functions at all. All implementations I have found
translate into a single ioread32() resp. iowrite32() call.

> Yes, I have seen them also. I actually thought to clean up them but do
> it in two phases - first make the post_word accessors to be common per
> arch and  define them as weak so it will not break existing code.
> Afterwords - eliminate an existing redundant code.
> 
> Thanks for the tips.  Please let me know how do you want me to proceed
> with the patch?

I think we should perform this cleanup in the following steps:

1) Move bootcount_store() and bootcount_load() to architecture
   specific generic locations; this includes both the PowerPC and ARM
   implementations

2) Move arch/blackfin/lib/post.c to post/

3) Eliminate post_word_store() and post_word_load() and use ioread32()
   resp. iowrite32() (or equivalents) directly.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I see that Microsoft's campaign  to  destroy  all  knowledge  of  any
operating   environment   but  its  own  environment-of-the-year  has
succeeded in creating a generation of users who don't understand  the
concept of a shell...
            -- L. Peter Deutsch in <m0x5jNX-000R2UC@lamp.aladdin.com>

[U-Boot] [RFC] PPC: post_word_{load/store} - eliminate redundant code

[Mike Frysinger]

On Wednesday 21 April 2010 17:40:05 Wolfgang Denk wrote:
> Michael Zaidman wrote:
> > > Actually there are two parts to it:
> > > 
> > > bootcount_store() and bootcount_load() are needed for the boot
> > > counter, a generic feature; I tend to move these into
> > > arch/powerpc/lib/bootcount.c; the code also needs to be rewritten to
> > > use I/O accessors.
> > > 
> > > post_word_store() and post_word_load() is architecture specific,
> > > common POST code that unfortunately also gets used by the logbuffer
> > > code. This should be split. Then we would have
> > > arch/powerpc/lib/logbuf.c and post/arch/powerpc/post_io.c or such.
> 
> Looking at the code I wonder why we need post_word_store() and
> post_word_load() functions at all. All implementations I have found
> translate into a single ioread32() resp. iowrite32() call.
> 
> > Yes, I have seen them also. I actually thought to clean up them but do
> > it in two phases - first make the post_word accessors to be common per
> > arch and  define them as weak so it will not break existing code.
> > Afterwords - eliminate an existing redundant code.
> > 
> > Thanks for the tips.  Please let me know how do you want me to proceed
> > with the patch?
> 
> I think we should perform this cleanup in the following steps:
> 
> 1) Move bootcount_store() and bootcount_load() to architecture
>    specific generic locations; this includes both the PowerPC and ARM
>    implementations
> 
> 2) Move arch/blackfin/lib/post.c to post/
> 
> 3) Eliminate post_word_store() and post_word_load() and use ioread32()
>    resp. iowrite32() (or equivalents) directly.

i'd love to see post/ be de-powerpc-ified and unify Blackfin stuff in there.  
it's been an item long standing on our side, but there's always been more 
pressing issues.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100421/8ff7a06c/attachment.pgp 

[U-Boot] [RFC] PPC: post_word_{load/store} - eliminate redundant code

[Michael Zaidman]

On Thu, Apr 22, 2010 at 12:40 AM, Wolfgang Denk <wd@denx.de> wrote:

> Looking at the code I wonder why we need post_word_store() and
> post_word_load() functions at all. All implementations I have found
> translate into a single ioread32() resp. iowrite32() call.
>
Probably, due to big and little endians that the PPC code should
support. On other hand, AFAIK, no one is really using a little endian
notation on PPC platforms in u-boot. I am not sure however, is this a
good enough reason to omit such support? Or am I missing something?

>> Yes, I have seen them also. I actually thought to clean up them but do
>> it in two phases - first make the post_word accessors to be common per
>> arch and ?define them as weak so it will not break existing code.
>> Afterwords - eliminate an existing redundant code.
>>
>> Thanks for the tips. ?Please let me know how do you want me to proceed
>> with the patch?
>
> I think we should perform this cleanup in the following steps:
>
> 1) Move bootcount_store() and bootcount_load() to architecture
> ? specific generic locations; this includes both the PowerPC and ARM
> ? implementations
>
> 2) Move arch/blackfin/lib/post.c to post/

> 3) Eliminate post_word_store() and post_word_load() and use ioread32()
> ? resp. iowrite32() (or equivalents) directly.
>

Thanks Wolfgang, it looks like I can cope with this task, of course if
no one has any objections.

Regards,
Michael

[U-Boot] [RFC] PPC: post_word_{load/store} - eliminate redundant code

[Wolfgang Denk]

Dear Michael Zaidman,

In message <i2o660c0f821004212341w74ed641aw94cd8738f1b9c93b@mail.gmail.com> you wrote:
> 
> > Looking at the code I wonder why we need post_word_store() and
> > post_word_load() functions at all. All implementations I have found
> > translate into a single ioread32() resp. iowrite32() call.
> >
> Probably, due to big and little endians that the PPC code should
> support. On other hand, AFAIK, no one is really using a little endian
> notation on PPC platforms in u-boot. I am not sure however, is this a
> good enough reason to omit such support? Or am I missing something?

We don;t indterface to any specific device register here, so the
actual byte order used when writing and reading the data does not
matter at all (as long as writer and reader agree). I see no reason
why we should not simply use the respective accessor macro for this
architecture.

> Thanks Wolfgang, it looks like I can cope with this task, of course if
> no one has any objections.

Thanks in advance.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
God grant me the senility to accept the things I cannot  change,  The
frustration  to  try to change things I cannot affect, and the wisdom
to tell the difference.

[U-Boot] [RFC] PPC: post_word_{load/store} - eliminate redundant code

[Michael Zaidman]

On Thu, Apr 22, 2010 at 12:03 PM, Wolfgang Denk <wd@denx.de> wrote:
>>
>> > Looking at the code I wonder why we need post_word_store() and
>> > post_word_load() functions at all. All implementations I have found
>> > translate into a single ioread32() resp. iowrite32() call.
>> >
>> Probably, due to big and little endians that the PPC code should
>> support. On other hand, AFAIK, no one is really using a little endian
>> notation on PPC platforms in u-boot. I am not sure however, is this a
>> good enough reason to omit such support? Or am I missing something?
>
> We don;t indterface to any specific device register here, so the
> actual byte order used when writing and reading the data does not
> matter at all (as long as writer and reader agree). I see no reason
> why we should not simply use the respective accessor macro for this
> architecture.
>
Ah, of course, that's correct.

Thanks,
Michael