diff mbox

[U-Boot,U-boot] net: phy: marvell: add errata w/a for 88E151* chips

Message ID 1414607903-3079-1-git-send-email-ivan.khoronzhuk@ti.com
State Changes Requested
Delegated to: Tom Rini
Headers show

Commit Message

Ivan Khoronzhuk Oct. 29, 2014, 6:38 p.m. UTC 
From: Hao Zhang <hzhang@ti.com>

As per Marvell Release Notes - Alaska 88E1510/88E1518/88E1512/88E1514
Rev A0, Errata Section 3.1 Marvell PHY has an errata which requires
that certain registers get written in order to restart
autonegotiation.

Signed-off-by: Hao Zhang <hzhang@ti.com>
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 drivers/net/phy/marvell.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

Comments

Stefan Roese Oct. 30, 2014, 6:53 a.m. UTC | #1 
On 29.10.2014 19:38, Ivan Khoronzhuk wrote:
> From: Hao Zhang <hzhang@ti.com>
>
> As per Marvell Release Notes - Alaska 88E1510/88E1518/88E1512/88E1514
> Rev A0, Errata Section 3.1 Marvell PHY has an errata which requires
> that certain registers get written in order to restart
> autonegotiation.
>
> Signed-off-by: Hao Zhang <hzhang@ti.com>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
>   drivers/net/phy/marvell.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index d2ecadc..425db94 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -276,6 +276,55 @@ static int m88e1111s_config(struct phy_device *phydev)
>   	return 0;
>   }
>
> +/**
> + * m88e1518_phy_writebits - write bits to a register
> + */
> +void m88e1518_phy_writebits(struct phy_device *phydev,
> +		   u8 reg_num, u16 offset, u16 len, u16 data)
> +{
> +	u16 reg, mask;
> +
> +	if ((len + offset) >= 16)
> +		mask = 0 - (1 << offset);
> +	else
> +		mask = (1 << (len + offset)) - (1 << offset);
> +
> +	reg = phy_read(phydev, MDIO_DEVAD_NONE, reg_num);
> +
> +	reg &= ~mask;
> +	reg |= data << offset;
> +
> +	phy_write(phydev, MDIO_DEVAD_NONE, reg_num, reg);
> +}
> +
> +static int m88e1518_config(struct phy_device *phydev)
> +{
> +	/*
> +	 * As per Marvell Release Notes - Alaska 88E1510/88E1518/88E1512/88E1514
> +	 * Rev A0, Errata Section 3.1
> +	 */
> +	phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x00ff);	/* reg page 0xff */
> +	phy_write(phydev, MDIO_DEVAD_NONE, 17, 0x214B);
> +	phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2144);
> +	phy_write(phydev, MDIO_DEVAD_NONE, 17, 0x0C28);
> +	phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2146);
> +	phy_write(phydev, MDIO_DEVAD_NONE, 17, 0xB233);
> +	phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x214D);
> +	phy_write(phydev, MDIO_DEVAD_NONE, 17, 0xCC0C);
> +	phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2159);
> +	phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x0000);	/* reg page 0 */
> +	phy_write(phydev, MDIO_DEVAD_NONE, 22, 18);	/* reg page 18 */
> +	/* Write HWCFG_MODE = SGMII to Copper */
> +	m88e1518_phy_writebits(phydev, 20, 0, 3, 1);

Won't this set the mode to SGMII for all users of this code? I know of 
at least one board that uses this driver and uses RGMII. So you 
shouldn't set this mode here to SGMII unconditionally.

