[PATCH] Adding support for LXT971/2 PHYs

[PATCH] Adding support for LXT971/2 PHYs

[Graham Stoney]

Hi Dan & friends,

The following patch to the Fast Ethernet Controller driver adds support for
the LXT971 and LXT972 PHYs, which have a slightly diffent programming
interface to the LXT970, Thank you Level One. The LXT model number is
autodetected and we've tested the result on an LXT972.

I've also taken the liberty of moving the PHY interrupt definition out so it
can be placed in each board's 'board.h' file, allowing me to replace one of
the #ifdef CONFIG_FADS with a more generic PHY_INTERRUPT definition. This
makes it easier to add new board types which use different interrupt
assignments without a maze of #ifdefs and I think is a good general direction
to take in the 8xx drivers.

I'm pretty sure that the "This version of the driver is specific to the
FADS..." comment in the header is old and incorrect, but I wasn't sure what
it should say about the FADS, so I've left it alone for now.

Comments welcome!

Index: include/asm-ppc/fads.h
===================================================================
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 fads.h
--- include/asm-ppc/fads.h	1999/12/30 06:00:31	1.1.1.1
+++ include/asm-ppc/fads.h	2000/05/03 04:34:14
@@ -36,4 +36,6 @@
 #define PCMCIA_MEM_ADDR		((uint)0x04000000)
 #define PCMCIA_MEM_SIZE		((uint)(64 * 1024))

+#define PHY_INTERRUPT	SIU_IRQ2
+
 #endif
Index: arch/ppc/8xx_io/fec.c
===================================================================
retrieving revision 1.1.1.2
retrieving revision 1.9
diff -u -r1.1.1.2 -r1.9
--- arch/ppc/8xx_io/fec.c	2000/03/10 01:11:06	1.1.1.2
+++ arch/ppc/8xx_io/fec.c	2000/05/04 06:48:35	1.9
@@ -2,6 +2,8 @@
  * Fast Ethernet Controller (FEC) driver for Motorola MPC8xx.
  * Copyright (c) 1997 Dan Malek (dmalek@jlc.net)
  *
+ * Includes support for the following PHYs: QS6612, LXT970, LXT971/2.
+ *
  * This version of the driver is specific to the FADS implementation,
  * since the board contains control registers external to the processor
  * for the control of the LevelOne LXT970 transceiver.  The MPC860T manual
@@ -16,6 +18,7 @@
  * small packets.
  *
  */
+#include <linux/config.h>
 #include <linux/kernel.h>
 #include <linux/sched.h>
 #include <linux/string.h>
@@ -139,8 +142,23 @@

 static	int	phyaddr;
 static	uint	phytype;
+static  int	lxt970;    /* == 1 if model LXT970, == 0 otherwise. */
+
+/* Level One, in their infinite wisdom, have changed the locations
+   of some of the important bits in the registers of the LXT9xx series.
+   Thanks guys. */
+
+#define CHIP_STATUS_REG		(lxt970 ? 20 : 17)
+#define CHIP_STATUS_SPEED	(lxt970 ? 0x0800 : 0x4000)
+#define CHIP_STATUS_DUPLEX	(lxt970 ? 0x1000 : 0x0200)
+#define INTERRUPT_STATUS_REG	(lxt970 ? 18 : 19)
+#define INTERRUPT_PENDING	(lxt970 ? 0x8000 : 0x0004)
+#define INTERRUPT_ENABLE_REG	(lxt970 ? 17 : 18)
+#define INTERRUPT_ENABLE_VAL	(lxt970 ? 0x0002 : 0x0032)
+#define CONFIG_REG		(lxt970 ? 19 : 16)

 static int	mii_queue(int request, void (*func)(uint, struct device *));
+static void	mii_startup_cmds(void);

 /* Make MII read/write commands for the FEC.
 */
@@ -606,17 +624,17 @@
 			printk(",auto complete");
 		printk("\n");
 	}
