[U-Boot] [PATCH 03/17] net: phy: Support Marvell 88E1680

[Dirk Eibach]

2016-11-30 0:00 GMT+01:00 Joe Hershberger <joe.hershberger at gmail.com>:
> On Wed, Nov 23, 2016 at 9:12 AM, Mario Six <mario.six at gdsys.cc> wrote:
>> From: Dirk Eibach <dirk.eibach at gdsys.cc>
>>
>> Add support for Marvell 88E1680 Integrated Octal
>> 10/100/1000 Mbps Energy Efficient Ethernet Transceiver.
>>
>> Signed-off-by: Dirk Eibach <dirk.eibach at gdsys.cc>
>> ---
>>  drivers/net/phy/marvell.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>
>> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
>> index 4eeb0f6..72f3f92 100644
>> --- a/drivers/net/phy/marvell.c
>> +++ b/drivers/net/phy/marvell.c
>> @@ -480,6 +480,46 @@ static int m88e1310_config(struct phy_device *phydev)
>>         return genphy_config_aneg(phydev);
>>  }
>>
>> +static int m88e1680_config(struct phy_device *phydev)
>> +{
>> +       /*
>> +        * As per Marvell Release Notes - Alaska V 88E1680 Rev A2
>> +        * Errata Section 4.1
>> +        */
>> +       u16 reg;
>> +
>> +       /* Matrix LED mode (not neede if single LED mode is used */
>> +       phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x0004);
>
> I realize the 88e151* driver went in without my ack (35fa0dda: "net:
> phy: marvell: add errata w/a for 88E151* chips"), and is loaded with
> magic numbers, but let's not proliferate the problem. Please define
> register offsets or use already-defined register offsets. If
> reasonable, use defined field values to build values from defines and
> something like bitfield_replace() from bitfield.h or clrsetbits_le32()
> from asm/io.h. When it is a constant that represents an encoded
> physical value that will never be used elsewhere, it's ok to just keep
> the hard-coded number in the write, but it should be preceeded with a
> comment that describes the actual meaning in engineering units and
> prefereably the equation used to come up with the constant.  If you
> have the information to improve the 151* implementation as well, that
> would be very welcome.

Problem is that the initialization sequence from the Marvell Release
Notes is writing undocumented values to undocumented registers. It
should be considered a binary blob to get this chip up and running.
All the information that is available is added as comments.

>
>> +       reg = phy_read(phydev, MDIO_DEVAD_NONE, 27);
>> +       reg |= (1 << 5);
>> +       phy_write(phydev, MDIO_DEVAD_NONE, 27, reg);
>> +
>> +       /* QSGMII TX amplitude change */
>> +       phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x00fd);
>> +       phy_write(phydev, MDIO_DEVAD_NONE,  8, 0x0b53);
>> +       phy_write(phydev, MDIO_DEVAD_NONE,  7, 0x200d);
>> +       phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x0000);
>> +
>> +       /* EEE initialization */
>> +       phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x00ff);
>> +       phy_write(phydev, MDIO_DEVAD_NONE, 17, 0xb030);
>> +       phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x215c);
>> +       phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x00fc);
>> +       phy_write(phydev, MDIO_DEVAD_NONE, 24, 0x888c);
>> +       phy_write(phydev, MDIO_DEVAD_NONE, 25, 0x888c);
>> +       phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x0000);
>> +       phy_write(phydev, MDIO_DEVAD_NONE,  0, 0x9140);
>> +
>> +       genphy_config_aneg(phydev);
>
> This should check the return code and return it if negative.

Mario, would you take care of this in V2?

Cheers
Dirk