Thanks,
Stefan
Ivan Khoronzhuk Oct. 30, 2014, 9:52 a.m. UTC | #2 
On 10/30/2014 08:53 AM, Stefan Roese wrote:
> On 29.10.2014 19:38, Ivan Khoronzhuk wrote:
>> From: Hao Zhang <hzhang@ti.com>
>>
>> As per Marvell Release Notes - Alaska 88E1510/88E1518/88E1512/88E1514
>> Rev A0, Errata Section 3.1 Marvell PHY has an errata which requires
>> that certain registers get written in order to restart
>> autonegotiation.
>>
>> Signed-off-by: Hao Zhang <hzhang@ti.com>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> ---
>>   drivers/net/phy/marvell.c | 51 
>> ++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
>> index d2ecadc..425db94 100644
>> --- a/drivers/net/phy/marvell.c
>> +++ b/drivers/net/phy/marvell.c
>> @@ -276,6 +276,55 @@ static int m88e1111s_config(struct phy_device 
>> *phydev)
>>       return 0;
>>   }
>>
>> +/**
>> + * m88e1518_phy_writebits - write bits to a register
>> + */
>> +void m88e1518_phy_writebits(struct phy_device *phydev,
>> +           u8 reg_num, u16 offset, u16 len, u16 data)
>> +{
>> +    u16 reg, mask;
>> +
>> +    if ((len + offset) >= 16)
>> +        mask = 0 - (1 << offset);
>> +    else
>> +        mask = (1 << (len + offset)) - (1 << offset);
>> +
>> +    reg = phy_read(phydev, MDIO_DEVAD_NONE, reg_num);
>> +
>> +    reg &= ~mask;
>> +    reg |= data << offset;
>> +
>> +    phy_write(phydev, MDIO_DEVAD_NONE, reg_num, reg);
>> +}
>> +
>> +static int m88e1518_config(struct phy_device *phydev)
>> +{
>> +    /*
>> +     * As per Marvell Release Notes - Alaska 
>> 88E1510/88E1518/88E1512/88E1514
>> +     * Rev A0, Errata Section 3.1
>> +     */
>> +    phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x00ff);    /* reg page 
>> 0xff */
>> +    phy_write(phydev, MDIO_DEVAD_NONE, 17, 0x214B);
>> +    phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2144);
>> +    phy_write(phydev, MDIO_DEVAD_NONE, 17, 0x0C28);
>> +    phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2146);
>> +    phy_write(phydev, MDIO_DEVAD_NONE, 17, 0xB233);
>> +    phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x214D);
>> +    phy_write(phydev, MDIO_DEVAD_NONE, 17, 0xCC0C);
>> +    phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2159);
>> +    phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x0000);    /* reg page 0 */
>> +    phy_write(phydev, MDIO_DEVAD_NONE, 22, 18);    /* reg page 18 */
>> +    /* Write HWCFG_MODE = SGMII to Copper */
>> +    m88e1518_phy_writebits(phydev, 20, 0, 3, 1);
>
> Won't this set the mode to SGMII for all users of this code? I know of 
> at least one board that uses this driver and uses RGMII. So you 
> shouldn't set this mode here to SGMII unconditionally.
>
> Thanks,
> Stefan
>

Yes.
I will put whole errata w/o under:
if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {

}

as I can face it only for SGMII

Thanks!
Stefan Roese Oct. 30, 2014, 11:39 a.m. UTC | #3 
On 30.10.2014 10:52, Ivan Khoronzhuk wrote:
>>> +static int m88e1518_config(struct phy_device *phydev)
>>> +{
>>> +    /*
>>> +     * As per Marvell Release Notes - Alaska
>>> 88E1510/88E1518/88E1512/88E1514
>>> +     * Rev A0, Errata Section 3.1
>>> +     */
>>> +    phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x00ff);    /* reg page
>>> 0xff */
>>> +    phy_write(phydev, MDIO_DEVAD_NONE, 17, 0x214B);
>>> +    phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2144);
>>> +    phy_write(phydev, MDIO_DEVAD_NONE, 17, 0x0C28);
>>> +    phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2146);
>>> +    phy_write(phydev, MDIO_DEVAD_NONE, 17, 0xB233);
>>> +    phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x214D);
>>> +    phy_write(phydev, MDIO_DEVAD_NONE, 17, 0xCC0C);
>>> +    phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2159);
>>> +    phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x0000);    /* reg page 0 */
>>> +    phy_write(phydev, MDIO_DEVAD_NONE, 22, 18);    /* reg page 18 */
>>> +    /* Write HWCFG_MODE = SGMII to Copper */
>>> +    m88e1518_phy_writebits(phydev, 20, 0, 3, 1);
>>
>> Won't this set the mode to SGMII for all users of this code? I know of
>> at least one board that uses this driver and uses RGMII. So you
>> shouldn't set this mode here to SGMII unconditionally.
>>
>> Thanks,
>> Stefan
>>
>
> Yes.
> I will put whole errata w/o under:
> if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
>
> }
>
> as I can face it only for SGMII

