Change in coreboot[master]: soc/intel/ehl: Provide function to change PHY-to-MAC IRQ polarity
Attention is currently required from: Paul Menzel, Mario Scheithauer. Werner Zeh 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 5:
(12 comments)
File src/soc/intel/elkhartlake/chip.h:
https://review.coreboot.org/c/coreboot/+/63888/comment/d3b3713d_ce5b6e66 PS5, Line 481: change PHY-to-MAC IRQ polarity set Phy-to-MAC IRQ polarity to rising edge
https://review.coreboot.org/c/coreboot/+/63888/comment/0760a6f3_d539fde3 PS5, Line 482: bool PchTsnPhy2MacIrqActiveHigh; : bool PseTsnPhy2MacIrqActiveHigh[MAX_PSE_TSN_PORTS]; How about calling it {Pch,Pse}TsnPhyIRQEdge and using default (0) as falling edge while 1 as rising edge?
File src/soc/intel/elkhartlake/include/soc/tsn_gbe.h:
https://review.coreboot.org/c/coreboot/+/63888/comment/6cb1060f_01a41022 PS5, Line 11: #define TSN_GMII_TIMEOUT_MS 20 : : #define TSN_MAC_MDIO_ADR 0x200 /* MAC MDIO Address register */ : #define TSN_MAC_MDIO_ADR_MASK 0x03FF7F0E : #define TSN_MAC_PHYAD(pa) (pa << 21) /* Physical Layer Address */ : #define TSN_MAC_REGAD(rda) (rda << 16) /* Register/Device Address */ : #define TSN_MAC_CLK_TRAIL_4 (4 << 12) /* 4 Trailing Clocks */ : #define TSN_MAC_CSR_CLK_DIV_62 (1 << 8) /* 0001: CSR = 100-150 MHz; CSR/62 */ : #define TSN_MAC_OP_CMD_WRITE (1 << 2) /* GMII Operation Command Write */ : #define TSN_MAC_OP_CMD_READ (3 << 2) /* GMII Operation Command Read */ : #define TSN_MAC_GMII_BUSY (1 << 0) /* GMII Busy bit */ : : #define TSN_MAC_MDIO_ADHOC_ADR 0x15 /* MDIO - Adhoc PHY Sublayer Register */ : #define TSN_MAC_MDIO_GCR 0x0 /* Global Configuration Register */ : #define TSN_MAC_PHY2MAC_INTR_POL (1 << 6) /* PHY to MAC Interrupt Polarity bit */ : : #define TSN_MAC_MDIO_DATA 0x204 /* MAC MDIO Data register */ Align the TABs
https://review.coreboot.org/c/coreboot/+/63888/comment/03cbe401_69a1dc5a PS5, Line 33: phy_gmii_busy_status maybe better 'phy_gmii_ready()' to indicate that this function waits for the PHY to become ready?
File src/soc/intel/elkhartlake/tsn_gbe.c:
https://review.coreboot.org/c/coreboot/+/63888/comment/4e964f38_a475bb7d PS5, Line 49: = 0 Not needed since it will be overwritten on line 63.
https://review.coreboot.org/c/coreboot/+/63888/comment/0b48fe90_334f1e03 PS5, Line 57: transfer complete transfer to complete
https://review.coreboot.org/c/coreboot/+/63888/comment/83d7c50f_d90c3f06 PS5, Line 60: printk(BIOS_ERR, "TSN GMII Busy. MIDO phy_add: 0x%x, Register 0x%x " : "read invalid\n", phy_adr, reg_adr); Can we get this a bit shorter to fit the string at least on a single line?
https://review.coreboot.org/c/coreboot/+/63888/comment/04fdd77c_f325ee4c PS5, Line 64: printk(BIOS_DEBUG, "TSN MDIO Read phy_add: 0x%x, reg_add: 0x%x , " : "Value: 0x%x\n", phy_adr, reg_adr, mac_mdio_data); Same here.
https://review.coreboot.org/c/coreboot/+/63888/comment/8b2564b9_647134ab PS5, Line 70: void tsn_mdio_write(uint32_t base, uint8_t phy_adr, uint8_t reg_adr, : uint16_t mac_mdio_data) This fits on a single line.
https://review.coreboot.org/c/coreboot/+/63888/comment/53d28491_9f11e339 PS5, Line 84: printk(BIOS_ERR, "TSN GMII Busy. MIDO phy_add: 0x%x, Register 0x%x " : "read invalid\n", phy_adr, reg_adr); Same string length comment applies here.
https://review.coreboot.org/c/coreboot/+/63888/comment/aff8e982_5a1fed7e PS5, Line 95: uint16_t gcr_reg; Variable definition first.
https://review.coreboot.org/c/coreboot/+/63888/comment/ebb68d0f_cfc055e0 PS5, Line 117: if (config->PchTsnPhy2MacIrqActiveHigh) : irq_active_high = true; What about 'irq_active_high = !!config->PchTsnPhy2MacIrqActiveHigh;' ?
-
Werner Zeh (Code Review)