Messages in this thread Patch in this message | | | Date | Mon, 6 Mar 2023 10:09:53 +0200 | From | Ioana Ciornei <> | Subject | Re: [PATCH net v2] net: dpaa2-mac: Get serdes only for backplane links |
| |
On Fri, Mar 03, 2023 at 07:31:59PM -0500, Sean Anderson wrote: > When commenting on what would become commit 085f1776fa03 ("net: dpaa2-mac: > add backplane link mode support"), Ioana Ciornei said [1]: > > > ...DPMACs in TYPE_BACKPLANE can have both their PCS and SerDes managed > > by Linux (since the firmware is not touching these). That being said, > > DPMACs in TYPE_PHY (the type that is already supported in dpaa2-mac) can > > also have their PCS managed by Linux (no interraction from the > > firmware's part with the PCS, just the SerDes). > > This implies that Linux only manages the SerDes when the link type is > backplane. Modify the condition in dpaa2_mac_connect to reflect this, > moving the existing conditions to more appropriate places.
I am not sure I understand why are you moving the conditions to different places. Could you please explain?
Why not just append the existing condition from dpaa2_mac_connect() with "mac->attr.link_type == DPMAC_LINK_TYPE_BACKPLANE"?
This way, the serdes_phy is populated only if all the conditions pass and you don't have to scatter them all around the driver.
> > [1] https://lore.kernel.org/netdev/20210120221900.i6esmk6uadgqpdtu@skbuf/ > > Fixes: f978fe85b8d1 ("dpaa2-mac: configure the SerDes phy on a protocol change") > Signed-off-by: Sean Anderson <sean.anderson@seco.com> > --- > For v2 I tested a variety of setups to try and determine what the > behavior is. I evaluated the following branches on a variety of commits > on an LS1088ARDB: > > - net/master > - this commit alone > - my lynx10g series [1] alone > - both of the above together > > I also switched between MC firmware 10.30 (no protocol change support) > and 10.34 (with protocol change support), and I tried MAC link types of > of FIXED, PHY, and BACKPLANE. After loading the MC firmware, DPC, > kernel, and dtb, I booted up and ran > > $ ls-addni dpmac.1 > > I had a 10G fiber SFP module plugged in and connected on the other end > to my computer. > > My results are as follows: > > - When the link type is FIXED, all configurations work. > - PHY and BACKPLANE do not work on net/master. > - I occasionally saw an ENOTSUPP error from dpmac_set_protocol with MC > version 10.30. I am not sure what the cause of this is, as I was > unable to reproduce it reliably. > - Occasionally, the link did not come up with my lynx10g series without > this commit. Like the above issue, this would persist across reboots, > but switching to another configuration and back would often fix this > issue. > > Unfortunately, I was unable to pinpoint any "smoking gun" due to > difficulty in reproducing errors. However, I still think this commit is > correct, and should be applied. If Linux and the MC are out of sync, > most of the time things will work correctly but occasionally they won't. > > [1] https://lore.kernel.org/linux-arm-kernel/20221230000139.2846763-1-sean.anderson@seco.com/ > But with some additional changes for v10. > > Changes in v2: > - Fix incorrect condition in dpaa2_mac_set_supported_interfaces > > drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > index c886f33f8c6f..9b40c862d807 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > @@ -179,9 +179,13 @@ static void dpaa2_mac_config(struct phylink_config *config, unsigned int mode, > 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); > + if (!phy_interface_mode_is_rgmii(mode)) { > + 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, > @@ -317,7 +321,8 @@ static void dpaa2_mac_set_supported_interfaces(struct dpaa2_mac *mac) > } > } > > - if (!mac->serdes_phy) > + if (!(mac->features & DPAA2_MAC_FEATURE_PROTOCOL_CHANGE) || > + !mac->serdes_phy) > return;
For example, you removed the check against DPAA2_MAC_FEATURE_PROTOCOL_CHANGE from below in dpaa2_mac_connect() just to put it here.
> > /* In case we have access to the SerDes phy/lane, then ask the SerDes > @@ -377,8 +382,7 @@ 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) && > + if (mac->attr.link_type == DPMAC_LINK_TYPE_BACKPLANE && > is_of_node(dpmac_node)) { > serdes_phy = of_phy_get(to_of_node(dpmac_node), NULL); > > --
| |