Martijn Berger has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/39963 )
Change subject: nicintel: add I350 support ......................................................................
nicintel: add I350 support
While trying to flash I350 Intel NIC's on arm64 linux I had to change some things to make it work well
Change-Id: Id203d2ce247cf39a8d650887322ccced0151c6a9 Signed-off-by: Martijn Berger martijn.berger@gmail.com --- M nicintel_eeprom.c M nicintel_spi.c 2 files changed, 39 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/63/39963/1
diff --git a/nicintel_eeprom.c b/nicintel_eeprom.c index 4d45f79..be03e41 100644 --- a/nicintel_eeprom.c +++ b/nicintel_eeprom.c @@ -27,6 +27,9 @@ * 8.4.3: EEPROM-Mode Read Register * 8.4.6: EEPROM-Mode Write Register * Write process inspired on kernel e1000_i210.c + * + * Intel I350 Gigabit controller Datasheet + * https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/ethe... */
#include <stdlib.h> @@ -75,8 +78,6 @@ static struct pci_dev *nicintel_pci; static bool done_i20_write = false;
-#define UNPROG_DEVICE 0x1509 - /* * Warning: is_i210() below makes assumptions on these PCI ids. * It may have to be updated when this list is extended. @@ -87,7 +88,7 @@ {PCI_VENDOR_ID_INTEL, 0x1510, NT , "Intel", "82580 Quad Gigabit Ethernet Controller (Backplane)"}, {PCI_VENDOR_ID_INTEL, 0x1511, NT , "Intel", "82580 Quad Gigabit Ethernet Controller (Ext. PHY)"}, {PCI_VENDOR_ID_INTEL, 0x1511, NT , "Intel", "82580 Dual Gigabit Ethernet Controller (Copper)"}, - {PCI_VENDOR_ID_INTEL, UNPROG_DEVICE, OK, "Intel", "Unprogrammed 82580 Quad/Dual Gigabit Ethernet Controller"}, + {PCI_VENDOR_ID_INTEL, 0x1509, OK, "Intel", "Unprogrammed 82580 Quad/Dual Gigabit Ethernet Controller"}, {PCI_VENDOR_ID_INTEL, 0x1531, NT, "Intel", "I210 Gigabit Network Connection Unprogrammed"}, {PCI_VENDOR_ID_INTEL, 0x1532, NT, "Intel", "I211 Gigabit Network Connection Unprogrammed"}, {PCI_VENDOR_ID_INTEL, 0x1533, OK, "Intel", "I210 Gigabit Network Connection"}, @@ -95,9 +96,26 @@ {PCI_VENDOR_ID_INTEL, 0x1537, NT, "Intel", "I210 Gigabit Network Connection SERDES Backplane"}, {PCI_VENDOR_ID_INTEL, 0x1538, NT, "Intel", "I210 Gigabit Network Connection SGMII"}, {PCI_VENDOR_ID_INTEL, 0x1539, NT, "Intel", "I211 Gigabit Network Connection"}, + {PCI_VENDOR_ID_INTEL, 0x151f, OK, "Intel", "I350 Gigabit Network Connection Unprogrammed"}, + {PCI_VENDOR_ID_INTEL, 0x1521, OK, "Intel", "I350 Gigabit Network Connection Copper"}, + {PCI_VENDOR_ID_INTEL, 0x1522, NT, "Intel", "I350 Gigabit Network Connection Fiber"}, + {PCI_VENDOR_ID_INTEL, 0x1523, NT, "Intel", "I350 Gigabit Network Connection 1000BASE-KX / 1000BASE-BX backplane"}, + {PCI_VENDOR_ID_INTEL, 0x1522, NT, "Intel", "I350 Gigabit Network Connection External SGMII PHY"}, {0}, };
+static inline bool is_unprog_device(uint16_t device_id) +{ + switch (device_id) + { + case 0x1509: // Unprogrammed 82580 Quad/Dual Gigabit Ethernet Controller + case 0x151f: // I350 Gigabit Network Connection Unprogrammed + return true; + default: + return false; + } +} + static inline bool is_i210(uint16_t device_id) { return (device_id & 0xfff0) == 0x1530; @@ -118,7 +136,7 @@
static int nicintel_ee_probe_82580(struct flashctx *flash) { - if (nicintel_pci->device_id == UNPROG_DEVICE) + if (is_unprog_device(nicintel_pci->device_id)) flash->chip->total_size = 16; /* Fall back to minimum supported size. */ else { uint32_t tmp = pci_mmio_readl(nicintel_eebar + EEC); @@ -456,7 +474,7 @@ if (!dev) return 1;
- uint32_t io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); + uintptr_t io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); if (!io_base_addr) return 1;
@@ -466,7 +484,7 @@ return 1;
nicintel_pci = dev; - if ((dev->device_id != UNPROG_DEVICE)) { + if (!is_unprog_device(dev->device_id)) { uint32_t eec = pci_mmio_readl(nicintel_eebar + EEC);
/* C.f. 3.3.1.5 for the detection mechanism (maybe? contradicting diff --git a/nicintel_spi.c b/nicintel_spi.c index 1173ef7..09a37dc 100644 --- a/nicintel_spi.c +++ b/nicintel_spi.c @@ -28,6 +28,9 @@ * * Intel 82599 10 GbE Controller Datasheet (331520) * http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/82599... + * + * Intel I350 Gigabit controller Datasheet + * https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/ethe... */
#include <stdlib.h> @@ -104,6 +107,11 @@ {PCI_VENDOR_ID_INTEL, 0x1538, NT, "Intel", "I210 Gigabit Network Connection SGMII"}, {PCI_VENDOR_ID_INTEL, 0x1539, NT, "Intel", "I211 Gigabit Network Connection"},
+ {PCI_VENDOR_ID_INTEL, 0x151f, NT, "Intel", "I350 Gigabit Network Connection Unprogrammed"}, + {PCI_VENDOR_ID_INTEL, 0x1521, NT, "Intel", "I350 Gigabit Network Connection Copper"}, + {PCI_VENDOR_ID_INTEL, 0x1522, NT, "Intel", "I350 Gigabit Network Connection Fiber"}, + {PCI_VENDOR_ID_INTEL, 0x1523, NT, "Intel", "I350 Gigabit Network Connection 1000BASE-KX / 1000BASE-BX backplane"}, + {PCI_VENDOR_ID_INTEL, 0x1522, NT, "Intel", "I350 Gigabit Network Connection External SGMII PHY"}, {0}, };
@@ -252,7 +260,7 @@ if (!dev) return 1;
- uint32_t io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); + uintptr_t io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); if (!io_base_addr) return 1;
@@ -266,6 +274,12 @@ MEMMAP_SIZE); if (!nicintel_spibar || nicintel_spi_82599_enable_flash()) return 1; + } else if ((dev->device_id & 0xfff0) == 0x1510 || + (dev->device_id & 0xfff0) == 0x1520) { + nicintel_spibar = rphysmap("Intel I350 Gigabit w/ SPI flash", io_base_addr, + MEMMAP_SIZE); + if (!nicintel_spibar || nicintel_spi_i210_enable_flash()) + return 1; } else { nicintel_spibar = rphysmap("Intel 10 Gigabit NIC w/ SPI flash", io_base_addr + 0x10000, MEMMAP_SIZE);
Peter Lemenkov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/39963 )
Change subject: nicintel: add I350 support ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c File nicintel_eeprom.c:
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c@103 PS1, Line 103: {PCI_VENDOR_ID_INTEL, 0x1522, NT, "Intel", "I350 Gigabit Network Connection External SGMII PHY"}, 0x1522? Should be 0x1524?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/39963 )
Change subject: nicintel: add I350 support ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c File nicintel_eeprom.c:
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c@99 PS1, Line 99: Unprogrammed Please put this at the beginning of the string
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c@109 PS1, Line 109: switch (device_id) Why use a switch? This could be simply:
return device_id == 0x1509 || device_id == 0x151f;
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c@477 PS1, Line 477: uintptr_t io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0); This change is probably needed because most PCI devices on x86_64 use 32-bit BARs
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_spi.c File nicintel_spi.c:
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_spi.c@277 PS1, Line 277: (dev->device_id & 0xfff0) == 0x1510 This check will be a false positive for 0x1517 and 0x151c
Martijn Berger has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/39963 )
Change subject: nicintel: add I350 support ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c File nicintel_eeprom.c:
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c@103 PS1, Line 103: {PCI_VENDOR_ID_INTEL, 0x1522, NT, "Intel", "I350 Gigabit Network Connection External SGMII PHY"},
0x1522? Should be 0x1524?
yes good catch
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c@477 PS1, Line 477: uintptr_t io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
This change is probably needed because most PCI devices on x86_64 use 32-bit BARs
it is needed as my platform is arm64 and the BAR0 has an address above the 32 bit boundary even if the pci device might not see it that way. Maybe a fixed size 64 bit would be more appropriate ?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/39963 )
Change subject: nicintel: add I350 support ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c File nicintel_eeprom.c:
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c@477 PS1, Line 477: uintptr_t io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
it is needed as my platform is arm64 and the BAR0 has an address above the 32 bit boundary even if t […]
Well, there may be many more instances of uint32_t to store addresses, so it would be a good idea to take care of them all at once
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/39963
to look at the new patch set (#3).
Change subject: nicintel: add I350 support ......................................................................
nicintel: add I350 support
While trying to flash I350 Intel NIC's on arm64 linux I had to change some things to make it work well
Change-Id: Id203d2ce247cf39a8d650887322ccced0151c6a9 Signed-off-by: Martijn Berger martijn.berger@gmail.com --- M nicintel_eeprom.c M nicintel_spi.c 2 files changed, 32 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/63/39963/3
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/39963
to look at the new patch set (#4).
Change subject: nicintel: add I350 support ......................................................................
nicintel: add I350 support
While trying to flash I350 Intel NIC's on arm64 linux I had to change some things to make it work well
Change-Id: Id203d2ce247cf39a8d650887322ccced0151c6a9 Signed-off-by: Martijn Berger martijn.berger@gmail.com --- M nicintel_eeprom.c M nicintel_spi.c 2 files changed, 32 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/63/39963/4
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/39963
to look at the new patch set (#5).
Change subject: nicintel: add I350 support ......................................................................
nicintel: add I350 support
While trying to flash I350 Intel NIC's on arm64 linux I had to change some things to make it work well
Change-Id: Id203d2ce247cf39a8d650887322ccced0151c6a9 Signed-off-by: Martijn Berger martijn.berger@gmail.com --- M nicintel_eeprom.c M nicintel_spi.c 2 files changed, 32 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/63/39963/5
Peter Lemenkov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/39963 )
Change subject: nicintel: add I350 support ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c File nicintel_eeprom.c:
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c@99 PS1, Line 99: Unprogrammed
Please put this at the beginning of the string
Done
Martijn Berger has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/39963 )
Change subject: nicintel: add I350 support ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c File nicintel_eeprom.c:
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c@103 PS1, Line 103: {PCI_VENDOR_ID_INTEL, 0x1522, NT, "Intel", "I350 Gigabit Network Connection External SGMII PHY"},
yes good catch
Done
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c@109 PS1, Line 109: switch (device_id)
Why use a switch? This could be simply: […]
Done
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_spi.c File nicintel_spi.c:
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_spi.c@277 PS1, Line 277: (dev->device_id & 0xfff0) == 0x1510
This check will be a false positive for 0x1517 and 0x151c
Done
Martijn Berger has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/39963 )
Change subject: nicintel: add I350 support ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c File nicintel_eeprom.c:
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c@103 PS1, Line 103: {PCI_VENDOR_ID_INTEL, 0x1522, NT, "Intel", "I350 Gigabit Network Connection External SGMII PHY"},
Done
Done
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c@109 PS1, Line 109: switch (device_id)
Done
Done
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_eeprom.c@477 PS1, Line 477: uintptr_t io_base_addr = pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
Well, there may be many more instances of uint32_t to store addresses, so it would be a good idea to […]
Ack
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_spi.c File nicintel_spi.c:
https://review.coreboot.org/c/flashrom/+/39963/1/nicintel_spi.c@277 PS1, Line 277: (dev->device_id & 0xfff0) == 0x1510
Done
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/39963 )
Change subject: nicintel: add I350 support ......................................................................
Patch Set 6: Code-Review+1
(2 comments)
https://review.coreboot.org/c/flashrom/+/39963/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/39963/6//COMMIT_MSG@10 PS6, Line 10: some things Mention it was necessary to change the type of the base address variables?
https://review.coreboot.org/c/flashrom/+/39963/6/nicintel_spi.c File nicintel_spi.c:
https://review.coreboot.org/c/flashrom/+/39963/6/nicintel_spi.c@110 PS6, Line 110: {PCI_VENDOR_ID_INTEL, 0x151f, NT, "Intel", "I350 Gigabit Network Connection Unprogrammed"}, : {PCI_VENDOR_ID_INTEL, 0x1521, NT, "Intel", "I350 Gigabit Network Connection Copper"}, : {PCI_VENDOR_ID_INTEL, 0x1522, NT, "Intel", "I350 Gigabit Network Connection Fiber"}, : {PCI_VENDOR_ID_INTEL, 0x1523, NT, "Intel", "I350 Gigabit Network Connection 1000BASE-KX / 1000BASE-BX backplane"}, : {PCI_VENDOR_ID_INTEL, 0x1524, NT, "Intel", "I350 Gigabit Network Connection External SGMII PHY"}, None of these are tested?
Martijn Berger has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/39963 )
Change subject: nicintel: add I350 support ......................................................................
Patch Set 6:
(2 comments)
I will augment the commit message
https://review.coreboot.org/c/flashrom/+/39963/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/39963/6//COMMIT_MSG@10 PS6, Line 10: some things
Mention it was necessary to change the type of the base address variables?
Ack
https://review.coreboot.org/c/flashrom/+/39963/6/nicintel_spi.c File nicintel_spi.c:
https://review.coreboot.org/c/flashrom/+/39963/6/nicintel_spi.c@110 PS6, Line 110: {PCI_VENDOR_ID_INTEL, 0x151f, NT, "Intel", "I350 Gigabit Network Connection Unprogrammed"}, : {PCI_VENDOR_ID_INTEL, 0x1521, NT, "Intel", "I350 Gigabit Network Connection Copper"}, : {PCI_VENDOR_ID_INTEL, 0x1522, NT, "Intel", "I350 Gigabit Network Connection Fiber"}, : {PCI_VENDOR_ID_INTEL, 0x1523, NT, "Intel", "I350 Gigabit Network Connection 1000BASE-KX / 1000BASE-BX backplane"}, : {PCI_VENDOR_ID_INTEL, 0x1524, NT, "Intel", "I350 Gigabit Network Connection External SGMII PHY"},
None of these are tested?
Yes, we abandoned the SPI for our product and I do not have the time to add the chips back to my Intel produced PCIe I350 that I used as a reference.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/39963 )
Change subject: nicintel: add I350 support ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/flashrom/+/39963/6/nicintel_spi.c File nicintel_spi.c:
https://review.coreboot.org/c/flashrom/+/39963/6/nicintel_spi.c@110 PS6, Line 110: {PCI_VENDOR_ID_INTEL, 0x151f, NT, "Intel", "I350 Gigabit Network Connection Unprogrammed"}, : {PCI_VENDOR_ID_INTEL, 0x1521, NT, "Intel", "I350 Gigabit Network Connection Copper"}, : {PCI_VENDOR_ID_INTEL, 0x1522, NT, "Intel", "I350 Gigabit Network Connection Fiber"}, : {PCI_VENDOR_ID_INTEL, 0x1523, NT, "Intel", "I350 Gigabit Network Connection 1000BASE-KX / 1000BASE-BX backplane"}, : {PCI_VENDOR_ID_INTEL, 0x1524, NT, "Intel", "I350 Gigabit Network Connection External SGMII PHY"},
Yes, we abandoned the SPI for our product and I do not have the time to add the chips back to my Int […]
Ack
-
Angel Pons (Code Review) -
Martijn Berger (Code Review)
-
Peter Lemenkov (Code Review)