From:   Ken Sloat <ken.s@variscite.com>
To:     Andrew Lunn <andrew@lunn.ch>, Jakub Kicinski <kuba@kernel.org>
CC:     Michael Hennerich <michael.hennerich@analog.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        "David S. Miller" <davem@davemloft.net>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Ken Sloat <ken.s@variscite.com>
Subject: RE: [PATCH v1] net: phy: adin: Add flags to disable enhanced link
 detection
Thread-Topic: [PATCH v1] net: phy: adin: Add flags to disable enhanced link
 detection
Thread-Index: AQHZS4L7Pv2IgKC+70qPmaRcnFBcv67kcbqAgAAD6KCAAANXgIAAzIOAgACfsACAAADx8A==
Date:   Wed, 1 Mar 2023 14:08:01 +0000
Message-ID: <DU0PR08MB9003892BC6DFC96B5FCC5ED5ECAD9@DU0PR08MB9003.eurprd08.prod.outlook.com>
References: <20230228144056.2246114-1-ken.s@variscite.com>
 <Y/4VV6MwM9xA/3KD@lunn.ch>
 <DU0PR08MB900305C9B7DD4460ED29F5FBECAC9@DU0PR08MB9003.eurprd08.prod.outlook.com>
 <Y/4ba4s37NayCIwW@lunn.ch> <20230228193105.0f378a9d@kernel.org>
 <Y/9M7nPZk8qMt0ZO@lunn.ch>
