Attention is currently required from: Angel Pons, Lean Sheng Tan, Werner Zeh.
Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69383 )
Change subject: soc/intel/ehl: Add EHL MDIO operation ......................................................................
Patch Set 3:
(12 comments)
File src/soc/intel/elkhartlake/include/soc/mdio.h:
https://review.coreboot.org/c/coreboot/+/69383/comment/d920c644_ba0c53fb PS1, Line 8: Address
address
Done
https://review.coreboot.org/c/coreboot/+/69383/comment/343fbb3b_9cd31cf7 PS1, Line 10: Address
address
Done
https://review.coreboot.org/c/coreboot/+/69383/comment/3db73e5c_a232b490 PS1, Line 11: Address
address
Done
https://review.coreboot.org/c/coreboot/+/69383/comment/ce9ee283_1a2ea246 PS1, Line 12: Trailing Clocks
trailing clocks
Done
https://review.coreboot.org/c/coreboot/+/69383/comment/38583b72_10b8b731 PS1, Line 18: Data
data
Done
https://review.coreboot.org/c/coreboot/+/69383/comment/d75a03fe_dfc079b3 PS1, Line 21: mdio
Since you spell MDIO uppercase in comments elsewhere, I would stay consistent in all comments.
Done
File src/soc/intel/elkhartlake/mdio.c:
https://review.coreboot.org/c/coreboot/+/69383/comment/b5777815_5a2b9c9b 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 […]
Ack
https://review.coreboot.org/c/coreboot/+/69383/comment/ef4c7873_a0a63c66 PS1, Line 46: status = phy_gmii_ready(io_mem_base); : if (status == CB_ERR) {
You can get rid of the extra status variable here: […]
Done
https://review.coreboot.org/c/coreboot/+/69383/comment/447687a4_363b980f PS1, Line 52: BIOS_DEBUG
Should we move this to SPEW-level?
Ack
https://review.coreboot.org/c/coreboot/+/69383/comment/901dfa37_d46274bc PS1, Line 61: /* Get the base address of the I/O registers in memory space */
Same here.
Done
https://review.coreboot.org/c/coreboot/+/69383/comment/259ce665_bd73a7db PS1, Line 75: status = phy_gmii_ready(io_mem_base); : if (status == CB_ERR)
Same status-comment applies here.
Done
File src/soc/intel/elkhartlake/tsn_gbe.c:
https://review.coreboot.org/c/coreboot/+/69383/comment/1e77151d_7973e492 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 devic […]
Ack and I changed from WARNING to ALERT