| Message ID | 20220309172748.3460862-1-ioana.ciornei@nxp.com |
|---|---|
| Headers | show |
| Series | dpaa2-mac: add support for changing the protocol at runtime | expand |
Hi Ioana, On Wed, Mar 09, 2022 at 07:27:47PM +0200, Ioana Ciornei wrote: > This patch integrates the dpaa2-eth driver with the generic PHY > infrastructure in order to search, find and reconfigure the SerDes lanes > in case of a protocol change. > > On the .mac_config() callback, the phy_set_mode_ext() API is called so > that the Lynx 28G SerDes PHY driver can change the lane's configuration. > In the same phylink callback the MC firmware is called so that it > reconfigures the MAC side to run using the new protocol. > > The consumer drivers - dpaa2-eth and dpaa2-switch - are updated to call > the dpaa2_mac_start/stop functions newly added which will > power_on/power_off the associated SerDes lane. > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > --- > Changes in v2: > - none > > .../net/ethernet/freescale/dpaa2/dpaa2-eth.c | 5 +- > .../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 91 +++++++++++++++++++ > .../net/ethernet/freescale/dpaa2/dpaa2-mac.h | 6 ++ > .../ethernet/freescale/dpaa2/dpaa2-switch.c | 5 +- > 4 files changed, 105 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > index 939fa9db6a2e..b87369f0605f 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > @@ -2077,8 +2077,10 @@ static int dpaa2_eth_open(struct net_device *net_dev) > goto enable_err; > } > > - if (dpaa2_eth_is_type_phy(priv)) > + if (dpaa2_eth_is_type_phy(priv)) { > phylink_start(priv->mac->phylink); > + dpaa2_mac_start(priv->mac); Is this safe? Shouldn't dpaa2_mac_start() come before phylink_start() in case phylink determines that the link is somehow already up? I'm a big fan of teardown being in the reverse order of setup so having the start and stop below in the same order just doesn't look right. > + } > > return 0; > > @@ -2153,6 +2155,7 @@ static int dpaa2_eth_stop(struct net_device *net_dev) > > if (dpaa2_eth_is_type_phy(priv)) { > phylink_stop(priv->mac->phylink); > + dpaa2_mac_stop(priv->mac); > } else { > netif_tx_stop_all_queues(net_dev); > netif_carrier_off(net_dev); > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > index e6e758eaafea..bd90acc49cdb 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > @@ -3,6 +3,7 @@ > > #include <linux/acpi.h> > #include <linux/pcs-lynx.h> > +#include <linux/phy/phy.h> > #include <linux/property.h> > > #include "dpaa2-eth.h" > @@ -60,6 +61,26 @@ static int phy_mode(enum dpmac_eth_if eth_if, phy_interface_t *if_mode) > return 0; > } > > +static enum dpmac_eth_if dpmac_eth_if_mode(phy_interface_t if_mode) > +{ > + switch (if_mode) { > + case PHY_INTERFACE_MODE_RGMII: Shouldn't this also include the other RGMII modes (which, from the MAC point of view, are all synonymous? > + return DPMAC_ETH_IF_RGMII; > + case PHY_INTERFACE_MODE_USXGMII: > + return DPMAC_ETH_IF_USXGMII; > + case PHY_INTERFACE_MODE_QSGMII: > + return DPMAC_ETH_IF_QSGMII; > + case PHY_INTERFACE_MODE_SGMII: > + return DPMAC_ETH_IF_SGMII; > + case PHY_INTERFACE_MODE_10GBASER: > + return DPMAC_ETH_IF_XFI; > + case PHY_INTERFACE_MODE_1000BASEX: > + return DPMAC_ETH_IF_1000BASEX; > + default: > + return DPMAC_ETH_IF_MII; > + } > +} > + > static struct fwnode_handle *dpaa2_mac_get_node(struct device *dev, > u16 dpmac_id) > { > @@ -147,6 +168,19 @@ static void dpaa2_mac_config(struct phylink_config *config, unsigned int mode, > if (err) > netdev_err(mac->net_dev, "%s: dpmac_set_link_state() = %d\n", > __func__, err); > + > + if (!mac->serdes_phy) > + return; > + > + /* This happens only if we support changing of protocol at runtime */ > + err = dpmac_set_protocol(mac->mc_io, 0, mac->mc_dev->mc_handle, > + dpmac_eth_if_mode(state->interface)); > + if (err) > + netdev_err(mac->net_dev, "dpmac_set_protocol() = %d\n", err); > + > + err = phy_set_mode_ext(mac->serdes_phy, PHY_MODE_ETHERNET, state->interface); > + if (err) > + netdev_err(mac->net_dev, "phy_set_mode_ext() = %d\n", err); > } > > static void dpaa2_mac_link_up(struct phylink_config *config, > @@ -200,12 +234,21 @@ static void dpaa2_mac_link_down(struct phylink_config *config, > netdev_err(mac->net_dev, "dpmac_set_link_state() = %d\n", err); > } > > +static int dpaa2_mac_prepare(struct phylink_config *config, unsigned int mode, > + phy_interface_t interface) > +{ > + dpaa2_mac_link_down(config, mode, interface); You should never see a reconfiguration while the link is up. However, if the link is in in-band mode, then obviously the link could come up at any moment, and in that case, forcing it down in mac_prepare() is a good idea - but that forcing needs to be removed in mac_finish() to allow in-band to work again. Not sure that your firmware allows that though, and I'm not convinced that calling the above function achieves any of those guarantees. > + > + return 0; > +} > + > static const struct phylink_mac_ops dpaa2_mac_phylink_ops = { > .validate = phylink_generic_validate, > .mac_select_pcs = dpaa2_mac_select_pcs, > .mac_config = dpaa2_mac_config, > .mac_link_up = dpaa2_mac_link_up, > .mac_link_down = dpaa2_mac_link_down, > + .mac_prepare = dpaa2_mac_prepare, > }; > > static int dpaa2_pcs_create(struct dpaa2_mac *mac, > @@ -259,6 +302,8 @@ static void dpaa2_pcs_destroy(struct dpaa2_mac *mac) > > static void dpaa2_mac_set_supported_interfaces(struct dpaa2_mac *mac) > { > + int intf, err; > + > /* We support the current interface mode, and if we have a PCS > * similar interface modes that do not require the SerDes lane to be > * reconfigured. > @@ -278,12 +323,40 @@ static void dpaa2_mac_set_supported_interfaces(struct dpaa2_mac *mac) > break; > } > } > + > + if (!mac->serdes_phy) > + return; > + > + /* In case we have access to the SerDes phy/lane, then ask the SerDes > + * driver what interfaces are supported based on the current PLL > + * configuration. > + */ > + for (intf = 0; intf < PHY_INTERFACE_MODE_MAX; intf++) { You probably want to avoid PHY_INTERFACE_MODE_NA here, even though your driver may reject it anyway. > + err = phy_validate(mac->serdes_phy, PHY_MODE_ETHERNET, intf, NULL); > + if (err) > + continue; > + > + __set_bit(intf, mac->phylink_config.supported_interfaces); > + } > +} > + > +void dpaa2_mac_start(struct dpaa2_mac *mac) > +{ > + if (mac->serdes_phy) > + phy_power_on(mac->serdes_phy); > +} > + > +void dpaa2_mac_stop(struct dpaa2_mac *mac) > +{ > + if (mac->serdes_phy) > + phy_power_off(mac->serdes_phy); > } > > int dpaa2_mac_connect(struct dpaa2_mac *mac) > { > struct net_device *net_dev = mac->net_dev; > struct fwnode_handle *dpmac_node; > + struct phy *serdes_phy = NULL; > struct phylink *phylink; > int err; > > @@ -300,6 +373,22 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) > return -EINVAL; > mac->if_mode = err; > > + if (mac->features & DPAA2_MAC_FEATURE_PROTOCOL_CHANGE && > + !phy_interface_mode_is_rgmii(mac->if_mode) && > + is_of_node(dpmac_node)) { > + serdes_phy = of_phy_get(to_of_node(dpmac_node), NULL); > + > + if (IS_ERR(serdes_phy)) { > + if (PTR_ERR(serdes_phy) == -ENODEV) > + serdes_phy = NULL; > + else > + return PTR_ERR(serdes_phy); > + } else { > + phy_init(serdes_phy); > + } > + } > + mac->serdes_phy = serdes_phy; > + > /* The MAC does not have the capability to add RGMII delays so > * error out if the interface mode requests them and there is no PHY > * to act upon them > @@ -363,6 +452,8 @@ void dpaa2_mac_disconnect(struct dpaa2_mac *mac) > phylink_disconnect_phy(mac->phylink); > phylink_destroy(mac->phylink); > dpaa2_pcs_destroy(mac); > + of_phy_put(mac->serdes_phy); > + mac->serdes_phy = NULL; > } > > int dpaa2_mac_open(struct dpaa2_mac *mac) > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h > index d2e51d21c80c..a58cab188a99 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h > @@ -26,6 +26,8 @@ struct dpaa2_mac { > enum dpmac_link_type if_link_type; > struct phylink_pcs *pcs; > struct fwnode_handle *fw_node; > + > + struct phy *serdes_phy; > }; > > bool dpaa2_mac_is_type_fixed(struct fsl_mc_device *dpmac_dev, > @@ -45,4 +47,8 @@ void dpaa2_mac_get_strings(u8 *data); > > void dpaa2_mac_get_ethtool_stats(struct dpaa2_mac *mac, u64 *data); > > +void dpaa2_mac_start(struct dpaa2_mac *mac); > + > +void dpaa2_mac_stop(struct dpaa2_mac *mac); > + > #endif /* DPAA2_MAC_H */ > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > index 9a561072aa4a..e4f8f927e223 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c > @@ -703,8 +703,10 @@ static int dpaa2_switch_port_open(struct net_device *netdev) > > dpaa2_switch_enable_ctrl_if_napi(ethsw); > > - if (dpaa2_switch_port_is_type_phy(port_priv)) > + if (dpaa2_switch_port_is_type_phy(port_priv)) { > phylink_start(port_priv->mac->phylink); > + dpaa2_mac_start(port_priv->mac); Same comments as for dpaa2-mac. > + } > > return 0; > } > @@ -717,6 +719,7 @@ static int dpaa2_switch_port_stop(struct net_device *netdev) > > if (dpaa2_switch_port_is_type_phy(port_priv)) { > phylink_stop(port_priv->mac->phylink); > + dpaa2_mac_stop(port_priv->mac); > } else { > netif_tx_stop_all_queues(netdev); > netif_carrier_off(netdev); > -- > 2.33.1 > > Thanks!
On Wed, Mar 09, 2022 at 06:46:26PM +0000, Russell King (Oracle) wrote: > Hi Ioana, > Hi Russell, > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > @@ -2077,8 +2077,10 @@ static int dpaa2_eth_open(struct net_device *net_dev) > > goto enable_err; > > } > > > > - if (dpaa2_eth_is_type_phy(priv)) > > + if (dpaa2_eth_is_type_phy(priv)) { > > phylink_start(priv->mac->phylink); > > + dpaa2_mac_start(priv->mac); > > Is this safe? Shouldn't dpaa2_mac_start() come before phylink_start() > in case phylink determines that the link is somehow already up? I'm > a big fan of teardown being in the reverse order of setup so having > the start and stop below in the same order just doesn't look right. Agree that the teardown being done in the reverse order just looks better. I can change it, of course. I didn't really spot any actual problems with how are things now, but it would be better to just bring up the SerDes lanes and then call phylink_start(). > > +static enum dpmac_eth_if dpmac_eth_if_mode(phy_interface_t if_mode) > > +{ > > + switch (if_mode) { > > + case PHY_INTERFACE_MODE_RGMII: > > Shouldn't this also include the other RGMII modes (which, from the MAC > point of view, are all synonymous? > Good point. Thanks for pointing it out. > > +static int dpaa2_mac_prepare(struct phylink_config *config, unsigned int mode, > > + phy_interface_t interface) > > +{ > > + dpaa2_mac_link_down(config, mode, interface); > > You should never see a reconfiguration while the link is up. However, > if the link is in in-band mode, then obviously the link could come up > at any moment, and in that case, forcing it down in mac_prepare() is > a good idea - but that forcing needs to be removed in mac_finish() > to allow in-band to work again. Not sure that your firmware allows > that though, and I'm not convinced that calling the above function > achieves any of those guarantees. > Ok, I didn't know that I this was a guarantee from phylink's part. In this case, I can just remove the prepare step, sure. > > + /* In case we have access to the SerDes phy/lane, then ask the SerDes > > + * driver what interfaces are supported based on the current PLL > > + * configuration. > > + */ > > + for (intf = 0; intf < PHY_INTERFACE_MODE_MAX; intf++) { > > You probably want to avoid PHY_INTERFACE_MODE_NA here, even though your > driver may reject it anyway. Yes, I'll just start from PHY_INTERFACE_MODE_INTERNAL. > > - if (dpaa2_switch_port_is_type_phy(port_priv)) > > + if (dpaa2_switch_port_is_type_phy(port_priv)) { > > phylink_start(port_priv->mac->phylink); > > + dpaa2_mac_start(port_priv->mac); > > Same comments as for dpaa2-mac. Sure. I'll change the order. > > Thanks! > Thanks! Ioana