> -----Original Message----- > From: Michal Vokáč [mailto:michal.vo...@ysoft.com] > Sent: Friday, March 01, 2019 8: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. >
Thanks for your advice, I'm using v5.0-rc1, and I tested on net-next and got an same result. > Best regards, > Michal > > > > reference: > > [1] https://lore.kernel.org/patchwork/patch/991682/ > > [2] https://patchwork.ozlabs.org/patch/795435/ > >
