Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Date:   Tue, 3 Oct 2023 18:00:47 +0300
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc:     linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Madalin Bucur <madalin.bucur@nxp.com>,
        Ioana Ciornei <ioana.ciornei@nxp.com>,
        Camelia Groza <camelia.groza@nxp.com>,
        Li Yang <leoyang.li@nxp.com>, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor@kernel.org>,
        Sean Anderson <sean.anderson@seco.com>,
        Maxime Chevallier <maxime.chevallier@bootlin.com>,
        Vinod Koul <vkoul@kernel.org>,
        Kishon Vijay Abraham I <kishon@kernel.org>
Subject: Re: [RFC PATCH v2 net-next 08/15] net: phylink: allow PCS to handle
 C73 autoneg for phy-mode = "internal"
Message-ID: <20231003150047.qyzbe4gwtdgcw474@skbuf>
References: <20230923134904.3627402-1-vladimir.oltean@nxp.com>
 <20230923134904.3627402-9-vladimir.oltean@nxp.com>
 <20231002141743.lbmb66q22dmuyi6f@skbuf>
 <ZRv1qt7+WbQd457L@shell.armlinux.org.uk>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZRv1qt7+WbQd457L@shell.armlinux.org.uk>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?4jqAMcoQrW74V4BWs9umGLxvw611emGMr/3E20y/jJEuamXkdzvA2F8aIaJT?=
 =?us-ascii?Q?XSk6RecjzHVYtOwmU1mplLfDppxWvgg1Ax8R47VGKxf6qTo/dLSKIfebhQN3?=
 =?us-ascii?Q?23CBiInhnsRKgYoPFZ+71Q5fDyfN4PR9KhBZ0bE9UY81AAO/fuluqZbAh+Wa?=
 =?us-ascii?Q?Li5GCMdQC0yPS3OT1SqwIgVZVNR/jofy1Mr/QOvQIJAIxIh1NwJE5WwI3PDp?=
 =?us-ascii?Q?kkodKNx3HJswfOq+0OMZIugHz4xuEMR/nBcOMHaEhzNExO0M0/cG4YXiyYp4?=
 =?us-ascii?Q?Oj4ag+oFUMHe2X1uD3lwv7/mz0CnD7EtSkSqCH9xARdv2K5w4whIEw32BJBi?=
 =?us-ascii?Q?3WbLuH/40IBMB/qll1I8iPLwSWy5SlUXJSzGMhVhvcB6WWz4lVHvzuVAgkMs?=
 =?us-ascii?Q?FwupHNmrOzwi1yxuocJzXCrXbdS+EjwbTqgzHOtT20O9uiwL3f8okeERIvMO?=
 =?us-ascii?Q?XHaelHchr4xOdfJdeOODUD6/w0WIiOS+PzXyFF7qnIm9WG60OsjoUFeO+dS7?=
 =?us-ascii?Q?uiEhvcTlkcmv3xpcFPB6duBRb+rrYIJ3720RfEvmmFh9SnQJlZbS3EHz4Uvt?=
 =?us-ascii?Q?8IePn9DZcPV6om1Qse8gWeEEJ7+Otcpd9Jw/XZeImTTfjK96xGa7TEfVYPSq?=
 =?us-ascii?Q?GyFr0PCudML1/FV/aTmagAGAAOKr/OsFIOHekqX4FWYkBPaKUIGu6pR/ZZ7a?=
 =?us-ascii?Q?Lxv8OCoKekIq6nvtTpOTdN6I0Hz5psvhqtJ6wQg+VA7q9b2LQX9GG48bcko9?=
 =?us-ascii?Q?1ZTusQ/7XmrN4s/+GKRnc58w2A8JwX/ggBEwlM7qu8zCbjGHWSBeiiVNLr4a?=
 =?us-ascii?Q?QQ4P7KcO6RIpUzFSdUbXbaz9N1jdnG4O9LU3umLP4BXhSlPEDHIeOhrVcTvE?=
 =?us-ascii?Q?QsIsKFGXoWH4/iJ02ccCiXUsV4zPEnV+XHEcT9rGr8xPsUBIj+p1fcFTL6n0?=
 =?us-ascii?Q?RL1FQfW6tAhWHH0dcoPVo6/hWRLAOR/4EHucxYDSOA5kF+a98+N+iVutlXN9?=
 =?us-ascii?Q?vGI2Ua35gWEALu71kAsRw8vG0mRmeeypTJKVKa4nKc5UJK8yXrX+G8Hbm9fX?=
 =?us-ascii?Q?Zwm8WGlD7FhllvhqPGDzb6ayWvXEjhkJdZqruarGlF8vqsFZnRaztMmr9ZzA?=
 =?us-ascii?Q?QBoYTC/CBPf4dedva/yfg0Hy6C7DVgMTYRVAs3FGTz1qrmK0SUg9J7ZnnTuK?=
 =?us-ascii?Q?sPuEG0C0YnEv0VFo04kd1ITclmtJRqFQvo2iAB/7Ovc/tZCdkA7rw6XZ510U?=
 =?us-ascii?Q?ZLwU0haNJwwJlh8NWzRp9PbVsi8uIShwEiiLD3rK6Zwiy9s8KjjEld+EMaYo?=
 =?us-ascii?Q?CHDLPGbYKbI64D3DZAYjgxScdXWNDsDvPAi06hFH5LijCOccxhUm8KbjhdaR?=
 =?us-ascii?Q?5XE35aZDK8DcxoYux6/EuNiK3NNg+fV6X+WjYnkPKFCyS+ByuHElhDl5o32R?=
 =?us-ascii?Q?wAVrKlyQxN8tGc5IPDbfrNowa+Qpj7FnmMsF+/bsGQz8CUXBLvVpvC0/9ScL?=
 =?us-ascii?Q?jVE+S6vCdQaOB3CpFZ0ns2NR/z2JQW8brcH/KJg96J53O4MlaawcRtok1xu6?=
 =?us-ascii?Q?J56gYBvMExMKTIRJDRrJSLhJr1Qt2wTlXLKrST8OYRo5hv7sDtkHSRpeLMgX?=
 =?us-ascii?Q?/w=3D=3D?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: aca0eada-0433-4c4f-c38c-08dbc4218947
