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

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

On Sat, 2024-05-18 at 03:29 +0200, Andrew Lunn wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  > +static int mt798x_2p5ge_phy_config_init(struct phy_device
> *phydev)
> > +{
> > +struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
> > +struct device *dev = &phydev->mdio.dev;
> > +const struct firmware *fw;
> > +struct pinctrl *pinctrl;
> > +int ret, i;
> > +u16 reg;
> > +
> > +if (!priv->fw_loaded) {
> > +if (!priv->md32_en_cfg_base || !priv->pmb_addr) {
> > +dev_err(dev, "MD32_EN_CFG base & PMB addresses aren't valid\n");
> > +return -EINVAL;
> > +}
> > +
> > +ret = request_firmware(&fw, MT7988_2P5GE_PMB, dev);
> > +if (ret) {
> > +dev_err(dev, "failed to load firmware: %s, ret: %d\n",
> > +MT7988_2P5GE_PMB, ret);
> > +return ret;
> > +}
> > +
> > +reg = readw(priv->md32_en_cfg_base);
> > +if (reg & MD32_EN) {
> > +phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
> > +usleep_range(10000, 11000);
> > +}
> > +phy_set_bits(phydev, MII_BMCR, BMCR_PDOWN);
> > +
> > +/* Write magic number to safely stall MCU */
> > +phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x800e, 0x1100);
> > +phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x800f, 0x00df);
> > +
> > +for (i = 0; i < fw->size - 1; i += 4)
> > +writel(*((uint32_t *)(fw->data + i)), priv->pmb_addr + i);
> 
> You should not trust the firmware. At least do a range check. How big
> is the SRAM the firmware is being written into? If you are given a
> firmware which is 1MB in size, what will happen?
> 
Fix in v3.

> > +release_firmware(fw);
> > +
> > +writew(reg & ~MD32_EN, priv->md32_en_cfg_base);
> > +writew(reg | MD32_EN, priv->md32_en_cfg_base);
> > +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 loading/trigger ok.\n");
> 
> Is there a version available anywhere for the firmware?
> 
Currently, I use "$md5sum /lib/firmware/mediatek/mt7988/i2p5ge-phy-
pmb.bin" command to check version.

> > +static int mt798x_2p5ge_phy_get_features(struct phy_device
> *phydev)
> > +{
> > +int ret;
> > +
> > +ret = genphy_c45_pma_read_abilities(phydev);
> > +if (ret)
> > +return ret;
> > +
> > +/* We don't support HDX at MAC layer on mt7988.
> 
> That is a MAC limitation, so it should be the MAC which disables
> this,
> not the Phy.
> 
Actually this phy is strictly binded to (XFI)MAC on this platform.
So I directly disable HDX feature of PHY.

> > +/* FIXME: AN device (MDIO_DEVS_AN)is indeed in this package.
> However, MDIO_DEVS_AN seems
> > + * that it won't be set as we detect phydev->c45_ids.mmds_present. 
> So Autoneg_BIT won't be
> > + * set in genphy_c45_pma_read_abilities(), either. Workaround here
> temporarily.
> > + */
> > +linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev-
> >supported);
> > +
> > +return 0;
> > +}
> > +
> > +static int mt798x_2p5ge_phy_read_status(struct phy_device *phydev)
> > +{
> > +u16 bmsr;
> > +int ret;
> > +
> > +/* Use this instead of genphy_c45_read_link() because MDIO_DEVS_AN
> bit isn't set in
> > + * phydev->c45_ids.mmds_present.
> 
> You have this twice now. Is the hardware broken? If so, maybe change
> phydev->c45_ids.mmds_present in the probe function to set the bit?
> 
Fix in v3.

> > + */
> > +ret = genphy_update_link(phydev);
> > +if (ret)
> > +return ret;
> > +
> > +phydev->speed = SPEED_UNKNOWN;
> > +phydev->duplex = DUPLEX_UNKNOWN;
> > +phydev->pause = 0;
> > +phydev->asym_pause = 0;
> > +
> > +/* We'll read link speed through vendor specific registers down
> below. So remove
> > + * phy_resolve_aneg_linkmode (AN on) & genphy_c45_read_pma (AN
> off).
> > + */
> > +if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) 
> {
> > +ret = genphy_c45_read_lpa(phydev);
> > +if (ret < 0)
> > +return ret;
> > +
> > +/* Clause 45 doesn't define 1000BaseT support. Read the link
> partner's 1G
> > + * advertisement via Clause 22
> > + */
> > +ret = phy_read(phydev, MII_STAT1000);
> > +if (ret < 0)
> > +return ret;
> > +mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, ret);
> > +} else if (phydev->autoneg == AUTONEG_DISABLE) {
> > +/* Mask link partner's all advertising capabilities when AN is
> off. In fact,
> > + * if we disable antuneg, we can't link up correctly:
> > + *   2.5G/1G: Need AN to exchange master/slave information.
> > + *   100M: Without AN, link starts at half duplex, which this phy
> doesn't support.
> > + *   10M: Deprecated in this ethernet phy.
> > + */
> 
> So it sounds like phydev->autoneg == AUTONEG_DISABLE is broken with
> this hardware. So just don't allow it, return -EOPNOTSUPP in
> config_aneg()
> 
>      Andrew
Fix in v3.