| Message ID | 20211129195823.11766-7-kabel@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mv88e6xxx fixes (mainly 88E6393X family) | expand |
On Mon, Nov 29, 2021 at 08:58:23PM +0100, Marek Behún wrote: > static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, > - u16 status, u16 lpa, > + u16 ctrl, u16 status, u16 lpa, > struct phylink_link_state *state) > { > + state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK); > + > if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) { > - state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK); > + state->an_complete = !!(ctrl & BMCR_ANENABLE); I think I'd much rather report the value of BMSR_ANEGCAPABLE - since an_complete controls the BMSR_ANEGCAPABLE bit in the emulated PHY that userspace sees. Otherwise, an_complete is not used. state->link is the key that phylink uses to know whether it can trust the status being reported.
On Mon, 29 Nov 2021 23:14:17 +0000 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Mon, Nov 29, 2021 at 08:58:23PM +0100, Marek Behún wrote: > > static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, > > - u16 status, u16 lpa, > > + u16 ctrl, u16 status, u16 lpa, > > struct phylink_link_state *state) > > { > > + state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK); > > + > > if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) { > > - state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK); > > + state->an_complete = !!(ctrl & BMCR_ANENABLE); > > I think I'd much rather report the value of BMSR_ANEGCAPABLE - since > an_complete controls the BMSR_ANEGCAPABLE bit in the emulated PHY > that userspace sees. Otherwise, an_complete is not used. > > state->link is the key that phylink uses to know whether it can > trust the status being reported. Isn't BMSR_ANEGCAPABLE set to 1 even if aneg is disabled in BMCR? I will test tomorrow. The reason why I used BMCR_ANENABLE is that if we don't enable AN, the PHY will report SPD_DPL_VALID if link is up. But clearly AN is not completed in that case, because it was never enabled in the first place. Marek
On Tue, 30 Nov 2021 01:18:12 +0100 Marek Behún <kabel@kernel.org> wrote: > On Mon, 29 Nov 2021 23:14:17 +0000 > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > > > On Mon, Nov 29, 2021 at 08:58:23PM +0100, Marek Behún wrote: > > > static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, > > > - u16 status, u16 lpa, > > > + u16 ctrl, u16 status, u16 lpa, > > > struct phylink_link_state *state) > > > { > > > + state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK); > > > + > > > if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) { > > > - state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK); > > > + state->an_complete = !!(ctrl & BMCR_ANENABLE); > > > > I think I'd much rather report the value of BMSR_ANEGCAPABLE - since > > an_complete controls the BMSR_ANEGCAPABLE bit in the emulated PHY > > that userspace sees. Otherwise, an_complete is not used. > > > > state->link is the key that phylink uses to know whether it can > > trust the status being reported. > > Isn't BMSR_ANEGCAPABLE set to 1 even if aneg is disabled in BMCR? > I will test tomorrow. > > The reason why I used BMCR_ANENABLE is that if we don't enable AN, the > PHY will report SPD_DPL_VALID if link is up. But clearly AN is not > completed in that case, because it was never enabled in the first place. Seems that BMSR_ANEGCAPABLE is set even if AN is disabled in BMCR. If phylink_autoneg_inband() is not true mv88e6*_serdes_pcs_config(), then we aren't enabling AN. But in this case SPD_DPL_VALID is always 1, so if we use, as you say, an_complete=BMSR_ANEGCAPABLE in mv88e6xxx_serdes_pcs_get_state(), we will always set an_complete=true, even if AN wasn't enabled. I was under the impression that state->an_complete should only be set to true if AN is enabled. Marek
On Tue, Nov 30, 2021 at 05:09:11PM +0100, Marek Behún wrote: > Seems that BMSR_ANEGCAPABLE is set even if AN is disabled in BMCR. Hmm, that behaviour goes against 22.2.4.2.10: A PHY shall return a value of zero in bit 1.5 if Auto-Negotiation is disabled by clearing bit 0.12. A PHY shall also return a value of zero in bit 1.5 if it lacks the ability to perform Auto-Negotiation. > I was under the impression that > state->an_complete > should only be set to true if AN is enabled. Yes - however as you've stated, the PHY doesn't follow 802.3 behaviour so I guess we should make the emulation appear compliant by fixing it like this.
On Tue, 30 Nov 2021 16:15:50 +0000 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Tue, Nov 30, 2021 at 05:09:11PM +0100, Marek Behún wrote: > > Seems that BMSR_ANEGCAPABLE is set even if AN is disabled in BMCR. > > Hmm, that behaviour goes against 22.2.4.2.10: > > A PHY shall return a value of zero in bit 1.5 if Auto-Negotiation is > disabled by clearing bit 0.12. A PHY shall also return a value of zero > in bit 1.5 if it lacks the ability to perform Auto-Negotiation. > > > I was under the impression that > > state->an_complete > > should only be set to true if AN is enabled. > > Yes - however as you've stated, the PHY doesn't follow 802.3 behaviour > so I guess we should make the emulation appear compliant by fixing it > like this. OK, I will use BMCR_ANENABLE and add a comment explaining that we can't use BMSR_ANEGCAPABLE because the PHY violates standard. Would that be okay?
On Tue, Nov 30, 2021 at 05:18:59PM +0100, Marek Behún wrote: > On Tue, 30 Nov 2021 16:15:50 +0000 > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > > > On Tue, Nov 30, 2021 at 05:09:11PM +0100, Marek Behún wrote: > > > Seems that BMSR_ANEGCAPABLE is set even if AN is disabled in BMCR. > > > > Hmm, that behaviour goes against 22.2.4.2.10: > > > > A PHY shall return a value of zero in bit 1.5 if Auto-Negotiation is > > disabled by clearing bit 0.12. A PHY shall also return a value of zero > > in bit 1.5 if it lacks the ability to perform Auto-Negotiation. > > > > > I was under the impression that > > > state->an_complete > > > should only be set to true if AN is enabled. > > > > Yes - however as you've stated, the PHY doesn't follow 802.3 behaviour > > so I guess we should make the emulation appear compliant by fixing it > > like this. > > OK, I will use BMCR_ANENABLE and add a comment explaining that we can't > use BMSR_ANEGCAPABLE because the PHY violates standard. Would that be > okay? Yes, thanks.
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c index d297131afe21..a145598905a5 100644 --- a/drivers/net/dsa/mv88e6xxx/serdes.c +++ b/drivers/net/dsa/mv88e6xxx/serdes.c @@ -50,11 +50,13 @@ static int mv88e6390_serdes_write(struct mv88e6xxx_chip *chip, } static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, - u16 status, u16 lpa, + u16 ctrl, u16 status, u16 lpa, struct phylink_link_state *state) { + state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK); + if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) { - state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK); + state->an_complete = !!(ctrl & BMCR_ANENABLE); state->duplex = status & MV88E6390_SGMII_PHY_STATUS_DUPLEX_FULL ? DUPLEX_FULL : DUPLEX_HALF; @@ -81,6 +83,17 @@ static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, dev_err(chip->dev, "invalid PHY speed\n"); return -EINVAL; } + } else if (state->link && + state->interface != PHY_INTERFACE_MODE_SGMII) { + /* The Spped and Duplex Resolved register is always 1 when AN is + * disabled. If it is not set, and link is up, it means that the + * SerDes PHY AN bypass feature was invoked. + */ + state->duplex = DUPLEX_FULL; + if (state->interface == PHY_INTERFACE_MODE_2500BASEX) + state->speed = SPEED_2500; + else + state->speed = SPEED_1000; } else { state->link = false; } @@ -168,9 +181,15 @@ int mv88e6352_serdes_pcs_config(struct mv88e6xxx_chip *chip, int port, int mv88e6352_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, int port, int lane, struct phylink_link_state *state) { - u16 lpa, status; + u16 lpa, status, ctrl; int err; + err = mv88e6352_serdes_read(chip, MII_BMCR, &ctrl); + if (err) { + dev_err(chip->dev, "can't read Serdes PHY control: %d\n", err); + return err; + } + err = mv88e6352_serdes_read(chip, 0x11, &status); if (err) { dev_err(chip->dev, "can't read Serdes PHY status: %d\n", err); @@ -183,7 +202,7 @@ int mv88e6352_serdes_pcs_get_state(struct mv88e6xxx_chip *chip, int port, return err; } - return mv88e6xxx_serdes_pcs_get_state(chip, status, lpa, state); + return mv88e6xxx_serdes_pcs_get_state(chip, ctrl, status, lpa, state); } int mv88e6352_serdes_pcs_an_restart(struct mv88e6xxx_chip *chip, int port, @@ -883,9 +902,16 @@ int mv88e6390_serdes_pcs_config(struct mv88e6xxx_chip *chip, int port, static int mv88e6390_serdes_pcs_get_state_sgmii(struct mv88e6xxx_chip *chip, int port, int lane, struct phylink_link_state *state) { - u16 lpa, status; + u16 lpa, status, ctrl; int err; + err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS, + MV88E6390_SGMII_BMCR, &ctrl); + if (err) { + dev_err(chip->dev, "can't read Serdes PHY control: %d\n", err); + return err; + } + err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS, MV88E6390_SGMII_PHY_STATUS, &status); if (err) { @@ -900,7 +926,7 @@ static int mv88e6390_serdes_pcs_get_state_sgmii(struct mv88e6xxx_chip *chip, return err; } - return mv88e6xxx_serdes_pcs_get_state(chip, status, lpa, state); + return mv88e6xxx_serdes_pcs_get_state(chip, ctrl, status, lpa, state); } static int mv88e6390_serdes_pcs_get_state_10g(struct mv88e6xxx_chip *chip,
Function mv88e6xxx_serdes_pcs_get_state() currently does not report link up if AN is enabled, Link bit is set, but Speed and Duplex Resolved bit is not set, which testing shows is the case for when auto-negotiation was bypassed (we have enabled AN but link partner does not). An example of such link partner is Marvell 88X3310 PHY, when put into the mode where host interface changes between 10gbase-r, 5gbase-r, 2500base-x and sgmii according to copper speed. The 88X3310 does not enable AN in 2500base-x, and so SerDes on mv88e6xxx currently does not link with it. Fix this. Fixes: a5a6858b793f ("net: dsa: mv88e6xxx: extend phylink to Serdes PHYs") Signed-off-by: Marek Behún <kabel@kernel.org> --- drivers/net/dsa/mv88e6xxx/serdes.c | 38 +++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 6 deletions(-)