[PATCH v1] net: phy: adin: Add flags to disable enhanced link detection

[PATCH v1] net: phy: adin: Add flags to disable enhanced link detection

[Ken Sloat]

Enhanced link detection is an ADI PHY feature that allows for earlier
detection of link down if certain signal conditions are met. This
feature is for the most part enabled by default on the PHY. This is
not suitable for all applications and breaks the IEEE standard as
explained in the ADI datasheet.

To fix this, add override flags to disable enhanced link detection
for 1000BASE-T and 100BASE-TX respectively by clearing any related
feature enable bits.

This new feature was tested on an ADIN1300 but according to the
datasheet applies equally for 100BASE-TX on the ADIN1200.

Signed-off-by: Ken Sloat <ken.s@variscite.com>
---
 drivers/net/phy/adin.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index da65215d19bb..8809f3e036a4 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -69,6 +69,15 @@
 #define ADIN1300_EEE_CAP_REG			0x8000
 #define ADIN1300_EEE_ADV_REG			0x8001
 #define ADIN1300_EEE_LPABLE_REG			0x8002
+#define ADIN1300_FLD_EN_REG				0x8E27
+#define   ADIN1300_FLD_PCS_ERR_100_EN			BIT(7)
+#define   ADIN1300_FLD_PCS_ERR_1000_EN			BIT(6)
+#define   ADIN1300_FLD_SLCR_OUT_STUCK_100_EN	BIT(5)
+#define   ADIN1300_FLD_SLCR_OUT_STUCK_1000_EN	BIT(4)
+#define   ADIN1300_FLD_SLCR_IN_ZDET_100_EN		BIT(3)
+#define   ADIN1300_FLD_SLCR_IN_ZDET_1000_EN		BIT(2)
+#define   ADIN1300_FLD_SLCR_IN_INVLD_100_EN		BIT(1)
+#define   ADIN1300_FLD_SLCR_IN_INVLD_1000_EN	BIT(0)
 #define ADIN1300_CLOCK_STOP_REG			0x9400
 #define ADIN1300_LPI_WAKE_ERR_CNT_REG		0xa000
 
@@ -508,6 +517,31 @@ static int adin_config_clk_out(struct phy_device *phydev)
 			      ADIN1300_GE_CLK_CFG_MASK, sel);
 }
 
+static int adin_config_fld_en(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	int reg;
+
+	reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_FLD_EN_REG);
+	if (reg < 0)
+		return reg;
+
+	if (device_property_read_bool(dev, "adi,disable-fld-1000base-t"))
+		reg &= ~(ADIN1300_FLD_PCS_ERR_1000_EN |
+				 ADIN1300_FLD_SLCR_OUT_STUCK_1000_EN |
+				 ADIN1300_FLD_SLCR_IN_ZDET_1000_EN |
+				 ADIN1300_FLD_SLCR_IN_INVLD_1000_EN);
+
+	if (device_property_read_bool(dev, "adi,disable-fld-100base-tx"))
+		reg &= ~(ADIN1300_FLD_PCS_ERR_100_EN |
+				 ADIN1300_FLD_SLCR_OUT_STUCK_100_EN |
+				 ADIN1300_FLD_SLCR_IN_ZDET_100_EN |
+				 ADIN1300_FLD_SLCR_IN_INVLD_100_EN);
+
+	return phy_write_mmd(phydev, MDIO_MMD_VEND1,
+			     ADIN1300_FLD_EN_REG, reg);
+}
+
 static int adin_config_init(struct phy_device *phydev)
 {
 	int rc;
@@ -534,6 +568,10 @@ static int adin_config_init(struct phy_device *phydev)
 	if (rc < 0)
 		return rc;
 
+	rc = adin_config_fld_en(phydev);
+	if (rc < 0)
+		return rc;
+
 	phydev_dbg(phydev, "PHY is using mode '%s'\n",
 		   phy_modes(phydev->interface));
 
-- 
2.34.1

Re: [PATCH v1] net: phy: adin: Add flags to disable enhanced link detection

[Andrew Lunn]

On Tue, Feb 28, 2023 at 09:40:56AM -0500, Ken Sloat wrote:
> Enhanced link detection is an ADI PHY feature that allows for earlier
> detection of link down if certain signal conditions are met. This
> feature is for the most part enabled by default on the PHY. This is
> not suitable for all applications and breaks the IEEE standard as
> explained in the ADI datasheet.
> 
> To fix this, add override flags to disable enhanced link detection
> for 1000BASE-T and 100BASE-TX respectively by clearing any related
> feature enable bits.
> 
> This new feature was tested on an ADIN1300 but according to the
> datasheet applies equally for 100BASE-TX on the ADIN1200.
> 
> Signed-off-by: Ken Sloat <ken.s@variscite.com>
Hi Ken

> +static int adin_config_fld_en(struct phy_device *phydev)

Could we have a better name please. I guess it means Fast Link Down,
but the commit messages talks about Enhanced link detection. This
function is also not enabling fast link down, but disabling it, so _en
seems wrong.

> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	int reg;
> +
> +	reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_FLD_EN_REG);
> +	if (reg < 0)
> +		return reg;
> +
> +	if (device_property_read_bool(dev, "adi,disable-fld-1000base-t"))

You need to document these two properties in the device tree binding.

Please also take a read of
https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#netdev-faq

    Andrew

RE: [PATCH v1] net: phy: adin: Add flags to disable enhanced link detection

[Ken Sloat]

Hi Andrew,

