Re: [PATCH net-next v2 3/3] net: phy: mscc: coma mode disabled for VSC8514

[Bjarni.Jonasson]

On Mon, 2021-02-15 at 21:08 +0100, Heiner Kallweit wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On 15.02.2021 17:58, Bjarni Jonasson wrote:
> > The 'coma mode' (configurable through sw or hw) provides an
> > optional feature that may be used to control when the PHYs become
> > active.
> > The typical usage is to synchronize the link-up time across
> > all PHY instances. This patch releases coma mode if not done by
> > hardware,
> > otherwise the phys will not link-up
> > 
> > Signed-off-by: Steen Hegelund <steen.hegelund@xxxxxxxxxxxxx>
> > Signed-off-by: Bjarni Jonasson <bjarni.jonasson@xxxxxxxxxxxxx>
> > Fixes: e4f9ba642f0b ("net: phy: mscc: add support for VSC8514
> > PHY.")
> > ---
> > v1 -> v2:
> >   Modified coma mode config
> >   Changed net to net-next
> > 
> >  drivers/net/phy/mscc/mscc.h      |  3 +++
> >  drivers/net/phy/mscc/mscc_main.c | 16 ++++++++++++++++
> >  2 files changed, 19 insertions(+)
> > 
> > diff --git a/drivers/net/phy/mscc/mscc.h
> > b/drivers/net/phy/mscc/mscc.h
> > index 9d8ee387739e..2b70ccd1b256 100644
> > --- a/drivers/net/phy/mscc/mscc.h
> > +++ b/drivers/net/phy/mscc/mscc.h
> > @@ -160,6 +160,9 @@ enum rgmii_clock_delay {
> >  #define MSCC_PHY_PAGE_TR               0x52b5 /* Token ring
> > registers */
> >  #define MSCC_PHY_GPIO_CONTROL_2        14
> > 
> > +#define MSCC_PHY_COMA_MODE             0x2000 /* input(1) /
> > output(0) */
> > +#define MSCC_PHY_COMA_OUTPUT           0x1000 /* value to output
> > */
> > +
> >  /* Extended Page 1 Registers */
> >  #define MSCC_PHY_CU_MEDIA_CRC_VALID_CNT        18
> >  #define VALID_CRC_CNT_CRC_MASK                 GENMASK(13, 0)
> > diff --git a/drivers/net/phy/mscc/mscc_main.c
> > b/drivers/net/phy/mscc/mscc_main.c
> > index 03181542bcb7..29302ccf7e7b 100644
> > --- a/drivers/net/phy/mscc/mscc_main.c
> > +++ b/drivers/net/phy/mscc/mscc_main.c
> > @@ -1520,6 +1520,21 @@ static void vsc8584_get_base_addr(struct
> > phy_device *phydev)
> >       vsc8531->addr = addr;
> >  }
> > 
> > +static void vsc85xx_coma_mode_release(struct phy_device *phydev)
> > +{
> > +     /* The coma mode (pin or reg) provides an optional feature
> > that
> > +      * may be used to control when the PHYs become active.
> > +      * Alternatively the COMA_MODE pin may be connected low
> > +      * so that the PHYs are fully active once out of reset.
> > +      */
> > +     phy_unlock_mdio_bus(phydev);
> > +     /* Enable output (mode=0) and write zero to it */
> > +     phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO,
> > +                      MSCC_PHY_GPIO_CONTROL_2,
> > +                      MSCC_PHY_COMA_MODE | MSCC_PHY_COMA_OUTPUT,
> > 0);
> > +     phy_lock_mdio_bus(phydev);
> 
> The temporary unlock is a little bit hacky. Better do:
> vsc85xx_phy_write_page(MSCC_PHY_PAGE_EXTENDED_GPIO)
> __phy_modify()
> vsc85xx_phy_write_page(default page)
> 
> Alternatively we could add __phy_modify_paged(). But this may not
> be worth the effort for now.

I will follow your suggestion.
Thx
--
Bjarni Jonasson
Microchip

> 
> > +}
> > +
> >  static int vsc8584_config_init(struct phy_device *phydev)
> >  {
> >       struct vsc8531_private *vsc8531 = phydev->priv;
> > @@ -2604,6 +2619,7 @@ static int vsc8514_config_init(struct
> > phy_device *phydev)
> >               ret = vsc8514_config_host_serdes(phydev);
> >               if (ret)
> >                       goto err;
> > +             vsc85xx_coma_mode_release(phydev);
> >       }
> > 
> >       phy_unlock_mdio_bus(phydev);
> > 
> 
>