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

Change in flashrom[master]: rayer_spi.c: Implement `set_sck_set_mosi` operation
by Angel Pons (Code Review) 21 Jun '21

21 Jun '21

Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/49265 to look at the new patch set (#2). Change subject: rayer_spi.c: Implement `set_sck_set_mosi` operation ...................................................................... rayer_spi.c: Implement `set_sck_set_mosi` operation Change-Id: Ieb05a35967685c2444d064cb61cec98e2a00f590 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M rayer_spi.c 1 file changed, 12 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/65/49265/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/49265 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ieb05a35967685c2444d064cb61cec98e2a00f590 Gerrit-Change-Number: 49265 Gerrit-PatchSet: 2 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Shawn C <citypw(a)hardenedlinux.org> Gerrit-CC: Xiang W <wxjstz(a)126.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops
by Angel Pons (Code Review) 21 Jun '21

21 Jun '21

Attention is currently required from: Nico Huber, Michał Żygowski. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 ) Change subject: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops ...................................................................... Patch Set 1: (41 comments) File Makefile: https://review.coreboot.org/c/flashrom/+/55715/comment/dc9687ac_9ce8aa8c PS1, Line 409: ifeq ($(CONFIG_TUXEDO_EC), yes) : UNSUPPORTED_FEATURES += CONFIG_TUXEDO_EC=yes : else : override CONFIG_TUXEDO_EC = no : endif Any reason why this programmer wouldn't work with libpayload? I'm not asking you to test it, just remove this if there's no particular reason for it. https://review.coreboot.org/c/flashrom/+/55715/comment/da05f5ed_876b75d3 PS1, Line 839: override CONFIG_TUXEDO_EC = no hmmm, but this programmer does not use libpci? https://review.coreboot.org/c/flashrom/+/55715/comment/d8769ee6_62d5e9a5 PS1, Line 903: # Disable internal DMI decoder to be able to locate DMI tables on EFI systems and match TUXEDO laptops Why? File flashrom.8.tmpl: https://review.coreboot.org/c/flashrom/+/55715/comment/a5393331_c54e324f PS1, Line 648: .SS Was this removed on purpose? https://review.coreboot.org/c/flashrom/+/55715/comment/69a4426a_d31bcf81 PS1, Line 1399: autoloading What's autoloading? https://review.coreboot.org/c/flashrom/+/55715/comment/01531e8b_dea281c7 PS1, Line 1411: ite5570 How would the reader of the man page know when to enable IT5570 support? File flashrom.c: https://review.coreboot.org/c/flashrom/+/55715/comment/f7933f94_db2063d4 PS1, Line 42: #if CONFIG_TUXEDO_EC == 1 I'd appreciate if we could avoid using preprocessor. https://review.coreboot.org/c/flashrom/+/55715/comment/bc9eb3c6_9ac04a48 PS1, Line 2109: #if CONFIG_TUXEDO_EC == 1 I can see why you'd want to do this, but tacking it right here feels odd. File tuxedo_ec.c: https://review.coreboot.org/c/flashrom/+/55715/comment/2bea369a_7368e169 PS1, Line 68: typedef struct Typedefs are frowned upon: https://www.kernel.org/doc/html/latest/process/coding-style.html#typedefs https://review.coreboot.org/c/flashrom/+/55715/comment/a87e95b9_99fce5f1 PS1, Line 184: flashrom(a)flashrom.org Personally, I wouldn't know what to do with such a report on the mailing list. https://review.coreboot.org/c/flashrom/+/55715/comment/3c9d2b16_ea6ced3f PS1, Line 179: msg_pwarn("Querying EC ROM size returned unexpected result.\n" : "Probably the EC has just been flashed and the EC " : "RAM has been reset.\nYou may need to pass the flash" : " size via the programmer parameters or try again in" : " a while.\nIf this warning persists please email a " : "report to flashrom(a)flashrom.org\n"); Breaking user-visible strings makes it harder to grep for them. How about: "Querying EC ROM size returned unexpected result.\n" "Probably the EC has just been flashed and the EC RAM has been reset.\n" "You may need to pass the flash size via the programmer parameters or try again in a while.\n" "If this warning persists please email a report to flashrom(a)flashrom.org\n" https://review.coreboot.org/c/flashrom/+/55715/comment/cf1d674d_5df7f5e7 PS1, Line 205: ec_read_reg Why `ec_read_reg` and not `tuxedo_ec_read_byte`? https://review.coreboot.org/c/flashrom/+/55715/comment/42ac0440_6399bf29 PS1, Line 205: if (!ec_read_reg(0x10, &reg_value)) { : msg_perr("Failed to query first byte of state register.\n"); : return false; : } : if (!ec_read_reg(0x10, &reg_value)) { : msg_perr("Failed to query AC adapter state.\n"); : return false; : } Do I understand correctly that reading this register results in a side-effect? How does one know which is the first byte? https://review.coreboot.org/c/flashrom/+/55715/comment/f8211bb5_f7813961 PS1, Line 235: len % BYTES_PER_BLOCK != 0 && len < BYTES_PER_BLOCK Are you sure this check is supposed to use an AND? Shouldn't it be checking that the length is a multiple of `BYTES_PER_BLOCK`? if (!len || len % BYTES_PER_BLOCK != 0) { https://review.coreboot.org/c/flashrom/+/55715/comment/26360eae_af77017d PS1, Line 286: return tuxedo_ec_write_byte(ctx_data, data); `break;` would be equivalent and easier to understand. https://review.coreboot.org/c/flashrom/+/55715/comment/0959c5c0_39a08140 PS1, Line 316: unsigned int uint8_t ? https://review.coreboot.org/c/flashrom/+/55715/comment/034d8966_16164f54 PS1, Line 318: unsigned int uint8_t ? https://review.coreboot.org/c/flashrom/+/55715/comment/b36e6ff3_2957ea64 PS1, Line 324: CHUNKS_PER_KBYTE * BYTES_PER_CHUNK sizeof(ctx_data->first_kbyte) https://review.coreboot.org/c/flashrom/+/55715/comment/4dfbc001_8529b8a6 PS1, Line 361: CHUNKS_PER_KBYTE * BYTES_PER_CHUNK sizeof(ctx_data->first_kbyte) https://review.coreboot.org/c/flashrom/+/55715/comment/6d2a8098_7beb2773 PS1, Line 384: offset `start + i`? https://review.coreboot.org/c/flashrom/+/55715/comment/0a032c47_8bd7f0f1 PS1, Line 387: offset - ctx_data->autoload_offset != 1 Can these checks ever be true? The state is reset to `AUTOLOAD_NONE` already. https://review.coreboot.org/c/flashrom/+/55715/comment/47b58b86_62653769 PS1, Line 433: len % BYTES_PER_BLOCK != 0 && len < BYTES_PER_BLOCK Are you sure this check is supposed to use an AND? Shouldn't it be checking that the length is a multiple of `BYTES_PER_BLOCK`? if (!len || len % BYTES_PER_BLOCK != 0) { https://review.coreboot.org/c/flashrom/+/55715/comment/33cb1117_82a6511c PS1, Line 477: internal_sleep(15000 * 64); No way to poll for completion? https://review.coreboot.org/c/flashrom/+/55715/comment/647dcced_84be2ca9 PS1, Line 482: if (!ec_wait_for_obuf(ctx_data->control_port, : EC_MAX_STATUS_CHECKS * 3)) : return 1; : : if (INB(ctx_data->data_port) == 0xf8) : return 0; Looks like this is open-coding `tuxedo_ec_read_byte` because the `max_checks` parameter isn't exposed. https://review.coreboot.org/c/flashrom/+/55715/comment/4943ce49_24efca15 PS1, Line 504: ++to_chunk This would make flashrom erase too much? https://review.coreboot.org/c/flashrom/+/55715/comment/05c505d6_d9d25bad PS1, Line 534: legnth typo: length https://review.coreboot.org/c/flashrom/+/55715/comment/74409476_f24632ee PS1, Line 551: ctx_data->rom_size_in_kbytes This is the only place where `rom_size_in_kbytes` is used. Given that it provides redundant information, why not drop the member and use `ctx_data->rom_size_in_blocks * KBYTES_PER_BLOCK` here? Oh wait, this is what the block eraser size already does. If you don't want to repeat the calculation twice (it currently appears three times), make `rom_size_in_kbytes` a local constant inside this function. https://review.coreboot.org/c/flashrom/+/55715/comment/a60e1d93_eee2cd83 PS1, Line 555: * Erase operation must be done in one sway. Doesn't seem to be the case for IT5570? The existing code doesn't allow making use of this, though. https://review.coreboot.org/c/flashrom/+/55715/comment/bf51e078_dac2d240 PS1, Line 581: p = extract_programmer_param("type"); : if (p && strcmp(p, "ec")) { : msg_pdbg("%s(): tuxedo_ec only supports \"ec\" type devices\n", : __func__); : ret = false; : } : free(p); Remove this, please. I don't expect anything else to be supported in the foreseeable future, and this could be added later anyway. https://review.coreboot.org/c/flashrom/+/55715/comment/b093cce1_f1150854 PS1, Line 666: static void get_flash_part_from_id(tuxedo_ec_data_t *ctx_data, uint32_t manuf_id, Please ignore. I left a comment here before, but Gerrit doesn't let me delete the draft... https://review.coreboot.org/c/flashrom/+/55715/comment/c744ec55_8dde3553 PS1, Line 713: if (id_length == 4) If you zero-initialise the array, this "if" can be removed. https://review.coreboot.org/c/flashrom/+/55715/comment/2c5bd501_d88ca095 PS1, Line 722: = NULL Dead assignment https://review.coreboot.org/c/flashrom/+/55715/comment/bdadb228_0989ce42 PS1, Line 734: if (!dmi_match("^InfinityBook S 14 Gen6$") && : !dmi_match("^InfinityBook S 15 Gen6$")) { To ease adding support for more laptops in the future, put the strings in an array and loop over it? https://review.coreboot.org/c/flashrom/+/55715/comment/9b2a2cd9_1609d200 PS1, Line 750: extra extra in which sense? https://review.coreboot.org/c/flashrom/+/55715/comment/7f79f33c_1a350a6f PS1, Line 792: if (register_opaque_master(&programmer_opaque_tuxedo_ec, ctx_data)) : return 1; : : msg_pdbg("%s(): successfully initialized tuxedo_ec\n", __func__); : return 0; Drop the print and return `register_opaque_master` directly? https://review.coreboot.org/c/flashrom/+/55715/comment/c8481420_c7fb292c PS1, Line 798: tuxedo_ec_init_exit: : tuxedo_ec_shutdown(ctx_data); : return 1; : : tuxedo_ec_init_exit_shutdown: : tuxedo_ec_shutdown(ctx_data); : return 1; Am I missing something, or are these two paths identical? https://review.coreboot.org/c/flashrom/+/55715/comment/c7c4c308_a88d4f21 PS1, Line 825: nto typo: not https://review.coreboot.org/c/flashrom/+/55715/comment/0ed47353_dcc878cf PS1, Line 826: Ifthe missing a space https://review.coreboot.org/c/flashrom/+/55715/comment/5c0ba19e_d008b047 PS1, Line 827: If the current and the file's EC version is identical, : * return two (skip flashing) Is there any reason to skip flashing if the EC version matches? flashrom already checks if the current contents already match, and skipping flashing when only the version matches could prevent recovering from a botched flash. https://review.coreboot.org/c/flashrom/+/55715/comment/f93ebc00_f2ab9df9 PS1, Line 832: Contects typo: Contents https://review.coreboot.org/c/flashrom/+/55715/comment/50da9b94_3d0fe6cc PS1, Line 904: .name = "tuxedo_ec", : .type = OTHER, : .devs.note = "Embedded Controller of TUXEDO laptops.\n", : .init = tuxedo_ec_init, : .map_flash_region = fallback_map, : .unmap_flash_region = fallback_unmap, : .delay = internal_delay, One tab too many in the indentation? -- To view, visit https://review.coreboot.org/c/flashrom/+/55715 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0e42260155ffea38a6f60790871cd8da7b657031 Gerrit-Change-Number: 55715 Gerrit-PatchSet: 1 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Comment-Date: Mon, 21 Jun 2021 10:45:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: acpi_ec: Implement basic ACPI embedded controller API
by Michał Żygowski (Code Review) 21 Jun '21

21 Jun '21

Attention is currently required from: Angel Pons. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55714 ) Change subject: acpi_ec: Implement basic ACPI embedded controller API ...................................................................... Patch Set 2: (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/55714/comment/9bdc1e09_9fdbec2c PS1, Line 11: e > typo: c Done File acpi_ec.h: https://review.coreboot.org/c/flashrom/+/55714/comment/6bc4df46_8be57f0b PS1, Line 41: bool ec_wait_for_ibuf(uint8_t control_port); : bool ec_wait_for_obuf(uint8_t control_port, unsigned int max_checks); > Any reason why only one of these has the `max_checks` parameter? The erase command required longer timeout on output buffer flag. The input buffer did not require any non-default timeouts so only one had the additional parameters. Added the max_checks to `ec_wait_for_ibuf` too fro consistency. File acpi_ec.c: https://review.coreboot.org/c/flashrom/+/55714/comment/a0be26f6_b6586c2c PS1, Line 84: ec_wait_for_ibuf > obuf? Yes, it should be obuf. The reference implementation had ibuf which is probably a bug and we missed it. https://review.coreboot.org/c/flashrom/+/55714/comment/f949ba5d_3deb2a49 PS1, Line 72: : : bool ec_read_reg(uint8_t address, uint8_t *data) : { : if (!ec_wait_for_ibuf(EC_CONTROL)) : return false; : OUTB(EC_CMD_READ_REG, EC_CONTROL); : : if (!ec_wait_for_ibuf(EC_CONTROL)) : return false; : OUTB(address, EC_DATA); : : if (!ec_wait_for_ibuf(EC_CONTROL)) : return false; : *data = INB(EC_DATA); : : return true; : } : : bool ec_write_reg(uint8_t address, uint8_t data) : { : if (!ec_wait_for_ibuf(EC_CONTROL)) : return false; : OUTB(EC_CMD_WRITE_REG, EC_CONTROL); : : if (!ec_wait_for_ibuf(EC_CONTROL)) : return false; : OUTB(address, EC_DATA); : : if (!ec_wait_for_ibuf(EC_CONTROL)) : return false; : OUTB(data, EC_DATA); : : return true; : } > Why not use the functions you defined above? […] No idea why we didn't do that ;) Applied the suggestion. -- To view, visit https://review.coreboot.org/c/flashrom/+/55714 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie3bae8d81c80ae2f286b619e974869e3f2f4545d Gerrit-Change-Number: 55714 Gerrit-PatchSet: 2 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 21 Jun 2021 10:41:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: Add 64KiB write granularity
by Michał Żygowski (Code Review) 21 Jun '21

21 Jun '21

Attention is currently required from: Angel Pons. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55713 ) Change subject: flashrom.c: Add 64KiB write granularity ...................................................................... Patch Set 2: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/55713/comment/a881a8fc_07129b99 PS1, Line 7: KB > Strictly speaking, this should be `KiB`. […] Done File flashrom.c: https://review.coreboot.org/c/flashrom/+/55713/comment/215a8093_cb22952d PS1, Line 506: 1024 * 64 > nit: I'd use `64 * 1024` to be in line with `64 kilobytes`. Done https://review.coreboot.org/c/flashrom/+/55713/comment/1e2b8fdc_fc61c319 PS1, Line 578: 1024 * 64 > Same here Done -- To view, visit https://review.coreboot.org/c/flashrom/+/55713 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I69801f581d0cb9b85f6596de7e1267ef9317673f Gerrit-Change-Number: 55713 Gerrit-PatchSet: 2 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Mon, 21 Jun 2021 10:38:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: acpi_ec: Implement basic ACPI embedded controller API
by Michał Żygowski (Code Review) 21 Jun '21

