On Tue, Jul 16, 2024 at 05:03:48PM +0530, Raju Lakkaraju wrote:

+static void lan743x_phylink_mac_link_up(struct phylink_config *config,
+					struct phy_device *phydev,
+					unsigned int link_an_mode,
+					phy_interface_t interface,
+					int speed, int duplex,
+					bool tx_pause, bool rx_pause)
+{
+	struct net_device *netdev = to_net_dev(config->dev);
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
+	int mac_cr;
+	u8 cap;
+
+	mac_cr = lan743x_csr_read(adapter, MAC_CR);
+	/* Pre-initialize register bits.
+	 * Resulting value corresponds to SPEED_10
+	 */
+	mac_cr &= ~(MAC_CR_CFG_H_ | MAC_CR_CFG_L_);
+	if (speed == SPEED_2500)
+		mac_cr |= (MAC_CR_CFG_H_ | MAC_CR_CFG_L_);
+	else if (speed == SPEED_1000)
+		mac_cr |= (MAC_CR_CFG_H_);
+	else if (speed == SPEED_100)
+		mac_cr |= (MAC_CR_CFG_L_);

These parens in each of these if() sub-blocks is not required. |=
operates the same way as = - all such operators are treated the same
in C.

+
+	lan743x_csr_write(adapter, MAC_CR, mac_cr);
+
+	lan743x_ptp_update_latency(adapter, speed);
+
+	/* Flow Control operation */
+	cap = 0;
+	if (tx_pause)
+		cap |= FLOW_CTRL_TX;
+	if (rx_pause)
+		cap |= FLOW_CTRL_RX;
+
+	lan743x_mac_flow_ctrl_set_enables(adapter,
+					  cap & FLOW_CTRL_TX,
+					  cap & FLOW_CTRL_RX);
+
+	netif_tx_wake_all_queues(to_net_dev(config->dev));

You already have "netdev", so there's no need to do the to_net_dev()
dance again here.

+}
+
+static const struct phylink_mac_ops lan743x_phylink_mac_ops = {
+	.mac_config = lan743x_phylink_mac_config,
+	.mac_link_down = lan743x_phylink_mac_link_down,
+	.mac_link_up = lan743x_phylink_mac_link_up,
+};

I guess as there's no PCS support here, you don't support inband mode
for 1000base-X (which is rather fundamental for it).

+
+static int lan743x_phylink_create(struct net_device *netdev)
+{
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
+	struct phylink *pl;
+
+	adapter->phylink_config.dev = &netdev->dev;
+	adapter->phylink_config.type = PHYLINK_NETDEV;
+	adapter->phylink_config.mac_managed_pm = false;
+
+	adapter->phylink_config.mac_capabilities = MAC_ASYM_PAUSE |
+		MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000FD | MAC_2500FD;
+
+	lan743x_phy_interface_select(adapter);
+
+	switch (adapter->phy_interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+		__set_bit(PHY_INTERFACE_MODE_SGMII,
+			  adapter->phylink_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
+			  adapter->phylink_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
+			  adapter->phylink_config.supported_interfaces);
+		break;
+	case PHY_INTERFACE_MODE_GMII:
+		__set_bit(PHY_INTERFACE_MODE_GMII,
+			  adapter->phylink_config.supported_interfaces);
+		break;
+	case PHY_INTERFACE_MODE_MII:
+		__set_bit(PHY_INTERFACE_MODE_MII,
+			  adapter->phylink_config.supported_interfaces);
+		break;
+	default:
+		__set_bit(PHY_INTERFACE_MODE_RGMII,
+			  adapter->phylink_config.supported_interfaces);

Do you really only support RGMII and not RGMII_ID/RGMII_TXID/RGMII_RXID
(which are normally implemented by tweaking the delays at the PHY end
of the RGMII link) ?

+static bool lan743x_phy_handle_exists(struct device_node *dn)
+{
+	dn = of_parse_phandle(dn, "phy-handle", 0);
+	of_node_put(dn);
+	if (IS_ERR(dn))
+		return false;
+
+	return true;

This likely doesn't work. Have you checked what the return values for
of_parse_phandle() actually are before creating this, because to me
this looks like you haven't - and thus what you've created is wrong.
of_parse_phandle() doesn't return error-pointers when it fails, it
returns NULL. Therefore, this will always return true.

We have another implementation of something very similar in the macb
driver - see macb_phy_handle_exists(), and this one is implemented
correctly.

+}
+
+static int lan743x_phylink_connect(struct lan743x_adapter *adapter)
+{
+	struct device_node *dn = adapter->pdev->dev.of_node;
+	struct net_device *dev = adapter->netdev;
+	struct fixed_phy_status fphy_status = {
+		.link = 1,
+		.speed = SPEED_1000,
+		.duplex = DUPLEX_FULL,
+	};
+	struct phy_device *phydev;
+	int ret;
+
+	if (dn)
+		ret = phylink_of_phy_connect(adapter->phylink, dn, 0);
+
+	if (!dn || (ret && !lan743x_phy_handle_exists(dn))) {
+		phydev = phy_find_first(adapter->mdiobus);
+		if (!phydev) {
+			if (((adapter->csr.id_rev & ID_REV_ID_MASK_) ==
+			      ID_REV_ID_LAN7431_) || adapter->is_pci11x1x) {
+				phydev = fixed_phy_register(PHY_POLL,
+							    &fphy_status,
+							    NULL);
+				if (IS_ERR(phydev)) {
+					netdev_err(dev, "No PHY/fixed_PHY found\n");
+					return PTR_ERR(phydev);
+				}

Eww. Given that phylink has its own internal fixed-PHY support, can we
not find some way to avoid the legacy fixed-PHY usage here?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!