November 2021 - flashrom-gerrit

Change in flashrom[master]: dmi.c: Hide has_dmi_support global behind method
by Edward O'Callaghan (Code Review) 14 Nov '21

14 Nov '21

Attention is currently required from: Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59283 ) Change subject: dmi.c: Hide has_dmi_support global behind method ...................................................................... Patch Set 1: (1 comment) File dmi.c: https://review.coreboot.org/c/flashrom/+/59283/comment/5e5debe8_c0148dba PS1, Line 473: g_has_dmi_support has_dmi_support() -- To view, visit https://review.coreboot.org/c/flashrom/+/59283 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibded9714998ea6f2e5d4e0512fa7c6b105f9638a Gerrit-Change-Number: 59283 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 14 Nov 2021 05:56:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: internal: Hide internal_buses_supported state behind methods
by Edward O'Callaghan (Code Review) 14 Nov '21

14 Nov '21

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/59284 ) Change subject: internal: Hide internal_buses_supported state behind methods ...................................................................... internal: Hide internal_buses_supported state behind methods Push the internal_buses_supported global state inwards to become a static local to internal.c and hide the manipulation of the state behind methods to allow reasoning about it. BUG=none TEST=builds Change-Id: I2f2e136b2d618f120a177f4c2ca15957783800c1 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M board_enable.c M chipset_enable.c M ichspi.c M internal.c M it85spi.c M it87spi.c M programmer.h 7 files changed, 44 insertions(+), 21 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/84/59284/1 diff --git a/board_enable.c b/board_enable.c index 0141d1c..7832da5 100644 --- a/board_enable.c +++ b/board_enable.c @@ -601,7 +601,7 @@ uint8_t tmp; int ret = 0; - if (!(internal_buses_supported & BUS_PARALLEL)) + if (!is_internal_buses_supported(BUS_PARALLEL, BUS_NONE)) return 1; enter_conf_mode_ite(port); @@ -609,7 +609,7 @@ /* Check if at least one flash segment is enabled. */ if (tmp & 0xf0) { /* The IT8705F will respond to LPC cycles and translate them. */ - internal_buses_supported &= BUS_PARALLEL; + add_internal_buses_supported(BUS_PARALLEL); /* Flash ROM I/F Writes Enable */ tmp |= 0x04; msg_pdbg("Enabling IT8705F flash ROM interface write.\n"); diff --git a/chipset_enable.c b/chipset_enable.c index f48d311..145a570 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -67,13 +67,13 @@ tmp = pci_read_byte(dev, 0x40) & 0x3; switch (tmp) { case 3: - internal_buses_supported &= BUS_FWH; + add_internal_buses_supported(BUS_FWH); break; case 2: - internal_buses_supported &= BUS_LPC; + add_internal_buses_supported(BUS_LPC); break; default: - internal_buses_supported &= BUS_PARALLEL; + add_internal_buses_supported(BUS_PARALLEL); break; } @@ -223,7 +223,7 @@ uint16_t old, new; uint16_t xbcs = 0x4e; /* X-Bus Chip Select register. */ - internal_buses_supported &= BUS_PARALLEL; + add_internal_buses_supported(BUS_PARALLEL); old = pci_read_word(dev, xbcs); @@ -558,7 +558,7 @@ if ((err = enable_flash_ich_fwh_decode(dev, ich_generation)) != 0) return err; - internal_buses_supported &= BUS_FWH; + add_internal_buses_supported(BUS_FWH); return enable_flash_ich_bios_cntl_config_space(dev, ich_generation, bios_cntl); } @@ -1013,7 +1013,7 @@ if (ret_fwh == ERROR_FATAL) return ret_fwh; - internal_buses_supported &= BUS_FWH; + add_internal_buses_supported(BUS_FWH); /* Get physical address of SPI Base Address and map it */ uint32_t sbase = pci_read_long(dev, 0x54) & 0xfffffe00; @@ -1167,7 +1167,7 @@ #define CS5530_ENABLE_SA2320 (1 << 2) #define CS5530_ENABLE_SA20 (1 << 6) - internal_buses_supported &= BUS_PARALLEL; + add_internal_buses_supported(BUS_PARALLEL); /* Decode 0x000E0000-0x000FFFFF (128 kB), not just 64 kB, and * decode 0xFF000000-0xFFFFFFFF (16 MB), not just 256 kB. * FIXME: Should we really touch the low mapping below 1 MB? Flashrom @@ -1355,7 +1355,7 @@ msg_pdbg("done.\n"); } - internal_buses_supported &= BUS_LPC | BUS_FWH; + add_internal_buses_supported(BUS_LPC | BUS_FWH); ret = sb600_probe_spi(dev); @@ -1503,7 +1503,7 @@ { uint8_t tmp; - internal_buses_supported &= BUS_PARALLEL; + add_internal_buses_supported(BUS_PARALLEL); tmp = INB(0xc06); tmp |= 0x1; @@ -1592,7 +1592,7 @@ switch ((val >> 5) & 0x3) { case 0x0: ret = enable_flash_mcp55(dev, name); - internal_buses_supported &= BUS_LPC; + add_internal_buses_supported(BUS_LPC); msg_pdbg("Flash bus type is LPC\n"); break; case 0x2: @@ -1600,12 +1600,12 @@ /* SPI is added in mcp6x_spi_init if it works. * Do we really want to disable LPC in this case? */ - internal_buses_supported = BUS_NONE; + clear_internal_buses_supported(); msg_pdbg("Flash bus type is SPI\n"); break; default: /* Should not happen. */ - internal_buses_supported = BUS_NONE; + clear_internal_buses_supported(); msg_pwarn("Flash bus type is unknown (none)\n"); msg_pinfo("Please send the log files created by \"flashrom -p internal -o logfile\" to\n" "flashrom(a)flashrom.org with \"your board name: flashrom -V\" as the subject to\n" @@ -2152,7 +2152,7 @@ "flashrom(a)flashrom.org including a verbose " "(-V) log.\nThank you!\n"); } - if (!(chipset_enables[i].buses & (internal_buses_supported | BUS_SPI))) { + if (!is_internal_buses_supported(chipset_enables[i].buses, BUS_SPI)) { msg_pdbg("Skipping chipset enable: No supported buses enabled.\n"); continue; } diff --git a/ichspi.c b/ichspi.c index d345af2..047a82a 100644 --- a/ichspi.c +++ b/ichspi.c @@ -2072,7 +2072,7 @@ /* Do we really need no write enable? Like the LPC one at D17F0 0x40 */ /* Not sure if it speaks all these bus protocols. */ - internal_buses_supported &= BUS_LPC | BUS_FWH; + add_internal_buses_supported(BUS_LPC | BUS_FWH); ich_generation = CHIPSET_ICH7; register_spi_master(&spi_master_via, NULL); diff --git a/internal.c b/internal.c index 508906d..189a1e9 100644 --- a/internal.c +++ b/internal.c @@ -27,7 +27,27 @@ int force_boardenable = 0; int force_boardmismatch = 0; -enum chipbustype internal_buses_supported = BUS_NONE; +static enum chipbustype internal_buses_supported = BUS_NONE; + +void clear_internal_buses_supported(void) +{ + internal_buses_supported = BUS_NONE; +} + +enum chipbustype get_internal_buses_supported(void) +{ + return internal_buses_supported; +} + +void add_internal_buses_supported(enum chipbustype flag) +{ + internal_buses_supported &= flag; +} + +bool is_internal_buses_supported(enum chipbustype flag, enum chipbustype mask) +{ + return (internal_buses_supported | mask) & flag; +} #if defined(__i386__) || defined(__x86_64__) void probe_superio(void) diff --git a/it85spi.c b/it85spi.c index 17e4e10..cbdfcbd 100644 --- a/it85spi.c +++ b/it85spi.c @@ -297,13 +297,13 @@ unsigned int shm_io_base = 0; msg_pdbg("%s():%d superio.vendor=0x%02x internal_buses_supported=0x%x\n", - __func__, __LINE__, s.vendor, internal_buses_supported); + __func__, __LINE__, s.vendor, get_internal_buses_supported()); /* Check for FWH because IT85 listens to FWH cycles. * FIXME: The big question is whether FWH cycles are necessary * for communication even if LPC_IO is defined. */ - if (!(internal_buses_supported & BUS_FWH)) { + if (!is_internal_buses_supported(BUS_FWH, BUS_NONE)) { msg_pdbg("%s():%d buses not support FWH\n", __func__, __LINE__); return 1; } diff --git a/it87spi.c b/it87spi.c index 5f6fb65..3655cbc 100644 --- a/it87spi.c +++ b/it87spi.c @@ -418,7 +418,7 @@ data->flashport = flashport; data->fast_spi = 1; - if (internal_buses_supported & BUS_SPI) + if (is_internal_buses_supported(BUS_SPI, BUS_NONE)) msg_pdbg("Overriding chipset SPI with IT87 SPI.\n"); /* FIXME: Add the SPI bus or replace the other buses with it? */ return register_spi_master(&spi_master_it87xx, data); diff --git a/programmer.h b/programmer.h index 1f64d5a..d6dd68e 100644 --- a/programmer.h +++ b/programmer.h @@ -279,7 +279,10 @@ extern int force_boardmismatch; void probe_superio(void); int register_superio(struct superio s); -extern enum chipbustype internal_buses_supported; +void clear_internal_buses_supported(void); +enum chipbustype get_internal_buses_supported(void); +void add_internal_buses_supported(enum chipbustype flag); +bool is_internal_buses_supported(enum chipbustype flag, enum chipbustype mask); #endif /* bitbang_spi.c */ -- To view, visit https://review.coreboot.org/c/flashrom/+/59284 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2f2e136b2d618f120a177f4c2ca15957783800c1 Gerrit-Change-Number: 59284 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: dmi.c: Hide has_dmi_support global behind method
by Edward O'Callaghan (Code Review) 14 Nov '21

