Received-SPF: pass (google.com: domain of linux-kernel+bounces-73575-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Received-SPF: Pass (protection.outlook.com: domain of seco.com designates
 20.160.56.85 as permitted sender) receiver=protection.outlook.com;
 client-ip=20.160.56.85; helo=repost-eu.tmcas.trendmicro.com; pr=C
Authentication-Results-Original: dkim=none (message not signed)
 header.d=none;dmarc=none action=none header.from=seco.com;
Message-ID: <191b4477-7b4b-47eb-bb3e-0e4d08b3b32e@seco.com>
Date: Tue, 20 Feb 2024 14:12:52 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] arm64: ls1046ardb: Replace XGMII with 10GBASE-R phy mode
Content-Language: en-US
To: Vladimir Oltean <vladimir.oltean@nxp.com>,
 zachary.goldstein@concurrent-rt.com
Cc: Shawn Guo <shawnguo@kernel.org>, Madalin Bucur <madalin.bucur@nxp.com>,
 Li Yang <leoyang.li@nxp.com>, Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>, Andrew Lunn <andrew@lunn.ch>,
 Heiner Kallweit <hkallweit1@gmail.com>, Russell King
 <linux@armlinux.org.uk>, "David S. Miller" <davem@davemloft.net>,
 Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>,
 Paolo Abeni <pabeni@redhat.com>, linux-arm-kernel@lists.infradead.org,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
 netdev@vger.kernel.org
References: <20240220145037.kf3avnykjif24kkr@skbuf>
From: Sean Anderson <sean.anderson@seco.com>
In-Reply-To: <20240220145037.kf3avnykjif24kkr@skbuf>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
X-Microsoft-Antispam-Message-Info-Original:
 CwghI5hrI/Tm8BnhkYOiA8IbfePlfkZiRPt47XJ/z3Duwc1JoMTCp0oCTrvOIDwHpGMpNf1jmKrWCXsXrC3hKhKVpc/HOJ+u7lB05W0oxbtdHkLyvQGv5Jwz21uCn9jr6eVRyqxnNL5VqCR8IiMFwC4fHnbE9NwxFnXWkqs8LLd65yroPjCYVg1b7m44FFmVmOWRl/OZ66qLOIR67ZXg7ZS1ORNfkloZt1/z0Lbo3IfYoGGIAtqTASWNsYkObLzc6WgEbeZhmbcQpP6AgVwWajgmA7Iiqa85rDzIXNCm0KBbNrRcOv3KKPnshabQwIp20XJVosYFNq8fYNCDA/fMTGMumDGRtzppHf+fawmJg+qz49i84tbasEViOHgIMHoK1/LDvRXwg7G8kHKdbDdASxXu1qMwczBNC7f0LNpewwYHsOQcgsEWHLUVFQvQxP9Po2eAWOiovkdMAFC+NqlaY/CEc26lJHwqcAKKB/QB2g5lUO/4T3uoWWnvWMwn+6ReEFxa4nM4OQ/bU3dbVSbloefsxHjgHc4pldkPCIonmUkzGt7kS1K+dNtEI7T1X680v5/2rnTfQ8jsVje36P88UzIxxT6Ie4O6ppXONZkLZu8=
