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 <[email protected]>
---
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
> @@ -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.
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 <[email protected]>:
>
>> @@ -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
> >> + /* 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
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 <[email protected]>:
>
>>>> + /* 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 :)
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.
> 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
> -----Original Message-----
> From: Roelof Berg <[email protected]>
> Sent: Sunday, May 17, 2020 4:45 PM
> To: Andrew Lunn <[email protected]>
> Cc: Bryan Whitehead - C21958 <[email protected]>;
> UNGLinuxDriver <[email protected]>; David S. Miller
> <[email protected]>; [email protected]; linux-
> [email protected]
> 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
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 <[email protected]>:
>
>> 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
>
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 <[email protected]>
Sent: Tuesday, May 19, 2020 12:43 PM
To: Andrew Lunn <[email protected]>
Cc: Bryan Whitehead - C21958 <[email protected]>; UNGLinuxDriver <[email protected]>; David S. Miller <[email protected]>; [email protected]; [email protected]
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 <[email protected]>:
>
>> 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
>