In-Reply-To: <Y/9M7nPZk8qMt0ZO@lunn.ch>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?KXKoFGS5TSuqzE5l9Tk6MBy2L1hnYa9mUTdAEg7b0WRo/oW4OOGrVuZ99JJM?=
 =?us-ascii?Q?I9J0YdHjQFIb37Dy1rNVP/+vDI7gqqu+DhM7Hyfv2cLrUUlLtvOQ80bq+bxx?=
 =?us-ascii?Q?rd1v+pHTQDIOLNBGoZzvE2wKxf1vyjS+enLeFAx8P0SNDfxK4/ey/p+tTTaw?=
 =?us-ascii?Q?NF9BFi5bIhEXi3Xs2GV3tx0SPjXoGzkcTvP98i/gnDDTKwtx+kVm6U7Rooep?=
 =?us-ascii?Q?xJ3MK8BdmO+Ww2fc/FyjN9gO9unH282pJAuoUMmRytccu4AKu8tvhsDUZL2O?=
 =?us-ascii?Q?NRI44czJcq11JS8cnRbael+4PuiMfRvuSuN24wM9Ec1lmBh/BdEUsrclSg9R?=
 =?us-ascii?Q?ivxKC7o3w0yyuYtVBwIoff37Osc8P6TnQE5qZEyNrqGX9dgFiHwXtinWCKwb?=
 =?us-ascii?Q?uvHD3QNL5UpCE8mUUDw8xGddtWsDlg8jDmtyZ+iCXVbhd0EM9VORQX4wS8yi?=
 =?us-ascii?Q?BFL7W8HeNoPc+qQsoZo9mPJBtVF/tqGIJsuVBnBSIqaiEqZ1qU6xrcekPb8u?=
 =?us-ascii?Q?RLcEjD51wWHr+yhjPfQMFeatUDdXpKPTJ0JYVGiRGt8cN1DOznqw1hfQVUSP?=
 =?us-ascii?Q?UeMz2a4eiZUll/wgrGEU4TXiDxKYFE17QW9Pdk2CycVrorpRc6Tjng0e2X/p?=
 =?us-ascii?Q?9ihMAupga6DawCtk/efX/9aj0EP9EwGe7EGhi4p8jHaozuwizydK7VnazFuc?=
 =?us-ascii?Q?nsEDjWX1dmspAmAHnZZCzeukfcafY17wLkalfidqBPdzCWEduxRQCFkogLdP?=
 =?us-ascii?Q?GthDaeuqxv8DGXE6SB/LGziyFZ0c5u6deeWBQdPiO7bQ5lml5OdRWGKf3Yb+?=
 =?us-ascii?Q?iQAEpSpGyrsMBd91M4KHEnmF6gfKwln49hIcWWzIKSD3PPp+17q6vpvF/xTu?=
 =?us-ascii?Q?fZg+r8D0a7dZ6LuvvnyJyaVUu7YYeGAbUvn0OKyXKhJBSsObpe1WpY8hc+kN?=
 =?us-ascii?Q?xzucTc6qZI4QNb5Zw2WV4+PSbdud3CYyNCM8PuIZfhBINbKg9tWpF5iuyrtl?=
 =?us-ascii?Q?2WD7emtHUGx/22wXNeMo1d4ahqPGvN1VsQGNTf+Q+bIFXfYxBZaeHK3fC6ER?=
 =?us-ascii?Q?gdhWBgJ/X8Am5B6JN3B7iPPn7MAjHP/pwK4mQSprduEKFW3rtIah3vNAM+tE?=
 =?us-ascii?Q?GGGehtwvasi6DZ6NZR5X9c1pbxqd/+dH3VxFzHc1rbJ+TeNF28GQYYaUD6tJ?=
 =?us-ascii?Q?wPj1z9cUIL6v1VDCHPaSziAX+SdhmjIE6X8CkCntQpVDmVEHwcKQoKn4aVcU?=
 =?us-ascii?Q?zwlp5ZJ68mP1+jKuCQ1Wa5/iKXljV5BvKNDaPbviG0K+puEl3HPtu4VpuEIq?=
 =?us-ascii?Q?iX9gxE0Mz1z1zAG+hL4BSOC4eIftSPi8rMIPQpsoHY31sMck6DkJnAXewmYP?=
 =?us-ascii?Q?Yn6cJtk1B/R/L9Vi5wyceAVCyfJB233Iw9P6g0Gftp3o4V5gTA/eooa/4Uuj?=
 =?us-ascii?Q?s610nY5a7BlJ2VuaSi87I0/D6bLkBm119GIy3PSNflFqvfILFAt6gB9XF2WC?=
 =?us-ascii?Q?+yCqI+vFvJFtE+B0Opvkk0OlqwYTrlhREU12pQY/h/5UGeXN1wTDpmu0Aaa2?=
 =?us-ascii?Q?ADIENLyL1pyW19alXEVbdCb/VmwnJjjb8uch69+H?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: variscite.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DU0PR08MB9003.eurprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 068f084f-d22f-41f5-d612-08db1a5e5e55
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Mar 2023 14:08:01.4832
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 399ae6ac-38f4-4ef0-94a8-440b0ad581de
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: T+upMAagMiMe1NHaxaqyLU+47dt9Wkw4ayaPf5Hb0vdShjZ1g+E7Yla4HGYKZsnODGsbIpxVjIQJuSuVtTLqow==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PR3PR08MB5865
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, March 1, 2023 8:03 AM
> To: Jakub Kicinski <kuba@kernel.org>
> Cc: Ken Sloat <ken.s@variscite.com>; Michael Hennerich
> <michael.hennerich@analog.com>; Heiner Kallweit
> <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>; David S.
> Miller <davem@davemloft.net>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v1] net: phy: adin: Add flags to disable enhanced lin=
k
> detection
>=20
> On Tue, Feb 28, 2023 at 07:31:05PM -0800, Jakub Kicinski wrote:
> > On Tue, 28 Feb 2023 16:19:07 +0100 Andrew Lunn wrote:
> > > The Marvell PHYs also support a fast link down mode, so i think
> > > using fast link down everywhere, in the code and the commit message
> > > would be good. How about adin_fast_down_disable().
> >
> > Noob question - does this "break the IEEE standard" from the MAC<>PHY
> > perspective or the media perspective? I'm guessing it's the former and
> > the setting will depend on the MAC, given configuration via the DT?
>=20
> IEEE 802.3 says something like you need to wait 1 second before declaring
> the link down. For applications like MetroLAN, 1 second is too long, they
> want to know within something like 50ms so they can swap to a hot standby=
.
>=20
> Marvell PHYs have something similar, there is a register you can poke to
> shorten the time it waits until it declares the link down. I'm sure other=
s PHYs
> have it too.
>=20
> Ah, we already have a PHY tunable for it, ETHTOOL_PHY_FAST_LINK_DOWN.
> I had forgotten about that. The Marvell PHY supports its.
>=20
> So i have two questions i guess:
>=20
> 1) Since it is not compliant with 802.3 by default, do we actually want i=
t
> disabled by default? But is that going to cause regressions?
> Or there devices actually making use of this feature of this PHY?
>=20
I would think you have a risk here like you said of regression, perhaps som=
e users are not even aware of this feature, but their system is somehow rel=
iant on it.

