* [PATCH net 1/3] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode
@ 2021-11-10 4:10 Marek Behún
2021-11-10 4:10 ` [PATCH net 2/3] net: dsa: mv88e6xxx: Fix reading sgmii link status register Marek Behún
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Marek Behún @ 2021-11-10 4:10 UTC (permalink / raw)
To: netdev, Andrew Lunn
Cc: Jakub Kicinski, David Miller, Russell King, Marek Behún
Commit 64d47d50be7a ("net: dsa: mv88e6xxx: configure interface settings
in mac_config") removed forcing of speed and duplex from
mv88e6xxx_mac_config(), where the link is forced down, and left it only
in mv88e6xxx_mac_link_up(), by which time link is unforced.
It seems that in 2500base-x (at least on 88E6190), if the link is not
forced down, the forcing of new settings for speed and duplex doesn't
take.
Fix this by forcing link down.
Fixes: 64d47d50be7a ("net: dsa: mv88e6xxx: configure interface settings in mac_config")
Signed-off-by: Marek Behún <kabel@kernel.org>
---
drivers/net/dsa/mv88e6xxx/chip.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index f00cbf5753b9..ddb13cecb3ac 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -785,12 +785,17 @@ static void mv88e6xxx_mac_link_up(struct dsa_switch *ds, int port,
if ((!mv88e6xxx_phy_is_internal(ds, port) &&
!mv88e6xxx_port_ppu_updates(chip, port)) ||
mode == MLO_AN_FIXED) {
- /* FIXME: for an automedia port, should we force the link
- * down here - what if the link comes up due to "other" media
- * while we're bringing the port up, how is the exclusivity
- * handled in the Marvell hardware? E.g. port 2 on 88E6390
- * shared between internal PHY and Serdes.
+ /* FIXME: we need to force the link down here, otherwise the
+ * forcing of link speed and duplex by .port_set_speed_duplex()
+ * doesn't work for some modes.
+ * But what if the link comes up due to "other" media while
+ * we're bringing the port up, how is the exclusivity handled in
+ * the Marvell hardware? E.g. port 2 on 88E6390 shared between
+ * internal PHY and Serdes.
*/
+ if (ops->port_sync_link)
+ err = ops->port_sync_link(chip, port, mode, false);
+
err = mv88e6xxx_serdes_pcs_link_up(chip, port, mode, speed,
duplex);
if (err)
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net 2/3] net: dsa: mv88e6xxx: Fix reading sgmii link status register
2021-11-10 4:10 [PATCH net 1/3] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode Marek Behún
@ 2021-11-10 4:10 ` Marek Behún
2021-11-10 4:10 ` [PATCH net 3/3] net: dsa: mv88e6xxx: Link in pcs_get_state() even if LP has AN disabled Marek Behún
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Marek Behún @ 2021-11-10 4:10 UTC (permalink / raw)
To: netdev, Andrew Lunn
Cc: Jakub Kicinski, David Miller, Russell King, Marek Behún
The documentation says this about the SerDes PHY register 4.2001.2:
This register bit indicates when link was lost since the last
read. For the current link status, read this register
back-to-back.
Thus we need to read it twice to get the current value of the register.
The wrong value is read when phylink requests change from sgmii to
2500base-x mode, and link won't come up. This fixes the bug.
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 | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 6ea003678798..bc198ef06745 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -1041,15 +1041,25 @@ static void mv88e6390_serdes_irq_link_sgmii(struct mv88e6xxx_chip *chip,
u16 bmsr;
int err;
- /* If the link has dropped, we want to know about it. */
+ /* If the link has dropped, we want to know about it.
+ * This register bit indicates when link was lost since the last read.
+ * We need to read it twice to get the current value.
+ */
err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
MV88E6390_SGMII_BMSR, &bmsr);
- if (err) {
- dev_err(chip->dev, "can't read Serdes BMSR: %d\n", err);
- return;
- }
+ if (err)
+ goto err;
+
+ err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
+ MV88E6390_SGMII_BMSR, &bmsr);
+ if (err)
+ goto err;
dsa_port_phylink_mac_change(chip->ds, port, !!(bmsr & BMSR_LSTATUS));
+
+ return;
+err:
+ dev_err(chip->dev, "can't read Serdes BMSR: %d\n", err);
}
static void mv88e6393x_serdes_irq_link_10g(struct mv88e6xxx_chip *chip,
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net 3/3] net: dsa: mv88e6xxx: Link in pcs_get_state() even if LP has AN disabled
2021-11-10 4:10 [PATCH net 1/3] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode Marek Behún
2021-11-10 4:10 ` [PATCH net 2/3] net: dsa: mv88e6xxx: Fix reading sgmii link status register Marek Behún
@ 2021-11-10 4:10 ` Marek Behún
2021-11-10 8:01 ` [PATCH net 1/3] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode Russell King (Oracle)
2021-11-10 13:18 ` Andrew Lunn
3 siblings, 0 replies; 6+ messages in thread
From: Marek Behún @ 2021-11-10 4:10 UTC (permalink / raw)
To: netdev, Andrew Lunn
Cc: Jakub Kicinski, David Miller, Russell King, Marek Behún
Function mv88e6xxx_serdes_pcs_get_state() currently does not report
link up if AN is not complete. This is in contrast to for example the
mvneta's mvneta_mac_pcs_get_state() implementation, where link is simply
taken from link bit.
For 1000base-x and 2500base-x modes, it is possible that the link
partner has autonegotiation disabled. In this case we get zero in the
SPD_DPL_VALID and we won't link, even if we can.
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 | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index bc198ef06745..dd3fba7aab99 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -53,8 +53,11 @@ static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip,
u16 status, u16 lpa,
struct phylink_link_state *state)
{
- if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) {
- state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
+ state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
+ state->an_complete = !!(status &
+ MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID);
+
+ if (state->an_complete) {
state->duplex = status &
MV88E6390_SGMII_PHY_STATUS_DUPLEX_FULL ?
DUPLEX_FULL : DUPLEX_HALF;
@@ -81,8 +84,13 @@ static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip,
dev_err(chip->dev, "invalid PHY speed\n");
return -EINVAL;
}
- } else {
- state->link = false;
+ } else if (state->link &&
+ state->interface != PHY_INTERFACE_MODE_SGMII) {
+ state->duplex = DUPLEX_FULL;
+ if (state->interface == PHY_INTERFACE_MODE_2500BASEX)
+ state->speed = SPEED_2500;
+ else
+ state->speed = SPEED_1000;
}
if (state->interface == PHY_INTERFACE_MODE_2500BASEX)
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net 1/3] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode
2021-11-10 4:10 [PATCH net 1/3] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode Marek Behún
2021-11-10 4:10 ` [PATCH net 2/3] net: dsa: mv88e6xxx: Fix reading sgmii link status register Marek Behún
2021-11-10 4:10 ` [PATCH net 3/3] net: dsa: mv88e6xxx: Link in pcs_get_state() even if LP has AN disabled Marek Behún
@ 2021-11-10 8:01 ` Russell King (Oracle)
2021-11-10 13:18 ` Andrew Lunn
3 siblings, 0 replies; 6+ messages in thread
From: Russell King (Oracle) @ 2021-11-10 8:01 UTC (permalink / raw)
To: Marek Behún; +Cc: netdev, Andrew Lunn, Jakub Kicinski, David Miller
Please wait before applying; I have comments on these patches, but I
won't have time to provide those comments for a couple of days.
Thanks.
On Wed, Nov 10, 2021 at 05:10:08AM +0100, Marek Behún wrote:
> Commit 64d47d50be7a ("net: dsa: mv88e6xxx: configure interface settings
> in mac_config") removed forcing of speed and duplex from
> mv88e6xxx_mac_config(), where the link is forced down, and left it only
> in mv88e6xxx_mac_link_up(), by which time link is unforced.
>
> It seems that in 2500base-x (at least on 88E6190), if the link is not
> forced down, the forcing of new settings for speed and duplex doesn't
> take.
>
> Fix this by forcing link down.
>
> Fixes: 64d47d50be7a ("net: dsa: mv88e6xxx: configure interface settings in mac_config")
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
> drivers/net/dsa/mv88e6xxx/chip.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index f00cbf5753b9..ddb13cecb3ac 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -785,12 +785,17 @@ static void mv88e6xxx_mac_link_up(struct dsa_switch *ds, int port,
> if ((!mv88e6xxx_phy_is_internal(ds, port) &&
> !mv88e6xxx_port_ppu_updates(chip, port)) ||
> mode == MLO_AN_FIXED) {
> - /* FIXME: for an automedia port, should we force the link
> - * down here - what if the link comes up due to "other" media
> - * while we're bringing the port up, how is the exclusivity
> - * handled in the Marvell hardware? E.g. port 2 on 88E6390
> - * shared between internal PHY and Serdes.
> + /* FIXME: we need to force the link down here, otherwise the
> + * forcing of link speed and duplex by .port_set_speed_duplex()
> + * doesn't work for some modes.
> + * But what if the link comes up due to "other" media while
> + * we're bringing the port up, how is the exclusivity handled in
> + * the Marvell hardware? E.g. port 2 on 88E6390 shared between
> + * internal PHY and Serdes.
> */
> + if (ops->port_sync_link)
> + err = ops->port_sync_link(chip, port, mode, false);
> +
> err = mv88e6xxx_serdes_pcs_link_up(chip, port, mode, speed,
> duplex);
> if (err)
> --
> 2.32.0
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 1/3] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode
2021-11-10 4:10 [PATCH net 1/3] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode Marek Behún
` (2 preceding siblings ...)
2021-11-10 8:01 ` [PATCH net 1/3] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode Russell King (Oracle)
@ 2021-11-10 13:18 ` Andrew Lunn
2021-11-10 14:07 ` Marek Behún
3 siblings, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2021-11-10 13:18 UTC (permalink / raw)
To: Marek Behún; +Cc: netdev, Jakub Kicinski, David Miller, Russell King
Hi Marek
Please always include a patch 0/X for a patchset explaining what the
patches as a whole do. The text should then be used as the merge
commit for the patchset.
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 1/3] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode
2021-11-10 13:18 ` Andrew Lunn
@ 2021-11-10 14:07 ` Marek Behún
0 siblings, 0 replies; 6+ messages in thread
From: Marek Behún @ 2021-11-10 14:07 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev, Jakub Kicinski, David Miller, Russell King
On Wed, 10 Nov 2021 14:18:10 +0100
Andrew Lunn <andrew@lunn.ch> wrote:
> Hi Marek
>
> Please always include a patch 0/X for a patchset explaining what the
> patches as a whole do. The text should then be used as the merge
> commit for the patchset.
>
> Andrew
OK, but in this case I will wait till Russell gives his comments before
sending v2.
Marek
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-10 14:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-10 4:10 [PATCH net 1/3] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode Marek Behún
2021-11-10 4:10 ` [PATCH net 2/3] net: dsa: mv88e6xxx: Fix reading sgmii link status register Marek Behún
2021-11-10 4:10 ` [PATCH net 3/3] net: dsa: mv88e6xxx: Link in pcs_get_state() even if LP has AN disabled Marek Behún
2021-11-10 8:01 ` [PATCH net 1/3] net: dsa: mv88e6xxx: Fix forcing speed & duplex when changing to 2500base-x mode Russell King (Oracle)
2021-11-10 13:18 ` Andrew Lunn
2021-11-10 14:07 ` Marek Behún
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.