netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable
@ 2020-03-25 10:17 Marek Vasut
  2020-03-25 10:17 ` [RFC][PATCH 2/2] net: phy: tja11xx: Add BroadRReach Master/Slave support into TJA11xx PHY driver Marek Vasut
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Marek Vasut @ 2020-03-25 10:17 UTC (permalink / raw)
  To: netdev
  Cc: o.rempel, Marek Vasut, Andrew Lunn, Florian Fainelli,
	Guenter Roeck, Heiner Kallweit, Jean Delvare

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;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [RFC][PATCH 2/2] net: phy: tja11xx: Add BroadRReach Master/Slave support into TJA11xx PHY driver
  2020-03-25 10:17 [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable Marek Vasut
@ 2020-03-25 10:17 ` Marek Vasut
  2020-03-25 13:47   ` Andrew Lunn
  2020-03-25 13:43 ` [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable Andrew Lunn
  2020-03-25 16:49 ` RFC: future of ethtool tunables (Re: [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable) Michal Kubecek
  2 siblings, 1 reply; 10+ messages in thread
From: Marek Vasut @ 2020-03-25 10:17 UTC (permalink / raw)
  To: netdev
  Cc: o.rempel, Marek Vasut, Andrew Lunn, Florian Fainelli,
	Guenter Roeck, Heiner Kallweit, Jean Delvare, linux-hwmon

Add PHY tunable callbacks to allow configuring BroadRReach
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: linux-hwmon@vger.kernel.org
---
 drivers/net/phy/nxp-tja11xx.c | 58 +++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index f1aeef449c33..78f6594a6acc 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -26,6 +26,7 @@
 #define MII_ECTRL_WAKE_REQUEST		BIT(0)
 
 #define MII_CFG1			18
+#define MII_CFG1_MASTER_SLAVE		BIT(15)
 #define MII_CFG1_AUTO_OP		BIT(14)
 #define MII_CFG1_SLEEP_CONFIRM		BIT(6)
 #define MII_CFG1_LED_MODE_MASK		GENMASK(5, 4)
@@ -115,6 +116,11 @@ static int tja11xx_enable_link_control(struct phy_device *phydev)
 	return phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_LINK_CONTROL);
 }
 
+static int tja11xx_disable_link_control(struct phy_device *phydev)
+{
+	return phy_clear_bits(phydev, MII_ECTRL, MII_ECTRL_LINK_CONTROL);
+}
+
 static int tja11xx_wakeup(struct phy_device *phydev)
 {
 	int ret;
@@ -299,6 +305,52 @@ static void tja11xx_get_stats(struct phy_device *phydev,
 	}
 }
 
+static int tja11xx_get_tunable(struct phy_device *phydev,
+			       struct ethtool_tunable *tuna, void *data)
+{
+	u8 *mode = (u8 *)data;
+	int ret;
+
+	switch (tuna->id) {
+	case ETHTOOL_PHY_BRR_MODE:
+		ret = phy_read(phydev, MII_CFG1);
+		if (ret < 0)
+			return ret;
+		*mode = !!(ret & MII_CFG1_MASTER_SLAVE);
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int tja11xx_set_tunable(struct phy_device *phydev,
+			       struct ethtool_tunable *tuna, const void *data)
+{
+	u8 mode = *(u8 *)data;
+	int ret;
+
+	switch (tuna->id) {
+	case ETHTOOL_PHY_BRR_MODE:
+		ret = tja11xx_disable_link_control(phydev);
+		if (ret)
+			return ret;
+
+		ret = phy_modify(phydev, MII_CFG1, MII_CFG1_MASTER_SLAVE,
+				 mode ? MII_CFG1_MASTER_SLAVE : 0);
+		if (ret)
+			return ret;
+
+		ret = tja11xx_enable_link_control(phydev);
+		if (ret)
+			return ret;
+
+		/* TJA1100 needs this to bring the link back up. */
+		return genphy_soft_reset(phydev);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static int tja11xx_hwmon_read(struct device *dev,
 			      enum hwmon_sensor_types type,
 			      u32 attr, int channel, long *value)
@@ -425,6 +477,9 @@ static struct phy_driver tja11xx_driver[] = {
 		.get_sset_count = tja11xx_get_sset_count,
 		.get_strings	= tja11xx_get_strings,
 		.get_stats	= tja11xx_get_stats,
+		/* Tunables */
+		.get_tunable	= tja11xx_get_tunable,
+		.set_tunable	= tja11xx_set_tunable,
 	}, {
 		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
 		.name		= "NXP TJA1101",
@@ -444,6 +499,9 @@ static struct phy_driver tja11xx_driver[] = {
 		.get_sset_count = tja11xx_get_sset_count,
 		.get_strings	= tja11xx_get_strings,
 		.get_stats	= tja11xx_get_stats,
+		/* Tunables */
+		.get_tunable	= tja11xx_get_tunable,
+		.set_tunable	= tja11xx_set_tunable,
 	}
 };
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable
  2020-03-25 10:17 [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable Marek Vasut
  2020-03-25 10:17 ` [RFC][PATCH 2/2] net: phy: tja11xx: Add BroadRReach Master/Slave support into TJA11xx PHY driver Marek Vasut
@ 2020-03-25 13:43 ` Andrew Lunn
  2020-03-25 13:51   ` Marek Vasut
  2020-03-25 16:49 ` RFC: future of ethtool tunables (Re: [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable) Michal Kubecek
  2 siblings, 1 reply; 10+ messages in thread
From: Andrew Lunn @ 2020-03-25 13:43 UTC (permalink / raw)
  To: Marek Vasut
  Cc: netdev, o.rempel, Florian Fainelli, Guenter Roeck,
	Heiner Kallweit, Jean Delvare

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC][PATCH 2/2] net: phy: tja11xx: Add BroadRReach Master/Slave support into TJA11xx PHY driver
  2020-03-25 10:17 ` [RFC][PATCH 2/2] net: phy: tja11xx: Add BroadRReach Master/Slave support into TJA11xx PHY driver Marek Vasut
@ 2020-03-25 13:47   ` Andrew Lunn
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Lunn @ 2020-03-25 13:47 UTC (permalink / raw)
  To: Marek Vasut
  Cc: netdev, o.rempel, Florian Fainelli, Guenter Roeck,
	Heiner Kallweit, Jean Delvare, linux-hwmon

> +static int tja11xx_get_tunable(struct phy_device *phydev,
> +			       struct ethtool_tunable *tuna, void *data)
> +{
> +	u8 *mode = (u8 *)data;
> +	int ret;
> +
> +	switch (tuna->id) {
> +	case ETHTOOL_PHY_BRR_MODE:
> +		ret = phy_read(phydev, MII_CFG1);
> +		if (ret < 0)
> +			return ret;
> +		*mode = !!(ret & MII_CFG1_MASTER_SLAVE);

It is not so easy to see if master is 1 or 0?


> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int tja11xx_set_tunable(struct phy_device *phydev,
> +			       struct ethtool_tunable *tuna, const void *data)
> +{
> +	u8 mode = *(u8 *)data;
> +	int ret;
> +
> +	switch (tuna->id) {
> +	case ETHTOOL_PHY_BRR_MODE:
> +		ret = tja11xx_disable_link_control(phydev);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_modify(phydev, MII_CFG1, MII_CFG1_MASTER_SLAVE,
> +				 mode ? MII_CFG1_MASTER_SLAVE : 0);

And if the user passes 42?

    Andrew

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable
  2020-03-25 13:43 ` [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable Andrew Lunn
@ 2020-03-25 13:51   ` Marek Vasut
  0 siblings, 0 replies; 10+ messages in thread
From: Marek Vasut @ 2020-03-25 13:51 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, o.rempel, Florian Fainelli, Guenter Roeck,
	Heiner Kallweit, Jean Delvare

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 !

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RFC: future of ethtool tunables (Re: [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable)
  2020-03-25 10:17 [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable Marek Vasut
  2020-03-25 10:17 ` [RFC][PATCH 2/2] net: phy: tja11xx: Add BroadRReach Master/Slave support into TJA11xx PHY driver Marek Vasut
  2020-03-25 13:43 ` [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable Andrew Lunn
@ 2020-03-25 16:49 ` Michal Kubecek
  2020-03-25 21:55   ` Andrew Lunn
  2 siblings, 1 reply; 10+ messages in thread
From: Michal Kubecek @ 2020-03-25 16:49 UTC (permalink / raw)
  To: netdev
  Cc: Marek Vasut, o.rempel, Andrew Lunn, Florian Fainelli,
	Guenter Roeck, Heiner Kallweit, Jean Delvare, Jiri Pirko,
	Jakub Kicinski

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: RFC: future of ethtool tunables (Re: [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable)
  2020-03-25 16:49 ` RFC: future of ethtool tunables (Re: [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable) Michal Kubecek
@ 2020-03-25 21:55   ` Andrew Lunn
  2020-03-25 22:04     ` Florian Fainelli
  2020-03-26  9:05     ` Oleksij Rempel
  0 siblings, 2 replies; 10+ messages in thread
From: Andrew Lunn @ 2020-03-25 21:55 UTC (permalink / raw)
  To: Michal Kubecek
  Cc: netdev, Marek Vasut, o.rempel, Florian Fainelli, Guenter Roeck,
	Heiner Kallweit, Jean Delvare, Jiri Pirko, Jakub Kicinski

> 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




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: RFC: future of ethtool tunables (Re: [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable)
  2020-03-25 21:55   ` Andrew Lunn
@ 2020-03-25 22:04     ` Florian Fainelli
  2020-03-25 23:42       ` Michal Kubecek
  2020-03-26  9:05     ` Oleksij Rempel
  1 sibling, 1 reply; 10+ messages in thread
From: Florian Fainelli @ 2020-03-25 22:04 UTC (permalink / raw)
  To: Andrew Lunn, Michal Kubecek
  Cc: netdev, Marek Vasut, o.rempel, Guenter Roeck, Heiner Kallweit,
	Jean Delvare, Jiri Pirko, Jakub Kicinski



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: RFC: future of ethtool tunables (Re: [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable)
  2020-03-25 22:04     ` Florian Fainelli
@ 2020-03-25 23:42       ` Michal Kubecek
  0 siblings, 0 replies; 10+ messages in thread
From: Michal Kubecek @ 2020-03-25 23:42 UTC (permalink / raw)
  To: netdev
  Cc: Florian Fainelli, Andrew Lunn, Marek Vasut, o.rempel,
	Guenter Roeck, Heiner Kallweit, Jean Delvare, Jiri Pirko,
	Jakub Kicinski

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: RFC: future of ethtool tunables (Re: [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable)
  2020-03-25 21:55   ` Andrew Lunn
  2020-03-25 22:04     ` Florian Fainelli
@ 2020-03-26  9:05     ` Oleksij Rempel
  1 sibling, 0 replies; 10+ messages in thread
From: Oleksij Rempel @ 2020-03-26  9:05 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Michal Kubecek, netdev, Marek Vasut, Florian Fainelli,
	Guenter Roeck, Heiner Kallweit, Jean Delvare, Jiri Pirko,
	Jakub Kicinski, mkl, David Miller, kernel,
	Russell King - ARM Linux admin, David Jander

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
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-03-26  9:05 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-25 10:17 [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable Marek Vasut
2020-03-25 10:17 ` [RFC][PATCH 2/2] net: phy: tja11xx: Add BroadRReach Master/Slave support into TJA11xx PHY driver Marek Vasut
2020-03-25 13:47   ` Andrew Lunn
2020-03-25 13:43 ` [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable Andrew Lunn
2020-03-25 13:51   ` Marek Vasut
2020-03-25 16:49 ` RFC: future of ethtool tunables (Re: [RFC][PATCH 1/2] ethtool: Add BroadRReach Master/Slave PHY tunable) Michal Kubecek
2020-03-25 21:55   ` Andrew Lunn
2020-03-25 22:04     ` Florian Fainelli
2020-03-25 23:42       ` Michal Kubecek
2020-03-26  9:05     ` Oleksij Rempel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).