Change in flashrom[master]: chipset_enable.c: Add Apollolake and Geminilake support

List overview All Threads
Download

newer

older

Change in flashrom[master]:...

Change in flashrom[master]: Add...

Edward O'Callaghan (Code Review)

2 Nov 2020 2 Nov '20

5:58 a.m.

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/47091 )

Change subject: chipset_enable.c: Add Apollolake and Geminilake support ......................................................................

chipset_enable.c: Add Apollolake and Geminilake support

Adds product id's: 0x5af0 for Apollolake and 0x31f0 for Geminilake.

Currently, on Apollolake platform, the SPI PCI device is hidden in the OS. Thus, flashrom is not able to find the SPI device while walking the PCI tree. Instead use the PCI ID of APL host bridge. In the callback for enabling APL flash, use mmio-based access to mmap SPI PCI device and work with it.

BUG=none BRANCH=none TEST=none

Change-Id: I533a426bbf8e9c5efef0cf693693086e8efc1f57 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M chipset_enable.c 1 file changed, 2 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/91/47091/1

diff --git a/chipset_enable.c b/chipset_enable.c index c664918..01d0291 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -1896,6 +1896,7 @@ {0x8086, 0x2917, B_FS, DEP, "Intel", "ICH9M-E", enable_flash_ich9}, {0x8086, 0x2918, B_FS, DEP, "Intel", "ICH9", enable_flash_ich9}, {0x8086, 0x2919, B_FS, DEP, "Intel", "ICH9M", enable_flash_ich9}, + {0x8086, 0x31f0, B_FS, OK, "Intel", "Geminilake", enable_flash_apl}, {0x8086, 0x3a10, B_FS, NT, "Intel", "ICH10R Eng. Sample", enable_flash_ich10}, {0x8086, 0x3a14, B_FS, DEP, "Intel", "ICH10DO", enable_flash_ich10}, {0x8086, 0x3a16, B_FS, DEP, "Intel", "ICH10R", enable_flash_ich10}, @@ -1920,6 +1921,7 @@ {0x8086, 0x3b16, B_FS, NT, "Intel", "3450", enable_flash_pch5}, {0x8086, 0x3b1e, B_FS, NT, "Intel", "B55", enable_flash_pch5}, {0x8086, 0x5031, B_FS, OK, "Intel", "EP80579", enable_flash_ich7}, + {0x8086, 0x5af0, B_FS, OK, "Intel", "Apollolake", enable_flash_apl}, {0x8086, 0x7000, B_P, OK, "Intel", "PIIX3", enable_flash_piix4}, {0x8086, 0x7110, B_P, OK, "Intel", "PIIX4/4E/4M", enable_flash_piix4}, {0x8086, 0x7198, B_P, OK, "Intel", "440MX", enable_flash_piix4},

Attachments:

attachment.htm (text/html — 4.0 KB)

Show replies by thread

Edward O'Callaghan (Code Review)

13 Nov 13 Nov

9:10 a.m.

New subject: Change in flashrom[master]: chipset_enable.c: Add Geminilake support

Hello build bot (Jenkins), Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/47091

