diff mbox

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

Message ID f764a9570be4e3d4bd5d175e32a35084cfdae0c4.1479913469.git.mario.six@gdsys.cc
State Superseded
Delegated to: Stefan Roese
Headers show

Commit Message

Mario Six Nov. 23, 2016, 3:12 p.m. UTC 
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(+)

Comments

Joe Hershberger Nov. 29, 2016, 11 p.m. UTC | #1 
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.

> +       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
Dirk Eibach Dec. 1, 2016, 11:01 a.m. UTC | #2 
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
diff mbox

Patch

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);
+	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);
+
+	/* 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;
+}
+
 static struct phy_driver M88E1011S_driver = {
 	.name = "Marvell 88E1011S",
 	.uid = 0x1410c60,
@@ -580,6 +620,16 @@  static struct phy_driver M88E1310_driver = {
 	.shutdown = &genphy_shutdown,
 };
 
+static struct phy_driver M88E1680_driver = {
+	.name = "Marvell 88E1680",
+	.uid = 0x1410ed0,
+	.mask = 0xffffff0,
+	.features = PHY_GBIT_FEATURES,
+	.config = &m88e1680_config,
+	.startup = &genphy_startup,
+	.shutdown = &genphy_shutdown,
+};
+
 int phy_marvell_init(void)
 {
 	phy_register(&M88E1310_driver);
@@ -592,6 +642,7 @@  int phy_marvell_init(void)
 	phy_register(&M88E1011S_driver);
 	phy_register(&M88E1510_driver);
 	phy_register(&M88E1518_driver);
+	phy_register(&M88E1680_driver);
 
 	return 0;
 }