X-Forefront-Antispam-Report-Untrusted:
 CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DB9PR03MB8847.eurprd03.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230031)(230273577357003)(38350700005);DIR:OUT;SFP:1102;
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM9PR03MB6898
X-EOPAttributedMessage: 0
X-MS-Exchange-Transport-CrossTenantHeadersStripped:
 DU2PEPF00028D10.eurprd03.prod.outlook.com
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id-Prvs:
	1dfb0ef7-3a79-4d4c-a77e-08dc3247f218
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info:
	sLS2A0Mi86ZrWVfMp9yGJNxgq+FQ52iQjWDE3jDfdAeRSKpG7K3Sv5TTJ/s/dxAB7xMEkrxsu2QneR6z+CEuA2jVVw2knfvhRtt+7PQRXBJMZD3GhkSXIxe3FwjK8JVseHlt9qXUWHa3TwEmjB1Ye51gh/WJTXqGmgvsv+jBBEZL10Gp508HmAF/WDe0/h4+h9PYLBBlCP49jPHSCDPbPqYzA2UKbHsjpgfJQXwSdrmmVds1ZUwPyEBN6wF8oO6Q2PRMTNg8Q16yH7ENyLzLr4bRqSHum3TALF5+jKCSoTSMBgM7u6hrThp54LnAKkhxpk27IVT6pMe0xj4KDjpXcYd+qXAELKkMasC7/324I190zBvzIK9852lXaJ9apQv5s1yo5ySvFnIBJtCuP20WhcbaKgtsDyytcIZQGGIbVo8USByKkjloaaSNPFMXb+mGInb2Av0XXUKkOPsHanojAEb6y31aCsw3xkqZcpuUOHXmxxUfBM1sr8jnmsa8jnr7Bjr05UjmiHAhS5tepCycWC0Et76nB/h+9ItRkXwNVtEiiyg9VMPPtcalSj5t2Ee1A9lbXzBscdP77cf/6qW2xoe5491/aYuHGRPVkLPj/ECrYisLnAsw2eLVeOm17GRszouAMgWz/TLOPIAaTWhRlA==
X-Forefront-Antispam-Report:
	CIP:20.160.56.85;CTRY:NL;LANG:en;SCL:1;SRV:;IPV:CAL;SFV:NSPM;H:repost-eu.tmcas.trendmicro.com;PTR:repost-eu.tmcas.trendmicro.com;CAT:NONE;SFS:(13230031)(230273577357003)(36860700004)(46966006)(40470700004);DIR:OUT;SFP:1102;
X-OriginatorOrg: seco.com
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Feb 2024 19:13:02.7376
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 3b7e00d7-82b2-42bd-8e82-08dc3247f5d6
X-MS-Exchange-CrossTenant-Id: bebe97c3-6438-442e-ade3-ff17aa50e733
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=bebe97c3-6438-442e-ade3-ff17aa50e733;Ip=[20.160.56.85];Helo=[repost-eu.tmcas.trendmicro.com]
X-MS-Exchange-CrossTenant-AuthSource:
	DU2PEPF00028D10.eurprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR03MB8284

