Attention is currently required from: Mario Scheithauer. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63888 )
Change subject: soc/intel/elkhartlake: Provide function to change PHY to MAC IRQ polarity ......................................................................
Patch Set 1:
(22 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/63888/comment/da395104_81e58ec5 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 internal MAC which is set up here. In addition, the headline seems to be one char too long.
https://review.coreboot.org/c/coreboot/+/63888/comment/7f8428b9_97dd9769 PS1, Line 9: It may be that a connected PHY requires an inverting of the interrupt : signal. Maybe: 'Based on the mainboard wiring it could be necessary to change the interrupt polarity.'
File src/soc/intel/elkhartlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/63888/comment/abee1bd4_026f4d4e 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 current one. Something like IRQ_ACTIVE_HIGH and IRQ_ACTIVE_LOW would have the benefit that the user is sure witch level is set up.
https://review.coreboot.org/c/coreboot/+/63888/comment/90c3234a_49fdc481 PS1, Line 251: This invert Maybe: 'Setting this switch inverts'
File src/soc/intel/elkhartlake/include/soc/tsn_gbe.h:
https://review.coreboot.org/c/coreboot/+/63888/comment/f748aa11_0a8d65d5 PS1, Line 12: TSN_GMII_DELAY Add _US to the define to indicated that the timout is in microsecond units?
https://review.coreboot.org/c/coreboot/+/63888/comment/3f6814dc_8888857b PS1, Line 14: ADDR Maybe 'ADR'?
https://review.coreboot.org/c/coreboot/+/63888/comment/9900bf8f_37145fd5 PS1, Line 24: ADD 'ADR' as you mead address here? ADD just look weird here.
https://review.coreboot.org/c/coreboot/+/63888/comment/b2a83c9a_af72cf67 PS1, Line 26: 0x40 Once you have started to use the shift syntax, why not continue here? (1 << 6) would be more consistent across the file.
File src/soc/intel/elkhartlake/tsn_gbe.c:
https://review.coreboot.org/c/coreboot/+/63888/comment/0b090273_c871685f 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 defining the overall timeout value you need.
https://review.coreboot.org/c/coreboot/+/63888/comment/ee13ce3b_d61e9503 PS1, Line 48: uint8_t phy_add, uint8_t reg_add I would rather call them pyh_adr and reg_adr here.
https://review.coreboot.org/c/coreboot/+/63888/comment/82bdbb85_b899a894 PS1, Line 52: Status status
https://review.coreboot.org/c/coreboot/+/63888/comment/16e9be1c_263b2720 PS1, Line 55: mac_mdio_address = read32((void *)(io_mem_base + TSN_MAC_MDIO_ADDR)); : mac_mdio_address &= (uint32_t)~(TSN_MAC_MDIO_ADDR_MASK); : mac_mdio_address |= TSN_MAC_PHYAD(phy_add) | TSN_MAC_REGAD(reg_add) : | TSN_MAC_CLK_TRAIL_4 | TSN_MAC_CSR_CLK_DIV_62 : | TSN_MAC_OP_CMD_READ | TSN_MAC_GMII_BUSY; : write32((void *)(io_mem_base + TSN_MAC_MDIO_ADDR), mac_mdio_address) You could use clrsetbits() here instead, but up to you.
https://review.coreboot.org/c/coreboot/+/63888/comment/49f4458e_04310a52 PS1, Line 62: // Wait for MDIO frame transfer complete before reading MDIO DATA register Please use the propper comment style here.
https://review.coreboot.org/c/coreboot/+/63888/comment/4487ff89_1fc888cf 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.
https://review.coreboot.org/c/coreboot/+/63888/comment/b5e8d025_a8090cf5 PS1, Line 68: BIOS_INFO This one should be on DEBUG level (or even on SPEW) I guess, or?
https://review.coreboot.org/c/coreboot/+/63888/comment/10d7be38_817ee7b2 PS1, Line 73: phy_add, uint8_t reg_add pyh_adr and reg_adr
https://review.coreboot.org/c/coreboot/+/63888/comment/a1e2344b_d596d404 PS1, Line 77: Status status
https://review.coreboot.org/c/coreboot/+/63888/comment/d908f80f_684888d2 PS1, Line 79: write16((void *)(io_mem_base + TSN_MAC_MDIO_DATA), mac_mdio_data); : mac_mdio_address = read32((void *)(io_mem_base + TSN_MAC_MDIO_ADDR)); : mac_mdio_address &= (uint32_t)~(TSN_MAC_MDIO_ADDR_MASK); : mac_mdio_address |= TSN_MAC_PHYAD(phy_add) | TSN_MAC_REGAD(reg_add) : | TSN_MAC_CLK_TRAIL_4 | TSN_MAC_CSR_CLK_DIV_62 : | TSN_MAC_OP_CMD_WRITE | TSN_MAC_GMII_BUSY; : write32((void *)(io_mem_base + TSN_MAC_MDIO_ADDR), mac_mdio_address); The same clrsetbist() commetn applies here as well.
https://review.coreboot.org/c/coreboot/+/63888/comment/14bfe97e_e3906fdc PS1, Line 87: // Wait for MDIO frame transfer complete before reading MDIO DATA register Please use the proper comment style.
https://review.coreboot.org/c/coreboot/+/63888/comment/67700302_fd1af9a2 PS1, Line 88: Status status
https://review.coreboot.org/c/coreboot/+/63888/comment/6f71bf65_4f8f91cd 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.
https://review.coreboot.org/c/coreboot/+/63888/comment/fc607a4f_ade15079 PS1, Line 93: BIOS_INFO DEBUG or even SPEW would be better here.