mbox series

[net-next,v2,0/2] DP83822 Fiber enablement

Message ID 20200710143733.30751-1-dmurphy@ti.com
Headers show
Series DP83822 Fiber enablement | expand

Message

Dan Murphy July 10, 2020, 2:37 p.m. UTC 
Hello

The DP83822 Ethernet PHY has the ability to connect via a Fiber port.  The
derivative PHYs DP83825 and DP83826 do not have this ability. In fiber mode
the DP83822 disables auto negotiation and has a fixed 100Mbps speed with
support for full or half duplex modes.

A devicetree binding was added to set the signal polarity for the fiber
connection.  This property is only applicable if the FX_EN strap is set in
hardware other wise the signal loss detection is disabled on the PHY.

If the FX_EN is not strapped the device can be configured to run in fiber mode
via the device tree. All be it the PHY will not perfomr signal loss detection.

Dan

Dan Murphy (2):
  dt-bindings: net: dp83822: Add TI dp83822 phy
  net: phy: DP83822: Add ability to advertise Fiber connection

 .../devicetree/bindings/net/ti,dp83822.yaml   |  80 +++++++++
 drivers/net/phy/dp83822.c                     | 161 ++++++++++++++++++
 2 files changed, 241 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ti,dp83822.yaml

Comments

Andrew Lunn July 11, 2020, 6:45 p.m. UTC | #1 
> +#define MII_DP83822_FIBER_ADVERTISE	(SUPPORTED_AUI | SUPPORTED_FIBRE | \
> +					 SUPPORTED_BNC | SUPPORTED_Pause | \
> +					 SUPPORTED_Asym_Pause | \
> +					 SUPPORTED_100baseT_Full)
> +
> +		/* Setup fiber advertisement */
> +		err = phy_modify_changed(phydev, MII_ADVERTISE,
> +					 ADVERTISE_1000XFULL |
> +					 ADVERTISE_1000XPAUSE |
> +					 ADVERTISE_1000XPSE_ASYM,
> +					 MII_DP83822_FIBER_ADVERTISE);

That looks very odd. SUPPORTED_AUI #define has nothing to do with
MII_ADVERTISE register. It is not a bit you can read/write in that
register.

	Andrew
Andrew Lunn July 11, 2020, 6:54 p.m. UTC | #2 
> @@ -302,6 +357,48 @@ static int dp83822_config_init(struct phy_device *phydev)
>  		}
>  	}
>  
> +	if (dp83822->fx_enabled) {
> +		err = phy_modify(phydev, MII_DP83822_CTRL_2,
> +				 DP83822_FX_ENABLE, 1);
> +		if (err < 0)
> +			return err;
> +
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT,
> +				 phydev->supported);
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT,
> +				 phydev->advertising);
> +
> +		/* Auto neg is not supported in fiber mode */
> +		bmcr = phy_read(phydev, MII_BMCR);
> +		if (bmcr < 0)
> +			return bmcr;
> +
> +		if (bmcr & BMCR_ANENABLE) {
> +			err =  phy_modify(phydev, MII_BMCR, BMCR_ANENABLE, 0);
> +			if (err < 0)
> +				return err;
> +		}
> +		phydev->autoneg = AUTONEG_DISABLE;

You should also be removing ETHTOOL_LINK_MODE_Autoneg_BIT from
phydev->supported, to make it clear autoneg is not supported. Assuming
genphy_read_abilities() cannot figure this out for itself.

			Andrew
Dan Murphy July 13, 2020, 3:50 p.m. UTC | #3 
Andrew

On 7/11/20 1:54 PM, Andrew Lunn wrote:
>> @@ -302,6 +357,48 @@ static int dp83822_config_init(struct phy_device *phydev)
>>   		}
>>   	}
>>   
>> +	if (dp83822->fx_enabled) {
>> +		err = phy_modify(phydev, MII_DP83822_CTRL_2,
>> +				 DP83822_FX_ENABLE, 1);
>> +		if (err < 0)
>> +			return err;
>> +
>> +		linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT,
>> +				 phydev->supported);
>> +		linkmode_set_bit(ETHTOOL_LINK_MODE_FIBRE_BIT,
>> +				 phydev->advertising);
>> +
>> +		/* Auto neg is not supported in fiber mode */
>> +		bmcr = phy_read(phydev, MII_BMCR);
>> +		if (bmcr < 0)
>> +			return bmcr;
>> +
>> +		if (bmcr & BMCR_ANENABLE) {
>> +			err =  phy_modify(phydev, MII_BMCR, BMCR_ANENABLE, 0);
>> +			if (err < 0)
>> +				return err;
>> +		}
>> +		phydev->autoneg = AUTONEG_DISABLE;
> You should also be removing ETHTOOL_LINK_MODE_Autoneg_BIT from
> phydev->supported, to make it clear autoneg is not supported. Assuming
> genphy_read_abilities() cannot figure this out for itself.

In our testing we are finding that it cannot determine that for itself 
so I will have to clear the bit.

Dan
Dan Murphy July 13, 2020, 3:51 p.m. UTC | #4 
Andrew

On 7/11/20 1:45 PM, Andrew Lunn wrote:
>> +#define MII_DP83822_FIBER_ADVERTISE	(SUPPORTED_AUI | SUPPORTED_FIBRE | \
>> +					 SUPPORTED_BNC | SUPPORTED_Pause | \
>> +					 SUPPORTED_Asym_Pause | \
>> +					 SUPPORTED_100baseT_Full)
>> +
>> +		/* Setup fiber advertisement */
>> +		err = phy_modify_changed(phydev, MII_ADVERTISE,
>> +					 ADVERTISE_1000XFULL |
>> +					 ADVERTISE_1000XPAUSE |
>> +					 ADVERTISE_1000XPSE_ASYM,
>> +					 MII_DP83822_FIBER_ADVERTISE);
> That looks very odd. SUPPORTED_AUI #define has nothing to do with
> MII_ADVERTISE register. It is not a bit you can read/write in that
> register.

ACK removed the SUPPORTED_AUI.

I also going to update the MII_DP83822_FIBER_ADVERTISE defines from 
SUPPORTED_* to ADVERTISED_*

Dan


> 	Andrew