On 2/20/24 09:50, Vladimir Oltean wrote:
> Hi Zachary,
>
> On Wed, Feb 14, 2024 at 05:21:54PM -0500, Zachary Goldstein via B4 Relay =
wrote:
>> From: Zachary Goldstein <zachary.goldstein@concurrent-rt.com>
>>
>> The AQR107 family does not support XGMII, but USXGMII and
>> 10GBASE-R instead. Since ce64c1f77a9d ("net: phy: aquantia: add USXGMII
>> support and warn if XGMII mode is set") the kernel warns about XGMII
>> being used. The LS1046A SoC does not support USXGMII, so use 10GBASE-R
>> instead.
>>
>> Signed-off-by: Zachary Goldstein <zachary.goldstein@concurrent-rt.com>
>> ---
>>  arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts b/arch/ar=
m64/boot/dts/freescale/fsl-ls1046a-rdb.dts
>> index 07f6cc6e354a..d2066f733dc5 100644
>> --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
>> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
>> @@ -149,7 +149,7 @@ ethernet@ea000 {
>>
>>      ethernet@f0000 { /* 10GEC1 */
>>              phy-handle =3D <&aqr106_phy>;
>> -            phy-connection-type =3D "xgmii";
>> +            phy-connection-type =3D "10gbase-r";
>>      };
>>
>>      ethernet@f2000 { /* 10GEC2 */
>>
>> ---
>> 2.40.1
>> base-commit: 841c35169323cd833294798e58b9bf63fa4fa1de
>>
>>
>
> You do not need this patch in upstream, and I strongly advise against
> merging it as-is. I've just tested pure net-next on LS1046A-RDB, and I
> can bring up fm1-mac9 without any warning.
>
> You'll notice that commit 5d93cfcf7360 ("net: dpaa: Convert to phylink")
> did this in fman_memac.c:
>
>       /* The internal connection to the serdes is XGMII, but this isn't
>        * really correct for the phy mode (which is the external connectio=
n).
>        * However, this is how all older device trees say that they want
>        * 10GBASE-R (aka XFI), so just convert it for them.
>        */
>       if (mac_dev->phy_if =3D=3D PHY_INTERFACE_MODE_XGMII)
>               mac_dev->phy_if =3D PHY_INTERFACE_MODE_10GBASER;
>
> So, what gets passed to the Aquantia PHY is PHY_INTERFACE_MODE_10GBASER,
> even if in the device tree, phy-connection-type =3D "xgmii".
>
> Now, _if_ your patch were to be applied on top of upstream, it would
> actually break fm1-mac9 like this (WARN_ON added by me for a call stack
> of the phylink_validate() failure path):
>
> WARNING: CPU: 2 PID: 1 at drivers/net/phy/phylink.c:763 phylink_create+0x=
8f8/0x90c
> Modules linked in:
> CPU: 2 PID: 1 Comm: swapper/0 Tainted: G        W          6.8.0-rc4-0105=
8-g9e1deba407fb #1812
> Hardware name: LS1046A RDB Board (DT)
> Call trace:
>  phylink_create+0x8f8/0x90c
>  dpaa_netdev_init+0x1a8/0x2c8
>  dpaa_eth_probe+0xd70/0xf64
>  platform_probe+0xa8/0xd0
>  really_probe+0x130/0x2e4
>  __driver_probe_device+0xa0/0x128
>  driver_probe_device+0x3c/0x200
>  __driver_attach+0xe8/0x1b4
>  bus_for_each_dev+0xec/0x144
>  driver_attach+0x24/0x30
>  bus_add_driver+0x154/0x244
>  driver_register+0x68/0x100
>  __platform_driver_register+0x24/0x30
>  dpaa_load+0x34/0x64
>  do_one_initcall+0xf8/0x34c
> ---[ end trace 0000000000000000 ]---
> fsl_dpaa_mac 1af0000.ethernet (unnamed net_device) (uninitialized): faile=
d to validate link configuration for in-band status
> fsl_dpaa_mac 1af0000.ethernet: error -EINVAL: Could not create phylink
> fsl_dpa: probe of dpaa-ethernet.4 failed with error -22
>
> It fails because of this in phylink_validate():
>
>       if (!test_bit(state->interface, interfaces))
>               return -EINVAL;
>
> And state->interface (PHY_INTERFACE_MODE_10GBASER) is not in
> mac_dev->phylink_config.supported_interfaces, because the fman_memac
> code is not prepared to handle this combination.
>
> The device tree node for fm1-mac9 looks like this, generated by an
> awkward merge between the following:
>
>       ethernet@f0000 {
>               phy-connection-type =3D "xgmii";  // fsl-ls1046a-rdb.dts
>               local-mac-address =3D [...];      // U-Boot
>               cell-index =3D <0x8>;             // qoriq-fman3-0-10g-0.dt=
si
>               pcsphy-handle =3D <0x31>;         // qoriq-fman3-0-10g-0.dt=
si
>               compatible =3D "fsl,fman-memac";  // qoriq-fman3-0-10g-0.dt=
si
>               reg =3D <0xf0000 0x1000>;         // qoriq-fman3-0-10g-0.dt=
si
>               phy-handle =3D <&aqr106_phy>;     // fsl-ls1046a-rdb.dts
>               fsl,fman-ports =3D <0x2f 0x30>;   // qoriq-fman3-0-10g-0.dt=
si
>       };
>
> Notice that unlike fm1-mac10 (node "ethernet@f2000"), there is no
> pcs-handle-names property (fm1-mac10 has it defined in fsl-ls1046-post.dt=
si,
> whereas fm1-mac9 doesn't. Don't ask me why, I don't know....)

I think this is just because this ethernet is always XFI and never (Q)SGMII=
.

> So, knowing that of_property_match_string(mac_node, "pcs-handle-names", "=
xfi")
> will return an error code for fm1-mac9, now please walk through memac_ini=
tialization()
> and see what happens in the 2 cases:
>
> - mac_dev->phy_if =3D=3D PHY_INTERFACE_MODE_XGMII (current device tree). =
The
>   code creates a default PCS and assigns it to memac->xfi_pcs like this:
>       if (err =3D=3D -EINVAL || err =3D=3D -ENODATA)
>               pcs =3D memac_pcs_create(mac_node, 0);
>       (...)
>       if (err && mac_dev->phy_if =3D=3D PHY_INTERFACE_MODE_XGMII)
>               memac->xfi_pcs =3D pcs;
>
> - mac_dev->phy_if =3D=3D PHY_INTERFACE_MODE_10GBASER (your modification).
>   The code will still create the default PCS, but assign it to
>   memac->sgmii_pcs instead!
>
>       if (err && mac_dev->phy_if =3D=3D PHY_INTERFACE_MODE_XGMII) // not =
XGMII, but 10GBASER!
>               memac->xfi_pcs =3D pcs;
>       else
>               memac->sgmii_pcs =3D pcs;
>
> And this is why, with a NULL memac->xfi_pcs, PHY_INTERFACE_MODE_10GBASER
> will not be in phylink's supported_interfaces.
>
> To make your device tree patch work as intended with the current
> mainline code, what you want is to also modify the driver like this:
>
> From d6bda34b132d17d1c236d27436b9335fac22c062 Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Tue, 20 Feb 2024 16:12:27 +0200
> Subject: [PATCH] net: dpaa: fman_memac: accept phy-interface-type =3D
>  "10gbase-r" in the device tree
>
> We support the phy-interface-mode =3D "xgmii" conversion to "10gbase-r"
> through code, but not actually through the device tree proper. This is
> because boards such as LS1046A-RDB do not define pcs-handle-names in the
> ethernet@f0000 device tree node, and the code only has fallback xfi_pcs
> determination logic for "xgmii".
>
> By reversing the order between the fallback xfi_pcs assignment and the
> "xgmii" overwrite with "10gbase-r", we are able to support both values
> in the device tree, with identical behavior.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  .../net/ethernet/freescale/fman/fman_memac.c   | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fman/fman_memac.c b/drivers/n=
et/ethernet/freescale/fman/fman_memac.c
> index e30bf75b1d48..0996759907a8 100644
> --- a/drivers/net/ethernet/freescale/fman/fman_memac.c
> +++ b/drivers/net/ethernet/freescale/fman/fman_memac.c
> @@ -1076,6 +1076,14 @@ int memac_initialization(struct mac_device *mac_de=
v,
>       unsigned long            capabilities;
>       unsigned long           *supported;
>
> +     /* The internal connection to the serdes is XGMII, but this isn't
> +      * really correct for the phy mode (which is the external connectio=
n).
> +      * However, this is how all older device trees say that they want
> +      * 10GBASE-R (aka XFI), so just convert it for them.
> +      */
> +     if (mac_dev->phy_if =3D=3D PHY_INTERFACE_MODE_XGMII)
> +             mac_dev->phy_if =3D PHY_INTERFACE_MODE_10GBASER;
> +
>       mac_dev->phylink_ops            =3D &memac_mac_ops;
>       mac_dev->set_promisc            =3D memac_set_promiscuous;
>       mac_dev->change_addr            =3D memac_modify_mac_address;
> @@ -1142,7 +1150,7 @@ int memac_initialization(struct mac_device *mac_dev=
,
>        * (and therefore that xfi_pcs cannot be set). If we are defaulting=
 to
>        * XGMII, assume this is for XFI. Otherwise, assume it is for SGMII=
.
>        */
> -     if (err && mac_dev->phy_if =3D=3D PHY_INTERFACE_MODE_XGMII)
> +     if (err && mac_dev->phy_if =3D=3D PHY_INTERFACE_MODE_10GBASER)
>               memac->xfi_pcs =3D pcs;
>       else
>               memac->sgmii_pcs =3D pcs;
> @@ -1156,14 +1164,6 @@ int memac_initialization(struct mac_device *mac_de=
v,
>               goto _return_fm_mac_free;
>       }
>
> -     /* The internal connection to the serdes is XGMII, but this isn't
> -      * really correct for the phy mode (which is the external connectio=
n).
> -      * However, this is how all older device trees say that they want
> -      * 10GBASE-R (aka XFI), so just convert it for them.
> -      */
> -     if (mac_dev->phy_if =3D=3D PHY_INTERFACE_MODE_XGMII)
> -             mac_dev->phy_if =3D PHY_INTERFACE_MODE_10GBASER;
> -
>       /* TODO: The following interface modes are supported by (some) hard=
ware
>        * but not by this driver:
>        * - 1000BASE-KX

Can you please submit this patch? I noticed this but never had the chance t=
o go
back and debug it.

--Sean

> But!
>
> Device tree is stable ABI, and changes made to the device tree file are
> meant to be backwards and forwards compatible with the code (it can be
> provided separately and not necessarily in lockstep with the kernel
> version. For example, I understand Arm SystemReady IR wants U-Boot to
> provide its own device tree to Linux through EFI). So, in general,
> device tree changes which only work with a corresponding kernel change
> are frowned upon (unless maybe if the kernel change is a bug fix that is
> backported to all relevant stable kernel branches).
>
> So at this stage we should take a step back and re-analyze the cost/benef=
it.
> You said there is a stack trace in the Aquantia PHY driver, which there
> is not (in current mainline kernels). I _think_ you are seeing the stack
> trace with LSDK, which is currently distributed on top of linux-6.1.y
> and has not yet integrated the fman_memac conversion to phylink - thus,
> it does not contain commit 5d93cfcf7360 ("net: dpaa: Convert to phylink")=
.
> At least, I do see this stack trace there. I think it can also be seen
> with mainline kernels before the phylink conversion, but I did not test
> those.
>
> The main take-away is: ALWAYS test the patch you are submitting to the
> target kernel it is going to be applied to. Especially in the area of
> device tree bindings for DPAA1, things are rarely as simple as they
> appear :) If you don't, you will have an unintended negative effect
> upon current mainline kernels (which must still work), and not the
> intended effect upon LSDK (more below).
>
> There are a few options to go forward from here.
>
> As there is nothing broken in the mainline kernel where you are
> submitting this patch, the simplest one, as bland as it may sound, is
> simply to wait for a new LSDK release on top of linux-6.6.y. Even in
> lf-6.1.y, AFAICS, nothing is broken except for the stack trace. You can
> keep the patch in your local kernel tree copy to suppress that.
>
> The other option would be to submit the fman_memac change as a bug fix
> for stable, wait for a while for it to have time to propagate, then
> modify the device tree as well. But, it is much higher effort, and
> there is no procedure in place, AFAIK, for LSDK to integrate your patch
> (other than through upstream + a few months of waiting). The upcoming
> LSDK release will be on top of linux-6.6.y, it will make your effort
> irrelevant if it's only for suppressing the stack trace, and you are
> racing against it. This path is only worth it if you have the dedication
> to dig a bit deeper and tidy things up in the DPAA1 kernel support
> (which would be appreciated though).
>
> _If_ you are using an older linux-stable branch but not LSDK, yes, the
> feedback loop between your patch and its effect will close much sooner,
> but you will have to convince the linux-stable people that it's worth
> accepting your driver rework patches for a functional reason and not
> just aesthetic (see Documentation/process/stable-kernel-rules.rst for
> reference).
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=
=3Dhttp%3a%2f%2flists.infradead.org%2fmailman%2flistinfo%2flinux%2darm%2dke=
rnel&umid=3D8bfcbf91-eb84-4daf-8653-378929ed0326&auth=3Dd807158c60b7d2502ab=
de8a2fc01f40662980862-d89d931594581c69e3bf19f86f11d6de905c5a8b


[Embedded World 2024, SECO SpA]<https://www.messe-ticket.de/Nuernberg/embed=
dedworld2024/Register/ew24517689>