Mimoja has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Icelake U to known systems ......................................................................
chipset_enable.c: Add Icelake U to known systems
Intel Icelake Systems use an (so far) underdescribed 495 Series Chipset that behaves compaitble to pch300 chips.
Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15 Signed-off-by: Johanna Schander git@mimoja.de --- M chipset_enable.c 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/37987/1
diff --git a/chipset_enable.c b/chipset_enable.c index e826d90..988720b 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -2046,6 +2046,7 @@ {0x8086, 0xa30c, B_S, NT, "Intel", "QM370", enable_flash_pch300}, {0x8086, 0xa30d, B_S, NT, "Intel", "HM370", enable_flash_pch300}, {0x8086, 0xa30e, B_S, DEP, "Intel", "CM246", enable_flash_pch300}, + {0x8086, 0x3482, B_S, DEP, "Intel", "Icelake U Premium", enable_flash_pch300}, #endif {0}, };
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Icelake U to known systems ......................................................................
Patch Set 1: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Icelake U to known systems ......................................................................
Patch Set 1: Code-Review+2
Would be nice to add any other IceLake U PCI IDs as NT, for the sake of completeness.
Mimoja has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Icelake U to known systems ......................................................................
Patch Set 1:
Patch Set 1: Code-Review+2
Would be nice to add any other IceLake U PCI IDs as NT, for the sake of completeness.
Is adding untested hardware really the best option here?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Icelake U to known systems ......................................................................
Patch Set 1: Code-Review+1
(4 comments)
Tested on?
https://review.coreboot.org/c/flashrom/+/37987/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/37987/1//COMMIT_MSG@7 PS1, Line 7: Icelake Ice Lake
https://review.coreboot.org/c/flashrom/+/37987/1//COMMIT_MSG@9 PS1, Line 9: Icelake Ditto.
https://review.coreboot.org/c/flashrom/+/37987/1//COMMIT_MSG@10 PS1, Line 10: compaitble compatible
https://review.coreboot.org/c/flashrom/+/37987/1/chipset_enable.c File chipset_enable.c:
https://review.coreboot.org/c/flashrom/+/37987/1/chipset_enable.c@2049 PS1, Line 2049: Icelake Ice Lake
Hello Angel Pons, Paul Menzel, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/37987
to look at the new patch set (#2).
Change subject: chipset_enable.c: Add Ice Lake U to known systems ......................................................................
chipset_enable.c: Add Ice Lake U to known systems
Intel Ice Lake Systems use an (so far) underdescribed 495 Series Chipset that behaves compatible to pch300 chips.
This was tested on the late 2019 Razer Blade Stealth with Core i7 1065G7 CPU and the coresponding Ice Lake LP U PCH.
Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15 Signed-off-by: Johanna Schander git@mimoja.de --- M chipset_enable.c 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/37987/2
Mimoja has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Ice Lake U to known systems ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/flashrom/+/37987/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/37987/1//COMMIT_MSG@7 PS1, Line 7: Icelake
Ice Lake
Done
https://review.coreboot.org/c/flashrom/+/37987/1//COMMIT_MSG@9 PS1, Line 9: Icelake
Ditto.
Done
https://review.coreboot.org/c/flashrom/+/37987/1//COMMIT_MSG@10 PS1, Line 10: compaitble
compatible
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Ice Lake U to known systems ......................................................................
Patch Set 2:
Hi, Philipp mentioned that you had to tell flashrom that your device is not a laptop? If that is still the case, can you provide a verbose log please. I've reviewed the respective chipset_enable code against the ICL datasheet but couldn't find any issue.
Hello Angel Pons, Paul Menzel, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/37987
to look at the new patch set (#3).
Change subject: chipset_enable.c: Add Ice Lake U to known systems ......................................................................
chipset_enable.c: Add Ice Lake U to known systems
Intel Ice Lake Systems use an (so far) underdescribed 495 Series Chipset that behaves compaitble to pch300 chips.
Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15 Signed-off-by: Johanna Schander git@mimoja.de --- M chipset_enable.c 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/37987/3
Mimoja has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Ice Lake U to known systems ......................................................................
Patch Set 3:
Patch Set 2:
Hi, Philipp mentioned that you had to tell flashrom that your device is not a laptop? If that is still the case, can you provide a verbose log please. I've reviewed the respective chipset_enable code against the ICL datasheet but couldn't find any issue.
No, we first tried that before adding it as an target. Here is my logging:
mimoja@mimoja-Razer-Blade-Stealth-13-Late-2019:~/flashrom$ sudo ./flashrom -p internal -r icelake_readtest.bin flashrom v1.1-rc1-126-g9a4f1d6 on Linux 5.3.0-24-generic (x86_64) flashrom is free software, get the source code at https://flashrom.org
Using clock_gettime for delay loops (clk_id: 1, resolution: 1ns). No DMI table found. Found chipset "Intel Ice Lake U Premium". Enabling flash write... SPI Configuration is locked down. Enabling hardware sequencing because some important opcode is locked. OK. Found Programmer flash chip "Opaque flash chip" (16384 kB, Programmer-specific) mapped at physical address 0x0000000000000000. Reading flash... done.
Also I renamed all occurences to "Ice Lake" now.
Mimoja has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Ice Lake U to known systems ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/flashrom/+/37987/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/37987/1//COMMIT_MSG@7 PS1, Line 7: Icelake
Done
Done
https://review.coreboot.org/c/flashrom/+/37987/1//COMMIT_MSG@9 PS1, Line 9: Icelake
Done
Done
https://review.coreboot.org/c/flashrom/+/37987/1//COMMIT_MSG@10 PS1, Line 10: compaitble
Done
Done
https://review.coreboot.org/c/flashrom/+/37987/1/chipset_enable.c File chipset_enable.c:
https://review.coreboot.org/c/flashrom/+/37987/1/chipset_enable.c@2049 PS1, Line 2049: Icelake
Ice Lake
Done
Mimoja has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Ice Lake U to known systems ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/flashrom/+/37987/1/chipset_enable.c File chipset_enable.c:
https://review.coreboot.org/c/flashrom/+/37987/1/chipset_enable.c@2049 PS1, Line 2049: Icelake
Done
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Ice Lake U to known systems ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/flashrom/+/37987/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/37987/3//COMMIT_MSG@7 PS3, Line 7: known systems That's not entirely true: It is adding ICL-U as known *and* tested. Has it been tested (read/write/erase)?
https://review.coreboot.org/c/flashrom/+/37987/3//COMMIT_MSG@9 PS3, Line 9: underdescribed `undocumented` sounds better
https://review.coreboot.org/c/flashrom/+/37987/3//COMMIT_MSG@9 PS3, Line 9: Systems `systems` in lowercase
https://review.coreboot.org/c/flashrom/+/37987/3//COMMIT_MSG@9 PS3, Line 9: an (so far) this looks rather weird. Maybe use 'a currently' instead?
https://review.coreboot.org/c/flashrom/+/37987/3//COMMIT_MSG@10 PS3, Line 10: compaitble Not fixed?
Mimoja has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Ice Lake U to known systems ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/flashrom/+/37987/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/flashrom/+/37987/3//COMMIT_MSG@7 PS3, Line 7: known systems
That's not entirely true: It is adding ICL-U as known *and* tested. […]
Done
https://review.coreboot.org/c/flashrom/+/37987/3//COMMIT_MSG@9 PS3, Line 9: underdescribed
`undocumented` sounds better
Done
https://review.coreboot.org/c/flashrom/+/37987/3//COMMIT_MSG@9 PS3, Line 9: Systems
`systems` in lowercase
Done
https://review.coreboot.org/c/flashrom/+/37987/3//COMMIT_MSG@9 PS3, Line 9: an (so far)
this looks rather weird. […]
Done
https://review.coreboot.org/c/flashrom/+/37987/3//COMMIT_MSG@10 PS3, Line 10: compaitble
Not fixed?
Done
Hello Angel Pons, Paul Menzel, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/37987
to look at the new patch set (#4).
Change subject: chipset_enable.c: Add Ice Lake U to known and tested systems ......................................................................
chipset_enable.c: Add Ice Lake U to known and tested systems
Intel Ice Lake systems use an 495 Series Chipset that behaves compatible to pch300 chips but chip names are undocumented at this point.
This change was tested in read/write/erase on the Razer Blade Stealth (late 2019) with intel 1065G7 CPU and "Ice Lake U Premium PCH".
Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15 Signed-off-by: Johanna Schander git@mimoja.de --- M chipset_enable.c 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/37987/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Ice Lake U to known and tested systems ......................................................................
Patch Set 4: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Ice Lake U to known and tested systems ......................................................................
Patch Set 4:
Can you upload your IFD dump somewhere? or check if `ich_descriptors_tool` detects the correct chipset? or both :)
Mimoja has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Ice Lake U to known and tested systems ......................................................................
Patch Set 4:
Patch Set 4:
Can you upload your IFD dump somewhere? or check if `ich_descriptors_tool` detects the correct chipset? or both :)
I only have full spi dumps left. Cannot test anything anymore, broke my laptop yesterday :/
Hello Angel Pons, Paul Menzel, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/37987
to look at the new patch set (#5).
Change subject: chipset_enable.c: Add Ice Lake U to known systems ......................................................................
chipset_enable.c: Add Ice Lake U to known systems
Intel Ice Lake Systems use an (so far) underdescribed 495 Series Chipset that behaves compaitble to pch300 chips.
Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15 Signed-off-by: Johanna Schander git@mimoja.de --- M chipset_enable.c 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/37987/5
Hello Angel Pons, Paul Menzel, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/37987
to look at the new patch set (#6).
Change subject: chipset_enable.c: Add Ice Lake U to known and tested systems ......................................................................
chipset_enable.c: Add Ice Lake U to known and tested systems
Intel Ice Lake systems use an 495 Series Chipset that behaves compatible to pch300 chips but chip names are undocumented at this point.
This change was tested in read/write/erase on the Razer Blade Stealth (late 2019) with intel 1065G7 CPU and "Ice Lake U Premium PCH".
Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15 Signed-off-by: Johanna Schander git@mimoja.de --- M chipset_enable.c 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/37987/6
Christoph Pomaska has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Ice Lake U to known and tested systems ......................................................................
Patch Set 6: Code-Review+1
David Hendricks has submitted this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Ice Lake U to known and tested systems ......................................................................
chipset_enable.c: Add Ice Lake U to known and tested systems
Intel Ice Lake systems use an 495 Series Chipset that behaves compatible to pch300 chips but chip names are undocumented at this point.
This change was tested in read/write/erase on the Razer Blade Stealth (late 2019) with intel 1065G7 CPU and "Ice Lake U Premium PCH".
Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15 Signed-off-by: Johanna Schander git@mimoja.de Reviewed-on: https://review.coreboot.org/c/flashrom/+/37987 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Christoph Pomaska github@slrie.de Reviewed-by: Angel Pons th3fanbus@gmail.com --- M chipset_enable.c 1 file changed, 1 insertion(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Christoph Pomaska: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
diff --git a/chipset_enable.c b/chipset_enable.c index 3b44d93..84e4b6b 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -2046,6 +2046,7 @@ {0x8086, 0xa30c, B_S, NT, "Intel", "QM370", enable_flash_pch300}, {0x8086, 0xa30d, B_S, NT, "Intel", "HM370", enable_flash_pch300}, {0x8086, 0xa30e, B_S, DEP, "Intel", "CM246", enable_flash_pch300}, + {0x8086, 0x3482, B_S, DEP, "Intel", "Ice Lake U Premium", enable_flash_pch300}, #endif {0}, };
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Ice Lake U to known and tested systems ......................................................................
Patch Set 8:
(1 comment)
Patchset:
PS8: This DID is not belongs to Icelake SPI controller -> 0x3482, rather this is ice lake LP PCH premium SKU DID.
we should have added 0x34a4 to have support for ice lake in flashrom
Attention is currently required from: Mimoja. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Ice Lake U to known and tested systems ......................................................................
Patch Set 8:
(1 comment)
Patchset:
PS8:
This DID is not belongs to Icelake SPI controller -> 0x3482, rather this is ice lake LP PCH premium […]
No, flashrom uses the LPC device's PCI IDs. The SPI PCI device isn't always visible: I have an HP 280 G2 mainboard (LGA1151 socket with Sunrise Point H110 PCH) where vendor firmware hides the SPI PCI device.
Attention is currently required from: Mimoja. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Ice Lake U to known and tested systems ......................................................................
Patch Set 8:
(1 comment)
Patchset:
PS8:
No, flashrom uses the LPC device's PCI IDs. The SPI PCI device isn't always visible: I have an HP 280 G2 mainboard (LGA1151 socket with Sunrise Point H110 PCH) where vendor firmware hides the SPI PCI device.
With postboot_sai being implemented with CNP PCH, things are little different between how to disable/hide a controller like SPI during boot, won't go into that. I believe that is decision made by some OEM to hide the SPI PCI device.
But if follow that design choice then how feasible is to pass the SPI PCI device (0x1f) and function number (5) as below when SPI device itself is not listed there?
static int enable_flash_pch300(struct pci_dev *const dev, const char *const name) { return enable_flash_pch100_or_c620(dev, name, 0x1f, 5, CHIPSET_300_SERIES_CANNON_POINT); }
Please take a look into DID used in flashrom for TGL, ADL and MTL chipset, those are all SPI DID instead eSPI/PCH DID.
Attention is currently required from: Mimoja. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Ice Lake U to known and tested systems ......................................................................
Patch Set 8:
(1 comment)
Patchset:
PS8:
No, flashrom uses the LPC device's PCI IDs. […]
TGL IDs in flashrom are LPC/eSPI DIDs. Also, see this comment in `enable_flash_pch100_or_c620`:
/* * The SPI PCI device is usually hidden (by hiding PCI vendor * and device IDs). So we need a PCI access method that works * even when the OS doesn't know the PCI device. We can't use * this method globally since it would bring along other con- * straints (e.g. on PCI domains, extended PCIe config space). */
flashrom can run on any system, not just Intel systems. The internal programmer can't assume anything about the system it runs on, so it can't try to blindly access a PCI device that doesn't enumerate normally (who knows what that device could be). That's why flashrom uses the LPC/eSPI device ID to detect the chipset it's running on: the LPC/eSPI device is always visible (AFAIK it can't even be disabled), so it's possible to look at all the enumerated PCI devices to see if there's any VID/DID match. Only when the chipset is known can flashrom try accessing a potentially hidden PCI device (i.e. the SPI PCI device).
Attention is currently required from: Mimoja. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37987 )
Change subject: chipset_enable.c: Add Ice Lake U to known and tested systems ......................................................................
Patch Set 8:
(1 comment)
Patchset:
PS8:
TGL IDs in flashrom are LPC/eSPI DIDs. Also, see this comment in `enable_flash_pch100_or_c620`: […]
It's somewhat all true. The missing link is that Intel stopped telling firmware developers to hide the device. So for newer platforms we can use the ID of the SPI PCI device. It's a bit confusing because on these platforms we now find the SPI PCI device, then look it up again as if it was hidden. A patch to untangle that would be welcome (IIRC there are comments on this in the Elkhart Lake review).
I guess for the newer platforms it's reasonably safe to assume that the device is _not_ hidden. Hence we just use the SPI PCI ID. If ever some new machines turn up where it is hidden, we can still add the LPC/eSPI IDs. But once we have added an LPC ID, we can't safely remove it without risking regressions. So I'd just keep that ID for Ice Lake and add the SPI PCI ID for it too.
-
Angel Pons (Code Review) -
Christoph Pomaska (Code Review)
-
David Hendricks (Code Review)
-
Mimoja (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)
-
Philipp Deppenwiese (Code Review)
-
Subrata Banik (Code Review)