Attention is currently required from: Arthur Heymans, David Hendricks, Jincheng Li, Jonathan Zhang, Lean Sheng Tan, Nicholas Chin, Patrick Rudolph, Paul Menzel, TangYiwei, Varshit Pandya.
Shuo Liu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81319?usp=email )
Change subject: mb/intel/avenuecity_crb: Add 2S GNR/SRF-AP server board Avenue City ......................................................................
Patch Set 32:
(7 comments)
File src/mainboard/intel/avenuecity_crb/bootblock.c:
https://review.coreboot.org/c/coreboot/+/81319/comment/71be718a_bb88a90d : PS31, Line 5: #include <device/pci_def.h> : #include <device/pci_ops.h>
unused?
Done
https://review.coreboot.org/c/coreboot/+/81319/comment/f5401f8a_54a8779c : PS31, Line 9: #include <soc/pci_devs.h>
unused?
Done
https://review.coreboot.org/c/coreboot/+/81319/comment/23beaa1d_67f1443b : PS31, Line 21: CONFIG_ASPEED_SIO_PORT
Does this need to be a Kconfig?
Done
File src/mainboard/intel/avenuecity_crb/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/81319/comment/6f13a390_983d363a : PS31, Line 16: register "gen2_dec" = "0x0" : register "gen3_dec" = "0x0" : register "gen4_dec" = "0x0"
0 is default so no need to set it explicitly
Done
https://review.coreboot.org/c/coreboot/+/81319/comment/2e9117e4_5386fcb3 : PS31, Line 21: register "serial_io_uart_debug_io_base" = "0x3F8"
Can't CONFIG_TTYS0_BASE be reused?
The devicetree.cb cannot directly refer to a config macro and cannot update at the moment. Will check if there is method to add the support.
File src/mainboard/intel/avenuecity_crb/include/config/iio.h:
https://review.coreboot.org/c/coreboot/+/81319/comment/95a384d8_6db31df9 : PS31, Line 12: _IIO_PORT_CFG_STRUCT(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, : 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0),
suggestion: It looks like slots are either: […]
In this basic example it should be no problem. But I checked the full configuration cases, others fields also needs to be set to enable some further features hence the full configuration macros still needs to be kept. I tried to optimize a bit. Not sure if this is better.
https://review.coreboot.org/c/coreboot/+/81319/comment/62ff15a4_5a57d3d0 : PS31, Line 8: static const iio_pe_config avc_iio_config_table[] = { : {_IIO_PE_CFG_STRUCT(0x0, PE0, IIO_BIFURCATE_x2x2x2x2x8, PE_TYPE_PCIE) { : _IIO_PORT_CFG_STRUCT(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, : 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x0, 0x0, 0x0, 0x4B, 0x1), : _IIO_PORT_CFG_STRUCT(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, : 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0), : _IIO_PORT_CFG_STRUCT(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, : 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0), : _IIO_PORT_CFG_STRUCT(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, : 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0), : _IIO_PORT_CFG_STRUCT(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, : 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x0, 0x0, 0x0, 0x4B, 0x2), : _IIO_PORT_CFG_STRUCT(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, : 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x0, 0x0, 0x0, 0x4B, 0x3), : _IIO_PORT_CFG_STRUCT(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, : 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x0, 0x0, 0x0, 0x4B, 0x4), : _IIO_PORT_CFG_STRUCT(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, : 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x0, 0x0, 0x0, 0x4B, 0x5) : }}, : }; :
move to romstage.c file? Data structures in a header is not a good idea.
I see. I move the avenuecity_crb/include/config/iio.h to avenuecity_crb/config/iio.c and update Makefile to include it. We have some future config tables which could be put into config/xxx.c as well. Your opinion?