[U-Boot] [PATCH 0/5] HWW-1U-1A: e1000 driver cleanups and new features

[U-Boot] [PATCH 0/5] HWW-1U-1A: e1000 driver cleanups and new features

[Kyle Moffett]

This is a reposted patch series from several months that enhances the
e1000 driver in U-Boot to support the Intel 82571EB and provides
several new features for the board manufacturing process.

Based on prior review, the EEPROM programming and SPI code has been
moved into a separate file, "e1000_spi.c", and it calls a few exported
functions in the main e1000.c file to coordinate EEPROM access.

Comments, critiques, and compliments are highly appreciated.

Cheers,
Kyle Moffett

--
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/

[U-Boot] [PATCH 1/5] e1000: Clean up handling of dual-port NICs and support 82571

[Kyle Moffett]

Consolidate the test for a dual-port NIC to one location for easy
modification, then fix support for the dual-port 82571.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
Cc: Ben Warren <biggerbadderben@gmail.com>
---
 drivers/net/e1000.c |   66 +++++++++++++++++++++++++-------------------------
 drivers/net/e1000.h |    6 ----
 2 files changed, 33 insertions(+), 39 deletions(-)

diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
index 98145bc..4715050 100644
--- a/drivers/net/e1000.c
+++ b/drivers/net/e1000.c
@@ -1100,6 +1100,20 @@ e1000_swfw_sync_acquire(struct e1000_hw *hw, uint16_t mask)
 	return E1000_SUCCESS;
 }
 
