linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 3/3] dm9601: add support ethtool style utility
@ 2016-03-10 11:24 Joseph CHANG
  2016-03-10 11:49 ` Ben Hutchings
  0 siblings, 1 reply; 6+ messages in thread
From: Joseph CHANG @ 2016-03-10 11:24 UTC (permalink / raw)
  To: Peter Korsgaard, netdev, linux-usb, linux-kernel, Joseph CHANG
  Cc: Joseph Chang

Add function dm9601_set_eeprom which tested good with ethtool
utility, include the eeprom words dump and the eeprom byte write.

Signed-off-by: Joseph CHANG <josright123@gmail.com>
---
 drivers/net/usb/dm9601.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c
index 50095df..a6904f4 100644
--- a/drivers/net/usb/dm9601.c
+++ b/drivers/net/usb/dm9601.c
@@ -61,6 +61,7 @@
 #define DM_RX_OVERHEAD	7	/* 3 byte header + 4 byte crc tail */
 #define DM_TIMEOUT	1000
 #define	DM_EP3I_VAL	0x07
+#define	MD96XX_EEPROM_MAGIC	0x9620
 
 static int dm_read(struct usbnet *dev, u8 reg, u16 length, void *data)
 {
@@ -289,6 +290,43 @@ static int dm9601_get_eeprom(struct net_device *net,
 	return 0;
 }
 
+static int dm9601_set_eeprom(struct net_device *net,
+			     struct ethtool_eeprom *eeprom, u8 *data)
+{
+	struct usbnet *dev = netdev_priv(net);
+	int offset = eeprom->offset;
+	int len = eeprom->len;
+	int done;
+
+	if (eeprom->magic != MD96XX_EEPROM_MAGIC) {
+		netdev_dbg(dev->net, "EEPROM: magic value mismatch, magic = 0x%x",
+			   eeprom->magic);
+		return -EINVAL;
+	}
+
+	while (len > 0) {
+		if (len & 1 || offset & 1) {
+			int which = offset & 1;
+			u8 tmp[2];
+
+			dm_read_eeprom_word(dev, offset / 2, tmp);
+			tmp[which] = *data;
+			dm_write_eeprom_word(dev, offset / 2,
+					     tmp[0] | tmp[1] << 8);
+			mdelay(10);
+			done = 1;
+		} else {
+			dm_write_eeprom_word(dev, offset / 2,
+					     data[0] | data[1] << 8);
+			done = 2;
+		}
+		data += done;
+		offset += done;
+		len -= done;
+	}
+	return 0;
+}
+
 static int dm9601_mdio_read(struct net_device *netdev, int phy_id, int loc)
 {
 	struct usbnet *dev = netdev_priv(netdev);
@@ -354,6 +392,7 @@ static const struct ethtool_ops dm9601_ethtool_ops = {
 	.set_msglevel	= usbnet_set_msglevel,
 	.get_eeprom_len	= dm9601_get_eeprom_len,
 	.get_eeprom	= dm9601_get_eeprom,
+	.set_eeprom	= dm9601_set_eeprom,
 	.get_settings	= usbnet_get_settings,
 	.set_settings	= usbnet_set_settings,
 	.nway_reset	= usbnet_nway_reset,
-- 
2.1.4

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

* Re: [PATCH 3/3] dm9601: add support ethtool style utility
  2016-03-10 11:24 [PATCH 3/3] dm9601: add support ethtool style utility Joseph CHANG
@ 2016-03-10 11:49 ` Ben Hutchings
  0 siblings, 0 replies; 6+ messages in thread
From: Ben Hutchings @ 2016-03-10 11:49 UTC (permalink / raw)
  To: Joseph CHANG, Peter Korsgaard, netdev, linux-usb, linux-kernel
  Cc: Joseph Chang

[-- Attachment #1: Type: text/plain, Size: 2571 bytes --]

The subject line on this is very vague; it should say which ethtool
operation you're implementing.

On Thu, 2016-03-10 at 19:24 +0800, Joseph CHANG wrote:

> Add function dm9601_set_eeprom which tested good with ethtool
> utility, include the eeprom words dump and the eeprom byte write.
> 
> Signed-off-by: Joseph CHANG <josright123@gmail.com>
> ---
>  drivers/net/usb/dm9601.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c
> index 50095df..a6904f4 100644
> --- a/drivers/net/usb/dm9601.c
> +++ b/drivers/net/usb/dm9601.c
> @@ -61,6 +61,7 @@
>  #define DM_RX_OVERHEAD	7	/* 3 byte header + 4 byte crc tail */
>  #define DM_TIMEOUT	1000
>  #define	DM_EP3I_VAL	0x07
> +#define	MD96XX_EEPROM_MAGIC	0x9620

The get_eeprom operation needs to be changed, to set eeprom->magic to
this value.

>  static int dm_read(struct usbnet *dev, u8 reg, u16 length, void *data)
>  {
> @@ -289,6 +290,43 @@ static int dm9601_get_eeprom(struct net_device *net,
>  	return 0;
>  }
>  
> +static int dm9601_set_eeprom(struct net_device *net,
> +			     struct ethtool_eeprom *eeprom, u8 *data)
> +{
> +	struct usbnet *dev = netdev_priv(net);
> +	int offset = eeprom->offset;
> +	int len = eeprom->len;
> +	int done;
> +
> +	if (eeprom->magic != MD96XX_EEPROM_MAGIC) {
> +		netdev_dbg(dev->net, "EEPROM: magic value mismatch, magic = 0x%x",
> +			   eeprom->magic);
> +		return -EINVAL;
> +	}
> +
> +	while (len > 0) {
> +		if (len & 1 || offset & 1) {

Given that the get_eeprom operation only handles word-aligned reads, is
it really important to support misaligned writes in set_eeprom?

Also, this test should be 'if (len == 1 || offset & 1)'.  Consider a
write with offset = 2, len = 3.  You want to write a word on the first
iteration, then a byte on the second iteration.

> +			int which = offset & 1;
> +			u8 tmp[2];
> +
> +			dm_read_eeprom_word(dev, offset / 2, tmp);
> +			tmp[which] = *data;
> +			dm_write_eeprom_word(dev, offset / 2,
> +					     tmp[0] | tmp[1] << 8);
> +			mdelay(10);

Why is a delay required here, but not in the other case?

> +			done = 1;
> +		} else {
> +			dm_write_eeprom_word(dev, offset / 2,
> +					     data[0] | data[1] << 8);
> +			done = 2;
> +		}
> +		data += done;
> +		offset += done;
> +		len -= done;
> +	}
> +	return 0;
> +}
[...]

Ben.

-- 
Ben Hutchings
To err is human; to really foul things up requires a computer.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

* Re: [PATCH 3/3] dm9601: add support ethtool style utility
  2016-03-11 11:33     ` Ben Hutchings
@ 2016-03-11 11:42       ` Ben Hutchings
  0 siblings, 0 replies; 6+ messages in thread
From: Ben Hutchings @ 2016-03-11 11:42 UTC (permalink / raw)
  To: Joseph Chang, 'Joseph CHANG', 'Peter Korsgaard',
	netdev, linux-usb, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

On Fri, 2016-03-11 at 11:33 +0000, Ben Hutchings wrote:
> On Fri, 2016-03-11 at 19:08 +0800, Joseph Chang wrote:
> > 
> > I tested by
> >  ./ethtool -E eth0 magic 0x9620 offset 0 length 3 value 0xf1 value 0xf2 value 0xf3
> > 
> > I think ethtool need [ value N ] for each byte (so need three "value xx" in above command line), 
> > am I right?
> > 
> > Oh, I can see it goes wrong~ Thanks~
> [...]
> 
> You can only pass one byte on the command line and that forces the
> length to be 1.  To set multiple bytes, you need to provide them on
> stdin instead.

...as raw binary data, not hexadecimal strings.

Ben.

-- 
Ben Hutchings
73.46% of all statistics are made up.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

* Re: [PATCH 3/3] dm9601: add support ethtool style utility
  2016-03-11 11:08   ` Joseph Chang
@ 2016-03-11 11:33     ` Ben Hutchings
  2016-03-11 11:42       ` Ben Hutchings
  0 siblings, 1 reply; 6+ messages in thread
From: Ben Hutchings @ 2016-03-11 11:33 UTC (permalink / raw)
  To: Joseph Chang, 'Joseph CHANG', 'Peter Korsgaard',
	netdev, linux-usb, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

On Fri, 2016-03-11 at 19:08 +0800, Joseph Chang wrote:
> I tested by
>  ./ethtool -E eth0 magic 0x9620 offset 0 length 3 value 0xf1 value 0xf2 value 0xf3
> 
> I think ethtool need [ value N ] for each byte (so need three "value xx" in above command line), 
> am I right?
> 
> Oh, I can see it goes wrong~ Thanks~
[...]

You can only pass one byte on the command line and that forces the
length to be 1.  To set multiple bytes, you need to provide them on
stdin instead.

Ben.

-- 
Ben Hutchings
73.46% of all statistics are made up.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

* RE: [PATCH 3/3] dm9601: add support ethtool style utility
  2016-03-10 13:17 ` Ben Hutchings
@ 2016-03-11 11:08   ` Joseph Chang
  2016-03-11 11:33     ` Ben Hutchings
  0 siblings, 1 reply; 6+ messages in thread
From: Joseph Chang @ 2016-03-11 11:08 UTC (permalink / raw)
  To: 'Ben Hutchings', 'Joseph Chang',
	'Joseph CHANG', 'Peter Korsgaard',
	netdev, linux-usb, linux-kernel

I tested by
 ./ethtool -E eth0 magic 0x9620 offset 0 length 3 value 0xf1 value 0xf2 value 0xf3

I think ethtool need [ value N ] for each byte (so need three "value xx" in above command line), 
am I right?

Oh, I can see it goes wrong~ Thanks~

* I will go a vacation from now on, this issue will be study later.
 Any helpful reference implementation data is appreciated. 
 
Best Regards,
Joseph CHANG
System Application Engineering Division
Davicom Semiconductor, Inc.
No. 6 Li-Hsin 6th Rd., Science-Based Park,
Hsin-Chu, Taiwan.
Tel: 886-3-5798797 Ex 8534
Fax: 886-3-5646929
Web: http://www.davicom.com.tw


-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk] 
Sent: Thursday, March 10, 2016 9:18 PM
To: Joseph Chang; 'Joseph CHANG'; 'Peter Korsgaard'; netdev@vger.kernel.org; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/3] dm9601: add support ethtool style utility

