Re: [PATCH net v1 3/3] net: phy: mscc: coma mode disabled for VSC8514

[Bjarni.Jonasson]

On Fri, 2021-02-12 at 16:32 +0000, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Fri, Feb 12, 2021 at 03:06:43PM +0100, 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.")
> > ---
> >  drivers/net/phy/mscc/mscc_main.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/net/phy/mscc/mscc_main.c
> > b/drivers/net/phy/mscc/mscc_main.c
> > index 7546d9cc3abd..0600b592618b 100644
> > --- a/drivers/net/phy/mscc/mscc_main.c
> > +++ b/drivers/net/phy/mscc/mscc_main.c
> > @@ -1418,6 +1418,18 @@ 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_write(phydev, MSCC_EXT_PAGE_ACCESS,
> > MSCC_PHY_PAGE_EXTENDED_GPIO);
> > +     __phy_write(phydev, MSCC_PHY_GPIO_CONTROL_2, 0x0600);
> > +     __phy_write(phydev, MSCC_EXT_PAGE_ACCESS,
> > MSCC_PHY_PAGE_STANDARD);
> 
> Can you please do:
>         phy_write_paged(phydev, MSCC_PHY_PAGE_EXTENDED_GPIO,
>                         MSCC_PHY_GPIO_CONTROL_2, 0x0600);
> 
> And can you please provide some definitions for what 0x0600 is?
> My reference manual says that:
> 
> Bit 13:
> COMA_MODE output enable (active low)
> Bit 12:
> COMA_MODE output data
> Bit 11:
> COMA_MODE input data
> Bit 10:
> Reserved
> Bit 9:
> Tri-state enable for LEDs
> 
> 0x600 is BIT(10) | BIT(9). But BIT(10) is reserved. Sure this is
> correct?

I can see this is unclear.  The code is actualy writing zero to bit 12
and 13.  Bit 9 and 10 are not interesting in this context.  I will
change it to use phy_modify_paged() bit 12 and 13.

> 
> > +}
> > +
> >  static int vsc8584_config_init(struct phy_device *phydev)
> >  {
> >       struct vsc8531_private *vsc8531 = phydev->priv;
> > @@ -2610,6 +2622,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);
> > --
> > 2.17.1
> >