May 2021 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: Add support for TerribleFire TF530 SPI controller
by Angel Pons (Code Review) 26 May '21

26 May '21

Attention is currently required from: Stefan Reinauer. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54944 ) Change subject: Add support for TerribleFire TF530 SPI controller ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/54944/comment/4ab3b33b_914a7f37 PS1, Line 8: I'd greatly appreciate if you could elaborate a bit about this programmer. Where can one find this SPI controller? Is it part of a chipset? Was this code tested? -- To view, visit https://review.coreboot.org/c/flashrom/+/54944 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I109eec4dec89650bb80d3d1da905d33f65f60f5a Gerrit-Change-Number: 54944 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Wed, 26 May 2021 09:06:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add support for TerribleFire TF530 SPI controller
by Angel Pons (Code Review) 26 May '21

26 May '21

Attention is currently required from: Stefan Reinauer. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54944 ) Change subject: Add support for TerribleFire TF530 SPI controller ...................................................................... Patch Set 1: Code-Review+1 (11 comments) File tf530_spi.c: https://review.coreboot.org/c/flashrom/+/54944/comment/8bf26232_729fe20c PS1, Line 17: * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA I don't think we need this paragraph https://review.coreboot.org/c/flashrom/+/54944/comment/f761131f_87369065 PS1, Line 20: #if CONFIG_TF530_SPI == 1 Is this really necessary? I know other files have these guards, but I don't think they're actually useful. https://review.coreboot.org/c/flashrom/+/54944/comment/6e02616e_fa9d92d9 PS1, Line 62: static struct ConfigDev *cd = NULL; There's an ongoing effort to remove global state from programmers. See changes to `digilent_spi.c` in CB:54012 and CB:54067 as an example of what should be changed here. https://review.coreboot.org/c/flashrom/+/54944/comment/3af232c9_df3b790d PS1, Line 64: #define TF53x_CTRL_CS0 1 : #define TF53x_CTRL_CS1 2 : #define TF53x_CTRL_BUSY 4 : #define TF53x_CTRL_CS2 8 Looks like these are bits within a register. Why not rewrite them as shifts? #define TF53x_CTRL_CS0 (1 << 0) #define TF53x_CTRL_CS1 (1 << 1) #define TF53x_CTRL_BUSY (1 << 2) #define TF53x_CTRL_CS2 (1 << 3) https://review.coreboot.org/c/flashrom/+/54944/comment/1b4e7db9_bf3e29a2 PS1, Line 71: uint8_t busy = 0; : : while (busy == 0) { : busy = port->ctrl & TF53x_CTRL_BUSY; : } nit: I'd remove the `busy` variable, the name is confusing (it's active-low): do {} while ((port->ctrl & TF53x_CTRL_BUSY) == 0); Same below. https://review.coreboot.org/c/flashrom/+/54944/comment/793764a9_450ff517 PS1, Line 106: return 0; Don't we need to release `struct ConfigDev *cd` or similar? https://review.coreboot.org/c/flashrom/+/54944/comment/57caca48_0d5a32ba PS1, Line 119: unsigned int Support for for-loop initial declarations was introduced with C99. Some old compilers that do not use C99 mode by default may complain about it. It's not a big deal in this particular case because this programmer is tied to a particular OS, but it's something to keep in mind. https://review.coreboot.org/c/flashrom/+/54944/comment/4a47284a_29851863 PS1, Line 125: uint8_t volatile nit: `volatile uint8_t` Also, would this work? (volatile uint8_t)spi_recv_byte(port); https://review.coreboot.org/c/flashrom/+/54944/comment/b9a142c6_71e09fa3 PS1, Line 149: : struct Library *ExpansionBase = NULL; : : if ((ExpansionBase = (struct Library *) : OpenLibrary((unsigned char *)"expansion.library", 0L)) == NULL) { Moving the assignment out of the if-block's condition should be easier to read: struct Library *ExpansionBase = (struct Library *) OpenLibrary((unsigned char *)"expansion.library", 0L); if ((ExpansionBase == NULL) { return 0; } Also, are the type casts really necessary? If not, please drop them. https://review.coreboot.org/c/flashrom/+/54944/comment/afa638da_69ab5f68 PS1, Line 154: 0 Why return 0? https://review.coreboot.org/c/flashrom/+/54944/comment/dda622a6_9d03416c PS1, Line 169: register_spi_master(&spi_master_tf53x, NULL); This function returns something. Why not propagate the return value? return register_spi_master(&spi_master_tf53x, NULL); -- To view, visit https://review.coreboot.org/c/flashrom/+/54944 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I109eec4dec89650bb80d3d1da905d33f65f60f5a Gerrit-Change-Number: 54944 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Wed, 26 May 2021 08:57:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add AmigaOS suppport to flashrom
by Angel Pons (Code Review) 26 May '21

