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
