[M] Change in flashrom[main]: ichspi.c: Add support for region 9 and beyond in Meteor Lake

[Anastasia Klimchuk (Code Review)]

Attention is currently required from: Hsuan Ting Chen, Hsuan-ting Chen, Subrata Banik.

Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81357?usp=email )

Change subject: ichspi.c: Add support for region 9 and beyond in Meteor Lake
......................................................................


Patch Set 10:

(11 comments)

Commit Message:

https://review.coreboot.org/c/flashrom/+/81357/comment/e96b43c1_539e5275 :
PS10, Line 28: 
Is MTL a Meteor Lake and ADL Alder Point?

Is it possible to test the chipsets that used to be after Meteor in the enum, and now they are before?


File ichspi.c:

https://review.coreboot.org/c/flashrom/+/81357/comment/f25256b6_1456dcf5 :
PS10, Line 1853: mmio_readw
Why it is `readw` (reads into uint16_t) here, but `readl` below (reads into uint32_t)?
You declare `region_read_access` and `region_write_access` as uint32_t actually


https://review.coreboot.org/c/flashrom/+/81357/comment/e789d35a_3e357092 :
PS10, Line 1854: 		*region_write_access = mmio_readw(ich_spibar + ICH_REG_BIOS_BM_WAP);
Maybe you can also add some debugging prints in this branch too


https://review.coreboot.org/c/flashrom/+/81357/comment/47f1115f_18bd15d7 :
PS10, Line 1876: inline
I don't think this needs to be inline?


https://review.coreboot.org/c/flashrom/+/81357/comment/8a8684c3_be89553c :
PS10, Line 1877: if (ich_generation >= CHIPSET_METEOR_LAKE)
               : 		return 16;
               : 	return 8;
maybe make it one line
`return (ich_generation >= CHIPSET_METEOR_LAKE) ? 16 : 8;`


https://review.coreboot.org/c/flashrom/+/81357/comment/2b4ac08b_fdc32cc8 :
PS10, Line 2109: case CHIPSET_METEOR_LAKE:
Let's re-order Meteor Lake here too


https://review.coreboot.org/c/flashrom/+/81357/comment/735165af_ce23a858 :
PS10, Line 2149: case CHIPSET_METEOR_LAKE:
and here


https://review.coreboot.org/c/flashrom/+/81357/comment/119727ec_9cb0c1f7 :
PS10, Line 2211: case CHIPSET_METEOR_LAKE:
and here


https://review.coreboot.org/c/flashrom/+/81357/comment/7ed5d9c0_fe2647b9 :
PS10, Line 2229:  
you can leave the word "registers", so that it is "... and region access registers"


https://review.coreboot.org/c/flashrom/+/81357/comment/923bd8a6_b0f2d5b8 :
PS10, Line 2291: case CHIPSET_METEOR_LAKE:
and here


https://review.coreboot.org/c/flashrom/+/81357/comment/e5a66a3d_d85dc523 :
PS10, Line 2331: case CHIPSET_METEOR_LAKE:
and here



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/81357?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I1e06e7b3d470423a6014e623826d9234fdebfbf9
Gerrit-Change-Number: 81357
Gerrit-PatchSet: 10
Gerrit-Owner: Hsuan-ting Chen <[e-mail]>
Gerrit-Reviewer: Anastasia Klimchuk <[e-mail]>
Gerrit-Reviewer: Hsuan Ting Chen <[e-mail]>
Gerrit-Reviewer: Subrata Banik <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Jamie Ryu <[e-mail]>
Gerrit-CC: Tyler Wang <[e-mail]>
Gerrit-Attention: Hsuan Ting Chen <[e-mail]>
Gerrit-Attention: Subrata Banik <[e-mail]>
Gerrit-Attention: Hsuan-ting Chen <[e-mail]>
Gerrit-Comment-Date: Sun, 28 Apr 2024 08:39:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment