Furquan Shaikh 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:
(10 comments)
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?
Done
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. […]
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/41272/2/src/soc/amd/common/block/lp... PS2, Line 438: 0
Can we return an error?
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/41272/2/src/soc/amd/common/block/lp... PS2, Line 621: ntentional fall-through
Same as above
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/41272/2/src/soc/amd/common/block/lp... PS2, Line 763: enabled
When we get verstage in PSP enabled, it could setup the other channels as well. […]
The channels are reset by eSPI RESET# which is the first thing that espi_setup() does. So, the initial state should be known for all the channels.
https://review.coreboot.org/c/coreboot/+/41272/2/src/soc/amd/common/block/lp... PS2, Line 886:
nit: space
Done
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.
Done