Thanks for your quick reply!

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, February 28, 2023 9:53 AM
> To: Ken Sloat <ken.s@variscite.com>
> Cc: Michael Hennerich <michael.hennerich@analog.com>; Heiner Kallweit
> <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>; David S.
> Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1] net: phy: adin: Add flags to disable enhanced link
> detection
> 
> On Tue, Feb 28, 2023 at 09:40:56AM -0500, Ken Sloat wrote:
> > Enhanced link detection is an ADI PHY feature that allows for earlier
> > detection of link down if certain signal conditions are met. This
> > feature is for the most part enabled by default on the PHY. This is
> > not suitable for all applications and breaks the IEEE standard as
> > explained in the ADI datasheet.
> >
> > To fix this, add override flags to disable enhanced link detection for
> > 1000BASE-T and 100BASE-TX respectively by clearing any related feature
> > enable bits.
> >
> > This new feature was tested on an ADIN1300 but according to the
> > datasheet applies equally for 100BASE-TX on the ADIN1200.
> >
> > Signed-off-by: Ken Sloat <ken.s@variscite.com>
> Hi Ken
> 
> > +static int adin_config_fld_en(struct phy_device *phydev)
> 
> Could we have a better name please. I guess it means Fast Link Down, but
> the commit messages talks about Enhanced link detection. This function is
> also not enabling fast link down, but disabling it, so _en seems wrong.
> 
"Enhanced Link Detection" is the ADI term, but the associated register for controlling this feature is called "FLD_EN." I considered "ELD" as that makes more sense language wise but it did not match the datasheet and did not want to invent a new term. I was not sure what the F was but perhaps you are right, as the link is brought down as part of this feature when conditions are met. I am guessing then that this FLD is a carryover from some initial name of the feature that was later re-branded.

I am happy to change fld -> eld or something else that might make more sense for users and am open to any suggestions.

> > +{
> > +	struct device *dev = &phydev->mdio.dev;
> > +	int reg;
> > +
> > +	reg = phy_read_mmd(phydev, MDIO_MMD_VEND1,
> ADIN1300_FLD_EN_REG);
> > +	if (reg < 0)
> > +		return reg;
> > +
> > +	if (device_property_read_bool(dev, "adi,disable-fld-1000base-t"))
> 
> You need to document these two properties in the device tree binding.
> 

I already have a separate patch for this. I will send both patches when I re-submit and CC additional parties.

> Please also take a read of
> https://www.kernel.org/doc/html/latest/process/maintainer-
> netdev.html#netdev-faq
> 
>     Andrew

Sincerely,
Ken Sloat

Re: [PATCH v1] net: phy: adin: Add flags to disable enhanced link detection

[Andrew Lunn]

On Tue, Feb 28, 2023 at 03:13:59PM +0000, Ken Sloat wrote:
> Hi Andrew,
> 
> Thanks for your quick reply!
> 
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Tuesday, February 28, 2023 9:53 AM
> > To: Ken Sloat <ken.s@variscite.com>
> > Cc: Michael Hennerich <michael.hennerich@analog.com>; Heiner Kallweit
> > <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>; David S.
> > Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>;
> > netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v1] net: phy: adin: Add flags to disable enhanced link
> > detection
> > 
> > On Tue, Feb 28, 2023 at 09:40:56AM -0500, Ken Sloat wrote:
> > > Enhanced link detection is an ADI PHY feature that allows for earlier
> > > detection of link down if certain signal conditions are met. This
> > > feature is for the most part enabled by default on the PHY. This is
> > > not suitable for all applications and breaks the IEEE standard as
> > > explained in the ADI datasheet.
> > >
> > > To fix this, add override flags to disable enhanced link detection for
> > > 1000BASE-T and 100BASE-TX respectively by clearing any related feature
> > > enable bits.
> > >
> > > This new feature was tested on an ADIN1300 but according to the
> > > datasheet applies equally for 100BASE-TX on the ADIN1200.
> > >
> > > Signed-off-by: Ken Sloat <ken.s@variscite.com>
> > Hi Ken
> > 
> > > +static int adin_config_fld_en(struct phy_device *phydev)
> > 
> > Could we have a better name please. I guess it means Fast Link Down, but
> > the commit messages talks about Enhanced link detection. This function is
> > also not enabling fast link down, but disabling it, so _en seems wrong.
> > 
> "Enhanced Link Detection" is the ADI term, but the associated register for controlling this feature is called "FLD_EN." I considered "ELD" as that makes more sense language wise but it did not match the datasheet and did not want to invent a new term. I was not sure what the F was but perhaps you are right, as the link is brought down as part of this feature when conditions are met. I am guessing then that this FLD is a carryover from some initial name of the feature that was later re-branded.
> 
> I am happy to change fld -> eld or something else that might make more sense for users and am open to any suggestions.

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().

> > You need to document these two properties in the device tree binding.
> > 
> 
> I already have a separate patch for this. I will send both patches
> when I re-submit and CC additional parties.

It is normal to submit them together as a patch set. What generally
happens is that the DT maintainers ACK the documentation patch, and
then it gets merged via the netdev tree.

   Andrew

RE: [PATCH v1] net: phy: adin: Add flags to disable enhanced link detection

[Ken Sloat]

Thanks Andrew!

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, February 28, 2023 10:19 AM
> To: Ken Sloat <ken.s@variscite.com>
> Cc: Michael Hennerich <michael.hennerich@analog.com>; Heiner Kallweit
> <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>; David S.
> Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1] net: phy: adin: Add flags to disable enhanced link
> detection
> 
> On Tue, Feb 28, 2023 at 03:13:59PM +0000, Ken Sloat wrote:
> > Hi Andrew,
> >
> > Thanks for your quick reply!
> >
> > > -----Original Message-----
> > > From: Andrew Lunn <andrew@lunn.ch>
> > > Sent: Tuesday, February 28, 2023 9:53 AM
> > > To: Ken Sloat <ken.s@variscite.com>
> > > Cc: Michael Hennerich <michael.hennerich@analog.com>; Heiner
> > > Kallweit <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>;
> David S.
> > > Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>;
> > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH v1] net: phy: adin: Add flags to disable
> > > enhanced link detection
> > >
> > > On Tue, Feb 28, 2023 at 09:40:56AM -0500, Ken Sloat wrote:
> > > > Enhanced link detection is an ADI PHY feature that allows for
> > > > earlier detection of link down if certain signal conditions are
> > > > met. This feature is for the most part enabled by default on the
> > > > PHY. This is not suitable for all applications and breaks the IEEE
> > > > standard as explained in the ADI datasheet.
> > > >
> > > > To fix this, add override flags to disable enhanced link detection
> > > > for 1000BASE-T and 100BASE-TX respectively by clearing any related
> > > > feature enable bits.
> > > >
> > > > This new feature was tested on an ADIN1300 but according to the
> > > > datasheet applies equally for 100BASE-TX on the ADIN1200.
> > > >
> > > > Signed-off-by: Ken Sloat <ken.s@variscite.com>
> > > Hi Ken
> > >
> > > > +static int adin_config_fld_en(struct phy_device *phydev)
> > >
> > > Could we have a better name please. I guess it means Fast Link Down,
> > > but the commit messages talks about Enhanced link detection. This
> > > function is also not enabling fast link down, but disabling it, so _en seems
> wrong.
> > >
> > "Enhanced Link Detection" is the ADI term, but the associated register for
> controlling this feature is called "FLD_EN." I considered "ELD" as that makes
> more sense language wise but it did not match the datasheet and did not
> want to invent a new term. I was not sure what the F was but perhaps you
> are right, as the link is brought down as part of this feature when conditions
> are met. I am guessing then that this FLD is a carryover from some initial
> name of the feature that was later re-branded.
> >
> > I am happy to change fld -> eld or something else that might make more
> sense for users and am open to any suggestions.
> 
> 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().
> 
I am good with that. I'll probably also make mention in the comment along with that ADI's term for thoroughness.

