Message-Id: <20240529.084826.1300161327983399018.fujita.tomonori@gmail.com>
Date: Wed, 29 May 2024 08:48:26 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: linux@...linux.org.uk
Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org, andrew@...n.ch,
 horms@...nel.org, kuba@...nel.org, jiri@...nulli.us, pabeni@...hat.com,
 hfdevel@....net, naveenm@...vell.com, jdamato@...tly.com
Subject: Re: [PATCH net-next v7 6/6] net: tn40xx: add phylink support

Hi,
Thanks for reviewing the patch!

On Tue, 28 May 2024 13:02:42 +0100
"Russell King (Oracle)" <linux@...linux.org.uk> wrote:

> On Tue, May 28, 2024 at 05:39:28AM +0900, FUJITA Tomonori wrote:
>> This patch adds supports for multiple PHY hardware with phylink. The
>> adapters with TN40xx chips use multiple PHY hardware; AMCC QT2025, TI
>> TLK10232, Aqrate AQR105, and Marvell 88X3120, 88X3310, and MV88E2010.
>> 
>> For now, the PCI ID table of this driver enables adapters using only
>> QT2025 PHY. I've tested this driver and the QT2025 PHY driver (SFP+
>> 10G SR) with Edimax EN-9320 10G adapter.
>> 
> i> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
> 
> A few comments - I don't recall seeing previous versions of these
> patches, despite it being at version 7.

Sorry, my bad. The first version doesn't use phylink. I should have
added you to the CC list when I changed the driver to use phylink over
phylib according to Andrew's comment.

To provide some background, a vendor released this driver but it
hasn't been merged into the tree. The vendor went out of
bushiness. I'm trying to get the original driver to be mainline
quality. I don't have hardware spec doc.


>> @@ -1082,6 +1083,10 @@ static void tn40_link_changed(struct tn40_priv *priv)
>>  				 TN40_REG_MAC_LNK_STAT) & TN40_MAC_LINK_STAT;
>>  
>>  	netdev_dbg(priv->ndev, "link changed %u\n", link);
>> +	if (link)
>> +		phylink_mac_change(priv->phylink, true);
>> +	else
>> +		phylink_mac_change(priv->phylink, false);
> 
> This is only useful if you have a PCS, and I don't see anything in the
> driver that suggests you do. What link is this referring to?

When I plug the fiber, the above function is triggered with the link
on. With the fiber unplugged, the function is called with the link
off.

The original driver resets the MAC when the link is down. It
configures the MAC according to the cable speed (seems the original
driver is tested with only 10G though).


> In any case, you could eliminate the if() and just pass !!link if it's
> not already boolean in nature (the if() suggests it is, so passing just
> "link" would also work.)

Oops, right.


>> @@ -1381,10 +1389,17 @@ static int tn40_open(struct net_device *dev)
>>  	struct tn40_priv *priv = netdev_priv(dev);
>>  	int ret;
>>  
>> +	ret = phylink_connect_phy(priv->phylink, priv->phydev);
>> +	if (ret)
>> +		return ret;
>> +
>>  	tn40_sw_reset(priv);
>> +	phylink_start(priv->phylink);
> 
> At this point, the link could have come up (mac_link_up() could well
> be called.) Is the driver prepared to cope with that happening right
> _now_ in the tn40_open sequence? If not, you will need to move this
> to a point where the driver is ready to begin operation.

Understood, I'll change the order.


> phylink_stop() is its opposite, and you need to call that at the
> point where you want the link to be taken down (iow, where you want
> .mac_link_down() to be guaranteed to have been called if the link
> was already up.)

I see. The usage of phylink_stop() looks fine.


>> @@ -143,6 +143,9 @@ struct tn40_priv {
>>  	char *b0_va; /* Virtual address of buffer */
>>  
>>  	struct mii_bus *mdio;
>> +	struct phy_device *phydev;
>> +	struct phylink *phylink;
>> +	struct phylink_config phylink_config;
> 
> So phylink_config is embedded in tn40_priv - that's fine. What this does
> mean is you can trivially go from the phylink_config pointer to a
> pointer to tn40_priv *without* multiple dereferences:
> 
> static inline struct tn40_priv *
> config_to_tn40_priv(struct phylink_config *config)
> {
> 	return container_of(config, struct tn40_priv, phylink_config);
> }

I'll update the code.

>> +static void tn40_link_up(struct phylink_config *config, struct phy_device *phy,
>> +			 unsigned int mode, phy_interface_t interface,
>> +			 int speed, int duplex, bool tx_pause, bool rx_pause)
>> +{
>> +	struct net_device *ndev = to_net_dev(config->dev);
>> +	struct tn40_priv *priv = netdev_priv(ndev);
>> +
>> +	tn40_set_link_speed(priv, speed);
>> +	netif_wake_queue(priv->ndev);
>> +}
>> +
>> +static void tn40_link_down(struct phylink_config *config, unsigned int mode,
>> +			   phy_interface_t interface)
>> +{
>> +	struct net_device *ndev = to_net_dev(config->dev);
>> +	struct tn40_priv *priv = netdev_priv(ndev);
>> +
>> +	tn40_set_link_speed(priv, 0);
>> +	netif_stop_queue(priv->ndev);
> 
> Shouldn't the queue be stopped first?

Indeed, I'll fix.

>> +}
>> +
>> +static void tn40_mac_config(struct phylink_config *config, unsigned int mode,
>> +			    const struct phylink_link_state *state)
>> +{
>> +}
> 
> Nothing needs to be done here?

Seems that nothing is necessary here.

When I try something like 1G SFP or BASE-T in the future, I might find
something.

>> +
>> +static const struct phylink_mac_ops tn40_mac_ops = {
>> +	.mac_config = tn40_mac_config,
>> +	.mac_link_up = tn40_link_up,
>> +	.mac_link_down = tn40_link_down,
>> +};
>> +
>> +int tn40_phy_register(struct tn40_priv *priv)
>> +{
>> +	struct phylink_config *config;
>> +	struct phy_device *phydev;
>> +	struct phylink *phylink;
>> +
>> +	phydev = phy_find_first(priv->mdio);
>> +	if (!phydev) {
>> +		dev_err(&priv->pdev->dev, "PHY isn't found\n");
>> +		return -1;
>> +	}
>> +
>> +	config = &priv->phylink_config;
>> +	config->dev = &priv->ndev->dev;
>> +	config->type = PHYLINK_NETDEV;
>> +	config->mac_capabilities = MAC_10000FD | MLO_AN_PHY;
> 
> MLO_AN_PHY is not a MAC capability, it shouldn't be here.

I don't know why I use MLO_AN_PHY here. I thought that I tried to
imitate wangxun driver but I can't find. I'll drop it in the next
version.

Thanks a lot!

Powered by blists - more mailing lists