| Message ID | E1j6WgG-0000TJ-CC@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rework phylink interface for split MAC/PCS support | expand |
> Subject: [PATCH net-next 5/8] net: dpaa2-mac: use resolved link config in > mac_link_up() > > Convert the DPAA2 ethernet driver to use the finalised link parameters in > mac_link_up() rather than the parameters in mac_config(), which are more > suited to the needs of the DPAA2 MC firmware than those available via > mac_config(). > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> Tested-by: Ioana Ciornei <ioana.ciornei@nxp.com> > --- > .../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 54 +++++++++++-------- > .../net/ethernet/freescale/dpaa2/dpaa2-mac.h | 1 + > 2 files changed, 33 insertions(+), 22 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > index 3a75c5b58f95..3ee236c5fc37 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > @@ -123,35 +123,16 @@ static void dpaa2_mac_config(struct phylink_config > *config, unsigned int mode, > struct dpmac_link_state *dpmac_state = &mac->state; > int err; > > - if (state->speed != SPEED_UNKNOWN) > - dpmac_state->rate = state->speed; > - > - if (state->duplex != DUPLEX_UNKNOWN) { > - if (!state->duplex) > - dpmac_state->options |= > DPMAC_LINK_OPT_HALF_DUPLEX; > - else > - dpmac_state->options &= > ~DPMAC_LINK_OPT_HALF_DUPLEX; > - } > - > if (state->an_enabled) > dpmac_state->options |= DPMAC_LINK_OPT_AUTONEG; > else > dpmac_state->options &= ~DPMAC_LINK_OPT_AUTONEG; > > - if (state->pause & MLO_PAUSE_RX) > - dpmac_state->options |= DPMAC_LINK_OPT_PAUSE; > - else > - dpmac_state->options &= ~DPMAC_LINK_OPT_PAUSE; > - > - if (!!(state->pause & MLO_PAUSE_RX) ^ !!(state->pause & > MLO_PAUSE_TX)) > - dpmac_state->options |= DPMAC_LINK_OPT_ASYM_PAUSE; > - else > - dpmac_state->options &= ~DPMAC_LINK_OPT_ASYM_PAUSE; > - > err = dpmac_set_link_state(mac->mc_io, 0, > mac->mc_dev->mc_handle, dpmac_state); > if (err) > - netdev_err(mac->net_dev, "dpmac_set_link_state() = %d\n", > err); > + netdev_err(mac->net_dev, "%s: dpmac_set_link_state() = > %d\n", > + __func__, err); > } > > static void dpaa2_mac_link_up(struct phylink_config *config, @@ -165,10 > +146,37 @@ static void dpaa2_mac_link_up(struct phylink_config *config, > int err; > > dpmac_state->up = 1; > + > + if (mac->if_link_type == DPMAC_LINK_TYPE_PHY) { > + /* If the DPMAC is configured for PHY mode, we need > + * to pass the link parameters to the MC firmware. > + */ > + dpmac_state->rate = speed; > + > + if (duplex == DUPLEX_HALF) > + dpmac_state->options |= > DPMAC_LINK_OPT_HALF_DUPLEX; > + else if (duplex == DUPLEX_FULL) > + dpmac_state->options &= > ~DPMAC_LINK_OPT_HALF_DUPLEX; > + > + /* This is lossy; the firmware really should take the pause > + * enablement status rather than pause/asym pause status. > + */ In what sense it's lossy? I cannot see how information can be lost by translating the rx/tx pause state to pause/asym. If it's just about the unnecessary double translation, then I agree.. this could have been done in an easier manner. > + if (rx_pause) > + dpmac_state->options |= DPMAC_LINK_OPT_PAUSE; > + else > + dpmac_state->options &= ~DPMAC_LINK_OPT_PAUSE; > + > + if (rx_pause ^ tx_pause) > + dpmac_state->options |= > DPMAC_LINK_OPT_ASYM_PAUSE; > + else > + dpmac_state->options &= > ~DPMAC_LINK_OPT_ASYM_PAUSE; > + } > + > err = dpmac_set_link_state(mac->mc_io, 0, > mac->mc_dev->mc_handle, dpmac_state); > if (err) > - netdev_err(mac->net_dev, "dpmac_set_link_state() = %d\n", > err); > + netdev_err(mac->net_dev, "%s: dpmac_set_link_state() = > %d\n", > + __func__, err); > } > > static void dpaa2_mac_link_down(struct phylink_config *config, @@ -241,6 > +249,8 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) > goto err_close_dpmac; > } > > + mac->if_link_type = attr.link_type; > + > dpmac_node = dpaa2_mac_get_node(attr.id); > if (!dpmac_node) { > netdev_err(net_dev, "No dpmac@%d node found.\n", attr.id); > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h > b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h > index 4da8079b9155..2130d9c7d40e 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h > @@ -20,6 +20,7 @@ struct dpaa2_mac { > struct phylink_config phylink_config; > struct phylink *phylink; > phy_interface_t if_mode; > + enum dpmac_link_type if_link_type; > }; > > bool dpaa2_mac_is_type_fixed(struct fsl_mc_device *dpmac_dev, > -- > 2.20.1
On Tue, Feb 25, 2020 at 04:36:32PM +0000, Ioana Ciornei wrote: > > Subject: [PATCH net-next 5/8] net: dpaa2-mac: use resolved link config in > > mac_link_up() > > > > Convert the DPAA2 ethernet driver to use the finalised link parameters in > > mac_link_up() rather than the parameters in mac_config(), which are more > > suited to the needs of the DPAA2 MC firmware than those available via > > mac_config(). > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > Tested-by: Ioana Ciornei <ioana.ciornei@nxp.com> Thanks. > > + > > + /* This is lossy; the firmware really should take the pause > > + * enablement status rather than pause/asym pause status. > > + */ > > In what sense it's lossy? I cannot see how information can be lost by translating the rx/tx pause state to pause/asym. > If it's just about the unnecessary double translation, then I agree.. this could have been done in an easier manner. If you're just translating rx/tx to pause/asym and then doing the reverse, then it isn't lossy, but if the firmware is resolving pause/asym according to the table in IEEE 802.3, then it will be lossy. If the firmware doesn't interpret the bits, then why not do the sensible thing and just pass the enablement status rather than trying to confusingly encode it back to pause/asym?
> Subject: Re: [PATCH net-next 5/8] net: dpaa2-mac: use resolved link config in > mac_link_up() > > On Tue, Feb 25, 2020 at 04:36:32PM +0000, Ioana Ciornei wrote: > > > Subject: [PATCH net-next 5/8] net: dpaa2-mac: use resolved link > > > config in > > > mac_link_up() > > > > > > Convert the DPAA2 ethernet driver to use the finalised link > > > parameters in > > > mac_link_up() rather than the parameters in mac_config(), which are > > > more suited to the needs of the DPAA2 MC firmware than those > > > available via mac_config(). > > > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > > > Tested-by: Ioana Ciornei <ioana.ciornei@nxp.com> > > Thanks. > > > > + > > > + /* This is lossy; the firmware really should take the pause > > > + * enablement status rather than pause/asym pause status. > > > + */ > > > > In what sense it's lossy? I cannot see how information can be lost by > translating the rx/tx pause state to pause/asym. > > If it's just about the unnecessary double translation, then I agree.. this could > have been done in an easier manner. > > If you're just translating rx/tx to pause/asym and then doing the reverse, then it > isn't lossy, but if the firmware is resolving pause/asym according to the table in > IEEE 802.3, then it will be lossy. The firmware is just doing the reverse translation. > > If the firmware doesn't interpret the bits, then why not do the sensible thing and > just pass the enablement status rather than trying to confusingly encode it back > to pause/asym? I agree. It's unnecessary and confusing. I'll add this on the list of fixups to be made by the firmware team. Ioana > > --
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c index 3a75c5b58f95..3ee236c5fc37 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c @@ -123,35 +123,16 @@ static void dpaa2_mac_config(struct phylink_config *config, unsigned int mode, struct dpmac_link_state *dpmac_state = &mac->state; int err; - if (state->speed != SPEED_UNKNOWN) - dpmac_state->rate = state->speed; - - if (state->duplex != DUPLEX_UNKNOWN) { - if (!state->duplex) - dpmac_state->options |= DPMAC_LINK_OPT_HALF_DUPLEX; - else - dpmac_state->options &= ~DPMAC_LINK_OPT_HALF_DUPLEX; - } - if (state->an_enabled) dpmac_state->options |= DPMAC_LINK_OPT_AUTONEG; else dpmac_state->options &= ~DPMAC_LINK_OPT_AUTONEG; - if (state->pause & MLO_PAUSE_RX) - dpmac_state->options |= DPMAC_LINK_OPT_PAUSE; - else - dpmac_state->options &= ~DPMAC_LINK_OPT_PAUSE; - - if (!!(state->pause & MLO_PAUSE_RX) ^ !!(state->pause & MLO_PAUSE_TX)) - dpmac_state->options |= DPMAC_LINK_OPT_ASYM_PAUSE; - else - dpmac_state->options &= ~DPMAC_LINK_OPT_ASYM_PAUSE; - err = dpmac_set_link_state(mac->mc_io, 0, mac->mc_dev->mc_handle, dpmac_state); if (err) - netdev_err(mac->net_dev, "dpmac_set_link_state() = %d\n", err); + netdev_err(mac->net_dev, "%s: dpmac_set_link_state() = %d\n", + __func__, err); } static void dpaa2_mac_link_up(struct phylink_config *config, @@ -165,10 +146,37 @@ static void dpaa2_mac_link_up(struct phylink_config *config, int err; dpmac_state->up = 1; + + if (mac->if_link_type == DPMAC_LINK_TYPE_PHY) { + /* If the DPMAC is configured for PHY mode, we need + * to pass the link parameters to the MC firmware. + */ + dpmac_state->rate = speed; + + if (duplex == DUPLEX_HALF) + dpmac_state->options |= DPMAC_LINK_OPT_HALF_DUPLEX; + else if (duplex == DUPLEX_FULL) + dpmac_state->options &= ~DPMAC_LINK_OPT_HALF_DUPLEX; + + /* This is lossy; the firmware really should take the pause + * enablement status rather than pause/asym pause status. + */ + if (rx_pause) + dpmac_state->options |= DPMAC_LINK_OPT_PAUSE; + else + dpmac_state->options &= ~DPMAC_LINK_OPT_PAUSE; + + if (rx_pause ^ tx_pause) + dpmac_state->options |= DPMAC_LINK_OPT_ASYM_PAUSE; + else + dpmac_state->options &= ~DPMAC_LINK_OPT_ASYM_PAUSE; + } + err = dpmac_set_link_state(mac->mc_io, 0, mac->mc_dev->mc_handle, dpmac_state); if (err) - netdev_err(mac->net_dev, "dpmac_set_link_state() = %d\n", err); + netdev_err(mac->net_dev, "%s: dpmac_set_link_state() = %d\n", + __func__, err); } static void dpaa2_mac_link_down(struct phylink_config *config, @@ -241,6 +249,8 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) goto err_close_dpmac; } + mac->if_link_type = attr.link_type; + dpmac_node = dpaa2_mac_get_node(attr.id); if (!dpmac_node) { netdev_err(net_dev, "No dpmac@%d node found.\n", attr.id); diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h index 4da8079b9155..2130d9c7d40e 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.h @@ -20,6 +20,7 @@ struct dpaa2_mac { struct phylink_config phylink_config; struct phylink *phylink; phy_interface_t if_mode; + enum dpmac_link_type if_link_type; }; bool dpaa2_mac_is_type_fixed(struct fsl_mc_device *dpmac_dev,
Convert the DPAA2 ethernet driver to use the finalised link parameters in mac_link_up() rather than the parameters in mac_config(), which are more suited to the needs of the DPAA2 MC firmware than those available via mac_config(). Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- .../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 54 +++++++++++-------- .../net/ethernet/freescale/dpaa2/dpaa2-mac.h | 1 + 2 files changed, 33 insertions(+), 22 deletions(-)