Change in flashrom[master]: chipset_enable.c: Add Icelake U to known systems

[Mimoja (Code Review)]

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 <[e-mail]>
---
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},
 };

-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 1
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-MessageType: newchange

[Philipp Deppenwiese (Code Review)]

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


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 1
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Comment-Date: Sun, 29 Dec 2019 14:22:25 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Angel Pons (Code Review)]

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.


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 1
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Sat, 04 Jan 2020 14:54:23 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Mimoja (Code Review)]

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?


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 1
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Sun, 05 Jan 2020 01:25:05 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Paul Menzel (Code Review)]

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



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 1
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Sun, 05 Jan 2020 10:48:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known systems

[Mimoja (Code Review)]

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 <[e-mail]>
---
M chipset_enable.c
1 file changed, 1 insertion(+), 0 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/37987/2
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 2
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-MessageType: newpatchset

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known systems

[Mimoja (Code Review)]

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

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known systems

[Nico Huber (Code Review)]

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.


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 2
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-Comment-Date: Sun, 05 Jan 2020 22:15:07 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known systems

[Mimoja (Code Review)]

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 <[e-mail]>
---
M chipset_enable.c
1 file changed, 1 insertion(+), 0 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/37987/3
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 3
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-MessageType: newpatchset

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known systems

[Mimoja (Code Review)]

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.


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 3
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-Comment-Date: Mon, 06 Jan 2020 15:57:38 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known systems

[Mimoja (Code Review)]

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

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known systems

[Mimoja (Code Review)]

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

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known systems

[Angel Pons (Code Review)]

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?



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 3
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-Comment-Date: Thu, 09 Jan 2020 21:22:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known systems

[Mimoja (Code Review)]

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

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known and tested systems

[Mimoja (Code Review)]

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 <[e-mail]>
---
M chipset_enable.c
1 file changed, 1 insertion(+), 0 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/37987/4
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 4
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-MessageType: newpatchset

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known and tested systems

[Angel Pons (Code Review)]

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


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 4
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-Comment-Date: Sun, 12 Jan 2020 09:59:03 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known and tested systems

[Nico Huber (Code Review)]

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 :)


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 4
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-Comment-Date: Sun, 12 Jan 2020 14:07:10 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known and tested systems

[Mimoja (Code Review)]

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 :/


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 4
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-Comment-Date: Mon, 13 Jan 2020 14:26:46 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known systems

[Mimoja (Code Review)]

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 <[e-mail]>
---
M chipset_enable.c
1 file changed, 1 insertion(+), 0 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/37987/5
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 5
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-MessageType: newpatchset

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known and tested systems

[Mimoja (Code Review)]

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 <[e-mail]>
---
M chipset_enable.c
1 file changed, 1 insertion(+), 0 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/37987/6
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 6
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-MessageType: newpatchset

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known and tested systems

[Christoph Pomaska (Code Review)]

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


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 6
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Christoph Pomaska <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-Comment-Date: Fri, 07 Feb 2020 21:00:45 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known and tested systems

[David Hendricks (Code Review)]

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 <[e-mail]>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/37987
Tested-by: build bot (Jenkins) <[e-mail]>
Reviewed-by: Christoph Pomaska <[e-mail]>
Reviewed-by: Angel Pons <[e-mail]>
---
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},
 };

-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 8
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Christoph Pomaska <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-MessageType: merged

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known and tested systems

[Subrata Banik (Code Review)]

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



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 8
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Christoph Pomaska <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Subrata Banik <[e-mail]>
Gerrit-Comment-Date: Tue, 12 Apr 2022 11:01:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known and tested systems

[Angel Pons (Code Review)]

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.

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known and tested systems

[Subrata Banik (Code Review)]

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.



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 8
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Christoph Pomaska <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Subrata Banik <[e-mail]>
Gerrit-Attention: Mimoja <[e-mail]>
Gerrit-Comment-Date: Wed, 13 Apr 2022 05:51:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <[e-mail]>
Comment-In-Reply-To: Angel Pons <[e-mail]>
Gerrit-MessageType: comment

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known and tested systems

[Angel Pons (Code Review)]

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).



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 8
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Christoph Pomaska <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Subrata Banik <[e-mail]>
Gerrit-Attention: Mimoja <[e-mail]>
Gerrit-Comment-Date: Wed, 13 Apr 2022 17:37:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <[e-mail]>
Comment-In-Reply-To: Angel Pons <[e-mail]>
Gerrit-MessageType: comment

Change in flashrom[master]: chipset_enable.c: Add Ice Lake U to known and tested systems

[Nico Huber (Code Review)]

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.



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/37987
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6227d32f4476420cf1aeec37ebd4b7648e0b3d15
Gerrit-Change-Number: 37987
Gerrit-PatchSet: 8
Gerrit-Owner: Mimoja <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Christoph Pomaska <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Mimoja <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Philipp Deppenwiese <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Subrata Banik <[e-mail]>
Gerrit-Attention: Mimoja <[e-mail]>
Gerrit-Comment-Date: Wed, 13 Apr 2022 17:59:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <[e-mail]>
Comment-In-Reply-To: Angel Pons <[e-mail]>
Gerrit-MessageType: comment