Attention is currently required from: Christian Walter, Jonathan Zhang, Krystian Hebel, Martin L Roth, Martin Roth, Michał Żygowski.
Sergii Dmytruk 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/f729c03c_244258a9 : PS10, Line 68: : config BMC_BT_BASE
Would this be better as a register in devicetree? Since this already needs a devicetree entry, that […]
As far as I remember this option wasn't present in the first version, but there was some issue with getting this information out of devtree in ramstage (worked, but needed some workaround) and it might have not worked in romstage at all, which resulted in adding an analogue of `BMC_KCS_BASE`, which I initially thought wasn't necessary.
https://review.coreboot.org/c/coreboot/+/67057/comment/f844e10a_c5ec14ca : 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?
It can because implementations are mutually exclusive. However, this would break uses of `IPMI_KCS_TIMEOUT_MS` in existing configs. If that's OK, I'll merge the options.
File src/drivers/ipmi/ipmi_bt.c:
https://review.coreboot.org/c/coreboot/+/67057/comment/541f65b2_9489de3e : 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 […]
I agree, the check seems unnecessary.
https://review.coreboot.org/c/coreboot/+/67057/comment/187894b6_eb8bf857 : 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 periphera […]
It can't be longer, length is a single byte, which is why `MAX_SIZE` is 255.
File src/drivers/ipmi/ipmi_bt_ops.c:
https://review.coreboot.org/c/coreboot/+/67057/comment/0cc6d167_41d107a4 : PS10, Line 66: bmc_revision_minor
the minor revision isn't allowed to be zero?
It is, probably a mistake while editing a conditional. Thanks.
File src/drivers/ipmi/ipmi_ops_premem.c:
https://review.coreboot.org/c/coreboot/+/67057/comment/740d9fc0_17aa9d01 : PS10, Line 12: #if CONFIG(IPMI_BT)
We generally shouldn't have to ifdef out included header files. […]
Done
https://review.coreboot.org/c/coreboot/+/67057/comment/d7d9d93e_feefe09a : 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 […]
Done
-
Sergii Dmytruk (Code Review)