Attention is currently required from: Michał Żygowski, Nico Huber, Paul Menzel.
Maciej Pijanowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80608?usp=email )
Change subject: soc/intel/common/block/fast_spi: probe for 2nd flash component ......................................................................
Patch Set 5:
(3 comments)
Patchset:
PS4:
Code change looks ok. But I don't know why the code uses SFDP in general...
Thanks for the review, please let me know if you expect some action from me here.
File src/soc/intel/common/block/fast_spi/fast_spi_def.h:
https://review.coreboot.org/c/coreboot/+/80608/comment/3fdae2d0_73ac260a : PS4, Line 156: #define SPIBAR_PTINX_IDX_MASK 0xffc
Why drop it?
Because it is not used anymore. The function used to be limited to reading SPDF parameter. Now the function is more generic, allowing for other actions, depending on the HORD bits. This mask was too limiting, allowing for bits 11:2 only to be set. Instead, we construct the PTINX register value using the existing (and a few new) defines. We do not want PTINX index only (bits 11:2), we need the whole (15:2) to cover the rest of the fields. Do you suggest we should define a mask covering this and use it inside a function? It should be then renamed accordingly to reflect that.
File src/soc/intel/common/block/fast_spi/fast_spi_flash.c:
https://review.coreboot.org/c/coreboot/+/80608/comment/b8f76b5b_29e684ab : PS4, Line 308: * SFDP table. FLCOMP.C0DEN is no longer used by the Flash Controller.
This seems a little odd, the code was added in 2017. SPI guides up to […]
I cannot possibly comment on this code as a whole, I just needed to get rid of the error and support a scenario with two flashes. Please let me know if you expect some action from me here.