Hi Hakan, I am grateful to be able to collaborate. I review your branch and submit the next step. Thank you for the detail of your comments
Diego El vie, 4 sept 2026 a la(s) 3:45 p.m., Hakan Candar ([email protected]) escribió: > Hi Diego, > > Thanks again, for your patch. I split it in half and applied > the UART half with minor changes. For the FDT/DTB parser, > I am going to reuse Sergey's implementation under wip-aarch64; > mainly to prevent duplication of work, but also because it is > more feature complete and architectured without platform-specific > assumptions. > > I also rebased my branch against latest upstream. You will want > to update your development branch against mine. > > Since I already applied your patch, you don't need to send a v2. > However, I am providing this review as a feedback. > > Let's start with the commit message: > > > Add an NS16550 UART driver (uart.c/uart.h) that provides polled > > input/output for early boot and the Mach console subsystem. The > > driver supports the QEMU virt machine (0x10000000) and the Allwinner > > D1 / C906 (0x02500000), with the base address selectable at compile > > time via UART_BASE. > > > > The console interface (uart_cnprobe, uart_cninit, uart_cnputc, > > uart_cngetc) is wired into cons_conf.c so that Mach's printf and > > getc/putc work over the serial port from the moment the kernel > > starts executing. > > > > Add a minimal FDT parser (fdt.c/fdt.h) that walks the flattened > > device tree passed by OpenSBI in a0 to extract memory regions and > > the kernel command line. The parser handles big-endian DTB fields, > > variable address/size cell widths, and the /chosen/bootargs property. > > This is intentionally minimal — only the subset needed for early > > boot is implemented. > > > > Update riscv64/Makefrag.am to include the new source files. > > > > Signed-off-by: Diego Meretta <[email protected]> > > The message is detailed and communicates what changed without much > ambiguity, however it is a bit verbose and goes into particulars > of implementation details. Also, it doesn't prefix the title with the > changed subsystem, which in our case is riscv64. > > I changed it as follows. This follows GNU ChangeLog style: > > < riscv64: Add UART console support. > < > < * riscv64/riscv64/uart.c: Add a polled NS16550 driver and console > interface. > < * riscv64/riscv64/uart.h: Declare the UART interface and platform base > addresses. > < * riscv64/riscv64/cons_conf.c: Register the UART console. > < * riscv64/Makefrag.am: Register the UART sources. > > Let's continue with the contents of the UART half: > > > diff --git a/riscv64/riscv64/uart.c b/riscv64/riscv64/uart.c > > new file mode 100644 > > index 00000000..3b5a1603 > > --- /dev/null > > +++ b/riscv64/riscv64/uart.c > > [...] > > +void > > +uart_init(void) > > +{ > > + /* Disable interrupts */ > > + uart_write(UART_IER, 0); > > + > > + /* Enable FIFO, clear TX and RX */ > > + uart_write(UART_FCR, FCR_FIFO_EN | FCR_FIFO_CLR); > > + > > + /* 8N1, no DLAB */ > > + uart_write(UART_LCR, LCR_8N1); > > + > > + /* No modem control */ > > + uart_write(UART_MCR, 0); > > + > > + /* Enable received data interrupt */ > > + uart_write(UART_IER, IER_ERDAI); > > +} > > We don't have the interrupt handler ready yet, so I dropped that part > where we enable the receive interrupt in the next commit. > > > +/* > > + * Console interface: probe > > + * Always report as available — we know the UART is there on QEMU virt. > > + */ > > I have not come across (or haven't noticed) non-ASCII characters elsewhere > in the codebase, so I am of the opinion that we should not introduce such > characters for consistency. I replaced the em dash (—) with a hyphen (-). > > > +/* > > + * Console interface: putc > > + */ > > +int > > +uart_cnputc(dev_t dev, int c) > > +{ > > + if (c == '\n') > > + uart_putc('\r'); > > + uart_putc(c); > > + return 0; > > +} > > The generic `cnputc` under `device/cons.c` already adds carriage returns, > so this would duplicate them. I dropped that part in the next commit. > > That's all. Thanks for your patch! > > Hakan >
