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

On 2024/11/26 11:15, Frank Sae wrote:

Hi Andrew,

On 2024/11/25 22:18, Andrew Lunn wrote:

quoted

quoted

quoted

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

But if you pass PHY_INTERFACE_MODE_RGMII to the PHY it is not adding
the 2ns delay. So how does this work now?

I'm sorry. Maybe PHY_INTERFACE_MODE_RGMII is enough.
YT6801 is a pcie NIC chip that integrates one yt8531s phy.
Therefore, a delay of 2ns is unnecessary, as the hardware has
 already ensured this.

YT6801 looks like that:

                      ||                      
  ********************++**********************
  *            | PCIE Endpoint |             *
  *            +---------------+             *
  *                | GMAC |                  *
  *                +--++--+      YT6801      *
  *                  |**|                    *
  *         GMII --> |**| <-- MDIO           *
  *                 +-++--+                  *
  *                 | PHY |                  *
  *                 +--++-+                  *
  *********************||*********************

yt8531s phy driver not support GMII.

quoted

quoted

quoted

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)'?

There are only a few phylib API calls you should be using

phy_connect() or one of its variants.
phy_start()
phy_stop()
phy_disconnect()

Those four are the core. Those should be all you need to minimum
support.

phy_support_asym_pause()
phy_support_eee()
phy_speed_up()
phy_speed_down()

and these are just nice to have to let phylib know about things the
MAC supports, so phylib can manage the PHY to make them available to
the MAC. This is the API between the MAC driver and phylib. phylib
will then manage the PHY. Any time you want to use a phy_* function,
look to see if other MAC drivers do. If they don't you should not
either.

Tanks for your clear explanation.

quoted

quoted

quoted

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);' ?

Two things here:

phy_speed_up()/phy_speed_down() is part of suspend/resume when using
WoL. This code has nothing to do with that. So why is it here?

There should not be any need to call genphy_soft_reset(). You should
figure out why you need it, because it could be a PHY driver bug, or a
MAC driver bug.

	Andrew