diff mbox series

[V2,net-next] net: marvell: prestera: add phylink support

Message ID 20220713172013.29531-1-oleksandr.mazur@plvision.eu (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [V2,net-next] net: marvell: prestera: add phylink support | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 1 maintainers not CCed: tchornyi@marvell.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Oleksandr Mazur July 13, 2022, 5:20 p.m. UTC
For SFP port prestera driver will use kernel
phylink infrastucture to configure port mode based on
the module that has beed inserted

Co-developed-by: Yevhen Orlov <yevhen.orlov@plvision.eu>
Signed-off-by: Yevhen Orlov <yevhen.orlov@plvision.eu>
Co-developed-by: Taras Chornyi <taras.chornyi@plvision.eu>
Signed-off-by: Taras Chornyi <taras.chornyi@plvision.eu>
Signed-off-by: Oleksandr Mazur <oleksandr.mazur@plvision.eu>

PATCH V2:
  - fix mistreat of bitfield values as if they were bools.
  - remove phylink_config ifdefs.
  - remove obsolete phylink pcs / mac callbacks;
  - rework mac (/pcs) config to not look for speed / duplex
    parameters while link is not yet set up.
  - remove unused functions.
  - add phylink select cfg to prestera Kconfig.
---
 drivers/net/ethernet/marvell/prestera/Kconfig |   1 +
 .../net/ethernet/marvell/prestera/prestera.h  |   9 +
 .../marvell/prestera/prestera_ethtool.c       |  28 +-
 .../marvell/prestera/prestera_ethtool.h       |   3 -
 .../ethernet/marvell/prestera/prestera_main.c | 348 ++++++++++++++++--
 5 files changed, 327 insertions(+), 62 deletions(-)

Comments

Russell King (Oracle) July 13, 2022, 8:05 p.m. UTC | #1
On Wed, Jul 13, 2022 at 08:20:13PM +0300, Oleksandr Mazur wrote:
> For SFP port prestera driver will use kernel
> phylink infrastucture to configure port mode based on
> the module that has beed inserted
> 
> Co-developed-by: Yevhen Orlov <yevhen.orlov@plvision.eu>
> Signed-off-by: Yevhen Orlov <yevhen.orlov@plvision.eu>
> Co-developed-by: Taras Chornyi <taras.chornyi@plvision.eu>
> Signed-off-by: Taras Chornyi <taras.chornyi@plvision.eu>
> Signed-off-by: Oleksandr Mazur <oleksandr.mazur@plvision.eu>
> 
> PATCH V2:
>   - fix mistreat of bitfield values as if they were bools.
>   - remove phylink_config ifdefs.
>   - remove obsolete phylink pcs / mac callbacks;
>   - rework mac (/pcs) config to not look for speed / duplex
>     parameters while link is not yet set up.
>   - remove unused functions.
>   - add phylink select cfg to prestera Kconfig.

I would appreciate answers to my questions, rather than just another
patch submission. So I'll repeat my question in the hope of an answer:

First question which applies to everything in this patch is - why make
phylink conditional for this driver?

The reason that this needs to be answered is that I would like an
explanation why it's conditional, because it shouldn't be. By making it
conditional, you will have multiple separate paths through the driver
code trying to do the same thing, but differently, which means more time
an effort maintaining the driver.

> +static int prestera_pcs_config(struct phylink_pcs *pcs,
> +			       unsigned int mode,
> +			       phy_interface_t interface,
> +			       const unsigned long *advertising,
> +			       bool permit_pause_to_mac)
> +{
> +	struct prestera_port *port = port = prestera_pcs_to_port(pcs);
> +	struct prestera_port_mac_config cfg_mac;
> +	int err;
> +
> +	err = prestera_port_cfg_mac_read(port, &cfg_mac);
> +	if (err)
> +		return err;
> +
> +	cfg_mac.admin = true;
> +	cfg_mac.fec = PRESTERA_PORT_FEC_OFF;
> +
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_10GBASER:
> +		cfg_mac.speed = SPEED_10000;
> +		cfg_mac.inband = 0;
> +		cfg_mac.mode = PRESTERA_MAC_MODE_SR_LR;
> +		break;
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		cfg_mac.speed = SPEED_2500;
> +		cfg_mac.duplex = DUPLEX_FULL;
> +		cfg_mac.inband = test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> +					  advertising);
> +		cfg_mac.mode = PRESTERA_MAC_MODE_SGMII;
> +		break;
> +	case PHY_INTERFACE_MODE_SGMII:
> +		cfg_mac.inband = test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> +					  advertising);

