linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] lan743x: Added fixed link support
@ 2020-05-20 17:10 Roelof Berg
  2020-05-21 14:04 ` Andrew Lunn
  0 siblings, 1 reply; 14+ messages in thread
From: Roelof Berg @ 2020-05-20 17:10 UTC (permalink / raw)
  To: rberg
  Cc: andrew, Bryan Whitehead, Microchip Linux Driver Support,
	David S. Miller, netdev, linux-kernel

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.

New behavior:
. If no device tree node is configured the behavior of the driver remains
  unchanged. This will ensure backwards compatibility to off-the-shelve
  PC hardware.

. If a device tree node is configured but no fixed-link node is present
  the phy-connection-type (as specified in the device tree)  will be
  applied and of_phy_connect() will be used instead of
  phy_connect_direct().

. If a device tree node is configured and a fixed-link node is present
  the phy-connection-type will be applied, of_phy_connect() will be
  used and the MAC will be configured to use the fix speed and duplex
  mode configured in the fixed-link node. Also ASD and ADD will be
  switched off.

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@berg-solutions.de>
---
 drivers/net/ethernet/microchip/lan743x_main.c | 101 ++++++++++++++++--
 drivers/net/ethernet/microchip/lan743x_main.h |   4 +
 2 files changed, 97 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index a43140f7b5eb..50ca223e8b8f 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"
 
@@ -946,6 +949,10 @@ static void lan743x_phy_link_status_change(struct net_device *netdev)
 {
 	struct lan743x_adapter *adapter = netdev_priv(netdev);
 	struct phy_device *phydev = netdev->phydev;
+	struct device_node *phynode;
+	phy_interface_t phyifc;
+	int ret = -EIO;
+	u32 data;
 
 	phy_print_status(phydev);
 	if (phydev->state == PHY_RUNNING) {
@@ -953,6 +960,53 @@ static void lan743x_phy_link_status_change(struct net_device *netdev)
 		int remote_advertisement = 0;
 		int local_advertisement = 0;
 
+		/* check if a device-tree configuration is present */
+		phynode = of_node_get(adapter->pdev->dev.of_node);
+		if (phynode) {
+			data = lan743x_csr_read(adapter, MAC_CR);
+
+			/* set interface mode */
+			ret = of_get_phy_mode(phynode, &phyifc);
+			if (ret)
+				phyifc = PHY_INTERFACE_MODE_GMII;
+
+			if (phy_interface_mode_is_rgmii(phyifc))
+				/* RGMII */
+				data &= ~MAC_CR_MII_EN_;
+			else
+				/* GMII */
+				data |= MAC_CR_MII_EN_;
+
+			/* apply fixed-link configuration */
+			if (of_phy_is_fixed_link(phynode)) {
+				/* disable auto duplex, and speed detection */
+				data &= ~(MAC_CR_ADD_ | MAC_CR_ASD_);
+				/* set fixed duplex mode */
+				if (phydev->duplex)
+					data |= MAC_CR_DPX_;
+				else
+					data &= ~MAC_CR_DPX_;
+				/* set fixed 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;
+				}
+			}
+
+			lan743x_csr_write(adapter, MAC_CR, data);
+			of_node_put(phynode);
+		}
+
 		memset(&ksettings, 0, sizeof(ksettings));
 		phy_ethtool_get_link_ksettings(netdev, &ksettings);
 		local_advertisement =
@@ -974,26 +1028,57 @@ 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;
 }
 
 static int lan743x_phy_open(struct lan743x_adapter *adapter)
 {
 	struct lan743x_phy *phy = &adapter->phy;
+	struct device_node *phynode;
 	struct phy_device *phydev;
 	struct net_device *netdev;
+	phy_interface_t phyifc;
 	int ret = -EIO;
 
 	netdev = adapter->netdev;
-	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);
-	if (ret)
-		goto return_error;
+	/* is a device tree configuration present */
+	phynode = of_node_get(adapter->pdev->dev.of_node);
+	if (phynode) {
+		/* apply device tree configuration */
+		if (of_phy_is_fixed_link(phynode)) {
+			ret = of_phy_register_fixed_link(phynode);
+			if (ret) {
+				netdev_err(netdev, "cannot register fixed PHY\n");
+				of_node_put(phynode);
+				goto return_error;
+			}
+		}
+
+		ret = of_get_phy_mode(phynode, &phyifc);
+		if (ret)
+			phyifc = PHY_INTERFACE_MODE_GMII;
+
+		phydev = of_phy_connect(netdev, phynode,
+					lan743x_phy_link_status_change,
+					0, phyifc);
+		of_node_put(phynode);
+		if (!phydev)
+			goto return_error;
+	} else {
+		/* no device tree configuration */
+		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);
+		if (ret)
+			goto return_error;
+	}
 
 	/* MAC doesn't support 1000T Half */
 	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 3b02eeae5f45..e49f6b6cd440 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -104,10 +104,14 @@
 	((value << 0) & FCT_FLOW_CTL_ON_THRESHOLD_)
 
 #define MAC_CR				(0x100)
+#define MAC_CR_MII_EN_			BIT(19)
 #define MAC_CR_EEE_EN_			BIT(17)
 #define MAC_CR_ADD_			BIT(12)
 #define MAC_CR_ASD_			BIT(11)
 #define MAC_CR_CNTR_RST_		BIT(5)
+#define MAC_CR_DPX_			BIT(3)
+#define MAC_CR_CFG_H_			BIT(2)
+#define MAC_CR_CFG_L_			BIT(1)
 #define MAC_CR_RST_			BIT(0)
 
 #define MAC_RX				(0x104)
-- 
2.25.1


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

* Re: [PATCH] lan743x: Added fixed link support
  2020-05-20 17:10 [PATCH] lan743x: Added fixed link support Roelof Berg
@ 2020-05-21 14:04 ` Andrew Lunn
  0 siblings, 0 replies; 14+ messages in thread
From: Andrew Lunn @ 2020-05-21 14:04 UTC (permalink / raw)
  To: Roelof Berg
  Cc: Bryan Whitehead, Microchip Linux Driver Support, David S. Miller,
	netdev, linux-kernel

On Wed, May 20, 2020 at 07:10:06PM +0200, Roelof Berg wrote:

Hi Roelof

