Re: [Qemu-devel] [PATCH 2/2] 8250: Customized base baudrate

[andrzej zaborowski]

On 13/04/2008, Jan Kiszka <address@hidden> wrote:
> 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 <address@hidden>
>  ---
>   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)
>  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.

>      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,
>  -                            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).

>          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;
>
>
>


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