[PATCH net-next 0/5] Adding PHY-Tunables and downshift support

[PATCH net-next 0/5] Adding PHY-Tunables and downshift support

[Allan W. Nielsen]

Hi All,

This series add support for PHY tunables, and uses this facility to configure
downshifting. The downshifting mechanism is implemented for MSCC phys.

This series tries to address the comments provided back in mid October when this
feature was posted along with fast-link-failure. Fast-link-failure has been
separated out, but we would like to pick continue on that if/when we agree on
how the phy-tunables and downshifting should be done.

The proposed generic interface is similar to ETHTOOL_GTUNABLE/ETHTOOL_STUNABLE,
it uses the same type (ethtool_tunable/tunable_type_id) but a new enum
(phy_tunable_id) is added to reflect the PHY tunable.

The implementation just call the newly added function pointers in
get_tunable/set_tunable phy_device structure.

To configure downshifting, the ethtool_tunable structure is used. 'id' must be
set to 'ETHTOOL_PHY_DOWNSHIFT', 'type_id' must be set to 'ETHTOOL_TUNABLE_U8'
and 'data' value configure the amount of downshift re-tries.

If configured to DOWNSHIFT_DEV_DISABLE, then downshift is disabled
If configured to DOWNSHIFT_DEV_DEFAULT_COUNT, then it is up to the device to
choose a device-specific re-try count.

Patches to implement this in ethtool will follow in a few minutes.

Please review.

Best regards
Allan and Raju

Raju Lakkaraju (5):
  ethtool: (uapi) Add ETHTOOL_PHY_GTUNABLE and ETHTOOL_PHY_STUNABLE
  ethtool: Implements ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE
  ethtool: (uapi) Add ETHTOOL_PHY_DOWNSHIFT to PHY tunables
  ethtool: Core impl for ETHTOOL_PHY_DOWNSHIFT tunable
  net: phy: Add downshift get/set support in Microsemi PHYs driver

 drivers/net/phy/mscc.c       | 102 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          |   7 +++
 include/uapi/linux/ethtool.h |  11 ++++-
 net/core/ethtool.c           |  82 ++++++++++++++++++++++++++++++++++
 4 files changed, 201 insertions(+), 1 deletion(-)

-- 
2.7.3

[PATCH net-next 1/5] ethtool: (uapi) Add ETHTOOL_PHY_GTUNABLE and ETHTOOL_PHY_STUNABLE

[Allan W. Nielsen]

From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>

Defines a generic API to get/set phy tunables. The API is using the
existing ethtool_tunable/tunable_type_id types which is already being used
for mac level tunables.

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
Signed-off-by: Allan W. Nielsen <allan.nielsen@microsemi.com>
---
 include/uapi/linux/ethtool.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 8e54723..fd0bd36 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -248,6 +248,10 @@ struct ethtool_tunable {
 	void	*data[0];
 };
 
+enum phy_tunable_id {
+	ETHTOOL_PHY_ID_UNSPEC,
+};
+
 /**
  * struct ethtool_regs - hardware register dump
  * @cmd: Command number = %ETHTOOL_GREGS
@@ -1313,7 +1317,8 @@ struct ethtool_per_queue_op {
 
 #define ETHTOOL_GLINKSETTINGS	0x0000004c /* Get ethtool_link_settings */
 #define ETHTOOL_SLINKSETTINGS	0x0000004d /* Set ethtool_link_settings */
-
+#define ETHTOOL_PHY_GTUNABLE	0x0000004e /* Get PHY tunable configuration */
+#define ETHTOOL_PHY_STUNABLE	0x0000004f /* Set PHY tunable configuration */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
-- 
2.7.3

[PATCH net-next 2/5] ethtool: Implements ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE

[Allan W. Nielsen]

From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>

Adding get_tunable/set_tunable function pointer to the phy_driver
structure, and uses these function pointers to implement the
ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE ioctls.

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
Signed-off-by: Allan W. Nielsen <allan.nielsen@microsemi.com>
---
 include/linux/phy.h |  7 +++++
 net/core/ethtool.c  | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index e7e1fd3..d30daf8 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -611,6 +611,13 @@ struct phy_driver {
 	void (*get_strings)(struct phy_device *dev, u8 *data);
 	void (*get_stats)(struct phy_device *dev,
 			  struct ethtool_stats *stats, u64 *data);
+
+	/* Get and Set PHY tunables */
+	int (*get_tunable)(struct phy_device *dev,
+			   struct ethtool_tunable *tuna, void *data);
+	int (*set_tunable)(struct phy_device *dev,
+			    struct ethtool_tunable *tuna,
+			    const void *data);
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 9774898..75f19ab 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -2422,6 +2422,76 @@ static int ethtool_set_per_queue(struct net_device *dev, void __user *useraddr)
 	};
 }
 
