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

[Ken Sloat]

Hi Nuno,

> -----Original Message-----
> From: Nuno Sá <noname.nuno@xxxxxxxxx>
> Sent: Tuesday, February 28, 2023 10:10 AM
> To: Ken Sloat <ken.s@xxxxxxxxxxxxx>
> Cc: Michael Hennerich <michael.hennerich@xxxxxxxxxx>; Andrew Lunn
> <andrew@xxxxxxx>; Heiner Kallweit <hkallweit1@xxxxxxxxx>; Russell King
> <linux@xxxxxxxxxxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>; Jakub
> Kicinski <kuba@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxx>
> > ---
> >  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