Message ID | 20240530034844.11176-6-SkyLake.Huang@mediatek.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | net: phy: mediatek: Introduce mtk-phy-lib and add 2.5Gphy support | expand |
On Thu, May 30, 2024 at 11:48:44AM +0800, Sky Huang wrote: > +static int mt798x_2p5ge_phy_config_aneg(struct phy_device *phydev) > +{ > + bool changed = false; > + u32 adv; > + int ret; > + > + /* In fact, if we disable autoneg, we can't link up correctly: > + * 2.5G/1G: Need AN to exchange master/slave information. > + * 100M: Without AN, link starts at half duplex(According to IEEE 802.3-2018), > + * which this phy doesn't support. > + * 10M: Deprecated in this ethernet phy. > + */ > + if (phydev->autoneg == AUTONEG_DISABLE) > + return -EOPNOTSUPP; We have another driver (stmmac) where a platform driver is wanting to put a hack in the ksettings_set() ethtool path to error out on disabling AN for 1G speeds. This sounds like something that is applicable to more than one hardware (and I've been wondering whether it is universally true that 1G copper links and faster all require AN to function.) Thus, I'm wondering whether this is something that the core code should be doing. > + /* 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. > + */ What the MAC can and can't do really has little bearing on what link modes the PHY driver should be providing. It is the responsibility of the MAC driver to appropriately change what is supported when attaching to the PHY. If using phylink, this is done by phylink via the MAC driver telling phylink what it is capable of via mac_capabilities. > +static int mt798x_2p5ge_phy_get_rate_matching(struct phy_device *phydev, > + phy_interface_t iface) > +{ > + if (iface == PHY_INTERFACE_MODE_XGMII) > + return RATE_MATCH_PAUSE; You mention above XFI... XFI is 10GBASE-R protocol to XFP module electrical standards. SFI is 10GBASE-R protocol to SFP+ module electrical standards. phy_interface_t is interested in the protocol. So, given that you mention XFI, why doesn't this test for PHY_INTERFACE_MODE_10GBASER? > +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 */ > + phydev->c45_ids.mmds_present |= (MDIO_DEVS_PCS | MDIO_DEVS_AN | > + MDIO_DEVS_VEND1 | MDIO_DEVS_VEND2); No need for parens on the RHS. The RHS is an expression in its own right, and there's no point in putting parens around the expression to turn it into another expression!
On Thu, 2024-05-30 at 11:35 +0100, Russell King (Oracle) wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On Thu, May 30, 2024 at 11:48:44AM +0800, Sky Huang wrote: > > +static int mt798x_2p5ge_phy_config_aneg(struct phy_device *phydev) > > +{ > > +bool changed = false; > > +u32 adv; > > +int ret; > > + > > +/* In fact, if we disable autoneg, we can't link up correctly: > > + * 2.5G/1G: Need AN to exchange master/slave information. > > + * 100M: Without AN, link starts at half duplex(According to IEEE > 802.3-2018), > > + * which this phy doesn't support. > > + * 10M: Deprecated in this ethernet phy. > > + */ > > +if (phydev->autoneg == AUTONEG_DISABLE) > > +return -EOPNOTSUPP; > > We have another driver (stmmac) where a platform driver is wanting to > put a hack in the ksettings_set() ethtool path to error out on > disabling AN for 1G speeds. This sounds like something that is > applicable to more than one hardware (and I've been wondering whether > it is universally true that 1G copper links and faster all require > AN to function.) > > Thus, I'm wondering whether this is something that the core code > should > be doing. > Yeah..As far as I know, 1G/2.5G/5G/10G speed require AN to decide master/slave role. Actually I can use force mode by calling genphy_c45_pma_set_forced, which will set correspoding C45 registers. However, after that, this 2.5G PHY can't still link up with partners. I'll leave EOPNOTSUPP here temporarily. Hope phylib can be patched someday. > > +/* 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. > > + */ > > What the MAC can and can't do really has little bearing on what link > modes the PHY driver should be providing. It is the responsibility of > the MAC driver to appropriately change what is supported when > attaching > to the PHY. If using phylink, this is done by phylink via the MAC > driver > telling phylink what it is capable of via mac_capabilities. > > > +static int mt798x_2p5ge_phy_get_rate_matching(struct phy_device > *phydev, > > + phy_interface_t iface) > > +{ > > +if (iface == PHY_INTERFACE_MODE_XGMII) > > +return RATE_MATCH_PAUSE; > > You mention above XFI... > > XFI is 10GBASE-R protocol to XFP module electrical standards. > SFI is 10GBASE-R protocol to SFP+ module electrical standards. > > phy_interface_t is interested in the protocol. So, given that you > mention XFI, why doesn't this test for PHY_INTERFACE_MODE_10GBASER? > We have 2 XFI-MAC on mt7988 platform. One is connected to internal 2.5Gphy(SoC built-in), as we discussed here (We don't test this phy for 10G speed.) Another one is connected to external 10G phy. > > +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 */ > > +phydev->c45_ids.mmds_present |= (MDIO_DEVS_PCS | MDIO_DEVS_AN | > > + MDIO_DEVS_VEND1 | MDIO_DEVS_VEND2); > > No need for parens on the RHS. The RHS is an expression in its own > right, and there's no point in putting parens around the expression > to turn it into another expression! > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! Do you mean these two line? +phydev->c45_ids.mmds_present |= (MDIO_DEVS_PCS | MDIO_DEVS_AN | + MDIO_DEVS_VEND1 | MDIO_DEVS_VEND2); What do you mean by "RHS is an expression in its own right"? I put parens here to enhance readability so we don't need check operator precedence again. Sky
On Thu, May 30, 2024 at 04:25:56PM +0000, SkyLake Huang (黃啟澤) wrote: > On Thu, 2024-05-30 at 11:35 +0100, Russell King (Oracle) wrote: > > > > External email : Please do not click links or open attachments until > > you have verified the sender or the content. > > On Thu, May 30, 2024 at 11:48:44AM +0800, Sky Huang wrote: > > > +static int mt798x_2p5ge_phy_config_aneg(struct phy_device *phydev) > > > +{ > > > +bool changed = false; > > > +u32 adv; > > > +int ret; > > > + > > > +/* In fact, if we disable autoneg, we can't link up correctly: > > > + * 2.5G/1G: Need AN to exchange master/slave information. > > > + * 100M: Without AN, link starts at half duplex(According to IEEE > > 802.3-2018), > > > + * which this phy doesn't support. > > > + * 10M: Deprecated in this ethernet phy. > > > + */ > > > +if (phydev->autoneg == AUTONEG_DISABLE) > > > +return -EOPNOTSUPP; > > > > We have another driver (stmmac) where a platform driver is wanting to > > put a hack in the ksettings_set() ethtool path to error out on > > disabling AN for 1G speeds. This sounds like something that is > > applicable to more than one hardware (and I've been wondering whether > > it is universally true that 1G copper links and faster all require > > AN to function.) > > > > Thus, I'm wondering whether this is something that the core code > > should > > be doing. > > > Yeah..As far as I know, 1G/2.5G/5G/10G speed require AN to decide > master/slave role. Actually I can use force mode by calling > genphy_c45_pma_set_forced, which will set correspoding C45 registers. > However, after that, this 2.5G PHY can't still link up with partners. > > I'll leave EOPNOTSUPP here temporarily. Hope phylib can be patched > someday. Please no. "someday" tends to never happen, and you're basically throwing the problem over the wall to other people to solve who then have to spot your hack and eventually remove it. We need this solved properly, not by people hacking drivers. This is open source, you can propose a patch to phylib to fix this for everyone. > > > +/* 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. > > > + */ > > > > What the MAC can and can't do really has little bearing on what link > > modes the PHY driver should be providing. It is the responsibility of > > the MAC driver to appropriately change what is supported when > > attaching > > to the PHY. If using phylink, this is done by phylink via the MAC > > driver > > telling phylink what it is capable of via mac_capabilities. > > > > > +static int mt798x_2p5ge_phy_get_rate_matching(struct phy_device > > *phydev, > > > + phy_interface_t iface) > > > +{ > > > +if (iface == PHY_INTERFACE_MODE_XGMII) > > > +return RATE_MATCH_PAUSE; > > > > You mention above XFI... > > > > XFI is 10GBASE-R protocol to XFP module electrical standards. > > SFI is 10GBASE-R protocol to SFP+ module electrical standards. > > > > phy_interface_t is interested in the protocol. So, given that you > > mention XFI, why doesn't this test for PHY_INTERFACE_MODE_10GBASER? > > > We have 2 XFI-MAC on mt7988 platform. One is connected to internal > 2.5Gphy(SoC built-in), as we discussed here (We don't test this phy for > 10G speed.) Another one is connected to external 10G phy. I can't parse your response in a meaningful way, to me it doesn't address my point. > > > > +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 */ > > > +phydev->c45_ids.mmds_present |= (MDIO_DEVS_PCS | MDIO_DEVS_AN | > > > + MDIO_DEVS_VEND1 | MDIO_DEVS_VEND2); > > > > No need for parens on the RHS. The RHS is an expression in its own > > right, and there's no point in putting parens around the expression > > to turn it into another expression! > > > > -- > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! > > Do you mean these two line? > +phydev->c45_ids.mmds_present |= (MDIO_DEVS_PCS | MDIO_DEVS_AN | > + MDIO_DEVS_VEND1 | MDIO_DEVS_VEND2); > > What do you mean by "RHS is an expression in its own right"? > I put parens here to enhance readability so we don't need check > operator precedence again. |= one of the assignment operators, all of which have one of the lowest precedence. Only the , operator has a lower precedence. Therefore, everything except , has higher precedence. Therefore, the parens on the right hand side of |= make no difference.
Hi Sky, kernel test robot noticed the following build warnings: [auto build test WARNING on net-next/main] url: https://github.com/intel-lab-lkp/linux/commits/Sky-Huang/net-phy-mediatek-Re-organize-MediaTek-ethernet-phy-drivers/20240530-115522 base: net-next/main patch link: https://lore.kernel.org/r/20240530034844.11176-6-SkyLake.Huang%40mediatek.com patch subject: [PATCH net-next v5 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988 config: openrisc-randconfig-r121-20240531 (https://download.01.org/0day-ci/archive/20240531/202405310819.c3Dv1OM3-lkp@intel.com/config) compiler: or1k-linux-gcc (GCC) 13.2.0 reproduce: (https://download.01.org/0day-ci/archive/20240531/202405310819.c3Dv1OM3-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202405310819.c3Dv1OM3-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/net/phy/mediatek/mtk-2p5ge.c:106:9: sparse: sparse: cast to restricted __be16 vim +106 drivers/net/phy/mediatek/mtk-2p5ge.c 56 57 static int mt798x_2p5ge_phy_load_fw(struct phy_device *phydev) 58 { 59 struct mtk_i2p5ge_phy_priv *priv = phydev->priv; 60 void __iomem *md32_en_cfg_base, *pmb_addr; 61 struct device *dev = &phydev->mdio.dev; 62 const struct firmware *fw; 63 int ret, i; 64 u16 reg; 65 66 if (priv->fw_loaded) 67 return 0; 68 69 pmb_addr = ioremap(MT7988_2P5GE_PMB_BASE, MT7988_2P5GE_PMB_LEN); 70 if (!pmb_addr) 71 return -ENOMEM; 72 md32_en_cfg_base = ioremap(MT7988_2P5GE_MD32_EN_CFG_BASE, MT7988_2P5GE_MD32_EN_CFG_LEN); 73 if (!md32_en_cfg_base) { 74 ret = -ENOMEM; 75 goto free_pmb; 76 } 77 78 ret = request_firmware(&fw, MT7988_2P5GE_PMB, dev); 79 if (ret) { 80 dev_err(dev, "failed to load firmware: %s, ret: %d\n", 81 MT7988_2P5GE_PMB, ret); 82 goto free; 83 } 84 85 if (fw->size != MT7988_2P5GE_PMB_SIZE) { 86 dev_err(dev, "Firmware size 0x%zx != 0x%x\n", 87 fw->size, MT7988_2P5GE_PMB_SIZE); 88 ret = -EINVAL; 89 goto free; 90 } 91 92 reg = readw(md32_en_cfg_base); 93 if (reg & MD32_EN) { 94 phy_set_bits(phydev, MII_BMCR, BMCR_RESET); 95 usleep_range(10000, 11000); 96 } 97 phy_set_bits(phydev, MII_BMCR, BMCR_PDOWN); 98 99 /* Write magic number to safely stall MCU */ 100 phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x800e, 0x1100); 101 phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x800f, 0x00df); 102 103 for (i = 0; i < MT7988_2P5GE_PMB_SIZE - 1; i += 4) 104 writel(*((uint32_t *)(fw->data + i)), pmb_addr + i); 105 release_firmware(fw); > 106 dev_info(dev, "Firmware date code: %x/%x/%x, version: %x.%x\n", 107 be16_to_cpu(*((uint16_t *)(fw->data + MT7988_2P5GE_PMB_SIZE - 8))), 108 *(fw->data + MT7988_2P5GE_PMB_SIZE - 6), 109 *(fw->data + MT7988_2P5GE_PMB_SIZE - 5), 110 *(fw->data + MT7988_2P5GE_PMB_SIZE - 2), 111 *(fw->data + MT7988_2P5GE_PMB_SIZE - 1)); 112 113 writew(reg & ~MD32_EN, md32_en_cfg_base); 114 writew(reg | MD32_EN, md32_en_cfg_base); 115 phy_set_bits(phydev, MII_BMCR, BMCR_RESET); 116 /* We need a delay here to stabilize initialization of MCU */ 117 usleep_range(7000, 8000); 118 dev_info(dev, "Firmware loading/trigger ok.\n"); 119 120 priv->fw_loaded = true; 121 122 free: 123 iounmap(md32_en_cfg_base); 124 free_pmb: 125 iounmap(pmb_addr); 126 127 return ret ? ret : 0; 128 } 129
On Thu, May 30, 2024 at 11:48:44AM +0800, Sky Huang wrote: > From: "SkyLake.Huang" <skylake.huang@mediatek.com> > > v1: > Add support for internal 2.5Gphy on MT7988. This driver will load > necessary firmware, add appropriate time delay and figure out LED. > Also, certain control registers will be set to fix link-up issues. > > v2: > 1. Move md32_en_cfg_base & pmb_addr detection in probe function. > 2. Do not read PMB & MD32_EN_CFG base addresses from dts. We won't > change that from board to board. Leave them in driver code. Also, > release those addresses after firmware is triggered. > 3. Remove half duplex code which leads to ambiguity. Those are for > testing & developing previously. > 4. Use correct BMCR definitions. > 5. Correct config_aneg / get_features / read_status functions. > 6. Change mt7988_2p5ge prefix to mt798x_2p5ge in case that our next > platform uses this 2.5Gphy driver. > > v3: > 1. Add range check for firmware. > 2. Fix c45_ids.mmds_present in probe function. > 3. Still use genphy_update_link() in read_status because > genphy_c45_read_link() can't correct detect link on this phy. > > v4: > 1. Move firmware loading function to mt798x_2p5ge_phy_load_fw() > 2. Add AN disable warning in mt798x_2p5ge_phy_config_aneg() > 3. Clarify the HDX comments in mt798x_2p5ge_phy_get_features() > > v5: > 1. Move md32_en_cfg_base & pmb_addr to local variables to achieve > symmetric code. > 2. Print out firmware date code & version. > 3. Don't return error if LED pinctrl switching fails. Also, add > comments to this unusual operations. > 4. Return -EOPNOTSUPP for AN off case in config_aneg(). > Hi Sky, This is a somewhat unusual way to arrange a patch description. Usually the description describes the change, particularly why the change is being made. While the per-version changes are listed below the scissors ("---"). > Signed-off-by: SkyLake.Huang <skylake.huang@mediatek.com> ... > diff --git a/drivers/net/phy/mediatek/mtk-2p5ge.c b/drivers/net/phy/mediatek/mtk-2p5ge.c ... > +static int mt798x_2p5ge_phy_load_fw(struct phy_device *phydev) > +{ > + struct mtk_i2p5ge_phy_priv *priv = phydev->priv; > + void __iomem *md32_en_cfg_base, *pmb_addr; > + struct device *dev = &phydev->mdio.dev; > + const struct firmware *fw; > + int ret, i; > + u16 reg; > + > + if (priv->fw_loaded) > + return 0; > + > + pmb_addr = ioremap(MT7988_2P5GE_PMB_BASE, MT7988_2P5GE_PMB_LEN); > + if (!pmb_addr) > + return -ENOMEM; > + md32_en_cfg_base = ioremap(MT7988_2P5GE_MD32_EN_CFG_BASE, MT7988_2P5GE_MD32_EN_CFG_LEN); nit: Networking still prefers code to be 80 columns wide or less. It looks like that can be trivially achieved here and several other places in this patch. OTOH, I don't think there is no need to break lines to meet this requirement where it is particularly awkward to do so. Flagged by checkpatch.pl --max-line-length=80 > + if (!md32_en_cfg_base) { > + ret = -ENOMEM; > + goto free_pmb; > + } > + > + 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); > + goto free; > + } > + > + if (fw->size != MT7988_2P5GE_PMB_SIZE) { > + dev_err(dev, "Firmware size 0x%zx != 0x%x\n", > + fw->size, MT7988_2P5GE_PMB_SIZE); > + ret = -EINVAL; > + goto free; > + } > + > + reg = readw(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 < MT7988_2P5GE_PMB_SIZE - 1; i += 4) > + writel(*((uint32_t *)(fw->data + i)), pmb_addr + i); > + release_firmware(fw); > + dev_info(dev, "Firmware date code: %x/%x/%x, version: %x.%x\n", > + be16_to_cpu(*((uint16_t *)(fw->data + MT7988_2P5GE_PMB_SIZE - 8))), If the data at fw->data + MT7988_2P5GE_PMB_SIZE - 8 is a 16-bit Big Endian value, then I think the cast should be to __be16 rather than uint16_t (and in any case u16 would be preferred to uint16_t as this is Kernel code). Flagged by Sparse. > + *(fw->data + MT7988_2P5GE_PMB_SIZE - 6), > + *(fw->data + MT7988_2P5GE_PMB_SIZE - 5), > + *(fw->data + MT7988_2P5GE_PMB_SIZE - 2), > + *(fw->data + MT7988_2P5GE_PMB_SIZE - 1)); > + > + writew(reg & ~MD32_EN, md32_en_cfg_base); > + writew(reg | MD32_EN, 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"); > + > + priv->fw_loaded = true; > + > +free: > + iounmap(md32_en_cfg_base); > +free_pmb: > + iounmap(pmb_addr); > + > + return ret ? ret : 0; > +} ... > +static int mt798x_2p5ge_phy_led_blink_set(struct phy_device *phydev, u8 index, > + unsigned long *delay_on, > + unsigned long *delay_off) > +{ > + bool blinking = false; > + int err = 0; > + struct mtk_i2p5ge_phy_priv *priv = phydev->priv; nit: Please consider arranging local variables in reverse xmas tree order - longest line to shortest. Edward Cree's tool can be helpful: https://github.com/ecree-solarflare/xmastree > + > + if (index > 1) > + return -EINVAL; > + > + if (delay_on && delay_off && (*delay_on > 0) && (*delay_off > 0)) { > + blinking = true; > + *delay_on = 50; > + *delay_off = 50; > + } > + > + err = mtk_phy_hw_led_blink_set(phydev, index, &priv->led_state, blinking); > + if (err) > + return err; > + > + return mtk_phy_hw_led_on_set(phydev, index, &priv->led_state, > + MTK_2P5GPHY_LED_ON_MASK, false); > +} ...
On Thu, 2024-05-30 at 17:55 +0100, Russell King (Oracle) wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On Thu, May 30, 2024 at 04:25:56PM +0000, SkyLake Huang (黃啟澤) wrote: > > On Thu, 2024-05-30 at 11:35 +0100, Russell King (Oracle) wrote: > > > > > > External email : Please do not click links or open attachments > until > > > you have verified the sender or the content. > > > On Thu, May 30, 2024 at 11:48:44AM +0800, Sky Huang wrote: > > > > +static int mt798x_2p5ge_phy_config_aneg(struct phy_device > *phydev) > > > > +{ > > > > +bool changed = false; > > > > +u32 adv; > > > > +int ret; > > > > + > > > > +/* In fact, if we disable autoneg, we can't link up correctly: > > > > + * 2.5G/1G: Need AN to exchange master/slave information. > > > > + * 100M: Without AN, link starts at half duplex(According to > IEEE > > > 802.3-2018), > > > > + * which this phy doesn't support. > > > > + * 10M: Deprecated in this ethernet phy. > > > > + */ > > > > +if (phydev->autoneg == AUTONEG_DISABLE) > > > > +return -EOPNOTSUPP; > > > > > > We have another driver (stmmac) where a platform driver is > wanting to > > > put a hack in the ksettings_set() ethtool path to error out on > > > disabling AN for 1G speeds. This sounds like something that is > > > applicable to more than one hardware (and I've been wondering > whether > > > it is universally true that 1G copper links and faster all > require > > > AN to function.) > > > > > > Thus, I'm wondering whether this is something that the core code > > > should > > > be doing. > > > > > Yeah..As far as I know, 1G/2.5G/5G/10G speed require AN to decide > > master/slave role. Actually I can use force mode by calling > > genphy_c45_pma_set_forced, which will set correspoding C45 > registers. > > However, after that, this 2.5G PHY can't still link up with > partners. > > > > I'll leave EOPNOTSUPP here temporarily. Hope phylib can be patched > > someday. > > Please no. "someday" tends to never happen, and you're basically > throwing the problem over the wall to other people to solve who > then have to spot your hack and eventually remove it. > > We need this solved properly, not by people hacking drivers. This > is open source, you can propose a patch to phylib to fix this for > everyone. > I don't intend to throw problems to other people. And actually this isn't a "problem" currently(at least in this driver). IMHO, disabling AN isn't a normal operation for current ethernet environment. However, now, ethtool supports this kind of configuration. Maybe we should prohibit "AN disable" config for certain speed? Or maybe for all speed? This will take lots of time for discussion. No matter what, these discussions have little relevance to this driver. For your reference, there's the same design in en8811h_config_aneg() of drivers/net/phy/air_en8811h.c. > > > > +/* 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. > > > > + */ > > > > > > What the MAC can and can't do really has little bearing on what > link > > > modes the PHY driver should be providing. It is the > responsibility of > > > the MAC driver to appropriately change what is supported when > > > attaching > > > to the PHY. If using phylink, this is done by phylink via the MAC > > > driver > > > telling phylink what it is capable of via mac_capabilities. > > > > > > > +static int mt798x_2p5ge_phy_get_rate_matching(struct > phy_device > > > *phydev, > > > > + phy_interface_t iface) > > > > +{ > > > > +if (iface == PHY_INTERFACE_MODE_XGMII) > > > > +return RATE_MATCH_PAUSE; > > > > > > You mention above XFI... > > > > > > XFI is 10GBASE-R protocol to XFP module electrical standards. > > > SFI is 10GBASE-R protocol to SFP+ module electrical standards. > > > > > > phy_interface_t is interested in the protocol. So, given that you > > > mention XFI, why doesn't this test for > PHY_INTERFACE_MODE_10GBASER? > > > > > We have 2 XFI-MAC on mt7988 platform. One is connected to internal > > 2.5Gphy(SoC built-in), as we discussed here (We don't test this phy > for > > 10G speed.) Another one is connected to external 10G phy. > > I can't parse your response in a meaningful way, to me it doesn't > address my point. > I guess I got your point. On mt7988 1st XFI-MAC (XGMAC2): For built-in 2.5Gphy or external 10Gphy 2nd XFI-MAC (XGMAC3): Only for external 10Gphy Basically, if we use this driver for built-in 2.5Gphy. We'll only pass phy-mode = "xgmii" or phy-mode = "internal" in dts, i.e, PHY_INTERFACE_MODE_XGMII or PHY_INTERFACE_MODE_INTERNAL. Also, we pass phy-mode = "usxgmii" (PHY_INTERFACE_MODE_10GBASER) in dts once we use external 10Gphy with XGMAC2. So we don't test PHY_INTERFACE_MODE_10GBASER or PHY_INTERFACE_MODE_10GKR in driver's rate_matching(). But I think I should change it this way: if (iface == PHY_INTERFACE_MODE_XGMII || iface == PHY_INTERFACE_MODE_INTERNAL) return RATE_MATCH_PAUSE; return RATE_MATCH_NONE; > > > > > > +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 */ > > > > +phydev->c45_ids.mmds_present |= (MDIO_DEVS_PCS | MDIO_DEVS_AN > | > > > > + MDIO_DEVS_VEND1 | MDIO_DEVS_VEND2); > > > > > > No need for parens on the RHS. The RHS is an expression in its > own > > > right, and there's no point in putting parens around the > expression > > > to turn it into another expression! > > > > > > -- > > > RMK's Patch system: > https://www.armlinux.org.uk/developer/patches/ > > > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! > > > > Do you mean these two line? > > +phydev->c45_ids.mmds_present |= (MDIO_DEVS_PCS | MDIO_DEVS_AN | > > + MDIO_DEVS_VEND1 | MDIO_DEVS_VEND2); > > > > What do you mean by "RHS is an expression in its own right"? > > I put parens here to enhance readability so we don't need check > > operator precedence again. > > |= one of the assignment operators, all of which have one of the > lowest precedence. Only the , operator has a lower precedence. > Therefore, everything except , has higher precedence. Therefore, > the parens on the right hand side of |= make no difference. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! I'll fix this in v6. Sky
On Sat, 2024-06-01 at 13:51 +0100, Simon Horman wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On Thu, May 30, 2024 at 11:48:44AM +0800, Sky Huang wrote: > > From: "SkyLake.Huang" <skylake.huang@mediatek.com> > > > > v1: > > Add support for internal 2.5Gphy on MT7988. This driver will load > > necessary firmware, add appropriate time delay and figure out LED. > > Also, certain control registers will be set to fix link-up issues. > > > > v2: > > 1. Move md32_en_cfg_base & pmb_addr detection in probe function. > > 2. Do not read PMB & MD32_EN_CFG base addresses from dts. We won't > > change that from board to board. Leave them in driver code. Also, > > release those addresses after firmware is triggered. > > 3. Remove half duplex code which leads to ambiguity. Those are for > > testing & developing previously. > > 4. Use correct BMCR definitions. > > 5. Correct config_aneg / get_features / read_status functions. > > 6. Change mt7988_2p5ge prefix to mt798x_2p5ge in case that our next > > platform uses this 2.5Gphy driver. > > > > v3: > > 1. Add range check for firmware. > > 2. Fix c45_ids.mmds_present in probe function. > > 3. Still use genphy_update_link() in read_status because > > genphy_c45_read_link() can't correct detect link on this phy. > > > > v4: > > 1. Move firmware loading function to mt798x_2p5ge_phy_load_fw() > > 2. Add AN disable warning in mt798x_2p5ge_phy_config_aneg() > > 3. Clarify the HDX comments in mt798x_2p5ge_phy_get_features() > > > > v5: > > 1. Move md32_en_cfg_base & pmb_addr to local variables to achieve > > symmetric code. > > 2. Print out firmware date code & version. > > 3. Don't return error if LED pinctrl switching fails. Also, add > > comments to this unusual operations. > > 4. Return -EOPNOTSUPP for AN off case in config_aneg(). > > > > Hi Sky, > > This is a somewhat unusual way to arrange a patch description. > > Usually the description describes the change, particularly why > the change is being made. > > While the per-version changes are listed below the scissors ("---"). > > > Signed-off-by: SkyLake.Huang <skylake.huang@mediatek.com> > > ... > > > diff --git a/drivers/net/phy/mediatek/mtk-2p5ge.c > b/drivers/net/phy/mediatek/mtk-2p5ge.c > > ... > > > +static int mt798x_2p5ge_phy_load_fw(struct phy_device *phydev) > > +{ > > +struct mtk_i2p5ge_phy_priv *priv = phydev->priv; > > +void __iomem *md32_en_cfg_base, *pmb_addr; > > +struct device *dev = &phydev->mdio.dev; > > +const struct firmware *fw; > > +int ret, i; > > +u16 reg; > > + > > +if (priv->fw_loaded) > > +return 0; > > + > > +pmb_addr = ioremap(MT7988_2P5GE_PMB_BASE, MT7988_2P5GE_PMB_LEN); > > +if (!pmb_addr) > > +return -ENOMEM; > > +md32_en_cfg_base = ioremap(MT7988_2P5GE_MD32_EN_CFG_BASE, > MT7988_2P5GE_MD32_EN_CFG_LEN); > > nit: Networking still prefers code to be 80 columns wide or less. > It looks like that can be trivially achieved here and > several other places in this patch. > > OTOH, I don't think there is no need to break lines to meet this > requirement where it is particularly awkward to do so. > > Flagged by checkpatch.pl --max-line-length=80 > > > +if (!md32_en_cfg_base) { > > +ret = -ENOMEM; > > +goto free_pmb; > > +} > > + > > +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); > > +goto free; > > +} > > + > > +if (fw->size != MT7988_2P5GE_PMB_SIZE) { > > +dev_err(dev, "Firmware size 0x%zx != 0x%x\n", > > +fw->size, MT7988_2P5GE_PMB_SIZE); > > +ret = -EINVAL; > > +goto free; > > +} > > + > > +reg = readw(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 < MT7988_2P5GE_PMB_SIZE - 1; i += 4) > > +writel(*((uint32_t *)(fw->data + i)), pmb_addr + i); > > +release_firmware(fw); > > +dev_info(dev, "Firmware date code: %x/%x/%x, version: %x.%x\n", > > + be16_to_cpu(*((uint16_t *)(fw->data + MT7988_2P5GE_PMB_SIZE - > 8))), > > If the data at fw->data + MT7988_2P5GE_PMB_SIZE - 8 is a 16-bit > Big Endian value, then I think the cast should be to __be16 rather > than uint16_t (and in any case u16 would be preferred to uint16_t > as this is Kernel code). > > Flagged by Sparse. > > > + *(fw->data + MT7988_2P5GE_PMB_SIZE - 6), > > + *(fw->data + MT7988_2P5GE_PMB_SIZE - 5), > > + *(fw->data + MT7988_2P5GE_PMB_SIZE - 2), > > + *(fw->data + MT7988_2P5GE_PMB_SIZE - 1)); > > + > > +writew(reg & ~MD32_EN, md32_en_cfg_base); > > +writew(reg | MD32_EN, 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"); > > + > > +priv->fw_loaded = true; > > + > > +free: > > +iounmap(md32_en_cfg_base); > > +free_pmb: > > +iounmap(pmb_addr); > > + > > +return ret ? ret : 0; > > +} > > ... > > > +static int mt798x_2p5ge_phy_led_blink_set(struct phy_device > *phydev, u8 index, > > + unsigned long *delay_on, > > + unsigned long *delay_off) > > +{ > > +bool blinking = false; > > +int err = 0; > > +struct mtk_i2p5ge_phy_priv *priv = phydev->priv; > > nit: Please consider arranging local variables in reverse xmas tree > order - > longest line to shortest. > > Edward Cree's tool can be helpful: > https://github.com/ecree-solarflare/xmastree > > > + > > +if (index > 1) > > +return -EINVAL; > > + > > +if (delay_on && delay_off && (*delay_on > 0) && (*delay_off > 0)) > { > > +blinking = true; > > +*delay_on = 50; > > +*delay_off = 50; > > +} > > + > > +err = mtk_phy_hw_led_blink_set(phydev, index, &priv->led_state, > blinking); > > +if (err) > > +return err; > > + > > +return mtk_phy_hw_led_on_set(phydev, index, &priv->led_state, > > + MTK_2P5GPHY_LED_ON_MASK, false); > > +} > > ... Thanks. I'll fix all of the above in v6. Sky
diff --git a/MAINTAINERS b/MAINTAINERS index e58e05c..fe380f2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13793,6 +13793,7 @@ M: Qingfang Deng <dqfext@gmail.com> M: SkyLake Huang <SkyLake.Huang@mediatek.com> L: netdev@vger.kernel.org S: Maintained +F: drivers/net/phy/mediatek/mtk-2p5ge.c F: drivers/net/phy/mediatek/mtk-ge-soc.c F: drivers/net/phy/mediatek/mtk-phy-lib.c F: drivers/net/phy/mediatek/mtk-ge.c diff --git a/drivers/net/phy/mediatek/Kconfig b/drivers/net/phy/mediatek/Kconfig index 448bc20..1490352 100644 --- a/drivers/net/phy/mediatek/Kconfig +++ b/drivers/net/phy/mediatek/Kconfig @@ -25,3 +25,14 @@ config MEDIATEK_GE_SOC_PHY the MT7981 and MT7988 SoCs. These PHYs need calibration data present in the SoCs efuse and will dynamically calibrate VCM (common-mode voltage) during startup. + +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. diff --git a/drivers/net/phy/mediatek/Makefile b/drivers/net/phy/mediatek/Makefile index 814879d..c6db8ab 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 diff --git a/drivers/net/phy/mediatek/mtk-2p5ge.c b/drivers/net/phy/mediatek/mtk-2p5ge.c new file mode 100644 index 0000000..33d1c8d --- /dev/null +++ b/drivers/net/phy/mediatek/mtk-2p5ge.c @@ -0,0 +1,422 @@ +// SPDX-License-Identifier: GPL-2.0+ +#include <linux/bitfield.h> +#include <linux/firmware.h> +#include <linux/module.h> +#include <linux/nvmem-consumer.h> +#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> + +#include "mtk.h" + +#define MTK_2P5GPHY_ID_MT7988 (0x00339c11) + +#define MT7988_2P5GE_PMB "mediatek/mt7988/i2p5ge-phy-pmb.bin" +#define MT7988_2P5GE_PMB_SIZE (0x20000) +#define MT7988_2P5GE_PMB_BASE (0x0f100000) +#define MT7988_2P5GE_PMB_LEN (0x20000) +#define MT7988_2P5GE_MD32_EN_CFG_BASE (0x0f0f0018) +#define MT7988_2P5GE_MD32_EN_CFG_LEN (0x20) +#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) + +#define MTK_PHY_LPI_PCS_DSP_CTRL (0x121) +#define MTK_PHY_LPI_SIG_EN_LO_THRESH100_MASK GENMASK(12, 8) + +/* Registers on MTK phy page 1*/ +#define MTK_PHY_PAGE_EXTENDED_1 0x0001 +#define MTK_PHY_AUX_CTRL_AND_STATUS (0x14) +#define MTK_PHY_ENABLE_DOWNSHIFT BIT(4) + +/* Registers on Token Ring debug nodes */ +/* ch_addr = 0x0, node_addr = 0xf, data_addr = 0x3c */ +#define AUTO_NP_10XEN BIT(6) + +struct mtk_i2p5ge_phy_priv { + bool fw_loaded; + unsigned long led_state; +}; + +enum { + PHY_AUX_SPD_10 = 0, + PHY_AUX_SPD_100, + PHY_AUX_SPD_1000, + PHY_AUX_SPD_2500, +}; + +static int mt798x_2p5ge_phy_load_fw(struct phy_device *phydev) +{ + struct mtk_i2p5ge_phy_priv *priv = phydev->priv; + void __iomem *md32_en_cfg_base, *pmb_addr; + struct device *dev = &phydev->mdio.dev; + const struct firmware *fw; + int ret, i; + u16 reg; + + if (priv->fw_loaded) + return 0; + + pmb_addr = ioremap(MT7988_2P5GE_PMB_BASE, MT7988_2P5GE_PMB_LEN); + if (!pmb_addr) + return -ENOMEM; + md32_en_cfg_base = ioremap(MT7988_2P5GE_MD32_EN_CFG_BASE, MT7988_2P5GE_MD32_EN_CFG_LEN); + if (!md32_en_cfg_base) { + ret = -ENOMEM; + goto free_pmb; + } + + 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); + goto free; + } + + if (fw->size != MT7988_2P5GE_PMB_SIZE) { + dev_err(dev, "Firmware size 0x%zx != 0x%x\n", + fw->size, MT7988_2P5GE_PMB_SIZE); + ret = -EINVAL; + goto free; + } + + reg = readw(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 < MT7988_2P5GE_PMB_SIZE - 1; i += 4) + writel(*((uint32_t *)(fw->data + i)), pmb_addr + i); + release_firmware(fw); + dev_info(dev, "Firmware date code: %x/%x/%x, version: %x.%x\n", + be16_to_cpu(*((uint16_t *)(fw->data + MT7988_2P5GE_PMB_SIZE - 8))), + *(fw->data + MT7988_2P5GE_PMB_SIZE - 6), + *(fw->data + MT7988_2P5GE_PMB_SIZE - 5), + *(fw->data + MT7988_2P5GE_PMB_SIZE - 2), + *(fw->data + MT7988_2P5GE_PMB_SIZE - 1)); + + writew(reg & ~MD32_EN, md32_en_cfg_base); + writew(reg | MD32_EN, 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"); + + priv->fw_loaded = true; + +free: + iounmap(md32_en_cfg_base); +free_pmb: + iounmap(pmb_addr); + + return ret ? ret : 0; +} + +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev) +{ + struct pinctrl *pinctrl; + int ret; + + ret = mt798x_2p5ge_phy_load_fw(phydev); + if (ret < 0) + return ret; + + /* 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"); + + phy_modify_mmd(phydev, MDIO_MMD_VEND1, MTK_PHY_LPI_PCS_DSP_CTRL, + MTK_PHY_LPI_SIG_EN_LO_THRESH100_MASK, 0); + + /* Enable 16-bit next page exchange bit if 1000-BT isn't advertising */ + tr_modify(phydev, 0x0, 0xf, 0x3c, AUTO_NP_10XEN, + FIELD_PREP(AUTO_NP_10XEN, 0x1)); + + /* Enable HW auto downshift */ + phy_modify_paged(phydev, MTK_PHY_PAGE_EXTENDED_1, MTK_PHY_AUX_CTRL_AND_STATUS, + 0, MTK_PHY_ENABLE_DOWNSHIFT); + + return 0; +} + +static int mt798x_2p5ge_phy_config_aneg(struct phy_device *phydev) +{ + bool changed = false; + u32 adv; + int ret; + + /* In fact, if we disable autoneg, we can't link up correctly: + * 2.5G/1G: Need AN to exchange master/slave information. + * 100M: Without AN, link starts at half duplex(According to IEEE 802.3-2018), + * which this phy doesn't support. + * 10M: Deprecated in this ethernet phy. + */ + if (phydev->autoneg == AUTONEG_DISABLE) + return -EOPNOTSUPP; + + 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_c45_check_and_restart_aneg(phydev, changed); +} + +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); + + return 0; +} + +static int mt798x_2p5ge_phy_read_status(struct phy_device *phydev) +{ + int ret; + + /* When MDIO_STAT1_LSTATUS is raised genphy_c45_read_link(), this phy actually + * hasn't finished AN. So use CL22's link update function instead. + */ + 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) { + linkmode_zero(phydev->lp_advertising); + } + + if (phydev->link) { + ret = phy_read(phydev, PHY_AUX_CTRL_STATUS); + if (ret < 0) + return ret; + + switch (FIELD_GET(PHY_AUX_SPEED_MASK, ret)) { + case PHY_AUX_SPD_10: + phydev->speed = SPEED_10; + break; + case PHY_AUX_SPD_100: + phydev->speed = SPEED_100; + break; + case PHY_AUX_SPD_1000: + phydev->speed = SPEED_1000; + break; + case PHY_AUX_SPD_2500: + phydev->speed = SPEED_2500; + break; + } + + phydev->duplex = DUPLEX_FULL; + /* FIXME: The current firmware always enables rate adaptation mode. */ + phydev->rate_matching = RATE_MATCH_PAUSE; + } + + return 0; +} + +static int mt798x_2p5ge_phy_get_rate_matching(struct phy_device *phydev, + phy_interface_t iface) +{ + if (iface == PHY_INTERFACE_MODE_XGMII) + return RATE_MATCH_PAUSE; + return RATE_MATCH_NONE; +} + +static const unsigned long supported_triggers = (BIT(TRIGGER_NETDEV_FULL_DUPLEX) | + BIT(TRIGGER_NETDEV_LINK) | + BIT(TRIGGER_NETDEV_LINK_10) | + BIT(TRIGGER_NETDEV_LINK_100) | + BIT(TRIGGER_NETDEV_LINK_1000) | + BIT(TRIGGER_NETDEV_LINK_2500) | + BIT(TRIGGER_NETDEV_RX) | + BIT(TRIGGER_NETDEV_TX)); + +static int mt798x_2p5ge_phy_led_blink_set(struct phy_device *phydev, u8 index, + unsigned long *delay_on, + unsigned long *delay_off) +{ + bool blinking = false; + int err = 0; + struct mtk_i2p5ge_phy_priv *priv = phydev->priv; + + if (index > 1) + return -EINVAL; + + if (delay_on && delay_off && (*delay_on > 0) && (*delay_off > 0)) { + blinking = true; + *delay_on = 50; + *delay_off = 50; + } + + err = mtk_phy_hw_led_blink_set(phydev, index, &priv->led_state, blinking); + if (err) + return err; + + return mtk_phy_hw_led_on_set(phydev, index, &priv->led_state, + MTK_2P5GPHY_LED_ON_MASK, false); +} + +static int mt798x_2p5ge_phy_led_brightness_set(struct phy_device *phydev, + u8 index, enum led_brightness value) +{ + int err; + struct mtk_i2p5ge_phy_priv *priv = phydev->priv; + + err = mtk_phy_hw_led_blink_set(phydev, index, &priv->led_state, false); + if (err) + return err; + + return mtk_phy_hw_led_on_set(phydev, index, &priv->led_state, + MTK_2P5GPHY_LED_ON_MASK, (value != LED_OFF)); +} + +static int mt798x_2p5ge_phy_led_hw_is_supported(struct phy_device *phydev, u8 index, + unsigned long rules) +{ + return mtk_phy_led_hw_is_supported(phydev, index, rules, supported_triggers); +} + +static int mt798x_2p5ge_phy_led_hw_control_get(struct phy_device *phydev, u8 index, + unsigned long *rules) +{ + struct mtk_i2p5ge_phy_priv *priv = phydev->priv; + + return mtk_phy_led_hw_ctrl_get(phydev, index, rules, &priv->led_state, + MTK_2P5GPHY_LED_ON_SET, + MTK_2P5GPHY_LED_RX_BLINK_SET, + MTK_2P5GPHY_LED_TX_BLINK_SET); +}; + +static int mt798x_2p5ge_phy_led_hw_control_set(struct phy_device *phydev, u8 index, + unsigned long rules) +{ + struct mtk_i2p5ge_phy_priv *priv = phydev->priv; + + return mtk_phy_led_hw_ctrl_set(phydev, index, rules, &priv->led_state, + MTK_2P5GPHY_LED_ON_SET, + MTK_2P5GPHY_LED_RX_BLINK_SET, + MTK_2P5GPHY_LED_TX_BLINK_SET); +}; + +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 */ + 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); + + return 0; +} + +static struct phy_driver mtk_gephy_driver[] = { + { + PHY_ID_MATCH_MODEL(MTK_2P5GPHY_ID_MT7988), + .name = "MediaTek MT7988 2.5GbE PHY", + .probe = mt798x_2p5ge_phy_probe, + .config_init = mt798x_2p5ge_phy_config_init, + .config_aneg = mt798x_2p5ge_phy_config_aneg, + .get_features = mt798x_2p5ge_phy_get_features, + .read_status = mt798x_2p5ge_phy_read_status, + .get_rate_matching = mt798x_2p5ge_phy_get_rate_matching, + .suspend = genphy_suspend, + .resume = genphy_resume, + .read_page = mtk_phy_read_page, + .write_page = mtk_phy_write_page, + .led_blink_set = mt798x_2p5ge_phy_led_blink_set, + .led_brightness_set = mt798x_2p5ge_phy_led_brightness_set, + .led_hw_is_supported = mt798x_2p5ge_phy_led_hw_is_supported, + .led_hw_control_get = mt798x_2p5ge_phy_led_hw_control_get, + .led_hw_control_set = mt798x_2p5ge_phy_led_hw_control_set, + }, +}; + +module_phy_driver(mtk_gephy_driver); + +static struct mdio_device_id __maybe_unused mtk_2p5ge_phy_tbl[] = { + { PHY_ID_MATCH_VENDOR(0x00339c00) }, + { } +}; + +MODULE_DESCRIPTION("MediaTek 2.5Gb Ethernet PHY driver"); +MODULE_AUTHOR("SkyLake Huang <SkyLake.Huang@mediatek.com>"); +MODULE_LICENSE("GPL"); + +MODULE_DEVICE_TABLE(mdio, mtk_2p5ge_phy_tbl);