Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46091 )
Change subject: mb/intel/adlrvp: Add ADL-P romstage mainboard code ......................................................................
Patch Set 2:
(6 comments)
ir
https://review.coreboot.org/c/coreboot/+/46091/2/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/romstage_fsp_params.c:
https://review.coreboot.org/c/coreboot/+/46091/2/src/mainboard/intel/adlrvp/... PS2, Line 40: { .spd_smbus_address[0] = 0xA0,
I suggest formatting this struct as follows: […]
Getting below error
src/mainboard/intel/adlrvp/romstage_fsp_params.c: In function 'mainboard_memory_init_params': src/mainboard/intel/adlrvp/romstage_fsp_params.c:41:6: error: expected identifier before '[' token .[0] = 0xa0, ^ src/mainboard/intel/adlrvp/romstage_fsp_params.c:42:5: error: expected '}' before '.' token .[1] = 0xa2, ^ src/mainboard/intel/adlrvp/romstage_fsp_params.c:40:25: note: to match this '{' .spd_smbus_address = { ^ src/mainboard/intel/adlrvp/romstage_fsp_params.c: At top level:
https://review.coreboot.org/c/coreboot/+/46091/2/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/spd/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/46091/2/src/mainboard/intel/adlrvp/... PS2, Line 6: SPD_DEPS := $(foreach f, $(SPD_SOURCES), src/mainboard/$(MAINBOARDDIR)/spd/$(f).spd.hex) : : # Include spd ROM data : $(SPD_BIN): $(SPD_DEPS) : for f in $+; \ : do for c in $$(cat $$f | grep -v ^#); \ : do printf $$(printf '%o' 0x$$c); \ : done; \ : done > $@ : : cbfs-files-y += spd.bin : spd.bin-file := $(SPD_BIN) : spd.bin-type := spd
Most of this can be eliminated by using the HAVE_SPD_BIN_IN_CBFS Kconfig option
Ack
https://review.coreboot.org/c/coreboot/+/46091/2/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/spd/spd.h:
https://review.coreboot.org/c/coreboot/+/46091/2/src/mainboard/intel/adlrvp/... PS2, Line 6: void mainboard_fill_dq_map_ch0(void *dq_map_ptr); : void mainboard_fill_dq_map_ch1(void *dq_map_ptr); : void mainboard_fill_dqs_map_ch0(void *dqs_map_ptr); : void mainboard_fill_dqs_map_ch1(void *dqs_map_ptr); : void mainboard_fill_rcomp_res_data(void *rcomp_ptr); : void mainboard_fill_rcomp_strength_data(void *rcomp_strength_ptr);
unused?
my bad :(
https://review.coreboot.org/c/coreboot/+/46091/2/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/variants/adlrvp_p/memory.c:
https://review.coreboot.org/c/coreboot/+/46091/2/src/mainboard/intel/adlrvp/... PS2, Line 8: __weak
shouldn't be weak
Ack
https://review.coreboot.org/c/coreboot/+/46091/2/src/mainboard/intel/adlrvp/... PS2, Line 16: { 0, 2, 3, 1, 6, 7, 5, 4, /* Byte 0 */ : 10, 8, 11, 9, 14, 12, 13, 15 }, /* Byte 1 */ : { 12, 8, 14, 10, 11, 13, 15, 9, /* Byte 2 */ : 5, 0, 7, 3, 6, 2, 1, 4 }, /* Byte 3 */ : { 3, 0, 2, 1, 6, 5, 4, 7, /* Byte 4 */ : 12, 13, 14, 15, 10, 9, 8, 11 }, /* Byte 5 */ : { 2, 6, 7, 1, 3, 4, 0, 5, /* Byte 6 */ : 9, 13, 8, 15, 14, 11, 12, 10 }, /* Byte 7 */ : { 3, 0, 1, 2, 7, 4, 6, 5, /* Byte 0 */ : 10, 8, 11, 9, 14, 13, 12, 15 }, /* Byte 1 */ : { 10, 12, 14, 8, 9, 13, 15, 11, /* Byte 2 */ : 3, 7, 6, 2, 0, 4, 5, 1 }, /* Byte 3 */ : { 12, 15, 14, 13, 9, 10, 11, 8, /* Byte 4 */ : 7, 4, 6, 5, 0, 1, 3, 2 }, /* Byte 5 */ : { 0, 2, 4, 3, 1, 6, 7, 5, /* Byte 6 */ : 13, 9, 10, 11, 8, 12, 14, 15 },
nit: right-align the numbers to make this more readable: […]
Ack
https://review.coreboot.org/c/coreboot/+/46091/2/src/mainboard/intel/adlrvp/... PS2, Line 54: __weak
This shouldn't be weak
Ack