coreboot-gerrit January 2020

coreboot-gerrit@coreboot.org

1 participants
1869 discussions

Change in coreboot[master]: mb/intel/tglrvp : Add ACPI support for CNVi
by Furquan Shaikh (Code Review) 06 Jan '20

06 Jan '20

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37887 ) Change subject: mb/intel/tglrvp : Add ACPI support for CNVi ...................................................................... Patch Set 3: (1 comment) https://review.coreboot.org/c/coreboot/+/37887/3/src/mainboard/intel/tglrvp… File src/mainboard/intel/tglrvp/acpi/cnvi.asl: PS3: I think this file needs to be cleaned up, but is there any reason why this is put under mainboard and not SoC? -- To view, visit https://review.coreboot.org/c/coreboot/+/37887 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I66091374f8ebcb06457296252c9a8924734a7fa5 Gerrit-Change-Number: 37887 Gerrit-PatchSet: 3 Gerrit-Owner: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Shaunak Saha <shaunak.saha(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-CC: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.corp-partner.google.com> Gerrit-Comment-Date: Mon, 06 Jan 2020 17:17:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/inteltool: Add GPIO dumping capabilites for Icelake U systems
by Mimoja (Code Review) 06 Jan '20

06 Jan '20

Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38175 ) Change subject: util/inteltool: Add GPIO dumping capabilites for Icelake U systems ...................................................................... Patch Set 7: (1 comment) https://review.coreboot.org/c/coreboot/+/38175/6/util/inteltool/gpio_groups… File util/inteltool/gpio_groups.c: https://review.coreboot.org/c/coreboot/+/38175/6/util/inteltool/gpio_groups… PS6, Line 2455: .pad_count = 82 - 59 + 1, > Take a look at the previous impl. Done -- To view, visit https://review.coreboot.org/c/coreboot/+/38175 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib40f1dbae57169678e92ea9ad0df60ff91b5b22c Gerrit-Change-Number: 38175 Gerrit-PatchSet: 7 Gerrit-Owner: Mimoja <coreboot(a)mimoja.de> Gerrit-Reviewer: Mimoja <coreboot(a)mimoja.de> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jeremy Soller <jeremy(a)system76.com> Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 06 Jan 2020 16:38:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/inteltool: Add GPIO dumping capabilites for Icelake U systems
by Mimoja (Code Review) 06 Jan '20

06 Jan '20

Hello Stefan Reinauer, Philipp Deppenwiese, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/38175 to look at the new patch set (#7). Change subject: util/inteltool: Add GPIO dumping capabilites for Icelake U systems ...................................................................... util/inteltool: Add GPIO dumping capabilites for Icelake U systems This GPIO dumping was implemented using the Intel® 495 Series Chipset Family On-Package Platform Controller Hub datasheet. The GPIO community ports can be found in table 36-1, while the community and pin descriptions are taken from linux/pinctrl/intel/pinctrl-icelake.c . This commit was tested on the late 2019 Razer Blade Stealth with 1065G7 and Chipset 495 PCH and the output manually compared against linux/pinctrl-intel. Change-Id: Ib40f1dbae57169678e92ea9ad0df60ff91b5b22c Signed-off-by: Johanna Schander <coreboot(a)mimoja.de> --- M util/inteltool/gpio.c M util/inteltool/gpio_groups.c M util/inteltool/pcr.c 3 files changed, 478 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/38175/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/38175 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib40f1dbae57169678e92ea9ad0df60ff91b5b22c Gerrit-Change-Number: 38175 Gerrit-PatchSet: 7 Gerrit-Owner: Mimoja <coreboot(a)mimoja.de> Gerrit-Reviewer: Mimoja <coreboot(a)mimoja.de> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jeremy Soller <jeremy(a)system76.com> Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/intel/tigerlake: Update GPIO config
by Furquan Shaikh (Code Review) 06 Jan '20

06 Jan '20

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37427 ) Change subject: soc/intel/tigerlake: Update GPIO config ...................................................................... Patch Set 13: > Patch Set 13: > > I got the confirmation from the GPIO team regarding the EDS information. They said "index mapping should be identical (or set the same) for DW0, DW1 and DW2 – so the EDS is incorrect if DW2 still shows the old mapping". So we do not need any code modification for dealing with DW0,1 or 2 separately. Is there any addendum or new EDS version # that has this information? Can you please share that with me? -- To view, visit https://review.coreboot.org/c/coreboot/+/37427 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I585100375feee39b5a9105bdf6d9f5ca3a5bb2fa Gerrit-Change-Number: 37427 Gerrit-PatchSet: 13 Gerrit-Owner: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Francois Toguo Fotso <francois.toguo.fotso(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Shaunak Saha <shaunak.saha(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Venkata Krishna Nimmagadda Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 06 Jan 2020 15:16:07 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/tigerlake: Update Kconfig
by Furquan Shaikh (Code Review) 06 Jan '20

06 Jan '20

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37426 ) Change subject: soc/intel/tigerlake: Update Kconfig ...................................................................... Patch Set 12: (1 comment) https://review.coreboot.org/c/coreboot/+/37426/12/src/soc/intel/tigerlake/K… File src/soc/intel/tigerlake/Kconfig: https://review.coreboot.org/c/coreboot/+/37426/12/src/soc/intel/tigerlake/K… PS12, Line 169: 0x25a Just curious: How were these m/n values calculated? -- To view, visit https://review.coreboot.org/c/coreboot/+/37426 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I04020d55f1063d521b15f8d0dabbd6f1dabf577c Gerrit-Change-Number: 37426 Gerrit-PatchSet: 12 Gerrit-Owner: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Francois Toguo Fotso <francois.toguo.fotso(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Shaunak Saha <shaunak.saha(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Venkata Krishna Nimmagadda Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-CC: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.corp-partner.google.com> Gerrit-Comment-Date: Mon, 06 Jan 2020 15:14:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: drives/spi_flash: add spi_flash_cmd_write_page_program()
by Aaron Durbin (Code Review) 06 Jan '20

06 Jan '20

