- flashrom-gerrit - mail.coreboot.org

Change in flashrom[staging]: serial: Support custom baud rates on linux
by Nico Huber (Code Review) 19 Oct '17

19 Oct '17

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/20224 ) Change subject: serial: Support custom baud rates on linux ...................................................................... serial: Support custom baud rates on linux The function to do this is contained in custom_baud.c because of broken include stuff. Change-Id: I2a20f9182cb85e7bce5d6654a2caf20e6202b195 Signed-off-by: Urja Rannikko <urjaman(a)gmail.com> Reviewed-on: https://review.coreboot.org/20224 Reviewed-by: Nico Huber <nico.h(a)gmx.de> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M Makefile A custom_baud.c A custom_baud.h M serial.c 4 files changed, 136 insertions(+), 11 deletions(-) Approvals: Nico Huber: Looks good to me, approved build bot (Jenkins): Verified diff --git a/Makefile b/Makefile index 5573127..52f6c98 100644 --- a/Makefile +++ b/Makefile @@ -921,7 +921,7 @@ endif ifneq ($(NEED_SERIAL), ) -LIB_OBJS += serial.o +LIB_OBJS += serial.o custom_baud.o endif ifneq ($(NEED_POSIX_SOCKETS), ) diff --git a/custom_baud.c b/custom_baud.c new file mode 100644 index 0000000..0caca80 --- /dev/null +++ b/custom_baud.c @@ -0,0 +1,78 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2017 Urja Rannikko <urjaman(a)gmail.com> + * + * 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; either version 2 of the License, or + * (at your option) any later version. + * + * 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 + */ + +#include "platform.h" +#include "custom_baud.h" + +#if IS_LINUX +#include <sys/ioctl.h> +#include <fcntl.h> +#include <asm-generic/termbits.h> +#include <asm-generic/ioctls.h> + +/* + * This include hell above is why this is in a separate source file. See eg. + * https://www.downtowndougbrown.com/2013/11/linux-custom-serial-baud-rates/ + * https://stackoverflow.com/questions/12646324/how-to-set-a-custom-baud-rate-… + * https://github.com/jbkim/Linux-custom-baud-rate + * for more info. + */ + +int set_custom_baudrate(int fd, unsigned int baud) +{ + struct termios2 tio; + if (ioctl(fd, TCGETS2, &tio)) { + return -1; + } + tio.c_cflag &= ~CBAUD; + tio.c_cflag |= BOTHER; + tio.c_ispeed = baud; + tio.c_ospeed = baud; + return ioctl(fd, TCSETS2, &tio); +} + +int use_custom_baud(unsigned int baud, const struct baudentry *baudtable) +{ + int i; + for (i = 0; baudtable[i].baud; i++) { + if (baudtable[i].baud == baud) + return 0; + + if (baudtable[i].baud > baud) + return 1; + } + return 1; +} + +#else +#include <errno.h> + +/* Stub, should not get called. */ +int set_custom_baudrate(int fd, unsigned int baud) +{ + errno = ENOSYS; /* Hoping "Function not supported" will make you look here. */ + return -1; +} + +int use_custom_baud(unsigned int baud, const struct baudentry *baudtable) +{ + return 0; +} +#endif diff --git a/custom_baud.h b/custom_baud.h new file mode 100644 index 0000000..ae286f5 --- /dev/null +++ b/custom_baud.h @@ -0,0 +1,35 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2017 Urja Rannikko <urjaman(a)gmail.com> + * + * 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; either version 2 of the License, or + * (at your option) any later version. + * + * 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 + */ + +#ifndef __CUSTOM_BAUD_H__ +#define __CUSTOM_BAUD_H__ 1 + +struct baudentry { + int flag; + unsigned int baud; +}; + +int set_custom_baudrate(int fd, unsigned int baud); + +/* Returns 1 if non-exact rate would be used, and setting a custom rate is supported. + The baudtable must be in ascending order and terminated with a 0-baud entry. */ +int use_custom_baud(unsigned int baud, const struct baudentry *baudtable); + +#endif diff --git a/serial.c b/serial.c index a64a51d..608464b 100644 --- a/serial.c +++ b/serial.c @@ -40,6 +40,7 @@ #endif #include "flash.h" #include "programmer.h" +#include "custom_baud.h" fdtype sp_fd = SER_INV_FD; @@ -49,18 +50,14 @@ * The code below creates a mapping in sp_baudtable between these macros and the numerical baud rates to deal * with numerical user input. * - * On Linux there is a non-standard way to use arbitrary baud rates that flashrom does not support (yet), cf. - * http://www.downtowndougbrown.com/2013/11/linux-custom-serial-baud-rates/ + * On Linux there is a non-standard way to use arbitrary baud rates that we use if there is no + * matching standard rate, see custom_baud.c * * On Windows there exist similar macros (starting with CBR_ instead of B) but they are only defined for * backwards compatibility and the API supports arbitrary baud rates in the same manner as the macros, see * http://msdn.microsoft.com/en-us/library/windows/desktop/aa363214(v=vs.85).a… */ #if !IS_WINDOWS -struct baudentry { - int flag; - unsigned int baud; -}; #define BAUDENTRY(baud) { B##baud, baud }, static const struct baudentry sp_baudtable[] = { @@ -195,10 +192,25 @@ } wanted = observed; if (baud >= 0) { - const struct baudentry *entry = round_baud(baud); - if (cfsetispeed(&wanted, entry->flag) != 0 || cfsetospeed(&wanted, entry->flag) != 0) { - msg_perr_strerror("Could not set serial baud rate: "); - return 1; + if (use_custom_baud(baud, sp_baudtable)) { + if (set_custom_baudrate(fd, baud)) { + msg_perr_strerror("Could not set custom baudrate: "); + return 1; + } + /* We want whatever the termios looks like now, so the rest of the + setup doesnt mess up the custom rate. */ + if (tcgetattr(fd, &wanted) != 0) { + /* This should pretty much never happen (see above), but.. */ + msg_perr_strerror("Could not fetch serial port configuration: "); + return 1; + } + msg_pdbg("Using custom baud rate.\n"); + } else { + const struct baudentry *entry = round_baud(baud); + if (cfsetispeed(&wanted, entry->flag) != 0 || cfsetospeed(&wanted, entry->flag) != 0) { + msg_perr_strerror("Could not set serial baud rate: "); + return 1; + } } } wanted.c_cflag &= ~(PARENB | CSTOPB | CSIZE | CRTSCTS); -- To view, visit https://review.coreboot.org/20224 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: merged Gerrit-Change-Id: I2a20f9182cb85e7bce5d6654a2caf20e6202b195 Gerrit-Change-Number: 20224 Gerrit-PatchSet: 6 Gerrit-Owner: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[staging]: spi25: Enable direct 4BA read and write using feature bits
by Nico Huber (Code Review) 17 Oct '17

