[U-Boot-Users] [PATCH 2/2 (resubmit)] NET: Add Ethernet 1000BASE-X support for PPC4xx

[Larry Johnson <lrj@arlinx.com>]

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, &reg)) {
>> -        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, &reg)) {
>> -        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;
>>      }
>>      /* Check if auto-negotiation is on. */
>> -    if ((reg & PHY_BMCR_AUTON) != 0) {
>> +    if (bmcr & PHY_BMCR_AUTON) {
>>          /* Get auto-negotiation results. */
>> -        if (miiphy_read (devname, addr, PHY_ANLPAR, &reg)) {
>> -            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;
>>      }
>>      /* Get speed from basic control settings. */
>> -    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, &reg)) {
>> -        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, &reg)) {
>> -        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;
>>      }
>>      /* Check if auto-negotiation is on. */
>> -    if ((reg & PHY_BMCR_AUTON) != 0) {
>> +    if (bmcr & PHY_BMCR_AUTON) {
>>          /* Get auto-negotiation results. */
>> -        if (miiphy_read (devname, addr, PHY_ANLPAR, &reg)) {
>> -            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