Attention is currently required from: Mario Scheithauer, Angel Pons, Lean Sheng Tan.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69383 )
Change subject: soc/intel/ehl: Add EHL MDIO operation ......................................................................
Patch Set 1:
(12 comments)
File src/soc/intel/elkhartlake/include/soc/mdio.h:
https://review.coreboot.org/c/coreboot/+/69383/comment/b49f8f2e_37167dcf PS1, Line 8: Address address
https://review.coreboot.org/c/coreboot/+/69383/comment/e6b893bd_cee68c9f PS1, Line 10: Address address
https://review.coreboot.org/c/coreboot/+/69383/comment/d76e7694_f5258f55 PS1, Line 11: Address address
https://review.coreboot.org/c/coreboot/+/69383/comment/96d1bf09_df91e8c5 PS1, Line 12: Trailing Clocks trailing clocks
https://review.coreboot.org/c/coreboot/+/69383/comment/04ba4d25_9fc59df9 PS1, Line 18: Data data
https://review.coreboot.org/c/coreboot/+/69383/comment/d73a7ce2_29329cbd PS1, Line 21: mdio Since you spell MDIO uppercase in comments elsewhere, I would stay consistent in all comments.
File src/soc/intel/elkhartlake/mdio.c:
https://review.coreboot.org/c/coreboot/+/69383/comment/c4403d72_7bd3daff PS1, Line 33: /* Get the base address of the I/O registers in memory space */ Do we need this comment here? It decreases code readability while it does not provide any additional information that cannot be derived from the code itself.
https://review.coreboot.org/c/coreboot/+/69383/comment/8310afb8_84633218 PS1, Line 46: status = phy_gmii_ready(io_mem_base); : if (status == CB_ERR) { You can get rid of the extra status variable here:
if (phy_gmii_ready(io_mem_base) == CB_ERROR) {
If you do not use it elsewhere, why should we have it at all?
https://review.coreboot.org/c/coreboot/+/69383/comment/da3d1a57_d56b6ce9 PS1, Line 52: BIOS_DEBUG Should we move this to SPEW-level?
https://review.coreboot.org/c/coreboot/+/69383/comment/1fc89c8f_3fee9d65 PS1, Line 61: /* Get the base address of the I/O registers in memory space */ Same here.
https://review.coreboot.org/c/coreboot/+/69383/comment/534b26cc_9a20eafe PS1, Line 75: status = phy_gmii_ready(io_mem_base); : if (status == CB_ERR) Same status-comment applies here.
File src/soc/intel/elkhartlake/tsn_gbe.c:
https://review.coreboot.org/c/coreboot/+/69383/comment/256e6c31_83623612 PS1, Line 41: specified mdio device is missing mdio ops! Since you have access to the device, print the device path here instead of the "specified mdio device"?