Attention is currently required from: Alexander Couzens, Keith Hui, Martin L Roth, Nicholas Chin.
Angel Pons has posted comments on this change by Keith Hui. ( https://review.coreboot.org/c/coreboot/+/79025?usp=email )
Change subject: nb/intel/haswell: Move SPD addresses to devicetree ......................................................................
Patch Set 4:
(4 comments)
Patchset:
PS4: I'm still not fond of having SPD settings spread between devicetree and code.
Commit Message:
https://review.coreboot.org/c/coreboot/+/79025/comment/7ef92cd8_f50d1394?usp... : PS4, Line 14: Patch also covers recently added Z97 boards using Broadwell MRC. IIRC autoport support for Haswell also landed recently, but this change doesn't seem to update autoport.
File src/mainboard/asrock/h81m-hds/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/79025/comment/f2037813_a6367358?usp... : PS4, Line 5: register "spd_addresses" = "{0x50, 0, 0x52, 0}" nit: I would prefer an initialiser with indices:
```suggestion register "spd_addresses" = "{ [0] = 0x50, [2] = 0x52 }" ```
If I get bored enough, I'll replace this with a struct:
``` register "spd_addresses" = "{ .ch0 = { .slot0 = 0x50 }, .ch1 = { .slot0 = 0x52 }, }" ```
File src/northbridge/intel/haswell/broadwell_mrc/raminit.c:
https://review.coreboot.org/c/coreboot/+/79025/comment/df3b34d6_e4298e38?usp... : PS4, Line 377: struct spd_info spdi = {0}; : if (CONFIG(HAVE_SPD_IN_CBFS)) { : /* Obtain the SPD addresses from mainboard code */ : mb_get_spd_map(&spdi); : } else { : /* Boards without memory down: Obtain the SPD addresses from devicetree */ : memcpy(spdi.addresses, cfg->spd_addresses, ARRAY_SIZE(spdi.addresses)); : } : Would be nice to avoid having this pattern repeated three times. How about adding a helper function?
``` struct spd_info get_spd_info(void) { struct spd_info spdi = {0}; if (CONFIG(HAVE_SPD_IN_CBFS)) { /* Obtain the SPD addresses from mainboard code */ mb_get_spd_map(&spdi); } else { /* Boards without memory down: Obtain the SPD addresses from devicetree */ memcpy(spdi.addresses, cfg->spd_addresses, ARRAY_SIZE(spdi.addresses)); } return spdi; } ```