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

[Joe Hershberger]

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.

> +       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.

> +
> +       /* soft reset */
> +       reg = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMCR);
> +       reg |= BMCR_RESET;
> +       phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, reg);
> +
> +       return 0;
> +}

Thanks,
-Joe