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

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

On Wed, 2025-05-14 at 13:13 +0100, Russell King (Oracle) wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> 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.
> 
I'll clean this up in v4.

> > +#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.
> 
Thanks. I missed this part. I'll fix it in v4.

> > +     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.
> 
I'll move this part (LED initialization) to .probe then.

> ...
> 
> > +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.
> 
I'll replace "return __genphy_config_aneg(phydev, changed);" with
"return genphy_c45_check_and_restart_aneg(phydev, changed);".

BRs,
Sky

> --
> RMK's Patch system:
> https://urldefense.com/v3/__https://www.armlinux.org.uk/developer/patches/__;!!CTRNKA9wMg0ARbw!iEpNINN4map9ydq6phOk034MHxZo6vhBJJn7U5XFcEMHthdxJrjhgfHUdsH5R3wJAUyyPe_DlFIiuRWE0GE3Uz0$
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!