On Thu, 2016-03-10 at 20:51 +0800, Joseph Chang wrote:
> I did verify to dump EEPROM data and also write EEPROM by per byte.
> 
> 1.Plug dm9601/dm9621 adapter and has get dm9601.ko be 'insmod' to have 'eht0',
> 2.Run ethtool v3.7 (as attached executable file and it's help display.)
> 3. Commands:
>    ./ethtool -e eth0  (dump EEPROM data for all the .get_eeprom_len )
>    ./ethtool -E eth0 magic 0x9620 offset 0 value 0xf1  (write 0xf1 to eeprom byte0)
>    ./ethtool -E eth0 magic 0x9620 offset 1 value 0xf2  (write 0xf2 to eeprom byte1)
>    ./ethtool -E eth0 magic 0x9620 offset 2 value 0xf3  (write 0xf3 to eeprom byte2)
[...]

So you only tested writing 1 byte at a time.  Try again with 3 bytes
and you'll see how it goes wrong.

Ben.

-- 
Ben Hutchings
To err is human; to really foul things up requires a computer.

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

* Re: [PATCH 3/3] dm9601: add support ethtool style utility
       [not found] <201603101253.u2ACrnUJ002444@mail.davicom.com.tw>
@ 2016-03-10 13:17 ` Ben Hutchings
  2016-03-11 11:08   ` Joseph Chang
  0 siblings, 1 reply; 6+ messages in thread
From: Ben Hutchings @ 2016-03-10 13:17 UTC (permalink / raw)
  To: Joseph Chang, 'Joseph CHANG', 'Peter Korsgaard',
	netdev, linux-usb, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

On Thu, 2016-03-10 at 20:51 +0800, Joseph Chang wrote:
> I did verify to dump EEPROM data and also write EEPROM by per byte.
> 
> 1.Plug dm9601/dm9621 adapter and has get dm9601.ko be 'insmod' to have 'eht0',
> 2.Run ethtool v3.7 (as attached executable file and it's help display.)
> 3. Commands:
>    ./ethtool -e eth0  (dump EEPROM data for all the .get_eeprom_len )
>    ./ethtool -E eth0 magic 0x9620 offset 0 value 0xf1  (write 0xf1 to eeprom byte0)
>    ./ethtool -E eth0 magic 0x9620 offset 1 value 0xf2  (write 0xf2 to eeprom byte1)
>    ./ethtool -E eth0 magic 0x9620 offset 2 value 0xf3  (write 0xf3 to eeprom byte2)
[...]

So you only tested writing 1 byte at a time.  Try again with 3 bytes
and you'll see how it goes wrong.

Ben.

-- 
Ben Hutchings
To err is human; to really foul things up requires a computer.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

end of thread, other threads:[~2016-03-11 11:42 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-10 11:24 [PATCH 3/3] dm9601: add support ethtool style utility Joseph CHANG
2016-03-10 11:49 ` Ben Hutchings
     [not found] <201603101253.u2ACrnUJ002444@mail.davicom.com.tw>
2016-03-10 13:17 ` Ben Hutchings
2016-03-11 11:08   ` Joseph Chang
2016-03-11 11:33     ` Ben Hutchings
2016-03-11 11:42       ` Ben Hutchings

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).