Attention is currently required from: Angel Pons, Arthur Heymans.
Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61540 )
Change subject: mb/asus/p8x7x-series: Refactor mainboard_get_spd() ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4: All, let's start that weakness discussion right now.
As far as precedents go, it has already been set. 254 C files in the tree (including those being discussed here) defined weak functions. Plus i945 northbridge also has 3 weak functions, one of them about SPD, almost exactly the use case here. So does i440bx, with weak hooks to enable/disable SPD, with the only user being asus/p3b-f because it does need them. (I did for a time try to deprecate this one and move the offending code elsewhere.) I have another patch train up consolidating a MRC raminit hook on SNB/IVB as well with only one board overriding, and it was Angel who said it benefits from having a weak default. These are but just a few examples of weak functions.
If weak functions are not to be used, let's look at a few alternatives:
1. Refactor code of concern into separate compilation units and switch between them with Kconfig options and Makefile.inc. This in essence is how board variants work. Does add some complexity to the makefiles. Might as well abandon this patch.
2. Conditionals in the function by board variant or # of RAM slots (which still needs to be defined somewhere). Compiler and the next programmer looking will have to sort through the inevitable rats nest.
3. Leave out the __weak and leave it to the guy porting a P8x7x with less than 4 RAM slots (eg. P8H77-I, ITX with 2 slots) to patch the override. This could just be kicking the ball down the road, and his patch could be more ugly.
4. Leave out the __weak and force all current and future P8x7x variants to try reading 4 sets of SPD data. Current variants are not the problem (otherwise this patch would not exist) but future <4 slots boards would have useless work done in runtime.
-
Keith Hui (Code Review)