This looks wrong to me. In SGMII mode, it is normal for the advertising
mask to indicate the media modes on the PHY to advertise, and whether to
enable advertisements on the _media_. Whether media advertisements are
enabled or not doesn't have any bearing on the PCS<->PHY link. If the
interface is in in-band mode, then the SGMII control word exchange
should always happen.

> +		cfg_mac.mode = PRESTERA_MAC_MODE_SGMII;
> +		break;
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +	default:
> +		cfg_mac.speed = SPEED_1000;
> +		cfg_mac.duplex = DUPLEX_FULL;
> +		cfg_mac.inband = test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> +					  advertising);
> +		cfg_mac.mode = PRESTERA_MAC_MODE_1000BASE_X;
> +		break;
>  	}
>  
> +	err = prestera_port_cfg_mac_write(port, &cfg_mac);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static void prestera_pcs_an_restart(struct phylink_pcs *pcs)
> +{
> +}

No way to restart 1000base-X autoneg?

> @@ -530,25 +777,48 @@ static int prestera_create_ports(struct prestera_switch *sw)
>  static void prestera_port_handle_event(struct prestera_switch *sw,
>  				       struct prestera_event *evt, void *arg)
>  {
> +	struct prestera_port_mac_state smac;
> +	struct prestera_port_event *pevt;
>  	struct delayed_work *caching_dw;
>  	struct prestera_port *port;
>  
> -	port = prestera_find_port(sw, evt->port_evt.port_id);
> -	if (!port || !port->dev)
> -		return;
> -
> -	caching_dw = &port->cached_hw_stats.caching_dw;
> -
> -	prestera_ethtool_port_state_changed(port, &evt->port_evt);
> -
>  	if (evt->id == PRESTERA_PORT_EVENT_MAC_STATE_CHANGED) {
> +		pevt = &evt->port_evt;
> +		port = prestera_find_port(sw, pevt->port_id);
> +		if (!port || !port->dev)
> +			return;
> +
> +		caching_dw = &port->cached_hw_stats.caching_dw;
> +
> +		if (port->phy_link) {
> +			memset(&smac, 0, sizeof(smac));
> +			smac.valid = true;
> +			smac.oper = pevt->data.mac.oper;
> +			if (smac.oper) {
> +				smac.mode = pevt->data.mac.mode;
> +				smac.speed = pevt->data.mac.speed;
> +				smac.duplex = pevt->data.mac.duplex;
> +				smac.fc = pevt->data.mac.fc;
> +				smac.fec = pevt->data.mac.fec;
> +			}
> +			prestera_port_mac_state_cache_write(port, &smac);

I think you should be calling phylink_mac_change() here, rather than
below.

> +		}
> +
>  		if (port->state_mac.oper) {
> -			netif_carrier_on(port->dev);
> +			if (port->phy_link)
> +				phylink_mac_change(port->phy_link, true);
> +			else
> +				netif_carrier_on(port->dev);
> +
>  			if (!delayed_work_pending(caching_dw))
>  				queue_delayed_work(prestera_wq, caching_dw, 0);
>  		} else if (netif_running(port->dev) &&
>  			   netif_carrier_ok(port->dev)) {
> -			netif_carrier_off(port->dev);
> +			if (port->phy_link)
> +				phylink_mac_change(port->phy_link, false);
> +			else
> +				netif_carrier_off(port->dev);
> +
>  			if (delayed_work_pending(caching_dw))
>  				cancel_delayed_work(caching_dw);
>  		}
Andrew Lunn July 13, 2022, 9:52 p.m. UTC | #2
On Wed, Jul 13, 2022 at 09:05:21PM +0100, Russell King (Oracle) wrote:
> On Wed, Jul 13, 2022 at 08:20:13PM +0300, Oleksandr Mazur wrote:
> > For SFP port prestera driver will use kernel
> > phylink infrastucture to configure port mode based on
> > the module that has beed inserted
> > 
> > Co-developed-by: Yevhen Orlov <yevhen.orlov@plvision.eu>
> > Signed-off-by: Yevhen Orlov <yevhen.orlov@plvision.eu>
> > Co-developed-by: Taras Chornyi <taras.chornyi@plvision.eu>
> > Signed-off-by: Taras Chornyi <taras.chornyi@plvision.eu>
> > Signed-off-by: Oleksandr Mazur <oleksandr.mazur@plvision.eu>
> > 
> > PATCH V2:
> >   - fix mistreat of bitfield values as if they were bools.
> >   - remove phylink_config ifdefs.
> >   - remove obsolete phylink pcs / mac callbacks;
> >   - rework mac (/pcs) config to not look for speed / duplex
> >     parameters while link is not yet set up.
> >   - remove unused functions.
> >   - add phylink select cfg to prestera Kconfig.
> 
> I would appreciate answers to my questions, rather than just another
> patch submission. So I'll repeat my question in the hope of an answer:
> 
> First question which applies to everything in this patch is - why make
> phylink conditional for this driver?

Hi Oleksandr

I agree with Russell here. This driver should depend on PHYLINK and
remove all the #ifdefs. We try to avoid this sort of code, it hides
bugs and does not get compile tested very well etc.

You need to give us a good reason if you want to keep the code like this.

    Andrew
Oleksandr Mazur July 14, 2022, 8:56 a.m. UTC | #3
Hello Andrew, Russel,
Thanks for the input and sorry for the inconviniences.

>First question which applies to everything in this patch is - why make
>phylink conditional for this driver?

1. As for the phylink ifdefs:
The original idea was to support mac config on devices (DB boards) that don't have full sfp support on board, however we scrapped out
this idea as we could simply use fixed-link DTS configs; also due to this solution being non-upstream-friendly.
Please note that V2 holds no phylink ifdefs;

>In SGMII mode, it is normal for the advertising
>mask to indicate the media modes on the PHY to advertise
2. As for the SGMII mode, yes, Russel, you're right; V3 will hold a fix for this, and keep the inband enabled for SGMII.

>No way to restart 1000base-X autoneg?
3. As for AN restart, no, it's not yet supported by our FW as of now.

>I think you should be calling phylink_mac_change() here, rather than
>below.

4. V3 is gonna fix this, thanks for the input.
Russell King (Oracle) July 14, 2022, 12:39 p.m. UTC | #4
On Thu, Jul 14, 2022 at 08:56:26AM +0000, Oleksandr Mazur wrote:
> Hello Andrew, Russel,
> Thanks for the input and sorry for the inconviniences.
> 
> >First question which applies to everything in this patch is - why make
> >phylink conditional for this driver?
> 
> 1. As for the phylink ifdefs:
> The original idea was to support mac config on devices (DB boards) that don't have full sfp support on board, however we scrapped out
> this idea as we could simply use fixed-link DTS configs; also due to this solution being non-upstream-friendly.
> Please note that V2 holds no phylink ifdefs;

I didn't specifically ask about the ifdefs - I asked the general
question about "why make phylink conditional for this driver".
Yes, v1 had ifdefs. v2 does not, but phylink is _still_ conditional.
You are introduing lots of this pattern of code:

	if (blah->phy_link)
		do something phylink related
	else
		do something else

And I want to know why.

> >In SGMII mode, it is normal for the advertising
> >mask to indicate the media modes on the PHY to advertise
> 2. As for the SGMII mode, yes, Russel, you're right; V3 will hold a fix for this, and keep the inband enabled for SGMII.
> 
> >No way to restart 1000base-X autoneg?
> 3. As for AN restart, no, it's not yet supported by our FW as of now.

Maybe put a comment in the code to that effect?

> >I think you should be calling phylink_mac_change() here, rather than
> >below.
> 
> 4. V3 is gonna fix this, thanks for the input.

Thanks.
Andrew Lunn July 14, 2022, 1:06 p.m. UTC | #5
> > >No way to restart 1000base-X autoneg?
> > 3. As for AN restart, no, it's not yet supported by our FW as of now.
> 
> Maybe put a comment in the code to that effect?

And also think about forward/backward compatibility. At some point
your FW will support it. Do you need to do anything now in order to
smoothly add support for it in the future?

	 Andrew
Oleksandr Mazur July 15, 2022, 9:33 a.m. UTC | #6
Hello Russel,

First of all - sorry for the inconveniences with the rush of new versioned-patches, my bad; won't happen again

As for the comments:

> I didn't specifically ask about the ifdefs - I asked the general
> question about "why make phylink conditional for this driver".
> Yes, v1 had ifdefs. v2 does not, but phylink is _still_ conditional.
> You are introduing lots of this pattern of code:

Sorry i think i misunderstood your question of the <conditional> use of phylink driver.
The reason is the following: the current state of things with ports is that PHY ports are controlled by FW,
and we're using phylink only for SFP ports. We don't have an PHY regs read/write invocation interfaces from driver right now,
and thus PHY ports control is limited to controlling them from FW side only, for now.

> 3. As for AN restart, no, it's not yet supported by our FW as of now.
> Maybe put a comment in the code to that effect?
That would make sense, i will add a comment in the next patch version.
Oleksandr Mazur July 15, 2022, 9:35 a.m. UTC | #7
Hello Andrew,

>And also think about forward/backward compatibility. At some point
>your FW will support it. Do you need to do anything now in order to
>smoothly add support for it in the future?

thanks for the comment, that's a good point;
We currently have AN restart HW API call, you are right, however whenever we add AN restart support for 1000basex,
we won't change the ABI nature of HW API calls between FW and Driver.
So yes, the transition for adding 1000basex AN restart functionality would be indeed smooth without ABI changes.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/marvell/prestera/Kconfig b/drivers/net/ethernet/marvell/prestera/Kconfig
index b6f20e2034c6..f2f7663c3d10 100644
--- a/drivers/net/ethernet/marvell/prestera/Kconfig
+++ b/drivers/net/ethernet/marvell/prestera/Kconfig
@@ -8,6 +8,7 @@  config PRESTERA
 	depends on NET_SWITCHDEV && VLAN_8021Q
 	depends on BRIDGE || BRIDGE=n
 	select NET_DEVLINK
+	select PHYLINK
 	help
 	  This driver supports Marvell Prestera Switch ASICs family.
 
diff --git a/drivers/net/ethernet/marvell/prestera/prestera.h b/drivers/net/ethernet/marvell/prestera/prestera.h
index bff9651f0a89..832c27e0c284 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera.h
+++ b/drivers/net/ethernet/marvell/prestera/prestera.h
@@ -7,6 +7,7 @@ 
 #include <linux/notifier.h>
 #include <linux/skbuff.h>
 #include <linux/workqueue.h>
+#include <linux/phylink.h>
 #include <net/devlink.h>
 #include <uapi/linux/if_ether.h>
 
@@ -92,6 +93,7 @@  struct prestera_lag {
 struct prestera_flow_block;
 
 struct prestera_port_mac_state {
+	bool valid;
 	u32 mode;
 	u32 speed;
 	bool oper;
@@ -151,6 +153,12 @@  struct prestera_port {
 	struct prestera_port_phy_config cfg_phy;
 	struct prestera_port_mac_state state_mac;
 	struct prestera_port_phy_state state_phy;
+
+	struct phylink_config phy_config;
+	struct phylink *phy_link;
+	struct phylink_pcs phylink_pcs;
+
+	rwlock_t state_mac_lock;
 };
 
 struct prestera_device {
@@ -291,6 +299,7 @@  struct prestera_switch {
 	u32 mtu_min;
 	u32 mtu_max;
 	u8 id;
+	struct device_node *np;
 	struct prestera_router *router;
 	struct prestera_lag *lags;
 	struct prestera_counter *counter;
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_ethtool.c b/drivers/net/ethernet/marvell/prestera/prestera_ethtool.c
index 40d5b89573bb..1da7ff889417 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_ethtool.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_ethtool.c
@@ -521,6 +521,9 @@  prestera_ethtool_get_link_ksettings(struct net_device *dev,
 	ecmd->base.speed = SPEED_UNKNOWN;
 	ecmd->base.duplex = DUPLEX_UNKNOWN;
 
+	if (port->phy_link)
+		return phylink_ethtool_ksettings_get(port->phy_link, ecmd);
+
 	ecmd->base.autoneg = port->autoneg ? AUTONEG_ENABLE : AUTONEG_DISABLE;
 
 	if (port->caps.type == PRESTERA_PORT_TYPE_TP) {
@@ -648,6 +651,9 @@  prestera_ethtool_set_link_ksettings(struct net_device *dev,
 	u8 adver_fec;
 	int err;
 
+	if (port->phy_link)
+		return phylink_ethtool_ksettings_set(port->phy_link, ecmd);
+
 	err = prestera_port_type_set(ecmd, port);
 	if (err)
 		return err;
@@ -782,28 +788,6 @@  static int prestera_ethtool_nway_reset(struct net_device *dev)
 	return -EINVAL;
 }
 
-void prestera_ethtool_port_state_changed(struct prestera_port *port,
-					 struct prestera_port_event *evt)
-{
-	struct prestera_port_mac_state *smac = &port->state_mac;
-
-	smac->oper = evt->data.mac.oper;
-
-	if (smac->oper) {
-		smac->mode = evt->data.mac.mode;
-		smac->speed = evt->data.mac.speed;
-		smac->duplex = evt->data.mac.duplex;
-		smac->fc = evt->data.mac.fc;
-		smac->fec = evt->data.mac.fec;
-	} else {
-		smac->mode = PRESTERA_MAC_MODE_MAX;
-		smac->speed = SPEED_UNKNOWN;
-		smac->duplex = DUPLEX_UNKNOWN;
-		smac->fc = 0;
-		smac->fec = 0;
-	}
-}
-
 const struct ethtool_ops prestera_ethtool_ops = {
 	.get_drvinfo = prestera_ethtool_get_drvinfo,
 	.get_link_ksettings = prestera_ethtool_get_link_ksettings,
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_ethtool.h b/drivers/net/ethernet/marvell/prestera/prestera_ethtool.h
index 9eb18e99dea6..bd5600886bc6 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_ethtool.h
+++ b/drivers/net/ethernet/marvell/prestera/prestera_ethtool.h
@@ -11,7 +11,4 @@  struct prestera_port;
 
 extern const struct ethtool_ops prestera_ethtool_ops;
 
-void prestera_ethtool_port_state_changed(struct prestera_port *port,
-					 struct prestera_port_event *evt);
-
 #endif /* _PRESTERA_ETHTOOL_H_ */
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
index ea5bd5069826..a0d9c186df6b 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_main.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c
@@ -9,6 +9,7 @@ 
 #include <linux/of.h>
 #include <linux/of_net.h>
 #include <linux/if_vlan.h>
+#include <linux/phylink.h>
 
 #include "prestera.h"
 #include "prestera_hw.h"
@@ -142,18 +143,24 @@  static int prestera_port_open(struct net_device *dev)
 	struct prestera_port_mac_config cfg_mac;
 	int err = 0;
 
-	if (port->caps.transceiver == PRESTERA_PORT_TCVR_SFP) {
-		err = prestera_port_cfg_mac_read(port, &cfg_mac);
-		if (!err) {
-			cfg_mac.admin = true;
-			err = prestera_port_cfg_mac_write(port, &cfg_mac);
-		}
+	if (port->phy_link) {
+		phylink_start(port->phy_link);
 	} else {
-		port->cfg_phy.admin = true;
-		err = prestera_hw_port_phy_mode_set(port, true, port->autoneg,
-						    port->cfg_phy.mode,
-						    port->adver_link_modes,
-						    port->cfg_phy.mdix);
+		if (port->caps.transceiver == PRESTERA_PORT_TCVR_SFP) {
+			err = prestera_port_cfg_mac_read(port, &cfg_mac);
+			if (!err) {
+				cfg_mac.admin = true;
+				err = prestera_port_cfg_mac_write(port,
+								  &cfg_mac);
+			}
+		} else {
+			port->cfg_phy.admin = true;
+			err = prestera_hw_port_phy_mode_set(port, true,
+							    port->autoneg,
+							    port->cfg_phy.mode,
+							    port->adver_link_modes,
+							    port->cfg_phy.mdix);
+		}
 	}
 
 	netif_start_queue(dev);
@@ -169,23 +176,254 @@  static int prestera_port_close(struct net_device *dev)
 
 	netif_stop_queue(dev);
 
-	if (port->caps.transceiver == PRESTERA_PORT_TCVR_SFP) {
+	if (port->phy_link) {
+		phylink_stop(port->phy_link);
+		phylink_disconnect_phy(port->phy_link);
 		err = prestera_port_cfg_mac_read(port, &cfg_mac);
 		if (!err) {
 			cfg_mac.admin = false;
 			prestera_port_cfg_mac_write(port, &cfg_mac);
 		}
 	} else {
-		port->cfg_phy.admin = false;
-		err = prestera_hw_port_phy_mode_set(port, false, port->autoneg,
-						    port->cfg_phy.mode,
-						    port->adver_link_modes,
-						    port->cfg_phy.mdix);
+		if (port->caps.transceiver == PRESTERA_PORT_TCVR_SFP) {
+			err = prestera_port_cfg_mac_read(port, &cfg_mac);
+			if (!err) {
+				cfg_mac.admin = false;
+				prestera_port_cfg_mac_write(port, &cfg_mac);
+			}
+		} else {
+			port->cfg_phy.admin = false;
+			err = prestera_hw_port_phy_mode_set(port, false, port->autoneg,
+							    port->cfg_phy.mode,
+							    port->adver_link_modes,
+							    port->cfg_phy.mdix);
+		}
+	}
+
+	return err;
+}
+
+static void
+prestera_port_mac_state_cache_read(struct prestera_port *port,
+				   struct prestera_port_mac_state *state)
+{
+	read_lock(&port->state_mac_lock);
+	*state = port->state_mac;
+	read_unlock(&port->state_mac_lock);
+}
+
+static void
+prestera_port_mac_state_cache_write(struct prestera_port *port,
+				    struct prestera_port_mac_state *state)
+{
+	write_lock(&port->state_mac_lock);
+	port->state_mac = *state;
+	write_unlock(&port->state_mac_lock);
+}
+
+static struct prestera_port *prestera_pcs_to_port(struct phylink_pcs *pcs)
+{
+	return container_of(pcs, struct prestera_port, phylink_pcs);
+}
+
+static void prestera_mac_config(struct phylink_config *config,
+				unsigned int an_mode,
+				const struct phylink_link_state *state)
+{
+}
+
+static void prestera_mac_link_down(struct phylink_config *config,
+				   unsigned int mode, phy_interface_t interface)
+{
+	struct net_device *ndev = to_net_dev(config->dev);
+	struct prestera_port *port = netdev_priv(ndev);
+	struct prestera_port_mac_state state_mac;
+
+	/* Invalidate. Parameters will update on next link event. */
+	memset(&state_mac, 0, sizeof(state_mac));
+	state_mac.valid = false;
+	prestera_port_mac_state_cache_write(port, &state_mac);
+}
+
+static void prestera_mac_link_up(struct phylink_config *config,
+				 struct phy_device *phy,
+				 unsigned int mode, phy_interface_t interface,
+				 int speed, int duplex,
+				 bool tx_pause, bool rx_pause)
+{
+}
+
+static struct phylink_pcs *prestera_mac_select_pcs(struct phylink_config *config,
+						   phy_interface_t interface)
+{
+	struct net_device *dev = to_net_dev(config->dev);
+	struct prestera_port *port = netdev_priv(dev);
+
+	return &port->phylink_pcs;
+}
+
+static void prestera_pcs_get_state(struct phylink_pcs *pcs,
+				   struct phylink_link_state *state)
+{
+	struct prestera_port *port = container_of(pcs, struct prestera_port,
+						  phylink_pcs);
+	struct prestera_port_mac_state smac;
+
+	prestera_port_mac_state_cache_read(port, &smac);
+
+	if (smac.valid) {
+		state->link = smac.oper ? 1 : 0;
+		/* AN is completed, when port is up */
+		state->an_complete = (smac.oper && port->autoneg) ? 1 : 0;
+		state->speed = smac.speed;
+		state->duplex = smac.duplex;
+	} else {
+		state->link = 0;
+		state->an_complete = 0;
+	}
+}
+
+static int prestera_pcs_config(struct phylink_pcs *pcs,
+			       unsigned int mode,
+			       phy_interface_t interface,
+			       const unsigned long *advertising,
+			       bool permit_pause_to_mac)
+{
+	struct prestera_port *port = port = prestera_pcs_to_port(pcs);
+	struct prestera_port_mac_config cfg_mac;
+	int err;
+
+	err = prestera_port_cfg_mac_read(port, &cfg_mac);
+	if (err)
+		return err;
+
+	cfg_mac.admin = true;
+	cfg_mac.fec = PRESTERA_PORT_FEC_OFF;
+
+	switch (interface) {
+	case PHY_INTERFACE_MODE_10GBASER:
+		cfg_mac.speed = SPEED_10000;
+		cfg_mac.inband = 0;
+		cfg_mac.mode = PRESTERA_MAC_MODE_SR_LR;
+		break;
+	case PHY_INTERFACE_MODE_2500BASEX:
+		cfg_mac.speed = SPEED_2500;
+		cfg_mac.duplex = DUPLEX_FULL;
+		cfg_mac.inband = test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+					  advertising);
+		cfg_mac.mode = PRESTERA_MAC_MODE_SGMII;
+		break;
+	case PHY_INTERFACE_MODE_SGMII:
+		cfg_mac.inband = test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+					  advertising);
+		cfg_mac.mode = PRESTERA_MAC_MODE_SGMII;
+		break;
+	case PHY_INTERFACE_MODE_1000BASEX:
+	default:
+		cfg_mac.speed = SPEED_1000;
+		cfg_mac.duplex = DUPLEX_FULL;
+		cfg_mac.inband = test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
+					  advertising);
+		cfg_mac.mode = PRESTERA_MAC_MODE_1000BASE_X;
+		break;
 	}
 
+	err = prestera_port_cfg_mac_write(port, &cfg_mac);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static void prestera_pcs_an_restart(struct phylink_pcs *pcs)
+{
+}
+
+static const struct phylink_mac_ops prestera_mac_ops = {
+	.validate = phylink_generic_validate,
+	.mac_select_pcs = prestera_mac_select_pcs,
+	.mac_config = prestera_mac_config,
+	.mac_link_down = prestera_mac_link_down,
+	.mac_link_up = prestera_mac_link_up,
+};
+
+static const struct phylink_pcs_ops prestera_pcs_ops = {
+	.pcs_get_state = prestera_pcs_get_state,
+	.pcs_config = prestera_pcs_config,
+	.pcs_an_restart = prestera_pcs_an_restart,
+};
+
+static int prestera_port_sfp_bind(struct prestera_port *port)
+{
+	struct prestera_switch *sw = port->sw;
+	struct device_node *ports, *node;
+	struct fwnode_handle *fwnode;
+	struct phylink *phy_link;
+	int err;
+
+	if (!sw->np)
+		return 0;
+
+	ports = of_find_node_by_name(sw->np, "ports");
+
+	for_each_child_of_node(ports, node) {
+		int num;
+
+		err = of_property_read_u32(node, "prestera,port-num", &num);
+		if (err) {
+			dev_err(sw->dev->dev,
+				"device node %pOF has no valid reg property: %d\n",
+				node, err);
+			goto out;
+		}
+
+		if (port->fp_id != num)
+			continue;
+
+		port->phylink_pcs.ops = &prestera_pcs_ops;
+
+		port->phy_config.dev = &port->dev->dev;
+		port->phy_config.type = PHYLINK_NETDEV;
+
+		fwnode = of_fwnode_handle(node);
+
+		__set_bit(PHY_INTERFACE_MODE_10GBASER,
+			  port->phy_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
+			  port->phy_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_SGMII,
+			  port->phy_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
+			  port->phy_config.supported_interfaces);
+
+		port->phy_config.mac_capabilities = MAC_1000 | MAC_2500FD | MAC_10000FD;
+
+		phy_link = phylink_create(&port->phy_config, fwnode,
+					  PHY_INTERFACE_MODE_INTERNAL,
+					  &prestera_mac_ops);
+		if (IS_ERR(phy_link)) {
+			netdev_err(port->dev, "failed to create phylink\n");
+			err = PTR_ERR(phy_link);
+			goto out;
+		}
+
+		port->phy_link = phy_link;
+		break;
+	}
+
+out:
+	of_node_put(ports);
 	return err;
 }
 
+static int prestera_port_sfp_unbind(struct prestera_port *port)
+{
+	if (port->phy_link)
+		phylink_destroy(port->phy_link);
+
+	return 0;
+}
+
 static netdev_tx_t prestera_port_xmit(struct sk_buff *skb,
 				      struct net_device *dev)
 {
@@ -380,8 +618,10 @@  static int prestera_port_create(struct prestera_switch *sw, u32 id)
 	dev->features |= NETIF_F_NETNS_LOCAL | NETIF_F_HW_TC;
 	dev->netdev_ops = &prestera_netdev_ops;
 	dev->ethtool_ops = &prestera_ethtool_ops;
+	SET_NETDEV_DEV(dev, sw->dev->dev);
 
-	netif_carrier_off(dev);
+	if (port->caps.transceiver != PRESTERA_PORT_TCVR_SFP)
+		netif_carrier_off(dev);
 
 	dev->mtu = min_t(unsigned int, sw->mtu_max, PRESTERA_MTU_DEFAULT);
 	dev->min_mtu = sw->mtu_min;
@@ -432,7 +672,7 @@  static int prestera_port_create(struct prestera_switch *sw, u32 id)
 		cfg_mac.admin = false;
 		cfg_mac.mode = PRESTERA_MAC_MODE_MAX;
 	}
-	cfg_mac.inband = false;
+	cfg_mac.inband = 0;
 	cfg_mac.speed = 0;
 	cfg_mac.duplex = DUPLEX_UNKNOWN;
 	cfg_mac.fec = PRESTERA_PORT_FEC_OFF;
@@ -474,8 +714,13 @@  static int prestera_port_create(struct prestera_switch *sw, u32 id)
 
 	prestera_devlink_port_set(port);
 
+	err = prestera_port_sfp_bind(port);
+	if (err)
+		goto err_sfp_bind;
+
 	return 0;
 
+err_sfp_bind:
 err_register_netdev:
 	prestera_port_list_del(port);
 err_port_init:
@@ -521,8 +766,10 @@  static int prestera_create_ports(struct prestera_switch *sw)
 	return 0;
 
 err_port_create:
-	list_for_each_entry_safe(port, tmp, &sw->port_list, list)
+	list_for_each_entry_safe(port, tmp, &sw->port_list, list) {
+		prestera_port_sfp_unbind(port);
 		prestera_port_destroy(port);
+	}
 
 	return err;
 }
@@ -530,25 +777,48 @@  static int prestera_create_ports(struct prestera_switch *sw)
 static void prestera_port_handle_event(struct prestera_switch *sw,
 				       struct prestera_event *evt, void *arg)
 {
+	struct prestera_port_mac_state smac;
+	struct prestera_port_event *pevt;
 	struct delayed_work *caching_dw;
 	struct prestera_port *port;
 
-	port = prestera_find_port(sw, evt->port_evt.port_id);
-	if (!port || !port->dev)
-		return;
-
-	caching_dw = &port->cached_hw_stats.caching_dw;
-
-	prestera_ethtool_port_state_changed(port, &evt->port_evt);
-
 	if (evt->id == PRESTERA_PORT_EVENT_MAC_STATE_CHANGED) {
+		pevt = &evt->port_evt;
+		port = prestera_find_port(sw, pevt->port_id);
+		if (!port || !port->dev)
+			return;
+
+		caching_dw = &port->cached_hw_stats.caching_dw;
+
+		if (port->phy_link) {
+			memset(&smac, 0, sizeof(smac));
+			smac.valid = true;
+			smac.oper = pevt->data.mac.oper;
+			if (smac.oper) {
+				smac.mode = pevt->data.mac.mode;
+				smac.speed = pevt->data.mac.speed;
+				smac.duplex = pevt->data.mac.duplex;
+				smac.fc = pevt->data.mac.fc;
+				smac.fec = pevt->data.mac.fec;
+			}
+			prestera_port_mac_state_cache_write(port, &smac);
+		}
+
 		if (port->state_mac.oper) {
-			netif_carrier_on(port->dev);
+			if (port->phy_link)
+				phylink_mac_change(port->phy_link, true);
+			else
+				netif_carrier_on(port->dev);
+
 			if (!delayed_work_pending(caching_dw))
 				queue_delayed_work(prestera_wq, caching_dw, 0);
 		} else if (netif_running(port->dev) &&
 			   netif_carrier_ok(port->dev)) {
-			netif_carrier_off(port->dev);
+			if (port->phy_link)
+				phylink_mac_change(port->phy_link, false);
+			else
+				netif_carrier_off(port->dev);
+
 			if (delayed_work_pending(caching_dw))
 				cancel_delayed_work(caching_dw);
 		}
@@ -571,19 +841,20 @@  static void prestera_event_handlers_unregister(struct prestera_switch *sw)
 static int prestera_switch_set_base_mac_addr(struct prestera_switch *sw)
 {
 	struct device_node *base_mac_np;
-	struct device_node *np;
 	int ret;
 
-	np = of_find_compatible_node(NULL, NULL, "marvell,prestera");
-	base_mac_np = of_parse_phandle(np, "base-mac-provider", 0);
+	if (sw->np) {
+		base_mac_np = of_parse_phandle(sw->np, "base-mac-provider", 0);
+		if (base_mac_np) {
+			ret = of_get_mac_address(base_mac_np, sw->base_mac);
+			of_node_put(base_mac_np);
+		}
+	}
 
-	ret = of_get_mac_address(base_mac_np, sw->base_mac);
-	if (ret) {
+	if (!is_valid_ether_addr(sw->base_mac) || ret) {
 		eth_random_addr(sw->base_mac);
 		dev_info(prestera_dev(sw), "using random base mac address\n");
 	}
-	of_node_put(base_mac_np);
-	of_node_put(np);
 
 	return prestera_hw_switch_mac_set(sw, sw->base_mac);
 }
@@ -1083,6 +1354,8 @@  static int prestera_switch_init(struct prestera_switch *sw)
 {
 	int err;
 
+	sw->np = of_find_compatible_node(NULL, NULL, "marvell,prestera");
+
 	err = prestera_hw_switch_init(sw);
 	if (err) {
 		dev_err(prestera_dev(sw), "Failed to init Switch device\n");
@@ -1183,6 +1456,7 @@  static void prestera_switch_fini(struct prestera_switch *sw)
 	prestera_router_fini(sw);
 	prestera_netdev_event_handler_unregister(sw);
 	prestera_hw_switch_fini(sw);
+	of_node_put(sw->np);
 }
 
 int prestera_device_register(struct prestera_device *dev)