Re: ppc405 enet changes (fwd)

Re: ppc405 enet changes (fwd)

[andrew may]

On Mon, Sep 03, 2001 at 08:29:15PM -0700, Tom Rini wrote:
> On Mon, Sep 03, 2001 at 03:14:03PM -0700, John Tyner wrote:
>
> > Is this patch going to be applied to the kernel? I haven't seen it in the
> > logs...
>
> iirc, there were some questions/comments about the driver which weren't
> answered.  Also, this adds some LINUX_VERSION(x,y,z) bits which are also
> bad since 405 doesn't quite work yet for everyone.  So if nothing else,
> these should be removed from the patch.

I think the main question on the reason to remove the statics was answered.
That is really the main change in the code, there are some tweaks to the
interupt handlers but not much. The other questions are more on future
changes to the driver.

The reason for the Linux Vers macros was to at least let us(and others) test the
changes on the last working kernel from mvista. You should be able to pull
them if you want to merge the code in, but I thought it would be best to
leave them in so the other code changes could be tested.

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

Re: ppc405 enet changes (fwd)

[andrew may]

On Tue, Sep 04, 2001 at 06:38:02PM -0400, Dan Malek wrote:
> John Tyner wrote:
> >
> > Is this patch going to be applied to the kernel? I haven't seen it in the
> > logs...
>
> I dunno.....from the code it doesn't look like you allow multiple
> device support (probe is only done once for one device).  If you
> have something that actually enables a new feature, add that too.
> Otherwise, it just looks like you added some additional indirection
> that doesn't add anything new and just adds overhead to the existing
> driver.  I'm also trying to determine why your patch seems to add
> things to the driver that should already be there.

Well we don't have the hardware to do multiple devices. I just know
that is possible in the future. I would prefer to go through the
pain of removing the static's now rather than later.

If you want to argue that we are just adding more redirection I will
be happy to send a patch to remove the entire struct ppc405_enet_private
to remove some more "useless" redirection. One of my biggest gripes
with the driver is that half of the stuff is kept as static and the
other half in the private sturct, for example ep_xmit_skb is in the
private stuct and ppc405_skb_rx is static. With things like this it
makes the entire driver hard to follow. I can't really think of an
explanation on why this is done in the driver. My only guess would
be that the person that did the driver has not done a linux network
driver before. There are other little clues throughout the driver
that point to this as well.

> We don't just blindly take stuff from people and patch it.  We
> actually have to believe it adds some benefits, we have to test
> it, and then it gets checked in.  This doesn't happen overnight.

No I don't expect the patch to go in quickly. I just wanted John
to start the process off. This patch does not do much to fix anything
but I would like to make any future changes off the new variable
names rather than what is in there now. Since this patch touches
so much stuff I think it would be better to keep any real changes
out of it until it gets tested.

So if you want to point to specific code that you don't like let
us know.

--
Thanks

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

Re: ppc405 enet changes (fwd)

[Dan Malek]

andrew may wrote:

> Well we don't have the hardware to do multiple devices. I just know
> that is possible in the future. I would prefer to go through the
> pain of removing the static's now rather than later.

Ummm...OK, I guess.


> No I don't expect the patch to go in quickly. I just wanted John
> to start the process off.

OK.  Once I actually get the 2_4_devel kernel booting and running with
some sense of reliability, I'll add the patch and give it a go.

> So if you want to point to specific code that you don't like let
> us know.

Yeah, all of the linux version stuff.  It is nice to get patches against
the current 2_4 or 2_4_devel that are known to be tested and working.
If you can't test it, don't expect a quick turn around because someone
else (like me :-) has to spend the time doing it.

Thanks.


	-- Dan

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

Re: ppc405 enet changes (fwd)

[Tom Rini]

On Tue, Sep 04, 2001 at 06:09:15PM -0700, andrew may wrote:
> > Yeah, all of the linux version stuff.  It is nice to get patches against
> > the current 2_4 or 2_4_devel that are known to be tested and working.
>
> Well the linux version stuff was our compromise for not being able to test
> with the new 2_4_devel kernel since we can't build it. I will be happy to
> see the ifdef's go away. I have to deal with them my other kenerl modules.

Well, I've heard rumors about it booting, and even to some degree working,
depending on HW.  When'd did you last try? :)  Do you get to the
Linux/PPC load: prompt still?

--
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

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

Re: ppc405 enet changes (fwd)

[acmay]

On Wed, Sep 05, 2001 at 07:12:59AM -0700, jpeters@mvista.com wrote:
....
> It is probably time for some more clean up in it.  Remember that the
> people who are responding to you have been working at this level
> for a long time.  They have the say (or at least some of it) because
> they have shown they know what they are doing.  It you get
> defensive and make statements like you did below you will drive
> people away.

You have noticed that you are defending yourself. You should also
notice that having to send a lot of emails to get a patch in would have
tendency to prevent people from even bothering to send patches.

The focus should be on the code, listing your work history is
not a good defense of the code that is there.

We can include a "Thank You for getting things working in bad
conditions" with the patch if it makes it go in any smoother.

If you read the lkml for any amount of time you will notice
that resends, follow-ups, insults, and bruised egos are all
part of the game of getting a patch in.

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

Re: ppc405 enet changes (fwd)

[acmay]

On Wed, Sep 05, 2001 at 07:12:59AM -0700, jpeters@mvista.com wrote:
> In addition, many of the embedded system boards around are still
> not what you would call blindingly fast.  The 405 chips I had would
> do a maximum of 200 Mhz and the additional software cache
> coherency code made it look a lot slower.  In a situation like
> this anything to reduce instruction count speeds up execution.
> Especially if it is code that runs at interrupt level.

So did you chose the vars that were to be static and the ones in
dev->priv for speed? If so it would be nice to see how you decided
which ones when where.

I would think that it would be best to keep all the vars as close
together in mem as possible, to keep them on the fewest number of
cache-lines. By splitting them between two sections I would think
it hurts more than it helps.


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