netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next] net: phy: bcm87xx: improve feature configuration
@ 2019-04-16 22:06 Heiner Kallweit
  2019-04-16 22:25 ` Andrew Lunn
  2019-04-18 23:30 ` David Miller
  0 siblings, 2 replies; 6+ messages in thread
From: Heiner Kallweit @ 2019-04-16 22:06 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev

This driver is the only user of PHY_10GBIT_FEC_FEATURES. So we may be
able to remove this predefined feature constant later. Setting
phydev->advertising to what is supported is done by phy_probe(),
therefore we don't have to do it in the config_init callback.
Resetting phydev->autoneg if autoneg isn't supported is also done by
phy_probe() now, and manually setting the phylib state machine to
NOLINK is neither needed nor advisable.
In addition propagate the return value of bcm87xx_of_reg_init.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/bcm87xx.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/bcm87xx.c b/drivers/net/phy/bcm87xx.c
index f0c0eefe2..805fe62e4 100644
--- a/drivers/net/phy/bcm87xx.c
+++ b/drivers/net/phy/bcm87xx.c
@@ -81,22 +81,19 @@ static int bcm87xx_of_reg_init(struct phy_device *phydev)
 }
 #endif /* CONFIG_OF_MDIO */
 
-static int bcm87xx_config_init(struct phy_device *phydev)
+static int bcm87xx_get_features(struct phy_device *phydev)
 {
 	linkmode_zero(phydev->supported);
 	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseR_FEC_BIT,
 			 phydev->supported);
-	linkmode_zero(phydev->advertising);
-	linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseR_FEC_BIT,
-			 phydev->advertising);
-	phydev->state = PHY_NOLINK;
-	phydev->autoneg = AUTONEG_DISABLE;
-
-	bcm87xx_of_reg_init(phydev);
-
 	return 0;
 }
 
