Conversation
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Refactors duplicated PC16552D/NS16550 UART console implementations across NXP QorIQ (PPC) and LS1028A (AArch64) targets into a shared instance-based NS16550 driver, with unit tests validating the shared behavior.
Changes:
- Introduces a generic instance-based NS16550 UART driver and optional platform MMIO accessors.
- Migrates NXP PPC and LS1028A console paths to the shared driver while keeping a minimal stage1-only fallback on size-constrained builds.
- Adds host-based unit tests exercising divisor math, register spacing, CRLF handling, and bounded waits.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/unit-tests/unit-ns16550.c | Adds host-emulated unit tests for the new NS16550 driver |
| tools/unit-tests/Makefile | Registers and builds the new NS16550 unit test |
| test-app/Makefile | Ensures test-app links the shared NS16550 object for affected targets |
| stage1/Makefile | Adds build rule/vpath support for sources under hal/uart |
| options.mk | Adds NS16550 build option to include the driver independent of DEBUG_UART |
| include/nxp_ppc_io.h | Adds PPC-specific ordered MMIO accessor hooks for byte IO |
| include/ns16550.h | Adds the public instance-based NS16550 driver API and IO hooks |
| hal/uart/ns16550.c | Implements the shared instance-based NS16550 driver |
| hal/nxp_t2080.h | Removes duplicated UART register macros now covered by shared driver |
| hal/nxp_t2080.c | Replaces UART_MCR macro usage with explicit DUART offset write |
| hal/nxp_t10xx.c | Removes duplicated UART register macros now covered by shared driver |
| hal/nxp_ppc.c | Switches PPC loader console to shared driver; keeps stage1 minimal UART |
| hal/nxp_p1021.c | Removes duplicated UART register macros now covered by shared driver |
| hal/nxp_ls1028a.h | Fixes DUART stride and removes duplicated UART register macros |
| hal/nxp_ls1028a.c | Switches LS1028A console to shared driver (no floating point divisor) |
| arch.mk | Links NS16550 object and sets PPC IO hook macro where needed |
| .gitignore | Ignores the newly added unit-ns16550 test binary |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+121
to
+127
| /* The latch is 16 bits and 0 means divide-by-65536, so out of range is a | ||
| * bad clock/baud pairing rather than something to clamp. Rounded, not | ||
| * truncated: 0.5% baud error instead of 1.4% at 115200 on 99.999 MHz. */ | ||
| div = (clk + (baud * 8U)) / (baud * 16U); | ||
| if (div == 0U || div > 0xFFFFU) { | ||
| return NS16550_ERR_CLK; | ||
| } |
Comment on lines
+30
to
+41
| /* MMIO access. A port whose bus needs more than a volatile access supplies | ||
| * these via -DNS16550_IO_H='"myport_io.h"' - PowerPC QorIQ must keep its | ||
| * sync/twi/isync and sync/eieio sequences or MMIO ordering is lost. */ | ||
| #ifdef NS16550_IO_H | ||
| #include NS16550_IO_H | ||
| #endif | ||
| #ifndef NS16550_RD8 | ||
| #define NS16550_RD8(a) (*(volatile uint8_t*)(a)) | ||
| #endif | ||
| #ifndef NS16550_WR8 | ||
| #define NS16550_WR8(a, v) (*(volatile uint8_t*)(a) = (uint8_t)(v)) | ||
| #endif |
Comment on lines
+35
to
+36
| #include "../../hal/uart/ns16550.c" | ||
|
|
Comment on lines
+46
to
+50
| return (volatile uint32_t *)(regs + d->reg_off + (r << d->reg_shift)); | ||
| } | ||
| static volatile uint8_t *reg8(const struct ns16550_dev *d, uint32_t r) | ||
| { | ||
| return (volatile uint8_t *)(regs + d->reg_off + (r << d->reg_shift)); |
Comment on lines
+1713
to
+1715
| * pre-bootm value. MCR is byte offset 4; the rest of the layout is in | ||
| * include/ns16550.h. */ | ||
| set8((volatile unsigned char*)(UART_BASE(0) + 4), 0x03); |
dgarske
force-pushed
the
ns16550_dedup
branch
from
September 18, 2026 01:28
95220ae to
168d504
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
wolfBoot carried four copies of the same PC16552D console: a shared PowerPC implementation in
hal/nxp_ppc.creaching T2080, T1024, T1040 and P1021, a verbatim fork of it inhal/nxp_ls1028a.cfor AArch64, and three byte-identical register tables innxp_t2080.h,nxp_t10xx.candnxp_p1021.c. All of them now use one driver.Bugs fixed
Two latent bugs in the LS1028A fork, which the shared code removes:
UART_BASE(n)used a decimal100where the DUART stride is0x100. Harmless only becauseUART_SELis 0.Hardware testing
LS1028ARDB (the AArch64 fork) the console output is byte-for-byte identical before and after.
T1040D4RDB (the shared
hal/nxp_ppc.cpath) every wolfBoot line is identical through HAL init, QE/FMAN microcode upload, PHY enumeration, multicore start, image verification, the FDT fixups and handoff; the only deltas are the test app's own size, since it links the refactored HAL.T2080 and P1021 are build-only, including
OS_64BIT=1: Unit tests cover the divisor arithmetic,reg-shift/reg-offsetaddressing, CRLF and the bounded busy-waits against an emulated register block.