Attention is currently required from: Christian Walter, Jonathan Zhang, Krystian Hebel, Martin Roth, Michał Żygowski, Sergii Dmytruk.
Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/67057?usp=email )
Change subject: drivers/ipmi: add BT interface ......................................................................
Patch Set 10:
(7 comments)
File src/drivers/ipmi/Kconfig:
https://review.coreboot.org/c/coreboot/+/67057/comment/adb8e558_26a55c67 : PS10, Line 68: : config BMC_BT_BASE Would this be better as a register in devicetree? Since this already needs a devicetree entry, that seems like a better place to put it/
https://review.coreboot.org/c/coreboot/+/67057/comment/29c8ef10_7d06cedf : PS10, Line 77: IPMI_BT_TIMEOUT_MS Can this be combined with the IPMI_KCS_TIMEOUT_MS value? Rename it to IPMI_TIMEOUT_MS or something?
File src/drivers/ipmi/ipmi_bt.c:
https://review.coreboot.org/c/coreboot/+/67057/comment/17a4f4a2_dc600461 : PS10, Line 58: if ((inb(port + BT_CTRL_INC) & H_BUSY) == 0) Do you actually need to check first? It looks like either way you go to the next line and wait for B_BUSY, so it seems like you could skip the check and just try to set it.
https://review.coreboot.org/c/coreboot/+/67057/comment/267dd1af_c8552efc : PS10, Line 134: i < len Add a check to make sure the response isn't longer than MAX_SIZE? Since it's coming from a peripheral, the value can't be trusted to be correct.
File src/drivers/ipmi/ipmi_bt_ops.c:
https://review.coreboot.org/c/coreboot/+/67057/comment/912add44_c3cd9633 : PS10, Line 66: bmc_revision_minor the minor revision isn't allowed to be zero?
File src/drivers/ipmi/ipmi_ops_premem.c:
https://review.coreboot.org/c/coreboot/+/67057/comment/21cb662a_1f7b1185 : PS10, Line 12: #if CONFIG(IPMI_BT) We generally shouldn't have to ifdef out included header files. It's fine if nothing inside them is used, the compiler just ignores it.
https://review.coreboot.org/c/coreboot/+/67057/comment/5d724a8e_d46e4cac : PS10, Line 57: #if CONFIG(IPMI_BT) : if (ipmi_bt_clear(dev->path.pnp.port)) With the header above always included, you should be able to turn this into a regular if instead of using the ifdef.