diff mbox series

[net-next,2/3] net: dp83869: Add ability to advertise Fiber connection

Message ID 20200915181708.25842-3-dmurphy@ti.com
State New
Headers show
Series None | expand

Commit Message

Dan Murphy Sept. 15, 2020, 6:17 p.m. UTC 
Add the ability to advertise the Fiber connection if the strap or the
op-mode is configured for 100Base-FX.

Auto negotiation is not supported on this PHY when in fiber mode.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/net/phy/dp83869.c | 73 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

Comments

Andrew Lunn Sept. 15, 2020, 8:17 p.m. UTC | #1 
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseFX_Full_BIT,

> +				 phydev->supported);

> +		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseFX_Half_BIT,

> +				 phydev->supported);

> +

> +		/* Auto neg is not supported in 100base FX mode */


Hi Dan

If it does not support auto neg, how do you decide to do half duplex?
I don't see any code here which allows the user to configure it.

  Andrew
Dan Murphy Sept. 16, 2020, 8:54 p.m. UTC | #2 
Andrew

On 9/15/20 3:17 PM, Andrew Lunn wrote:
>> +		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseFX_Full_BIT,

>> +				 phydev->supported);

>> +		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseFX_Half_BIT,

>> +				 phydev->supported);

>> +

>> +		/* Auto neg is not supported in 100base FX mode */

> Hi Dan

>

> If it does not support auto neg, how do you decide to do half duplex?

> I don't see any code here which allows the user to configure it.


Ethtool has the provisions to set the duplex and speed right?.

The only call back I see which is valid is config_aneg which would still 
require a user space tool to set the needed link modes.

I could implement the config_aneg to call genphy_setup_forced if auto 
neg is disabled but that function just writes the BMCR which is already 
updated and if auto neg is enabled it would just call 
genphy_check_and_restart_aneg.

I verified the ethtool path with the DP83822 by reading the BMCR and 
ethtool displayed the correct advertisement

root@am335x-evm:~# ethtool -s eth0 speed 100 duplex full
root@am335x-evm:~# ethtool eth0
Settings for eth0:
         Supported ports: [ TP MII FIBRE ]
         Supported link modes:   10baseT/Half 10baseT/Full
                                 100baseT/Half 100baseT/Full
         Supported pause frame use: Symmetric Receive-only
         Supports auto-negotiation: No
         Supported FEC modes: Not reported
         Advertised link modes:  100baseT/Full

<snip>

root@am335x-evm:~# ethtool -s eth0 speed 10 duplex half
root@am335x-evm:~# ethtool eth0
Settings for eth0:
         Supported ports: [ TP MII FIBRE ]
         Supported link modes:   10baseT/Half 10baseT/Full
                                 100baseT/Half 100baseT/Full
         Supported pause frame use: Symmetric Receive-only
         Supports auto-negotiation: No
         Supported FEC modes: Not reported
         Advertised link modes:  10baseT/Half

root@am335x-evm:~# ./mdio-test g eth0 0
0x0000
root@am335x-evm:~# ethtool -s eth0 speed 100 duplex full
root@am335x-evm:~# ./mdio-test g eth0 0
0x2100
root@am335x-evm:~# ethtool -s eth0 speed 10 duplex half
root@am335x-evm:~# ./mdio-test g eth0 0
0x0000
root@am335x-evm:~# ethtool -s eth0 speed 10 duplex full
root@am335x-evm:~# ./mdio-test g eth0 0
0x0100
root@am335x-evm:~# ethtool eth0
Settings for eth0:
         Supported ports: [ TP MII FIBRE ]
         Supported link modes:   10baseT/Half 10baseT/Full
                                 100baseT/Half 100baseT/Full
         Supported pause frame use: Symmetric Receive-only
         Supports auto-negotiation: No
         Supported FEC modes: Not reported
         Advertised link modes:  10baseT/Full