26 May '21

Attention is currently required from: Paul Menzel, Stefan Reinauer. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54943 ) Change subject: Add AmigaOS suppport to flashrom ...................................................................... Patch Set 1: Code-Review+1 (7 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/54943/comment/1541c8da_5b73dde0 PS1, Line 8: > How can this be tested? In QEMU? I'm not sure if it's doable (licensing issues, maybe?), but it would be nice to add a target to util/manibuilder to build-test for AmigaOS. File Makefile: https://review.coreboot.org/c/flashrom/+/54943/comment/40641b8f_96cba8e5 PS1, Line 141: Bus Pirate, Serprog and PonyProg are not supported under AmigaOS (missing serial support). I'd rather not list all the programmers in the comment. Instead, I'd say something like this: # AmigaOS is missing serial support, disable incompatible programmers https://review.coreboot.org/c/flashrom/+/54943/comment/0f1aae50_7b64caf7 PS1, Line 157: # Digilent SPI, Dediprog, Developerbox, USB-Blaster, PICkit2, CH341A and FT2232 are not supported under AmigaOS (missing USB support). Same story here: # AmigaOS is missing USB support, disable incompatible programmers https://review.coreboot.org/c/flashrom/+/54943/comment/b9460cae_60a1f251 PS1, Line 213: DOS AmigaOS File cli_classic.c: https://review.coreboot.org/c/flashrom/+/54943/comment/b2517d92_28b543ab PS1, Line 162: *argv[] I guess this type is what makes AmigaOS unhappy? File flash.h: https://review.coreboot.org/c/flashrom/+/54943/comment/51d11540_5010b230 PS1, Line 55: wrong Out of curiosity, what does `PRIxPTR` get defined as? https://review.coreboot.org/c/flashrom/+/54943/comment/9dd92f6b_c2996753 PS1, Line 56: __AMIGA__ Why not use `IS_AMIGA` here? -- To view, visit https://review.coreboot.org/c/flashrom/+/54943 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3e4a3885f88313aad019454fdbdf4be176b35330 Gerrit-Change-Number: 54943 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Wed, 26 May 2021 08:26:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: mstarddc_spi.c: Drop mstarddc_ prefix for spi data struct members
by Edward O'Callaghan (Code Review) 26 May '21

26 May '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/54034 ) Change subject: mstarddc_spi.c: Drop mstarddc_ prefix for spi data struct members ...................................................................... mstarddc_spi.c: Drop mstarddc_ prefix for spi data struct members The name of the struct already contains mstarddc_ prefix, so prefix doesn't need to be repeated in members names. BUG=b:185191942 TEST=builds Change-Id: Ic04b14ce502917ae3b959cf2acf23d58b8752d47 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/54034 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M mstarddc_spi.c 1 file changed, 16 insertions(+), 15 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved diff --git a/mstarddc_spi.c b/mstarddc_spi.c index da9f152..0f638df 100644 --- a/mstarddc_spi.c +++ b/mstarddc_spi.c @@ -32,9 +32,9 @@ #include "spi.h" struct mstarddc_spi_data { - int mstarddc_fd; - int mstarddc_addr; - int mstarddc_doreset; + int fd; + int addr; + int doreset; }; // MSTAR DDC Commands @@ -49,9 +49,9 @@ struct mstarddc_spi_data *mstarddc_data = data; // Reset, disables ISP mode - if (mstarddc_data->mstarddc_doreset == 1) { + if (mstarddc_data->doreset == 1) { uint8_t cmd = MSTARDDC_SPI_RESET; - if (write(mstarddc_data->mstarddc_fd, &cmd, 1) < 0) { + if (write(mstarddc_data->fd, &cmd, 1) < 0) { msg_perr("Error sending reset command: errno %d.\n", errno); return -1; @@ -62,7 +62,7 @@ "or an error occurred.\n"); } - if (close(mstarddc_data->mstarddc_fd) < 0) { + if (close(mstarddc_data->fd) < 0) { msg_perr("Error closing device: errno %d.\n", errno); return -1; } @@ -89,7 +89,7 @@ if (!ret && writecnt) { cmd[0] = MSTARDDC_SPI_WRITE; memcpy(cmd + 1, writearr, writecnt); - if (write(mstarddc_data->mstarddc_fd, cmd, writecnt + 1) < 0) { + if (write(mstarddc_data->fd, cmd, writecnt + 1) < 0) { msg_perr("Error sending write command: errno %d.\n", errno); ret = -1; @@ -103,16 +103,16 @@ cmd[0] = MSTARDDC_SPI_READ; i2c_data.nmsgs = 2; i2c_data.msgs = msg; - i2c_data.msgs[0].addr = mstarddc_data->mstarddc_addr; + i2c_data.msgs[0].addr = mstarddc_data->addr; i2c_data.msgs[0].len = 1; i2c_data.msgs[0].flags = 0; i2c_data.msgs[0].buf = cmd; - i2c_data.msgs[1].addr = mstarddc_data->mstarddc_addr; + i2c_data.msgs[1].addr = mstarddc_data->addr; i2c_data.msgs[1].len = readcnt; i2c_data.msgs[1].flags = I2C_M_RD; i2c_data.msgs[1].buf = readarr; - if (ioctl(mstarddc_data->mstarddc_fd, I2C_RDWR, &i2c_data) < 0) { + if (ioctl(mstarddc_data->fd, I2C_RDWR, &i2c_data) < 0) { msg_perr("Error sending read command: errno %d.\n", errno); ret = -1; @@ -121,7 +121,7 @@ if (!ret && (writecnt || readcnt)) { cmd[0] = MSTARDDC_SPI_END; - if (write(mstarddc_data->mstarddc_fd, cmd, 1) < 0) { + if (write(mstarddc_data->fd, cmd, 1) < 0) { msg_perr("Error sending end command: errno %d.\n", errno); ret = -1; @@ -131,7 +131,7 @@ /* Do not reset if something went wrong, as it might prevent from * retrying flashing. */ if (ret != 0) - mstarddc_data->mstarddc_doreset = 0; + mstarddc_data->doreset = 0; if (cmd) free(cmd); @@ -235,9 +235,10 @@ ret = -1; goto out; } - mstarddc_data->mstarddc_fd = mstarddc_fd; - mstarddc_data->mstarddc_addr = mstarddc_addr; - mstarddc_data->mstarddc_doreset = mstarddc_doreset; + + mstarddc_data->fd = mstarddc_fd; + mstarddc_data->addr = mstarddc_addr; + mstarddc_data->doreset = mstarddc_doreset; // Register shutdown function register_shutdown(mstarddc_spi_shutdown, mstarddc_data); -- To view, visit https://review.coreboot.org/c/flashrom/+/54034 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic04b14ce502917ae3b959cf2acf23d58b8752d47 Gerrit-Change-Number: 54034 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: mstarddc_spi.c: Refactor singleton states into reentrant pattern
by Edward O'Callaghan (Code Review) 26 May '21

26 May '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/54033 ) Change subject: mstarddc_spi.c: Refactor singleton states into reentrant pattern ...................................................................... mstarddc_spi.c: Refactor singleton states into reentrant pattern Move global singleton states into a struct and store within the spi_master data field for the life-time of the driver. This is one of the steps on the way to move spi_master data memory management behind the initialisation API, for more context see other patches under the same topic "register_master_api". BUG=b:185191942 TEST=builds Change-Id: Ia0e5f6879bfbfac591a40119ee3e0942a5cbc4b9 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/54033 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M mstarddc_spi.c 1 file changed, 38 insertions(+), 14 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved diff --git a/mstarddc_spi.c b/mstarddc_spi.c index 32b77cd..da9f152 100644 --- a/mstarddc_spi.c +++ b/mstarddc_spi.c @@ -31,9 +31,11 @@ #include "programmer.h" #include "spi.h" -static int mstarddc_fd; -static int mstarddc_addr; -static int mstarddc_doreset = 1; +struct mstarddc_spi_data { + int mstarddc_fd; + int mstarddc_addr; + int mstarddc_doreset; +}; // MSTAR DDC Commands #define MSTARDDC_SPI_WRITE 0x10 @@ -44,10 +46,12 @@ /* Returns 0 upon success, a negative number upon errors. */ static int mstarddc_spi_shutdown(void *data) { + struct mstarddc_spi_data *mstarddc_data = data; + // Reset, disables ISP mode - if (mstarddc_doreset == 1) { + if (mstarddc_data->mstarddc_doreset == 1) { uint8_t cmd = MSTARDDC_SPI_RESET; - if (write(mstarddc_fd, &cmd, 1) < 0) { + if (write(mstarddc_data->mstarddc_fd, &cmd, 1) < 0) { msg_perr("Error sending reset command: errno %d.\n", errno); return -1; @@ -58,10 +62,12 @@ "or an error occurred.\n"); } - if (close(mstarddc_fd) < 0) { + if (close(mstarddc_data->mstarddc_fd) < 0) { msg_perr("Error closing device: errno %d.\n", errno); return -1; } + + free(data); return 0; } @@ -72,6 +78,7 @@ const unsigned char *writearr, unsigned char *readarr) { + struct mstarddc_spi_data *mstarddc_data = flash->mst->spi.data; int ret = 0; uint8_t *cmd = malloc((writecnt + 1) * sizeof(uint8_t)); if (cmd == NULL) { @@ -82,7 +89,7 @@ if (!ret && writecnt) { cmd[0] = MSTARDDC_SPI_WRITE; memcpy(cmd + 1, writearr, writecnt); - if (write(mstarddc_fd, cmd, writecnt + 1) < 0) { + if (write(mstarddc_data->mstarddc_fd, cmd, writecnt + 1) < 0) { msg_perr("Error sending write command: errno %d.\n", errno); ret = -1; @@ -96,16 +103,16 @@ cmd[0] = MSTARDDC_SPI_READ; i2c_data.nmsgs = 2; i2c_data.msgs = msg; - i2c_data.msgs[0].addr = mstarddc_addr; + i2c_data.msgs[0].addr = mstarddc_data->mstarddc_addr; i2c_data.msgs[0].len = 1; i2c_data.msgs[0].flags = 0; i2c_data.msgs[0].buf = cmd; - i2c_data.msgs[1].addr = mstarddc_addr; + i2c_data.msgs[1].addr = mstarddc_data->mstarddc_addr; i2c_data.msgs[1].len = readcnt; i2c_data.msgs[1].flags = I2C_M_RD; i2c_data.msgs[1].buf = readarr; - if (ioctl(mstarddc_fd, I2C_RDWR, &i2c_data) < 0) { + if (ioctl(mstarddc_data->mstarddc_fd, I2C_RDWR, &i2c_data) < 0) { msg_perr("Error sending read command: errno %d.\n", errno); ret = -1; @@ -114,7 +121,7 @@ if (!ret && (writecnt || readcnt)) { cmd[0] = MSTARDDC_SPI_END; - if (write(mstarddc_fd, cmd, 1) < 0) { + if (write(mstarddc_data->mstarddc_fd, cmd, 1) < 0) { msg_perr("Error sending end command: errno %d.\n", errno); ret = -1; @@ -124,7 +131,7 @@ /* Do not reset if something went wrong, as it might prevent from * retrying flashing. */ if (ret != 0) - mstarddc_doreset = 0; + mstarddc_data->mstarddc_doreset = 0; if (cmd) free(cmd); @@ -146,6 +153,10 @@ int mstarddc_spi_init(void) { int ret = 0; + int mstarddc_fd = -1; + int mstarddc_addr; + int mstarddc_doreset = 1; + struct mstarddc_spi_data *mstarddc_data; // Get device, address from command-line char *i2c_device = extract_programmer_param("dev"); @@ -217,13 +228,26 @@ goto out; } } + + mstarddc_data = calloc(1, sizeof(*mstarddc_data)); + if (!mstarddc_data) { + msg_perr("Unable to allocate space for SPI master data\n"); + ret = -1; + goto out; + } + mstarddc_data->mstarddc_fd = mstarddc_fd; + mstarddc_data->mstarddc_addr = mstarddc_addr; + mstarddc_data->mstarddc_doreset = mstarddc_doreset; + // Register shutdown function - register_shutdown(mstarddc_spi_shutdown, NULL); + register_shutdown(mstarddc_spi_shutdown, mstarddc_data); // Register programmer - register_spi_master(&spi_master_mstarddc, NULL); + register_spi_master(&spi_master_mstarddc, mstarddc_data); out: free(i2c_device); + if (ret && (mstarddc_fd >= 0)) + close(mstarddc_fd); return ret; } -- To view, visit https://review.coreboot.org/c/flashrom/+/54033 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia0e5f6879bfbfac591a40119ee3e0942a5cbc4b9 Gerrit-Change-Number: 54033 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: Add support for TerribleFire TF530 SPI controller
by Paul Menzel (Code Review) 26 May '21

26 May '21

Attention is currently required from: Stefan Reinauer. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54944 ) Change subject: Add support for TerribleFire TF530 SPI controller ...................................................................... Patch Set 1: (1 comment) File tf530_spi.c: https://review.coreboot.org/c/flashrom/+/54944/comment/6a3ad7e9_2d7b77d4 PS1, Line 4: * Copyright (C) 2019 Steven J Leary This differs from the commit author data. -- To view, visit https://review.coreboot.org/c/flashrom/+/54944 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I109eec4dec89650bb80d3d1da905d33f65f60f5a Gerrit-Change-Number: 54944 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Wed, 26 May 2021 06:20:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add AmigaOS suppport to flashrom
by Paul Menzel (Code Review) 26 May '21

