Message-ID: <aeb9f17c-ea94-4362-aeda-7d94c5845462@gmail.com>
Date: Tue, 5 Mar 2024 09:19:41 +0100
From: Eric Woudstra <ericwouds@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Heiner Kallweit <hkallweit1@...il.com>,
 Russell King <linux@...linux.org.uk>,
 Matthias Brugger <matthias.bgg@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Frank Wunderlich <frank-w@...lic-files.de>,
 Daniel Golle <daniel@...rotopia.org>, Lucien Jheng
 <lucien.jheng@...oha.com>, Zhi-Jun You <hujy652@...tonmail.com>,
 netdev@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 net-next 2/2] net: phy: air_en8811h: Add the Airoha
 EN8811H PHY driver



On 3/3/24 18:51, Andrew Lunn wrote:
>> +static int en8811h_config_init(struct phy_device *phydev)
>> +{
>> +	struct en8811h_priv *priv = phydev->priv;
>> +	struct device *dev = &phydev->mdio.dev;
>> +	int ret, pollret, reg_value;
>> +	u32 pbus_value;
>> +
>> +	if (!priv->firmware_version)
>> +		ret = en8811h_load_firmware(phydev);
> 
> How long does this take for your hardware?

It takes 1.87 and 0.24 seconds to load the firmware files.

> We have a phylib design issue with loading firmware. It would be
> better if it happened during probe, but it might not be finished by
> the time the MAC driver tries to attach to the PHY and so that fails.
> This is the second PHY needing this, so maybe we need to think about
> adding a thread kicked off in probe to download the firmware? But
> probably later, not part of this patchset.

The main reason I have it in config_init() is that by then the
rootfs is available. The EN8811H does not depend on the firmware
to respond to get_features(). It is therefore possible to not
have the firmware in initramfs or included in the kernel image.
I could not get this result using EPROBE_DEFER, I think this is
not an option in phylink.

It does however work, loading firmware in probe(), without running
a thread, so it is possible to have it there.

>> +	/* Select mode 1, the only mode supported */
> 
> Maybe a comment about what mode 1 actually is?

I'll need to contact Airoha to get a better description. There
should be a datasheet coming for external use, but it isn't
published yet.

>> +	ret = air_leds_init(phydev, EN8811H_LED_COUNT, AIR_PHY_LED_DUR,
>> +			    AIR_LED_MODE_USER_DEFINE);
>> +	if (ret < 0) {
>> +		phydev_err(phydev, "Failed to initialize leds: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = air_buckpbus_reg_modify(phydev, EN8811H_GPIO_OUTPUT,
>> +				      EN8811H_GPIO_OUTPUT_345,
>> +				      EN8811H_GPIO_OUTPUT_345);
> 
> What does this do? Configure them as inputs? Hopefully they are inputs
> by default, or at least Hi-Z.

I'll add a comment that it set's these 3 gpio's as output for the leds.

>> +static int en8811h_get_features(struct phy_device *phydev)
>> +{
>> +	linkmode_set_bit_array(phy_basic_ports_array,
>> +			       ARRAY_SIZE(phy_basic_ports_array),
>> +			       phydev->supported);
>> +
>> +	return genphy_c45_pma_read_abilities(phydev);
>> +}
>> +
> 
>> +static int en8811h_config_aneg(struct phy_device *phydev)
>> +{
>> +	bool changed = false;
>> +	int ret;
>> +	u32 adv;
>> +
>> +	adv = linkmode_adv_to_mii_10gbt_adv_t(phydev->advertising);
>> +
>> +	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_10GBT_CTRL,
>> +				     MDIO_AN_10GBT_CTRL_ADV2_5G, adv);
>> +	if (ret < 0)
>> +		return ret;
>> +	if (ret > 0)
>> +		changed = true;
>> +
>> +	return __genphy_config_aneg(phydev, changed);
> 
> There was a comment that it does not support forced link mode, only
> auto-neg? It would be good to test the configuration here and return
> EOPNOTSUPP, or EINVAL if auto-neg is turned off.
> 
>> +}
>> +
>> +static int en8811h_read_status(struct phy_device *phydev)
>> +{
>> +	struct en8811h_priv *priv = phydev->priv;
>> +	u32 pbus_value;
>> +	int ret, val;
>> +
>> +	ret = genphy_update_link(phydev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	phydev->master_slave_get = MASTER_SLAVE_CFG_UNSUPPORTED;
>> +	phydev->master_slave_state = MASTER_SLAVE_STATE_UNSUPPORTED;
>> +	phydev->speed = SPEED_UNKNOWN;
>> +	phydev->duplex = DUPLEX_UNKNOWN;
>> +	phydev->pause = 0;
>> +	phydev->asym_pause = 0;
>> +
>> +	ret = genphy_read_master_slave(phydev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = genphy_read_lpa(phydev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Get link partner 2.5GBASE-T ability from vendor register */
>> +	ret = air_buckpbus_reg_read(phydev, EN8811H_2P5G_LPA, &pbus_value);
>> +	if (ret < 0)
>> +		return ret;
>> +	linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
>> +			 phydev->lp_advertising,
>> +			 pbus_value & EN8811H_2P5G_LPA_2P5G);
>> +
>> +	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete)
> 
> Is the first part of that expression needed? I thought you could not
> turn auto-neg off?

Will returning an error in config_aneg() prevent auto-neg being disabled?
If so, then indeed I could drop the first part then.

>> +
>> +	/* Only supports full duplex */
>> +	phydev->duplex = DUPLEX_FULL;
> 
> What does en8811h_get_features() indicate the PHY can do? Are any 1/2
> duplex modes listed?


Partial output from ethtool:

	Supported ports: [ TP	 MII ]
	Supported link modes:   100baseT/Full
	                        1000baseT/Full
	                        2500baseT/Full
	Supported pause frame use: Symmetric Receive-only
	Supports auto-negotiation: Yes
	Supported FEC modes: Not reported
	Advertised link modes:  100baseT/Full
	                        1000baseT/Full
	                        2500baseT/Full
	Advertised pause frame use: Symmetric Receive-only
	Advertised auto-negotiation: Yes
	Advertised FEC modes: Not reported

Best regards,

Eric Woudstra

Powered by blists - more mailing lists