Re: [PATCH net-next v6] net: ngbe: Add ngbe mdio bus driver.

[Andrew Lunn <andrew@lunn.ch>]

> +static int ngbe_phy_read_reg_internal(struct mii_bus *bus, int phy_addr, int regnum)
> +{
> +	struct wx *wx = bus->priv;
> +
> +	if (regnum & MII_ADDR_C45)
> +		return -EOPNOTSUPP;
> +	return (u16)rd32(wx, NGBE_PHY_CONFIG(regnum));

You ignore phy_addr. Which suggests you only allow one internal
PHY. Best practice here is to put the internal PHY on phy_addr 0, and
return 0xffff for all other phy_addr values. If phylib probes the full
range, or userspace tries to access the full range, it will look like
there is no PHY at these other addresses.

> +}
> +
> +static int ngbe_phy_write_reg_internal(struct mii_bus *bus, int phy_addr, int regnum, u16 value)
> +{
> +	struct wx *wx = bus->priv;
> +
> +	if (regnum & MII_ADDR_C45)
> +		return -EOPNOTSUPP;
> +	wr32(wx, NGBE_PHY_CONFIG(regnum), value);
> +	return 0;

Here, silently ignore writes to phy_addr != 0.

> +	/* wait to complete */
> +	ret = read_poll_timeout(rd32, val, !(val & NGBE_MSCC_BUSY), 1000,
> +				100000, false, wx, NGBE_MSCC);
> +	if (ret) {
> +		wx_err(wx, "PHY address command did not complete.\n");
> +		return ret;
> +	}
> +
> +	return (u16)rd32(wx, NGBE_MSCC);
> +}
> +
> +	/* wait to complete */
> +	ret = read_poll_timeout(rd32, val, !(val & NGBE_MSCC_BUSY), 1000,
> +				100000, false, wx, NGBE_MSCC);
> +	if (ret)
> +		wx_err(wx, "PHY address command did not complete.\n");

You have the exact same error message. When you see such an error in
the log, it can sometimes be useful to know was it a read or a write
which failed. So i would suggest you put read/write into the message.

> +static void ngbe_phy_fixup(struct wx *wx)
> +{
> +	struct phy_device *phydev = wx->phydev;
> +	struct ethtool_eee eee;
> +
> +	if (wx->mac_type != em_mac_type_mdi)
> +		return;

Does this mean that if using the internal PHY the MAC does support EEE
and half duplex?

> +	/* disable EEE, EEE not supported by mac */
> +	memset(&eee, 0, sizeof(eee));
> +	phy_ethtool_set_eee(phydev, &eee);
> +
> +	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
> +	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
> +	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> +}
> +
> +int ngbe_mdio_init(struct wx *wx)
> +{
> +	struct pci_dev *pdev = wx->pdev;
> +	struct mii_bus *mii_bus;
> +	int ret;
> +
> +	mii_bus = devm_mdiobus_alloc(&pdev->dev);
> +	if (!mii_bus)
> +		return -ENOMEM;
> +
> +	mii_bus->name = "ngbe_mii_bus";
> +	mii_bus->read = ngbe_phy_read_reg;
> +	mii_bus->write = ngbe_phy_write_reg;
> +	mii_bus->phy_mask = GENMASK(31, 4);
> +	mii_bus->probe_capabilities = MDIOBUS_C22_C45;

That is not strictly true. The internal MDIO bus does not suport
C45. In practice, it probably does not matter.

     Andrew