fix(igc): stabilize i225 phy bring-up

awkernel_drivers/src/pcie/intel/igc.rs

@@ -251,12 +251,11 @@ impl Igc {
         Ok(igc)
     }
 
-    fn intr(&self, _irq: Option<u16>) -> Result<(), IgcDriverErr> {
+    fn intr(&self, irq: Option<u16>) -> Result<(), IgcDriverErr> {
         // TODO: Handle Tx/Rx interrupts.
 
         let mut inner = self.inner.read();
         let igc_icr = read_reg(&inner.info, igc_regs::IGC_ICR)?;
-
         if (igc_icr & igc_defines::IGC_ICR_LSC) != 0 {
             // Link status change interrupt.
             drop(inner);
@@ -265,6 +264,13 @@ impl Igc {
                 inner.igc_intr_link()?;
             }
             inner = self.inner.read();
+        } else if irq.is_none() {
+            drop(inner);
+            {
+                let mut inner = self.inner.write();
+                inner.igc_poll_link()?;
+            }
+            inner = self.inner.read();
         }
 
         write_reg(&inner.info, igc_regs::IGC_IMS, igc_defines::IGC_IMS_LSC)?;
@@ -802,6 +808,16 @@ impl IgcInner {
         )
     }
 
+    /// Polls link status when called from a tick-driven path (i.e. `intr` is
+    /// invoked without a real IRQ).  Forces `get_link_status = true` so that
+    /// `igc_intr_link` re-checks the hardware even if a link-change interrupt
+    /// was not observed.
+    #[inline(always)]
+    fn igc_poll_link(&mut self) -> Result<(), IgcDriverErr> {
+        self.hw.mac.get_link_status = true;
+        self.igc_intr_link()
+    }
+
     fn igc_iff(&mut self) -> Result<(), IgcDriverErr> {
         use igc_regs::*;
 
@@ -1154,6 +1170,8 @@ fn igc_update_link_status(
     hw: &mut IgcHw,
     link_info: &mut LinkInfo,
 ) -> Result<(), IgcDriverErr> {
+    let previous_status = link_info.link_status;
+
     if hw.mac.get_link_status {
         ops.check_for_link(info, hw)?;
     }
@@ -1164,6 +1182,7 @@ fn igc_update_link_status(
             link_info.link_speed = Some(speed);
             link_info.link_duplex = Some(duplex);
             link_info.link_active = true;
+            log::debug!("igc: link up: speed={speed:?}, duplex={duplex:?}");
         }
 
         if link_info.link_duplex == Some(IgcDuplex::Full) {
@@ -1176,10 +1195,15 @@ fn igc_update_link_status(
             link_info.link_speed = None;
             link_info.link_duplex = None;
             link_info.link_active = false;
+            log::debug!("igc: link down");
         }
         LinkStatus::Down
     };
 
+    if previous_status != link_info.link_status {
+        log::info!("igc: link status changed: {}", link_info.link_status);
+    }
+
     Ok(())
 }
 

awkernel_drivers/src/pcie/intel/igc/i225.rs

@@ -5,7 +5,7 @@ use crate::pcie::{
         igc_mac::igc_config_fc_after_link_up_generic,
         igc_phy::{
             igc_check_downshift_generic, igc_phy_has_link_generic, igc_setup_copper_link_generic,
-            IGC_I225_PHPM, IGC_I225_PHPM_GO_LINKD,
+            IGC_I225_PHPM, IGC_I225_PHPM_GO_LINKD, IGC_I225_PHPM_ULP,
         },
         IgcMacType,
     },
@@ -30,7 +30,7 @@ use super::{
     igc_nvm::{acquire_nvm, igc_read_nvm_eerd, igc_validate_nvm_checksum_generic},
     igc_phy::{
         igc_get_phy_id, igc_phy_hw_reset_generic, igc_power_up_phy_copper, igc_read_phy_reg_gpy,
-        igc_write_phy_reg_gpy,
+        igc_sync_mdic_phy_addr, igc_write_phy_reg_gpy,
     },
     igc_regs::*,
     read_reg, write_flush, write_reg, IgcDriverErr,
@@ -341,7 +341,11 @@ fn igc_reset_hw_i225(
     let ctrl = read_reg(info, IGC_CTRL)?;
     write_reg(info, IGC_CTRL, ctrl | IGC_CTRL_DEV_RST)?;
 
-    igc_get_auto_rd_done_generic(info)?;
+    if let Err(e) = igc_get_auto_rd_done_generic(info) {
+        // Matching the BSD drivers here avoids failing bring-up on parts
+        // without usable NVM auto-read completion while still surfacing it.
+        log::debug!("igc: auto read done did not complete after MAC reset: {e:?}");
+    }
 
     // Clear any pending interrupt events.
     write_reg(info, IGC_IMC, 0xffffffff)?;
@@ -544,6 +548,13 @@ fn igc_init_phy_params_i225(
 
     phy.autoneg_mask = AUTONEG_ADVERTISE_SPEED_DEFAULT_2500;
     phy.reset_delay_us = 100;
+    let phy_addr = igc_sync_mdic_phy_addr(info, hw, 1)?;
+
+    let mut phpm = read_reg(info, IGC_I225_PHPM)?;
+    phpm &= !(IGC_I225_PHPM_GO_LINKD | IGC_I225_PHPM_ULP);
+    write_reg(info, IGC_I225_PHPM, phpm)?;
+
+    ops.power_up(info, hw)?;
 
     // Make sure the PHY is in a good state. Several people have reported
     // firmware leaving the PHY's page select register set to something
@@ -553,6 +564,7 @@ fn igc_init_phy_params_i225(
 
     igc_get_phy_id(ops, info, hw)?;
     hw.phy.phy_type = IgcPhyType::I225;
+    log::debug!("igc: initialized PHY address {phy_addr}");
 
     Ok(())
 }

awkernel_drivers/src/pcie/intel/igc/igc_phy.rs

@@ -22,6 +22,8 @@ const IGP01IGC_PHY_PAGE_SELECT: u32 = 0x1F; // Page Select
 const BM_PHY_PAGE_SELECT: u32 = 22; // Page Select for BM
 const IGP_PAGE_SHIFT: u32 = 5;
 const PHY_REG_MASK: u32 = 0x1F;
+const IGC_MDICNFG_PHY_MASK: u32 = 0x03E00000;
+const IGC_MDICNFG_PHY_SHIFT: u32 = 21;
 pub(super) const IGC_I225_PHPM: usize = 0x0E14; // I225 PHY Power Management
 const IGC_I225_PHPM_DIS_1000_D3: u32 = 0x0008; // Disable 1G in D3
 const IGC_I225_PHPM_LINK_ENERGY: u32 = 0x0010; // Link Energy Detect
@@ -30,10 +32,35 @@ const IGC_I225_PHPM_DIS_1000: u32 = 0x0040; // Disable 1G globally
 const IGC_I225_PHPM_SPD_B2B_EN: u32 = 0x0080; // Smart Power Down Back2Back
 const IGC_I225_PHPM_RST_COMPL: u32 = 0x0100; // PHY Reset Completed
 const IGC_I225_PHPM_DIS_100_D3: u32 = 0x0200; // Disable 100M in D3
-const IGC_I225_PHPM_ULP: u32 = 0x0400; // Ultra Low-Power Mode
+pub(super) const IGC_I225_PHPM_ULP: u32 = 0x0400; // Ultra Low-Power Mode
 const IGC_I225_PHPM_DIS_2500: u32 = 0x0800; // Disable 2.5G globally
 const IGC_I225_PHPM_DIS_2500_D3: u32 = 0x1000; // Disable 2.5G in D3
 
+/// Reads the PHY address from MDICNFG, falls back to `default_addr` if the
+/// field is zero, writes the resolved address back to MDICNFG, and stores it
+/// in `hw.phy.addr`.  The default value of 1 matches the hardware power-on
+/// default and is consistent with the BSD igc driver.
+pub(super) fn igc_sync_mdic_phy_addr(
+    info: &mut PCIeInfo,
+    hw: &mut IgcHw,
+    default_addr: u32,
+) -> Result<u32, IgcDriverErr> {
+    let mdicnfg = read_reg(info, IGC_MDICNFG)?;
+    let phy_addr = (mdicnfg & IGC_MDICNFG_PHY_MASK) >> IGC_MDICNFG_PHY_SHIFT;
+    let phy_addr = if phy_addr == 0 {
+        default_addr
+    } else {
+        phy_addr
+    };
+
+    let mdicnfg = (mdicnfg & !IGC_MDICNFG_PHY_MASK) | (phy_addr << IGC_MDICNFG_PHY_SHIFT);
+    write_reg(info, IGC_MDICNFG, mdicnfg)?;
+    write_flush(info)?;
+    hw.phy.addr = phy_addr;
+
+    Ok(phy_addr)
+}
+
 /// Reads the MDI control register in the PHY at offset and stores the
 /// information read to data.
 fn igc_read_phy_reg_mdic(
@@ -250,7 +277,7 @@ fn acquire_phy<F, R>(
     f: F,
 ) -> Result<R, IgcDriverErr>
 where
-    F: Fn(&dyn IgcPhyOperations, &mut PCIeInfo, &IgcHw) -> Result<R, IgcDriverErr>,
+    F: Fn(&dyn IgcPhyOperations, &mut PCIeInfo, &mut IgcHw) -> Result<R, IgcDriverErr>,
 {
     IgcPhyOperations::acquire(ops, info, hw)?;
     let result = f(ops, info, hw);
@@ -267,6 +294,9 @@ pub(super) fn igc_phy_hw_reset_generic(
     info: &mut PCIeInfo,
     hw: &mut IgcHw,
 ) -> Result<(), IgcDriverErr> {
+    const PHY_RESET_POLL_TIGHT_LOOPS: usize = 150;
+    const PHY_RESET_POLL_BACKOFF_MSEC: usize = 100;
+
     match ops.check_reset_block(info) {
         Err(IgcDriverErr::BlkPhyReset) => {
             return Ok(());
@@ -277,29 +307,65 @@ pub(super) fn igc_phy_hw_reset_generic(
         _ => (),
     }
 
-    acquire_phy(ops, info, hw, |_, info, hw| {
-        let _phpm = read_reg(info, IGC_I225_PHPM)?;
-
+    acquire_phy(ops, info, hw, |_ops, info, hw| {
         let ctrl = read_reg(info, IGC_CTRL)?;
-        write_reg(info, IGC_CTRL, ctrl | IGC_CTRL_PHY_RST)?;
-        write_flush(info)?;
-
-        wait_microsec(hw.phy.reset_delay_us as u64);
-
-        write_reg(info, IGC_CTRL, ctrl)?;
-        write_flush(info)?;
+        let mut phpm = read_reg(info, IGC_I225_PHPM)?;
+
+        for attempt in 0..2 {
+            // Firmware can leave the PHY in go-link-down or ULP state, which
+            // prevents the reset-complete bit from asserting reliably.
+            phpm &= !(IGC_I225_PHPM_GO_LINKD | IGC_I225_PHPM_ULP);
+            write_reg(info, IGC_I225_PHPM, phpm)?;
+            write_flush(info)?;
+
+            write_reg(info, IGC_CTRL, ctrl | IGC_CTRL_PHY_RST)?;
+            write_flush(info)?;
+
+            wait_microsec(hw.phy.reset_delay_us as u64);
+
+            write_reg(info, IGC_CTRL, ctrl)?;
+            write_flush(info)?;
+
+            wait_microsec(150);
+
+            // Some firmware leaves RST_COMPL deasserted even when the PHY is
+            // already responsive over MDIC. Keep a short tight poll for the
+            // fast-success case, then fall back to millisecond backoff to
+            // avoid burning CPU during bring-up.
+            for _ in 0..PHY_RESET_POLL_TIGHT_LOOPS {
+                phpm = read_reg(info, IGC_I225_PHPM)?;
+                if phpm & IGC_I225_PHPM_RST_COMPL != 0 {
+                    return Ok(());
+                }
+                wait_microsec(1);
+            }
 
-        wait_microsec(150);
+            for _ in 0..PHY_RESET_POLL_BACKOFF_MSEC {
+                phpm = read_reg(info, IGC_I225_PHPM)?;
+                if phpm & IGC_I225_PHPM_RST_COMPL != 0 {
+                    return Ok(());
+                }
+                wait_millisec(1);
+            }
 
-        for _ in 0..10000 {
-            let phpm = read_reg(info, IGC_I225_PHPM)?;
-            wait_microsec(1);
-            if phpm & IGC_I225_PHPM_RST_COMPL != 0 {
-                return Ok(());
+            if attempt == 0 {
+                wait_millisec(1);
             }
         }
 
-        log::debug!("Timeout expired after a phy reset");
+        if igc_read_phy_reg_mdic(info, hw, PHY_ID1).is_ok()
+            && igc_read_phy_reg_mdic(info, hw, PHY_ID2).is_ok()
+        {
+            log::debug!(
+                "PHY reset completion bit did not assert, but PHY responded: ctrl={ctrl:#010x}, phpm={phpm:#010x}"
+            );
+            return Ok(());
+        }
+
+        let status = read_reg(info, IGC_STATUS).unwrap_or(0);
+        log::warn!(
+            "Timeout expired after a phy reset: ctrl={ctrl:#010x}, status={status:#010x}, phpm={phpm:#010x}"
+        );
 
         Ok(())
     })