Hi Ben,
Thank you for your comments. See below...
Ben Warren wrote:
Larry,
I think this can be simplified a bit. More later on...
Larry Johnson wrote:
This patch adds a new switch: "CONFIG_PHY_DYNAMIC_ANEG". When this symbol is defined, the PHY will advertise it's capabilities for autonegotiation based on the capabilities shown in the PHY's status registers, including 1000BASE-X. When "CONFIG_PHY_DYNAMIC_ANEG" is not defined, the PHY will advertise hard-coded capabilities, as before.
Signed-off-by: Larry Johnson lrj@acm.org
common/miiphyutil.c | 155 +++++++++++++++++++++++++++++++++------------------ include/miiphy.h | 21 +++++++ 2 files changed, 121 insertions(+), 55 deletions(-)
diff --git a/common/miiphyutil.c b/common/miiphyutil.c index 58ebc5e..b2f62d0 100644 --- a/common/miiphyutil.c +++ b/common/miiphyutil.c @@ -344,101 +344,146 @@ int miiphy_reset (char *devname, unsigned char addr)
/*****************************************************************************
- Determine the ethernet speed (10/100).
*/
- Determine the ethernet speed (10/100/1000). Return 10 on error.
int miiphy_speed (char *devname, unsigned char addr) {
- unsigned short reg;
- u16 bmcr;
#if defined(CONFIG_PHY_GIGE)
- if (miiphy_read (devname, addr, PHY_1000BTSR, ®)) {
printf ("PHY 1000BT Status read failed\n");- } else {
if (reg != 0xFFFF) {if ((reg & (PHY_1000BTSR_1000FD | PHY_1000BTSR_1000HD))!= 0) {return (_1000BASET);}
- u16 btsr;
+#if defined(CONFIG_PHY_DYNAMIC_ANEG)
I don't think you need this CONFIG. It doesn't really do anything.
I only put it in to keep the footprint smaller for boards that aren't using fiber, so I'll get rid if it.
- u16 bmsr, exsr;
- /* Check for 1000BASE-X. */
- if (miiphy_read (devname, addr, PHY_BMSR, &bmsr)) {
printf ("PHY status");goto miiphy_read_failed;- }
- if (bmsr & PHY_BMSR_EXT_STAT) {
if (miiphy_read (devname, addr, PHY_EXSR, &exsr)) {printf ("PHY extended status");goto miiphy_read_failed;}if (exsr & (PHY_EXSR_1000XF | PHY_EXSR_1000XH)) {/* 1000BASE-X */return _1000BASET;Per IEEE 802.3-2005, All PHYs with capabilities > 100Mbps must have PHY_MBSR_EXT_STAT set, and EXSR contains capability info for 1000BX and 1000BT, so you can check for all four here. Please correct me if I'm wrong.
Checking PHY_BMSR_EXT_STAT when CONIF_PHY_GIGE is defined does seem a bit paranoid, now that you mention it.
For the rest, let me explain my thinking. The board I'm writing for, named "Korat", has ethernet ports that are configurable for either 10BASE-T/100BASE-TX/1000BASE-T copper, or 1000BASE-X fiber. IEEE defines some registers differently depending on which is configured. To determine board-independently how the registers are to be interpreted, I look in the extended status register. If 1000BASE-X full or half duplex is listed as a capability, I assume fiber, otherwise copper. This works on the PHY I'm using, and I can't see it working differently on another PHY because some bits in some registers are used differently depending on the medium.
At this particular point in the code, if the PHY is configured for 1000BASE-X, then I report the speed as 1000 without checking for any autonegotiation results. If the PHY is configured for copper, then it is still necessary to check 1000BTSR (as in the original code) to see what speed was negotiated.
} }+#endif /* defined(CONFIG_PHY_DYNAMIC_ANEG) */
- /* Check for 1000BASE-T. */
- if (miiphy_read (devname, addr, PHY_1000BTSR, &btsr)) {
printf ("PHY 1000BT status");goto miiphy_read_failed;- }
- if (btsr != 0xFFFF &&
(btsr & (PHY_1000BTSR_1000FD | PHY_1000BTSR_1000HD))) {return _1000BASET;- }
And then you don't need to check 1000BTSR
See above.
#endif /* CONFIG_PHY_GIGE */
/* Check Basic Management Control Register first. */
- if (miiphy_read (devname, addr, PHY_BMCR, ®)) {
puts ("PHY speed read failed, assuming 10bT\n");return (_10BASET);
- if (miiphy_read (devname, addr, PHY_BMCR, &bmcr)) {
printf ("PHY speed");goto miiphy_read_failed;
- if ((reg & PHY_BMCR_AUTON) != 0) {
- if (bmcr & PHY_BMCR_AUTON) { /* Get auto-negotiation results. */
if (miiphy_read (devname, addr, PHY_ANLPAR, ®)) {puts ("PHY AN speed read failed, assuming 10bT\n");return (_10BASET);}if ((reg & PHY_ANLPAR_100) != 0) {return (_100BASET);} else {return (_10BASET);
u16 anlpar;Please move the anlpar declaration to the top of the function
OK. (I missed the anlpar declarations. Sorry.)
if (miiphy_read (devname, addr, PHY_ANLPAR, &anlpar)) {printf ("PHY AN speed");goto miiphy_read_failed; }return (anlpar & PHY_ANLPAR_100) ? _100BASET : _10BASET;
- else if (reg & PHY_BMCR_100MB) {
return (_100BASET);- } else {
return (_10BASET);- }
return (bmcr & PHY_BMCR_100MB) ? _100BASET : _10BASET;
miiphy_read_failed:printf (" read failed, assuming 10BASE-T\n");
return _10BASET;
}
/*****************************************************************************
- Determine full/half duplex.
*/
- Determine full/half duplex. Return half on error.
int miiphy_duplex (char *devname, unsigned char addr) {
- unsigned short reg;
- u16 bmcr;
#if defined(CONFIG_PHY_GIGE)
- if (miiphy_read (devname, addr, PHY_1000BTSR, ®)) {
printf ("PHY 1000BT Status read failed\n");- } else {
if ((reg != 0xFFFF) &&(reg & (PHY_1000BTSR_1000FD | PHY_1000BTSR_1000HD))) {if ((reg & PHY_1000BTSR_1000FD) != 0) {return (FULL);} else {return (HALF);
- u16 btsr;
+#if defined(CONFIG_PHY_DYNAMIC_ANEG)
Same as above. Don't need it, I don't think.
Good.
- u16 bmsr, exsr;
- /* Check for 1000BASE-X. */
- if (miiphy_read (devname, addr, PHY_BMSR, &bmsr)) {
printf ("PHY status");goto miiphy_read_failed;- }
- if (bmsr & PHY_BMSR_EXT_STAT) {
if (miiphy_read (devname, addr, PHY_EXSR, &exsr)) {printf ("PHY extended status");goto miiphy_read_failed;}if (exsr & (PHY_EXSR_1000XF | PHY_EXSR_1000XH)) {/* 1000BASE-X */u16 anlpar;if (miiphy_read (devname, addr, PHY_ANLPAR, &anlpar)) {printf ("1000BASE-X PHY AN duplex");goto miiphy_read_failed; }return (anlpar & PHY_X_ANLPAR_FD) ? FULL : HALF;}- }
+#endif /* defined(CONFIG_PHY_DYNAMIC_ANEG) */
- /* Check for 1000BASE-T. */
- if (miiphy_read (devname, addr, PHY_1000BTSR, &btsr)) {
printf ("PHY 1000BT status");goto miiphy_read_failed;- }
- if (btsr != 0xFFFF) {
if (btsr & PHY_1000BTSR_1000FD) {return FULL;} else if (btsr & PHY_1000BTSR_1000HD) {return HALF; }Same as above. All GigE capabilities are listed in EXSR
See note above. Again it's not the capabilities, but the autonegotiation results that need to be checked. In this case, I cannot make any assumption based on being in 1000BASE-X mode, but must check the autonegotiation result, which is in ANLPAR, but with different bit definitions than for 10/100 Mbs.
}#endif /* CONFIG_PHY_GIGE */
/* Check Basic Management Control Register first. */
- if (miiphy_read (devname, addr, PHY_BMCR, ®)) {
puts ("PHY duplex read failed, assuming half duplex\n");return (HALF);
- if (miiphy_read (devname, addr, PHY_BMCR, &bmcr)) {
puts ("PHY duplex");goto miiphy_read_failed;
- if ((reg & PHY_BMCR_AUTON) != 0) {
- if (bmcr & PHY_BMCR_AUTON) { /* Get auto-negotiation results. */
if (miiphy_read (devname, addr, PHY_ANLPAR, ®)) {puts ("PHY AN duplex read failed, assuming half duplex\n");return (HALF);}
u16 anlpar;See comment above
Sorry, again.
[snip]
Nice work.
regards, Ben
Thank you. I'm thinking now that there should now be a function to report whether the PHY is in 1000BASE-X mode, to go along with the speed and duplex functions. I'd like to try that to see if it makes the code clearer. I'll post the revised patch after I've had a chance to test it to get your and others' opinions.
Best regards, Larry