17 Oct '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/22021 ) Change subject: spi25: Enable direct 4BA read and write using feature bits ...................................................................... Patch Set 1: (2 comments) https://review.coreboot.org/#/c/22021/1/flash.h File flash.h: https://review.coreboot.org/#/c/22021/1/flash.h@124 PS1, Line 124: #define FEATURE_4BA_READ (1 << 12) /* Native 4BA read instruction (0x13) is supported */ https://review.coreboot.org/#/c/22021/1/flash.h@125 PS1, Line 125: #define FEATURE_4BA_WRITE (1 << 13) /* Native 4BA byte program (0x12) is supported */ -- To view, visit https://review.coreboot.org/22021 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I2f6817ca198bf923671a7aa67e956e5477d71848 Gerrit-Change-Number: 22021 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 17 Oct 2017 18:21:53 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: spi25: Add native 4BA support
by Nico Huber (Code Review) 17 Oct '17

17 Oct '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/22020 ) Change subject: spi25: Add native 4BA support ...................................................................... Patch Set 1: (2 comments) > The code here looks fine, I just want to check my understanding of > how you're deciding whether to use 4BA native instructions or > extended address mode. As you're probably already aware, some chips > support one but not the other, so we need to be a little careful. I target to have all information (excluding erase instructions) about supported instructions in the feature bits. > From what I've seen, it's safer to use native 4BA instructions when > they're available since they should work no matter what mode the > chip is in. This is exactly what (I think) I'm doing in the next patch. e.g. `FEATURE_4BA_READ` means the native 4BA read instruction is sup- ported and if it's set, we just use that. It only works, though, if `FEATURE_4BA_SUPPORT` is set too and `FEATURE_4BA_EXT_ADDR` is not. I have to admit I'm a bit clueless about the latter, e.g. are there chips that support an extended address register and native 4BA instructions? If yes, or if we just want to pre- pare for it, spi_prepare_address() will need a parameter to override the default choice, I guess. > > Also, we should consider how we might override the default behavior > in case the programmer can only operate in a particular mode (I'm > thinking Intel chipsets and Dediprog programmers). My preferred solution is to have more feature bits. One for each sup- ported instruction if need be. The programmer driver can choose for itself then what to do (or to bail out). https://review.coreboot.org/#/c/22020/1/flash.h File flash.h: https://review.coreboot.org/#/c/22020/1/flash.h@123 PS1, Line 123: #define FEATURE_4BA_EXT_ADDR (1 << 11) I shall comment the feature bits in my next iteration. This one: /* Regular 3-byte operations can be used by writing the most signifcant address byte into an extended address register. */ https://review.coreboot.org/#/c/22020/1/spi25.c File spi25.c: https://review.coreboot.org/#/c/22020/1/spi25.c@363 PS1, Line 363: flash->address_high_byte = addr_high; > The follow-up patch seems to use native 4BA instructions for read and write On certain feature-bit combinations this can indeed be a problem. -- To view, visit https://review.coreboot.org/22020 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I644600beaab9a571b97b67f7516abe571d3460c1 Gerrit-Change-Number: 22020 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 17 Oct 2017 18:17:13 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: spi25: Use common code for nbyte read/write and erase
by Nico Huber (Code Review) 17 Oct '17

