Re: [PATCH net-next V2 3/4] net: lan743x: Migrate phylib to phylink

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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!