[PATCH] smsc95xx: support ethtool get_regs

[PATCH] smsc95xx: support ethtool get_regs

[Émeric Vigier]

From: Emeric Vigier <emeric.vigier@savoirfairelinux.com>

Inspired by implementation in smsc911x.c and smsc9420.c
Tested on ARM/pandaboard rev A3

Signed-off-by: Emeric Vigier <emeric.vigier@savoirfairelinux.com>
---
 drivers/net/usb/smsc95xx.c |   37 +++++++++++++++++++++++++++++++++++++
 1 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index b1112e7..bce14f6 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -578,6 +578,41 @@ static int smsc95xx_ethtool_set_eeprom(struct net_device *netdev,
 	return smsc95xx_write_eeprom(dev, ee->offset, ee->len, data);
 }
 
+
+static int smsc95xx_ethtool_getregslen(struct net_device *dev)
+{
+	/* all smsc95xx registers plus all phy registers */
+	return COE_CR - ID_REV + 1 + 32 * sizeof(u32);
+}
+
+static void
+smsc95xx_ethtool_getregs(struct net_device *netdev, struct ethtool_regs *regs,
+			 void *buf)
+{
+	struct usbnet *dev = netdev_priv(netdev);
+	unsigned int i, j = 0, retval;
+	u32 *data = buf;
+
+	netif_dbg(dev, hw, dev->net, "ethtool_getregs\n");
+
+	retval = smsc95xx_read_reg(dev, ID_REV, &regs->version);
+	if (retval < 0) {
+		netdev_warn(dev->net, "REGS: cannot read ID_REV\n");
+		return;
+	}
+
+	for (i = 0; i <= COE_CR; i += (sizeof(u32))) {
+		retval = smsc95xx_read_reg(dev, i, &data[j++]);
+		if (retval < 0) {
+			netdev_warn(dev->net, "REGS: cannot read reg[%x]\n", i);
+			return;
+		}
+	}
+
+	for (i = 0; i <= PHY_SPECIAL; i++)
+		data[j++] = smsc95xx_mdio_read(netdev, dev->mii.phy_id, i);
+}
+
 static const struct ethtool_ops smsc95xx_ethtool_ops = {
 	.get_link	= usbnet_get_link,
 	.nway_reset	= usbnet_nway_reset,
@@ -589,6 +624,8 @@ static const struct ethtool_ops smsc95xx_ethtool_ops = {
 	.get_eeprom_len	= smsc95xx_ethtool_get_eeprom_len,
 	.get_eeprom	= smsc95xx_ethtool_get_eeprom,
 	.set_eeprom	= smsc95xx_ethtool_set_eeprom,
+	.get_regs_len	= smsc95xx_ethtool_getregslen,
+	.get_regs	= smsc95xx_ethtool_getregs,
 };
 
 static int smsc95xx_ioctl(struct net_device *netdev, struct ifreq *rq, int cmd)
-- 
1.7.5.4

Emeric

Re: [PATCH] smsc95xx: support ethtool get_regs

[Francois Romieu]

Émeric Vigier <emeric.vigier@savoirfairelinux.com> :
[...]
> +static int smsc95xx_ethtool_getregslen(struct net_device *dev)
> +{
> +	/* all smsc95xx registers plus all phy registers */
> +	return COE_CR - ID_REV + 1 + 32 * sizeof(u32);

I do not see where ID_REV is accounted for in the loops below.

s/32 */PHY_SPECIAL */ or s/PHY_SPECIAL/32/ below.

I thought PHY registers were 16 bits wide. Moreover they are already
available through smsc95xx_ioctl().

> +}
> +
> +static void
> +smsc95xx_ethtool_getregs(struct net_device *netdev, struct ethtool_regs *regs,
> +			 void *buf)
> +{
> +	struct usbnet *dev = netdev_priv(netdev);
> +	unsigned int i, j = 0, retval;

	unsigned int i, j, retval;

> +	u32 *data = buf;
> +
> +	netif_dbg(dev, hw, dev->net, "ethtool_getregs\n");

The tracing framework does provide almost the same information.

> +
> +	retval = smsc95xx_read_reg(dev, ID_REV, &regs->version);
> +	if (retval < 0) {
> +		netdev_warn(dev->net, "REGS: cannot read ID_REV\n");

s/dev->net/netdev/ ?


> +		return;
> +	}
> +
> +	for (i = 0; i <= COE_CR; i += (sizeof(u32))) {
> +		retval = smsc95xx_read_reg(dev, i, &data[j++]);

	for (i = 0, j = 0; i <= COE_CR; i += sizeof(u32), j++) {
		retval = smsc95xx_read_reg(dev, i, data + j);

-- 
Ueimor

Re: [PATCH] smsc95xx: support ethtool get_regs

[Émeric Vigier]

----- Mail original -----
> De: "Francois Romieu" <romieu@fr.zoreil.com>
> À: "Émeric Vigier" <emeric.vigier@savoirfairelinux.com>
> Cc: "Steve Glendinning" <steve@shawell.net>, "steve glendinning" <steve.glendinning@smsc.com>,
> netdev@vger.kernel.org, "Nancy Lin" <nancy.lin@smsc.com>
> Envoyé: Vendredi 6 Juillet 2012 16:01:46
> Objet: Re: [PATCH] smsc95xx: support ethtool get_regs
> 
> Émeric Vigier <emeric.vigier@savoirfairelinux.com> :
> [...]
> > +static int smsc95xx_ethtool_getregslen(struct net_device *dev)
> > +{
> > +	/* all smsc95xx registers plus all phy registers */
> > +	return COE_CR - ID_REV + 1 + 32 * sizeof(u32);
> 
> I do not see where ID_REV is accounted for in the loops below.
> 
> s/32 */PHY_SPECIAL */ or s/PHY_SPECIAL/32/ below.

I will go for the second proposal. I love your sed syntax btw :-)

> 
> I thought PHY registers were 16 bits wide. Moreover they are already
> available through smsc95xx_ioctl().

Yes, there are 16 bits wide according to smsc95xx.h.
But other smsc drivers define 32bit wide PHY regs. I made myself believe that smsc would use the same PHY for each ethernet chip.
So would something like s/32 * sizeof(u32)/PHY_SPECIAL * sizeof(u16)/ solve the issue here?

Concerning the ioctl, I found ethtool much easier to use. And I believe smsc9514 is a very popular chipset, so this could help others debugging it.

> 
> > +}
> > +
> > +static void
> > +smsc95xx_ethtool_getregs(struct net_device *netdev, struct
> > ethtool_regs *regs,
> > +			 void *buf)
> > +{
> > +	struct usbnet *dev = netdev_priv(netdev);
> > +	unsigned int i, j = 0, retval;
> 
> 	unsigned int i, j, retval;
> 
> > +	u32 *data = buf;
> > +
> > +	netif_dbg(dev, hw, dev->net, "ethtool_getregs\n");
> 
> The tracing framework does provide almost the same information.

Do you mean LTT? I am not familiar with it, I should have a look.
I remove netif_dbg then.

> 
> > +
> > +	retval = smsc95xx_read_reg(dev, ID_REV, &regs->version);
> > +	if (retval < 0) {
> > +		netdev_warn(dev->net, "REGS: cannot read ID_REV\n");
> 
> s/dev->net/netdev/ ?

You are right, I also changed smsc95xx_ethtool_getregslen() definition to match this syntax.

> 
> 
> > +		return;
> > +	}
> > +
> > +	for (i = 0; i <= COE_CR; i += (sizeof(u32))) {
> > +		retval = smsc95xx_read_reg(dev, i, &data[j++]);
> 
> 	for (i = 0, j = 0; i <= COE_CR; i += sizeof(u32), j++) {
> 		retval = smsc95xx_read_reg(dev, i, data + j);

I should change that in previous "for" loop as well I suppose?

> 
> --
> Ueimor
> 

Emeric

Re: [PATCH] smsc95xx: support ethtool get_regs

[Francois Romieu]

Émeric Vigier <emeric.vigier@savoirfairelinux.com> :
[...]
> Yes, there are 16 bits wide according to smsc95xx.h.
> But other smsc drivers define 32bit wide PHY regs. I made myself believe
> that smsc would use the same PHY for each ethernet chip.

SMSC people would surely answer before I find the relevant datasheet.

Anyway the PHY registers are accessed indirectly through the MII_{ADDR, DATA}
registers and MII_DATA r/w mask is limited to the lower 16 bits.

> So would something like s/32 * sizeof(u32)/PHY_SPECIAL * sizeof(u16)/ solve the issue here?

You would have to pack data[] as well. Or use u16 *.

> Concerning the ioctl, I found ethtool much easier to use. And I believe
> smsc9514 is a very popular chipset, so this could help others debugging it.

# mii-tool -vv e1000
Using SIOCGMIIPHY=0x8947
e1000: no autonegotiation, 10baseT-HD, link ok
  registers for MII PHY 0: 
    1140 796d 0141 0c30 0de1 0021 0004 0000
    0000 0200 0000 0000 0000 0000 0000 3000
    0000 0000 0000 0000 0174 0000 0000 0000
    4100 0000 000d 000f 0000 0000 0000 0000
  product info: vendor 00:50:43, model 3 rev 0
  basic mode:   autonegotiation enabled
  basic status: autonegotiation complete, link ok
  capabilities: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
  advertising:  1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD flow-control
  link partner: 10baseT-HD

It is not that bad for the first 32 PHY registers.

[...]
> Do you mean LTT? I am not familiar with it, I should have a look.

Documentation/trace/ftrace.txt

[...]
> I should change that in previous "for" loop as well I suppose?

You may.

-- 
Ueimor

Re: [PATCH] smsc95xx: support ethtool get_regs

[Émeric Vigier]

----- Mail original -----
> Émeric Vigier <emeric.vigier@savoirfairelinux.com> :
> [...]
> > Yes, there are 16 bits wide according to smsc95xx.h.
> > But other smsc drivers define 32bit wide PHY regs. I made myself
> > believe
> > that smsc would use the same PHY for each ethernet chip.
> 
> SMSC people would surely answer before I find the relevant datasheet.
> 
> Anyway the PHY registers are accessed indirectly through the
> MII_{ADDR, DATA}
> registers and MII_DATA r/w mask is limited to the lower 16 bits.
> 
> > So would something like s/32 * sizeof(u32)/PHY_SPECIAL *
> > sizeof(u16)/ solve the issue here?
> 
> You would have to pack data[] as well. Or use u16 *.

I will check this out next week.

> 
> > Concerning the ioctl, I found ethtool much easier to use. And I
> > believe
> > smsc9514 is a very popular chipset, so this could help others
> > debugging it.
> 
> # mii-tool -vv e1000
> Using SIOCGMIIPHY=0x8947
> e1000: no autonegotiation, 10baseT-HD, link ok
>   registers for MII PHY 0:
>     1140 796d 0141 0c30 0de1 0021 0004 0000
>     0000 0200 0000 0000 0000 0000 0000 3000
>     0000 0000 0000 0000 0174 0000 0000 0000
>     4100 0000 000d 000f 0000 0000 0000 0000
>   product info: vendor 00:50:43, model 3 rev 0
>   basic mode:   autonegotiation enabled
>   basic status: autonegotiation complete, link ok
>   capabilities: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD
>   10baseT-HD
>   advertising:  1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD
>   10baseT-HD flow-control
>   link partner: 10baseT-HD
> 
> It is not that bad for the first 32 PHY registers.

I didn't know about mii-tool. Thanks.

> 
> [...]
> > Do you mean LTT? I am not familiar with it, I should have a look.
> 
> Documentation/trace/ftrace.txt

ok

> 
> [...]
> > I should change that in previous "for" loop as well I suppose?
> 
> You may.

Thanks for your patience.

> 
> --
> Ueimor
> 

-- 
Emeric

Re: [PATCH] smsc95xx: support ethtool get_regs

[Ben Hutchings]

On Fri, 2012-07-06 at 14:15 -0400, Émeric Vigier wrote:
> From: Emeric Vigier <emeric.vigier@savoirfairelinux.com>
> 
> Inspired by implementation in smsc911x.c and smsc9420.c
> Tested on ARM/pandaboard rev A3
> 
> Signed-off-by: Emeric Vigier <emeric.vigier@savoirfairelinux.com>
> ---
>  drivers/net/usb/smsc95xx.c |   37 +++++++++++++++++++++++++++++++++++++
>  1 files changed, 37 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index b1112e7..bce14f6 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -578,6 +578,41 @@ static int smsc95xx_ethtool_set_eeprom(struct net_device *netdev,
>  	return smsc95xx_write_eeprom(dev, ee->offset, ee->len, data);
>  }
>  
> +
> +static int smsc95xx_ethtool_getregslen(struct net_device *dev)
> +{
> +	/* all smsc95xx registers plus all phy registers */
> +	return COE_CR - ID_REV + 1 + 32 * sizeof(u32);
> +}
> +
> +static void
> +smsc95xx_ethtool_getregs(struct net_device *netdev, struct ethtool_regs *regs,
> +			 void *buf)
> +{
> +	struct usbnet *dev = netdev_priv(netdev);
> +	unsigned int i, j = 0, retval;
> +	u32 *data = buf;
> +
> +	netif_dbg(dev, hw, dev->net, "ethtool_getregs\n");
> +
> +	retval = smsc95xx_read_reg(dev, ID_REV, &regs->version);
> +	if (retval < 0) {
> +		netdev_warn(dev->net, "REGS: cannot read ID_REV\n");
> +		return;
> +	}
> +
> +	for (i = 0; i <= COE_CR; i += (sizeof(u32))) {
> +		retval = smsc95xx_read_reg(dev, i, &data[j++]);
> +		if (retval < 0) {
> +			netdev_warn(dev->net, "REGS: cannot read reg[%x]\n", i);
> +			return;
> +		}
> +	}

Why does this start with i = 0 whereas the calculation of the length
uses ID_REV as the starting point?  Maybe ID_REV == 0, but you should be
consistent in whether you use the name or literal number.

> +	for (i = 0; i <= PHY_SPECIAL; i++)
> +		data[j++] = smsc95xx_mdio_read(netdev, dev->mii.phy_id, i);
> +}

Again, why use PHY_SPECIAL (+ 1) here as opposed to 32 in the
calculation of the length?

Ben.

>  static const struct ethtool_ops smsc95xx_ethtool_ops = {
>  	.get_link	= usbnet_get_link,
>  	.nway_reset	= usbnet_nway_reset,
> @@ -589,6 +624,8 @@ static const struct ethtool_ops smsc95xx_ethtool_ops = {
>  	.get_eeprom_len	= smsc95xx_ethtool_get_eeprom_len,
>  	.get_eeprom	= smsc95xx_ethtool_get_eeprom,
>  	.set_eeprom	= smsc95xx_ethtool_set_eeprom,
> +	.get_regs_len	= smsc95xx_ethtool_getregslen,
> +	.get_regs	= smsc95xx_ethtool_getregs,
>  };
>  
>  static int smsc95xx_ioctl(struct net_device *netdev, struct ifreq *rq, int cmd)

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] smsc95xx: support ethtool get_regs

[Émeric Vigier]

----- Mail original -----
> On Fri, 2012-07-06 at 14:15 -0400, Émeric Vigier wrote:
> > From: Emeric Vigier <emeric.vigier@savoirfairelinux.com>
> > 
> > Inspired by implementation in smsc911x.c and smsc9420.c
> > Tested on ARM/pandaboard rev A3
> > 
> > Signed-off-by: Emeric Vigier <emeric.vigier@savoirfairelinux.com>
> > ---
> >  drivers/net/usb/smsc95xx.c |   37
> >  +++++++++++++++++++++++++++++++++++++
> >  1 files changed, 37 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/net/usb/smsc95xx.c
> > b/drivers/net/usb/smsc95xx.c
> > index b1112e7..bce14f6 100644
> > --- a/drivers/net/usb/smsc95xx.c
> > +++ b/drivers/net/usb/smsc95xx.c
> > @@ -578,6 +578,41 @@ static int smsc95xx_ethtool_set_eeprom(struct
> > net_device *netdev,
> >  	return smsc95xx_write_eeprom(dev, ee->offset, ee->len, data);
> >  }
> >  
> > +
> > +static int smsc95xx_ethtool_getregslen(struct net_device *dev)
> > +{
> > +	/* all smsc95xx registers plus all phy registers */
> > +	return COE_CR - ID_REV + 1 + 32 * sizeof(u32);
> > +}
> > +
> > +static void
> > +smsc95xx_ethtool_getregs(struct net_device *netdev, struct
> > ethtool_regs *regs,
> > +			 void *buf)
> > +{
> > +	struct usbnet *dev = netdev_priv(netdev);
> > +	unsigned int i, j = 0, retval;
> > +	u32 *data = buf;
> > +
> > +	netif_dbg(dev, hw, dev->net, "ethtool_getregs\n");
> > +
> > +	retval = smsc95xx_read_reg(dev, ID_REV, &regs->version);
> > +	if (retval < 0) {
> > +		netdev_warn(dev->net, "REGS: cannot read ID_REV\n");
> > +		return;
> > +	}
> > +
> > +	for (i = 0; i <= COE_CR; i += (sizeof(u32))) {
> > +		retval = smsc95xx_read_reg(dev, i, &data[j++]);
> > +		if (retval < 0) {
> > +			netdev_warn(dev->net, "REGS: cannot read reg[%x]\n", i);
> > +			return;
> > +		}
> > +	}
> 
> Why does this start with i = 0 whereas the calculation of the length
> uses ID_REV as the starting point?  Maybe ID_REV == 0, but you should
> be
> consistent in whether you use the name or literal number.

You are right. I will broadcast ID_REV usage.

> 
> > +	for (i = 0; i <= PHY_SPECIAL; i++)
> > +		data[j++] = smsc95xx_mdio_read(netdev, dev->mii.phy_id, i);
> > +}
> 
> Again, why use PHY_SPECIAL (+ 1) here as opposed to 32 in the
> calculation of the length?

32 was ok, but I hesitated between defining a SMSC95XX_PHY_END or using the last defined register.
Are 32 register-PHY generic to most devices? I mean could 32 be use widely?

> 
> Ben.
> 
> >  static const struct ethtool_ops smsc95xx_ethtool_ops = {
> >  	.get_link	= usbnet_get_link,
> >  	.nway_reset	= usbnet_nway_reset,
> > @@ -589,6 +624,8 @@ static const struct ethtool_ops
> > smsc95xx_ethtool_ops = {
> >  	.get_eeprom_len	= smsc95xx_ethtool_get_eeprom_len,
> >  	.get_eeprom	= smsc95xx_ethtool_get_eeprom,
> >  	.set_eeprom	= smsc95xx_ethtool_set_eeprom,
> > +	.get_regs_len	= smsc95xx_ethtool_getregslen,
> > +	.get_regs	= smsc95xx_ethtool_getregs,
> >  };
> >  
> >  static int smsc95xx_ioctl(struct net_device *netdev, struct ifreq
> >  *rq, int cmd)
> 
> --
> Ben Hutchings, Staff Engineer, Solarflare
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
> 
> 

-- 
Emeric

Re: [PATCH] smsc95xx: support ethtool get_regs

[Ben Hutchings]

On Sat, 2012-07-07 at 09:58 -0400, Émeric Vigier wrote:
[...]
> > > +	for (i = 0; i <= PHY_SPECIAL; i++)
> > > +		data[j++] = smsc95xx_mdio_read(netdev, dev->mii.phy_id, i);
> > > +}
> > 
> > Again, why use PHY_SPECIAL (+ 1) here as opposed to 32 in the
> > calculation of the length?
> 
> 32 was ok, but I hesitated between defining a SMSC95XX_PHY_END or using the last defined register.
> Are 32 register-PHY generic to most devices? I mean could 32 be use widely?

Yes, the address space for the original MDIO protocol ('clause 22')
allows for 32 registers.  Perhaps that number should be named in
<linux/mii.h>.

As another reviewer commented, though, MDIO PHY registers should be
accessible with SIOCGMIIREG and mii-tool so it's not really necessary to
duplicate them here.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] smsc95xx: support ethtool get_regs

[Émeric Vigier]

----- Mail original -----
> On Sat, 2012-07-07 at 09:58 -0400, Émeric Vigier wrote:
> [...]
> > > > +	for (i = 0; i <= PHY_SPECIAL; i++)
> > > > +		data[j++] = smsc95xx_mdio_read(netdev, dev->mii.phy_id, i);
> > > > +}
> > > 
> > > Again, why use PHY_SPECIAL (+ 1) here as opposed to 32 in the
> > > calculation of the length?
> > 
> > 32 was ok, but I hesitated between defining a SMSC95XX_PHY_END or
> > using the last defined register.
> > Are 32 register-PHY generic to most devices? I mean could 32 be use
> > widely?
> 
> Yes, the address space for the original MDIO protocol ('clause 22')
> allows for 32 registers.  Perhaps that number should be named in
> <linux/mii.h>.

I have not found any definition of this in mii.h.

> 
> As another reviewer commented, though, MDIO PHY registers should be
> accessible with SIOCGMIIREG and mii-tool so it's not really necessary
> to
> duplicate them here.

Ok, I remove this then.

> 
> Ben.
> 
> --
> Ben Hutchings, Staff Engineer, Solarflare
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
> 
> 

-- 
Emeric