- flashrom-gerrit - mail.coreboot.org

[M] Change in flashrom[master]: parallel: Drop explicit fallback_chip_X boilerplate
by Edward O'Callaghan (Code Review) 09 Jan '23

09 Jan '23

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/71745 ) Change subject: parallel: Drop explicit fallback_chip_X boilerplate ...................................................................... parallel: Drop explicit fallback_chip_X boilerplate A NULL func pointer is necessary and sufficient for the condition `NULL func pointer => fallback_chip_X' as to not need this explicit specification. Therefore drop the explicit need to specify these fallback callback function pointer in the par_master struct. This is a reasonable default for every driver in the tree. Furthermore, move the 'fallback_chip_X()' func from the generic programmer.c register logic into its relevant home of parallel.c and make static local to clean up link-time symbol space. This simplifies the code and driver development. Change-Id: If25c0048a07057aa72be6ffa8d8ad7f0a568dcf7 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M atahpt.c M atapromise.c M atavia.c M drkaiser.c M gfxnvidia.c M include/programmer.h M internal.c M it8212.c M nic3com.c M nicintel.c M nicnatsemi.c M nicrealtek.c M parallel.c M programmer.c M satamv.c M satasii.c M serprog.c 17 files changed, 95 insertions(+), 144 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/45/71745/1 diff --git a/atahpt.c b/atahpt.c index 89b44c2..71fd4d5 100644 --- a/atahpt.c +++ b/atahpt.c @@ -74,13 +74,7 @@ static const struct par_master par_master_atahpt = { .chip_readb = atahpt_chip_readb, - .chip_readw = fallback_chip_readw, - .chip_readl = fallback_chip_readl, - .chip_readn = fallback_chip_readn, .chip_writeb = atahpt_chip_writeb, - .chip_writew = fallback_chip_writew, - .chip_writel = fallback_chip_writel, - .chip_writen = fallback_chip_writen, .shutdown = atahpt_shutdown, }; diff --git a/atapromise.c b/atapromise.c index e1b8638..9beebf1 100644 --- a/atapromise.c +++ b/atapromise.c @@ -113,13 +113,7 @@ static const struct par_master par_master_atapromise = { .chip_readb = atapromise_chip_readb, - .chip_readw = fallback_chip_readw, - .chip_readl = fallback_chip_readl, - .chip_readn = fallback_chip_readn, .chip_writeb = atapromise_chip_writeb, - .chip_writew = fallback_chip_writew, - .chip_writel = fallback_chip_writel, - .chip_writen = fallback_chip_writen, .shutdown = atapromise_shutdown, }; diff --git a/atavia.c b/atavia.c index 8af52d6..f35771a 100644 --- a/atavia.c +++ b/atavia.c @@ -135,13 +135,7 @@ static const struct par_master lpc_master_atavia = { .map_flash_region = atavia_map, .chip_readb = atavia_chip_readb, - .chip_readw = fallback_chip_readw, - .chip_readl = fallback_chip_readl, - .chip_readn = fallback_chip_readn, .chip_writeb = atavia_chip_writeb, - .chip_writew = fallback_chip_writew, - .chip_writel = fallback_chip_writel, - .chip_writen = fallback_chip_writen, }; static int atavia_init(const struct programmer_cfg *cfg) diff --git a/drkaiser.c b/drkaiser.c index 96f2c99..ebf5119 100644 --- a/drkaiser.c +++ b/drkaiser.c @@ -71,13 +71,7 @@ static const struct par_master par_master_drkaiser = { .chip_readb = drkaiser_chip_readb, - .chip_readw = fallback_chip_readw, - .chip_readl = fallback_chip_readl, - .chip_readn = fallback_chip_readn, .chip_writeb = drkaiser_chip_writeb, - .chip_writew = fallback_chip_writew, - .chip_writel = fallback_chip_writel, - .chip_writen = fallback_chip_writen, .shutdown = drkaiser_shutdown, }; diff --git a/gfxnvidia.c b/gfxnvidia.c index 1d484ea..b420975 100644 --- a/gfxnvidia.c +++ b/gfxnvidia.c @@ -96,13 +96,7 @@ static const struct par_master par_master_gfxnvidia = { .chip_readb = gfxnvidia_chip_readb, - .chip_readw = fallback_chip_readw, - .chip_readl = fallback_chip_readl, - .chip_readn = fallback_chip_readn, .chip_writeb = gfxnvidia_chip_writeb, - .chip_writew = fallback_chip_writew, - .chip_writel = fallback_chip_writel, - .chip_writen = fallback_chip_writen, .shutdown = gfxnvidia_shutdown, }; diff --git a/include/programmer.h b/include/programmer.h index db7a05a..ad3c502 100644 --- a/include/programmer.h +++ b/include/programmer.h @@ -442,12 +442,6 @@ int register_par_master(const struct par_master *mst, const enum chipbustype buses, void *data); /* programmer.c */ -void fallback_chip_writew(const struct flashctx *flash, uint16_t val, chipaddr addr); -void fallback_chip_writel(const struct flashctx *flash, uint32_t val, chipaddr addr); -void fallback_chip_writen(const struct flashctx *flash, const uint8_t *buf, chipaddr addr, size_t len); -uint16_t fallback_chip_readw(const struct flashctx *flash, const chipaddr addr); -uint32_t fallback_chip_readl(const struct flashctx *flash, const chipaddr addr); -void fallback_chip_readn(const struct flashctx *flash, uint8_t *buf, const chipaddr addr, size_t len); struct registered_master { enum chipbustype buses_supported; struct { diff --git a/internal.c b/internal.c index 9b80f37..f21c4fc 100644 --- a/internal.c +++ b/internal.c @@ -116,7 +116,6 @@ .chip_writeb = internal_chip_writeb, .chip_writew = internal_chip_writew, .chip_writel = internal_chip_writel, - .chip_writen = fallback_chip_writen, }; static int get_params(const struct programmer_cfg *cfg, diff --git a/it8212.c b/it8212.c index 8fe2b59..3c1161d 100644 --- a/it8212.c +++ b/it8212.c @@ -64,13 +64,7 @@ static const struct par_master par_master_it8212 = { .chip_readb = it8212_chip_readb, - .chip_readw = fallback_chip_readw, - .chip_readl = fallback_chip_readl, - .chip_readn = fallback_chip_readn, .chip_writeb = it8212_chip_writeb, - .chip_writew = fallback_chip_writew, - .chip_writel = fallback_chip_writel, - .chip_writen = fallback_chip_writen, .shutdown = it8212_shutdown, }; diff --git a/nic3com.c b/nic3com.c index efba979..a578d48 100644 --- a/nic3com.c +++ b/nic3com.c @@ -91,13 +91,7 @@ static const struct par_master par_master_nic3com = { .chip_readb = nic3com_chip_readb, - .chip_readw = fallback_chip_readw, - .chip_readl = fallback_chip_readl, - .chip_readn = fallback_chip_readn, .chip_writeb = nic3com_chip_writeb, - .chip_writew = fallback_chip_writew, - .chip_writel = fallback_chip_writel, - .chip_writen = fallback_chip_writen, .shutdown = nic3com_shutdown, }; diff --git a/nicintel.c b/nicintel.c index 47edbb7..feb07b6 100644 --- a/nicintel.c +++ b/nicintel.c @@ -67,13 +67,7 @@ static const struct par_master par_master_nicintel = { .chip_readb = nicintel_chip_readb, - .chip_readw = fallback_chip_readw, - .chip_readl = fallback_chip_readl, - .chip_readn = fallback_chip_readn, .chip_writeb = nicintel_chip_writeb, - .chip_writew = fallback_chip_writew, - .chip_writel = fallback_chip_writel, - .chip_writen = fallback_chip_writen, .shutdown = nicintel_shutdown, }; diff --git a/nicnatsemi.c b/nicnatsemi.c index efa879a..65377dc 100644 --- a/nicnatsemi.c +++ b/nicnatsemi.c @@ -78,13 +78,7 @@ static const struct par_master par_master_nicnatsemi = { .chip_readb = nicnatsemi_chip_readb, - .chip_readw = fallback_chip_readw, - .chip_readl = fallback_chip_readl, - .chip_readn = fallback_chip_readn, .chip_writeb = nicnatsemi_chip_writeb, - .chip_writew = fallback_chip_writew, - .chip_writel = fallback_chip_writel, - .chip_writen = fallback_chip_writen, .shutdown = nicnatsemi_shutdown, }; diff --git a/nicrealtek.c b/nicrealtek.c index 103ea9e..5937e35 100644 --- a/nicrealtek.c +++ b/nicrealtek.c @@ -87,13 +87,7 @@ static const struct par_master par_master_nicrealtek = { .chip_readb = nicrealtek_chip_readb, - .chip_readw = fallback_chip_readw, - .chip_readl = fallback_chip_readl, - .chip_readn = fallback_chip_readn, .chip_writeb = nicrealtek_chip_writeb, - .chip_writew = fallback_chip_writew, - .chip_writel = fallback_chip_writel, - .chip_writen = fallback_chip_writen, .shutdown = nicrealtek_shutdown, }; diff --git a/parallel.c b/parallel.c index cea9e7a..b193723 100644 --- a/parallel.c +++ b/parallel.c @@ -27,19 +27,49 @@ flash->mst->par.chip_writeb(flash, val, addr); } +/* Little-endian fallback for drivers not supporting 16 bit accesses */ +static void fallback_chip_writew(const struct flashctx *flash, uint16_t val, + chipaddr addr) +{ + chip_writeb(flash, val & 0xff, addr); + chip_writeb(flash, (val >> 8) & 0xff, addr + 1); +} + void chip_writew(const struct flashctx *flash, uint16_t val, chipaddr addr) { - flash->mst->par.chip_writew(flash, val, addr); + if (flash->mst->par.chip_writew) + flash->mst->par.chip_writew(flash, val, addr); + fallback_chip_writew(flash, val, addr); +} + +/* Little-endian fallback for drivers not supporting 32 bit accesses */ +static void fallback_chip_writel(const struct flashctx *flash, uint32_t val, + chipaddr addr) +{ + chip_writew(flash, val & 0xffff, addr); + chip_writew(flash, (val >> 16) & 0xffff, addr + 2); } void chip_writel(const struct flashctx *flash, uint32_t val, chipaddr addr) { - flash->mst->par.chip_writel(flash, val, addr); + if (flash->mst->par.chip_writel) + flash->mst->par.chip_writel(flash, val, addr); + fallback_chip_writel(flash, val, addr); +} + +static void fallback_chip_writen(const struct flashctx *flash, const uint8_t *buf, chipaddr addr, size_t len) +{ + size_t i; + for (i = 0; i < len; i++) + chip_writeb(flash, buf[i], addr + i); + return; } void chip_writen(const struct flashctx *flash, const uint8_t *buf, chipaddr addr, size_t len) { - flash->mst->par.chip_writen(flash, buf, addr, len); + if (flash->mst->par.chip_writen) + flash->mst->par.chip_writen(flash, buf, addr, len); + fallback_chip_writen(flash, buf, addr, len); } uint8_t chip_readb(const struct flashctx *flash, const chipaddr addr) @@ -47,20 +77,53 @@ return flash->mst->par.chip_readb(flash, addr); } +/* Little-endian fallback for drivers not supporting 16 bit accesses */ +static uint16_t fallback_chip_readw(const struct flashctx *flash, const chipaddr addr) +{ + uint16_t val; + val = chip_readb(flash, addr); + val |= chip_readb(flash, addr + 1) << 8; + return val; +} + uint16_t chip_readw(const struct flashctx *flash, const chipaddr addr) { - return flash->mst->par.chip_readw(flash, addr); + if (flash->mst->par.chip_readw) + return flash->mst->par.chip_readw(flash, addr); + return fallback_chip_readw(flash, addr); +} + +/* Little-endian fallback for drivers not supporting 32 bit accesses */ +static uint32_t fallback_chip_readl(const struct flashctx *flash, const chipaddr addr) +{ + uint32_t val; + val = chip_readw(flash, addr); + val |= chip_readw(flash, addr + 2) << 16; + return val; } uint32_t chip_readl(const struct flashctx *flash, const chipaddr addr) { - return flash->mst->par.chip_readl(flash, addr); + if (flash->mst->par.chip_readl) + return flash->mst->par.chip_readl(flash, addr); + return fallback_chip_readl(flash, addr); +} + +static void fallback_chip_readn(const struct flashctx *flash, uint8_t *buf, + chipaddr addr, size_t len) +{ + size_t i; + for (i = 0; i < len; i++) + buf[i] = chip_readb(flash, addr + i); + return; } void chip_readn(const struct flashctx *flash, uint8_t *buf, chipaddr addr, size_t len) { - flash->mst->par.chip_readn(flash, buf, addr, len); + if (flash->mst->par.chip_readn) + flash->mst->par.chip_readn(flash, buf, addr, len); + fallback_chip_readn(flash, buf, addr, len); } int register_par_master(const struct par_master *mst, @@ -87,9 +150,7 @@ return ERROR_FLASHROM_BUG; } - if (!mst->chip_writeb || !mst->chip_writew || !mst->chip_writel || - !mst->chip_writen || !mst->chip_readb || !mst->chip_readw || - !mst->chip_readl || !mst->chip_readn) { + if (!mst->chip_writeb || !mst->chip_readb) { msg_perr("%s called with incomplete master definition. " "Please report a bug at flashrom(a)flashrom.org\n", __func__); diff --git a/programmer.c b/programmer.c index d4f15a1..15fd1b8 100644 --- a/programmer.c +++ b/programmer.c @@ -17,57 +17,6 @@ #include "flash.h" #include "programmer.h" -/* Little-endian fallback for drivers not supporting 16 bit accesses */ -void fallback_chip_writew(const struct flashctx *flash, uint16_t val, - chipaddr addr) -{ - chip_writeb(flash, val & 0xff, addr); - chip_writeb(flash, (val >> 8) & 0xff, addr + 1); -} - -/* Little-endian fallback for drivers not supporting 16 bit accesses */ -uint16_t fallback_chip_readw(const struct flashctx *flash, const chipaddr addr) -{ - uint16_t val; - val = chip_readb(flash, addr); - val |= chip_readb(flash, addr + 1) << 8; - return val; -} - -/* Little-endian fallback for drivers not supporting 32 bit accesses */ -void fallback_chip_writel(const struct flashctx *flash, uint32_t val, - chipaddr addr) -{ - chip_writew(flash, val & 0xffff, addr); - chip_writew(flash, (val >> 16) & 0xffff, addr + 2); -} - -/* Little-endian fallback for drivers not supporting 32 bit accesses */ -uint32_t fallback_chip_readl(const struct flashctx *flash, const chipaddr addr) -{ - uint32_t val; - val = chip_readw(flash, addr); - val |= chip_readw(flash, addr + 2) << 16; - return val; -} - -void fallback_chip_writen(const struct flashctx *flash, const uint8_t *buf, chipaddr addr, size_t len) -{ - size_t i; - for (i = 0; i < len; i++) - chip_writeb(flash, buf[i], addr + i); - return; -} - -void fallback_chip_readn(const struct flashctx *flash, uint8_t *buf, - chipaddr addr, size_t len) -{ - size_t i; - for (i = 0; i < len; i++) - buf[i] = chip_readb(flash, addr + i); - return; -} - /* The limit of 4 is totally arbitrary. */ #define MASTERS_MAX 4 struct registered_master registered_masters[MASTERS_MAX]; diff --git a/satamv.c b/satamv.c index dcd53c7..fea4c2b 100644 --- a/satamv.c +++ b/satamv.c @@ -88,13 +88,7 @@ static const struct par_master par_master_satamv = { .chip_readb = satamv_chip_readb, - .chip_readw = fallback_chip_readw, - .chip_readl = fallback_chip_readl, - .chip_readn = fallback_chip_readn, .chip_writeb = satamv_chip_writeb, - .chip_writew = fallback_chip_writew, - .chip_writel = fallback_chip_writel, - .chip_writen = fallback_chip_writen, .shutdown = satamv_shutdown, }; diff --git a/satasii.c b/satasii.c index 011562e..e881f96 100644 --- a/satasii.c +++ b/satasii.c @@ -95,13 +95,7 @@ static const struct par_master par_master_satasii = { .chip_readb = satasii_chip_readb, - .chip_readw = fallback_chip_readw, - .chip_readl = fallback_chip_readl, - .chip_readn = fallback_chip_readn, .chip_writeb = satasii_chip_writeb, - .chip_writew = fallback_chip_writew, - .chip_writel = fallback_chip_writel, - .chip_writen = fallback_chip_writen, .shutdown = satasii_shutdown, }; diff --git a/serprog.c b/serprog.c index 4e131e5..5ddbe3d 100644 --- a/serprog.c +++ b/serprog.c @@ -594,13 +594,8 @@ static const struct par_master par_master_serprog = { .map_flash_region = serprog_map, .chip_readb = serprog_chip_readb, - .chip_readw = fallback_chip_readw, - .chip_readl = fallback_chip_readl, .chip_readn = serprog_chip_readn, .chip_writeb = serprog_chip_writeb, - .chip_writew = fallback_chip_writew, - .chip_writel = fallback_chip_writel, - .chip_writen = fallback_chip_writen, .delay = serprog_delay, }; -- To view, visit https://review.coreboot.org/c/flashrom/+/71745 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If25c0048a07057aa72be6ffa8d8ad7f0a568dcf7 Gerrit-Change-Number: 71745 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: tree/: Change chip restore data type from uint8_t to void ptr
by Nikolai Artemiev (Code Review) 09 Jan '23

09 Jan '23

Attention is currently required from: Nikolai Artemiev. Hello build bot (Jenkins), Edward O'Callaghan, Sergii Dmytruk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/70349 to look at the new patch set (#7). Change subject: tree/: Change chip restore data type from uint8_t to void ptr ...................................................................... tree/: Change chip restore data type from uint8_t to void ptr Chip restore callbacks currently are used by - spi25_statusreg.c unlock functions to restore status register 1. - s25f.c to restore config register 3. Both of these cases only need to save a single uint8_t value to restore the original chip state, however storing a void pointer will allow more flexible chip restore behaviour. In particular, it will allow flashrom_wp_cfg objects to be saved and restored, enabling writeprotect-based unlocking. BUG=b:237485865,b:247421511 BRANCH=none TEST=Tested on grunt DUT (prog: sb600spi, flash: W25Q128.W): `flashrom --wp-range 0x0,0x1000000 \ flashrom --wp-status # Result: range=0x0,0x1000000 \ flashrom -w random.bin # Result: success \ flashrom -v random.bin # Result: success \ flashrom --wp-status # Result: range=0x0,0x1000000` Change-Id: I311b468a4b0349f4da9584c12b36af6ec2394527 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M flashrom.c M include/flash.h M s25f.c M spi25_statusreg.c 4 files changed, 57 insertions(+), 10 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/49/70349/7 -- To view, visit https://review.coreboot.org/c/flashrom/+/70349 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I311b468a4b0349f4da9584c12b36af6ec2394527 Gerrit-Change-Number: 70349 Gerrit-PatchSet: 7 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: programmer.h: Guard against sending spi commands on non-spi mst
by Edward O'Callaghan (Code Review) 08 Jan '23

