[M] Change in coreboot[main]: mb/amd/birman_plus/ec: Rectify ECRAM register bits

[Felix Held (Code Review)]

Attention is currently required from: Ana Carolina Cabral.

Felix Held has posted comments on this change by Ana Carolina Cabral. ( https://review.coreboot.org/c/coreboot/+/86084?usp=email )

Change subject: mb/amd/birman_plus/ec: Rectify ECRAM register bits
......................................................................

Patch Set 13:

(9 comments)

File src/mainboard/amd/birman_plus/ec.c:

https://review.coreboot.org/c/coreboot/+/86084/comment/3caf17fa_3392af72?usp... :
PS13, Line 11: #define EC_GPIO_0_ADDR		0xA0
the defines are added, but not used in the code; is this intended?

https://review.coreboot.org/c/coreboot/+/86084/comment/cf2bbc92_32e445e2?usp... :
PS13, Line 33: #define EC_GPIO_7_ADDR		0xA7
do we also need to bring BIT(6) of this one into a defined state?

https://review.coreboot.org/c/coreboot/+/86084/comment/81e140ab_c14c9beb?usp... :
PS13, Line 36: _N
i don't think that this one is inverted. sure, the signal on the m.2 slot is inverted, but there's an n-channel fet driving that one which converts the noninverted signal from the ec to an inverted open drain signal to the card

https://review.coreboot.org/c/coreboot/+/86084/comment/827604dc_bd7fac63?usp... :
PS13, Line 38: #define EC_GPIO_8_ADDR		0xA8
is it intended that this register isn't written to?

https://review.coreboot.org/c/coreboot/+/86084/comment/b4092d96_e23ed2ad?usp... :
PS13, Line 79: #define EC_GPIO_F_ADDR		0xAF
i wonder if we'll need some of the other bits in this register too or does the EC drive those by itself? only looked at the schematics, not the reference code

https://review.coreboot.org/c/coreboot/+/86084/comment/a30ff7f5_70e99160?usp... :
PS13, Line 127: EC7_WL_RADIO_DIS_N
see my comment on the register bit; i'd expect this one needing to be cleared, not set

https://review.coreboot.org/c/coreboot/+/86084/comment/7c5e275e_0c35ff78?usp... :
PS13, Line 138: 	tmp = ECA_MUX1_S0 | ECA_SMBUS1_EN | ECA_SMBUS0_EN;
would probably be good to configure the other muxes here too

https://review.coreboot.org/c/coreboot/+/86084/comment/8ee97103_2a809053?usp... :
PS13, Line 142: 	tmp = ec_read(EC_GPIO_6_ADDR);
              : 	tmp |= EC6_TPNL_BUF_EN | EC6_TPAD_BUF_EN;
              : 	printk(BIOS_SPEW, "Write reg [0x%02x] = 0x%02x\n", EC_GPIO_6_ADDR, tmp);
              : 	ec_write(EC_GPIO_6_ADDR, tmp);
i'd move this block between the ones for EC_GPIO_3_ADDR and EC_GPIO_7_ADDR

https://review.coreboot.org/c/coreboot/+/86084/comment/0b6d0692_7375a7fa?usp... :
PS13, Line 156: 	if (CONFIG(DISABLE_WWAN_GBE_BIRMANPLUS)) {	// no WWAN, turn off WWAN power
i'd find it a bit easier to read when doing the bit clear/bit set in the if/else branches and not have a default and override it in one case. same thing about the 3 if(CONFIG(...)) statements below