+static int ethtool_phy_tunable_valid(const struct ethtool_tunable *tuna)
+{
+	switch (tuna->id) {
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int get_phy_tunable(struct net_device *dev, void __user *useraddr)
+{
+	int ret;
+	struct ethtool_tunable tuna;
+	struct phy_device *phydev = dev->phydev;
+	void *data;
+
+	if (!(phydev && phydev->drv && phydev->drv->get_tunable))
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&tuna, useraddr, sizeof(tuna)))
+		return -EFAULT;
+	ret = ethtool_phy_tunable_valid(&tuna);
+	if (ret)
+		return ret;
+	data = kmalloc(tuna.len, GFP_USER);
+	if (!data)
+		return -ENOMEM;
+	ret = phydev->drv->get_tunable(phydev, &tuna, data);
+	if (ret)
+		goto out;
+	useraddr += sizeof(tuna);
+	ret = -EFAULT;
+	if (copy_to_user(useraddr, data, tuna.len))
+		goto out;
+	ret = 0;
+
+out:
+	kfree(data);
+	return ret;
+}
+
+static int set_phy_tunable(struct net_device *dev, void __user *useraddr)
+{
+	int ret;
+	struct ethtool_tunable tuna;
+	struct phy_device *phydev = dev->phydev;
+	void *data;
+
+	if (!(phydev && phydev->drv && phydev->drv->set_tunable))
+		return -EOPNOTSUPP;
+	if (copy_from_user(&tuna, useraddr, sizeof(tuna)))
+		return -EFAULT;
+	ret = ethtool_phy_tunable_valid(&tuna);
+	if (ret)
+		return ret;
+	data = kmalloc(tuna.len, GFP_USER);
+	if (!data)
+		return -ENOMEM;
+	useraddr += sizeof(tuna);
+	ret = -EFAULT;
+	if (copy_from_user(data, useraddr, tuna.len))
+		goto out;
+	ret = phydev->drv->set_tunable(phydev, &tuna, data);
+
+out:
+	kfree(data);
+	return ret;
+}
+
 /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
 
 int dev_ethtool(struct net *net, struct ifreq *ifr)
@@ -2479,6 +2549,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GET_TS_INFO:
 	case ETHTOOL_GEEE:
 	case ETHTOOL_GTUNABLE:
+	case ETHTOOL_PHY_GTUNABLE:
 		break;
 	default:
 		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
@@ -2684,6 +2755,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_SLINKSETTINGS:
 		rc = ethtool_set_link_ksettings(dev, useraddr);
 		break;