Dan
Andrew Lunn Sept. 16, 2020, 10:13 p.m. UTC | #3 
On Wed, Sep 16, 2020 at 03:54:34PM -0500, Dan Murphy wrote:
> Andrew
> 
> On 9/15/20 3:17 PM, Andrew Lunn wrote:
> > > +		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseFX_Full_BIT,
> > > +				 phydev->supported);
> > > +		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseFX_Half_BIT,
> > > +				 phydev->supported);
> > > +
> > > +		/* Auto neg is not supported in 100base FX mode */
> > Hi Dan
> > 
> > If it does not support auto neg, how do you decide to do half duplex?
> > I don't see any code here which allows the user to configure it.
> 
> Ethtool has the provisions to set the duplex and speed right?.

What i'm getting at is you say you support
ETHTOOL_LINK_MODE_100baseFX_Full_BIT &
ETHTOOL_LINK_MODE_100baseFX_Half_BIT. If there is no auto neg in FX
mode, i'm questioning how these two different modes code be used? I'm
guessing the PHY defaults to ETHTOOL_LINK_MODE_100baseFX_Full_BIT? How
does the user set it to ETHTOOL_LINK_MODE_100baseFX_Half_BIT?

> The only call back I see which is valid is config_aneg which would still
> require a user space tool to set the needed link modes.

Correct. Maybe all you need to do is point me at the code in the
driver which actually sets the PHY into half duplex in FX mode when
the user asks for it. Is it just clearing BMCR_FULLDPLX?

    Andrew
Dan Murphy Sept. 17, 2020, 2:57 p.m. UTC | #4 
Andrew

On 9/16/20 5:13 PM, Andrew Lunn wrote:
> On Wed, Sep 16, 2020 at 03:54:34PM -0500, Dan Murphy wrote:
>> Andrew
>>
>> On 9/15/20 3:17 PM, Andrew Lunn wrote:
>>>> +		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseFX_Full_BIT,
>>>> +				 phydev->supported);
>>>> +		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseFX_Half_BIT,
>>>> +				 phydev->supported);
>>>> +
>>>> +		/* Auto neg is not supported in 100base FX mode */
>>> Hi Dan
>>>
>>> If it does not support auto neg, how do you decide to do half duplex?
>>> I don't see any code here which allows the user to configure it.
>> Ethtool has the provisions to set the duplex and speed right?.
> What i'm getting at is you say you support
> ETHTOOL_LINK_MODE_100baseFX_Full_BIT &
> ETHTOOL_LINK_MODE_100baseFX_Half_BIT. If there is no auto neg in FX
> mode, i'm questioning how these two different modes code be used? I'm
> guessing the PHY defaults to ETHTOOL_LINK_MODE_100baseFX_Full_BIT? How
> does the user set it to ETHTOOL_LINK_MODE_100baseFX_Half_BIT?

The user can use ethtool to set the speed and duplex. And ethtool uses 
the IOCTLs to configure the device.

So if the user creates their own HAL then they can use those IOCTLs as well.

The data sheet indicates

"In fiber mode, the speed is not
decided through auto-negotiation. Both sides of the link must be 
configured to the same operating speed."

>
>> The only call back I see which is valid is config_aneg which would still
>> require a user space tool to set the needed link modes.
> Correct. Maybe all you need to do is point me at the code in the
> driver which actually sets the PHY into half duplex in FX mode when
> the user asks for it. Is it just clearing BMCR_FULLDPLX?

Here is the full flow when setting the speed and duplex mode from the 
Ethtool or when the IOCTL's are called to update the PHY

phy_ethtool_ksettings_set updates the phydev->speed and phydev->duplex

Since Auto Neg is disabled the call to genphy_setup_forced is done in 
the __genphy_config_aneg in phy_device.

genphy_setup_forced updates the BMCR with the updated values.

So IMO there is no need to populate the config_aneg call back to

root@am335x-evm:~# ./ethtool -s eth0 speed 10 duplex half
[   92.098491] phy_ethtool_ksettings_set
[   92.102247] phy_ethtool_ksettings_set: speed 10 duplex 0
[   92.107755] phy_sanitize_settings
[   92.111085] phy_config_aneg
[   92.113930] genphy_config_aneg
[   92.116997] __genphy_config_aneg
[   92.120237] genphy_setup_forced
[   92.123419] genphy_setup_forced: Update the BMCR
root@am335x-evm:~# ./ethtool -s eth0 speed 100 duplex full
[  102.693105] phy_ethtool_ksettings_set
[  102.697029] phy_ethtool_ksettings_set: speed 100 duplex 1
[  102.702462] phy_sanitize_settings
[  102.705892] phy_config_aneg
[  102.708702] genphy_config_aneg
[  102.711770] __genphy_config_aneg
[  102.715051] genphy_setup_forced
[  102.718209] genphy_setup_forced: Update the BMCR