Aaron Durbin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37963 ) Change subject: drives/spi_flash: add spi_flash_cmd_write_page_program() ...................................................................... drives/spi_flash: add spi_flash_cmd_write_page_program() The SPI flashes that support page programming mode had duplicated the logic for writing in every driver. Add spi_flash_cmd_write_page_program() and use the common implementation to reduce code size that comes from duplication. Change-Id: Ie6db03fa8ad33789f1d07a718a769e4ca8bffe1d Signed-off-by: Aaron Durbin <adurbin(a)chromium.org> --- M src/drivers/spi/adesto.c M src/drivers/spi/amic.c M src/drivers/spi/atmel.c M src/drivers/spi/eon.c M src/drivers/spi/gigadevice.c M src/drivers/spi/macronix.c M src/drivers/spi/spansion.c M src/drivers/spi/spi_flash.c M src/drivers/spi/spi_flash_internal.h M src/drivers/spi/sst.c M src/drivers/spi/stmicro.c M src/drivers/spi/winbond.c M src/include/spi_flash.h 13 files changed, 94 insertions(+), 593 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/37963/1 diff --git a/src/drivers/spi/adesto.c b/src/drivers/spi/adesto.c index 974f8ad..5f46741 100644 --- a/src/drivers/spi/adesto.c +++ b/src/drivers/spi/adesto.c @@ -148,66 +148,8 @@ }, }; -static int adesto_write(const struct spi_flash *flash, u32 offset, size_t len, - const void *buf) -{ - unsigned long byte_addr; - unsigned long page_size; - size_t chunk_len; - size_t actual; - int ret; - u8 cmd[4]; - - page_size = flash->page_size; - - for (actual = 0; actual < len; actual += chunk_len) { - byte_addr = offset % page_size; - chunk_len = MIN(len - actual, page_size - byte_addr); - chunk_len = spi_crop_chunk(&flash->spi, sizeof(cmd), chunk_len); - - cmd[0] = CMD_AT25DF_PP; - cmd[1] = (offset >> 16) & 0xff; - cmd[2] = (offset >> 8) & 0xff; - cmd[3] = offset & 0xff; -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, "PP: %p => cmd = { 0x%02x 0x%02x%02x%02x }" - " chunk_len = %zu\n", buf + actual, - cmd[0], cmd[1], cmd[2], cmd[3], chunk_len); -#endif - - ret = spi_flash_cmd(&flash->spi, CMD_AT25DF_WREN, NULL, 0); - if (ret < 0) { - printk(BIOS_WARNING, "SF: Enabling Write failed\n"); - goto out; - } - - ret = spi_flash_cmd_write(&flash->spi, cmd, sizeof(cmd), - buf + actual, chunk_len); - if (ret < 0) { - printk(BIOS_WARNING, "SF: adesto Page Program failed\n"); - goto out; - } - - ret = spi_flash_cmd_wait_ready(flash, - SPI_FLASH_PROG_TIMEOUT_MS); - if (ret) - goto out; - - offset += chunk_len; - } - -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, "SF: adesto: Successfully programmed %zu bytes @" - " 0x%lx\n", len, (unsigned long)(offset - len)); -#endif - ret = 0; - -out: - return ret; -} - static const struct spi_flash_ops spi_flash_ops = { - .write = adesto_write, + .write = spi_flash_cmd_write_page_program, .erase = spi_flash_cmd_erase, }; @@ -237,6 +179,8 @@ flash->size = flash->sector_size *params->sectors_per_block * params->nr_blocks; flash->erase_cmd = CMD_AT25DF_SE; + flash->pp_cmd = CMD_AT25DF_PP; + flash->wren_cmd = CMD_AT25DF_WREN; flash->ops = &spi_flash_ops; diff --git a/src/drivers/spi/amic.c b/src/drivers/spi/amic.c index 254a5b2..54c03c0 100644 --- a/src/drivers/spi/amic.c +++ b/src/drivers/spi/amic.c @@ -119,67 +119,8 @@ }, }; -static int amic_write(const struct spi_flash *flash, u32 offset, size_t len, - const void *buf) -{ - unsigned long byte_addr; - unsigned long page_size; - size_t chunk_len; - size_t actual; - int ret; - u8 cmd[4]; - - page_size = flash->page_size; - byte_addr = offset % page_size; - - for (actual = 0; actual < len; actual += chunk_len) { - chunk_len = MIN(len - actual, page_size - byte_addr); - chunk_len = spi_crop_chunk(&flash->spi, sizeof(cmd), chunk_len); - - cmd[0] = CMD_A25_PP; - cmd[1] = (offset >> 16) & 0xff; - cmd[2] = (offset >> 8) & 0xff; - cmd[3] = offset & 0xff; -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, "PP: %p => cmd = { 0x%02x 0x%02x%02x%02x }" - " chunk_len = %zu\n", buf + actual, - cmd[0], cmd[1], cmd[2], cmd[3], chunk_len); -#endif - - ret = spi_flash_cmd(&flash->spi, CMD_A25_WREN, NULL, 0); - if (ret < 0) { - printk(BIOS_WARNING, "SF: Enabling Write failed\n"); - goto out; - } - - ret = spi_flash_cmd_write(&flash->spi, cmd, sizeof(cmd), - buf + actual, chunk_len); - if (ret < 0) { - printk(BIOS_WARNING, "SF: AMIC Page Program failed\n"); - goto out; - } - - ret = spi_flash_cmd_wait_ready(flash, - SPI_FLASH_PROG_TIMEOUT_MS); - if (ret) - goto out; - - offset += chunk_len; - byte_addr = 0; - } - -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, "SF: AMIC: Successfully programmed %zu bytes @" - " 0x%lx\n", len, (unsigned long)(offset - len)); -#endif - ret = 0; - -out: - return ret; -} - static const struct spi_flash_ops spi_flash_ops = { - .write = amic_write, + .write = spi_flash_cmd_write_page_program, .erase = spi_flash_cmd_erase, }; @@ -210,6 +151,8 @@ flash->size = flash->sector_size * params->sectors_per_block * params->nr_blocks; flash->erase_cmd = CMD_A25_SE; + flash->pp_cmd = CMD_A25_PP; + flash->wren_cmd = CMD_A25_WREN; flash->ops = &spi_flash_ops; diff --git a/src/drivers/spi/atmel.c b/src/drivers/spi/atmel.c index 4496f4b..5e22eb6 100644 --- a/src/drivers/spi/atmel.c +++ b/src/drivers/spi/atmel.c @@ -103,66 +103,8 @@ }, }; -static int atmel_write(const struct spi_flash *flash, u32 offset, size_t len, - const void *buf) -{ - unsigned long byte_addr; - unsigned long page_size; - size_t chunk_len; - size_t actual; - int ret; - u8 cmd[4]; - - page_size = flash->page_size; - - for (actual = 0; actual < len; actual += chunk_len) { - byte_addr = offset % page_size; - chunk_len = MIN(len - actual, page_size - byte_addr); - chunk_len = spi_crop_chunk(&flash->spi, sizeof(cmd), chunk_len); - - cmd[0] = CMD_AT25_PP; - cmd[1] = (offset >> 16) & 0xff; - cmd[2] = (offset >> 8) & 0xff; - cmd[3] = offset & 0xff; -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, "PP: %p => cmd = { 0x%02x 0x%02x%02x%02x }" - " chunk_len = %zu\n", buf + actual, - cmd[0], cmd[1], cmd[2], cmd[3], chunk_len); -#endif - - ret = spi_flash_cmd(&flash->spi, CMD_AT25_WREN, NULL, 0); - if (ret < 0) { - printk(BIOS_WARNING, "SF: Enabling Write failed\n"); - goto out; - } - - ret = spi_flash_cmd_write(&flash->spi, cmd, sizeof(cmd), - buf + actual, chunk_len); - if (ret < 0) { - printk(BIOS_WARNING, "SF: Atmel Page Program failed\n"); - goto out; - } - - ret = spi_flash_cmd_wait_ready(flash, - SPI_FLASH_PROG_TIMEOUT_MS); - if (ret) - goto out; - - offset += chunk_len; - } - -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, "SF: Atmel: Successfully programmed %zu bytes @" - " 0x%lx\n", len, (unsigned long)(offset - len)); -#endif - ret = 0; - -out: - return ret; -} - static const struct spi_flash_ops spi_flash_ops = { - .write = atmel_write, + .write = spi_flash_cmd_write_page_program, .erase = spi_flash_cmd_erase, }; @@ -193,6 +135,8 @@ flash->size = flash->sector_size * params->sectors_per_block * params->nr_blocks; flash->erase_cmd = CMD_AT25_SE; + flash->pp_cmd = CMD_AT25_PP; + flash->wren_cmd = CMD_AT25_WREN; flash->ops = &spi_flash_ops; diff --git a/src/drivers/spi/eon.c b/src/drivers/spi/eon.c index 2cdbdbb..1f3cbf3 100644 --- a/src/drivers/spi/eon.c +++ b/src/drivers/spi/eon.c @@ -235,68 +235,8 @@ }, }; -static int eon_write(const struct spi_flash *flash, - u32 offset, size_t len, const void *buf) -{ - unsigned long byte_addr; - unsigned long page_size; - size_t chunk_len; - size_t actual; - int ret = 0; - u8 cmd[4]; - - page_size = flash->page_size; - - for (actual = 0; actual < len; actual += chunk_len) { - byte_addr = offset % page_size; - chunk_len = MIN(len - actual, page_size - byte_addr); - chunk_len = spi_crop_chunk(&flash->spi, sizeof(cmd), chunk_len); - - ret = spi_flash_cmd(&flash->spi, CMD_EN25_WREN, NULL, 0); - if (ret < 0) { - printk(BIOS_WARNING, "SF: Enabling Write failed\n"); - goto out; - } - - cmd[0] = CMD_EN25_PP; - cmd[1] = (offset >> 16) & 0xff; - cmd[2] = (offset >> 8) & 0xff; - cmd[3] = offset & 0xff; - -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, - "PP: %p => cmd = { 0x%02x 0x%02x%02x%02x } chunk_len = %zu\n", - buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], chunk_len); -#endif - - ret = spi_flash_cmd_write(&flash->spi, cmd, sizeof(cmd), - buf + actual, chunk_len); - if (ret < 0) { - printk(BIOS_WARNING, "SF: EON Page Program failed\n"); - goto out; - } - - ret = spi_flash_cmd_wait_ready(flash, - SPI_FLASH_PROG_TIMEOUT_MS); - if (ret) { - printk(BIOS_WARNING, "SF: EON Page Program timeout\n"); - goto out; - } - - offset += chunk_len; - } - -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, "SF: EON: Successfully programmed %zu bytes @ %#x\n", - len, (unsigned int)(offset - len)); -#endif - -out: - return ret; -} - static const struct spi_flash_ops spi_flash_ops = { - .write = eon_write, + .write = spi_flash_cmd_write_page_program, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, }; @@ -327,6 +267,8 @@ flash->size = flash->sector_size * params->nr_sectors; flash->erase_cmd = CMD_EN25_SE; flash->status_cmd = CMD_EN25_RDSR; + flash->pp_cmd = CMD_EN25_PP; + flash->wren_cmd = CMD_EN25_WREN; flash->ops = &spi_flash_ops; diff --git a/src/drivers/spi/gigadevice.c b/src/drivers/spi/gigadevice.c index 65494fa..1f478a8 100644 --- a/src/drivers/spi/gigadevice.c +++ b/src/drivers/spi/gigadevice.c @@ -164,71 +164,8 @@ }, }; -static int gigadevice_write(const struct spi_flash *flash, u32 offset, - size_t len, const void *buf) -{ - unsigned long byte_addr; - unsigned long page_size; - size_t chunk_len; - size_t actual; - int ret = 0; - u8 cmd[4]; - - page_size = flash->page_size; - - for (actual = 0; actual < len; actual += chunk_len) { - byte_addr = offset % page_size; - chunk_len = MIN(len - actual, page_size - byte_addr); - chunk_len = spi_crop_chunk(&flash->spi, sizeof(cmd), chunk_len); - - ret = spi_flash_cmd(&flash->spi, CMD_GD25_WREN, NULL, 0); - if (ret < 0) { - printk(BIOS_WARNING, - "SF gigadevice.c: Enabling Write failed\n"); - goto out; - } - - cmd[0] = CMD_GD25_PP; - cmd[1] = (offset >> 16) & 0xff; - cmd[2] = (offset >> 8) & 0xff; - cmd[3] = offset & 0xff; -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, - "PP gigadevice.c: %p => cmd = { 0x%02x 0x%02x%02x%02x }" - " chunk_len = %zu\n", buf + actual, - cmd[0], cmd[1], cmd[2], cmd[3], chunk_len); -#endif - - ret = spi_flash_cmd_write(&flash->spi, cmd, sizeof(cmd), - buf + actual, chunk_len); - if (ret < 0) { - printk(BIOS_WARNING, - "SF gigadevice.c: Page Program failed\n"); - goto out; - } - - ret = spi_flash_cmd_wait_ready(flash, - SPI_FLASH_PROG_TIMEOUT_MS); - if (ret) - goto out; - - offset += chunk_len; - } - -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, - "SF gigadevice.c: Successfully programmed %zu bytes @ %#x\n", - len, (unsigned int)(offset - len)); -#endif - - ret = 0; - -out: - return ret; -} - static const struct spi_flash_ops spi_flash_ops = { - .write = gigadevice_write, + .write = spi_flash_cmd_write_page_program, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, }; @@ -264,6 +201,8 @@ (1 << params->nr_blocks_shift); flash->erase_cmd = CMD_GD25_SE; flash->status_cmd = CMD_GD25_RDSR; + flash->pp_cmd = CMD_GD25_PP; + flash->wren_cmd = CMD_GD25_WREN; flash->ops = &spi_flash_ops; diff --git a/src/drivers/spi/macronix.c b/src/drivers/spi/macronix.c index 6643dfa..adf4f23 100644 --- a/src/drivers/spi/macronix.c +++ b/src/drivers/spi/macronix.c @@ -200,64 +200,8 @@ }, }; -static int macronix_write(const struct spi_flash *flash, u32 offset, size_t len, - const void *buf) -{ - unsigned long byte_addr; - unsigned long page_size; - size_t chunk_len; - size_t actual; - int ret = 0; - u8 cmd[4]; - - page_size = flash->page_size; - - for (actual = 0; actual < len; actual += chunk_len) { - byte_addr = offset % page_size; - chunk_len = MIN(len - actual, page_size - byte_addr); - chunk_len = spi_crop_chunk(&flash->spi, sizeof(cmd), chunk_len); - - cmd[0] = CMD_MX25XX_PP; - cmd[1] = (offset >> 16) & 0xff; - cmd[2] = (offset >> 8) & 0xff; - cmd[3] = offset & 0xff; -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, "PP: %p => cmd = { 0x%02x 0x%02x%02x%02x }" - " chunk_len = %zu\n", - buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], chunk_len); -#endif - - ret = spi_flash_cmd(&flash->spi, CMD_MX25XX_WREN, NULL, 0); - if (ret < 0) { - printk(BIOS_WARNING, "SF: Enabling Write failed\n"); - break; - } - - ret = spi_flash_cmd_write(&flash->spi, cmd, sizeof(cmd), - buf + actual, chunk_len); - if (ret < 0) { - printk(BIOS_WARNING, "SF: Macronix Page Program failed\n"); - break; - } - - ret = spi_flash_cmd_wait_ready(flash, - SPI_FLASH_PROG_TIMEOUT_MS); - if (ret) - break; - - offset += chunk_len; - } - -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, "SF: Macronix: Successfully programmed %zu bytes @" - " 0x%lx\n", len, (unsigned long)(offset - len)); -#endif - - return ret; -} - static const struct spi_flash_ops spi_flash_ops = { - .write = macronix_write, + .write = spi_flash_cmd_write_page_program, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, }; @@ -288,6 +232,8 @@ params->nr_blocks; flash->erase_cmd = CMD_MX25XX_SE; flash->status_cmd = CMD_MX25XX_RDSR; + flash->pp_cmd = CMD_MX25XX_PP; + flash->wren_cmd = CMD_MX25XX_WREN; flash->ops = &spi_flash_ops; diff --git a/src/drivers/spi/spansion.c b/src/drivers/spi/spansion.c index 0b119eb..01911a0 100644 --- a/src/drivers/spi/spansion.c +++ b/src/drivers/spi/spansion.c @@ -218,65 +218,8 @@ }, }; -static int spansion_write(const struct spi_flash *flash, u32 offset, size_t len, - const void *buf) -{ - unsigned long byte_addr; - unsigned long page_size; - size_t chunk_len; - size_t actual; - int ret = 0; - u8 cmd[4]; - - page_size = flash->page_size; - - for (actual = 0; actual < len; actual += chunk_len) { - byte_addr = offset % page_size; - chunk_len = MIN(len - actual, page_size - byte_addr); - chunk_len = spi_crop_chunk(&flash->spi, sizeof(cmd), chunk_len); - - cmd[0] = CMD_S25FLXX_PP; - cmd[1] = (offset >> 16) & 0xff; - cmd[2] = (offset >> 8) & 0xff; - cmd[3] = offset & 0xff; - -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, "PP: %p => cmd = { 0x%02x 0x%02x%02x%02x }" - " chunk_len = %zu\n", - buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], chunk_len); -#endif - - ret = spi_flash_cmd(&flash->spi, CMD_S25FLXX_WREN, NULL, 0); - if (ret < 0) { - printk(BIOS_WARNING, "SF: Enabling Write failed\n"); - break; - } - - ret = spi_flash_cmd_write(&flash->spi, cmd, 4, - buf + actual, chunk_len); - if (ret < 0) { - printk(BIOS_WARNING, "SF: SPANSION Page Program failed\n"); - break; - } - - ret = spi_flash_cmd_wait_ready(flash, - SPI_FLASH_PROG_TIMEOUT_MS); - if (ret) - break; - - offset += chunk_len; - } - -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, "SF: SPANSION: Successfully programmed %zu bytes @ 0x%x\n", - len, offset); -#endif - - return ret; -} - static const struct spi_flash_ops spi_flash_ops = { - .write = spansion_write, + .write = spi_flash_cmd_write_page_program, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, }; @@ -307,6 +250,8 @@ flash->size = flash->sector_size * params->nr_sectors; flash->erase_cmd = CMD_S25FLXX_SE; flash->status_cmd = CMD_S25FLXX_RDSR; + flash->pp_cmd = CMD_S25FLXX_PP; + flash->wren_cmd = CMD_S25FLXX_WREN; flash->ops = &spi_flash_ops; diff --git a/src/drivers/spi/spi_flash.c b/src/drivers/spi/spi_flash.c index f0d0159..ff69a9a 100644 --- a/src/drivers/spi/spi_flash.c +++ b/src/drivers/spi/spi_flash.c @@ -255,6 +255,60 @@ return spi_flash_cmd(&flash->spi, flash->status_cmd, reg, sizeof(*reg)); } +int spi_flash_cmd_write_page_program(const struct spi_flash *flash, u32 offset, + size_t len, const void *buf) +{ + unsigned long byte_addr; + unsigned long page_size; + size_t chunk_len; + size_t actual; + int ret = 0; + u8 cmd[4]; + + page_size = flash->page_size; + cmd[0] = flash->pp_cmd; + + for (actual = 0; actual < len; actual += chunk_len) { + byte_addr = offset % page_size; + chunk_len = MIN(len - actual, page_size - byte_addr); + chunk_len = spi_crop_chunk(&flash->spi, sizeof(cmd), chunk_len); + + spi_flash_addr(offset, cmd); + if (CONFIG(DEBUG_SPI_FLASH)) { + printk(BIOS_SPEW, "PP: %p => cmd = { 0x%02x 0x%02x%02x%02x } chunk_len = %zu\n", + buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], + chunk_len); + } + + ret = spi_flash_cmd(&flash->spi, flash->wren_cmd, NULL, 0); + if (ret < 0) { + printk(BIOS_WARNING, "SF: Enabling Write failed\n"); + goto out; + } + + ret = spi_flash_cmd_write(&flash->spi, cmd, sizeof(cmd), + buf + actual, chunk_len); + if (ret < 0) { + printk(BIOS_WARNING, "SF: Page Program failed\n"); + goto out; + } + + ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT_MS); + if (ret) + goto out; + + offset += chunk_len; + } + + if (CONFIG(DEBUG_SPI_FLASH)) + printk(BIOS_SPEW, "SF: : Successfully programmed %zu bytes @ 0x%lx\n", + len, (unsigned long)(offset - len)); + ret = 0; + +out: + return ret; +} + /* * The following table holds all device probe functions * diff --git a/src/drivers/spi/spi_flash_internal.h b/src/drivers/spi/spi_flash_internal.h index 36fa66d..f99c90e 100644 --- a/src/drivers/spi/spi_flash_internal.h +++ b/src/drivers/spi/spi_flash_internal.h @@ -62,6 +62,10 @@ /* Read status register. */ int spi_flash_cmd_status(const struct spi_flash *flash, u8 *reg); +/* Write to flash utilizing page program semantics. */ +int spi_flash_cmd_write_page_program(const struct spi_flash *flash, u32 offset, + size_t len, const void *buf); + /* Manufacturer-specific probe functions */ int spi_flash_probe_spansion(const struct spi_slave *spi, u8 *idcode, struct spi_flash *flash); diff --git a/src/drivers/spi/sst.c b/src/drivers/spi/sst.c index 499db35..bcc4770 100644 --- a/src/drivers/spi/sst.c +++ b/src/drivers/spi/sst.c @@ -53,8 +53,6 @@ static int sst_write_ai(const struct spi_flash *flash, u32 offset, size_t len, const void *buf); -static int sst_write_256(const struct spi_flash *flash, u32 offset, size_t len, - const void *buf); static const struct spi_flash_ops spi_flash_ops_write_ai = { .write = sst_write_ai, @@ -63,7 +61,7 @@ }; static const struct spi_flash_ops spi_flash_ops_write_256 = { - .write = sst_write_256, + .write = spi_flash_cmd_write_page_program, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, }; @@ -187,60 +185,6 @@ return spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT_MS); } -static int sst_write_256(const struct spi_flash *flash, u32 offset, size_t len, - const void *buf) -{ - size_t actual, chunk_len; - unsigned long byte_addr; - unsigned long page_size; - int ret = 0; - u8 cmd[4]; - - page_size = 256; - - for (actual = 0; actual < len; actual += chunk_len) { - byte_addr = offset % page_size; - chunk_len = MIN(len - actual, page_size - byte_addr); - chunk_len = spi_crop_chunk(&flash->spi, sizeof(cmd), chunk_len); - - cmd[0] = CMD_SST_PP; - cmd[1] = (offset >> 16) & 0xff; - cmd[2] = (offset >> 8) & 0xff; - cmd[3] = offset & 0xff; -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, "PP: %p => cmd = { 0x%02x 0x%02x%02x%02x }" - " chunk_len = %zu\n", - buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], chunk_len); -#endif - - ret = spi_flash_cmd(&flash->spi, CMD_SST_WREN, NULL, 0); - if (ret < 0) { - printk(BIOS_WARNING, "SF: Enabling Write failed\n"); - break; - } - - ret = spi_flash_cmd_write(&flash->spi, cmd, sizeof(cmd), - buf + actual, chunk_len); - if (ret < 0) { - printk(BIOS_WARNING, "SF: SST Page Program failed\n"); - break; - } - - ret = spi_flash_cmd_wait_ready(flash, - SPI_FLASH_PROG_TIMEOUT_MS); - if (ret) - break; - - offset += chunk_len; - } - -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, "SF: SST: program %s %zu bytes @ 0x%lx\n", - ret ? "failure" : "success", len, (unsigned long)offset - actual); -#endif - return ret; -} - static int sst_write_ai(const struct spi_flash *flash, u32 offset, size_t len, const void *buf) { @@ -350,6 +294,12 @@ flash->size = flash->sector_size * params->nr_sectors; flash->erase_cmd = CMD_SST_SE; flash->status_cmd = CMD_SST_RDSR; + flash->wren_cmd = CMD_SST_WREN; + + if (params->ops == &spi_flash_ops_write_256) { + flash->page_size = 256; + flash->pp_cmd = CMD_SST_PP; + } flash->ops = params->ops; diff --git a/src/drivers/spi/stmicro.c b/src/drivers/spi/stmicro.c index ad0a0dc..57bd3dd 100644 --- a/src/drivers/spi/stmicro.c +++ b/src/drivers/spi/stmicro.c @@ -284,66 +284,8 @@ }, }; -static int stmicro_write(const struct spi_flash *flash, - u32 offset, size_t len, const void *buf) -{ - unsigned long byte_addr; - unsigned long page_size; - size_t chunk_len; - size_t actual; - int ret = 0; - u8 cmd[4]; - - page_size = flash->page_size; - - for (actual = 0; actual < len; actual += chunk_len) { - byte_addr = offset % page_size; - chunk_len = MIN(len - actual, page_size - byte_addr); - chunk_len = spi_crop_chunk(&flash->spi, sizeof(cmd), chunk_len); - - cmd[0] = CMD_M25PXX_PP; - cmd[1] = (offset >> 16) & 0xff; - cmd[2] = (offset >> 8) & 0xff; - cmd[3] = offset & 0xff; -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, "PP: %p => cmd = { 0x%02x 0x%02x%02x%02x }" - " chunk_len = %zu\n", - buf + actual, cmd[0], cmd[1], cmd[2], cmd[3], chunk_len); -#endif - - ret = spi_flash_cmd(&flash->spi, CMD_M25PXX_WREN, NULL, 0); - if (ret < 0) { - printk(BIOS_WARNING, "SF: Enabling Write failed\n"); - goto out; - } - - ret = spi_flash_cmd_write(&flash->spi, cmd, sizeof(cmd), - buf + actual, chunk_len); - if (ret < 0) { - printk(BIOS_WARNING, "SF: STMicro Page Program failed\n"); - goto out; - } - - ret = spi_flash_cmd_wait_ready(flash, - SPI_FLASH_PROG_TIMEOUT_MS); - if (ret) - goto out; - - offset += chunk_len; - } - -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, "SF: STMicro: Successfully programmed %zu bytes @" - " 0x%lx\n", len, (unsigned long)(offset - len)); -#endif - ret = 0; - -out: - return ret; -} - static const struct spi_flash_ops spi_flash_ops = { - .write = stmicro_write, + .write = spi_flash_cmd_write_page_program, .erase = spi_flash_cmd_erase, }; @@ -384,6 +326,8 @@ flash->sector_size = params->page_size * params->pages_per_sector; flash->size = flash->sector_size * params->nr_sectors; flash->erase_cmd = params->op_erase; + flash->pp_cmd = CMD_M25PXX_PP; + flash->wren_cmd = CMD_M25PXX_WREN; flash->ops = &spi_flash_ops; diff --git a/src/drivers/spi/winbond.c b/src/drivers/spi/winbond.c index 3790ce7..b6735c0 100644 --- a/src/drivers/spi/winbond.c +++ b/src/drivers/spi/winbond.c @@ -296,64 +296,6 @@ }, }; -static int winbond_write(const struct spi_flash *flash, u32 offset, size_t len, - const void *buf) -{ - unsigned long byte_addr; - unsigned long page_size; - size_t chunk_len; - size_t actual; - int ret = 0; - u8 cmd[4]; - - page_size = flash->page_size; - - for (actual = 0; actual < len; actual += chunk_len) { - byte_addr = offset % page_size; - chunk_len = MIN(len - actual, page_size - byte_addr); - chunk_len = spi_crop_chunk(&flash->spi, sizeof(cmd), chunk_len); - - cmd[0] = CMD_W25_PP; - cmd[1] = (offset >> 16) & 0xff; - cmd[2] = (offset >> 8) & 0xff; - cmd[3] = offset & 0xff; -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, "PP: %p => cmd = { 0x%02x 0x%02x%02x%02x }" - " chunk_len = %zu\n", buf + actual, - cmd[0], cmd[1], cmd[2], cmd[3], chunk_len); -#endif - - ret = spi_flash_cmd(&flash->spi, CMD_W25_WREN, NULL, 0); - if (ret < 0) { - printk(BIOS_WARNING, "SF: Enabling Write failed\n"); - goto out; - } - - ret = spi_flash_cmd_write(&flash->spi, cmd, sizeof(cmd), - buf + actual, chunk_len); - if (ret < 0) { - printk(BIOS_WARNING, "SF: Winbond Page Program failed\n"); - goto out; - } - - ret = spi_flash_cmd_wait_ready(flash, - SPI_FLASH_PROG_TIMEOUT_MS); - if (ret) - goto out; - - offset += chunk_len; - } - -#if CONFIG(DEBUG_SPI_FLASH) - printk(BIOS_SPEW, "SF: Winbond: Successfully programmed %zu bytes @" - " 0x%lx\n", len, (unsigned long)(offset - len)); -#endif - ret = 0; - -out: - return ret; -} - /* * Convert BPx, TB and CMP to a region. * SEC (if available) must be zero. @@ -669,7 +611,7 @@ } static const struct spi_flash_ops spi_flash_ops = { - .write = winbond_write, + .write = spi_flash_cmd_write_page_program, .erase = spi_flash_cmd_erase, .status = spi_flash_cmd_status, .get_write_protection = winbond_get_write_protection, @@ -706,6 +648,8 @@ (1 << params->nr_blocks_shift); flash->erase_cmd = CMD_W25_SE; flash->status_cmd = CMD_W25_RDSR; + flash->pp_cmd = CMD_W25_PP; + flash->wren_cmd = CMD_W25_WREN; flash->flags.dual_spi = params->dual_spi; diff --git a/src/include/spi_flash.h b/src/include/spi_flash.h index 3a0c383..1a5a829 100644 --- a/src/include/spi_flash.h +++ b/src/include/spi_flash.h @@ -104,6 +104,8 @@ u32 page_size; u8 erase_cmd; u8 status_cmd; + u8 pp_cmd; /* Page program command. */ + u8 wren_cmd; /* Write Enable command. */ const struct spi_flash_ops *ops; const void *driver_private; }; -- To view, visit https://review.coreboot.org/c/coreboot/+/37963 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie6db03fa8ad33789f1d07a718a769e4ca8bffe1d Gerrit-Change-Number: 37963 Gerrit-PatchSet: 1 Gerrit-Owner: Aaron Durbin <adurbin(a)chromium.org> Gerrit-MessageType: newchange

3 4

Change in coreboot[master]: [WIP] Nvidia Optimus support for ThinkPads
by Patrik Tesarik (Code Review) 06 Jan '20

06 Jan '20

Patrik Tesarik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/28380 ) Change subject: [WIP] Nvidia Optimus support for ThinkPads ...................................................................... Patch Set 13: > Patch Set 13: > > > Patch Set 13: > > > > > Patch Set 13: > > > > > > The latest build does not work anymore for my T430, internal and external displays don't activate at all. Yesterday with the build dated 04-06 or 06-06-19 it worked. I'll try to reproduce later today. > > > > > > Though I really appreciate the progress. Thanks! > > > > Edit: Works now. I most probably missed to flag "Load proprietary blob repos" after a 'make distclean'. > > So you're using Optimus? Please tell more about your setup. Which nvidia driver you use, kernel version, how stable it is, etc, etc. This is valuable information. As my T430 machine is my daily driver and just occasionally got hacked on during the OSF-Hackathon in Bochum. I did not investigate further. But now have got my backup machine running, so I'll might start hacking again. I am and was using the open source nvidia driver in addition to the open source intel driver as far as I remember. In the end the coordination between Intel and nvidia did not work out. I got a lot glitches and freezed screens. But that could also be a artifact of using wayland. I have to check again the next time the T430 I open the machine for hacking. -- To view, visit https://review.coreboot.org/c/coreboot/+/28380 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I277808d6c1d8bd6e0a267a53f25471597698f8d5 Gerrit-Change-Number: 28380 Gerrit-PatchSet: 13 Gerrit-Owner: Evgeny Zinoviev <me(a)ch1p.io> Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Reviewer: Evgeny Zinoviev <me(a)ch1p.io> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Matt Parnell <mparnell(a)gmail.com> Gerrit-CC: Patrik Tesarik Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-CC: Prasun Gera Gerrit-Comment-Date: Mon, 06 Jan 2020 14:48:20 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/inteltool: Add GPIO dumping capabilites for Icelake U systems
by Philipp Deppenwiese (Code Review) 06 Jan '20

