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

[Jakub Kicinski <kuba@kernel.org>]

On Tue,  6 Dec 2022 19:40:35 +0800 Mengyuan Lou wrote:
> Add mdio bus register for ngbe.
> The internal phy and external phy need to be handled separately.
> Add phy changed event detection.

>  static void ngbe_down(struct ngbe_adapter *adapter)
>  {
> -	netif_carrier_off(adapter->netdev);
> -	netif_tx_disable(adapter->netdev);
> +	struct ngbe_hw *hw = &adapter->hw;
> +
> +	phy_stop(hw->phydev);
> +	ngbe_disable_device(adapter);
>  };
>  
> +static void ngbe_up(struct ngbe_adapter *adapter)
> +{
> +	struct ngbe_hw *hw = &adapter->hw;
> +
> +	pci_set_master(adapter->pdev);

why set/clear bus master on up/down? This is normally done during probe
and cleared on remove. Having bus mastering enabled doesn't cost
anything.

> +	if (hw->gpio_ctrl)
> +		/* gpio0 is used to power on control*/
> +		wr32(&hw->wxhw, NGBE_GPIO_DR, 0);
> +	phy_start(hw->phydev);

> +static int ngbe_phy_read_reg_mdi(struct mii_bus *bus, int phy_addr, int regnum)
> +{
> +	u32 command = 0, device_type = 0;
> +	struct ngbe_hw *hw = bus->priv;
> +	struct wx_hw *wxhw = &hw->wxhw;
> +	u32 phy_data = 0;
> +	u32 val = 0;
> +	int ret = 0;
> +
> +	if (regnum & MII_ADDR_C45) {
> +		wr32(wxhw, NGBE_MDIO_CLAUSE_SELECT, 0x0);
> +		/* setup and write the address cycle command */
> +		command = NGBE_MSCA_RA(mdiobus_c45_regad(regnum)) |
> +			  NGBE_MSCA_PA(phy_addr) |
> +			  NGBE_MSCA_DA(mdiobus_c45_devad(regnum));
> +	} else {
> +		wr32(wxhw, NGBE_MDIO_CLAUSE_SELECT, 0xF);
> +		/* setup and write the address cycle command */
> +		command = NGBE_MSCA_RA(regnum) |
> +			  NGBE_MSCA_PA(phy_addr) |
> +			  NGBE_MSCA_DA(device_type);
> +	}
> +	wr32(wxhw, NGBE_MSCA, command);
> +	command = NGBE_MSCC_CMD(NGBE_MSCA_CMD_READ) |
> +		  NGBE_MSCC_BUSY |
> +		  NGBE_MDIO_CLK(6);
> +	wr32(wxhw, NGBE_MSCC, command);
> +
> +	/* wait to complete */
> +	ret = read_poll_timeout(rd32, val, !(val & NGBE_MSCC_BUSY), 1000,
> +				100000, false, wxhw, NGBE_MSCC);
> +

spurious new line

> +	if (ret)
> +		wx_dbg(wxhw, "PHY address command did not complete.\n");
> +
> +	/* read data from MSCC */
> +	phy_data = 0xffff & rd32(wxhw, NGBE_MSCC);
> +
> +	return phy_data;
> +}
> +
> +static int ngbe_phy_write_reg_mdi(struct mii_bus *bus, int phy_addr, int regnum, u16 value)
> +{
> +	u32 command = 0, device_type = 0;
> +	struct ngbe_hw *hw = bus->priv;
> +	struct wx_hw *wxhw = &hw->wxhw;
> +	int ret = 0;
> +	u16 val = 0;

Don't pointlessly initialize all variables.
There is a compiler option which does that, for the paranoid.

> +	if (regnum & MII_ADDR_C45) {
> +		wr32(wxhw, NGBE_MDIO_CLAUSE_SELECT, 0x0);
> +		/* setup and write the address cycle command */
> +		command = NGBE_MSCA_RA(mdiobus_c45_regad(regnum)) |
> +			  NGBE_MSCA_PA(phy_addr) |
> +			  NGBE_MSCA_DA(mdiobus_c45_devad(regnum));
> +	} else {
> +		wr32(wxhw, NGBE_MDIO_CLAUSE_SELECT, 0xF);
> +		/* setup and write the address cycle command */
> +		command = NGBE_MSCA_RA(regnum) |
> +			  NGBE_MSCA_PA(phy_addr) |
> +			  NGBE_MSCA_DA(device_type);
> +	}
> +	wr32(wxhw, NGBE_MSCA, command);
> +	command = value |
> +		  NGBE_MSCC_CMD(NGBE_MSCA_CMD_WRITE) |
> +		  NGBE_MSCC_BUSY |
> +		  NGBE_MDIO_CLK(6);
> +	wr32(wxhw, NGBE_MSCC, command);
> +
> +	/* wait to complete */
> +	ret = read_poll_timeout(rd32, val, !(val & NGBE_MSCC_BUSY), 1000,
> +				100000, false, wxhw, NGBE_MSCC);
> +

spurious empty line

> +	if (ret)
> +		wx_dbg(wxhw, "PHY address command did not complete.\n");
> +
> +	return ret;
> +}
> +
> +static int ngbe_phy_read_reg(struct mii_bus *bus, int phy_addr, int regnum)
> +{
> +	struct ngbe_hw *hw = bus->priv;
> +	u16 phy_data = 0;

Why 0? If the mac_type is not known error would seem more appropriate?

> +	if (hw->mac_type == ngbe_mac_type_mdi)
> +		phy_data = ngbe_phy_read_reg_internal(bus, phy_addr, regnum);
> +	else if (hw->mac_type == ngbe_mac_type_rgmii)
> +		phy_data = ngbe_phy_read_reg_mdi(bus, phy_addr, regnum);
> +
> +	return phy_data;
> +}
> +
> +static int ngbe_phy_write_reg(struct mii_bus *bus, int phy_addr, int regnum, u16 value)
> +{
> +	struct ngbe_hw *hw = bus->priv;
> +	int ret = 0;

Why 0? If the mac_type is not known error would seem more appropriate?

> +	if (hw->mac_type == ngbe_mac_type_mdi)
> +		ret = ngbe_phy_write_reg_internal(bus, phy_addr, regnum, value);
> +	else if (hw->mac_type == ngbe_mac_type_rgmii)
> +		ret = ngbe_phy_write_reg_mdi(bus, phy_addr, regnum, value);
> +	return ret;
> +}
> +
> +static void ngbe_handle_link_change(struct net_device *dev)
> +{
> +	struct ngbe_adapter *adapter = netdev_priv(dev);
> +	struct ngbe_hw *hw = &adapter->hw;
> +	struct wx_hw *wxhw = &hw->wxhw;
> +	u32 lan_speed = 2, reg;
> +	bool changed = false;
> +
> +	struct phy_device *phydev = hw->phydev;

Local variables should be in one block. 
If there is a problem with the ordering because of inter-dependencies
just initialize in the code rather than inline.

> +	if (hw->link != phydev->link ||
> +	    hw->speed != phydev->speed ||
> +	    hw->duplex != phydev->duplex) {
> +		changed = true;

This seems unnecessary, flip the condition and return here
without the need for @changed.

> +		hw->link = phydev->link;
> +		hw->speed = phydev->speed;
> +		hw->duplex = phydev->duplex;
> +	}

> +int ngbe_phy_connect(struct ngbe_hw *hw)
> +{
> +	struct ngbe_adapter *adapter = container_of(hw,
> +						    struct ngbe_adapter,
> +						    hw);
> +	int ret;
> +
> +	ret = phy_connect_direct(adapter->netdev,
> +				 hw->phydev,
> +				 ngbe_handle_link_change,
> +				 PHY_INTERFACE_MODE_RGMII_ID);
> +	if (ret) {
> +		wx_err(&hw->wxhw,
> +		       "PHY connect failed.\n");

Unnecessary line break

> +	snprintf(hw->mii_bus->id, MII_BUS_ID_SIZE, "ngbe-%x",
> +		 (pdev->bus->number << 8) |
> +		 pdev->devfn);

Unnecessary line break