Re: [PATCH net-next v2 05/21] motorcomm:yt6801: Implement the fxgmac_start function

Hi Andrew,

On 2024/11/23 07:16, Andrew Lunn wrote:

quoted

+static  void fxgmac_phylink_handler(struct net_device *ndev)
+{
+	struct fxgmac_pdata *pdata = netdev_priv(ndev);
+	struct fxgmac_hw_ops *hw_ops = &pdata->hw_ops;
+
+	pdata->phy_link = pdata->phydev->link;
+	pdata->phy_speed = pdata->phydev->speed;
+	pdata->phy_duplex = pdata->phydev->duplex;
+
+	yt_dbg(pdata, "EPHY_CTRL:%x, link:%d, speed:%d,  duplex:%x.\n",
+	       rd32_mem(pdata, EPHY_CTRL), pdata->phy_link, pdata->phy_speed,
+	       pdata->phy_duplex);
+
+	if (pdata->phy_link) {
+		hw_ops->config_mac_speed(pdata);
+		hw_ops->enable_rx(pdata);
+		hw_ops->enable_tx(pdata);
+		netif_carrier_on(pdata->netdev);

phylib controls the carrier, not the MAC driver.

quoted

+static int fxgmac_phy_connect(struct fxgmac_pdata *pdata)
+{
+	struct phy_device *phydev = pdata->phydev;
+	int ret;
+
+	ret = phy_connect_direct(pdata->netdev, phydev, fxgmac_phylink_handler,
+				 PHY_INTERFACE_MODE_RGMII);

RGMII is unusual, you normally want RGMII_ID. Where are the 2ns delays
added?

Yes, you are right. PHY_INTERFACE_MODE_RGMII should be PHY_INTERFACE_MODE_RGMII_ID.
YT6801 NIC integrated with YT8531S phy, and the 2ns delays added in the phy driver.
https://elixir.bootlin.com/linux/v6.12/source/drivers/net/phy/motorcomm.c#L895

quoted

+int fxgmac_phy_irq_enable(struct fxgmac_pdata *pdata, bool clear_phy_interrupt)
+{
+	struct phy_device *phydev = pdata->phydev;
+
+	if (clear_phy_interrupt &&
+	    phy_read(phydev, PHY_INT_STATUS) < 0)
+		return -ETIMEDOUT;
+
+	return phy_modify(phydev, PHY_INT_MASK,
+				     PHY_INT_MASK_LINK_UP |
+					     PHY_INT_MASK_LINK_DOWN,
+				     PHY_INT_MASK_LINK_UP |
+					     PHY_INT_MASK_LINK_DOWN);

The PHY driver is in charge of PHY interrupts.

quoted

+int fxgmac_start(struct fxgmac_pdata *pdata)
+{
+	struct fxgmac_hw_ops *hw_ops = &pdata->hw_ops;
+	u32 val;
+	int ret;
+
+	if (pdata->dev_state != FXGMAC_DEV_OPEN &&
+	    pdata->dev_state != FXGMAC_DEV_STOP &&
+	    pdata->dev_state != FXGMAC_DEV_RESUME) {
+		yt_dbg(pdata, " dev_state err:%x\n", pdata->dev_state);
+		return 0;
+	}
+
+	if (pdata->dev_state != FXGMAC_DEV_STOP) {
+		hw_ops->reset_phy(pdata);
+		hw_ops->release_phy(pdata);
+		yt_dbg(pdata, "reset phy.\n");
+	}
+
+	if (pdata->dev_state == FXGMAC_DEV_OPEN) {
+		ret = fxgmac_phy_connect(pdata);
+		if (ret < 0)
+			return ret;
+
+		yt_dbg(pdata, "fxgmac_phy_connect.\n");
+	}
+
+	phy_init_hw(pdata->phydev);
+	phy_resume(pdata->phydev);

The MAC should not be doing this.

Does this mean deleting 'phy_resume(pdata->phydev)'?

quoted

+
+	hw_ops->pcie_init(pdata);
+	if (test_bit(FXGMAC_POWER_STATE_DOWN, &pdata->powerstate)) {
+		yt_err(pdata,
+		       "fxgmac powerstate is %lu when config power up.\n",
+		       pdata->powerstate);
+	}
+
+	hw_ops->config_power_up(pdata);
+	hw_ops->dismiss_all_int(pdata);
+	ret = hw_ops->init(pdata);
+	if (ret < 0) {
+		yt_err(pdata, "fxgmac hw init error.\n");
+		return ret;
+	}
+
+	fxgmac_napi_enable(pdata);
+	ret = fxgmac_request_irqs(pdata);
+	if (ret < 0)
+		return ret;
+
+	/* Config interrupt to level signal */
+	val = rd32_mac(pdata, DMA_MR);
+	fxgmac_set_bits(&val, DMA_MR_INTM_POS, DMA_MR_INTM_LEN, 2);
+	fxgmac_set_bits(&val, DMA_MR_QUREAD_POS, DMA_MR_QUREAD_LEN, 1);
+	wr32_mac(pdata, val, DMA_MR);
+
+	hw_ops->enable_mgm_irq(pdata);
+	hw_ops->set_interrupt_moderation(pdata);
+
+	if (pdata->per_channel_irq) {
+		fxgmac_enable_msix_irqs(pdata);
+		ret = fxgmac_phy_irq_enable(pdata, true);
+		if (ret < 0)
+			goto dis_napi;
+	}
+
+	fxgmac_enable_rx_tx_ints(pdata);
+	phy_speed_up(pdata->phydev);
+	genphy_soft_reset(pdata->phydev);

More things the MAC driver should not be doing.

Does this mean deleting 'phy_speed_up(pdata->phydev);' and 'genphy_soft_reset(pdata->phydev);' ?

	Andrew