08 Jan '23

Attention is currently required from: Sam McNally, Felix Singer, Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71269 ) Change subject: programmer.h: Guard against sending spi commands on non-spi mst ...................................................................... Patch Set 2: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/71269/comment/08fd139d_ef244c11 PS1, Line 1937: spi_master_no_4ba_modes > I feel like the code needs a bit of work.. […] Sam, https://review.coreboot.org/c/flashrom/+/71742 -- To view, visit https://review.coreboot.org/c/flashrom/+/71269 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7cce4f9c032d33c01bf616e27a50b9727a40fe1b Gerrit-Change-Number: 71269 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Sam McNally <sammc(a)google.com> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 08 Jan 2023 01:42:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sam McNally <sammc(a)google.com> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom.c: Rewrite prepare_chip_access() 4BA enablement
by Edward O'Callaghan (Code Review) 08 Jan '23

08 Jan '23

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/71742 ) Change subject: flashrom.c: Rewrite prepare_chip_access() 4BA enablement ...................................................................... flashrom.c: Rewrite prepare_chip_access() 4BA enablement Factor out the 4BA enablement logic into its own function and break down predicates into a series of easy to follow base-cases complete with comments. TEST=`$ sudo ./flashrom -p internal -r /tmp/bios`. on a Found chipset "Intel Tiger Lake U Premium". Change-Id: I10a426331cb2a3485e3dd786fa771e20870cbd84 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashrom.c 1 file changed, 57 insertions(+), 24 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/42/71742/1 diff --git a/flashrom.c b/flashrom.c index 822594b..f7499c3 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1905,6 +1905,45 @@ return 0; } +static int enable_4ba_if_required(struct flashctx *const flash) +{ + /* Check if chip too small to need 4BA. */ + if (flash->chip->total_size <= 16 * 1024) { + return 0; + } + + /* Check if SPI bus with 4BA enabled has a SPI master missing 4BA mode. */ + if (!spi_master_no_4ba_modes(flash)) { + return 0; + } + + /* Check if SPI bus with 4BA has a SPI master that supports a 4BA mode. */ + if (!spi_master_4ba(flash)) { + msg_cerr("Programmer doesn't support this chip. Aborting.\n"); + return 1; + } + + /* Check chip supports native 4BA instructions and if not bail out. */ + if ((flash->chip->feature_bits & FEATURE_4BA_NATIVE) != FEATURE_4BA_NATIVE) { + msg_cerr("Chip does not support 4BA Native instructions. Aborting.\n"); + return 1; + } + + /* Check if chip is on a chipbus that supports 4BA addressing mode. */ + if (!spi_chip_4ba(flash)) { + return 0; + } + + /* Enable/disable 4-byte addressing mode. */ + int ret = spi_master_4ba(flash) ? spi_enter_4ba(flash): spi_exit_4ba(flash); + if (ret) { + msg_cerr("Failed to set correct 4BA mode! Aborting.\n"); + return 1; + } + + return 0; +} + int prepare_flash_access(struct flashctx *const flash, const bool read_it, const bool write_it, const bool erase_it, const bool verify_it) @@ -1933,30 +1972,7 @@ flash->address_high_byte = -1; flash->in_4ba_mode = false; - /* Be careful about 4BA chips and broken masters */ - if (flash->chip->total_size > 16 * 1024 && spi_master_no_4ba_modes(flash)) { - /* If we can't use native instructions, bail out */ - if ((flash->chip->feature_bits & FEATURE_4BA_NATIVE) != FEATURE_4BA_NATIVE - || !spi_master_4ba(flash)) { - msg_cerr("Programmer doesn't support this chip. Aborting.\n"); - return 1; - } - } - - /* Enable/disable 4-byte addressing mode if flash chip supports it */ - if (spi_chip_4ba(flash)) { - int ret; - if (spi_master_4ba(flash)) - ret = spi_enter_4ba(flash); - else - ret = spi_exit_4ba(flash); - if (ret) { - msg_cerr("Failed to set correct 4BA mode! Aborting.\n"); - return 1; - } - } - - return 0; + return enable_4ba_if_required(flash); } void finalize_flash_access(struct flashctx *const flash) -- To view, visit https://review.coreboot.org/c/flashrom/+/71742 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I10a426331cb2a3485e3dd786fa771e20870cbd84 Gerrit-Change-Number: 71742 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[master]: programmer.h: Guard against sending spi commands on non-spi mst
by Edward O'Callaghan (Code Review) 07 Jan '23

