Attention is currently required from: Angel Pons, Maximilian Brune.
David Milosevic has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68137 )
Change subject: [WIP] mb/prodrive/atlas: Populate smbios table with VPD from ECs EMI ......................................................................
Patch Set 6:
(9 comments)
File src/mainboard/prodrive/atlas/emi.h:
https://review.coreboot.org/c/coreboot/+/68137/comment/ad2de4e9_4df9b242 PS6, Line 40: typedef enum {
avoid typedef as per coreboot coding style
Coreboot is full of typedefs. I don't see the problem here.
https://review.coreboot.org/c/coreboot/+/68137/comment/41039720_c3d6e195 PS6, Line 43: EMI_REGION_1 ///< EMI region 1
the comment is unnecessary
Agreed.
https://review.coreboot.org/c/coreboot/+/68137/comment/80566f69_b75158c6 PS6, Line 47: typedef enum {
avoid typedef as per coreboot coding style
See previous reply.
File src/mainboard/prodrive/atlas/emi.c:
https://review.coreboot.org/c/coreboot/+/68137/comment/eba107d9_f40fd7a1 PS6, Line 83: {
wouldn't it be easier to directly use an u32 value instead of a pointer? It would remove the need to […]
I already considered using a u32 but I did not like this approach so I switched to an u8* byte-buffer. The problem here is that I would like to keep some consistency across the EMI functions (emi_read_region_block function needs to use u8* in order to read in an arbitrary amount of bytes). In my opinion it makes sense to let all EMI functions operate on the same type.
Also where do you see an issue with the typecasts below ?
Maybe we don't need this function yet, but it is good to offer complete read/write capabilities for the EMI regions and not only the functionality we currently need. I would like to keep this function.
https://review.coreboot.org/c/coreboot/+/68137/comment/8221f83a_e45c6b9d PS6, Line 139: inline void emi_write_register(const EMI_INSTANCE instance, const u8 offs, const u8 value)
Also, explicitly declaring things inline is generally not a good idea in coreboot.
I wont remove this function for the same reason as above. Both functions (emi_read_register, emi_write_register) can be used to interact with the EMI runtime registers without having to think about the EMI base address.
But I can remove the inlining.
https://review.coreboot.org/c/coreboot/+/68137/comment/53490ef3_adc9f378 PS6, Line 144: inline u8 emi_read_register(const EMI_INSTANCE instance, const u8 offs)
function can be removed, since it's not used anywhere (only clutters up code).
See previous replies.
File src/mainboard/prodrive/atlas/ld_config.c:
PS6:
Please use `device/pnp_ops. […]
Thats why this patch is marked as WIP. I still have to figure out how to use pnp_ops properly for this task.
File src/mainboard/prodrive/atlas/mainboard.c:
https://review.coreboot.org/c/coreboot/+/68137/comment/1bef6115_0ce19cb1 PS6, Line 46: strncpy((char *) pn, prefix, sizeof(prefix));
Please check if what we've done in CB:68322 and CB:68323 can also be done for Atlas.
Regarding CB:68322, there is already an additional nullbyte at the end of the pn-buffer at line 44 (sizeof(prefix) + PN_LENGTH + 1).
And for CB:68323, I don't see why snprintf would be safer than the current approach.
There are (sizeof(prefix) + PN_LENGTH + 1) bytes allocated for the part-number and we copy sizeof(prefix) bytes to the start of the part-number. Then there are (PN_LENGTH + 1) bytes left for the part-number. After that, vpd_get() reads PN_LENGTH bytes into the part-number buffer right after the previously copied prefix. By now, the part-number buffer should only have 1 byte left, which is used for the nullbyte.
https://review.coreboot.org/c/coreboot/+/68137/comment/e36e323e_c144ddb7 PS6, Line 46: strncpy((char *) pn, prefix, sizeof(prefix));
Please check if what we've done in CB:68322 and CB:68323 can also be done for Atlas.
Regarding