NetBSD Problem Report #47765

From www@NetBSD.org  Thu Apr 25 07:10:14 2013
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	by www.NetBSD.org (Postfix) with ESMTP id 3D81863F4BF
	for <gnats-bugs@gnats.NetBSD.org>; Thu, 25 Apr 2013 07:10:14 +0000 (UTC)
Message-Id: <20130425071013.102D763F4BF@www.NetBSD.org>
Date: Thu, 25 Apr 2013 07:10:13 +0000 (UTC)
From: webpages@sprow.co.uk
Reply-To: webpages@sprow.co.uk
To: gnats-bugs@NetBSD.org
Subject: Patches for src/sys/ic/smc91cxx.c
X-Send-Pr-Version: www-1.0

>Number:         47765
>Category:       kern
>Synopsis:       Patches for src/sys/ic/smc91cxx.c
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Apr 25 07:15:00 +0000 2013
>Closed-Date:    Mon Oct 07 09:53:41 +0000 2013
>Last-Modified:  Mon Oct 07 09:53:41 +0000 2013
>Originator:     Robert Sprowson
>Release:        N/A
>Organization:
N/A
>Environment:
N/A
>Description:
Have recently been porting the smc91cxx driver to another platform and have a few fixes for it to feed back, see patch below. Change details are as follows.

Line 126, 146, 158:
Mark tables as static to remove compiler warning.

Line 494:
Resolve the correct reset recovery time. The slowest of the chips supported is the 91C111 which requires 50ms for its PHY to reset. The reset pulse itself can be very short, but is set to arbitrary 5us here.

Line 709, 812:
Spelling mistakes ammended.

