Attention is currently required from: Jason Glenesk, Paul Menzel, Martin Roth, Fred Reitberger.
Felix Held 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:
(14 comments)
Patchset:
PS9: i'm finally done with the ec code review
File src/mainboard/amd/birman/Kconfig:
https://review.coreboot.org/c/coreboot/+/73971/comment/36d8e53c_9dd48d0d PS9, Line 94: IO i'd say PCIe and not IO here
https://review.coreboot.org/c/coreboot/+/73971/comment/e6dcf884_4c63366f PS9, Line 94: Ether probably a typo
https://review.coreboot.org/c/coreboot/+/73971/comment/daa2eb7f_71f2a8b2 PS9, Line 117: bool "Only WLAN Enabled" maybe add that it'll use 2 lanes in this case?
https://review.coreboot.org/c/coreboot/+/73971/comment/c7875f44_b27672c4 PS9, Line 120: bool "Only WWAN Enabled" same here
File src/mainboard/amd/birman/ec.c:
https://review.coreboot.org/c/coreboot/+/73971/comment/0e89495e_b9de94ca PS9, Line 57: ECC_TAD_BUF_EN ECC_T*P*AD_BUF_EN
https://review.coreboot.org/c/coreboot/+/73971/comment/4eda3934_60edb34b 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
https://review.coreboot.org/c/coreboot/+/73971/comment/9a496137_7aca90cb PS9, Line 122: tmp |= EC3_DT_RST_AUX | EC3_LOM_RST_AUX; EC3_EVAL_RST_AUX probably should be set. not sure if conditionally on CONFIG(ENABLE_EVAL_CARD) or in any case
https://review.coreboot.org/c/coreboot/+/73971/comment/0346b0e4_dfb47bbc 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 if there's some reason to not enable it
https://review.coreboot.org/c/coreboot/+/73971/comment/f0412524_a3aa78ca 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 up on the expected bus. since we'll need to specify all bits, it's not needed to read the previous state here
https://review.coreboot.org/c/coreboot/+/73971/comment/441bc735_2c99f511 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 in EC_GPIO_C_ADDR
https://review.coreboot.org/c/coreboot/+/73971/comment/6841b315_414d6cb4 PS9, Line 153: tmp |= ECD_TPNL_PWR_EN | ECD_TPNL_EN; i'd set ECD_TPAD_DISABLE_N here
https://review.coreboot.org/c/coreboot/+/73971/comment/fb25a3ff_f31a1cfd 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'll work
https://review.coreboot.org/c/coreboot/+/73971/comment/45e2fa17_c21ef64c PS9, Line 179: CONFIG(WLAN0_WWAN0) || CONFIG(WWAN01) this is equivalent to !CONFIG(WLAN01); not sure which one is better. in the CONFIG(WLAN01) case, i'd drop the ! and swap the branches. feel free to just ack this one if you prefer the way it is set up at the moment. same in line 184
-
Felix Held (Code Review)