I am hoping this answers your question.

Dan
diff mbox series

Patch

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index 6b98d74b5102..81899bc99add 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -52,6 +52,10 @@ 
 					 BMCR_FULLDPLX | \
 					 BMCR_SPEED1000)
 
+#define MII_DP83869_FIBER_ADVERTISE    (ADVERTISED_FIBRE | \
+					ADVERTISED_Pause | \
+					ADVERTISED_Asym_Pause)
+
 /* This is the same bit mask as the BMCR so re-use the BMCR default */
 #define DP83869_FX_CTRL_DEFAULT	MII_DP83869_BMCR_DEFAULT
 
@@ -118,6 +122,28 @@  struct dp83869_private {
 	int mode;
 };
 
+static int dp83869_read_status(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	int ret;
+
+	ret = genphy_read_status(phydev);
+	if (ret)
+		return ret;
+
+	if (linkmode_test_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->supported)) {
+		if (phydev->link) {
+			if (dp83869->mode == DP83869_RGMII_100_BASE)
+				phydev->speed = SPEED_100;
+		} else {
+			phydev->speed = SPEED_UNKNOWN;
+			phydev->duplex = DUPLEX_UNKNOWN;
+		}
+	}
+
+	return 0;
+}
+
 static int dp83869_ack_interrupt(struct phy_device *phydev)
 {
 	int err = phy_read(phydev, MII_DP83869_ISR);
@@ -295,6 +321,51 @@  static int dp83869_configure_rgmii(struct phy_device *phydev,
 	return ret;
 }
 
+static int dp83869_configure_fiber(struct phy_device *phydev,
+				   struct dp83869_private *dp83869)
+{
+	int bmcr;
+	int ret;
+
+	/* Only allow advertising what this PHY supports */
+	linkmode_and(phydev->advertising, phydev->advertising,
+		     phydev->supported);
+
+	linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT, phydev->supported);
+	linkmode_set_bit(ADVERTISED_FIBRE, phydev->advertising);
+
+	if (dp83869->mode == DP83869_RGMII_1000_BASE) {
+		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+				 phydev->supported);
+	} else {
+		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseFX_Full_BIT,
+				 phydev->supported);
+		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseFX_Half_BIT,
+				 phydev->supported);
+
+		/* Auto neg is not supported in 100base FX mode */
+		bmcr = phy_read(phydev, MII_BMCR);
+		if (bmcr < 0)
+			return bmcr;
+
+		phydev->autoneg = AUTONEG_DISABLE;
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);
+		linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->advertising);
+
+		if (bmcr & BMCR_ANENABLE) {
+			ret =  phy_modify(phydev, MII_BMCR, BMCR_ANENABLE, 0);
+			if (ret < 0)
+				return ret;
+		}
+	}
+
+	/* Update advertising from supported */
+	linkmode_or(phydev->advertising, phydev->advertising,
+		    phydev->supported);
+
+	return 0;
+}
+
 static int dp83869_configure_mode(struct phy_device *phydev,
 				  struct dp83869_private *dp83869)
 {
@@ -384,6 +455,7 @@  static int dp83869_configure_mode(struct phy_device *phydev,
 		break;
 	case DP83869_RGMII_1000_BASE:
 	case DP83869_RGMII_100_BASE:
+		ret = dp83869_configure_fiber(phydev, dp83869);
 		break;
 	default:
 		return -EINVAL;
@@ -494,6 +566,7 @@  static struct phy_driver dp83869_driver[] = {
 		/* IRQ related */
 		.ack_interrupt	= dp83869_ack_interrupt,
 		.config_intr	= dp83869_config_intr,
+		.read_status	= dp83869_read_status,
 
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,