Attention is currently required from: Alexander Couzens, Angel Pons, Elyes Haouas, Martin L Roth.
Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/76962?usp=email )
Change subject: SNB+MRC boards: Do not redo PEI data struct in hook ......................................................................
Patch Set 4:
(6 comments)
File src/mainboard/asus/p8x7x-series/variants/p8z77-m_pro/early_init.c:
https://review.coreboot.org/c/coreboot/+/76962/comment/70e776bd_bb04f7a4 : PS4, Line 16: #include <northbridge/intel/sandybridge/pei_data.h>
why remove this? I think we'd want this in every file - we prefer not to use indirect includes.
Part of my goal with this is to have all the boards include only raminit.h no matter what raminit it is going to use. Is the reason why I placed the Haswell API definitions in raminit.h. Question: If I want to achieve this while sticking to the no-indirect-include rule, would I have to get rid of mainboard_fill_pei_data() completely as well? raminit.h itself already includes pei_data.h for its prototypes.
https://review.coreboot.org/c/coreboot/+/76962/comment/cce6cd4e_d302ec4f : PS4, Line 59: pei_data)
This name was changed in this file, but not in others. […]
I used the shorter "pei" when I added p8z77-m board. Although I do like to shorten this as I go as well, I try to keep this name same to reduce the number of lines changed. Mismatches are most likely oversight.
https://review.coreboot.org/c/coreboot/+/76962/comment/f0ccf2f0_4ce7125f : PS4, Line 60: []
Nit: I'd probably use a #define here to make sure it matches the size of spd_addresses in the pei_da […]
I'm following existing convention (as in p8z77-m, the only board already doing what this patch looks to achieve). And as you see in subsequent patches in the train, this paricular line won't be around for long.
https://review.coreboot.org/c/coreboot/+/76962/comment/20f24c97_b88878e9 : PS4, Line 61: 16][3
Nit: These magic numbers could use some #defines to make sure they're in sync with the definition of […]
This will need to be addressed separately because it is part of a bigger problem: SNB/IVB boards also have to specify their USB configurations twice, because value orders are different and one of them has different representation. Eventually we also want to harmonize this value and then stuff it into devicetree, this being another set of static data. We have a setting reserved for this but it also remains unused to this date.
https://review.coreboot.org/c/coreboot/+/76962/comment/9cf37237_53e40037 : PS4, Line 62: /* {enabled, oc_pin, cable len 0x0080=<8inches/20cm} */
This comment or something similar could be applied in other files if you wanted. Up to you.
To be addressed separately.
https://review.coreboot.org/c/coreboot/+/76962/comment/c32e5e80_520342a3 : PS4, Line 79: sizeof(usbcfg)
Maybe use the size of pei.usb_port_config & pei. […]
Makes sense. I'll update the USB stuff for next patch set. SPD stuff won't be around for long.
-
Keith Hui (Code Review)