Line 1022, 1160:
When performing a read from the FIFO there must be a delay of at least 375ns after changing the FIFO pointer to allow it to preload. Add that delay (well, 1000ns, since delay() isn't that fine).

Line 1097:
Missing interrupt ACK write meant that this would hang the machine with an interrupt jammed on. This has not surfaced before because in NetBSD the internal MII interrupts are never unmasked so this code never fires, instead relying on the 1s mii_tick polling. In my port I don't poll, which exposed this problem.

Line 1244:
Dead variable 'dp' removed.

Line 1298:
The read of packetno is repeated at the label 'again', so no need to do it twice. Just goto 'again'.
>How-To-Repeat:

>Fix:
--- smc91cxx_183.c	Tue Feb 05 22:50:34 2013
+++ smc91cxx_new.c	Thu Apr 25 06:49:32 2013
@@ -126,7 +126,7 @@
 /* XXX Hardware padding doesn't work yet(?) */
 #define	SMC91CXX_SW_PAD

-const char *smc91cxx_idstrs[] = {
+static const char *smc91cxx_idstrs[] = {
 	NULL,				/* 0 */
 	NULL,				/* 1 */
 	NULL,				/* 2 */
@@ -146,7 +146,7 @@
 };

 /* Supported media types. */
-const int smc91cxx_media[] = {
+static const int smc91cxx_media[] = {
 	IFM_ETHER|IFM_10_T,
 	IFM_ETHER|IFM_10_5,
 };
@@ -158,7 +158,7 @@
 u_int32_t smc91cxx_mii_bitbang_read(device_t);
 void smc91cxx_mii_bitbang_write(device_t, u_int32_t);

-const struct mii_bitbang_ops smc91cxx_mii_bitbang_ops = {
+static const struct mii_bitbang_ops smc91cxx_mii_bitbang_ops = {
 	smc91cxx_mii_bitbang_read,
 	smc91cxx_mii_bitbang_write,
 	{
@@ -494,16 +494,14 @@

 	/*
 	 * This resets the registers mostly to defaults, but doesn't
-	 * affect the EEPROM.  After the reset cycle, we pause briefly
-	 * for the chip to recover.
-	 *
-	 * XXX how long are we really supposed to delay?  --thorpej
+	 * affect the EEPROM.  The longest reset recovery time of those devices
+	 * supported is the 91C111. Section 7.8 of its datasheet asks for 50ms.
 	 */
 	SMC_SELECT_BANK(sc, 0);
 	bus_space_write_2(bst, bsh, RECV_CONTROL_REG_W, RCR_SOFTRESET);
-	delay(100);
+	delay(5);
 	bus_space_write_2(bst, bsh, RECV_CONTROL_REG_W, 0);
-	delay(200);
+	delay(50000);

 	bus_space_write_2(bst, bsh, TXMIT_CONTROL_REG_W, 0);

@@ -709,7 +707,7 @@
 	if (packetno & ARR_FAILED || timo == 0) {
 		/*
 		 * No transmit memory is available.  Record the number
-		 * of requestd pages and enable the allocation completion
+		 * of requested pages and enable the allocation completion
 		 * interrupt.  Set up the watchdog timer in case we miss
 		 * the interrupt.  Mark the interface as active so that
 		 * no one else attempts to transmit while we're allocating
@@ -812,7 +810,7 @@

  readcheck:
 	/*
-	 * Check for incoming pcakets.  We don't want to overflow the small
+	 * Check for incoming packets.  We don't want to overflow the small
 	 * RX FIFO.  If nothing has arrived, attempt to queue another
 	 * transmit packet.
 	 */
@@ -1022,10 +1020,12 @@
 		bus_space_write_1(bst, bsh, PACKET_NUM_REG_B, packetno);

 		/*
-		 * Position the pointer to the beginning of the packet.
+		 * Position the pointer to the beginning of the packet, wait
+		 * for preload.
 		 */
 		bus_space_write_2(bst, bsh, POINTER_REG_W,
 		    PTR_AUTOINC | PTR_READ /* | 0x0000 */);
+		delay(1);

 		/*
 		 * Fetch the TX status word.  This will be a copy of
@@ -1097,11 +1097,12 @@
 		ifp->if_timer = 0;
 	}

+	/*
+	 * Internal PHY status change
+	 */
 	if (sc->sc_chipid == CHIP_91C111 && sc->sc_internal_phy &&
 	    (status & IM_MD_INT)) {
-		/*
-		 * Internal PHY status change
-		 */
+		smc91cxx_intr_ack_write(bst, bsh, IM_MD_INT);
 		mii_tick(&sc->sc_mii);
 	}

@@ -1160,6 +1161,7 @@

 	bus_space_write_2(bst, bsh, POINTER_REG_W,
 	    PTR_READ | PTR_RCV | PTR_AUTOINC /* | 0x0000 */);
+	delay(1);

 	/*
 	 * First two words are status and packet length.
@@ -1244,11 +1246,9 @@
 			*data = bus_space_read_1(bst, bsh, DATA_REG_B);
 		}
 	} else {
-		u_int8_t *dp;
-
 		m->m_data = (void *) ALIGN(mtod(m, void *));
 		eh = mtod(m, struct ether_header *);
-		dp = data = mtod(m, u_int8_t *);
+		data = mtod(m, u_int8_t *);
 		KASSERT(trunc_page((uintptr_t)data) == trunc_page((uintptr_t)data + packetlen - 1));
 		if (packetlen > 3)
 			bus_space_read_multi_stream_4(bst, bsh, DATA_REG_W,
@@ -1298,9 +1298,6 @@
 	/*
 	 * Check for another packet.
 	 */
-	packetno = bus_space_read_2(bst, bsh, FIFO_PORTS_REG_W);
-	if (packetno & FIFO_REMPTY)
-		return;
 	goto again;
 }


>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: port-arm32-maintainer->kern-bug-people
Responsible-Changed-By: dholland@NetBSD.org
Responsible-Changed-When: Sun, 28 Apr 2013 16:29:32 +0000
Responsible-Changed-Why:
machine-independent driver -> machine-independent bug


From: Sprow <webpages@sprow.co.uk>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/47765
Date: Sat, 06 Jul 2013 18:10:39 +0100

 Hi,
 Back in April I submitted 2 patches for sys/dev/ic/smc91cxx.c (and its
 companion patch PR/47788). 

 Chuck (chs) kindly tested the patches on an m68k big endian system, while my
 work was on an ARM little endian system. I believe Chuck is very busy (last
 email 1 month ago). 

 There's an easy win for someone with CVS write permission to get these 2
 tickets off the nightly ticket reports. Is that possible?

 Thanks,
 Robert.

From: "Chuck Silvers" <chs@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/47765 CVS commit: src/sys/dev/ic
Date: Sat, 7 Sep 2013 18:55:29 +0000

 Module Name:	src
 Committed By:	chs
 Date:		Sat Sep  7 18:55:29 UTC 2013

 Modified Files:
 	src/sys/dev/ic: smc91cxx.c

 Log Message:
 apply changes from Robert Sprowson in PR 47765 and PR 47788:
  - make sure we wait long enough after resetting the chip.
  - add the necessary delay after changing the FIFO pointer.
  - add a missing interrupt ACK.
  - fix padding of short and odd-length packets.

 and a few more changes from me:
  - do 2-byte writes in most places even if SMC91CXX_NO_BYTE_WRITE
    is not defined.  the only ones still conditionalized are
    writing to the interrupt ack/mask registers.
  - the only big-endian front-end of this driver (on mac68k) uses
    a bus_space that does all the byte-swapping in that layer,
    so remove the explicit byte-swapping in the MI part.

 tested on mac68k.


 To generate a diff of this commit:
 cvs rdiff -u -r1.84 -r1.85 src/sys/dev/ic/smc91cxx.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->feedback
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Mon, 07 Oct 2013 01:09:28 +0000
State-Changed-Why:
Is this fixed too?


From: Robert Sprowson <webpages@sprow.co.uk>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/47765
Date: Mon, 07 Oct 2013 08:51:04 +0100

 On 07 Oct, <dholland@NetBSD.org> wrote:
 > Synopsis: Patches for src/sys/ic/smc91cxx.c
 > State-Changed-From-To: open->feedback
 > State-Changed-By: dholland@NetBSD.org
 > State-Changed-When: Mon, 07 Oct 2013 01:09:28 +0000
 > State-Changed-Why:
 > Is this fixed too?

 Yes - Chuck committed this in revision 1.86. PR can be closed.

State-Changed-From-To: feedback->closed
State-Changed-By: martin@NetBSD.org
State-Changed-When: Mon, 07 Oct 2013 09:53:41 +0000
State-Changed-Why:
confirmed fixed


>Unformatted:

Home	PR Database Search