Re: [PATCH] lan743x: Added fixed link support

[Florian Fainelli]

On 5/13/2020 12:06 PM, Roelof Berg wrote:
> Microchip lan7431 is frequently connected to a phy. However, it
> can also be directly connected to a MII remote peer without
> any phy in between. For supporting such a phyless hardware setup
> in Linux we added the capability to the driver to understand
> the fixed-link and the phy-connection-type entries in the device
> tree.
> 
> If a fixed-link node is configured in the device tree the lan7431
> device will deactivate auto negotiation and uses the speed and
> duplex settings configured in the fixed-link node.
> 
> Also the phy-connection-type can be configured in the device tree
> and in case of a fixed-link connection the RGMII mode can be
> configured, all other modes fall back to the default: GMII.
> 
> Example:
> 
>  &pcie {
> 	status = "okay";
> 
> 	host@0 {
> 		reg = <0 0 0 0 0>;
> 
> 		#address-cells = <3>;
> 		#size-cells = <2>;
> 
> 		ethernet@0 {
> 			compatible = "weyland-yutani,noscom1", "microchip,lan743x";
> 			status = "okay";
> 			reg = <0 0 0 0 0>;
> 			phy-connection-type = "rgmii";
> 
> 			fixed-link {
> 				speed = <100>;
> 				full-duplex;
> 			};
> 		};
> 	};
> };
> 
> Signed-off-by: Roelof Berg <rberg@xxxxxxxxxxxxxxxxx>
> ---
>  drivers/net/ethernet/microchip/lan743x_main.c | 94 +++++++++++++++++--
>  drivers/net/ethernet/microchip/lan743x_main.h |  4 +
>  2 files changed, 89 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index a43140f7b5eb..85f12881340b 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -9,9 +9,12 @@
>  #include <linux/microchipphy.h>
>  #include <linux/net_tstamp.h>
>  #include <linux/phy.h>
> +#include <linux/phy_fixed.h>
>  #include <linux/rtnetlink.h>
>  #include <linux/iopoll.h>
>  #include <linux/crc16.h>
> +#include <linux/of_mdio.h>
> +#include <linux/of_net.h>
>  #include "lan743x_main.h"
>  #include "lan743x_ethtool.h"
>  
> @@ -974,6 +977,8 @@ static void lan743x_phy_close(struct lan743x_adapter *adapter)
>  
>  	phy_stop(netdev->phydev);
>  	phy_disconnect(netdev->phydev);
> +	if (of_phy_is_fixed_link(adapter->pdev->dev.of_node))
> +		of_phy_deregister_fixed_link(adapter->pdev->dev.of_node);
>  	netdev->phydev = NULL;
>  }
>  
> @@ -982,18 +987,86 @@ static int lan743x_phy_open(struct lan743x_adapter *adapter)
>  	struct lan743x_phy *phy = &adapter->phy;
>  	struct phy_device *phydev;
>  	struct net_device *netdev;
> +	struct device_node *phynode;
> +	u32 data;
> +	phy_interface_t phyifc = PHY_INTERFACE_MODE_GMII;
> +	bool fixed_link = false;
>  	int ret = -EIO;
>  
>  	netdev = adapter->netdev;
> -	phydev = phy_find_first(adapter->mdiobus);
> -	if (!phydev)
> -		goto return_error;
> +	phynode = of_node_get(adapter->pdev->dev.of_node);
> +	if (phynode)
> +		of_get_phy_mode(phynode, &phyifc);
> +
> +	/* check if a fixed-link is defined in device-tree */
> +	if (phynode && of_phy_is_fixed_link(phynode)) {
> +		fixed_link = true;
> +		netdev_dbg(netdev, "fixed-link detected\n");
> +
> +		ret = of_phy_register_fixed_link(phynode);
> +		if (ret) {
> +			netdev_err(netdev, "cannot register fixed PHY\n");
> +			goto return_error;
> +		}
>  
> -	ret = phy_connect_direct(netdev, phydev,
> -				 lan743x_phy_link_status_change,
> -				 PHY_INTERFACE_MODE_GMII);
> -	if (ret)
> -		goto return_error;
> +		phydev = of_phy_connect(netdev, phynode,
> +					lan743x_phy_link_status_change,
> +					0, phyifc);
> +		if (!phydev)
> +			goto return_error;
> +
> +		/* Configure MAC to fixed link parameters */
> +		data = lan743x_csr_read(adapter, MAC_CR);
> +		/* Disable auto negotiation */
> +		data &= ~(MAC_CR_ADD_ | MAC_CR_ASD_);
> +		/* Set duplex mode */
> +		if (phydev->duplex)
> +			data |= MAC_CR_DPX_;
> +		else
> +			data &= ~MAC_CR_DPX_;
> +		/* Set bus speed */
> +		switch (phydev->speed) {
> +		case 10:
> +			data &= ~MAC_CR_CFG_H_;
> +			data &= ~MAC_CR_CFG_L_;
> +			break;
> +		case 100:
> +			data &= ~MAC_CR_CFG_H_;
> +			data |= MAC_CR_CFG_L_;
> +			break;
> +		case 1000:
> +			data |= MAC_CR_CFG_H_;
> +			data |= MAC_CR_CFG_L_;
> +			break;
> +		}
> +		/* Set interface mode */
> +		if (phyifc == PHY_INTERFACE_MODE_RGMII ||
> +		    phyifc == PHY_INTERFACE_MODE_RGMII_ID ||
> +		    phyifc == PHY_INTERFACE_MODE_RGMII_RXID ||
> +		    phyifc == PHY_INTERFACE_MODE_RGMII_TXID)
> +			/* RGMII */
> +			data &= ~MAC_CR_MII_EN_;
> +		else
> +			/* GMII */
> +			data |= MAC_CR_MII_EN_;
> +		lan743x_csr_write(adapter, MAC_CR, data);

All of this should be done in your adjust_link callback, I believe you
did it here because you made phy_start_aneg() conditional on not finding
a fixed PHY, but the point of a fixed PHY is to provide a full
emulation, including that of the state machine transitions.

> +	} else {
> +		phydev = phy_find_first(adapter->mdiobus);
> +		if (!phydev)
> +			goto return_error;
> +
> +		ret = phy_connect_direct(netdev, phydev,
> +					 lan743x_phy_link_status_change,
> +					 PHY_INTERFACE_MODE_GMII);
> +		/* Note: We cannot use phyifc here because this would be SGMII
> +		 * on a standard PC.
> +		 */
> +		if (ret)
> +			goto return_error;
> +	}
> +
> +	if (phynode)
> +		of_node_put(phynode);
>  
>  	/* MAC doesn't support 1000T Half */
>  	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> @@ -1004,10 +1077,13 @@ static int lan743x_phy_open(struct lan743x_adapter *adapter)
>  	phy->fc_autoneg = phydev->autoneg;
>  
>  	phy_start(phydev);
> -	phy_start_aneg(phydev);
> +	if (!fixed_link)
> +		phy_start_aneg(phydev);

phy_start() should already trigger an auto-negotiation restart if
necessary, so this calls seems to be redundant if nothing else. If
someone it must be kept, it should not be made conditional on the fixed
link.
-- 
Florian