Date:   Mon, 6 Mar 2023 10:09:53 +0200
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Sean Anderson <sean.anderson@...o.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org,
        Paolo Abeni <pabeni@...hat.com>
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@...o.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);
>  
> -- 

Powered by blists - more mailing lists