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 <[email protected]>
Signed-off-by: Yevhen Orlov <[email protected]>
Co-developed-by: Taras Chornyi <[email protected]>
Signed-off-by: Taras Chornyi <[email protected]>
Signed-off-by: Oleksandr Mazur <[email protected]>
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(-)
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)
--
2.17.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 <[email protected]>
> Signed-off-by: Yevhen Orlov <[email protected]>
> Co-developed-by: Taras Chornyi <[email protected]>
> Signed-off-by: Taras Chornyi <[email protected]>
> Signed-off-by: Oleksandr Mazur <[email protected]>
>
> 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);
> }
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
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 <[email protected]>
> > Signed-off-by: Yevhen Orlov <[email protected]>
> > Co-developed-by: Taras Chornyi <[email protected]>
> > Signed-off-by: Taras Chornyi <[email protected]>
> > Signed-off-by: Oleksandr Mazur <[email protected]>
> >
> > 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
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.
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> > >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
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.
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.