Here is how i would do this. I don't like this MII bus snooping. It is
a microchip propriety's thing, which is not well understood. It adds
no value over just doing what every other MAC driver does in the
link_change callback, which lots of people understand.

I also removed all OF handing from within the callback. And i unified
fixed-link and normal PHY.

This is compile tested only, so is probably broken...

     Andrew


diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index 3a0b289d9771..c533d06fbe3a 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -2,11 +2,11 @@
 /* Copyright (C) 2018 Microchip Technology Inc. */
 
 #include <linux/netdevice.h>
-#include "lan743x_main.h"
-#include "lan743x_ethtool.h"
 #include <linux/net_tstamp.h>
 #include <linux/pci.h>
 #include <linux/phy.h>
+#include "lan743x_main.h"
+#include "lan743x_ethtool.h"
 
 /* eeprom */
 #define LAN743X_EEPROM_MAGIC		    (0x74A5)
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index a43140f7b5eb..5aaa0ac96970 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -8,7 +8,10 @@
 #include <linux/crc32.h>
 #include <linux/microchipphy.h>
 #include <linux/net_tstamp.h>
+#include <linux/of_mdio.h>
+#include <linux/of_net.h>
 #include <linux/phy.h>
+#include <linux/phy_fixed.h>
 #include <linux/rtnetlink.h>
 #include <linux/iopoll.h>
 #include <linux/crc16.h>
@@ -798,9 +801,9 @@ static int lan743x_mac_init(struct lan743x_adapter *adapter)
 
 	netdev = adapter->netdev;
 
-	/* setup auto duplex, and speed detection */
+	/* disable auto duplex, and speed detection. Phylib does that */
 	data = lan743x_csr_read(adapter, MAC_CR);
-	data |= MAC_CR_ADD_ | MAC_CR_ASD_;
+	data &= ~(MAC_CR_ADD_ | MAC_CR_ASD_);
 	data |= MAC_CR_CNTR_RST_;
 	lan743x_csr_write(adapter, MAC_CR, data);
 
@@ -946,6 +949,7 @@ static void lan743x_phy_link_status_change(struct net_device *netdev)
 {
 	struct lan743x_adapter *adapter = netdev_priv(netdev);
 	struct phy_device *phydev = netdev->phydev;
+	u32 data;
 
 	phy_print_status(phydev);
 	if (phydev->state == PHY_RUNNING) {
@@ -953,6 +957,39 @@ static void lan743x_phy_link_status_change(struct net_device *netdev)
 		int remote_advertisement = 0;
 		int local_advertisement = 0;
 
+		data = lan743x_csr_read(adapter, MAC_CR);
+
+		/* set interface mode */
+		if (phy_interface_mode_is_rgmii(adapter->phy_mode))
+			/* RGMII */
+			data &= ~MAC_CR_MII_EN_;
+		else
+			/* GMII */
+			data |= MAC_CR_MII_EN_;
+
+		/* set fixed duplex mode */
+		if (phydev->duplex)
+			data |= MAC_CR_DPX_;
+		else
+			data &= ~MAC_CR_DPX_;
+
+		/* set fixed bus speed */
+		switch (phydev->speed) {
+		case SPEED_10:
+			data &= ~MAC_CR_CFG_H_;
+			data &= ~MAC_CR_CFG_L_;
+			break;
+		case SPEED_100:
+			data &= ~MAC_CR_CFG_H_;
+			data |= MAC_CR_CFG_L_;
+			break;
+		case SPEED_1000:
+			data |= MAC_CR_CFG_H_;
+			data |= MAC_CR_CFG_L_;
+			break;
+		}
+		lan743x_csr_write(adapter, MAC_CR, data);
+
 		memset(&ksettings, 0, sizeof(ksettings));
 		phy_ethtool_get_link_ksettings(netdev, &ksettings);
 		local_advertisement =
@@ -974,26 +1011,53 @@ 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;
 }
 
 static int lan743x_phy_open(struct lan743x_adapter *adapter)
 {
 	struct lan743x_phy *phy = &adapter->phy;
+	struct device_node *phynode;
 	struct phy_device *phydev;
 	struct net_device *netdev;
 	int ret = -EIO;
 
 	netdev = adapter->netdev;
-	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);
-	if (ret)
-		goto return_error;
+	phynode = of_node_get(adapter->pdev->dev.of_node);
+	adapter->phy_mode = PHY_INTERFACE_MODE_GMII;
+
+	if (phynode) {
+		of_get_phy_mode(phynode, &adapter->phy_mode);
+
+		if (of_phy_is_fixed_link(phynode)) {
+			ret = of_phy_register_fixed_link(phynode);
+			if (ret) {
+				netdev_err(netdev,
+					   "cannot register fixed PHY\n");
+				of_node_put(phynode);
+				goto return_error;
+			}
+		}
+		phydev = of_phy_connect(netdev, phynode,
+					lan743x_phy_link_status_change, 0,
+					adapter->phy_mode);
+		of_node_put(phynode);
+		if (!phydev)
+			goto return_error;
+	} else {
+		phydev = phy_find_first(adapter->mdiobus);
+		if (!phydev)
+			goto return_error;
+
+		ret = phy_connect_direct(netdev, phydev,
+					 lan743x_phy_link_status_change,
+					 adapter->phy_mode);
+		if (ret)
+			goto return_error;
+	}
 
 	/* MAC doesn't support 1000T Half */
 	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 3b02eeae5f45..f769903538e4 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -104,10 +104,14 @@
 	((value << 0) & FCT_FLOW_CTL_ON_THRESHOLD_)
 
 #define MAC_CR				(0x100)
+#define MAC_CR_MII_EN_			BIT(19)
 #define MAC_CR_EEE_EN_			BIT(17)
 #define MAC_CR_ADD_			BIT(12)
 #define MAC_CR_ASD_			BIT(11)
 #define MAC_CR_CNTR_RST_		BIT(5)
+#define MAC_CR_DPX_			BIT(3)
+#define MAC_CR_CFG_H_			BIT(2)
+#define MAC_CR_CFG_L_			BIT(1)
 #define MAC_CR_RST_			BIT(0)
 
 #define MAC_RX				(0x104)
@@ -698,6 +702,7 @@ struct lan743x_rx {
 struct lan743x_adapter {
 	struct net_device       *netdev;
 	struct mii_bus		*mdiobus;
+	phy_interface_t		phy_mode;
 	int                     msg_enable;
 #ifdef CONFIG_PM
 	u32			wolopts;
diff --git a/drivers/net/ethernet/microchip/lan743x_ptp.c b/drivers/net/ethernet/microchip/lan743x_ptp.c
index 9399f6a98748..b1df214a6973 100644
--- a/drivers/net/ethernet/microchip/lan743x_ptp.c
+++ b/drivers/net/ethernet/microchip/lan743x_ptp.c
@@ -2,12 +2,12 @@
 /* Copyright (C) 2018 Microchip Technology Inc. */
 
 #include <linux/netdevice.h>
-#include "lan743x_main.h"
-
+#include <linux/phy.h>
 #include <linux/ptp_clock_kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/net_tstamp.h>
+#include "lan743x_main.h"
 
 #include "lan743x_ptp.h"
 

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

* RE: [PATCH] lan743x: Added fixed link support
  2020-05-19 16:42           ` Roelof Berg
@ 2020-05-19 22:06             ` Ronnie.Kunin
  0 siblings, 0 replies; 14+ messages in thread
From: Ronnie.Kunin @ 2020-05-19 22:06 UTC (permalink / raw)
  To: rberg, andrew
  Cc: Bryan.Whitehead, UNGLinuxDriver, davem, netdev, linux-kernel

Hi Roelof,

You are correct that "Auto-detection" from the MAC_CR does not have anything to do with the PHY Auto-negotiation or with access to the PHY's registers being done by the MAC over MDIO. Auto-detection just a way for the MAC to automatically set it's speed and duplex mode, determined by ***passively*** looking at standard Rx RGMII signals and at the state (high or low) of a non-standard (but often available - think of a signal driving a duplex LED coming from a real PHY) "duplex" signal. As you said when using this feature the driver/MCU does not need to do any register access (except maybe a one-time init write to ADP in MAC_CR) to keep the LAN7431 MAC in sync with whatever speed/duplex the PHY is operating at.

Regarding your conclusions:
ASD should be pretty safe to use all the time I think, because in all implementations you use a LAN7431 you will always have the standard RGMII Rx signals coming in, so the speed detection should always be accurate.
ADD is not a given will be usable in all implementations though, it relies on the existence of a signal you can input into the LAN7431 that will accurately tell it what the current duplex is (0/1<->half/full; or 0/1<->full/half  does not matter, polarity is configurable). This is not a standard signal so it may not be available.
I'd say there are three cases: 
	- If the duplex mode is permanently fixed in your design, you can use ADD: just tie the duplex pin of LAN7431 (i.e.: Keep the ADP =1 default in MAC_CR; tie the pin low if half duplex, tie the pin high if full duplex)
	- If your duplex mode can change and you have a signal like this available in your design you can use ADD, just connect that signal to the duplex pin of LAN7431 and configure the proper ADP for the signal polarity in MAC_CR
	- If your duplex mode can change and you don’t have a signal like this available in your design you cannot use ADD. 

Hope this helps.

Regards,
Ronnie

-----Original Message-----
From: Roelof Berg <rberg@berg-solutions.de> 
Sent: Tuesday, May 19, 2020 12:43 PM
To: Andrew Lunn <andrew@lunn.ch>
Cc: Bryan Whitehead - C21958 <Bryan.Whitehead@microchip.com>; UNGLinuxDriver <UNGLinuxDriver@microchip.com>; David S. Miller <davem@davemloft.net>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] lan743x: Added fixed link support

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Hi Andrew,

thank you for the example, your input got me further. Sorry if my e-mails made the impression that the MAC is sending MDIO on its own. It can issue MDIO but I assume it will do this only on request of the MCU.

I read the data sheets again and found what might have confused us. There is:
a) Auto Negotiation (Phy-Phy)
b) Automatic Speed Detection, ASD (Mac-Phy)
c) Automatic Duplex Detection, ADD (Mac-Phy)

My current hypothesis is: When Phy-Phy auto negotiation is done, the ASD and ADD of the MAC will implicitly catch up the new mode of the Phy on a low level (clocks, pins). A dumb silicon would need the MCU to re-configure the MAC after MDIO told the MCU about a change in the Phy mode. But this ultra smart silicon would neither need MDIO, nor an MCU to understand what’s going on on the busses :)

If this hypothesis is correct, I should change in the driver all comments that mention „auto negoriation“ to „ADD, ASD“, and future readers will not be confused anymore.

Conclusion:
- Maybe I can leave ASD and ADD even active in fixed-link scenarios, when in the device tree an empty fixed-link node is present.
- And I need to disable ASD and/or ADD only if speed and/or duplex is configured inside the fixed-link mode.

I need to verify this hypothesis.

Thank you for reviewing and sharing topics we need to consider, Roelof

> Am 18.05.2020 um 22:34 schrieb Andrew Lunn <andrew@lunn.ch>:
>
>> I double checked the vendor documentation and according to the data 
>> sheet in this device the MAC detects speed and duplex mode. It uses 
>> PINs, traces clocks … Also according to an application note of the 
>> vendor duplex and speed detection should be enabled in the MAC 
>> registers.
>
> In general, the MAC should not perform MDIO requests on the PHY.  The 
> MAC has no access to the mutex which phylib users. So if the MAC 
> directly accesses registers in the PHY, it could do it at the wrong 
> time, when the PHY driver is active.
>
> This can be particularly bad when Marvell PHYs are used. They have 
> paged registers. One example is the page with the temperature sensor.
> This can be selected due to a read on the hwmon device. If the MAC 
> tried to read the speed/duplex which the temperature sensor is 
> selected, it would wrongly read the temperature sensor registers, not 
> the link state.
>
> There is no need for the MAC to directly access the PHY. It will get 
> told what the result of auto-neg is. So please turn this off all the 
> time.
>
>       Andrew
>


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

