From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KRAhn-0006wB-1K for qemu-devel@nongnu.org; Thu, 07 Aug 2008 14:59:23 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KRAhl-0006vf-0q for qemu-devel@nongnu.org; Thu, 07 Aug 2008 14:59:22 -0400 Received: from [199.232.76.173] (port=46594 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KRAhk-0006vc-Nz for qemu-devel@nongnu.org; Thu, 07 Aug 2008 14:59:20 -0400 Received: from wr-out-0506.google.com ([64.233.184.239]:55226) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KRAhk-0007GD-HT for qemu-devel@nongnu.org; Thu, 07 Aug 2008 14:59:20 -0400 Received: by wr-out-0506.google.com with SMTP id c46so504878wra.18 for ; Thu, 07 Aug 2008 11:59:19 -0700 (PDT) Message-ID: <489B45E3.9040005@codemonkey.ws> Date: Thu, 07 Aug 2008 13:58:43 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] upgrading emulated UART to 16550A References: <489B3278.4080001@eu.citrix.com> In-Reply-To: <489B3278.4080001@eu.citrix.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Stefano Stabellini wrote: > This is an improved version of the UART 16550A emulation patch. > The changes compared to previous version are: > > - no token bucket anymore; > > - fixed a small bug handling IRQs; this was the problem that prevented > kgdb to work over the serial (thanks to Jason Wessel for the help > spotting and reproducing this bug). > > Signed-off-by: Stefano Stabellini > > --- > > diff --git a/hw/serial.c b/hw/serial.c > index ffd6ac9..5aab00e 100644 > --- a/hw/serial.c > +++ b/hw/serial.c > @@ -1,7 +1,7 @@ > /* > - * QEMU 16450 UART emulation > + * QEMU 16550A UART emulation > * > - * Copyright (c) 2003-2004 Fabrice Bellard > + * Copyright (c) 2003-2008 Fabrice Bellard > I think the idea was to add a new copyright entry, not to modify Fabrice's copyright. > * > * Permission is hereby granted, free of charge, to any person obtaining a copy > * of this software and associated documentation files (the "Software"), to deal > @@ -21,6 +21,10 @@ > * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > * THE SOFTWARE. > */ > + > +#include > +#include > This doesn't look Windows friendly. > @@ -100,55 +131,105 @@ struct SerialState { > target_phys_addr_t base; > int it_shift; > int baudbase; > - QEMUTimer *tx_timer; > - int tx_burst; > + int tsr_retry; > + > + uint64_t last_xmit_ts; /* Time when the last byte was successfully sent out of the tsr */ > + SerialFIFO recv_fifo; > + SerialFIFO xmit_fifo; > + > + struct QEMUTimer *fifo_timeout_timer; > + int timeout_ipending; /* timeout interrupt pending state */ > + struct QEMUTimer *transmit_timer; > + > + > + uint64_t char_transmit_time; /* time to transmit a char in ticks*/ > + int poll_msl; > + > + struct QEMUTimer *modem_status_poll; > }; > > +static void serial_receive1(void *opaque, const uint8_t *buf, int size); > + > +static void fifo_clear(SerialState *s, int fifo) { > minor nit, but the '{' should be on a new line. > + SerialFIFO *f = ( fifo ) ? &s->recv_fifo : &s->xmit_fifo; > + memset(f->data, 0, UART_FIFO_LENGTH); > + f->count = 0; > + f->head = 0; > + f->tail = 0; > +} > + > +static int fifo_put(SerialState *s, int fifo, uint8_t chr) { > + SerialFIFO *f = ( fifo ) ? &s->recv_fifo : &s->xmit_fifo; > + > + f->data[f->head++] = chr; > + > + if (f->head == UART_FIFO_LENGTH) > + f->head = 0; > + f->count++; > + > + return 1; > +} > + > +uint8_t fifo_get(SerialState *s, int fifo) { > Any reason for this not to be static? > -static void serial_tx_done(void *opaque) > -{ > - SerialState *s = opaque; > > - if (s->tx_burst < 0) { > - uint16_t divider; > + if ( ( s->ier & UART_IER_RLSI ) && (s->lsr & UART_LSR_INT_ANY ) ) { > + tmp_iir = UART_IIR_RLSI; > + } else if ( s->timeout_ipending ) { > + tmp_iir = UART_IIR_CTI; > + } else if ( ( s->ier & UART_IER_RDI ) && (s->lsr & UART_LSR_DR ) ) { > + if ( !(s->fcr & UART_FCR_FE) ) { > + tmp_iir = UART_IIR_RDI; > + } else if ( s->recv_fifo.count >= s->recv_fifo.itl ) { > + tmp_iir = UART_IIR_RDI; > + } > + } else if ( (s->ier & UART_IER_THRI) && s->thr_ipending ) { > + tmp_iir = UART_IIR_THRI; > + } else if ( (s->ier & UART_IER_MSI) && (s->msr & UART_MSR_ANY_DELTA) ) { > + tmp_iir = UART_IIR_MSI; > + } > > - if (s->divider) > - divider = s->divider; > - else > - divider = 1; > + s->iir = tmp_iir | ( s->iir & 0xF0 ); > > - /* We assume 10 bits/char, OK for this purpose. */ > - s->tx_burst = THROTTLE_TX_INTERVAL * 1000 / > - (1000000 * 10 / (s->baudbase / divider)); > + if ( tmp_iir != UART_IIR_NO_INT ) { > + qemu_irq_raise(s->irq); > + } else { > + qemu_irq_lower(s->irq); > More nits, lose the whitespace in the conditions. For instance: } else if ( ( s->ier & UART_IER_RDI ) => } else if ((s->ier & UART_IER_RDI). The rest of the file uses the later style so it's a little weird to have portions of the code be different. > } > - s->thr_ipending = 1; > - s->lsr |= UART_LSR_THRE; > - s->lsr |= UART_LSR_TEMT; > - serial_update_irq(s); > } > > static void serial_update_parameters(SerialState *s) > { > - int speed, parity, data_bits, stop_bits; > + int speed, parity, data_bits, stop_bits, frame_size; > QEMUSerialSetParams ssp; > > + if (s->divider == 0) > + return; > + > + frame_size = 1; > if (s->lcr & 0x08) { > if (s->lcr & 0x10) > parity = 'E'; > @@ -156,19 +237,21 @@ static void serial_update_parameters(SerialState *s) > parity = 'O'; > } else { > parity = 'N'; > + frame_size = 0; > } > if (s->lcr & 0x04) > stop_bits = 2; > else > stop_bits = 1; > + > data_bits = (s->lcr & 0x03) + 5; > - if (s->divider == 0) > - return; > + frame_size += data_bits + stop_bits; > speed = s->baudbase / s->divider; > ssp.speed = speed; > ssp.parity = parity; > ssp.data_bits = data_bits; > ssp.stop_bits = stop_bits; > + s->char_transmit_time = ( ticks_per_sec / speed ) * frame_size; > qemu_chr_ioctl(s->chr, CHR_IOCTL_SERIAL_SET_PARAMS, &ssp); > #if 0 > printf("speed=%d parity=%c data=%d stop=%d\n", > @@ -176,6 +259,88 @@ static void serial_update_parameters(SerialState *s) > #endif > } > > +static void serial_update_msl( SerialState *s ) > More bad whitespace. > +static void serial_xmit(void *opaque) { > + SerialState *s = opaque; > + uint64_t new_xmit_ts = qemu_get_clock(vm_clock); > + > + if ( s->tsr_retry <= 0 ) { > + if (s->fcr & UART_FCR_FE) { > + s->tsr = fifo_get(s,XMIT_FIFO); > + if ( !s->xmit_fifo.count ) > + s->lsr |= UART_LSR_THRE; > + } else { > + s->tsr = s->thr; > + s->lsr |= UART_LSR_THRE; > + } > + } > + > + if ( (s->mcr & UART_MCR_LOOP > + /* in loopback mode, say that we just received a char */ > + ? (serial_receive1(s, &s->tsr, 1), 1) > + : qemu_chr_write(s->chr, &s->tsr, 1)) > This is just nutty :-) Please rewrite this if() statement to be a little less obscure. > @@ -524,18 +832,11 @@ SerialState *serial_mm_init (target_phys_addr_t base, int it_shift, > s = qemu_mallocz(sizeof(SerialState)); > if (!s) > return NULL; > - 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) > - return NULL; > - > - qemu_register_reset(serial_reset, s); > - serial_reset(s); > > + serial_init_core(s, irq, baudbase, chr); > register_savevm("serial", base, 2, serial_save, serial_load, s); > Isn't it necessary to bump the savevm() version number since you've changed the format. Regards, Anthony Liguori