Attention is currently required from: Furquan Shaikh, Sugnan Prabhu S, Paul Menzel. Tim Wawrzynczak 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 12:
(10 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56750/comment/e89d46cf_d499036a PS12, Line 14: wdgs WGDS
https://review.coreboot.org/c/coreboot/+/56750/comment/b30de7c7_e4e8289c PS12, Line 15: wdgs WGDS
https://review.coreboot.org/c/coreboot/+/56750/comment/137e78f2_613d8b84 PS12, Line 22: TEST=Checked the SSDT entries for WRDS, EWRD, WGDS and PPAG with : different binaries generated by setting different versions in the : config.star Does this mean you verified revisions 0, 1 and 2 are all generated and decoded correctly?
File src/drivers/wifi/generic/acpi.c:
https://review.coreboot.org/c/coreboot/+/56750/comment/42623c6a_fef7a746 PS12, Line 55: if (sar_table->version > 2) { : printk(BIOS_ERR, "Invalid SAR table version: %x02\n", sar_table->version); : return; : } suggestion: move this to an `else` clause after line 75, e.g.:
``` } else if (sar_table->version == 2) { sar_limit_len = BYTES_PER_SAR_LIMIT_REV2; } else { printk(BIOS_ERR, "Unsupported SAR table version: %x02\n", sar_table->version); return; } ```
https://review.coreboot.org/c/coreboot/+/56750/comment/d1881577_0e752baa PS12, Line 141: wgds WGDS
File src/include/sar.h:
https://review.coreboot.org/c/coreboot/+/56750/comment/3c4fa5f4_532ead8e PS12, Line 7: #define NUM_SAR_LIMITS 4 : #define BYTES_PER_SAR_LIMIT 10 : #define BYTES_PER_SAR_LIMIT_REV1 22 : #define BYTES_PER_SAR_LIMIT_REV2 (BYTES_PER_SAR_LIMIT_REV1 * 2) : #define ANT_GAINS_TABLE_COUNT 2 : #define REVISION_SIZE 1 : #define PPAG_MODE_SIZE 1 nit: line these all up on the right side
File src/vendorcode/google/chromeos/sar_v2.c:
PS12: I am not convinced this has to be broken out into a separate file. I think a few `if CONFIG(USE_SAR_V2)` can be used to be paper over the differences.
https://review.coreboot.org/c/coreboot/+/56750/comment/b4838d17_4905bb8c PS12, Line 14: if (sar_table->version == 0) : return BYTES_PER_SAR_LIMIT * NUM_SAR_LIMITS + REVISION_SIZE; : else if (sar_table->version == 1) : return BYTES_PER_SAR_LIMIT_REV1 * NUM_SAR_LIMITS + REVISION_SIZE; : else if (sar_table->version == 2) : return BYTES_PER_SAR_LIMIT_REV2 * NUM_SAR_LIMITS + REVISION_SIZE; : else : return REVISION_SIZE; with above, e.g. ``` if (CONFIG(USE_SAR)) return BYTES_PER_SAR_LIMIT * NUM_SAR_LIMITS;
size_t size = 0; if (sar_table->version == 0) size = BYTES_PER_SAR_LIMIT * NUM_SAR_LIMITS; else if (sar_table->version == 1) return BYTES_PER_SAR_LIMIT_REV1 * NUM_SAR_LIMITS; else if (sar_table->version == 2) return BYTES_PER_SAR_LIMIT_REV2 * NUM_SAR_LIMITS; if (CONFIG(USE_SAR_V2)) size += REVISION_SIZE;
return size; ```
https://review.coreboot.org/c/coreboot/+/56750/comment/8fe32aae_3ce16450 PS12, Line 24: wgds_table_size similar here
https://review.coreboot.org/c/coreboot/+/56750/comment/354cc9c8_f4fcad2b PS12, Line 34: antgains_table_size would just return `0` for USE_SAR