From: Andrew Lunn <andrew@lunn.ch>
To: Roelof Berg <rberg@berg-solutions.de>
Cc: Bryan Whitehead <bryan.whitehead@microchip.com>,
Microchip Linux Driver Support <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
Date: Sun, 17 May 2020 20:37:10 +0200 [thread overview]
Message-ID: <20200517183710.GC606317@lunn.ch> (raw)
In-Reply-To: <20200516192402.4201-1-rberg@berg-solutions.de>
> @@ -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.
next prev parent reply other threads:[~2020-05-17 18:37 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-16 19:24 [PATCH] lan743x: Added fixed link support Roelof Berg
2020-05-17 18:37 ` Andrew Lunn [this message]
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
-- strict thread matches above, loose matches on Subject: below --
2020-05-20 17:10 Roelof Berg
2020-05-21 14:04 ` Andrew Lunn
2020-05-13 19:06 Roelof Berg
2020-05-13 19:17 ` Florian Fainelli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200517183710.GC606317@lunn.ch \
--to=andrew@lunn.ch \
--cc=UNGLinuxDriver@microchip.com \
--cc=bryan.whitehead@microchip.com \
--cc=davem@davemloft.net \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=rberg@berg-solutions.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).