Attention is currently required from: Thomas Heijligen, Jonathon Hall.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68092 )
Change subject: flashrom.c: Remove custom mappers from opaque_master
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/68092
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I36f05154edda371b51f8ff416f019837ff1c243d
Gerrit-Change-Number: 68092
Gerrit-PatchSet: 3
Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Comment-Date: Sat, 08 Oct 2022 18:41:55 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Jonathon Hall.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68091 )
Change subject: dummyflasher.c: Remove custom mapper from opaque_master
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/68091
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I76ae3e0c2e91ecba4fd320941bd1eff038050731
Gerrit-Change-Number: 68091
Gerrit-PatchSet: 3
Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Comment-Date: Sat, 08 Oct 2022 18:41:43 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Jonathon Hall.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68090 )
Change subject: ichspi: Do not attempt to map physical memory for hwseq
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/68090
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie698071c3181e988f10b750b0e50c9700efaa1a3
Gerrit-Change-Number: 68090
Gerrit-PatchSet: 3
Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Comment-Date: Sat, 08 Oct 2022 18:39:59 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Jonathon Hall, Nikolai Artemiev.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67695 )
Change subject: drivers: Move (un)map_flash_region to par/spi/opaque_master
......................................................................
Patch Set 10: Code-Review+2
(1 comment)
File parallel.c:
https://review.coreboot.org/c/flashrom/+/67695/comment/662456d9_bc002658
PS10, Line 82: |
nit: spacing needed around `|`
--
To view, visit https://review.coreboot.org/c/flashrom/+/67695
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9c3df6ae260bcdb246dfb0cd8e043919609b014b
Gerrit-Change-Number: 67695
Gerrit-PatchSet: 10
Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Sat, 08 Oct 2022 18:39:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67404 )
Change subject: drivers/: Make 'fallback_{un}map' the default unless defined
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit https://review.coreboot.org/c/flashrom/+/67404
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I5ea7bd68f7ae2cd4af9902ef07255ab6ce0bfdb3
Gerrit-Change-Number: 67404
Gerrit-PatchSet: 5
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Sat, 08 Oct 2022 18:35:37 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68238 )
Change subject: rayer_spi.c: Roll up programmer type search logic into func
......................................................................
Patch Set 2:
(2 comments)
File rayer_spi.c:
https://review.coreboot.org/c/flashrom/+/68238/comment/f2fef1d6_e10ed986
PS2, Line 264: *prog_type = strdup(arg);
> As mentioned in CB:68230, calling `get_progtype()` here and having this function "return" the prog t […]
CB:68239 does it
https://review.coreboot.org/c/flashrom/+/68238/comment/a2ef32c8_bbf03790
PS2, Line 271: static const struct rayer_programmer *find_progtype(const char *prog_type)
If you want to keep CB:68239 as a separate commit, could you please move this function above `get_params()`?
--
To view, visit https://review.coreboot.org/c/flashrom/+/68238
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id226ea61132ecc30fd8696e1d8ea50373e752cac
Gerrit-Change-Number: 68238
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Sat, 08 Oct 2022 13:25:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68232 )
Change subject: rayer_spi.c: Drop lpt_outbyte intermediate var
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
Patchset:
PS2:
Other than the confusing code comment, LGTM
--
To view, visit https://review.coreboot.org/c/flashrom/+/68232
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Idedabb7b1c401d666b3b7e621e75704c7e765fd1
Gerrit-Change-Number: 68232
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Sat, 08 Oct 2022 13:24:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Edward O'Callaghan.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68230 )
Change subject: rayer_spi.c: Move param parse logic into own func
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
File rayer_spi.c:
https://review.coreboot.org/c/flashrom/+/68230/comment/e58782f3_efe06263
PS1, Line 292: if (prog_type) {
: for (; prog->type != NULL; prog++) {
: if (strcasecmp(prog_type, prog->type) == 0) {
: break;
: }
: }
: free(prog_type);
:
: if (prog->type == NULL) {
: msg_perr("Error: Invalid device type specified.\n");
: return 1;
: }
: }
> No, actually I didn't push CB:68238 before which made it unclear what I was thinking. However the idea here was to deconflate the parameter parse logic from the programmer type lookup logic into their own respective functions. This makes the init entry-point more linear in its control flow without all the resource management getting in the way. Hopefully the thinking is a little more clear with CB:68238 push please do let me know if not.
Just took a look at CB:68238 and it makes sense, it's similar to what CB:67906 and CB:67908 try to achieve for ft2232_spi. The only thing is that parameter parsing could be encapsulated a bit more. We commented on CB:68238 with more concrete implementation details.
Actually... We just made CB:68239 to show exactly what we're thinking of. Feel free to integrate it into CB:68238 or keep it as a separate commit.
See CB:68238 for discussion.
--
To view, visit https://review.coreboot.org/c/flashrom/+/68230
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I287aa2e5d94e872553d08c0750f8dc6d60b9caff
Gerrit-Change-Number: 68230
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Sat, 08 Oct 2022 13:23:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/68239 )
Change subject: rayer_spi.c: Get rid of temporary `prog_type` string
......................................................................
rayer_spi.c: Get rid of temporary `prog_type` string
Make the `get_params()` function provide a pointer to `struct
rayer_programmer` directly, instead of having a `prog_type` string
passed around three functions.
Change-Id: I83e34382ee9814f224025e21e5099fdab73cee8c
Signed-off-by: Angel Pons <th3fanbus(a)gmail.com>
---
M rayer_spi.c
1 file changed, 58 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/39/68239/1
diff --git a/rayer_spi.c b/rayer_spi.c
index 6984099..8ebfb2d 100644
--- a/rayer_spi.c
+++ b/rayer_spi.c
@@ -225,49 +225,6 @@
.half_period = 0,
};
-static int get_params(const struct programmer_cfg *cfg, uint16_t *lpt_iobase, char **prog_type)
-{
- /* Pick a default value for the I/O base. */
- *lpt_iobase = 0x378;
- /* no programmer type specified. */
- *prog_type = NULL;
-
- /* Non-default port requested? */
- char *arg = extract_programmer_param_str(cfg, "iobase");
- if (arg) {
- char *endptr = NULL;
- unsigned long tmp = strtoul(arg, &endptr, 0);
- /* Port 0, port >0x10000, unaligned ports and garbage strings
- * are rejected.
- */
- if (!tmp || (tmp >= 0x10000) || (tmp & 0x3) ||
- (*endptr != '\0')) {
- /* Using ports below 0x100 is a really bad idea, and
- * should only be done if no port between 0x100 and
- * 0xfffc works due to routing issues.
- */
- msg_perr("Error: iobase= specified, but the I/O base "
- "given was invalid.\nIt must be a multiple of "
- "0x4 and lie between 0x100 and 0xfffc.\n");
- free(arg);
- return -1;
- } else {
- *lpt_iobase = (uint16_t)tmp;
- msg_pinfo("Non-default I/O base requested. This will "
- "not change the hardware settings.\n");
- }
- free(arg);
- }
-
- arg = extract_programmer_param_str(cfg, "type");
- if (arg) {
- *prog_type = strdup(arg);
- free(arg);
- }
-
- return 0;
-}
-
static const struct rayer_programmer *find_progtype(const char *prog_type)
{
if (!prog_type)
@@ -297,21 +254,58 @@
return prog;
}
+static int get_params(const struct programmer_cfg *cfg, uint16_t *lpt_iobase,
+ const struct rayer_programmer **prog)
+{
+ /* Pick a default value for the I/O base. */
+ *lpt_iobase = 0x378;
+ /* no programmer type specified. */
+ *prog = NULL;
+
+ /* Non-default port requested? */
+ char *arg = extract_programmer_param_str(cfg, "iobase");
+ if (arg) {
+ char *endptr = NULL;
+ unsigned long tmp = strtoul(arg, &endptr, 0);
+ /* Port 0, port >0x10000, unaligned ports and garbage strings
+ * are rejected.
+ */
+ if (!tmp || (tmp >= 0x10000) || (tmp & 0x3) ||
+ (*endptr != '\0')) {
+ /* Using ports below 0x100 is a really bad idea, and
+ * should only be done if no port between 0x100 and
+ * 0xfffc works due to routing issues.
+ */
+ msg_perr("Error: iobase= specified, but the I/O base "
+ "given was invalid.\nIt must be a multiple of "
+ "0x4 and lie between 0x100 and 0xfffc.\n");
+ free(arg);
+ return -1;
+ } else {
+ *lpt_iobase = (uint16_t)tmp;
+ msg_pinfo("Non-default I/O base requested. This will "
+ "not change the hardware settings.\n");
+ }
+ free(arg);
+ }
+
+ arg = extract_programmer_param_str(cfg, "type");
+ *prog = find_progtype(arg);
+ free(arg);
+
+ return *prog ? 0 : -1;
+}
+
static int rayer_spi_init(const struct programmer_cfg *cfg)
{
+ const struct rayer_programmer *prog;
struct rayer_pinout *pinout = NULL;
uint16_t lpt_iobase;
- char *prog_type;
if (rget_io_perms())
return 1;
- if (get_params(cfg, &lpt_iobase, &prog_type) < 0)
- return 1;
-
- const struct rayer_programmer *prog = find_progtype(prog_type);
- free(prog_type);
- if (!prog)
+ if (get_params(cfg, &lpt_iobase, &prog) < 0)
return 1;
msg_pdbg("Using address 0x%x as I/O base for parallel port access.\n",
--
To view, visit https://review.coreboot.org/c/flashrom/+/68239
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I83e34382ee9814f224025e21e5099fdab73cee8c
Gerrit-Change-Number: 68239
Gerrit-PatchSet: 1
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: newchange