diff mbox series

[RFC,net-next,v2,16/17] net: stmmac: Move internal PCS init method to stmmac_pcs.c

Message ID 20240624132802.14238-8-fancer.lancer@gmail.com (mailing list archive)
State New, archived
Headers show
Series net: stmmac: convert stmmac "pcs" to phylink | expand

Commit Message

Serge Semin June 24, 2024, 1:26 p.m. UTC
Currently the internal PCS initialization procedure is split up into
several functions and special flags:

1. stmmac_check_pcs_mode() - determine the internal PCS based on the
specified interface and init the mac_device_info::pcs field with the
respective flag (STMMAC_PCS_RGMII or STMMAC_PCS_SGMII);

2. stmmac_ops::phylink_select_pcs() - callback specific for the DW GMAC
and DW QoS Eth IP-cores which returns the pointer to the phylink_pcs
structure if any of the STMMAC_PCS_RGMII or STMMAC_PCS_SGMII flag is found
in the mac_device_info::pcs field.

The initialization procedure described above can be simplified by
converting the stmmac_check_pcs_mode() to the PCS-init method defined in
stmmac_pcs.c, which besides would take the STMMAC_FLAG_HAS_INTEGRATED_PCS
flag into account. Seeing the RGMII and SGMII MAC-interfaces can be only
found on the DW GMAC and DW QoS Eth, the stmmac_ops::phylink_select_pcs()
callbacks content can be freely moved right into the generic
phylink_mac_ops::mac_select_pcs() function.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>

---

The change looks very promising and greatly simplifying the internal PCS
initialization procedure by providing a single coherent method activating
the internal PCS capability based on the requested interface and the
detected DW MAC HW-capability.
---
 .../ethernet/stmicro/stmmac/dwmac1000_core.c  | 16 +------------
 .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 17 +------------
 drivers/net/ethernet/stmicro/stmmac/hwif.h    | 15 ++----------
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 24 ++++---------------
 .../net/ethernet/stmicro/stmmac/stmmac_mdio.c |  3 +++
 .../net/ethernet/stmicro/stmmac/stmmac_pcs.c  | 15 ++++++++++++
 .../net/ethernet/stmicro/stmmac/stmmac_pcs.h  |  2 ++
 7 files changed, 28 insertions(+), 64 deletions(-)

Comments

Russell King (Oracle) June 28, 2024, 2:36 p.m. UTC | #1
On Mon, Jun 24, 2024 at 04:26:33PM +0300, Serge Semin wrote:
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 72c2d3e2c121..743d356f6d12 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -950,13 +950,16 @@ static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,
>  {
>  	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
>  
> +	if (priv->hw->pcs)
> +		return &priv->hw->mac_pcs;
> +
>  	if (priv->hw->xpcs)
>  		return &priv->hw->xpcs->pcs;
>  
>  	if (priv->hw->phylink_pcs)
>  		return priv->hw->phylink_pcs;
>  
> -	return stmmac_mac_phylink_select_pcs(priv, interface);
> +	return NULL;

I really really don't like this due to:

1. I spent a long time working out what the priority here should be, and
you've just thrown all that work away by changing it - to something that
I believe is incorrect.

2. I want to eventually see this function checking the interface type
before just handing out a random PCS, and it was my intention to
eventually that into the MACs own select_pcs() methods. Getting rid of
those methods means that the MACs themselves now can't make the
decision which is where that should be.

3. When operating in RGMII "inband" mode, the .pcs_config etc doesn't
make much sense (we're probably accessing registers that don't exist)
and I had plans to split this into a RGMII "PCS" which was just a PCS
that implemented .pcs_get_state(), a stub .pcs_config(), and a separate
fully-featured "SGMII PCS".

So, I would like to eventually see here something like:

	if (priv->hw->xpcs)
		return &priv->hw->xpcs->pcs;

	if (priv->hw->phylink_pcs)
		return priv->hw->phylink_pcs;

	if (!(priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS)) {
		if (phy_interface_mode_is_rgmii(priv->plat->mac_interface))
			return &priv->hw->mac_rgmii_pcs;

		if (priv->dma_cap.pcs &&
		    priv->plat->mac_interface == PHY_INTERFACE_MODE_SGMII)
			return &priv->hw->mac_sgmii_pcs;
	}

	return NULL;

> +void dwmac_pcs_init(struct mac_device_info *hw)
> +{
> +	struct stmmac_priv *priv = hw->priv;
> +	int interface = priv->plat->mac_interface;
> +
> +	if (priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS)
> +		return;
> +	else if (phy_interface_mode_is_rgmii(interface))
> +		hw->pcs = STMMAC_PCS_RGMII;
> +	else if (priv->dma_cap.pcs && interface == PHY_INTERFACE_MODE_SGMII)
> +		hw->pcs = STMMAC_PCS_SGMII;
> +
> +	hw->mac_pcs.neg_mode = true;
> +}

Please move "hw->mac_pcs.neg_mode = true;" to where the PCS method
functions are implemented - it determines whether the PCS method
functions take the AN mode or the neg mode, and this is a property of
their implementations. It should not be split away from them.

Thanks.
Serge Semin July 4, 2024, 12:55 p.m. UTC | #2
On Fri, Jun 28, 2024 at 03:36:20PM +0100, Russell King (Oracle) wrote:
> On Mon, Jun 24, 2024 at 04:26:33PM +0300, Serge Semin wrote:
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 72c2d3e2c121..743d356f6d12 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -950,13 +950,16 @@ static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,
> >  {
> >  	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
> >  
> > +	if (priv->hw->pcs)
> > +		return &priv->hw->mac_pcs;
> > +
> >  	if (priv->hw->xpcs)
> >  		return &priv->hw->xpcs->pcs;
> >  
> >  	if (priv->hw->phylink_pcs)
> >  		return priv->hw->phylink_pcs;
> >  
> > -	return stmmac_mac_phylink_select_pcs(priv, interface);
> > +	return NULL;
> 
> I really really don't like this due to:
> 

> 1. I spent a long time working out what the priority here should be, and
> you've just thrown all that work away by changing it - to something that
> I believe is incorrect.
> 

Right, the correct precedence would be to use the external PCS if one
available. It's easy to fix anyway.

> 2. I want to eventually see this function checking the interface type
> before just handing out a random PCS,

The only problem is that currently it relies on the
plat_stmmaenet_data::mac_interface field value instead of parsing the
specified interface type.(

> and it was my intention to
> eventually that into the MACs own select_pcs() methods. Getting rid of
> those methods means that the MACs themselves now can't make the
> decision which is where that should be.

Ok. Why not. We can preserve the MAC-own select_pcs() method.
(See my last comment on this email for details.)

> 
> 3. When operating in RGMII "inband" mode, the .pcs_config etc doesn't
> make much sense (we're probably accessing registers that don't exist)

Absolutely right. Current dwmac_pcs_config() implementation is fully
SGMII/TBI/RTBI-specific.

> and I had plans to split this into a RGMII "PCS" which was just a PCS
> that implemented .pcs_get_state(), a stub .pcs_config(), and a separate
> fully-featured "SGMII PCS".

Actually it's a good idea. We should have that implemented in v3.

> 
> So, I would like to eventually see here something like:
> 
> 	if (priv->hw->xpcs)
> 		return &priv->hw->xpcs->pcs;
> 
> 	if (priv->hw->phylink_pcs)
> 		return priv->hw->phylink_pcs;
> 
> 	if (!(priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS)) {
> 		if (phy_interface_mode_is_rgmii(priv->plat->mac_interface))
> 			return &priv->hw->mac_rgmii_pcs;
> 
> 		if (priv->dma_cap.pcs &&
> 		    priv->plat->mac_interface == PHY_INTERFACE_MODE_SGMII)
> 			return &priv->hw->mac_sgmii_pcs;
> 	}
> 
> 	return NULL;

So the differences of my and your implementations are:
1. priv->hw->pcs field is utilized to determine the RGMII/SGMII PCS
availability (it's initialized in dwmac_pcs_init()).
2. The order of the PCS selection: internal PCS has precedence over
the external PCS'es.
3. There is a single PHY-link PCS descriptor for both RGMII "inband"
and SGMII PCSes.

There is nothing hard to settle the 2. and 3. notes. The only
problematic part is 1. due to the damn mac_device_info::ps field
implying the fixed-speed semantics for the MAC2MAC case. The field is
initialized in the stmmac_hw_setup() method based on the
mac_device_info::pcs field content. The mac_device_info::ps value is
then utilized in the stmmac_ops::core_init() methods and in
dwmac_pcs_config() to pre-define the link speed. Since I hadn't come
up with a good idea of what to do with that MAC2MAC stuff back then I
decided to preserve the mac_device_info::pcs-based semantics
everywhere.

But now I guess I've got a good idea. We can use the
plat_stmmacenet_data::mac_port_sel_speed field directly where it is
relevant. Like this:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:
static int stmmac_hw_setup(struct net_device *dev, bool ptp_register)
{
	// Drop everything priv->hw.pcs and priv->hw.ps related from here
	// due to the changes suggested further.
}

drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c:
drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:
static void dwmac*_core_init(...)
{
	...
	// Directly use the plat_stmmacenet_data::mac_port_sel_speed value
	switch (priv->plat->mac_port_sel_speed) {
	case SPEED_1000:
		ps_speed = hw->link.speed1000;
		break;
	case SPEED_100:
		ps_speed = hw->link.speed100;
		break;
	case SPEED_10:
		ps_speed = hw->link.speed10;
		break;
	default:
		dev_warn(priv->device, "Unsupported port speed\n");
		break;
	}

	if (ps_speed) {
		value &= hw->link.speed_mask;
		value |= ps_speed | GMAC_CONFIG_TE;
	}
	...
}

drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c:
static void dwmac*_core_init(...)
{
	// There is no internal PCS in DW XGMACes. So we can freely drop
	// the hw->ps clause from here.
}

drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c:
static int dwmac_pcs_config(...)
{
	...
	// Directly use the plat_stmmacenet_data::mac_port_sel_speed value
	if (priv->plat->mac_port_sel_speed)
		val |= PCS_AN_CTRL_SGMRAL;
	...
}

After that we can freely drop the mac_device_info::ps and
mac_device_info::pcs fields. Thoughts?

> 
> > +void dwmac_pcs_init(struct mac_device_info *hw)
> > +{
> > +	struct stmmac_priv *priv = hw->priv;
> > +	int interface = priv->plat->mac_interface;
> > +
> > +	if (priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS)
> > +		return;
> > +	else if (phy_interface_mode_is_rgmii(interface))
> > +		hw->pcs = STMMAC_PCS_RGMII;
> > +	else if (priv->dma_cap.pcs && interface == PHY_INTERFACE_MODE_SGMII)
> > +		hw->pcs = STMMAC_PCS_SGMII;
> > +
> > +	hw->mac_pcs.neg_mode = true;
> > +}
> 

> Please move "hw->mac_pcs.neg_mode = true;" to where the PCS method
> functions are implemented - it determines whether the PCS method
> functions take the AN mode or the neg mode, and this is a property of
> their implementations. It should not be split away from them.

Ok.

---

Seeing the series introducing the plat_stmmacenet_data::select_pcs()
method has been recently merged in, let's discuss the entire PCS
selection code a bit more. Taking into account what you said above I
guess we can implement something like this:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:
static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,
                                                 phy_interface_t interface)
{
	// Platform-specific PCS selection method implying DW XPCS and
	// Lynx PCS selection (and internal PCS selection if relevant)
        if (priv->plat->select_pcs) {
                pcs = priv->plat->select_pcs(priv, interface);
                if (!IS_ERR(pcs))
                        return pcs;
        }

	// MAC-specific PCS selection method
	pcs = stmmac_mac_select_int_pcs(priv, priv->hw, priv->plat->mac_interface);
        if (!IS_ERR(pcs))
                return pcs;

        return NULL;
}

drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c:
static struct phylink_pcs *dwmac1000_select_pcs(struct mac_device_info *hw,
						phy_interface_t interface)
{
	if (phy_interface_mode_is_rgmii(interface))
		return &hw->mac_rgmii_pcs;
	else if (priv->dma_cap.pcs && interface == PHY_INTERFACE_MODE_SGMII)
		return &hw->mac_sgmii_pcs;

	return NULLL
}

...

const struct stmmac_ops dwmac1000_ops = {
	...
	.select_pcs = dwmac1000_select_pcs,
	...
};

drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:
// The same changes as in the dwmac1000_core.c file.

drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c:
// Drop my dwmac_pcs_init() implementation if we get to eliminate the 
// mac_device_info::ps and mac_device_info::pcs fields as I suggested
// earlier in this message


So what do you think?

-Serge(y)

> 
> Thanks.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
index 2d77ffd16f0a..2fbb853cc19c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
@@ -16,7 +16,7 @@ 
 #include <linux/slab.h>
 #include <linux/ethtool.h>
 #include <linux/io.h>
-#include <linux/phylink.h>
+
 #include "stmmac.h"
 #include "stmmac_pcs.h"
 #include "dwmac1000.h"
@@ -388,17 +388,6 @@  static u16 dwmac1000_pcs_get_config_reg(struct mac_device_info *hw)
 	return FIELD_GET(GMAC_RGSMIIIS_CONFIG_REG, val);
 }
 
-static struct phylink_pcs *
-dwmac1000_phylink_select_pcs(struct stmmac_priv *priv,
-			     phy_interface_t interface)
-{
-	if (priv->hw->pcs & STMMAC_PCS_RGMII ||
-	    priv->hw->pcs & STMMAC_PCS_SGMII)
-		return &priv->hw->mac_pcs;
-
-	return NULL;
-}
-
 static void dwmac1000_debug(struct stmmac_priv *priv, void __iomem *ioaddr,
 			    struct stmmac_extra_stats *x,
 			    u32 rx_queues, u32 tx_queues)
@@ -489,7 +478,6 @@  static void dwmac1000_set_mac_loopback(void __iomem *ioaddr, bool enable)
 
 const struct stmmac_ops dwmac1000_ops = {
 	.core_init = dwmac1000_core_init,
-	.phylink_select_pcs = dwmac1000_phylink_select_pcs,
 	.set_mac = stmmac_set_mac,
 	.rx_ipc = dwmac1000_rx_ipc_enable,
 	.dump_regs = dwmac1000_dump_regs,
@@ -541,7 +529,5 @@  int dwmac1000_setup(struct stmmac_priv *priv)
 	mac->mii.clk_csr_shift = 2;
 	mac->mii.clk_csr_mask = GENMASK(5, 2);
 
-	mac->mac_pcs.neg_mode = true;
-
 	return 0;
 }
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
index c58dc20eddeb..f5449f0962ad 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
@@ -14,7 +14,7 @@ 
 #include <linux/slab.h>
 #include <linux/ethtool.h>
 #include <linux/io.h>
-#include <linux/phylink.h>
+
 #include "stmmac.h"
 #include "stmmac_pcs.h"
 #include "dwmac4.h"
@@ -780,16 +780,6 @@  static void dwmac4_flow_ctrl(struct mac_device_info *hw, unsigned int duplex,
 	}
 }
 
-static struct phylink_pcs *
-dwmac4_phylink_select_pcs(struct stmmac_priv *priv, phy_interface_t interface)
-{
-	if (priv->hw->pcs & STMMAC_PCS_RGMII ||
-	    priv->hw->pcs & STMMAC_PCS_SGMII)
-		return &priv->hw->mac_pcs;
-
-	return NULL;
-}
-
 static int dwmac4_irq_mtl_status(struct stmmac_priv *priv,
 				 struct mac_device_info *hw, u32 chan)
 {
@@ -1178,7 +1168,6 @@  static void dwmac4_set_hw_vlan_mode(struct mac_device_info *hw)
 const struct stmmac_ops dwmac4_ops = {
 	.core_init = dwmac4_core_init,
 	.update_caps = dwmac4_update_caps,
-	.phylink_select_pcs = dwmac4_phylink_select_pcs,
 	.set_mac = stmmac_set_mac,
 	.rx_ipc = dwmac4_rx_ipc_enable,
 	.rx_queue_enable = dwmac4_rx_queue_enable,
@@ -1224,7 +1213,6 @@  const struct stmmac_ops dwmac4_ops = {
 const struct stmmac_ops dwmac410_ops = {
 	.core_init = dwmac4_core_init,
 	.update_caps = dwmac4_update_caps,
-	.phylink_select_pcs = dwmac4_phylink_select_pcs,
 	.set_mac = stmmac_dwmac4_set_mac,
 	.rx_ipc = dwmac4_rx_ipc_enable,
 	.rx_queue_enable = dwmac4_rx_queue_enable,
@@ -1274,7 +1262,6 @@  const struct stmmac_ops dwmac410_ops = {
 const struct stmmac_ops dwmac510_ops = {
 	.core_init = dwmac4_core_init,
 	.update_caps = dwmac4_update_caps,
-	.phylink_select_pcs = dwmac4_phylink_select_pcs,
 	.set_mac = stmmac_dwmac4_set_mac,
 	.rx_ipc = dwmac4_rx_ipc_enable,
 	.rx_queue_enable = dwmac4_rx_queue_enable,
@@ -1389,7 +1376,5 @@  int dwmac4_setup(struct stmmac_priv *priv)
 	mac->mii.clk_csr_mask = GENMASK(11, 8);
 	mac->num_vlan = dwmac4_get_num_vlan(priv->ioaddr);
 
-	mac->mac_pcs.neg_mode = true;
-
 	return 0;
 }
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index 3d39417e906d..4bacfe8a18e0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -18,17 +18,13 @@ 
 	} \
 	__result; \
 })
-#define stmmac_do_typed_callback(__type, __fail_ret, __priv, __module, \
-				 __cname,  __arg0, __args...) \
+#define stmmac_do_callback(__priv, __module, __cname,  __arg0, __args...) \
 ({ \
-	__type __result = __fail_ret; \
+	int __result = -EINVAL; \
 	if ((__priv)->hw->__module && (__priv)->hw->__module->__cname) \
 		__result = (__priv)->hw->__module->__cname((__arg0), ##__args); \
 	__result; \
 })
-#define stmmac_do_callback(__priv, __module, __cname,  __arg0, __args...) \
-	stmmac_do_typed_callback(int, -EINVAL, __priv, __module, __cname, \
-				 __arg0, ##__args)
 
 struct stmmac_extra_stats;
 struct stmmac_priv;
@@ -315,9 +311,6 @@  struct stmmac_ops {
 	void (*core_init)(struct mac_device_info *hw, struct net_device *dev);
 	/* Update MAC capabilities */
 	void (*update_caps)(struct stmmac_priv *priv);
-	/* Get phylink PCS (for MAC */
-	struct phylink_pcs *(*phylink_select_pcs)(struct stmmac_priv *priv,
-						  phy_interface_t interface);
 	/* Enable the MAC RX/TX */
 	void (*set_mac)(void __iomem *ioaddr, bool enable);
 	/* Enable and verify that the IPC module is supported */
@@ -439,10 +432,6 @@  struct stmmac_ops {
 	stmmac_do_void_callback(__priv, mac, core_init, __args)
 #define stmmac_mac_update_caps(__priv) \
 	stmmac_do_void_callback(__priv, mac, update_caps, __priv)
-#define stmmac_mac_phylink_select_pcs(__priv, __interface) \
-	stmmac_do_typed_callback(struct phylink_pcs *, ERR_PTR(-EOPNOTSUPP), \
-				 __priv, mac, phylink_select_pcs, __priv,\
-				 __interface)
 #define stmmac_mac_set(__priv, __args...) \
 	stmmac_do_void_callback(__priv, mac, set_mac, __args)
 #define stmmac_rx_ipc(__priv, __args...) \
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 72c2d3e2c121..743d356f6d12 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -950,13 +950,16 @@  static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,
 {
 	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
 
+	if (priv->hw->pcs)
+		return &priv->hw->mac_pcs;
+
 	if (priv->hw->xpcs)
 		return &priv->hw->xpcs->pcs;
 
 	if (priv->hw->phylink_pcs)
 		return priv->hw->phylink_pcs;
 
-	return stmmac_mac_phylink_select_pcs(priv, interface);
+	return NULL;
 }
 
 static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
@@ -1121,23 +1124,6 @@  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
 	.mac_link_up = stmmac_mac_link_up,
 };
 
-/**
- * stmmac_check_pcs_mode - verify if RGMII/SGMII is supported
- * @priv: driver private structure
- * Description: this is to verify if the HW supports the PCS.
- * Physical Coding Sublayer (PCS) interface that can be used when the MAC is
- * configured for the TBI, RTBI, or SGMII PHY interface.
- */
-static void stmmac_check_pcs_mode(struct stmmac_priv *priv)
-{
-	int interface = priv->plat->mac_interface;
-
-	if (phy_interface_mode_is_rgmii(interface))
-		priv->hw->pcs = STMMAC_PCS_RGMII;
-	else if (priv->dma_cap.pcs && interface == PHY_INTERFACE_MODE_SGMII)
-		priv->hw->pcs = STMMAC_PCS_SGMII;
-}
-
 /**
  * stmmac_init_phy - PHY initialization
  * @dev: net device structure
@@ -7704,8 +7690,6 @@  int stmmac_dvr_probe(struct device *device,
 	else
 		stmmac_clk_csr_set(priv);
 
-	stmmac_check_pcs_mode(priv);
-
 	pm_runtime_get_noresume(device);
 	pm_runtime_set_active(device);
 	if (!pm_runtime_enabled(device))
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index aa43117134d3..985b4b8c021f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -22,6 +22,7 @@ 
 
 #include "dwxgmac2.h"
 #include "stmmac.h"
+#include "stmmac_pcs.h"
 
 #define MII_BUSY 0x00000001
 #define MII_WRITE 0x00000002
@@ -505,6 +506,8 @@  int stmmac_pcs_setup(struct net_device *ndev)
 	priv = netdev_priv(ndev);
 	mode = priv->plat->phy_interface;
 
+	dwmac_pcs_init(priv->hw);
+
 	if (priv->plat->pcs_init) {
 		ret = priv->plat->pcs_init(priv);
 	} else if (priv->plat->mdio_bus_data &&
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c
index aac49f6472f0..fdfc95299f89 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.c
@@ -167,3 +167,18 @@  void dwmac_pcs_isr(struct mac_device_info *hw, unsigned int intr_status,
 	if (an_status || sr_status)
 		phylink_pcs_change(&hw->mac_pcs, false);
 }
+
+void dwmac_pcs_init(struct mac_device_info *hw)
+{
+	struct stmmac_priv *priv = hw->priv;
+	int interface = priv->plat->mac_interface;
+
+	if (priv->plat->flags & STMMAC_FLAG_HAS_INTEGRATED_PCS)
+		return;
+	else if (phy_interface_mode_is_rgmii(interface))
+		hw->pcs = STMMAC_PCS_RGMII;
+	else if (priv->dma_cap.pcs && interface == PHY_INTERFACE_MODE_SGMII)
+		hw->pcs = STMMAC_PCS_SGMII;
+
+	hw->mac_pcs.neg_mode = true;
+}
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h
index 6e364285a4ef..a17e5b37c411 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pcs.h
@@ -61,4 +61,6 @@ 
 void dwmac_pcs_isr(struct mac_device_info *hw, unsigned int intr_status,
 		   struct stmmac_extra_stats *x);
 
+void dwmac_pcs_init(struct mac_device_info *hw);
+
 #endif /* __STMMAC_PCS_H__ */