linux-kernel - Re: [PATCH net-next v2 05/21] motorcomm:yt6801: Implement the fxgmac

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a48d76ac-db08-46d5-9528-f046a7b541dc@motor-comm.com>
Date: Tue, 26 Nov 2024 17:28:08 +0800
From: Frank Sae <Frank.Sae@...or-comm.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 xiaogang.fan@...or-comm.com, fei.zhang@...or-comm.com, hua.sun@...or-comm.com
Subject: 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:
>>>> 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.


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