to look at the new patch set (#2).

Change subject: chipset_enable.c: Add Geminilake support ......................................................................

chipset_enable.c: Add Geminilake support

Adds product id's for 0x31f0 for Geminilake. Apollolake is already in the list.

BUG=none BRANCH=none TEST=none

Change-Id: I533a426bbf8e9c5efef0cf693693086e8efc1f57 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M chipset_enable.c 1 file changed, 1 insertion(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/91/47091/2

Edward O'Callaghan (Code Review)

9:42 a.m.

New subject: Change in flashrom[master]: chipset_enable.c: Add Geminilake support

Hello Sam McNally, build bot (Jenkins), Angel Pons,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/flashrom/+/47091

to look at the new patch set (#4).

Change subject: chipset_enable.c: Add Geminilake support ......................................................................

chipset_enable.c: Add Geminilake support

Adds product id's for 0x31f0 for Geminilake. Apollolake is already in the list.

BUG=none BRANCH=none TEST=none

Change-Id: I533a426bbf8e9c5efef0cf693693086e8efc1f57 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M chipset_enable.c 1 file changed, 1 insertion(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/91/47091/4

Arthur Heymans (Code Review)

10:16 a.m.

New subject: Change in flashrom[master]: chipset_enable.c: Add Geminilake support

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47091 )

Change subject: chipset_enable.c: Add Geminilake support ......................................................................

Patch Set 4: Code-Review+2

-- To view, visit https://review.coreboot.org/c/flashrom/+/47091 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I533a426bbf8e9c5efef0cf693693086e8efc1f57 Gerrit-Change-Number: 47091 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 13 Nov 2020 09:16:16 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Angel Pons (Code Review)

12:47 p.m.

New subject: Change in flashrom[master]: chipset_enable.c: Add Geminilake support

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47091 )

Change subject: chipset_enable.c: Add Geminilake support ......................................................................

Patch Set 4:

Uh, Gemini Lake has a different descriptor layout. This change as-is will make flashrom assume an invalid descriptor (that for WildcatPoint). While the APL layout is good enough, there's a few differences to account for.

I've got CB:43349 which handles this, but I went berserk and made a full descriptor dumper out of it...

-- To view, visit https://review.coreboot.org/c/flashrom/+/47091 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I533a426bbf8e9c5efef0cf693693086e8efc1f57 Gerrit-Change-Number: 47091 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 13 Nov 2020 11:47:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Edward O'Callaghan (Code Review)

14 Nov 14 Nov

4:42 a.m.

New subject: Change in flashrom[master]: chipset_enable.c: Add Geminilake support

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47091 )

Change subject: chipset_enable.c: Add Geminilake support ......................................................................

Patch Set 4:

...

Patch Set 4:

Uh, Gemini Lake has a different descriptor layout. This change as-is will make flashrom assume an invalid descriptor (that for WildcatPoint). While the APL layout is good enough, there's a few differences to account for.

I've got CB:43349 which handles this, but I went berserk and made a full descriptor dumper out of it...

So while everything you said is true the support here seems sufficiently good enough as I am taking it verbatim from the ChromiumOS Flashrom that is used in production. Not that I am saying that's a gold standard by any means! Just that it's better than not working at all.

I am not sure what is the best way forwards here; have the basic support just merged and iterate on that or maybe move forwards you series? In my view the former seems like a pragmatic path however, either way, we should get it across the line so ich is completely in-sync with CrOS and so we have Intel directly sending these sorts of patches to Flashrom moving forwards rather than adhoc fixes.

-- To view, visit https://review.coreboot.org/c/flashrom/+/47091 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I533a426bbf8e9c5efef0cf693693086e8efc1f57 Gerrit-Change-Number: 47091 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sat, 14 Nov 2020 03:42:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Paul Menzel (Code Review)

9:02 a.m.

New subject: Change in flashrom[master]: chipset_enable.c: Add Geminilake support

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47091 )

Change subject: chipset_enable.c: Add Geminilake support ......................................................................

Patch Set 4:

(3 comments)

https://review.coreboot.org/c/flashrom/+/47091/4//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/flashrom/+/47091/4//COMMIT_MSG@7 PS4, Line 7: Geminilake Nit: Gemini Lake

https://review.coreboot.org/c/flashrom/+/47091/4//COMMIT_MSG@9 PS4, Line 9: Apollolake Apollo Lake

https://review.coreboot.org/c/flashrom/+/47091/4/chipset_enable.c File chipset_enable.c:

https://review.coreboot.org/c/flashrom/+/47091/4/chipset_enable.c@1898 PS4, Line 1898: Geminilake Gemini Lake

(see Apollo Lake spelling below)

-- To view, visit https://review.coreboot.org/c/flashrom/+/47091 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I533a426bbf8e9c5efef0cf693693086e8efc1f57 Gerrit-Change-Number: 47091 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: Stefan Reinauer reinauer@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sat, 14 Nov 2020 08:02:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Edward O'Callaghan (Code Review)

11:21 a.m.

New subject: Change in flashrom[master]: chipset_enable.c: Add Geminilake support

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47091 )

Change subject: chipset_enable.c: Add Geminilake support ......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/flashrom/+/47091/4/chipset_enable.c File chipset_enable.c:

https://review.coreboot.org/c/flashrom/+/47091/4/chipset_enable.c@1898 PS4, Line 1898: Geminilake

...

Gemini Lake […]

This file is badly ordered as well. We should make a follow up patch to re-order by device-id

-- To view, visit https://review.coreboot.org/c/flashrom/+/47091 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I533a426bbf8e9c5efef0cf693693086e8efc1f57 Gerrit-Change-Number: 47091 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: Stefan Reinauer reinauer@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sat, 14 Nov 2020 10:21:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: comment

Angel Pons (Code Review)

1:58 p.m.

