[U-Boot] [PATCH v2 2/2] net: phy: Add PHY driver for mv88e61xx switches

[Kevin Smith]

Hi Joe,

Thanks for the review.  ACK to all comments.  A few clarifications are 
included below.  I will submit a v3, including multichip addressing for 
Albert's platform.

On 01/26/2016 06:11 PM, Joe Hershberger wrote:
>> +               int reg, u16 data)
>> +{
>> +       struct mii_dev *phys_bus;
> Why "phys_bus"? Isn't it an mdio_bus? Maybe that would be a better name.
Agreed.  This is extracting the actual mdio bus from the "fake" indirect 
mii device (see below).  I will change to a better name.
>> +
>> +static int mv88e61xx_probe(struct phy_device *phydev)
>> +{
>> +       struct mii_dev *mii_dev;
>> +
>> +       /* This device requires indirect reads/writes to the PHY registers
>> +        * which the generic PHY code can't handle.  Make a fake MII device to
>> +        * handle reads/writes */
>> +       mii_dev = mdio_alloc();
>> +       if (!mii_dev)
>> +               return -1;
>>
>> +
>> +       /* Store the actual bus in the fake mii device */
>> +       mii_dev->priv = phydev->bus;
>> +       strncpy(mii_dev->name, "mv88e61xx_protocol", sizeof(mii_dev->name));
>> +       mii_dev->read = mv88e61xx_phy_read_bus;
>> +       mii_dev->write = mv88e61xx_phy_write_bus;
>> +
>> +       /* Replace the bus with the fake device */
> Fake how? This is a confusing comment to me as written.
The genphy functions assume that they can write to the PHY directly 
using the MII bus, and the address it uses is the address of a 
register.  This is not the case for this chip with multiple PHY 
interfaces, which have to be accessed indirectly.  To handle this, I 
have created a "fake" mii_dev whose read/write functions are the 
indirection functions, and stored the actual mdio_bus in the private 
data for the "fake" device, which is then used by the indirect 
functions.  This allows this driver to make use of common genphy stuff 
where appropriate.  Maybe "wrapper" or "indirect bus" is a better name 
for it.  Let me know if you have a preference or better idea.
>> +
>> +       mac_addr = phydev->addr;
>> +
>> +       for (i = 0; i < PORT_COUNT; i++) {
>> +               if ((1 << i) & CONFIG_MV88E61XX_PHY_PORTS) {
>> +                       phydev->addr = i;
>> +                       mv88e61xx_phy_enable(phydev, i);
>> +                       mv88e61xx_phy_setup(phydev, i);
>> +                       mv88e61xx_phy_config_port(phydev, i);
> These all return status, but are ignored.
Even if one fails, it seems appropriate to me to continue initializing 
the others and not bail completely.  Should I catch the error, print a 
warning and "continue" in the loop?  Or is completely bailing the right 
thing to do?  If there are some errors, but other successes, what should 
the function return?
>> +
>> +
>> +
>> +       /* After reset, the energy detect signal remains high for a few seconds
>> +        * regardless of whether a cable is connected.  This function will
>> +        * return false positives during this time. */
> This is OK?
Yes.  This is used to skip the autonegotiation process if nothing is 
plugged into the port.  If you have 5-6 ports enabled but only one cable 
plugged in, waiting for all the other autonegotiation timeouts takes a 
long time.  This tries to be smart and not wait when nothing is plugged 
in.  A false positive just tries to autonegotiate.  After the first 
autonegotiate timeout, the energy detection hysteresis (or whatever) has 
corrected itself, so you only have to wait for one.

Thanks,
Kevin