Enable MAC SerDes autonegotiation to distinguish between
1000BASE-X, SGMII and QSGMII MAC.
Signed-off-by: Raag Jadav <[email protected]>
---
drivers/net/phy/mscc/mscc.h | 2 ++
drivers/net/phy/mscc/mscc_main.c | 24 ++++++++++++++++++++++++
2 files changed, 26 insertions(+)
diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index a50235f..366db14 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h
@@ -195,6 +195,8 @@ enum rgmii_clock_delay {
#define MSCC_PHY_EXTENDED_INT_MS_EGR BIT(9)
/* Extended Page 3 Registers */
+#define MSCC_PHY_SERDES_PCS_CTRL 16
+#define MSCC_PHY_SERDES_ANEG BIT(7)
#define MSCC_PHY_SERDES_TX_VALID_CNT 21
#define MSCC_PHY_SERDES_TX_CRC_ERR_CNT 22
#define MSCC_PHY_SERDES_RX_VALID_CNT 28
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index ebfeeb3..6db43a5 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -1685,6 +1685,25 @@ static int vsc8574_config_host_serdes(struct phy_device *phydev)
PROC_CMD_RST_CONF_PORT | PROC_CMD_FIBER_1000BASE_X);
}
+static int vsc85xx_config_inband_aneg(struct phy_device *phydev, bool enabled)
+{
+ int rc;
+ u16 reg_val = 0;
+
+ if (enabled)
+ reg_val = MSCC_PHY_SERDES_ANEG;
+
+ mutex_lock(&phydev->lock);
+
+ rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_3,
+ MSCC_PHY_SERDES_PCS_CTRL, MSCC_PHY_SERDES_ANEG,
+ reg_val);
+
+ mutex_unlock(&phydev->lock);
+
+ return rc;
+}
+
static int vsc8584_config_init(struct phy_device *phydev)
{
struct vsc8531_private *vsc8531 = phydev->priv;
@@ -1772,6 +1791,11 @@ static int vsc8584_config_init(struct phy_device *phydev)
VSC8572_RGMII_TX_DELAY_MASK);
if (ret)
return ret;
+ } else {
+ /* Enable clause 37 */
+ ret = vsc85xx_config_inband_aneg(phydev, true);
+ if (ret)
+ return ret;
}
ret = genphy_soft_reset(phydev);
--
2.7.4
On Sun, Feb 06, 2022 at 10:42:34PM +0530, Raag Jadav wrote:
> On Sat, Feb 05, 2022 at 03:57:49PM +0100, Andrew Lunn wrote:
> > On Sat, Feb 05, 2022 at 12:14:52PM +0530, Raag Jadav wrote:
> > > Enable MAC SerDes autonegotiation to distinguish between
> > > 1000BASE-X, SGMII and QSGMII MAC.
> >
> > How does autoneg help you here? It just tells you about duplex, pause
> > etc. It does not indicate 1000BaseX, SGMII etc. The PHY should be
> > using whatever mode it was passed in phydev->interface, which the MAC
> > sets when it calls the connection function. If the PHY dynamically
> > changes its host side mode as a result of what that line side is
> > doing, it should also change phydev->interface. However, as far as i
> > can see, the mscc does not do this.
> >
>
> Once the PHY auto-negotiates parameters such as speed and duplex mode
> with its link partner over the copper link as per IEEE 802.3 Clause 27,
> the link partner’s capabilities are then transferred by PHY to MAC
> over 1000BASE-X or SGMII link using the auto-negotiation functionality
> defined in IEEE 802.3z Clause 37.
None of this allows you to distinguish between 1000BASE-X, SGMII and
QSGMII, which is what the commit message says.
It does allow you to get duplex, pause, and maybe speed via in band
signalling. But you should also be getting the same information out of
band, via the phylib callback.
There are some MACs which don't seem to work correctly without the in
band signalling, so maybe that is your problem? Please could you give
more background about your problem, what MAC and PHY combination are
you using, what problem you are seeing, etc.
Andrew
> MAC implementation[1] in a lot of NXP SoCs comes with in-band aneg enabled
> by default, and it does expect Clause 37 auto-negotiation to complete
> between MAC and PHY before the actual data transfer happens.
>
> [1] https://community.nxp.com/pwmxy87654/attachments/pwmxy87654/t-series/3241/1/AN3869(1).pdf
>
> I faced such issue while integrating VSC85xx PHY
> with one of the recent NXP SoC having similar MAC implementation.
> Not sure if this is a problem on MAC side or PHY side,
> But having Clause 37 support should help in most cases I believe.
So please use this information in the commit message.
The only danger with this change is, is the PHY O.K with auto-neg
turned on, with a MAC which does not actually perform auto-neg? It
could be we have boards which work now because PHY autoneg is turned
off.
Andrew
On Tue, Feb 08, 2022 at 09:27:52PM +0530, Raag Jadav wrote:
> On Tue, Feb 08, 2022 at 03:09:48AM +0100, Andrew Lunn wrote:
> > > MAC implementation[1] in a lot of NXP SoCs comes with in-band aneg enabled
> > > by default, and it does expect Clause 37 auto-negotiation to complete
> > > between MAC and PHY before the actual data transfer happens.
> > >
> > > [1] https://community.nxp.com/pwmxy87654/attachments/pwmxy87654/t-series/3241/1/AN3869(1).pdf
> > >
> > > I faced such issue while integrating VSC85xx PHY
> > > with one of the recent NXP SoC having similar MAC implementation.
> > > Not sure if this is a problem on MAC side or PHY side,
> > > But having Clause 37 support should help in most cases I believe.
> >
> > So please use this information in the commit message.
> >
> > The only danger with this change is, is the PHY O.K with auto-neg
> > turned on, with a MAC which does not actually perform auto-neg? It
> > could be we have boards which work now because PHY autoneg is turned
> > off.
> >
>
> Introducing an optional device tree property could be of any help?
Or find out what this PHY does when the host is not performing auto
neg. Does the datasheet say anything about that?
Andrew
On Mon, Feb 07, 2022 at 11:19:48PM +0530, Raag Jadav wrote:
> On Sun, Feb 06, 2022 at 07:01:41PM +0100, Andrew Lunn wrote:
> > On Sun, Feb 06, 2022 at 10:42:34PM +0530, Raag Jadav wrote:
> > > On Sat, Feb 05, 2022 at 03:57:49PM +0100, Andrew Lunn wrote:
> > > > On Sat, Feb 05, 2022 at 12:14:52PM +0530, Raag Jadav wrote:
> > > > > Enable MAC SerDes autonegotiation to distinguish between
> > > > > 1000BASE-X, SGMII and QSGMII MAC.
> > > >
> > > > How does autoneg help you here? It just tells you about duplex, pause
> > > > etc. It does not indicate 1000BaseX, SGMII etc. The PHY should be
> > > > using whatever mode it was passed in phydev->interface, which the MAC
> > > > sets when it calls the connection function. If the PHY dynamically
> > > > changes its host side mode as a result of what that line side is
> > > > doing, it should also change phydev->interface. However, as far as i
> > > > can see, the mscc does not do this.
> > > >
> > >
> > > Once the PHY auto-negotiates parameters such as speed and duplex mode
> > > with its link partner over the copper link as per IEEE 802.3 Clause 27,
> > > the link partner’s capabilities are then transferred by PHY to MAC
> > > over 1000BASE-X or SGMII link using the auto-negotiation functionality
> > > defined in IEEE 802.3z Clause 37.
> >
> > None of this allows you to distinguish between 1000BASE-X, SGMII and
> > QSGMII, which is what the commit message says.
> >
>
> I agree, the current commit message is misleading.
>
> > It does allow you to get duplex, pause, and maybe speed via in band
> > signalling. But you should also be getting the same information out of
> > band, via the phylib callback.
> >
> > There are some MACs which don't seem to work correctly without the in
> > band signalling, so maybe that is your problem? Please could you give
> > more background about your problem, what MAC and PHY combination are
> > you using, what problem you are seeing, etc.
> >
>
> MAC implementation[1] in a lot of NXP SoCs comes with in-band aneg enabled
> by default, and it does expect Clause 37 auto-negotiation to complete
> between MAC and PHY before the actual data transfer happens.
>
> [1] https://community.nxp.com/pwmxy87654/attachments/pwmxy87654/t-series/3241/1/AN3869(1).pdf
>
> I faced such issue while integrating VSC85xx PHY
> with one of the recent NXP SoC having similar MAC implementation.
> Not sure if this is a problem on MAC side or PHY side,
> But having Clause 37 support should help in most cases I believe.
Clause 37 is 1000BASE-X negotiation, which is different from SGMII - a
point which is even made in your PDF above in section 1.1.
You will need both ends to be operating in SGMII mode for 10M and 100M
to work. If one end is in 1000BASE-X mdoe and the other is in SGMII,
it can appear to work, but it won't be working correctly.
Please get the terminology correct here when talking about SGMII or
1000BASE-X.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Feb 08, 2022 at 09:45:13AM +0000, Russell King (Oracle) wrote:
> On Mon, Feb 07, 2022 at 11:19:48PM +0530, Raag Jadav wrote:
> > On Sun, Feb 06, 2022 at 07:01:41PM +0100, Andrew Lunn wrote:
> > > On Sun, Feb 06, 2022 at 10:42:34PM +0530, Raag Jadav wrote:
> > > > On Sat, Feb 05, 2022 at 03:57:49PM +0100, Andrew Lunn wrote:
> > > > > On Sat, Feb 05, 2022 at 12:14:52PM +0530, Raag Jadav wrote:
> > > > > > Enable MAC SerDes autonegotiation to distinguish between
> > > > > > 1000BASE-X, SGMII and QSGMII MAC.
> > > > >
> > > > > How does autoneg help you here? It just tells you about duplex, pause
> > > > > etc. It does not indicate 1000BaseX, SGMII etc. The PHY should be
> > > > > using whatever mode it was passed in phydev->interface, which the MAC
> > > > > sets when it calls the connection function. If the PHY dynamically
> > > > > changes its host side mode as a result of what that line side is
> > > > > doing, it should also change phydev->interface. However, as far as i
> > > > > can see, the mscc does not do this.
> > > > >
> > > >
> > > > Once the PHY auto-negotiates parameters such as speed and duplex mode
> > > > with its link partner over the copper link as per IEEE 802.3 Clause 27,
> > > > the link partner’s capabilities are then transferred by PHY to MAC
> > > > over 1000BASE-X or SGMII link using the auto-negotiation functionality
> > > > defined in IEEE 802.3z Clause 37.
> > >
> > > None of this allows you to distinguish between 1000BASE-X, SGMII and
> > > QSGMII, which is what the commit message says.
> > >
> >
> > I agree, the current commit message is misleading.
> >
> > > It does allow you to get duplex, pause, and maybe speed via in band
> > > signalling. But you should also be getting the same information out of
> > > band, via the phylib callback.
> > >
> > > There are some MACs which don't seem to work correctly without the in
> > > band signalling, so maybe that is your problem? Please could you give
> > > more background about your problem, what MAC and PHY combination are
> > > you using, what problem you are seeing, etc.
> > >
> >
> > MAC implementation[1] in a lot of NXP SoCs comes with in-band aneg enabled
> > by default, and it does expect Clause 37 auto-negotiation to complete
> > between MAC and PHY before the actual data transfer happens.
> >
> > [1] https://community.nxp.com/pwmxy87654/attachments/pwmxy87654/t-series/3241/1/AN3869(1).pdf
> >
> > I faced such issue while integrating VSC85xx PHY
> > with one of the recent NXP SoC having similar MAC implementation.
> > Not sure if this is a problem on MAC side or PHY side,
> > But having Clause 37 support should help in most cases I believe.
>
> Clause 37 is 1000BASE-X negotiation, which is different from SGMII - a
> point which is even made in your PDF above in section 1.1.
>
> You will need both ends to be operating in SGMII mode for 10M and 100M
> to work. If one end is in 1000BASE-X mdoe and the other is in SGMII,
> it can appear to work, but it won't be working correctly.
>
> Please get the terminology correct here when talking about SGMII or
> 1000BASE-X.
>
Thank you for the clarification.
Really appreciate it.
Cheers,
Raag
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Feb 08, 2022 at 09:27:52PM +0530, Raag Jadav wrote:
> On Tue, Feb 08, 2022 at 03:09:48AM +0100, Andrew Lunn wrote:
> > > MAC implementation[1] in a lot of NXP SoCs comes with in-band aneg enabled
> > > by default, and it does expect Clause 37 auto-negotiation to complete
> > > between MAC and PHY before the actual data transfer happens.
> > >
> > > [1] https://community.nxp.com/pwmxy87654/attachments/pwmxy87654/t-series/3241/1/AN3869(1).pdf
> > >
> > > I faced such issue while integrating VSC85xx PHY
> > > with one of the recent NXP SoC having similar MAC implementation.
> > > Not sure if this is a problem on MAC side or PHY side,
> > > But having Clause 37 support should help in most cases I believe.
> >
> > So please use this information in the commit message.
> >
> > The only danger with this change is, is the PHY O.K with auto-neg
> > turned on, with a MAC which does not actually perform auto-neg? It
> > could be we have boards which work now because PHY autoneg is turned
> > off.
> >
>
> Introducing an optional device tree property could be of any help?
Preferably not. We do need some way that the MAC and PHY can co-operate
to work out whether inband should be used or not. Vladimir had some
patches for that a while back which were touching phylib and phylink
which may be worth looking into to see whether some of that can be
applied to your situation.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Sun, Feb 06, 2022 at 07:01:41PM +0100, Andrew Lunn wrote:
> On Sun, Feb 06, 2022 at 10:42:34PM +0530, Raag Jadav wrote:
> > On Sat, Feb 05, 2022 at 03:57:49PM +0100, Andrew Lunn wrote:
> > > On Sat, Feb 05, 2022 at 12:14:52PM +0530, Raag Jadav wrote:
> > > > Enable MAC SerDes autonegotiation to distinguish between
> > > > 1000BASE-X, SGMII and QSGMII MAC.
> > >
> > > How does autoneg help you here? It just tells you about duplex, pause
> > > etc. It does not indicate 1000BaseX, SGMII etc. The PHY should be
> > > using whatever mode it was passed in phydev->interface, which the MAC
> > > sets when it calls the connection function. If the PHY dynamically
> > > changes its host side mode as a result of what that line side is
> > > doing, it should also change phydev->interface. However, as far as i
> > > can see, the mscc does not do this.
> > >
> >
> > Once the PHY auto-negotiates parameters such as speed and duplex mode
> > with its link partner over the copper link as per IEEE 802.3 Clause 27,
> > the link partner’s capabilities are then transferred by PHY to MAC
> > over 1000BASE-X or SGMII link using the auto-negotiation functionality
> > defined in IEEE 802.3z Clause 37.
>
> None of this allows you to distinguish between 1000BASE-X, SGMII and
> QSGMII, which is what the commit message says.
>
I agree, the current commit message is misleading.
> It does allow you to get duplex, pause, and maybe speed via in band
> signalling. But you should also be getting the same information out of
> band, via the phylib callback.
>
> There are some MACs which don't seem to work correctly without the in
> band signalling, so maybe that is your problem? Please could you give
> more background about your problem, what MAC and PHY combination are
> you using, what problem you are seeing, etc.
>
MAC implementation[1] in a lot of NXP SoCs comes with in-band aneg enabled
by default, and it does expect Clause 37 auto-negotiation to complete
between MAC and PHY before the actual data transfer happens.
[1] https://community.nxp.com/pwmxy87654/attachments/pwmxy87654/t-series/3241/1/AN3869(1).pdf
I faced such issue while integrating VSC85xx PHY
with one of the recent NXP SoC having similar MAC implementation.
Not sure if this is a problem on MAC side or PHY side,
But having Clause 37 support should help in most cases I believe.
Cheers,
Raag
> Andrew
>
On Tue, Feb 08, 2022 at 03:09:48AM +0100, Andrew Lunn wrote:
> > MAC implementation[1] in a lot of NXP SoCs comes with in-band aneg enabled
> > by default, and it does expect Clause 37 auto-negotiation to complete
> > between MAC and PHY before the actual data transfer happens.
> >
> > [1] https://community.nxp.com/pwmxy87654/attachments/pwmxy87654/t-series/3241/1/AN3869(1).pdf
> >
> > I faced such issue while integrating VSC85xx PHY
> > with one of the recent NXP SoC having similar MAC implementation.
> > Not sure if this is a problem on MAC side or PHY side,
> > But having Clause 37 support should help in most cases I believe.
>
> So please use this information in the commit message.
>
> The only danger with this change is, is the PHY O.K with auto-neg
> turned on, with a MAC which does not actually perform auto-neg? It
> could be we have boards which work now because PHY autoneg is turned
> off.
>
Introducing an optional device tree property could be of any help?
> Andrew
On Tue, Feb 08, 2022 at 08:12:59PM +0100, Andrew Lunn wrote:
> On Tue, Feb 08, 2022 at 09:27:52PM +0530, Raag Jadav wrote:
> > On Tue, Feb 08, 2022 at 03:09:48AM +0100, Andrew Lunn wrote:
> > > > MAC implementation[1] in a lot of NXP SoCs comes with in-band aneg enabled
> > > > by default, and it does expect Clause 37 auto-negotiation to complete
> > > > between MAC and PHY before the actual data transfer happens.
> > > >
> > > > [1] https://community.nxp.com/pwmxy87654/attachments/pwmxy87654/t-series/3241/1/AN3869(1).pdf
> > > >
> > > > I faced such issue while integrating VSC85xx PHY
> > > > with one of the recent NXP SoC having similar MAC implementation.
> > > > Not sure if this is a problem on MAC side or PHY side,
> > > > But having Clause 37 support should help in most cases I believe.
> > >
> > > So please use this information in the commit message.
> > >
> > > The only danger with this change is, is the PHY O.K with auto-neg
> > > turned on, with a MAC which does not actually perform auto-neg? It
> > > could be we have boards which work now because PHY autoneg is turned
> > > off.
> > >
> >
> > Introducing an optional device tree property could be of any help?
>
> Or find out what this PHY does when the host is not performing auto
> neg. Does the datasheet say anything about that?
>
Not sure if such behaviour is explicitly mentioned in the datasheet.
Maybe someone from Microchip can shed some light on this.
Cheers,
Raag
> Andrew
Sorry for the late comment on this patch.
On Sat, Feb 05, 2022 at 12:14:52PM +0530, Raag Jadav wrote:
> +static int vsc85xx_config_inband_aneg(struct phy_device *phydev, bool enabled)
> +{
> + int rc;
> + u16 reg_val = 0;
> +
> + if (enabled)
> + reg_val = MSCC_PHY_SERDES_ANEG;
> +
> + mutex_lock(&phydev->lock);
> +
> + rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_3,
> + MSCC_PHY_SERDES_PCS_CTRL, MSCC_PHY_SERDES_ANEG,
> + reg_val);
> +
> + mutex_unlock(&phydev->lock);
What is the reason for the locking here?
phy_modify_paged() itself is safe due to the MDIO bus lock, so you
shouldn't need locking around it.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hi All,
On 05/02/22 12:14, Raag Jadav wrote:
> Enable MAC SerDes autonegotiation to distinguish between
> 1000BASE-X, SGMII and QSGMII MAC.
>
> Signed-off-by: Raag Jadav <[email protected]>
> ---
> drivers/net/phy/mscc/mscc.h | 2 ++
> drivers/net/phy/mscc/mscc_main.c | 24 ++++++++++++++++++++++++
> 2 files changed, 26 insertions(+)
>
> diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
> index a50235f..366db14 100644
> --- a/drivers/net/phy/mscc/mscc.h
> +++ b/drivers/net/phy/mscc/mscc.h
> @@ -195,6 +195,8 @@ enum rgmii_clock_delay {
> #define MSCC_PHY_EXTENDED_INT_MS_EGR BIT(9)
>
> /* Extended Page 3 Registers */
> +#define MSCC_PHY_SERDES_PCS_CTRL 16
> +#define MSCC_PHY_SERDES_ANEG BIT(7)
> #define MSCC_PHY_SERDES_TX_VALID_CNT 21
> #define MSCC_PHY_SERDES_TX_CRC_ERR_CNT 22
> #define MSCC_PHY_SERDES_RX_VALID_CNT 28
> diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
> index ebfeeb3..6db43a5 100644
> --- a/drivers/net/phy/mscc/mscc_main.c
> +++ b/drivers/net/phy/mscc/mscc_main.c
> @@ -1685,6 +1685,25 @@ static int vsc8574_config_host_serdes(struct phy_device *phydev)
> PROC_CMD_RST_CONF_PORT | PROC_CMD_FIBER_1000BASE_X);
> }
>
> +static int vsc85xx_config_inband_aneg(struct phy_device *phydev, bool enabled)
> +{
> + int rc;
> + u16 reg_val = 0;
> +
> + if (enabled)
> + reg_val = MSCC_PHY_SERDES_ANEG;
> +
> + mutex_lock(&phydev->lock);
> +
> + rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_3,
> + MSCC_PHY_SERDES_PCS_CTRL, MSCC_PHY_SERDES_ANEG,
> + reg_val);
> +
> + mutex_unlock(&phydev->lock);
> +
> + return rc;
> +}
> +
> static int vsc8584_config_init(struct phy_device *phydev)
> {
> struct vsc8531_private *vsc8531 = phydev->priv;
> @@ -1772,6 +1791,11 @@ static int vsc8584_config_init(struct phy_device *phydev)
> VSC8572_RGMII_TX_DELAY_MASK);
> if (ret)
> return ret;
> + } else {
> + /* Enable clause 37 */
> + ret = vsc85xx_config_inband_aneg(phydev, true);
> + if (ret)
> + return ret;
> }
>
> ret = genphy_soft_reset(phydev);
The same auto-negotiation configuration is also required for VSC8514.
The following patch is required to get Ethernet working with the Quad
port Ethernet Add-On card (QSGMII mode) connected to Texas Instruments
J7 common processor board.
Let me know if I should send it as a separate patch.
Thanks and Regards,
Siddharth Vadapalli.
8<------------------SNIP----------------------------
From 2ab92251ba7a09bc97476cef6c760beefb0d3cae Mon Sep 17 00:00:00 2001
From: Siddharth Vadapalli <[email protected]>
Date: Thu, 17 Feb 2022 15:45:20 +0530
Subject: [PATCH] net: phy: mscc: Add auto-negotiation feature to VSC8514
Auto-negotiation is currently enabled for VSC8584. It is also required
for VSC8514. Invoke the vsc85xx_config_inband_aneg() function from the
vsc8514_config_init() function present in mscc_main.c to start the
auto-negotiation process. This is required to get Ethernet working with
the Quad port Ethernet Add-On card (QSGMII mode) connected to Texas
Instruments J7 common processor board.
Signed-off-by: Siddharth Vadapalli <[email protected]>
---
drivers/net/phy/mscc/mscc_main.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/phy/mscc/mscc_main.c
b/drivers/net/phy/mscc/mscc_main.c
index 6db43a5c3b5e..b9a5662e7934 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -2119,6 +2119,11 @@ static int vsc8514_config_init(struct phy_device
*phydev)
ret = genphy_soft_reset(phydev);
+ if (ret)
+ return ret;
+
+ ret = vsc85xx_config_inband_aneg(phydev, true);
+
if (ret)
return ret;
On Thu, Feb 24, 2022 at 10:48:57AM +0000, Russell King (Oracle) wrote:
> Sorry for the late comment on this patch.
>
> On Sat, Feb 05, 2022 at 12:14:52PM +0530, Raag Jadav wrote:
> > +static int vsc85xx_config_inband_aneg(struct phy_device *phydev, bool enabled)
> > +{
> > + int rc;
> > + u16 reg_val = 0;
> > +
> > + if (enabled)
> > + reg_val = MSCC_PHY_SERDES_ANEG;
> > +
> > + mutex_lock(&phydev->lock);
> > +
> > + rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_3,
> > + MSCC_PHY_SERDES_PCS_CTRL, MSCC_PHY_SERDES_ANEG,
> > + reg_val);
> > +
> > + mutex_unlock(&phydev->lock);
>
> What is the reason for the locking here?
>
> phy_modify_paged() itself is safe due to the MDIO bus lock, so you
> shouldn't need locking around it.
>
True.
My initial thought was to have serialized access at PHY level,
as we have multiple ports to work with.
But I guess MDIO bus lock could do the job as well.
Will fix it in v2 if required.
I've gone through Vladimir's patches and they look more promising
than this approach.
Let me know if I could be of any help.
Cheers,
Raag
> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Sat, Feb 26, 2022 at 12:53:27PM +0530, Raag Jadav wrote:
> On Thu, Feb 24, 2022 at 10:48:57AM +0000, Russell King (Oracle) wrote:
> > Sorry for the late comment on this patch.
> >
> > On Sat, Feb 05, 2022 at 12:14:52PM +0530, Raag Jadav wrote:
> > > +static int vsc85xx_config_inband_aneg(struct phy_device *phydev, bool enabled)
> > > +{
> > > + int rc;
> > > + u16 reg_val = 0;
> > > +
> > > + if (enabled)
> > > + reg_val = MSCC_PHY_SERDES_ANEG;
> > > +
> > > + mutex_lock(&phydev->lock);
> > > +
> > > + rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_3,
> > > + MSCC_PHY_SERDES_PCS_CTRL, MSCC_PHY_SERDES_ANEG,
> > > + reg_val);
> > > +
> > > + mutex_unlock(&phydev->lock);
> >
> > What is the reason for the locking here?
> >
> > phy_modify_paged() itself is safe due to the MDIO bus lock, so you
> > shouldn't need locking around it.
> >
>
> True.
>
> My initial thought was to have serialized access at PHY level,
> as we have multiple ports to work with.
> But I guess MDIO bus lock could do the job as well.
The MDIO bus lock is the only lock that will guarantee that no other
users can nip onto the bus and possibly access your PHY in the middle
of an operation that requires more than one access to complete. Adding
local locking at PHY driver level does not give you those guarantees.
This is exactly why phy_modify() etc was added - because phy_read()..
phy_write() does not give that guarantee.
As an example of something that could interfere - the userspace MII
ioctls.
> I've gone through Vladimir's patches and they look more promising
> than this approach.
> Let me know if I could be of any help.
I haven't seen them - so up to you.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hi Raag,
On 26/02/22 12:53, Raag Jadav wrote:
> On Thu, Feb 24, 2022 at 10:48:57AM +0000, Russell King (Oracle) wrote:
>> Sorry for the late comment on this patch.
>>
>> On Sat, Feb 05, 2022 at 12:14:52PM +0530, Raag Jadav wrote:
>>> +static int vsc85xx_config_inband_aneg(struct phy_device *phydev, bool enabled)
>>> +{
>>> + int rc;
>>> + u16 reg_val = 0;
>>> +
>>> + if (enabled)
>>> + reg_val = MSCC_PHY_SERDES_ANEG;
>>> +
>>> + mutex_lock(&phydev->lock);
>>> +
>>> + rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_3,
>>> + MSCC_PHY_SERDES_PCS_CTRL, MSCC_PHY_SERDES_ANEG,
>>> + reg_val);
>>> +
>>> + mutex_unlock(&phydev->lock);
>>
>> What is the reason for the locking here?
>>
>> phy_modify_paged() itself is safe due to the MDIO bus lock, so you
>> shouldn't need locking around it.
>>
>
> True.
>
> My initial thought was to have serialized access at PHY level,
> as we have multiple ports to work with.
> But I guess MDIO bus lock could do the job as well.
>
> Will fix it in v2 if required.
Could you please let me know if you plan to post the v2 patch?
The autonegotiation feature is also required for VSC8514, and has to be invoked
in vsc8514_config_init(). Let me know if you need my help for this.
Regards,
Siddharth
On Sat, Feb 26, 2022 at 04:31:57PM +0000, Russell King (Oracle) wrote:
> On Sat, Feb 26, 2022 at 12:53:27PM +0530, Raag Jadav wrote:
> > On Thu, Feb 24, 2022 at 10:48:57AM +0000, Russell King (Oracle) wrote:
> > > Sorry for the late comment on this patch.
> > >
> > > On Sat, Feb 05, 2022 at 12:14:52PM +0530, Raag Jadav wrote:
> > > > +static int vsc85xx_config_inband_aneg(struct phy_device *phydev, bool enabled)
> > > > +{
> > > > + int rc;
> > > > + u16 reg_val = 0;
> > > > +
> > > > + if (enabled)
> > > > + reg_val = MSCC_PHY_SERDES_ANEG;
> > > > +
> > > > + mutex_lock(&phydev->lock);
> > > > +
> > > > + rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_3,
> > > > + MSCC_PHY_SERDES_PCS_CTRL, MSCC_PHY_SERDES_ANEG,
> > > > + reg_val);
> > > > +
> > > > + mutex_unlock(&phydev->lock);
> > >
> > > What is the reason for the locking here?
> > >
> > > phy_modify_paged() itself is safe due to the MDIO bus lock, so you
> > > shouldn't need locking around it.
> > >
> >
> > True.
> >
> > My initial thought was to have serialized access at PHY level,
> > as we have multiple ports to work with.
> > But I guess MDIO bus lock could do the job as well.
>
> The MDIO bus lock is the only lock that will guarantee that no other
> users can nip onto the bus and possibly access your PHY in the middle
> of an operation that requires more than one access to complete. Adding
> local locking at PHY driver level does not give you those guarantees.
> This is exactly why phy_modify() etc was added - because phy_read()..
> phy_write() does not give that guarantee.
>
> As an example of something that could interfere - the userspace MII
> ioctls.
>
> > I've gone through Vladimir's patches and they look more promising
> > than this approach.
> > Let me know if I could be of any help.
>
> I haven't seen them - so up to you.
>
Definitely worth a look.
> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hi Raag,
On 27/03/22 14:00, Raag Jadav wrote:
> On Thu, Mar 24, 2022 at 03:36:02PM +0530, Siddharth Vadapalli wrote:
>> Hi Raag,
>>
>> On 26/02/22 12:53, Raag Jadav wrote:
>>> On Thu, Feb 24, 2022 at 10:48:57AM +0000, Russell King (Oracle) wrote:
>>>> Sorry for the late comment on this patch.
>>>>
>>>> On Sat, Feb 05, 2022 at 12:14:52PM +0530, Raag Jadav wrote:
>>>>> +static int vsc85xx_config_inband_aneg(struct phy_device *phydev, bool enabled)
>>>>> +{
>>>>> + int rc;
>>>>> + u16 reg_val = 0;
>>>>> +
>>>>> + if (enabled)
>>>>> + reg_val = MSCC_PHY_SERDES_ANEG;
>>>>> +
>>>>> + mutex_lock(&phydev->lock);
>>>>> +
>>>>> + rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_3,
>>>>> + MSCC_PHY_SERDES_PCS_CTRL, MSCC_PHY_SERDES_ANEG,
>>>>> + reg_val);
>>>>> +
>>>>> + mutex_unlock(&phydev->lock);
>>>>
>>>> What is the reason for the locking here?
>>>>
>>>> phy_modify_paged() itself is safe due to the MDIO bus lock, so you
>>>> shouldn't need locking around it.
>>>>
>>>
>>> True.
>>>
>>> My initial thought was to have serialized access at PHY level,
>>> as we have multiple ports to work with.
>>> But I guess MDIO bus lock could do the job as well.
>>>
>>> Will fix it in v2 if required.
>>
>> Could you please let me know if you plan to post the v2 patch?
>>
>> The autonegotiation feature is also required for VSC8514, and has to be invoked
>> in vsc8514_config_init(). Let me know if you need my help for this.
>>
>
> Maybe this is what you're looking for.
> https://www.spinics.net/lists/netdev/msg768517.html
Thank you for pointing me to the thread. I will follow up regarding the progress
of the autonegotiation feature on that thread.
Regards,
Siddharth
On Thu, Mar 24, 2022 at 03:36:02PM +0530, Siddharth Vadapalli wrote:
> Hi Raag,
>
> On 26/02/22 12:53, Raag Jadav wrote:
> > On Thu, Feb 24, 2022 at 10:48:57AM +0000, Russell King (Oracle) wrote:
> >> Sorry for the late comment on this patch.
> >>
> >> On Sat, Feb 05, 2022 at 12:14:52PM +0530, Raag Jadav wrote:
> >>> +static int vsc85xx_config_inband_aneg(struct phy_device *phydev, bool enabled)
> >>> +{
> >>> + int rc;
> >>> + u16 reg_val = 0;
> >>> +
> >>> + if (enabled)
> >>> + reg_val = MSCC_PHY_SERDES_ANEG;
> >>> +
> >>> + mutex_lock(&phydev->lock);
> >>> +
> >>> + rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_3,
> >>> + MSCC_PHY_SERDES_PCS_CTRL, MSCC_PHY_SERDES_ANEG,
> >>> + reg_val);
> >>> +
> >>> + mutex_unlock(&phydev->lock);
> >>
> >> What is the reason for the locking here?
> >>
> >> phy_modify_paged() itself is safe due to the MDIO bus lock, so you
> >> shouldn't need locking around it.
> >>
> >
> > True.
> >
> > My initial thought was to have serialized access at PHY level,
> > as we have multiple ports to work with.
> > But I guess MDIO bus lock could do the job as well.
> >
> > Will fix it in v2 if required.
>
> Could you please let me know if you plan to post the v2 patch?
>
> The autonegotiation feature is also required for VSC8514, and has to be invoked
> in vsc8514_config_init(). Let me know if you need my help for this.
>
Maybe this is what you're looking for.
https://www.spinics.net/lists/netdev/msg768517.html
Cheers,
Raag
> Regards,
> Siddharth