Message-ID: <370ec3e9-033a-49d4-8f9d-44eedd404be7@lunn.ch>
Date: Sun, 17 Dec 2023 00:33:18 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Eric Woudstra <ericwouds@...il.com>
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 RFC net-next 2/2] Add the Airoha EN8811H PHY driver

> index 107880d13d21..bb89cf57de25 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -409,6 +409,11 @@ config XILINX_GMII2RGMII
>  	  the Reduced Gigabit Media Independent Interface(RGMII) between
>  	  Ethernet physical media devices and the Gigabit Ethernet controller.
>  
> +config AIR_EN8811H_PHY
> +	tristate "Drivers for Airoha EN8811H 2.5 Gigabit PHY"

If you look at the naming pattern, this should be

tristate "Airoha EN8811H 2.5 Gigabit PHY"

and these Kconfig entries are sorted for the tristate value, so this
should be between Analog and aquantia.

> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -95,3 +95,4 @@ obj-$(CONFIG_STE10XP)		+= ste10Xp.o
>  obj-$(CONFIG_TERANETICS_PHY)	+= teranetics.o
>  obj-$(CONFIG_VITESSE_PHY)	+= vitesse.o
>  obj-$(CONFIG_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
> +obj-$(CONFIG_AIR_EN8811H_PHY)   += air_en8811h.o

Makefile is sorted by CONFIG_ so that belongs much earlier.

> + * - Forced speed (AN off) is not supported by hardware (!00Mbps)
> + * - Hardware does not report link-partner 2500Base-T advertisement
> +static int __air_buckpbus_reg_write(struct phy_device *phydev,
> +				    u32 pbus_address, u32 pbus_data)
> +{
> +	int ret;
> +
> +	ret = __phy_write(phydev, AIR_EXT_PAGE_ACCESS, AIR_PHY_PAGE_EXTENDED_4);
> +	if (ret < 0)
> +		return ret;



> +	ret = __phy_write(phydev, AIR_EXT_PAGE_ACCESS, AIR_PHY_PAGE_STANDARD);
> +	if (ret < 0)
> +		return ret;

Please implement the .read_page and .write_page methods in struct phy_driver

> +
> +	return 0;
> +}
> +
> +static int air_buckpbus_reg_write(struct phy_device *phydev,
> +				  u32 pbus_address, u32 pbus_data)
> +{
> +	int ret;
> +
> +	phy_lock_mdio_bus(phydev);
> +	ret = __air_buckpbus_reg_write(phydev, pbus_address, pbus_data);
> +	phy_unlock_mdio_bus(phydev);

This then becomes:

        saved_page = phy_save_page(phydev);
	ret = __air_buckpbus_reg_write(phydev, pbus_address, pbus_data);
	return phy_restore_page(phydev, saved_page, ret);

and all the locking is performed for you.

> +	for (offset = 0; offset < fw->size; offset += 4) {
> +		val = MAKEWORD(fw->data[offset + 2], fw->data[offset + 3]);

get_unaligned_le16() might do what you want. Its better to use the
existing macros than define your own. They are also more likely to do
the correct thing on big-endian.

> +static int en8811h_load_firmware(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	const struct firmware *fw1, *fw2;
> +	int ret;
> +	unsigned int pbus_value;

netdev uses reverse christmass tree. Longest lines first, shortest
last.

> +static int en8811h_config_aneg(struct phy_device *phydev)
> +{
> +	u32 adv;
> +	bool changed = false;
> +	int ret;
> +
> +	phydev_dbg(phydev, "%s: advertising=%*pb\n", __func__,
> +		   __ETHTOOL_LINK_MODE_MASK_NBITS, phydev->advertising);
> +
> +	if (phydev->autoneg == AUTONEG_DISABLE)
> +		return genphy_c45_pma_setup_forced(phydev);

There is a comment saying AUTONEG Off is not supported?

> +
> +	ret = genphy_c45_an_config_aneg(phydev);
> +	if (ret < 0)
> +		return ret;
> +	if (ret > 0)
> +		changed = true;
> +
> +	/* Clause 45 has no standardized support for 1000BaseT, therefore
> +	 * use Clause 22 registers for this mode.
> +	 */
> +	adv = linkmode_adv_to_mii_ctrl1000_t(phydev->advertising);
> +	ret = phy_modify_changed(phydev, MII_CTRL1000, ADVERTISE_1000FULL, adv);
> +	if (ret < 0)
> +		return ret;
> +	if (ret > 0)
> +		changed = true;
> +

Could genphy_config_advert() be used here for the C22 registers?

> +int en8811h_c45_read_link(struct phy_device *phydev)
> +{
> +	int val;
> +
> +	/* Read link state from known reliable register (latched) */
> +
> +	if (!phy_polling_mode(phydev) || !phydev->link) {
> +		val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> +		if (val < 0)
> +			return val;
> +		phydev->link = !!(val & MDIO_STAT1_LSTATUS);
> +
> +		if (phydev->link)
> +			return 0;
> +	}
> +
> +	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> +	if (val < 0)
> +		return val;
> +	phydev->link = !!(val & MDIO_STAT1_LSTATUS);
> +
> +	return 0;
> +}
> +
> +static int en8811h_read_status(struct phy_device *phydev)
> +{
> +	int ret, lpagb;
> +	unsigned int pbus_value;
> +
> +	ret = en8811h_c45_read_link(phydev);
> +	if (ret)
> +		return ret;
> +
> +	phydev->speed = SPEED_UNKNOWN;
> +	phydev->duplex = DUPLEX_UNKNOWN;
> +	phydev->pause = 0;
> +	phydev->asym_pause = 0;
> +
> +	if (phydev->autoneg == AUTONEG_ENABLE) {
> +		ret = genphy_c45_read_lpa(phydev);
> +		if (ret)
> +			return ret;
> +
> +		/* Clause 45 has no standardized support for 1000BaseT,
> +		 * therefore use Clause 22 registers for this mode.
> +		 */
> +		lpagb = phy_read(phydev, MII_STAT1000);
> +		if (lpagb < 0)
> +			return lpagb;
> +		mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, lpagb);

Can you use genphy_read_lpa() here?

In general, if you can use the genphy_ functions to handle the
10/100/1000 speeds, please do.


    Andrew

---
pw-bot: cr

Powered by blists - more mailing lists