26 May '21

Attention is currently required from: Stefan Reinauer. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54943 ) Change subject: Add AmigaOS suppport to flashrom ...................................................................... Patch Set 1: Code-Review+1 (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/54943/comment/8d4dbf0c_a782cf12 PS1, Line 8: How can this be tested? In QEMU? Patchset: PS1: Had to look twice to verify I am not misreading. Very nice! -- To view, visit https://review.coreboot.org/c/flashrom/+/54943 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3e4a3885f88313aad019454fdbdf4be176b35330 Gerrit-Change-Number: 54943 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Comment-Date: Wed, 26 May 2021 06:19:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: sb600spi.c: Make use of new register_spi_master() API
by Anastasia Klimchuk (Code Review) 26 May '21

26 May '21

Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54713 ) Change subject: sb600spi.c: Make use of new register_spi_master() API ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/54713/comment/b81acc59_bfc70c81 PS1, Line 13: TEST=builds > Ack I tested on AMD device: probe, read, write, and then rebooted device. All good! Sorry for messaging after this patch is already merged, but better late than never :) -- To view, visit https://review.coreboot.org/c/flashrom/+/54713 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id555dc5e125309883a816e00afb26d7141fd870d Gerrit-Change-Number: 54713 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Wed, 26 May 2021 05:03:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: Add support for TerribleFire TF530 SPI controller
by Stefan Reinauer (Code Review) 26 May '21

