Re: linuxppc_2_4_devel patch: 8xx FEC extensions

In message [off-list ref] 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/

On Mon, Dec 30, 2002 at 05:26:16PM +0100, Wolfgang Denk wrote:

In message [off-list ref] you wrote:

quoted

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.

quoted

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/