17 Oct '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/22019 ) Change subject: spi25: Use common code for nbyte read/write and erase ...................................................................... Patch Set 1: (3 comments) https://review.coreboot.org/#/c/22019/1/spi25.c File spi25.c: https://review.coreboot.org/#/c/22019/1/spi25.c@326 PS1, Line 326: spi_write_status > spi_poll_status? Yeah, that is what it does. Though, with all the FIXMEs fixed, it should be more like check_write_status()... Don't have a strong preference (but it should keep `write` in its name). spi_poll_wip() maybe? https://review.coreboot.org/#/c/22019/1/spi25.c@377 PS1, Line 377: JEDEC_WREN > Hmmm, what do we do about chips that only use EWSR? I know this patch is on I'm not an expert, but thought EWSR is actually only used before status register updates. https://review.coreboot.org/#/c/22019/1/spi25.c@383 PS1, Line 383: > Check if out_len < 1? Hmmm, no as it's also used for block erasers. I guess the function name needs to reflect that better, or the signature needs to be commented. -- To view, visit https://review.coreboot.org/22019 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ibc1ae48acbfbd427a30bcd64bdc080dc3dc20503 Gerrit-Change-Number: 22019 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 17 Oct 2017 17:38:57 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: spi25: Introduce spi_simple_write_cmd()
by Nico Huber (Code Review) 17 Oct '17

17 Oct '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/22018 ) Change subject: spi25: Introduce spi_simple_write_cmd() ...................................................................... Patch Set 1: (1 comment) https://review.coreboot.org/#/c/22018/1/spi25.c File spi25.c: https://review.coreboot.org/#/c/22018/1/spi25.c@326 PS1, Line 326: spi_simple_write_cmd > Is there anything other than chip erase commands that this applies to? I th Yes, I've repurposed this later for 4BA-enable functions that need a WREN. With that said, we could also name the functions after WREN, e.g. static int spi_simple_wren_cmd(...); and static int spi_wren_cmd(...); instead of `spi_write_cmd()` (that is added later) as it's also used for erasures. Or if we want to emphasize the command parts even more: static int spi_wren_simple_cmd(...); static int spi_wren_address_cmd(...); My head is full of ideas, I just don't have a strong preference. Another thought: Would you prefer both functions to be commented, i.e. a doxygen style interface documentation? I'm often unsure if it's necessary as the code is already very specific about what is done ;) and having it commented means we have to take special care to keep the comment in sync with the implementation. -- To view, visit https://review.coreboot.org/22018 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ib244356fa471e15863b52e6037899d19113cb4a9 Gerrit-Change-Number: 22018 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 17 Oct 2017 17:27:17 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: Move get_layout() from flashrom.c to layout.c
by build bot (Jenkins) (Code Review) 17 Oct '17

17 Oct '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/21367 ) Change subject: Move get_layout() from flashrom.c to layout.c ...................................................................... Patch Set 7: Build Successful https://qa.coreboot.org/job/flashrom-customrules/794/ : SUCCESS -- To view, visit https://review.coreboot.org/21367 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ic67cf53abddc0aa905674acbcde717d9aed2f66e Gerrit-Change-Number: 21367 Gerrit-PatchSet: 7 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philippe Mathieu-Daudé <f4bug(a)amsat.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 17 Oct 2017 06:50:12 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: Move get_layout() from flashrom.c to layout.c
by David Hendricks (Code Review) 17 Oct '17

17 Oct '17