21 Jun '21

Attention is currently required from: Michał Żygowski. Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/55714 to look at the new patch set (#2). Change subject: acpi_ec: Implement basic ACPI embedded controller API ...................................................................... acpi_ec: Implement basic ACPI embedded controller API Implement basic functions to read/write EC registers and send standard ACPI EC commands. Those may prove useful in possible future implementations of embedded controllers in flashrom and are required to support EC firmware flashing on Tuxedo laptops. Signed-off-by: Michał Żygowski <michal.zygowski(a)3mdeb.com> Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Change-Id: Ie3bae8d81c80ae2f286b619e974869e3f2f4545d --- A acpi_ec.c A acpi_ec.h 2 files changed, 146 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/14/55714/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/55714 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie3bae8d81c80ae2f286b619e974869e3f2f4545d Gerrit-Change-Number: 55714 Gerrit-PatchSet: 2 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: flashrom.c: Add 64KiB write granularity
by Michał Żygowski (Code Review) 21 Jun '21

21 Jun '21

Attention is currently required from: Michał Żygowski. Hello build bot (Jenkins), Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/55713 to look at the new patch set (#2). Change subject: flashrom.c: Add 64KiB write granularity ...................................................................... flashrom.c: Add 64KiB write granularity Add 64KiB write granularity for the incoming Embedded Controller of Tuxedo laptops. These ECs can only operate on 64KiB blocks and any different operations result in a failure. Signed-off-by: Michał Żygowski <michal.zygowski(a)3mdeb.com> Change-Id: I69801f581d0cb9b85f6596de7e1267ef9317673f --- M flash.h M flashrom.c 2 files changed, 7 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/55713/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/55713 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I69801f581d0cb9b85f6596de7e1267ef9317673f Gerrit-Change-Number: 55713 Gerrit-PatchSet: 2 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: ft2232_spi: Revise comments about output pin states
by Nico Huber (Code Review) 21 Jun '21

21 Jun '21

Attention is currently required from: Alan Green, Samir Ibradžić. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55695 ) Change subject: ft2232_spi: Revise comments about output pin states ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > I think I understand what's going on here: the intent of the csgpiol parameter was to specify additional GPIOL pins that should mirror the CS pin. My changes were attempting to use the same machinery as csgpiol, but to control a general GPIO mechanism. > > Is that right? That's how I comprehend it too. > > (For unfortunate reasons, this worked fine with the ad-hoc programmer we are using, and I didn't notice the change in behavior it caused for other programmers). It's probably a very limited issue. There are two programmer entries that change cs_bits, but it seems they could live with a constant 0 on the GPIO as well. What definitely broke (already before your patches) was the csgpiol option. > It's probably still useful to have a mechanism to set the GPIO pins, Absolutely, yes. Angel has also already asked for it :) -- To view, visit https://review.coreboot.org/c/flashrom/+/55695 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2b84ede01759c80f69d5ad17e43783d09ecd1107 Gerrit-Change-Number: 55695 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Alan Green <avg(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Alan Green <avg(a)google.com> Gerrit-Attention: Samir Ibradžić <sibradzic(a)gmail.com> Gerrit-Comment-Date: Mon, 21 Jun 2021 10:33:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Alan Green <avg(a)google.com> Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: acpi_ec: Implement basic ACPI embedded controller API
by Angel Pons (Code Review) 21 Jun '21

21 Jun '21

Attention is currently required from: Michał Żygowski. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55714 ) Change subject: acpi_ec: Implement basic ACPI embedded controller API ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/55714/comment/b7e2e45a_3efa1b6e PS1, Line 7: ACPI > Please take a look at: https://uefi.org/specs/ACPI/6. […] Ack -- To view, visit https://review.coreboot.org/c/flashrom/+/55714 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie3bae8d81c80ae2f286b619e974869e3f2f4545d Gerrit-Change-Number: 55714 Gerrit-PatchSet: 1 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Comment-Date: Mon, 21 Jun 2021 10:21:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: buspirate: Add psus option
by Nico Huber (Code Review) 21 Jun '21

21 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/54887 ) Change subject: buspirate: Add psus option ...................................................................... buspirate: Add psus option This change adds a 'psus=<on|off>' option, to control the external Vcc state of the bus pirate, allowing hardware where the SPI flash chip is powered by the 3V3/5V lines directly. Change-Id: I8a7d4b40c0f7f04f6976f6757f05b61f2c9958f9 Signed-off-by: Jeremy Kerr <jk(a)codeconstruct.com.au> Reviewed-on: https://review.coreboot.org/c/flashrom/+/54887 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M buspirate_spi.c M flashrom.8.tmpl 2 files changed, 31 insertions(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/buspirate_spi.c b/buspirate_spi.c index 04b8fde..bfd0e05 100644 --- a/buspirate_spi.c +++ b/buspirate_spi.c @@ -326,6 +326,7 @@ int serialspeed_index = -1; int ret = 0; int pullup = 0; + int psu = 0; unsigned char *bp_commbuf; int bp_commbufsize; @@ -377,6 +378,17 @@ } free(tmp); + tmp = extract_programmer_param("psus"); + if (tmp) { + if (strcasecmp("on", tmp) == 0) + psu = 1; + else if (strcasecmp("off", tmp) == 0) + ; // ignore + else + msg_perr("Invalid psus state, not enabling.\n"); + } + free(tmp); + /* Default buffer size is 19: 16 bytes data, 3 bytes control. */ #define DEFAULT_BUFSIZE (16 + 3) bp_commbuf = malloc(DEFAULT_BUFSIZE); @@ -638,6 +650,10 @@ bp_commbuf[0] |= (1 << 2); msg_pdbg("Enabling pull-up resistors.\n"); } + if (psu == 1) { + bp_commbuf[0] |= (1 << 3); + msg_pdbg("Enabling PSUs.\n"); + } ret = buspirate_sendrecv(bp_commbuf, 1, 1); if (ret) { ret = 1; diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index 4ae7e71..228e17b 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -952,7 +952,21 @@ More information about the Bus Pirate pull-up resistors and their purpose is available .URLB "http://dangerousprototypes.com/docs/Practical_guide_to_Bus_Pirate_pull-up_r…" \ "in a guide by dangerousprototypes" . -Only the external supply voltage (Vpu) is supported as of this writing. +.sp +The state of the Bus Pirate power supply pins is controllable through an optional +.B psus +parameter. Syntax is +.sp +.B " flashrom -p buspirate_spi:psus=state" +.sp +where +.B state +can be +.BR on " or " off . +This allows the bus pirate to power the ROM chip directly. This may also be used to provide the +required pullup voltage (when using the +.B pullups +option), by connecting the Bus Pirate's Vpu input to the appropriate Vcc pin. .SS .BR "pickit2_spi " programmer .IP -- To view, visit https://review.coreboot.org/c/flashrom/+/54887 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8a7d4b40c0f7f04f6976f6757f05b61f2c9958f9 Gerrit-Change-Number: 54887 Gerrit-PatchSet: 3 Gerrit-Owner: Jeremy Kerr <jk(a)ozlabs.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: buspirate: Add psus option
by Nico Huber (Code Review) 21 Jun '21

21 Jun '21

Attention is currently required from: Jeremy Kerr. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54887 ) Change subject: buspirate: Add psus option ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: Thanks for the quick update! -- To view, visit https://review.coreboot.org/c/flashrom/+/54887 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8a7d4b40c0f7f04f6976f6757f05b61f2c9958f9 Gerrit-Change-Number: 54887 Gerrit-PatchSet: 2 Gerrit-Owner: Jeremy Kerr <jk(a)ozlabs.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jeremy Kerr <jk(a)ozlabs.org> Gerrit-Comment-Date: Mon, 21 Jun 2021 10:17:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit June 2021