Re: [PATCH v6 04/15] emul/ns16x50: implement DLL/DLM registers

[dmukhin@xen.org]

On Sun, Sep 07, 2025 at 12:12:08AM +0300, Mykola Kvach wrote:
> Hi Denis,
> 
> On Sat, Sep 6, 2025 at 3:11 AM <dmukhin@xen.org> wrote:
> >
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > Add DLL/DLM registers emulation.
> >
> > DLL/DLM registers report hardcoded 115200 baud rate to the guest OS.
> >
> > Add stub for ns16x50_dlab_get() helper.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > ---
> > Changes since v5:
> > - dropped ns16x50_dlab_get() hunk (moved to emulator stub)
> > - Link to v5: https://lore.kernel.org/xen-devel/20250828235409.2835815-5-dmukhin@ford.com/
> > ---
> >  xen/common/emul/vuart/ns16x50.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >
> > diff --git a/xen/common/emul/vuart/ns16x50.c b/xen/common/emul/vuart/ns16x50.c
> > index 0462a961e785..7f479a5be4a2 100644
> > --- a/xen/common/emul/vuart/ns16x50.c
> > +++ b/xen/common/emul/vuart/ns16x50.c
> > @@ -97,8 +97,13 @@ static uint8_t ns16x50_dlab_get(const struct vuart_ns16x50 *vdev)
> >  static int ns16x50_io_write8(
> >      struct vuart_ns16x50 *vdev, uint32_t reg, uint8_t *data)
> >  {
> > +    uint8_t *regs = vdev->regs;
> > +    uint8_t val = *data;
> >      int rc = 0;
> >
> > +    if ( ns16x50_dlab_get(vdev) && (reg == UART_DLL || reg == UART_DLM) )
> > +        regs[NS16X50_REGS_NUM + reg] = val;
> 
> Some documentation (e.g., DesignWare DW_apb_uart Databook, v3.04a)
> notes that if the Divisor Latch Registers (DLL and DLH) are set to
> zero, the baud clock is disabled and no serial communications occur.
> 
> Should we handle the zero-divisor case to emulate this behavior more
> accurately?

Good idea, thanks!
I will plumb zero-divisor logic into RBR/THR handling.

> 
> > +
> >      return rc;
> >  }
> >
> > @@ -109,8 +114,16 @@ static int ns16x50_io_write8(
> >  static int ns16x50_io_write16(
> >      struct vuart_ns16x50 *vdev, uint32_t reg, uint16_t *data)
> >  {
> > +    uint16_t val = *data;
> >      int rc = -EINVAL;
> >
> > +    if ( ns16x50_dlab_get(vdev) && reg == UART_DLL )
> > +    {
> > +        vdev->regs[NS16X50_REGS_NUM + UART_DLL] = val & 0xff;
> > +        vdev->regs[NS16X50_REGS_NUM + UART_DLM] = (val >> 8) & 0xff;
> 
> Instead of hardcoding 0xff here (and in similar lines below), consider
> using UINT8_MAX. This makes it explicit that the value is the maximum
> for a uint8_t and avoids magic numbers.

Thanks for suggestion!
Will update.