[PATCH] natsemi: make cable length magic configurable

[PATCH] natsemi: make cable length magic configurable

[Jean Delvare]

From: Olaf Kirch <okir@suse.de>

We had a customer report concerning problems with a Natsemi DP83815-D
and long cables. With 100m cables, the network would be essentially dead,
not a single packet would get through either way. We had to apply the
patch below to make it work.

The patch adds a module parameter named "no_cable_magic" that does
two things:

 -	Unconditionally set the DSPCFG register to the
	fixed value. Without this change, the chip apparently
	never completes autonegotiation in the tested configuration.

	This has been an unconditional assignment for a long time,
	until this was changed in 2.6.11 (there's an interesting
	explanation in the ChangeLog, subject is
	"[PATCH] natsemi long cable fix", bk commit is
	5871b81bf2b5cf188deab0d414dce104fcb69ca6, git commit in
	tglx/history tree is c0d51c67f9c398279a95c5a7df387f2d9a586c98.

 -	Skip the bit banging in {,un}do_cable_magic. It seems that
	if we write the DSPCFG register as above, a rev D chip will report
	all cables as "short cables", which do_cable_magic detects, and
	trying to be helpful it will "fix" the attenuation coefficient.

I admit the use of a module parameter is ugly, but I didn't find a sane
way to fix this - especially since the magic registers we're changing
are undocumented.

Signed-off-by: Olaf Kirch <okir@suse.de>
Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
We have been including this patch in the SLES kernels since
January 2007, and some POS customers still need that today.

 drivers/net/natsemi.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

--- a/drivers/net/ethernet/natsemi/natsemi.c
+++ b/drivers/net/ethernet/natsemi/natsemi.c
@@ -83,6 +83,9 @@ static int rx_copybreak;
 
 static int dspcfg_workaround = 1;
 
+/* Set to disable cable magic - needed for very long cables on some chips */
+static int no_cable_magic;
+
 /* Used to pass the media type, etc.
    Both 'options[]' and 'full_duplex[]' should exist for driver
    interoperability.
@@ -141,6 +144,7 @@ module_param(mtu, int, 0);
 module_param(debug, int, 0);
 module_param(rx_copybreak, int, 0);
 module_param(dspcfg_workaround, int, 0);
+module_param(no_cable_magic, int, 0);
 module_param_array(options, int, NULL, 0);
 module_param_array(full_duplex, int, NULL, 0);
 MODULE_PARM_DESC(mtu, "DP8381x MTU (all boards)");
@@ -148,6 +152,9 @@ MODULE_PARM_DESC(debug, "DP8381x default
 MODULE_PARM_DESC(rx_copybreak,
 	"DP8381x copy breakpoint for copy-only-tiny-frames");
 MODULE_PARM_DESC(dspcfg_workaround, "DP8381x: control DspCfg workaround");
+MODULE_PARM_DESC(no_cable_magic,
+	"DP8381x: set to 1 to disable magic workaround for short cables "
+	"(may help with long cables");
 MODULE_PARM_DESC(options,
 	"DP8381x: Bits 0-3: media type, bit 17: full duplex");
 MODULE_PARM_DESC(full_duplex, "DP8381x full duplex setting(s) (1)");
@@ -1221,7 +1228,7 @@ static void init_phy_fixup(struct net_de
 		writew(1, ioaddr + PGSEL);
 		writew(PMDCSR_VAL, ioaddr + PMDCSR);
 		writew(TSTDAT_VAL, ioaddr + TSTDAT);
-		np->dspcfg = (np->srr <= SRR_DP83815_C)?
+		np->dspcfg = (np->srr <= SRR_DP83815_C || no_cable_magic) ?
 			DSPCFG_VAL : (DSPCFG_COEF | readw(ioaddr + DSPCFG));
 		writew(np->dspcfg, ioaddr + DSPCFG);
 		writew(SDCFG_VAL, ioaddr + SDCFG);
@@ -1587,7 +1594,7 @@ static void do_cable_magic(struct net_de
 	if (dev->if_port != PORT_TP)
 		return;
 
-	if (np->srr >= SRR_DP83816_A5)
+	if (np->srr >= SRR_DP83816_A5 || no_cable_magic)
 		return;
 
 	/*
@@ -1632,7 +1639,7 @@ static void undo_cable_magic(struct net_
 	if (dev->if_port != PORT_TP)
 		return;
 
-	if (np->srr >= SRR_DP83816_A5)
+	if (np->srr >= SRR_DP83816_A5 || no_cable_magic)
 		return;
 
 	writew(1, ioaddr + PGSEL);

-- 
Jean Delvare
Suse L3

Re: [PATCH] natsemi: make cable length magic configurable

[Ben Hutchings]

On Thu, 2011-11-24 at 14:43 +0100, Jean Delvare wrote:
> From: Olaf Kirch <okir@suse.de>
> 
> We had a customer report concerning problems with a Natsemi DP83815-D
> and long cables. With 100m cables, the network would be essentially dead,
> not a single packet would get through either way. We had to apply the
> patch below to make it work.
> 
> The patch adds a module parameter named "no_cable_magic" that does
> two things:
> 
>  -	Unconditionally set the DSPCFG register to the
> 	fixed value. Without this change, the chip apparently
> 	never completes autonegotiation in the tested configuration.
> 
> 	This has been an unconditional assignment for a long time,
> 	until this was changed in 2.6.11 (there's an interesting
> 	explanation in the ChangeLog, subject is
> 	"[PATCH] natsemi long cable fix", bk commit is
> 	5871b81bf2b5cf188deab0d414dce104fcb69ca6, git commit in
> 	tglx/history tree is c0d51c67f9c398279a95c5a7df387f2d9a586c98.
> 
>  -	Skip the bit banging in {,un}do_cable_magic. It seems that
> 	if we write the DSPCFG register as above, a rev D chip will report
> 	all cables as "short cables", which do_cable_magic detects, and
> 	trying to be helpful it will "fix" the attenuation coefficient.
> 
> I admit the use of a module parameter is ugly, but I didn't find a sane
> way to fix this - especially since the magic registers we're changing
> are undocumented.
[...]

This could be implemented as an ethtool 'private flag'.  However, the
ethtool utility currently does not provide an interface to them.
Perhaps you could implement both the private flag and the module
parameter for now, and then drop the module parameter some time after
the utility has been updated.

You would need to:
1. Number the flags starting from 0.  Well, that was easy.
2. Implement {get,set}_priv_flags() operations to access all flags as
   a bitmask.
3. Expose the flag names as string set ETH_SS_PRIV_FLAGS accessed by
   get_sset_count() and get_strings() operations.

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] natsemi: make cable length magic configurable

[David Miller]

From: Jean Delvare <jdelvare@suse.de>
Date: Thu, 24 Nov 2011 14:43:59 +0100

> We had a customer report concerning problems with a Natsemi DP83815-D
> and long cables. With 100m cables, the network would be essentially dead,
> not a single packet would get through either way. We had to apply the
> patch below to make it work.

Please do not add new private device driver module knobs.

Instead, add generic flags to the ethtool interface or similar to
control device behavior.

Re: [PATCH] natsemi: make cable length magic configurable

[Ben Hutchings]

On Fri, 2011-11-25 at 01:37 -0500, David Miller wrote:
> From: Jean Delvare <jdelvare@suse.de>
> Date: Thu, 24 Nov 2011 14:43:59 +0100
> 
> > We had a customer report concerning problems with a Natsemi DP83815-D
> > and long cables. With 100m cables, the network would be essentially dead,
> > not a single packet would get through either way. We had to apply the
> > patch below to make it work.
> 
> Please do not add new private device driver module knobs.
> 
> Instead, add generic flags to the ethtool interface or similar to
> control device behavior.

This isn't a generic flag; it seems to be a workaround for a specific
bug that unfortunately can't be automatic.

That's why I suggested an ethtool private flag.

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] natsemi: make cable length magic configurable

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Mon, 28 Nov 2011 17:51:04 +0000

> On Fri, 2011-11-25 at 01:37 -0500, David Miller wrote:
>> From: Jean Delvare <jdelvare@suse.de>
>> Date: Thu, 24 Nov 2011 14:43:59 +0100
>> 
>> > We had a customer report concerning problems with a Natsemi DP83815-D
>> > and long cables. With 100m cables, the network would be essentially dead,
>> > not a single packet would get through either way. We had to apply the
>> > patch below to make it work.
>> 
>> Please do not add new private device driver module knobs.
>> 
>> Instead, add generic flags to the ethtool interface or similar to
>> control device behavior.
> 
> This isn't a generic flag; it seems to be a workaround for a specific
> bug that unfortunately can't be automatic.
> 
> That's why I suggested an ethtool private flag.

Fair enough.

Re: [PATCH] natsemi: make cable length magic configurable

[Jean Delvare]

Hi Ben,

Thanks for you fast reply and sorry for my very slow one.

Le jeudi 24 novembre 2011 à 19:25 +0000, Ben Hutchings a écrit :
> On Thu, 2011-11-24 at 14:43 +0100, Jean Delvare wrote:
> > From: Olaf Kirch <okir@suse.de>
> > 
> > We had a customer report concerning problems with a Natsemi DP83815-D
> > and long cables. With 100m cables, the network would be essentially dead,
> > not a single packet would get through either way. We had to apply the
> > patch below to make it work.
> > 
> > The patch adds a module parameter named "no_cable_magic" that does
> > two things:
> > 
> >  -	Unconditionally set the DSPCFG register to the
> > 	fixed value. Without this change, the chip apparently
> > 	never completes autonegotiation in the tested configuration.
> > 
> > 	This has been an unconditional assignment for a long time,
> > 	until this was changed in 2.6.11 (there's an interesting
> > 	explanation in the ChangeLog, subject is
> > 	"[PATCH] natsemi long cable fix", bk commit is
> > 	5871b81bf2b5cf188deab0d414dce104fcb69ca6, git commit in
> > 	tglx/history tree is c0d51c67f9c398279a95c5a7df387f2d9a586c98.
> > 
> >  -	Skip the bit banging in {,un}do_cable_magic. It seems that
> > 	if we write the DSPCFG register as above, a rev D chip will report
> > 	all cables as "short cables", which do_cable_magic detects, and
> > 	trying to be helpful it will "fix" the attenuation coefficient.
> > 
> > I admit the use of a module parameter is ugly, but I didn't find a sane
> > way to fix this - especially since the magic registers we're changing
> > are undocumented.
> [...]
> 
> This could be implemented as an ethtool 'private flag'.  However, the
> ethtool utility currently does not provide an interface to them.
> Perhaps you could implement both the private flag and the module
> parameter for now, and then drop the module parameter some time after
> the utility has been updated.

I see that you updated the ethtool utility meanwhile, thanks for doing
that.

> You would need to:
> 1. Number the flags starting from 0.  Well, that was easy.
> 2. Implement {get,set}_priv_flags() operations to access all flags as
>    a bitmask.
> 3. Expose the flag names as string set ETH_SS_PRIV_FLAGS accessed by
>    get_sset_count() and get_strings() operations.

I was looking for an example, but there doesn't seem to be any driver
implementing private flags at the moment?

I am also unsure if a private flag is a suitable solution to the problem
at hand. I am a little worried about the timing, as the non-default
value can't be set before the network device is available, I'm afraid
the network initialization scripts may kick in before one has a chance
to set the flag with ethtool. I suppose this could be problematic, but
then again I don't know much about network device drivers, I don't even
know for sure when method .ndo_open is called...

Furthermore I don't quite get why we can't just go with the module
parameter. As I understand it, this is a crappy driver for crappy, rare
hardware. The driver already has a module parameter to work around a
hardware bug (dspcfg_workaround), I don't quite see why adding a second
one would be a problem. At least it is consistent.

Thanks,
-- 
Jean Delvare
Suse L3

Re: [PATCH] natsemi: make cable length magic configurable

[Ben Hutchings]

On Mon, 2012-07-16 at 14:26 +0200, Jean Delvare wrote:
[...]
> > You would need to:
> > 1. Number the flags starting from 0.  Well, that was easy.
> > 2. Implement {get,set}_priv_flags() operations to access all flags as
> >    a bitmask.
> > 3. Expose the flag names as string set ETH_SS_PRIV_FLAGS accessed by
> >    get_sset_count() and get_strings() operations.
> 
> I was looking for an example, but there doesn't seem to be any driver
> implementing private flags at the moment?

No.

> I am also unsure if a private flag is a suitable solution to the problem
> at hand. I am a little worried about the timing, as the non-default
> value can't be set before the network device is available, I'm afraid
> the network initialization scripts may kick in before one has a chance
> to set the flag with ethtool. I suppose this could be problematic, but
> then again I don't know much about network device drivers, I don't even
> know for sure when method .ndo_open is called...

It's called when the interface is brought up (ifconfig up, ip link set
up, start NetworkManager...).  Some distributions provide a
configuration option(s) to run ethtool while bringing an interface up.

> Furthermore I don't quite get why we can't just go with the module
> parameter. As I understand it, this is a crappy driver for crappy, rare
> hardware. The driver already has a module parameter to work around a
> hardware bug (dspcfg_workaround), I don't quite see why adding a second
> one would be a problem. At least it is consistent.

David can be quite insistent about finding an alternative to module
parameters.

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] natsemi: make cable length magic configurable

[Mark Brown]

On Mon, Jul 16, 2012 at 02:08:54PM +0100, Ben Hutchings wrote:
> On Mon, 2012-07-16 at 14:26 +0200, Jean Delvare wrote:

> > Furthermore I don't quite get why we can't just go with the module
> > parameter. As I understand it, this is a crappy driver for crappy, rare
> > hardware. The driver already has a module parameter to work around a
> > hardware bug (dspcfg_workaround), I don't quite see why adding a second
> > one would be a problem. At least it is consistent.

> David can be quite insistent about finding an alternative to module
> parameters.

The hardware isn't that rare or bad - it's pretty widely deployed in
100Mbit systems (including lots of embedded ones) and performs well for
the systems it's targetting.  You'd not use it for a modern server but
if what you need is 100M it's a fairly good part.

The dspcfg_workaround change was added for one specific board, I'd be
really surprised if it were useful for anything other than the board it
was originally developed for, I'd not be surprised to learn that it's a
hardware issue in that board.  It disables a documented erratum
workaround to give a performance improvement, it was done for one board
that for some reason triggered the erratum a lot but with less severe
consequences than normal.

Re: [PATCH] natsemi: make cable length magic configurable

[David Miller]

From: Mark Brown <broonie@opensource.wolfsonmicro.com>
Date: Mon, 16 Jul 2012 14:57:40 +0100

> On Mon, Jul 16, 2012 at 02:08:54PM +0100, Ben Hutchings wrote:
>> On Mon, 2012-07-16 at 14:26 +0200, Jean Delvare wrote:
> 
>> > Furthermore I don't quite get why we can't just go with the module
>> > parameter. As I understand it, this is a crappy driver for crappy, rare
>> > hardware. The driver already has a module parameter to work around a
>> > hardware bug (dspcfg_workaround), I don't quite see why adding a second
>> > one would be a problem. At least it is consistent.
> 
>> David can be quite insistent about finding an alternative to module
>> parameters.
> 
> The hardware isn't that rare or bad - it's pretty widely deployed in
> 100Mbit systems (including lots of embedded ones) and performs well for
> the systems it's targetting.  You'd not use it for a modern server but
> if what you need is 100M it's a fairly good part.

I don't want to hear any excuses for not implementing a configuration
interface properly.  I'm sure if I allowed it, everyone would be able
to come up with a special set of circumstances which justified the
module parameter.

Sorry, no.