* Re: [PATCH] lan743x: Added fixed link support
  2020-05-18 20:34         ` Andrew Lunn
@ 2020-05-19 16:42           ` Roelof Berg
  2020-05-19 22:06             ` Ronnie.Kunin
  0 siblings, 1 reply; 14+ messages in thread
From: Roelof Berg @ 2020-05-19 16:42 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Bryan Whitehead, Microchip Linux Driver Support, David S. Miller,
	netdev, linux-kernel

Hi Andrew,

thank you for the example, your input got me further. Sorry if my e-mails made the impression that the MAC is sending MDIO on its own. It can issue MDIO but I assume it will do this only on request of the MCU.

I read the data sheets again and found what might have confused us. There is:
a) Auto Negotiation (Phy-Phy)
b) Automatic Speed Detection, ASD (Mac-Phy)
c) Automatic Duplex Detection, ADD (Mac-Phy)

My current hypothesis is: When Phy-Phy auto negotiation is done, the ASD and ADD of the MAC will implicitly catch up the new mode of the Phy on a low level (clocks, pins). A dumb silicon would need the MCU to re-configure the MAC after MDIO told the MCU about a change in the Phy mode. But this ultra smart silicon would neither need MDIO, nor an MCU to understand what’s going on on the busses :)

If this hypothesis is correct, I should change in the driver all comments that mention „auto negoriation“ to „ADD, ASD“, and future readers will not be confused anymore.

Conclusion:
- Maybe I can leave ASD and ADD even active in fixed-link scenarios, when in the device tree an empty fixed-link node is present.
- And I need to disable ASD and/or ADD only if speed and/or duplex is configured inside the fixed-link mode.

I need to verify this hypothesis.

Thank you for reviewing and sharing topics we need to consider,
Roelof

> Am 18.05.2020 um 22:34 schrieb Andrew Lunn <andrew@lunn.ch>:
> 
>> I double checked the vendor documentation and according to the data
>> sheet in this device the MAC detects speed and duplex mode. It uses
>> PINs, traces clocks … Also according to an application note of the
>> vendor duplex and speed detection should be enabled in the MAC
>> registers.
> 
> In general, the MAC should not perform MDIO requests on the PHY.  The
> MAC has no access to the mutex which phylib users. So if the MAC
> directly accesses registers in the PHY, it could do it at the wrong
> time, when the PHY driver is active.
> 
> This can be particularly bad when Marvell PHYs are used. They have
> paged registers. One example is the page with the temperature sensor.
> This can be selected due to a read on the hwmon device. If the MAC
> tried to read the speed/duplex which the temperature sensor is
> selected, it would wrongly read the temperature sensor registers, not
> the link state.
> 
> There is no need for the MAC to directly access the PHY. It will get
> told what the result of auto-neg is. So please turn this off all the
> time.
> 
> 	Andrew
> 


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

* RE: [PATCH] lan743x: Added fixed link support
  2020-05-17 20:44   ` Roelof Berg
  2020-05-17 23:50     ` Andrew Lunn
@ 2020-05-18 20:53     ` Bryan.Whitehead
  1 sibling, 0 replies; 14+ messages in thread
From: Bryan.Whitehead @ 2020-05-18 20:53 UTC (permalink / raw)
  To: rberg, andrew; +Cc: UNGLinuxDriver, davem, netdev, linux-kernel

> -----Original Message-----
> From: Roelof Berg <rberg@berg-solutions.de>
> Sent: Sunday, May 17, 2020 4:45 PM
> To: Andrew Lunn <andrew@lunn.ch>
> Cc: Bryan Whitehead - C21958 <Bryan.Whitehead@microchip.com>;
> UNGLinuxDriver <UNGLinuxDriver@microchip.com>; David S. Miller
> <davem@davemloft.net>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] lan743x: Added fixed link support
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> To Everyone: I need a test hardware recommendation for a lan7431/0 NIC in
> normal mode (not fixed-link mode). In prior patches this was not necessary,
> because I was able to ensure 100% backwards compatibility by careful coding
> alone. But I might soon come to a point where I need to test phy-connected
> devices as well.

Hi Roelof,

I believe I can find the hardware back at the office. However at this time, due to virus fears, I'm working from home.

Can hardware testing wait until we return to the office?

Regards,
Bryan


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

* Re: [PATCH] lan743x: Added fixed link support
  2020-05-18 19:31       ` Roelof Berg
@ 2020-05-18 20:34         ` Andrew Lunn
  2020-05-19 16:42           ` Roelof Berg
  0 siblings, 1 reply; 14+ messages in thread
From: Andrew Lunn @ 2020-05-18 20:34 UTC (permalink / raw)
  To: Roelof Berg
  Cc: Bryan Whitehead, Microchip Linux Driver Support, David S. Miller,
	netdev, linux-kernel

> I double checked the vendor documentation and according to the data
> sheet in this device the MAC detects speed and duplex mode. It uses
> PINs, traces clocks … Also according to an application note of the
> vendor duplex and speed detection should be enabled in the MAC
> registers.

In general, the MAC should not perform MDIO requests on the PHY.  The
MAC has no access to the mutex which phylib users. So if the MAC
directly accesses registers in the PHY, it could do it at the wrong
time, when the PHY driver is active.

This can be particularly bad when Marvell PHYs are used. They have
paged registers. One example is the page with the temperature sensor.
This can be selected due to a read on the hwmon device. If the MAC
tried to read the speed/duplex which the temperature sensor is
selected, it would wrongly read the temperature sensor registers, not
the link state.

There is no need for the MAC to directly access the PHY. It will get
told what the result of auto-neg is. So please turn this off all the
time.

	Andrew

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

* Re: [PATCH] lan743x: Added fixed link support
  2020-05-17 18:37 ` Andrew Lunn
  2020-05-17 20:44   ` Roelof Berg
@ 2020-05-18 20:01   ` Roelof Berg
  1 sibling, 0 replies; 14+ messages in thread
From: Roelof Berg @ 2020-05-18 20:01 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Bryan Whitehead, Microchip Linux Driver Support, David S. Miller,
	netdev, linux-kernel

Hi,

thanks a lot for guiding me. A thought on the transparency of fixed-phy regarding this section:

> 
>> +			/* 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:
(…)
> 
> The current code is unusual, in that it uses
> phy_ethtool_get_link_ksettings(). That should do the right thing with
> a fixed-link PHY, although i don't know if anybody uses it like
> this. So in theory, the current code should take care of duplex, flow
> control, and speed for you. Just watch out for bug/missing features in
> fixed link.
> 
> 

I checked your recommendations and I really would have loved to find a transparent layer that hides the speed/duplex configuration. But unfortunately I found no place that would set above’s bits in the registers (it was me who introduced this bits to the header file in my patch, they are unknown to the kernel). But this register settings need to be done in fixed-link mode. (Fixed-Link => no auto-negotiation => driver has to configure this bits for speed and duplex.)

The working mode of this device is usually:
- Turn on auto-negotiation (by setting a register in the MAC from the MCU)
- Wait until auto-negotiation is completed
- The auto-negotiation can now read back (speed and duplex) from the MAC registers
- However also the PHYs can be enumerated indirectly via MAC registers, which is where the kernel today gets the auto negotiation result from

Example from the data sheet of the lan7431 MAC layer:
————————
Automatic Speed Detection (ASD)
When set, the MAC ignores the setting of the MAC Configuration (CFG) field
and automatically determines the speed of operation. The MAC samples the
RX_CLK input to accomplish speed detection and reports the last determined
speed via the MAC Configuration (CFG) field.
When reset, the setting of the MAC Configuration (CFG) field determines
operational speed.
————————

So regarding the last sentence the driver will have to configure speed (also duplex) in fixed link mode and because no part of the kernel accesses this bits up to now, I’m afraid to come to the conclusion that we probably  need above’s code.

Thanks for sparring our patch, highly appreciated. I’m sorry that there might be no better solution than the one provided, at least I found none.


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

* Re: [PATCH] lan743x: Added fixed link support
  2020-05-17 23:50     ` Andrew Lunn
@ 2020-05-18 19:31       ` Roelof Berg
  2020-05-18 20:34         ` Andrew Lunn
  0 siblings, 1 reply; 14+ messages in thread
From: Roelof Berg @ 2020-05-18 19:31 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Bryan Whitehead, Microchip Linux Driver Support, David S. Miller,
	netdev, linux-kernel

Hi,

thanks a lot for going into detail. I also want to make sure that everything will be right.

> Am 18.05.2020 um 01:50 schrieb Andrew Lunn <andrew@lunn.ch>:
> 
>>>> +			/* Configure MAC to fixed link parameters */
>>>> +			data = lan743x_csr_read(adapter, MAC_CR);
>>>> +			/* Disable auto negotiation */
>>>> +			data &= ~(MAC_CR_ADD_ | MAC_CR_ASD_);
>>> 
>>> Why does the MAC care about autoneg? In general, all the MAC needs to
>>> know is the speed and duplex.
>>> 
>> 
> 
>> My assumption is, that in fixed-link mode we should switch off the
>> autonegotiation between MAC and remote peer (e.g. a switch). I
>> didn’t test, if it would also wun with the hardware doing
>> auto-negotiation, however it feels cleaner to me to prevent the
>> hardware from initiating any auto-negotiation in fixed-link mode.
> 
> The MAC is not involved in autoneg. autoneg is between two PHYs. They
> talk with each other, and then phylibs sees the results and tells the
> MAC the results of the negotiation. That happens via this call
> back. So i have no idea what this is doing in general in the MAC. And
> in your setup, you don't have any PHYs at all. So there is no
> auto-neg. You should read the datasheet and understand what this is
> controlling. It might need to be disabled in general.

Thanks for making sure we’re doing things right.

I double checked the vendor documentation and according to the data sheet in this device the MAC detects speed and duplex mode. It uses PINs, traces clocks … Also according to an application note of the vendor duplex and speed detection should be enabled in the MAC registers. The current driver version (which is not fixed-link capable) does this. However, in a fixed-link scenario I don’t think that the autodetection, that the vendor recommends to turn on, of speed and duplex would solve problems, it rather likely introduces problems when the auto-detection from the MAC (yes, the MAC) yields different results than configured in the device tree.

So I think we should:
a) Keep the behavior to activate auto detection in the MAC in normal cases, as recommended by the data sheet. (And ensure backwards compatibility this way as well.)
b) But add the behavior to deactivate this kind of MAC auto detection in fixed link cases.

I found no documentation for fixed link operation in the data sheets, so a statement from the Vendor could give us higher confidence here. Unfortunately I have no access to the Vendor’s specialists (also not via the Microchip customer support for some reasons), but I think the vendor is on CC on this thread ;)

> 
>> Using get_phy_mode() in all cases is not possible on a PC as it
>> returns SGMII on a standard PC.
> 
> Why do you think that?

printk made me think so :) But I printk’ed without checking the error return value, so that was maybe just invalid or even random data.

> 
>>> I don't understand this comment.
>>> 
>> 
>> See above the lengthy section. On a PC SGMII is returned when I call of_get_phy_mode(phynode, &phyifc);
> 
> There are two things possible here:
> 
> A PC has no OF support, so you are using:
> 
> https://elixir.bootlin.com/linux/latest/source/include/linux/of_net.h#L19
> 
> So you get the error code -ENODEV, and phyifc is not changed.
> 
> Or you are using:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/of/of_net.c#L25
> 
> There is unlikely to be a device node, so phyifc is set to
> PHY_INTERFACE_MODE_NA and -ENODEV is returned.
> 
> So if of_get_phy_mode() returns an error, use RMII. Otherwise use what
> value it set phyifc to.
> 
>      Andrew
> 

Ok, consider it done, thanks :)


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

* Re: [PATCH] lan743x: Added fixed link support
  2020-05-17 20:44   ` Roelof Berg
@ 2020-05-17 23:50     ` Andrew Lunn
  2020-05-18 19:31       ` Roelof Berg
  2020-05-18 20:53     ` Bryan.Whitehead
  1 sibling, 1 reply; 14+ messages in thread
From: Andrew Lunn @ 2020-05-17 23:50 UTC (permalink / raw)
  To: Roelof Berg
  Cc: Bryan Whitehead, Microchip Linux Driver Support, David S. Miller,
	netdev, linux-kernel

> >> +			/* Configure MAC to fixed link parameters */
> >> +			data = lan743x_csr_read(adapter, MAC_CR);
> >> +			/* Disable auto negotiation */
> >> +			data &= ~(MAC_CR_ADD_ | MAC_CR_ASD_);
> > 
> > Why does the MAC care about autoneg? In general, all the MAC needs to
> > know is the speed and duplex.
> > 
> 

> My assumption is, that in fixed-link mode we should switch off the
> autonegotiation between MAC and remote peer (e.g. a switch). I
> didn’t test, if it would also wun with the hardware doing
> auto-negotiation, however it feels cleaner to me to prevent the
> hardware from initiating any auto-negotiation in fixed-link mode.

The MAC is not involved in autoneg. autoneg is between two PHYs. They
talk with each other, and then phylibs sees the results and tells the
MAC the results of the negotiation. That happens via this call
back. So i have no idea what this is doing in general in the MAC. And
in your setup, you don't have any PHYs at all. So there is no
auto-neg. You should read the datasheet and understand what this is
controlling. It might need to be disabled in general.

> Using get_phy_mode() in all cases is not possible on a PC as it
> returns SGMII on a standard PC.

Why do you think that?

> > I don't understand this comment.
> > 
> 
> See above the lengthy section. On a PC SGMII is returned when I call of_get_phy_mode(phynode, &phyifc);

There are two things possible here:

A PC has no OF support, so you are using:

https://elixir.bootlin.com/linux/latest/source/include/linux/of_net.h#L19

So you get the error code -ENODEV, and phyifc is not changed.

Or you are using:

https://elixir.bootlin.com/linux/latest/source/drivers/of/of_net.c#L25

There is unlikely to be a device node, so phyifc is set to
PHY_INTERFACE_MODE_NA and -ENODEV is returned.

So if of_get_phy_mode() returns an error, use RMII. Otherwise use what
value it set phyifc to.

      Andrew

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

* Re: [PATCH] lan743x: Added fixed link support
  2020-05-17 18:37 ` Andrew Lunn
@ 2020-05-17 20:44   ` Roelof Berg
  2020-05-17 23:50     ` Andrew Lunn
  2020-05-18 20:53     ` Bryan.Whitehead
  2020-05-18 20:01   ` Roelof Berg
  1 sibling, 2 replies; 14+ messages in thread
From: Roelof Berg @ 2020-05-17 20:44 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Bryan Whitehead, Microchip Linux Driver Support, David S. Miller,
	netdev, linux-kernel

To Everyone: I need a test hardware recommendation for a lan7431/0 NIC in normal mode (not fixed-link mode). In prior patches this was not necessary, because I was able to ensure 100% backwards compatibility by careful coding alone. But I might soon come to a point where I need to test phy-connected devices as well.

Hi Andrew,

thanks for commenting on my patch.


> Am 17.05.2020 um 20:37 schrieb Andrew Lunn <andrew@lunn.ch>:
> 
>> @@ -946,6 +949,9 @@ static void lan743x_phy_link_status_change(struct net_device *netdev)
>> {
>> 	struct lan743x_adapter *adapter = netdev_priv(netdev);
>> 	struct phy_device *phydev = netdev->phydev;
>> +	struct device_node *phynode;
>> +	phy_interface_t phyifc = PHY_INTERFACE_MODE_GMII;
>> +	u32 data;
>> 
>> 	phy_print_status(phydev);
>> 	if (phydev->state == PHY_RUNNING) {
>> @@ -953,6 +959,48 @@ static void lan743x_phy_link_status_change(struct net_device *netdev)
>> 		int remote_advertisement = 0;
>> 		int local_advertisement = 0;
>> 
>> +		/* check if a fixed-link is defined in device-tree */
>> +		phynode = of_node_get(adapter->pdev->dev.of_node);
>> +		if (phynode && of_phy_is_fixed_link(phynode)) {
> 
> Hi Roelof
> 
> The whole point for fixed link is that it looks like a PHY. You should
> not need to care if it is a real PHY or a fixed link.
> 

Ok, I can try to remove the additional speed and baud configuration, when the PHY simulation should lead to the same result. Understood, thanks, I’ll test this and remove the overhead.

> 
>> +			/* Configure MAC to fixed link parameters */
>> +			data = lan743x_csr_read(adapter, MAC_CR);
>> +			/* Disable auto negotiation */
>> +			data &= ~(MAC_CR_ADD_ | MAC_CR_ASD_);
> 
> Why does the MAC care about autoneg? In general, all the MAC needs to
> know is the speed and duplex.
> 

My assumption is, that in fixed-link mode we should switch off the autonegotiation between MAC and remote peer (e.g. a switch). I didn’t test, if it would also wun with the hardware doing auto-negotiation, however it feels cleaner to me to prevent the hardware from initiating any auto-negotiation in fixed-link mode.

>> +			/* 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;
>> +			}
> 
> The current code is unusual, in that it uses
> phy_ethtool_get_link_ksettings(). That should do the right thing with
> a fixed-link PHY, although i don't know if anybody uses it like
> this. So in theory, the current code should take care of duplex, flow
> control, and speed for you. Just watch out for bug/missing features in
> fixed link.

Ok, I test and report if it works. Now I understand the concept.

> 
> 
>> +			/* Set interface mode */
>> +			of_get_phy_mode(phynode, &phyifc);
>> +			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);
>> +		}
>> +		of_node_put(phynode);
> 
> It is normal to do of_get_phy_mode when connecting to the PHY, and
> store the value in the private structure. This is also not specific to
> fixed link.
> 
> There is also a helper you can use phy_interface_mode_is_rgmii().

Thanks for pointing to the method is_rgmii, very handy.

Using get_phy_mode() in all cases is not possible on a PC as it returns SGMII on a standard PC, but using GMII is today’s driver behavior. So what I basically did (on two places) is:

if(fixed-link)
   Use get_phy_mode()’s result in of_phy_connect() and in the lan7431 register configuration.
else
   Keep the prior behavior for backwards compatibility (i.e. ignoring the wrong interface mode config on a PC and use GMII constant)

The method is_rgmii you mention can lessen the pain here, thanks, and lead to:

if(is_rgmii()
	use RGMII
else
	use GMII

I need to think about this, because NOT passing get_phy_mode’s result directly into of_phy_connect or phy_connect_direct (and instead use above's (is_rgmii() ? RGMII : GMII) code) could have side effects.

However I don’t dare to pass get_phy_mode’s result directly into of_phy_connect or phy_connect_direct on a PC because then I might change the behavior of all standard PC NIC drivers. I haven’t researched yet why on a PC SGMII is returned by get_phy_mode(), does someone know ?. 

> 
>> +
>> 		memset(&ksettings, 0, sizeof(ksettings));
>> 		phy_ethtool_get_link_ksettings(netdev, &ksettings);
>> 		local_advertisement =
>> @@ -974,6 +1022,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 +1032,44 @@ 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 = NULL;
>> +	phy_interface_t phyifc = PHY_INTERFACE_MODE_GMII;
>> 	int ret = -EIO;
> 
> netdev uses reverse christmas tree, meaning the lines should be
> sorted, longest first, getting shorter.
Ok
> 
>> 
>> 	netdev = adapter->netdev;
>> -	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);
>> -	if (ret)
>> -		goto return_error;
>> +	/* check if a fixed-link is defined in device-tree */
>> +	phynode = of_node_get(adapter->pdev->dev.of_node);
>> +	if (phynode && of_phy_is_fixed_link(phynode)) {
>> +		netdev_dbg(netdev, "fixed-link detected\n");
> 
> This is something which is useful during debug. But once it works can
> be removed.
Ok
> 
>> +		ret = of_phy_register_fixed_link(phynode);
>> +		if (ret) {
>> +			netdev_err(netdev, "cannot register fixed PHY\n");
>> +			goto return_error;
>> +		}
>> +
>> +		of_get_phy_mode(phynode, &phyifc);
>> +		phydev = of_phy_connect(netdev, phynode,
>> +					lan743x_phy_link_status_change,
>> +					0, phyifc);
>> +		if (!phydev)
>> +			goto return_error;
>> +	} 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.
>> +		 */
> 
> I don't understand this comment.
> 

See above the lengthy section. On a PC SGMII is returned when I call of_get_phy_mode(phynode, &phyifc); but the original driver is using PHY_INTERFACE_MODE_GMII; and I don’t dare to change this behavior. Which I would do when I would pass on the result of of_get_phy_mode(). That’s what I meant by the comment.

Thanks a lot directing me to the proper way,
Roelof



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

* Re: [PATCH] lan743x: Added fixed link support
  2020-05-16 19:24 Roelof Berg
@ 2020-05-17 18:37 ` Andrew Lunn
  2020-05-17 20:44   ` Roelof Berg
  2020-05-18 20:01   ` Roelof Berg
  0 siblings, 2 replies; 14+ messages in thread
From: Andrew Lunn @ 2020-05-17 18:37 UTC (permalink / raw)
  To: Roelof Berg
  Cc: Bryan Whitehead, Microchip Linux Driver Support, David S. Miller,
	netdev, linux-kernel

> @@ -946,6 +949,9 @@ static void lan743x_phy_link_status_change(struct net_device *netdev)
>  {
>  	struct lan743x_adapter *adapter = netdev_priv(netdev);
>  	struct phy_device *phydev = netdev->phydev;
> +	struct device_node *phynode;
> +	phy_interface_t phyifc = PHY_INTERFACE_MODE_GMII;
> +	u32 data;
>  
>  	phy_print_status(phydev);
>  	if (phydev->state == PHY_RUNNING) {
> @@ -953,6 +959,48 @@ static void lan743x_phy_link_status_change(struct net_device *netdev)
>  		int remote_advertisement = 0;
>  		int local_advertisement = 0;
>  
> +		/* check if a fixed-link is defined in device-tree */
> +		phynode = of_node_get(adapter->pdev->dev.of_node);
> +		if (phynode && of_phy_is_fixed_link(phynode)) {

Hi Roelof

The whole point for fixed link is that it looks like a PHY. You should
not need to care if it is a real PHY or a fixed link.


> +			/* Configure MAC to fixed link parameters */
> +			data = lan743x_csr_read(adapter, MAC_CR);
> +			/* Disable auto negotiation */
> +			data &= ~(MAC_CR_ADD_ | MAC_CR_ASD_);

Why does the MAC care about autoneg? In general, all the MAC needs to
know is the speed and duplex.

> +			/* 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;
> +			}

The current code is unusual, in that it uses
phy_ethtool_get_link_ksettings(). That should do the right thing with
a fixed-link PHY, although i don't know if anybody uses it like
this. So in theory, the current code should take care of duplex, flow
control, and speed for you. Just watch out for bug/missing features in
fixed link.


> +			/* Set interface mode */
> +			of_get_phy_mode(phynode, &phyifc);
> +			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);
> +		}
> +		of_node_put(phynode);

It is normal to do of_get_phy_mode when connecting to the PHY, and
store the value in the private structure. This is also not specific to
fixed link.

There is also a helper you can use phy_interface_mode_is_rgmii().

> +
>  		memset(&ksettings, 0, sizeof(ksettings));
>  		phy_ethtool_get_link_ksettings(netdev, &ksettings);
>  		local_advertisement =
> @@ -974,6 +1022,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 +1032,44 @@ 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 = NULL;
> +	phy_interface_t phyifc = PHY_INTERFACE_MODE_GMII;
>  	int ret = -EIO;

netdev uses reverse christmas tree, meaning the lines should be
sorted, longest first, getting shorter.

>  
>  	netdev = adapter->netdev;
> -	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);
> -	if (ret)
> -		goto return_error;
> +	/* check if a fixed-link is defined in device-tree */
> +	phynode = of_node_get(adapter->pdev->dev.of_node);
> +	if (phynode && of_phy_is_fixed_link(phynode)) {
> +		netdev_dbg(netdev, "fixed-link detected\n");

This is something which is useful during debug. But once it works can
be removed.

> +		ret = of_phy_register_fixed_link(phynode);
> +		if (ret) {
> +			netdev_err(netdev, "cannot register fixed PHY\n");
> +			goto return_error;
> +		}
> +
> +		of_get_phy_mode(phynode, &phyifc);
> +		phydev = of_phy_connect(netdev, phynode,
> +					lan743x_phy_link_status_change,
> +					0, phyifc);
> +		if (!phydev)
> +			goto return_error;
> +	} 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.
> +		 */

I don't understand this comment.


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

* [PATCH] lan743x: Added fixed link support
@ 2020-05-16 19:24 Roelof Berg
  2020-05-17 18:37 ` Andrew Lunn
  0 siblings, 1 reply; 14+ messages in thread
From: Roelof Berg @ 2020-05-16 19:24 UTC (permalink / raw)
  To: rberg
  Cc: andrew, Bryan Whitehead, Microchip Linux Driver Support,
	David S. Miller, netdev, linux-kernel

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@berg-solutions.de>
---
 drivers/net/ethernet/microchip/lan743x_main.c | 93 +++++++++++++++++--
 drivers/net/ethernet/microchip/lan743x_main.h |  4 +
 2 files changed, 89 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index a43140f7b5eb..278765dfc3b3 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"
 
@@ -946,6 +949,9 @@ static void lan743x_phy_link_status_change(struct net_device *netdev)
 {
 	struct lan743x_adapter *adapter = netdev_priv(netdev);
 	struct phy_device *phydev = netdev->phydev;
+	struct device_node *phynode;
+	phy_interface_t phyifc = PHY_INTERFACE_MODE_GMII;
+	u32 data;
 
 	phy_print_status(phydev);
 	if (phydev->state == PHY_RUNNING) {
@@ -953,6 +959,48 @@ static void lan743x_phy_link_status_change(struct net_device *netdev)
 		int remote_advertisement = 0;
 		int local_advertisement = 0;
 
+		/* check if a fixed-link is defined in device-tree */
+		phynode = of_node_get(adapter->pdev->dev.of_node);
+		if (phynode && of_phy_is_fixed_link(phynode)) {
+			/* 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 */
+			of_get_phy_mode(phynode, &phyifc);
+			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);
+		}
+		of_node_put(phynode);
+
 		memset(&ksettings, 0, sizeof(ksettings));
 		phy_ethtool_get_link_ksettings(netdev, &ksettings);
 		local_advertisement =
@@ -974,6 +1022,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 +1032,44 @@ 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 = NULL;
+	phy_interface_t phyifc = PHY_INTERFACE_MODE_GMII;
 	int ret = -EIO;
 
 	netdev = adapter->netdev;
-	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);
-	if (ret)
-		goto return_error;
+	/* check if a fixed-link is defined in device-tree */
+	phynode = of_node_get(adapter->pdev->dev.of_node);
+	if (phynode && of_phy_is_fixed_link(phynode)) {
+		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;
+		}
+
+		of_get_phy_mode(phynode, &phyifc);
+		phydev = of_phy_connect(netdev, phynode,
+					lan743x_phy_link_status_change,
+					0, phyifc);
+		if (!phydev)
+			goto return_error;
+	} 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;
+	}
+	of_node_put(phynode);
 
 	/* MAC doesn't support 1000T Half */
 	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
@@ -1008,6 +1084,7 @@ static int lan743x_phy_open(struct lan743x_adapter *adapter)
 	return 0;
 
 return_error:
+	of_node_put(phynode);
 	return ret;
 }
 
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 3b02eeae5f45..e49f6b6cd440 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -104,10 +104,14 @@
 	((value << 0) & FCT_FLOW_CTL_ON_THRESHOLD_)
 
 #define MAC_CR				(0x100)
+#define MAC_CR_MII_EN_			BIT(19)
 #define MAC_CR_EEE_EN_			BIT(17)
 #define MAC_CR_ADD_			BIT(12)
 #define MAC_CR_ASD_			BIT(11)
 #define MAC_CR_CNTR_RST_		BIT(5)
+#define MAC_CR_DPX_			BIT(3)
+#define MAC_CR_CFG_H_			BIT(2)
+#define MAC_CR_CFG_L_			BIT(1)
 #define MAC_CR_RST_			BIT(0)
 
 #define MAC_RX				(0x104)
-- 
2.20.1


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

* Re: [PATCH] lan743x: Added fixed link support
  2020-05-13 19:06 Roelof Berg
@ 2020-05-13 19:17 ` Florian Fainelli
  0 siblings, 0 replies; 14+ messages in thread
From: Florian Fainelli @ 2020-05-13 19:17 UTC (permalink / raw)
  To: Roelof Berg
  Cc: andrew, Bryan Whitehead, Microchip Linux Driver Support,
	David S. Miller, netdev, linux-kernel



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@berg-solutions.de>
> ---
>  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

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

* [PATCH] lan743x: Added fixed link support
@ 2020-05-13 19:06 Roelof Berg
  2020-05-13 19:17 ` Florian Fainelli
  0 siblings, 1 reply; 14+ messages in thread
From: Roelof Berg @ 2020-05-13 19:06 UTC (permalink / raw)
  To: rberg
  Cc: andrew, Bryan Whitehead, Microchip Linux Driver Support,
	David S. Miller, netdev, linux-kernel

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@berg-solutions.de>
---
 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);
+	} 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);
 	return 0;
 
 return_error:
+	if (phynode)
+		of_node_put(phynode);
 	return ret;
 }
 
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 3b02eeae5f45..e49f6b6cd440 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -104,10 +104,14 @@
 	((value << 0) & FCT_FLOW_CTL_ON_THRESHOLD_)
 
 #define MAC_CR				(0x100)
+#define MAC_CR_MII_EN_			BIT(19)
 #define MAC_CR_EEE_EN_			BIT(17)
 #define MAC_CR_ADD_			BIT(12)
 #define MAC_CR_ASD_			BIT(11)
 #define MAC_CR_CNTR_RST_		BIT(5)
+#define MAC_CR_DPX_			BIT(3)
+#define MAC_CR_CFG_H_			BIT(2)
+#define MAC_CR_CFG_L_			BIT(1)
 #define MAC_CR_RST_			BIT(0)
 
 #define MAC_RX				(0x104)
-- 
2.20.1


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

end of thread, other threads:[~2020-05-21 14:04 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 17:10 [PATCH] lan743x: Added fixed link support Roelof Berg
2020-05-21 14:04 ` Andrew Lunn
  -- strict thread matches above, loose matches on Subject: below --
2020-05-16 19:24 Roelof Berg
2020-05-17 18:37 ` Andrew Lunn
2020-05-17 20:44   ` Roelof Berg
2020-05-17 23:50     ` Andrew Lunn
2020-05-18 19:31       ` Roelof Berg
2020-05-18 20:34         ` Andrew Lunn
2020-05-19 16:42           ` Roelof Berg
2020-05-19 22:06             ` Ronnie.Kunin
2020-05-18 20:53     ` Bryan.Whitehead
2020-05-18 20:01   ` Roelof Berg
2020-05-13 19:06 Roelof Berg
2020-05-13 19:17 ` Florian Fainelli

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