New subject: Change in flashrom[master]: chipset_enable.c: Add Geminilake support

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47091 )

Change subject: chipset_enable.c: Add Geminilake support ......................................................................

Patch Set 4:

...

Patch Set 4:

...
Patch Set 4:

Uh, Gemini Lake has a different descriptor layout. This change as-is will make flashrom assume an invalid descriptor (that for WildcatPoint). While the APL layout is good enough, there's a few differences to account for.

I've got CB:43349 which handles this, but I went berserk and made a full descriptor dumper out of it...

So while everything you said is true the support here seems sufficiently good enough as I am taking it verbatim from the ChromiumOS Flashrom that is used in production. Not that I am saying that's a gold standard by any means! Just that it's better than not working at all.

I am not sure what is the best way forwards here; have the basic support just merged and iterate on that or maybe move forwards you series? In my view the former seems like a pragmatic path however, either way, we should get it across the line so ich is completely in-sync with CrOS and so we have Intel directly sending these sorts of patches to Flashrom moving forwards rather than adhoc fixes.

If you can at least adapt `guess_ich_chipset_from_content` to assume Apollo Lake compatibility, it would be good enough:

https://review.coreboot.org/c/flashrom/+/43349/4/ich_descriptors.c#1321

-- To view, visit https://review.coreboot.org/c/flashrom/+/47091 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I533a426bbf8e9c5efef0cf693693086e8efc1f57 Gerrit-Change-Number: 47091 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: Stefan Reinauer reinauer@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sat, 14 Nov 2020 12:58:05 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Angel Pons (Code Review)

13 May 13 May

4:22 p.m.

New subject: Change in flashrom[master]: chipset_enable.c: Add Geminilake support

Attention is currently required from: Edward O'Callaghan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47091 )

Change subject: chipset_enable.c: Add Geminilake support ......................................................................

Patch Set 4:

(1 comment)

Patchset:

PS4:

...

Patch Set 4:

...
Patch Set 4:

...
Patch Set 4:

Uh, Gemini Lake has a different descriptor layout. This change as-is will make flashrom assume an invalid descriptor (that for WildcatPoint). While the APL layout is good enough, there's a few differences to account for.

I've got CB:43349 which handles this, but I went berserk and made a full descriptor dumper out of it...

So while everything you said is true the support here seems sufficiently good enough as I am taking it verbatim from the ChromiumOS Flashrom that is used in production. Not that I am saying that's a gold standard by any means! Just that it's better than not working at all.

I am not sure what is the best way forwards here; have the basic support just merged and iterate on that or maybe move forwards you series? In my view the former seems like a pragmatic path however, either way, we should get it across the line so ich is completely in-sync with CrOS and so we have Intel directly sending these sorts of patches to Flashrom moving forwards rather than adhoc fixes.

If you can at least adapt `guess_ich_chipset_from_content` to assume Apollo Lake compatibility, it would be good enough:

https://review.coreboot.org/c/flashrom/+/43349/4/ich_descriptors.c#1321

I made another patch that doesn't include full descriptor dumping code: CB:54276

-- To view, visit https://review.coreboot.org/c/flashrom/+/47091 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I533a426bbf8e9c5efef0cf693693086e8efc1f57 Gerrit-Change-Number: 47091 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: Stefan Reinauer reinauer@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-Attention: Edward O'Callaghan quasisec@chromium.org Gerrit-Comment-Date: Thu, 13 May 2021 14:22:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Edward O'Callaghan (Code Review)

14 May 14 May

2:42 p.m.

New subject: Change in flashrom[master]: chipset_enable.c: Add Geminilake support

Edward O'Callaghan has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/47091 )

Change subject: chipset_enable.c: Add Geminilake support ......................................................................

Abandoned

Angel's work much higher quality.

-- To view, visit https://review.coreboot.org/c/flashrom/+/47091 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I533a426bbf8e9c5efef0cf693693086e8efc1f57 Gerrit-Change-Number: 47091 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan quasisec@chromium.org Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Arthur Heymans arthur@aheymans.xyz Gerrit-Reviewer: Sam McNally sammc@google.com Gerrit-Reviewer: Stefan Reinauer reinauer@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@mailbox.org Gerrit-MessageType: abandon

1072

days inactive

1265

days old

flashrom-gerrit@flashrom.org

10 comments

4 participants

tags (0)

participants (4)

Angel Pons (Code Review)
Arthur Heymans (Code Review)
Edward O'Callaghan (Code Review)
Paul Menzel (Code Review)