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@xxxxxxxxxxxxx>
> ---
> 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á
