- flashrom-gerrit - mail.coreboot.org

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

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: Code-Review+1 (2 comments) Maybe just s/page/part/? can't find a better word atm. 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. 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? -- 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 15 Jun 2017 10:55:40 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: Enable continuous SPI reads
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/20223 ) Change subject: Enable continuous SPI reads ...................................................................... Patch Set 1: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom_gerrit/358/ : SUCCESS https://qa.coreboot.org/job/flashrom-customrules/368/ : SUCCESS -- 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 15 Jun 2017 10:43:40 +0000 Gerrit-HasComments: No

1 0

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

15 Jun '17

Urja Rannikko has uploaded this change for review. ( https://review.coreboot.org/20223 Change subject: Enable continuous SPI reads ...................................................................... Enable continuous SPI reads Change-Id: Iadf909c9216578b1c5dacd4c4991bb436e32edc9 Signed-off-by: Urja Rannikko <urjaman(a)gmail.com> --- M serprog.c M spi25.c 2 files changed, 4 insertions(+), 24 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/23/20223/1 diff --git a/serprog.c b/serprog.c index 98aac83..25c9944 100644 --- a/serprog.c +++ b/serprog.c @@ -303,15 +303,13 @@ unsigned int writecnt, unsigned int readcnt, const unsigned char *writearr, unsigned char *readarr); -static int serprog_spi_read(struct flashctx *flash, uint8_t *buf, - unsigned int start, unsigned int len); static struct spi_master spi_master_serprog = { .type = SPI_CONTROLLER_SERPROG, .max_data_read = MAX_DATA_READ_UNLIMITED, .max_data_write = MAX_DATA_WRITE_UNLIMITED, .command = serprog_spi_send_command, .multicommand = default_spi_send_multicommand, - .read = serprog_spi_read, + .read = default_spi_read, .write_256 = default_spi_write_256, .write_aai = default_spi_write_aai, }; @@ -931,25 +929,6 @@ readarr); free(parmbuf); return ret; -} - -/* FIXME: This function is optimized so that it does not split each transaction - * into chip page_size long blocks unnecessarily like spi_read_chunked. This has - * the advantage that it is much faster for most chips, but breaks those with - * non-continuous reads. When spi_read_chunked is fixed this method can be removed. */ -static int serprog_spi_read(struct flashctx *flash, uint8_t *buf, - unsigned int start, unsigned int len) -{ - unsigned int i, cur_len; - const unsigned int max_read = spi_master_serprog.max_data_read; - for (i = 0; i < len; i += cur_len) { - int ret; - cur_len = min(max_read, (len - i)); - ret = spi_nbyte_read(flash, start + i, buf + i, cur_len); - if (ret) - return ret; - } - return 0; } void *serprog_map(const char *descr, uintptr_t phys_addr, size_t len) diff --git a/spi25.c b/spi25.c index af4b6db..30e4dc3 100644 --- a/spi25.c +++ b/spi25.c @@ -940,14 +940,15 @@ /* * Read a part of the flash chip. * FIXME: Use the chunk code from Michael Karcher instead. - * Each page is read separately in chunks with a maximum size of chunksize. + * Each "page" is read separately in chunks with a maximum size of chunksize. */ int spi_read_chunked(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len, unsigned int chunksize) { int rc = 0; unsigned int i, j, starthere, lenhere, toread; - unsigned int page_size = flash->chip->page_size; + /* Limit page_size for multi-die 4-byte-addressing chips. */ + unsigned int page_size = min(flash->chip->total_size*1024,16*1024*1024); /* Warning: This loop has a very unusual condition and body. * The loop needs to go through each page with at least one affected -- To view, visit https://review.coreboot.org/20223 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: newchange Gerrit-Change-Id: Iadf909c9216578b1c5dacd4c4991bb436e32edc9 Gerrit-Change-Number: 20223 Gerrit-PatchSet: 1 Gerrit-Owner: Urja Rannikko <urjaman(a)gmail.com>

1 0

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

15 Jun '17

Urja Rannikko has posted comments on this change. ( https://review.coreboot.org/17954 ) Change subject: Enable continuous SPI reads ...................................................................... Patch Set 3: I want something like this in, but i think it could be improved, and I'm unsure as to whether we'd need a new function for this. 1. I dont know of a 3-byte addressing chip that would have issues with this, but eg, N25Q512A is limited to the 256Mb segment the read started in due to it being a multi-die chip (and my memory says i read of a multi-die chip with 128Mb segments, but couldnt find it now). 2. serprog already overrided the read function to something like this. But i guess that can be removed in later patch. To address the point 1, i'd rather just make spi_read_chunked have the "page_size" set to min(flash->chip->total_size*1024,16*1024*1024). This wouldnt hurt existing other users of spi_read_chunked, since they're already using it with their appropriate chunksize (linux_spi might benefit since it uses it with getpagesize()), and it would be a lot less code. -- To view, visit https://review.coreboot.org/17954 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Idf7aa130b1ceb3d0864bdeab5835f3341f558d41 Gerrit-Change-Number: 17954 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philippe Mathieu-Daudé <philippe.mathieu.daude(a)gmail.com> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 15 Jun 2017 10:21:03 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: udelay: Use clock_gettime() if available and precise
by Philippe Mathieu-Daudé (Code Review) 14 Jun '17

14 Jun '17

Philippe Mathieu-Daudé has posted comments on this change. ( https://review.coreboot.org/19391 ) Change subject: udelay: Use clock_gettime() if available and precise ...................................................................... Patch Set 4: Code-Review+1 -- To view, visit https://review.coreboot.org/19391 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I85ad359823875237ada9cd027af3017d62e9a235 Gerrit-Change-Number: 19391 Gerrit-PatchSet: 4 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: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philippe Mathieu-Daudé <philippe.mathieu.daude(a)gmail.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 14 Jun 2017 15:44:53 +0000 Gerrit-HasComments: No

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit