Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1671319

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

Show key headers only | View raw

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
> 
> .
> 

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test l00371289 <linyunsheng@huawei.com> - 2017-06-20 05:10 +0200
  Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test Andrew Lunn <andrew@lunn.ch> - 2017-06-20 15:30 +0200
    Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test l00371289 <linyunsheng@huawei.com> - 2017-06-21 04:10 +0200
      Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test Andrew Lunn <andrew@lunn.ch> - 2017-06-21 05:20 +0200
        Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test l00371289 <linyunsheng@huawei.com> - 2017-06-21 05:50 +0200
          Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test Andrew Lunn <andrew@lunn.ch> - 2017-06-21 15:40 +0200
            Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test l00371289 <linyunsheng@huawei.com> - 2017-06-22 03:50 +0200
              Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test Andrew Lunn <andrew@lunn.ch> - 2017-06-22 05:40 +0200
                Re: [PATCH NET] net/hns:bugfix of ethtool -t phy self_test l00371289 <linyunsheng@huawei.com> - 2017-06-22 06:00 +0200

csiph-web