Attention is currently required from: Werner Zeh. Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63888 )
Change subject: soc/intel/ehl: Provide function to change PHY-to-MAC IRQ polarity ......................................................................
Patch Set 2:
(20 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/63888/comment/9e026077_3ef04532 PS1, Line 7: PHY to MAC
'PHY-to-MAC' could make things more clear here as it is the IRQ routed from the external PHY to the […]
Done
https://review.coreboot.org/c/coreboot/+/63888/comment/1a423c0d_3df8fb99 PS1, Line 9: It may be that a connected PHY requires an inverting of the interrupt : signal.
Maybe: […]
Done
File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/63888/comment/47991c3f_158aa94a PS1, Line 246: TSN_GBE_PHY2MAC_INTR_POL
Should we move to a model where the polarity is given directly instead of just 'inverting' the curre […]
Done
https://review.coreboot.org/c/coreboot/+/63888/comment/2b0d011b_cfb94be0 PS1, Line 251: This invert
Maybe: […]
Done
File src/soc/intel/elkhartlake/include/soc/tsn_gbe.h:
https://review.coreboot.org/c/coreboot/+/63888/comment/46745fc4_6a5c5b76 PS1, Line 12: TSN_GMII_DELAY
Add _US to the define to indicated that the timout is in microsecond units?
Done
https://review.coreboot.org/c/coreboot/+/63888/comment/46650aa1_93ce834c PS1, Line 14: ADDR
Maybe 'ADR'?
Done
https://review.coreboot.org/c/coreboot/+/63888/comment/43f53229_4a23f261 PS1, Line 24: ADD
'ADR' as you mead address here? ADD just look weird here.
Done
https://review.coreboot.org/c/coreboot/+/63888/comment/4564753d_bb95c95d PS1, Line 26: 0x40
Once you have started to use the shift syntax, why not continue here? […]
Done
File src/soc/intel/elkhartlake/tsn_gbe.c:
https://review.coreboot.org/c/coreboot/+/63888/comment/fb535f1a_652b34a5 PS1, Line 34: : uint32_t i; : : for (i = 0; i < TSN_MAX_LOOP_TIME; ++i) { : if (read32((void *)(io_mem_base + TSN_MAC_MDIO_ADDR)) & TSN_MAC_GMII_BUSY) : udelay(TSN_GMII_DELAY); : else : return CB_SUCCESS; : } : printk(BIOS_ERR, "%s Timeout in %d micro seconds\n", __func__, : TSN_MAX_LOOP_TIME * TSN_GMII_DELAY); : return CB_ERR;
Would it make sense to use a stopwatch here instead? You could get rid of the for-loop by just defin […]
Done
https://review.coreboot.org/c/coreboot/+/63888/comment/ce391451_af205b28 PS1, Line 48: uint8_t phy_add, uint8_t reg_add
I would rather call them pyh_adr and reg_adr here.
Done
https://review.coreboot.org/c/coreboot/+/63888/comment/b22b687a_e5e7cbf4 PS1, Line 52: Status
status
Done
https://review.coreboot.org/c/coreboot/+/63888/comment/c37dd945_547f074c PS1, Line 62: // Wait for MDIO frame transfer complete before reading MDIO DATA register
Please use the propper comment style here.
Done
https://review.coreboot.org/c/coreboot/+/63888/comment/46e51238_dbc62e16 PS1, Line 65: die
Maybe die() is a bit to hard here. Let's try to continue as far as possible while throwing an error.
Done
https://review.coreboot.org/c/coreboot/+/63888/comment/2bccfb56_0496857a PS1, Line 68: BIOS_INFO
This one should be on DEBUG level (or even on SPEW) I guess, or?
Done
https://review.coreboot.org/c/coreboot/+/63888/comment/b23e8814_0bcb5bbe PS1, Line 73: phy_add, uint8_t reg_add
pyh_adr and reg_adr
Done
https://review.coreboot.org/c/coreboot/+/63888/comment/e3e51cb8_56bffe8c PS1, Line 77: Status
status
Done
https://review.coreboot.org/c/coreboot/+/63888/comment/d031aee2_2975be5e PS1, Line 87: // Wait for MDIO frame transfer complete before reading MDIO DATA register
Please use the proper comment style.
Done
https://review.coreboot.org/c/coreboot/+/63888/comment/2898dd5b_cea4adcd PS1, Line 88: Status
status
Done
https://review.coreboot.org/c/coreboot/+/63888/comment/f10f1bf3_0c820ba5 PS1, Line 90: die("TSN GMII Busy. MIDO phy_add: 0x%x, Register 0x%x read invalid\n", : phy_add, reg_add);
Again, die() is a bit too hard here.
Done
https://review.coreboot.org/c/coreboot/+/63888/comment/f88e01f8_da4c1653 PS1, Line 93: BIOS_INFO
DEBUG or even SPEW would be better here.
Done