26 May '21

Stefan Reinauer has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/54944 ) Change subject: Add support for TerribleFire TF530 SPI controller ...................................................................... Add support for TerribleFire TF530 SPI controller Change-Id: I109eec4dec89650bb80d3d1da905d33f65f60f5a Signed-off-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> --- M Makefile M programmer.h A tf530_spi.c 3 files changed, 188 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/44/54944/1 diff --git a/Makefile b/Makefile index c486c12..0026898 100644 --- a/Makefile +++ b/Makefile @@ -138,6 +138,7 @@ ifeq ($(TARGET_OS), AmigaOS) override CPPFLAGS += -m68000 -mcrt=clib2 override LIBS += -mcrt=clib2 -lamiga -lm -lunix +override CONFIG_TF530_SPI = yes # Bus Pirate, Serprog and PonyProg are not supported under AmigaOS (missing serial support). ifeq ($(CONFIG_BUSPIRATE_SPI), yes) UNSUPPORTED_FEATURES += CONFIG_BUSPIRATE_SPI=yes @@ -1207,6 +1208,11 @@ PROGRAMMER_OBJS += jlink_spi.o endif +ifeq ($(CONFIG_TF530_SPI), yes) +FEATURE_CFLAGS += -D'CONFIG_TF530_SPI=1' +PROGRAMMER_OBJS += tf530_spi.o +endif + ifeq ($(CONFIG_NI845X_SPI), yes) FEATURE_CFLAGS += -D'CONFIG_NI845X_SPI=1' diff --git a/programmer.h b/programmer.h index 790fcc8..cbb5977 100644 --- a/programmer.h +++ b/programmer.h @@ -112,6 +112,9 @@ #if CONFIG_LINUX_SPI == 1 PROGRAMMER_LINUX_SPI, #endif +#if CONFIG_TF530_SPI == 1 + PROGRAMMER_TF530_SPI, +#endif #if CONFIG_USBBLASTER_SPI == 1 PROGRAMMER_USBBLASTER_SPI, #endif @@ -559,6 +562,11 @@ int linux_spi_init(void); #endif +/* tf530_spi.c */ +#if CONFIG_TF530_SPI == 1 +int tf53x_spi_init(void); +#endif + /* dediprog.c */ #if CONFIG_DEDIPROG == 1 int dediprog_init(void); diff --git a/tf530_spi.c b/tf530_spi.c new file mode 100644 index 0000000..76a7e43 --- /dev/null +++ b/tf530_spi.c @@ -0,0 +1,174 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2019 Steven J Leary + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#if CONFIG_TF530_SPI == 1 + +#include <stdio.h> +#include <string.h> +#include <stdlib.h> +#include <fcntl.h> +#include <errno.h> +#include <ctype.h> +#include "flash.h" +#include "chipdrivers.h" +#include "programmer.h" +#include "spi.h" + +#include <exec/resident.h> +#include <exec/errors.h> +#include <exec/memory.h> +#include <exec/lists.h> +#include <exec/alerts.h> +#include <exec/tasks.h> +#include <exec/io.h> +#include <exec/execbase.h> + +#include <libraries/expansion.h> + +#include <devices/trackdisk.h> +#include <devices/timer.h> +#include <devices/scsidisk.h> + +#include <dos/filehandler.h> + +#include <proto/exec.h> +#include <proto/disk.h> +#include <proto/expansion.h> + +struct TF53xSDRegs { + volatile uint8_t ctrl; // Control register + volatile uint8_t unused1; // padding + volatile uint8_t unused2; // padding + volatile uint8_t unused3; // padding + volatile uint8_t data; // data +}; + +static struct ConfigDev *cd = NULL; + +#define TF53x_CTRL_CS0 1 +#define TF53x_CTRL_CS1 2 +#define TF53x_CTRL_BUSY 4 +#define TF53x_CTRL_CS2 8 + +static void spi_send_byte(struct TF53xSDRegs *port, uint8_t value) +{ + uint8_t busy = 0; + + while (busy == 0) { + busy = port->ctrl & TF53x_CTRL_BUSY; + } + + port->data = value; +} + +inline uint8_t spi_recv_byte(struct TF53xSDRegs *port) +{ + uint8_t busy = 0; + + while (busy == 0) { + busy = port->ctrl & TF53x_CTRL_BUSY; + } + + return port->data; +} + +static void spi_cs_unassert(struct TF53xSDRegs *port) +{ + uint8_t current = port->ctrl; + port->ctrl = current | TF53x_CTRL_CS0 | \ + TF53x_CTRL_CS1 | TF53x_CTRL_CS2 | TF53x_CTRL_BUSY; +} + +static void spi_cs_assert(struct TF53xSDRegs *port) +{ + uint8_t current = port->ctrl; + port->ctrl = current & (~TF53x_CTRL_CS2); +} + +static int tf53x_spi_shutdown(void *data) +{ + return 0; +} + +static int tf53x_spi_send_command(const struct flashctx *flash, + unsigned int writecnt, + unsigned int readcnt, + const unsigned char *txbuf, + unsigned char *rxbuf) +{ + struct TF53xSDRegs *port = cd->cd_BoardAddr; + + spi_cs_assert(port); + + for (unsigned int i = 0; i < writecnt; i++) { + spi_send_byte(port, txbuf[i]); + } + + /* read the dummy byte and make sure + * the compiler doesnt optimize it away */ + uint8_t volatile dummy = spi_recv_byte(port); + dummy; + + for (unsigned int i = 0; i < readcnt; i++) { + rxbuf[i] = spi_recv_byte(port); + } + + spi_cs_unassert(port); + + return 0; +} + +static const struct spi_master spi_master_tf53x = { + .features = SPI_MASTER_4BA, + .max_data_read = 256, + .max_data_write = 256, + .command = tf53x_spi_send_command, + .multicommand = default_spi_send_multicommand, + .read = default_spi_read, + .write_256 = default_spi_write_256, + .write_aai = default_spi_write_aai, +}; + +int tf53x_spi_init(void) +{ + struct Library *ExpansionBase = NULL; + + if ((ExpansionBase = (struct Library *) + OpenLibrary((unsigned char *)"expansion.library", 0L)) == NULL) { + return 0; + } + + if (cd = (struct ConfigDev *) FindConfigDev(cd, 0x13D8, 0x81)) { + msg_cinfo("\nTF530 SPI found in autoconfig chain.\n"); + CloseLibrary(ExpansionBase); + } else { + msg_perr("\nTF530 SPI not found in autoconfig chain.\n"); + CloseLibrary(ExpansionBase); + return -1; + } + + if (register_shutdown(tf53x_spi_shutdown, NULL)) + return 1; + + register_spi_master(&spi_master_tf53x, NULL); + + return 0; +} + +#endif // CONFIG_TF530_SPI == 1 -- To view, visit https://review.coreboot.org/c/flashrom/+/54944 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I109eec4dec89650bb80d3d1da905d33f65f60f5a Gerrit-Change-Number: 54944 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: Add AmigaOS suppport to flashrom
by Stefan Reinauer (Code Review) 26 May '21

