Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45939 )
Change subject: fw_config: Convert fw_config to a 64-bit field ......................................................................
Patch Set 9:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45939/9/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/45939/9/src/ec/google/chromeec/ec.c... PS9, Line 853: if (cbi_get_uint32(&ssfc, CBI_TAG_SSFC))
I think the whole SSFC is still under review to evaluate whether it is really required. […]
SGTM.
https://review.coreboot.org/c/coreboot/+/45939/2/util/sconfig/main.c File util/sconfig/main.c:
https://review.coreboot.org/c/coreboot/+/45939/2/util/sconfig/main.c@406 PS2, Line 406: /* Check that field is within 64 bits. */
But FW_CONFIG doesn't have to come from the chrome EC, it can also be sourced from a file in CBFS (F […]
Ack
https://review.coreboot.org/c/coreboot/+/45939/2/util/sconfig/main.c@554 PS2, Line 554: (uint64_t)(option->value << (uint64_t)field->start_bit)
You raise a good point, I think actually perhaps option->value should be a 64-bit number, but I'm no […]
Done
https://review.coreboot.org/c/coreboot/+/45939/9/util/sconfig/main.c File util/sconfig/main.c:
https://review.coreboot.org/c/coreboot/+/45939/9/util/sconfig/main.c@456 PS9, Line 456: uint64_t value
The way SSFC support is being added in this CL expects that the devicetree representation takes care of transforming the SSFC fields into a 64-bit field when defining the fw_config field and options. We had some discussion (captured in b/169182605) and the conclusion was that we should allow mainboard devicetree to represent fw_config and fw_source separately and have sconfig handle the transformation from 2 32-bit fields into a single 64-bit field. This would allow the devicetree field/options to match what is really configured in CBI without having to handle that manually.
Upgrading FW_CONFIG in general to 64-bit support means that when FW_CONFIG is supplied from CBFS, devtree won't limit you to dealing with a 32-bit field 😊 so regardless of fw_source vs. SSFC-as-upper-32-bits, I think the 64-bit support here is needed for that reason anyway
Given that the fate of SSFC is still undetermined, I think we can continue with what you have. But, if we ever decide to support SSFC as a separate field, we might have to support in sconfig to handle fw_source as well.
Sure, that more or less becomes a clone of the fw_config infrastructure.