-	if (((mii_reg >> 18) & 0x1f) == 0x14) {
+	if (((mii_reg >> 18) & 0x1f) == CHIP_STATUS_REG) {
 		/* Extended chip status register.
 		*/
 		prev_duplex = full_duplex;
 		printk("fec: ");
-		if (mii_reg & 0x0800)
+		if (mii_reg & CHIP_STATUS_SPEED)
 			printk("100 Mbps");
 		else
 			printk("10 Mbps");

-		if (mii_reg & 0x1000) {
+		if (mii_reg & CHIP_STATUS_DUPLEX) {
 			printk(", Full-Duplex\n");
 			full_duplex = 1;
 		}
@@ -663,14 +681,19 @@
 	phytype <<= 16;
 	phytype |= (mii_reg & 0xffff);
 	printk("fec: Phy @ 0x%x, type 0x%08x\n", phyno, phytype);
+
+        /* Strictly speaking we should test the whole of phytype,
+         * but there is confusion about exactly what is in the
+         * OUI part of the Identification registers.
+	 */
+        lxt970 = ((mii_reg & 0xfffc) == 0);
+
 	mii_startup_cmds();
 }

 static void
 mii_discover_phy(uint mii_reg, struct device *dev)
 {
-	volatile uint	prev_duplex;
-
 	if (phyno < 32) {
 		if ((phytype = (mii_reg & 0xffff)) != 0xffff) {
 			phyaddr = phyno;
@@ -729,20 +752,20 @@
 	/* Read status registers to clear any pending interrupt.
 	*/
 	mii_queue(mk_mii_read(1), mii_status);
-	mii_queue(mk_mii_read(18), mii_status);
+	mii_queue(mk_mii_read(INTERRUPT_STATUS_REG), mii_status);

 	/* Read extended chip status register.
 	*/
-	mii_queue(mk_mii_read(0x14), mii_status);
+	mii_queue(mk_mii_read(CHIP_STATUS_REG), mii_status);

 	/* Set default operation of 100-TX....for some reason
 	 * some of these bits are set on power up, which is wrong.
 	 */
-	mii_queue(mk_mii_write(0x13, 0), NULL);
+	mii_queue(mk_mii_write(CONFIG_REG, 0), NULL);

 	/* Enable Link status change interrupts.
 	*/
-	mii_queue(mk_mii_write(0x11, 0x0002), NULL);
+	mii_queue(mk_mii_write(INTERRUPT_ENABLE_REG, INTERRUPT_ENABLE_VAL), NULL);

 	/* Don't advertize Full duplex.
 	mii_queue(mk_mii_write(0x04, 0x0021), NULL);
@@ -753,7 +776,7 @@

 /* This supports the mii_link interrupt below.
  * We should get called three times.  Once for register 1, once for
- * register 18, and once for register 20.
+ * register INTERRUPT_STATUS_REG, and once for register CHIP_STATUS_REG.
  */
 static	uint mii_saved_reg1;

@@ -769,16 +792,16 @@
 		mii_saved_reg1 = mii_reg;
 		return;
 	}
-	if (((mii_reg >> 18) & 0x1f) == 18) {
+	if (((mii_reg >> 18) & 0x1f) == INTERRUPT_STATUS_REG) {
 		/* Not much here, but has to be read to clear the
 		 * interrupt condition.
 		 */
-		if ((mii_reg & 0x8000) == 0)
+		if ((mii_reg & INTERRUPT_PENDING) == 0)
 			printk("fec: re-link and no IRQ?\n");
-		if ((mii_reg & 0x4000) == 0)
+		if (lxt970 && ((mii_reg & 0x4000) == 0))
 			printk("fec: no PHY power?\n");
 	}
-	if (((mii_reg >> 18) & 0x1f) == 20) {
+	if (((mii_reg >> 18) & 0x1f) == CHIP_STATUS_REG) {
 		/* Extended chip status register.
 		 * OK, now we have it all, so figure out what is going on.
 		 */
@@ -794,12 +817,12 @@
 		if (mii_saved_reg1 & 0x0020)
 			printk(", auto complete");

-		if (mii_reg & 0x0800)
+		if (mii_reg & CHIP_STATUS_SPEED)
 			printk(", 100 Mbps");
 		else
 			printk(", 10 Mbps");

-		if (mii_reg & 0x1000) {
+		if (mii_reg & CHIP_STATUS_DUPLEX) {
 			printk(", Full-Duplex\n");
 			full_duplex = 1;
 		}
@@ -871,11 +894,11 @@
 	fep = (struct fec_enet_private *)dev->priv;
 	ep = &(((immap_t *)IMAP_ADDR)->im_cpm.cp_fec);

-	/* We need to sequentially read registers 1 and 18 to clear
+	/* We need to sequentially read registers 1 and INTERRUPT_STATUS_REG to clear
 	 * the interrupt.  We don't need to do that here because this
 	 * is an edge triggered interrupt that has already been acknowledged
 	 * by the top level handler.  We also read the extended status
-	 * register 20.  We just queue the commands and let them happen
+	 * register CHIP_STATUS_REG.  We just queue the commands and let them happen
 	 * as part of the "normal" processing.
 	 */
 #ifdef CONFIG_RPXCLASSIC
@@ -884,8 +907,8 @@
 	mii_queue(mk_mii_read(31), mii_relink);
 #else
 	mii_queue(mk_mii_read(1), mii_relink);
-	mii_queue(mk_mii_read(18), mii_relink);
-	mii_queue(mk_mii_read(20), mii_relink);
+	mii_queue(mk_mii_read(INTERRUPT_STATUS_REG), mii_relink);
+	mii_queue(mk_mii_read(CHIP_STATUS_REG), mii_relink);
 #endif

 }
@@ -1111,9 +1134,8 @@
 	immap->im_ioport.iop_pcso &= ~0x0001;
 	immap->im_ioport.iop_pcint |= 0x0001;
 	cpm_install_handler(CPMVEC_PIO_PC15, mii_link_interrupt, dev);
-#endif
-#ifdef CONFIG_FADS
-	if (request_8xxirq(SIU_IRQ2, mii_link_interrupt, 0, "mii", dev) != 0)
+#elif defined (PHY_INTERRUPT)
+	if (request_8xxirq(PHY_INTERRUPT, mii_link_interrupt, 0, "mii", dev) != 0)
 		panic("Could not allocate MII IRQ!");
 #endif

@@ -1154,7 +1176,7 @@
 	volatile	cbd_t	*bdp;
 	volatile	immap_t	*immap;
 	volatile	fec_t	*fecp;
-	extern		uint	_get_IMMR();
+	extern		uint	_get_IMMR(void);

 	immap = (immap_t *)IMAP_ADDR;	/* pointer to internal registers */

--
Graham Stoney
Principal Hardware/Software Engineer
Canon Information Systems Research Australia
Ph: +61 2 9805 2909  Fax: +61 2 9805 2929

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

Re: [PATCH] Adding support for LXT971/2 PHYs

[Dan Malek]

Graham Stoney wrote:
>
> Hi Dan & friends,
>
> The following patch to the Fast Ethernet Controller driver adds support for
> the LXT971 and LXT972 PHYs,

Oooops, you almost missed out :-).  I have received a couple of these
lately, all implemented slightly differently.  I am going to shake-n-bake
all of these and see what falls out of the bag.......

I'll put it into the 2.3.99 tree first, then into one of the 2.2.xx
releases.



> I've also taken the liberty of moving the PHY interrupt definition

Well, everyone seems to want this someplace other than where it is,
but no one wants it in the same place.

> ........, allowing me to replace one of
> the #ifdef CONFIG_FADS

I think the #ifdefs for 8xx are soon going to be gone, replaced with
a significantly larger board information structure.  Things like this
will go there.  The #define IMMR is going to disappear, replaced with
a memory pointer, among other things.



> I'm pretty sure that the "This version of the driver is specific to the
> FADS..." comment in the header is old and incorrect,


That's not the only one :-).  When I see these, I try to update them,
but all too often I am worried about "line 1503" and don't pay attention
to the comments.



	-- Dan

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

Re: [PATCH] Adding support for LXT971/2 PHYs

[Graham Stoney]

Hi Dan,

Dan Malek writes:
> Oooops, you almost missed out :-).  I have received a couple of these
> lately, all implemented slightly differently.  I am going to shake-n-bake
> all of these and see what falls out of the bag.......

Hmmm. Ours is the first patch for this I've seen posted to the mailing list,
unless there's another list somewhere I'm not aware of. We don't want to be
re-inventing the wheel; we'd rather contribute something useful that other
people aren't also doing. It would help if people posted stuff like this on
the list, so we know what's coming in the pipeline.

> I'll put it into the 2.3.99 tree first, then into one of the 2.2.xx
> releases.

OK. How do those of us without BitKeeper access grab your 2.3.99 tree?

> I think the #ifdefs for 8xx are soon going to be gone, replaced with
> a significantly larger board information structure.  Things like this
> will go there.  The #define IMMR is going to disappear, replaced with
> a memory pointer, among other things.

Are you sure we want to do that? I kind of like being able to define a
minimal board info structure, where the board info structure only contains
stuff that can potentially vary between models of the same board. My ideal
solution to the problem of supporting multiple board types is to move the
magic numbers like the PHY interrupt pin and the PxPAR/PxDIR/etc values into
each board-specific header file, giving definitions like:

    #define PAPAR_VALUE	0x....
    #define PADIR_VALUE	0x....

Doing this exhaustively (for all boards) would allow all the '#ifdef
CONFIG_boardname's to be replaced with generic macro usages which allow new
board types to be added by simply writing the new board.h file, and adding the
#include for it in include/asm-ppc/mpc8xx.h. The other advantage over
complicating the board info structure is that there's no runtime code size
overhead this way. Keep it simple and all that. The only problem is that the
support for existing boards only goes part way towards doing this at present.

We took this approach with our new custom board, and haven't had to add a
single #ifdef CONFIG_ourboardname outside the one which chooses our
board-specific header in mpc8xx.h. Of course we'll go with the flow, but I
think we can do a better job of multiple board support without a complex board
info structure. On the other hand, the mimimal-size approach runs counter to
the potential goal of having a single kernel config which runs on anything.
I personally don't think that's desirable, but it may be another motivation
behind the put-it-all-in-board-info approach.

I can post the relevant sections of our board.h file if an example help
illustrate this better.

Regards,
Graham
--
Graham Stoney
Principal Hardware/Software Engineer
Canon Information Systems Research Australia
Ph: +61 2 9805 2909  Fax: +61 2 9805 2929

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

Re: [PATCH] Adding support for LXT971/2 PHYs

[Dan Malek]

Graham Stoney wrote:

> Hmmm. Ours is the first patch for this I've seen posted to the mailing list,

There is lots of stuff that comes straight to me for merging into
sources.......


> OK. How do those of us without BitKeeper access grab your 2.3.99 tree?

If you want to do this kind of stuff, I would suggest anonymous
BitKeeper pulls to keep you up to date.  It shows up in kernel.org
rather quickly......

In this specific case, it wouldn't matter.  For some reason, this
must have been Fast Ethernet week, as I received several patches
over the last few days :-).


> Are you sure we want to do that?

I don't have a solution yet, just several completely different approaches
from people.  Something has to give, because I agree it is getting too
complicated.  It's OK for me, because I have been living it for years
and know what it all means, but it doesn't help someone looking at it
for the first time.


> .......... My ideal
> solution to the problem of supporting multiple board types is to move the
> magic numbers like the PHY interrupt pin and the PxPAR/PxDIR/etc values into
> each board-specific header file,


I understand your point, but the other side of the argument is when you
don't keep things like this together, people don't realize how many
examples of how to do it exist.  They also don't realize how their
change may affect someone else.


> complicating the board info structure is that there's no runtime code size
> overhead this way.

Yes, but this is only initialization, not run time overhead.

> We took this approach with our new custom board, and haven't had to add a
> single #ifdef CONFIG_ourboardname outside the one which chooses our
> board-specific header in mpc8xx.h.

I do that with lots of custom boards.  Most are simple derivative of
what is already there.  In fact, some have even choose the same I/O
pins for Ethernet (there aren't many options, and most have been used :-),
so we don't even need to add anything but another conditional on an
#ifdef.

Anyway, this has kind of gone down a rat hole...I have a bunch of
code from several people I am trying to sort out right now.  I want
to use it all, and I am choosing the best from all of it.  Don't
worry, I'll use some of yours too :-).  It's just not as easy as
getting a patch and checking it in.


	-- Dan

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

Better board support (was Re: [PATCH] Adding support for LXT971/2 PHYs)

[Graham Stoney]

Hi again,

Dan Malek writes:
> There is lots of stuff that comes straight to me for merging into
> sources.......

I know it's beyond your control; My plea was for other developers reading this
to cc the mailing list when they send you patches, so we all know what's going
on.

> > .......... My ideal solution to the problem of supporting multiple board
> > types is to move the magic numbers like the PHY interrupt pin and the
> > PxPAR/PxDIR/etc values into each board-specific header file,
>
> I understand your point, but the other side of the argument is when you
> don't keep things like this together, people don't realize how many
> examples of how to do it exist.  They also don't realize how their
> change may affect someone else.

Not sure I'm with you here, but I think we need to have a simple single
solution to multiple board support, and to use it universally. If we replace
all the board-specific #ifdefs with feature-specific ones defined in the board
specific header files, everything related to a specific board is all kept
together. I don't think I'm proposing anything new here, just a more complete
use of the current scheme where each board type has its own header defining
the features present on that board. This is also the approach 8xxROM/PPCBOOT
uses, and it would be nice to keep them all consistent.

Another example under the scheme would be to replace most of the usages of
"#ifdef CONFIG_RPXCLASSIC" in fec.c with "#ifdef PHY_QS6612", and put
"#define PHY_QS6612" in rpxclassic.h. At present, anyone adding a board which
uses the QS6612 has to edit fec.c, even though it already supports their PHY.

> > We took this approach with our new custom board, and haven't had to add a
> > single #ifdef CONFIG_ourboardname outside the one which chooses our
> > board-specific header in mpc8xx.h.
>
> I do that with lots of custom boards.  Most are simple derivative of
> what is already there.  In fact, some have even choose the same I/O
> pins for Ethernet (there aren't many options, and most have been used :-),
> so we don't even need to add anything but another conditional on an
> #ifdef.

I'd much prefer the conditionals to test for board features rather than
specific boards. That way you _don't_ have to add another conditional on an
#ifdef every time you add a new board with existing features. In cases like
register names and bit definitions, we can even avoid the conditionals
altogether by using definitions like:

/* Ethernet : PHY controls - connected to port X
 *
 * Field        Value   Explanation
 * -----        -----   -----------
 * PHYPAUSE[x]      0   We don't support pause
 * PHYPWRDWN[x]     0   We don't support power down
 * PHYSLEEP[x]      0   We don't support sleep
 */
#define PHY_PAUSE               ((ushort) 0x0800)
#define PHY_PWRDWN              ((ushort) 0x0200)
#define PHY_SLEEP               ((ushort) 0x0040)

#define PHY_PAUSE_PORT          im_ioport.iop_pXdat
#define PHY_PWRDWN_PORT         im_ioport.iop_pXdat
#define PHY_SLEEP_PORT          im_ioport.iop_pXdat

> It's just not as easy as getting a patch and checking it in.

I appreciate your efforts co-ordinating all this. One of the reasons for
posting a patch is to stimulate discussion, and I'm interested in comments
from other people too.

Thanks,
Graham
--
Graham Stoney
Principal Hardware/Software Engineer
Canon Information Systems Research Australia
Ph: +61 2 9805 2909  Fax: +61 2 9805 2929

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

Re: Better board support (was Re: [PATCH] Adding support for LXT971/2 PHYs)

[Dan Malek]

Graham Stoney wrote:

> Another example under the scheme would be to replace most of the usages of
> "#ifdef CONFIG_RPXCLASSIC" in fec.c with "#ifdef PHY_QS6612",

Actually, that's already gone (but not yet checked in).  I don't even
have #ifdefs for the PHYS.  I scan the MII looking for all PHYs, when
one responds I match the mfg/device ID to a table and go.

There are thing unique to individual boards that still require board
specific #ifdefs.

> /* Ethernet : PHY controls - connected to port X

Many boards don't use 8xx I/O pins for PHY controls.  They have
external hardware registers for this because they use the 8xx pins
for other communication I/O.  This requires minor board specific
code to be included in the driver.


	-- Dan

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