On Wed, Jul 01, 2026 at 01:34:27PM +0930, Joel Stanley wrote:
> On Wed, 17 Jun 2026 at 04:31, Kuan-Wei Chiu <[email protected]> wrote:
> >
> > Add the dw8250 uart support. This hardware is a widely used 16550A
> > derivative that includes additional registers.
> >
> > Without this specific device support, the Linux 8250_dw driver fails to
> > probe the extended registers (UCV, CPR, etc.), which are essential for
> > correct feature detection:
>
> Nice. The atlantis board works around this by adding an uimp region,
> but having a real model is even better.
>
> Some minor comments below.
>
> > [ 0.293566] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> > [ 0.306929] Oops - store (or AMO) access fault [#1]
> > [ 0.307020] Modules linked in:
> > [ 0.307192] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 7.0.0 #1
> > PREEMPTLAZY
> > [ 0.307250] Hardware name: Milk-V Duo (DT)
> > [ 0.307294] epc : dw8250_setup_port+0x22/0x520
> > [ 0.307504] ra : dw8250_probe+0x57e/0x5b8
> > [ 0.307518] epc : ffffffff80708dd6 ra : ffffffff8070a49e sp :
> > ffffffc60000b820
> > [ 0.307525] gp : ffffffff81a32ba8 tp : ffffffd602180cc0 t0 :
> > 0000000000000073
> > [ 0.307533] t1 : 000000000000006c t2 : 0000000000000000 s0 :
> > ffffffc60000b830
> > [ 0.307539] s1 : ffffffd6028c8640 a0 : ffffffc60000b848 a1 :
> > ffffffff813162c1
> > [ 0.307546] a2 : ffffffff813162c0 a3 : ffffffd6028c8640 a4 :
> > ffffffc60002d0b4
> > [ 0.307552] a5 : 0000000000000001 a6 : 0000000000000094 a7 :
> > 0000000000000060
> > [ 0.307558] s2 : ffffffd60225d410 s3 : ffffffd60225d400 s4 :
> > 0000000000000000
> > [ 0.307573] s5 : ffffffff80e31a48 s6 : 0000000000000008 s7 :
> > 0000000000000000
> > [ 0.307584] s8 : 0000000000000149 s9 : 0000000000000000 s10:
> > 0000000000000000
> > [ 0.307590] s11: 0000000000000000 t3 : ffffffd602007c00 t4 :
> > ffffffff81601540
> > [ 0.307604] t5 : 0000000000000003 t6 : ffffffd602a42f82 ssp :
> > 0000000000000000
> > [ 0.307611] status: 0000000200000120 badaddr: ffffffc60002d0b4 cause:
> > 0000000000000007
> > [ 0.307652] [<ffffffff80708dd6>] dw8250_setup_port+0x22/0x520
> > [ 0.307695] [<ffffffff8070a49e>] dw8250_probe+0x57e/0x5b8
> > [ 0.307702] [<ffffffff80731f0e>] platform_probe+0x46/0x80
> > [ 0.307708] [<ffffffff8072f67c>] really_probe+0x84/0x22c
> > [ 0.307715] [<ffffffff8072f880>] __driver_probe_device+0x5c/0xd4
> > [ 0.307721] [<ffffffff8072f9be>] driver_probe_device+0x2e/0xf4
> > [ 0.307727] [<ffffffff8072fbe6>] __driver_attach+0x6e/0x14c
> > [ 0.307734] [<ffffffff8072d5f0>] bus_for_each_dev+0x60/0xb0
> > [ 0.307740] [<ffffffff8072f1b2>] driver_attach+0x1a/0x24
> > [ 0.307746] [<ffffffff8072e8da>] bus_add_driver+0xca/0x1d8
> > [ 0.307752] [<ffffffff80730aaa>] driver_register+0x3e/0xdc
> > [ 0.307757] [<ffffffff80731c54>] __platform_driver_register+0x1c/0x24
> > [ 0.307779] [<ffffffff80c334f6>] dw8250_platform_driver_init+0x1a/0x24
> > [ 0.307793] [<ffffffff80011992>] do_one_initcall+0x4e/0x2a4
> > [ 0.307800] [<ffffffff80c01362>] kernel_init_freeable+0x226/0x2b0
> > [ 0.307807] [<ffffffff80bc3798>] kernel_init+0x1c/0x144
> > [ 0.307813] [<ffffffff8001361c>] ret_from_fork_kernel+0x18/0x164
> > [ 0.307820] [<ffffffff80bcefb6>] ret_from_fork_kernel_asm+0x16/0x18
> > [ 0.307914] Code: 3683 2085 0b63 32f7 000f 0140 6918 4785 0713 0b47
> > (c31c) 2583
> > [ 0.308041] ---[ end trace 0000000000000000 ]---
> > [ 0.308180] Kernel panic - not syncing: Fatal exception in interrupt
> > [ 0.315760] ---[ end Kernel panic - not syncing: Fatal exception in
> > interrupt ]---
>
> The kernel oops doesn't add much; I'd drop it from the commit message.
> But your call if you want to keep it.
>
will drop the stack trace.
> > --- /dev/null
> > +++ b/hw/char/dw8250.c
>
> > +#define DW_UART_RE_EN 0xB4 /* Receiver Output Enable Register */
> > +#define DW_UART_DLF 0xC0 /* Divisor Latch Fraction Register */
> > +#define DW_UART_CPR 0xF4 /* Component Parameter Register */
> > +#define DW_UART_UCV 0xF8 /* UART Component Version */
> > +#define DW_UART_CTR 0xFC /* Component Type Register */
> > +
> > +#define DW_UART_UCV_VALUE 0x3332332A /* "323*" -> v3.23a */
>
> I was curious what this was used for, and it seems Linux has a
> dev_dbg but otherwise it's not important. The values here should be
> fine.
>
> > +#define DW_UART_CTR_VALUE 0x44570110 /* "DW" */
> > +
> > +static uint64_t dw8250_ext_read(void *opaque, hwaddr addr, unsigned int
> > size)
> > +{
> > + switch (addr) {
> > + case DW_UART_UCV:
> > + return DW_UART_UCV_VALUE;
> > + case DW_UART_CPR:
> > + return 0x00000000; /* No advanced features (DMA, extra FIFOs) */
>
> Linux will detect zero and use a value from the platform data, but
> this is only to support a quirk on renesas rzn1, so again this should
> be fine.
>
> > + case DW_UART_CTR:
> > + return DW_UART_CTR_VALUE;
> > +
> > + case DW_UART_RE_EN:
> > + case DW_UART_DLF:
> > + /*
> > + * Return 0 to indicate these optional features
> > + * (RS485 and Fractional Divisor) are not implemented.
> > + */
> > + return 0x00000000;
> > +
> > + default:
> > + return 0;
> > + }
> > +}
> > +
> > +static void dw8250_ext_write(void *opaque, hwaddr addr, uint64_t val,
> > unsigned int size)
> > +{
>
> LOG_UNIMP (or LOG_GUEST_ERROR)?
Ack. Will add it in the next version.
>
>
> > +static void dw8250_realize(DeviceState *dev, Error **errp)
> > +{
> > + DW8250State *s = DW8250(dev);
> > + SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> > + SysBusDevice *serial_sbd = SYS_BUS_DEVICE(s->serial_mm);
> > +
> > + memory_region_init(&s->container, OBJECT(dev), "dw8250-container",
> > + DW_UART_REGION_SIZE);
> > + sysbus_init_mmio(sbd, &s->container);
> > +
> > + qdev_prop_set_uint8(s->serial_mm, "regshift", s->regshift);
> > + qdev_prop_set_uint8(s->serial_mm, "endianness", DEVICE_LITTLE_ENDIAN);
> > + sysbus_realize(serial_sbd, errp);
>
> Return on error?
Ack.
>
> > +
> > + memory_region_init_io(&s->ext_iomem, OBJECT(dev), &dw8250_ext_ops, s,
> > + "dw8250-ext", DW_UART_REGION_SIZE);
> > + memory_region_add_subregion(&s->container, 0, &s->ext_iomem);
> > +
> > + memory_region_add_subregion_overlap(&s->container, 0,
> > + sysbus_mmio_get_region(serial_sbd,
> > 0), 1);
> > +
> > + sysbus_pass_irq(sbd, serial_sbd);
> > +}
> > +
> > +static const Property dw8250_properties[] = {
> > + DEFINE_PROP_UINT8("regshift", DW8250State, regshift, 2),
>
> When boards have a value other than 2, does this affect the DW
> extension registers too? Or only the 8250 ones?
I current use hardcoded byte offsets which inherently assumes
regshift = 2.
I'll update the switch statements in the extension memory region to
shift the addresses based on s->regshift.
Regards,
Kuan-Wei