26 May '21

Stefan Reinauer has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/54943 ) Change subject: Add AmigaOS suppport to flashrom ...................................................................... Add AmigaOS suppport to flashrom Change-Id: I3e4a3885f88313aad019454fdbdf4be176b35330 Signed-off-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> --- M Makefile M cli_classic.c M flash.h M helpers.c M os.h M platform.h M udelay.c 7 files changed, 109 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/43/54943/1 diff --git a/Makefile b/Makefile index b8b40ca..c486c12 100644 --- a/Makefile +++ b/Makefile @@ -135,6 +135,89 @@ override LDFLAGS += -L/usr/local/lib endif +ifeq ($(TARGET_OS), AmigaOS) +override CPPFLAGS += -m68000 -mcrt=clib2 +override LIBS += -mcrt=clib2 -lamiga -lm -lunix +# Bus Pirate, Serprog and PonyProg are not supported under AmigaOS (missing serial support). +ifeq ($(CONFIG_BUSPIRATE_SPI), yes) +UNSUPPORTED_FEATURES += CONFIG_BUSPIRATE_SPI=yes +else +override CONFIG_BUSPIRATE_SPI = no +endif +ifeq ($(CONFIG_SERPROG), yes) +UNSUPPORTED_FEATURES += CONFIG_SERPROG=yes +else +override CONFIG_SERPROG = no +endif +ifeq ($(CONFIG_PONY_SPI), yes) +UNSUPPORTED_FEATURES += CONFIG_PONY_SPI=yes +else +override CONFIG_PONY_SPI = no +endif +# Digilent SPI, Dediprog, Developerbox, USB-Blaster, PICkit2, CH341A and FT2232 are not supported under AmigaOS (missing USB support). +ifeq ($(CONFIG_DIGILENT_SPI), yes) +UNSUPPORTED_FEATURES += CONFIG_DIGILENT_SPI=yes +else +override CONFIG_DIGILENT_SPI = no +endif +ifeq ($(CONFIG_DEDIPROG), yes) +UNSUPPORTED_FEATURES += CONFIG_DEDIPROG=yes +else +override CONFIG_DEDIPROG = no +endif +ifeq ($(CONFIG_DEVELOPERBOX_SPI), yes) +UNSUPPORTED_FEATURES += CONFIG_DEVELOPERBOX_SPI=yes +else +override CONFIG_DEVELOPERBOX_SPI = no +endif +ifeq ($(CONFIG_ENE_LPC), yes) +UNSUPPORTED_FEATURES += CONFIG_ENE_LPC=yes +else +override CONFIG_ENE_LPC = no +endif +ifeq ($(CONFIG_FT2232_SPI), yes) +UNSUPPORTED_FEATURES += CONFIG_FT2232_SPI=yes +else +override CONFIG_FT2232_SPI = no +endif +ifeq ($(CONFIG_MEC1308), yes) +UNSUPPORTED_FEATURES += CONFIG_MEC1308=yes +else +override CONFIG_MEC1308 = no +endif +ifeq ($(CONFIG_USBBLASTER_SPI), yes) +UNSUPPORTED_FEATURES += CONFIG_USBBLASTER_SPI=yes +else +override CONFIG_USBBLASTER_SPI = no +endif +ifeq ($(CONFIG_PICKIT2_SPI), yes) +UNSUPPORTED_FEATURES += CONFIG_PICKIT2_SPI=yes +else +override CONFIG_PICKIT2_SPI = no +endif +ifeq ($(CONFIG_CH341A_SPI), yes) +UNSUPPORTED_FEATURES += CONFIG_CH341A_SPI=yes +else +override CONFIG_CH341A_SPI = no +endif +ifeq ($(CONFIG_RAIDEN_DEBUG_SPI), yes) +UNSUPPORTED_FEATURES += CONFIG_RAIDEN_DEBUG_SPI=yes +else +override CONFIG_RAIDEN_DEBUG_SPI = no +endif +ifeq ($(CONFIG_STLINKV3_SPI), yes) +UNSUPPORTED_FEATURES += CONFIG_STLINKV3_SPI=yes +else +override CONFIG_STLINKV3_SPI = no +endif +# libjaylink is also not available for DOS +ifeq ($(CONFIG_JLINK_SPI), yes) +UNSUPPORTED_FEATURES += CONFIG_JLINK_SPI=yes +else +override CONFIG_JLINK_SPI = no +endif +endif + ifeq ($(TARGET_OS), DOS) EXEC_SUFFIX := .exe # DJGPP has odd uint*_t definitions which cause lots of format string warnings. diff --git a/cli_classic.c b/cli_classic.c index 5fd8092..31fac36 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -154,6 +154,11 @@ return 0; } +#if IS_AMIGA +#define ARGV_TYPE (const char **) +#else +#define ARGV_TYPE +#endif int main(int argc, char *argv[]) { const struct flashchip *chip = NULL; @@ -260,7 +265,7 @@ /* FIXME: Delay all operation_specified checks until after command * line parsing to allow --help overriding everything else. */ - while ((opt = getopt_long(argc, argv, optstring, + while ((opt = getopt_long(argc, ARGV_TYPE argv, optstring, long_options, &option_index)) != EOF) { switch (opt) { case 'r': diff --git a/flash.h b/flash.h index dd8e156..c2fef2c 100644 --- a/flash.h +++ b/flash.h @@ -52,6 +52,11 @@ /* TODO: check using code for correct usage of types */ typedef uintptr_t chipaddr; #define PRIxPTR_WIDTH ((int)(sizeof(uintptr_t)*2)) +/* Standard Amiga toolchain defines PRIxPTR wrong. */ +#ifdef __AMIGA__ +#undef PRIxPTR +#define PRIxPTR "x" +#endif int register_shutdown(int (*function) (void *data), void *data); void *programmer_map_flash_region(const char *descr, uintptr_t phys_addr, size_t len); @@ -326,7 +331,7 @@ void tolower_string(char *str); uint8_t reverse_byte(uint8_t x); void reverse_bytes(uint8_t *dst, const uint8_t *src, size_t length); -#ifdef __MINGW32__ +#if defined(__MINGW32__) || IS_AMIGA char* strtok_r(char *str, const char *delim, char **nextp); char *strndup(const char *str, size_t size); #endif diff --git a/helpers.c b/helpers.c index 289848d..56d881c 100644 --- a/helpers.c +++ b/helpers.c @@ -84,7 +84,7 @@ } /* FIXME: Find a better solution for MinGW. Maybe wrap strtok_s (C11) if it becomes available */ -#ifdef __MINGW32__ +#if defined(__MINGW32__) || IS_AMIGA char* strtok_r(char *str, const char *delim, char **nextp) { if (str == NULL) diff --git a/os.h b/os.h index ddfa46b..ba45da0 100644 --- a/os.h +++ b/os.h @@ -55,6 +55,9 @@ // GNU Hurd #elif defined(__gnu_hurd__) #define __FLASHROM_OS__ "Hurd" +// AmigaOS +#elif defined(__AMIGA__) +#define __FLASHROM_OS__ "AmigaOS" // Linux #elif defined(__linux__) // There are various flags in use on Android apparently. __ANDROID__ seems to be the most trustworthy. diff --git a/platform.h b/platform.h index 751957c..5d74f9a 100644 --- a/platform.h +++ b/platform.h @@ -36,6 +36,11 @@ #else #define IS_WINDOWS 0 #endif +#if defined(__AMIGA__) +#define IS_AMIGA 1 +#else +#define IS_AMIGA 0 +#endif // Likewise for target architectures #if defined (__i386__) || defined (__x86_64__) || defined(__amd64__) @@ -122,6 +127,10 @@ #define __FLASHROM_LITTLE_ENDIAN__ 1 #endif +#elif IS_M68K + +#define __FLASHROM_BIG_ENDIAN__ 1 + #elif IS_SPARC /* SPARC is big endian in general (but allows to access data in little endian too). */ #define __FLASHROM_BIG_ENDIAN__ 1 diff --git a/udelay.c b/udelay.c index 6c0efc4..b4f700d 100644 --- a/udelay.c +++ b/udelay.c @@ -226,7 +226,7 @@ { #if IS_WINDOWS Sleep((usecs + 999) / 1000); -#elif defined(__DJGPP__) +#elif defined(__DJGPP__) || IS_AMIGA sleep(usecs / 1000000); usleep(usecs % 1000000); #else -- To view, visit https://review.coreboot.org/c/flashrom/+/54943 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3e4a3885f88313aad019454fdbdf4be176b35330 Gerrit-Change-Number: 54943 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: newchange

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2021