Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41272 )
Change subject: soc/amd/common/block: Add support for configuring eSPI connection to slave ......................................................................
Patch Set 2: Code-Review+1
(9 comments)
Overall this is a great refactor. It's really easy to read.
https://review.coreboot.org/c/coreboot/+/41272/2/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/espi.h:
https://review.coreboot.org/c/coreboot/+/41272/2/src/soc/amd/common/block/in... PS2, Line 36: VAL VALUE?
https://review.coreboot.org/c/coreboot/+/41272/2/src/soc/amd/common/block/in... PS2, Line 38: enum espi_io_mode { nit: Move the enum declarations after the register declaration. This way all the macros are in one block and it's easier to visually see they are all related.
https://review.coreboot.org/c/coreboot/+/41272/2/src/soc/amd/common/block/lp... File src/soc/amd/common/block/lpc/espi_util.c:
https://review.coreboot.org/c/coreboot/+/41272/2/src/soc/amd/common/block/lp... PS2, Line 351: type How about cmd_type so it looks closer to enum espi_cmd_type.
https://review.coreboot.org/c/coreboot/+/41272/2/src/soc/amd/common/block/lp... PS2, Line 438: 0 Can we return an error?
https://review.coreboot.org/c/coreboot/+/41272/2/src/soc/amd/common/block/lp... PS2, Line 591: Can we print something out saying that we are falling back to a different mode. Otherwise someone might assume we are running in quad, but actually running in dual.
https://review.coreboot.org/c/coreboot/+/41272/2/src/soc/amd/common/block/lp... PS2, Line 621: ntentional fall-through Same as above
https://review.coreboot.org/c/coreboot/+/41272/2/src/soc/amd/common/block/lp... PS2, Line 749: espi_send_pltrst_deassert Can you move this to the caller. Seems like an undocummented side effect
https://review.coreboot.org/c/coreboot/+/41272/2/src/soc/amd/common/block/lp... PS2, Line 886: nit: space
https://review.coreboot.org/c/coreboot/+/41272/2/src/soc/amd/common/block/lp... PS2, Line 907: /* Enable subtractive decode if configured */ : global_ctrl_reg = espi_read32(ESPI_GLOBAL_CONTROL_1); : if (cfg->subtractive_decode) { : global_ctrl_reg &= ~ESPI_SUB_DECODE_SLV_MASK; : global_ctrl_reg |= ESPI_SUB_DECODE_EN; : : } else { : global_ctrl_reg &= ~ESPI_SUB_DECODE_EN; : } : espi_write32(ESPI_GLOBAL_CONTROL_1, global_ctrl_reg); Move this into espi_setup_subtractive_decode so it matches the rest of the function.