Attention is currently required from: Jason Glenesk, Paul Menzel, Martin Roth, Felix Held.
Fred Reitberger has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73971 )
Change subject: mb/amd/birman/ec.c: Update EC configuration ......................................................................
Patch Set 9:
(13 comments)
File src/mainboard/amd/birman/Kconfig:
https://review.coreboot.org/c/coreboot/+/73971/comment/4158b960_964b92e3 PS9, Line 94: IO
i'd say PCIe and not IO here
Done
https://review.coreboot.org/c/coreboot/+/73971/comment/1fa80281_f3bd8991 PS9, Line 94: Ether
probably a typo
Done
https://review.coreboot.org/c/coreboot/+/73971/comment/3ff5344c_64be90f3 PS9, Line 117: bool "Only WLAN Enabled"
maybe add that it'll use 2 lanes in this case?
Done
https://review.coreboot.org/c/coreboot/+/73971/comment/7f4878fd_d1950f5c PS9, Line 120: bool "Only WWAN Enabled"
same here
Done
File src/mainboard/amd/birman/ec.c:
https://review.coreboot.org/c/coreboot/+/73971/comment/92d03379_b5a7ae5f PS9, Line 57: ECC_TAD_BUF_EN
ECC_T*P*AD_BUF_EN
Done
https://review.coreboot.org/c/coreboot/+/73971/comment/dc55c1f9_c564dfbc PS9, Line 64: #define ECD_FPR_RSVD BIT(4)
i'd drop this reserved bit' it's also an input and not an output like the rest
Done
https://review.coreboot.org/c/coreboot/+/73971/comment/ecd0d163_81a57179 PS9, Line 122: tmp |= EC3_DT_RST_AUX | EC3_LOM_RST_AUX;
EC3_EVAL_RST_AUX probably should be set. […]
Setting in any case, to match the DT_RST_AUX case.
https://review.coreboot.org/c/coreboot/+/73971/comment/0e7f4aa0_0c2c6aca PS9, Line 143: tmp |= EC9_CAM0_PWR_EN | EC9_CAM1_PWR_EN | EC9_WWAN_RST;
should EC9_TPM_PWR_EN be set too or not? i don't think that we'll be using the tpm module; not sure […]
I'm fine with enabling it even if we are not using it
https://review.coreboot.org/c/coreboot/+/73971/comment/42f494c3_015088ae PS9, Line 151:
even though they're enabled by default, i'd set ECC_NFC_BUF_EN, ECC_TPAD_BUF_EN and ECC_TPNL_BUF_EN […]
Done
https://review.coreboot.org/c/coreboot/+/73971/comment/224cc47c_65d17bf5 PS9, Line 151:
i'd write the config we want to EC_GPIO_A_ADDR so that we can be sure that the i2c devices will show […]
Done
https://review.coreboot.org/c/coreboot/+/73971/comment/4a5aa96e_0623af0c PS9, Line 153: tmp |= ECD_TPNL_PWR_EN | ECD_TPNL_EN;
i'd set ECD_TPAD_DISABLE_N here
Done
https://review.coreboot.org/c/coreboot/+/73971/comment/c95b3095_3c460f8d PS9, Line 162: tmp = ec_read(EC_GPIO_E_ADDR);
i'd clear ECE_BT_N_TPNL_SEL to make sure that the bluetooth module gets connected to usb so that it' […]
Done
https://review.coreboot.org/c/coreboot/+/73971/comment/0c184654_9d742324 PS9, Line 179: CONFIG(WLAN0_WWAN0) || CONFIG(WWAN01)
this is equivalent to !CONFIG(WLAN01); not sure which one is better. […]
The only part of that I'm not fond of is the somewhat reverse logic of the CONFIG to the bits getting set cleared (on register E above, but comments can help explain that).
For the lane selection, `CONFIG(WLAN01)` makes more sense here
-
Fred Reitberger (Code Review)