Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39232 )
Change subject: soc/intel/tigerlake: Enable Hybride storage mode ......................................................................
Patch Set 6:
(7 comments)
Patch Set 6:
(4 comments)
Why is this done by the FSP and not in coreboot?
FSP sets up the PCIe lanes already, so it just got added there I guess.
https://review.coreboot.org/c/coreboot/+/39232/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39232/6//COMMIT_MSG@7 PS6, Line 7: Hybride remove the `e` at the end: Hybrid
https://review.coreboot.org/c/coreboot/+/39232/6//COMMIT_MSG@9 PS6, Line 9: For using To use
https://review.coreboot.org/c/coreboot/+/39232/6//COMMIT_MSG@9 PS6, Line 9: storage(optane)
Please add a space before the (.
I think `Optane` should be capitalized as well
https://review.coreboot.org/c/coreboot/+/39232/6//COMMIT_MSG@10 PS6, Line 10: And this can be : configured by fit configuration which is built time. There are lots of words related to `configure` close together. How about rewriting this sentence as:
The mode can be selected using the FIT tool at build time.
https://review.coreboot.org/c/coreboot/+/39232/6//COMMIT_MSG@12 PS6, Line 12: FSP
What FSP component is that?
FSP-S
https://review.coreboot.org/c/coreboot/+/39232/6//COMMIT_MSG@13 PS6, Line 13: and save configuration : for next boot up. Where does it get saved to? Also, to avoid repeating `configure`, I would use:
... and save the settings for the next boot.
https://review.coreboot.org/c/coreboot/+/39232/6/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/39232/6/src/soc/intel/tigerlake/chi... PS6, Line 266: /* Hybrid storage mode enable(1)/disable(0) */
Please add a space before the (.
It would be inconsistent with the comments above, though...
I would mention that this is about Optane/NVMe