Peer review feedback (thanks Copilot)

Author: dgarske

.github/workflows/test-configs.yml

@@ -307,16 +307,21 @@ jobs:
     with:
       arch: riscv64
       config-file: ./config/examples/polarfire_mpfs250_qspi.config
+  # M-mode + LPDDR4 build: point LIBERO_FPGA_CONFIG_DIR at the in-tree
+  # CI stub so MPFS_DDR_INIT actually compiles in CI (the upstream config
+  # leaves the var empty so real boards must override it explicitly).
   microchip_mpfs250_m_test:
     uses: ./.github/workflows/test-build-riscv.yml
     with:
       arch: riscv64
       config-file: ./config/examples/polarfire_mpfs250_m.config
+      make-args: LIBERO_FPGA_CONFIG_DIR=tools/ci/mpfs_libero_stub
   microchip_mpfs250_m_qspi_test:
     uses: ./.github/workflows/test-build-riscv.yml
     with:
       arch: riscv64
       config-file: ./config/examples/polarfire_mpfs250_m_qspi.config
+      make-args: LIBERO_FPGA_CONFIG_DIR=tools/ci/mpfs_libero_stub
 
   raspi3_test:
     uses: ./.github/workflows/test-build.yml

docs/Targets.md

@@ -828,6 +828,13 @@ still produces a working M-mode wolfBoot without DDR. Add
 `-DDEBUG_DDR` to `CFLAGS_EXTRA` for verbose register-level traces
 during bring-up.
 
+A compile-only stub `tools/ci/mpfs_libero_stub/fpga_design_config.h`
+exists for GitHub Actions: every `LIBERO_SETTING_*` symbol referenced
+by the HAL is defined to `0` so the M-Mode + DDR build path stays
+under continuous integration. The stub is **not** runnable - real
+boards must point `LIBERO_FPGA_CONFIG_DIR` at the actual Libero / HSS
+output.
+
 Key build settings that differ between configurations:
 
 | Setting | SDCard | eMMC | QSPI | L2-LIM | M-Mode | M-Mode + DDR |

hal/mpfs250.c

@@ -279,16 +279,14 @@ static inline void mb(void)
     __asm__ volatile("fence iorw, iorw" ::: "memory");
 }
 
-/* Simple busy-loop delay
- * Approximately us microseconds at ~40MHz (early boot clock)
- * This is used before the timer is fully reliable */
+/* DDR-init delay.  Forwards to rdcycle-based udelay() so the effective
+ * delay tracks the current CPU frequency.  The previous implementation
+ * was a hardcoded ~40 MHz busy loop; after mss_pll_init() switches the
+ * CPU clock to ~600 MHz it ran ~15x too short, silently violating
+ * LPDDR4 reset / MR-write timing windows. */
 static void ddr_delay(uint32_t us)
 {
-    volatile uint32_t i;
-    /* At ~40MHz, ~10 loop iterations per microsecond */
-    for (i = 0; i < us * 10; i++) {
-        __asm__ volatile("nop");
-    }
+    udelay(us);
 }
 
 /* IOSCB Bank Controllers and DLL bases */
@@ -559,8 +557,13 @@ static int mss_pll_init(void)
                 }
             }
 
-            /* Reinitialize UART for new clock frequency */
+            /* Reinitialize UART for new clock frequency.  hal_uart_reinit
+             * is defined under #ifdef DEBUG_UART (the only path that uses
+             * the UART driver); skip the call when that block is absent so
+             * the build links cleanly without DEBUG_UART. */
+#ifdef DEBUG_UART
             hal_uart_reinit();