+	case ETHTOOL_PHY_GTUNABLE:
+		rc = get_phy_tunable(dev, useraddr);
+		break;
+	case ETHTOOL_PHY_STUNABLE:
+		rc = set_phy_tunable(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
-- 
2.7.3

[PATCH net-next 3/5] ethtool: (uapi) Add ETHTOOL_PHY_DOWNSHIFT to PHY tunables

[Allan W. Nielsen]

From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>

For operation in cabling environments that are incompatible with
1000BAST-T, PHY device may provide an automatic link speed downshift
operation. When enabled, the device automatically changes its 1000BAST-T
auto-negotiation to the next slower speed after a configured number of
failed attempts at 1000BAST-T.  This feature is useful in setting up in
networks using older cable installations that include only pairs A and B,
and not pairs C and D.

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
Signed-off-by: Allan W. Nielsen <allan.nielsen@microsemi.com>
---
 include/uapi/linux/ethtool.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index fd0bd36..040c5b5 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -248,8 +248,12 @@ struct ethtool_tunable {
 	void	*data[0];
 };
 
+#define DOWNSHIFT_DEV_DEFAULT_COUNT	0xff
+#define DOWNSHIFT_DEV_DISABLE		0
+
 enum phy_tunable_id {
 	ETHTOOL_PHY_ID_UNSPEC,
+	ETHTOOL_PHY_DOWNSHIFT,
 };
 
 /**
-- 
2.7.3

[PATCH net-next 4/5] ethtool: Core impl for ETHTOOL_PHY_DOWNSHIFT tunable

[Allan W. Nielsen]

From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>

Adding validation support for the ETHTOOL_PHY_DOWNSHIFT. Functional
implementation needs to be done in the individual PHY drivers.

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
Signed-off-by: Allan W. Nielsen <allan.nielsen@microsemi.com>
---
 net/core/ethtool.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 75f19ab..1a66faa 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -2425,6 +2425,11 @@ static int ethtool_set_per_queue(struct net_device *dev, void __user *useraddr)
 static int ethtool_phy_tunable_valid(const struct ethtool_tunable *tuna)
 {
 	switch (tuna->id) {
+	case ETHTOOL_PHY_DOWNSHIFT:
+		if (tuna->len != sizeof(u8) ||
+		    tuna->type_id != ETHTOOL_TUNABLE_U8)
+			return -EINVAL;
+		break;
 	default:
 		return -EINVAL;
 	}
-- 
2.7.3

[PATCH net-next 5/5] net: phy: Add downshift get/set support in Microsemi PHYs driver

[Allan W. Nielsen]

From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>

Implements the phy tunable function pointers and implement downshift
functionality for MSCC PHYs.

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
Signed-off-by: Allan W. Nielsen <allan.nielsen@microsemi.com>
---
 drivers/net/phy/mscc.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index d0026ab..7edd32d 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -46,8 +46,15 @@ enum rgmii_rx_clock_delay {
 
 #define MSCC_EXT_PAGE_ACCESS		  31
 #define MSCC_PHY_PAGE_STANDARD		  0x0000 /* Standard registers */
+#define MSCC_PHY_PAGE_EXTENDED		  0x0001 /* Extended registers */
 #define MSCC_PHY_PAGE_EXTENDED_2	  0x0002 /* Extended reg - page 2 */
 
+/* Extended Page 1 Registers */
+#define MSCC_PHY_ACTIPHY_CNTL		  20
+#define DOWNSHIFT_CNTL_MASK		  0x001C
+#define DOWNSHIFT_EN			  0x0010
+#define DOWNSHIFT_CNTL_POS		  2
+
 /* Extended Page 2 Registers */
 #define MSCC_PHY_RGMII_CNTL		  20
 #define RGMII_RX_CLK_DELAY_MASK		  0x0070
@@ -75,6 +82,8 @@ enum rgmii_rx_clock_delay {
 #define MSCC_VDDMAC_2500		  2500
 #define MSCC_VDDMAC_3300		  3300
 
+#define DOWNSHIFT_COUNT_MAX		  5
+
 struct vsc8531_private {
 	int rate_magic;
 };
@@ -101,6 +110,66 @@ static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
 	return rc;
 }
 
+static int vsc85xx_downshift_get(struct phy_device *phydev, u8 *count)
+{
+	int rc;
+	u16 reg_val;
+
+	mutex_lock(&phydev->lock);
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
+	if (rc != 0)
+		goto out_unlock;
+
+	reg_val = phy_read(phydev, MSCC_PHY_ACTIPHY_CNTL);
+	reg_val &= DOWNSHIFT_CNTL_MASK;
+	if (!(reg_val & DOWNSHIFT_EN))
+		*count = 0;
+	else
+		*count = ((reg_val & ~DOWNSHIFT_EN) >> DOWNSHIFT_CNTL_POS) + 2;
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+
+out_unlock:
+	mutex_unlock(&phydev->lock);
+
+	return rc;
+}
+
+static int vsc85xx_downshift_set(struct phy_device *phydev, u8 count)
+{
+	int rc;
+	u16 reg_val;
+
+	if (count == DOWNSHIFT_DEV_DEFAULT_COUNT) {
+		/* Default downshift count 3 (i.e. Bit3:2 = 0b01) */
+		count = ((1 << DOWNSHIFT_CNTL_POS) | DOWNSHIFT_EN);
+	} else if (count > DOWNSHIFT_COUNT_MAX || count == 1) {
+		phydev_err(phydev, "Invalid downshift count\n");
+		return -EINVAL;
+	} else if (count) {
+		/* Downshift count is either 2,3,4 or 5 */
+		count = (((count - 2) << DOWNSHIFT_CNTL_POS) | DOWNSHIFT_EN);
+	}
+
+	mutex_lock(&phydev->lock);
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
+	if (rc != 0)
+		goto out_unlock;
+
+	reg_val = phy_read(phydev, MSCC_PHY_ACTIPHY_CNTL);
+	reg_val &= ~(DOWNSHIFT_CNTL_MASK);
+	reg_val |= count;
+	rc = phy_write(phydev, MSCC_PHY_ACTIPHY_CNTL, reg_val);
+	if (rc != 0)
+		goto out_unlock;
+
+	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+
+out_unlock:
+	mutex_unlock(&phydev->lock);
+
+	return rc;
+}
+
 static int vsc85xx_wol_set(struct phy_device *phydev,
 			   struct ethtool_wolinfo *wol)
 {
@@ -329,6 +398,31 @@ static int vsc85xx_default_config(struct phy_device *phydev)
 	return rc;
 }
 
+static int vsc85xx_get_tunable(struct phy_device *phydev,
+			       struct ethtool_tunable *tuna, void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_DOWNSHIFT:
+		return vsc85xx_downshift_get(phydev, (u8 *)data);
+	default:
+		phydev_err(phydev, "Unsupported PHY tunable id\n");
+		return -EINVAL;
+	}
+}
+
+static int vsc85xx_set_tunable(struct phy_device *phydev,
+			       struct ethtool_tunable *tuna,
+			       const void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_DOWNSHIFT:
+		return vsc85xx_downshift_set(phydev, *(u8 *)data);
+	default:
+		phydev_err(phydev, "Unsupported PHY tunable id\n");
+		return -EINVAL;
+	}
+}
+
 static int vsc85xx_config_init(struct phy_device *phydev)
 {
 	int rc;
@@ -418,6 +512,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.probe		= &vsc85xx_probe,
 	.set_wol	= &vsc85xx_wol_set,
 	.get_wol	= &vsc85xx_wol_get,
+	.get_tunable	= &vsc85xx_get_tunable,
+	.set_tunable	= &vsc85xx_set_tunable,
 },
 {
 	.phy_id		= PHY_ID_VSC8531,
@@ -437,6 +533,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.probe		= &vsc85xx_probe,
 	.set_wol	= &vsc85xx_wol_set,
 	.get_wol	= &vsc85xx_wol_get,
+	.get_tunable	= &vsc85xx_get_tunable,
+	.set_tunable	= &vsc85xx_set_tunable,
 },
 {
 	.phy_id		= PHY_ID_VSC8540,
@@ -456,6 +554,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.probe		= &vsc85xx_probe,
 	.set_wol	= &vsc85xx_wol_set,
 	.get_wol	= &vsc85xx_wol_get,
+	.get_tunable	= &vsc85xx_get_tunable,
+	.set_tunable	= &vsc85xx_set_tunable,
 },
 {
 	.phy_id		= PHY_ID_VSC8541,
@@ -475,6 +575,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.probe		= &vsc85xx_probe,
 	.set_wol	= &vsc85xx_wol_set,
 	.get_wol	= &vsc85xx_wol_get,
+	.get_tunable	= &vsc85xx_get_tunable,
+	.set_tunable	= &vsc85xx_set_tunable,
 }
 
 };
