2016-11-30 0:00 GMT+01:00 Joe Hershberger joe.hershberger@gmail.com:
On Wed, Nov 23, 2016 at 9:12 AM, Mario Six mario.six@gdsys.cc wrote:
From: Dirk Eibach dirk.eibach@gdsys.cc
Add support for Marvell 88E1680 Integrated Octal 10/100/1000 Mbps Energy Efficient Ethernet Transceiver.
Signed-off-by: Dirk Eibach dirk.eibach@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