+static int bcm87xx_config_init(struct phy_device *phydev)
+{
+	return bcm87xx_of_reg_init(phydev);
+}
+
 static int bcm87xx_config_aneg(struct phy_device *phydev)
 {
 	return -EINVAL;
@@ -194,7 +191,7 @@ static struct phy_driver bcm87xx_driver[] = {
 	.phy_id		= PHY_ID_BCM8706,
 	.phy_id_mask	= 0xffffffff,
 	.name		= "Broadcom BCM8706",
-	.features	= PHY_10GBIT_FEC_FEATURES,
+	.get_features	= bcm87xx_get_features,
 	.config_init	= bcm87xx_config_init,
 	.config_aneg	= bcm87xx_config_aneg,
 	.read_status	= bcm87xx_read_status,
@@ -206,7 +203,7 @@ static struct phy_driver bcm87xx_driver[] = {
 	.phy_id		= PHY_ID_BCM8727,
 	.phy_id_mask	= 0xffffffff,
 	.name		= "Broadcom BCM8727",
-	.features	= PHY_10GBIT_FEC_FEATURES,
+	.get_features	= bcm87xx_get_features,
 	.config_init	= bcm87xx_config_init,
 	.config_aneg	= bcm87xx_config_aneg,
 	.read_status	= bcm87xx_read_status,
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next] net: phy: bcm87xx: improve feature configuration
  2019-04-16 22:06 [PATCH net-next] net: phy: bcm87xx: improve feature configuration Heiner Kallweit
@ 2019-04-16 22:25 ` Andrew Lunn
  2019-04-16 22:34   ` Heiner Kallweit
  2019-04-18 23:30 ` David Miller
  1 sibling, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2019-04-16 22:25 UTC (permalink / raw)
  To: Heiner Kallweit; +Cc: Florian Fainelli, David Miller, netdev

> @@ -194,7 +191,7 @@ static struct phy_driver bcm87xx_driver[] = {
>  	.phy_id		= PHY_ID_BCM8706,
>  	.phy_id_mask	= 0xffffffff,
>  	.name		= "Broadcom BCM8706",
> -	.features	= PHY_10GBIT_FEC_FEATURES,
> +	.get_features	= bcm87xx_get_features,
>  	.config_init	= bcm87xx_config_init,
>  	.config_aneg	= bcm87xx_config_aneg,
>  	.read_status	= bcm87xx_read_status,
> @@ -206,7 +203,7 @@ static struct phy_driver bcm87xx_driver[] = {
>  	.phy_id		= PHY_ID_BCM8727,
>  	.phy_id_mask	= 0xffffffff,
>  	.name		= "Broadcom BCM8727",
> -	.features	= PHY_10GBIT_FEC_FEATURES,
> +	.get_features	= bcm87xx_get_features,
>  	.config_init	= bcm87xx_config_init,
>  	.config_aneg	= bcm87xx_config_aneg,
>  	.read_status	= bcm87xx_read_status,

Hi Heiner

The product briefs for these two devices say:

MDIO interface compliant to IEEE802.3ae Clause 45 with
extended indirect address register access

It might be possible to just detect most of this?

Florian: Do you have access to any boards using these devices?

	 Andrew

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next] net: phy: bcm87xx: improve feature configuration
  2019-04-16 22:25 ` Andrew Lunn
@ 2019-04-16 22:34   ` Heiner Kallweit
  2019-04-16 23:47     ` Florian Fainelli
  0 siblings, 1 reply; 6+ messages in thread
From: Heiner Kallweit @ 2019-04-16 22:34 UTC (permalink / raw)
  To: Andrew Lunn; +Cc: Florian Fainelli, David Miller, netdev

On 17.04.2019 00:25, Andrew Lunn wrote:
>> @@ -194,7 +191,7 @@ static struct phy_driver bcm87xx_driver[] = {
>>  	.phy_id		= PHY_ID_BCM8706,
>>  	.phy_id_mask	= 0xffffffff,
>>  	.name		= "Broadcom BCM8706",
>> -	.features	= PHY_10GBIT_FEC_FEATURES,
>> +	.get_features	= bcm87xx_get_features,
>>  	.config_init	= bcm87xx_config_init,
>>  	.config_aneg	= bcm87xx_config_aneg,
>>  	.read_status	= bcm87xx_read_status,
>> @@ -206,7 +203,7 @@ static struct phy_driver bcm87xx_driver[] = {
>>  	.phy_id		= PHY_ID_BCM8727,
>>  	.phy_id_mask	= 0xffffffff,
>>  	.name		= "Broadcom BCM8727",
>> -	.features	= PHY_10GBIT_FEC_FEATURES,
>> +	.get_features	= bcm87xx_get_features,
>>  	.config_init	= bcm87xx_config_init,
>>  	.config_aneg	= bcm87xx_config_aneg,
>>  	.read_status	= bcm87xx_read_status,
> 
> Hi Heiner
> 
> The product briefs for these two devices say:
> 
> MDIO interface compliant to IEEE802.3ae Clause 45 with
> extended indirect address register access
> 
> It might be possible to just detect most of this?
> 
Unfortunately I have no datasheet. There would be more things to
fix in this old driver. The C45 register accesses should be
switched to proper phy_mmd_read/write calls.

> Florian: Do you have access to any boards using these devices?
> 
> 	 Andrew
> 
Heiner

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next] net: phy: bcm87xx: improve feature configuration
  2019-04-16 22:34   ` Heiner Kallweit
@ 2019-04-16 23:47     ` Florian Fainelli
  0 siblings, 0 replies; 6+ messages in thread
From: Florian Fainelli @ 2019-04-16 23:47 UTC (permalink / raw)
  To: Heiner Kallweit, Andrew Lunn; +Cc: David Miller, netdev



On 16/04/2019 15:34, Heiner Kallweit wrote:
> On 17.04.2019 00:25, Andrew Lunn wrote:
>>> @@ -194,7 +191,7 @@ static struct phy_driver bcm87xx_driver[] = {
>>>   	.phy_id		= PHY_ID_BCM8706,
>>>   	.phy_id_mask	= 0xffffffff,
>>>   	.name		= "Broadcom BCM8706",
>>> -	.features	= PHY_10GBIT_FEC_FEATURES,
>>> +	.get_features	= bcm87xx_get_features,
>>>   	.config_init	= bcm87xx_config_init,
>>>   	.config_aneg	= bcm87xx_config_aneg,
>>>   	.read_status	= bcm87xx_read_status,
>>> @@ -206,7 +203,7 @@ static struct phy_driver bcm87xx_driver[] = {
>>>   	.phy_id		= PHY_ID_BCM8727,
>>>   	.phy_id_mask	= 0xffffffff,
>>>   	.name		= "Broadcom BCM8727",
>>> -	.features	= PHY_10GBIT_FEC_FEATURES,
>>> +	.get_features	= bcm87xx_get_features,
>>>   	.config_init	= bcm87xx_config_init,
>>>   	.config_aneg	= bcm87xx_config_aneg,
>>>   	.read_status	= bcm87xx_read_status,
>>
>> Hi Heiner
>>
>> The product briefs for these two devices say:
>>
>> MDIO interface compliant to IEEE802.3ae Clause 45 with
>> extended indirect address register access
>>
>> It might be possible to just detect most of this?
>>
> Unfortunately I have no datasheet. There would be more things to
> fix in this old driver. The C45 register accesses should be
> switched to proper phy_mmd_read/write calls.

Checking the datasheet would not be a problem, I can reply later next 
week when I am back to work.

> 
>> Florian: Do you have access to any boards using these devices?

That I do not unfortunately, those are early 10G PHYs.
-- 
Florian

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next] net: phy: bcm87xx: improve feature configuration
  2019-04-16 22:06 [PATCH net-next] net: phy: bcm87xx: improve feature configuration Heiner Kallweit
  2019-04-16 22:25 ` Andrew Lunn
@ 2019-04-18 23:30 ` David Miller
  2019-04-25 17:32   ` Heiner Kallweit
  1 sibling, 1 reply; 6+ messages in thread
From: David Miller @ 2019-04-18 23:30 UTC (permalink / raw)
  To: hkallweit1; +Cc: andrew, f.fainelli, netdev

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Wed, 17 Apr 2019 00:06:28 +0200

> This driver is the only user of PHY_10GBIT_FEC_FEATURES. So we may be
> able to remove this predefined feature constant later. Setting
> phydev->advertising to what is supported is done by phy_probe(),
> therefore we don't have to do it in the config_init callback.
> Resetting phydev->autoneg if autoneg isn't supported is also done by
> phy_probe() now, and manually setting the phylib state machine to
> NOLINK is neither needed nor advisable.
> In addition propagate the return value of bcm87xx_of_reg_init.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

I read the discussion on this patch, there doesn't seem to be outright
NACK but rather some talk about future enhancements and something about
Florian looking at a few datasheets next week.

What's the story?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next] net: phy: bcm87xx: improve feature configuration
  2019-04-18 23:30 ` David Miller
@ 2019-04-25 17:32   ` Heiner Kallweit
  0 siblings, 0 replies; 6+ messages in thread
From: Heiner Kallweit @ 2019-04-25 17:32 UTC (permalink / raw)
  To: David Miller; +Cc: andrew, f.fainelli, netdev

On 19.04.2019 01:30, David Miller wrote:
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Date: Wed, 17 Apr 2019 00:06:28 +0200
> 
>> This driver is the only user of PHY_10GBIT_FEC_FEATURES. So we may be
>> able to remove this predefined feature constant later. Setting
>> phydev->advertising to what is supported is done by phy_probe(),
>> therefore we don't have to do it in the config_init callback.
>> Resetting phydev->autoneg if autoneg isn't supported is also done by
>> phy_probe() now, and manually setting the phylib state machine to
>> NOLINK is neither needed nor advisable.
>> In addition propagate the return value of bcm87xx_of_reg_init.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> I read the discussion on this patch, there doesn't seem to be outright
> NACK but rather some talk about future enhancements and something about
> Florian looking at a few datasheets next week.
> 
> What's the story?
> 
Apart from this smaller improvement there may be more to improve in this
driver, but it needs the datasheet to find out.
I think it should also be ok to apply the current patch now, and further
improvements later.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-04-25 17:33 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-16 22:06 [PATCH net-next] net: phy: bcm87xx: improve feature configuration Heiner Kallweit
2019-04-16 22:25 ` Andrew Lunn
2019-04-16 22:34   ` Heiner Kallweit
2019-04-16 23:47     ` Florian Fainelli
2019-04-18 23:30 ` David Miller
2019-04-25 17:32   ` Heiner Kallweit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).