linuxppc_2_4_devel patch: 8xx FEC extensions

linuxppc_2_4_devel patch: 8xx FEC extensions

[Wolfgang Denk]

[-- Attachment #1: Type: text/plain, Size: 727 bytes --]


Hi,

this is a patch against linuxppc_2_4_devel BK Changeset 1.1197
(trini@kernel.crashing.org|ChangeSet|20021219180614|11718)

It makes the following modifications to the MPC8xx FEC driver:

- change PHY configuration from #define to kernel config mechanism
- add support for AMD79C874 PHY
- add multicast support
- add PACKETHOOK support


Best regards,

Wolfgang Denk

--
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd@denx.de
"Remember, Information is not knowledge;  Knowledge  is  not  Wisdom;
Wisdom is not truth; Truth is not beauty; Beauty is not love; Love is
not music; Music is the best."                          - Frank Zappa


[-- Attachment #2: 8xx-fec.patch --]
[-- Type: application/x-patch , Size: 32680 bytes --]

Re: linuxppc_2_4_devel patch: 8xx FEC extensions

[Tom Rini]

On Fri, Dec 20, 2002 at 02:05:07AM +0100, Wolfgang Denk wrote:

> It makes the following modifications to the MPC8xx FEC driver:
>
> - change PHY configuration from #define to kernel config mechanism
> - add support for AMD79C874 PHY

Both of these look OK, but can you please split this out into a seperate
patch which just does PHY configuration and then adds AMD79C874 support?

> - add multicast support

Sounds fine, but can you split this portion of the code from the rest of
the patch please?  Thanks.

> - add PACKETHOOK support

This was removed, intentionally back on March 22nd, 2002.  From what I
recall, it was decided this code was broken / unmaintained and should be
yanked.  Have you tested this particular section of code to verify it
still compiles and works as expected?

In sum: On hold for now.

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

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

Re: linuxppc_2_4_devel patch: 8xx FEC extensions

[Wolfgang Denk]

[-- Attachment #1: Type: text/plain, Size: 1760 bytes --]

In message <20021223151254.GC15397@opus.bloom.county> you wrote:
>
> > It makes the following modifications to the MPC8xx FEC driver:
> >
> > - change PHY configuration from #define to kernel config mechanism
> > - add support for AMD79C874 PHY
>
> Both of these look OK, but can you please split this out into a seperate
> patch which just does PHY configuration and then adds AMD79C874 support?

Of course I can.

PHY configuration ==> patch.1
add AMD79C874 support ==> patch.2

> > - add multicast support
>
> Sounds fine, but can you split this portion of the code from the rest of
> the patch please?  Thanks.

Of course I can.

cleanup & add multicast support ==> patch.3



But really, why do I have to do this?

I'm pretty sure you can read the patch as  is  so  you  are  able  to
either  complain about stuff you don't like or to accept these things
in one patch. Is this just some form  of  bullying,  or  is  there  a
rational reason behind this requirement?


> > - add PACKETHOOK support
>
> This was removed, intentionally back on March 22nd, 2002.  From what I
> recall, it was decided this code was broken / unmaintained and should be
> yanked.  Have you tested this particular section of code to verify it
> still compiles and works as expected?
>
> In sum: On hold for now.

If that was a decision I'm not in the  position  to  discuss  such  a
things. Just forget it.



Wolfgang Denk

--
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd@denx.de
The complexity of software is an essential property, not an  acciden-
tal  one. Hence, descriptions of a software entity that abstract away
its complexity often abstract away its essence.    - Fred Brooks, Jr.


[-- Attachment #2: 8xx-fec.patches.tar.gz --]
[-- Type: application/x-gzip , Size: 7964 bytes --]

Re: linuxppc_2_4_devel patch: 8xx FEC extensions

[Tom Rini]

On Sat, Dec 28, 2002 at 01:27:45AM +0100, Wolfgang Denk wrote:
> In message <20021223151254.GC15397@opus.bloom.county> you wrote:
> >
> > > It makes the following modifications to the MPC8xx FEC driver:
> > >
> > > - change PHY configuration from #define to kernel config mechanism
> > > - add support for AMD79C874 PHY
> >
> > Both of these look OK, but can you please split this out into a seperate
> > patch which just does PHY configuration and then adds AMD79C874 support?
>
> Of course I can.
>
> PHY configuration ==> patch.1
> add AMD79C874 support ==> patch.2

Thank you, I'm going to take out the part where it defines
CONFIG_SCCSX_ENET=n when none are selected, as things like that
should never need to be done (and if needed, there's a bug there) and
commit those.

> > > - add multicast support
> >
> > Sounds fine, but can you split this portion of the code from the rest of
> > the patch please?  Thanks.
>
> Of course I can.
>
> cleanup & add multicast support ==> patch.3

Now I have to question some of the 'cleanup'.  A lot of it is just
whitespace / braces.  IMHO, and what I did with the OCP drivers, if
scripts/Lindent is 'happy', then that's good enough.  Also, is there any
reason to go from 'volatile uint *s = &(...->...);' to 'uint s =
...->...;' ?  Maybe it's too early in the morning for me, but why
couldn't it be just 'uint *s', if that volatile isn't needed?
This is on hold for now, and for future patches please keep cleanup
seperate from functionality as much as possible.

> But really, why do I have to do this?

As you are the person submitting these patches, you should have the most
knowledge of what's going on and why.  I could have split out patches 1
and 2 and probably dropped the FEC_PACKETHOOK stuff, but I would have
probably dropped some of the 'cleanup' portion, it would have taken me
longer, and while it's not so obvious with this set of patches, I don't
always have access to the same HW you do, so if you can send out tested
patches which I don't change, there's a good chance that things will
still work :)

> I'm pretty sure you can read the patch as  is  so  you  are  able  to
> either  complain about stuff you don't like or to accept these things
> in one patch. Is this just some form  of  bullying,  or  is  there  a
> rational reason behind this requirement?

When things go upstream, we really have to split things up into logical
chunks.  Furthermore, if there happens to be a problem with some portion
of a patch which does N things, all of those N things don't get in until
the whole patch is 'good'.

I don't like to think of it as 'bullying', but more 'passing the buck'.
Linus / Marcelo don't like big patches from anyone, and now that they
both use BK, it is nice to have a good history.  And I try and pass
this along to everyone, including myself.

> > > - add PACKETHOOK support
> >
> > This was removed, intentionally back on March 22nd, 2002.  From what I
> > recall, it was decided this code was broken / unmaintained and should be
> > yanked.  Have you tested this particular section of code to verify it
> > still compiles and works as expected?
> >
> > In sum: On hold for now.
>
> If that was a decision I'm not in the  position  to  discuss  such  a
> things. Just forget it.

It was removed since no one wanted to maintain it and it didn't work as
is.  If you would like to maintain this bit of code it can come back (as
it wasn't the concept which was the problem, it was the code being
non-functional).

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

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

Re: linuxppc_2_4_devel patch: 8xx FEC extensions

[Wolfgang Denk]

In message <20021230152945.GD5564@opus.bloom.county> you wrote:
>
> scripts/Lindent is 'happy', then that's good enough.  Also, is there any
> reason to go from 'volatile uint *s = &(...->...);' to 'uint s =
> ...->...;' ?  Maybe it's too early in the morning for me, but why
> couldn't it be just 'uint *s', if that volatile isn't needed?

Yes, there is a reason. The old code will access the device  register
several  times,  and  create  intermediate  states  that  may  not be
intended, or even harmful. For example:

	static void mii_parse_sr( ... )
	{
		...
		volatile uint *s = &(fep->phy_status);

		*s &= ~(PHY_STAT_LINK | PHY_STAT_FAULT | PHY_STAT_ANC);

		if (mii_reg & 0x0004)
			*s |= PHY_STAT_LINK;
		if (mii_reg & 0x0010)
			*s |= PHY_STAT_FAULT;
		if (mii_reg & 0x0020)
			*s |= PHY_STAT_ANC;
		...

Assume all the relevant bits in mii_reg are set, then we first  clear
all  the  LINK,  FAULT,  and ANC bits in phy_status, just to tun them
back on step by step.

I cannot really prove it, but I think I have seen at least one  board
where this cause / contributed to some problems with the PHY.

> This is on hold for now, and for future patches please keep cleanup
> seperate from functionality as much as possible.

This _is_ functionality.

Best regards,

Wolfgang Denk

--
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd@denx.de
"All my life I wanted to be someone; I guess I should have been  more
specific."                                              - Jane Wagner

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

Re: linuxppc_2_4_devel patch: 8xx FEC extensions

[Tom Rini]

On Mon, Dec 30, 2002 at 05:26:16PM +0100, Wolfgang Denk wrote:
> In message <20021230152945.GD5564@opus.bloom.county> you wrote:
> >
> > scripts/Lindent is 'happy', then that's good enough.  Also, is there any
> > reason to go from 'volatile uint *s = &(...->...);' to 'uint s =
> > ...->...;' ?  Maybe it's too early in the morning for me, but why
> > couldn't it be just 'uint *s', if that volatile isn't needed?
[snip a good explanation]

Sounds reasonable.

> > This is on hold for now, and for future patches please keep cleanup
> > seperate from functionality as much as possible.
>
> This _is_ functionality.

It is not part of the multicast functionality, it's either an unrelated
but none the less important bug fix, or like other parts of the patch
just 'cleanup'.  So please in the future submit them as a seperate
logical chunk, since that's how I have to submit them.

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

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

Re: linuxppc_2_4_devel patch: 8xx FEC extensions

[Tom Rini]

On Tue, Dec 31, 2002 at 04:58:34PM +0100, Wolfgang Denk wrote:
> In message <20021231155241.GA12063@opus.bloom.county> you wrote:
> >
> > But I'll hold off on this section of the patch until everyone is happy.
>
> How do we _get_ happy, then?

Well, Dan thinks that dropping the 'violatile' here will do bad things,
you believe it will fix a problem, and I think it's fishy that a
volatile is needed here at all.  If you want to get just the multicast
stuff in, a seperate patch of just the multicast code would expidite
that.  For this particular problem I'd like you and Dan to agree on
something.

And BTW, the other patches haven't been committed yet only because my
machine with all of my bk trees on it is still being reinstalled.  They
will be as soon as I get the machine useable again. :)

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

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

Re: linuxppc_2_4_devel patch: 8xx FEC extensions

[Wolfgang Denk]

In message <20021231161305.GC12063@opus.bloom.county> you wrote:
>
> > How do we _get_ happy, then?
>
> Well, Dan thinks that dropping the 'violatile' here will do bad things,

I don't see where a "volatile" gets dropped in any really significant
way.

For example, the old code has:

	volatile uint *s = &(fep->phy_status);

	*s &= ~(PHY_STAT_LINK | PHY_STAT_FAULT | PHY_STAT_ANC);
(1)
	if (mii_reg & 0x0004)
		*s |= PHY_STAT_LINK;
(2)
	...

The new code has:

	uint s = fep->phy_status;

	s &= ~(PHY_STAT_LINK | PHY_STAT_FAULT | PHY_STAT_ANC);

	if (mii_reg & 0x0004)
		s |= PHY_STAT_LINK;
(3)
	...

For (1), I expect to see code like "lwz,  and[i],  stw";  for  (2)  I
expect  "lwz,  or[i], stw"; (3) will probably look like "lwz, and[i],
or[i], stw". If there is  any  problem  with  asynchronously  running
threads  than  we  have a race condition in the gap between "lwz" and
"stw" anyway. The only significant difference between the old and the
new code is that the old code has many short gaps while our patch has
only one slightly longer one.

> you believe it will fix a problem, and I think it's fishy that a
> volatile is needed here at all.  If you want to get just the multicast

I did not see any problems with the patched code yet. Actually, I see
less problems with the patches version than with the original one.

> that.  For this particular problem I'd like you and Dan to agree on
> something.

Dan, what do you think under which conditions the patches  code  will
show a problem that was not present with the old code?


Best regards, and a Happy New Year!

Wolfgang Denk

--
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-4596-87  Fax: (+49)-8142-4596-88  Email: wd@denx.de
There are two ways to write error-free programs. Only the  third  one
works.

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

Re: linuxppc_2_4_devel patch: 8xx FEC extensions

[Tom Rini]

On Thu, Jan 02, 2003 at 01:11:50PM -0500, Dan Malek wrote:
> Wolfgang Denk wrote:
>
>
> >I don't see where a "volatile" gets dropped in any really significant
> >way.
>
> No, it doesn't.  The function logically does exactly the same thing,
> so I don't understand why these changes were necessary. :-)
>
> The original code simply accessed the 'phy_status' in the data structure
> as a volatile object.  The modification from Wolfgang makes any data
> structure access volatile, and then updates the 'phy_status' only once
> at the end.
>
> If it makes something work better for Wolfgang, that's fine :-).  To me,
> it seems to be covering up some other timing problem since the only
> thing different is how many times a particular memory location is accessed.

Would the patch, along with a comment about this potentially covering up
HW timing issues (since this seems to have 'fixed' a problem on a
certain HW config) be OK with everyone?

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

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

Re: linuxppc_2_4_devel patch: 8xx FEC extensions

[Tom Rini]

On Thu, Jan 02, 2003 at 01:28:41PM -0500, Dan Malek wrote:
> Tom Rini wrote:
>
> >Would the patch, along with a comment about this potentially covering up
> >HW timing issues (since this seems to have 'fixed' a problem on a
> >certain HW config) be OK with everyone?
>
> Go for it.

Okay, it's all pushed out now.

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

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

Re: linuxppc_2_4_devel patch: 8xx FEC extensions

[Dan Malek]

Tom Rini wrote:

> Erm.  That sounds bad.  So the values can change under the functions
> feet?  Even if the values tested for are only tested by this function,
> that still sounds like a bad idea.

If you look at the code, you will notice that the object that is declared
volatile is part of a shared driver data structure.  It isn't a hardware
register.  The reason it was originally declared volatile (and is a valid
programming method) was the MII interrupt would set this value, and later
a normal thread of execution would read this value in another function.
These functions can, which is also a valid compiler optimization technique,
cache these global values in a processor register.  In order for the normal
thread of execution to see the update from the interrupt handler, you have
to do something to force it to use the shared data structure, not the
optimization.  It's no big deal, and quite a common multithreaded programming
practice.

I don't understand Wolfgang's original "multiple access" comment since it's
only accessing a data structure in memory, unless there was some weird software
timing that occurred here due to the additional memory accesses.  There shouldn't
be any race conditions even though the status is updated in multiple C program
statements.

Thanks.


	-- Dan


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

Re: linuxppc_2_4_devel patch: 8xx FEC extensions

[Paul Mackerras]

Dan Malek writes:

> It needs to be volatile as a means of synchronization between the interrupt
> handler that will set the flags and a normal thread of execution that will
> test them.  The normal thread would cache a value in a register and never
> look at any updates that occurred as part of an interrupt.

If we have a variable which is being updated both by the mainline and
an interrupt handler, then we should be using some explicit
synchronization (e.g. disabling interrupts around the region where we
do the update) or else we should be using atomic operations.  Just
declaring it volatile is *not* enough, because, clearly, something
like "a &= ~1;" in the mainline will do a load and a store and that
will create a window where an update done by an interrupt handler will
get ignored.

In general we should have a spinlock and do spin_lock_irqsave /
spin_unlock_irqrestore around the update.  If the code will never be
used on an SMP system then we can reduce that to save_flags/cli and
restore_flags.

Note that __cli et al. include a gcc memory barrier precisely because
they are often used to protect accesses to variables that are accessed
both by mainline and interrupt-level code.

Paul.

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

Re: linuxppc_2_4_devel patch: 8xx FEC extensions

[Dan Malek]

Paul Mackerras wrote:


> If we have a variable which is being updated both by the mainline and
> an interrupt handler,...

In this case, it wasn't being updated by both.  It was updated in
an interrupt handler, then a mainline function would test the value later.
The mainline function had the optimization habit of caching the value
in a processor register since it didn't know it was being changed outside
of the scope of the function.  The volatile declaration works just fine
in this case.

Thanks.


	-- Dan


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