Date:   Mon, 6 Mar 2023 10:09:53 +0200
From:   Ioana Ciornei <ioana.ciornei@nxp.com>
To:     Sean Anderson <sean.anderson@seco.com>
Cc:     "David S . Miller" <davem@davemloft.net>,
        Russell King <linux@armlinux.org.uk>, netdev@vger.kernel.org,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>, linux-kernel@vger.kernel.org,
        Paolo Abeni <pabeni@redhat.com>
Subject: Re: [PATCH net v2] net: dpaa2-mac: Get serdes only for backplane
 links
Message-ID: <20230306080953.3wbprojol4gs5bel@LXL00007.wbi.nxp.com>
References: <20230304003159.1389573-1-sean.anderson@seco.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230304003159.1389573-1-sean.anderson@seco.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?mvSB1kRqfTFhXpEKBrSrQIM0WlZi91mZtJNKfhiQKAvBCeXSL2bikF9L5dU2?=
 =?us-ascii?Q?5YDKzczdr9a8HHlOoLicXsmsOhYlZMLF0+omqrUIFmzdlUvVcnNQguKgPcJn?=
 =?us-ascii?Q?k0vRK2A1JPiR1cXaULtHOfpyzx3YFULzUbIH6vEbRF/MywJMDOSB7cFTo59d?=
 =?us-ascii?Q?5GGfY5Zs8FbJtuEkhvh1/ZoZoWD8DcnF59YGmdqyq0gKFGiIE/UgV35pl/PG?=
 =?us-ascii?Q?ciyB7aQ0vXgkgjU3eYzQcH1myp4Hqa8LpqOHz/TiuiQig++Ra61arhMO4sc7?=
 =?us-ascii?Q?+Tp+wFmFMZ21qpSVDQ/juFRyghpuMofxKfaOt4wZb98tNmPk7jSVCmAyIH0J?=
 =?us-ascii?Q?KYvsndo9CQnoBYF5noOLbNA66oMmX6p72bFBq8j3F2XEK0HjIWbqStGaXmzT?=
 =?us-ascii?Q?slGFOY8IJn4yVj8xrmLom8g0Q1V3fdJ9V61K/+MdmzK6U52upA0zYB923tdb?=
 =?us-ascii?Q?U+J5Z04u/EXkrIsOWNr8MrNxlPL74kD7MRCuw1O8kXglDroqRT1h1OFAPe9v?=
 =?us-ascii?Q?GrWfBJBpIZ91JoW6odtrU/TvR8sPTDD52ra9gynT82l1cQD8OfRj0CiSi7oT?=
 =?us-ascii?Q?3RO5V6IK/tQWIeef3og4B9gya/VpV13Cn8Kkrm74LorX4y9ErrKz5O5l3LkI?=
 =?us-ascii?Q?67vaW9cVUJTuUBl2dVqTaKLfVuhunVaEu4whUAFahgwc6P9NRFN7WzFMoAXd?=
 =?us-ascii?Q?j7CVmdgWRxUclOR0OrK4AIHOsKTxt9nWkP+ed7u9JYa0y4OaTSZOfDcOx0ID?=
 =?us-ascii?Q?Al2E6OpoHXtAl9sit+45Gvqtl+ROOoyM1TSKPlzn+CgHh0V3oQe9De28BHVI?=
 =?us-ascii?Q?0lcI0iZMy1/j5eFXqKO97yD9cW/XPMphTynudPONJgYm+wDPRWm13/LD2qcv?=
 =?us-ascii?Q?UKK3/2MdYUiPksDl4zFwPTualvJjaaW8ETVWUBeLCzPfrPnfgOjQa1ExsGbV?=
 =?us-ascii?Q?7hr6bWc2WetM8KuWGLNK9+nOrIj+B2TkYq8pO75EW6DnH3wH44qVp1IKa0ns?=
 =?us-ascii?Q?hES/9J712BD+ZYOIiaeEzcOeslblxjAv2t7Qs7DSHC7laySzMGlTV/VHQHzK?=
 =?us-ascii?Q?6Q8pzCEmHkdPuLmqGmEc+7EKToLOtWmnWjFqqipLoKhbljY16ergeM3N5eWH?=
 =?us-ascii?Q?PKK3GVI1+XfeT3MaGCeiO2svrN2sCIfMEbnW5xUKZNkZYWVOj8IIr941I3ep?=
 =?us-ascii?Q?raoIJ/ApCUrQwW7XbRF0K/TEKChxUcNEO1QUaXe5T8Bek1eyROBiRYwJRi4Y?=
 =?us-ascii?Q?3bGWw0qKLII4XT1EcHpPU34XsPKdhfccmCNvM7iJC2CuA0pzHpMVThQ9y0mI?=
 =?us-ascii?Q?5OABlyCavizCFtYebDMQrIWDJFHnpwAqvm9hhhRTS3UC61ZvJJ+C1gYBucic?=
 =?us-ascii?Q?KcWs87I6jmfw9cKYw3c+Atwiu+1GMvdKzkjpb7zP6TWng9+fWeeuXw17UH97?=
 =?us-ascii?Q?M+beMCbJSt+i35ikYm7GOwIx21kWAQUXfyS2okyESABoivlPcbg43e50DLvC?=
 =?us-ascii?Q?gfONknhjklFmj8QFvM5jKkvAsSmp68dPQCJB1zDBw6oIxi3xURd23yEKZ8vL?=
 =?us-ascii?Q?EFx0bAV/6NR+++jNqEfRnLL+KtPHSKgIRG1ERzdOonSb6fAWnnQ9C8uRZL+I?=
 =?us-ascii?Q?SA=3D=3D?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bcafd2e2-42dc-490d-6b7e-08db1e1a2c53
X-MS-Exchange-CrossTenant-AuthSource: GV1PR04MB9055.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Mar 2023 08:09:56.7047
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 0MWP+mganp6BQwT9xu+/xn8CA/71VCfQ+bZ/kMHJibN5RcreFVzknsI9tM54CfHB3UrAtGsi/G9T7LxpAsFE9w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB7120
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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);
>  
> --