Re: [RFC PATCH net v2 2/3] dt: bindings: add ethernet phy eee-disable-advert option documentation

[Jerome Brunet]

On Mon, 2016-11-21 at 21:35 -0800, Florian Fainelli wrote:
> Le 21/11/2016 Ã 08:47, Andrew Lunn a Ãcrit :
> > 
> > > 
> > > What I did not realize when doing this patch for the realtek
> > > driver is
> > > that there is already 6 valid modes defined in the kernel
> > > 
> > > #define MDIO_EEE_100TX		MDIO_AN_EEE_ADV_100TX	
> > > /*
> > > 100TX EEE cap */
> > > #define MDIO_EEE_1000T		MDIO_AN_EEE_ADV_1000T	
> > > /*
> > > 1000T EEE cap */
> > > #define MDIO_EEE_10GT		0x0008	/* 10GT EEE
> > > cap */
> > > #define MDIO_EEE_1000KX		0x0010	/* 1000KX
> > > EEE cap
> > > */
> > > #define MDIO_EEE_10GKX4		0x0020	/* 10G KX4
> > > EEE cap
> > > */
> > > #define MDIO_EEE_10GKR		0x0040	/* 10G KR EEE
> > > cap
> > > */
> > > 
> > > I took care of only 2 in the case of realtek.c since it only
> > > support
> > > MDIO_EEE_100TX and MDIO_EEE_1000T.
> > > 
> > > Defining a property for each is certainly doable but it does not
> > > look
> > > very nice either. If it extends in the future, it will get even
> > > more
> > > messier, especially if you want to disable everything.
> > 
> > Yes, agreed.
> 
> One risk with the definition a group of advertisement capabilities
> (under the form of a bitmask for instance) to enable/disable is that
> we
> end up with Device Tree contain some kind of configuration policy as
> opposed to just flagging particular hardware features as broken.

The code proposed only allows to disable EEE advertisement (not
enable), so we should not see it used as a configuration policy in DT.
To make this more explicit, I could replace the property "eee-advert-
disable" by "eee-broken" ?

> 
> Fortunately, there does not seem to be a ton of PHYs out there which
> require EEE

It is quite difficult to have the real picture here because some PHYs
have EEE disabled by default and you have to explicitly enable it.
I have no idea of the ratio between the 2 phy policies.

> to be disabled to function properly so having individual
> properties vs. bitmasks/groups is kind of speculative here.

In the particular instance of the OdroidC2, disabling EEE for GbE only
enough. However, If you have a PHY broken with, I think it is likely
that you might want to disable all (supported) EEE modes. That's reason
why I prefer bitmask. I agree both are functionally similar, this is
kind of a cosmetic debate.

> 
> Another approach to solving this problem could be to register a PHY
> fixup which disables EEE at the PHY level, and which is only called
> for
> specific boards affected by this problem
> (of_machine_is_compatible()).
> This code can leave in arch/*/* when that is possible, 

That something I was looking at, but we don't have these files anymore
on ARM64 (looking at your comment, you already know this)

> or it can just be
> somewhere where it is relevant, e.g; in the PHY driver for instance
> (similarly to how PCI fixups are done).

Do you prefer having board specific code inside generic driver than
having the setting living in DT? Peppe told me they also had a few
platform with similar issues. The point is that this could be useful to
other people, so it could spread a grow a bit.

I would prefer having this in the DT, but I can definitely do it the
PHY withÂof_machine_is_compatible() and register_fixup is this what you
prefer/want.Â

Cheers
Jerome