Skip to contentSkip to conversation

44x ethernet MDIO access

PATCH Applied Ralph Siemsen 3 participants 6 messages Aug 5, 2004 – Aug 6, 2004 Network drivers

lore
Share & tools
lore

Ralph Siemsen

author
#0 lore raw

PATCH 1/1 44x ethernet MDIO access

There is a small bug in the emac_phy_write() routine defined in drivers/net/ibm_emac/ibm_emac_core.c (present in latest 2.6.8-rc3). At the end of the function, check is done for phy error; however as written this will always be false, because "stacr" contains the value _written_ to the hardware, rather than the value read back from hardware.

Simple on-line fix attached.

In a related issue, both the PHY read and write functions use a fixed time delay (MDIO_DELAY = 50) presently... it seems I need a longer value on my hardware, and having such hardcoded values is trouble anyways... I suggest we change the code to poll for completion, with a short delay and an upper limit on the number of loops.

=Ralph

Allow proper detection of phy write error.

drivers/net/ibm_emac/ibm_emac_core.c renamed from drivers/net/ibm_emac/ibm_emac_core.c.orig +1 -1
@@ -453,7 +453,7 @@
 
 	udelay(MDIO_DELAY);
 
-	if ((in_be32(&emacp->em0stacr) & EMAC_STACR_OC) == 0)
+	if (((stacr = in_be32(&emacp->em0stacr)) & EMAC_STACR_OC) == 0)
 		printk(KERN_WARNING "%s: PHY write timeout #2!\n", dev->name);
 
 	/* Check for a write error */
author
#1 lore raw
Quoted 5 lines · Ralph Siemsen

In a related issue, both the PHY read and write functions use a fixed time delay (MDIO_DELAY = 50) presently... it seems I need a longer value on my hardware, and having such hardcoded values is trouble anyways... I suggest we change the code to poll for completion, with a short delay and an upper limit on the number of loops.

I've just checked the hardware, it seems that the MDIO clock is driven at EPB clock divided by 10. So on the IBM eval board its 7.6MHz. Each MDIO transfer involves moving 64 bits of data. So that means just the data transfer (never mind any processing in the PHY) takes 8.4us.

The important part to note is that EPB could vary over a considerable range, so the time needed for MDIO is potentially longer than the 50us currently used in the code.

Attached is a patch that "works for me" - turns the long udelay() into a polling loop with short little delays instead. This allows MDIO_DELAY to be increased to a larger value (say 150) without slowing down most systems.

Also I can be used to measure just how long the transfers actually take.
If MDIO_DEBUG is enabled then you'll see how many "loops" were
needed... on the Ocotea board i get values around 25.

-Ralph

Change MDIO read/write to poll for completion rather than waiting an arbitrary length of time. Note this changes the semantics of the MDIO_DELAY definition - it is now a maximal loop count, rather than a time delay in microseconds.

