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: <43341290-15e3-4784-9b69-7f3f13f34e01@motor-comm.com>
Date: Tue, 26 Nov 2024 11:15:46 +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

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.

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