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

["mengyuanlou@net-swift.com" <mengyuanlou@net-swift.com>]

> 2022年12月10日 18:47，Andrew Lunn <andrew@lunn.ch> 写道：
> 
>> int ngbe_reset_hw(struct ngbe_hw *hw)
>> {
>> 	struct wx_hw *wxhw = &hw->wxhw;
>> -	int status = 0;
>> -	u32 reset = 0;
>> +	u32 val = 0;
>> +	int ret = 0;
>> 
>> 	/* Call adapter stop to disable tx/rx and clear interrupts */
>> -	status = wx_stop_adapter(wxhw);
>> -	if (status != 0)
>> -		return status;
>> -	reset = WX_MIS_RST_LAN_RST(wxhw->bus.func);
>> -	wr32(wxhw, WX_MIS_RST, reset | rd32(wxhw, WX_MIS_RST));
>> +	ret = wx_stop_adapter(wxhw);
>> +	if (ret != 0)
>> +		return ret;
>> +
>> +	if (hw->mac_type != ngbe_mac_type_mdi) {
>> +		val = WX_MIS_RST_LAN_RST(wxhw->bus.func);
>> +		wr32(wxhw, WX_MIS_RST, val | rd32(wxhw, WX_MIS_RST));
>> +
>> +		ret = read_poll_timeout(rd32, val,
>> +					!(val & (BIT(9) << wxhw->bus.func)), 1000,
>> +					100000, false, wxhw, 0x10028);
>> +		if (ret)
>> +			wx_dbg(wxhw, "Lan reset exceed s maximum times.\n");
> 
> This is a real error. Return it up the call stack so the user gets to
> know. This should be netdev_err(), since it is an error.
> 
>> +static void ngbe_up(struct ngbe_adapter *adapter)
>> +{
>> +	struct ngbe_hw *hw = &adapter->hw;
>> +
>> +	pci_set_master(adapter->pdev);
>> +	if (hw->gpio_ctrl)
>> +		/* gpio0 is used to power on control*/
>> +		wr32(&hw->wxhw, NGBE_GPIO_DR, 0);
> 
> Control of what?
Control for sfp modules power down/up.
The chip has not i2c interface, so I do not use phylink.

> 
>> +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);
>> +
>> +	if (ret)
>> +		wx_dbg(wxhw, "PHY address command did not complete.\n");
> 
> netdev_err() and return the error code.
> 
>> +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;
>> +
>> +	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);
>> +
>> +	if (ret)
>> +		wx_dbg(wxhw, "PHY address command did not complete.\n");
> 
> dev_error().
> 
>> +
>> +	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;
>> +
>> +	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;
> 
> You return 0 if it is neither MDI or RGMII. It would be better to
> either return -EINVAL, or 0xffff. A read on an MDIO bus for an address
> without a device returns 0xffff due to the pull up resisters. That
> will let Linux know there is no device there.
> 
>> +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;
>> +
>> +	if (hw->link != phydev->link ||
>> +	    hw->speed != phydev->speed ||
>> +	    hw->duplex != phydev->duplex) {
>> +		changed = true;
>> +		hw->link = phydev->link;
>> +		hw->speed = phydev->speed;
>> +		hw->duplex = phydev->duplex;
>> +	}
>> +
>> +	if (!changed)
>> +		return;
>> +
>> +	switch (phydev->speed) {
>> +	case SPEED_1000:
>> +		lan_speed = 2;
>> +		break;
>> +	case SPEED_100:
>> +		lan_speed = 1;
>> +		break;
>> +	case SPEED_10:
>> +		lan_speed = 0;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +	wr32m(wxhw, NGBE_CFG_LAN_SPEED, 0x3, lan_speed);
>> +
>> +	if (phydev->link) {
>> +		if (phydev->speed & (SPEED_1000 | SPEED_100 | SPEED_10)) {
> 
> You logically OR 10, 100 and 1000? These SPEED_FOO macros are not
> bits.
> 
> 	Andrew
>