14 Nov '21

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/59283 ) Change subject: dmi.c: Hide has_dmi_support global behind method ...................................................................... dmi.c: Hide has_dmi_support global behind method This allows has_dmi_support to be become static local to just the scope of dmi.c BUG=none TEST=builds Change-Id: Ibded9714998ea6f2e5d4e0512fa7c6b105f9638a Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M board_enable.c M dmi.c M programmer.h 3 files changed, 10 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/83/59283/1 diff --git a/board_enable.c b/board_enable.c index a3a8b01..0141d1c 100644 --- a/board_enable.c +++ b/board_enable.c @@ -2656,7 +2656,7 @@ #if defined(__i386__) || defined(__x86_64__) if (board->dmi_pattern) { - if (!has_dmi_support) { + if (!has_dmi_support()) { msg_pwarn("Warning: Can't autodetect %s %s, DMI info unavailable.\n", board->vendor_name, board->board_name); msg_pinfo("Please supply the board vendor and model name with the " diff --git a/dmi.c b/dmi.c index d2e149d..8bce800 100644 --- a/dmi.c +++ b/dmi.c @@ -40,7 +40,12 @@ /* Strings longer than 4096 in DMI are just insane. */ #define DMI_MAX_ANSWER_LEN 4096 -int has_dmi_support = 0; +static int g_has_dmi_support = 0; + +int has_dmi_support(void) +{ + return g_has_dmi_support; +} static struct { const char *const keyword; @@ -405,7 +410,7 @@ break; } - has_dmi_support = 1; + g_has_dmi_support = 1; unsigned int i; for (i = 0; i < ARRAY_SIZE(dmi_strings); i++) { msg_pdbg("DMI string %s: \"%s\"\n", dmi_strings[i].keyword, @@ -465,7 +470,7 @@ { unsigned int i; - if (!has_dmi_support) + if (!g_has_dmi_support) return 0; for (i = 0; i < ARRAY_SIZE(dmi_strings); i++) { diff --git a/programmer.h b/programmer.h index ffd2516..1f64d5a 100644 --- a/programmer.h +++ b/programmer.h @@ -254,7 +254,7 @@ /* dmi.c */ #if defined(__i386__) || defined(__x86_64__) -extern int has_dmi_support; +int has_dmi_support(void); void dmi_init(void); int dmi_match(const char *pattern); #endif // defined(__i386__) || defined(__x86_64__) -- To view, visit https://review.coreboot.org/c/flashrom/+/59283 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibded9714998ea6f2e5d4e0512fa7c6b105f9638a Gerrit-Change-Number: 59283 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: pcidev.c: Rename pci_dev_find() -> pcidev_find()
by Edward O'Callaghan (Code Review) 13 Nov '21

