Attention is currently required from: Felix Singer, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev, Nicholas Chin.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58735 )
Change subject: ichspi: Split very long init function into two
......................................................................
Patch Set 7:
(1 comment)
Patchset:
PS7:
> Now that this is tested I've found a ThinkPad T60 in one of my […]
My memory tries to tell me something... back in 2012 we were
first trying coreboot on a spare T60 with broken display. This
might be the very first computer I've ever used flashrom on *and*
the first I've flashed coreboot to!!! ^^
--
To view, visit https://review.coreboot.org/c/flashrom/+/58735
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6789bc456a4878e6555831ae0b80ecbdbf62938b
Gerrit-Change-Number: 58735
Gerrit-PatchSet: 7
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
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-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Comment-Date: Wed, 26 Jan 2022 23:43:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev, Nicholas Chin.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58735 )
Change subject: ichspi: Split very long init function into two
......................................................................
Patch Set 7: Code-Review+1
(2 comments)
Patchset:
PS7:
Now that this is tested I've found a ThinkPad T60 in one of my
laptop piles /o\ didn't try to boot it up yet, and I have no
idea whose it is oO
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/58735/comment/fb5f3205_2049dfa2
PS7, Line 1771: ich_generation = ich_gen;
: ich_spibar = spibar;
These two are globals and also affect the ICH7 paths. I guess we
missed them because they are not used for ICH7 in the original init
function but only later during runtime.
I've also checked all other writes to globals on the common paths
of the original init function. There is only `swseq_data`, which is
not used for ICH7 (although I noticed that it could be used to
avoid code duplication in some cases, but that's for another day).
--
To view, visit https://review.coreboot.org/c/flashrom/+/58735
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6789bc456a4878e6555831ae0b80ecbdbf62938b
Gerrit-Change-Number: 58735
Gerrit-PatchSet: 7
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
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-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Comment-Date: Wed, 26 Jan 2022 23:36:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons.
Neill Corlett has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61288 )
Change subject: Add mediatek_i2c_spi interface
......................................................................
Patch Set 2:
(3 comments)
Patchset:
PS1:
> Ah, hmmm, I'm not sure but I think there are SMBus controller drivers that could
> still advertise I2C_FUNC_I2C. Can you please confirm that this is not the case.
> Or can you tell us what controller is used specifically?
>
> Also, is the chip aware if it is connected over SMBus or I2C, e.g. by a pin strap?
I will research this and get back to you. If necessary, we can remove the SMBus code path from mainline, if that would simplify things. Only very early prototype hardware needs it.
> Who is PanVision in this story?
They are a vendor who has provided us with the scaler firmware running on the MST chip.
> Also, where do you use these chips, integrated display or something external?
> If you used SMBus, is it even on the mainboard?
This is the scaler chip for the integrated display on the Google Meet Series One Desk 27. It's attached to I2C on the main x86_64 application CPU.
File mediatek_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/61288/comment/dbcfc9df_5a9edf76
PS1, Line 96: args.data->block
> I would use `args.data->block + 1` when targeting an array and […]
Inclined to leave as-is unless there are strong objections.
https://review.coreboot.org/c/flashrom/+/61288/comment/1b0b2ff1_d36d7578
PS1, Line 249: 0x426, 7
> Hmmm, I see. […]
My understanding is that this is the usual, expected, line that's supposed to go to WP#. In theory it could be wired up another way.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61288
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I24adb14e7b4f7160e1c3ff941774064d5a81e820
Gerrit-Change-Number: 61288
Gerrit-PatchSet: 2
Gerrit-Owner: Neill Corlett <corlett(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Wed, 26 Jan 2022 20:33:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Neill Corlett <corlett(a)google.com>
Gerrit-MessageType: comment
Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/60055 )
Change subject: SFDP: make mandatory table length check work with newer SFDP revisions
......................................................................
SFDP: make mandatory table length check work with newer SFDP revisions
The JEDEC SFDP specification JESD216A (1.5) adds five new DWORDs to the
Basic Flash Parameter Table. Later versions of the spec add even more
fields. This increases the table being read from 36 bytes to currently
64 bytes and makes flashrom bail out for any SFDP version >= 1.5 due to
a static table length check.
This was discovered on a GigaDevice GD25B127DSIGR from 2021 with SFDP
revision 1.6, while another flash of the same model from 2020 with SFDP
revision 1.0 was detected fine by flashrom.
GD25B127DSIGR - 2020 version:
Probing for Unknown SFDP-capable chip, 0 kB: SFDP revision = 1.0
SFDP number of parameter headers is 2 (NPH = 1).
SFDP parameter table header 0/1:
ID 0x00, version 1.0
Length 36 B, Parameter Table Pointer 0x000030
GD25B127DSIGR - 2021 version:
Probing for Unknown SFDP-capable chip, 0 kB: SFDP revision = 1.6
SFDP number of parameter headers is 2 (NPH = 1).
SFDP parameter table header 0/1:
ID 0x00, version 1.6
Length 64 B, Parameter Table Pointer 0x000030
...
Length of the mandatory JEDEC SFDP parameter table is wrong (64 B),
skipping it.
The specification says that changes of the minor SFDP revision will
maintain compatibility. Thus, simply check for the minimal required
table length, which is 16 bytes for legacy Intel pre-SFDP and 36 bytes
for SFDP.
Change-Id: Id84cde4ebc805d68e2984e8041fbc48d7ceebe34
Signed-off-by: Michael Niewöhner <foss(a)mniewoehner.de>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/60055
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Nico Huber <nico.h(a)gmx.de>
---
M sfdp.c
1 file changed, 1 insertion(+), 1 deletion(-)
Approvals:
build bot (Jenkins): Verified
Nico Huber: Looks good to me, approved
diff --git a/sfdp.c b/sfdp.c
index 85e698d..b549417 100644
--- a/sfdp.c
+++ b/sfdp.c
@@ -369,7 +369,7 @@
msg_cdbg("The chip contains an unknown "
"version of the JEDEC flash "
"parameters table, skipping it.\n");
- } else if (len != 9 * 4 && len != 4 * 4) {
+ } else if (len != 4 * 4 && len < 9 * 4) {
msg_cdbg("Length of the mandatory JEDEC SFDP "
"parameter table is wrong (%d B), "
"skipping it.\n", len);
--
To view, visit https://review.coreboot.org/c/flashrom/+/60055
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id84cde4ebc805d68e2984e8041fbc48d7ceebe34
Gerrit-Change-Number: 60055
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <edward.ocallaghan(a)koparo.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-MessageType: merged
Attention is currently required from: Edward O'Callaghan, Angel Pons, Neill Corlett.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61288 )
Change subject: Add mediatek_i2c_spi interface
......................................................................
Patch Set 2:
(2 comments)
Patchset:
PS1:
> - We had two different board designs, one of which connects the TSUM* to SMBus and the other to raw I2C. This driver is tested on both. The finished product will be using I2C. I've left the SMBus code in as a convenience so we can continue to flash our earlier prototypes.
Ah, hmmm, I'm not sure but I think there are SMBus controller drivers that could
still advertise I2C_FUNC_I2C. Can you please confirm that this is not the case.
Or can you tell us what controller is used specifically?
Also, is the chip aware if it is connected over SMBus or I2C, e.g. by a pin strap?
> - GPIO numbers are specific to TSUMOP82JUQ. My understanding is that these particular GPIO lines, for that chip, are standard for connecting to SPI WP#. Documentation that we received from PanVision just says to poke them and doesn't provide further detail.
Who is PanVision in this story? Did they acquire the MSTAR chip line and provide
the chips? or do they provide the board? or ...?
Also, where do you use these chips, integrated display or something external?
If you used SMBus, is it even on the mainboard?
File mediatek_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/61288/comment/e2d50ac8_de21b96a
PS1, Line 96: args.data->block
> I imagine Edward would like to avoid pointer arithmetics, substituting `args. […]
I would use `args.data->block + 1` when targeting an array and
`&args.data->block[1]` only if I target a single member.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61288
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I24adb14e7b4f7160e1c3ff941774064d5a81e820
Gerrit-Change-Number: 61288
Gerrit-PatchSet: 2
Gerrit-Owner: Neill Corlett <corlett(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Neill Corlett <corlett(a)google.com>
Gerrit-Comment-Date: Wed, 26 Jan 2022 15:18:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Neill Corlett <corlett(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Werner Zeh.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61362 )
Change subject: ich_descriptors.c Invert the meaning of 'dual_output' bit
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/61362
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If6282ac8326ab0b92e9c70c09dba0299bf0deb6f
Gerrit-Change-Number: 61362
Gerrit-PatchSet: 2
Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Comment-Date: Wed, 26 Jan 2022 13:19:19 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Neill Corlett.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61288 )
Change subject: Add mediatek_i2c_spi interface
......................................................................
Patch Set 2:
(3 comments)
File mediatek_i2c_spi.c:
https://review.coreboot.org/c/flashrom/+/61288/comment/4c5d2b8f_a59c3ac9
PS1, Line 96: args.data->block
> This is copying the data out of the i2c_smbus_data, starting at 1, using the size specified by [0].
I imagine Edward would like to avoid pointer arithmetics, substituting `args.data->block + 1` with `&args.data->block[1]`.
https://review.coreboot.org/c/flashrom/+/61288/comment/1c39616f_6a30553b
PS1, Line 242: GPIO line
> This is specific to the revision of chip that our board uses (TSUMOP82JUQ).
At the very least, I'd add an option to enable WP# control.
https://review.coreboot.org/c/flashrom/+/61288/comment/6b711c64_fa9c87a6
PS1, Line 249: 0x426, 7
> The TSUMOP82JUQ has a GPIO line going to WP# on SPI, and these two registers (0x426 and 0x428) contr […]
Hmmm, I see. Could other boards with the same chip use a different GPIO for WP#?
--
To view, visit https://review.coreboot.org/c/flashrom/+/61288
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I24adb14e7b4f7160e1c3ff941774064d5a81e820
Gerrit-Change-Number: 61288
Gerrit-PatchSet: 2
Gerrit-Owner: Neill Corlett <corlett(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Neill Corlett <corlett(a)google.com>
Gerrit-Comment-Date: Wed, 26 Jan 2022 13:01:37 +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>
Comment-In-Reply-To: Neill Corlett <corlett(a)google.com>
Gerrit-MessageType: comment