| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d6107622-b0c2-5b07-e7b2-01a417596c8e@gmail.com>
Date: Wed, 13 May 2020 12:17:08 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: Roelof Berg <rberg@...g-solutions.de>
Cc: andrew@...n.ch, Bryan Whitehead <bryan.whitehead@...rochip.com>,
Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] lan743x: Added fixed link support
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@...g-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
Powered by blists - more mailing lists