-- 
2.7.3

Re: [PATCH net-next 1/5] ethtool: (uapi) Add ETHTOOL_PHY_GTUNABLE and ETHTOOL_PHY_STUNABLE

[Andrew Lunn]

On Fri, Nov 04, 2016 at 11:35:38AM +0100, Allan W. Nielsen wrote:
> From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> 
> Defines a generic API to get/set phy tunables. The API is using the
> existing ethtool_tunable/tunable_type_id types which is already being used
> for mac level tunables.
> 
> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> Signed-off-by: Allan W. Nielsen <allan.nielsen@microsemi.com>
> ---
>  include/uapi/linux/ethtool.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 8e54723..fd0bd36 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -248,6 +248,10 @@ struct ethtool_tunable {
>  	void	*data[0];
>  };
>  
> +enum phy_tunable_id {
> +	ETHTOOL_PHY_ID_UNSPEC,
> +};

Hi Allen

Do you have any idea what this is for? A grep for
ETHTOOL_TUNABLE_UNSPEC does not turn up anything.

       Andrew

Re: [PATCH net-next 2/5] ethtool: Implements ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE

[Andrew Lunn]

On Fri, Nov 04, 2016 at 11:35:39AM +0100, Allan W. Nielsen wrote:
> From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> 
> Adding get_tunable/set_tunable function pointer to the phy_driver
> structure, and uses these function pointers to implement the
> ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE ioctls.