What about for the bindings, how is something like "adi,disable-fast-down-1000base-t?"

> > > You need to document these two properties in the device tree binding.
> > >
> >
> > I already have a separate patch for this. I will send both patches
> > when I re-submit and CC additional parties.
> 
> It is normal to submit them together as a patch set. What generally happens
> is that the DT maintainers ACK the documentation patch, and then it gets
> merged via the netdev tree.
> 
Good to know thanks!

>    Andrew

Sincerely,
Ken Sloat

Re: [PATCH v1] net: phy: adin: Add flags to disable enhanced link detection

[Andrew Lunn]

> What about for the bindings, how is something like "adi,disable-fast-down-1000base-t?"

Yes, that is good. Plus you have a free text field to describe what it
actually does.

	 Andrew

Re: [PATCH v1] net: phy: adin: Add flags to disable enhanced link detection

[Jakub Kicinski]

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?

Re: [PATCH v1] net: phy: adin: Add flags to disable enhanced link detection

[Andrew Lunn]

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?

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.

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
others PHYs have it too.

Ah, we already have a PHY tunable for it,
ETHTOOL_PHY_FAST_LINK_DOWN. I had forgotten about that. The Marvell
PHY supports its.

So i have two questions i guess:

1) Since it is not compliant with 802.3 by default, do we actually
want it disabled by default? But is that going to cause regressions?
Or there devices actually making use of this feature of this PHY?

2) Rather than a vendor specific DT bool to disable it, should we add
a generic DT property listing the actual delay in milliseconds, which
basically does what the PHY tunable does.

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.

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.

	  Andrew

RE: [PATCH v1] net: phy: adin: Add flags to disable enhanced link detection

[Ken Sloat]

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 link
> detection
> 
> 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?
> 
> 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.
> 
> 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 others PHYs
> have it too.
> 
> Ah, we already have a PHY tunable for it, ETHTOOL_PHY_FAST_LINK_DOWN.
> I had forgotten about that. The Marvell PHY supports its.
> 
> So i have two questions i guess:
> 
> 1) Since it is not compliant with 802.3 by default, do we actually want it
> disabled by default? But is that going to cause regressions?
> Or there devices actually making use of this feature of this PHY?
> 
I would think you have a risk here like you said of regression, perhaps some users are not even aware of this feature, but their system is somehow reliant on it.

I am not an IEEE expert, but by examining the datasheet, we can see that clearing 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 technically, you would also need to reverse this default as well if IEEE compliance was the goal - and perhaps there are even others. 

The motivating reason for this change is a customer is having broken link issues, and as the ADI datasheet suggests "Having enhanced link detection enabled is not suitable for all applications because it causes the PHY to react quickly to high levels of disturbance on the MDI lines. This configuration needs to be considered when performing conformance testing and EMC testing where the media-dependent interface can be exposed to fast transients. These 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 the 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 generic
> DT property listing the actual delay in milliseconds, which basically does what
> the PHY tunable does.
> 
> 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.
> 
How would the new structure look in your mind? I can foresee two obvious implementations:

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 proper 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 >= 750 mS and then turn feature off, as the datasheet says "more than either 350 ms or 750 ms in 1000BASE-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.
> 
> 	  Andrew

Perhaps I am overcomplicating this, but I am open to suggestions and will await your feedback.

Thanks!

Sincerely,
Ken Sloat

Re: [PATCH v1] net: phy: adin: Add flags to disable enhanced link detection

[Nuno Sá]

Hi,

Thanks for the patch! Some comments from my side...

