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

["Nuno Sá" <noname.nuno@gmail.com>]

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á