8250: Customized base baudrate

PATCH In review Jan Kiszka 2 participants 7 messages Apr 13, 2008 – Apr 19, 2008

Share & tools

Jan Kiszka

author

2008-04-13 08:29 UTC #0 lore raw

PATCH 2/2 8250: Customized base baudrate

Previous patch revealed another issue of the 8250 emulation: The base baudrate is assumed to be 115200. This is surely not correct and could already cause wrong configurations if the virtual UART is forwarded to real one. With the previous patch applied, a wrong baudbase is even more problematic as it could lead to wrong throttling.

This patch extends serial[_mm]_init by a basebaud parameter and attempts to fix all call sites - without claiming to be correct in all cases (I tried to grab the information from current Linux). Please verify.

Signed-off-by Jan Kiszka <[email protected]>

---
 hw/mips_jazz.c    |    4 ++--
 hw/mips_malta.c   |    7 ++++---
 hw/mips_mipssim.c |    2 +-
 hw/mips_r4k.c     |    3 ++-
 hw/omap1.c        |    3 ++-
 hw/pc.c           |    3 ++-
 hw/pc.h           |    7 ++++---
 hw/ppc405_uc.c    |    2 +-
 hw/ppc_chrp.c     |    2 +-
 hw/ppc_oldworld.c |    2 +-
 hw/ppc_prep.c     |    2 +-
 hw/pxa2xx.c       |    6 ++++--
 hw/serial.c       |   14 +++++++++-----
 13 files changed, 34 insertions(+), 23 deletions(-)

hw/serial.c +9 -5

Index: b/hw/serial.c
===================================================================
@@ -99,6 +99,7 @@ struct SerialState {
     int last_break_enable;
     target_phys_addr_t base;
     int it_shift;
+    int baudbase;
     QEMUTimer *tx_timer;
     int tx_burst;
     char tx_buf;

@@ -129,7 +130,7 @@ static void serial_tx_done(void *opaque)
     if (s->tx_burst < 0) {
         /* We assume 10 bits/char, OK for this purpose. */
         s->tx_burst = THROTTLE_TX_INTERVAL * 1000 /
-            (1000000 * 10 / (115200 / s->divider));
+            (1000000 * 10 / (s->baudbase / s->divider));
     }
     s->thr_ipending = 1;
     s->lsr |= UART_LSR_THRE;

@@ -161,7 +162,7 @@ static void serial_update_parameters(Ser
     data_bits = (s->lcr & 0x03) + 5;
     if (s->divider == 0)
         return;
-    speed = 115200 / s->divider;
+    speed = s->baudbase / s->divider;
     ssp.speed = speed;
     ssp.parity = parity;
     ssp.data_bits = data_bits;

@@ -407,7 +408,8 @@ static void serial_reset(void *opaque)
 }
 
 /* If fd is zero, it means that the serial device uses the console */
-SerialState *serial_init(int base, qemu_irq irq, CharDriverState *chr)
+SerialState *serial_init(int base, qemu_irq irq, int baudbase,
+                         CharDriverState *chr)
 {
     SerialState *s;

@@ -415,6 +417,7 @@ SerialState *serial_init(int base, qemu_
     if (!s)
         return NULL;
     s->irq = irq;
+    s->baudbase = baudbase;
 
     s->tx_timer = qemu_new_timer(vm_clock, serial_tx_done, s);
     if (!s->tx_timer)

@@ -506,8 +509,8 @@ static CPUWriteMemoryFunc *serial_mm_wri
 };
 
 SerialState *serial_mm_init (target_phys_addr_t base, int it_shift,
-                             qemu_irq irq, CharDriverState *chr,
-                             int ioregister)
+                             qemu_irq irq, int baudbase,
+                             CharDriverState *chr, int ioregister)
 {
     SerialState *s;
     int s_io_memory;

@@ -518,6 +521,7 @@ SerialState *serial_mm_init (target_phys
     s->irq = irq;
     s->base = base;
     s->it_shift = it_shift;
+    s->baudbase= baudbase;
 
     s->tx_timer = qemu_new_timer(vm_clock, serial_tx_done, s);
     if (!s->tx_timer)

hw/omap1.c +2 -1

Index: b/hw/omap1.c
===================================================================
@@ -1734,7 +1734,8 @@ struct omap_uart_s *omap_uart_init(targe
     struct omap_uart_s *s = (struct omap_uart_s *)
             qemu_mallocz(sizeof(struct omap_uart_s));
     if (chr)
-        s->serial = serial_mm_init(base, 2, irq, chr, 1);
+        s->serial =
+            serial_mm_init(base, 2, irq, omap_clk_getrate(clk)/16, chr, 1);
     return s;
 }

hw/pc.c +2 -1

Index: b/hw/pc.c
===================================================================
@@ -909,7 +909,8 @@ static void pc_init1(int ram_size, int v
 
     for(i = 0; i < MAX_SERIAL_PORTS; i++) {
         if (serial_hds[i]) {
-            serial_init(serial_io[i], i8259[serial_irq[i]], serial_hds[i]);
+            serial_init(serial_io[i], i8259[serial_irq[i]], 115200,
+                        serial_hds[i]);
         }
     }

hw/pc.h +4 -3

Index: b/hw/pc.h
===================================================================
@@ -4,10 +4,11 @@
 
 /* serial.c */
 
-SerialState *serial_init(int base, qemu_irq irq, CharDriverState *chr);
+SerialState *serial_init(int base, qemu_irq irq, int baudbase,
+                         CharDriverState *chr);
 SerialState *serial_mm_init (target_phys_addr_t base, int it_shift,
-                             qemu_irq irq, CharDriverState *chr,
-                             int ioregister);
+                             qemu_irq irq, int baudbase,
+                             CharDriverState *chr, int ioregister);
 uint32_t serial_mm_readb (void *opaque, target_phys_addr_t addr);
 void serial_mm_writeb (void *opaque, target_phys_addr_t addr, uint32_t value);
 uint32_t serial_mm_readw (void *opaque, target_phys_addr_t addr);

hw/pxa2xx.c +4 -2

Index: b/hw/pxa2xx.c
===================================================================
@@ -2077,7 +2077,8 @@ struct pxa2xx_state_s *pxa270_init(unsig
     for (i = 0; pxa270_serial[i].io_base; i ++)
         if (serial_hds[i])
             serial_mm_init(pxa270_serial[i].io_base, 2,
-                            s->pic[pxa270_serial[i].irqn], serial_hds[i], 1);
+                           s->pic[pxa270_serial[i].irqn], 14857000/16,
+                           serial_hds[i], 1);
         else
             break;
     if (serial_hds[i])

@@ -2202,7 +2203,8 @@ struct pxa2xx_state_s *pxa255_init(unsig
     for (i = 0; pxa255_serial[i].io_base; i ++)
         if (serial_hds[i])
             serial_mm_init(pxa255_serial[i].io_base, 2,
-                            s->pic[pxa255_serial[i].irqn], serial_hds[i], 1);
+                           s->pic[pxa255_serial[i].irqn], 14745600/16,
+                           serial_hds[i], 1);
         else
             break;
     if (serial_hds[i])

hw/mips_jazz.c +2 -2

Index: b/hw/mips_jazz.c
===================================================================
@@ -234,9 +234,9 @@ void mips_jazz_init (int ram_size, int v
 
     /* Serial ports */
     if (serial_hds[0])
-        serial_mm_init(0x80006000, 0, rc4030[8], serial_hds[0], 1);
+        serial_mm_init(0x80006000, 0, rc4030[8], 8000000/16, serial_hds[0], 1);
     if (serial_hds[1])
-        serial_mm_init(0x80007000, 0, rc4030[9], serial_hds[1], 1);
+        serial_mm_init(0x80007000, 0, rc4030[9], 8000000/16, serial_hds[1], 1);
 
     /* Parallel port */
     if (parallel_hds[0])

hw/mips_malta.c +4 -3

Index: b/hw/mips_malta.c
===================================================================
@@ -449,7 +449,8 @@ static MaltaFPGAState *malta_fpga_init(t
 
     uart_chr = qemu_chr_open("vc");
     qemu_chr_printf(uart_chr, "CBUS UART\r\n");
-    s->uart = serial_mm_init(base + 0x900, 3, env->irq[2], uart_chr, 1);
+    s->uart =
+        serial_mm_init(base + 0x900, 3, env->irq[2], 230400, uart_chr, 1);
 
     malta_fpga_reset(s);
     qemu_register_reset(malta_fpga_reset, s);

@@ -918,9 +919,9 @@ void mips_malta_init (int ram_size, int 
     i8042_init(i8259[1], i8259[12], 0x60);
     rtc_state = rtc_init(0x70, i8259[8]);
     if (serial_hds[0])
-        serial_init(0x3f8, i8259[4], serial_hds[0]);
+        serial_init(0x3f8, i8259[4], 115200, serial_hds[0]);
     if (serial_hds[1])
-        serial_init(0x2f8, i8259[3], serial_hds[1]);
+        serial_init(0x2f8, i8259[3], 115200, serial_hds[1]);
     if (parallel_hds[0])
         parallel_init(0x378, i8259[7], parallel_hds[0]);
     for(i = 0; i < MAX_FD; i++) {

hw/mips_mipssim.c +1 -1

Index: b/hw/mips_mipssim.c
===================================================================
@@ -174,7 +174,7 @@ mips_mipssim_init (int ram_size, int vga
     /* A single 16450 sits at offset 0x3f8. It is attached to
        MIPS CPU INT2, which is interrupt 4. */
     if (serial_hds[0])
-        serial_init(0x3f8, env->irq[4], serial_hds[0]);
+        serial_init(0x3f8, env->irq[4], 115200, serial_hds[0]);
 
     if (nd_table[0].vlan) {
         if (nd_table[0].model == NULL

hw/mips_r4k.c +2 -1

Index: b/hw/mips_r4k.c
===================================================================
@@ -229,7 +229,8 @@ void mips_r4k_init (int ram_size, int vg
 
     for(i = 0; i < MAX_SERIAL_PORTS; i++) {
         if (serial_hds[i]) {
-            serial_init(serial_io[i], i8259[serial_irq[i]], serial_hds[i]);
+            serial_init(serial_io[i], i8259[serial_irq[i]], 115200,
+                        serial_hds[i]);
         }
     }

hw/ppc405_uc.c +1 -1

Index: b/hw/ppc405_uc.c
===================================================================
@@ -1223,7 +1223,7 @@ void ppc405_serial_init (CPUState *env, 
 #ifdef DEBUG_SERIAL
     printf("%s: offset " PADDRX "\n", __func__, offset);
 #endif
-    serial = serial_mm_init(offset, 0, irq, chr, 0);
+    serial = serial_mm_init(offset, 0, irq, 399193, chr, 0);
     ppc4xx_mmio_register(env, mmio, offset, 0x008,
                          serial_mm_read, serial_mm_write, serial);
 }

hw/ppc_chrp.c +1 -1

Index: b/hw/ppc_chrp.c
===================================================================
@@ -264,7 +264,7 @@ static void ppc_core99_init (int ram_siz
     dummy_irq = i8259_init(NULL);
 
     /* XXX: use Mac Serial port */
-    serial_init(0x3f8, dummy_irq[4], serial_hds[0]);
+    serial_init(0x3f8, dummy_irq[4], 115200, serial_hds[0]);
     for(i = 0; i < nb_nics; i++) {
         if (!nd_table[i].model)
             nd_table[i].model = "ne2k_pci";

hw/ppc_oldworld.c +1 -1

Index: b/hw/ppc_oldworld.c
===================================================================
@@ -287,7 +287,7 @@ static void ppc_heathrow_init (int ram_s
     dummy_irq = i8259_init(NULL);
 
     /* XXX: use Mac Serial port */
-    serial_init(0x3f8, dummy_irq[4], serial_hds[0]);
+    serial_init(0x3f8, dummy_irq[4], 115200, serial_hds[0]);
 
     for(i = 0; i < nb_nics; i++) {
         if (!nd_table[i].model)

hw/ppc_prep.c +1 -1

Index: b/hw/ppc_prep.c
===================================================================
@@ -667,7 +667,7 @@ static void ppc_prep_init (int ram_size,
     //    pit = pit_init(0x40, i8259[0]);
     rtc_init(0x70, i8259[8]);
 
-    serial_init(0x3f8, i8259[4], serial_hds[0]);
+    serial_init(0x3f8, i8259[4], 115200, serial_hds[0]);
     nb_nics1 = nb_nics;
     if (nb_nics1 > NE2000_NB_MAX)
         nb_nics1 = NE2000_NB_MAX;

andrzej zaborowski

2008-04-14 18:37 UTC #1 lore raw

Quoted 94 lines · Jan Kiszka

Previous patch revealed another issue of the 8250 emulation: The base
baudrate is assumed to be 115200. This is surely not correct and could already cause wrong configurations if the virtual UART is forwarded to real one. With the previous patch applied, a wrong baudbase is even more problematic as it could lead to wrong throttling.
This patch extends serial[_mm]_init by a basebaud parameter and attempts to fix all call sites - without claiming to be correct in all cases (I tried to grab the information from current Linux). Please verify.
Signed-off-by Jan Kiszka <[email protected]>
---
  hw/mips_jazz.c    |    4 ++--
  hw/mips_malta.c   |    7 ++++---
  hw/mips_mipssim.c |    2 +-
  hw/mips_r4k.c     |    3 ++-
  hw/omap1.c        |    3 ++-
  hw/pc.c           |    3 ++-
  hw/pc.h           |    7 ++++---
  hw/ppc405_uc.c    |    2 +-
  hw/ppc_chrp.c     |    2 +-
  hw/ppc_oldworld.c |    2 +-
  hw/ppc_prep.c     |    2 +-
  hw/pxa2xx.c       |    6 ++++--
  hw/serial.c       |   14 +++++++++-----
  13 files changed, 34 insertions(+), 23 deletions(-)
Index: b/hw/serial.c
===================================================================
--- a/hw/serial.c
+++ b/hw/serial.c
 @@ -99,6 +99,7 @@ struct SerialState {
     int last_break_enable;
     target_phys_addr_t base;
     int it_shift;
 +    int baudbase;
     QEMUTimer *tx_timer;
     int tx_burst;
     char tx_buf;
 @@ -129,7 +130,7 @@ static void serial_tx_done(void *opaque)
     if (s->tx_burst < 0) {
         /* We assume 10 bits/char, OK for this purpose. */
         s->tx_burst = THROTTLE_TX_INTERVAL * 1000 /
 -            (1000000 * 10 / (115200 / s->divider));
 +            (1000000 * 10 / (s->baudbase / s->divider));
     }
     s->thr_ipending = 1;
     s->lsr |= UART_LSR_THRE;
 @@ -161,7 +162,7 @@ static void serial_update_parameters(Ser
     data_bits = (s->lcr & 0x03) + 5;
     if (s->divider == 0)
         return;
 -    speed = 115200 / s->divider;
 +    speed = s->baudbase / s->divider;
     ssp.speed = speed;
     ssp.parity = parity;
     ssp.data_bits = data_bits;
@@ -407,7 +408,8 @@ static void serial_reset(void *opaque)
}
/* If fd is zero, it means that the serial device uses the console */
-SerialState *serial_init(int base, qemu_irq irq, CharDriverState *chr) +SerialState *serial_init(int base, qemu_irq irq, int baudbase,
+ CharDriverState *chr)
  {
     SerialState *s;
@@ -415,6 +417,7 @@ SerialState *serial_init(int base, qemu_
     if (!s)
         return NULL;
     s->irq = irq;
+ s->baudbase = baudbase;
     s->tx_timer = qemu_new_timer(vm_clock, serial_tx_done, s);
     if (!s->tx_timer)
@@ -506,8 +509,8 @@ static CPUWriteMemoryFunc *serial_mm_wri
};
SerialState *serial_mm_init (target_phys_addr_t base, int it_shift,
 -                             qemu_irq irq, CharDriverState *chr,
 -                             int ioregister)
 +                             qemu_irq irq, int baudbase,
 +                             CharDriverState *chr, int ioregister)
  {
     SerialState *s;
     int s_io_memory;
 @@ -518,6 +521,7 @@ SerialState *serial_mm_init (target_phys
     s->irq = irq;
     s->base = base;
     s->it_shift = it_shift;
+ s->baudbase= baudbase;
     s->tx_timer = qemu_new_timer(vm_clock, serial_tx_done, s);
     if (!s->tx_timer)

Quoted 11 lines

Index: b/hw/omap1.c
===================================================================
--- a/hw/omap1.c
+++ b/hw/omap1.c
@@ -1734,7 +1734,8 @@ struct omap_uart_s *omap_uart_init(targe
struct omap_uart_s *s = (struct omap_uart_s *)
qemu_mallocz(sizeof(struct omap_uart_s));
if (chr)
- s->serial = serial_mm_init(base, 2, irq, chr, 1);
+ s->serial =
+ serial_mm_init(base, 2, irq, omap_clk_getrate(clk)/16, chr, 1);

Yes, this should be correct.

Quoted 54 lines

     return s;

}

Index: b/hw/pc.c
===================================================================
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -909,7 +909,8 @@ static void pc_init1(int ram_size, int v

     for(i = 0; i < MAX_SERIAL_PORTS; i++) {
         if (serial_hds[i]) {
 -            serial_init(serial_io[i], i8259[serial_irq[i]], serial_hds[i]);
 +            serial_init(serial_io[i], i8259[serial_irq[i]], 115200,
 +                        serial_hds[i]);
         }
     }

Index: b/hw/pc.h
===================================================================
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -4,10 +4,11 @@

/* serial.c */

-SerialState *serial_init(int base, qemu_irq irq, CharDriverState *chr); +SerialState *serial_init(int base, qemu_irq irq, int baudbase,
+ CharDriverState *chr);
SerialState *serial_mm_init (target_phys_addr_t base, int it_shift,

 -                             qemu_irq irq, CharDriverState *chr,
 -                             int ioregister);
 +                             qemu_irq irq, int baudbase,
 +                             CharDriverState *chr, int ioregister);
  uint32_t serial_mm_readb (void *opaque, target_phys_addr_t addr);
  void serial_mm_writeb (void *opaque, target_phys_addr_t addr, uint32_t value);
  uint32_t serial_mm_readw (void *opaque, target_phys_addr_t addr);
 Index: b/hw/pxa2xx.c
 ===================================================================
 --- a/hw/pxa2xx.c
 +++ b/hw/pxa2xx.c
 @@ -2077,7 +2077,8 @@ struct pxa2xx_state_s *pxa270_init(unsig
     for (i = 0; pxa270_serial[i].io_base; i ++)
         if (serial_hds[i])
             serial_mm_init(pxa270_serial[i].io_base, 2,
 -                            s->pic[pxa270_serial[i].irqn], serial_hds[i], 1);
 +                           s->pic[pxa270_serial[i].irqn], 14857000/16,
 +                           serial_hds[i], 1);
         else
             break;
     if (serial_hds[i])
 @@ -2202,7 +2203,8 @@ struct pxa2xx_state_s *pxa255_init(unsig
     for (i = 0; pxa255_serial[i].io_base; i ++)
         if (serial_hds[i])
             serial_mm_init(pxa255_serial[i].io_base, 2,

Quoted 3 lines

- s->pic[pxa255_serial[i].irqn], serial_hds[i], 1);
+ s->pic[pxa255_serial[i].irqn], 14745600/16,
+ serial_hds[i], 1);

This also (although I, too, only know from looking at Linux code).

Quoted 127 lines

         else
             break;
     if (serial_hds[i])

Index: b/hw/mips_jazz.c
===================================================================
--- a/hw/mips_jazz.c
+++ b/hw/mips_jazz.c
@@ -234,9 +234,9 @@ void mips_jazz_init (int ram_size, int v

     /* Serial ports */
     if (serial_hds[0])
 -        serial_mm_init(0x80006000, 0, rc4030[8], serial_hds[0], 1);
 +        serial_mm_init(0x80006000, 0, rc4030[8], 8000000/16, serial_hds[0], 1);
     if (serial_hds[1])

- serial_mm_init(0x80007000, 0, rc4030[9], serial_hds[1], 1);
+ serial_mm_init(0x80007000, 0, rc4030[9], 8000000/16, serial_hds[1], 1);

     /* Parallel port */
     if (parallel_hds[0])

Index: b/hw/mips_malta.c
===================================================================
--- a/hw/mips_malta.c
+++ b/hw/mips_malta.c
@@ -449,7 +449,8 @@ static MaltaFPGAState *malta_fpga_init(t

     uart_chr = qemu_chr_open("vc");
     qemu_chr_printf(uart_chr, "CBUS UART\r\n");

- s->uart = serial_mm_init(base + 0x900, 3, env->irq[2], uart_chr, 1);
+ s->uart =
+ serial_mm_init(base + 0x900, 3, env->irq[2], 230400, uart_chr, 1);

     malta_fpga_reset(s);
     qemu_register_reset(malta_fpga_reset, s);
 @@ -918,9 +919,9 @@ void mips_malta_init (int ram_size, int
     i8042_init(i8259[1], i8259[12], 0x60);
     rtc_state = rtc_init(0x70, i8259[8]);
     if (serial_hds[0])
 -        serial_init(0x3f8, i8259[4], serial_hds[0]);
 +        serial_init(0x3f8, i8259[4], 115200, serial_hds[0]);
     if (serial_hds[1])
 -        serial_init(0x2f8, i8259[3], serial_hds[1]);
 +        serial_init(0x2f8, i8259[3], 115200, serial_hds[1]);
     if (parallel_hds[0])
         parallel_init(0x378, i8259[7], parallel_hds[0]);
     for(i = 0; i < MAX_FD; i++) {
 Index: b/hw/mips_mipssim.c
 ===================================================================
 --- a/hw/mips_mipssim.c
 +++ b/hw/mips_mipssim.c
 @@ -174,7 +174,7 @@ mips_mipssim_init (int ram_size, int vga
     /* A single 16450 sits at offset 0x3f8. It is attached to
        MIPS CPU INT2, which is interrupt 4. */
     if (serial_hds[0])

- serial_init(0x3f8, env->irq[4], serial_hds[0]);
+ serial_init(0x3f8, env->irq[4], 115200, serial_hds[0]);

     if (nd_table[0].vlan) {
         if (nd_table[0].model == NULL

Index: b/hw/mips_r4k.c
===================================================================
--- a/hw/mips_r4k.c
+++ b/hw/mips_r4k.c
@@ -229,7 +229,8 @@ void mips_r4k_init (int ram_size, int vg

     for(i = 0; i < MAX_SERIAL_PORTS; i++) {
         if (serial_hds[i]) {
 -            serial_init(serial_io[i], i8259[serial_irq[i]], serial_hds[i]);
 +            serial_init(serial_io[i], i8259[serial_irq[i]], 115200,
 +                        serial_hds[i]);
         }
     }

Index: b/hw/ppc405_uc.c
===================================================================
--- a/hw/ppc405_uc.c
+++ b/hw/ppc405_uc.c
@@ -1223,7 +1223,7 @@ void ppc405_serial_init (CPUState *env,

  #ifdef DEBUG_SERIAL
     printf("%s: offset " PADDRX "\n", __func__, offset);
  #endif
 -    serial = serial_mm_init(offset, 0, irq, chr, 0);
 +    serial = serial_mm_init(offset, 0, irq, 399193, chr, 0);
     ppc4xx_mmio_register(env, mmio, offset, 0x008,
                          serial_mm_read, serial_mm_write, serial);
  }
 Index: b/hw/ppc_chrp.c
 ===================================================================
 --- a/hw/ppc_chrp.c
 +++ b/hw/ppc_chrp.c
 @@ -264,7 +264,7 @@ static void ppc_core99_init (int ram_siz
     dummy_irq = i8259_init(NULL);

     /* XXX: use Mac Serial port */
 -    serial_init(0x3f8, dummy_irq[4], serial_hds[0]);
 +    serial_init(0x3f8, dummy_irq[4], 115200, serial_hds[0]);
     for(i = 0; i < nb_nics; i++) {
         if (!nd_table[i].model)
             nd_table[i].model = "ne2k_pci";
 Index: b/hw/ppc_oldworld.c
 ===================================================================
 --- a/hw/ppc_oldworld.c
 +++ b/hw/ppc_oldworld.c
 @@ -287,7 +287,7 @@ static void ppc_heathrow_init (int ram_s
     dummy_irq = i8259_init(NULL);

     /* XXX: use Mac Serial port */

- serial_init(0x3f8, dummy_irq[4], serial_hds[0]);
+ serial_init(0x3f8, dummy_irq[4], 115200, serial_hds[0]);

     for(i = 0; i < nb_nics; i++) {
         if (!nd_table[i].model)
 Index: b/hw/ppc_prep.c
 ===================================================================
 --- a/hw/ppc_prep.c
 +++ b/hw/ppc_prep.c
 @@ -667,7 +667,7 @@ static void ppc_prep_init (int ram_size,
     //    pit = pit_init(0x40, i8259[0]);
     rtc_init(0x70, i8259[8]);

- serial_init(0x3f8, i8259[4], serial_hds[0]);

 +    serial_init(0x3f8, i8259[4], 115200, serial_hds[0]);
     nb_nics1 = nb_nics;
     if (nb_nics1 > NE2000_NB_MAX)
         nb_nics1 = NE2000_NB_MAX;

Signature · 2 lines

Please do not print this email unless absolutely necessary. Spread
environmental awareness.

Jan Kiszka

author

2008-04-14 19:22 UTC #2 lore raw

Quoted 11 lines · andrzej zaborowski

Quoted 9 lines · Jan Kiszka
--- a/hw/omap1.c
+++ b/hw/omap1.c
@@ -1734,7 +1734,8 @@ struct omap_uart_s *omap_uart_init(targe
struct omap_uart_s *s = (struct omap_uart_s *)
qemu_mallocz(sizeof(struct omap_uart_s));
if (chr)
- s->serial = serial_mm_init(base, 2, irq, chr, 1);
+ s->serial =
+ serial_mm_init(base, 2, irq, omap_clk_getrate(clk)/16, chr, 1);

Quoted 1 line

Yes, this should be correct.

While typing those line I wondered if it is possible and reasonable that a guest changes this clock rate during runtime? In that case, the perfect, but probably much more complex solution would be to track those changes also with the serial emulation...

Jan

andrzej zaborowski

2008-04-14 19:41 UTC #3 lore raw

Quoted 13 lines · Jan Kiszka

Quoted 12 lines · andrzej zaborowski
Quoted 9 lines · Jan Kiszka
--- a/hw/omap1.c
+++ b/hw/omap1.c
@@ -1734,7 +1734,8 @@ struct omap_uart_s *omap_uart_init(targe
struct omap_uart_s *s = (struct omap_uart_s *)
qemu_mallocz(sizeof(struct omap_uart_s));
if (chr)
- s->serial = serial_mm_init(base, 2, irq, chr, 1);
+ s->serial =
+ serial_mm_init(base, 2, irq, omap_clk_getrate(clk)/16, chr, 1);

Quoted 6 lines

Quoted 1 line
Yes, this should be correct.
While typing those line I wondered if it is possible and reasonable that
a guest changes this clock rate during runtime? In that case, the perfect, but probably much more complex solution would be to track those changes also with the serial emulation...

Yes, UART1 and UART3 (I think) can switch between 48MHz (dpll) and 16MHz clock domains on OMAP1 and the guest has control over this (and the characters flow indeed slows down three times when you switch to 16MHz on the real hw). omap_clk.c allows a peripheral to register for notifications of rate change for a particular clock. I don't think it's worth bothering too much because anything that depends on the timing of QEMU will likely have problems anyway.

Regards

Signature · 2 lines

Please do not print this email unless absolutely necessary. Spread
environmental awareness.

andrzej zaborowski

2008-04-14 19:42 UTC #4 lore raw

Quoted 16 lines · andrzej zaborowski

Quoted 15 lines · Jan Kiszka
Quoted 13 lines · andrzej zaborowski
Quoted 9 lines · Jan Kiszka
--- a/hw/omap1.c
+++ b/hw/omap1.c
@@ -1734,7 +1734,8 @@ struct omap_uart_s *omap_uart_init(targe
struct omap_uart_s *s = (struct omap_uart_s *)
qemu_mallocz(sizeof(struct omap_uart_s));
if (chr)
- s->serial = serial_mm_init(base, 2, irq, chr, 1);
+ s->serial =
+ serial_mm_init(base, 2, irq, omap_clk_getrate(clk)/16, chr, 1);
Yes, this should be correct.

Quoted 8 lines

Quoted 4 lines
While typing those line I wondered if it is possible and reasonable that
a guest changes this clock rate during runtime? In that case, the perfect, but probably much more complex solution would be to track those changes also with the serial emulation...
Yes, UART1 and UART3 (I think) can switch between 48MHz (dpll) and
16MHz clock domains on OMAP1 and the guest has control over this (and

Err, 48MHz and 12MHz actually.

Jan Kiszka

author

2008-04-14 21:35 UTC #5 lore raw

In reply to andrzej zaborowski

Quoted 21 lines · andrzej zaborowski

Quoted 13 lines · Jan Kiszka
Quoted 12 lines · andrzej zaborowski
Quoted 9 lines · Jan Kiszka
--- a/hw/omap1.c
+++ b/hw/omap1.c
@@ -1734,7 +1734,8 @@ struct omap_uart_s *omap_uart_init(targe
struct omap_uart_s *s = (struct omap_uart_s *)
qemu_mallocz(sizeof(struct omap_uart_s));
if (chr)
- s->serial = serial_mm_init(base, 2, irq, chr, 1);
+ s->serial =
+ serial_mm_init(base, 2, irq, omap_clk_getrate(clk)/16, chr, 1);
Quoted 6 lines
Quoted 1 line
Yes, this should be correct.
While typing those line I wondered if it is possible and reasonable that
a guest changes this clock rate during runtime? In that case, the perfect, but probably much more complex solution would be to track those changes also with the serial emulation...

Quoted 7 lines

Yes, UART1 and UART3 (I think) can switch between 48MHz (dpll) and 16MHz clock domains on OMAP1 and the guest has control over this (and the characters flow indeed slows down three times when you switch to 16MHz on the real hw). omap_clk.c allows a peripheral to register for notifications of rate change for a particular clock. I don't think it's worth bothering too much because anything that depends on the timing of QEMU will likely have problems anyway.

Well, depends. The baudrate set by the guest is also propagated to the host tty which is attached to that virtual channel. If that tty bothers about baudrate, you will have real impact. Besides this, my patch 1/2 makes use of the baudrate to emulate more realistic output (IRQ) rates.

Jan

Jan Kiszka

author

2008-04-19 19:37 UTC #6 lore raw

In reply to Jan Kiszka

Quoted 9 lines · Jan Kiszka

Previous patch revealed another issue of the 8250 emulation: The base baudrate is assumed to be 115200. This is surely not correct and could already cause wrong configurations if the virtual UART is forwarded to real one. With the previous patch applied, a wrong baudbase is even more problematic as it could lead to wrong throttling.
This patch extends serial[_mm]_init by a basebaud parameter and attempts to fix all call sites - without claiming to be correct in all cases (I tried to grab the information from current Linux). Please verify.

Here is a rebase against latest SVN (#4220). Andrzej, omap_uart_init recently started to confront me with two clocks instead of one. Did I picked the right one here? What's the deal with the other?

Signed-off-by Jan Kiszka <[email protected]>

---
 hw/mips_jazz.c    |    4 ++--
 hw/mips_malta.c   |    7 ++++---
 hw/mips_mipssim.c |    2 +-
 hw/mips_r4k.c     |    3 ++-
 hw/omap1.c        |    3 ++-
 hw/pc.c           |    3 ++-
 hw/pc.h           |    7 ++++---
 hw/ppc405_uc.c    |    2 +-
 hw/ppc_chrp.c     |    2 +-
 hw/ppc_oldworld.c |    2 +-
 hw/ppc_prep.c     |    2 +-
 hw/pxa2xx.c       |    6 ++++--
 hw/serial.c       |   14 +++++++++-----
 13 files changed, 34 insertions(+), 23 deletions(-)

hw/serial.c +9 -5

Index: b/hw/serial.c
===================================================================
@@ -99,6 +99,7 @@ struct SerialState {
     int last_break_enable;
     target_phys_addr_t base;
     int it_shift;
+    int baudbase;
     QEMUTimer *tx_timer;
     int tx_burst;
     char tx_buf;

@@ -129,7 +130,7 @@ static void serial_tx_done(void *opaque)
     if (s->tx_burst < 0) {
         /* We assume 10 bits/char, OK for this purpose. */
         s->tx_burst = THROTTLE_TX_INTERVAL * 1000 /
-            (1000000 * 10 / (115200 / s->divider));
+            (1000000 * 10 / (s->baudbase / s->divider));
     }
     s->thr_ipending = 1;
     s->lsr |= UART_LSR_THRE;

@@ -161,7 +162,7 @@ static void serial_update_parameters(Ser
     data_bits = (s->lcr & 0x03) + 5;
     if (s->divider == 0)
         return;
-    speed = 115200 / s->divider;
+    speed = s->baudbase / s->divider;
     ssp.speed = speed;
     ssp.parity = parity;
     ssp.data_bits = data_bits;

@@ -407,7 +408,8 @@ static void serial_reset(void *opaque)
 }
 
 /* If fd is zero, it means that the serial device uses the console */
-SerialState *serial_init(int base, qemu_irq irq, CharDriverState *chr)
+SerialState *serial_init(int base, qemu_irq irq, int baudbase,
+                         CharDriverState *chr)
 {
     SerialState *s;

@@ -415,6 +417,7 @@ SerialState *serial_init(int base, qemu_
     if (!s)
         return NULL;
     s->irq = irq;
+    s->baudbase = baudbase;
 
     s->tx_timer = qemu_new_timer(vm_clock, serial_tx_done, s);
     if (!s->tx_timer)

@@ -506,8 +509,8 @@ static CPUWriteMemoryFunc *serial_mm_wri
 };
 
 SerialState *serial_mm_init (target_phys_addr_t base, int it_shift,
-                             qemu_irq irq, CharDriverState *chr,
-                             int ioregister)
+                             qemu_irq irq, int baudbase,
+                             CharDriverState *chr, int ioregister)
 {
     SerialState *s;
     int s_io_memory;

@@ -518,6 +521,7 @@ SerialState *serial_mm_init (target_phys
     s->irq = irq;
     s->base = base;
     s->it_shift = it_shift;
+    s->baudbase= baudbase;
 
     s->tx_timer = qemu_new_timer(vm_clock, serial_tx_done, s);
     if (!s->tx_timer)

hw/omap1.c +2 -1

Index: b/hw/omap1.c
===================================================================
@@ -2004,7 +2004,8 @@ struct omap_uart_s *omap_uart_init(targe
     struct omap_uart_s *s = (struct omap_uart_s *)
             qemu_mallocz(sizeof(struct omap_uart_s));
 
-    s->serial = serial_mm_init(base, 2, irq, chr ?: qemu_chr_open("null"), 1);
+    s->serial = serial_mm_init(base, 2, irq, omap_clk_getrate(fclk)/16,
+                               chr ?: qemu_chr_open("null"), 1);
 
     return s;
 }

hw/pc.c +2 -1

Index: b/hw/pc.c
===================================================================
@@ -909,7 +909,8 @@ static void pc_init1(int ram_size, int v
 
     for(i = 0; i < MAX_SERIAL_PORTS; i++) {
         if (serial_hds[i]) {
-            serial_init(serial_io[i], i8259[serial_irq[i]], serial_hds[i]);
+            serial_init(serial_io[i], i8259[serial_irq[i]], 115200,
+                        serial_hds[i]);
         }
     }

hw/pc.h +4 -3

Index: b/hw/pc.h
===================================================================
@@ -4,10 +4,11 @@
 
 /* serial.c */
 
-SerialState *serial_init(int base, qemu_irq irq, CharDriverState *chr);
+SerialState *serial_init(int base, qemu_irq irq, int baudbase,
+                         CharDriverState *chr);
 SerialState *serial_mm_init (target_phys_addr_t base, int it_shift,
-                             qemu_irq irq, CharDriverState *chr,
-                             int ioregister);
+                             qemu_irq irq, int baudbase,
+                             CharDriverState *chr, int ioregister);
 uint32_t serial_mm_readb (void *opaque, target_phys_addr_t addr);
 void serial_mm_writeb (void *opaque, target_phys_addr_t addr, uint32_t value);
 uint32_t serial_mm_readw (void *opaque, target_phys_addr_t addr);

hw/pxa2xx.c +4 -2

Index: b/hw/pxa2xx.c
===================================================================
@@ -2077,7 +2077,8 @@ struct pxa2xx_state_s *pxa270_init(unsig
     for (i = 0; pxa270_serial[i].io_base; i ++)
         if (serial_hds[i])
             serial_mm_init(pxa270_serial[i].io_base, 2,
-                            s->pic[pxa270_serial[i].irqn], serial_hds[i], 1);
+                           s->pic[pxa270_serial[i].irqn], 14857000/16,
+                           serial_hds[i], 1);
         else
             break;
     if (serial_hds[i])

@@ -2202,7 +2203,8 @@ struct pxa2xx_state_s *pxa255_init(unsig
     for (i = 0; pxa255_serial[i].io_base; i ++)
         if (serial_hds[i])
             serial_mm_init(pxa255_serial[i].io_base, 2,
-                            s->pic[pxa255_serial[i].irqn], serial_hds[i], 1);
+                           s->pic[pxa255_serial[i].irqn], 14745600/16,
+                           serial_hds[i], 1);
         else
             break;
     if (serial_hds[i])

hw/mips_jazz.c +2 -2

Index: b/hw/mips_jazz.c
===================================================================
@@ -234,9 +234,9 @@ void mips_jazz_init (int ram_size, int v
 
     /* Serial ports */
     if (serial_hds[0])
-        serial_mm_init(0x80006000, 0, rc4030[8], serial_hds[0], 1);
+        serial_mm_init(0x80006000, 0, rc4030[8], 8000000/16, serial_hds[0], 1);
     if (serial_hds[1])
-        serial_mm_init(0x80007000, 0, rc4030[9], serial_hds[1], 1);
+        serial_mm_init(0x80007000, 0, rc4030[9], 8000000/16, serial_hds[1], 1);
 
     /* Parallel port */
     if (parallel_hds[0])

hw/mips_malta.c +4 -3

Index: b/hw/mips_malta.c
===================================================================
@@ -449,7 +449,8 @@ static MaltaFPGAState *malta_fpga_init(t
 
     uart_chr = qemu_chr_open("vc");
     qemu_chr_printf(uart_chr, "CBUS UART\r\n");
-    s->uart = serial_mm_init(base + 0x900, 3, env->irq[2], uart_chr, 1);
+    s->uart =
+        serial_mm_init(base + 0x900, 3, env->irq[2], 230400, uart_chr, 1);
 
     malta_fpga_reset(s);
     qemu_register_reset(malta_fpga_reset, s);

@@ -918,9 +919,9 @@ void mips_malta_init (int ram_size, int 
     i8042_init(i8259[1], i8259[12], 0x60);
     rtc_state = rtc_init(0x70, i8259[8]);
     if (serial_hds[0])
-        serial_init(0x3f8, i8259[4], serial_hds[0]);
+        serial_init(0x3f8, i8259[4], 115200, serial_hds[0]);
     if (serial_hds[1])
-        serial_init(0x2f8, i8259[3], serial_hds[1]);
+        serial_init(0x2f8, i8259[3], 115200, serial_hds[1]);
     if (parallel_hds[0])
         parallel_init(0x378, i8259[7], parallel_hds[0]);
     for(i = 0; i < MAX_FD; i++) {

hw/mips_mipssim.c +1 -1

Index: b/hw/mips_mipssim.c
===================================================================
@@ -174,7 +174,7 @@ mips_mipssim_init (int ram_size, int vga
     /* A single 16450 sits at offset 0x3f8. It is attached to
        MIPS CPU INT2, which is interrupt 4. */
     if (serial_hds[0])
-        serial_init(0x3f8, env->irq[4], serial_hds[0]);
+        serial_init(0x3f8, env->irq[4], 115200, serial_hds[0]);
 
     if (nd_table[0].vlan) {
         if (nd_table[0].model == NULL

hw/mips_r4k.c +2 -1

Index: b/hw/mips_r4k.c
===================================================================
@@ -229,7 +229,8 @@ void mips_r4k_init (int ram_size, int vg
 
     for(i = 0; i < MAX_SERIAL_PORTS; i++) {
         if (serial_hds[i]) {
-            serial_init(serial_io[i], i8259[serial_irq[i]], serial_hds[i]);
+            serial_init(serial_io[i], i8259[serial_irq[i]], 115200,
+                        serial_hds[i]);
         }
     }

hw/ppc405_uc.c +1 -1

Index: b/hw/ppc405_uc.c
===================================================================
@@ -1223,7 +1223,7 @@ void ppc405_serial_init (CPUState *env, 
 #ifdef DEBUG_SERIAL
     printf("%s: offset " PADDRX "\n", __func__, offset);
 #endif
-    serial = serial_mm_init(offset, 0, irq, chr, 0);
+    serial = serial_mm_init(offset, 0, irq, 399193, chr, 0);
     ppc4xx_mmio_register(env, mmio, offset, 0x008,
                          serial_mm_read, serial_mm_write, serial);
 }

hw/ppc_chrp.c +1 -1

Index: b/hw/ppc_chrp.c
===================================================================
@@ -264,7 +264,7 @@ static void ppc_core99_init (int ram_siz
     dummy_irq = i8259_init(NULL);
 
     /* XXX: use Mac Serial port */
-    serial_init(0x3f8, dummy_irq[4], serial_hds[0]);
+    serial_init(0x3f8, dummy_irq[4], 115200, serial_hds[0]);
     for(i = 0; i < nb_nics; i++) {
         if (!nd_table[i].model)
             nd_table[i].model = "ne2k_pci";

hw/ppc_oldworld.c +1 -1

Index: b/hw/ppc_oldworld.c
===================================================================
@@ -287,7 +287,7 @@ static void ppc_heathrow_init (int ram_s
     dummy_irq = i8259_init(NULL);
 
     /* XXX: use Mac Serial port */
-    serial_init(0x3f8, dummy_irq[4], serial_hds[0]);
+    serial_init(0x3f8, dummy_irq[4], 115200, serial_hds[0]);
 
     for(i = 0; i < nb_nics; i++) {
         if (!nd_table[i].model)

hw/ppc_prep.c +1 -1

Index: b/hw/ppc_prep.c
===================================================================
@@ -667,7 +667,7 @@ static void ppc_prep_init (int ram_size,
     //    pit = pit_init(0x40, i8259[0]);
     rtc_init(0x70, i8259[8]);
 
-    serial_init(0x3f8, i8259[4], serial_hds[0]);
+    serial_init(0x3f8, i8259[4], 115200, serial_hds[0]);
     nb_nics1 = nb_nics;
     if (nb_nics1 > NE2000_NB_MAX)
         nb_nics1 = NE2000_NB_MAX;