From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from rv-out-0506.google.com (rv-out-0506.google.com [209.85.198.226]) by ozlabs.org (Postfix) with ESMTP id 0757FDDF81 for ; Fri, 8 May 2009 12:02:55 +1000 (EST) Received: by rv-out-0506.google.com with SMTP id f9so187612rvb.1 for ; Thu, 07 May 2009 19:02:53 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1241640919-4650-1-git-send-email-wd@denx.de> <1241640919-4650-3-git-send-email-wd@denx.de> <20090506220119.7322783420E8@gemini.denx.de> <20090506224134.83F1583420E8@gemini.denx.de> Date: Thu, 7 May 2009 20:02:53 -0600 Message-ID: <4b73d43f0905071902y3b51d36ct2a04c560b5acdfb9@mail.gmail.com> Subject: Re: [PATCH 02/12] fs_enet: Add MPC5121 FEC support. From: John Rigby To: linuxppc , Wolfgang Denk Content-Type: multipart/alternative; boundary=000e0cd32f98ce7eb804695d06d5 Cc: netdev@vger.kernel.org, Piotr Ziecik , Detlev Zundel List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , --000e0cd32f98ce7eb804695d06d5 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Wolfgang, Welcome to my world and why I gave up on this months ago. Everyone else, One thing to consider here is a rewrite with the goal of a new improved fec driver that would work on both 5121 and the various mx platforms that also have this same fec core. Also don't forget that the register map is the same on 512x, mx and coldfire platforms but not on the other ppc platforms so if you want to one binary to rule them all you will need to have an offest table or some such. John On Thu, May 7, 2009 at 8:09 AM, Grant Likely wrote: > On Wed, May 6, 2009 at 4:41 PM, Wolfgang Denk wrote: > > Dear Grant, > > > > In message > you wrote: > >> > >> > Agreed that it's ugly, but duplicatio9ng the code would have been even > >> > worse. I don't think that it has multiplatform - at least not as long > >> > as you don't ask for one image that runs on 83xx and on 512x. > >> > >> Actually, I *am* asking for one image that runs on 83xx, 52xx and > >> 521x. I already can and do build and test a single image which boots > >> on all my 52xx boards, on my 8349 board, and on my G4 Mac. > > > > He. I was afraid you'd say that ;-) > > > > In this case I need a helping hand as I can't figure out how to make > > this work. Any suggestions? > > Hmmm, it is rather ugly because the layout of fec_t is so different. > Easiest solution would be to duplicate the driver in its entirety, but > as you say it results in a lot of duplicate code. It might be the > lesser of many weevils though. > > Second easiest would be to factor out all the common code in the > driver into a separate .c file that gets included by a 'wrapper' .c > file for each config which first includes the correct definition of > fec_t. This approach solves the duplicate code problem, but it also > fell out of the ugly tree and hit every branch on the way down. > > ie: the wrappers would look something like this: > > fs_enet_cpm1.c: > #include > #include "fs_enet_main.c" > > fs_enet_cpm2.c: > #include > #include "fs_enet_main.c" > > fs_enet_512x.c: > #include > #include "fs_enet_main.c" > > And then the makefile would be something along the lines of: > obj-${CONFIG_FS_ENET_CPM1_ += fs_enet_cpm1.o > obj-${CONFIG_FS_ENET_CPM2_ += fs_enet_cpm2.o > obj-${CONFIG_FS_ENET_MPC512x_ += fs_enet_512x.o > > A brief look at the driver suggests that access to the fec_t structure > is restricted to the fec-mii.c and mac-fec.c files. It might be > appropriate to duplicate just those files and do some form of > fs_enet_ops to select between them. > > While on the topic, it looks to me like the driver could really use > some refactoring love. Having multiple definitions of "fec_t" is > confusing and potentially lead to hard to find bugs if the wrong > header gets included by anyone. I'd like to see all the fec specific > stuff in arch/powerpc/include/asm moved into drivers/net/fs_enet and > renamed to reflect the hardware it is associate with. Stuff like > "fec_t" is far to generic, not to mention that typedefs are > discouraged now. > > Regardless, I feel your pain. It is not a pretty situation. > > g. > > -- > Grant Likely, B.Sc., P.Eng. > Secret Lab Technologies Ltd. > --000e0cd32f98ce7eb804695d06d5 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Wolfgang,

Welcome to my world and why I gave up on this months ago.<= br>
Everyone else,

One thing to consider here is a rewrite with t= he goal of a new improved fec driver that would work on both 5121 and the v= arious mx platforms that also have this same fec core.

Also don't forget that the register map is the same on 512x, mx and= coldfire platforms but not on the other ppc platforms so if you want to on= e binary to rule them all you will need to have an offest table or some suc= h.

John

On Thu, May 7, 2009 at 8:09 AM, = Grant Likely <grant.likely@secretlab.ca> wrote:
On Wed, May 6, 2009 at 4:41 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Grant,
>
> In message <fa686aa40905061529u11b231afle3b5bb10a2334ad0@mail.g= mail.com> you wrote:
>>
>> > Agreed that it's ugly, but duplicatio9ng the code would h= ave been even
>> > worse. I don't think that it has multiplatform - at least= not as long
>> > as you don't ask for one image that runs on 83xx and on 5= 12x.
>>
>> Actually, I *am* asking for one image that runs on 83xx, 52xx and<= br> >> 521x. =A0I already can and do build and test a single image which = boots
>> on all my 52xx boards, on my 8349 board, and on my G4 Mac.
>
> He. I was afraid you'd say that ;-)
>
> In this case I need a helping hand as I can't figure out how to ma= ke
> this work. Any suggestions?

Hmmm, it is rather ugly because the layout of fec_t is so different.<= br> Easiest solution would be to duplicate the driver in its entirety, but
as you say it results in a lot of duplicate code. =A0It might be the
lesser of many weevils though.

Second easiest would be to factor out all the common code in the
driver into a separate .c file that gets included by a 'wrapper' .c=
file for each config which first includes the correct definition of
fec_t. =A0This approach solves the duplicate code problem, but it also
fell out of the ugly tree and hit every branch on the way down.

ie: the wrappers would look something like this:

fs_enet_cpm1.c:
#include <asm/cpm1.h>
#include "fs_enet_main.c"

fs_enet_cpm2.c:
#include <asm/cpm2.h>
#include "fs_enet_main.c"

fs_enet_512x.c:
#include <asm/mpc512x.h>
#include "fs_enet_main.c"

And then the makefile would be something along the lines of:
obj-${CONFIG_FS_ENET_CPM1_ +=3D fs_enet_cpm1.o
obj-${CONFIG_FS_ENET_CPM2_ +=3D fs_enet_cpm2.o
obj-${CONFIG_FS_ENET_MPC512x_ +=3D fs_enet_512x.o

A brief look at the driver suggests that access to the fec_t structure
is restricted to the fec-mii.c and mac-fec.c files. =A0It might be
appropriate to duplicate just those files and do some form of
fs_enet_ops to select between them.

While on the topic, it looks to me like the driver could really use
some refactoring love. =A0Having multiple definitions of "fec_t" = is
confusing and potentially lead to hard to find bugs if the wrong
header gets included by anyone. =A0I'd like to see all the fec specific=
stuff in arch/powerpc/include/asm moved into drivers/net/fs_enet and
renamed to reflect the hardware it is associate with. =A0Stuff like
"fec_t" is far to generic, not to mention that typedefs are
discouraged now.

Regardless, I feel your pain. =A0It is not a pretty situation.

g.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

--000e0cd32f98ce7eb804695d06d5--