| Message ID | 1479742524-30222-3-git-send-email-jbrunet@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Nov 21, 2016 at 04:35:23PM +0100, Jerome Brunet wrote: > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > Documentation/devicetree/bindings/net/phy.txt | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt > index bc1c3c8bf8fa..7f066b7c1e2c 100644 > --- a/Documentation/devicetree/bindings/net/phy.txt > +++ b/Documentation/devicetree/bindings/net/phy.txt > @@ -35,6 +35,11 @@ Optional Properties: > - broken-turn-around: If set, indicates the PHY device does not correctly > release the turn around line low at the end of a MDIO transaction. > > +- eee-advert-disable: Bits to clear in the MDIO_AN_EEE_ADV register to > + disable EEE modes. Example > + * 0x4: disable EEE for 1000T, > + * 0x6: disable EEE for 100TX and 1000T > + Hi Jerome I like the direction this patchset is taking. But hex values are pretty unfriendly. Please add a set of boolean properties, and do the mapping to hex in the C code. That would also make extending this API easier. e.g. say you have a 10Gbps PHY with EEE, and you need to disable it. This hex value quickly gets ugly, eee-advert-disable-10000 is nice and simple. Andrew
On Mon, 2016-11-21 at 17:01 +0100, Andrew Lunn wrote: > On Mon, Nov 21, 2016 at 04:35:23PM +0100, Jerome Brunet wrote: > > > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > > --- > > Documentation/devicetree/bindings/net/phy.txt | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/net/phy.txt > > b/Documentation/devicetree/bindings/net/phy.txt > > index bc1c3c8bf8fa..7f066b7c1e2c 100644 > > --- a/Documentation/devicetree/bindings/net/phy.txt > > +++ b/Documentation/devicetree/bindings/net/phy.txt > > @@ -35,6 +35,11 @@ Optional Properties: > > - broken-turn-around: If set, indicates the PHY device does not > > correctly > > release the turn around line low at the end of a MDIO > > transaction. > > > > +- eee-advert-disable: Bits to clear in the MDIO_AN_EEE_ADV > > register to > > + disable EEE modes. Example > > + * 0x4: disable EEE for 1000T, > > + * 0x6: disable EEE for 100TX and 1000T > > + > > Hi Jerome > > I like the direction this patchset is taking. But hex values are > pretty unfriendly. Agreed > Please add a set of boolean properties, and do the > mapping to hex in the C code. > > That would also make extending this API easier. e.g. say you have a > 10Gbps PHY with EEE, and you need to disable it. This hex value > quickly gets ugly, eee-advert-disable-10000 is nice and simple. 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. What do you think about keeping a single mask value but use the define above in the DT ? It would be more readable than hex and easy to extend, don't you think ? These defines are already part of the uapi so I guess we can use those in the DT bindings ? > > Andrew
> 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. > What do you think about keeping a single mask value but use the define > above in the DT ? It would be more readable than hex and easy to > extend, don't you think ? > > These defines are already part of the uapi so I guess we can use those > in the DT bindings ? I don't think they are accessible from the dtc include path. You will need to make a copy, in include/dt-bindings/net/phy.h But yes, using these defines is a good idea. Andrew
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. Fortunately, there does not seem to be a ton of PHYs out there which require EEE to be disabled to function properly so having individual properties vs. bitmasks/groups is kind of speculative here. 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, 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).
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
diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt index bc1c3c8bf8fa..7f066b7c1e2c 100644 --- a/Documentation/devicetree/bindings/net/phy.txt +++ b/Documentation/devicetree/bindings/net/phy.txt @@ -35,6 +35,11 @@ Optional Properties: - broken-turn-around: If set, indicates the PHY device does not correctly release the turn around line low at the end of a MDIO transaction. +- eee-advert-disable: Bits to clear in the MDIO_AN_EEE_ADV register to + disable EEE modes. Example + * 0x4: disable EEE for 1000T, + * 0x6: disable EEE for 100TX and 1000T + Example: ethernet-phy@0 {
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- Documentation/devicetree/bindings/net/phy.txt | 5 +++++ 1 file changed, 5 insertions(+)