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

[Andrew Lunn]

> > 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?

> > > +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.

> > > +	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