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

Change in flashrom[master]: linux_spi.c: Refactor singleton states into reentrant pattern
by Anastasia Klimchuk (Code Review) 15 Apr '21

15 Apr '21

Attention is currently required from: Angel Pons, Anastasia Klimchuk. Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/52285 to look at the new patch set (#2). Change subject: linux_spi.c: Refactor singleton states into reentrant pattern ...................................................................... linux_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". TEST=builds BUG=b:140394053 Change-Id: I93408c2ca846fca6a1c7eda7180862c51bd48078 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M linux_spi.c 1 file changed, 33 insertions(+), 12 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/85/52285/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52285 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I93408c2ca846fca6a1c7eda7180862c51bd48078 Gerrit-Change-Number: 52285 Gerrit-PatchSet: 2 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Sam McNally <sammc(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: cli_classic: Make -r/-w/-v argument optional
by Daniel Campello (Code Review) 15 Apr '21

15 Apr '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52362 ) Change subject: cli_classic: Make -r/-w/-v argument optional ...................................................................... Patch Set 1: (1 comment) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/52362/comment/a038df9e_12a3bd05 PS1, Line 140: static char *get_optional_filename(char *argv[]) > Can you leave a comment on this function what precisely it is expected to do here. […] The diff is for a different functionality. In chromiumos you can specify a file (current flashrom), not specify a file (my proposed change here) and specify - (not in this change, and implemented by that diff) to used stdin and stdout instead of regular files for the fd. -- To view, visit https://review.coreboot.org/c/flashrom/+/52362 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I15384b92cb987a6ea1bb4369ef415488b9cf7f41 Gerrit-Change-Number: 52362 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Campello <campello(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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Thu, 15 Apr 2021 04:13:11 +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]: layout: Add -i <region>[:<file>] support
by Daniel Campello (Code Review) 15 Apr '21

15 Apr '21

Attention is currently required from: Sam McNally, Nico Huber, Nicola Corna, Paul Menzel, Stefan Reinauer, David Hendricks, Arthur Heymans, Carl-Daniel Hailfinger. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/23021 ) Change subject: layout: Add -i <region>[:<file>] support ...................................................................... Patch Set 27: (1 comment) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/23021/comment/012d23ac_6a9ef86b PS27, Line 324: free(tempstr); > Extremely minor and sorry to be a bother, do you mind splitting out this leak fix into a separate pa […] Not a leak fix... before the string was stored directly on the layout entry... Now I'm strdup it (see layout.c new line 145) so freeing it right after here -- To view, visit https://review.coreboot.org/c/flashrom/+/23021 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic5465659605d8431d931053967b40290195cfd99 Gerrit-Change-Number: 23021 Gerrit-PatchSet: 27 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nicola Corna <nicola(a)corna.info> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Nicola Corna <nicola(a)corna.info> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net> Gerrit-Comment-Date: Thu, 15 Apr 2021 04:10:38 +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]: layout: Add -i <region>[:<file>] support
by Edward O'Callaghan (Code Review) 15 Apr '21

15 Apr '21

Attention is currently required from: Sam McNally, Nico Huber, Nicola Corna, Paul Menzel, Stefan Reinauer, David Hendricks, Daniel Campello, Arthur Heymans, Carl-Daniel Hailfinger. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/23021 ) Change subject: layout: Add -i <region>[:<file>] support ...................................................................... Patch Set 27: Code-Review+2 (1 comment) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/23021/comment/0bbe6862_17c69124 PS27, Line 324: free(tempstr); Extremely minor and sorry to be a bother, do you mind splitting out this leak fix into a separate patch? -- To view, visit https://review.coreboot.org/c/flashrom/+/23021 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic5465659605d8431d931053967b40290195cfd99 Gerrit-Change-Number: 23021 Gerrit-PatchSet: 27 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nicola Corna <nicola(a)corna.info> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Nicola Corna <nicola(a)corna.info> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net> Gerrit-Comment-Date: Thu, 15 Apr 2021 04:05:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: cli_classic: Make -r/-w/-v argument optional
by Edward O'Callaghan (Code Review) 15 Apr '21

15 Apr '21

Attention is currently required from: Nico Huber, Angel Pons, Daniel Campello. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52362 ) Change subject: cli_classic: Make -r/-w/-v argument optional ...................................................................... Patch Set 1: Code-Review+2 (4 comments) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/52362/comment/f3874057_6b03ab9d PS1, Line 140: static char *get_optional_filename(char *argv[]) Can you leave a comment on this function what precisely it is expected to do here. Longer term as the flashrom team here works on improving unit-test coverage I guess things like this are nice candidates for folks to get started with. Therefore comments help them form the right tests. I left some comments in flashrom.c with the diff with how the chromiumos tree behaves with the file descriptors when the user doesn't specify them. I was wondering if you can speak to that diff more so that we can close the gap off there once this lands? File flashrom.c: https://review.coreboot.org/c/flashrom/+/52362/comment/4beaa0f6_50ea259d PS1, Line 1351: if ((image = fopen(filename, "rb")) == NULL) { ``` - if (!strncmp(filename, "-", sizeof("-"))) - image = fdopen(STDIN_FILENO, "rb"); - else - image = fopen(filename, "rb"); - if (image == NULL) { + if ((image = fopen(filename, "rb")) == NULL) { ``` https://review.coreboot.org/c/flashrom/+/52362/comment/1f3c9283_663aa16f PS1, Line 1361: : if (image_stat.st_size != (intmax_t)size) { ``` - if ((image_stat.st_size != (__off_t)size) && - (strncmp(filename, "-", sizeof("-")))) { + if (image_stat.st_size != (intmax_t)size) { ``` https://review.coreboot.org/c/flashrom/+/52362/comment/871adb21_c9c6dd74 PS1, Line 1439: if ((image = fopen(filename, "wb")) == NULL) { ``` - if (!strncmp(filename, "-", sizeof("-"))) - image = fdopen(STDOUT_FILENO, "wb"); - else - image = fopen(filename, "wb"); - if (image == NULL) { + if ((image = fopen(filename, "wb")) == NULL) { ``` -- To view, visit https://review.coreboot.org/c/flashrom/+/52362 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I15384b92cb987a6ea1bb4369ef415488b9cf7f41 Gerrit-Change-Number: 52362 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Campello <campello(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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Comment-Date: Thu, 15 Apr 2021 04:01:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: jlink_spi.c: Separate shutdown from failed init cleanup
by Anastasia Klimchuk (Code Review) 15 Apr '21

15 Apr '21

Attention is currently required from: Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52308 ) Change subject: jlink_spi.c: Separate shutdown from failed init cleanup ...................................................................... Patch Set 2: (1 comment) File jlink_spi.c: https://review.coreboot.org/c/flashrom/+/52308/comment/271bf53f_86a27b21 PS1, Line 181: init_ret > just call this ret and default to failure, that is to say initialise with 1 instead of 0. […] Default to failure: done. Makes sense, there are a lot of reasons for failure here. I did not rename because there is one variable called "ret" already (line 206 in the latest patch set). Renaming would produce lots of diffs, maybe I will do this later (not in this patch)? -- To view, visit https://review.coreboot.org/c/flashrom/+/52308 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I71f64ed38154af670d4d28b8c7914d87fbc75679 Gerrit-Change-Number: 52308 Gerrit-PatchSet: 2 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Sam McNally <sammc(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Thu, 15 Apr 2021 03:58:01 +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]: jlink_spi.c: Separate shutdown from failed init cleanup
by Anastasia Klimchuk (Code Review) 15 Apr '21

15 Apr '21

Attention is currently required from: Angel Pons, Anastasia Klimchuk. Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/52308 to look at the new patch set (#2). Change subject: jlink_spi.c: Separate shutdown from failed init cleanup ...................................................................... jlink_spi.c: Separate shutdown from failed init cleanup Shutdown function was covering two different jobs here: 1) the actual shutdown which is run at the end of the driver's lifecycle and 2) cleanup in cases when initialisation failed. Now, shutdown is only doing its main job (#1), and the driver itself is doing cleanup when init fails (#2). The good thing is that now resources are released/closed immediately in cases when init fails (vs shutdown function which was run at some point later), and the driver leaves clean space after itself if init fails. And very importantly this unlocks API change which plans to move register_shutdown inside register master API, see this https://review.coreboot.org/c/flashrom/+/51761 TEST=builds BUG=b:185191942 Change-Id: I71f64ed38154af670d4d28b8c7914d87fbc75679 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M jlink_spi.c 1 file changed, 30 insertions(+), 19 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/52308/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/52308 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I71f64ed38154af670d4d28b8c7914d87fbc75679 Gerrit-Change-Number: 52308 Gerrit-PatchSet: 2 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Sam McNally <sammc(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: flashrom.c: Fixup stale FIXME comment when doit() was removed
by Edward O'Callaghan (Code Review) 15 Apr '21

15 Apr '21

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52365 ) Change subject: flashrom.c: Fixup stale FIXME comment when doit() was removed ...................................................................... flashrom.c: Fixup stale FIXME comment when doit() was removed Once upon a time flashrom had a entry point function called doit(). Excise the last mention of it here so that we may never mention it again. BUG=none TEST=none Change-Id: I40d815b7154456c323b4230cd3fed2cc2e8e3641 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashrom.c 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/65/52365/1 diff --git a/flashrom.c b/flashrom.c index c89abad..056fc57 100644 --- a/flashrom.c +++ b/flashrom.c @@ -2189,8 +2189,8 @@ return ret; } -/* FIXME: This function signature needs to be improved once doit() has a better - * function signature. +/* FIXME: This function signature needs to be improved once prepare_flash_access() + * has a better function signature. */ static int chip_safety_check(const struct flashctx *flash, int force, int read_it, int write_it, int erase_it, int verify_it) -- To view, visit https://review.coreboot.org/c/flashrom/+/52365 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I40d815b7154456c323b4230cd3fed2cc2e8e3641 Gerrit-Change-Number: 52365 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: programmer.h,chipset_enable.c: Make penable cb more descript
by Edward O'Callaghan (Code Review) 15 Apr '21

15 Apr '21

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52364 ) Change subject: programmer.h,chipset_enable.c: Make penable cb more descript ...................................................................... programmer.h,chipset_enable.c: Make penable cb more descript The enable_flash callback function pointer embedded within the penable struct is used by the chipset_enable entry-point to dispatch to the correct concrete implementation. The handles have the canonical form `flash_enable_<chipset-name>()` and therefore `doit()` is hard to comprehend while groking code and seeing to what it refers to. It is also a little more confusing as there use to be another doit() in flashrom once upon a time. BUG=none TEST=builds Change-Id: I8b3a4ec51ea488c40c20ccafea883f8dea93577d Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M chipset_enable.c M programmer.h 2 files changed, 2 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/64/52364/1 diff --git a/chipset_enable.c b/chipset_enable.c index d5c10c4..fd8bec2 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -2138,7 +2138,7 @@ continue; } msg_pinfo("Enabling flash write... "); - ret = chipset_enables[i].doit(dev, chipset_enables[i].device_name); + ret = chipset_enables[i].enable_flash_xxx(dev, chipset_enables[i].device_name); if (ret == NOT_DONE_YET) { ret = -2; msg_pinfo("OK - searching further chips.\n"); diff --git a/programmer.h b/programmer.h index 29a100b..93540a6 100644 --- a/programmer.h +++ b/programmer.h @@ -225,7 +225,7 @@ const enum test_state status; const char *vendor_name; const char *device_name; - int (*doit) (struct pci_dev *dev, const char *name); + int (*enable_flash_xxx) (struct pci_dev *dev, const char *name); }; extern const struct penable chipset_enables[]; -- To view, visit https://review.coreboot.org/c/flashrom/+/52364 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8b3a4ec51ea488c40c20ccafea883f8dea93577d Gerrit-Change-Number: 52364 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

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

15 Apr '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/52256 ) Change subject: ft2232_spi.c: Refactor singleton states into reentrant pattern ...................................................................... ft2232_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". TEST=builds BUG=b:140394053 Change-Id: I67518a58b4f35e0edaf06ac09c9374bdf06db0df Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/52256 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M ft2232_spi.c 1 file changed, 57 insertions(+), 34 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved diff --git a/ft2232_spi.c b/ft2232_spi.c index 1fb795b..662dca5 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -98,13 +98,15 @@ * The pin signal direction bit offsets follow the same order; 0 means that * pin at the matching bit index is an input, 1 means pin is an output. * - * The default values (set below) are used for most devices: + * The default values (set below in ft2232_spi_init) are used for most devices: * value: 0x08 CS=high, DI=low, DO=low, SK=low * dir: 0x0b CS=output, DI=input, DO=output, SK=output */ -static uint8_t pinlvl = 0x08; -static uint8_t pindir = 0x0b; -static struct ftdi_context ftdic_context; +struct ft2232_data { + uint8_t pinlvl; + uint8_t pindir; + struct ftdi_context ftdic_context; +}; static const char *get_ft2232_devicename(int ft2232_vid, int ft2232_type) { @@ -162,7 +164,8 @@ static int ft2232_shutdown(void *data) { int f; - struct ftdi_context *ftdic = (struct ftdi_context *) data; + struct ft2232_data *spi_data = (struct ft2232_data *) data; + struct ftdi_context *ftdic = &spi_data->ftdic_context; unsigned char buf[3]; msg_pdbg("Releasing I/Os\n"); @@ -180,6 +183,7 @@ return f; } + free(spi_data); return 0; } @@ -189,7 +193,8 @@ const unsigned char *writearr, unsigned char *readarr) { - struct ftdi_context *ftdic = &ftdic_context; + struct ft2232_data *spi_data = flash->mst->spi.data; + struct ftdi_context *ftdic = &spi_data->ftdic_context; static unsigned char *buf = NULL; /* failed is special. We use bitwise ops, but it is essentially bool. */ int i = 0, ret = 0, failed = 0; @@ -220,8 +225,8 @@ */ msg_pspew("Assert CS#\n"); buf[i++] = SET_BITS_LOW; - buf[i++] = ~ 0x08 & pinlvl; /* assert CS (3rd) bit only */ - buf[i++] = pindir; + buf[i++] = ~ 0x08 & spi_data->pinlvl; /* assert CS (3rd) bit only */ + buf[i++] = spi_data->pindir; if (writecnt) { buf[i++] = MPSSE_DO_WRITE | MPSSE_WRITE_NEG; @@ -261,8 +266,8 @@ msg_pspew("De-assert CS#\n"); buf[i++] = SET_BITS_LOW; - buf[i++] = pinlvl; - buf[i++] = pindir; + buf[i++] = spi_data->pinlvl; + buf[i++] = spi_data->pindir; ret = send_buf(ftdic, buf, i); failed |= ret; if (ret) @@ -271,7 +276,7 @@ return failed ? -1 : 0; } -static const struct spi_master spi_master_ft2232 = { +static struct spi_master spi_master_ft2232 = { .features = SPI_MASTER_4BA, .max_data_read = 64 * 1024, .max_data_write = 256, @@ -286,7 +291,6 @@ int ft2232_spi_init(void) { int ret = 0; - struct ftdi_context *ftdic = &ftdic_context; unsigned char buf[512]; int ft2232_vid = FTDI_VID; int ft2232_type = FTDI_FT4232H_PID; @@ -312,6 +316,11 @@ char *arg; double mpsse_clk; + uint8_t pinlvl = 0x08; + uint8_t pindir = 0x0b; + struct ftdi_context ftdic; + struct ft2232_data *spi_data; + arg = extract_programmer_param("type"); if (arg) { if (!strcasecmp(arg, "2232H")) { @@ -521,50 +530,50 @@ (ft2232_interface == INTERFACE_B) ? "B" : (ft2232_interface == INTERFACE_C) ? "C" : "D"); - if (ftdi_init(ftdic) < 0) { + if (ftdi_init(&ftdic) < 0) { msg_perr("ftdi_init failed.\n"); return -3; } - if (ftdi_set_interface(ftdic, ft2232_interface) < 0) { - msg_perr("Unable to select channel (%s).\n", ftdi_get_error_string(ftdic)); + if (ftdi_set_interface(&ftdic, ft2232_interface) < 0) { + msg_perr("Unable to select channel (%s).\n", ftdi_get_error_string(&ftdic)); } arg = extract_programmer_param("serial"); - f = ftdi_usb_open_desc(ftdic, ft2232_vid, ft2232_type, NULL, arg); + f = ftdi_usb_open_desc(&ftdic, ft2232_vid, ft2232_type, NULL, arg); free(arg); if (f < 0 && f != -5) { msg_perr("Unable to open FTDI device: %d (%s)\n", f, - ftdi_get_error_string(ftdic)); + ftdi_get_error_string(&ftdic)); return -4; } - if (ftdic->type != TYPE_2232H && ftdic->type != TYPE_4232H && ftdic->type != TYPE_232H) { - msg_pdbg("FTDI chip type %d is not high-speed.\n", ftdic->type); + if (ftdic.type != TYPE_2232H && ftdic.type != TYPE_4232H && ftdic.type != TYPE_232H) { + msg_pdbg("FTDI chip type %d is not high-speed.\n", ftdic.type); clock_5x = 0; } - if (ftdi_usb_reset(ftdic) < 0) { - msg_perr("Unable to reset FTDI device (%s).\n", ftdi_get_error_string(ftdic)); + if (ftdi_usb_reset(&ftdic) < 0) { + msg_perr("Unable to reset FTDI device (%s).\n", ftdi_get_error_string(&ftdic)); } - if (ftdi_set_latency_timer(ftdic, 2) < 0) { - msg_perr("Unable to set latency timer (%s).\n", ftdi_get_error_string(ftdic)); + if (ftdi_set_latency_timer(&ftdic, 2) < 0) { + msg_perr("Unable to set latency timer (%s).\n", ftdi_get_error_string(&ftdic)); } - if (ftdi_write_data_set_chunksize(ftdic, 270)) { - msg_perr("Unable to set chunk size (%s).\n", ftdi_get_error_string(ftdic)); + if (ftdi_write_data_set_chunksize(&ftdic, 270)) { + msg_perr("Unable to set chunk size (%s).\n", ftdi_get_error_string(&ftdic)); } - if (ftdi_set_bitmode(ftdic, 0x00, BITMODE_BITBANG_SPI) < 0) { - msg_perr("Unable to set bitmode to SPI (%s).\n", ftdi_get_error_string(ftdic)); + if (ftdi_set_bitmode(&ftdic, 0x00, BITMODE_BITBANG_SPI) < 0) { + msg_perr("Unable to set bitmode to SPI (%s).\n", ftdi_get_error_string(&ftdic)); } if (clock_5x) { msg_pdbg("Disable divide-by-5 front stage\n"); buf[0] = DIS_DIV_5; - if (send_buf(ftdic, buf, 1)) { + if (send_buf(&ftdic, buf, 1)) { ret = -5; goto ftdi_err; } @@ -577,7 +586,7 @@ buf[0] = TCK_DIVISOR; buf[1] = (divisor / 2 - 1) & 0xff; buf[2] = ((divisor / 2 - 1) >> 8) & 0xff; - if (send_buf(ftdic, buf, 3)) { + if (send_buf(&ftdic, buf, 3)) { ret = -6; goto ftdi_err; } @@ -588,7 +597,7 @@ /* Disconnect TDI/DO to TDO/DI for loopback. */ msg_pdbg("No loopback of TDI/DO TDO/DI\n"); buf[0] = LOOPBACK_END; - if (send_buf(ftdic, buf, 1)) { + if (send_buf(&ftdic, buf, 1)) { ret = -7; goto ftdi_err; } @@ -597,20 +606,34 @@ buf[0] = SET_BITS_LOW; buf[1] = pinlvl; buf[2] = pindir; - if (send_buf(ftdic, buf, 3)) { + if (send_buf(&ftdic, buf, 3)) { ret = -8; goto ftdi_err; } - register_shutdown(ft2232_shutdown, ftdic); + spi_data = calloc(1, sizeof(*spi_data)); + if (!spi_data) { + msg_perr("Unable to allocate space for SPI master data\n"); + return SPI_GENERIC_ERROR; + } + spi_data->pinlvl = pinlvl; + spi_data->pindir = pindir; + spi_data->ftdic_context = ftdic; + + spi_master_ft2232.data = spi_data; + + if (register_shutdown(ft2232_shutdown, spi_data)) { + free(spi_data); + goto ftdi_err; + } register_spi_master(&spi_master_ft2232); return 0; ftdi_err: - if ((f = ftdi_usb_close(ftdic)) < 0) { + if ((f = ftdi_usb_close(&ftdic)) < 0) { msg_perr("Unable to close FTDI device: %d (%s)\n", f, - ftdi_get_error_string(ftdic)); + ftdi_get_error_string(&ftdic)); } return ret; } -- To view, visit https://review.coreboot.org/c/flashrom/+/52256 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I67518a58b4f35e0edaf06ac09c9374bdf06db0df Gerrit-Change-Number: 52256 Gerrit-PatchSet: 4 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2021