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 12:
(4 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/81357/comment/e7205458_afc84bff : PS12, Line 26: TEST=On MTL, use flashrom -VV to see correct FREG9 access : TEST=On ADL, use flashrom -VV to see not break anything : TEST=On APL, use flashrom -VV to see not break anything Would you mind running the test scenarious again on the latest version of code? Thank you!
Patchset:
PS12: Subrata, just wanted to check, do you have interest to review the latest version of the patch? I know you reviewed, but maybe you want to have the last look with all the changes made.
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/81357/comment/af31708c_ee193798 : PS10, Line 1853: mmio_readw
You could see the document of Intel Core Ultra: […]
Thanks as usual for giving all details! appreciate it.
I was staring on the code for some time, but it looks like it should work. I was mostly looking at conversion to smaller size
*region_read_access = (uint16_t)ICH_BRRA(tmp);
*region_write_access = (uint16_t)ICH_BRWA(tmp);
while previously `ICH_BRRA(tmp)` and `ICH_BRWA(tmp)` were used as is, without conversion to smaller size. But, as you pointed in docs, only 2 bytes are needed...
https://review.coreboot.org/c/flashrom/+/81357/comment/998aaaf2_094d074e : PS10, Line 1876: inline
Done […]
I am thinking from the other side: are there any reasons we should explicitly say it needs to be inline, and if there are no reasons, better leave this to compiler. With just one callsite, I think the function will be inlined anyway.