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

dmukhin@xen.org posted 15 patches 5 months, 1 week ago
There is a newer version of this series
[PATCH v6 04/15] emul/ns16x50: implement DLL/DLM registers
Posted by dmukhin@xen.org 5 months, 1 week ago
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;
+
     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;
+        rc = 0;
+    }
+
     return rc;
 }
 
@@ -146,9 +159,13 @@ static int ns16x50_io_write(
 static int ns16x50_io_read8(
     struct vuart_ns16x50 *vdev, uint32_t reg, uint8_t *data)
 {
+    uint8_t *regs = vdev->regs;
     uint8_t val = 0xff;
     int rc = 0;
 
+    if ( ns16x50_dlab_get(vdev) && (reg == UART_DLL || reg == UART_DLM) )
+        val = regs[NS16X50_REGS_NUM + reg];
+
     *data = val;
 
     return rc;
@@ -163,6 +180,13 @@ static int ns16x50_io_read16(
     uint16_t val = 0xffff;
     int rc = -EINVAL;
 
+    if ( ns16x50_dlab_get(vdev) && reg == UART_DLL )
+    {
+        val = vdev->regs[NS16X50_REGS_NUM + UART_DLM] << 8 |
+              vdev->regs[NS16X50_REGS_NUM + UART_DLL];
+        rc = 0;
+    }
+
     *data = val;
 
     return rc;
@@ -280,12 +304,17 @@ out:
 
 static int ns16x50_init(void *arg)
 {
+    const uint16_t divisor = (UART_CLOCK_HZ / 115200) >> 4;
     struct vuart_ns16x50 *vdev = arg;
     const struct vuart_info *info = vdev->info;
     struct domain *d = vdev->owner;
 
     ASSERT(vdev);
 
+    /* NB: report 115200 baud rate. */
+    vdev->regs[NS16X50_REGS_NUM + UART_DLL] = divisor & 0xff;
+    vdev->regs[NS16X50_REGS_NUM + UART_DLM] = (divisor >> 8) & 0xff;
+
     register_portio_handler(d, info->base_addr, info->size, ns16x50_io_handle);
 
     return 0;
-- 
2.51.0
Re: [PATCH v6 04/15] emul/ns16x50: implement DLL/DLM registers
Posted by Mykola Kvach 5 months ago
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?

> +
>      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.

> +        rc = 0;
> +    }
> +
>      return rc;
>  }
>
> @@ -146,9 +159,13 @@ static int ns16x50_io_write(
>  static int ns16x50_io_read8(
>      struct vuart_ns16x50 *vdev, uint32_t reg, uint8_t *data)
>  {
> +    uint8_t *regs = vdev->regs;
>      uint8_t val = 0xff;
>      int rc = 0;
>
> +    if ( ns16x50_dlab_get(vdev) && (reg == UART_DLL || reg == UART_DLM) )
> +        val = regs[NS16X50_REGS_NUM + reg];
> +
>      *data = val;
>
>      return rc;
> @@ -163,6 +180,13 @@ static int ns16x50_io_read16(
>      uint16_t val = 0xffff;
>      int rc = -EINVAL;
>
> +    if ( ns16x50_dlab_get(vdev) && reg == UART_DLL )
> +    {
> +        val = vdev->regs[NS16X50_REGS_NUM + UART_DLM] << 8 |
> +              vdev->regs[NS16X50_REGS_NUM + UART_DLL];
> +        rc = 0;
> +    }
> +
>      *data = val;
>
>      return rc;
> @@ -280,12 +304,17 @@ out:
>
>  static int ns16x50_init(void *arg)
>  {
> +    const uint16_t divisor = (UART_CLOCK_HZ / 115200) >> 4;
>      struct vuart_ns16x50 *vdev = arg;
>      const struct vuart_info *info = vdev->info;
>      struct domain *d = vdev->owner;
>
>      ASSERT(vdev);
>
> +    /* NB: report 115200 baud rate. */
> +    vdev->regs[NS16X50_REGS_NUM + UART_DLL] = divisor & 0xff;
> +    vdev->regs[NS16X50_REGS_NUM + UART_DLM] = (divisor >> 8) & 0xff;
> +
>      register_portio_handler(d, info->base_addr, info->size, ns16x50_io_handle);
>
>      return 0;
> --
> 2.51.0
>
>

Best regards,
Mykola
Re: [PATCH v6 04/15] emul/ns16x50: implement DLL/DLM registers
Posted by dmukhin@xen.org 5 months ago
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.

Re: [PATCH v6 04/15] emul/ns16x50: implement DLL/DLM registers
Posted by Stefano Stabellini 5 months, 1 week ago
On Fri, 5 Sep 2025, 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>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>