06 Jan '20

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38175 ) Change subject: util/inteltool: Add GPIO dumping capabilites for Icelake U systems ...................................................................... Patch Set 6: Code-Review+1 (1 comment) Please fix the hardcoded pad size in a better way, see comment https://review.coreboot.org/c/coreboot/+/38175/6/util/inteltool/gpio_groups… File util/inteltool/gpio_groups.c: https://review.coreboot.org/c/coreboot/+/38175/6/util/inteltool/gpio_groups… PS6, Line 2455: .pad_count = 82 - 59 + 1, Take a look at the previous impl. -- To view, visit https://review.coreboot.org/c/coreboot/+/38175 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib40f1dbae57169678e92ea9ad0df60ff91b5b22c Gerrit-Change-Number: 38175 Gerrit-PatchSet: 6 Gerrit-Owner: Mimoja <coreboot(a)mimoja.de> Gerrit-Reviewer: Mimoja <coreboot(a)mimoja.de> Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jeremy Soller <jeremy(a)system76.com> Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 06 Jan 2020 14:01:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/smc: Add and use new tool smcbiosinfo
by Patrick Rudolph (Code Review) 06 Jan '20

06 Jan '20

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35484 ) Change subject: util/smc: Add and use new tool smcbiosinfo ...................................................................... util/smc: Add and use new tool smcbiosinfo The BMC and tools interacting with it depend on metadata placed inside the ROM in order the flash the BIOS. Add a new tool smcbiosinfo, integrate it into the build system, and generate a 128byte metadata file called smcbiosinfo.bin on build. You need to provide the BoardID for every SMC mainboard through a new Kconfig symbol: SUPERMICRO_BOARDID Some fields are unknown, but it's sufficient to flash it using SMC vendor tools. Tested on Supermicro X11SSH: * Flashing using the WebUI works * Flashing using SMCIPMITool works No further validation is done on the firmware. Change-Id: Id608c2ce78614b45a2fd0b26d97d666f02223998 Signed-off-by: Patrick Rudolph <patrick.rudolph(a)9elements.com> --- M src/mainboard/supermicro/x11ssh/Kconfig M src/vendorcode/Makefile.inc A src/vendorcode/smc/Makefile.inc A util/smc/smcbiosinfo/Makefile A util/smc/smcbiosinfo/description.md A util/smc/smcbiosinfo/smcbiosinfo.c 6 files changed, 237 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/35484/1 diff --git a/src/mainboard/supermicro/x11ssh/Kconfig b/src/mainboard/supermicro/x11ssh/Kconfig index e6f342d..8ee47b8 100644 --- a/src/mainboard/supermicro/x11ssh/Kconfig +++ b/src/mainboard/supermicro/x11ssh/Kconfig @@ -76,4 +76,8 @@ int default 512 +config SUPERMICRO_BOARDID + string + default "089C" + endif # BOARD_SUPERMICRO_BASEBOARD_X11SSH diff --git a/src/vendorcode/Makefile.inc b/src/vendorcode/Makefile.inc index 8ccb0d0..d79fb0b 100644 --- a/src/vendorcode/Makefile.inc +++ b/src/vendorcode/Makefile.inc @@ -4,3 +4,4 @@ subdirs-y += siemens subdirs-y += cavium subdirs-y += eltan +subdirs-y += smc diff --git a/src/vendorcode/smc/Makefile.inc b/src/vendorcode/smc/Makefile.inc new file mode 100644 index 0000000..f1ac3a6 --- /dev/null +++ b/src/vendorcode/smc/Makefile.inc @@ -0,0 +1,19 @@ +ifeq ($(CONFIG_VENDOR_SUPERMICRO),y) +ifneq ($(CONFIG_SUPERMICRO_BOARDID), "") + +SMCBIOSINFOTOOL:=util/smc/smcbiosinfo/smcbiosinfo +cbfs-files-y += smcbiosinfo.bin + +smcbiosinfo.bin-file := $(obj)/mainboard/$(MAINBOARDDIR)/smcbiosinfo.bin +smcbiosinfo.bin-type := raw +smcbiosinfo.bin-compression := none + +$(SMCBIOSINFOTOOL): util/smc/smcbiosinfo/smcbiosinfo.c + printf " MAKE Creating SMCBIOSINFO tool\n" + $(MAKE) -C util/smc/smcbiosinfo + +$(obj)/mainboard/$(MAINBOARDDIR)/smcbiosinfo.bin: util/smc/smcbiosinfo/smcbiosinfo + printf " TOOL Creating SMC BIOSINFO metadata\n" + $(SMCBIOSINFOTOOL) -b $(CONFIG_SUPERMICRO_BOARDID) -o $(top)/$@ +endif +endif diff --git a/util/smc/smcbiosinfo/Makefile b/util/smc/smcbiosinfo/Makefile new file mode 100644 index 0000000..a0901bb --- /dev/null +++ b/util/smc/smcbiosinfo/Makefile @@ -0,0 +1,9 @@ +HOSTCC ?= cc + +smcbiosinfo: smcbiosinfo.c + $(HOSTCC) smcbiosinfo.c -I$(top)/$(obj)/ -o smcbiosinfo + +all: smcbiosinfo + +clean: + @rm -f smcbiosinfo diff --git a/util/smc/smcbiosinfo/description.md b/util/smc/smcbiosinfo/description.md new file mode 100644 index 0000000..21170eb --- /dev/null +++ b/util/smc/smcbiosinfo/description.md @@ -0,0 +1 @@ +Generates SMC biosinfo for BMC BIOS updates `C` diff --git a/util/smc/smcbiosinfo/smcbiosinfo.c b/util/smc/smcbiosinfo/smcbiosinfo.c new file mode 100644 index 0000000..a892b19 --- /dev/null +++ b/util/smc/smcbiosinfo/smcbiosinfo.c @@ -0,0 +1,203 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 9elements Agency GmbH <patrick.rudolph(a)9elements.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <stdio.h> +#include <string.h> +#include <stdlib.h> +#include <stdint.h> +#include <unistd.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <getopt.h> +#include <errno.h> +#include <stdarg.h> +#include "build.h" + +/* Place the following struct somewhere in the ROM: */ +struct SMC_BIOS_Update { + uint32_t magic0; // always 0xaa00b1ed + char magic1[4]; // always $FID + uint16_t magic2; // always 0x7804 + uint8_t space0; // always zero + // SMCinfotool doesn't care for the first letter + // The BMC webinterface does + char boardid[9]; // "100000000" + uint8_t space1[15]; // unknown data + uint8_t space2; // always 0x1f + char ukn_majorVer[2]; + uint8_t space3; // always zero + char ukn_minorVer[2]; + uint8_t space4; // always zero + char majorVer[3]; + char minorVer[2]; + uint8_t space5; // always zero + uint16_t year; + uint8_t month; + uint8_t day; + uint32_t space6; // unknown data + uint8_t space7; // all ones + char str[15]; // "SUPERMSMCI--MB1" + uint8_t space8[3]; // always zero + uint64_t space9[6]; // all ones +} __packed; + +// Unused values: CDEPqR +static const char *optstring = "o:b:h"; + +static struct option long_options[] = { + {"output", required_argument, 0, 'o' }, + {"boardid", required_argument, 0, 'b' }, + {"help", no_argument, 0, 'h' }, +}; + +static void usage(void) +{ + printf("smcbiosinfo: Create BIOSInfo for BMC BIOS updates\n"); + printf("Usage: smcbiosinfo [options] -b <boardid> -o <filename>\n"); + printf("-o | --output <FILE> The file to generate\n"); + printf("-b | --boardid <id> The board ID\n"); + printf("-h | --help Print this help\n"); +} + +static int bcd2int(int hex) +{ + return ((hex & 0xF0) >> 4) * 10 + (hex & 0x0F); +} + +int main(int argc, char **argv) +{ + int c; + int ret = 0; + char *boardid = NULL; + char *filename = NULL; + + while (1) { + int optindex = 0; + + c = getopt_long(argc, argv, optstring, long_options, &optindex); + + if (c == -1) + break; + + switch (c) { + case 'o': + filename = strdup(optarg); + break; + case 'b': + boardid = strdup(optarg); + break; + case 'h': + usage(); + goto out; + default: + break; + } + } + if (!filename) { + fprintf(stderr, "E: Must specify a destination filename\n"); + ret = 1; + goto out; + } + + if (!boardid) { + fprintf(stderr, "E: Must specify a board ID\n"); + ret = 1; + goto out; + } else if (strlen(boardid) > 8) { + fprintf(stderr, "E: Board ID must be less than 8 characters\n"); + ret = 1; + goto out; + } + + // generate the table + + struct SMC_BIOS_Update sbu = { + 0xaa00b1ed, + "$FID", + 0x7804, + 0, // space + "100000000", //boardid + {}, // unknown data + 0x1f, // space + "05", + 0, // zero + "06", + 0, // zero + "000", // major, MSB first + "00", // minor + 0, // zero + bcd2int(COREBOOT_BUILD_YEAR_BCD) + 2000, // year + bcd2int(COREBOOT_BUILD_MONTH_BCD), // month + bcd2int(COREBOOT_BUILD_DAY_BCD), //day + 0, // zero + 0xff, // space + "SUPERMSMCI--MB1", + {0, 0, 0}, // all zero + {~0, ~0, ~0, ~0, ~0, ~0}, // all ones + }; + + if (COREBOOT_MAJOR_VERSION < 999) { + char tmp[4]; + snprintf(tmp, sizeof(tmp), "%03d", COREBOOT_MAJOR_VERSION); + memcpy(&sbu.majorVer, &tmp, sizeof(sbu.majorVer)); + } else { + fprintf(stderr, "E: Unsupported coreboot major version\n"); + ret = 1; + goto out; + } + + if (COREBOOT_MINOR_VERSION < 99) { + char tmp[3]; + snprintf(tmp, sizeof(tmp), "%02d", COREBOOT_MINOR_VERSION); + memcpy(&sbu.minorVer, &tmp, sizeof(sbu.minorVer)); + } else { + fprintf(stderr, "E: Unsupported coreboot minor version\n"); + ret = 1; + goto out; + } + memcpy(&sbu.boardid[1], boardid, strlen(boardid)); + + // write the table + + FILE *fd = fopen(filename, "wb"); + + if (!fd) { + fprintf(stderr, "%s open failed: %s\n", filename, strerror(errno)); + ret = 1; + goto out; + } + + if (fwrite(&sbu, 1, sizeof(sbu), fd) != sizeof(sbu)) { + fprintf(stderr, "%s write failed: %s\n", filename, strerror(errno)); + fclose(fd); + ret = 1; + goto out; + } + + if (fclose(fd)) { + fprintf(stderr, "%s close failed: %s\n", filename, strerror(errno)); + ret = 1; + goto out; + } + +out: + if (boardid) + free(boardid); + if (filename) + free(filename); + + return ret; +} -- To view, visit https://review.coreboot.org/c/coreboot/+/35484 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id608c2ce78614b45a2fd0b26d97d666f02223998 Gerrit-Change-Number: 35484 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-MessageType: newchange

