NetBSD Problem Report #49622

From www@NetBSD.org  Sun Feb  1 10:07:55 2015
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 69D35A654C
	for <gnats-bugs@gnats.NetBSD.org>; Sun,  1 Feb 2015 10:07:55 +0000 (UTC)
Message-Id: <20150201100753.B42A6A65B8@mollari.NetBSD.org>
Date: Sun,  1 Feb 2015 10:07:53 +0000 (UTC)
From: webpages@sprow.co.uk
Reply-To: webpages@sprow.co.uk
To: gnats-bugs@NetBSD.org
Subject: Minor enhancements to if_cpsw
X-Send-Pr-Version: www-1.0

>Number:         49622
>Category:       port-arm
>Synopsis:       Minor enhancements to if_cpsw
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    skrll
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Feb 01 10:10:00 +0000 2015
>Closed-Date:    Fri Mar 27 09:16:49 +0000 2015
>Last-Modified:  Fri Mar 27 09:16:49 +0000 2015
>Originator:     Robert Sprowson
>Release:        
>Organization:
>Environment:
>Description:
These changes fix a couple of bugs and add clarity in a couple of places in the source. They are patches relative to revision 1.6 of src/sys/arch/arm/omap/if_cpsw.c and 1.2 of src/sys/arch/arm/omap/if_cpswreg.h details as follows.

if_cpswreg.h
~~~~~~~~~~~~
Add definitions of number of host ports (1) and ethernet ports (2) so they can be used as loop counters in the driver.
Add the other registers in CPSW_SL_.
Add the other flag bits in CPDMA_BD_.
Correct 2x spelling mistakes.

if_cpsw.c
~~~~~~~~~
Line 599:
Parenthesis added for clarity.
Line 818:
Use bit definitions from the header to build the register poke.
Also, remove the 'BYPASS' bit (4), otherwise all the careful setup of the ALE is futile - the BYPASS bit is described in the AM3358 datasheet as "When in bypass mode, all CPGMAC_SL received packets are forwarded only to the host port (port 0)" so effectively it's a promiscuous enable. This was confirmed by injecting some packets via a crossover cable with known non-matching EUI48 destination and observing they arrive on the interface (obviously with a switch in the way these would have been masked).
Line 921:
Make use of the MDIO defines from the header.
Line 959:
During cpsw_stop the MISC interrupts were being left enabled, looks like a copy & paste from cpsw_init.
General:
Couple of typos, swap to using host/ethernet port limits from the header file, some const's.

>How-To-Repeat:

>Fix:
--- if_cpsw_1_6.c	2014-10-31 17:12:48 +0000
+++ if_cpsw_new.c	2015-02-01 09:41:48 +0000
@@ -599,9 +599,9 @@
 			if (seg == 0)
 				dw[3] |= CPDMA_BD_SOP | CPDMA_BD_OWNER |
 				    MAX(mlen, CPSW_PAD_LEN);

-			if (seg == dm->dm_nsegs - 1 && !pad)
+			if ((seg == dm->dm_nsegs - 1) && !pad)
 				dw[3] |= CPDMA_BD_EOP;

 			cpsw_set_txdesc(sc, sc->sc_txnext, &bd);
 			txfree--;
@@ -818,13 +818,13 @@
 	/* Reset SS */
 	cpsw_write_4(sc, CPSW_SS_SOFT_RESET, 1);
 	while(cpsw_read_4(sc, CPSW_SS_SOFT_RESET) & 1);

-	/* Clear table (30) and enable ALE(31) and set passthrough (4) */
-	cpsw_write_4(sc, CPSW_ALE_CONTROL, (3 << 30) | 0x10);
+	/* Clear table and enable ALE */
+	cpsw_write_4(sc, CPSW_ALE_CONTROL, ALECTL_ENABLE_ALE | ALECTL_CLEAR_TABLE);

 	/* Reset and init Sliver port 1 and 2 */
-	for (i = 0; i < 2; i++) {
+	for (i = 0; i < CPSW_ETH_PORTS; i++) {
 		uint32_t macctl;

 		/* Reset */
 		cpsw_write_4(sc, CPSW_SL_SOFT_RESET(i), 1);
@@ -921,9 +921,9 @@
 	cpsw_write_4(sc, CPSW_CPDMA_CPDMA_EOI_VECTOR, CPSW_INTROFF_MISC);

 	/* Initialze MDIO - ENABLE, PREAMBLE=0, FAULTENB, CLKDIV=0xFF */
 	/* TODO Calculate MDCLK=CLK/(CLKDIV+1) */
-	cpsw_write_4(sc, MDIOCONTROL, (1<<30) | (1<<18) | 0xFF);
+	cpsw_write_4(sc, MDIOCONTROL, MDIOCTL_ENABLE | MDIOCTL_FAULTENB | MDIOCTL_CLKDIV(0xff));

 	mii_mediachg(mii);

 	/* Write channel 0 RX HDP */
@@ -959,9 +959,9 @@
 	cpsw_write_4(sc, CPSW_CPDMA_TX_INTMASK_CLEAR, 1);
 	cpsw_write_4(sc, CPSW_CPDMA_RX_INTMASK_CLEAR, 1);
 	cpsw_write_4(sc, CPSW_WR_C_TX_EN(0), 0x0);
 	cpsw_write_4(sc, CPSW_WR_C_RX_EN(0), 0x0);
-	cpsw_write_4(sc, CPSW_WR_C_MISC_EN(0), 0x1F);
+	cpsw_write_4(sc, CPSW_WR_C_MISC_EN(0), 0x0);

 	cpsw_write_4(sc, CPSW_CPDMA_TX_TEARDOWN, 0);
 	cpsw_write_4(sc, CPSW_CPDMA_RX_TEARDOWN, 0);
 	i = 0;
@@ -982,9 +982,9 @@
 	/* Reset SS */
 	cpsw_write_4(sc, CPSW_SS_SOFT_RESET, 1);
 	while(cpsw_read_4(sc, CPSW_SS_SOFT_RESET) & 1);

-	for (i = 0; i < 2; i++) {
+	for (i = 0; i < CPSW_ETH_PORTS; i++) {
 		cpsw_write_4(sc, CPSW_SL_SOFT_RESET(i), 1);
 		while(cpsw_read_4(sc, CPSW_SL_SOFT_RESET(i)) & 1);
 	}

@@ -1148,8 +1148,9 @@

 	tx0_cp = cpsw_read_4(sc, CPSW_CPDMA_TX_CP(0));

 	if (tx0_cp == 0xfffffffc) {
+		/* Teardown, ack it */
 		cpsw_write_4(sc, CPSW_CPDMA_TX_CP(0), 0xfffffffc);
 		cpsw_write_4(sc, CPSW_CPDMA_TX_HDP(0), 0);
 		sc->sc_txrun = false;
 		return 0;
@@ -1310,9 +1311,9 @@
 	ale_entry[1] = 0x0000ffff;
 }

 static void
-cpsw_ale_entry_set(uint32_t *ale_entry, ale_entry_filed_t field, uint32_t val)
+cpsw_ale_entry_set(uint32_t *ale_entry, ale_entry_field_t field, uint32_t val)
 {
 	/* Entry type[61:60] is addr entry(1), Mcast fwd state[63:62] is fw(3)*/
 	switch (field) {
 	case ALE_ENTRY_TYPE:
@@ -1367,9 +1368,9 @@
 	ale_entry[2] = cpsw_read_4(sc, CPSW_ALE_TBLW2);
 }

 static void
-cpsw_ale_write_entry(struct cpsw_softc *sc, uint16_t idx, uint32_t *ale_entry)
+cpsw_ale_write_entry(struct cpsw_softc *sc, uint16_t idx, const uint32_t *ale_entry)
 {
 	cpsw_write_4(sc, CPSW_ALE_TBLW0, ale_entry[0]);
 	cpsw_write_4(sc, CPSW_ALE_TBLW1, ale_entry[1]);
 	cpsw_write_4(sc, CPSW_ALE_TBLW2, ale_entry[2]);
@@ -1450,9 +1451,9 @@
 	cpsw_ale_entry_set(ale_entry, ALE_PORT_NUMBER, 0);
 	cpsw_ale_write_entry(sc, 0, ale_entry);

 	/* Set outgoing MAC Address for Ports 1 and 2. */
-	for (i = 1; i < 3; ++i)
+	for (i = 1; i < (CPSW_ETH_PORTS + CPSW_CPPI_PORTS); ++i)
 		cpsw_ale_set_outgoing_mac(sc, i, mac);

 	/* Keep the broadcast address at table entry 1. */
 	cpsw_ale_entry_init(ale_entry);

--- if_cpswreg_1.2.h	2014-10-31 17:15:02 +0000
+++ if_cpswreg_new.h	2015-02-01 09:36:24 +0000
@@ -28,8 +28,11 @@

 #ifndef	_IF_CPSWREG_H
 #define	_IF_CPSWREG_H

+#define CPSW_ETH_PORTS			2
+#define CPSW_CPPI_PORTS			1
+
 #define CPSW_SS_OFFSET			0x0000
 #define CPSW_SS_IDVER			(CPSW_SS_OFFSET + 0x00)
 #define CPSW_SS_SOFT_RESET		(CPSW_SS_OFFSET + 0x08)
 #define CPSW_SS_STAT_PORT_EN		(CPSW_SS_OFFSET + 0x0C)
@@ -87,12 +90,19 @@
 #define CPSW_ALE_TBLW0			(CPSW_ALE_OFFSET + 0x3C)
 #define CPSW_ALE_PORTCTL(p)		(CPSW_ALE_OFFSET + 0x40 + ((p) * 0x04))

 #define CPSW_SL_OFFSET			0x0D80
+#define CPSW_SL_IDVER(p)		(CPSW_SL_OFFSET + (0x40 * (p)) + 0x00)
 #define CPSW_SL_MACCONTROL(p)		(CPSW_SL_OFFSET + (0x40 * (p)) + 0x04)
+#define CPSW_SL_MACSTATUS(p)		(CPSW_SL_OFFSET + (0x40 * (p)) + 0x08)
 #define CPSW_SL_SOFT_RESET(p)		(CPSW_SL_OFFSET + (0x40 * (p)) + 0x0C)
 #define CPSW_SL_RX_MAXLEN(p)		(CPSW_SL_OFFSET + (0x40 * (p)) + 0x10)
+#define CPSW_SL_BOFFTEST(p)		(CPSW_SL_OFFSET + (0x40 * (p)) + 0x14)
+#define CPSW_SL_RX_PAUSE(p)		(CPSW_SL_OFFSET + (0x40 * (p)) + 0x18)
+#define CPSW_SL_TX_PAUSE(p)		(CPSW_SL_OFFSET + (0x40 * (p)) + 0x1C)
+#define CPSW_SL_EMCONTROL(p)		(CPSW_SL_OFFSET + (0x40 * (p)) + 0x20)
 #define CPSW_SL_RX_PRI_MAP(p)		(CPSW_SL_OFFSET + (0x40 * (p)) + 0x24)
+#define CPSW_SL_TX_GAP(p)		(CPSW_SL_OFFSET + (0x40 * (p)) + 0x28)

 #define MDIO_OFFSET			0x1000
 #define MDIOCONTROL			(MDIO_OFFSET + 0x04)
 #define MDIOUSERACCESS0			(MDIO_OFFSET + 0x80)
@@ -116,16 +126,26 @@

 #define __BIT32(x) ((uint32_t)__BIT(x))
 #define __BITS32(x, y) ((uint32_t)__BITS((x), (y)))

-/* flags for desciptor word 3 */
+/* flags for descriptor word 3 */
 #define CPDMA_BD_SOP		__BIT32(31)
 #define CPDMA_BD_EOP		__BIT32(30)
 #define CPDMA_BD_OWNER		__BIT32(29)
 #define CPDMA_BD_EOQ		__BIT32(28)
 #define CPDMA_BD_TDOWNCMPLT	__BIT32(27)
 #define CPDMA_BD_PASSCRC	__BIT32(26)
+
+#define CPDMA_BD_LONG		__BIT32(25) /* Rx descriptor only */
+#define CPDMA_BD_SHORT		__BIT32(24)
+#define CPDMA_BD_MAC_CTL	__BIT32(23)
+#define CPDMA_BD_OVERRUN	__BIT32(22)
 #define CPDMA_BD_PKT_ERR_MASK	__BITS32(21,20)
+#define CPDMA_BD_RX_VLAN_ENCAP	__BIT32(19)
+#define CPDMA_BD_FROM_PORT	__BITS32(18,16)
+
+#define CPDMA_BD_TO_PORT_EN	__BIT32(20) /* Tx descriptor only */
+#define CPDMA_BD_TO_PORT	__BITS32(17,16)

 struct cpsw_cpdma_bd {
 	uint32_t word[4];
 } __packed __aligned(4);
@@ -196,9 +216,9 @@
 	ALE_ENTRY_TYPE,
 	ALE_MCAST_FWD_STATE,
 	ALE_PORT_MASK,
 	ALE_PORT_NUMBER,
-} ale_entry_filed_t;
+} ale_entry_field_t;

 #define ALE_TYPE_FREE		0
 #define ALE_TYPE_ADDRESS	1
 #define ALE_TYPE_VLAN		2


>Release-Note:

>Audit-Trail:
From: Sprow <webpages@sprow.co.uk>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: port-arm/49622: Minor enhancements to if_cpsw
Date: Wed, 11 Feb 2015 08:14:58 +0000 (GMT)

 Patch updated to apply against if_cpsw.c revision 1.7.

 if_cpsw.c
 ~~~~~~~~~
 Line 875:
 Use bit definitions from the header to build the register poke.
 Also, remove the 'BYPASS' bit (4), otherwise all the careful setup of the
 ALE is futile - the BYPASS bit is described in the AM3358 datasheet as "When
 in bypass mode, all CPGMAC_SL received packets are forwarded only to the
 host port (port 0)" so effectively it's a promiscuous enable. This was
 confirmed by injecting some packets via a crossover cable with known
 non-matching EUI48 destination and observing they arrive on the interface
 (obviously with a switch in the way these would have been masked).
 Line 1016:
 During cpsw_stop the MISC interrupts were being left enabled, looks like a
 copy & paste mistake from cpsw_init.
 Line 1205:
 Remove unnecessary calculation of cpi, since it's recalculated as the second
 thing inside the loop that processes receive buffers.
 General/others:
 Couple of typos, swap to using host/ethernet port limits from the header
 file as for loop limits, some const's, parenthesis for clarity.

 --- if_cpsw_1_7.c	2015-02-01 20:35:10 +0000
 +++ if_cpsw_new.c	2015-02-11 08:12:34 +0000
 @@ -656,7 +656,7 @@
  				dw[3] |= CPDMA_BD_SOP | CPDMA_BD_OWNER |
  				    MAX(mlen, CPSW_PAD_LEN);

 -			if (seg == dm->dm_nsegs - 1 && !pad)
 +			if ((seg == dm->dm_nsegs - 1) && !pad)
  				dw[3] |= CPDMA_BD_EOP;

  			cpsw_set_txdesc(sc, sc->sc_txnext, &bd);
 @@ -875,11 +875,11 @@
  	cpsw_write_4(sc, CPSW_SS_SOFT_RESET, 1);
  	while(cpsw_read_4(sc, CPSW_SS_SOFT_RESET) & 1);

 -	/* Clear table (30) and enable ALE(31) and set passthrough (4) */
 -	cpsw_write_4(sc, CPSW_ALE_CONTROL, (3 << 30) | 0x10);
 +	/* Clear table and enable ALE */
 +	cpsw_write_4(sc, CPSW_ALE_CONTROL, ALECTL_ENABLE_ALE | ALECTL_CLEAR_TABLE);

  	/* Reset and init Sliver port 1 and 2 */
 -	for (i = 0; i < 2; i++) {
 +	for (i = 0; i < CPSW_ETH_PORTS; i++) {
  		uint32_t macctl;

  		/* Reset */
 @@ -976,9 +976,9 @@
  	cpsw_write_4(sc, CPSW_CPDMA_CPDMA_EOI_VECTOR, CPSW_INTROFF_TX);
  	cpsw_write_4(sc, CPSW_CPDMA_CPDMA_EOI_VECTOR, CPSW_INTROFF_MISC);

 -	/* Initialze MDIO - ENABLE, PREAMBLE=0, FAULTENB, CLKDIV=0xFF */
 +	/* Initialize MDIO - ENABLE, PREAMBLE=0, FAULTENB, CLKDIV=0xFF */
  	/* TODO Calculate MDCLK=CLK/(CLKDIV+1) */
 -	cpsw_write_4(sc, MDIOCONTROL, (1<<30) | (1<<18) | 0xFF);
 +	cpsw_write_4(sc, MDIOCONTROL, MDIOCTL_ENABLE | MDIOCTL_FAULTENB |
 MDIOCTL_CLKDIV(0xff));

  	mii_mediachg(mii);

 @@ -1016,7 +1016,7 @@
  	cpsw_write_4(sc, CPSW_CPDMA_RX_INTMASK_CLEAR, 1);
  	cpsw_write_4(sc, CPSW_WR_C_TX_EN(0), 0x0);
  	cpsw_write_4(sc, CPSW_WR_C_RX_EN(0), 0x0);
 -	cpsw_write_4(sc, CPSW_WR_C_MISC_EN(0), 0x1F);
 +	cpsw_write_4(sc, CPSW_WR_C_MISC_EN(0), 0x0);

  	cpsw_write_4(sc, CPSW_CPDMA_TX_TEARDOWN, 0);
  	cpsw_write_4(sc, CPSW_CPDMA_RX_TEARDOWN, 0);
 @@ -1039,7 +1039,7 @@
  	cpsw_write_4(sc, CPSW_SS_SOFT_RESET, 1);
  	while(cpsw_read_4(sc, CPSW_SS_SOFT_RESET) & 1);

 -	for (i = 0; i < 2; i++) {
 +	for (i = 0; i < CPSW_ETH_PORTS; i++) {
  		cpsw_write_4(sc, CPSW_SL_SOFT_RESET(i), 1);
  		while(cpsw_read_4(sc, CPSW_SL_SOFT_RESET(i)) & 1);
  	}
 @@ -1205,14 +1205,13 @@
  	tx0_cp = cpsw_read_4(sc, CPSW_CPDMA_TX_CP(0));

  	if (tx0_cp == 0xfffffffc) {
 +		/* Teardown, ack it */
  		cpsw_write_4(sc, CPSW_CPDMA_TX_CP(0), 0xfffffffc);
  		cpsw_write_4(sc, CPSW_CPDMA_TX_HDP(0), 0);
  		sc->sc_txrun = false;
  		return 0;
  	}

 -	cpi = (tx0_cp - sc->sc_txdescs_pa) / sizeof(struct cpsw_cpdma_bd);
 -
  	for (;;) {
  		tx0_cp = cpsw_read_4(sc, CPSW_CPDMA_TX_CP(0));
  		cpi = (tx0_cp - sc->sc_txdescs_pa) / sizeof(struct cpsw_cpdma_bd);
 @@ -1367,7 +1366,7 @@
  }

  static void
 -cpsw_ale_entry_set(uint32_t *ale_entry, ale_entry_filed_t field, uint32_t
 val)
 +cpsw_ale_entry_set(uint32_t *ale_entry, ale_entry_field_t field, uint32_t
 val)
  {
  	/* Entry type[61:60] is addr entry(1), Mcast fwd state[63:62] is fw(3)*/
  	switch (field) {
 @@ -1424,7 +1423,7 @@
  }

  static void
 -cpsw_ale_write_entry(struct cpsw_softc *sc, uint16_t idx, uint32_t
 *ale_entry)
 +cpsw_ale_write_entry(struct cpsw_softc *sc, uint16_t idx, const uint32_t
 *ale_entry)
  {
  	cpsw_write_4(sc, CPSW_ALE_TBLW0, ale_entry[0]);
  	cpsw_write_4(sc, CPSW_ALE_TBLW1, ale_entry[1]);
 @@ -1507,7 +1506,7 @@
  	cpsw_ale_write_entry(sc, 0, ale_entry);

  	/* Set outgoing MAC Address for Ports 1 and 2. */
 -	for (i = 1; i < 3; ++i)
 +	for (i = CPSW_CPPI_PORTS; i < (CPSW_ETH_PORTS + CPSW_CPPI_PORTS); ++i)
  		cpsw_ale_set_outgoing_mac(sc, i, mac);

  	/* Keep the broadcast address at table entry 1. */

Responsible-Changed-From-To: port-arm-maintainer->skrll
Responsible-Changed-By: skrll@NetBSD.org
Responsible-Changed-When: Thu, 12 Mar 2015 20:52:59 +0000
Responsible-Changed-Why:
Take


From: "Nick Hudson" <skrll@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/49622 CVS commit: src/sys/arch/arm/omap
Date: Fri, 13 Mar 2015 08:56:35 +0000

 Module Name:	src
 Committed By:	skrll
 Date:		Fri Mar 13 08:56:35 UTC 2015

 Modified Files:
 	src/sys/arch/arm/omap: if_cpsw.c if_cpswreg.h

 Log Message:
 Stylistic and non-functional changes from

 PR/49622 (Minor enhancements to if_cpsw)


 To generate a diff of this commit:
 cvs rdiff -u -r1.9 -r1.10 src/sys/arch/arm/omap/if_cpsw.c
 cvs rdiff -u -r1.3 -r1.4 src/sys/arch/arm/omap/if_cpswreg.h

 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: skrll@NetBSD.org
State-Changed-When: Thu, 26 Mar 2015 22:03:51 +0000
State-Changed-Why:
Fixes committed. OK to close?


From: "Nick Hudson" <skrll@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/49622 CVS commit: src/sys/arch/arm/omap
Date: Thu, 26 Mar 2015 22:00:46 +0000

 Module Name:	src
 Committed By:	skrll
 Date:		Thu Mar 26 22:00:45 UTC 2015

 Modified Files:
 	src/sys/arch/arm/omap: if_cpsw.c

 Log Message:
 Fixes from PR/49622

 - Use bit definitions from the header instead of magic numbers
 - Remove the 'BYPASS' bit (4), otherwise all the careful setup
   of the ALE is futile - the BYPASS bit is described in the
   AM3358 datasheet as "When in bypass mode, all CPGMAC_SL
   received packets are forwarded only to the host port (port 0)"
   so effectively it's a promiscuous enable
 - During cpsw_stop the MISC interrupts were being left enabled
 - Remove unnecessary cpi assignment before loop.


 To generate a diff of this commit:
 cvs rdiff -u -r1.10 -r1.11 src/sys/arch/arm/omap/if_cpsw.c

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

From: Robert Sprowson <webpages@sprow.co.uk>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: port-arm/49622 (Minor enhancements to if_cpsw)
Date: Fri, 27 Mar 2015 08:55:54 +0000 (GMT)

 On 26 Mar, <skrll@NetBSD.org> wrote:
 > Fixes committed. OK to close?

 Looks good to close. Thanks for picking this PR up.

State-Changed-From-To: feedback->closed
State-Changed-By: skrll@NetBSD.org
State-Changed-When: Fri, 27 Mar 2015 09:16:49 +0000
State-Changed-Why:
Fixed


>Unformatted:

Home	PR Database Search