Hi Allan

For consistency, it would also be nice to add code in
__ethtool_get_strings().

	Andrew

Re: [PATCH net-next 1/5] ethtool: (uapi) Add ETHTOOL_PHY_GTUNABLE and ETHTOOL_PHY_STUNABLE

[Allan W. Nielsen]

Hi,

On 04/11/16 13:03, Andrew Lunn wrote:
> On Fri, Nov 04, 2016 at 11:35:38AM +0100, Allan W. Nielsen wrote:
> > From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> >
> > Defines a generic API to get/set phy tunables. The API is using the
> > existing ethtool_tunable/tunable_type_id types which is already being used
> > for mac level tunables.
> >
> > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> > Signed-off-by: Allan W. Nielsen <allan.nielsen@microsemi.com>
> > ---
> >  include/uapi/linux/ethtool.h | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> > index 8e54723..fd0bd36 100644
> > --- a/include/uapi/linux/ethtool.h
> > +++ b/include/uapi/linux/ethtool.h
> > @@ -248,6 +248,10 @@ struct ethtool_tunable {
> >       void    *data[0];
> >  };
> >
> > +enum phy_tunable_id {
> > +     ETHTOOL_PHY_ID_UNSPEC,
> > +};
> 
> Do you have any idea what this is for? A grep for
> ETHTOOL_TUNABLE_UNSPEC does not turn up anything.
It is not used...

It was "just" to mimic how "tunable_type_id/ETHTOOL_TUNABLE_UNSPEC" (and other)
is done.

The thinking was that we did not want an "ID" of zero do to anything - because
that could mean the programmer had forgot to set the field...

I have on strong feelings about this, please let us know if you would like this
done in an other way.

/Allan

Re: [PATCH net-next 5/5] net: phy: Add downshift get/set support in Microsemi PHYs driver

[Andrew Lunn]

Hi Allan

