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

["SkyLake Huang (黃啟澤)" <SkyLake.Huang@mediatek.com>]

On Wed, 2025-02-19 at 16:47 +0100, Andrew Lunn wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> > +config MEDIATEK_2P5GE_PHY
> > +     tristate "MediaTek 2.5Gb Ethernet PHYs"
> > +     depends on (ARM64 && ARCH_MEDIATEK) || COMPILE_TEST
> > +     select MTK_NET_PHYLIB
> > +     help
> > +       Supports MediaTek SoC built-in 2.5Gb Ethernet PHYs.
> > +
> > +       This will load necessary firmware and add appropriate time
> > delay.
> > +       Accelerate this procedure through internal pbus instead of
> > MDIO
> > +       bus. Certain link-up issues will also be fixed here.
> 
> Please keep the file sorted, this should be the first entry.
> 
I'll sort in this way in v3:
config MEDIATEK_2P5GE_PHY
...
config MEDIATEK_GE_SOC_PHY
...
config MTK_NET_PHYLIB
...
config MEDIATEK_GE_PHY
...

> > diff --git a/drivers/net/phy/mediatek/Makefile
> > b/drivers/net/phy/mediatek/Makefile
> > index 814879d0abe5..c6db8abd2c9c 100644
> > --- a/drivers/net/phy/mediatek/Makefile
> > +++ b/drivers/net/phy/mediatek/Makefile
> > @@ -2,3 +2,4 @@
> >  obj-$(CONFIG_MTK_NET_PHYLIB)         += mtk-phy-lib.o
> >  obj-$(CONFIG_MEDIATEK_GE_PHY)                += mtk-ge.o
> >  obj-$(CONFIG_MEDIATEK_GE_SOC_PHY)    += mtk-ge-soc.o
> > +obj-$(CONFIG_MEDIATEK_2P5GE_PHY)     += mtk-2p5ge.o
> 
> I suppose you could say this file is sorted in reverse order so is
> correct?
> 
I'll change this in v3 in this way:
obj-$(CONFIG_MEDIATEK_2P5GE_PHY)     += mtk-2p5ge.o
obj-$(CONFIG_MEDIATEK_GE_SOC_PHY)    += mtk-ge-soc.o
obj-$(CONFIG_MTK_NET_PHYLIB)         += mtk-phy-lib.o
obj-$(CONFIG_MEDIATEK_GE_PHY)        += mtk-ge.o

> > diff --git a/drivers/net/phy/mediatek/mtk-2p5ge.c
> > b/drivers/net/phy/mediatek/mtk-2p5ge.c
> > new file mode 100644
> > index 000000000000..adb03df331ab
> > --- /dev/null
> > +++ b/drivers/net/phy/mediatek/mtk-2p5ge.c
> > @@ -0,0 +1,346 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +#include <linux/bitfield.h>
> > +#include <linux/firmware.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> 
> Is this header needed?
> 
> > +#include <linux/of_address.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/phy.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/pm_runtime.h>
> 
> And these two? Please only use those that are needed.
> 
I'll remove linux/nvmem-
consumer.h>/<linux/pm_domain.h>/<linux/pm_runtime.h> in v3. I forgot
removing them after developing this driver.

> > +static int mt798x_2p5ge_phy_load_fw(struct phy_device *phydev)
> > +{
> > +
> > +     writew(reg & ~MD32_EN, mcu_csr_base + MD32_EN_CFG);
> > +     writew(reg | MD32_EN, mcu_csr_base + MD32_EN_CFG);
> > +     phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
> > +     /* We need a delay here to stabilize initialization of MCU */
> > +     usleep_range(7000, 8000);
> 
> Does the reset bit clear when the MCU is ready? That is what 802.3
> defines. phy_poll_reset() might do what you need.

CL22 reset bit will be cleared right after I set it on MT7988.
usleep_range() here is used to allow the MCU to stabilize, rather than
to wait for the reset to complete.
> 
> > +     dev_info(dev, "Firmware loading/trigger ok.\n");
> 
> dev_dbg(), if at all. You have already spammed the log with the
> firmware version, so this adds nothing useful.

Well then... In v3, I'll remove this line and print firmware version at
last like this:

for (i = 0; i < MT7988_2P5GE_PMB_FW_SIZE - 1; i += 4)
	writel(*((uint32_t *)(fw->data + i)), pmb_addr + i);

writew(reg & ~MD32_EN, mcu_csr_base + MD32_EN_CFG);
writew(reg | MD32_EN, mcu_csr_base + MD32_EN_CFG);
phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
/* We need a delay here to stabilize initialization of MCU */
usleep_range(7000, 8000);

dev_info(dev, "Firmware date code: %x/%x/%x, version: %x.%x\n",
	 be16_to_cpu(*((__be16 *)(fw->data +
				  MT7988_2P5GE_PMB_FW_SIZE - 8))),
	 *(fw->data + MT7988_2P5GE_PMB_FW_SIZE - 6),
	 *(fw->data + MT7988_2P5GE_PMB_FW_SIZE - 5),
	 *(fw->data + MT7988_2P5GE_PMB_FW_SIZE - 2),
	 *(fw->data + MT7988_2P5GE_PMB_FW_SIZE - 1));

priv->fw_loaded = true;

> > +             phydev->duplex = DUPLEX_FULL;
> > +             /* FIXME:
> > +              * The current firmware always enables rate
> > adaptation mode.
> > +              */
> > +             phydev->rate_matching = RATE_MATCH_PAUSE;
> 
> Can we tell current firmware for future firmware? Is this actually
> fixable?
> 
We decided to always enable rate adaptation mode for MT7988's current
and future built-in 2.5GbE firmware. I'll remove FIXME comment here in
v3.

> > +     }
> > +
> > +     return 0;
> > +}
> > +
> 
> 
> 
> > +static int mt798x_2p5ge_phy_probe(struct phy_device *phydev)
> > +{
> > +     struct mtk_i2p5ge_phy_priv *priv;
> > +
> > +     priv = devm_kzalloc(&phydev->mdio.dev,
> > +                         sizeof(struct mtk_i2p5ge_phy_priv),
> > GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     switch (phydev->drv->phy_id) {
> > +     case MTK_2P5GPHY_ID_MT7988:
> > +             /* The original hardware only sets MDIO_DEVS_PMAPMD
> > */
> 
> What do you mean by "original hardware"?
> 
> You use PHY_ID_MATCH_MODEL(MTK_2P5GPHY_ID_MT7988), so do you mean
> revision 0 is broken, but revision 1 fixed it?
> 
I'll fix this ambiguous comment to:
/* This built-in 2.5GbE hardware only sets MDIO_DEVS_PMAPMD. Set the  *
rest by this driver since PCS/AN/VEND1/VEND2 mmds exist.
 */

> 
> > +             phydev->c45_ids.mmds_present |= MDIO_DEVS_PCS |
> > +                                             MDIO_DEVS_AN |
> > +                                             MDIO_DEVS_VEND1 |
> > +                                             MDIO_DEVS_VEND2;
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     priv->fw_loaded = false;
> > +     phydev->priv = priv;
> > +
> > +     mtk_phy_leds_state_init(phydev);
> 
> The LEDs work without firmware?
> 
>     Andrew
> 
> ---
> pw-bot: cr
I'll remove this line v3 and propose LED part in later patchset.

BRs,
Sky