| Message ID | 20200325101736.2100-1-marex@denx.de |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RFC,1/2] ethtool: Add BroadRReach Master/Slave PHY tunable | expand |
On Wed, Mar 25, 2020 at 11:17:35AM +0100, Marek Vasut wrote: > Add a PHY tunable to select BroadRReach PHY Master/Slave mode. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Andrew Lunn <andrew@lunn.ch> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Heiner Kallweit <hkallweit1@gmail.com> > Cc: Jean Delvare <jdelvare@suse.com> > Cc: netdev@vger.kernel.org > --- > include/uapi/linux/ethtool.h | 1 + > net/core/ethtool.c | 2 ++ > 2 files changed, 3 insertions(+) > > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h > index dc69391d2bba..ebe658804ef1 100644 > --- a/include/uapi/linux/ethtool.h > +++ b/include/uapi/linux/ethtool.h > @@ -259,6 +259,7 @@ struct ethtool_tunable { > enum phy_tunable_id { > ETHTOOL_PHY_ID_UNSPEC, > ETHTOOL_PHY_DOWNSHIFT, > + ETHTOOL_PHY_BRR_MODE, > /* > * Add your fresh new phy tunable attribute above and remember to update > * phy_tunable_strings[] in net/core/ethtool.c > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index 09d828a6a173..553f3d0e2624 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c > @@ -133,6 +133,7 @@ static const char > phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN] = { > [ETHTOOL_ID_UNSPEC] = "Unspec", > [ETHTOOL_PHY_DOWNSHIFT] = "phy-downshift", > + [ETHTOOL_PHY_BRR_MODE] = "phy-broadrreach-mode", > }; > > static int ethtool_get_features(struct net_device *dev, void __user *useraddr) > @@ -2524,6 +2525,7 @@ static int ethtool_phy_tunable_valid(const struct ethtool_tunable *tuna) > { > switch (tuna->id) { > case ETHTOOL_PHY_DOWNSHIFT: > + case ETHTOOL_PHY_BRR_MODE: > if (tuna->len != sizeof(u8) || > tuna->type_id != ETHTOOL_TUNABLE_U8) > return -EINVAL; Hi Marek As far as i understand, there are only two settings. Master and Slave. So you can make the validation here more specific. I would also add some #defines for this in include/uapi/linux/ethtool.h so it is clear what value represents master and what is slave. Andrew
On 3/25/20 2:43 PM, Andrew Lunn wrote: > On Wed, Mar 25, 2020 at 11:17:35AM +0100, Marek Vasut wrote: >> Add a PHY tunable to select BroadRReach PHY Master/Slave mode. >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Andrew Lunn <andrew@lunn.ch> >> Cc: Florian Fainelli <f.fainelli@gmail.com> >> Cc: Guenter Roeck <linux@roeck-us.net> >> Cc: Heiner Kallweit <hkallweit1@gmail.com> >> Cc: Jean Delvare <jdelvare@suse.com> >> Cc: netdev@vger.kernel.org >> --- >> include/uapi/linux/ethtool.h | 1 + >> net/core/ethtool.c | 2 ++ >> 2 files changed, 3 insertions(+) >> >> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h >> index dc69391d2bba..ebe658804ef1 100644 >> --- a/include/uapi/linux/ethtool.h >> +++ b/include/uapi/linux/ethtool.h >> @@ -259,6 +259,7 @@ struct ethtool_tunable { >> enum phy_tunable_id { >> ETHTOOL_PHY_ID_UNSPEC, >> ETHTOOL_PHY_DOWNSHIFT, >> + ETHTOOL_PHY_BRR_MODE, >> /* >> * Add your fresh new phy tunable attribute above and remember to update >> * phy_tunable_strings[] in net/core/ethtool.c >> diff --git a/net/core/ethtool.c b/net/core/ethtool.c >> index 09d828a6a173..553f3d0e2624 100644 >> --- a/net/core/ethtool.c >> +++ b/net/core/ethtool.c >> @@ -133,6 +133,7 @@ static const char >> phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN] = { >> [ETHTOOL_ID_UNSPEC] = "Unspec", >> [ETHTOOL_PHY_DOWNSHIFT] = "phy-downshift", >> + [ETHTOOL_PHY_BRR_MODE] = "phy-broadrreach-mode", >> }; > >> >> static int ethtool_get_features(struct net_device *dev, void __user *useraddr) >> @@ -2524,6 +2525,7 @@ static int ethtool_phy_tunable_valid(const struct ethtool_tunable *tuna) >> { >> switch (tuna->id) { >> case ETHTOOL_PHY_DOWNSHIFT: >> + case ETHTOOL_PHY_BRR_MODE: >> if (tuna->len != sizeof(u8) || >> tuna->type_id != ETHTOOL_TUNABLE_U8) >> return -EINVAL; > > Hi Marek > > As far as i understand, there are only two settings. Master and > Slave. So you can make the validation here more specific. > > I would also add some #defines for this in > include/uapi/linux/ethtool.h so it is clear what value represents > master and what is slave. I'll let Oleksij take this series over, since he's working with the TJA1102 . In the meantime, I'll continue on the KS8851. Thanks for the feedback !
On Wed, Mar 25, 2020 at 11:17:35AM +0100, Marek Vasut wrote:
> Add a PHY tunable to select BroadRReach PHY Master/Slave mode.
IMHO this should be preceded by more general discussion about future of
ethtool tunables so I changed the subject and added people who were most
active in review of the ethtool netlink interface.
The way the ethtool tunables are designed rather feels like a workaround
for lack of extensibility of the ioctl interface. And at least in one
case (PFC stall timeout) it was actually the case:
http://lkml.kernel.org/r/CAKHjkjkGWoeeGXBSNZCcAND3bYaNhna-q1UAp=8UeeuBAN1=fQ@mail.gmail.com
Thus it's natural to ask if we want to preserve the idea of assorted
tunables in the netlink interface and add more or if we rather prefer
adding new attributes and finding suitable place for existing tunables.
Personally, I like the latter more.
What might be useful, on the other hand, would be device specific
tunables: an interface allowing device drivers to define a list of
tunables and their types for each device. It would be a generalization
of private flags. There is, of course, the risk that we could end up
with multiple NIC vendors defining the same parameters, each under
a different name and with slightly different semantics.
Ideas and opinions are welcome.
Michal
> What might be useful, on the other hand, would be device specific > tunables: an interface allowing device drivers to define a list of > tunables and their types for each device. It would be a generalization > of private flags. There is, of course, the risk that we could end up > with multiple NIC vendors defining the same parameters, each under > a different name and with slightly different semantics. Hi Michal I'm not too happy to let PHY drivers do whatever they want. So far, all PHY tunables have been generic. Any T1 PHY can implement control of master/slave, and there is no reason for each PHY to do it different to any other PHY. Downshift is a generic concept, multiple PHYs have implemented it, and they all implement it the same. Only Marvell currently supports fast link down, but the API is generic enough that other PHYs could implement it, if the hardware supports it. I don't however mind if it gets a different name, or a different tool, etc. I will let others comment on NICs. They are a different beast. Andrew
On 3/25/2020 2:55 PM, Andrew Lunn wrote: >> What might be useful, on the other hand, would be device specific >> tunables: an interface allowing device drivers to define a list of >> tunables and their types for each device. It would be a generalization >> of private flags. There is, of course, the risk that we could end up >> with multiple NIC vendors defining the same parameters, each under >> a different name and with slightly different semantics. > > Hi Michal > > I'm not too happy to let PHY drivers do whatever they want. So far, > all PHY tunables have been generic. Any T1 PHY can implement control > of master/slave, and there is no reason for each PHY to do it > different to any other PHY. Downshift is a generic concept, multiple > PHYs have implemented it, and they all implement it the same. Only > Marvell currently supports fast link down, but the API is generic > enough that other PHYs could implement it, if the hardware supports > it. > > I don't however mind if it gets a different name, or a different tool, > etc. BroadRReach is a standard feature that is available on other PHYs for instance (Broadcom at least has it too) so defining a common name for this particular tunable knob here would make sense. If we are to create vendor/device specific tunables, can we agree on a namespace to use, something like: <vendor>:<device>:<parameter name>
On Wed, Mar 25, 2020 at 03:04:07PM -0700, Florian Fainelli wrote: > > > On 3/25/2020 2:55 PM, Andrew Lunn wrote: > >> What might be useful, on the other hand, would be device specific > >> tunables: an interface allowing device drivers to define a list of > >> tunables and their types for each device. It would be a generalization > >> of private flags. There is, of course, the risk that we could end up > >> with multiple NIC vendors defining the same parameters, each under > >> a different name and with slightly different semantics. > > > > Hi Michal > > > > I'm not too happy to let PHY drivers do whatever they want. So far, > > all PHY tunables have been generic. Any T1 PHY can implement control > > of master/slave, and there is no reason for each PHY to do it > > different to any other PHY. Downshift is a generic concept, multiple > > PHYs have implemented it, and they all implement it the same. Only > > Marvell currently supports fast link down, but the API is generic > > enough that other PHYs could implement it, if the hardware supports > > it. > > > > I don't however mind if it gets a different name, or a different tool, > > etc. > > BroadRReach is a standard feature that is available on other PHYs for > instance (Broadcom at least has it too) so defining a common name for > this particular tunable knob here would make sense. > > If we are to create vendor/device specific tunables, can we agree on a > namespace to use, something like: > > <vendor>:<device>:<parameter name> That's not exactly what wanted to know. From my point of view, the most important question is if we want to preserve the concept of tunables as assorted parameters of various types, add netlink requests for querying and setting them (plus notifications) and keep adding new tunables. Or if we rather see them as a temporary workaround for the lack of extensibility and handle all future parameters through regular command line arguments and netlink attributes. For the record, I can imagine that the answer might be different for (netdev) tunables and for PHY tunables. Michal
On Wed, Mar 25, 2020 at 10:55:38PM +0100, Andrew Lunn wrote: > > What might be useful, on the other hand, would be device specific > > tunables: an interface allowing device drivers to define a list of > > tunables and their types for each device. It would be a generalization > > of private flags. There is, of course, the risk that we could end up > > with multiple NIC vendors defining the same parameters, each under > > a different name and with slightly different semantics. > > Hi Michal > > I'm not too happy to let PHY drivers do whatever they want. So far, > all PHY tunables have been generic. Any T1 PHY can implement control > of master/slave, and there is no reason for each PHY to do it > different to any other PHY. Downshift is a generic concept, multiple > PHYs have implemented it, and they all implement it the same. Only > Marvell currently supports fast link down, but the API is generic > enough that other PHYs could implement it, if the hardware supports > it. > > I don't however mind if it gets a different name, or a different tool, > etc. > > I will let others comment on NICs. They are a different beast. IMO, this is not a T1 specific feature. Since T1 lacks the auto negotiation functionality, we are forced to use Master/Slave configuration in one or other way. But even (non T1) PHYs with autoneg available, implement this feature. We may need to be able to force master or slave mode, at least as workaround for existing errata. Here is one example: ------------------------------------------------------------------------------- http://ww1.microchip.com/downloads/en/DeviceDoc/80000692D.pdf Module 2: Duty cycle variation for optional 125MHz reference output clock DESCRIPTION When the device links in the 1000BASE-T slave mode only, the optional 125MHz reference output clock (CLK125_NDO, Pin 41) has wide duty cycle variation. END USER IMPLICATIONS The optional CLK125_NDO clock does not meet the RGMII 45/55 percent (min/max) duty cycle requirement and there- fore cannot be used directly by the MAC side for clocking applications that have setup/hold time requirements on rising and falling clock edges (e.g., to clock out RGMII transmit data from MAC to PHY (KSZ9031RNX device)). Work around [...] Another solution requires the device to always operate in master mode (Register 9h, Bits [12:11] = '11') whenever there is 1000BASE-T link-up, which is workable only in those applications where the link partner is known and can always be configured to slave mode for 1000BASE-T. ------------------------------------------------------------------------------- In this example we see, that even on non T1 PHYs we sometimes want to force Master or Slave mode. At least for testing or workaround. The BASE-T1 related example is described in 802.3-2018: ------------------------------------------------------------------------------- 45.2.1.185.1 MASTER-SLAVE config value (1.2100.14) Bit 1.2100.14 is used to select MASTER or SLAVE operation when Auto-Negotiation enable bit 7.512.12 is set to zero, or if Auto-Negotiation is not implemented. If bit 1.2100.14 is set to one the PHY shall operate as MASTER. If bit 1.2100.14 is set to zero the PHY shall operate as SLAVE. This bit shall be ignored when the Auto-Negotiation enable bit 7.512.12 is set to one. ------------------------------------------------------------------------------- This example shows, that forcing Master or Slave modes is documented part of 802.3-2018 specification. IMO, this feature fits to the already existing LINKMODES_SET interface, as forcing of Master/Slave Mode only makes sense of autoneg is not implemented or disabled. LINKMODES_SET/GET: Request contents: ==================================== ====== ========================== ``ETHTOOL_A_LINKMODES_HEADER`` nested request header ``ETHTOOL_A_LINKMODES_AUTONEG`` u8 autonegotiation status ``ETHTOOL_A_LINKMODES_OURS`` bitset advertised link modes ``ETHTOOL_A_LINKMODES_PEER`` bitset partner link modes ``ETHTOOL_A_LINKMODES_SPEED`` u32 link speed (Mb/s) ``ETHTOOL_A_LINKMODES_DUPLEX`` u8 duplex mode ==================================== ====== ========================== Regards, Oleksij & Marc
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index dc69391d2bba..ebe658804ef1 100644 --- a/include/uapi/linux/ethtool.h +++ b/include/uapi/linux/ethtool.h @@ -259,6 +259,7 @@ struct ethtool_tunable { enum phy_tunable_id { ETHTOOL_PHY_ID_UNSPEC, ETHTOOL_PHY_DOWNSHIFT, + ETHTOOL_PHY_BRR_MODE, /* * Add your fresh new phy tunable attribute above and remember to update * phy_tunable_strings[] in net/core/ethtool.c diff --git a/net/core/ethtool.c b/net/core/ethtool.c index 09d828a6a173..553f3d0e2624 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -133,6 +133,7 @@ static const char phy_tunable_strings[__ETHTOOL_PHY_TUNABLE_COUNT][ETH_GSTRING_LEN] = { [ETHTOOL_ID_UNSPEC] = "Unspec", [ETHTOOL_PHY_DOWNSHIFT] = "phy-downshift", + [ETHTOOL_PHY_BRR_MODE] = "phy-broadrreach-mode", }; static int ethtool_get_features(struct net_device *dev, void __user *useraddr) @@ -2524,6 +2525,7 @@ static int ethtool_phy_tunable_valid(const struct ethtool_tunable *tuna) { switch (tuna->id) { case ETHTOOL_PHY_DOWNSHIFT: + case ETHTOOL_PHY_BRR_MODE: if (tuna->len != sizeof(u8) || tuna->type_id != ETHTOOL_TUNABLE_U8) return -EINVAL;
Add a PHY tunable to select BroadRReach PHY Master/Slave mode. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: Guenter Roeck <linux@roeck-us.net> Cc: Heiner Kallweit <hkallweit1@gmail.com> Cc: Jean Delvare <jdelvare@suse.com> Cc: netdev@vger.kernel.org --- include/uapi/linux/ethtool.h | 1 + net/core/ethtool.c | 2 ++ 2 files changed, 3 insertions(+)