X-MS-Exchange-CrossTenant-AuthSource: AM0PR04MB6452.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Oct 2023 15:00:52.1283
 (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: TJerrem/A9lfmpRbfIi0rSPSFz2bZln192u/dM5CWUEoMqe9ycTOXOqTLuuSMtOvykpIBeueQl0rbT+bd5PPgA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PA4PR04MB8062
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Tue, 03 Oct 2023 08:01:14 -0700 (PDT)

Hi Russell,

On Tue, Oct 03, 2023 at 12:06:18PM +0100, Russell King (Oracle) wrote:
> I wonder whether this should just set all link modes, much like
> phylink_get_capabilities() allows.
> 
> I'm also wondering whether the contents of this switch() statement
> should now just do:
> 
> 		case PHY_INTERFACE_... (for each supported mode):
> 			cals = ~(MAC_SYM_PAUSE | MAC_ASYM_PAUSE);
> 			caps = phylink_get_capabilities(interface, caps,
> 							RATE_MATCH_NONE);
> 			phylink_caps_to_linkmodes(pl->supported, caps);
> 			break;
> 
> rather than duplicating the logic.

You mean something like this?

[PATCH] net: phylink: reimplement population of pl->supported for in-band

phylink_parse_mode() populates all possible supported link modes for a
given phy_interface_t, for the case where a phylib phy may be absent and
we can't retrieve the supported link modes from that.

Russell points out that since the introduction of the generic validation
helpers phylink_get_capabilities() and phylink_caps_to_linkmodes(), we
can rewrite this procedure to populate the pl->supported mask, so that
instead of spelling out the link modes, we derive an intermediary
mac_capabilities bit field, and we convert that to the equivalent link
modes.

Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/phy/phylink.c | 71 +++------------------------------------
 1 file changed, 5 insertions(+), 66 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 548130d77302..88a4b726a9fc 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -864,6 +864,7 @@ static int phylink_parse_mode(struct phylink *pl,
 {
 	struct fwnode_handle *dn;
 	const char *managed;
+	unsigned long caps;
 
 	dn = fwnode_get_named_child_node(fwnode, "fixed-link");
 	if (dn || fwnode_property_present(fwnode, "fixed-link"))
@@ -896,80 +897,18 @@ static int phylink_parse_mode(struct phylink *pl,
 		case PHY_INTERFACE_MODE_RGMII_RXID:
 		case PHY_INTERFACE_MODE_RGMII_TXID:
 		case PHY_INTERFACE_MODE_RTBI:
-			phylink_set(pl->supported, 10baseT_Half);
-			phylink_set(pl->supported, 10baseT_Full);
-			phylink_set(pl->supported, 100baseT_Half);
-			phylink_set(pl->supported, 100baseT_Full);
-			phylink_set(pl->supported, 1000baseT_Half);
-			phylink_set(pl->supported, 1000baseT_Full);
-			break;
-
 		case PHY_INTERFACE_MODE_1000BASEX:
-			phylink_set(pl->supported, 1000baseX_Full);
-			break;
-
 		case PHY_INTERFACE_MODE_2500BASEX:
-			phylink_set(pl->supported, 2500baseX_Full);
-			break;
-
 		case PHY_INTERFACE_MODE_5GBASER:
-			phylink_set(pl->supported, 5000baseT_Full);
-			break;
-
 		case PHY_INTERFACE_MODE_25GBASER:
-			phylink_set(pl->supported, 25000baseCR_Full);
-			phylink_set(pl->supported, 25000baseKR_Full);
-			phylink_set(pl->supported, 25000baseSR_Full);
-			fallthrough;
 		case PHY_INTERFACE_MODE_USXGMII:
 		case PHY_INTERFACE_MODE_10GKR:
 		case PHY_INTERFACE_MODE_10GBASER:
-			phylink_set(pl->supported, 10baseT_Half);
-			phylink_set(pl->supported, 10baseT_Full);
-			phylink_set(pl->supported, 100baseT_Half);
-			phylink_set(pl->supported, 100baseT_Full);
-			phylink_set(pl->supported, 1000baseT_Half);
-			phylink_set(pl->supported, 1000baseT_Full);
-			phylink_set(pl->supported, 1000baseX_Full);
-			phylink_set(pl->supported, 1000baseKX_Full);
-			phylink_set(pl->supported, 2500baseT_Full);
-			phylink_set(pl->supported, 2500baseX_Full);
-			phylink_set(pl->supported, 5000baseT_Full);
-			phylink_set(pl->supported, 10000baseT_Full);
-			phylink_set(pl->supported, 10000baseKR_Full);
-			phylink_set(pl->supported, 10000baseKX4_Full);
-			phylink_set(pl->supported, 10000baseCR_Full);
-			phylink_set(pl->supported, 10000baseSR_Full);
-			phylink_set(pl->supported, 10000baseLR_Full);
-			phylink_set(pl->supported, 10000baseLRM_Full);
-			phylink_set(pl->supported, 10000baseER_Full);
-			break;
-
 		case PHY_INTERFACE_MODE_XLGMII:
-			phylink_set(pl->supported, 25000baseCR_Full);
-			phylink_set(pl->supported, 25000baseKR_Full);
-			phylink_set(pl->supported, 25000baseSR_Full);
-			phylink_set(pl->supported, 40000baseKR4_Full);
-			phylink_set(pl->supported, 40000baseCR4_Full);
-			phylink_set(pl->supported, 40000baseSR4_Full);
-			phylink_set(pl->supported, 40000baseLR4_Full);
-			phylink_set(pl->supported, 50000baseCR2_Full);
-			phylink_set(pl->supported, 50000baseKR2_Full);
-			phylink_set(pl->supported, 50000baseSR2_Full);
-			phylink_set(pl->supported, 50000baseKR_Full);
-			phylink_set(pl->supported, 50000baseSR_Full);
-			phylink_set(pl->supported, 50000baseCR_Full);
-			phylink_set(pl->supported, 50000baseLR_ER_FR_Full);
-			phylink_set(pl->supported, 50000baseDR_Full);
-			phylink_set(pl->supported, 100000baseKR4_Full);
-			phylink_set(pl->supported, 100000baseSR4_Full);
-			phylink_set(pl->supported, 100000baseCR4_Full);
-			phylink_set(pl->supported, 100000baseLR4_ER4_Full);
-			phylink_set(pl->supported, 100000baseKR2_Full);
-			phylink_set(pl->supported, 100000baseSR2_Full);
-			phylink_set(pl->supported, 100000baseCR2_Full);
-			phylink_set(pl->supported, 100000baseLR2_ER2_FR2_Full);
-			phylink_set(pl->supported, 100000baseDR2_Full);
+			caps = ~(MAC_SYM_PAUSE | MAC_ASYM_PAUSE);
+			caps = phylink_get_capabilities(pl->link_config.interface, caps,
+							RATE_MATCH_NONE);
+			phylink_caps_to_linkmodes(pl->supported, caps);
 			break;
 
 		default:

I've superficially tested it on 2 boards (LS1028A-RDB and Turris MOX)
and I didn't notice regressions.

But...

> That said, 10GBASER and 10GKR are treated slightly differently because
> of the problem with PHYs like 88x3310, and I think it's now difficult
> to undo that bit of history.

I don't really understand this. Can you please show me where and how
PHY_INTERFACE_MODE_10GBASER and PHY_INTERFACE_MODE_10GKR are treated
differently in the kernel?

> > > +
> > >  		default:
> > >  			phylink_err(pl,
> > >  				    "incorrect link mode %s for in-band status\n",
> > > @@ -1109,7 +1124,9 @@ static void phylink_mac_config(struct phylink *pl,
> > >  
> > >  static bool phylink_pcs_handles_an(phy_interface_t iface, unsigned int mode)
> > >  {
> > > -	return phy_interface_mode_is_8023z(iface) && phylink_autoneg_inband(mode);
> > > +	return (phy_interface_mode_is_8023z(iface) ||
> > > +		iface == PHY_INTERFACE_MODE_INTERNAL) &&
> > > +	       phylink_autoneg_inband(mode);
> 
> Is this true also for DSA devices that use "internal" mode? I'm
> wondering whether this will cause the PHY to be ignored/remain
> unattached in DSA switches because of the changes in patch 7.

phylink_pcs_handles_an() shouldn't return true for those, no. For that
to happen, phylink_autoneg_inband() should also return true for that link.

Which I believe it never does, because prior to this change, phylink_parse_mode()
would have errored out with -EINVAL and the "incorrect link mode %s for in-band status\n"
text for phy-mode = "internal".

> > >  }
> > >  
> > >  static void phylink_pcs_an_restart(struct phylink *pl)
> > > diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> > > index 2b886ea654bb..7e8e26001587 100644
> > > --- a/include/linux/phylink.h
> > > +++ b/include/linux/phylink.h
> > > @@ -141,6 +141,7 @@ static inline unsigned int phylink_pcs_neg_mode(unsigned int mode,
> > >  
> > >  	case PHY_INTERFACE_MODE_1000BASEX:
> > >  	case PHY_INTERFACE_MODE_2500BASEX:
> > > +	case PHY_INTERFACE_MODE_INTERNAL:
> > >  		/* 1000base-X is designed for use media-side for Fibre
> > >  		 * connections, and thus the Autoneg bit needs to be
> > >  		 * taken into account. We also do this for 2500base-X
> 
> Thinking about DSA cases, I don't think this change would be an issue
> because where DSA uses "internal" there isn't a PCS, so this won't
> matter.

I agree, and also see the justification above.

> Note that as there is now no need for anything outside phylink.c to
> reference this function, I have plans at some point to move it into
> the .c file rather than keeping it as an inline in the header file.
> It was temporarily necessary while introducing it to be in the
> header.

Ok. Do you want me to move phylink_pcs_neg_mode() to phylink.c as part
of this series?