[NET] net/hns:bugfix of ethtool -t phy self_test

Submitted by Lin Yun Sheng on June 16, 2017, 9:24 a.m.

Details

Message ID 1497605091-94725-1-git-send-email-linyunsheng@huawei.com
State Accepted
Delegated to: David Miller
Headers show

Commit Message

Lin Yun Sheng June 16, 2017, 9:24 a.m.
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(-)

Comments

David Miller June 19, 2017, 6:21 p.m.
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.
Florian Fainelli June 19, 2017, 9 p.m.
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);
>  
>
Andrew Lunn June 19, 2017, 9:54 p.m.
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
Lin Yun Sheng June 20, 2017, 3:05 a.m.
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);
>>  
>>
> 
>
Lin Yun Sheng June 20, 2017, 3:18 a.m.
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
> 
> .
>
Andrew Lunn June 20, 2017, 1:27 p.m.
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
Andrew Lunn June 20, 2017, 1:28 p.m.
> >> 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
Lin Yun Sheng June 21, 2017, 2:06 a.m.
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

Patch hide | download patch | download mbox

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