* [PATCH NET] net/hns:bugfix of ethtool -t phy self_test @ 2017-06-16 9:24 Lin Yun Sheng 2017-06-19 18:21 ` David Miller 2017-06-19 21:00 ` Florian Fainelli 0 siblings, 2 replies; 16+ messages in thread From: Lin Yun Sheng @ 2017-06-16 9:24 UTC (permalink / raw) To: davem Cc: huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, yisen.zhuang, salil.mehta, lipeng321, yankejian, tremyfr, xieqianqian, netdev, linux-kernel This patch fixes the phy loopback self_test failed issue. when Marvell Phy Module is loaded, it will powerdown fiber when doing phy loopback self test, which cause phy loopback self_test fail. Signed-off-by: Lin Yun Sheng <linyunsheng@huawei.com> --- drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c index b8fab14..e95795b 100644 --- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c +++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c @@ -288,9 +288,15 @@ static int hns_nic_config_phy_loopback(struct phy_device *phy_dev, u8 en) /* Force 1000M Link, Default is 0x0200 */ phy_write(phy_dev, 7, 0x20C); - phy_write(phy_dev, HNS_PHY_PAGE_REG, 0); - /* Enable PHY loop-back */ + /* Powerup Fiber */ + phy_write(phy_dev, HNS_PHY_PAGE_REG, 1); + val = phy_read(phy_dev, COPPER_CONTROL_REG); + val &= ~PHY_POWER_DOWN; + phy_write(phy_dev, COPPER_CONTROL_REG, val); + + /* Enable Phy Loopback */ + phy_write(phy_dev, HNS_PHY_PAGE_REG, 0); val = phy_read(phy_dev, COPPER_CONTROL_REG); val |= PHY_LOOP_BACK; val &= ~PHY_POWER_DOWN; @@ -299,6 +305,12 @@ static int hns_nic_config_phy_loopback(struct phy_device *phy_dev, u8 en) phy_write(phy_dev, HNS_PHY_PAGE_REG, 0xFA); phy_write(phy_dev, 1, 0x400); phy_write(phy_dev, 7, 0x200); + + phy_write(phy_dev, HNS_PHY_PAGE_REG, 1); + val = phy_read(phy_dev, COPPER_CONTROL_REG); + val |= PHY_POWER_DOWN; + phy_write(phy_dev, COPPER_CONTROL_REG, val); + phy_write(phy_dev, HNS_PHY_PAGE_REG, 0); phy_write(phy_dev, 9, 0xF00); -- 1.9.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test 2017-06-16 9:24 [PATCH NET] net/hns:bugfix of ethtool -t phy self_test Lin Yun Sheng @ 2017-06-19 18:21 ` David Miller 2017-06-19 21:00 ` Florian Fainelli 1 sibling, 0 replies; 16+ messages in thread From: David Miller @ 2017-06-19 18:21 UTC (permalink / raw) To: linyunsheng Cc: huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, yankejian, tremyfr, xieqianqian, netdev, linux-kernel From: Lin Yun Sheng <linyunsheng@huawei.com> Date: Fri, 16 Jun 2017 17:24:51 +0800 > This patch fixes the phy loopback self_test failed issue. when > Marvell Phy Module is loaded, it will powerdown fiber when doing > phy loopback self test, which cause phy loopback self_test fail. > > Signed-off-by: Lin Yun Sheng <linyunsheng@huawei.com> Applied. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test 2017-06-16 9:24 [PATCH NET] net/hns:bugfix of ethtool -t phy self_test Lin Yun Sheng 2017-06-19 18:21 ` David Miller @ 2017-06-19 21:00 ` Florian Fainelli 2017-06-19 21:54 ` Andrew Lunn 2017-06-20 3:05 ` l00371289 1 sibling, 2 replies; 16+ messages in thread From: Florian Fainelli @ 2017-06-19 21:00 UTC (permalink / raw) To: Lin Yun Sheng, davem Cc: huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, yankejian, tremyfr, xieqianqian, netdev, linux-kernel, Andrew Lunn On 06/16/2017 02:24 AM, Lin Yun Sheng wrote: > This patch fixes the phy loopback self_test failed issue. when > Marvell Phy Module is loaded, it will powerdown fiber when doing > phy loopback self test, which cause phy loopback self_test fail. > > Signed-off-by: Lin Yun Sheng <linyunsheng@huawei.com> > --- > drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c > index b8fab14..e95795b 100644 > --- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c > +++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c > @@ -288,9 +288,15 @@ static int hns_nic_config_phy_loopback(struct phy_device *phy_dev, u8 en) The question really is, why is not this properly integrated into the PHY driver and PHYLIB such that the only thing the Ethernet MAC driver has to call is a function of the PHY driver putting it in self-test? > > /* Force 1000M Link, Default is 0x0200 */ > phy_write(phy_dev, 7, 0x20C); > - phy_write(phy_dev, HNS_PHY_PAGE_REG, 0); > > - /* Enable PHY loop-back */ > + /* Powerup Fiber */ > + phy_write(phy_dev, HNS_PHY_PAGE_REG, 1); > + val = phy_read(phy_dev, COPPER_CONTROL_REG); > + val &= ~PHY_POWER_DOWN; > + phy_write(phy_dev, COPPER_CONTROL_REG, val); > + > + /* Enable Phy Loopback */ > + phy_write(phy_dev, HNS_PHY_PAGE_REG, 0); > val = phy_read(phy_dev, COPPER_CONTROL_REG); > val |= PHY_LOOP_BACK; > val &= ~PHY_POWER_DOWN; > @@ -299,6 +305,12 @@ static int hns_nic_config_phy_loopback(struct phy_device *phy_dev, u8 en) > phy_write(phy_dev, HNS_PHY_PAGE_REG, 0xFA); > phy_write(phy_dev, 1, 0x400); > phy_write(phy_dev, 7, 0x200); > + > + phy_write(phy_dev, HNS_PHY_PAGE_REG, 1); > + val = phy_read(phy_dev, COPPER_CONTROL_REG); > + val |= PHY_POWER_DOWN; > + phy_write(phy_dev, COPPER_CONTROL_REG, val); > + > phy_write(phy_dev, HNS_PHY_PAGE_REG, 0); > phy_write(phy_dev, 9, 0xF00); > > -- Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test 2017-06-19 21:00 ` Florian Fainelli @ 2017-06-19 21:54 ` Andrew Lunn 2017-06-20 3:18 ` l00371289 2017-06-20 3:05 ` l00371289 1 sibling, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2017-06-19 21:54 UTC (permalink / raw) To: Florian Fainelli Cc: Lin Yun Sheng, davem, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, yankejian, tremyfr, xieqianqian, netdev, linux-kernel On Mon, Jun 19, 2017 at 02:00:43PM -0700, Florian Fainelli wrote: > On 06/16/2017 02:24 AM, Lin Yun Sheng wrote: > > This patch fixes the phy loopback self_test failed issue. when > > Marvell Phy Module is loaded, it will powerdown fiber when doing > > phy loopback self test, which cause phy loopback self_test fail. > > > > Signed-off-by: Lin Yun Sheng <linyunsheng@huawei.com> > > --- > > drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 16 ++++++++++++++-- > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c > > index b8fab14..e95795b 100644 > > --- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c > > +++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c > > @@ -288,9 +288,15 @@ static int hns_nic_config_phy_loopback(struct phy_device *phy_dev, u8 en) > > The question really is, why is not this properly integrated into the PHY > driver and PHYLIB such that the only thing the Ethernet MAC driver has > to call is a function of the PHY driver putting it in self-test? This whole driver pokes various PHY registers, rather than use phylib. And it does so without taking the PHY lock. It also assumes it is a Marvell PHY and i don't see anywhere it actually verifies this. This is all broken. Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test 2017-06-19 21:54 ` Andrew Lunn @ 2017-06-20 3:18 ` l00371289 2017-06-20 13:28 ` Andrew Lunn 0 siblings, 1 reply; 16+ messages in thread From: l00371289 @ 2017-06-20 3:18 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli Cc: davem, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, yankejian, tremyfr, xieqianqian, netdev, linux-kernel Hi, Andrew On 2017/6/20 5:54, Andrew Lunn wrote: > On Mon, Jun 19, 2017 at 02:00:43PM -0700, Florian Fainelli wrote: >> On 06/16/2017 02:24 AM, Lin Yun Sheng wrote: >>> This patch fixes the phy loopback self_test failed issue. when >>> Marvell Phy Module is loaded, it will powerdown fiber when doing >>> phy loopback self test, which cause phy loopback self_test fail. >>> >>> Signed-off-by: Lin Yun Sheng <linyunsheng@huawei.com> >>> --- >>> drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 16 ++++++++++++++-- >>> 1 file changed, 14 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c >>> index b8fab14..e95795b 100644 >>> --- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c >>> +++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c >>> @@ -288,9 +288,15 @@ static int hns_nic_config_phy_loopback(struct phy_device *phy_dev, u8 en) >> >> The question really is, why is not this properly integrated into the PHY >> driver and PHYLIB such that the only thing the Ethernet MAC driver has >> to call is a function of the PHY driver putting it in self-test? > > This whole driver pokes various PHY registers, rather than use > phylib. And it does so without taking the PHY lock. I will consider using phylib as much as possible, thanks. It also assumes it > is a Marvell PHY and i don't see anywhere it actually verifies this. When it said Marvell Phy , I meant Marvell Phy with fibre support. I will send anther patch to only setting bit in Fiber Control when it is a Marvell Phy with fibre support. Thanks for reply. Best Regards Yunsheng Lin > > This is all broken. > > Andrew > > . > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test 2017-06-20 3:18 ` l00371289 @ 2017-06-20 13:28 ` Andrew Lunn 2017-06-21 2:06 ` l00371289 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2017-06-20 13:28 UTC (permalink / raw) To: l00371289 Cc: Florian Fainelli, davem, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, yankejian, tremyfr, xieqianqian, netdev, linux-kernel > >> The question really is, why is not this properly integrated into the PHY > >> driver and PHYLIB such that the only thing the Ethernet MAC driver has > >> to call is a function of the PHY driver putting it in self-test? > > > > This whole driver pokes various PHY registers, rather than use > > phylib. And it does so without taking the PHY lock. > I will consider using phylib as much as possible, thanks. > > It also assumes it > > is a Marvell PHY and i don't see anywhere it actually verifies this. > When it said Marvell Phy , I meant Marvell Phy with fibre support. > I will send anther patch to only setting bit in Fiber Control when > it is a Marvell Phy with fibre support. There is a lot more broken than just that. You really should remove all code which is accessing the PHY, and add support to phylib and the drivers for what you need. Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test 2017-06-20 13:28 ` Andrew Lunn @ 2017-06-21 2:06 ` l00371289 0 siblings, 0 replies; 16+ messages in thread From: l00371289 @ 2017-06-21 2:06 UTC (permalink / raw) To: Andrew Lunn Cc: Florian Fainelli, davem, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, yankejian, tremyfr, xieqianqian, netdev, linux-kernel Hi, Andrew On 2017/6/20 21:28, Andrew Lunn wrote: >>>> The question really is, why is not this properly integrated into the PHY >>>> driver and PHYLIB such that the only thing the Ethernet MAC driver has >>>> to call is a function of the PHY driver putting it in self-test? >>> >>> This whole driver pokes various PHY registers, rather than use >>> phylib. And it does so without taking the PHY lock. >> I will consider using phylib as much as possible, thanks. >> >> It also assumes it >>> is a Marvell PHY and i don't see anywhere it actually verifies this. >> When it said Marvell Phy , I meant Marvell Phy with fibre support. >> I will send anther patch to only setting bit in Fiber Control when >> it is a Marvell Phy with fibre support. > > There is a lot more broken than just that. > > You really should remove all code which is accessing the PHY, and add > support to phylib and the drivers for what you need. > > Andrew After adding genphy_loopback support, I will try it. Thanks for pointing out. Best Regards Yunsheng Lin ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test 2017-06-19 21:00 ` Florian Fainelli 2017-06-19 21:54 ` Andrew Lunn @ 2017-06-20 3:05 ` l00371289 2017-06-20 13:27 ` Andrew Lunn 1 sibling, 1 reply; 16+ messages in thread From: l00371289 @ 2017-06-20 3:05 UTC (permalink / raw) To: Florian Fainelli, davem Cc: huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, yankejian, tremyfr, xieqianqian, netdev, linux-kernel, Andrew Lunn hi, Florian On 2017/6/20 5:00, Florian Fainelli wrote: > On 06/16/2017 02:24 AM, Lin Yun Sheng wrote: >> This patch fixes the phy loopback self_test failed issue. when >> Marvell Phy Module is loaded, it will powerdown fiber when doing >> phy loopback self test, which cause phy loopback self_test fail. >> >> Signed-off-by: Lin Yun Sheng <linyunsheng@huawei.com> >> --- >> drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c >> index b8fab14..e95795b 100644 >> --- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c >> +++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c >> @@ -288,9 +288,15 @@ static int hns_nic_config_phy_loopback(struct phy_device *phy_dev, u8 en) > > The question really is, why is not this properly integrated into the PHY > driver and PHYLIB such that the only thing the Ethernet MAC driver has > to call is a function of the PHY driver putting it in self-test? Do you meaning calling phy_dev->drv->resume and phy_dev->drv->suspend function? I tried it, but it failed. if that is what you mean, I will look into it why it fail. Thanks for your reply. Best regards YunSheng Lin > >> >> /* Force 1000M Link, Default is 0x0200 */ >> phy_write(phy_dev, 7, 0x20C); >> - phy_write(phy_dev, HNS_PHY_PAGE_REG, 0); >> >> - /* Enable PHY loop-back */ >> + /* Powerup Fiber */ >> + phy_write(phy_dev, HNS_PHY_PAGE_REG, 1); >> + val = phy_read(phy_dev, COPPER_CONTROL_REG); >> + val &= ~PHY_POWER_DOWN; >> + phy_write(phy_dev, COPPER_CONTROL_REG, val); >> + >> + /* Enable Phy Loopback */ >> + phy_write(phy_dev, HNS_PHY_PAGE_REG, 0); >> val = phy_read(phy_dev, COPPER_CONTROL_REG); >> val |= PHY_LOOP_BACK; >> val &= ~PHY_POWER_DOWN; >> @@ -299,6 +305,12 @@ static int hns_nic_config_phy_loopback(struct phy_device *phy_dev, u8 en) >> phy_write(phy_dev, HNS_PHY_PAGE_REG, 0xFA); >> phy_write(phy_dev, 1, 0x400); >> phy_write(phy_dev, 7, 0x200); >> + >> + phy_write(phy_dev, HNS_PHY_PAGE_REG, 1); >> + val = phy_read(phy_dev, COPPER_CONTROL_REG); >> + val |= PHY_POWER_DOWN; >> + phy_write(phy_dev, COPPER_CONTROL_REG, val); >> + >> phy_write(phy_dev, HNS_PHY_PAGE_REG, 0); >> phy_write(phy_dev, 9, 0xF00); >> >> > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test 2017-06-20 3:05 ` l00371289 @ 2017-06-20 13:27 ` Andrew Lunn 2017-06-21 2:03 ` l00371289 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2017-06-20 13:27 UTC (permalink / raw) To: l00371289 Cc: Florian Fainelli, davem, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, yankejian, tremyfr, xieqianqian, netdev, linux-kernel On Tue, Jun 20, 2017 at 11:05:54AM +0800, l00371289 wrote: > hi, Florian > > On 2017/6/20 5:00, Florian Fainelli wrote: > > On 06/16/2017 02:24 AM, Lin Yun Sheng wrote: > >> This patch fixes the phy loopback self_test failed issue. when > >> Marvell Phy Module is loaded, it will powerdown fiber when doing > >> phy loopback self test, which cause phy loopback self_test fail. > >> > >> Signed-off-by: Lin Yun Sheng <linyunsheng@huawei.com> > >> --- > >> drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 16 ++++++++++++++-- > >> 1 file changed, 14 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c > >> index b8fab14..e95795b 100644 > >> --- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c > >> +++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c > >> @@ -288,9 +288,15 @@ static int hns_nic_config_phy_loopback(struct phy_device *phy_dev, u8 en) > > > > The question really is, why is not this properly integrated into the PHY > > driver and PHYLIB such that the only thing the Ethernet MAC driver has > > to call is a function of the PHY driver putting it in self-test? > Do you meaning calling phy_dev->drv->resume and phy_dev->drv->suspend function? No. Florian is saying you should add support for phylib and the drivers to enable/disable loopback. The BMCR loopback bit is pretty much standardised. So you can implement a genphy_loopback(phydev, enable), which most drivers can use. Those that need there own can implement it in there driver. Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test 2017-06-20 13:27 ` Andrew Lunn @ 2017-06-21 2:03 ` l00371289 2017-06-21 3:13 ` Andrew Lunn 0 siblings, 1 reply; 16+ messages in thread From: l00371289 @ 2017-06-21 2:03 UTC (permalink / raw) To: Andrew Lunn Cc: Florian Fainelli, davem, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, yankejian, tremyfr, xieqianqian, netdev, linux-kernel Hi, Andrew On 2017/6/20 21:27, Andrew Lunn wrote: > On Tue, Jun 20, 2017 at 11:05:54AM +0800, l00371289 wrote: >> hi, Florian >> >> On 2017/6/20 5:00, Florian Fainelli wrote: >>> On 06/16/2017 02:24 AM, Lin Yun Sheng wrote: >>>> This patch fixes the phy loopback self_test failed issue. when >>>> Marvell Phy Module is loaded, it will powerdown fiber when doing >>>> phy loopback self test, which cause phy loopback self_test fail. >>>> >>>> Signed-off-by: Lin Yun Sheng <linyunsheng@huawei.com> >>>> --- >>>> drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 16 ++++++++++++++-- >>>> 1 file changed, 14 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c >>>> index b8fab14..e95795b 100644 >>>> --- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c >>>> +++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c >>>> @@ -288,9 +288,15 @@ static int hns_nic_config_phy_loopback(struct phy_device *phy_dev, u8 en) >>> >>> The question really is, why is not this properly integrated into the PHY >>> driver and PHYLIB such that the only thing the Ethernet MAC driver has >>> to call is a function of the PHY driver putting it in self-test? >> Do you meaning calling phy_dev->drv->resume and phy_dev->drv->suspend function? > > No. Florian is saying you should add support for phylib and the > drivers to enable/disable loopback. > > The BMCR loopback bit is pretty much standardised. So you can > implement a genphy_loopback(phydev, enable), which most drivers can > use. Those that need there own can implement it in there driver. I tried to add the genphy_loopback support you mentioned, please look at it if that is what you mean. If Yes, I will try to send out a new patch. Best Regards Yinsheng Lin diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 1219eea..54fecad 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1628,6 +1628,31 @@ static int gen10g_resume(struct phy_device *phydev) return 0; } +int genphy_loopback(struct phy_device *phydev, bool enable) +{ + int value; + + mutex_lock(&phydev->lock); + + if (enable) { + value = phy_read(phydev, MII_BMCR); + phy_write(phydev, MII_BMCR, value | BMCR_LOOPBACK); + } else { + value = phy_read(phydev, MII_BMCR); + phy_write(phydev, MII_BMCR, value & ~BMCR_LOOPBACK); + } + + mutex_unlock(&phydev->lock); + + return 0; +} +EXPORT_SYMBOL(genphy_loopback); + +static int gen10g_loopback(struct phy_device *phydev, bool enable) +{ + return 0; +} + static int __set_phy_supported(struct phy_device *phydev, u32 max_speed) { /* The default values for phydev->supported are provided by the PHY @@ -1874,6 +1899,7 @@ void phy_drivers_unregister(struct phy_driver *drv, int n) .read_status = genphy_read_status, .suspend = genphy_suspend, .resume = genphy_resume, + .set_loopback = genphy_loopback, }, { .phy_id = 0xffffffff, .phy_id_mask = 0xffffffff, @@ -1885,6 +1911,7 @@ void phy_drivers_unregister(struct phy_driver *drv, int n) .read_status = gen10g_read_status, .suspend = gen10g_suspend, .resume = gen10g_resume, + .set_loopback = gen10g_loopback, } }; static int __init phy_init(void) diff --git a/include/linux/phy.h b/include/linux/phy.h index e76e4ad..fc7a5c8 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -639,6 +639,7 @@ struct phy_driver { int (*set_tunable)(struct phy_device *dev, struct ethtool_tunable *tuna, const void *data); + int (*set_loopback(struct phy_device *dev, bool enable); }; #define to_phy_driver(d) container_of(to_mdio_common_driver(d), \ struct phy_driver, mdiodrv) ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test 2017-06-21 2:03 ` l00371289 @ 2017-06-21 3:13 ` Andrew Lunn 2017-06-21 3:42 ` l00371289 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2017-06-21 3:13 UTC (permalink / raw) To: l00371289 Cc: Florian Fainelli, davem, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, yankejian, tremyfr, xieqianqian, netdev, linux-kernel On Wed, Jun 21, 2017 at 10:03:29AM +0800, l00371289 wrote: > Hi, Andrew > > On 2017/6/20 21:27, Andrew Lunn wrote: > > On Tue, Jun 20, 2017 at 11:05:54AM +0800, l00371289 wrote: > >> hi, Florian > >> > >> On 2017/6/20 5:00, Florian Fainelli wrote: > >>> On 06/16/2017 02:24 AM, Lin Yun Sheng wrote: > >>>> This patch fixes the phy loopback self_test failed issue. when > >>>> Marvell Phy Module is loaded, it will powerdown fiber when doing > >>>> phy loopback self test, which cause phy loopback self_test fail. > >>>> > >>>> Signed-off-by: Lin Yun Sheng <linyunsheng@huawei.com> > >>>> --- > >>>> drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 16 ++++++++++++++-- > >>>> 1 file changed, 14 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c > >>>> index b8fab14..e95795b 100644 > >>>> --- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c > >>>> +++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c > >>>> @@ -288,9 +288,15 @@ static int hns_nic_config_phy_loopback(struct phy_device *phy_dev, u8 en) > >>> > >>> The question really is, why is not this properly integrated into the PHY > >>> driver and PHYLIB such that the only thing the Ethernet MAC driver has > >>> to call is a function of the PHY driver putting it in self-test? > >> Do you meaning calling phy_dev->drv->resume and phy_dev->drv->suspend function? > > > > No. Florian is saying you should add support for phylib and the > > drivers to enable/disable loopback. > > > > The BMCR loopback bit is pretty much standardised. So you can > > implement a genphy_loopback(phydev, enable), which most drivers can > > use. Those that need there own can implement it in there driver. > > I tried to add the genphy_loopback support you mentioned, please look > at it if that is what you mean. If Yes, I will try to send out a new patch. > > Best Regards > Yinsheng Lin > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 1219eea..54fecad 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1628,6 +1628,31 @@ static int gen10g_resume(struct phy_device *phydev) > return 0; > } > > +int genphy_loopback(struct phy_device *phydev, bool enable) > +{ > + int value; > + > + mutex_lock(&phydev->lock); Do you look at the other genphy_ functions? How many take the mutex? > + if (enable) { > + value = phy_read(phydev, MII_BMCR); > + phy_write(phydev, MII_BMCR, value | BMCR_LOOPBACK); > + } else { > + value = phy_read(phydev, MII_BMCR); > + phy_write(phydev, MII_BMCR, value & ~BMCR_LOOPBACK); > + } > + > + mutex_unlock(&phydev->lock); > + > + return 0; > +} > +EXPORT_SYMBOL(genphy_loopback); > + > +static int gen10g_loopback(struct phy_device *phydev, bool enable) > +{ > + return 0; > +} > + > static int __set_phy_supported(struct phy_device *phydev, u32 max_speed) > { > /* The default values for phydev->supported are provided by the PHY > @@ -1874,6 +1899,7 @@ void phy_drivers_unregister(struct phy_driver *drv, int n) > .read_status = genphy_read_status, > .suspend = genphy_suspend, > .resume = genphy_resume, > + .set_loopback = genphy_loopback, > }, { > .phy_id = 0xffffffff, > .phy_id_mask = 0xffffffff, > @@ -1885,6 +1911,7 @@ void phy_drivers_unregister(struct phy_driver *drv, int n) > .read_status = gen10g_read_status, > .suspend = gen10g_suspend, > .resume = gen10g_resume, > + .set_loopback = gen10g_loopback, > } }; > > static int __init phy_init(void) > diff --git a/include/linux/phy.h b/include/linux/phy.h > index e76e4ad..fc7a5c8 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -639,6 +639,7 @@ struct phy_driver { > int (*set_tunable)(struct phy_device *dev, > struct ethtool_tunable *tuna, > const void *data); > + int (*set_loopback(struct phy_device *dev, bool enable); Does this even compile? It looks to be missing a ) Also, where is the exported function the MAC driver should call? Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test 2017-06-21 3:13 ` Andrew Lunn @ 2017-06-21 3:42 ` l00371289 2017-06-21 13:34 ` Andrew Lunn 0 siblings, 1 reply; 16+ messages in thread From: l00371289 @ 2017-06-21 3:42 UTC (permalink / raw) To: Andrew Lunn Cc: Florian Fainelli, davem, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, yankejian, tremyfr, xieqianqian, netdev, linux-kernel Hi, Andrew On 2017/6/21 11:13, Andrew Lunn wrote: > On Wed, Jun 21, 2017 at 10:03:29AM +0800, l00371289 wrote: >> Hi, Andrew >> >> On 2017/6/20 21:27, Andrew Lunn wrote: >>> On Tue, Jun 20, 2017 at 11:05:54AM +0800, l00371289 wrote: >>>> hi, Florian >>>> >>>> On 2017/6/20 5:00, Florian Fainelli wrote: >>>>> On 06/16/2017 02:24 AM, Lin Yun Sheng wrote: >>>>>> This patch fixes the phy loopback self_test failed issue. when >>>>>> Marvell Phy Module is loaded, it will powerdown fiber when doing >>>>>> phy loopback self test, which cause phy loopback self_test fail. >>>>>> >>>>>> Signed-off-by: Lin Yun Sheng <linyunsheng@huawei.com> >>>>>> --- >>>>>> drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 16 ++++++++++++++-- >>>>>> 1 file changed, 14 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c >>>>>> index b8fab14..e95795b 100644 >>>>>> --- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c >>>>>> +++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c >>>>>> @@ -288,9 +288,15 @@ static int hns_nic_config_phy_loopback(struct phy_device *phy_dev, u8 en) >>>>> >>>>> The question really is, why is not this properly integrated into the PHY >>>>> driver and PHYLIB such that the only thing the Ethernet MAC driver has >>>>> to call is a function of the PHY driver putting it in self-test? >>>> Do you meaning calling phy_dev->drv->resume and phy_dev->drv->suspend function? >>> >>> No. Florian is saying you should add support for phylib and the >>> drivers to enable/disable loopback. >>> >>> The BMCR loopback bit is pretty much standardised. So you can >>> implement a genphy_loopback(phydev, enable), which most drivers can >>> use. Those that need there own can implement it in there driver. >> >> I tried to add the genphy_loopback support you mentioned, please look >> at it if that is what you mean. If Yes, I will try to send out a new patch. >> >> Best Regards >> Yinsheng Lin >> >> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >> index 1219eea..54fecad 100644 >> --- a/drivers/net/phy/phy_device.c >> +++ b/drivers/net/phy/phy_device.c >> @@ -1628,6 +1628,31 @@ static int gen10g_resume(struct phy_device *phydev) >> return 0; >> } >> >> +int genphy_loopback(struct phy_device *phydev, bool enable) >> +{ >> + int value; >> + >> + mutex_lock(&phydev->lock); > > Do you look at the other genphy_ functions? How many take the mutex? only genphy_suspend and genphy_resume take the mutex, I will have to remove the lock taking, right? > >> + if (enable) { >> + value = phy_read(phydev, MII_BMCR); >> + phy_write(phydev, MII_BMCR, value | BMCR_LOOPBACK); >> + } else { >> + value = phy_read(phydev, MII_BMCR); >> + phy_write(phydev, MII_BMCR, value & ~BMCR_LOOPBACK); >> + } >> + >> + mutex_unlock(&phydev->lock); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(genphy_loopback); >> + >> +static int gen10g_loopback(struct phy_device *phydev, bool enable) >> +{ >> + return 0; >> +} >> + >> static int __set_phy_supported(struct phy_device *phydev, u32 max_speed) >> { >> /* The default values for phydev->supported are provided by the PHY >> @@ -1874,6 +1899,7 @@ void phy_drivers_unregister(struct phy_driver *drv, int n) >> .read_status = genphy_read_status, >> .suspend = genphy_suspend, >> .resume = genphy_resume, >> + .set_loopback = genphy_loopback, >> }, { >> .phy_id = 0xffffffff, >> .phy_id_mask = 0xffffffff, >> @@ -1885,6 +1911,7 @@ void phy_drivers_unregister(struct phy_driver *drv, int n) >> .read_status = gen10g_read_status, >> .suspend = gen10g_suspend, >> .resume = gen10g_resume, >> + .set_loopback = gen10g_loopback, >> } }; >> >> static int __init phy_init(void) >> diff --git a/include/linux/phy.h b/include/linux/phy.h >> index e76e4ad..fc7a5c8 100644 >> --- a/include/linux/phy.h >> +++ b/include/linux/phy.h >> @@ -639,6 +639,7 @@ struct phy_driver { >> int (*set_tunable)(struct phy_device *dev, >> struct ethtool_tunable *tuna, >> const void *data); >> + int (*set_loopback(struct phy_device *dev, bool enable); > > Does this even compile? It looks to be missing a ) My mistake, I will make sure it will compile before sending it. > > Also, where is the exported function the MAC driver should call? Here is a example: drivers/net/ph/marvell.c marvell_set_loopback(struct phy_device *dev, bool enable) { /* do some device specific setting */ ........ return genphy_loopback(dev, enable); } I don't know if this makes sense or not? Best Regards Yunsheng Lin > > Andrew > > . > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test 2017-06-21 3:42 ` l00371289 @ 2017-06-21 13:34 ` Andrew Lunn 2017-06-22 1:41 ` l00371289 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2017-06-21 13:34 UTC (permalink / raw) To: l00371289 Cc: Florian Fainelli, davem, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, yankejian, tremyfr, xieqianqian, netdev, linux-kernel > drivers/net/ph/marvell.c > marvell_set_loopback(struct phy_device *dev, bool enable) > { > /* do some device specific setting */ > ........ > > return genphy_loopback(dev, enable); > } > > I don't know if this makes sense or not? Nope, you want something in phy.c like this. Not compiled, not tested.... int phy_loopback(struct phy_device *dev, bool enable) { int err; mutex_lock(dev->mutex); if (enable && dev->loopback_enabled) { err = -EBUSY; goto out; } if (!enable && !dev->loopback_enabled) { err = -EINVAL; goto out; } if (dev->drv->loopback) err = dev->drv->loopback(dev, enable); if (!err) dev->loopback_enabled = enable mutex_unlock(dev->mutex); out: return err; } EXPORT_SYMBOL(phy_loopback); The interesting part is how to handle two attempts to enable loopback. Should the MAC driver be responsible for preventing that? Or do it in the PHY driver? And what will the MAC driver do if it gets EBUSY? Return it to user space? Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test 2017-06-21 13:34 ` Andrew Lunn @ 2017-06-22 1:41 ` l00371289 2017-06-22 3:35 ` Andrew Lunn 0 siblings, 1 reply; 16+ messages in thread From: l00371289 @ 2017-06-22 1:41 UTC (permalink / raw) To: Andrew Lunn Cc: Florian Fainelli, davem, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, yankejian, tremyfr, xieqianqian, netdev, linux-kernel Hi, Andrew On 2017/6/21 21:34, Andrew Lunn wrote: >> drivers/net/ph/marvell.c >> marvell_set_loopback(struct phy_device *dev, bool enable) >> { >> /* do some device specific setting */ >> ........ >> >> return genphy_loopback(dev, enable); >> } >> >> I don't know if this makes sense or not? > > Nope, you want something in phy.c like this. Not compiled, not > tested.... > > int phy_loopback(struct phy_device *dev, bool enable) > { > > int err; > > mutex_lock(dev->mutex); > > if (enable && dev->loopback_enabled) { > err = -EBUSY; > goto out; > } > > if (!enable && !dev->loopback_enabled) { > err = -EINVAL; > goto out; > } > > if (dev->drv->loopback) > err = dev->drv->loopback(dev, enable); > > if (!err) > dev->loopback_enabled = enable > > mutex_unlock(dev->mutex); > > out: > return err; > } > EXPORT_SYMBOL(phy_loopback); > > The interesting part is how to handle two attempts to enable loopback. > Should the MAC driver be responsible for preventing that? Or do it in > the PHY driver? And what will the MAC driver do if it gets EBUSY? > Return it to user space? I don't think returning to user space is an option, because it mean ethtool phy self test would fail because if availability of some software resoure, which is not some application would expect. here is what I can think of: driver/net/ethernet/hisilicon/hns/hns_ethtool.c int phy_loopback_selftest(struct phy_device *dev) { int err; mutex_lock(dev->mutex); if (dev->drv->loopback) err = dev->drv->loopback(dev, 1); if (err) goto out; /* mac driver do the test*/ ................. if (dev->drv->loopback) err = dev->drv->loopback(dev, 0); if (err) goto out; out: mutex_unlock(dev->mutex); return err; } One question I can think of is that, how do we ensure mac driver enable and disable phy loopback in pair, enable loopback after dev->mutex is taken, disable loopback before dev->mutex is released? I hope I am not missing something obvious, any suggestion and idea? Best Regards Yunsheng Lin ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test 2017-06-22 1:41 ` l00371289 @ 2017-06-22 3:35 ` Andrew Lunn 2017-06-22 3:49 ` l00371289 0 siblings, 1 reply; 16+ messages in thread From: Andrew Lunn @ 2017-06-22 3:35 UTC (permalink / raw) To: l00371289 Cc: Florian Fainelli, davem, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, yankejian, tremyfr, xieqianqian, netdev, linux-kernel > I don't think returning to user space is an option, because it mean > ethtool phy self test would fail because if availability of some software > resoure, which is not some application would expect. here is what I can > think of: > > driver/net/ethernet/hisilicon/hns/hns_ethtool.c > int phy_loopback_selftest(struct phy_device *dev) > { > int err; > > mutex_lock(dev->mutex); > > if (dev->drv->loopback) > err = dev->drv->loopback(dev, 1); > > if (err) > goto out; > > /* mac driver do the test*/ > ................. > > > if (dev->drv->loopback) > err = dev->drv->loopback(dev, 0); > > if (err) > goto out; > > out: > mutex_unlock(dev->mutex); > return err; > } I don't think you need a mutex here. dev_ioctl.c: case SIOCETHTOOL: dev_load(net, ifr.ifr_name); rtnl_lock(); ret = dev_ethtool(net, &ifr); rtnl_unlock(); So all ethtool operations are protected by the rtnl. You cannot have two tests running at once. > One question I can think of is that, how do we ensure mac driver enable and > disable phy loopback in pair, enable loopback after dev->mutex is taken, > disable loopback before dev->mutex is released? > I hope I am not missing something obvious, any suggestion and idea? You cannot ensure this. But it would be a pretty obvious bug. Andrew ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test 2017-06-22 3:35 ` Andrew Lunn @ 2017-06-22 3:49 ` l00371289 0 siblings, 0 replies; 16+ messages in thread From: l00371289 @ 2017-06-22 3:49 UTC (permalink / raw) To: Andrew Lunn Cc: Florian Fainelli, davem, huangdaode, xuwei5, liguozhu, Yisen.Zhuang, gabriele.paoloni, john.garry, linuxarm, salil.mehta, lipeng321, yankejian, tremyfr, xieqianqian, netdev, linux-kernel Hi, Andrew On 2017/6/22 11:35, Andrew Lunn wrote: >> I don't think returning to user space is an option, because it mean >> ethtool phy self test would fail because if availability of some software >> resoure, which is not some application would expect. here is what I can >> think of: >> >> driver/net/ethernet/hisilicon/hns/hns_ethtool.c >> int phy_loopback_selftest(struct phy_device *dev) >> { >> int err; >> >> mutex_lock(dev->mutex); >> >> if (dev->drv->loopback) >> err = dev->drv->loopback(dev, 1); >> >> if (err) >> goto out; >> >> /* mac driver do the test*/ >> ................. >> >> >> if (dev->drv->loopback) >> err = dev->drv->loopback(dev, 0); >> >> if (err) >> goto out; >> >> out: >> mutex_unlock(dev->mutex); >> return err; >> } > > I don't think you need a mutex here. dev_ioctl.c: > > case SIOCETHTOOL: > dev_load(net, ifr.ifr_name); > rtnl_lock(); > ret = dev_ethtool(net, &ifr); > rtnl_unlock(); > > So all ethtool operations are protected by the rtnl. You cannot have > two tests running at once. > >> One question I can think of is that, how do we ensure mac driver enable and >> disable phy loopback in pair, enable loopback after dev->mutex is taken, >> disable loopback before dev->mutex is released? >> I hope I am not missing something obvious, any suggestion and idea? > > You cannot ensure this. But it would be a pretty obvious bug. > Thanks for your reply, I will send a new patch based on the discussion above. Please let me know if there is anything else need changing. Best Regards Yunsheng Lin ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-06-22 3:51 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-16 9:24 [PATCH NET] net/hns:bugfix of ethtool -t phy self_test Lin Yun Sheng 2017-06-19 18:21 ` David Miller 2017-06-19 21:00 ` Florian Fainelli 2017-06-19 21:54 ` Andrew Lunn 2017-06-20 3:18 ` l00371289 2017-06-20 13:28 ` Andrew Lunn 2017-06-21 2:06 ` l00371289 2017-06-20 3:05 ` l00371289 2017-06-20 13:27 ` Andrew Lunn 2017-06-21 2:03 ` l00371289 2017-06-21 3:13 ` Andrew Lunn 2017-06-21 3:42 ` l00371289 2017-06-21 13:34 ` Andrew Lunn 2017-06-22 1:41 ` l00371289 2017-06-22 3:35 ` Andrew Lunn 2017-06-22 3:49 ` l00371289
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.