Yes. If this errata is really only for SGMII, then putting this code 
under this if() statement makes sense.

Thanks,
Stefan
Tom Rini Nov. 5, 2014, 9:30 p.m. UTC | #4 
On Wed, Oct 29, 2014 at 08:38:23PM +0200, Khoronzhuk, Ivan wrote:

> From: Hao Zhang <hzhang@ti.com>
> 
> As per Marvell Release Notes - Alaska 88E1510/88E1518/88E1512/88E1514
> Rev A0, Errata Section 3.1 Marvell PHY has an errata which requires
> that certain registers get written in order to restart
> autonegotiation.
> 
> Signed-off-by: Hao Zhang <hzhang@ti.com>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>

Applied to u-boot-ti/master, thanks!
Tom Rini Nov. 6, 2014, 4:05 p.m. UTC | #5 
On Wed, Nov 05, 2014 at 04:30:33PM -0500, Tom Rini wrote:
> On Wed, Oct 29, 2014 at 08:38:23PM +0200, Khoronzhuk, Ivan wrote:
> 
> > From: Hao Zhang <hzhang@ti.com>
> > 
> > As per Marvell Release Notes - Alaska 88E1510/88E1518/88E1512/88E1514
> > Rev A0, Errata Section 3.1 Marvell PHY has an errata which requires
> > that certain registers get written in order to restart
> > autonegotiation.
> > 
> > Signed-off-by: Hao Zhang <hzhang@ti.com>
> > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> 
> Applied to u-boot-ti/master, thanks!

... Ivan pointed out there's a v2, dropping this one and getting the
right one.
diff mbox

Patch

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index d2ecadc..425db94 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -276,6 +276,55 @@  static int m88e1111s_config(struct phy_device *phydev)
 	return 0;
 }
 
+/**
+ * m88e1518_phy_writebits - write bits to a register
+ */
+void m88e1518_phy_writebits(struct phy_device *phydev,
+		   u8 reg_num, u16 offset, u16 len, u16 data)
+{
+	u16 reg, mask;
+
+	if ((len + offset) >= 16)
+		mask = 0 - (1 << offset);
+	else
+		mask = (1 << (len + offset)) - (1 << offset);
+
+	reg = phy_read(phydev, MDIO_DEVAD_NONE, reg_num);
+
+	reg &= ~mask;
+	reg |= data << offset;
+
+	phy_write(phydev, MDIO_DEVAD_NONE, reg_num, reg);
+}
+
+static int m88e1518_config(struct phy_device *phydev)
+{
+	/*
+	 * As per Marvell Release Notes - Alaska 88E1510/88E1518/88E1512/88E1514
+	 * Rev A0, Errata Section 3.1
+	 */
+	phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x00ff);	/* reg page 0xff */
+	phy_write(phydev, MDIO_DEVAD_NONE, 17, 0x214B);
+	phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2144);
+	phy_write(phydev, MDIO_DEVAD_NONE, 17, 0x0C28);
+	phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2146);
+	phy_write(phydev, MDIO_DEVAD_NONE, 17, 0xB233);
+	phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x214D);
+	phy_write(phydev, MDIO_DEVAD_NONE, 17, 0xCC0C);
+	phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x2159);
+	phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x0000);	/* reg page 0 */
+	phy_write(phydev, MDIO_DEVAD_NONE, 22, 18);	/* reg page 18 */
+	/* Write HWCFG_MODE = SGMII to Copper */
+	m88e1518_phy_writebits(phydev, 20, 0, 3, 1);
+
+	/* Phy reset */
+	m88e1518_phy_writebits(phydev, 20, 15, 1, 1);
+	phy_write(phydev, MDIO_DEVAD_NONE, 22, 0);	/* reg page 18 */
+	udelay(100);
+
+	return m88e1111s_config(phydev);
+}
+
 /* Marvell 88E1118 */
 static int m88e1118_config(struct phy_device *phydev)
 {
@@ -493,7 +542,7 @@  static struct phy_driver M88E1518_driver = {
 	.uid = 0x1410dd1,
 	.mask = 0xffffff0,
 	.features = PHY_GBIT_FEATURES,
-	.config = &m88e1111s_config,
+	.config = &m88e1518_config,
 	.startup = &m88e1011s_startup,
 	.shutdown = &genphy_shutdown,
 };