drivers/net/ibm_emac/ibm_emac_core.c +13 -4
@@ -367,6 +371,7 @@
 
 int emac_phy_read(struct net_device *dev, int mii_id, int reg)
 {
+	int count;
 	uint32_t stacr;
 	struct ocp_enet_private *fep = dev->priv;
 	emac_t *emacp = fep->emacp;

@@ -398,8 +403,11 @@
 
 	out_be32(&emacp->em0stacr, stacr);
 
-	udelay(MDIO_DELAY);
-	stacr = in_be32(&emacp->em0stacr);
+	count =  0;
+	while ((((stacr = in_be32(&emacp->em0stacr)) & EMAC_STACR_OC) == 0)
+					&& (count++ < MDIO_DELAY))
+		udelay(1);
+	MDIO_DEBUG((" (count was %d)\n", count));
 
 	if ((stacr & EMAC_STACR_OC) == 0) {
 		printk(KERN_WARNING "%s: PHY read timeout #2!\n", dev->name);

@@ -419,6 +427,7 @@
 
 void emac_phy_write(struct net_device *dev, int mii_id, int reg, int data)
 {
+	int count;
 	uint32_t stacr;
 	struct ocp_enet_private *fep = dev->priv;
 	emac_t *emacp = fep->emacp;

@@ -451,9 +460,13 @@
 
 	out_be32(&emacp->em0stacr, stacr);
 
-	udelay(MDIO_DELAY);
+	count = 0;
+	while (((stacr = in_be32(&emacp->em0stacr) & EMAC_STACR_OC) == 0)
+					&& (count++ < 5000))
+		udelay(1);
+	MDIO_DEBUG((" (count was %d)\n", count));
 
-	if (((stacr = in_be32(&emacp->em0stacr)) & EMAC_STACR_OC) == 0)
+	if ((stacr & EMAC_STACR_OC) == 0)
 		printk(KERN_WARNING "%s: PHY write timeout #2!\n", dev->name);
 
 	/* Check for a write error */
#2 lore raw
Quoted 21 lines · Ralph Siemsen
Quoted 5 lines · Ralph Siemsen

In a related issue, both the PHY read and write functions use a fixed time delay (MDIO_DELAY = 50) presently... it seems I need a longer value on my hardware, and having such hardcoded values is trouble anyways... I suggest we change the code to poll for completion, with a short delay and an upper limit on the number of loops.

I've just checked the hardware, it seems that the MDIO clock is driven at EPB clock divided by 10. So on the IBM eval board its 7.6MHz. Each MDIO transfer involves moving 64 bits of data. So that means just the data transfer (never mind any processing in the PHY) takes 8.4us.

The important part to note is that EPB could vary over a considerable range, so the time needed for MDIO is potentially longer than the 50us currently used in the code.

Attached is a patch that "works for me" - turns the long udelay() into a polling loop with short little delays instead. This allows MDIO_DELAY to be increased to a larger value (say 150) without slowing down most systems.

Quoted 5 lines

Also I can be used to measure just how long the transfers actually take.
If MDIO_DEBUG is enabled then you'll see how many "loops" were
needed... on the Ocotea board i get values around 25.

-Ralph

Applied (with minor mods).

Ralph, can you take a look at http://kerneltrap.org/node/view/3180
and sign off your future patches accordingly?

Thanks,
Matt

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

#3 lore raw
In reply to Ralph Siemsen

drivers/net/ibm_emac/ibm_emac_core.c line 465

Quoted 13 lines · Ralph Siemsen

Attached is a patch that "works for me" - turns the long udelay() into a polling loop with short little delays instead. This allows MDIO_DELAY to be increased to a larger value (say 150) without slowing down most systems.

drivers/net/ibm_emac/ibm_emac_core.c +3 -1
@@ -451,9 +460,13 @@
 
 	out_be32(&emacp->em0stacr, stacr);
 
-	udelay(MDIO_DELAY);
+	count = 0;
+	while (((stacr = in_be32(&emacp->em0stacr) & EMAC_STACR_OC) == 0)
+					&& (count++ < 5000))

                                                      ^^^^
						      MDIO_DELAY?

And perhaps you want to create a (possibly inlined) function just for this loop?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

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

author
#4 lore raw
Quoted 10 lines · Geert Uytterhoeven

drivers/net/ibm_emac/ibm_emac_core.c line 465

Quoted 4 lines
drivers/net/ibm_emac/ibm_emac_core.c +3 -1
-	udelay(MDIO_DELAY);
+	count = 0;
+	while (((stacr = in_be32(&emacp->em0stacr) & EMAC_STACR_OC) == 0)
+					&& (count++ < 5000))

                                                      ^^^^
						      MDIO_DELAY?

And perhaps you want to create a (possibly inlined) function just for this loop?

Good catch - yes, should be MDIO_DELAY not 5000. I had found that 50 was not enough so I had upped it in my testing - forgot to change it back :(

I'd also suggest the default MDIO_DELAY be increased from its present value of 50 to perhaps 100 or 150... Does anyone know what the slowest possible EPB clock speed is?

-R

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

Applied
#5 lore raw
In reply to Ralph Siemsen
Quoted 8 lines · Ralph Siemsen

There is a small bug in the emac_phy_write() routine defined in drivers/net/ibm_emac/ibm_emac_core.c (present in latest 2.6.8-rc3). At the end of the function, check is done for phy error; however as written this will always be false, because "stacr" contains the value _written_ to the hardware, rather than the value read back from hardware.

Simple on-line fix attached.

Quoted 7 lines

In a related issue, both the PHY read and write functions use a fixed time delay (MDIO_DELAY = 50) presently... it seems I need a longer value on my hardware, and having such hardcoded values is trouble anyways... I suggest we change the code to poll for completion, with a short delay and an upper limit on the number of loops.

=Ralph

Applied, thanks.

-Matt

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