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

[Frank Sae <Frank.Sae@motor-comm.com>]

Hi Andrew,

On 2024/11/23 07:16, Andrew Lunn wrote:
>> +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.
> 
>> +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


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

> 
>> +
>> +	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