Attention is currently required from: Patrick Rudolph, Sean Rhodes.
Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77882?usp=email )
Change subject: include/commonlib/coreboot_tables: Add CFR structs ......................................................................
Patch Set 9:
(16 comments)
File Documentation/drivers/cfr.md:
https://review.coreboot.org/c/coreboot/+/77882/comment/4ca3736d_062680c5 : PS9, Line 9: S change to lowercase.
https://review.coreboot.org/c/coreboot/+/77882/comment/452accf7_495c72c1 : PS9, Line 18: ## Design Proposal After it's accepted, it's no longer a design proposal, so let's just write it as documentation for the design.
https://review.coreboot.org/c/coreboot/+/77882/comment/10c954db_1bd635d8 : PS9, Line 21: The coreboot table is generated from coreboot ramstage : code. : This sentence leads to questions that you might want to address here. - Is the data only available in ramstage? (I doubt it, but I wouldn't think that works well for many of the values that want to be stored.)
https://review.coreboot.org/c/coreboot/+/77882/comment/48d20d4f_f55af1df : PS9, Line 24: name Can we use an enum instead?
https://review.coreboot.org/c/coreboot/+/77882/comment/6f5b6593_973fd4eb : PS9, Line 24: it's its
https://review.coreboot.org/c/coreboot/+/77882/comment/302f9944_bc908027 : PS9, Line 25: tatus flags. Where is the current value stored? I assume that the minimum we need to use this are the name, flags, and current value.
https://review.coreboot.org/c/coreboot/+/77882/comment/f31b2bf0_77c36916 : PS9, Line 24: user : visible name, a help text, Maybe these could be stored separately? They are only needed for display, not for use.
https://review.coreboot.org/c/coreboot/+/77882/comment/2e1d8d41_ad296d15 : PS9, Line 25: default value Is there any thought of the default value needing to be calculated? If we change memory speeds, for example, we'd want to change the default value for timing.
https://review.coreboot.org/c/coreboot/+/77882/comment/06b90597_5843164c : PS9, Line 29: ore or
https://review.coreboot.org/c/coreboot/+/77882/comment/e3d85a94_0b648a48 : PS9, Line 30: and are never reachable should be marked as hidden Why not remove them?
https://review.coreboot.org/c/coreboot/+/77882/comment/8c803332_a5c63cb3 : PS9, Line 36: number can we differentiate between hex & decimal? Maybe with a flag?
https://review.coreboot.org/c/coreboot/+/77882/comment/5108f2f7_5facbf1a : PS9, Line 44: form generate?
https://review.coreboot.org/c/coreboot/+/77882/comment/f54b065b_0dc89776 : PS9, Line 69: immideatly immediately
File src/commonlib/include/commonlib/cfr.h:
https://review.coreboot.org/c/coreboot/+/77882/comment/64de19c1_7dcda42c : PS9, Line 102: I think we need a version number of the structures to be stored along with the structures somewhere. That should at least allow for backward compatibility when the structures change.
This is not needed as much for a payload, though I can see it catching issues there as well, but for an OS-based tool, I'd think it would be critical.
https://review.coreboot.org/c/coreboot/+/77882/comment/fe650dd1_375ea9bc : PS9, Line 128: Uniqueue Unique?
https://review.coreboot.org/c/coreboot/+/77882/comment/53d4835f_6d2fed90 : PS9, Line 145: Uniqueue Unique?