| Message ID | 20211117091858.1971414-1-horatiu.vultur@microchip.com |
|---|---|
| Headers | show |
| Series | net: lan966x: Add lan966x switch driver | expand |
Hi Horatio, On Wed, 2021-11-17 at 10:18 +0100, Horatiu Vultur wrote: > +static int lan966x_reset_switch(struct lan966x *lan966x) > +{ > + struct reset_control *reset; > + int val = 0; > + int ret; > + > + reset = devm_reset_control_get_shared(lan966x->dev, "switch"); > + if (IS_ERR(reset)) > + dev_warn(lan966x->dev, "Could not obtain switch reset: %ld\n", > + PTR_ERR(reset)); > + else > + reset_control_reset(reset); According to the device tree bindings, both resets are required. I'd expect this to return on error. Is there any chance of the device working with out the switch reset being triggered? > + > + reset = devm_reset_control_get_shared(lan966x->dev, "phy"); > + if (IS_ERR(reset)) { > + dev_warn(lan966x->dev, "Could not obtain phy reset: %ld\n", > + PTR_ERR(reset)); > + } else { > + reset_control_reset(reset); > + } Same as above. Consider printing errors with %pe or dev_err_probe(). > + lan_wr(SYS_RESET_CFG_CORE_ENA_SET(0), lan966x, SYS_RESET_CFG); > + lan_wr(SYS_RAM_INIT_RAM_INIT_SET(1), lan966x, SYS_RAM_INIT); > + ret = readx_poll_timeout(lan966x_ram_init, lan966x, > + val, (val & BIT(1)) == 0, READL_SLEEP_US, > + READL_TIMEOUT_US); > + if (ret) > + return ret; > + > + lan_wr(SYS_RESET_CFG_CORE_ENA_SET(1), lan966x, SYS_RESET_CFG); > + > + return 0; > +} > + > +static int lan966x_probe(struct platform_device *pdev) > +{ > + struct fwnode_handle *ports, *portnp; > + struct lan966x *lan966x; > + int err, i; > + > + lan966x = devm_kzalloc(&pdev->dev, sizeof(*lan966x), GFP_KERNEL); > + if (!lan966x) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, lan966x); > + lan966x->dev = &pdev->dev; > + > + ports = device_get_named_child_node(&pdev->dev, "ethernet-ports"); > + if (!ports) { > + dev_err(&pdev->dev, "no ethernet-ports child not found\n"); > + err = -ENODEV; > + goto out; No need to goto as long as there's just a "return err;" after the out: label. > + } > + > + err = lan966x_create_targets(pdev, lan966x); > + if (err) > + goto out; > + > + if (lan966x_reset_switch(lan966x)) { > + err = -EINVAL; This should propagate the error returned from lan966x_reset_switch() instead. > + goto out; > + } > + > + i = 0; > + fwnode_for_each_available_child_node(ports, portnp) > + ++i; > + > + lan966x->num_phys_ports = i; > + lan966x->ports = devm_kcalloc(&pdev->dev, lan966x->num_phys_ports, > + sizeof(struct lan966x_port *), > + GFP_KERNEL); if (!lan966x->ports) return -ENOMEM; regards Philipp
Hi, On Wed, Nov 17, 2021 at 10:18:56AM +0100, Horatiu Vultur wrote: > +static void lan966x_phylink_mac_link_state(struct phylink_config *config, > + struct phylink_link_state *state) > +{ > +} > + > +static void lan966x_phylink_mac_aneg_restart(struct phylink_config *config) > +{ > +} Since you always attach a PCS, it is not necessary to provide stubs for these functions. > +static int lan966x_pcs_config(struct phylink_pcs *pcs, > + unsigned int mode, > + phy_interface_t interface, > + const unsigned long *advertising, > + bool permit_pause_to_mac) > +{ > + struct lan966x_port *port = lan966x_pcs_to_port(pcs); > + struct lan966x_port_config config; > + int ret; > + > + memset(&config, 0, sizeof(config)); > + > + config = port->config; > + config.portmode = interface; > + config.inband = phylink_autoneg_inband(mode); > + config.autoneg = phylink_test(advertising, Autoneg); > + if (phylink_test(advertising, Pause)) > + config.pause_adv |= ADVERTISE_1000XPAUSE; > + if (phylink_test(advertising, Asym_Pause)) > + config.pause_adv |= ADVERTISE_1000XPSE_ASYM; There are patches around that add phylink_mii_c22_pcs_encode_advertisement() which will create the C22 advertisement for you. It would be good to get that patch merged so people can use it. That should also eliminate lan966x_get_aneg_word(), although I notice you need to set ADVERTISE_LPACK as well (can you check that please? Hardware should be managing that bit as it should only be set once the hardware has received the link partner's advertisement.) > +static void decode_cl37_word(u16 lp_abil, uint16_t ld_abil, > + struct lan966x_port_status *status) > +{ > + status->link = !(lp_abil & ADVERTISE_RFAULT) && status->link; > + status->an_complete = true; > + status->duplex = (ADVERTISE_1000XFULL & lp_abil) ? > + DUPLEX_FULL : DUPLEX_UNKNOWN; > + > + if ((ld_abil & ADVERTISE_1000XPAUSE) && > + (lp_abil & ADVERTISE_1000XPAUSE)) { > + status->pause = MLO_PAUSE_RX | MLO_PAUSE_TX; > + } else if ((ld_abil & ADVERTISE_1000XPSE_ASYM) && > + (lp_abil & ADVERTISE_1000XPSE_ASYM)) { > + status->pause |= (lp_abil & ADVERTISE_1000XPAUSE) ? > + MLO_PAUSE_TX : 0; > + status->pause |= (ld_abil & ADVERTISE_1000XPAUSE) ? > + MLO_PAUSE_RX : 0; > + } else { > + status->pause = MLO_PAUSE_NONE; > + } > +} We already have phylink_decode_c37_word() which will decode this for you, although it would need to be exported. Please re-use this code. > + > +static void decode_sgmii_word(u16 lp_abil, struct lan966x_port_status *status) > +{ > + status->an_complete = true; > + if (!(lp_abil & LPA_SGMII_LINK)) { > + status->link = false; > + return; > + } > + > + switch (lp_abil & LPA_SGMII_SPD_MASK) { > + case LPA_SGMII_10: > + status->speed = SPEED_10; > + break; > + case LPA_SGMII_100: > + status->speed = SPEED_100; > + break; > + case LPA_SGMII_1000: > + status->speed = SPEED_1000; > + break; > + default: > + status->link = false; > + return; > + } > + if (lp_abil & LPA_SGMII_FULL_DUPLEX) > + status->duplex = DUPLEX_FULL; > + else > + status->duplex = DUPLEX_HALF; > +} The above mentioned function will also handle SGMII as well. > +int lan966x_port_pcs_set(struct lan966x_port *port, > + struct lan966x_port_config *config) > +{ > + struct lan966x *lan966x = port->lan966x; > + bool sgmii = false, inband_aneg = false; > + int err; > + > + lan966x_port_link_down(port); > + > + if (config->inband) { > + if (config->portmode == PHY_INTERFACE_MODE_SGMII || > + config->portmode == PHY_INTERFACE_MODE_QSGMII) > + inband_aneg = true; /* Cisco-SGMII in-band-aneg */ > + else if (config->portmode == PHY_INTERFACE_MODE_1000BASEX && > + config->autoneg) > + inband_aneg = true; /* Clause-37 in-band-aneg */ > + > + if (config->speed > 0) { > + err = phy_set_speed(port->serdes, config->speed); > + if (err) > + return err; > + } > + > + } else { > + sgmii = true; /* Phy is connnected to the MAC */ This looks weird. SGMII can be in-band as well (and technically is in-band in its as-specified form.) > + } > + > + /* Choose SGMII or 1000BaseX/2500BaseX PCS mode */ > + lan_rmw(DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA_SET(sgmii), > + DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA, > + lan966x, DEV_PCS1G_MODE_CFG(port->chip_port)); With the code as you have it, what this means is if we specify SGMII + in-band, we end up configuring the port to be in 1000BaseX mode, so it's incapable of 10 and 100M speeds. This seems incorrect. Thanks.
Hi, By the way... On Wed, Nov 17, 2021 at 10:18:56AM +0100, Horatiu Vultur wrote: > + port->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE | > + MAC_10 | MAC_100 | MAC_1000FD | MAC_2500FD; > + > + __set_bit(PHY_INTERFACE_MODE_MII, > + port->phylink_config.supported_interfaces); > + __set_bit(PHY_INTERFACE_MODE_GMII, > + port->phylink_config.supported_interfaces); > + __set_bit(PHY_INTERFACE_MODE_SGMII, > + port->phylink_config.supported_interfaces); > + __set_bit(PHY_INTERFACE_MODE_QSGMII, > + port->phylink_config.supported_interfaces); > + __set_bit(PHY_INTERFACE_MODE_1000BASEX, > + port->phylink_config.supported_interfaces); > + __set_bit(PHY_INTERFACE_MODE_2500BASEX, > + port->phylink_config.supported_interfaces); ... > +const struct phylink_mac_ops lan966x_phylink_mac_ops = { > + .validate = phylink_generic_validate, Thank you for switching the driver to use phylink_generic_validate(), that's really very useful, and saves a chunk of code in your driver!
The 11/17/2021 10:52, Philipp Zabel wrote: > > Hi Horatio, Hi Phillip, > > On Wed, 2021-11-17 at 10:18 +0100, Horatiu Vultur wrote: > > +static int lan966x_reset_switch(struct lan966x *lan966x) > > +{ > > + struct reset_control *reset; > > + int val = 0; > > + int ret; > > + > > + reset = devm_reset_control_get_shared(lan966x->dev, "switch"); > > + if (IS_ERR(reset)) > > + dev_warn(lan966x->dev, "Could not obtain switch reset: %ld\n", > > + PTR_ERR(reset)); > > + else > > + reset_control_reset(reset); > > According to the device tree bindings, both resets are required. > I'd expect this to return on error. > Is there any chance of the device working with out the switch reset > being triggered? The only case that I see is if the bootloader triggers this switch reset and then when bootloader starts the kernel and doesn't set back the switch in reset. Is this a valid scenario or is a bug in the bootloader? > > > + > > + reset = devm_reset_control_get_shared(lan966x->dev, "phy"); > > + if (IS_ERR(reset)) { > > + dev_warn(lan966x->dev, "Could not obtain phy reset: %ld\n", > > + PTR_ERR(reset)); > > + } else { > > + reset_control_reset(reset); > > + } > > Same as above. > Consider printing errors with %pe or dev_err_probe(). > > > + lan_wr(SYS_RESET_CFG_CORE_ENA_SET(0), lan966x, SYS_RESET_CFG); > > + lan_wr(SYS_RAM_INIT_RAM_INIT_SET(1), lan966x, SYS_RAM_INIT); > > + ret = readx_poll_timeout(lan966x_ram_init, lan966x, > > + val, (val & BIT(1)) == 0, READL_SLEEP_US, > > + READL_TIMEOUT_US); > > + if (ret) > > + return ret; > > + > > + lan_wr(SYS_RESET_CFG_CORE_ENA_SET(1), lan966x, SYS_RESET_CFG); > > + > > + return 0; > > +} > > + > > +static int lan966x_probe(struct platform_device *pdev) > > +{ > > + struct fwnode_handle *ports, *portnp; > > + struct lan966x *lan966x; > > + int err, i; > > + > > + lan966x = devm_kzalloc(&pdev->dev, sizeof(*lan966x), GFP_KERNEL); > > + if (!lan966x) > > + return -ENOMEM; > > + > > + platform_set_drvdata(pdev, lan966x); > > + lan966x->dev = &pdev->dev; > > + > > + ports = device_get_named_child_node(&pdev->dev, "ethernet-ports"); > > + if (!ports) { > > + dev_err(&pdev->dev, "no ethernet-ports child not found\n"); > > + err = -ENODEV; > > + goto out; > > No need to goto as long as there's just a "return err;" after the out: > label. True, I will udate this. > > > + } > > + > > + err = lan966x_create_targets(pdev, lan966x); > > + if (err) > > + goto out; > > + > > + if (lan966x_reset_switch(lan966x)) { > > + err = -EINVAL; > > This should propagate the error returned from lan966x_reset_switch() > instead. I will fix it in the next version. > > > + goto out; > > + } > > + > > + i = 0; > > + fwnode_for_each_available_child_node(ports, portnp) > > + ++i; > > + > > + lan966x->num_phys_ports = i; > > + lan966x->ports = devm_kcalloc(&pdev->dev, lan966x->num_phys_ports, > > + sizeof(struct lan966x_port *), > > + GFP_KERNEL); > > if (!lan966x->ports) > return -ENOMEM; Good catch. > > regards > Philipp
The 11/17/2021 09:54, Russell King (Oracle) wrote: > > Hi, Hi Russell, > > On Wed, Nov 17, 2021 at 10:18:56AM +0100, Horatiu Vultur wrote: > > +static void lan966x_phylink_mac_link_state(struct phylink_config *config, > > + struct phylink_link_state *state) > > +{ > > +} > > + > > +static void lan966x_phylink_mac_aneg_restart(struct phylink_config *config) > > +{ > > +} > > Since you always attach a PCS, it is not necessary to provide stubs > for these functions. Pefect, I will remove these ones. > > > +static int lan966x_pcs_config(struct phylink_pcs *pcs, > > + unsigned int mode, > > + phy_interface_t interface, > > + const unsigned long *advertising, > > + bool permit_pause_to_mac) > > +{ > > + struct lan966x_port *port = lan966x_pcs_to_port(pcs); > > + struct lan966x_port_config config; > > + int ret; > > + > > + memset(&config, 0, sizeof(config)); > > + > > + config = port->config; > > + config.portmode = interface; > > + config.inband = phylink_autoneg_inband(mode); > > + config.autoneg = phylink_test(advertising, Autoneg); > > + if (phylink_test(advertising, Pause)) > > + config.pause_adv |= ADVERTISE_1000XPAUSE; > > + if (phylink_test(advertising, Asym_Pause)) > > + config.pause_adv |= ADVERTISE_1000XPSE_ASYM; > > There are patches around that add > phylink_mii_c22_pcs_encode_advertisement() which will create the C22 > advertisement for you. It would be good to get that patch merged so > people can use it. That should also eliminate lan966x_get_aneg_word(), > although I notice you need to set ADVERTISE_LPACK as well (can you > check that please? Hardware should be managing that bit as it should > only be set once the hardware has received the link partner's > advertisement.) Yes, I will keep an eye for phylink_mii_c22_pcs_encode_advertisement. Also I have also tried to remove ADVERTISE_LPACK and that seems to work fine. > > > +static void decode_cl37_word(u16 lp_abil, uint16_t ld_abil, > > + struct lan966x_port_status *status) > > +{ > > + status->link = !(lp_abil & ADVERTISE_RFAULT) && status->link; > > + status->an_complete = true; > > + status->duplex = (ADVERTISE_1000XFULL & lp_abil) ? > > + DUPLEX_FULL : DUPLEX_UNKNOWN; > > + > > + if ((ld_abil & ADVERTISE_1000XPAUSE) && > > + (lp_abil & ADVERTISE_1000XPAUSE)) { > > + status->pause = MLO_PAUSE_RX | MLO_PAUSE_TX; > > + } else if ((ld_abil & ADVERTISE_1000XPSE_ASYM) && > > + (lp_abil & ADVERTISE_1000XPSE_ASYM)) { > > + status->pause |= (lp_abil & ADVERTISE_1000XPAUSE) ? > > + MLO_PAUSE_TX : 0; > > + status->pause |= (ld_abil & ADVERTISE_1000XPAUSE) ? > > + MLO_PAUSE_RX : 0; > > + } else { > > + status->pause = MLO_PAUSE_NONE; > > + } > > +} > > We already have phylink_decode_c37_word() which will decode this for > you, although it would need to be exported. Please re-use this code. Yes, I will do that. > > > + > > +static void decode_sgmii_word(u16 lp_abil, struct lan966x_port_status *status) > > +{ > > + status->an_complete = true; > > + if (!(lp_abil & LPA_SGMII_LINK)) { > > + status->link = false; > > + return; > > + } > > + > > + switch (lp_abil & LPA_SGMII_SPD_MASK) { > > + case LPA_SGMII_10: > > + status->speed = SPEED_10; > > + break; > > + case LPA_SGMII_100: > > + status->speed = SPEED_100; > > + break; > > + case LPA_SGMII_1000: > > + status->speed = SPEED_1000; > > + break; > > + default: > > + status->link = false; > > + return; > > + } > > + if (lp_abil & LPA_SGMII_FULL_DUPLEX) > > + status->duplex = DUPLEX_FULL; > > + else > > + status->duplex = DUPLEX_HALF; > > +} > > The above mentioned function will also handle SGMII as well. I noticed that you have phylink_decode_sgmii_work(), so I will try to export it also. > > > +int lan966x_port_pcs_set(struct lan966x_port *port, > > + struct lan966x_port_config *config) > > +{ > > + struct lan966x *lan966x = port->lan966x; > > + bool sgmii = false, inband_aneg = false; > > + int err; > > + > > + lan966x_port_link_down(port); > > + > > + if (config->inband) { > > + if (config->portmode == PHY_INTERFACE_MODE_SGMII || > > + config->portmode == PHY_INTERFACE_MODE_QSGMII) > > + inband_aneg = true; /* Cisco-SGMII in-band-aneg */ > > + else if (config->portmode == PHY_INTERFACE_MODE_1000BASEX && > > + config->autoneg) > > + inband_aneg = true; /* Clause-37 in-band-aneg */ > > + > > + if (config->speed > 0) { > > + err = phy_set_speed(port->serdes, config->speed); > > + if (err) > > + return err; > > + } > > + > > + } else { > > + sgmii = true; /* Phy is connnected to the MAC */ > > This looks weird. SGMII can be in-band as well (and technically is > in-band in its as-specified form.) I think the names are a little bit misleading. We cover the case where SGMII is inbind in the code if (config->inband) { if (config->portmode == PHY_INTERFACE_MODE_SGMII || config->portmode == PHY_INTERFACE_MODE_QSGMII) } I think we can remove the sgmii variable and use directly the config->inband. > > > + } > > + > > + /* Choose SGMII or 1000BaseX/2500BaseX PCS mode */ > > + lan_rmw(DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA_SET(sgmii), > > + DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA, > > + lan966x, DEV_PCS1G_MODE_CFG(port->chip_port)); > > With the code as you have it, what this means is if we specify > SGMII + in-band, we end up configuring the port to be in 1000BaseX > mode, so it's incapable of 10 and 100M speeds. This seems incorrect. Actually I think the comment is misleading, that bit will just choose to enable or not the control word for inbound. > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Nov 18, 2021 at 10:57:03AM +0100, Horatiu Vultur wrote: > > > +static void decode_sgmii_word(u16 lp_abil, struct lan966x_port_status *status) > > > +{ > > > + status->an_complete = true; > > > + if (!(lp_abil & LPA_SGMII_LINK)) { > > > + status->link = false; > > > + return; > > > + } > > > + > > > + switch (lp_abil & LPA_SGMII_SPD_MASK) { > > > + case LPA_SGMII_10: > > > + status->speed = SPEED_10; > > > + break; > > > + case LPA_SGMII_100: > > > + status->speed = SPEED_100; > > > + break; > > > + case LPA_SGMII_1000: > > > + status->speed = SPEED_1000; > > > + break; > > > + default: > > > + status->link = false; > > > + return; > > > + } > > > + if (lp_abil & LPA_SGMII_FULL_DUPLEX) > > > + status->duplex = DUPLEX_FULL; > > > + else > > > + status->duplex = DUPLEX_HALF; > > > +} > > > > The above mentioned function will also handle SGMII as well. > > I noticed that you have phylink_decode_sgmii_work(), so I will try to > export it also. Another approach would be to split phylink_mii_c22_pcs_decode_state() so that the appropriate decode function is selected depending on the interface state, which may be a better idea.
Hi Horatiu, On Wed, 2021-11-17 at 22:42 +0100, Horatiu Vultur wrote: > > On Wed, 2021-11-17 at 10:18 +0100, Horatiu Vultur wrote: > > > +static int lan966x_reset_switch(struct lan966x *lan966x) > > > +{ > > > + struct reset_control *reset; > > > + int val = 0; > > > + int ret; > > > + > > > + reset = devm_reset_control_get_shared(lan966x->dev, "switch"); > > > + if (IS_ERR(reset)) > > > + dev_warn(lan966x->dev, "Could not obtain switch reset: %ld\n", > > > + PTR_ERR(reset)); > > > + else > > > + reset_control_reset(reset); > > > > According to the device tree bindings, both resets are required. > > I'd expect this to return on error. > > Is there any chance of the device working with out the switch reset > > being triggered? > > The only case that I see is if the bootloader triggers this switch > reset and then when bootloader starts the kernel and doesn't set back > the switch in reset. Is this a valid scenario or is a bug in the > bootloader? I'm not sure. In general, the kernel shouldn't rely on the bootloader to have put the devices into a certain working state. If the driver will not work or worse, if register access could hang the system if the bootloader has passed control to the kernel with the switch held in reset and no reset control is available to the driver, it should not continue after failure to get the reset handle. I'd suggest to just use: reset = devm_reset_control_get_shared(lan966x->dev, "switch"); if (IS_ERR(reset)) return dev_err_probe(lan966x->dev, PTR_ERR(reset), "Could not obtain switch reset"); reset_control_reset(reset); unless you have a good reason to do otherwise. regards Philipp
The 11/18/2021 11:19, Philipp Zabel wrote: > > Hi Horatiu, > > On Wed, 2021-11-17 at 22:42 +0100, Horatiu Vultur wrote: > > > On Wed, 2021-11-17 at 10:18 +0100, Horatiu Vultur wrote: > > > > +static int lan966x_reset_switch(struct lan966x *lan966x) > > > > +{ > > > > + struct reset_control *reset; > > > > + int val = 0; > > > > + int ret; > > > > + > > > > + reset = devm_reset_control_get_shared(lan966x->dev, "switch"); > > > > + if (IS_ERR(reset)) > > > > + dev_warn(lan966x->dev, "Could not obtain switch reset: %ld\n", > > > > + PTR_ERR(reset)); > > > > + else > > > > + reset_control_reset(reset); > > > > > > According to the device tree bindings, both resets are required. > > > I'd expect this to return on error. > > > Is there any chance of the device working with out the switch reset > > > being triggered? > > > > The only case that I see is if the bootloader triggers this switch > > reset and then when bootloader starts the kernel and doesn't set back > > the switch in reset. Is this a valid scenario or is a bug in the > > bootloader? > > I'm not sure. In general, the kernel shouldn't rely on the bootloader to > have put the devices into a certain working state. If the driver will > not work or worse, if register access could hang the system if the > bootloader has passed control to the kernel with the switch held in > reset and no reset control is available to the driver, it should not > continue after failure to get the reset handle. > > I'd suggest to just use: > > reset = devm_reset_control_get_shared(lan966x->dev, "switch"); > if (IS_ERR(reset)) > return dev_err_probe(lan966x->dev, PTR_ERR(reset), > "Could not obtain switch reset"); > reset_control_reset(reset); > > unless you have a good reason to do otherwise. I agree with you. I will do like you suggested in the next version. > > regards > Philipp
The 11/18/2021 09:59, Russell King (Oracle) wrote: > > On Thu, Nov 18, 2021 at 10:57:03AM +0100, Horatiu Vultur wrote: > > > > +static void decode_sgmii_word(u16 lp_abil, struct lan966x_port_status *status) > > > > +{ > > > > + status->an_complete = true; > > > > + if (!(lp_abil & LPA_SGMII_LINK)) { > > > > + status->link = false; > > > > + return; > > > > + } > > > > + > > > > + switch (lp_abil & LPA_SGMII_SPD_MASK) { > > > > + case LPA_SGMII_10: > > > > + status->speed = SPEED_10; > > > > + break; > > > > + case LPA_SGMII_100: > > > > + status->speed = SPEED_100; > > > > + break; > > > > + case LPA_SGMII_1000: > > > > + status->speed = SPEED_1000; > > > > + break; > > > > + default: > > > > + status->link = false; > > > > + return; > > > > + } > > > > + if (lp_abil & LPA_SGMII_FULL_DUPLEX) > > > > + status->duplex = DUPLEX_FULL; > > > > + else > > > > + status->duplex = DUPLEX_HALF; > > > > +} > > > > > > The above mentioned function will also handle SGMII as well. > > > > I noticed that you have phylink_decode_sgmii_work(), so I will try to > > export it also. > > Another approach would be to split phylink_mii_c22_pcs_decode_state() > so that the appropriate decode function is selected depending on the > interface state, which may be a better idea. I have tried to look for phylink_mii_c22_pcs_decode_state() and I have found it only here [1], and seems that it depends on [2]. But not much activity happened to these series since October. Do you think they will still get in? [1] https://patchwork.kernel.org/project/netdevbpf/patch/20211022160959.3350916-1-sean.anderson@seco.com/ [2] https://lore.kernel.org/netdev/20211022155914.3347672-1-sean.anderson@seco.com/ > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Nov 18, 2021 at 01:59:28PM +0100, Horatiu Vultur wrote: > The 11/18/2021 09:59, Russell King (Oracle) wrote: > > Another approach would be to split phylink_mii_c22_pcs_decode_state() > > so that the appropriate decode function is selected depending on the > > interface state, which may be a better idea. > > I have tried to look for phylink_mii_c22_pcs_decode_state() and I > have found it only here [1], and seems that it depends on [2]. But not > much activity happened to these series since October. > Do you think they will still get in? I don't see any reason the first two patches should not be sent. I'm carrying the second one locally because I use it in some changes I've made to the mv88e6xxx code - as I mentioned in the patchwork entry you linked to. See: http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue "net: phylink: Add helpers for c22 registers without MDIO" Although I notice I committed it to my tree with the wrong author. :( Sean, please can you submit the mdiodev patch and this patch for net-next as they have general utility? Thanks.
Hi Russell, On 11/18/21 8:31 AM, Russell King (Oracle) wrote: > On Thu, Nov 18, 2021 at 01:59:28PM +0100, Horatiu Vultur wrote: >> The 11/18/2021 09:59, Russell King (Oracle) wrote: >> > Another approach would be to split phylink_mii_c22_pcs_decode_state() >> > so that the appropriate decode function is selected depending on the >> > interface state, which may be a better idea. >> >> I have tried to look for phylink_mii_c22_pcs_decode_state() and I >> have found it only here [1], and seems that it depends on [2]. But not >> much activity happened to these series since October. >> Do you think they will still get in? > > I don't see any reason the first two patches should not be sent. I'm > carrying the second one locally because I use it in some changes I've > made to the mv88e6xxx code - as I mentioned in the patchwork entry you > linked to. See: > > http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue > > "net: phylink: Add helpers for c22 registers without MDIO" > > Although I notice I committed it to my tree with the wrong author. :( > > Sean, please can you submit the mdiodev patch and this patch for > net-next as they have general utility? Thanks. The mdiodev patch is already in the tree as 0ebecb2644c8 ("net: mdio: Add helper functions for accessing MDIO devices"). The c22 patch is submitted as [1]. --Sean [1] https://lore.kernel.org/netdev/20211022160959.3350916-1-sean.anderson@seco.com/
On Thu, Nov 18, 2021 at 10:36:58AM -0500, Sean Anderson wrote: > Hi Russell, > > On 11/18/21 8:31 AM, Russell King (Oracle) wrote: > > On Thu, Nov 18, 2021 at 01:59:28PM +0100, Horatiu Vultur wrote: > > > The 11/18/2021 09:59, Russell King (Oracle) wrote: > > > > Another approach would be to split phylink_mii_c22_pcs_decode_state() > > > > so that the appropriate decode function is selected depending on the > > > > interface state, which may be a better idea. > > > > > > I have tried to look for phylink_mii_c22_pcs_decode_state() and I > > > have found it only here [1], and seems that it depends on [2]. But not > > > much activity happened to these series since October. > > > Do you think they will still get in? > > > > I don't see any reason the first two patches should not be sent. I'm > > carrying the second one locally because I use it in some changes I've > > made to the mv88e6xxx code - as I mentioned in the patchwork entry you > > linked to. See: > > > > http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue > > > > "net: phylink: Add helpers for c22 registers without MDIO" > > > > Although I notice I committed it to my tree with the wrong author. :( > > > > Sean, please can you submit the mdiodev patch and this patch for > > net-next as they have general utility? Thanks. > > The mdiodev patch is already in the tree as 0ebecb2644c8 ("net: mdio: > Add helper functions for accessing MDIO devices"). The c22 patch is > submitted as [1]. > > --Sean > > [1] https://lore.kernel.org/netdev/20211022160959.3350916-1-sean.anderson@seco.com/ Patchwork says its deferrred: https://patchwork.kernel.org/project/netdevbpf/patch/20211022160959.3350916-1-sean.anderson@seco.com/ However, it does apply to current net-next, but Jakub did ask for it to be resubmitted. Given that patches are being quickly applied to net-next, I suggest resubmission may be just what's neeeded! Thanks.
On 11/18/21 11:11 AM, Russell King (Oracle) wrote: > On Thu, Nov 18, 2021 at 10:36:58AM -0500, Sean Anderson wrote: >> Hi Russell, >> >> On 11/18/21 8:31 AM, Russell King (Oracle) wrote: >> > On Thu, Nov 18, 2021 at 01:59:28PM +0100, Horatiu Vultur wrote: >> > > The 11/18/2021 09:59, Russell King (Oracle) wrote: >> > > > Another approach would be to split phylink_mii_c22_pcs_decode_state() >> > > > so that the appropriate decode function is selected depending on the >> > > > interface state, which may be a better idea. >> > > >> > > I have tried to look for phylink_mii_c22_pcs_decode_state() and I >> > > have found it only here [1], and seems that it depends on [2]. But not >> > > much activity happened to these series since October. >> > > Do you think they will still get in? >> > >> > I don't see any reason the first two patches should not be sent. I'm >> > carrying the second one locally because I use it in some changes I've >> > made to the mv88e6xxx code - as I mentioned in the patchwork entry you >> > linked to. See: >> > >> > http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue >> > >> > "net: phylink: Add helpers for c22 registers without MDIO" >> > >> > Although I notice I committed it to my tree with the wrong author. :( >> > >> > Sean, please can you submit the mdiodev patch and this patch for >> > net-next as they have general utility? Thanks. >> >> The mdiodev patch is already in the tree as 0ebecb2644c8 ("net: mdio: >> Add helper functions for accessing MDIO devices"). The c22 patch is >> submitted as [1]. >> >> --Sean >> >> [1] https://lore.kernel.org/netdev/20211022160959.3350916-1-sean.anderson@seco.com/ > > Patchwork says its deferrred: > > https://patchwork.kernel.org/project/netdevbpf/patch/20211022160959.3350916-1-sean.anderson@seco.com/ > > However, it does apply to current net-next, but Jakub did ask for > it to be resubmitted. Well, he suggested that I would have to resubmit it. But I ordered the patches such that they would apply cleanly in what I thought was the most likely scenario (which indeed come to pass). So I didn't think it was necessary to resend. > Given that patches are being quickly applied to net-next, I suggest > resubmission may be just what's neeeded! Resent. --Sean
Hi Sean, The 11/18/2021 11:18, Sean Anderson wrote: > > On 11/18/21 11:11 AM, Russell King (Oracle) wrote: > > On Thu, Nov 18, 2021 at 10:36:58AM -0500, Sean Anderson wrote: > > > Hi Russell, > > > > > > On 11/18/21 8:31 AM, Russell King (Oracle) wrote: > > > > On Thu, Nov 18, 2021 at 01:59:28PM +0100, Horatiu Vultur wrote: > > > > > The 11/18/2021 09:59, Russell King (Oracle) wrote: > > > > > > Another approach would be to split phylink_mii_c22_pcs_decode_state() > > > > > > so that the appropriate decode function is selected depending on the > > > > > > interface state, which may be a better idea. > > > > > > > > > > I have tried to look for phylink_mii_c22_pcs_decode_state() and I > > > > > have found it only here [1], and seems that it depends on [2]. But not > > > > > much activity happened to these series since October. > > > > > Do you think they will still get in? > > > > > > > > I don't see any reason the first two patches should not be sent. I'm > > > > carrying the second one locally because I use it in some changes I've > > > > made to the mv88e6xxx code - as I mentioned in the patchwork entry you > > > > linked to. See: > > > > > > > > http://git.armlinux.org.uk/cgit/linux-arm.git/log/?h=net-queue > > > > > > > > "net: phylink: Add helpers for c22 registers without MDIO" > > > > > > > > Although I notice I committed it to my tree with the wrong author. :( > > > > > > > > Sean, please can you submit the mdiodev patch and this patch for > > > > net-next as they have general utility? Thanks. > > > > > > The mdiodev patch is already in the tree as 0ebecb2644c8 ("net: mdio: > > > Add helper functions for accessing MDIO devices"). The c22 patch is > > > submitted as [1]. > > > > > > --Sean > > > > > > [1] https://lore.kernel.org/netdev/20211022160959.3350916-1-sean.anderson@seco.com/ > > > > Patchwork says its deferrred: > > > > https://patchwork.kernel.org/project/netdevbpf/patch/20211022160959.3350916-1-sean.anderson@seco.com/ > > > > However, it does apply to current net-next, but Jakub did ask for > > it to be resubmitted. > > Well, he suggested that I would have to resubmit it. But I ordered the > patches such that they would apply cleanly in what I thought was the > most likely scenario (which indeed come to pass). So I didn't think it > was necessary to resend. > > > Given that patches are being quickly applied to net-next, I suggest > > resubmission may be just what's neeeded! > > Resent. Thanks for resubmission the patch! Unfortunately, your patch introduced a new warning, so it got in the state "Changes Required". So I think you will need to send a new version. [1] https://patchwork.kernel.org/project/netdevbpf/patch/20211118161430.2547168-1-sean.anderson@seco.com/ > > --Sean