07 Jan '23

Attention is currently required from: Sam McNally, Felix Singer, Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71269 ) Change subject: programmer.h: Guard against sending spi commands on non-spi mst ...................................................................... Patch Set 2: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/71269/comment/3751906d_36a73bbc PS1, Line 1937: spi_master_no_4ba_modes > Could we skip this entire section on 4ba for non-SPI masters? I feel like the code needs a bit of work.. Can we just fix the immediate issue first (>16MB flashing is broken on ichspi). -- To view, visit https://review.coreboot.org/c/flashrom/+/71269 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7cce4f9c032d33c01bf616e27a50b9727a40fe1b Gerrit-Change-Number: 71269 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Sam McNally <sammc(a)google.com> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sat, 07 Jan 2023 09:16:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sam McNally <sammc(a)google.com> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashchips.c: Add support for IS25WQ040
by Vasily Galkin (Code Review) 07 Jan '23

07 Jan '23

Attention is currently required from: Felix Singer, Vasily Galkin, Thomas Heijligen, roman.stingler(a)gmail.com. Hello Felix Singer, build bot (Jenkins), Thomas Heijligen, roman.stingler(a)gmail.com, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/58179 to look at the new patch set (#5). Change subject: flashchips.c: Add support for IS25WQ040 ...................................................................... flashchips.c: Add support for IS25WQ040 Based on https://github.com/flashrom/flashrom/pull/204 squashed with fixes of IS25WQ040 size: it is 4Mbits, not 4MBytes, see https://www.issi.com/WW/pdf/25WQ020-040.pdf Tested read, write and erase with ft2232_spi-based "Tigard" programmer. Change-Id: I072c6b94d7931637d1c2721c3316205f2d57320e Signed-off-by: Roman Stingler <roman.stingler(a)gmail.com> Signed-off-by: Vasily Galkin <galkin-vv(a)ya.ru> --- M flashchips.c M include/flashchips.h 2 files changed, 60 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/58179/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/58179 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I072c6b94d7931637d1c2721c3316205f2d57320e Gerrit-Change-Number: 58179 Gerrit-PatchSet: 5 Gerrit-Owner: Vasily Galkin Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: roman.stingler(a)gmail.com Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Vasily Galkin Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: roman.stingler(a)gmail.com Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: flashchips.c: Add support for IS25WQ040
by Vasily Galkin (Code Review) 07 Jan '23

07 Jan '23

Attention is currently required from: Felix Singer, Thomas Heijligen, roman.stingler(a)gmail.com. Hello Felix Singer, build bot (Jenkins), Thomas Heijligen, roman.stingler(a)gmail.com, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/58179 to look at the new patch set (#4). Change subject: flashchips.c: Add support for IS25WQ040 ...................................................................... flashchips.c: Add support for IS25WQ040 Based on https://github.com/flashrom/flashrom/pull/204 squashed with fixes of IS25WQ040 size: it is 4Mbits, not 4MBytes, see https://www.issi.com/WW/pdf/25WQ020-040.pdf Tested read, write and erase with ft2232_spi-based "Tigard" programmer. Change-Id: I072c6b94d7931637d1c2721c3316205f2d57320e Signed-off-by: Roman Stingler <roman.stingler(a)gmail.com> Signed-off-by: Vasily Galkin <galkin-vv(a)ya.ru> --- M flashchips.c M include/flashchips.h 2 files changed, 60 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/58179/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/58179 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I072c6b94d7931637d1c2721c3316205f2d57320e Gerrit-Change-Number: 58179 Gerrit-PatchSet: 4 Gerrit-Owner: Vasily Galkin Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: roman.stingler(a)gmail.com Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: roman.stingler(a)gmail.com Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: chipset_enable.c: Add TL UP3 and UP4/Y id's
by Sam McNally (Code Review) 06 Jan '23

06 Jan '23

Attention is currently required from: Edward O'Callaghan, Angel Pons. Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71576 ) Change subject: chipset_enable.c: Add TL UP3 and UP4/Y id's ...................................................................... Patch Set 1: (1 comment) File chipset_enable.c: https://review.coreboot.org/c/flashrom/+/71576/comment/637d65cb_688eb081 PS1, Line 2083: {0x8086, 0xa083, B_S, DEP, "Intel", "Tiger Lake U Premium 3", enable_flash_pch500}, > Let's keep it, but mark it as untested (change `DEP` to `NT`). Sure; going off the coreboot #define, I think this should be "Tiger Lake Base UP4" or "Tiger Lake U Base", depending on the naming scheme used for the others.. -- To view, visit https://review.coreboot.org/c/flashrom/+/71576 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I77120612f7a770ae1319f4cd82913fa465f355f5 Gerrit-Change-Number: 71576 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 06 Jan 2023 10:50:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sam McNally <sammc(a)google.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: programmer.h: Guard against sending spi commands on non-spi mst
by Sam McNally (Code Review) 06 Jan '23

06 Jan '23

Attention is currently required from: Felix Singer, Edward O'Callaghan, Angel Pons. Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71269 ) Change subject: programmer.h: Guard against sending spi commands on non-spi mst ...................................................................... Patch Set 2: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/71269/comment/d2310077_4c7d1571 PS1, Line 1937: spi_master_no_4ba_modes > sort of but not really. This is a chip bustype check not a master bustype check. […] Could we skip this entire section on 4ba for non-SPI masters? -- To view, visit https://review.coreboot.org/c/flashrom/+/71269 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7cce4f9c032d33c01bf616e27a50b9727a40fe1b Gerrit-Change-Number: 71269 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Sam McNally <sammc(a)google.com> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 06 Jan 2023 10:48:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests/: Add subregion alignment unit-test
by Evan Benn (Code Review) 06 Jan '23

06 Jan '23

Attention is currently required from: Sam McNally, Edward O'Callaghan, Anastasia Klimchuk. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71659 ) Change subject: tests/: Add subregion alignment unit-test ...................................................................... Patch Set 2: (17 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/71659/comment/a709f11e_40075213 PS2, Line 7: tests/: Add subregion alignment unit-test is a subregion a thing? maybe it is not the alignment but the size relative to erase sizes thats important? https://review.coreboot.org/c/flashrom/+/71659/comment/7fba71d2_252033e7 PS2, Line 9: A written region that is sized below that of the erasure granularity I don't understand the bug from this description File tests/chip.c: https://review.coreboot.org/c/flashrom/+/71659/comment/5586f951_4838fef1 PS2, Line 110: va_list logfile_args; you dont need to va_copy because you only pass ap to vfprintf https://review.coreboot.org/c/flashrom/+/71659/comment/bc8150ef_1cbe26a8 PS2, Line 114: output_type = stderr; I don't know the motivation for printing the messages, so I also dont know the motivation for splitting them into two different streams, curious to learn though https://review.coreboot.org/c/flashrom/+/71659/comment/1e0e9263_e6658fa8 PS2, Line 116: if (level <= verbose_screen) { verbose_screen is part of `cli_output.c`, maybe something else here? https://review.coreboot.org/c/flashrom/+/71659/comment/fb83b4b7_1e1024df PS2, Line 118: /* msg_*spew often happens inside chip accessors in possibly why do we need to fflush at all? https://review.coreboot.org/c/flashrom/+/71659/comment/4a7a5156_58a84076 PS2, Line 444: void write_chip_subregion_with_dummyflasher_test_success(void **state) // XXX this is the first mention of a 'subregion' in flashrom, maybe there is an existing word you can choose, but I think its just a 'region'. maybe the test is 'write_region_smaller_than_erase_granularity' https://review.coreboot.org/c/flashrom/+/71659/comment/942e87bc_b03b1f47 PS2, Line 458: struct flashchip mock_chip = chip_W25Q128_V; do you want to assert something about the size of the erasers here? as much for documentation as ensuring it doesnt get modified accidently https://review.coreboot.org/c/flashrom/+/71659/comment/389cfb5d_b58bb732 PS2, Line 466: #define MOCK_CHIP_SUBREGION_CONTENTS 0xCC this can be a const int instead of a define https://review.coreboot.org/c/flashrom/+/71659/comment/005ad273_3e93ca71 PS2, Line 469: * {MOCK_CHIP_SUBREGION_CONTENTS} [..] {MOCK_CHIP_SUBREGION_CONTENTS}. im not sure what this is telling me https://review.coreboot.org/c/flashrom/+/71659/comment/45070264_b17b9b3a PS2, Line 471: uint8_t *newcontents = calloc(1, mock_chip_size); memsetting so calloc could be malloc https://review.coreboot.org/c/flashrom/+/71659/comment/49603d7c_332879b5 PS2, Line 477: flashrom_flag_set(&flashctx, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, false); suspect these flags are important what is being tested, but its not documented here or https://source.chromium.org/chromiumos/chromiumos/codesearch/+/HEAD:src/thi… or https://source.chromium.org/chromiumos/chromiumos/codesearch/+/HEAD:src/thi… so i dont actually know what this does https://review.coreboot.org/c/flashrom/+/71659/comment/56f20945_29dc8e05 PS2, Line 478: flashrom_set_log_callback((flashrom_log_callback *)&unittest_print_cb); why are we setting up a print callback? We also dont seem to undo this at the end, which will be tricky because the macros return early. maybe this should go at the unit test top level https://review.coreboot.org/c/flashrom/+/71659/comment/d91320c0_1f4bb962 PS2, Line 485: flashrom_layout_release(layout); flashctx->layout is dangling. could set it to default_layout or NULL, which seem to do the same thing but neither way is documented https://review.coreboot.org/c/flashrom/+/71659/comment/2fe11234_1c758365 PS2, Line 502: printf("Subregion chip W op..\n"); write the word https://review.coreboot.org/c/flashrom/+/71659/comment/ac67e596_0ffdf83e PS2, Line 508: //flashrom_layout_set(&flashctx, NULL); // use default layout. the layout you use is the same as the default or NULL layout, does that not work? https://review.coreboot.org/c/flashrom/+/71659/comment/f11e3614_95d8a45d PS2, Line 514: assert_int_equal(0, flashrom_image_verify(&flashctx, newcontents, mock_chip_size)); maybe documenting right here what the predicted bug is would be useful. does the test fail here or at the write step? Does this test catch a real bug on some patch? I would like to have a look. -- To view, visit https://review.coreboot.org/c/flashrom/+/71659 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id3ce5cd1936f0f348d34a6c77cee15e27a5c353f Gerrit-Change-Number: 71659 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 06 Jan 2023 03:35:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit