Re: [PATCH net-next v3 2/2] net: phy: mediatek: add driver for built-in 2.5G ethernet PHY on MT7988

["Russell King (Oracle)" <linux@armlinux.org.uk>]

Hi,

On Wed, May 14, 2025 at 06:57:38PM +0800, Sky Huang wrote:
> +#define MTK_2P5GPHY_ID_MT7988		(0x00339c11)
> +
> +#define MT7988_2P5GE_PMB_FW		"mediatek/mt7988/i2p5ge-phy-pmb.bin"
> +#define MT7988_2P5GE_PMB_FW_SIZE	(0x20000)
> +#define MT7988_2P5GE_PMB_FW_BASE	(0x0f100000)
> +#define MT7988_2P5GE_PMB_FW_LEN		(0x20000)
> +#define MTK_2P5GPHY_MCU_CSR_BASE	(0x0f0f0000)
> +#define MTK_2P5GPHY_MCU_CSR_LEN		(0x20)
> +#define MD32_EN_CFG			(0x18)

These parens are all unnecessary, as are ones below around a simple
number.

> +#define   MD32_EN			BIT(0)
> +
> +#define BASE100T_STATUS_EXTEND		(0x10)
> +#define BASE1000T_STATUS_EXTEND		(0x11)
> +#define EXTEND_CTRL_AND_STATUS		(0x16)
> +
> +#define PHY_AUX_CTRL_STATUS		(0x1d)
> +#define   PHY_AUX_DPX_MASK		GENMASK(5, 5)
> +#define   PHY_AUX_SPEED_MASK		GENMASK(4, 2)
> +
> +/* Registers on MDIO_MMD_VEND1 */
> +#define MTK_PHY_LPI_PCS_DSP_CTRL		(0x121)

...

> +static int mt798x_2p5ge_phy_load_fw(struct phy_device *phydev)
> +{
> +	void __iomem *mcu_csr_base, *pmb_addr;
> +	struct device *dev = &phydev->mdio.dev;

This will attract a comment about reverse christmas tree.

> +	const struct firmware *fw;
> +	int ret, i;
> +	u32 reg;

...

> +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> +{
> +	struct pinctrl *pinctrl;
> +
> +	/* Check if PHY interface type is compatible */
> +	if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL)
> +		return -ENODEV;
> +
> +	/* Setup LED */
> +	phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED0_ON_CTRL,
> +			 MTK_PHY_LED_ON_POLARITY | MTK_PHY_LED_ON_LINK10 |
> +			 MTK_PHY_LED_ON_LINK100 | MTK_PHY_LED_ON_LINK1000 |
> +			 MTK_PHY_LED_ON_LINK2500);
> +	phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED1_ON_CTRL,
> +			 MTK_PHY_LED_ON_FDX | MTK_PHY_LED_ON_HDX);
> +
> +	/* Switch pinctrl after setting polarity to avoid bogus blinking */
> +	pinctrl = devm_pinctrl_get_select(&phydev->mdio.dev, "i2p5gbe-led");
> +	if (IS_ERR(pinctrl))
> +		dev_err(&phydev->mdio.dev, "Fail to set LED pins!\n");

No, don't do this. config_init() can be called multiple times during
the lifetime of the driver bound to the device, and each time it is,
a new managed-dev structure will be allocated to release this action
each time, thus consuming more and more memory, or possibly failing
after the first depending on the pinctrl_get_select() behaviour.
Please find a different way.

...

> +static int mt798x_2p5ge_phy_config_aneg(struct phy_device *phydev)
> +{
> +	bool changed = false;
> +	u32 adv;
> +	int ret;
> +
> +	ret = genphy_c45_an_config_aneg(phydev);
> +	if (ret < 0)
> +		return ret;
> +	if (ret > 0)
> +		changed = true;
> +
> +	/* Clause 45 doesn't define 1000BaseT support. Use Clause 22 instead in
> +	 * our design.
> +	 */
> +	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;
> +
> +	return __genphy_config_aneg(phydev, changed);

Do you want this (which will program EEE and the 10/100 advert) or do
you want genphy_check_and_restart_aneg() here? Note that
genphy_c45_an_config_aneg() will already have programmed both the EEE
and 10/100 adverts via the C45 registers.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!