13 Nov '21

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/59282 ) Change subject: pcidev.c: Rename pci_dev_find() -> pcidev_find() ...................................................................... pcidev.c: Rename pci_dev_find() -> pcidev_find() Make symbol name consistent with the rest in pcidev.c BUG=none TEST=builds Change-Id: I41e0922d5429b4389367dd98b4241d50b8013183 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M board_enable.c M chipset_enable.c M pcidev.c M programmer.h M sb600spi.c 5 files changed, 23 insertions(+), 23 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/82/59282/1 diff --git a/board_enable.c b/board_enable.c index b86cfe6..a3a8b01 100644 --- a/board_enable.c +++ b/board_enable.c @@ -889,7 +889,7 @@ uint16_t smbba; uint8_t b; - dev = pci_dev_find(0x8086, 0x7113); /* Intel PIIX4, PM/SMBus function. */ + dev = pcidev_find(0x8086, 0x7113); /* Intel PIIX4, PM/SMBus function. */ if (!dev) { msg_perr("\nERROR: Intel PIIX4 PM not found.\n"); return -1; @@ -1041,7 +1041,7 @@ struct pci_dev *dev; uint32_t tmp; - dev = pci_dev_find(0x10DE, 0x03EB); /* NVIDIA MCP61 SMBus. */ + dev = pcidev_find(0x10DE, 0x03EB); /* NVIDIA MCP61 SMBus. */ if (!dev) { msg_perr("\nERROR: NVIDIA MCP61 SMBus not found.\n"); return -1; @@ -1269,7 +1269,7 @@ if (ret) return ret; - dev = pci_dev_find(0x10de, 0x0364); /* NVIDIA MCP55 LPC bridge */ + dev = pcidev_find(0x10de, 0x0364); /* NVIDIA MCP55 LPC bridge */ if (!dev) { msg_perr("\nERROR: NVIDIA MCP55 LPC bridge not found.\n"); return -1; @@ -1339,7 +1339,7 @@ struct pci_dev *dev; uint32_t reg; - dev = pci_dev_find(0x1002, 0x4372); /* AMD SMBus controller */ + dev = pcidev_find(0x1002, 0x4372); /* AMD SMBus controller */ if (!dev) { msg_perr("\nERROR: AMD SMBus Controller (0x4372) not found.\n"); return -1; @@ -1403,7 +1403,7 @@ {0} }; - dev = pci_dev_find(0x8086, 0x7110); /* Intel PIIX4 ISA bridge */ + dev = pcidev_find(0x8086, 0x7110); /* Intel PIIX4 ISA bridge */ if (!dev) { msg_perr("\nERROR: Intel PIIX4 ISA bridge not found.\n"); return -1; @@ -1422,7 +1422,7 @@ return -1; } - dev = pci_dev_find(0x8086, 0x7113); /* Intel PIIX4 PM */ + dev = pcidev_find(0x8086, 0x7113); /* Intel PIIX4 PM */ if (!dev) { msg_perr("\nERROR: Intel PIIX4 PM not found.\n"); return -1; @@ -1931,7 +1931,7 @@ uint32_t base, tmp; /* VT82C686 power management */ - dev = pci_dev_find(0x1106, 0x3057); + dev = pcidev_find(0x1106, 0x3057); if (!dev) { msg_perr("\nERROR: VT82C686 PM device not found.\n"); return -1; @@ -2004,7 +2004,7 @@ struct pci_dev *dev; uint16_t base, temp; - dev = pci_dev_find(0x1039, 0x0962); + dev = pcidev_find(0x1039, 0x0962); if (!dev) { msg_perr("Expected south bridge not found\n"); return 1; @@ -2068,7 +2068,7 @@ uint16_t rt_port; uint8_t val; - dev = pci_dev_find(0x8086, 0x2410); /* Intel 82801AA ISA bridge */ + dev = pcidev_find(0x8086, 0x2410); /* Intel 82801AA ISA bridge */ if (!dev) { msg_perr("\nERROR: Intel 82801AA ISA bridge not found.\n"); return -1; @@ -2593,13 +2593,13 @@ if (!cur_model || strcasecmp(cur_model, model)) continue; - if (!pci_dev_find(board->first_vendor, board->first_device)) { + if (!pcidev_find(board->first_vendor, board->first_device)) { msg_pdbg("Odd. Board name \"%s\":\"%s\" matches, but first PCI device %04x:%04x " "doesn't.\n", vendor, model, board->first_vendor, board->first_device); continue; } - if (!pci_dev_find(board->second_vendor, board->second_device)) { + if (!pcidev_find(board->second_vendor, board->second_device)) { msg_pdbg("Odd. Board name \"%s\":\"%s\" matches, but second PCI device %04x:%04x " "doesn't.\n", vendor, model, board->second_vendor, board->second_device); continue; @@ -2648,7 +2648,7 @@ board->second_card_device)) continue; } else { - if (!pci_dev_find(board->second_vendor, + if (!pcidev_find(board->second_vendor, board->second_device)) continue; } diff --git a/chipset_enable.c b/chipset_enable.c index 0873fed..f48d311 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -1083,7 +1083,7 @@ static int enable_flash_vt_vx(struct pci_dev *dev, const char *name) { - struct pci_dev *south_north = pci_dev_find(0x1106, 0xa353); + struct pci_dev *south_north = pcidev_find(0x1106, 0xa353); if (south_north == NULL) { msg_perr("Could not find South-North Module Interface Control device!\n"); return ERROR_FATAL; @@ -1523,7 +1523,7 @@ struct pci_dev *smbusdev; /* Look for the SMBus device. */ - smbusdev = pci_dev_find(0x1002, 0x4372); + smbusdev = pcidev_find(0x1002, 0x4372); if (!smbusdev) { msg_perr("ERROR: SMBus device not found. Aborting.\n"); @@ -2116,7 +2116,7 @@ /* Now let's try to find the chipset we have... */ for (i = 0; chipset_enables[i].vendor_name != NULL; i++) { - dev = pci_dev_find(chipset_enables[i].vendor_id, + dev = pcidev_find(chipset_enables[i].vendor_id, chipset_enables[i].device_id); if (!dev) continue; diff --git a/pcidev.c b/pcidev.c index 629304a..d6911b1 100644 --- a/pcidev.c +++ b/pcidev.c @@ -155,7 +155,7 @@ return (uintptr_t)addr; } -struct pci_dev *pci_dev_find(uint16_t vendor, uint16_t device) +struct pci_dev *pcidev_find(uint16_t vendor, uint16_t device) { struct pci_dev *temp; struct pci_filter filter; @@ -172,7 +172,7 @@ struct pci_dev *pcidev_find_vendorclass(uint16_t vendor, uint16_t devclass) { - struct pci_dev *temp = pci_dev_find(vendor, 0); + struct pci_dev *temp = pcidev_find(vendor, 0); if (temp) { uint16_t tmp2; /* Read PCI class */ @@ -187,7 +187,7 @@ struct pci_dev *pcidev_card_find(uint16_t vendor, uint16_t device, uint16_t card_vendor, uint16_t card_device) { - struct pci_dev *temp = pci_dev_find(vendor, device); + struct pci_dev *temp = pcidev_find(vendor, device); if (temp) { if ((card_vendor == pci_read_word(temp, PCI_SUBSYSTEM_VENDOR_ID)) && (card_device == pci_read_word(temp, PCI_SUBSYSTEM_ID))) diff --git a/programmer.h b/programmer.h index b9cd3de..ffd2516 100644 --- a/programmer.h +++ b/programmer.h @@ -128,7 +128,7 @@ struct pci_dev *pcidev_getdev(struct pci_dev *dev); struct pci_dev *pcidev_card_find(uint16_t vendor, uint16_t device, uint16_t card_vendor, uint16_t card_device); -struct pci_dev *pci_dev_find(uint16_t vendor, uint16_t device); +struct pci_dev *pcidev_find(uint16_t vendor, uint16_t device); /* rpci_write_* are reversible writes. The original PCI config space register * contents will be restored on shutdown. * To clone the pci_dev instances internally, the `pacc` global diff --git a/sb600spi.c b/sb600spi.c index d5e228c..de1e0fe 100644 --- a/sb600spi.c +++ b/sb600spi.c @@ -59,7 +59,7 @@ static int find_smbus_dev_rev(uint16_t vendor, uint16_t device) { - struct pci_dev *smbus_dev = pci_dev_find(vendor, device); + struct pci_dev *smbus_dev = pcidev_find(vendor, device); if (!smbus_dev) { msg_pdbg("No SMBus device with ID %04X:%04X found.\n", vendor, device); msg_perr("ERROR: SMBus device not found. Not enabling SPI.\n"); @@ -733,11 +733,11 @@ } /* Look for the SMBus device. */ - smbus_dev = pci_dev_find(0x1002, 0x4385); + smbus_dev = pcidev_find(0x1002, 0x4385); if (!smbus_dev) - smbus_dev = pci_dev_find(0x1022, 0x780b); /* AMD FCH */ + smbus_dev = pcidev_find(0x1022, 0x780b); /* AMD FCH */ if (!smbus_dev) - smbus_dev = pci_dev_find(0x1022, 0x790b); /* AMD FP4 */ + smbus_dev = pcidev_find(0x1022, 0x790b); /* AMD FP4 */ if (!smbus_dev) { msg_perr("ERROR: SMBus device not found. Not enabling SPI.\n"); return ERROR_NONFATAL; -- To view, visit https://review.coreboot.org/c/flashrom/+/59282 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I41e0922d5429b4389367dd98b4241d50b8013183 Gerrit-Change-Number: 59282 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: pcidev.c: Simplify by consolidating common logic
by Edward O'Callaghan (Code Review) 13 Nov '21

13 Nov '21

Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59281 to look at the new patch set (#2). Change subject: pcidev.c: Simplify by consolidating common logic ...................................................................... pcidev.c: Simplify by consolidating common logic BUG=none TEST=builds Change-Id: I0048fc6ab816d230ff48c84bc17122431753d55d Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M pcidev.c M programmer.h 2 files changed, 11 insertions(+), 30 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/81/59281/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/59281 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0048fc6ab816d230ff48c84bc17122431753d55d Gerrit-Change-Number: 59281 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: pcidev.c: Simplify by consolidating common logic
by Edward O'Callaghan (Code Review) 13 Nov '21

13 Nov '21

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/59281 ) Change subject: pcidev.c: Simplify by consolidating common logic ...................................................................... pcidev.c: Simplify by consolidating common logic BUG=none TEST=builds Change-Id: I0048fc6ab816d230ff48c84bc17122431753d55d Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M pcidev.c M programmer.h 2 files changed, 11 insertions(+), 30 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/81/59281/1 diff --git a/pcidev.c b/pcidev.c index 721243a..629304a 100644 --- a/pcidev.c +++ b/pcidev.c @@ -155,26 +155,26 @@ return (uintptr_t)addr; } -struct pci_dev *pcidev_scandev(struct pci_filter *filter) +struct pci_dev *pci_dev_find(uint16_t vendor, uint16_t device) { struct pci_dev *temp; + struct pci_filter filter; + + pci_filter_init(NULL, &filter); + filter.vendor = vendor; + filter.device = device; + for (temp = pacc->devices; temp; temp = temp->next) - if (pci_filter_match(filter, temp)) + if (pci_filter_match(&filter, temp)) return temp; return NULL; } struct pci_dev *pcidev_find_vendorclass(uint16_t vendor, uint16_t devclass) { - struct pci_dev *temp; - struct pci_filter filter; - uint16_t tmp2; - - pci_filter_init(NULL, &filter); - filter.vendor = vendor; - - temp = pcidev_scandev(&filter); + struct pci_dev *temp = pci_dev_find(vendor, 0); if (temp) { + uint16_t tmp2; /* Read PCI class */ tmp2 = pci_read_word(temp, PCI_CLASS_DEVICE); if (tmp2 == devclass) @@ -187,14 +187,7 @@ struct pci_dev *pcidev_card_find(uint16_t vendor, uint16_t device, uint16_t card_vendor, uint16_t card_device) { - struct pci_dev *temp; - struct pci_filter filter; - - pci_filter_init(NULL, &filter); - filter.vendor = vendor; - filter.device = device; - - temp = pcidev_scandev(&filter); + struct pci_dev *temp = pci_dev_find(vendor, device); if (temp) { if ((card_vendor == pci_read_word(temp, PCI_SUBSYSTEM_VENDOR_ID)) && (card_device == pci_read_word(temp, PCI_SUBSYSTEM_ID))) @@ -204,17 +197,6 @@ return NULL; } -struct pci_dev *pci_dev_find(uint16_t vendor, uint16_t device) -{ - struct pci_filter filter; - - pci_filter_init(NULL, &filter); - filter.vendor = vendor; - filter.device = device; - - return pcidev_scandev(&filter); -} - struct pci_dev *pcidev_getdev(struct pci_dev *dev) { #if !defined(OLD_PCI_GET_DEV) diff --git a/programmer.h b/programmer.h index 3b08f61..b9cd3de 100644 --- a/programmer.h +++ b/programmer.h @@ -124,7 +124,6 @@ int pci_init_common(void); uintptr_t pcidev_readbar(struct pci_dev *dev, int bar); struct pci_dev *pcidev_init(const struct dev_entry *devs, int bar); -struct pci_dev *pcidev_scandev(struct pci_filter *filter); struct pci_dev *pcidev_find_vendorclass(uint16_t vendor, uint16_t devclass); struct pci_dev *pcidev_getdev(struct pci_dev *dev); struct pci_dev *pcidev_card_find(uint16_t vendor, uint16_t device, -- To view, visit https://review.coreboot.org/c/flashrom/+/59281 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0048fc6ab816d230ff48c84bc17122431753d55d Gerrit-Change-Number: 59281 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: pcidev.c: Move pci_dev_find() from internal to canonical place
by Edward O'Callaghan (Code Review) 13 Nov '21

13 Nov '21

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/59280 ) Change subject: pcidev.c: Move pci_dev_find() from internal to canonical place ...................................................................... pcidev.c: Move pci_dev_find() from internal to canonical place BUG=none TEST=builds Change-Id: Ie21f87699481a84398ca4450b3f03548f0528191 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M internal.c M pcidev.c M programmer.h 3 files changed, 12 insertions(+), 14 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/80/59280/1 diff --git a/internal.c b/internal.c index 0fc150b..508906d 100644 --- a/internal.c +++ b/internal.c @@ -29,17 +29,6 @@ enum chipbustype internal_buses_supported = BUS_NONE; -struct pci_dev *pci_dev_find(uint16_t vendor, uint16_t device) -{ - struct pci_filter filter; - - pci_filter_init(NULL, &filter); - filter.vendor = vendor; - filter.device = device; - - return pcidev_scandev(&filter); -} - #if defined(__i386__) || defined(__x86_64__) void probe_superio(void) { diff --git a/pcidev.c b/pcidev.c index 9ecd042..721243a 100644 --- a/pcidev.c +++ b/pcidev.c @@ -204,6 +204,17 @@ return NULL; } +struct pci_dev *pci_dev_find(uint16_t vendor, uint16_t device) +{ + struct pci_filter filter; + + pci_filter_init(NULL, &filter); + filter.vendor = vendor; + filter.device = device; + + return pcidev_scandev(&filter); +} + struct pci_dev *pcidev_getdev(struct pci_dev *dev) { #if !defined(OLD_PCI_GET_DEV) diff --git a/programmer.h b/programmer.h index 35306ed..3b08f61 100644 --- a/programmer.h +++ b/programmer.h @@ -129,6 +129,7 @@ struct pci_dev *pcidev_getdev(struct pci_dev *dev); struct pci_dev *pcidev_card_find(uint16_t vendor, uint16_t device, uint16_t card_vendor, uint16_t card_device); +struct pci_dev *pci_dev_find(uint16_t vendor, uint16_t device); /* rpci_write_* are reversible writes. The original PCI config space register * contents will be restored on shutdown. * To clone the pci_dev instances internally, the `pacc` global @@ -271,9 +272,6 @@ #define SUPERIO_VENDOR_ITE 0x1 #define SUPERIO_VENDOR_WINBOND 0x2 #endif -#if NEED_PCI == 1 -struct pci_dev *pci_dev_find(uint16_t vendor, uint16_t device); -#endif int rget_io_perms(void); #if CONFIG_INTERNAL == 1 extern int is_laptop; -- To view, visit https://review.coreboot.org/c/flashrom/+/59280 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie21f87699481a84398ca4450b3f03548f0528191 Gerrit-Change-Number: 59280 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: pcidev.c: Move pci_card_find() from internal to canonical place
by Edward O'Callaghan (Code Review) 13 Nov '21

13 Nov '21

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/59279 ) Change subject: pcidev.c: Move pci_card_find() from internal to canonical place ...................................................................... pcidev.c: Move pci_card_find() from internal to canonical place BUG=none TEST=builds Change-Id: I026bfbecba114411728d4ad1ed8969b469fa7d2d Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M board_enable.c M internal.c M pcidev.c M programmer.h 4 files changed, 24 insertions(+), 24 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/59279/1 diff --git a/board_enable.c b/board_enable.c index d668cd8..b86cfe6 100644 --- a/board_enable.c +++ b/board_enable.c @@ -2635,14 +2635,14 @@ if (board->phase != phase) continue; - if (!pci_card_find(board->first_vendor, board->first_device, + if (!pcidev_card_find(board->first_vendor, board->first_device, board->first_card_vendor, board->first_card_device)) continue; if (board->second_vendor) { if (board->second_card_vendor) { - if (!pci_card_find(board->second_vendor, + if (!pcidev_card_find(board->second_vendor, board->second_device, board->second_card_vendor, board->second_card_device)) diff --git a/internal.c b/internal.c index 84ada73..0fc150b 100644 --- a/internal.c +++ b/internal.c @@ -40,26 +40,6 @@ return pcidev_scandev(&filter); } -struct pci_dev *pci_card_find(uint16_t vendor, uint16_t device, - uint16_t card_vendor, uint16_t card_device) -{ - struct pci_dev *temp; - struct pci_filter filter; - - pci_filter_init(NULL, &filter); - filter.vendor = vendor; - filter.device = device; - - temp = pcidev_scandev(&filter); - if (temp) { - if ((card_vendor == pci_read_word(temp, PCI_SUBSYSTEM_VENDOR_ID)) - && (card_device == pci_read_word(temp, PCI_SUBSYSTEM_ID))) - return temp; - } - - return NULL; -} - #if defined(__i386__) || defined(__x86_64__) void probe_superio(void) { diff --git a/pcidev.c b/pcidev.c index 2c1f6d7..9ecd042 100644 --- a/pcidev.c +++ b/pcidev.c @@ -184,6 +184,26 @@ return NULL; } +struct pci_dev *pcidev_card_find(uint16_t vendor, uint16_t device, + uint16_t card_vendor, uint16_t card_device) +{ + struct pci_dev *temp; + struct pci_filter filter; + + pci_filter_init(NULL, &filter); + filter.vendor = vendor; + filter.device = device; + + temp = pcidev_scandev(&filter); + if (temp) { + if ((card_vendor == pci_read_word(temp, PCI_SUBSYSTEM_VENDOR_ID)) + && (card_device == pci_read_word(temp, PCI_SUBSYSTEM_ID))) + return temp; + } + + return NULL; +} + struct pci_dev *pcidev_getdev(struct pci_dev *dev) { #if !defined(OLD_PCI_GET_DEV) diff --git a/programmer.h b/programmer.h index 87a8c22..35306ed 100644 --- a/programmer.h +++ b/programmer.h @@ -127,6 +127,8 @@ struct pci_dev *pcidev_scandev(struct pci_filter *filter); struct pci_dev *pcidev_find_vendorclass(uint16_t vendor, uint16_t devclass); struct pci_dev *pcidev_getdev(struct pci_dev *dev); +struct pci_dev *pcidev_card_find(uint16_t vendor, uint16_t device, + uint16_t card_vendor, uint16_t card_device); /* rpci_write_* are reversible writes. The original PCI config space register * contents will be restored on shutdown. * To clone the pci_dev instances internally, the `pacc` global @@ -271,8 +273,6 @@ #endif #if NEED_PCI == 1 struct pci_dev *pci_dev_find(uint16_t vendor, uint16_t device); -struct pci_dev *pci_card_find(uint16_t vendor, uint16_t device, - uint16_t card_vendor, uint16_t card_device); #endif int rget_io_perms(void); #if CONFIG_INTERNAL == 1 -- To view, visit https://review.coreboot.org/c/flashrom/+/59279 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I026bfbecba114411728d4ad1ed8969b469fa7d2d Gerrit-Change-Number: 59279 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: programmer.h: Make pacc global state static local
by Edward O'Callaghan (Code Review) 13 Nov '21

13 Nov '21

Attention is currently required from: Nico Huber, Anastasia Klimchuk, Peter Marheine. Hello build bot (Jenkins), Nico Huber, Anastasia Klimchuk, Peter Marheine, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/59278 to look at the new patch set (#3). Change subject: programmer.h: Make pacc global state static local ...................................................................... programmer.h: Make pacc global state static local chipset_enable.c has some fragile pci logic to deal with Intel's complexity of finding the SPI controller on the ICH. Here we provide function to allow pushing the scope of pci access global state inwards into a static local of just pcidev. While by no means perfection, this does at least make it easier to reason about 'pacc' control flow now. BUG=none TEST=builds Change-Id: I3bea1b93a7ca81dd42a64eec1e49a10f1fbda850 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M chipset_enable.c M pcidev.c M programmer.h 3 files changed, 12 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/78/59278/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/59278 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3bea1b93a7ca81dd42a64eec1e49a10f1fbda850 Gerrit-Change-Number: 59278 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: programmer.h: Make pacc global state static local
by Edward O'Callaghan (Code Review) 13 Nov '21

13 Nov '21

Attention is currently required from: Nico Huber, Anastasia Klimchuk, Peter Marheine. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/59278 ) Change subject: programmer.h: Make pacc global state static local ...................................................................... Patch Set 2: This change is ready for review. -- To view, visit https://review.coreboot.org/c/flashrom/+/59278 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3bea1b93a7ca81dd42a64eec1e49a10f1fbda850 Gerrit-Change-Number: 59278 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Sat, 13 Nov 2021 06:43:55 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit November 2021