David Hendricks has submitted this change and it was merged. ( https://review.coreboot.org/21367 ) Change subject: Move get_layout() from flashrom.c to layout.c ...................................................................... Move get_layout() from flashrom.c to layout.c Change-Id: Ic67cf53abddc0aa905674acbcde717d9aed2f66e Signed-off-by: David Hendricks <david.hendricks(a)gmail.com> Reviewed-on: https://review.coreboot.org/21367 Reviewed-by: Philippe Mathieu-Daudé <f4bug(a)amsat.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M flashrom.c M layout.c M layout.h 3 files changed, 10 insertions(+), 8 deletions(-) Approvals: Philippe Mathieu-Daudé: Looks good to me, but someone else must approve Nico Huber: Looks good to me, approved build bot (Jenkins): Verified diff --git a/flashrom.c b/flashrom.c index 4f17382..4fe1843 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1529,14 +1529,6 @@ return 0; } -static const struct flashrom_layout *get_layout(const struct flashctx *const flashctx) -{ - if (flashctx->layout && flashctx->layout->num_entries) - return flashctx->layout; - else - return &flashctx->fallback_layout.base; -} - /** * @brief Reads the included layout regions into a buffer. * diff --git a/layout.c b/layout.c index 9bf0b03..7ce7c57 100644 --- a/layout.c +++ b/layout.c @@ -40,6 +40,14 @@ return &layout; } +const struct flashrom_layout *get_layout(const struct flashrom_flashctx *const flashctx) +{ + if (flashctx->layout && flashctx->layout->num_entries) + return flashctx->layout; + else + return &flashctx->fallback_layout.base; +} + #ifndef __LIBPAYLOAD__ int read_romlayout(const char *name) { diff --git a/layout.h b/layout.h index f32dc82..fd1049d 100644 --- a/layout.h +++ b/layout.h @@ -58,6 +58,8 @@ }; struct flashrom_layout *get_global_layout(void); +struct flashrom_flashctx; +const struct flashrom_layout *get_layout(const struct flashrom_flashctx *const flashctx); int process_include_args(struct flashrom_layout *); -- To view, visit https://review.coreboot.org/21367 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: merged Gerrit-Change-Id: Ic67cf53abddc0aa905674acbcde717d9aed2f66e Gerrit-Change-Number: 21367 Gerrit-PatchSet: 7 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philippe Mathieu-Daudé <f4bug(a)amsat.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[staging]: Move get_layout() from flashrom.c to layout.c
by David Hendricks (Code Review) 17 Oct '17

17 Oct '17

David Hendricks has posted comments on this change. ( https://review.coreboot.org/21367 ) Change subject: Move get_layout() from flashrom.c to layout.c ...................................................................... Patch Set 6: trivial rebase -- To view, visit https://review.coreboot.org/21367 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ic67cf53abddc0aa905674acbcde717d9aed2f66e Gerrit-Change-Number: 21367 Gerrit-PatchSet: 6 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philippe Mathieu-Daudé <f4bug(a)amsat.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 17 Oct 2017 06:49:57 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: Move get_layout() from flashrom.c to layout.c
by build bot (Jenkins) (Code Review) 17 Oct '17

17 Oct '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/21367 ) Change subject: Move get_layout() from flashrom.c to layout.c ...................................................................... Patch Set 6: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/706/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/793/ : SUCCESS -- To view, visit https://review.coreboot.org/21367 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ic67cf53abddc0aa905674acbcde717d9aed2f66e Gerrit-Change-Number: 21367 Gerrit-PatchSet: 6 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philippe Mathieu-Daudé <f4bug(a)amsat.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 17 Oct 2017 06:49:32 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: spi25: Add native 4BA support
by David Hendricks (Code Review) 17 Oct '17

17 Oct '17

David Hendricks has posted comments on this change. ( https://review.coreboot.org/22020 ) Change subject: spi25: Add native 4BA support ...................................................................... Patch Set 1: -Code-Review (1 comment) The code here looks fine, I just want to check my understanding of how you're deciding whether to use 4BA native instructions or extended address mode. As you're probably already aware, some chips support one but not the other, so we need to be a little careful. >From what I've seen, it's safer to use native 4BA instructions when they're available since they should work no matter what mode the chip is in. Also, we should consider how we might override the default behavior in case the programmer can only operate in a particular mode (I'm thinking Intel chipsets and Dediprog programmers). https://review.coreboot.org/#/c/22020/1/spi25.c File spi25.c: https://review.coreboot.org/#/c/22020/1/spi25.c@363 PS1, Line 363: flash->address_high_byte = addr_high; The follow-up patch seems to use native 4BA instructions for read and write regardless of what you do here. -- To view, visit https://review.coreboot.org/22020 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I644600beaab9a571b97b67f7516abe571d3460c1 Gerrit-Change-Number: 22020 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 17 Oct 2017 03:40:42 +0000 Gerrit-HasComments: Yes

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit