- flashrom-gerrit - mail.coreboot.org

[M] Change in flashrom[main]: buspirate_spi: add support for hiz output with pullups=off
by David Reguera Garcia (Code Review) 07 Dec '23

07 Dec '23

Attention is currently required from: Anastasia Klimchuk, David Reguera Garcia, Thomas Heijligen. Hello Anastasia Klimchuk, Thomas Heijligen, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/79299?usp=email to look at the new patch set (#9). The following approvals got outdated and were removed: Code-Review+1 by Anastasia Klimchuk, Code-Review+1 by David Reguera Garcia, Verified+1 by build bot (Jenkins) Change subject: buspirate_spi: add support for hiz output with pullups=off ...................................................................... buspirate_spi: add support for hiz output with pullups=off When working with low-voltage chips, the internal 10k pull-ups of the Bus Pirate might be too high. In such cases, it's necessary to create an external pull-up using lower-value resistors. For this, you can use the 'hiz' parameter. This way, the Bus Pirate will operate as an open drain with 'pullups=off' So, why not just use the 'pullups=on' option for this scenario? I'm trying to prevent a very typical human error in the training sessions I conduct. For example, in the previous session, a user might have left the VPU (vextern) connected to 5V, and now they need to access a 1.8V chip. If 'pullups=on' is used, the following will happen: 5V (VPU) --> CD4060 --> 2K (internal Bus Pirate) --> MOSI target chip. By having the option to set a 'hiz=on', this can be avoided. Since the pull-ups will remain deactivated while the Bus Pirate will function in an open-drain mode Return init error for invalid values of pullups, hiz, psus. Previously, invalid values of the params pullups, hiz, psus were handled as "off" (i.e. disabled). This created a potential human error when users were running flashrom with e.g. pullups=1 and expected pullups to be on, while the value 1 was handled as invalid and off. Change-Id: I6d9909b23782ef3fcb1428fd127dae0d6b211855 Signed-off-by: David Reguera Garcia <dreg(a)rootkit.es> --- M buspirate_spi.c M doc/classic_cli_manpage.rst 2 files changed, 47 insertions(+), 10 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/99/79299/9 -- To view, visit https://review.coreboot.org/c/flashrom/+/79299?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6d9909b23782ef3fcb1428fd127dae0d6b211855 Gerrit-Change-Number: 79299 Gerrit-PatchSet: 9 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[main]: buspirate_spi: add support for hiz output with pullups=off
by Anastasia Klimchuk (Code Review) 07 Dec '23

07 Dec '23

Attention is currently required from: David Reguera Garcia, Thomas Heijligen. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79299?usp=email ) Change subject: buspirate_spi: add support for hiz output with pullups=off ...................................................................... Patch Set 8: Code-Review+1 (5 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/79299/comment/eb9f8f34_4cf3a7b1 : PS8, Line 12: Since you merged two patches together, you can add a paragraph from the other patch here, you can just copy-paste (with 72 chars width) > Return init error for invalid values of pullups, hiz, psus. Previously, invalid values of the params pullups, hiz, psus were handled as "off" (i.e. disabled). This created a potential human error when users were running flashrom with e.g. `pullups=1` and expected pullups to be on, while the value `1` was handled as invalid and off. Is the current text in commit message 72 chars width? Patchset: PS8: > Anastasia, can you take a look now? Great, thank you! I have few comments, which are just code style and commit message, and that would be it! File buspirate_spi.c: https://review.coreboot.org/c/flashrom/+/79299/comment/cb7f0137_cee9d4d9 : PS8, Line 374: if (strcasecmp("on", tmp) == 0) : pullup = true; : else if (strcasecmp("off", tmp) == 0) : ; // ignore : else { : msg_perr("Invalid pullups state. Use on/off.\n"); : free(tmp); : return 1; : } For this if-else-elseif and the ones below: As per coding style, if at least one branch is multi-line and needs braces, put braces for all branches. Previously, all the branches were 1-line, and no braces. Since now you added a multi-line else branch, you need to add braces for existing if and else if. You don't need to go through the whole file and check everything! Just update the places that you modified in the patch. (it is p.3 from here https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Doc…) https://review.coreboot.org/c/flashrom/+/79299/comment/e92bc517_cc6a0b42 : PS8, Line 390: else if (strcasecmp("off", tmp) == 0) : { : if (pullup) : { I think this can be more compact: else if ((strcasecmp("off", tmp) == 0) && pullup) { error message, free, return } else { ... <same as now> } https://review.coreboot.org/c/flashrom/+/79299/comment/e1053fea_32db6e1a : PS8, Line 394: pullups on & hiz off at same time is not possible Just slightly add on this message: > Invalid combination: pullups=on & hiz=off at same time is not possible.\n -- To view, visit https://review.coreboot.org/c/flashrom/+/79299?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6d9909b23782ef3fcb1428fd127dae0d6b211855 Gerrit-Change-Number: 79299 Gerrit-PatchSet: 8 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> 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: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Comment-Date: Thu, 07 Dec 2023 03:10:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: buspirate_spi: add support for hiz output with pullups=off
by David Reguera Garcia (Code Review) 07 Dec '23

07 Dec '23

Attention is currently required from: Anastasia Klimchuk, Thomas Heijligen. David Reguera Garcia has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79299?usp=email ) Change subject: buspirate_spi: add support for hiz output with pullups=off ...................................................................... Patch Set 8: (1 comment) Patchset: PS8: Anastasia, can you take a look now? -- To view, visit https://review.coreboot.org/c/flashrom/+/79299?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6d9909b23782ef3fcb1428fd127dae0d6b211855 Gerrit-Change-Number: 79299 Gerrit-PatchSet: 8 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 07 Dec 2023 02:23:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: buspirate_spi: add support for hiz output with pullups=off
by David Reguera Garcia (Code Review) 07 Dec '23

07 Dec '23

Attention is currently required from: Anastasia Klimchuk, Thomas Heijligen. David Reguera Garcia has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79299?usp=email ) Change subject: buspirate_spi: add support for hiz output with pullups=off ...................................................................... Patch Set 8: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/79299?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6d9909b23782ef3fcb1428fd127dae0d6b211855 Gerrit-Change-Number: 79299 Gerrit-PatchSet: 8 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 07 Dec 2023 02:23:32 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: buspirate_spi: add support for hiz output with pullups=off
by David Reguera Garcia (Code Review) 07 Dec '23

07 Dec '23

Attention is currently required from: Anastasia Klimchuk, David Reguera Garcia, Thomas Heijligen. Hello Anastasia Klimchuk, Thomas Heijligen, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/79299?usp=email to look at the new patch set (#8). The following approvals got outdated and were removed: Code-Review+1 by David Reguera Garcia, Verified+1 by build bot (Jenkins) Change subject: buspirate_spi: add support for hiz output with pullups=off ...................................................................... buspirate_spi: add support for hiz output with pullups=off When working with low-voltage chips, the internal 10k pull-ups of the Bus Pirate might be too high. In such cases, it's necessary to create an external pull-up using lower-value resistors. For this, you can use the 'hiz' parameter. This way, the Bus Pirate will operate as an open drain with 'pullups=off' So, why not just use the 'pullups=on' option for this scenario? I'm trying to prevent a very typical human error in the training sessions I conduct. For example, in the previous session, a user might have left the VPU (vextern) connected to 5V, and now they need to access a 1.8V chip. If 'pullups=on' is used, the following will happen: 5V (VPU) --> CD4060 --> 2K (internal Bus Pirate) --> MOSI target chip. By having the option to set a 'hiz=on', this can be avoided. Since the pull-ups will remain deactivated while the Bus Pirate will function in an open-drain mode Change-Id: I6d9909b23782ef3fcb1428fd127dae0d6b211855 Signed-off-by: David Reguera Garcia <dreg(a)rootkit.es> --- M buspirate_spi.c M doc/classic_cli_manpage.rst 2 files changed, 42 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/99/79299/8 -- To view, visit https://review.coreboot.org/c/flashrom/+/79299?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6d9909b23782ef3fcb1428fd127dae0d6b211855 Gerrit-Change-Number: 79299 Gerrit-PatchSet: 8 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> 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: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[main]: buspirate_spi: add support for hiz output with pullups=off
by David Reguera Garcia (Code Review) 07 Dec '23

07 Dec '23

David Reguera Garcia has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/79407?usp=email ) Change subject: buspirate_spi: add support for hiz output with pullups=off ...................................................................... Abandoned -- To view, visit https://review.coreboot.org/c/flashrom/+/79407?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I8944598b266fd77ee041f0ab03724655714a3344 Gerrit-Change-Number: 79407 Gerrit-PatchSet: 3 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: abandon

1 0

[S] Change in flashrom[main]: buspirate_spi: add support for hiz output with pullups=off
by David Reguera Garcia (Code Review) 07 Dec '23

07 Dec '23

Attention is currently required from: Anastasia Klimchuk. Hello Anastasia Klimchuk, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/79407?usp=email to look at the new patch set (#3). Change subject: buspirate_spi: add support for hiz output with pullups=off ...................................................................... buspirate_spi: add support for hiz output with pullups=off When working with low-voltage chips, the internal 10k pull-ups of the Bus Pirate might be too high. In such cases, it's necessary to create an external pull-up using lower-value resistors. For this, you can use the 'hiz' parameter. This way, the Bus Pirate will operate as an open drain with 'pullups=off' So, why not just use the 'pullups=on' option for this scenario? I'm trying to prevent a very typical human error in the training sessions I conduct. For example, in the previous session, a user might have left the VPU (vextern) connected to 5V, and now they need to access a 1.8V chip. If 'pullups=on' is used, the following will happen: 5V (VPU) --> CD4060 --> 2K (internal Bus Pirate) --> MOSI target chip. By having the option to set a 'hiz=on', this can be avoided. Since the pull-ups will remain deactivated while the Bus Pirate will function in an open-drain mode Previously, invalid values of the params pullups, hiz, psus were handled as "off" (i.e. disabled). This created a potential human error when users were running flashrom with e.g. pullups=1 and expected pullups to be on, while the value 1 was handled as invalid and off. Change-Id: I8944598b266fd77ee041f0ab03724655714a3344 Signed-off-by: David Reguera Garcia <dreg(a)rootkit.es> --- M buspirate_spi.c 1 file changed, 15 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/07/79407/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/79407?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I8944598b266fd77ee041f0ab03724655714a3344 Gerrit-Change-Number: 79407 Gerrit-PatchSet: 3 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[main]: buspirate_spi: return init error for invalid values of pullups, hiz, ...
by David Reguera Garcia (Code Review) 07 Dec '23

07 Dec '23

Attention is currently required from: Anastasia Klimchuk. David Reguera Garcia has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79407?usp=email ) Change subject: buspirate_spi: return init error for invalid values of pullups, hiz, psus ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/79407/comment/9ea7a1c6_375c097f : PS2, Line 7: buspirate_spi: return init error for invalid values of pullups, hiz, psus > Oh one extra char! Commit title and message need to fit into 72 chars width. […] oh sorry, I am a newbiew with this :( -- To view, visit https://review.coreboot.org/c/flashrom/+/79407?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I8944598b266fd77ee041f0ab03724655714a3344 Gerrit-Change-Number: 79407 Gerrit-PatchSet: 2 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 07 Dec 2023 02:02:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: buspirate_spi: Add support for hiz output with pullups=off
by David Reguera Garcia (Code Review) 07 Dec '23

07 Dec '23

Attention is currently required from: Anastasia Klimchuk, Thomas Heijligen. David Reguera Garcia has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79299?usp=email ) Change subject: buspirate_spi: Add support for hiz output with pullups=off ...................................................................... Patch Set 7: (3 comments) File doc/classic_cli_manpage.rst: https://review.coreboot.org/c/flashrom/+/79299/comment/42d87e20_bf4fccf0 : PS7, Line 765: : When working with low-voltage chips, the internal 10k pull-ups of the Bus Pirate might be too high. > Could you swap this new paragraph about `hiz` with existing about `pullups` param? […] Done https://review.coreboot.org/c/flashrom/+/79299/comment/a10b3b89_14549de5 : PS7, Line 766: > trailing whitespace, but you don't need a new line here, you can make new line after the end of sent […] Done https://review.coreboot.org/c/flashrom/+/79299/comment/1c67cb5e_ce190c3e : PS7, Line 767: > trailing whitespace Done -- To view, visit https://review.coreboot.org/c/flashrom/+/79299?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6d9909b23782ef3fcb1428fd127dae0d6b211855 Gerrit-Change-Number: 79299 Gerrit-PatchSet: 7 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 07 Dec 2023 02:00:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: buspirate_spi: return init error for invalid values of pullups, hiz, ...
by Anastasia Klimchuk (Code Review) 07 Dec '23

07 Dec '23

Attention is currently required from: David Reguera Garcia. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/79407?usp=email ) Change subject: buspirate_spi: return init error for invalid values of pullups, hiz, psus ...................................................................... Patch Set 2: Code-Review+2 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/79407/comment/936be066_b259fb66 : PS2, Line 7: buspirate_spi: return init error for invalid values of pullups, hiz, psus Oh one extra char! Commit title and message need to fit into 72 chars width. Since this is just one char, maybe just save on spaces, either remove one space after `buspirate_spi:` or replace the end with `pullups/hiz/psus`. Sorry, that was me suggesting this commit title, and I didn't even counted chars properly! I noticed you deleted the old patch and created a new one. You don't have to do this, you can push new patchset to the same patch. Locally, you amend the commit, and then push again. Keep an eye that `Change-Id` is the same, so that Gerrit knows which patch it is. Just in case, this is in the docs here: https://www.flashrom.org/dev_guide/development_guide.html -- To view, visit https://review.coreboot.org/c/flashrom/+/79407?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I8944598b266fd77ee041f0ab03724655714a3344 Gerrit-Change-Number: 79407 Gerrit-PatchSet: 2 Gerrit-Owner: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: David Reguera Garcia <dreg(a)rootkit.es> Gerrit-Comment-Date: Thu, 07 Dec 2023 01:50:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit