| Message ID | 20241028203804.41689-2-gerhard@engleder-embedded.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Support loopback mode speed selection | expand |
On 10/28/24 1:38 PM, Gerhard Engleder wrote: > PHY drivers support loopback mode, but it is not possible to select the > speed of the loopback mode. The speed is chosen by the set_loopback() > operation of the PHY driver. Same is valid for genphy_loopback(). > > There are PHYs that support loopback with different speeds. Extend > set_loopback() to make loopback speed selection possible. > > Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com> > --- > drivers/net/phy/adin1100.c | 5 ++++- > drivers/net/phy/dp83867.c | 5 ++++- > drivers/net/phy/marvell.c | 8 +++++++- > drivers/net/phy/mxl-gpy.c | 11 +++++++---- > drivers/net/phy/phy-c45.c | 5 ++++- > drivers/net/phy/phy_device.c | 12 +++++++++--- > drivers/net/phy/xilinx_gmii2rgmii.c | 7 ++++--- > include/linux/phy.h | 16 ++++++++++++---- > 8 files changed, 51 insertions(+), 18 deletions(-) > > diff --git a/drivers/net/phy/adin1100.c b/drivers/net/phy/adin1100.c > index 85f910e2d4fb..dd8cce925668 100644 > --- a/drivers/net/phy/adin1100.c > +++ b/drivers/net/phy/adin1100.c > @@ -215,8 +215,11 @@ static int adin_resume(struct phy_device *phydev) > return adin_set_powerdown_mode(phydev, false); > } > > -static int adin_set_loopback(struct phy_device *phydev, bool enable) > +static int adin_set_loopback(struct phy_device *phydev, bool enable, int speed) > { > + if (enable && speed) > + return -EOPNOTSUPP; > + > if (enable) > return phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_10T1L_CTRL, > BMCR_LOOPBACK); > diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c > index 4120385c5a79..b10ad482d566 100644 > --- a/drivers/net/phy/dp83867.c > +++ b/drivers/net/phy/dp83867.c > @@ -1009,8 +1009,11 @@ static void dp83867_link_change_notify(struct phy_device *phydev) > } > } > > -static int dp83867_loopback(struct phy_device *phydev, bool enable) > +static int dp83867_loopback(struct phy_device *phydev, bool enable, int speed) > { > + if (enable && speed) > + return -EOPNOTSUPP; > + > return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, > enable ? BMCR_LOOPBACK : 0); > } > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c > index 28aec37acd2c..c70c5c23b339 100644 > --- a/drivers/net/phy/marvell.c > +++ b/drivers/net/phy/marvell.c > @@ -2095,13 +2095,19 @@ static void marvell_get_stats_simple(struct phy_device *phydev, > data[i] = marvell_get_stat_simple(phydev, i); > } > > -static int m88e1510_loopback(struct phy_device *phydev, bool enable) > +static int m88e1510_loopback(struct phy_device *phydev, bool enable, int speed) > { > int err; > > if (enable) { > u16 bmcr_ctl, mscr2_ctl = 0; > > + if (speed == SPEED_10 || speed == SPEED_100 || > + speed == SPEED_1000) > + phydev->speed = speed; > + else if (speed) > + return -EINVAL; > + > bmcr_ctl = mii_bmcr_encode_fixed(phydev->speed, phydev->duplex); > > err = phy_write(phydev, MII_BMCR, bmcr_ctl); > diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c > index db3c1f72b407..9b863b18a043 100644 > --- a/drivers/net/phy/mxl-gpy.c > +++ b/drivers/net/phy/mxl-gpy.c > @@ -813,7 +813,7 @@ static void gpy_get_wol(struct phy_device *phydev, > wol->wolopts = priv->wolopts; > } > > -static int gpy_loopback(struct phy_device *phydev, bool enable) > +static int gpy_loopback(struct phy_device *phydev, bool enable, int speed) > { > struct gpy_priv *priv = phydev->priv; > u16 set = 0; > @@ -822,6 +822,9 @@ static int gpy_loopback(struct phy_device *phydev, bool enable) > if (enable) { > u64 now = get_jiffies_64(); > > + if (speed) > + return -EOPNOTSUPP; > + > /* wait until 3 seconds from last disable */ > if (time_before64(now, priv->lb_dis_to)) > msleep(jiffies64_to_msecs(priv->lb_dis_to - now)); > @@ -845,15 +848,15 @@ static int gpy_loopback(struct phy_device *phydev, bool enable) > return 0; > } > > -static int gpy115_loopback(struct phy_device *phydev, bool enable) > +static int gpy115_loopback(struct phy_device *phydev, bool enable, int speed) > { > struct gpy_priv *priv = phydev->priv; > > if (enable) > - return gpy_loopback(phydev, enable); > + return gpy_loopback(phydev, enable, speed); > > if (priv->fw_minor > 0x76) > - return gpy_loopback(phydev, 0); > + return gpy_loopback(phydev, 0, 0); > > return genphy_soft_reset(phydev); > } > diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c > index 5695935fdce9..3399028f0e92 100644 > --- a/drivers/net/phy/phy-c45.c > +++ b/drivers/net/phy/phy-c45.c > @@ -1230,8 +1230,11 @@ int gen10g_config_aneg(struct phy_device *phydev) > } > EXPORT_SYMBOL_GPL(gen10g_config_aneg); > > -int genphy_c45_loopback(struct phy_device *phydev, bool enable) > +int genphy_c45_loopback(struct phy_device *phydev, bool enable, int speed) > { > + if (enable && speed) > + return -EOPNOTSUPP; > + > return phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, > MDIO_PCS_CTRL1_LOOPBACK, > enable ? MDIO_PCS_CTRL1_LOOPBACK : 0); > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 563497a3274c..1c34cb947588 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -2112,9 +2112,9 @@ int phy_loopback(struct phy_device *phydev, bool enable) > } > > if (phydev->drv->set_loopback) > - ret = phydev->drv->set_loopback(phydev, enable); > + ret = phydev->drv->set_loopback(phydev, enable, 0); > else > - ret = genphy_loopback(phydev, enable); > + ret = genphy_loopback(phydev, enable, 0); > > if (ret) > goto out; > @@ -2906,12 +2906,18 @@ int genphy_resume(struct phy_device *phydev) > } > EXPORT_SYMBOL(genphy_resume); > > -int genphy_loopback(struct phy_device *phydev, bool enable) > +int genphy_loopback(struct phy_device *phydev, bool enable, int speed) > { > if (enable) { > u16 ctl = BMCR_LOOPBACK; > int ret, val; > > + if (speed == SPEED_10 || speed == SPEED_100 || > + speed == SPEED_1000) > + phydev->speed = speed; Why is this limited to 1000? Shouldn't the max loopback speed be limited by max hardware speed? We currently have definitions going up to SPEED_800000 so some devices should support higher than 1000. > + else if (speed) > + return -EINVAL; > + > ctl |= mii_bmcr_encode_fixed(phydev->speed, phydev->duplex); > > phy_modify(phydev, MII_BMCR, ~0, ctl); > diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c > index 7c51daecf18e..2024d8ef36d9 100644 > --- a/drivers/net/phy/xilinx_gmii2rgmii.c > +++ b/drivers/net/phy/xilinx_gmii2rgmii.c > @@ -64,15 +64,16 @@ static int xgmiitorgmii_read_status(struct phy_device *phydev) > return 0; > } > > -static int xgmiitorgmii_set_loopback(struct phy_device *phydev, bool enable) > +static int xgmiitorgmii_set_loopback(struct phy_device *phydev, bool enable, > + int speed) > { > struct gmii2rgmii *priv = mdiodev_get_drvdata(&phydev->mdio); > int err; > > if (priv->phy_drv->set_loopback) > - err = priv->phy_drv->set_loopback(phydev, enable); > + err = priv->phy_drv->set_loopback(phydev, enable, speed); > else > - err = genphy_loopback(phydev, enable); > + err = genphy_loopback(phydev, enable, speed); > if (err < 0) > return err; > > diff --git a/include/linux/phy.h b/include/linux/phy.h > index bf0eb4e5d35c..83b705cfbf46 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -1107,8 +1107,16 @@ struct phy_driver { > int (*set_tunable)(struct phy_device *dev, > struct ethtool_tunable *tuna, > const void *data); > - /** @set_loopback: Set the loopback mood of the PHY */ > - int (*set_loopback)(struct phy_device *dev, bool enable); > + /** > + * @set_loopback: Set the loopback mode of the PHY > + * enable selects if the loopback mode is enabled or disabled. If the > + * loopback mode is enabled, then the speed of the loopback mode can be > + * requested with the speed argument. If the speed argument is zero, > + * then any speed can be selected. If the speed argument is > 0, then > + * this speed shall be selected for the loopback mode or EOPNOTSUPP > + * shall be returned if speed selection is not supported. > + */ > + int (*set_loopback)(struct phy_device *dev, bool enable, int speed); > /** @get_sqi: Get the signal quality indication */ > int (*get_sqi)(struct phy_device *dev); > /** @get_sqi_max: Get the maximum signal quality indication */ > @@ -1895,7 +1903,7 @@ int genphy_read_status(struct phy_device *phydev); > int genphy_read_master_slave(struct phy_device *phydev); > int genphy_suspend(struct phy_device *phydev); > int genphy_resume(struct phy_device *phydev); > -int genphy_loopback(struct phy_device *phydev, bool enable); > +int genphy_loopback(struct phy_device *phydev, bool enable, int speed); > int genphy_soft_reset(struct phy_device *phydev); > irqreturn_t genphy_handle_interrupt_no_ack(struct phy_device *phydev); > > @@ -1937,7 +1945,7 @@ int genphy_c45_pma_baset1_read_master_slave(struct phy_device *phydev); > int genphy_c45_read_status(struct phy_device *phydev); > int genphy_c45_baset1_read_status(struct phy_device *phydev); > int genphy_c45_config_aneg(struct phy_device *phydev); > -int genphy_c45_loopback(struct phy_device *phydev, bool enable); > +int genphy_c45_loopback(struct phy_device *phydev, bool enable, int speed); > int genphy_c45_pma_resume(struct phy_device *phydev); > int genphy_c45_pma_suspend(struct phy_device *phydev); > int genphy_c45_fast_retrain(struct phy_device *phydev, bool enable); Why is speed defined as an int? It can never be negative, I normally see it defined as u32.
> - int (*set_loopback)(struct phy_device *dev, bool enable); > + /** > + * @set_loopback: Set the loopback mode of the PHY > + * enable selects if the loopback mode is enabled or disabled. If the > + * loopback mode is enabled, then the speed of the loopback mode can be > + * requested with the speed argument. If the speed argument is zero, > + * then any speed can be selected. If the speed argument is > 0, then > + * this speed shall be selected for the loopback mode or EOPNOTSUPP > + * shall be returned if speed selection is not supported. > + */ I think we need to take a step back here and think about how this currently works. The MAC and the PHY probably need to agree on the speed. Does an RGMII MAC sending at 10Mbps work against a PHY expecting 1000Mbps? The MAC is repeating the symbols 100 times to fill the channel, so it might? But does a PHY expecting 100 repeats work when actually passed a signal instance of a symbol? What about an SGMII link, 10Mbps one side, 1G the other? What about 1000BaseX vs 2500BaseX? I've never thought about how this actually works today. My _guess_ would be, you first either have the link perform auto-neg, or you force a speed using ethtool. In both cases, the adjust_link callback of phylib is called, which for phylink will result in the mac_link_up callback being called with the current link mode, etc. So the PHY and the MAC know the properties of the link between themselves. > + int (*set_loopback)(struct phy_device *dev, bool enable, int speed); You say: > + If the speed argument is zero, > + * then any speed can be selected. How do the PHY and the MAC agree on the speed? Are you assuming the adjust_link/mac_link_up will be called? Phylink has the hard assumption the link will go down before coming up at another speed. Is that guaranteed here? What happens when you pass a specific speed? How does the MAC get to know? I think we need to keep as close as possible to the current scheme. The problem is >= 1G, where the core will now enable autoneg even for forced speeds. If there is a link partner, auto-neg should succeed and adjust_link/mac_link_up will be called, and so the PHY and MAC will already be in sync when loopback is enabled. If there is no link partner, auto-neg will fail. In this case, we need set_loopback to trigger link up so that the PHY and MAC get in sync. And then it is disabled, the link needs to go down again. Andrew
On 29.10.24 03:45, Andrew Lunn wrote: >> - int (*set_loopback)(struct phy_device *dev, bool enable); >> + /** >> + * @set_loopback: Set the loopback mode of the PHY >> + * enable selects if the loopback mode is enabled or disabled. If the >> + * loopback mode is enabled, then the speed of the loopback mode can be >> + * requested with the speed argument. If the speed argument is zero, >> + * then any speed can be selected. If the speed argument is > 0, then >> + * this speed shall be selected for the loopback mode or EOPNOTSUPP >> + * shall be returned if speed selection is not supported. >> + */ > > I think we need to take a step back here and think about how this > currently works. > > The MAC and the PHY probably need to agree on the speed. Does an RGMII > MAC sending at 10Mbps work against a PHY expecting 1000Mbps? The MAC > is repeating the symbols 100 times to fill the channel, so it might? > But does a PHY expecting 100 repeats work when actually passed a > signal instance of a symbol? What about an SGMII link, 10Mbps one > side, 1G the other? What about 1000BaseX vs 2500BaseX? > > I've never thought about how this actually works today. My _guess_ > would be, you first either have the link perform auto-neg, or you > force a speed using ethtool. In both cases, the adjust_link callback > of phylib is called, which for phylink will result in the mac_link_up > callback being called with the current link mode, etc. So the PHY and > the MAC know the properties of the link between themselves. > >> + int (*set_loopback)(struct phy_device *dev, bool enable, int speed); > > You say: > >> + If the speed argument is zero, >> + * then any speed can be selected. > > How do the PHY and the MAC agree on the speed? Are you assuming the > adjust_link/mac_link_up will be called? Phylink has the hard > assumption the link will go down before coming up at another speed. Is > that guaranteed here? Yes, the PHY and the MAC has to agree on the speed. In my opinion adjust_link/mac_link_up shall be called like in normal operation without loopback. The question is when? adjust_link/mac_link_up could be called after phy_loopback() returns with success or before phy_loopback() returns. I would prefer that a successful return of phy_loopback() guarantees that the loopback mode is active and signaled. This would eliminate the need to wait for the loopback, which would be again error prone. It is not guaranteed that the link goes down before its coming up at another speed. That needs to be fixed. If the link is up and the speed matches, then the link shall stay up. If the link is up and the speed does not match, then the link shall first go down and then come up after the loopback is established. If the link is down, then the link come up after the loopback is established. Would that be ok? Or should the link always go down before the loopback is switched on? > What happens when you pass a specific speed? How does the MAC get to > know? I agree that adjust_link/mac_link_up shall still be used to let the MAC know the speed. > I think we need to keep as close as possible to the current > scheme. The problem is >= 1G, where the core will now enable autoneg > even for forced speeds. If there is a link partner, auto-neg should > succeed and adjust_link/mac_link_up will be called, and so the PHY and > MAC will already be in sync when loopback is enabled. If there is no > link partner, auto-neg will fail. In this case, we need set_loopback > to trigger link up so that the PHY and MAC get in sync. And then it is > disabled, the link needs to go down again. I would like to try it. Would be great to hear your opinion about when adjust_link/mac_link_up should be called in case of successful set_loopback() and if the link shall always go down before loopback. Thank you for your feedback! Gerhard
On 29.10.24 00:23, Lee Trager wrote: > On 10/28/24 1:38 PM, Gerhard Engleder wrote: >> -int genphy_loopback(struct phy_device *phydev, bool enable) >> +int genphy_loopback(struct phy_device *phydev, bool enable, int speed) >> { >> if (enable) { >> u16 ctl = BMCR_LOOPBACK; >> int ret, val; >> + if (speed == SPEED_10 || speed == SPEED_100 || >> + speed == SPEED_1000) >> + phydev->speed = speed; > Why is this limited to 1000? Shouldn't the max loopback speed be limited > by max hardware speed? We currently have definitions going up to > SPEED_800000 so some devices should support higher than 1000. This generic loopback implementation only supports those three speeds currently. If there is the need for higher speed, then there should be PHY specific implementations in the PHY drivers. > Why is speed defined as an int? It can never be negative, I normally > see it defined as u32. speed is defined within phy_device also as int. I aligned the data type to this structure. Gerhard
On Tue, Oct 29, 2024 at 06:58:12AM +0100, Gerhard Engleder wrote: > On 29.10.24 00:23, Lee Trager wrote: > > On 10/28/24 1:38 PM, Gerhard Engleder wrote: > > > -int genphy_loopback(struct phy_device *phydev, bool enable) > > > +int genphy_loopback(struct phy_device *phydev, bool enable, int speed) > > > { > > > if (enable) { > > > u16 ctl = BMCR_LOOPBACK; > > > int ret, val; > > > + if (speed == SPEED_10 || speed == SPEED_100 || > > > + speed == SPEED_1000) > > > + phydev->speed = speed; > > Why is this limited to 1000? Shouldn't the max loopback speed be limited > > by max hardware speed? We currently have definitions going up to > > SPEED_800000 so some devices should support higher than 1000. > > This generic loopback implementation only supports those three speeds > currently. If there is the need for higher speed, then there should be > PHY specific implementations in the PHY drivers. > > > Why is speed defined as an int? It can never be negative, I normally > > see it defined as u32. https://elixir.bootlin.com/linux/v6.11.5/source/include/uapi/linux/ethtool.h#L2172 #define SPEED_UNKNOWN -1 It cannot be unsigned. Andrew
Thanks for the explanations, both make sense. Lee On 10/29/24 6:02 AM, Andrew Lunn wrote: > On Tue, Oct 29, 2024 at 06:58:12AM +0100, Gerhard Engleder wrote: >> On 29.10.24 00:23, Lee Trager wrote: >>> On 10/28/24 1:38 PM, Gerhard Engleder wrote: >>>> -int genphy_loopback(struct phy_device *phydev, bool enable) >>>> +int genphy_loopback(struct phy_device *phydev, bool enable, int speed) >>>> { >>>> if (enable) { >>>> u16 ctl = BMCR_LOOPBACK; >>>> int ret, val; >>>> + if (speed == SPEED_10 || speed == SPEED_100 || >>>> + speed == SPEED_1000) >>>> + phydev->speed = speed; >>> Why is this limited to 1000? Shouldn't the max loopback speed be limited >>> by max hardware speed? We currently have definitions going up to >>> SPEED_800000 so some devices should support higher than 1000. >> This generic loopback implementation only supports those three speeds >> currently. If there is the need for higher speed, then there should be >> PHY specific implementations in the PHY drivers. >> >>> Why is speed defined as an int? It can never be negative, I normally >>> see it defined as u32. > https://elixir.bootlin.com/linux/v6.11.5/source/include/uapi/linux/ethtool.h#L2172 > > #define SPEED_UNKNOWN -1 > > It cannot be unsigned. > > Andrew >
diff --git a/drivers/net/phy/adin1100.c b/drivers/net/phy/adin1100.c index 85f910e2d4fb..dd8cce925668 100644 --- a/drivers/net/phy/adin1100.c +++ b/drivers/net/phy/adin1100.c @@ -215,8 +215,11 @@ static int adin_resume(struct phy_device *phydev) return adin_set_powerdown_mode(phydev, false); } -static int adin_set_loopback(struct phy_device *phydev, bool enable) +static int adin_set_loopback(struct phy_device *phydev, bool enable, int speed) { + if (enable && speed) + return -EOPNOTSUPP; + if (enable) return phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_10T1L_CTRL, BMCR_LOOPBACK); diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c index 4120385c5a79..b10ad482d566 100644 --- a/drivers/net/phy/dp83867.c +++ b/drivers/net/phy/dp83867.c @@ -1009,8 +1009,11 @@ static void dp83867_link_change_notify(struct phy_device *phydev) } } -static int dp83867_loopback(struct phy_device *phydev, bool enable) +static int dp83867_loopback(struct phy_device *phydev, bool enable, int speed) { + if (enable && speed) + return -EOPNOTSUPP; + return phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, enable ? BMCR_LOOPBACK : 0); } diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index 28aec37acd2c..c70c5c23b339 100644 --- a/drivers/net/phy/marvell.c +++ b/drivers/net/phy/marvell.c @@ -2095,13 +2095,19 @@ static void marvell_get_stats_simple(struct phy_device *phydev, data[i] = marvell_get_stat_simple(phydev, i); } -static int m88e1510_loopback(struct phy_device *phydev, bool enable) +static int m88e1510_loopback(struct phy_device *phydev, bool enable, int speed) { int err; if (enable) { u16 bmcr_ctl, mscr2_ctl = 0; + if (speed == SPEED_10 || speed == SPEED_100 || + speed == SPEED_1000) + phydev->speed = speed; + else if (speed) + return -EINVAL; + bmcr_ctl = mii_bmcr_encode_fixed(phydev->speed, phydev->duplex); err = phy_write(phydev, MII_BMCR, bmcr_ctl); diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c index db3c1f72b407..9b863b18a043 100644 --- a/drivers/net/phy/mxl-gpy.c +++ b/drivers/net/phy/mxl-gpy.c @@ -813,7 +813,7 @@ static void gpy_get_wol(struct phy_device *phydev, wol->wolopts = priv->wolopts; } -static int gpy_loopback(struct phy_device *phydev, bool enable) +static int gpy_loopback(struct phy_device *phydev, bool enable, int speed) { struct gpy_priv *priv = phydev->priv; u16 set = 0; @@ -822,6 +822,9 @@ static int gpy_loopback(struct phy_device *phydev, bool enable) if (enable) { u64 now = get_jiffies_64(); + if (speed) + return -EOPNOTSUPP; + /* wait until 3 seconds from last disable */ if (time_before64(now, priv->lb_dis_to)) msleep(jiffies64_to_msecs(priv->lb_dis_to - now)); @@ -845,15 +848,15 @@ static int gpy_loopback(struct phy_device *phydev, bool enable) return 0; } -static int gpy115_loopback(struct phy_device *phydev, bool enable) +static int gpy115_loopback(struct phy_device *phydev, bool enable, int speed) { struct gpy_priv *priv = phydev->priv; if (enable) - return gpy_loopback(phydev, enable); + return gpy_loopback(phydev, enable, speed); if (priv->fw_minor > 0x76) - return gpy_loopback(phydev, 0); + return gpy_loopback(phydev, 0, 0); return genphy_soft_reset(phydev); } diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c index 5695935fdce9..3399028f0e92 100644 --- a/drivers/net/phy/phy-c45.c +++ b/drivers/net/phy/phy-c45.c @@ -1230,8 +1230,11 @@ int gen10g_config_aneg(struct phy_device *phydev) } EXPORT_SYMBOL_GPL(gen10g_config_aneg); -int genphy_c45_loopback(struct phy_device *phydev, bool enable) +int genphy_c45_loopback(struct phy_device *phydev, bool enable, int speed) { + if (enable && speed) + return -EOPNOTSUPP; + return phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, MDIO_PCS_CTRL1_LOOPBACK, enable ? MDIO_PCS_CTRL1_LOOPBACK : 0); diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 563497a3274c..1c34cb947588 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -2112,9 +2112,9 @@ int phy_loopback(struct phy_device *phydev, bool enable) } if (phydev->drv->set_loopback) - ret = phydev->drv->set_loopback(phydev, enable); + ret = phydev->drv->set_loopback(phydev, enable, 0); else - ret = genphy_loopback(phydev, enable); + ret = genphy_loopback(phydev, enable, 0); if (ret) goto out; @@ -2906,12 +2906,18 @@ int genphy_resume(struct phy_device *phydev) } EXPORT_SYMBOL(genphy_resume); -int genphy_loopback(struct phy_device *phydev, bool enable) +int genphy_loopback(struct phy_device *phydev, bool enable, int speed) { if (enable) { u16 ctl = BMCR_LOOPBACK; int ret, val; + if (speed == SPEED_10 || speed == SPEED_100 || + speed == SPEED_1000) + phydev->speed = speed; + else if (speed) + return -EINVAL; + ctl |= mii_bmcr_encode_fixed(phydev->speed, phydev->duplex); phy_modify(phydev, MII_BMCR, ~0, ctl); diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c index 7c51daecf18e..2024d8ef36d9 100644 --- a/drivers/net/phy/xilinx_gmii2rgmii.c +++ b/drivers/net/phy/xilinx_gmii2rgmii.c @@ -64,15 +64,16 @@ static int xgmiitorgmii_read_status(struct phy_device *phydev) return 0; } -static int xgmiitorgmii_set_loopback(struct phy_device *phydev, bool enable) +static int xgmiitorgmii_set_loopback(struct phy_device *phydev, bool enable, + int speed) { struct gmii2rgmii *priv = mdiodev_get_drvdata(&phydev->mdio); int err; if (priv->phy_drv->set_loopback) - err = priv->phy_drv->set_loopback(phydev, enable); + err = priv->phy_drv->set_loopback(phydev, enable, speed); else - err = genphy_loopback(phydev, enable); + err = genphy_loopback(phydev, enable, speed); if (err < 0) return err; diff --git a/include/linux/phy.h b/include/linux/phy.h index bf0eb4e5d35c..83b705cfbf46 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -1107,8 +1107,16 @@ struct phy_driver { int (*set_tunable)(struct phy_device *dev, struct ethtool_tunable *tuna, const void *data); - /** @set_loopback: Set the loopback mood of the PHY */ - int (*set_loopback)(struct phy_device *dev, bool enable); + /** + * @set_loopback: Set the loopback mode of the PHY + * enable selects if the loopback mode is enabled or disabled. If the + * loopback mode is enabled, then the speed of the loopback mode can be + * requested with the speed argument. If the speed argument is zero, + * then any speed can be selected. If the speed argument is > 0, then + * this speed shall be selected for the loopback mode or EOPNOTSUPP + * shall be returned if speed selection is not supported. + */ + int (*set_loopback)(struct phy_device *dev, bool enable, int speed); /** @get_sqi: Get the signal quality indication */ int (*get_sqi)(struct phy_device *dev); /** @get_sqi_max: Get the maximum signal quality indication */ @@ -1895,7 +1903,7 @@ int genphy_read_status(struct phy_device *phydev); int genphy_read_master_slave(struct phy_device *phydev); int genphy_suspend(struct phy_device *phydev); int genphy_resume(struct phy_device *phydev); -int genphy_loopback(struct phy_device *phydev, bool enable); +int genphy_loopback(struct phy_device *phydev, bool enable, int speed); int genphy_soft_reset(struct phy_device *phydev); irqreturn_t genphy_handle_interrupt_no_ack(struct phy_device *phydev); @@ -1937,7 +1945,7 @@ int genphy_c45_pma_baset1_read_master_slave(struct phy_device *phydev); int genphy_c45_read_status(struct phy_device *phydev); int genphy_c45_baset1_read_status(struct phy_device *phydev); int genphy_c45_config_aneg(struct phy_device *phydev); -int genphy_c45_loopback(struct phy_device *phydev, bool enable); +int genphy_c45_loopback(struct phy_device *phydev, bool enable, int speed); int genphy_c45_pma_resume(struct phy_device *phydev); int genphy_c45_pma_suspend(struct phy_device *phydev); int genphy_c45_fast_retrain(struct phy_device *phydev, bool enable);
PHY drivers support loopback mode, but it is not possible to select the speed of the loopback mode. The speed is chosen by the set_loopback() operation of the PHY driver. Same is valid for genphy_loopback(). There are PHYs that support loopback with different speeds. Extend set_loopback() to make loopback speed selection possible. Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com> --- drivers/net/phy/adin1100.c | 5 ++++- drivers/net/phy/dp83867.c | 5 ++++- drivers/net/phy/marvell.c | 8 +++++++- drivers/net/phy/mxl-gpy.c | 11 +++++++---- drivers/net/phy/phy-c45.c | 5 ++++- drivers/net/phy/phy_device.c | 12 +++++++++--- drivers/net/phy/xilinx_gmii2rgmii.c | 7 ++++--- include/linux/phy.h | 16 ++++++++++++---- 8 files changed, 51 insertions(+), 18 deletions(-)