Change in flashrom[master]: sb600spi.c: Remove sb600_spibar out of global state
Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/48064 ) Change subject: sb600spi.c: Remove sb600_spibar out of global state ...................................................................... sb600spi.c: Remove sb600_spibar out of global state Make helper functions pure by taking the spibar as an argument and put the base address pointer within the internal spi master state structure to make the spi driver re-enterent. BUG=none BRANCH=none TEST=builds Change-Id: I2e692b230a77f3902d91d0ad36db0d668b7e9ffa Signed-off-by: Edward O'Callaghan <quasisec@google.com> --- M sb600spi.c 1 file changed, 34 insertions(+), 31 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/64/48064/1 diff --git a/sb600spi.c b/sb600spi.c index df1184a..a1089c5 100644 --- a/sb600spi.c +++ b/sb600spi.c @@ -40,7 +40,6 @@ *}; */ -static uint8_t *sb600_spibar = NULL; enum amd_chipset { CHIPSET_AMD_UNKNOWN, CHIPSET_SB6XX, @@ -57,6 +56,7 @@ struct sb600spi_data { bool map_flash_req; + uint8_t *sb600_spibar; }; static int find_smbus_dev_rev(uint16_t vendor, uint16_t device) @@ -140,7 +140,7 @@ return CHIPSET_AMD_UNKNOWN; } -static void reset_internal_fifo_pointer(void) +static void reset_internal_fifo_pointer(uint8_t *sb600_spibar) { mmio_writeb(mmio_readb(sb600_spibar + 2) | 0x10, sb600_spibar + 2); @@ -149,7 +149,7 @@ msg_pspew("reset\n"); } -static int compare_internal_fifo_pointer(uint8_t want) +static int compare_internal_fifo_pointer(uint8_t *sb600_spibar, uint8_t want) { uint8_t have = mmio_readb(sb600_spibar + 0xd) & 0x07; want %= FIFO_SIZE_OLD; @@ -183,7 +183,7 @@ return 0; } -static void execute_command(void) +static void execute_command(uint8_t *sb600_spibar) { msg_pspew("Executing... "); mmio_writeb(mmio_readb(sb600_spibar + 2) | 1, sb600_spibar + 2); @@ -197,11 +197,12 @@ const unsigned char *writearr, unsigned char *readarr) { + struct sb600spi_data * data = (struct sb600spi_data *)flash->mst->spi.data; /* First byte is cmd which can not be sent through the FIFO. */ unsigned char cmd = *writearr++; writecnt--; msg_pspew("%s, cmd=0x%02x, writecnt=%d, readcnt=%d\n", __func__, cmd, writecnt, readcnt); - mmio_writeb(cmd, sb600_spibar + 0); + mmio_writeb(cmd, data->sb600_spibar + 0); int ret = check_readwritecnt(flash, writecnt, readcnt); if (ret != 0) @@ -215,26 +216,26 @@ */ unsigned int readoffby1 = (writecnt > 0) ? 0 : 1; uint8_t readwrite = (readcnt + readoffby1) << 4 | (writecnt); - mmio_writeb(readwrite, sb600_spibar + 1); + mmio_writeb(readwrite, data->sb600_spibar + 1); - reset_internal_fifo_pointer(); + reset_internal_fifo_pointer(data->sb600_spibar); msg_pspew("Filling FIFO: "); unsigned int count; for (count = 0; count < writecnt; count++) { msg_pspew("[%02x]", writearr[count]); - mmio_writeb(writearr[count], sb600_spibar + 0xC); + mmio_writeb(writearr[count], data->sb600_spibar + 0xC); } msg_pspew("\n"); - if (compare_internal_fifo_pointer(writecnt)) + if (compare_internal_fifo_pointer(data->sb600_spibar, writecnt)) return SPI_PROGRAMMER_ERROR; /* * We should send the data in sequence, which means we need to reset * the FIFO pointer to the first byte we want to send. */ - reset_internal_fifo_pointer(); - execute_command(); - if (compare_internal_fifo_pointer(writecnt + readcnt)) + reset_internal_fifo_pointer(data->sb600_spibar); + execute_command(data->sb600_spibar); + if (compare_internal_fifo_pointer(data->sb600_spibar, writecnt + readcnt)) return SPI_PROGRAMMER_ERROR; /* @@ -248,27 +249,27 @@ * the opcode, the FIFO already stores the response from the chip. * Usually, the chip will respond with 0x00 or 0xff. */ - reset_internal_fifo_pointer(); + reset_internal_fifo_pointer(data->sb600_spibar); /* Skip the bytes we sent. */ msg_pspew("Skipping: "); for (count = 0; count < writecnt; count++) { - msg_pspew("[%02x]", mmio_readb(sb600_spibar + 0xC)); + msg_pspew("[%02x]", mmio_readb(data->sb600_spibar + 0xC)); } msg_pspew("\n"); - if (compare_internal_fifo_pointer(writecnt)) + if (compare_internal_fifo_pointer(data->sb600_spibar, writecnt)) return SPI_PROGRAMMER_ERROR; msg_pspew("Reading FIFO: "); for (count = 0; count < readcnt; count++) { - readarr[count] = mmio_readb(sb600_spibar + 0xC); + readarr[count] = mmio_readb(data->sb600_spibar + 0xC); msg_pspew("[%02x]", readarr[count]); } msg_pspew("\n"); - if (compare_internal_fifo_pointer(writecnt+readcnt)) + if (compare_internal_fifo_pointer(data->sb600_spibar, writecnt+readcnt)) return SPI_PROGRAMMER_ERROR; - if (mmio_readb(sb600_spibar + 1) != readwrite) { + if (mmio_readb(data->sb600_spibar + 1) != readwrite) { msg_perr("Unexpected change in AMD SPI read/write count!\n"); msg_perr("Something else is accessing the flash chip and causes random corruption.\n" "Please stop all applications and drivers and IPMI which access the flash chip.\n"); @@ -283,33 +284,34 @@ const unsigned char *writearr, unsigned char *readarr) { + struct sb600spi_data * data = (struct sb600spi_data *)flash->mst->spi.data; /* First byte is cmd which can not be sent through the buffer. */ unsigned char cmd = *writearr++; writecnt--; msg_pspew("%s, cmd=0x%02x, writecnt=%d, readcnt=%d\n", __func__, cmd, writecnt, readcnt); - mmio_writeb(cmd, sb600_spibar + 0); + mmio_writeb(cmd, data->sb600_spibar + 0); int ret = check_readwritecnt(flash, writecnt, readcnt); if (ret != 0) return ret; /* Use the extended TxByteCount and RxByteCount registers. */ - mmio_writeb(writecnt, sb600_spibar + 0x48); - mmio_writeb(readcnt, sb600_spibar + 0x4b); + mmio_writeb(writecnt, data->sb600_spibar + 0x48); + mmio_writeb(readcnt, data->sb600_spibar + 0x4b); msg_pspew("Filling buffer: "); unsigned int count; for (count = 0; count < writecnt; count++) { msg_pspew("[%02x]", writearr[count]); - mmio_writeb(writearr[count], sb600_spibar + 0x80 + count); + mmio_writeb(writearr[count], data->sb600_spibar + 0x80 + count); } msg_pspew("\n"); - execute_command(); + execute_command(data->sb600_spibar); msg_pspew("Reading buffer: "); for (count = 0; count < readcnt; count++) { - readarr[count] = mmio_readb(sb600_spibar + 0x80 + (writecnt + count) % FIFO_SIZE_YANGTZE); + readarr[count] = mmio_readb(data->sb600_spibar + 0x80 + (writecnt + count) % FIFO_SIZE_YANGTZE); msg_pspew("[%02x]", readarr[count]); } msg_pspew("\n"); @@ -344,7 +346,8 @@ "Fast Read", }; -static int set_speed(struct pci_dev *dev, enum amd_chipset amd_gen, uint8_t speed) +static int set_speed(struct pci_dev *dev, uint8_t *sb600_spibar, + enum amd_chipset amd_gen, uint8_t speed) { bool success = false; @@ -367,7 +370,7 @@ return 0; } -static int set_mode(struct pci_dev *dev, uint8_t mode) +static int set_mode(struct pci_dev *dev, uint8_t *sb600_spibar, uint8_t mode) { msg_pdbg("Setting SPI read mode to %s (%i)... ", spireadmodes[mode], mode); uint32_t tmp = mmio_readl(sb600_spibar + 0x00); @@ -382,7 +385,7 @@ return 0; } -static int handle_speed(struct pci_dev *dev, enum amd_chipset amd_gen) +static int handle_speed(struct pci_dev *dev, uint8_t *sb600_spibar, enum amd_chipset amd_gen) { uint32_t tmp; int16_t spispeed_idx = -1; @@ -450,7 +453,7 @@ msg_perr("Warning: spireadmode not set, " "leaving spireadmode unchanged."); } - else if (set_mode(dev, spireadmode_idx) != 0) { + else if (set_mode(dev, sb600_spibar, spireadmode_idx) != 0) { return 1; } @@ -493,7 +496,7 @@ msg_perr("Warning: spispeed not set, leaving spispeed unchanged."); return 0; } - return set_speed(dev, amd_gen, spispeed_idx); + return set_speed(dev, sb600_spibar, amd_gen, spispeed_idx); } static int handle_imc(struct pci_dev *dev, enum amd_chipset amd_gen) @@ -604,7 +607,7 @@ return 0; /* Physical memory has to be mapped at page (4k) boundaries. */ - sb600_spibar = rphysmap("SB600 SPI registers", tmp & 0xfffff000, 0x1000); + uint8_t *sb600_spibar = rphysmap("SB600 SPI registers", tmp & 0xfffff000, 0x1000); if (sb600_spibar == ERROR_PTR) return ERROR_FATAL; @@ -747,7 +750,7 @@ return 0; } - if (handle_speed(dev, amd_gen) != 0) + if (handle_speed(dev, sb600_spibar, amd_gen) != 0) return ERROR_FATAL; if (handle_imc(dev, amd_gen) != 0) -- To view, visit https://review.coreboot.org/c/flashrom/+/48064 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2e692b230a77f3902d91d0ad36db0d668b7e9ffa Gerrit-Change-Number: 48064 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec@chromium.org> Gerrit-MessageType: newchange
Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/48064 to look at the new patch set (#2). Change subject: sb600spi.c: Remove sb600_spibar out of global state ...................................................................... sb600spi.c: Remove sb600_spibar out of global state Make helper functions pure by taking the spibar as an argument and put the base address pointer within the internal spi master state structure to make the spi driver re-enterent. BUG=none BRANCH=none TEST=builds Change-Id: I2e692b230a77f3902d91d0ad36db0d668b7e9ffa Signed-off-by: Edward O'Callaghan <quasisec@google.com> --- M sb600spi.c 1 file changed, 35 insertions(+), 31 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/64/48064/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/48064 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2e692b230a77f3902d91d0ad36db0d668b7e9ffa Gerrit-Change-Number: 48064 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Hello Daniel Kurtz, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/48064 to look at the new patch set (#3). Change subject: sb600spi.c: Remove sb600_spibar out of global state ...................................................................... sb600spi.c: Remove sb600_spibar out of global state Make helper functions pure by taking the spibar as an argument and put the base address pointer within the internal spi master state structure to make the spi driver re-enterent. BUG=none BRANCH=none TEST=builds Change-Id: I2e692b230a77f3902d91d0ad36db0d668b7e9ffa Signed-off-by: Edward O'Callaghan <quasisec@google.com> --- M sb600spi.c 1 file changed, 35 insertions(+), 31 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/64/48064/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/48064 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2e692b230a77f3902d91d0ad36db0d668b7e9ffa Gerrit-Change-Number: 48064 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: Daniel Kurtz <djkurtz@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Hello Daniel Kurtz, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/48064 to look at the new patch set (#6). Change subject: sb600spi.c: Remove sb600_spibar out of global state ...................................................................... sb600spi.c: Remove sb600_spibar out of global state Make helper functions pure by taking the spibar as an argument and put the base address pointer within the internal spi master state structure to make the spi driver re-enterent. BUG=none BRANCH=none TEST=builds Change-Id: I2e692b230a77f3902d91d0ad36db0d668b7e9ffa Signed-off-by: Edward O'Callaghan <quasisec@google.com> --- M sb600spi.c 1 file changed, 35 insertions(+), 31 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/64/48064/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/48064 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2e692b230a77f3902d91d0ad36db0d668b7e9ffa Gerrit-Change-Number: 48064 Gerrit-PatchSet: 6 Gerrit-Owner: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: Daniel Kurtz <djkurtz@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Hello Daniel Kurtz, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/48064 to look at the new patch set (#7). Change subject: sb600spi.c: Remove sb600_spibar out of global state ...................................................................... sb600spi.c: Remove sb600_spibar out of global state Make helper functions pure by taking the spibar as an argument and put the base address pointer within the internal spi master state structure to make the spi driver re-enterent. BUG=none BRANCH=none TEST=builds Change-Id: I2e692b230a77f3902d91d0ad36db0d668b7e9ffa Signed-off-by: Edward O'Callaghan <quasisec@google.com> --- M sb600spi.c 1 file changed, 35 insertions(+), 31 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/64/48064/7 -- To view, visit https://review.coreboot.org/c/flashrom/+/48064 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2e692b230a77f3902d91d0ad36db0d668b7e9ffa Gerrit-Change-Number: 48064 Gerrit-PatchSet: 7 Gerrit-Owner: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: Daniel Kurtz <djkurtz@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48064 ) Change subject: sb600spi.c: Remove sb600_spibar out of global state ...................................................................... Patch Set 8: Code-Review+2 (2 comments) https://review.coreboot.org/c/flashrom/+/48064/8/sb600spi.c File sb600spi.c: https://review.coreboot.org/c/flashrom/+/48064/8/sb600spi.c@209 PS8, Line 209: struct sb600spi_data * data It looks like struct sb600spi_data *data is the more common style. If it's unlikely for more fields added/used, it may be simpler to pull out sb600_spibar as a local instead. https://review.coreboot.org/c/flashrom/+/48064/8/sb600spi.c@296 PS8, Line 296: struct sb600spi_data * data = (struct sb600spi_data *)flash->mst->spi.data; Here too. -- To view, visit https://review.coreboot.org/c/flashrom/+/48064 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2e692b230a77f3902d91d0ad36db0d668b7e9ffa Gerrit-Change-Number: 48064 Gerrit-PatchSet: 8 Gerrit-Owner: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: Daniel Kurtz <djkurtz@chromium.org> Gerrit-Reviewer: Sam McNally <sammc@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Wed, 16 Dec 2020 05:18:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Edward O'Callaghan has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/48064 ) Change subject: sb600spi.c: Remove sb600_spibar out of global state ...................................................................... Abandoned Done in https://review.coreboot.org/c/flashrom/+/54712 -- To view, visit https://review.coreboot.org/c/flashrom/+/48064 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2e692b230a77f3902d91d0ad36db0d668b7e9ffa Gerrit-Change-Number: 48064 Gerrit-PatchSet: 8 Gerrit-Owner: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: Daniel Kurtz <djkurtz@chromium.org> Gerrit-Reviewer: Sam McNally <sammc@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: abandon
participants (2)
-
Edward O'Callaghan (Code Review) -
Sam McNally (Code Review)