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

[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