+#endif
             return 0;
         }
         if ((timeout % 100000) == 0) {
@@ -1953,18 +1956,37 @@ static int run_training(void)
     uint32_t div0_1_orig = DDR_PLL_REG(PLL_DIV_0_1);
     uint32_t div2_3_orig = DDR_PLL_REG(PLL_DIV_2_3);
     {
-        uint32_t div0 = div0_1_orig & 0x3F00UL;
-        uint32_t div1 = div0_1_orig & 0x3F000000UL;
-        uint32_t div2 = div2_3_orig & 0x3F00UL;
-        uint32_t div3 = div2_3_orig & 0x3F000000UL;
-        uint32_t mult = 2;
-
-        /* Double the dividers for MR writes */
-        DDR_PLL_REG(PLL_DIV_0_1) = (div0 | div1) * mult;
-        DDR_PLL_REG(PLL_DIV_2_3) = (div2 | div3) * mult;
-
-        /* Wait for PHY PLL to lock */
-        while ((DDRPHY_REG(PHY_PLL_CTRL_MAIN) & 0x2000000UL) == 0) {}
+        /* Each register holds two 6-bit divider fields at bits [13:8] and
+         * [29:24].  Extract numeric values, double (LPDDR4 MR writes need a
+         * slower PLL output), clamp to the 6-bit field max so the doubled
+         * value cannot overflow into adjacent bits, then re-encode while
+         * preserving all other bits of the original register. */
+        uint32_t f0 = (div0_1_orig >> 8)  & 0x3FUL;
+        uint32_t f1 = (div0_1_orig >> 24) & 0x3FUL;
+        uint32_t f2 = (div2_3_orig >> 8)  & 0x3FUL;
+        uint32_t f3 = (div2_3_orig >> 24) & 0x3FUL;
+
+        f0 = (f0 > 0x1FUL) ? 0x3FUL : (f0 << 1);
+        f1 = (f1 > 0x1FUL) ? 0x3FUL : (f1 << 1);
+        f2 = (f2 > 0x1FUL) ? 0x3FUL : (f2 << 1);
+        f3 = (f3 > 0x1FUL) ? 0x3FUL : (f3 << 1);
+
+        DDR_PLL_REG(PLL_DIV_0_1) = (div0_1_orig & ~0x3F003F00UL) |
+            (f0 << 8) | (f1 << 24);
+        DDR_PLL_REG(PLL_DIV_2_3) = (div2_3_orig & ~0x3F003F00UL) |
+            (f2 << 8) | (f3 << 24);
+
+        /* Wait for PHY PLL to lock.  Bounded at 100 ms so a bad refclk,
+         * power glitch, or mis-programmed divider cannot brick boot in
+         * an infinite spin -- bail out so the caller can fail cleanly. */
+        timeout = 100000;
+        while ((DDRPHY_REG(PHY_PLL_CTRL_MAIN) & 0x2000000UL) == 0) {
+            if (timeout-- == 0) {
+                wolfBoot_printf("DDR: PHY PLL lock timeout (post-doubling)\n");
+                return -1;
+            }
+            udelay(1);
+        }
         ddr_delay(5000);
 
         /* Reset delay lines after frequency change */
@@ -2075,8 +2097,15 @@ static int run_training(void)
     DDR_PLL_REG(PLL_DIV_0_1) = div0_1_orig;
     DDR_PLL_REG(PLL_DIV_2_3) = div2_3_orig;
 
-    /* Wait for PHY PLL to lock */
-    while ((DDRPHY_REG(PHY_PLL_CTRL_MAIN) & 0x2000000UL) == 0) {}
+    /* Wait for PHY PLL to lock; bounded as in the post-doubling wait above. */
+    timeout = 100000;
+    while ((DDRPHY_REG(PHY_PLL_CTRL_MAIN) & 0x2000000UL) == 0) {
+        if (timeout-- == 0) {
+            wolfBoot_printf("DDR: PHY PLL lock timeout (post-restore)\n");
+            return -1;
+        }
+        udelay(1);
+    }
     ddr_delay(500);
 
     /* Reset delay lines after frequency change */

src/boot_riscv.c

@@ -133,7 +133,22 @@ static void handle_external_interrupt(void)
 }
 #endif /* PLIC_BASE */
 
+/* Backward-compatible 3-arg weak hook.  Out-of-tree platforms that override
+ * this in their own HAL keep working; the asm trap entry now calls
+ * handle_trap_ex, which forwards here so legacy overrides still execute. */
 unsigned long WEAKFUNCTION handle_trap(unsigned long cause, unsigned long epc,
+    unsigned long tval)
+{
+    (void)cause;
+    (void)tval;
+    return epc;
+}
+
+/* Regs-aware trap dispatch -- called directly from src/vector_riscv.S.
+ * Override this (also weak) to take full control including the saved
+ * register frame.  Falls through to weak handle_trap so legacy 3-arg
+ * overrides still run. */
+unsigned long WEAKFUNCTION handle_trap_ex(unsigned long cause, unsigned long epc,
     unsigned long tval, unsigned long *regs)
 {
     last_cause = cause;
@@ -225,7 +240,8 @@ unsigned long WEAKFUNCTION handle_trap(unsigned long cause, unsigned long epc,
     /* Synchronous exceptions are not handled - just record them */
 #endif
 
-    return epc;
+    /* Forward to the legacy 3-arg hook so out-of-tree overrides still run */
+    return handle_trap(cause, epc, tval);
 }
 
 /* ============================================================================

src/sdhci.c

@@ -1428,11 +1428,13 @@ int sdhci_init(void)
     /* Call platform-specific initialization (clocks, resets, pin mux) */
     sdhci_platform_init();
 
+#ifdef DEBUG_SDHCI
     /* Dump capability + presence registers so the bring-up log shows the
      * controller's reported base clock and whether a card was detected. */
     wolfBoot_printf("SDHCI: SRS09=0x%08X SRS16=0x%08X SRS17=0x%08X\n",
         SDHCI_REG(SDHCI_SRS09), SDHCI_REG(SDHCI_SRS16),
         SDHCI_REG(SDHCI_SRS17));
+#endif
 
     /* Allow controller to settle after platform init (slot type change,
      * soft reset, clock configuration). Without this, the controller may

src/vector_riscv.S

@@ -31,7 +31,10 @@
 
 #if __riscv_xlen == 64
 
-/* RV64: Save all caller-saved registers and call handle_trap */
+/* RV64: Save all caller-saved registers and call handle_trap_ex.
+ * handle_trap_ex (in src/boot_riscv.c) is the regs-aware weak hook;
+ * it forwards to the legacy 3-arg weak handle_trap so out-of-tree
+ * platform overrides keep working. */
 .macro trap_entry
     addi sp, sp, -32 * REGBYTES
     STORE x1,   1 * REGBYTES(sp)
@@ -71,7 +74,7 @@
     csrr a2, MODE_PREFIX(tval)
 
     mv a3, sp
-    jal handle_trap
+    jal handle_trap_ex
     csrw MODE_PREFIX(epc), a0
 
 .endm