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

[SkyLake Huang (黃啟澤)]

On Sun, 2025-01-19 at 18:31 +0100, Andrew Lunn wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> > +     np = of_find_compatible_node(NULL, NULL, "mediatek,2p5gphy-
> > fw");
> > +     if (!np)
> > +             return -ENOENT;
> 
> The device tree binding need documenting.
> 
> > +     /* 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);
> 
> 0x1100 and 0x00df are magic numbers, bit 0x800e and 0x800f are
> not. Please add #defines.
> 
> 
> > +
> > +     for (i = 0; i < MT7988_2P5GE_PMB_FW_SIZE - 1; i += 4)
> > +             writel(*((uint32_t *)(fw->data + i)), pmb_addr + i);
> > +     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));
> > +
> > +     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 loading/trigger ok.\n");
> 
> We generally don't spam the log for "Happy Days" conditions. Please
> only log if firmware download fails.
> 
Thanks. I'll fix all the above.

> > +static int mt798x_2p5ge_phy_get_features(struct phy_device
> > *phydev)
> > +{
> > +     int ret;
> > +
> > +     ret = genphy_c45_pma_read_abilities(phydev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* This phy can't handle collision, and neither can (XFI)MAC
> > it's
> > +      * connected to. Although it can do HDX handshake, it doesn't
> > support
> > +      * CSMA/CD that HDX requires.
> > +      */
> > +     linkmode_clear_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
> > +                        phydev->supported);
> 
> So it can do 10BaseT_Half? What about 1000BaseT_Half?
> 
> As you said somewhere, 10/100/1G are not in the C45 space. So does
> genphy_c45_pma_read_abilities() report these features?
> 
>         Andrew
Nope. It can neither do 10BaseT_Half nor 1000BaseT_Half. 10 & 1000
Base_Half bits are already cleared in firmware.

BRs,
Sky