Re: [PATCH net-next 3/3] net: phy: mediatek: support EcoNet EN751221 gbit SoC PHY

[Caleb James DeLisle]

On 26/08/2026 02:00, Andrew Lunn wrote:
> > +static int en751221_tx_amp_compensation_tbl[2][TRANSFORMER_TYPE_MAX][MDI_TYPE_MAX][CALIB_CONST_TYPE_MAX][4] = {
> const?
Ahh yes, indeed all of them should be. Note to Ansuel: This is relevant 
to your Airoha code as well.
> > +static u8 en751221_zcal_to_r50ohm[64] = {
> const?
>
> > +	125, 125, 125, 125, 125, 125, 124, 121, 118, 115, 112, 110, 108, 105, 103, 101,
> > +	 99,  97,  95,  77,  75,  73,  72,  70,  68,  67,  65,  64,  63,  45,  44,  43,
> > +	 41,  40,  39,  38,  37,  36,  35,  34,  32,  32,  31,  30,  13,  12,  11,  10,
> > +	  9,   8,   8,   7,   6,   5,   5,   4,   3,   2,   2,   1,   0,   0,   0,   0
> > +};
> > +/* EN751221 also mirrors RG_BG_RASEL into CHIP SCU register 0x16c. */
> > +static void en751221_gphy_mirror_rext(struct phy_device *phydev, u8 zcal)
> > +{
> > +	struct regmap *chip_scu;
> > +
> > +	chip_scu = syscon_regmap_lookup_by_compatible("econet,en751221-chip-scu");
> Does this need a change to the binding document?


Not as far as I know. econet,en751221-chip-scu is defined in 
mfd/syscon.yaml because it's a catch-all for configuration that the 
engineers didn't know what to do with.


> > +static int en751221_gphy_config_init(struct phy_device *phydev)
> > +{
> > +	phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, 0);
> Why is the EEE register being cleared?


From reading the reference implementation, I get the impression that 
this hardware is something of a basket case. There was a certain amount 
of "write three times and then read back" type magic that I just omitted 
because it really looks like they were actively debugging and as soon as 
it started working they shipped the code exactly as it was.


In the case of disabling EEE, I thought it more prudent to follow them 
because I don't have every SoC that this PHY ever appeared on and I 
would rather not diverge too greatly and risk it being unreliable on 
some devices.


> > +
> > +	phy_select_page(phydev, MTK_PHY_PAGE_EXTENDED_52B5);
> > +	__mtk_tr_write(phydev, 0x1, 0xf, 0x00, 0x00002b);
> > +	__mtk_tr_write(phydev, 0x1, 0xf, 0x03, 0x082422);
> > +	phy_restore_page(phydev, MTK_PHY_PAGE_STANDARD, 0);
> > +
> > +	ret = phy_write(phydev, MII_CTRL1000,
> > +			ADVERTISE_1000FULL | CTL1000_PREFER_MASTER |
> > +			CTL1000_AS_MASTER | CTL1000_ENABLE_MASTER);
> What does this default to?
It starts with only ADVERTISE_1000FULL. I don't know why the engineers 
wanted to set it to master, but its definitely intentional.
> > +
> > +	phy_set_bits(phydev, MII_BMCR, BMCR_ANRESTART | BMCR_ANENABLE);
> Why? config_aneg() will do this, if needed.


Okay this is probably just because the code I was working from is very 
old. I will review this and see if it's truly needed.


Also you're certainly more knowledgeable than me about what to expect 
from PHY hardware, so if it is your opinion that setting master and 
disabling EEE are probably not a real workaround for stability problems, 
then I'll be happy to give it a try with these omitted.


Thanks,

Caleb


>       Andrew