June 2017 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[staging]: Enable continuous SPI reads
by Urja Rannikko (Code Review) 16 Jun '17

16 Jun '17

Urja Rannikko has posted comments on this change. ( https://review.coreboot.org/20223 ) Change subject: Enable continuous SPI reads ...................................................................... Patch Set 1: (2 comments) https://review.coreboot.org/#/c/20223/1/spi25.c File spi25.c: https://review.coreboot.org/#/c/20223/1/spi25.c@951 PS1, Line 951: page_size > Please don't call it page_size, that's confusing. I'm thinking of using "area" (as the term for this huge page-like-thing) here. Comments? https://review.coreboot.org/#/c/20223/1/spi25.c@951 PS1, Line 951: *1024,16*1024* > Your keyboard does have a space bar, doesn't it? Yes but i dont like using it. Fixed :) -- To view, visit https://review.coreboot.org/20223 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Iadf909c9216578b1c5dacd4c4991bb436e32edc9 Gerrit-Change-Number: 20223 Gerrit-PatchSet: 1 Gerrit-Owner: Urja Rannikko <urjaman(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: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 16 Jun 2017 08:36:06 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: serial: Support custom baud rates on linux
by build bot (Jenkins) (Code Review) 16 Jun '17

16 Jun '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/20224 ) Change subject: serial: Support custom baud rates on linux ...................................................................... Patch Set 3: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/361/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/371/ : SUCCESS -- To view, visit https://review.coreboot.org/20224 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I2a20f9182cb85e7bce5d6654a2caf20e6202b195 Gerrit-Change-Number: 20224 Gerrit-PatchSet: 3 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 16 Jun 2017 06:43:46 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: serial: Support custom baud rates on linux
by Urja Rannikko (Code Review) 16 Jun '17

16 Jun '17

Hello Paul Menzel, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/20224 to look at the new patch set (#3). 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> --- M Makefile A custom_baud.c A custom_baud.h M serial.c 4 files changed, 135 insertions(+), 11 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/24/20224/3 -- To view, visit https://review.coreboot.org/20224 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2a20f9182cb85e7bce5d6654a2caf20e6202b195 Gerrit-Change-Number: 20224 Gerrit-PatchSet: 3 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: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

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

15 Jun '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/20224 ) Change subject: serial: Support custom baud rates on linux ...................................................................... Patch Set 2: (5 comments) https://review.coreboot.org/#/c/20224/2/custom_baud.c File custom_baud.c: https://review.coreboot.org/#/c/20224/2/custom_baud.c@56 PS2, Line 56: (void)baud; Do we have warnings for unused parameters enabled? I'm really curious, usually I have to do with compilers that warn about statements with no effect. ;) https://review.coreboot.org/#/c/20224/2/custom_baud.c@57 PS2, Line 57: return 0; `return -1;`? Just in case somebody calls it by accident? I wouldn't expect it, just an idea to set a good pattern. https://review.coreboot.org/#/c/20224/2/serial.c File serial.c: https://review.coreboot.org/#/c/20224/2/serial.c@145 PS2, Line 145: Superfluous space. (yeah, I know you just copied it) https://review.coreboot.org/#/c/20224/2/serial.c@155 PS2, Line 155: #endif You might have noticed that I'm not a fan of guards in code files... I would accept it as is, but here's another idea: Make that baudtable a parameter and move the whole function into custom_baud.c where we already have the Linux/non-Linux separation. Also, grouping the stubs together, would make it more obvious why the set_custom_baudrate() stub is never called. https://review.coreboot.org/#/c/20224/2/serial.c@218 PS2, Line 218: msg_perr_strerror("Could not set custom baudrate: "); We should bail out or fallback? -- To view, visit https://review.coreboot.org/20224 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I2a20f9182cb85e7bce5d6654a2caf20e6202b195 Gerrit-Change-Number: 20224 Gerrit-PatchSet: 2 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 15 Jun 2017 19:56:38 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: serial: Support custom baud rates on linux
by build bot (Jenkins) (Code Review) 15 Jun '17

15 Jun '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/20224 ) Change subject: serial: Support custom baud rates on linux ...................................................................... Patch Set 2: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/360/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/370/ : SUCCESS -- To view, visit https://review.coreboot.org/20224 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I2a20f9182cb85e7bce5d6654a2caf20e6202b195 Gerrit-Change-Number: 20224 Gerrit-PatchSet: 2 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 15 Jun 2017 16:22:05 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: serial: Support custom baud rates on linux
by Urja Rannikko (Code Review) 15 Jun '17

15 Jun '17

Hello Paul Menzel, build bot (Jenkins), Nico Huber, I'd like you to reexamine a change. Please visit https://review.coreboot.org/20224 to look at the new patch set (#2). 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> --- M Makefile A custom_baud.c A custom_baud.h M serial.c 4 files changed, 124 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/24/20224/2 -- To view, visit https://review.coreboot.org/20224 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2a20f9182cb85e7bce5d6654a2caf20e6202b195 Gerrit-Change-Number: 20224 Gerrit-PatchSet: 2 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: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[staging]: serial: support custom baud rates on linux
by Nico Huber (Code Review) 15 Jun '17

15 Jun '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/20224 ) Change subject: serial: support custom baud rates on linux ...................................................................... Patch Set 1: (10 comments) With a generic set_custom_baudrate() and guarding done in the header file, the code could be free of guards. See inline comments. https://review.coreboot.org/#/c/20224/1//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/20224/1//COMMIT_MSG@7 PS1, Line 7: support Upper-case "Support" please. https://review.coreboot.org/#/c/20224/1//COMMIT_MSG@9 PS1, Line 9: the Upper-case... https://review.coreboot.org/#/c/20224/1//COMMIT_MSG@9 PS1, Line 9: because : broken because *of* broken? https://review.coreboot.org/#/c/20224/1/linux_baud.h File linux_baud.h: PS1: Missing include guards and license header. https://review.coreboot.org/#/c/20224/1/linux_baud.h@1 PS1, Line 1: int linux_set_custom_baudrate(int fd, int speed); Why not call it generically `set_custom_baudrate`? You could provide a stub then in case `#if IS_LINUX != 1` and wouldn't have to bother with the guard in the code. https://review.coreboot.org/#/c/20224/1/linux_baud.c File linux_baud.c: https://review.coreboot.org/#/c/20224/1/linux_baud.c@35 PS1, Line 35: * for more info. */ Urgh, who invented this comment style? Not your fault, I know this style is common in flashrom. Though, I'd prefer the style like used above for the license header, with the dangling /* and */ on separate lines. Also, I wouldn't place URLs in the code. They tend to break. Rather put them into the commit message? https://review.coreboot.org/#/c/20224/1/serial.c File serial.c: https://review.coreboot.org/#/c/20224/1/serial.c@44 PS1, Line 44: #if IS_LINUX Guarding a #include shouldn't be necessary. https://review.coreboot.org/#/c/20224/1/serial.c@126 PS1, Line 126: * so setting a custom rate can be attempted. */ How about a use_custom_baud() function instead, that returns true in case the requested baud rate isn't present in the sp_baudtable? That is_custom_baud() could just always return false `#if IS_LINUX != 1`. https://review.coreboot.org/#/c/20224/1/serial.c@210 PS1, Line 210: if (baud >= 0) { With the mentioned use_custom_baud() and an always present set_custom_baudrate() the code flow could be free of guards and look as follows: if (use_custom_baud()) { ... if (set_custom_baudrate()) ... ... } else { const struct baudentry *entry = round_baud(baud); if (cfsetispeed...) { ... } } https://review.coreboot.org/#/c/20224/1/serial.c@257 PS1, Line 257: Was this line break added intentionally? -- To view, visit https://review.coreboot.org/20224 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I2a20f9182cb85e7bce5d6654a2caf20e6202b195 Gerrit-Change-Number: 20224 Gerrit-PatchSet: 1 Gerrit-Owner: Urja Rannikko <urjaman(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: Thu, 15 Jun 2017 13:14:32 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: serial: support custom baud rates on linux
by build bot (Jenkins) (Code Review) 15 Jun '17

15 Jun '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/20224 ) Change subject: serial: support custom baud rates on linux ...................................................................... Patch Set 1: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/359/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/369/ : SUCCESS -- To view, visit https://review.coreboot.org/20224 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I2a20f9182cb85e7bce5d6654a2caf20e6202b195 Gerrit-Change-Number: 20224 Gerrit-PatchSet: 1 Gerrit-Owner: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 15 Jun 2017 12:31:16 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: serial: support custom baud rates on linux
by Urja Rannikko (Code Review) 15 Jun '17

15 Jun '17

Urja Rannikko has uploaded this change for review. ( 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 linux_baud.c because broken include stuff (see the urls in that file). Change-Id: I2a20f9182cb85e7bce5d6654a2caf20e6202b195 Signed-off-by: Urja Rannikko <urjaman(a)gmail.com> --- M Makefile A linux_baud.c A linux_baud.h M serial.c 4 files changed, 79 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/24/20224/1 diff --git a/Makefile b/Makefile index 65026da..3d07c91 100644 --- a/Makefile +++ b/Makefile @@ -921,7 +921,7 @@ endif ifneq ($(NEED_SERIAL), ) -LIB_OBJS += serial.o +LIB_OBJS += serial.o linux_baud.o endif ifneq ($(NEED_POSIX_SOCKETS), ) diff --git a/linux_baud.c b/linux_baud.c new file mode 100644 index 0000000..6214767 --- /dev/null +++ b/linux_baud.c @@ -0,0 +1,50 @@ +/* + * 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" +#if IS_LINUX == 1 + +#include <sys/ioctl.h> +#include <fcntl.h> +#include <asm-generic/termbits.h> +#include <asm-generic/ioctls.h> + +#include "linux_baud.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 linux_set_custom_baudrate(int fd, int speed) +{ + struct termios2 tio; + if (ioctl(fd, TCGETS2, &tio)) { + return -1; + } + tio.c_cflag &= ~CBAUD; + tio.c_cflag |= BOTHER; + tio.c_ispeed = speed; + tio.c_ospeed = speed; + return ioctl(fd, TCSETS2, &tio); +} + +#endif diff --git a/linux_baud.h b/linux_baud.h new file mode 100644 index 0000000..bbcd4d4 --- /dev/null +++ b/linux_baud.h @@ -0,0 +1 @@ +int linux_set_custom_baudrate(int fd, int speed); diff --git a/serial.c b/serial.c index a64a51d..08a204b 100644 --- a/serial.c +++ b/serial.c @@ -41,6 +41,10 @@ #include "flash.h" #include "programmer.h" +#if IS_LINUX +#include "linux_baud.h" +#endif + fdtype sp_fd = SER_INV_FD; /* There is no way defined by POSIX to use arbitrary baud rates. It only defines some macros that can be used to @@ -49,8 +53,7 @@ * 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, see linux_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 @@ -119,6 +122,8 @@ {0, 0} /* Terminator */ }; +/* On linux, we dont return non-exact rates but instead NULL, + * so setting a custom rate can be attempted. */ static const struct baudentry *round_baud(unsigned int baud) { int i; @@ -128,13 +133,21 @@ return &sp_baudtable[i]; if (sp_baudtable[i].baud < baud) { +#if IS_LINUX + return NULL; +#else msg_pwarn("Warning: given baudrate %d rounded down to %d.\n", baud, sp_baudtable[i].baud); return &sp_baudtable[i]; +#endif } } +#if IS_LINUX + return NULL; +#else msg_pinfo("Using slowest possible baudrate: %d.\n", sp_baudtable[0].baud); return &sp_baudtable[0]; +#endif } #endif @@ -196,6 +209,17 @@ wanted = observed; if (baud >= 0) { const struct baudentry *entry = round_baud(baud); +#if IS_LINUX + if (!entry) { + msg_pdbg("Trying to set custom baud rate.\n"); + if (linux_set_custom_baudrate(fd, baud)) { + msg_perr_strerror("Could not set custom baudrate: "); + } + /* We want whatever the termios looks like now, so the rest of the + * setup doesnt mess up the custom rate, hopefully. */ + tcgetattr(fd, &wanted); + } else +#endif if (cfsetispeed(&wanted, entry->flag) != 0 || cfsetospeed(&wanted, entry->flag) != 0) { msg_perr_strerror("Could not set serial baud rate: "); return 1; @@ -230,6 +254,7 @@ (long)observed.c_oflag, (long)wanted.c_oflag ); } + if (cfgetispeed(&observed) != cfgetispeed(&wanted) || cfgetospeed(&observed) != cfgetospeed(&wanted)) { msg_pwarn("Could not set baud rates exactly.\n"); -- To view, visit https://review.coreboot.org/20224 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: newchange Gerrit-Change-Id: I2a20f9182cb85e7bce5d6654a2caf20e6202b195 Gerrit-Change-Number: 20224 Gerrit-PatchSet: 1 Gerrit-Owner: Urja Rannikko <urjaman(a)gmail.com>

1 0

Change in flashrom[staging]: Enable continuous SPI reads
by Nico Huber (Code Review) 15 Jun '17

15 Jun '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/20223 ) Change subject: Enable continuous SPI reads ...................................................................... Patch Set 1: On second though, how does linux_spi perform when the access crosses (host) pages? I guess it doesn't matter? -- To view, visit https://review.coreboot.org/20223 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Iadf909c9216578b1c5dacd4c4991bb436e32edc9 Gerrit-Change-Number: 20223 Gerrit-PatchSet: 1 Gerrit-Owner: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 15 Jun 2017 11:06:35 +0000 Gerrit-HasComments: No

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit June 2017