| Message ID | 20211124083915.2223065-1-horatiu.vultur@microchip.com |
|---|---|
| Headers | show |
| Series | net: lan966x: Add lan966x switch driver | expand |
Hi, On Wed, Nov 24, 2021 at 09:39:12AM +0100, Horatiu Vultur wrote: > +static int lan966x_port_open(struct net_device *dev) > +{ > + struct lan966x_port *port = netdev_priv(dev); > + struct lan966x *lan966x = port->lan966x; > + int err; > + > + if (port->serdes) { > + err = phy_set_mode_ext(port->serdes, PHY_MODE_ETHERNET, > + port->config.phy_mode); > + if (err) { > + netdev_err(dev, "Could not set mode of SerDes\n"); > + return err; > + } > + } This could be done in the mac_prepare() method. > +static void lan966x_cleanup_ports(struct lan966x *lan966x) > +{ > + struct lan966x_port *port; > + int portno; > + > + for (portno = 0; portno < lan966x->num_phys_ports; portno++) { > + port = lan966x->ports[portno]; > + if (!port) > + continue; > + > + if (port->phylink) { > + rtnl_lock(); > + lan966x_port_stop(port->dev); > + rtnl_unlock(); > + phylink_destroy(port->phylink); > + port->phylink = NULL; > + } > + > + if (port->fwnode) > + fwnode_handle_put(port->fwnode); > + > + if (port->dev) > + unregister_netdev(port->dev); This doesn't look like the correct sequence to me. Shouldn't the net device be unregistered first, which will take the port down by doing so and make it unavailable to userspace to further manipulate. Then we should start tearing other stuff down such as destroying phylink and disabling interrupts (in the caller of this.) Don't you need to free the netdev as well at some point? > static int lan966x_probe_port(struct lan966x *lan966x, u32 p, > - phy_interface_t phy_mode) > + phy_interface_t phy_mode, > + struct fwnode_handle *portnp) > { ... > + port->phylink_config.dev = &port->dev->dev; > + port->phylink_config.type = PHYLINK_NETDEV; > + port->phylink_config.pcs_poll = true; > + port->phylink_pcs.poll = true; You don't need to set both of these - please omit port->phylink_config.pcs_poll. > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h > index 7a1ff9d19fbf..ce2798db0449 100644 > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h ... > @@ -44,15 +58,48 @@ struct lan966x { > void __iomem *regs[NUM_TARGETS]; > > int shared_queue_sz; > + > + /* interrupts */ > + int xtr_irq; > +}; > + > +struct lan966x_port_config { > + phy_interface_t portmode; > + phy_interface_t phy_mode; What is the difference between "portmode" and "phy_mode"? Does it matter if port->config.phy_mode get zeroed when lan966x_port_pcs_set() is called from lan966x_pcs_config()? It looks to me like the first call will clear phy_mode, setting it to PHY_INTERFACE_MODE_NA from that point on. > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_port.c b/drivers/net/ethernet/microchip/lan966x/lan966x_port.c > new file mode 100644 > index 000000000000..ca1b0c8d1bf5 > --- /dev/null > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_port.c > @@ -0,0 +1,422 @@ ... > +void lan966x_port_status_get(struct lan966x_port *port, > + struct phylink_link_state *state) > +{ > + struct lan966x *lan966x = port->lan966x; > + u16 lp_adv, ld_adv; > + bool link_down; > + u16 bmsr = 0; > + u32 val; > + > + val = lan_rd(lan966x, DEV_PCS1G_STICKY(port->chip_port)); > + link_down = DEV_PCS1G_STICKY_LINK_DOWN_STICKY_GET(val); > + if (link_down) > + lan_wr(val, lan966x, DEV_PCS1G_STICKY(port->chip_port)); > + > + /* Get both current Link and Sync status */ > + val = lan_rd(lan966x, DEV_PCS1G_LINK_STATUS(port->chip_port)); > + state->link = DEV_PCS1G_LINK_STATUS_LINK_STATUS_GET(val) && > + DEV_PCS1G_LINK_STATUS_SYNC_STATUS_GET(val); > + state->link &= !link_down; > + > + if (port->config.portmode == PHY_INTERFACE_MODE_1000BASEX) > + state->speed = SPEED_1000; > + else if (port->config.portmode == PHY_INTERFACE_MODE_2500BASEX) > + state->speed = SPEED_2500; Why not use state->interface? state->interface will be the currently configured interface mode (which should be the same as your port->config.portmode.) > + > + state->duplex = DUPLEX_FULL; Also, what is the purpose of initialising state->speed and state->duplex here? phylink_mii_c22_pcs_decode_state() will do that for you when decoding the advertisements. If it's to deal with autoneg disabled, then it ought to be conditional on autoneg being disabled and the link being up. > + > + /* Get PCS ANEG status register */ > + val = lan_rd(lan966x, DEV_PCS1G_ANEG_STATUS(port->chip_port)); > + > + /* Aneg complete provides more information */ > + if (DEV_PCS1G_ANEG_STATUS_ANEG_COMPLETE_GET(val)) { > + lp_adv = DEV_PCS1G_ANEG_STATUS_LP_ADV_GET(val); > + state->an_complete = true; > + > + bmsr |= state->link ? BMSR_LSTATUS : 0; > + bmsr |= state->an_complete; Shouldn't this be setting BMSR_ANEGCOMPLETE? > + > + if (port->config.portmode == PHY_INTERFACE_MODE_SGMII) { > + phylink_mii_c22_pcs_decode_state(state, bmsr, lp_adv); > + } else { > + val = lan_rd(lan966x, DEV_PCS1G_ANEG_CFG(port->chip_port)); > + ld_adv = DEV_PCS1G_ANEG_CFG_ADV_ABILITY_GET(val); > + phylink_mii_c22_pcs_decode_state(state, bmsr, ld_adv); > + } This looks like it can be improved: if (DEV_PCS1G_ANEG_STATUS_ANEG_COMPLETE_GET(val)) { state->an_complete = true; bmsr |= state->link ? BMSR_LSTATUS : 0; bmsr |= BMSR_ANEGCOMPLETE; if (state->interface == PHY_INTERFACE_MODE_SGMII) { lp_adv = DEV_PCS1G_ANEG_STATUS_LP_ADV_GET(val); } else { val = lan_rd(lan966x, DEV_PCS1G_ANEG_CFG(port->chip_port)); lp_adv = DEV_PCS1G_ANEG_CFG_ADV_ABILITY_GET(val); } phylink_mii_c22_pcs_decode_state(state, bmsr, lp_adv); } I'm not sure that the non-SGMII code is actually correct though. Which advertisement are you extracting by reading the DEV_PCS1G_ANEG_CFG register and extracting DEV_PCS1G_ANEG_CFG_ADV_ABILITY_GET ? From the code in lan966x_port_pcs_set(), it suggests this is our advertisement, but it's supposed to always be the link partner's advertisement being passed to phylink_mii_c22_pcs_decode_state(). > +int lan966x_port_pcs_set(struct lan966x_port *port, > + struct lan966x_port_config *config) > +{ ... > + port->config = *config; As mentioned elsewhere, "config" won't have phy_mode set, so this clears port->config.phymode to PHY_INTERFACE_MODE_NA, which I think will cause e.g. lan966x_port_link_up() not to behave as intended. Thanks.
The 11/24/2021 10:20, Russell King (Oracle) wrote: > > Hi, Hi Russel, > > On Wed, Nov 24, 2021 at 09:39:12AM +0100, Horatiu Vultur wrote: > > +static int lan966x_port_open(struct net_device *dev) > > +{ > > + struct lan966x_port *port = netdev_priv(dev); > > + struct lan966x *lan966x = port->lan966x; > > + int err; > > + > > + if (port->serdes) { > > + err = phy_set_mode_ext(port->serdes, PHY_MODE_ETHERNET, > > + port->config.phy_mode); > > + if (err) { > > + netdev_err(dev, "Could not set mode of SerDes\n"); > > + return err; > > + } > > + } > > This could be done in the mac_prepare() method. Yes, I will move this in mac_prepare() > > > +static void lan966x_cleanup_ports(struct lan966x *lan966x) > > +{ > > + struct lan966x_port *port; > > + int portno; > > + > > + for (portno = 0; portno < lan966x->num_phys_ports; portno++) { > > + port = lan966x->ports[portno]; > > + if (!port) > > + continue; > > + > > + if (port->phylink) { > > + rtnl_lock(); > > + lan966x_port_stop(port->dev); > > + rtnl_unlock(); > > + phylink_destroy(port->phylink); > > + port->phylink = NULL; > > + } > > + > > + if (port->fwnode) > > + fwnode_handle_put(port->fwnode); > > + > > + if (port->dev) > > + unregister_netdev(port->dev); > > This doesn't look like the correct sequence to me. Shouldn't the net > device be unregistered first, which will take the port down by doing > so and make it unavailable to userspace to further manipulate. Then > we should start tearing other stuff down such as destroying phylink > and disabling interrupts (in the caller of this.) I can change the order as you suggested. Regarding the interrupts, shouldn't they be first disable and then do all the teardown? > > Don't you need to free the netdev as well at some point? It is not needed because they are allocated using devm_alloc_etherdev_mqs > > > static int lan966x_probe_port(struct lan966x *lan966x, u32 p, > > - phy_interface_t phy_mode) > > + phy_interface_t phy_mode, > > + struct fwnode_handle *portnp) > > { > ... > > + port->phylink_config.dev = &port->dev->dev; > > + port->phylink_config.type = PHYLINK_NETDEV; > > + port->phylink_config.pcs_poll = true; > > + port->phylink_pcs.poll = true; > > You don't need to set both of these - please omit > port->phylink_config.pcs_poll. I will remove it. > > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h > > index 7a1ff9d19fbf..ce2798db0449 100644 > > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h > > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h > ... > > @@ -44,15 +58,48 @@ struct lan966x { > > void __iomem *regs[NUM_TARGETS]; > > > > int shared_queue_sz; > > + > > + /* interrupts */ > > + int xtr_irq; > > +}; > > + > > +struct lan966x_port_config { > > + phy_interface_t portmode; > > + phy_interface_t phy_mode; > > What is the difference between "portmode" and "phy_mode"? Does it matter > if port->config.phy_mode get zeroed when lan966x_port_pcs_set() is > called from lan966x_pcs_config()? It looks to me like the first call > will clear phy_mode, setting it to PHY_INTERFACE_MODE_NA from that point > on. The purpose was to use portmode to configure the MAC and the phy_mode to configure the serdes. There are small issues regarding this which will be fix in the next series also I will add some comments just to make it clear. Actually, port->config.phy_mode will not get zeroed. Because right after the memset it follows: 'config = port->config'. > > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_port.c b/drivers/net/ethernet/microchip/lan966x/lan966x_port.c > > new file mode 100644 > > index 000000000000..ca1b0c8d1bf5 > > --- /dev/null > > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_port.c > > @@ -0,0 +1,422 @@ > ... > > +void lan966x_port_status_get(struct lan966x_port *port, > > + struct phylink_link_state *state) > > +{ > > + struct lan966x *lan966x = port->lan966x; > > + u16 lp_adv, ld_adv; > > + bool link_down; > > + u16 bmsr = 0; > > + u32 val; > > + > > + val = lan_rd(lan966x, DEV_PCS1G_STICKY(port->chip_port)); > > + link_down = DEV_PCS1G_STICKY_LINK_DOWN_STICKY_GET(val); > > + if (link_down) > > + lan_wr(val, lan966x, DEV_PCS1G_STICKY(port->chip_port)); > > + > > + /* Get both current Link and Sync status */ > > + val = lan_rd(lan966x, DEV_PCS1G_LINK_STATUS(port->chip_port)); > > + state->link = DEV_PCS1G_LINK_STATUS_LINK_STATUS_GET(val) && > > + DEV_PCS1G_LINK_STATUS_SYNC_STATUS_GET(val); > > + state->link &= !link_down; > > + > > + if (port->config.portmode == PHY_INTERFACE_MODE_1000BASEX) > > + state->speed = SPEED_1000; > > + else if (port->config.portmode == PHY_INTERFACE_MODE_2500BASEX) > > + state->speed = SPEED_2500; > > Why not use state->interface? state->interface will be the currently > configured interface mode (which should be the same as your > port->config.portmode.) I will use state->interface. > > > + > > + state->duplex = DUPLEX_FULL; > > Also, what is the purpose of initialising state->speed and state->duplex > here? phylink_mii_c22_pcs_decode_state() will do that for you when > decoding the advertisements. > > If it's to deal with autoneg disabled, then it ought to be conditional on > autoneg being disabled and the link being up. It was the case for autoneg disabled. > > > + > > + /* Get PCS ANEG status register */ > > + val = lan_rd(lan966x, DEV_PCS1G_ANEG_STATUS(port->chip_port)); > > + > > + /* Aneg complete provides more information */ > > + if (DEV_PCS1G_ANEG_STATUS_ANEG_COMPLETE_GET(val)) { > > + lp_adv = DEV_PCS1G_ANEG_STATUS_LP_ADV_GET(val); > > + state->an_complete = true; > > + > > + bmsr |= state->link ? BMSR_LSTATUS : 0; > > + bmsr |= state->an_complete; > > Shouldn't this be setting BMSR_ANEGCOMPLETE? That was a silly mistake from my side. > > > + > > + if (port->config.portmode == PHY_INTERFACE_MODE_SGMII) { > > + phylink_mii_c22_pcs_decode_state(state, bmsr, lp_adv); > > + } else { > > + val = lan_rd(lan966x, DEV_PCS1G_ANEG_CFG(port->chip_port)); > > + ld_adv = DEV_PCS1G_ANEG_CFG_ADV_ABILITY_GET(val); > > + phylink_mii_c22_pcs_decode_state(state, bmsr, ld_adv); > > + } > > This looks like it can be improved: > > if (DEV_PCS1G_ANEG_STATUS_ANEG_COMPLETE_GET(val)) { > state->an_complete = true; > > bmsr |= state->link ? BMSR_LSTATUS : 0; > bmsr |= BMSR_ANEGCOMPLETE; > > if (state->interface == PHY_INTERFACE_MODE_SGMII) { > lp_adv = DEV_PCS1G_ANEG_STATUS_LP_ADV_GET(val); > } else { > val = lan_rd(lan966x, DEV_PCS1G_ANEG_CFG(port->chip_port)); > lp_adv = DEV_PCS1G_ANEG_CFG_ADV_ABILITY_GET(val); > } > > phylink_mii_c22_pcs_decode_state(state, bmsr, lp_adv); > } > > I'm not sure that the non-SGMII code is actually correct though. Which > advertisement are you extracting by reading the DEV_PCS1G_ANEG_CFG > register and extracting DEV_PCS1G_ANEG_CFG_ADV_ABILITY_GET ? From the > code in lan966x_port_pcs_set(), it suggests this is our advertisement, > but it's supposed to always be the link partner's advertisement being > passed to phylink_mii_c22_pcs_decode_state(). Actually, like you mentioned it needs to be link partner's advertisement so that code can be simplified more: if (DEV_PCS1G_ANEG_STATUS_ANEG_COMPLETE_GET(val)) { state->an_complete = true; bmsr |= state->link ? BMSR_LSTATUS : 0; bmsr |= BMSR_ANEGCOMPLETE; lp_adv = DEV_PCS1G_ANEG_STATUS_LP_ADV_GET(val); phylink_mii_c22_pcs_decode_state(state, bmsr, lp_adv); } Because inside phylink_mii_c22_pcs_decode_state, more precisely in phylink_decode_c37_work, state->advertising will have the local advertising. > > > +int lan966x_port_pcs_set(struct lan966x_port *port, > > + struct lan966x_port_config *config) > > +{ > ... > > + port->config = *config; > > As mentioned elsewhere, "config" won't have phy_mode set, so this clears > port->config.phymode to PHY_INTERFACE_MODE_NA, which I think will cause > e.g. lan966x_port_link_up() not to behave as intended. Actually, the "config" will still have the phy_mode because config will get teh value of port->config and after that some fields are changed. > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Nov 24, 2021 at 03:58:00PM +0100, Horatiu Vultur wrote: > > This doesn't look like the correct sequence to me. Shouldn't the net > > device be unregistered first, which will take the port down by doing > > so and make it unavailable to userspace to further manipulate. Then > > we should start tearing other stuff down such as destroying phylink > > and disabling interrupts (in the caller of this.) > > I can change the order as you suggested. > Regarding the interrupts, shouldn't they be first disable and then do > all the teardown? Depends if you need them disabled before you do the teardown. However, what would be the effect of disabling interrupts while the user still has the ability to interact with the port - that is the main point. Generally the teardown should be the reverse of setup - where it's now accepted that all setup should be done prior to user publication. So, user interfaces should be removed and then teardown should proceed. > > What is the difference between "portmode" and "phy_mode"? Does it matter > > if port->config.phy_mode get zeroed when lan966x_port_pcs_set() is > > called from lan966x_pcs_config()? It looks to me like the first call > > will clear phy_mode, setting it to PHY_INTERFACE_MODE_NA from that point > > on. > > The purpose was to use portmode to configure the MAC and the phy_mode > to configure the serdes. There are small issues regarding this which > will be fix in the next series also I will add some comments just to > make it clear. > > Actually, port->config.phy_mode will not get zeroed. Because right after > the memset it follows: 'config = port->config'. Ah, missed that, thanks. However, why should portmode and phy_mode be different? > Actually, like you mentioned it needs to be link partner's advertisement > so that code can be simplified more: > > if (DEV_PCS1G_ANEG_STATUS_ANEG_COMPLETE_GET(val)) { > state->an_complete = true; > > bmsr |= state->link ? BMSR_LSTATUS : 0; > bmsr |= BMSR_ANEGCOMPLETE; > > lp_adv = DEV_PCS1G_ANEG_STATUS_LP_ADV_GET(val); > phylink_mii_c22_pcs_decode_state(state, bmsr, lp_adv); > } > > Because inside phylink_mii_c22_pcs_decode_state, more precisely in > phylink_decode_c37_work, state->advertising will have the local > advertising. Correct.
The 11/24/2021 15:03, Russell King (Oracle) wrote: > > On Wed, Nov 24, 2021 at 03:58:00PM +0100, Horatiu Vultur wrote: > > > This doesn't look like the correct sequence to me. Shouldn't the net > > > device be unregistered first, which will take the port down by doing > > > so and make it unavailable to userspace to further manipulate. Then > > > we should start tearing other stuff down such as destroying phylink > > > and disabling interrupts (in the caller of this.) > > > > I can change the order as you suggested. > > Regarding the interrupts, shouldn't they be first disable and then do > > all the teardown? > > Depends if you need them disabled before you do the teardown. However, > what would be the effect of disabling interrupts while the user still > has the ability to interact with the port - that is the main point. > > Generally the teardown should be the reverse of setup - where it's now > accepted that all setup should be done prior to user publication. So, > user interfaces should be removed and then teardown should proceed. Yes, I get your point. I will remove the interface and then I will disable the interrupts. > > > > What is the difference between "portmode" and "phy_mode"? Does it matter > > > if port->config.phy_mode get zeroed when lan966x_port_pcs_set() is > > > called from lan966x_pcs_config()? It looks to me like the first call > > > will clear phy_mode, setting it to PHY_INTERFACE_MODE_NA from that point > > > on. > > > > The purpose was to use portmode to configure the MAC and the phy_mode > > to configure the serdes. There are small issues regarding this which > > will be fix in the next series also I will add some comments just to > > make it clear. > > > > Actually, port->config.phy_mode will not get zeroed. Because right after > > the memset it follows: 'config = port->config'. > > Ah, missed that, thanks. However, why should portmode and phy_mode be > different? Because the serdes knows only few modes(QSGMII, SGMII, GMII) and this information will come from DT. So I would like to have one variable that will configure the serdes ('phy_mode') and one will configure the MAC ('portmode'). > > > Actually, like you mentioned it needs to be link partner's advertisement > > so that code can be simplified more: > > > > if (DEV_PCS1G_ANEG_STATUS_ANEG_COMPLETE_GET(val)) { > > state->an_complete = true; > > > > bmsr |= state->link ? BMSR_LSTATUS : 0; > > bmsr |= BMSR_ANEGCOMPLETE; > > > > lp_adv = DEV_PCS1G_ANEG_STATUS_LP_ADV_GET(val); > > phylink_mii_c22_pcs_decode_state(state, bmsr, lp_adv); > > } > > > > Because inside phylink_mii_c22_pcs_decode_state, more precisely in > > phylink_decode_c37_work, state->advertising will have the local > > advertising. > > Correct. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Nov 24, 2021 at 04:43:23PM +0100, Horatiu Vultur wrote: > > > Actually, port->config.phy_mode will not get zeroed. Because right after > > > the memset it follows: 'config = port->config'. > > > > Ah, missed that, thanks. However, why should portmode and phy_mode be > > different? > > Because the serdes knows only few modes(QSGMII, SGMII, GMII) and this > information will come from DT. So I would like to have one variable that > will configure the serdes ('phy_mode') and one will configure the MAC > ('portmode'). I don't follow why you need this to be different. Isn't the point of interfaces such as phy_set_mode_ext() such that we can achieve independence of the details of what is behind that interface - so, as it takes a PHY interface mode, if we're operating in 1000BASE-X, we pass that to phy_set_mode_ext(). It is then the responsibility of the Serdes PHY driver to decide that means "sgmii" mode for the Serdes? For example, the Marvell CP110 comphy driver does this: if (submode == PHY_INTERFACE_MODE_1000BASEX) submode = PHY_INTERFACE_MODE_SGMII; because the serdes phy settings for PHY_INTERFACE_MODE_1000BASEX are no different from PHY_INTERFACE_MODE_SGMII - and that detail is hidden from the network driver. The next question this brings up is... you're setting all the different interface modes in phylink_config.supported_interfaces, which basically means you're giving permission for phylink to switch between any of those modes. So, what if the serdes is in QSGMII mode but phylink requests SGMII mode. Doesn't your driver architecture mean that if you're in QSGMII mode you can't use SGMII or GMII mode? Is there some kind of restriction that you need to split this, or is this purely down to the way this driver has been written? I don't see any other driver in the kernel making this kind of split.
The 11/24/2021 16:04, Russell King (Oracle) wrote: > On Wed, Nov 24, 2021 at 04:43:23PM +0100, Horatiu Vultur wrote: > > > > Actually, port->config.phy_mode will not get zeroed. Because right after > > > > the memset it follows: 'config = port->config'. > > > > > > Ah, missed that, thanks. However, why should portmode and phy_mode be > > > different? > > > > Because the serdes knows only few modes(QSGMII, SGMII, GMII) and this > > information will come from DT. So I would like to have one variable that > > will configure the serdes ('phy_mode') and one will configure the MAC > > ('portmode'). > > I don't follow why you need this to be different. > > Isn't the point of interfaces such as phy_set_mode_ext() such that we > can achieve independence of the details of what is behind that > interface - so, as it takes a PHY interface mode, if we're operating > in 1000BASE-X, we pass that to phy_set_mode_ext(). It is then the > responsibility of the Serdes PHY driver to decide that means "sgmii" > mode for the Serdes? I have kept the responsability in the network driver to decide which interface should for serdes, but I can change that as you suggested. > > For example, the Marvell CP110 comphy driver does this: > > if (submode == PHY_INTERFACE_MODE_1000BASEX) > submode = PHY_INTERFACE_MODE_SGMII; > > because the serdes phy settings for PHY_INTERFACE_MODE_1000BASEX are > no different from PHY_INTERFACE_MODE_SGMII - and that detail is hidden > from the network driver. Yes, I will add a similar check in the serdes driver. > > The next question this brings up is... you're setting all the different > interface modes in phylink_config.supported_interfaces, which basically > means you're giving permission for phylink to switch between any of > those modes. So, what if the serdes is in QSGMII mode but phylink > requests SGMII mode. Doesn't your driver architecture mean that if > you're in QSGMII mode you can't use SGMII or GMII mode? > > Is there some kind of restriction that you need to split this, or is > this purely down to the way this driver has been written? It was just the way the driver has been written. > > I don't see any other driver in the kernel making this kind of split. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!