I am not an IEEE expert, but by examining the datasheet, we can see that cl=
earing this bit alone does not guarantee IEEE compliance. There is another =
related feature, which is "1000BASE-T retrain" which is disabled by default=
 because as the datasheet explains, it should not be enabled when "Enhanced=
 Link Detection" is enabled (default). It further explains that "Clause 40.=
4.6.1 of Standard 802.3 requires a PHY that is operating in 1000BASE-T mode=
 to retrain if it detects that its receiver status is not okay." So technic=
ally, you would also need to reverse this default as well if IEEE complianc=
e was the goal - and perhaps there are even others.=20

The motivating reason for this change is a customer is having broken link i=
ssues, and as the ADI datasheet suggests "Having enhanced link detection en=
abled is not suitable for all applications because it causes the PHY to rea=
ct quickly to high levels of disturbance on the MDI lines. This configurati=
on needs to be considered when performing conformance testing and EMC testi=
ng where the media-dependent interface can be exposed to fast transients. T=
hese transients may trigger enhanced link detection to bring the link down =
during such tests. In this case, it is preferred to configure all bits in t=
he FLD_EN register to 0."

Moreover, defaulting it to opposite the HW default might be confusing for a=
 user inspecting the datasheet. If we provide a parameter though, we allow =
for a reasonable way to override it if the feature causes a problem for the=
 user.

> 2) Rather than a vendor specific DT bool to disable it, should we add a g=
eneric
> DT property listing the actual delay in milliseconds, which basically doe=
s what
> the PHY tunable does.
>=20
> I think the answer to the second question should be Yes. It is a bit more
> effort for this change, but is a generic solution.
>=20
How would the new structure look in your mind? I can foresee two obvious im=
plementations:

1. Each PHY driver that wants to implement this feature would add a device_=
property_read_u32() call to read some DT param like "fast-down-threshold-ms=
" and then set associated registers.

2. The dt portion of #1 is done at a higher level which then calls down to =
the phy set_tunable with ETHTOOL_PHY_FAST_LINK_DOWN (not sure if that's pro=
per or not). In the ADIN case, this has the added benefit of providing the =
ability for ethtool to set this.

I also see further complication with the ADIN PHY though. This PHY doesn't =
support a threshold value, but rather this feature is full on or full off. =
While it is not hard to just compare if set >=3D 750 mS and then turn featu=
re off, as the datasheet says "more than either 350 ms or 750 ms in 1000BAS=
E-T, depending if the PHY is 1000BASE-T master or 1000BASE-T slave." Also, =
I am not sure what the standard says about 100BASE-TX and if you see in my =
changes there are separate bit sets for each - hence the two properties.

> I was pondering the first question while reviewing and decided to say
> nothing. There is a danger of regressions. But as this case shows, it can=
 also
> cause problems.
>=20
> 	  Andrew

Perhaps I am overcomplicating this, but I am open to suggestions and will a=
wait your feedback.

Thanks!

Sincerely,
Ken Sloat