Attention is currently required from: ritul guru. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50315 )
Change subject: mainboard/amd/bilby: Add Bilby CRB board ......................................................................
Patch Set 7:
(7 comments)
File src/mainboard/amd/bilby/Kconfig:
https://review.coreboot.org/c/coreboot/+/50315/comment/8d956d7e_92e6e00b PS7, Line 36: bool "0x4e/0x4d base address" 4e/4f
https://review.coreboot.org/c/coreboot/+/50315/comment/a6c0c463_75205461 PS7, Line 39: bool "0x164e/0x164d base address" .. 4e/4f ?
https://review.coreboot.org/c/coreboot/+/50315/comment/0a81d8d9_21e7f702 PS7, Line 43: config SUPERIO_ADDR_BASE Why is this a Kconfig?
if (CONFIG(BILBY_SMSC_SIO136_BASE_4E)) return 0x4e; if (CONFIG(BILBY_SMSC_SIO136_BASE_164E)) return 0x164e; dead_code();
https://review.coreboot.org/c/coreboot/+/50315/comment/a2c6a9cb_2583c833 PS7, Line 86: if !AMD_LPC_DEBUG_CARD Could an empty line.
File src/mainboard/amd/bilby/bootblock.c:
https://review.coreboot.org/c/coreboot/+/50315/comment/28658ea7_4d6223f6 PS7, Line 14: if (CONFIG(SUPERIO_SMSC_SIO1036)) { Looks like a copy from mandolin. If this add-on LPC debug card can be used with any platform with LPC pins conveniently routed, the code should have moved in a common place together with the related Kconfigs.
File src/mainboard/amd/bilby/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/50315/comment/7e1cc27a_45796fd8 PS7, Line 165: chip superio/smsc/sio1036 # optional debug card Currently this is an empty statement and there would be no superio init in ramstage.
Then again, if you did define the pnp devices here, you would need the SUPERIO_BASE_ADDR in Kconfig available and also SUPERIO_SMSC_SIO1036 unconditionally selected.
These conditionally enabled devices are bit of problem from static devicetree perspective.
File src/mainboard/amd/bilby/mainboard.c:
https://review.coreboot.org/c/coreboot/+/50315/comment/1f706991_1c377c3b PS7, Line 16: /* TODO: recheck IRQ tables */ Some of this should be re-evaluated for what really belongs under soc/amd/common. There may be few cases where board needs specific IRQ routing, but an override approach over 99% identical copies might be a better choice here.