Messages in this thread | | | Date | Thu, 25 Apr 2024 10:01:58 +0100 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH 3/3] net: phy: mediatek: add support for built-in 2.5G ethernet PHY on MT7988 |
| |
On Thu, Apr 25, 2024 at 10:33:25AM +0800, Sky Huang wrote: > +static int mt7988_2p5ge_phy_config_init(struct phy_device *phydev) > +{ > + int ret, i; > + const struct firmware *fw; > + struct device *dev = &phydev->mdio.dev; > + struct device_node *np; > + void __iomem *pmb_addr; > + void __iomem *md32_en_cfg_base; > + struct mtk_i2p5ge_phy_priv *priv = phydev->priv; > + u16 reg; > + struct pinctrl *pinctrl; > + > + if (!priv->fw_loaded) { > + np = of_find_compatible_node(NULL, NULL, "mediatek,2p5gphy-fw"); > + if (!np) > + return -ENOENT; > + pmb_addr = of_iomap(np, 0); > + if (!pmb_addr) > + return -ENOMEM; > + md32_en_cfg_base = of_iomap(np, 1); > + if (!md32_en_cfg_base) > + return -ENOMEM;
Wouldn't it be better to do this in the probe function rather than here?
> + > + 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; > + }
This will block for userspace while holding phydev->lock and the RTNL. That blocks much of the networking APIs, which is not a good idea. If you have a number of these PHYs, then the RTNL will serialise the loading of firmware.
> + > + reg = readw(md32_en_cfg_base); > + if (reg & MD32_EN) { > + phy_set_bits(phydev, 0, BIT(15));
This is probably the BMCR, so if it is, please use the established definitions.
> + usleep_range(10000, 11000); > + } > + phy_set_bits(phydev, 0, BIT(11));
This also looks like it's probably the BMCR.
> + > + /* 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)), pmb_addr + i); > + release_firmware(fw); > + > + writew(reg & ~MD32_EN, md32_en_cfg_base); > + writew(reg | MD32_EN, md32_en_cfg_base); > + phy_set_bits(phydev, 0, BIT(15));
And also probably the BMCR.
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
| |