6 35

Change in coreboot[master]: soc/intel/common/block/cse: Add boot partition related APIs
by Furquan Shaikh (Code Review) 06 Jan '20

06 Jan '20

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35402 ) Change subject: soc/intel/common/block/cse: Add boot partition related APIs ...................................................................... Patch Set 44: (67 comments) https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… File src/soc/intel/common/block/cse/cse_bp.c: https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 21: #define MKHI_GROUP_ID_BUP_COMMON 0xf0 Can we keep all the MKHI_GROUP_ID* in one place? Maybe in cse.h? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 23: /* Boot partition info and set boot partition info command ids */ : #define MKHI_GET_BOOT_PARTITION_INFO_CMD_REQ 0x1C : #define MKHI_SET_BOOT_PARTITION_CMD_REQ 0x1D Same as MKHI_GROUP_ID*. I think it would be good to have all of these in a single place. Also, I think the names should have the group in it for identifying what group it belongs to e.g. MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO, MKHI_BUP_COMMON_SET_BOOT_PARTITION https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 32: boot_partition_info_entry I think this can be just name cse_boot_partition_entry or cse_bp_entry? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 37: boot_partition_status This should be fixed width here since it is being used in a packed structure sent/received from CSE. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 40: bp_ What do you think about skipping prefix bp_ here since this is embedded within boot_partition_info_entry already? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 64: static struct cse_boot_partition_info cse_bp_info; Do we really need to maintain the global state of this? In general, I would expect this to be used only in the CSE FW update path and most of the information might be required within the same calling chain? Wouldn't it be better if the caller just passed in a structure around so that it can also be invalidated when necessary? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 81: memset(&info_req, 0, sizeof(info_req)); Is this really required? If you initialize the structure while declaring it above, then memset might not be required? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 109: Boot partition update failed This is very confusing. At first, it seems like update to boot partition failed. I think it might be better to name the above function as cse_get_boot_partition_info() instead of cse_update_boot_partition_info() and this string can be updated accordingly. In fact, in the failure path there are 3 error messages that get printed: "Get partition info resp failed: %d\n" "Could not get partition info\n" "Boot partition update failed\n" Not sure if we really need another print here. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 116: cse_send_set_boot_partition_info_cmd _info_ in the name is not correct. Just cse_send_set_bp(...)? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 132: " %d\n", partition_id); ... so that it is helpful when debugging. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 133: goto failed; Just return false; here directly. There is nothing happening under "failed:" label. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 136: Info I don't think adding "Info" here is correct. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 137: switch_req.next_bp = partition_id; Just set this when initialize switch_req above in L124? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 142: memset(&switch_resp, 0, sw_resp_sz); Why is this required? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 144: goto failed; return false; https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 149: goto failed; return false; https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 161: cse_send_set_boot_partition_info_cmd I am curious why CSE cannot just return a status in response to MKHI_SET_BOOT_PARTITION_CMD_REQ indicating whether it has updated the next boot partition or not. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 164: * space before * https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 212: cse_bp_info.bp_entries[boot_partition].bp_status; Before doing this shouldn't you also check if cse_bp_info.total_number_of_bp > boot_partition? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 223: boot_partition Same comment as above. Actually, it might make sense to have a helper function cse_get_bp_entry() and pass into it the response structure and the partition id. And then check for NULL to determine whether to return some value from the entry or just return false. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 254: if ((bp_info_flags & BP_INFO_MIN_RECOV_MODE_EN) != BP_INFO_MIN_RECOV_MODE_EN) { : printk(BIOS_INFO, "Minimum Recovery Mode is not enabled\n"); : *min_rec_mode_en = 0; : } else { : printk(BIOS_INFO, "Minimum Recovery Mode is enabled\n"); : *min_rec_mode_en = 1; : } : : return true; I think all of this can be just reduced to: return !!(bp_info_flags & BP_INFO_MIN_RECOV_MODE_EN); https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 284: me_rw_status In fact, why is there a need to introduce a new ME RW status? I think this function can simply be: static bool cse_is_bp_valid(enum boot_partition_id id) { uint8_t bp_status; if (!cse_get_bp_entry_status(id, &bp_status)) return false; if (bp_status != BP_STATUS_SUCCESS) return false; return true; } bool cse_is_rw_valid(void) { return cse_is_bp_valid(BP2) && cse_is_bp_valid(BP3); } https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 284: bool Does this really need to be a bool? Can't we just return the state directly as the return value? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 295: bp2_status | bp3_status So, if BP2 is valid and BP3 is invalid, me_rw_status would be both valid and invalid? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 312: 2 This should be cse_bp_info.total_number_of_bp https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 313: ME: %s version = %d.%d.%d.%d Each SoC has its own way of printing ME version. Now that the boards using redundancy within CSE print this in a different way, both the paths should be combined into a unified way of printing ME version without having to add knowledge into each SoC about redundancy being used or not. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… File src/soc/intel/common/block/include/intelblocks/cse.h: https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 171: layout Can you also add the graphic you put in commit message here? I think that is very helpful in understanding what the partitions and layout look like. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 172: nit: Just use spaces instead of tab? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 173: nit: same here https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 179: typedef Instead of typedef, just use enum boot_partition_id { ... }; https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 180: R nit: lowercase 'r'. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 184: M nit: lowercase 'm'. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 184: CSME I know it's referring to the same entity, but for consistency, let's use CSE everywhere. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 193: */ It would be good to add a sentence indicating that this is returned in response to MKHI command for GET_PARTITION_INFO. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 194: typedef same comment as above about typedef. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 195: nit: period (.) at the end. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 199: shall nit: should? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 210: This value isn't applicable to chrome platforms This is a common driver code file. The comment should indicate what the status message means and not make any assumptions about Chrome OS Platforms. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 214: BP_STATUS_MANIFEST_AND_INFO_OK What is the difference between BP_STATUS_SUCCESS and BP_STATUS_MANIFEST_AND_INFO_OK? Does BP_STATUS_MANIFEST_AND_INFO_OK indicate some kind of failure, but mainfest is good? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 216: I nit: lowercase 'i'. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 221: */ It would be good to mentioned that this is returned in response to MKHI command for GET_PARTITION_INFO. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 222: bp_info_flags I think it would be good to be consistent with the other enums i.e. use boot_partition_flags https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 224: Boot Partition Info Flag nit: All these are boot partition info flags and that is already indicated in the comment before the start of the enum. I think you can get rid of "Boot Partition Info Flag" text in front of the comments for each enum entry. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 232: RO nit: RO(BP1) https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 234: 2 nit: (1 << 1). I think it makes it easier to read. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 239: relavant nit: relevant https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 239: BP_INFO_MIN_RECOV_MODE_EN Why? Shouldn't this be BP_INFO_REDUNDANCY_EN? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 244: ME RW Status What does this indicate? Is this returned in response to some command? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 253: ME_RW_SIGN_INVALID What is the difference between ME_RW_INVALID and ME_RW_SIGN_INVALID? How is the difference determined? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 257: partition_fw_version Shouldn't this be cse_fw_version? I don't think it is really tied to boot partition. E.g. Can BP2 and BP3 have different firmware versions? Or would you treat BP2 and BP3 i.e. CSE-RW having a FW version? Similarly, CSE-RO could have a firmware version. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 264: * I see a number of functions being exposed here. Do you really expect all of these to be used outside of this driver? I would expect a lot of these to just be required within the cse driver path which performs an update. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 267: CSE Reset I am not sure how CSE reset works okay in this case. Once a different partition is selected to boot, shouldn't global reset be a requirement? There are other components that come from this partition and so should be re-loaded after the reset? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 267: can/should? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 270: Returns false on failure and true on success. I think this is fine, but I would really like the CSE functions to have a consistent scheme for return values. I think most other functions before this returned 0 on error and 1 on success. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 272: uint8_t Use "enum boot_partition_id" instead? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 278: uint8_t enum boot_partition_id https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 284: uint8_t enum boot_partition_id https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 290: uint8_t enum bp_info_flags https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 296: uint8_t enum boot_partition_status https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 296: uint8_t enum boot_partition_id https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 302: uint8_t enum boot_partition_id https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 308: uint8_t enum boot_partition_id https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 308: start_end_offset range? Just to keep the name shorter? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 313: must nit: must not required here. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 313: enabled to support CSE FW Update Why? Shouldn't it be redudancy and not min recovery? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 316: bool Can return value be used to indicate true = enabled, false = disabled/error? https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 328: bool Why is a return value required for debug/print function? -- To view, visit https://review.coreboot.org/c/coreboot/+/35402 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iaa62409c0616d5913d21374a8a6804f82258eb4f Gerrit-Change-Number: 35402 Gerrit-PatchSet: 44 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Andrey Petrov <anpetrov(a)fb.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Mon, 06 Jan 2020 10:17:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit January 2020