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

[Caleb James DeLisle]

On 26/08/2026 04:21, Andrew Lunn wrote:
> > > 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.
> Do you need a property in the PHY node to make this work?


No it's not requiring anything in the PHY node, instead it fishes for 
econet,en751221-chip-scu in the DT. I'm not much a fan of this pattern 
because of the magic, but this way of accessing is fairly common and in 
this case it is how Airoha likes to access the chip-scu thingy when they 
need it. 
https://lore.kernel.org/all/20241112-clk-en7581-syscon-v2-4-8ada5e394ae4@xxxxxxxxxx/


> > > > +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.
> Is EEE broken? If it is, this is not the correct way to disable
> it. You should call phy_disable_eee(phydev); This will also prevent
> user space enabling it again.


I tested my board removing the EEE disable and the MASTER line, and it 
seems to work. I can't get any CRC errors flooding the link, and 
MII_STAT1000 reads LPA_1000LOCALRXOK | LPA_1000REMRXOK | LPA_1000FULL 
which suggests to me that it's able to operate happily in slave mode.


So unless you or someone else thinks it's inadvisable, I'm inclined to 
just enable these things and then wait to see if anybody finds a board 
that has problems.


Thanks,

Caleb


> > > > +	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.
> Generally, switches take the master role, and client take the slave
> role. So this does make sense for a PHY used in a switch.
>
> 	Andrew