Messages in this thread | | | Date | Fri, 14 Apr 2023 03:49:08 +0200 | From | Andrew Lunn <> | Subject | Re: [PATCH net-next] net: phy: add driver for MediaTek SoC built-in GE PHYs |
| |
> +/* Registers on MDIO_MMD_VEND1 */ > +#define MTK_PHY_MIDDLE_LEVEL_SHAPPER_0TO1 0 > +#define MTK_PHY_1st_OVERSHOOT_LEVEL_0TO1 1 > +#define MTK_PHY_2nd_OVERSHOOT_LEVEL_0TO1 2 > +#define MTK_PHY_1st_OVERSHOOT_LEVEL_1TO0 4 > +#define MTK_PHY_2nd_OVERSHOOT_LEVEL_1TO0 5 /* N means negative */ > +#define MTK_PHY_1st_OVERSHOOT_LEVEL_0TON1 7 > +#define MTK_PHY_2nd_OVERSHOOT_LEVEL_0TON1 8 > +#define MTK_PHY_1st_OVERSHOOT_LEVEL_N1TO0 10 > +#define MTK_PHY_2nd_OVERSHOOT_LEVEL_N1TO0 11
Mixed case like this is very unusual.
> +static int tx_amp_fill_result(struct phy_device *phydev, u16 *buf) > +{ > + int i; > + int bias[16] = {0}; > + const int vals_9461[16] = { 7, 1, 4, 7, > + 7, 1, 4, 7, > + 7, 1, 4, 7, > + 7, 1, 4, 7 }; > + const int vals_9481[16] = { 10, 6, 6, 10, > + 10, 6, 6, 10, > + 10, 6, 6, 10, > + 10, 6, 6, 10 }; > + > + switch (phydev->drv->phy_id) { > + case MTK_GPHY_ID_MT7981: > + /* We add some calibration to efuse values > + * due to board level influence. > + * GBE: +7, TBT: +1, HBT: +4, TST: +7 > + */ > + memcpy(bias, (const void *)vals_9461, sizeof(bias)); > + for (i = 0; i <= 12; i += 4) { > + if (likely(buf[i >> 2] + bias[i] >= 32)) { > + bias[i] -= 13; > + } else { > + phy_modify_mmd(phydev, MDIO_MMD_VEND1, > + 0x5c, 0x7 << i, bias[i] << i); > + bias[i + 1] += 13; > + bias[i + 2] += 13; > + bias[i + 3] += 13;
How does 13 map to GBE: +7, TBT: +1, HBT: +4, TST: +7 ?
> +static inline void mt798x_phy_common_finetune(struct phy_device *phydev)
No inline functions in .c files. Let the compiler decide. There appears to be a number of these. And they are big function, too big to make sense to inline.
> static struct phy_driver mtk_gephy_driver[] = { > { > - PHY_ID_MATCH_EXACT(0x03a29412), > + PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7530), > .name = "MediaTek MT7530 PHY", > .config_init = mt7530_phy_config_init, > /* Interrupts are handled by the switch, not the PHY > @@ -84,7 +1205,7 @@ static struct phy_driver mtk_gephy_driver[] = { > .write_page = mtk_gephy_write_page, > }, > { > - PHY_ID_MATCH_EXACT(0x03a29441), > + PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7531), > .name = "MediaTek MT7531 PHY", > .config_init = mt7531_phy_config_init, > /* Interrupts are handled by the switch, not the PHY
Useful changes, but please put them in a separate patch.
> @@ -97,16 +1218,42 @@ static struct phy_driver mtk_gephy_driver[] = { > .read_page = mtk_gephy_read_page, > .write_page = mtk_gephy_write_page, > }, > +#if IS_ENABLED(CONFIG_MEDIATEK_GE_PHY_SOC) > + { > + PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7981), > + .name = "MediaTek MT7981 PHY", > + .probe = mt7981_phy_probe, > + .config_intr = genphy_no_config_intr, > + .handle_interrupt = genphy_handle_interrupt_no_ack, > + .suspend = genphy_suspend, > + .resume = genphy_resume, > + .read_page = mtk_gephy_read_page, > + .write_page = mtk_gephy_write_page, > + }, > + { > + PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7988), > + .name = "MediaTek MT7988 PHY", > + .probe = mt7988_phy_probe, > + .config_intr = genphy_no_config_intr, > + .handle_interrupt = genphy_handle_interrupt_no_ack, > + .suspend = genphy_suspend, > + .resume = genphy_resume, > + .read_page = mtk_gephy_read_page, > + .write_page = mtk_gephy_write_page,
So the only thing these two new PHYs share with the other two PHYs is mtk_gephy_read_page and mtk_gephy_write_page?
static int mtk_gephy_read_page(struct phy_device *phydev) { return __phy_read(phydev, MTK_EXT_PAGE_ACCESS); }
static int mtk_gephy_write_page(struct phy_device *phydev, int page) { return __phy_write(phydev, MTK_EXT_PAGE_ACCESS, page); }
Given the size of the new code, maybe consider adding mediatek-ge-soc.c and make a copy these two functions?
Andrew
| |