> +static int vsc85xx_downshift_get(struct phy_device *phydev, u8 *count)
> +{
> +	int rc;
> +	u16 reg_val;
> +
> +	mutex_lock(&phydev->lock);
> +	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
> +	if (rc != 0)
> +		goto out_unlock;
> +
> +	reg_val = phy_read(phydev, MSCC_PHY_ACTIPHY_CNTL);
> +	reg_val &= DOWNSHIFT_CNTL_MASK;
> +	if (!(reg_val & DOWNSHIFT_EN))
> +		*count = 0;

DOWNSHIFT_DEV_DISABLE

> +	else
> +		*count = ((reg_val & ~DOWNSHIFT_EN) >> DOWNSHIFT_CNTL_POS) + 2;
>
> +	rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
> +
> +out_unlock:
> +	mutex_unlock(&phydev->lock);
> +
> +	return rc;
> +}
> +
> +static int vsc85xx_downshift_set(struct phy_device *phydev, u8 count)
> +{
> +	int rc;
> +	u16 reg_val;
> +
> +	if (count == DOWNSHIFT_DEV_DEFAULT_COUNT) {
> +		/* Default downshift count 3 (i.e. Bit3:2 = 0b01) */
> +		count = ((1 << DOWNSHIFT_CNTL_POS) | DOWNSHIFT_EN);
> +	} else if (count > DOWNSHIFT_COUNT_MAX || count == 1) {
> +		phydev_err(phydev, "Invalid downshift count\n");

Maybe include a hint what is valid?

> +		return -EINVAL;

ERANGE? I don't know error codes too well, so this needs verifying.

> +	} else if (count) {
> +		/* Downshift count is either 2,3,4 or 5 */
> +		count = (((count - 2) << DOWNSHIFT_CNTL_POS) | DOWNSHIFT_EN);

Ah, now i see why + 2. But this means it never does what you ask it to
do. It would be better to round up < 2 to 2, and leave all the others
as is.

> +static int vsc85xx_get_tunable(struct phy_device *phydev,
> +			       struct ethtool_tunable *tuna, void *data)
> +{
> +	switch (tuna->id) {
> +	case ETHTOOL_PHY_DOWNSHIFT:
> +		return vsc85xx_downshift_get(phydev, (u8 *)data);
> +	default:
> +		phydev_err(phydev, "Unsupported PHY tunable id\n");
> +		return -EINVAL;

This is not really a error you should complain about. There could be
many tunables, and some your hardware cannot support. So return
-ENOSUPP and no phydev_err().

> +	}
> +}
> +
> +static int vsc85xx_set_tunable(struct phy_device *phydev,
> +			       struct ethtool_tunable *tuna,
> +			       const void *data)
> +{
> +	switch (tuna->id) {
> +	case ETHTOOL_PHY_DOWNSHIFT:
> +		return vsc85xx_downshift_set(phydev, *(u8 *)data);
> +	default:
> +		phydev_err(phydev, "Unsupported PHY tunable id\n");
> +		return -EINVAL;

Same as above.

     Andrew

Re: [PATCH net-next 1/5] ethtool: (uapi) Add ETHTOOL_PHY_GTUNABLE and ETHTOOL_PHY_STUNABLE

[Andrew Lunn]

> It was "just" to mimic how "tunable_type_id/ETHTOOL_TUNABLE_UNSPEC" (and other)
> is done.

Yes, i know. I'm wondering about cult cargo programming...

> The thinking was that we did not want an "ID" of zero do to anything - because
> that could mean the programmer had forgot to set the field...

Seems reasonable. Leave it as is.

      Andrew

Re: [PATCH net-next 2/5] ethtool: Implements ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE

[Allan W. Nielsen]

On 04/11/16 13:13, Andrew Lunn wrote:
> On Fri, Nov 04, 2016 at 11:35:39AM +0100, Allan W. Nielsen wrote:
> > From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> >
> > Adding get_tunable/set_tunable function pointer to the phy_driver
> > structure, and uses these function pointers to implement the
> > ETHTOOL_PHY_GTUNABLE/ETHTOOL_PHY_STUNABLE ioctls.
> For consistency, it would also be nice to add code in
> __ethtool_get_strings().
Okay, we add add that, and I assume you also want ethtool to use it when the
"-k|--show-features" options is invoked?

Are there other cases where this should be used?

/Allan

Re: [PATCH net-next 5/5] net: phy: Add downshift get/set support in Microsemi PHYs driver

[Allan W. Nielsen]

On 04/11/16 13:27, Andrew Lunn wrote:
> > +     } else if (count) {
> > +             /* Downshift count is either 2,3,4 or 5 */
> > +             count = (((count - 2) << DOWNSHIFT_CNTL_POS) | DOWNSHIFT_EN);
> 
> Ah, now i see why + 2. But this means it never does what you ask it to
> do. It would be better to round up < 2 to 2, and leave all the others
> as is.
Not sure I understand what you mean...

If the user configure "count == 1", then you want that to be rounded up to
"count == 2", because the HW does not support a count of 1???

If the user configure count to 6, 7, 8 etc. would you also like to round it down
to 5?

I'm okay with that but not sure it is want you mean... (and it will eliminate
your comment on ERANGE - because all values will be accepted, they are just
rounded to "closet" supported value).


(The other comments is understood, and we will get them fixed).


/Allan

Re: [PATCH net-next 5/5] net: phy: Add downshift get/set support in Microsemi PHYs driver

[Andrew Lunn]

On Fri, Nov 04, 2016 at 02:42:34PM +0100, Allan W. Nielsen wrote:
> On 04/11/16 13:27, Andrew Lunn wrote:
> > > +     } else if (count) {
> > > +             /* Downshift count is either 2,3,4 or 5 */
> > > +             count = (((count - 2) << DOWNSHIFT_CNTL_POS) | DOWNSHIFT_EN);
> > 
> > Ah, now i see why + 2. But this means it never does what you ask it to
> > do. It would be better to round up < 2 to 2, and leave all the others
> > as is.
> Not sure I understand what you mean...
> 
> If the user configure "count == 1", then you want that to be rounded up to
> "count == 2", because the HW does not support a count of 1???

Yes. The other option would be to return ERANGE when 1 is asked
for. The real question is, which is better for the user? Returning
ERANGE and letting the user make a guessing game to figure out what is
valid, or magically turn 1 into 2. I will let you decide which is
best.

> If the user configure count to 6, 7, 8 etc. would you also like to round it down
> to 5?

No. ERANGE. The user has to expect some upper limit, and ERANGE is a
good indication they have reached it. But having a lowered limit of 2
is less obvious.

   Andrew