Attention is currently required from: Sugnan Prabhu S, Tim Wawrzynczak, Paul Menzel. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56750 )
Change subject: vc/google/chromeos: Add support for new SAR tables revisions ......................................................................
Patch Set 17:
(4 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56750/comment/0df600d7_b30ec1b8 PS17, Line 7: vc/google/chromeos This is touching more than just vc/google/chromeos.
File src/include/sar.h:
https://review.coreboot.org/c/coreboot/+/56750/comment/b65d1209_abf1b06f PS17, Line 21: struct wifi_sar_delta_table { : uint8_t version; : union { : struct { : uint8_t power_max_2400mhz; : uint8_t power_chain_a_2400mhz; : uint8_t power_chain_b_2400mhz; : uint8_t power_max_5200mhz; : uint8_t power_chain_a_5200mhz; : uint8_t power_chain_b_5200mhz; : } __packed group_rev0[SAR_NUM_WGDS_GROUPS]; : struct { : uint8_t power_max_2400mhz; : uint8_t power_chain_a_2400mhz; : uint8_t power_chain_b_2400mhz; : uint8_t power_max_5200mhz; : uint8_t power_chain_a_5200mhz; : uint8_t power_chain_b_5200mhz; : uint8_t power_max_6000mhz; : uint8_t power_chain_a_6000mhz; : uint8_t power_chain_b_6000mhz; : } __packed group_rev1[SAR_NUM_WGDS_GROUPS]; : } __packed; : } __packed; I think we should hide the differences of different revisions on the SAR file generation side so that there are minimal changes in coreboot (especially the ACPI generation code) to support new revisions. I am thinking something like this:
``` struct sar_profile { uint8_t revision; uint8_t dsar_set_count; uint8_t chains_count; uint8_t subbands_count; uint8_t sar_table[0]; /* This will hold (dsar_set_count + 1) * chains_count * subbands_count bytes */ } __packed;
struct geo_profile { uint8_t revision; uint8_t chains_count; uint8_t bands_count; uint8_t wgds_table[0]; /* This will hold chains_count * bands_count bytes */ } __packed; ```
So, the SAR file generation will emit out:
``` sar_profile { .revision -> 0 .dsar_set_count -> 2 .chains_count -> 2 .subbands_count -> 5 .sar_table -> { set0 -> { .... }, set1 -> { .... }, set2 -> { .... } } } ```
`acpi.c` can then use this information to generate the required nodes:
```
static uint8_t *sar_fetch_set(const struct sar_profile *sar, size_t set_num) { uint8_t *sar_table = &sar->sar_table[0]; sar_table += sar->chains_count * sar->subbands_count * set;
return sar_table; }
static void sar_emit_wrds(const struct sar_profile *sar) { size_t package_size, i; uint8_t *set;
acpigen_write_name("WRDS"); acpigen_write_package(2); acpigen_write_dword(sar->revision);
table_size = sar->chains_count * sar->subbands_count;
/* Domain type + SAR enable + chains_count * subbands_count */ package_size = 1 + 1 + table_size;
acpigen_write_package(package_size); acpigen_write_dword(WRDS_DOMAIN_TYPE_WIFI); acpigen_write_dword(1);
set = sar_fetch_set(sar, 0); for (i = 0; i < table_size; i++) { acpigen_write_byte(set[i]); } }
static void sar_emit_ewrd(const struct sar_profile *sar) { size_t package_size, i; uint8_t *set;
acpigen_write_name("EWRD"); acpigen_write_package(2); acpigen_write_dword(sar->revision);
table_size = sar->chains_count * sar->subbands_count;
/* Domain type + DSAR enable + DSAR sets + chains_count * subbands_count * set_count */ package_size = 1 + 1 + 1 + table_size * sar->dsar_set_count;
acpigen_write_package(package_size); acpigen_write_dword(WRDS_DOMAIN_TYPE_WIFI); acpigen_write_dword(sar->dsar_set_count ? 1 : 0); acpigen_write_dword(sar->dsar_set_count);
for (set_num = 1; set_num <= sar->dsar_set_count; i++) { set = sar_fetch_set(sar, set_num); for (i = 0; i < table_size; i++) { acpigen_write_byte(set[i]); } } } ```
Similarly other tables can be handled. What do you think?
https://review.coreboot.org/c/coreboot/+/56750/comment/f77551cc_9f845ad1 PS17, Line 51: sar_limit I think you meant sar_limit[0] to indicate that this is a variable-length array. At least, that is how you are using it in acpi.c
File src/vendorcode/google/chromeos/sar.c:
https://review.coreboot.org/c/coreboot/+/56750/comment/ee7c8bf0_b908e32e PS17, Line 138: if (CONFIG(USE_SAR)) { : sar_limits->sar_table->version = 0; : sar_limits->sar_table->sar_enable = CONFIG(SAR_ENABLE); : sar_limits->sar_table->dsar_enable = CONFIG(DSAR_ENABLE); : sar_limits->sar_table->dsar_set_num = CONFIG_DSAR_SET_NUM; : } I like the fact that you are trying to fix the problems with the original representation of SAR tables in CBFS. It assumed that the revision will always be 0. And now we need to support the newer revisions as well. So, encoding the new revisions while massaging the older format to convert it into appropriate format here seems like the right thing to do.
One comment I have is that we should not really add another Kconfig `USE_SAR_V2` and instead rely on the CBFS data to make the decision. This is because the way you are organizing CBFS format eliminates the need for Kconfig-based version tracking (in fact it will be really confusing for partners to determine the correct version and keep those in sync in coreboot and the CBFS table). The only reason why you need `USE_SAR_V2` is to identify if that extra processing is required on the CBFS SAR table.
This is not the cleanest of approaches, but my recommendation is: 1. Read SAR table from CBFS. 2. Compare length to expected length in old format. If it matches, then assume old format. 3. Else assume that the format encodes revision information correctly.
If you really want to keep it foolproof and not rely on length, you can add a string "$SAR" at the start of the CBFS file to identify the new format. But, I think that is not really necessary.
With this, you can eliminate the need for `USE_SAR_V2` and all platforms are expected to only select `USE_SAR`. One thing that you will have to co-ordinate is the SAR table generation code to ensure that all *new* platforms move to this new format even if they are not using any of the new revisions i.e. even if a platform plans to use revision 0, it should still encode in the new format. This ensures consistency going forward. However, *older* platforms that have already shipped should continue using the old format. Thus, you don't have to go back and fix the firmware branches for each.