+static boolean_t e1000_is_second_port(struct e1000_hw *hw)
+{
+	switch (hw->mac_type) {
+	case e1000_80003es2lan:
+	case e1000_82546:
+	case e1000_82571:
+		if (E1000_READ_REG(hw, STATUS) & E1000_STATUS_FUNC_1)
+			return TRUE;
+		/* Fallthrough */
+	default:
+		return FALSE;
+	}
+}
+
 /******************************************************************************
  * Reads the adapter's MAC address from the EEPROM and inverts the LSB for the
  * second function of dual function devices
@@ -1126,11 +1140,11 @@ e1000_read_mac_addr(struct eth_device *nic)
 		nic->enetaddr[i] = eeprom_data & 0xff;
 		nic->enetaddr[i + 1] = (eeprom_data >> 8) & 0xff;
 	}
-	if ((hw->mac_type == e1000_82546) &&
-	    (E1000_READ_REG(hw, STATUS) & E1000_STATUS_FUNC_1)) {
-		/* Invert the last bit if this is the second device */
-		nic->enetaddr[5] += 1;
-	}
+
+	/* Invert the last bit if this is the second device */
+	if (e1000_is_second_port(hw))
+		nic->enetaddr[5] ^= 1;
+
 #ifdef CONFIG_E1000_FALLBACK_MAC
 	if ( *(u32*)(nic->enetaddr) == 0 || *(u32*)(nic->enetaddr) == ~0 ) {
 		unsigned char fb_mac[NODE_ADDRESS_SIZE] = CONFIG_E1000_FALLBACK_MAC;
@@ -2536,16 +2550,13 @@ e1000_check_mng_mode(struct e1000_hw *hw)
 static int32_t
 e1000_write_kmrn_reg(struct e1000_hw *hw, uint32_t reg_addr, uint16_t data)
 {
+	uint16_t swfw = E1000_SWFW_PHY0_SM;
 	uint32_t reg_val;
-	uint16_t swfw;
 	DEBUGFUNC();
 
-	if ((hw->mac_type == e1000_80003es2lan) &&
-		(E1000_READ_REG(hw, STATUS) & E1000_STATUS_FUNC_1)) {
+	if (e1000_is_second_port(hw))
 		swfw = E1000_SWFW_PHY1_SM;
-	} else {
-		swfw = E1000_SWFW_PHY0_SM;
-	}
+
 	if (e1000_swfw_sync_acquire(hw, swfw))
 		return -E1000_ERR_SWFW_SYNC;
 
@@ -2560,16 +2571,13 @@ e1000_write_kmrn_reg(struct e1000_hw *hw, uint32_t reg_addr, uint16_t data)
 static int32_t
 e1000_read_kmrn_reg(struct e1000_hw *hw, uint32_t reg_addr, uint16_t *data)
 {
+	uint16_t swfw = E1000_SWFW_PHY0_SM;
 	uint32_t reg_val;
-	uint16_t swfw;
 	DEBUGFUNC();
 
-	if ((hw->mac_type == e1000_80003es2lan) &&
-	    (E1000_READ_REG(hw, STATUS) & E1000_STATUS_FUNC_1)) {
+	if (e1000_is_second_port(hw))
 		swfw = E1000_SWFW_PHY1_SM;
-	} else {
-		swfw = E1000_SWFW_PHY0_SM;
-	}
+
 	if (e1000_swfw_sync_acquire(hw, swfw))
 		return -E1000_ERR_SWFW_SYNC;
 
@@ -4267,11 +4275,13 @@ e1000_get_phy_cfg_done(struct e1000_hw *hw)
 	default:
 		mdelay(10);
 		break;
+
 	case e1000_80003es2lan:
 		/* Separate *_CFG_DONE_* bit for each port */
-		if (E1000_READ_REG(hw, STATUS) & E1000_STATUS_FUNC_1)
+		if (e1000_is_second_port(hw))
 			cfg_mask = E1000_EEPROM_CFG_DONE_PORT_1;
-	/* Fall Through */
+		/* Fall Through */
+
 	case e1000_82571:
 	case e1000_82572:
 		while (timeout) {
@@ -4300,10 +4310,10 @@ e1000_get_phy_cfg_done(struct e1000_hw *hw)
 int32_t
 e1000_phy_hw_reset(struct e1000_hw *hw)
 {
+	uint16_t swfw = E1000_SWFW_PHY0_SM;
 	uint32_t ctrl, ctrl_ext;
 	uint32_t led_ctrl;
 	int32_t ret_val;
-	uint16_t swfw;
 
 	DEBUGFUNC();
 
@@ -4316,16 +4326,14 @@ e1000_phy_hw_reset(struct e1000_hw *hw)
 	DEBUGOUT("Resetting Phy...\n");
 
 	if (hw->mac_type > e1000_82543) {
-		if ((hw->mac_type == e1000_80003es2lan) &&
-			(E1000_READ_REG(hw, STATUS) & E1000_STATUS_FUNC_1)) {
+		if (e1000_is_second_port(hw))
 			swfw = E1000_SWFW_PHY1_SM;
-		} else {
-			swfw = E1000_SWFW_PHY0_SM;
-		}
+
 		if (e1000_swfw_sync_acquire(hw, swfw)) {
 			DEBUGOUT("Unable to acquire swfw sync\n");
 			return -E1000_ERR_SWFW_SYNC;
 		}
+
 		/* Read the device control register and assert the E1000_CTRL_PHY_RST
 		 * bit. Then, take it out of reset.
 		 */
@@ -4787,14 +4795,6 @@ e1000_sw_init(struct eth_device *nic, int cardnum)
 		break;
 	}
 
-	/* lan a vs. lan b settings */
-	if (hw->mac_type == e1000_82546)
-		/*this also works w/ multiple 82546 cards */
-		/*but not if they're intermingled /w other e1000s */
-		hw->lan_loc = (cardnum % 2) ? e1000_lan_b : e1000_lan_a;
-	else
-		hw->lan_loc = e1000_lan_a;
-
 	/* flow control settings */
 	hw->fc_high_water = E1000_FC_HIGH_THRESH;
 	hw->fc_low_water = E1000_FC_LOW_THRESH;
diff --git a/drivers/net/e1000.h b/drivers/net/e1000.h
index 720d8c6..5a02dc3 100644
--- a/drivers/net/e1000.h
+++ b/drivers/net/e1000.h
@@ -113,11 +113,6 @@ typedef enum {
 	e1000_100_full = 3
 } e1000_speed_duplex_type;
 
-typedef enum {
-	e1000_lan_a = 0,
-	e1000_lan_b = 1
-} e1000_lan_loc;
-
 /* Flow Control Settings */
 typedef enum {
 	e1000_fc_none = 0,
@@ -1059,7 +1054,6 @@ struct e1000_hw {
 	uint32_t phy_init_script;
 	uint32_t txd_cmd;
 	e1000_media_type media_type;
-	e1000_lan_loc lan_loc;
 	e1000_fc_type fc;
 	e1000_bus_type bus_type;
 #if 0
-- 
1.7.2.5

[U-Boot] [PATCH 1/5] e1000: Clean up handling of dual-port NICs and support 82571

[Wolfgang Denk]

Dear Kyle Moffett,

In message <1318971929-1160-2-git-send-email-Kyle.D.Moffett@boeing.com> you wrote:
> Consolidate the test for a dual-port NIC to one location for easy
> modification, then fix support for the dual-port 82571.
> 
> Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
> Cc: Ben Warren <biggerbadderben@gmail.com>
> ---
>  drivers/net/e1000.c |   66 +++++++++++++++++++++++++-------------------------
>  drivers/net/e1000.h |    6 ----
>  2 files changed, 33 insertions(+), 39 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"A great many people think they are thinking when they are merely re-
arranging their prejudices."                          - William James

[U-Boot] [PATCH 2/5] e1000: Restructure and streamline PCI device probing

[Kyle Moffett]

By allocating the e1000 device structures much earlier, we can easily
generate better error messages and siginficantly clean things up.

The only user-visable change (aside from reworded error messages) is
that a detected e1000 device which fails to initialize due to software
or hardware error will still be allocated a device number.

As one example, consider a system with 2 e1000 PCI devices where the
first controller has a corrupted EEPROM.  Using the old code the
second controller would be "e1000#0", while with this change it would be
"e1000#1".

This change should hopefully make such EEPROM errors much more
straightforward to handle correctly in boot scripts and the like.

It is also necessary for a followup patch which allows SPI programming
of an e1000 controller's EEPROM even if the checksum is invalid.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
Cc: Ben Warren <biggerbadderben@gmail.com>

--

Changelog:
  v2: Clean up error messages a bit more

NOTE: This patch generates 3 false positives from checkpatch because
of the existing use of the ",##args" construct in the E1000 debug
macros to get rid of the "," when no arguments are passed.

---
 drivers/net/e1000.c |  126 +++++++++++++++++++++++++--------------------------
 drivers/net/e1000.h |   19 +++++---
 2 files changed, 74 insertions(+), 71 deletions(-)

diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
index 4715050..ee70b73 100644
--- a/drivers/net/e1000.c
+++ b/drivers/net/e1000.c
@@ -67,7 +67,7 @@ static struct e1000_rx_desc *rx_base;
 static int tx_tail;
 static int rx_tail, rx_last;
 
-static struct pci_device_id supported[] = {
+static struct pci_device_id e1000_supported[] = {
 	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82542},
 	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82543GC_FIBER},
 	{PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82543GC_COPPER},
@@ -4762,7 +4762,7 @@ e1000_set_media_type(struct e1000_hw *hw)
  **/
 
 static int
-e1000_sw_init(struct eth_device *nic, int cardnum)
+e1000_sw_init(struct eth_device *nic)
 {
 	struct e1000_hw *hw = (typeof(hw)) nic->priv;
 	int result;
@@ -4780,7 +4780,7 @@ e1000_sw_init(struct eth_device *nic, int cardnum)
 	/* identify the MAC */
 	result = e1000_set_mac_type(hw);
 	if (result) {
-		E1000_ERR("Unknown MAC Type\n");
+		E1000_ERR(hw->nic, "Unknown MAC Type\n");
 		return result;
 	}
 
@@ -5112,9 +5112,9 @@ e1000_init(struct eth_device *nic, bd_t * bis)
 	if (ret_val < 0) {
 		if ((ret_val == -E1000_ERR_NOLINK) ||
 		    (ret_val == -E1000_ERR_TIMEOUT)) {
-			E1000_ERR("Valid Link not detected\n");
+			E1000_ERR(hw->nic, "Valid Link not detected\n");
 		} else {
-			E1000_ERR("Hardware Initialization Failed\n");
+			E1000_ERR(hw->nic, "Hardware Initialization Failed\n");
 		}
 		return 0;
 	}
@@ -5163,57 +5163,59 @@ You should omit the last argument struct pci_device * for a non-PCI NIC
 int
 e1000_initialize(bd_t * bis)
 {
+	unsigned int i;
 	pci_dev_t devno;
-	int card_number = 0;
-	struct eth_device *nic = NULL;
-	struct e1000_hw *hw = NULL;
-	u32 iobase;
-	int idx = 0;
-	u32 PciCommandWord;
 
 	DEBUGFUNC();
 
-	while (1) {		/* Find PCI device(s) */
-		if ((devno = pci_find_devices(supported, idx++)) < 0) {
-			break;
-		}
-
-		pci_read_config_dword(devno, PCI_BASE_ADDRESS_0, &iobase);
-		iobase &= ~0xf;	/* Mask the bits that say "this is an io addr" */
-		DEBUGOUT("e1000#%d: iobase 0x%08x\n", card_number, iobase);
-
-		pci_write_config_dword(devno, PCI_COMMAND,
-				       PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
-		/* Check if I/O accesses and Bus Mastering are enabled. */
-		pci_read_config_dword(devno, PCI_COMMAND, &PciCommandWord);
-		if (!(PciCommandWord & PCI_COMMAND_MEMORY)) {
-			printf("Error: Can not enable MEM access.\n");
-			continue;
-		} else if (!(PciCommandWord & PCI_COMMAND_MASTER)) {
-			printf("Error: Can not enable Bus Mastering.\n");
-			continue;
-		}
-
-		nic = (struct eth_device *) malloc(sizeof (*nic));
-		if (!nic) {
-			printf("Error: e1000 - Can not alloc memory\n");
-			return 0;
-		}
+	/* Find and probe all the matching PCI devices */
+	for (i = 0; (devno = pci_find_devices(e1000_supported, i)) >= 0; i++) {
+		u32 val;
 
-		hw = (struct e1000_hw *) malloc(sizeof (*hw));
-		if (!hw) {
+		/*
+		 * These will never get freed due to errors, this allows us to
+		 * perform SPI EEPROM programming from U-boot, for example.
+		 */
+		struct eth_device *nic = malloc(sizeof(*nic));
+		struct e1000_hw *hw = malloc(sizeof(*hw));
+		if (!nic || !hw) {
+			printf("e1000#%u: Out of Memory!\n", i);
 			free(nic);
-			printf("Error: e1000 - Can not alloc memory\n");
-			return 0;
+			free(hw);
+			continue;
 		}
 
+		/* Make sure all of the fields are initially zeroed */
 		memset(nic, 0, sizeof(*nic));
 		memset(hw, 0, sizeof(*hw));
 
+		/* Assign the passed-in values */
+		hw->cardnum = i;
 		hw->pdev = devno;
+		hw->nic = nic;
 		nic->priv = hw;
 
-		sprintf(nic->name, "e1000#%d", card_number);
+		/* Generate a card name */
+		sprintf(nic->name, "e1000#%u", hw->cardnum);
+
+		/* Print a debug message with the IO base address */
+		pci_read_config_dword(devno, PCI_BASE_ADDRESS_0, &val);
+		E1000_DBG(nic, "iobase 0x%08x\n", val & 0xfffffff0);
+
+		/* Try to enable I/O accesses and bus-mastering */
+		val = PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
+		pci_write_config_dword(devno, PCI_COMMAND, val);
+
+		/* Make sure it worked */
+		pci_read_config_dword(devno, PCI_COMMAND, &val);
+		if (!(val & PCI_COMMAND_MEMORY)) {
+			E1000_ERR(nic, "Can't enable I/O memory\n");
+			continue;
+		}
+		if (!(val & PCI_COMMAND_MASTER)) {
+			E1000_ERR(nic, "Can't enable bus-mastering\n");
+			continue;
+		}
 
 		/* Are these variables needed? */
 		hw->fc = e1000_fc_default;
@@ -5221,50 +5223,46 @@ e1000_initialize(bd_t * bis)
 		hw->autoneg_failed = 0;
 		hw->autoneg = 1;
 		hw->get_link_status = TRUE;
-		hw->hw_addr =
-			pci_map_bar(devno, PCI_BASE_ADDRESS_0, PCI_REGION_MEM);
+		hw->hw_addr = pci_map_bar(devno,	PCI_BASE_ADDRESS_0,
+							PCI_REGION_MEM);
 		hw->mac_type = e1000_undefined;
 
 		/* MAC and Phy settings */
-		if (e1000_sw_init(nic, card_number) < 0) {
-			free(hw);
-			free(nic);
-			return 0;
+		if (e1000_sw_init(nic) < 0) {
+			E1000_ERR(nic, "Software init failed\n");
+			continue;
 		}
 		if (e1000_check_phy_reset_block(hw))
-			printf("phy reset block error \n");
+			E1000_ERR(nic, "PHY Reset is blocked!\n");
+
+		/* Basic init was OK, reset the hardware */
 		e1000_reset_hw(hw);
+
+		/* Validate the EEPROM and get chipset information */
 #if !(defined(CONFIG_AP1000) || defined(CONFIG_MVBC_1G))
 		if (e1000_init_eeprom_params(hw)) {
-			printf("The EEPROM Checksum Is Not Valid\n");
-			free(hw);
-			free(nic);
-			return 0;
+			E1000_ERR(nic, "EEPROM is invalid!\n");
+			continue;
 		}
 		if (e1000_validate_eeprom_checksum(nic) < 0) {
-			printf("The EEPROM Checksum Is Not Valid\n");
-			free(hw);
-			free(nic);
-			return 0;
+			E1000_ERR(nic, "EEPROM checksum is bad!\n");
+			continue;
 		}
 #endif
 		e1000_read_mac_addr(nic);
-
-		/* get the bus type information */
 		e1000_get_bus_type(hw);
 
-		printf("e1000: %02x:%02x:%02x:%02x:%02x:%02x\n",
+		printf("e1000: %02x:%02x:%02x:%02x:%02x:%02x\n       ",
 		       nic->enetaddr[0], nic->enetaddr[1], nic->enetaddr[2],
 		       nic->enetaddr[3], nic->enetaddr[4], nic->enetaddr[5]);
 
+		/* Set up the function pointers and register the device */
 		nic->init = e1000_init;
 		nic->recv = e1000_poll;
 		nic->send = e1000_transmit;
 		nic->halt = e1000_disable;
-
 		eth_register(nic);
-
-		card_number++;
 	}
-	return card_number;
+
+	return i;
 }
diff --git a/drivers/net/e1000.h b/drivers/net/e1000.h
index 5a02dc3..b4e9cf2 100644
--- a/drivers/net/e1000.h
+++ b/drivers/net/e1000.h
@@ -41,16 +41,18 @@
 #include <asm/io.h>
 #include <pci.h>
 
-#define E1000_ERR(args...) printf("e1000: " args)
+#define E1000_ERR(NIC, fmt, args...) \
+	printf("e1000: %s: ERROR: " fmt, (NIC)->name ,##args)
 
 #ifdef E1000_DEBUG
-#define E1000_DBG(args...)	printf("e1000: " args)
-#define DEBUGOUT(fmt,args...) printf(fmt ,##args)
-#define DEBUGFUNC()	   printf("%s\n", __FUNCTION__);
+#define E1000_DBG(NIC, fmt, args...) \
+	printf("e1000: %s: DEBUG: " fmt, (NIC)->name ,##args)
+#define DEBUGOUT(fmt, args...)	printf(fmt ,##args)
+#define DEBUGFUNC()		printf("%s\n", __func__);
 #else
-#define E1000_DBG(args...)
-#define DEBUGFUNC()
-#define DEBUGOUT(fmt,args...)
+#define E1000_DBG(HW, args...)	do { } while (0)
+#define DEBUGFUNC()		do { } while (0)
+#define DEBUGOUT(fmt, args...)	do { } while (0)
 #endif
 
 /* Forward declarations of structures used by the shared code */
@@ -1047,6 +1049,9 @@ typedef enum {
 
 /* Structure containing variables used by the shared code (e1000_hw.c) */
 struct e1000_hw {
+	struct eth_device *nic;
+	unsigned int cardnum;
+
 	pci_dev_t pdev;
 	uint8_t *hw_addr;
 	e1000_mac_type mac_type;
-- 
1.7.2.5

[U-Boot] [PATCH 2/5] e1000: Restructure and streamline PCI device probing

[Wolfgang Denk]

Dear Kyle Moffett,

In message <1318971929-1160-3-git-send-email-Kyle.D.Moffett@boeing.com> you wrote:
> By allocating the e1000 device structures much earlier, we can easily
> generate better error messages and siginficantly clean things up.
> 
> The only user-visable change (aside from reworded error messages) is
> that a detected e1000 device which fails to initialize due to software
> or hardware error will still be allocated a device number.
> 
> As one example, consider a system with 2 e1000 PCI devices where the
> first controller has a corrupted EEPROM.  Using the old code the
> second controller would be "e1000#0", while with this change it would be
> "e1000#1".
> 
> This change should hopefully make such EEPROM errors much more
> straightforward to handle correctly in boot scripts and the like.
> 
> It is also necessary for a followup patch which allows SPI programming
> of an e1000 controller's EEPROM even if the checksum is invalid.
> 
> Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
> Cc: Ben Warren <biggerbadderben@gmail.com>
> 
> --
> 
> Changelog:
>   v2: Clean up error messages a bit more
> 
> NOTE: This patch generates 3 false positives from checkpatch because
> of the existing use of the ",##args" construct in the E1000 debug
> macros to get rid of the "," when no arguments are passed.
> 
> ---
>  drivers/net/e1000.c |  126 +++++++++++++++++++++++++--------------------------
>  drivers/net/e1000.h |   19 +++++---
>  2 files changed, 74 insertions(+), 71 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Lots of people drink from the wrong bottle sometimes.
	-- Edith Keeler, "The City on the Edge of Forever",
	   stardate unknown

[U-Boot] [PATCH 3/5] e1000: Rewrite EEPROM checksum error to give more information

[Kyle Moffett]

As an aide to debugging, we should print out the expected value of the
EEPROM checksum in addition to just saying that it is wrong.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
Cc: Ben Warren <biggerbadderben@gmail.com>
---
 drivers/net/e1000.c |   46 ++++++++++++++++++++++++++++------------------
 1 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
index ee70b73..0fe3723 100644
--- a/drivers/net/e1000.c
+++ b/drivers/net/e1000.c
@@ -877,29 +877,41 @@ e1000_read_eeprom(struct e1000_hw *hw, uint16_t offset,
  * If the the sum of the 64 16 bit words is 0xBABA, the EEPROM's checksum is
  * valid.
  *****************************************************************************/
-static int
-e1000_validate_eeprom_checksum(struct eth_device *nic)
+static int e1000_validate_eeprom_checksum(struct e1000_hw *hw)
 {
-	struct e1000_hw *hw = nic->priv;
-	uint16_t checksum = 0;
-	uint16_t i, eeprom_data;
+	uint16_t i, checksum, checksum_reg, *buf;
 
 	DEBUGFUNC();
 
-	for (i = 0; i < (EEPROM_CHECKSUM_REG + 1); i++) {
-		if (e1000_read_eeprom(hw, i, 1,  &eeprom_data) < 0) {
-			DEBUGOUT("EEPROM Read Error\n");
-			return -E1000_ERR_EEPROM;
-		}
-		checksum += eeprom_data;
+	/* Allocate a temporary buffer */
+	buf = malloc(sizeof(buf[0]) * (EEPROM_CHECKSUM_REG + 1));
+	if (!buf) {
+		E1000_ERR(hw->nic, "Unable to allocate EEPROM buffer!\n");
+		return -E1000_ERR_EEPROM;
 	}
 
-	if (checksum == (uint16_t) EEPROM_SUM) {
-		return 0;
-	} else {
-		DEBUGOUT("EEPROM Checksum Invalid\n");
+	/* Read the EEPROM */
+	if (e1000_read_eeprom(hw, 0, EEPROM_CHECKSUM_REG + 1, buf) < 0) {
+		E1000_ERR(hw->nic, "Unable to read EEPROM!\n");
 		return -E1000_ERR_EEPROM;
 	}
+
+	/* Compute the checksum */
+	for (i = 0; i < EEPROM_CHECKSUM_REG; i++)
+		checksum += buf[i];
+	checksum = ((uint16_t)EEPROM_SUM) - checksum;
+	checksum_reg = buf[i];
+
+	/* Verify it! */
+	if (checksum == checksum_reg)
+		return 0;
+
+	/* Hrm, verification failed, print an error */
+	E1000_ERR(hw->nic, "EEPROM checksum is incorrect!\n");
+	E1000_ERR(hw->nic, "  ...register was 0x%04hx, calculated 0x%04hx\n",
+			checksum_reg, checksum);
+
+	return -E1000_ERR_EEPROM;
 }
 
 /*****************************************************************************
@@ -5244,10 +5256,8 @@ e1000_initialize(bd_t * bis)
 			E1000_ERR(nic, "EEPROM is invalid!\n");
 			continue;
 		}
-		if (e1000_validate_eeprom_checksum(nic) < 0) {
-			E1000_ERR(nic, "EEPROM checksum is bad!\n");
+		if (e1000_validate_eeprom_checksum(hw))
 			continue;
-		}
 #endif
 		e1000_read_mac_addr(nic);
 		e1000_get_bus_type(hw);
-- 
1.7.2.5

[U-Boot] [PATCH 3/5] e1000: Rewrite EEPROM checksum error to give more information

[Wolfgang Denk]

Dear Kyle Moffett,

In message <1318971929-1160-4-git-send-email-Kyle.D.Moffett@boeing.com> you wrote:
> As an aide to debugging, we should print out the expected value of the
> EEPROM checksum in addition to just saying that it is wrong.
> 
> Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
> Cc: Ben Warren <biggerbadderben@gmail.com>
> ---
>  drivers/net/e1000.c |   46 ++++++++++++++++++++++++++++------------------
>  1 files changed, 28 insertions(+), 18 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Never ascribe to malice that which can  adequately  be  explained  by
stupidity.

[U-Boot] [PATCH 4/5] e1000: Export core EEPROM access functions for SPI support

[Kyle Moffett]

A followup patch will be adding a configurable feature to enable
programming of E1000 EEPROMs from the command line or via the generic
U-Boot SPI interface.

In order for it to work it needs access to certain E1000-internal
functions, so export those in the e1000.h header file.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
Cc: Ben Warren <biggerbadderben@gmail.com>
---
 drivers/net/e1000.c |   22 +++++-----------------
 drivers/net/e1000.h |   19 +++++++++++++++++++
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
index 0fe3723..0c74685 100644
--- a/drivers/net/e1000.c
+++ b/drivers/net/e1000.c
@@ -135,13 +135,6 @@ static void e1000_set_media_type(struct e1000_hw *hw);
 
 static int32_t e1000_swfw_sync_acquire(struct e1000_hw *hw, uint16_t mask);
 static int32_t e1000_check_phy_reset_block(struct e1000_hw *hw);
-#define E1000_WRITE_REG(a, reg, value) (writel((value), ((a)->hw_addr + E1000_##reg)))
-#define E1000_READ_REG(a, reg) (readl((a)->hw_addr + E1000_##reg))
-#define E1000_WRITE_REG_ARRAY(a, reg, offset, value) (\
-			writel((value), ((a)->hw_addr + E1000_##reg + ((offset) << 2))))
-#define E1000_READ_REG_ARRAY(a, reg, offset) ( \
-	readl((a)->hw_addr + E1000_##reg + ((offset) << 2)))
-#define E1000_WRITE_FLUSH(a) {uint32_t x; x = E1000_READ_REG(a, STATUS);}
 
 #ifndef CONFIG_AP1000 /* remove for warnings */
 static int32_t e1000_read_eeprom(struct e1000_hw *hw, uint16_t offset,
@@ -153,8 +146,7 @@ static int32_t e1000_read_eeprom(struct e1000_hw *hw, uint16_t offset,
  * hw - Struct containing variables accessed by shared code
  * eecd - EECD's current value
  *****************************************************************************/
-static void
-e1000_raise_ee_clk(struct e1000_hw *hw, uint32_t * eecd)
+void e1000_raise_ee_clk(struct e1000_hw *hw, uint32_t * eecd)
 {
 	/* Raise the clock input to the EEPROM (by setting the SK bit), and then
 	 * wait 50 microseconds.
@@ -171,8 +163,7 @@ e1000_raise_ee_clk(struct e1000_hw *hw, uint32_t * eecd)
  * hw - Struct containing variables accessed by shared code
  * eecd - EECD's current value
  *****************************************************************************/
-static void
-e1000_lower_ee_clk(struct e1000_hw *hw, uint32_t * eecd)
+void e1000_lower_ee_clk(struct e1000_hw *hw, uint32_t * eecd)
 {
 	/* Lower the clock input to the EEPROM (by clearing the SK bit), and then
 	 * wait 50 microseconds.
@@ -276,8 +267,7 @@ e1000_shift_in_ee_bits(struct e1000_hw *hw, uint16_t count)
  *
  * hw - Struct containing variables accessed by shared code
  *****************************************************************************/
-static void
-e1000_standby_eeprom(struct e1000_hw *hw)
+void e1000_standby_eeprom(struct e1000_hw *hw)
 {
 	struct e1000_eeprom_info *eeprom = &hw->eeprom;
 	uint32_t eecd;
@@ -355,8 +345,7 @@ static boolean_t e1000_is_onboard_nvm_eeprom(struct e1000_hw *hw)
  * Lowers EEPROM clock. Clears input pin. Sets the chip select pin. This
  * function should be called before issuing a command to the EEPROM.
  *****************************************************************************/
-static int32_t
-e1000_acquire_eeprom(struct e1000_hw *hw)
+int32_t e1000_acquire_eeprom(struct e1000_hw *hw)
 {
 	struct e1000_eeprom_info *eeprom = &hw->eeprom;
 	uint32_t eecd, i = 0;
@@ -672,8 +661,7 @@ e1000_read_eeprom_eerd(struct e1000_hw *hw,
 	return error;
 }
 
-static void
-e1000_release_eeprom(struct e1000_hw *hw)
+void e1000_release_eeprom(struct e1000_hw *hw)
 {
 	uint32_t eecd;
 
diff --git a/drivers/net/e1000.h b/drivers/net/e1000.h
index b4e9cf2..fc5ed57 100644
--- a/drivers/net/e1000.h
+++ b/drivers/net/e1000.h
@@ -55,10 +55,29 @@
 #define DEBUGOUT(fmt, args...)	do { } while (0)
 #endif
 
+/* I/O wrapper functions */
+#define E1000_WRITE_REG(a, reg, value) \
+	(writel((value), ((a)->hw_addr + E1000_##reg)))
+#define E1000_READ_REG(a, reg) \
+	(readl((a)->hw_addr + E1000_##reg))
+#define E1000_WRITE_REG_ARRAY(a, reg, offset, value) \
+	(writel((value), ((a)->hw_addr + E1000_##reg + ((offset) << 2))))
+#define E1000_READ_REG_ARRAY(a, reg, offset) \
+	(readl((a)->hw_addr + E1000_##reg + ((offset) << 2)))
+#define E1000_WRITE_FLUSH(a) \
+	do { uint32_t x = E1000_READ_REG(a, STATUS); } while (0)
+
 /* Forward declarations of structures used by the shared code */
 struct e1000_hw;
 struct e1000_hw_stats;
 
+/* Internal E1000 helper functions */
+int32_t e1000_acquire_eeprom(struct e1000_hw *hw);
+void e1000_standby_eeprom(struct e1000_hw *hw);
+void e1000_release_eeprom(struct e1000_hw *hw);
+void e1000_raise_ee_clk(struct e1000_hw *hw, uint32_t *eecd);
+void e1000_lower_ee_clk(struct e1000_hw *hw, uint32_t *eecd);
+
 typedef enum {
 	FALSE = 0,
 	TRUE = 1
-- 
1.7.2.5

[U-Boot] [PATCH 4/5] e1000: Export core EEPROM access functions for SPI support

[Wolfgang Denk]

Dear Kyle Moffett,

In message <1318971929-1160-5-git-send-email-Kyle.D.Moffett@boeing.com> you wrote:
> A followup patch will be adding a configurable feature to enable
> programming of E1000 EEPROMs from the command line or via the generic
> U-Boot SPI interface.
> 
> In order for it to work it needs access to certain E1000-internal
> functions, so export those in the e1000.h header file.
> 
> Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
> Cc: Ben Warren <biggerbadderben@gmail.com>
> ---
>  drivers/net/e1000.c |   22 +++++-----------------
>  drivers/net/e1000.h |   19 +++++++++++++++++++
>  2 files changed, 24 insertions(+), 17 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"More software projects have gone awry for lack of calendar time than
for all other causes combined."
                         - Fred Brooks, Jr., _The Mythical Man Month_

[U-Boot] [PATCH 4/5] e1000: Export core EEPROM access functions for SPI support

[Tabi Timur-B04825]

On Tue, Oct 18, 2011 at 4:05 PM, Kyle Moffett <Kyle.D.Moffett@boeing.com> wrote:
> A followup patch will be adding a configurable feature to enable
> programming of E1000 EEPROMs from the command line or via the generic
> U-Boot SPI interface.
>
> In order for it to work it needs access to certain E1000-internal
> functions, so export those in the e1000.h header file.
>
> Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
> Cc: Ben Warren <biggerbadderben@gmail.com>

This patches causes a ton of build warnings:

/home/b04825/vslick/bin/vsbuild -signal 39610 -command make -s -j 3
O=1022 && syncp 1022/u-boot.bin /tftpboot/timur/1022/
VSLICKERRORPATH=/home/b04825/git/u-boot.jerry
make -s -j 3 O=1022
Generating /home/b04825/git/u-boot.jerry/1022/include/generated/asm-offsets.h
e1000.c: In function 'e1000_raise_ee_clk':
e1000.c:155:2: warning: unused variable 'x'
e1000.c: In function 'e1000_lower_ee_clk':
e1000.c:172:2: warning: unused variable 'x'
e1000.c: In function 'e1000_shift_out_ee_bits':
e1000.c:208:3: warning: unused variable 'x'
e1000.c: In function 'e1000_standby_eeprom':
e1000.c:279:3: warning: unused variable 'x'
e1000.c:285:3: warning: unused variable 'x'
e1000.c:291:3: warning: unused variable 'x'
e1000.c:297:3: warning: unused variable 'x'
e1000.c:303:3: warning: unused variable 'x'
e1000.c:307:3: warning: unused variable 'x'
e1000.c: In function 'e1000_release_eeprom':
e1000.c:689:3: warning: unused variable 'x'
e1000.c:695:3: warning: unused variable 'x'
e1000.c: In function 'e1000_reset_hw':
e1000.c:1401:2: warning: unused variable 'x'
e1000.c:1428:3: warning: unused variable 'x'
e1000.c: In function 'e1000_init_hw':
e1000.c:1635:3: warning: unused variable 'x'
e1000.c:1647:3: warning: unused variable 'x'
e1000.c:1661:3: warning: unused variable 'x'
e1000.c: In function 'e1000_setup_fiber_link':
e1000.c:2029:2: warning: unused variable 'x'
e1000.c: In function 'e1000_config_collision_dist':
e1000.c:3235:2: warning: unused variable 'x'
e1000.c: In function 'e1000_raise_mdi_clk':
e1000.c:3960:2: warning: unused variable 'x'
e1000.c: In function 'e1000_lower_mdi_clk':
e1000.c:3977:2: warning: unused variable 'x'
e1000.c: In function 'e1000_shift_out_mdi_bits':
e1000.c:4020:3: warning: unused variable 'x'
e1000.c: In function 'e1000_shift_in_mdi_bits':
e1000.c:4059:2: warning: unused variable 'x'
e1000.c: In function 'e1000_phy_hw_reset':
e1000.c:4341:3: warning: unused variable 'x'
e1000.c:4349:3: warning: unused variable 'x'
e1000.c:4362:3: warning: unused variable 'x'
e1000.c:4366:3: warning: unused variable 'x'
e1000.c: In function 'e1000_configure_rx':
e1000.c:4991:3: warning: unused variable 'x'
e1000.c: In function 'e1000_transmit':
e1000.c:5049:2: warning: unused variable 'x'
e1000.c: In function 'e1000_initialize':
e1000.c:869:14: warning: 'checksum' may be used uninitialized in this function

The problem is this macro:

#define E1000_WRITE_FLUSH(a) \
 ? ? ? do { uint32_t x = E1000_READ_REG(a, STATUS); } while (0)

The 'x' is never used.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 4/5] e1000: Export core EEPROM access functions for SPI support

[Mike Frysinger]

On Tuesday 01 November 2011 11:22:09 Tabi Timur-B04825 wrote:
> On Tue, Oct 18, 2011 at 4:05 PM, Kyle Moffett wrote:
> > A followup patch will be adding a configurable feature to enable
> > programming of E1000 EEPROMs from the command line or via the generic
> > U-Boot SPI interface.
> > 
> > In order for it to work it needs access to certain E1000-internal
> > functions, so export those in the e1000.h header file.
> > 
> > Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
> > Cc: Ben Warren <biggerbadderben@gmail.com>
> 
> This patches causes a ton of build warnings:

wolfgang has already posted a patch for this
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111101/4097a88e/attachment.pgp 

[U-Boot] [PATCH 5/5] e1000: Allow direct access to the E1000 SPI EEPROM device

[Kyle Moffett]

As a part of the manufacturing process for some of our custom hardware,
we are programming the EEPROMs attached to our Intel 82571EB controllers
from software using U-Boot and Linux.

This code provides several conditionally-compiled features to assist in
our manufacturing process:

  CONFIG_CMD_E1000:
    This is a basic "e1000" command which allows querying the controller
    and (if other config options are set) performing EEPROM programming.
    In particular, with CONFIG_E1000_SPI this allows you to display a
    hex-dump of the EEPROM, copy to/from main memory, and verify/update
    the software checksum.

  CONFIG_E1000_SPI_GENERIC:
    Build a generic SPI driver providing the standard U-Boot SPI driver
    interface.  This allows commands such as "sspi" to access the bus
    attached to the E1000 controller.  Additionally, some E1000 chipsets
    can support user data in a reserved space in the E1000 EEPROM which
    could be used for U-Boot environment storage.

  CONFIG_E1000_SPI:
    The core SPI access code used by the above interfaces.

For example, the following commands allow you to program the EEPROM from
a USB device (assumes CONFIG_E1000_SPI and CONFIG_CMD_E1000 are enabled):
  usb start
  fatload usb 0 $loadaddr 82571EB_No_Mgmt_Discrete-LOM.bin
  e1000 0 spi program $loadaddr 0 1024
  e1000 0 spi checksum update

Please keep in mind that the Intel-provided .eep files are organized as
16-bit words.  When converting them to binary form for programming you
must byteswap each 16-bit word so that it is in little-endian form.

This means that when reading and writing words to the SPI EEPROM, the
bit ordering for each word looks like this on the wire:

  Time >>>
------------------------------------------------------------------
  ... [7, 6, 5, 4, 3, 2, 1, 0, 15, 14, 13, 12, 11, 10, 9, 8], ...
------------------------------------------------------------------
  (MSB is 15, LSB is 0).

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
Cc: Ben Warren <biggerbadderben@gmail.com>
---
 README                  |   15 ++-
 drivers/net/Makefile    |    1 +
 drivers/net/e1000.c     |   66 ++++++-
 drivers/net/e1000.h     |   15 ++
 drivers/net/e1000_spi.c |  576 +++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 671 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/e1000_spi.c

diff --git a/README b/README
index 7e032a9..a5722b0 100644
--- a/README
+++ b/README
@@ -922,7 +922,20 @@ The following options need to be configured:
 
 - NETWORK Support (PCI):
 		CONFIG_E1000
-		Support for Intel 8254x gigabit chips.
+		Support for Intel 8254x/8257x gigabit chips.
+
+		CONFIG_E1000_SPI
+		Utility code for direct access to the SPI bus on Intel 8257x.
+		This does not do anything useful unless you set at least one
+		of CONFIG_CMD_E1000 or CONFIG_E1000_SPI_GENERIC.
+
+		CONFIG_E1000_SPI_GENERIC
+		Allow generic access to the SPI bus on the Intel 8257x, for
+		example with the "sspi" command.
+
+		CONFIG_CMD_E1000
+		Management command for E1000 devices.  When used on devices
+		with SPI support you can reprogram the EEPROM from U-Boot.
 
 		CONFIG_E1000_FALLBACK_MAC
 		default MAC for empty EEPROM after production.
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 3f4f974..f4606ef 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -38,6 +38,7 @@ COBJS-$(CONFIG_DESIGNWARE_ETH) += designware.o
 COBJS-$(CONFIG_DRIVER_DM9000) += dm9000x.o
 COBJS-$(CONFIG_DNET) += dnet.o
 COBJS-$(CONFIG_E1000) += e1000.o
+COBJS-$(CONFIG_E1000_SPI) += e1000_spi.o
 COBJS-$(CONFIG_EEPRO100) += eepro100.o
 COBJS-$(CONFIG_ENC28J60) += enc28j60.o
 COBJS-$(CONFIG_ENC28J60_LPC2292) += enc28j60_lpc2292.o
diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
index 0c74685..6f440f7 100644
--- a/drivers/net/e1000.c
+++ b/drivers/net/e1000.c
@@ -5156,6 +5156,9 @@ void e1000_get_bus_type(struct e1000_hw *hw)
 	}
 }
 
+/* A list of all registered e1000 devices */
+static LIST_HEAD(e1000_hw_list);
+
 /**************************************************************************
 PROBE - Look for an adapter, this routine's visible to the outside
 You should omit the last argument struct pci_device * for a non-PCI NIC
@@ -5235,8 +5238,9 @@ e1000_initialize(bd_t * bis)
 		if (e1000_check_phy_reset_block(hw))
 			E1000_ERR(nic, "PHY Reset is blocked!\n");
 
-		/* Basic init was OK, reset the hardware */
+		/* Basic init was OK, reset the hardware and allow SPI access */
 		e1000_reset_hw(hw);
+		list_add_tail(&hw->list_node, &e1000_hw_list);
 
 		/* Validate the EEPROM and get chipset information */
 #if !(defined(CONFIG_AP1000) || defined(CONFIG_MVBC_1G))
@@ -5264,3 +5268,63 @@ e1000_initialize(bd_t * bis)
 
 	return i;
 }
+
+struct e1000_hw *e1000_find_card(unsigned int cardnum)
+{
+	struct e1000_hw *hw;
+
+	list_for_each_entry(hw, &e1000_hw_list, list_node)
+		if (hw->cardnum == cardnum)
+			return hw;
+
+	return NULL;
+}
+
+#ifdef CONFIG_CMD_E1000
+static int do_e1000(cmd_tbl_t *cmdtp, int flag,
+		int argc, char * const argv[])
+{
+	struct e1000_hw *hw;
+
+	if (argc < 3) {
+		cmd_usage(cmdtp);
+		return 1;
+	}
+
+	/* Make sure we can find the requested e1000 card */
+	hw = e1000_find_card(simple_strtoul(argv[1], NULL, 10));
+	if (!hw) {
+		printf("e1000: ERROR: No such device: e1000#%s\n", argv[1]);
+		return 1;
+	}
+
+	if (!strcmp(argv[2], "print-mac-address")) {
+		unsigned char *mac = hw->nic->enetaddr;
+		printf("%02x:%02x:%02x:%02x:%02x:%02x\n",
+			mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
+		return 0;
+	}
+
+#ifdef CONFIG_E1000_SPI
+	/* Handle the "SPI" subcommand */
+	if (!strcmp(argv[2], "spi"))
+		return do_e1000_spi(cmdtp, hw, argc - 3, argv + 3);
+#endif
+
+	cmd_usage(cmdtp);
+	return 1;
+}
+
+U_BOOT_CMD(
+	e1000, 7, 0, do_e1000,
+	"Intel e1000 controller management",
+	/*  */"<card#> print-mac-address\n"
+#ifdef CONFIG_E1000_SPI
+	"e1000 <card#> spi show [<offset> [<length>]]\n"
+	"e1000 <card#> spi dump <addr> <offset> <length>\n"
+	"e1000 <card#> spi program <addr> <offset> <length>\n"
+	"e1000 <card#> spi checksum [update]\n"
+#endif
+	"       - Manage the Intel E1000 PCI device"
+);
+#endif /* not CONFIG_CMD_E1000 */
diff --git a/drivers/net/e1000.h b/drivers/net/e1000.h
index fc5ed57..05f2bce 100644
--- a/drivers/net/e1000.h
+++ b/drivers/net/e1000.h
@@ -35,12 +35,17 @@
 #define _E1000_HW_H_
 
 #include <common.h>
+#include <linux/list.h>
 #include <malloc.h>
 #include <net.h>
 #include <netdev.h>
 #include <asm/io.h>
 #include <pci.h>
 
+#ifdef CONFIG_E1000_SPI
+#include <spi.h>
+#endif
+
 #define E1000_ERR(NIC, fmt, args...) \
 	printf("e1000: %s: ERROR: " fmt, (NIC)->name ,##args)
 
@@ -72,12 +77,18 @@ struct e1000_hw;
 struct e1000_hw_stats;
 
 /* Internal E1000 helper functions */
+struct e1000_hw *e1000_find_card(unsigned int cardnum);
 int32_t e1000_acquire_eeprom(struct e1000_hw *hw);
 void e1000_standby_eeprom(struct e1000_hw *hw);
 void e1000_release_eeprom(struct e1000_hw *hw);
 void e1000_raise_ee_clk(struct e1000_hw *hw, uint32_t *eecd);
 void e1000_lower_ee_clk(struct e1000_hw *hw, uint32_t *eecd);
 
+#ifdef CONFIG_E1000_SPI
+int do_e1000_spi(cmd_tbl_t *cmdtp, struct e1000_hw *hw,
+		int argc, char * const argv[]);
+#endif
+
 typedef enum {
 	FALSE = 0,
 	TRUE = 1
@@ -1068,7 +1079,11 @@ typedef enum {
 
 /* Structure containing variables used by the shared code (e1000_hw.c) */
 struct e1000_hw {
+	struct list_head list_node;
 	struct eth_device *nic;
+#ifdef CONFIG_E1000_SPI
+	struct spi_slave spi;
+#endif
 	unsigned int cardnum;
 
 	pci_dev_t pdev;
diff --git a/drivers/net/e1000_spi.c b/drivers/net/e1000_spi.c
new file mode 100644
index 0000000..5491780
--- /dev/null
+++ b/drivers/net/e1000_spi.c
@@ -0,0 +1,576 @@
+#include "e1000.h"
+
+/*-----------------------------------------------------------------------
+ * SPI transfer
+ *
+ * This writes "bitlen" bits out the SPI MOSI port and simultaneously clocks
+ * "bitlen" bits in the SPI MISO port.  That's just the way SPI works.
+ *
+ * The source of the outgoing bits is the "dout" parameter and the
+ * destination of the input bits is the "din" parameter.  Note that "dout"
+ * and "din" can point to the same memory location, in which case the
+ * input data overwrites the output data (since both are buffered by
+ * temporary variables, this is OK).
+ *
+ * This may be interrupted with Ctrl-C if "intr" is true, otherwise it will
+ * never return an error.
+ */
+static int e1000_spi_xfer(struct e1000_hw *hw, unsigned int bitlen,
+		const void *dout_mem, void *din_mem, boolean_t intr)
+{
+	const uint8_t *dout = dout_mem;
+	uint8_t *din = din_mem;
+
+	uint8_t mask = 0;
+	uint32_t eecd;
+	unsigned long i;
+
+	/* Pre-read the control register */
+	eecd = E1000_READ_REG(hw, EECD);
+
+	/* Iterate over each bit */
+	for (i = 0, mask = 0x80; i < bitlen; i++, mask = (mask >> 1)?:0x80) {
+		/* Check for interrupt */
+		if (intr && ctrlc())
+			return -1;
+
+		/* Determine the output bit */
+		if (dout && dout[i >> 3] & mask)
+			eecd |=  E1000_EECD_DI;
+		else
+			eecd &= ~E1000_EECD_DI;
+
+		/* Write the output bit and wait 50us */
+		E1000_WRITE_REG(hw, EECD, eecd);
+		E1000_WRITE_FLUSH(hw);
+		udelay(50);
+
+		/* Poke the clock (waits 50us) */
+		e1000_raise_ee_clk(hw, &eecd);
+
+		/* Now read the input bit */
+		eecd = E1000_READ_REG(hw, EECD);
+		if (din) {
+			if (eecd & E1000_EECD_DO)
+				din[i >> 3] |=  mask;
+			else
+				din[i >> 3] &= ~mask;
+		}
+
+		/* Poke the clock again (waits 50us) */
+		e1000_lower_ee_clk(hw, &eecd);
+	}
+
+	/* Now clear any remaining bits of the input */
+	if (din && (i & 7))
+		din[i >> 3] &= ~((mask << 1) - 1);
+
+	return 0;
+}
+
+#ifdef CONFIG_E1000_SPI_GENERIC
+static inline struct e1000_hw *e1000_hw_from_spi(struct spi_slave *spi)
+{
+	return container_of(spi, struct e1000_hw, spi);
+}
+
+/* Not sure why all of these are necessary */
+void spi_init_r(void) { /* Nothing to do */ }
+void spi_init_f(void) { /* Nothing to do */ }
+void spi_init(void)   { /* Nothing to do */ }
+
+struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
+		unsigned int max_hz, unsigned int mode)
+{
+	/* Find the right PCI device */
+	struct e1000_hw *hw = e1000_find_card(bus);
+	if (!hw) {
+		printf("ERROR: No such e1000 device: e1000#%u\n", bus);
+		return NULL;
+	}
+
+	/* Make sure it has an SPI chip */
+	if (hw->eeprom.type != e1000_eeprom_spi) {
+		E1000_ERR(hw->nic, "No attached SPI EEPROM found!\n");
+		return NULL;
+	}
+
+	/* Argument sanity checks */
+	if (cs != 0) {
+		E1000_ERR(hw->nic, "No such SPI chip: %u\n", cs);
+		return NULL;
+	}
+	if (mode != SPI_MODE_0) {
+		E1000_ERR(hw->nic, "Only SPI MODE-0 is supported!\n");
+		return NULL;
+	}
+
+	/* TODO: Use max_hz somehow */
+	E1000_DBG(hw->nic, "EEPROM SPI access requested\n");
+	return &hw->spi;
+}
+
+void spi_free_slave(struct spi_slave *spi)
+{
+	struct e1000_hw *hw = e1000_hw_from_spi(spi);
+	E1000_DBG(hw->nic, "EEPROM SPI access released\n");
+}
+
+int spi_claim_bus(struct spi_slave *spi)
+{
+	struct e1000_hw *hw = e1000_hw_from_spi(spi);
+
+	if (e1000_acquire_eeprom(hw)) {
+		E1000_ERR(hw->nic, "EEPROM SPI cannot be acquired!\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+void spi_release_bus(struct spi_slave *spi)
+{
+	struct e1000_hw *hw = e1000_hw_from_spi(spi);
+	e1000_release_eeprom(hw);
+}
+
+/* Skinny wrapper around e1000_spi_xfer */
+int spi_xfer(struct spi_slave *spi, unsigned int bitlen,
+		const void *dout_mem, void *din_mem, unsigned long flags)
+{
+	struct e1000_hw *hw = e1000_hw_from_spi(spi);
+	int ret;
+
+	if (flags & SPI_XFER_BEGIN)
+		e1000_standby_eeprom(hw);
+
+	ret = e1000_spi_xfer(hw, bitlen, dout_mem, din_mem, TRUE);
+
+	if (flags & SPI_XFER_END)
+		e1000_standby_eeprom(hw);
+
+	return ret;
+}
+
+#endif /* not CONFIG_E1000_SPI_GENERIC */
+
+#ifdef CONFIG_CMD_E1000
+
+/* The EEPROM opcodes */
+#define SPI_EEPROM_ENABLE_WR	0x06
+#define SPI_EEPROM_DISABLE_WR	0x04
+#define SPI_EEPROM_WRITE_STATUS	0x01
+#define SPI_EEPROM_READ_STATUS	0x05
+#define SPI_EEPROM_WRITE_PAGE	0x02
+#define SPI_EEPROM_READ_PAGE	0x03
+
+/* The EEPROM status bits */
+#define SPI_EEPROM_STATUS_BUSY	0x01
+#define SPI_EEPROM_STATUS_WREN	0x02
+
+static int e1000_spi_eeprom_enable_wr(struct e1000_hw *hw, boolean_t intr)
+{
+	u8 op[] = { SPI_EEPROM_ENABLE_WR };
+	e1000_standby_eeprom(hw);
+	return e1000_spi_xfer(hw, 8*sizeof(op), op, NULL, intr);
+}
+
+/*
+ * These have been tested to perform correctly, but they are not used by any
+ * of the EEPROM commands at this time.
+ */
+#if 0
+static int e1000_spi_eeprom_disable_wr(struct e1000_hw *hw, boolean_t intr)
+{
+	u8 op[] = { SPI_EEPROM_DISABLE_WR };
+	e1000_standby_eeprom(hw);
+	return e1000_spi_xfer(hw, 8*sizeof(op), op, NULL, intr);
+}
+
+static int e1000_spi_eeprom_write_status(struct e1000_hw *hw,
+		u8 status, boolean_t intr)
+{
+	u8 op[] = { SPI_EEPROM_WRITE_STATUS, status };
+	e1000_standby_eeprom(hw);
+	return e1000_spi_xfer(hw, 8*sizeof(op), op, NULL, intr);
+}
+#endif
+
+static int e1000_spi_eeprom_read_status(struct e1000_hw *hw, boolean_t intr)
+{
+	u8 op[] = { SPI_EEPROM_READ_STATUS, 0 };
+	e1000_standby_eeprom(hw);
+	if (e1000_spi_xfer(hw, 8*sizeof(op), op, op, intr))
+		return -1;
+	return op[1];
+}
+
+static int e1000_spi_eeprom_write_page(struct e1000_hw *hw,
+		const void *data, u16 off, u16 len, boolean_t intr)
+{
+	u8 op[] = {
+		SPI_EEPROM_WRITE_PAGE,
+		(off >> (hw->eeprom.address_bits - 8)) & 0xff, off & 0xff
+	};
+
+	e1000_standby_eeprom(hw);
+
+	if (e1000_spi_xfer(hw, 8 + hw->eeprom.address_bits, op, NULL, intr))
+		return -1;
+	if (e1000_spi_xfer(hw, len << 3, data, NULL, intr))
+		return -1;
+
+	return 0;
+}
+
+static int e1000_spi_eeprom_read_page(struct e1000_hw *hw,
+		void *data, u16 off, u16 len, boolean_t intr)
+{
+	u8 op[] = {
+		SPI_EEPROM_READ_PAGE,
+		(off >> (hw->eeprom.address_bits - 8)) & 0xff, off & 0xff
+	};
+
+	e1000_standby_eeprom(hw);
+
+	if (e1000_spi_xfer(hw, 8 + hw->eeprom.address_bits, op, NULL, intr))
+		return -1;
+	if (e1000_spi_xfer(hw, len << 3, NULL, data, intr))
+		return -1;
+
+	return 0;
+}
+
+static int e1000_spi_eeprom_poll_ready(struct e1000_hw *hw, boolean_t intr)
+{
+	int status;
+	while ((status = e1000_spi_eeprom_read_status(hw, intr)) >= 0) {
+		if (!(status & SPI_EEPROM_STATUS_BUSY))
+			return 0;
+	}
+	return -1;
+}
+
+static int e1000_spi_eeprom_dump(struct e1000_hw *hw,
+		void *data, u16 off, unsigned int len, boolean_t intr)
+{
+	/* Interruptibly wait for the EEPROM to be ready */
+	if (e1000_spi_eeprom_poll_ready(hw, intr))
+		return -1;
+
+	/* Dump each page in sequence */
+	while (len) {
+		/* Calculate the data bytes on this page */
+		u16 pg_off = off & (hw->eeprom.page_size - 1);
+		u16 pg_len = hw->eeprom.page_size - pg_off;
+		if (pg_len > len)
+			pg_len = len;
+
+		/* Now dump the page */
+		if (e1000_spi_eeprom_read_page(hw, data, off, pg_len, intr))
+			return -1;
+
+		/* Otherwise go on to the next page */
+		len  -= pg_len;
+		off  += pg_len;
+		data += pg_len;
+	}
+
+	/* We're done! */
+	return 0;
+}
+
+static int e1000_spi_eeprom_program(struct e1000_hw *hw,
+		const void *data, u16 off, u16 len, boolean_t intr)
+{
+	/* Program each page in sequence */
+	while (len) {
+		/* Calculate the data bytes on this page */
+		u16 pg_off = off & (hw->eeprom.page_size - 1);
+		u16 pg_len = hw->eeprom.page_size - pg_off;
+		if (pg_len > len)
+			pg_len = len;
+
+		/* Interruptibly wait for the EEPROM to be ready */
+		if (e1000_spi_eeprom_poll_ready(hw, intr))
+			return -1;
+
+		/* Enable write access */
+		if (e1000_spi_eeprom_enable_wr(hw, intr))
+			return -1;
+
+		/* Now program the page */
+		if (e1000_spi_eeprom_write_page(hw, data, off, pg_len, intr))
+			return -1;
+
+		/* Otherwise go on to the next page */
+		len  -= pg_len;
+		off  += pg_len;
+		data += pg_len;
+	}
+
+	/* Wait for the last write to complete */
+	if (e1000_spi_eeprom_poll_ready(hw, intr))
+		return -1;
+
+	/* We're done! */
+	return 0;
+}
+
+static int do_e1000_spi_show(cmd_tbl_t *cmdtp, struct e1000_hw *hw,
+		int argc, char * const argv[])
+{
+	unsigned int length = 0;
+	u16 i, offset = 0;
+	u8 *buffer;
+	int err;
+
+	if (argc > 2) {
+		cmd_usage(cmdtp);
+		return 1;
+	}
+
+	/* Parse the offset and length */
+	if (argc >= 1)
+		offset = simple_strtoul(argv[0], NULL, 0);
+	if (argc == 2)
+		length = simple_strtoul(argv[1], NULL, 0);
+	else if (offset < (hw->eeprom.word_size << 1))
+		length = (hw->eeprom.word_size << 1) - offset;
+
+	/* Extra sanity checks */
+	if (!length) {
+		E1000_ERR(hw->nic, "Requested zero-sized dump!\n");
+		return 1;
+	}
+	if ((0x10000 < length) || (0x10000 - length < offset)) {
+		E1000_ERR(hw->nic, "Can't dump past 0xFFFF!\n");
+		return 1;
+	}
+
+	/* Allocate a buffer to hold stuff */
+	buffer = malloc(length);
+	if (!buffer) {
+		E1000_ERR(hw->nic, "Out of Memory!\n");
+		return 1;
+	}
+
+	/* Acquire the EEPROM and perform the dump */
+	if (e1000_acquire_eeprom(hw)) {
+		E1000_ERR(hw->nic, "EEPROM SPI cannot be acquired!\n");
+		free(buffer);
+		return 1;
+	}
+	err = e1000_spi_eeprom_dump(hw, buffer, offset, length, TRUE);
+	e1000_release_eeprom(hw);
+	if (err) {
+		E1000_ERR(hw->nic, "Interrupted!\n");
+		free(buffer);
+		return 1;
+	}
+
+	/* Now hexdump the result */
+	printf("%s: ===== Intel e1000 EEPROM (0x%04hX - 0x%04hX) =====",
+			hw->nic->name, offset, offset + length - 1);
+	for (i = 0; i < length; i++) {
+		if ((i & 0xF) == 0)
+			printf("\n%s: %04hX: ", hw->nic->name, offset + i);
+		else if ((i & 0xF) == 0x8)
+			printf(" ");
+		printf(" %02hx", buffer[i]);
+	}
+	printf("\n");
+
+	/* Success! */
+	free(buffer);
+	return 0;
+}
+
+static int do_e1000_spi_dump(cmd_tbl_t *cmdtp, struct e1000_hw *hw,
+		int argc, char * const argv[])
+{
+	unsigned int length;
+	u16 offset;
+	void *dest;
+
+	if (argc != 3) {
+		cmd_usage(cmdtp);
+		return 1;
+	}
+
+	/* Parse the arguments */
+	dest = (void *)simple_strtoul(argv[0], NULL, 16);
+	offset = simple_strtoul(argv[1], NULL, 0);
+	length = simple_strtoul(argv[2], NULL, 0);
+
+	/* Extra sanity checks */
+	if (!length) {
+		E1000_ERR(hw->nic, "Requested zero-sized dump!\n");
+		return 1;
+	}
+	if ((0x10000 < length) || (0x10000 - length < offset)) {
+		E1000_ERR(hw->nic, "Can't dump past 0xFFFF!\n");
+		return 1;
+	}
+
+	/* Acquire the EEPROM */
+	if (e1000_acquire_eeprom(hw)) {
+		E1000_ERR(hw->nic, "EEPROM SPI cannot be acquired!\n");
+		return 1;
+	}
+
+	/* Perform the programming operation */
+	if (e1000_spi_eeprom_dump(hw, dest, offset, length, TRUE) < 0) {
+		E1000_ERR(hw->nic, "Interrupted!\n");
+		e1000_release_eeprom(hw);
+		return 1;
+	}
+
+	e1000_release_eeprom(hw);
+	printf("%s: ===== EEPROM DUMP COMPLETE =====\n", hw->nic->name);
+	return 0;
+}
+
+static int do_e1000_spi_program(cmd_tbl_t *cmdtp, struct e1000_hw *hw,
+		int argc, char * const argv[])
+{
+	unsigned int length;
+	const void *source;
+	u16 offset;
+
+	if (argc != 3) {
+		cmd_usage(cmdtp);
+		return 1;
+	}
+
+	/* Parse the arguments */
+	source = (const void *)simple_strtoul(argv[0], NULL, 16);
+	offset = simple_strtoul(argv[1], NULL, 0);
+	length = simple_strtoul(argv[2], NULL, 0);
+
+	/* Acquire the EEPROM */
+	if (e1000_acquire_eeprom(hw)) {
+		E1000_ERR(hw->nic, "EEPROM SPI cannot be acquired!\n");
+		return 1;
+	}
+
+	/* Perform the programming operation */
+	if (e1000_spi_eeprom_program(hw, source, offset, length, TRUE) < 0) {
+		E1000_ERR(hw->nic, "Interrupted!\n");
+		e1000_release_eeprom(hw);
+		return 1;
+	}
+
+	e1000_release_eeprom(hw);
+	printf("%s: ===== EEPROM PROGRAMMED =====\n", hw->nic->name);
+	return 0;
+}
+
+static int do_e1000_spi_checksum(cmd_tbl_t *cmdtp, struct e1000_hw *hw,
+		int argc, char * const argv[])
+{
+	uint16_t i, length, checksum, checksum_reg;
+	uint16_t *buffer;
+	boolean_t upd;
+
+	if (argc == 0)
+		upd = 0;
+	else if ((argc == 1) && !strcmp(argv[0], "update"))
+		upd = 1;
+	else {
+		cmd_usage(cmdtp);
+		return 1;
+	}
+
+	/* Allocate a temporary buffer */
+	length = sizeof(uint16_t) * (EEPROM_CHECKSUM_REG + 1);
+	buffer = malloc(length);
+	if (!buffer) {
+		E1000_ERR(hw->nic, "Unable to allocate EEPROM buffer!\n");
+		return 1;
+	}
+
+	/* Acquire the EEPROM */
+	if (e1000_acquire_eeprom(hw)) {
+		E1000_ERR(hw->nic, "EEPROM SPI cannot be acquired!\n");
+		return 1;
+	}
+
+	/* Read the EEPROM */
+	if (e1000_spi_eeprom_dump(hw, buffer, 0, length, TRUE) < 0) {
+		E1000_ERR(hw->nic, "Interrupted!\n");
+		e1000_release_eeprom(hw);
+		return 1;
+	}
+
+	/* Compute the checksum and read the expected value */
+	for (i = 0; i < EEPROM_CHECKSUM_REG; i++)
+		checksum += le16_to_cpu(buffer[i]);
+	checksum = ((uint16_t)EEPROM_SUM) - checksum;
+	checksum_reg = le16_to_cpu(buffer[i]);
+
+	/* Verify it! */
+	if (checksum_reg == checksum) {
+		printf("%s: INFO: EEPROM checksum is correct! (0x%04hx)\n",
+				hw->nic->name, checksum);
+		e1000_release_eeprom(hw);
+		return 0;
+	}
+
+	/* Hrm, verification failed, print an error */
+	E1000_ERR(hw->nic, "EEPROM checksum is incorrect!\n");
+	E1000_ERR(hw->nic, "  ...register was 0x%04hx, calculated 0x%04hx\n",
+			checksum_reg, checksum);
+
+	/* If they didn't ask us to update it, just return an error */
+	if (!upd) {
+		e1000_release_eeprom(hw);
+		return 1;
+	}
+
+	/* Ok, correct it! */
+	printf("%s: Reprogramming the EEPROM checksum...\n", hw->nic->name);
+	buffer[i] = cpu_to_le16(checksum);
+	if (e1000_spi_eeprom_program(hw, &buffer[i], i * sizeof(uint16_t),
+			sizeof(uint16_t), TRUE)) {
+		E1000_ERR(hw->nic, "Interrupted!\n");
+		e1000_release_eeprom(hw);
+		return 1;
+	}
+
+	e1000_release_eeprom(hw);
+	return 0;
+}
+
+int do_e1000_spi(cmd_tbl_t *cmdtp, struct e1000_hw *hw,
+		int argc, char * const argv[])
+{
+	if (argc < 1) {
+		cmd_usage(cmdtp);
+		return 1;
+	}
+
+	/* Make sure it has an SPI chip */
+	if (hw->eeprom.type != e1000_eeprom_spi) {
+		E1000_ERR(hw->nic, "No attached SPI EEPROM found!\n");
+		return 1;
+	}
+
+	/* Check the eeprom sub-sub-command arguments */
+	if (!strcmp(argv[0], "show"))
+		return do_e1000_spi_show(cmdtp, hw, argc - 1, argv + 1);
+
+	if (!strcmp(argv[0], "dump"))
+		return do_e1000_spi_dump(cmdtp, hw, argc - 1, argv + 1);
+
+	if (!strcmp(argv[0], "program"))
+		return do_e1000_spi_program(cmdtp, hw, argc - 1, argv + 1);
+
+	if (!strcmp(argv[0], "checksum"))
+		return do_e1000_spi_checksum(cmdtp, hw, argc - 1, argv + 1);
+
+	cmd_usage(cmdtp);
+	return 1;
+}
+
+#endif /* not CONFIG_CMD_E1000 */
-- 
1.7.2.5

[U-Boot] [PATCH 5/5] e1000: Allow direct access to the E1000 SPI EEPROM device

[Wolfgang Denk]

Dear Kyle Moffett,

In message <1318971929-1160-6-git-send-email-Kyle.D.Moffett@boeing.com> you wrote:
> As a part of the manufacturing process for some of our custom hardware,
> we are programming the EEPROMs attached to our Intel 82571EB controllers
> from software using U-Boot and Linux.
> 
> This code provides several conditionally-compiled features to assist in
> our manufacturing process:
> 
>   CONFIG_CMD_E1000:
>     This is a basic "e1000" command which allows querying the controller
>     and (if other config options are set) performing EEPROM programming.
>     In particular, with CONFIG_E1000_SPI this allows you to display a
>     hex-dump of the EEPROM, copy to/from main memory, and verify/update
>     the software checksum.
> 
>   CONFIG_E1000_SPI_GENERIC:
>     Build a generic SPI driver providing the standard U-Boot SPI driver
>     interface.  This allows commands such as "sspi" to access the bus
>     attached to the E1000 controller.  Additionally, some E1000 chipsets
>     can support user data in a reserved space in the E1000 EEPROM which
>     could be used for U-Boot environment storage.
> 
>   CONFIG_E1000_SPI:
>     The core SPI access code used by the above interfaces.
> 
> For example, the following commands allow you to program the EEPROM from
> a USB device (assumes CONFIG_E1000_SPI and CONFIG_CMD_E1000 are enabled):
>   usb start
>   fatload usb 0 $loadaddr 82571EB_No_Mgmt_Discrete-LOM.bin
>   e1000 0 spi program $loadaddr 0 1024
>   e1000 0 spi checksum update
> 
> Please keep in mind that the Intel-provided .eep files are organized as
> 16-bit words.  When converting them to binary form for programming you
> must byteswap each 16-bit word so that it is in little-endian form.
> 
> This means that when reading and writing words to the SPI EEPROM, the
> bit ordering for each word looks like this on the wire:
> 
>   Time >>>
> ------------------------------------------------------------------
>   ... [7, 6, 5, 4, 3, 2, 1, 0, 15, 14, 13, 12, 11, 10, 9, 8], ...
> ------------------------------------------------------------------
>   (MSB is 15, LSB is 0).
> 
> Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
> Cc: Ben Warren <biggerbadderben@gmail.com>
> ---
>  README                  |   15 ++-
>  drivers/net/Makefile    |    1 +
>  drivers/net/e1000.c     |   66 ++++++-
>  drivers/net/e1000.h     |   15 ++
>  drivers/net/e1000_spi.c |  576 +++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 671 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/net/e1000_spi.c

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"It is easier to port a shell than a shell script."      - Larry Wall

[U-Boot] [PATCH 0/5] HWW-1U-1A: e1000 driver cleanups and new features

[Wolfgang Denk]

Dear Kyle Moffett,

In message <1318971929-1160-1-git-send-email-Kyle.D.Moffett@boeing.com> you wrote:
> This is a reposted patch series from several months that enhances the
> e1000 driver in U-Boot to support the Intel 82571EB and provides
> several new features for the board manufacturing process.
> 
> Based on prior review, the EEPROM programming and SPI code has been
> moved into a separate file, "e1000_spi.c", and it calls a few exported
> functions in the main e1000.c file to coordinate EEPROM access.
> 
> Comments, critiques, and compliments are highly appreciated.

Critique:  I wonder how you tested your patches, if at all :-(

They generate a ton of warnings:

e1000.c: In function 'e1000_raise_ee_clk':
e1000.c:155: warning: unused variable 'x'
e1000.c: In function 'e1000_lower_ee_clk':
e1000.c:172: warning: unused variable 'x'
e1000.c: In function 'e1000_shift_out_ee_bits':
e1000.c:208: warning: unused variable 'x'
e1000.c: In function 'e1000_standby_eeprom':
e1000.c:279: warning: unused variable 'x'
e1000.c:285: warning: unused variable 'x'
e1000.c:291: warning: unused variable 'x'
e1000.c:297: warning: unused variable 'x'
e1000.c:303: warning: unused variable 'x'
e1000.c:307: warning: unused variable 'x'
e1000.c: In function 'e1000_release_eeprom':
e1000.c:689: warning: unused variable 'x'
e1000.c:695: warning: unused variable 'x'
e1000.c: In function 'e1000_reset_hw':
e1000.c:1401: warning: unused variable 'x'
e1000.c:1428: warning: unused variable 'x'
e1000.c: In function 'e1000_init_hw':
e1000.c:1635: warning: unused variable 'x'
e1000.c:1647: warning: unused variable 'x'
e1000.c:1661: warning: unused variable 'x'
e1000.c: In function 'e1000_setup_fiber_link':
e1000.c:2029: warning: unused variable 'x'
e1000.c: In function 'e1000_config_collision_dist':
e1000.c:3235: warning: unused variable 'x'
e1000.c: In function 'e1000_raise_mdi_clk':
e1000.c:3960: warning: unused variable 'x'
e1000.c: In function 'e1000_lower_mdi_clk':
e1000.c:3977: warning: unused variable 'x'
e1000.c: In function 'e1000_shift_out_mdi_bits':
e1000.c:4020: warning: unused variable 'x'
e1000.c: In function 'e1000_shift_in_mdi_bits':
e1000.c:4059: warning: unused variable 'x'
e1000.c: In function 'e1000_phy_hw_reset':
e1000.c:4341: warning: unused variable 'x'
e1000.c:4349: warning: unused variable 'x'
e1000.c:4362: warning: unused variable 'x'
e1000.c:4366: warning: unused variable 'x'
e1000.c: In function 'e1000_configure_rx':
e1000.c:4991: warning: unused variable 'x'
e1000.c: In function 'e1000_transmit':
e1000.c:5049: warning: unused variable 'x'
e1000.c: In function 'e1000_initialize':
e1000.c:869: warning: 'checksum' may be used uninitialized in this function
e1000.c:869: note: 'checksum' was declared here

Please fix!!!

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I am a computer. I am dumber than any human and smarter than any  ad-
ministrator.

[U-Boot] [PATCH] e1000: fix bugs from recent commits

[Wolfgang Denk]

Commit 114d7fc0 "e1000: Rewrite EEPROM checksum error to give more
information" failed to initialize the checksum variable which should
result in random results. Fix that.
[I wonder if that code has _ever_ been tested!!]

Commit 2326a94d caused a ton of "unused variable 'x'" warnings.
Fix these.  While we are at it, remove some bogus parens.

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---

Kyle,

I wonder if you have ever actually build and run this code???
With the "checksum" variable being random (due to not being
initialized) you should have seen serious checksum problems.
How did this escape your testing?

And all these build warnings - have you ever actully compiled that
code?  What's going on here???  - wd

Marek: Could you please be so kind and have a look at the debug code?
I think this needs a major cleanup, too.  Thanks in advance.  - wd

 drivers/net/e1000.c |    1 +
 drivers/net/e1000.h |   10 +++++-----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
index c86bf0a..6eab7b2 100644
--- a/drivers/net/e1000.c
+++ b/drivers/net/e1000.c
@@ -884,6 +884,7 @@ static int e1000_validate_eeprom_checksum(struct e1000_hw *hw)
 	}
 
 	/* Compute the checksum */
+	checksum = 0;
 	for (i = 0; i < EEPROM_CHECKSUM_REG; i++)
 		checksum += buf[i];
 	checksum = ((uint16_t)EEPROM_SUM) - checksum;
diff --git a/drivers/net/e1000.h b/drivers/net/e1000.h
index 05f2bce..db72604 100644
--- a/drivers/net/e1000.h
+++ b/drivers/net/e1000.h
@@ -62,15 +62,15 @@
 
 /* I/O wrapper functions */
 #define E1000_WRITE_REG(a, reg, value) \
-	(writel((value), ((a)->hw_addr + E1000_##reg)))
+	writel((value), ((a)->hw_addr + E1000_##reg))
 #define E1000_READ_REG(a, reg) \
-	(readl((a)->hw_addr + E1000_##reg))
+	readl((a)->hw_addr + E1000_##reg)
 #define E1000_WRITE_REG_ARRAY(a, reg, offset, value) \
-	(writel((value), ((a)->hw_addr + E1000_##reg + ((offset) << 2))))
+	writel((value), ((a)->hw_addr + E1000_##reg + ((offset) << 2)))
 #define E1000_READ_REG_ARRAY(a, reg, offset) \
-	(readl((a)->hw_addr + E1000_##reg + ((offset) << 2)))
+	readl((a)->hw_addr + E1000_##reg + ((offset) << 2))
 #define E1000_WRITE_FLUSH(a) \
-	do { uint32_t x = E1000_READ_REG(a, STATUS); } while (0)
+	E1000_READ_REG(a, STATUS)
 
 /* Forward declarations of structures used by the shared code */
 struct e1000_hw;
-- 
1.7.6.4

[U-Boot] [PATCH] e1000: fix bugs from recent commits

[Mike Frysinger]

On Fri, Oct 28, 2011 at 07:49, Wolfgang Denk wrote:
> --- a/drivers/net/e1000.h
> +++ b/drivers/net/e1000.h
>
> ?#define E1000_WRITE_FLUSH(a) \
> - ? ? ? do { uint32_t x = E1000_READ_REG(a, STATUS); } while (0)
> + ? ? ? E1000_READ_REG(a, STATUS)

i think we want the do{}while as this is a write command and we don't
want people accidentally trying to check the return value
-mike

[U-Boot] [PATCH] e1000: fix bugs from recent commits

[Wolfgang Denk]

Dear Mike Frysinger,

In message <CAJaTeTpeNDdV4g+n4cE0GVe8uBxgjUdRhrF2t=q-pOb5pAmfzg@mail.gmail.com> you wrote:
> On Fri, Oct 28, 2011 at 07:49, Wolfgang Denk wrote:
> > --- a/drivers/net/e1000.h
> > +++ b/drivers/net/e1000.h
> >
> >  #define E1000_WRITE_FLUSH(a) \
> > -       do { uint32_t x = E1000_READ_REG(a, STATUS); } while (0)
> > +       E1000_READ_REG(a, STATUS)
>
> i think we want the do{}while as this is a write command and we don't
> want people accidentally trying to check the return value

I don't see that this is a write command.  I'm seeing only reading of
the status register here.

And I don't understand the "accidentally trying to check the return
value" argument either. Why would one do that - and if one does
(probably after checking the implementation), what would be wrong
about it?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
My play was a complete success.  The audience was a failure.

[U-Boot] [PATCH] e1000: fix bugs from recent commits

[Mike Frysinger]

On Fri, Oct 28, 2011 at 22:19, Wolfgang Denk wrote:
> Mike Frysinger wrote:
>> On Fri, Oct 28, 2011 at 07:49, Wolfgang Denk wrote:
>> > --- a/drivers/net/e1000.h
>> > +++ b/drivers/net/e1000.h
>> >
>> > ?#define E1000_WRITE_FLUSH(a) \
>> > - ? ? ? do { uint32_t x = E1000_READ_REG(a, STATUS); } while (0)
>> > + ? ? ? E1000_READ_REG(a, STATUS)
>>
>> i think we want the do{}while as this is a write command and we don't
>> want people accidentally trying to check the return value
>
> I don't see that this is a write command. ?I'm seeing only reading of
> the status register here.

yes, the actual implementation is a read.  however, the API implied by
the naming (xxx_WRITE_xxx) is a write command.  sometimes that means a
read of the status register (if they're R1C bits), or a write (if
they're W1C bits), but the forward facing API is still a write.

> And I don't understand the "accidentally trying to check the return
> value" argument either. Why would one do that - and if one does
> (probably after checking the implementation), what would be wrong
> about it?

imo, using do{}while is simply "good programming".  it falls under the
same argument of why we respect type warnings with gcc.  this is
supposed to be a write operation with void return, so we should make
sure the implementation enforces that to keep the user from
accidentally trying to do things otherwise.
-mike

[U-Boot] [PATCH] e1000: fix bugs from recent commits

[Wolfgang Denk]

Dear Mike Frysinger,

In message <CAJaTeTrCOfVOMM3A00Yg73=h+CRmhcy6PYrZoMKpGCHHXD2XHQ@mail.gmail.com> you wrote:
>
> yes, the actual implementation is a read.  however, the API implied by
> the naming (xxx_WRITE_xxx) is a write command.  sometimes that means a
> read of the status register (if they're R1C bits), or a write (if
> they're W1C bits), but the forward facing API is still a write.

OK.  I'm not in the mood of fighting over this.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"A dirty mind is a joy forever."                       - Randy Kunkee

[U-Boot] [PATCH] e1000: fix bugs from recent commits

[Moffett, Kyle D]

On Oct 28, 2011, at 01:49, Wolfgang Denk wrote:
> Commit 114d7fc0 "e1000: Rewrite EEPROM checksum error to give more
> information" failed to initialize the checksum variable which should
> result in random results. Fix that.
> [I wonder if that code has _ever_ been tested!!]
> 

> I wonder if you have ever actually build and run this code???
> With the "checksum" variable being random (due to not being
> initialized) you should have seen serious checksum problems.
> How did this escape your testing?

Yes, sorry, that is the correct fix.

I'm running my old GIT checkout unmodified on my hardware right now
with no problems; I must have just gotten lucky with the compiler's
register allocation, perhaps?

*pokes through disassembly*

Yeah, it looks like I just got lucky and that stack slot is always
overwritten with zero by an earlier function call.


> Commit 2326a94d caused a ton of "unused variable 'x'" warnings.
> Fix these.  While we are at it, remove some bogus parens.
> 
> And all these build warnings - have you ever actully compiled that
> code?  What's going on here???  - wd
> 
> #define E1000_WRITE_FLUSH(a) \
> -	do { uint32_t x = E1000_READ_REG(a, STATUS); } while (0)
> +	E1000_READ_REG(a, STATUS)

The correct E1000_WRITE_FLUSH macro should be:
  #define E1000_WRITE_FLUSH(a) \
          do { uint32_t x = E1000_READ_REG(a, STATUS); (void)x; } while(0)

It shouldn't return a value, it's just ensuring that writes are properly
posted to the PCI bus.

I booted them and ran some standard load-and-performance tests on our
boards.  Unfortunately I was not paying enough attention to the build
log after splitting the e1000 patches to put the SPI code in a separate
file; that was when I removed the apparently-unnecessary "(void)x;".

Is there a good way to turn on "-Werror" for U-Boot builds by default?
I'll make sure to do that in the future to catch these problems first.

Ohh, actually, for some reason "-Wno-unused" was getting propagated in
to CFLAGS from my system environment variables, that's why I didn't see
the error originally.

My apologies for the bugs.

Cheers,
Kyle Moffett

--
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/

[U-Boot] [PATCH] e1000: fix bugs from recent commits

[Wolfgang Denk]

Dear "Moffett, Kyle D",

In message <FEA1AB06-A66F-4180-A2B3-076E7C719FC9@boeing.com> you wrote:
>
> The correct E1000_WRITE_FLUSH macro should be:
>   #define E1000_WRITE_FLUSH(a) \
>           do { uint32_t x =3D E1000_READ_REG(a, STATUS); (void)x; } while(0=
> )
> 
> It shouldn't return a value, it's just ensuring that writes are properly
> posted to the PCI bus.

Really? Should that not be guaranteed by the memory barriers inside
the E1000_READ_REG() code?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Here's a fish hangs in the net like a poor man's right in  the  law.
'Twill hardly come out."     - Shakespeare, Pericles, Act II, Scene 1

[U-Boot] [PATCH] e1000: fix bugs from recent commits

[Marek Vasut]

> Commit 114d7fc0 "e1000: Rewrite EEPROM checksum error to give more
> information" failed to initialize the checksum variable which should
> result in random results. Fix that.
> [I wonder if that code has _ever_ been tested!!]
> 
> Commit 2326a94d caused a ton of "unused variable 'x'" warnings.
> Fix these.  While we are at it, remove some bogus parens.
> 
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Kyle Moffett <Kyle.D.Moffett@boeing.com>
> ---
> 
> Kyle,
> 
> I wonder if you have ever actually build and run this code???
> With the "checksum" variable being random (due to not being
> initialized) you should have seen serious checksum problems.
> How did this escape your testing?
> 
> And all these build warnings - have you ever actully compiled that
> code?  What's going on here???  - wd
> 
> Marek: Could you please be so kind and have a look at the debug code?
> I think this needs a major cleanup, too.  Thanks in advance.  - wd

I might take a sneak peek ... -mv

[U-Boot] [PATCH v2] e1000: fix bugs from recent commits

[Wolfgang Denk]

Commit 114d7fc0 "e1000: Rewrite EEPROM checksum error to give more
information" failed to initialize the checksum variable which should
result in random results. Fix that.

Commit 2326a94d caused a ton of "unused variable 'x'" warnings.
Fix these.  While we are at it, remove some bogus parens.

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
v2: Added do{}while(0) for E1000_WRITE_FLUSH as requested.

 drivers/net/e1000.c |    1 +
 drivers/net/e1000.h |   10 +++++-----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
index c86bf0a..6eab7b2 100644
--- a/drivers/net/e1000.c
+++ b/drivers/net/e1000.c
@@ -884,6 +884,7 @@ static int e1000_validate_eeprom_checksum(struct e1000_hw *hw)
 	}
 
 	/* Compute the checksum */
+	checksum = 0;
 	for (i = 0; i < EEPROM_CHECKSUM_REG; i++)
 		checksum += buf[i];
 	checksum = ((uint16_t)EEPROM_SUM) - checksum;
diff --git a/drivers/net/e1000.h b/drivers/net/e1000.h
index 05f2bce..d8400d4 100644
--- a/drivers/net/e1000.h
+++ b/drivers/net/e1000.h
@@ -62,15 +62,15 @@
 
 /* I/O wrapper functions */
 #define E1000_WRITE_REG(a, reg, value) \
-	(writel((value), ((a)->hw_addr + E1000_##reg)))
+	writel((value), ((a)->hw_addr + E1000_##reg))
 #define E1000_READ_REG(a, reg) \
-	(readl((a)->hw_addr + E1000_##reg))
+	readl((a)->hw_addr + E1000_##reg)
 #define E1000_WRITE_REG_ARRAY(a, reg, offset, value) \
-	(writel((value), ((a)->hw_addr + E1000_##reg + ((offset) << 2))))
+	writel((value), ((a)->hw_addr + E1000_##reg + ((offset) << 2)))
 #define E1000_READ_REG_ARRAY(a, reg, offset) \
-	(readl((a)->hw_addr + E1000_##reg + ((offset) << 2)))
+	readl((a)->hw_addr + E1000_##reg + ((offset) << 2))
 #define E1000_WRITE_FLUSH(a) \
-	do { uint32_t x = E1000_READ_REG(a, STATUS); } while (0)
+	do { E1000_READ_REG(a, STATUS); } while (0)
 
 /* Forward declarations of structures used by the shared code */
 struct e1000_hw;
-- 
1.7.6.4

[U-Boot] [PATCH v2] e1000: fix bugs from recent commits

[Mike Frysinger]

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111030/81350751/attachment.pgp 

[U-Boot] [PATCH v2] e1000: fix bugs from recent commits

[Moffett, Kyle D]

On Oct 29, 2011, at 15:33, Wolfgang Denk wrote:
> Commit 114d7fc0 "e1000: Rewrite EEPROM checksum error to give more
> information" failed to initialize the checksum variable which should
> result in random results. Fix that.
> 
> Commit 2326a94d caused a ton of "unused variable 'x'" warnings.
> Fix these.  While we are at it, remove some bogus parens.
> 
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Kyle Moffett <Kyle.D.Moffett@boeing.com>

Tested-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>

Thanks for doing this; my apologies for missing these issues in the first place.

Cheers,
Kyle Moffett