(+) LinuxArm
> From: Michal Vokáč [mailto:michal.vo...@ysoft.com]
> Sent: Friday, March 1, 2019 12:58 PM
> To: liweihang <liweih...@hisilicon.com>
> Cc: netdev@vger.kernel.org; da...@davemloft.net; linyunsheng
> <linyunsh...@huawei.com>; Zhuangyuzeng (Yisen)
> <yisen.zhu...@huawei.com>; Salil Mehta <salil.me...@huawei.com>; lipeng
> (Y) <lipeng...@huawei.com>; shenjian (K) <shenjia...@huawei.com>
> Subject: Re: Question about setting speed and duplex failed after auto-
> negotiation disabled on marvell phy
>
> On 01. 03. 19 12:26, liweihang wrote:
> > Hi all,
> >
> > We encountered a problem that if there are two devices in kernel v5.0 with
> > marvell phy 88E1510 connect with each other directly, one with autoneg on
> > and
> > the other off. The one who has disabled auto-negotiation will failed on
> > setting
> > speed and duplex mode. Their speed and duplex mode will go to 10M/half. And
> > no matter what speed and duplex we set by ethtool -s ethx speed XX duplex
> > full,
> > they will go to 10M/half. If we disable auto-negotiation on both sides and
> > set
> > same speed and duplex, there speed and duplex mode will go to Unknown.
> >
> > I found that m88e1121_config_aneg() has been modified by commit
> > d6ab93364734 net: phy: marvell: Avoid unnecessary soft reset
> > And in that patch, genphy_soft_reset() was moved below genphy_config_aneg(),
> > which caused value of speed and duplex in MII_BMCR was cleared. And then
> > they
> > will go to 10M/half.
> >
> > static int m88e1121_config_aneg(struct phy_device *phydev)
> > {
> > + int changed = 0;
> > int err = 0;
> >
> > if (phy_interface_is_rgmii(phydev)) {
> > @@ -487,15 +455,26 @@ static int m88e1121_config_aneg(struct phy_device
> > *phydev)
> > return err;
> > }
> >
> > - err = genphy_soft_reset(phydev);
> > + err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
> > if (err < 0)
> > return err;
> >
> > - err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
> > + changed = err;
> > +
> > + err = genphy_config_aneg(phydev);
> > if (err < 0)
> > return err;
> >
> > - return genphy_config_aneg(phydev);
> > + if (phydev->autoneg != autoneg || changed) {
> > + /* A software reset is used to ensure a "commit" of the
> > + * changes is done.
> > + */
> > + err = genphy_soft_reset(phydev);
> > + if (err < 0)
> > + return err;
> > + }
> > +
> > + return 0;
> > }
> >
> > I moved genphy_soft_reset back and it tested ok. And in my opinion, if we
> > have to do
> > soft reset after genphy_config_aneg(phydev), it should be like this:
> >
> > if (phydev->autoneg != AUTONEG_ENABLE) {
> > int bmcr;
> >
> > bmcr = phy_read(phydev, MII_BMCR);
> > if (bmcr < 0)
> > return bmcr;
> >
> > err = phy_write(phydev, MII_BMCR, bmcr | BMCR_RESET);
> > if (err < 0)
> > return err;
> > }
> >
> > The above code has been mentioned in commit 3438634456c4
> > net: phy: marvell: Use core genphy_soft_reset(), phy_write() was replaced by
> > genphy_soft_reset() in that patch.
> >
> > I think this issue will affect devices with Marvell PHY ID
> > 88E1112/1111/1121/
> > 1318/1240/1510/1540/1545/6390.
> >
> > If there are any better info or suggestion regarding this problem, it would
> > be very
> > helpful, thanks in advance.
>
> I am not sure what exact -rc version you are using (kernel v5.0 is not yet
> released). I would recommend you to test the latest linux-next or even
> net-next as there is quite a lot of new patches and fixes targeting Marvell
> chips.
>
> Best regards,
> Michal
> >
> > reference:
> > [1] https://lore.kernel.org/patchwork/patch/991682/
> > [2] https://patchwork.ozlabs.org/patch/795435/
> >
