On Mon, Feb 01, 2021 at 11:56:01PM +0100, Andrew Lunn wrote:
> > +static int mv2222_config_init(struct phy_device *phydev)
> > +{
> > + linkmode_zero(phydev->supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> > phydev->supported);
> > + linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseKR_Full_BIT,
> > phydev->supported);
> > +
> > + phydev->pause = 0;
> > + phydev->asym_pause = 0;
> > + phydev->duplex = DUPLEX_FULL;
> > + phydev->autoneg = AUTONEG_DISABLE;
> > +
> > + return 0;
> > +}
> > +
> > +static void mv2222_update_interface(struct phy_device *phydev)
> > +{
> > + if ((phydev->speed == SPEED_1000 ||
> > + phydev->speed == SPEED_100 ||
> > + phydev->speed == SPEED_10) &&
> > + phydev->interface != PHY_INTERFACE_MODE_1000BASEX) {
> > + phydev->interface = PHY_INTERFACE_MODE_1000BASEX;
>
> The speeds 10 and 100 seem odd here. 1000BaseX only supports 1G. It
> would have to be SGMII in order to also support 10Mbps and 100Mbps.
> Plus you are not listing 10 and 100 as a supported value.
>
> > +/* Returns negative on error, 0 if link is down, 1 if link is up */
> > +static int mv2222_read_status_1g(struct phy_device *phydev)
> > +{
> > + int val, link = 0;
> > +
> > + val = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_1GBX_STAT);
> > + if (val < 0)
> > + return val;
> > +
> > + if (!(val & MDIO_STAT1_LSTATUS) ||
> > + (phydev->autoneg == AUTONEG_ENABLE && !(val &
> > MDIO_AN_STAT1_COMPLETE)))
> > + return 0;
> > +
> > + link = 1;
> > +
> > + if (phydev->autoneg == AUTONEG_DISABLE) {
> > + phydev->speed = SPEED_1000;
> > + phydev->duplex = DUPLEX_FULL;
> > +
> > + return link;
> > + }
> > +
> > + val = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_1GBX_PHY_STAT);
> > + if (val < 0)
> > + return val;
> > +
> > + if (val & MV_1GBX_PHY_STAT_AN_RESOLVED) {
> > + if (val & MV_1GBX_PHY_STAT_DUPLEX)
> > + phydev->duplex = DUPLEX_FULL;
> > + else
> > + phydev->duplex = DUPLEX_HALF;
> > +
> > + if (val & MV_1GBX_PHY_STAT_SPEED1000)
> > + phydev->speed = SPEED_1000;
> > + else if (val & MV_1GBX_PHY_STAT_SPEED100)
> > + phydev->speed = SPEED_100;
> > + else
> > + phydev->speed = SPEED_10;
>
> Are you sure it is not doing SGMII? Maybe it can do both, 1000BaseX
> and SGMII? You would generally use 1000BaseX to connect to a fibre
> SFP, and SGMII to connect to a copper SFP. So ideally you want to be
> able to swap between these modes as needed.
>
> > +static int mv2222_read_status(struct phy_device *phydev)
> > +{
> > + int link;
> > +
> > + linkmode_zero(phydev->lp_advertising);
> > + phydev->pause = 0;
> > + phydev->asym_pause = 0;
> > +
> > + switch (phydev->interface) {
> > + case PHY_INTERFACE_MODE_10GBASER:
> > + link = mv2222_read_status_10g(phydev);
> > + break;
> > + case PHY_INTERFACE_MODE_1000BASEX:
> > + default:
> > + link = mv2222_read_status_1g(phydev);
> > + break;
> > + }
> > +
> > + if (link < 0)
> > + return link;
> > +
> > + phydev->link = link;
> > +
> > + if (phydev->link)
> > + mv2222_link_led_on(phydev);
> > + else
> > + mv2222_link_led_off(phydev);
>
> You have to manually control the LED? That is odd for a PHY. Normally
> you just select a mode and it will do it all in hardware.
>
> Andrew
Thanks for the review, Andrew, your remarks are all valid. I'll
implement 1000Base-X/SGMII swapping. As for the LED, I'll just drop it
for now, as it is not essential. I'll commit LEDs config when core
funtionality will be accepted.
