Message ID | 201209101545.q8AFjn7u021483@localhost.localdomain |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Mon, Sep 10, 2012 at 05:45:49PM +0200, Christophe Leroy wrote: > This patch adds proper handling of the buggy revision A2 of LXT973 phy, adding > precautions linked to ERRATA Item 4: I have a few nit picking remarks, below... > Item 4: MDIO Interface and Repeated Polling > Problem: Repeated polling of odd-numbered registers via the MDIO interface > randomly returns the contents of the previous even register. > Implication: Managed applications may not obtain the correct register contents > when a particular register is monitored for device status. > Workaround: None. > Status: This erratum has been previously fixed (in rev A3) This text looks like it has been copied verbatim from a errata sheet. If so, that document is probably under someone else's copyright. If am right, then you should paraphrase the erratum instead, both here and in the code comment. > > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> > > diff -u linux-3.5-vanilla/drivers/net/phy/lxt.c linux-3.5/drivers/net/phy/lxt.c > --- linux-3.5-vanilla/drivers/net/phy/lxt.c 2012-07-21 22:58:29.000000000 +0200 > +++ linux-3.5/drivers/net/phy/lxt.c 2012-09-07 18:08:54.000000000 +0200 > @@ -7,6 +7,10 @@ > * > * Copyright (c) 2004 Freescale Semiconductor, Inc. > * > + * Copyright (c) 2010 CSSI > + * > + * Added support for buggy LXT973 rev A2 We don't do change logs in the code. That is what git is for. Also, I think it is a bit of a stretch to add your copyright here. > + * > * This program is free software; you can redistribute it and/or modify it > * under the terms of the GNU General Public License as published by the > * Free Software Foundation; either version 2 of the License, or (at your > @@ -54,8 +58,12 @@ > #define MII_LXT971_ISR 19 /* Interrupt Status Register */ > > /* register definitions for the 973 */ > -#define MII_LXT973_PCR 16 /* Port Configuration Register */ > +#define MII_LXT973_PCR 16 /* Port Configuration Register */ > #define PCR_FIBER_SELECT 1 > +#define MII_LXT973_SFR 27 /* Special Function Register */ > + > +#define PHYDEV_PRIV_FIBER 1 > +#define PHYDEV_PRIV_REVA2 2 > > MODULE_DESCRIPTION("Intel LXT PHY driver"); > MODULE_AUTHOR("Andy Fleming"); > @@ -99,6 +107,9 @@ > return err; > } > > +/* register definitions for the 973 */ > +#define MII_LXT973_PCR 16 /* Port Configuration Register */ > +#define PCR_FIBER_SELECT 1 > > static int lxt971_ack_interrupt(struct phy_device *phydev) > { > @@ -122,9 +133,141 @@ > return err; > } > > +/* > + * ERRATA on LXT973 > + * > + * Item 4: MDIO Interface and Repeated Polling > + * Problem: Repeated polling of odd-numbered registers via the MDIO interface randomly returns the > + * contents of the previous even register. > + * Implication: Managed applications may not obtain the correct register contents when a particular > + * register is monitored for device status. > + * Workaround: None. > + * Status: This erratum has been previously fixed (in rev A3) > + * > + */ Please make sure that the above text is your own words. > +static int lxt973a2_update_link(struct phy_device *phydev) > +{ > + int status; > + int control; > + int retry = 8; /* we try 8 times */ > + > + /* Do a fake read */ > + status = phy_read(phydev, MII_BMSR); > + > + if (status < 0) > + return status; > + > + control = phy_read(phydev, MII_BMCR); > + if (control < 0) > + return control; > + > + do { > + /* Read link and autonegotiation status */ > + status = phy_read(phydev, MII_BMSR); > + } while (status>=0 && retry-- && status == control); spacing: status >= 0 > + > + if (status < 0) > + return status; > + > + if ((status & BMSR_LSTATUS) == 0) > + phydev->link = 0; > + else > + phydev->link = 1; > + > + return 0; > +} > + > +int lxt973a2_read_status(struct phy_device *phydev) > +{ > + int adv; > + int err; > + int lpa; > + int lpagb = 0; > + > + /* Update the link, but return if there was an error */ > + err = lxt973a2_update_link(phydev); > + if (err) > + return err; > + > + if (AUTONEG_ENABLE == phydev->autoneg) { > + int retry = 1; > + > + adv = phy_read(phydev, MII_ADVERTISE); > + > + if (adv < 0) > + return adv; > + > + do { > + lpa = phy_read(phydev, MII_LPA); > + > + if (lpa < 0) > + return lpa; > + > + /* If both registers are equal, it is suspect but not impossible, hence a new try */ Trailing white space. Line is way too long. > + } while (lpa == adv && retry--); > + > + lpa &= adv; > + > + phydev->speed = SPEED_10; > + phydev->duplex = DUPLEX_HALF; > + phydev->pause = phydev->asym_pause = 0; > + > + if (lpagb & (LPA_1000FULL | LPA_1000HALF)) { > + phydev->speed = SPEED_1000; > + > + if (lpagb & LPA_1000FULL) > + phydev->duplex = DUPLEX_FULL; > + } else if (lpa & (LPA_100FULL | LPA_100HALF)) { > + phydev->speed = SPEED_100; > + > + if (lpa & LPA_100FULL) > + phydev->duplex = DUPLEX_FULL; > + } else I would either put this 'else' with the 'if' on the same line, or add braces like in the other cases. > + if (lpa & LPA_10FULL) > + phydev->duplex = DUPLEX_FULL; > + > + if (phydev->duplex == DUPLEX_FULL){ > + phydev->pause = lpa & LPA_PAUSE_CAP ? 1 : 0; > + phydev->asym_pause = lpa & LPA_PAUSE_ASYM ? 1 : 0; > + } > + } else { > + int bmcr = phy_read(phydev, MII_BMCR); > + > + if (bmcr < 0) > + return bmcr; > + > + if (bmcr & BMCR_FULLDPLX) > + phydev->duplex = DUPLEX_FULL; > + else > + phydev->duplex = DUPLEX_HALF; > + > + if (bmcr & BMCR_SPEED1000) > + phydev->speed = SPEED_1000; > + else if (bmcr & BMCR_SPEED100) > + phydev->speed = SPEED_100; > + else > + phydev->speed = SPEED_10; > + > + phydev->pause = phydev->asym_pause = 0; > + } > + > + return 0; > +} > + > +static int lxt973_read_status(struct phy_device *phydev) > +{ > + return (int)phydev->priv&PHYDEV_PRIV_REVA2 ? lxt973a2_read_status(phydev) : genphy_read_status(phydev); spacing, and way too long line. return (int) phydev->priv & PHYDEV_PRIV_REVA2 ? lxt973a2_read_status(phydev) : genphy_read_status(phydev); > +} > + > static int lxt973_probe(struct phy_device *phydev) > { > int val = phy_read(phydev, MII_LXT973_PCR); > + int priv = 0; > + > + phydev->priv = NULL; > + > + if (val<0) return val; spacing > > if (val & PCR_FIBER_SELECT) { > /* > @@ -136,17 +279,24 @@ > val &= ~BMCR_ANENABLE; > phy_write(phydev, MII_BMCR, val); > /* Remember that the port is in fiber mode. */ > - phydev->priv = lxt973_probe; > - } else { > - phydev->priv = NULL; > + priv |= PHYDEV_PRIV_FIBER; > } > + val = phy_read(phydev, MII_PHYSID2); > + > + if (val<0) return val; spacing > + > + if ((val & 0xf) == 0) { /* rev A2 */ > + dev_info(&phydev->dev, " LXT973 revision A2 has bugs\n"); > + priv |= PHYDEV_PRIV_REVA2; > + } > + phydev->priv = (void*)priv; spacing > return 0; > } > > static int lxt973_config_aneg(struct phy_device *phydev) > { > /* Do nothing if port is in fiber mode. */ > - return phydev->priv ? 0 : genphy_config_aneg(phydev); > + return (int)phydev->priv&PHYDEV_PRIV_FIBER ? 0 : genphy_config_aneg(phydev); spacing, and a long line. You might want to try checkpatch next time. Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 10, 2012 at 05:45:49PM +0200, Christophe Leroy wrote: > This patch adds proper handling of the buggy revision A2 of LXT973 phy, adding > precautions linked to ERRATA Item 4: > > Item 4: MDIO Interface and Repeated Polling > Problem: Repeated polling of odd-numbered registers via the MDIO interface > randomly returns the contents of the previous even register. > Implication: Managed applications may not obtain the correct register contents > when a particular register is monitored for device status. > Workaround: None. > Status: This erratum has been previously fixed (in rev A3) Hmm. Were did you get the hardware from? We have been using LXT973 in our products and the A2 was replaced by A3 in 2003. Best regards, Lutz
Le 10/09/2012 20:17, Lutz Jaenicke a écrit : > On Mon, Sep 10, 2012 at 05:45:49PM +0200, Christophe Leroy wrote: >> This patch adds proper handling of the buggy revision A2 of LXT973 phy, adding >> precautions linked to ERRATA Item 4: >> >> Item 4: MDIO Interface and Repeated Polling >> Problem: Repeated polling of odd-numbered registers via the MDIO interface >> randomly returns the contents of the previous even register. >> Implication: Managed applications may not obtain the correct register contents >> when a particular register is monitored for device status. >> Workaround: None. >> Status: This erratum has been previously fixed (in rev A3) > Hmm. Were did you get the hardware from? We have been using LXT973 in > our products and the A2 was replaced by A3 in 2003. > > Best regards, > Lutz They are custom boards that my company manufactures. Most boards manufactured before 2006 or 2007 have this buggy chip. Regards Christophe -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 22, 2012 at 06:21:38PM +0200, leroy christophe wrote: > > Le 10/09/2012 20:17, Lutz Jaenicke a écrit : > >On Mon, Sep 10, 2012 at 05:45:49PM +0200, Christophe Leroy wrote: > >>This patch adds proper handling of the buggy revision A2 of LXT973 phy, adding > >>precautions linked to ERRATA Item 4: > >> > >>Item 4: MDIO Interface and Repeated Polling > >>Problem: Repeated polling of odd-numbered registers via the MDIO interface > >> randomly returns the contents of the previous even register. > >>Implication: Managed applications may not obtain the correct register contents > >> when a particular register is monitored for device status. > >>Workaround: None. > >>Status: This erratum has been previously fixed (in rev A3) > >Hmm. Were did you get the hardware from? We have been using LXT973 in > >our products and the A2 was replaced by A3 in 2003. > > > >Best regards, > > Lutz > They are custom boards that my company manufactures. Most boards > manufactured before 2006 or 2007 have this buggy chip. Ok, so this problem indeed is related to (very) old hardware. I would not be sure whether the hack needed to work around this bug should be added one decade after the chip has been fixed. Best regards, Lutz PS.Yes, we have used the same workaround with such hardware, fortunately we only had a few first boards using A2 and the series production started just with A3 at that time.
diff -u linux-3.5-vanilla/drivers/net/phy/lxt.c linux-3.5/drivers/net/phy/lxt.c --- linux-3.5-vanilla/drivers/net/phy/lxt.c 2012-07-21 22:58:29.000000000 +0200 +++ linux-3.5/drivers/net/phy/lxt.c 2012-09-07 18:08:54.000000000 +0200 @@ -7,6 +7,10 @@ * * Copyright (c) 2004 Freescale Semiconductor, Inc. * + * Copyright (c) 2010 CSSI + * + * Added support for buggy LXT973 rev A2 + * * This program is free software; you can redistribute it and/or modify it * under the terms of the GNU General Public License as published by the * Free Software Foundation; either version 2 of the License, or (at your @@ -54,8 +58,12 @@ #define MII_LXT971_ISR 19 /* Interrupt Status Register */ /* register definitions for the 973 */ -#define MII_LXT973_PCR 16 /* Port Configuration Register */ +#define MII_LXT973_PCR 16 /* Port Configuration Register */ #define PCR_FIBER_SELECT 1 +#define MII_LXT973_SFR 27 /* Special Function Register */ + +#define PHYDEV_PRIV_FIBER 1 +#define PHYDEV_PRIV_REVA2 2 MODULE_DESCRIPTION("Intel LXT PHY driver"); MODULE_AUTHOR("Andy Fleming"); @@ -99,6 +107,9 @@ return err; } +/* register definitions for the 973 */ +#define MII_LXT973_PCR 16 /* Port Configuration Register */ +#define PCR_FIBER_SELECT 1 static int lxt971_ack_interrupt(struct phy_device *phydev) { @@ -122,9 +133,141 @@ return err; } +/* + * ERRATA on LXT973 + * + * Item 4: MDIO Interface and Repeated Polling + * Problem: Repeated polling of odd-numbered registers via the MDIO interface randomly returns the + * contents of the previous even register. + * Implication: Managed applications may not obtain the correct register contents when a particular + * register is monitored for device status. + * Workaround: None. + * Status: This erratum has been previously fixed (in rev A3) + * + */ + +static int lxt973a2_update_link(struct phy_device *phydev) +{ + int status; + int control; + int retry = 8; /* we try 8 times */ + + /* Do a fake read */ + status = phy_read(phydev, MII_BMSR); + + if (status < 0) + return status; + + control = phy_read(phydev, MII_BMCR); + if (control < 0) + return control; + + do { + /* Read link and autonegotiation status */ + status = phy_read(phydev, MII_BMSR); + } while (status>=0 && retry-- && status == control); + + if (status < 0) + return status; + + if ((status & BMSR_LSTATUS) == 0) + phydev->link = 0; + else + phydev->link = 1; + + return 0; +} + +int lxt973a2_read_status(struct phy_device *phydev) +{ + int adv; + int err; + int lpa; + int lpagb = 0; + + /* Update the link, but return if there was an error */ + err = lxt973a2_update_link(phydev); + if (err) + return err; + + if (AUTONEG_ENABLE == phydev->autoneg) { + int retry = 1; + + adv = phy_read(phydev, MII_ADVERTISE); + + if (adv < 0) + return adv; + + do { + lpa = phy_read(phydev, MII_LPA); + + if (lpa < 0) + return lpa; + + /* If both registers are equal, it is suspect but not impossible, hence a new try */ + } while (lpa == adv && retry--); + + lpa &= adv; + + phydev->speed = SPEED_10; + phydev->duplex = DUPLEX_HALF; + phydev->pause = phydev->asym_pause = 0; + + if (lpagb & (LPA_1000FULL | LPA_1000HALF)) { + phydev->speed = SPEED_1000; + + if (lpagb & LPA_1000FULL) + phydev->duplex = DUPLEX_FULL; + } else if (lpa & (LPA_100FULL | LPA_100HALF)) { + phydev->speed = SPEED_100; + + if (lpa & LPA_100FULL) + phydev->duplex = DUPLEX_FULL; + } else + if (lpa & LPA_10FULL) + phydev->duplex = DUPLEX_FULL; + + if (phydev->duplex == DUPLEX_FULL){ + phydev->pause = lpa & LPA_PAUSE_CAP ? 1 : 0; + phydev->asym_pause = lpa & LPA_PAUSE_ASYM ? 1 : 0; + } + } else { + int bmcr = phy_read(phydev, MII_BMCR); + + if (bmcr < 0) + return bmcr; + + if (bmcr & BMCR_FULLDPLX) + phydev->duplex = DUPLEX_FULL; + else + phydev->duplex = DUPLEX_HALF; + + if (bmcr & BMCR_SPEED1000) + phydev->speed = SPEED_1000; + else if (bmcr & BMCR_SPEED100) + phydev->speed = SPEED_100; + else + phydev->speed = SPEED_10; + + phydev->pause = phydev->asym_pause = 0; + } + + return 0; +} + +static int lxt973_read_status(struct phy_device *phydev) +{ + return (int)phydev->priv&PHYDEV_PRIV_REVA2 ? lxt973a2_read_status(phydev) : genphy_read_status(phydev); +} + static int lxt973_probe(struct phy_device *phydev) { int val = phy_read(phydev, MII_LXT973_PCR); + int priv = 0; + + phydev->priv = NULL; + + if (val<0) return val; if (val & PCR_FIBER_SELECT) { /* @@ -136,17 +279,24 @@ val &= ~BMCR_ANENABLE; phy_write(phydev, MII_BMCR, val); /* Remember that the port is in fiber mode. */ - phydev->priv = lxt973_probe; - } else { - phydev->priv = NULL; + priv |= PHYDEV_PRIV_FIBER; } + val = phy_read(phydev, MII_PHYSID2); + + if (val<0) return val; + + if ((val & 0xf) == 0) { /* rev A2 */ + dev_info(&phydev->dev, " LXT973 revision A2 has bugs\n"); + priv |= PHYDEV_PRIV_REVA2; + } + phydev->priv = (void*)priv; return 0; } static int lxt973_config_aneg(struct phy_device *phydev) { /* Do nothing if port is in fiber mode. */ - return phydev->priv ? 0 : genphy_config_aneg(phydev); + return (int)phydev->priv&PHYDEV_PRIV_FIBER ? 0 : genphy_config_aneg(phydev); } static struct phy_driver lxt970_driver = { @@ -184,7 +334,10 @@ .flags = 0, .probe = lxt973_probe, .config_aneg = lxt973_config_aneg, - .read_status = genphy_read_status, + .read_status = lxt973_read_status, + .suspend = genphy_suspend, + .resume = genphy_resume, + .isolate = genphy_isolate, .driver = { .owner = THIS_MODULE,}, };
This patch adds proper handling of the buggy revision A2 of LXT973 phy, adding precautions linked to ERRATA Item 4: Item 4: MDIO Interface and Repeated Polling Problem: Repeated polling of odd-numbered registers via the MDIO interface randomly returns the contents of the previous even register. Implication: Managed applications may not obtain the correct register contents when a particular register is monitored for device status. Workaround: None. Status: This erratum has been previously fixed (in rev A3) Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html