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

From: Bjarni.Jonasson
Date: Mon Feb 15 2021 - 04:37:56 EST


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
> >