diff mbox

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

Message ID 1297467482-14864-3-git-send-email-Kyle.D.Moffett@boeing.com
State Superseded, archived
Headers show

Commit Message

Kyle Moffett Feb. 11, 2011, 11:37 p.m. UTC 
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>
---
 drivers/net/e1000.c |  117 +++++++++++++++++++++++++--------------------------
 drivers/net/e1000.h |    3 +
 2 files changed, 60 insertions(+), 60 deletions(-)

Comments

Wolfgang Denk April 12, 2011, 8:17 p.m. UTC | #1 
Dear Kyle Moffett,

In message <1297467482-14864-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.

This patch has a number of overlong lines.  Please globally fix the
line length.

Thanks.

Best regards,

Wolfgang Denk
Kyle Moffett April 12, 2011, 10:56 p.m. UTC | #2 
On Apr 12, 2011, at 16:17, Wolfgang Denk wrote:
> In message <1297467482-14864-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.
> 
> This patch has a number of overlong lines.  Please globally fix the
> line length.

The only lines longer than 80 characters in this patch are these 4 when
indented by 3 levels:
> hw->hw_addr = pci_map_bar(devno, PCI_BASE_ADDRESS_0, PCI_REGION_MEM);
> printf("%s: ERROR: Can't enable I/O memory\n", nic->name);
> printf("%s: ERROR: Can't enable bus-mastering\n", nic->name);
> printf("%s: ERROR: EEPROM checksum is bad!\n", nic->name);

According to Documentation/CodingStyle:
> Statements longer than 80 columns will be broken into sensible chunks.
> Descendants are always substantially shorter than the parent and are placed
> substantially to the right. The same applies to function headers with a long
> argument list. Long strings are as well broken into shorter strings. The
> only exception to this is where exceeding 80 columns significantly increases
> readability and does not hide information.


Wrapping any of those lines will in fact make them much harder to read and
dissimilar to the other nearly identical printf() calls in that piece of
code; I strongly disagree that it is necessary.

Cheers,
Kyle Moffett
Wolfgang Denk April 13, 2011, 5:13 a.m. UTC | #3 
Dear "Moffett, Kyle D",

In message <85579850-725C-46CC-B6AF-9DE6D6683C82@boeing.com> you wrote:
>
> indented by 3 levels:
> > hw->hw_addr = pci_map_bar(devno, PCI_BASE_ADDRESS_0, PCI_REGION_MEM);
> > printf("%s: ERROR: Can't enable I/O memory\n", nic->name);
> > printf("%s: ERROR: Can't enable bus-mastering\n", nic->name);
> > printf("%s: ERROR: EEPROM checksum is bad!\n", nic->name);
...
> Wrapping any of those lines will in fact make them much harder to read and
> dissimilar to the other nearly identical printf() calls in that piece of
> code; I strongly disagree that it is necessary.

What makes you think that

	printf("%s: ERROR: Can't enable I/O memory\n", nic->name);

was "much harder to read" than

	printf("%s: ERROR: Can't enable I/O memory\n",
		nic->name);

?

Please fix these!

Best regards,

Wolfgang Denk
diff mbox

Patch

diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
index 5383064..ddf29c8 100644
--- a/drivers/net/e1000.c
+++ b/drivers/net/e1000.c
@@ -65,7 +65,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},
@@ -4746,7 +4746,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;
@@ -5146,57 +5146,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);
+		DEBUGOUT("%s: iobase 0x%08x\n", nic->name, 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)) {
+			printf("%s: ERROR: Can't enable I/O memory\n", nic->name);
+			continue;
+		}
+		if (!(val & PCI_COMMAND_MASTER)) {
+			printf("%s: ERROR: Can't enable bus-mastering\n", nic->name);
+			continue;
+		}
 
 		/* Are these variables needed? */
 		hw->fc = e1000_fc_default;
@@ -5204,50 +5206,45 @@  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) {
+			printf("%s: ERROR: Software init failed\n", nic->name);
+			continue;
 		}
 		if (e1000_check_phy_reset_block(hw))
-			printf("phy reset block error \n");
+			printf("%s: ERROR: PHY Reset is blocked!\n", nic->name);
+
+		/* 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;
+			printf("%s: ERROR: EEPROM is invalid!\n", nic->name);
+			continue;
 		}
 		if (e1000_validate_eeprom_checksum(nic) < 0) {
-			printf("The EEPROM Checksum Is Not Valid\n");
-			free(hw);
-			free(nic);
-			return 0;
+			printf("%s: ERROR: EEPROM checksum is bad!\n", nic->name);
+			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",
 		       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 8597e23..8573511 100644
--- a/drivers/net/e1000.h
+++ b/drivers/net/e1000.h
@@ -1043,6 +1043,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;