On Tue, 2023-02-28 at 09:40 -0500, Ken Sloat wrote:
> Enhanced link detection is an ADI PHY feature that allows for earlier
> detection of link down if certain signal conditions are met. This
> feature is for the most part enabled by default on the PHY. This is
> not suitable for all applications and breaks the IEEE standard as
> explained in the ADI datasheet.
> 
> To fix this, add override flags to disable enhanced link detection
> for 1000BASE-T and 100BASE-TX respectively by clearing any related
> feature enable bits.
> 
> This new feature was tested on an ADIN1300 but according to the
> datasheet applies equally for 100BASE-TX on the ADIN1200.
> 
> Signed-off-by: Ken Sloat <ken.s@variscite.com>
> ---
>  drivers/net/phy/adin.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index da65215d19bb..8809f3e036a4 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -69,6 +69,15 @@
>  #define ADIN1300_EEE_CAP_REG                   0x8000
>  #define ADIN1300_EEE_ADV_REG                   0x8001
>  #define ADIN1300_EEE_LPABLE_REG                        0x8002
> +#define ADIN1300_FLD_EN_REG                            0x8E27
> +#define   ADIN1300_FLD_PCS_ERR_100_EN                  BIT(7)
> +#define   ADIN1300_FLD_PCS_ERR_1000_EN                 BIT(6)
> +#define   ADIN1300_FLD_SLCR_OUT_STUCK_100_EN   BIT(5)
> +#define   ADIN1300_FLD_SLCR_OUT_STUCK_1000_EN  BIT(4)
> +#define   ADIN1300_FLD_SLCR_IN_ZDET_100_EN             BIT(3)
> +#define   ADIN1300_FLD_SLCR_IN_ZDET_1000_EN            BIT(2)
> +#define   ADIN1300_FLD_SLCR_IN_INVLD_100_EN            BIT(1)
> +#define   ADIN1300_FLD_SLCR_IN_INVLD_1000_EN   BIT(0)
>  #define ADIN1300_CLOCK_STOP_REG                        0x9400
>  #define ADIN1300_LPI_WAKE_ERR_CNT_REG          0xa000
>  
> @@ -508,6 +517,31 @@ static int adin_config_clk_out(struct phy_device
> *phydev)
>                               ADIN1300_GE_CLK_CFG_MASK, sel);
>  }
>  
> +static int adin_config_fld_en(struct phy_device *phydev)
> +{
> +       struct device *dev = &phydev->mdio.dev;
> +       int reg;
> +
> +       reg = phy_read_mmd(phydev, MDIO_MMD_VEND1,
> ADIN1300_FLD_EN_REG);
> +       if (reg < 0)
> +               return reg;
> +
> +       if (device_property_read_bool(dev, "adi,disable-fld-1000base-
> t"))

"adi,disable-fld-1000base-tx"?

> +               reg &= ~(ADIN1300_FLD_PCS_ERR_1000_EN |
> +                                ADIN1300_FLD_SLCR_OUT_STUCK_1000_EN
> |
> +                                ADIN1300_FLD_SLCR_IN_ZDET_1000_EN |
> +                                ADIN1300_FLD_SLCR_IN_INVLD_1000_EN);
> +
> +       if (device_property_read_bool(dev, "adi,disable-fld-100base-
> tx"))
> +               reg &= ~(ADIN1300_FLD_PCS_ERR_100_EN |
> +                                ADIN1300_FLD_SLCR_OUT_STUCK_100_EN |
> +                                ADIN1300_FLD_SLCR_IN_ZDET_100_EN |
> +                                ADIN1300_FLD_SLCR_IN_INVLD_100_EN);
> +
> +       return phy_write_mmd(phydev, MDIO_MMD_VEND1,
> +                            ADIN1300_FLD_EN_REG, reg);

nit: You could use phy_clear_bits_mmd() to simplify the function a bit.


You also need to add these new properties to:

Documentation/devicetree/bindings/net/adi,adin.yaml


- Nuno Sá

RE: [PATCH v1] net: phy: adin: Add flags to disable enhanced link detection

[Ken Sloat]

Hi Nuno,

> -----Original Message-----
> From: Nuno Sá <noname.nuno@gmail.com>
> Sent: Tuesday, February 28, 2023 10:10 AM
> To: Ken Sloat <ken.s@variscite.com>
> Cc: Michael Hennerich <michael.hennerich@analog.com>; Andrew Lunn
> <andrew@lunn.ch>; Heiner Kallweit <hkallweit1@gmail.com>; Russell King
> <linux@armlinux.org.uk>; David S. Miller <davem@davemloft.net>; Jakub
> Kicinski <kuba@kernel.org>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v1] net: phy: adin: Add flags to disable enhanced link
> detection
> 
> Hi,
> 
> Thanks for the patch! Some comments from my side...
> 
> On Tue, 2023-02-28 at 09:40 -0500, Ken Sloat wrote:
> > Enhanced link detection is an ADI PHY feature that allows for earlier
> > detection of link down if certain signal conditions are met. This
> > feature is for the most part enabled by default on the PHY. This is
> > not suitable for all applications and breaks the IEEE standard as
> > explained in the ADI datasheet.
> >
> > To fix this, add override flags to disable enhanced link detection for
> > 1000BASE-T and 100BASE-TX respectively by clearing any related feature
> > enable bits.
> >
> > This new feature was tested on an ADIN1300 but according to the
> > datasheet applies equally for 100BASE-TX on the ADIN1200.
> >
> > Signed-off-by: Ken Sloat <ken.s@variscite.com>
> > ---
> >  drivers/net/phy/adin.c | 38
> ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> >
> > diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c index
> > da65215d19bb..8809f3e036a4 100644
> > --- a/drivers/net/phy/adin.c
> > +++ b/drivers/net/phy/adin.c
> > @@ -69,6 +69,15 @@
> >  #define ADIN1300_EEE_CAP_REG                   0x8000
> >  #define ADIN1300_EEE_ADV_REG                   0x8001
> >  #define ADIN1300_EEE_LPABLE_REG                        0x8002
> > +#define ADIN1300_FLD_EN_REG                            0x8E27 #define
> > +ADIN1300_FLD_PCS_ERR_100_EN                  BIT(7) #define
> > +ADIN1300_FLD_PCS_ERR_1000_EN                 BIT(6) #define
> > +ADIN1300_FLD_SLCR_OUT_STUCK_100_EN   BIT(5) #define
> > +ADIN1300_FLD_SLCR_OUT_STUCK_1000_EN  BIT(4) #define
> > +ADIN1300_FLD_SLCR_IN_ZDET_100_EN             BIT(3) #define
> > +ADIN1300_FLD_SLCR_IN_ZDET_1000_EN            BIT(2) #define
> > +ADIN1300_FLD_SLCR_IN_INVLD_100_EN            BIT(1) #define
> > +ADIN1300_FLD_SLCR_IN_INVLD_1000_EN   BIT(0)
> >  #define ADIN1300_CLOCK_STOP_REG                        0x9400
> >  #define ADIN1300_LPI_WAKE_ERR_CNT_REG          0xa000
> >
> > @@ -508,6 +517,31 @@ static int adin_config_clk_out(struct phy_device
> > *phydev)
> >                               ADIN1300_GE_CLK_CFG_MASK, sel);
> >  }
> >
> > +static int adin_config_fld_en(struct phy_device *phydev) {
> > +       struct device *dev = &phydev->mdio.dev;
> > +       int reg;
> > +
> > +       reg = phy_read_mmd(phydev, MDIO_MMD_VEND1,
> > ADIN1300_FLD_EN_REG);
> > +       if (reg < 0)
> > +               return reg;
> > +
> > +       if (device_property_read_bool(dev, "adi,disable-fld-1000base-
> > t"))
> 
> "adi,disable-fld-1000base-tx"?
> 
No that was purposeful, it's just "T" this PHY supports "1000BASE-T" and "100BASE-TX"

> > +               reg &= ~(ADIN1300_FLD_PCS_ERR_1000_EN |
> > +                                ADIN1300_FLD_SLCR_OUT_STUCK_1000_EN
> > |
> > +                                ADIN1300_FLD_SLCR_IN_ZDET_1000_EN |
> > +                                ADIN1300_FLD_SLCR_IN_INVLD_1000_EN);
> > +
> > +       if (device_property_read_bool(dev, "adi,disable-fld-100base-
> > tx"))
> > +               reg &= ~(ADIN1300_FLD_PCS_ERR_100_EN |
> > +                                ADIN1300_FLD_SLCR_OUT_STUCK_100_EN |
> > +                                ADIN1300_FLD_SLCR_IN_ZDET_100_EN |
> > +                                ADIN1300_FLD_SLCR_IN_INVLD_100_EN);
> > +
> > +       return phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > +                            ADIN1300_FLD_EN_REG, reg);
> 
> nit: You could use phy_clear_bits_mmd() to simplify the function a bit.

Thanks, I will check out that function for v2.

> 
> 
> You also need to add these new properties to:
> 
> Documentation/devicetree/bindings/net/adi,adin.yaml
> 
I will submit the patch I have for this as well on v2.

> 
> - Nuno Sá

Thanks!

Sincerely,
Ken Sloat

[PATCH v2 1/2] net: phy: adin: Add flags to allow disabling of fast link down

[Ken Sloat]

"Enhanced Link Detection" is an ADI PHY feature that allows for earlier
detection of link down if certain signal conditions are met. Also known
on other PHYs as "Fast Link Down," this feature is for the most part
enabled by default on the PHY. This is not suitable for all applications
and breaks the IEEE standard as explained in the ADI datasheet.

To fix this, add override flags to disable fast link down for 1000BASE-T
and 100BASE-TX respectively by clearing any related feature enable bits.

This new feature was tested on an ADIN1300 but according to the
datasheet applies equally for 100BASE-TX on the ADIN1200.

Signed-off-by: Ken Sloat <ken.s@variscite.com>
---
Changes in v2:
 -Change "FLD" nomenclature to commonly used "Fast Link Down" phrase in
    source code and bindings. Also document this in the commit comments.
 -Utilize phy_clear_bits_mmd() in register bit operations.

 drivers/net/phy/adin.c | 43 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index da65215d19bb..0bab7e4d3e29 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -69,6 +69,15 @@
 #define ADIN1300_EEE_CAP_REG			0x8000
 #define ADIN1300_EEE_ADV_REG			0x8001
 #define ADIN1300_EEE_LPABLE_REG			0x8002
+#define ADIN1300_FLD_EN_REG				0x8E27
+#define   ADIN1300_FLD_PCS_ERR_100_EN			BIT(7)
+#define   ADIN1300_FLD_PCS_ERR_1000_EN			BIT(6)
+#define   ADIN1300_FLD_SLCR_OUT_STUCK_100_EN	BIT(5)
+#define   ADIN1300_FLD_SLCR_OUT_STUCK_1000_EN	BIT(4)
+#define   ADIN1300_FLD_SLCR_IN_ZDET_100_EN		BIT(3)
+#define   ADIN1300_FLD_SLCR_IN_ZDET_1000_EN		BIT(2)
+#define   ADIN1300_FLD_SLCR_IN_INVLD_100_EN		BIT(1)
+#define   ADIN1300_FLD_SLCR_IN_INVLD_1000_EN	BIT(0)
 #define ADIN1300_CLOCK_STOP_REG			0x9400
 #define ADIN1300_LPI_WAKE_ERR_CNT_REG		0xa000
 
@@ -508,6 +517,36 @@ static int adin_config_clk_out(struct phy_device *phydev)
 			      ADIN1300_GE_CLK_CFG_MASK, sel);
 }
 
+static int adin_fast_down_disable(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	int rc;
+
+	if (device_property_read_bool(dev, "adi,disable-fast-down-1000base-t")) {
+		rc = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
+					  ADIN1300_FLD_EN_REG,
+					  ADIN1300_FLD_PCS_ERR_1000_EN |
+					  ADIN1300_FLD_SLCR_OUT_STUCK_1000_EN |
+					  ADIN1300_FLD_SLCR_IN_ZDET_1000_EN |
+					  ADIN1300_FLD_SLCR_IN_INVLD_1000_EN);
+		if (rc < 0)
+			return rc;
+	}
+
+	if (device_property_read_bool(dev, "adi,disable-fast-down-100base-tx")) {
+		phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
+					  ADIN1300_FLD_EN_REG,
+					  ADIN1300_FLD_PCS_ERR_100_EN |
+					  ADIN1300_FLD_SLCR_OUT_STUCK_100_EN |
+					  ADIN1300_FLD_SLCR_IN_ZDET_100_EN |
+					  ADIN1300_FLD_SLCR_IN_INVLD_100_EN);
+		if (rc < 0)
+			return rc;
+	}
+
+	return 0;
+}
+
 static int adin_config_init(struct phy_device *phydev)
 {
 	int rc;
@@ -534,6 +573,10 @@ static int adin_config_init(struct phy_device *phydev)
 	if (rc < 0)
 		return rc;
 
+	rc = adin_fast_down_disable(phydev);
+	if (rc < 0)
+		return rc;
+
 	phydev_dbg(phydev, "PHY is using mode '%s'\n",
 		   phy_modes(phydev->interface));
 
-- 
2.34.1

[PATCH v2 2/2] dt-bindings: net: adin: Document bindings for fast link down disable

[Ken Sloat]

The ADI PHY contains a feature commonly known as "Fast Link Down" and
called "Enhanced Link Detection" by ADI. This feature is enabled by
default and provides earlier detection of link loss in certain
situations.

Document the new optional flags "adi,disable-fast-down-1000base-t" and
"adi,disable-fast-down-100base-tx" which disable the "Fast Link Down"
feature in the ADI PHY.

Signed-off-by: Ken Sloat <ken.s@variscite.com>
---
 Documentation/devicetree/bindings/net/adi,adin.yaml | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
index 64ec1ec71ccd..923baff26c3e 100644
--- a/Documentation/devicetree/bindings/net/adi,adin.yaml
+++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
@@ -52,6 +52,18 @@ properties:
     description: Enable 25MHz reference clock output on CLK25_REF pin.
     type: boolean
 
+  adi,disable-fast-down-1000base-t:
+    $ref: /schemas/types.yaml#definitions/flag
+    description: |
+      If set, disables any ADI fast link down ("Enhanced Link Detection")
+      function bits for 1000base-t interfaces.
+
+  adi,disable-fast-down-100base-tx:
+    $ref: /schemas/types.yaml#definitions/flag
+    description: |
+      If set, disables any ADI fast link down ("Enhanced Link Detection")
+      function bits for 100base-tx interfaces.
+
 unevaluatedProperties: false
 
   adi,phy-mode-override:
-- 
2.34.1

Re: [PATCH v2 2/2] dt-bindings: net: adin: Document bindings for fast link down disable

[Krzysztof Kozlowski]

On 28/02/2023 19:49, Ken Sloat wrote:
> The ADI PHY contains a feature commonly known as "Fast Link Down" and
> called "Enhanced Link Detection" by ADI. This feature is enabled by
> default and provides earlier detection of link loss in certain
> situations.
> 

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

> Document the new optional flags "adi,disable-fast-down-1000base-t" and
> "adi,disable-fast-down-100base-tx" which disable the "Fast Link Down"
> feature in the ADI PHY.

You did not explain why do you need it.

> 
> Signed-off-by: Ken Sloat <ken.s@variscite.com>
> ---

Don't attach your new patchsets to your old threads. It buries them deep
and make usage of our tools difficult.


>  Documentation/devicetree/bindings/net/adi,adin.yaml | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
> index 64ec1ec71ccd..923baff26c3e 100644
> --- a/Documentation/devicetree/bindings/net/adi,adin.yaml
> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
> @@ -52,6 +52,18 @@ properties:
>      description: Enable 25MHz reference clock output on CLK25_REF pin.
>      type: boolean
>  
> +  adi,disable-fast-down-1000base-t:
> +    $ref: /schemas/types.yaml#definitions/flag
> +    description: |
> +      If set, disables any ADI fast link down ("Enhanced Link Detection")
> +      function bits for 1000base-t interfaces.

And why disabling it per board should be a property of DT?

Best regards,
Krzysztof

RE: [PATCH v2 2/2] dt-bindings: net: adin: Document bindings for fast link down disable

[Ken Sloat]

Hi Krzysztof,

Thanks for your reply and sorry for the late response.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Thursday, March 2, 2023 2:00 AM
> To: Ken Sloat <ken.s@variscite.com>
> Cc: noname.nuno@gmail.com; pabeni@redhat.com;
> edumazet@google.com; Michael Hennerich
> <michael.hennerich@analog.com>; David S. Miller <davem@davemloft.net>;
> Jakub Kicinski <kuba@kernel.org>; Rob Herring <robh+dt@kernel.org>;
> Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit <hkallweit1@gmail.com>;
> Russell King <linux@armlinux.org.uk>; Alexandru Tachici
> <alexandru.tachici@analog.com>; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 2/2] dt-bindings: net: adin: Document bindings for
> fast link down disable
> 
> On 28/02/2023 19:49, Ken Sloat wrote:
> > The ADI PHY contains a feature commonly known as "Fast Link Down" and
> > called "Enhanced Link Detection" by ADI. This feature is enabled by
> > default and provides earlier detection of link loss in certain
> > situations.
> >
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people and
> lists to CC.  It might happen, that command when run on an older kernel,
> gives you outdated entries.  Therefore please be sure you base your patches
> on recent Linux kernel.
> 

Understood

> > Document the new optional flags "adi,disable-fast-down-1000base-t" and
> > "adi,disable-fast-down-100base-tx" which disable the "Fast Link Down"
> > feature in the ADI PHY.
> 
> You did not explain why do you need it.

My thoughts were this was explained in the feature patch and so was redundant here which is why I gave a brief summary, but if the norm is to duplicate this information I can certainly do that.

> 
> >
> > Signed-off-by: Ken Sloat <ken.s@variscite.com>
> > ---
> 
> Don't attach your new patchsets to your old threads. It buries them deep and
> make usage of our tools difficult.
> 
I added the in-reply-to id in git send-email as I thought this was the norm but I will not do this in the future, sorry.

> 
> >  Documentation/devicetree/bindings/net/adi,adin.yaml | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml
> > b/Documentation/devicetree/bindings/net/adi,adin.yaml
> > index 64ec1ec71ccd..923baff26c3e 100644
> > --- a/Documentation/devicetree/bindings/net/adi,adin.yaml
> > +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
> > @@ -52,6 +52,18 @@ properties:
> >      description: Enable 25MHz reference clock output on CLK25_REF pin.
> >      type: boolean
> >
> > +  adi,disable-fast-down-1000base-t:
> > +    $ref: /schemas/types.yaml#definitions/flag
> > +    description: |
> > +      If set, disables any ADI fast link down ("Enhanced Link Detection")
> > +      function bits for 1000base-t interfaces.
> 
> And why disabling it per board should be a property of DT?
> 
That seemed like a logical place to allow override on boards where it is undesired. Would you say that properties such as this should instead be custom PHY tunables, which may require patching of ethtool as well?

> Best regards,
> Krzysztof

Sincerely,
Ken Sloat

Re: [PATCH v2 2/2] dt-bindings: net: adin: Document bindings for fast link down disable

[Krzysztof Kozlowski]

On 07/03/2023 19:19, Ken Sloat wrote:
>>> Document the new optional flags "adi,disable-fast-down-1000base-t" and
>>> "adi,disable-fast-down-100base-tx" which disable the "Fast Link Down"
>>> feature in the ADI PHY.
>>
>> You did not explain why do you need it.
> 
> My thoughts were this was explained in the feature patch and so was redundant here which is why I gave a brief summary, but if the norm is to duplicate this information I can certainly do that.

At least something should be here. Bindings are separate from Linux, so
the commit should stand on its own.

>>>      description: Enable 25MHz reference clock output on CLK25_REF pin.
>>>      type: boolean
>>>
>>> +  adi,disable-fast-down-1000base-t:
>>> +    $ref: /schemas/types.yaml#definitions/flag
>>> +    description: |
>>> +      If set, disables any ADI fast link down ("Enhanced Link Detection")
>>> +      function bits for 1000base-t interfaces.
>>
>> And why disabling it per board should be a property of DT?
>>
> That seemed like a logical place to allow override on boards where it is undesired. Would you say that properties such as this should instead be custom PHY tunables, which may require patching of ethtool as well?

First, your other commit says that it breaks IEEE standard, so maybe it
should be always disabled?

Second, tunable per board, but why? DT describes the hardware, so just
because someone wants to tune something is not a reason to put it in DT.
The reason to put it in DT is - this boards requires or cannot work with
the feature because of some board characteristic.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] net: phy: adin: Add flags to allow disabling of fast link down

[Nuno Sá]

On Tue, 2023-02-28 at 13:49 -0500, Ken Sloat wrote:
> "Enhanced Link Detection" is an ADI PHY feature that allows for
> earlier
> detection of link down if certain signal conditions are met. Also
> known
> on other PHYs as "Fast Link Down," this feature is for the most part
> enabled by default on the PHY. This is not suitable for all
> applications
> and breaks the IEEE standard as explained in the ADI datasheet.
> 
> To fix this, add override flags to disable fast link down for
> 1000BASE-T
> and 100BASE-TX respectively by clearing any related feature enable
> bits.
> 
> This new feature was tested on an ADIN1300 but according to the
> datasheet applies equally for 100BASE-TX on the ADIN1200.
> 
> Signed-off-by: Ken Sloat <ken.s@variscite.com>
> ---
> Changes in v2:
>  -Change "FLD" nomenclature to commonly used "Fast Link Down" phrase
> in
>     source code and bindings. Also document this in the commit
> comments.
>  -Utilize phy_clear_bits_mmd() in register bit operations.
> 
>  drivers/net/phy/adin.c | 43
> ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index da65215d19bb..0bab7e4d3e29 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -69,6 +69,15 @@
>  #define ADIN1300_EEE_CAP_REG                   0x8000
>  #define ADIN1300_EEE_ADV_REG                   0x8001
>  #define ADIN1300_EEE_LPABLE_REG                        0x8002
> +#define ADIN1300_FLD_EN_REG                            0x8E27
> +#define   ADIN1300_FLD_PCS_ERR_100_EN                  BIT(7)
> +#define   ADIN1300_FLD_PCS_ERR_1000_EN                 BIT(6)
> +#define   ADIN1300_FLD_SLCR_OUT_STUCK_100_EN   BIT(5)
> +#define   ADIN1300_FLD_SLCR_OUT_STUCK_1000_EN  BIT(4)
> +#define   ADIN1300_FLD_SLCR_IN_ZDET_100_EN             BIT(3)
> +#define   ADIN1300_FLD_SLCR_IN_ZDET_1000_EN            BIT(2)
> +#define   ADIN1300_FLD_SLCR_IN_INVLD_100_EN            BIT(1)
> +#define   ADIN1300_FLD_SLCR_IN_INVLD_1000_EN   BIT(0)
>  #define ADIN1300_CLOCK_STOP_REG                        0x9400
>  #define ADIN1300_LPI_WAKE_ERR_CNT_REG          0xa000
>  
> @@ -508,6 +517,36 @@ static int adin_config_clk_out(struct phy_device
> *phydev)
>                               ADIN1300_GE_CLK_CFG_MASK, sel);
>  }
>  
> +static int adin_fast_down_disable(struct phy_device *phydev)
> +{
> +       struct device *dev = &phydev->mdio.dev;
> +       int rc;
> +
> +       if (device_property_read_bool(dev, "adi,disable-fast-down-
> 1000base-t")) {
> +               rc = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
> +                                         ADIN1300_FLD_EN_REG,
> +                                        
> ADIN1300_FLD_PCS_ERR_1000_EN |
> +                                        
> ADIN1300_FLD_SLCR_OUT_STUCK_1000_EN |
> +                                        
> ADIN1300_FLD_SLCR_IN_ZDET_1000_EN |
> +                                        
> ADIN1300_FLD_SLCR_IN_INVLD_1000_EN);
> +               if (rc < 0)
> +                       return rc;
> +       }
> +
> +       if (device_property_read_bool(dev, "adi,disable-fast-down-
> 100base-tx")) {
> +               phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
> +                                         ADIN1300_FLD_EN_REG,
> +                                         ADIN1300_FLD_PCS_ERR_100_EN
> |
> +                                        
> ADIN1300_FLD_SLCR_OUT_STUCK_100_EN |
> +                                        
> ADIN1300_FLD_SLCR_IN_ZDET_100_EN |
> +                                        
> ADIN1300_FLD_SLCR_IN_INVLD_100_EN);
> +               if (rc < 0)
> +                       return rc;
> +       }

This is not exactly what I had in mind... I was suggesting something
like caching the complete "bits word" in both of your if() statements
and then just calling phy_clear_bits_mmd() once. If I'm not missing
something obvious, something like this:

u16 bits = 0; //or any other name more appropriate

if (device_property_read_bool(...))
	bits = ADIN1300_FLD_PCS_ERR_1000_EN | ...

if (device_property_read_bool())
	bits |= ...

if (!bits)
	return 0;

return phy_clear_bits_mmd();

Does this sound sane to you?

Anyways, this is also not a big deal...

- Nuno Sá

RE: [PATCH v2 1/2] net: phy: adin: Add flags to allow disabling of fast link down

[Ken Sloat]

Hi Nuno,

> -----Original Message-----
> From: Nuno Sá <noname.nuno@gmail.com>
> Sent: Wednesday, March 1, 2023 2:34 AM
> To: Ken Sloat <ken.s@variscite.com>
> Cc: pabeni@redhat.com; edumazet@google.com; Michael Hennerich
> <michael.hennerich@analog.com>; David S. Miller
> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Rob Herring
> <robh+dt@kernel.org>; Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit
> <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>; Alexandru
> Tachici <alexandru.tachici@analog.com>; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 1/2] net: phy: adin: Add flags to allow disabling of fast
> link down
> 
> On Tue, 2023-02-28 at 13:49 -0500, Ken Sloat wrote:
> > "Enhanced Link Detection" is an ADI PHY feature that allows for
> > earlier detection of link down if certain signal conditions are met.
> > Also known on other PHYs as "Fast Link Down," this feature is for the
> > most part enabled by default on the PHY. This is not suitable for all
> > applications and breaks the IEEE standard as explained in the ADI
> > datasheet.
> >
> > To fix this, add override flags to disable fast link down for
> > 1000BASE-T and 100BASE-TX respectively by clearing any related feature
> > enable bits.
> >
> > This new feature was tested on an ADIN1300 but according to the
> > datasheet applies equally for 100BASE-TX on the ADIN1200.
> >
> > Signed-off-by: Ken Sloat <ken.s@variscite.com>
> > ---
> > Changes in v2:
> >  -Change "FLD" nomenclature to commonly used "Fast Link Down" phrase
> > in
> >     source code and bindings. Also document this in the commit
> > comments.
> >  -Utilize phy_clear_bits_mmd() in register bit operations.
> >
> >  drivers/net/phy/adin.c | 43
> > ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> >
> > diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c index
> > da65215d19bb..0bab7e4d3e29 100644
> > --- a/drivers/net/phy/adin.c
> > +++ b/drivers/net/phy/adin.c
> > @@ -69,6 +69,15 @@
> >  #define ADIN1300_EEE_CAP_REG                   0x8000
> >  #define ADIN1300_EEE_ADV_REG                   0x8001
> >  #define ADIN1300_EEE_LPABLE_REG                        0x8002
> > +#define ADIN1300_FLD_EN_REG                            0x8E27 #define
> > +ADIN1300_FLD_PCS_ERR_100_EN                  BIT(7) #define
> > +ADIN1300_FLD_PCS_ERR_1000_EN                 BIT(6) #define
> > +ADIN1300_FLD_SLCR_OUT_STUCK_100_EN   BIT(5) #define
> > +ADIN1300_FLD_SLCR_OUT_STUCK_1000_EN  BIT(4) #define
> > +ADIN1300_FLD_SLCR_IN_ZDET_100_EN             BIT(3) #define
> > +ADIN1300_FLD_SLCR_IN_ZDET_1000_EN            BIT(2) #define
> > +ADIN1300_FLD_SLCR_IN_INVLD_100_EN            BIT(1) #define
> > +ADIN1300_FLD_SLCR_IN_INVLD_1000_EN   BIT(0)
> >  #define ADIN1300_CLOCK_STOP_REG                        0x9400
> >  #define ADIN1300_LPI_WAKE_ERR_CNT_REG          0xa000
> >
> > @@ -508,6 +517,36 @@ static int adin_config_clk_out(struct phy_device
> > *phydev)
> >                               ADIN1300_GE_CLK_CFG_MASK, sel);
> >  }
> >
> > +static int adin_fast_down_disable(struct phy_device *phydev) {
> > +       struct device *dev = &phydev->mdio.dev;
> > +       int rc;
> > +
> > +       if (device_property_read_bool(dev, "adi,disable-fast-down-
> > 1000base-t")) {
> > +               rc = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
> > +                                         ADIN1300_FLD_EN_REG,
> > +
> > ADIN1300_FLD_PCS_ERR_1000_EN |
> > +
> > ADIN1300_FLD_SLCR_OUT_STUCK_1000_EN |
> > +
> > ADIN1300_FLD_SLCR_IN_ZDET_1000_EN |
> > +
> > ADIN1300_FLD_SLCR_IN_INVLD_1000_EN);
> > +               if (rc < 0)
> > +                       return rc;
> > +       }
> > +
> > +       if (device_property_read_bool(dev, "adi,disable-fast-down-
> > 100base-tx")) {
> > +               phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
> > +                                         ADIN1300_FLD_EN_REG,
> > +                                         ADIN1300_FLD_PCS_ERR_100_EN
> > |
> > +
> > ADIN1300_FLD_SLCR_OUT_STUCK_100_EN |
> > +
> > ADIN1300_FLD_SLCR_IN_ZDET_100_EN |
> > +
> > ADIN1300_FLD_SLCR_IN_INVLD_100_EN);
> > +               if (rc < 0)
> > +                       return rc;
> > +       }
> 
> This is not exactly what I had in mind... I was suggesting something like
> caching the complete "bits word" in both of your if() statements and then
> just calling phy_clear_bits_mmd() once. If I'm not missing something
> obvious, something like this:
> 
> u16 bits = 0; //or any other name more appropriate
> 
> if (device_property_read_bool(...))
> 	bits = ADIN1300_FLD_PCS_ERR_1000_EN | ...
> 
> if (device_property_read_bool())
> 	bits |= ...
> 
> if (!bits)
> 	return 0;
> 
> return phy_clear_bits_mmd();
> 
> Does this sound sane to you?
> 
> Anyways, this is also not a big deal...
Yes, I agree that would be cleaner. I will adjust.

> 
> - Nuno Sá

Thanks

Sincerely,
Ken Sloat