[Qemu-devel] [PATCH v1 00/11] Cadence UART cleanups and Tx path fixes

[Qemu-devel] [PATCH v1 00/11] Cadence UART cleanups and Tx path fixes

[Peter Crosthwaite]

When using QEMU in some terminal environments, char back-ends for serial
devices can return EAGAIN for non trivial periods. This coupled with use
of qemu_chr_fe_write_all() is a leading cause of:

main-loop: WARNING: I/O thread spun for 1000 iterations

This series fixes this for cadence_uart by replacing the open loop
tx-write logic with a proper closed-loop that is responsive to the
back-end short return.

Various trivial cleanups are at the front of the series. Last patch is
the main event.

There is also a major performance increase with removal of the tx timer
logic (P7).

As a test, I've been using QEMU in a terminal over trans-pacific SSH.
Before this series:

$ time ./qemu-build/arm-softmmu/qemu-system-arm -M xilinx-zynq-a9 ...
main-loop: WARNING: I/O thread spun for 1000 iterations
Booting Linux on physical CPU 0x0
...
PetaLinux v2013.04 (Yocto 1.3) zynq ttyPS0

zynq login: QEMU: Terminated

real    2m26.908s
user    1m41.430s
sys     1m7.116s

And after:

$ time ./qemu-build/arm-softmmu/qemu-system-arm -M xilinx-zynq-a9 ...
Booting Linux on physical CPU 0x0
...
PetaLinux v2013.04 (Yocto 1.3) zynq ttyPS0

zynq login: QEMU: Terminated

real    0m16.149s
user    0m10.357s
sys     0m0.164s


Peter Crosthwaite (11):
  char/cadence_uart: Mark struct fields as public/private
  char/cadence_uart: Add missing uart_update_state
  char/cadence_uart: Fix reset.
  char/cadence_uart: s/r_fifo/rx_fifo
  char/cadence_uart: Simplify status generation
  char/cadence_uart: Define Missing SR/ISR fields
  char/cadence_uart: Remove TX timer & add TX FIFO state
  char/cadence_uart: Fix can_receive logic
  char/cadence_uart: Use the TX fifo for transmission
  char/cadence_uart: Delete redundant rx rst logic
  char/cadence_uart: Implement Tx flow control

 hw/char/cadence_uart.c | 153 ++++++++++++++++++++++++++++---------------------
 1 file changed, 87 insertions(+), 66 deletions(-)

-- 
1.8.5.1

[Qemu-devel] [PATCH v1 01/11] char/cadence_uart: Mark struct fields as public/private

[Peter Crosthwaite]

As per current QOM conventions.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/char/cadence_uart.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index f18db53..2f19a53 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -110,7 +110,9 @@
 #define CADENCE_UART(obj) OBJECT_CHECK(UartState, (obj), TYPE_CADENCE_UART)
 
 typedef struct {
+    /* <private> */
     SysBusDevice parent_obj;
+    /* <public> */
 
     MemoryRegion iomem;
     uint32_t r[R_MAX];
-- 
1.8.5.1

[Qemu-devel] [PATCH v1 02/11] char/cadence_uart: Add missing uart_update_state

[Peter Crosthwaite]

This should be rechecked on bus write accesses as such accesses may
change the underlying state that generates the interrupt. Particular
relevant for when the guest touches the interrupt status or mask.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/char/cadence_uart.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index 2f19a53..a43f92c 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -403,6 +403,7 @@ static void uart_write(void *opaque, hwaddr offset,
         uart_parameters_setup(s);
         break;
     }
+    uart_update_status(s);
 }
 
 static uint64_t uart_read(void *opaque, hwaddr offset,
-- 
1.8.5.1

[Qemu-devel] [PATCH v1 03/11] char/cadence_uart: Fix reset.

[Peter Crosthwaite]

Don't reset the uart as an init step. Register the reset function as a
proper reset fn instead.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/char/cadence_uart.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index a43f92c..263d31e 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -431,8 +431,10 @@ static const MemoryRegionOps uart_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static void cadence_uart_reset(UartState *s)
+static void cadence_uart_reset(DeviceState *dev)
 {
+    UartState *s = CADENCE_UART(dev);
+
     s->r[R_CR] = 0x00000128;
     s->r[R_IMR] = 0;
     s->r[R_CISR] = 0;
@@ -465,8 +467,6 @@ static int cadence_uart_init(SysBusDevice *dev)
 
     s->chr = qemu_char_get_next_serial();
 
-    cadence_uart_reset(s);
-
     if (s->chr) {
         qemu_chr_add_handlers(s->chr, uart_can_receive, uart_receive,
                               uart_event, s);
@@ -508,6 +508,7 @@ static void cadence_uart_class_init(ObjectClass *klass, void *data)
 
     sdc->init = cadence_uart_init;
     dc->vmsd = &vmstate_cadence_uart;
+    dc->reset = cadence_uart_reset;
 }
 
 static const TypeInfo cadence_uart_info = {
-- 
1.8.5.1

[Qemu-devel] [PATCH v1 04/11] char/cadence_uart: s/r_fifo/rx_fifo

[Peter Crosthwaite]

Rename this field to match the many other uses of "rx". Xilinx
docmentation (UG585) also refers to this as "RxFIFO".

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/char/cadence_uart.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index 263d31e..e12d50e 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -116,7 +116,7 @@ typedef struct {
 
     MemoryRegion iomem;
     uint32_t r[R_MAX];
-    uint8_t r_fifo[RX_FIFO_SIZE];
+    uint8_t rx_fifo[RX_FIFO_SIZE];
     uint32_t rx_wpos;
     uint32_t rx_count;
     uint64_t char_tx_time;
@@ -280,7 +280,7 @@ static void uart_write_rx_fifo(void *opaque, const uint8_t *buf, int size)
         s->r[R_CISR] |= UART_INTR_ROVR;
     } else {
         for (i = 0; i < size; i++) {
-            s->r_fifo[s->rx_wpos] = buf[i];
+            s->rx_fifo[s->rx_wpos] = buf[i];
             s->rx_wpos = (s->rx_wpos + 1) % RX_FIFO_SIZE;
             s->rx_count++;
 
@@ -344,7 +344,7 @@ static void uart_read_rx_fifo(UartState *s, uint32_t *c)
     if (s->rx_count) {
         uint32_t rx_rpos =
                 (RX_FIFO_SIZE + s->rx_wpos - s->rx_count) % RX_FIFO_SIZE;
-        *c = s->r_fifo[rx_rpos];
+        *c = s->rx_fifo[rx_rpos];
         s->rx_count--;
 
         if (!s->rx_count) {
@@ -492,7 +492,7 @@ static const VMStateDescription vmstate_cadence_uart = {
     .post_load = cadence_uart_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32_ARRAY(r, UartState, R_MAX),
-        VMSTATE_UINT8_ARRAY(r_fifo, UartState, RX_FIFO_SIZE),
+        VMSTATE_UINT8_ARRAY(rx_fifo, UartState, RX_FIFO_SIZE),
         VMSTATE_UINT32(rx_count, UartState),
         VMSTATE_UINT32(rx_wpos, UartState),
         VMSTATE_TIMER(fifo_trigger_handle, UartState),
-- 
1.8.5.1

[Qemu-devel] [PATCH v1 05/11] char/cadence_uart: Simplify status generation

[Peter Crosthwaite]

The status register bits are always pure functions of other device
state. Move the generation of these bits to the update_status()
function to simplify. Makes developing much easier as theres now no need
to recheck status bits on all the changes to rx/tx fifo state.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/char/cadence_uart.c | 33 ++++++++-------------------------
 1 file changed, 8 insertions(+), 25 deletions(-)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index e12d50e..1d006c5 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -128,6 +128,13 @@ typedef struct {
 
 static void uart_update_status(UartState *s)
 {
+    s->r[R_SR] = 0;
+
+    s->r[R_SR] |= s->rx_count == RX_FIFO_SIZE ? UART_SR_INTR_RFUL : 0;
+    s->r[R_SR] |= !s->rx_count ? UART_SR_INTR_REMPTY : 0;
+    s->r[R_SR] |= s->rx_count >= s->r[R_RTRIG] ? UART_SR_INTR_RTRIG : 0;
+
+    s->r[R_SR] |= UART_SR_INTR_TEMPTY;
     s->r[R_CISR] |= s->r[R_SR] & UART_SR_TO_CISR_MASK;
     qemu_set_irq(s->irq, !!(s->r[R_IMR] & s->r[R_CISR]));
 }
@@ -166,15 +173,10 @@ static void uart_rx_reset(UartState *s)
     if (s->chr) {
         qemu_chr_accept_input(s->chr);
     }
-
-    s->r[R_SR] |= UART_SR_INTR_REMPTY;
-    s->r[R_SR] &= ~UART_SR_INTR_RFUL;
 }
 
 static void uart_tx_reset(UartState *s)
 {
-    s->r[R_SR] |= UART_SR_INTR_TEMPTY;
-    s->r[R_SR] &= ~UART_SR_INTR_TFUL;
 }
 
 static void uart_send_breaks(UartState *s)
@@ -274,8 +276,6 @@ static void uart_write_rx_fifo(void *opaque, const uint8_t *buf, int size)
         return;
     }
 
-    s->r[R_SR] &= ~UART_SR_INTR_REMPTY;
-
     if (s->rx_count == RX_FIFO_SIZE) {
         s->r[R_CISR] |= UART_INTR_ROVR;
     } else {
@@ -283,15 +283,6 @@ static void uart_write_rx_fifo(void *opaque, const uint8_t *buf, int size)
             s->rx_fifo[s->rx_wpos] = buf[i];
             s->rx_wpos = (s->rx_wpos + 1) % RX_FIFO_SIZE;
             s->rx_count++;
-
-            if (s->rx_count == RX_FIFO_SIZE) {
-                s->r[R_SR] |= UART_SR_INTR_RFUL;
-                break;
-            }
-
-            if (s->rx_count >= s->r[R_RTRIG]) {
-                s->r[R_SR] |= UART_SR_INTR_RTRIG;
-            }
         }
         timer_mod(s->fifo_trigger_handle, new_rx_time +
                                                 (s->char_tx_time * 4));
@@ -339,26 +330,17 @@ static void uart_read_rx_fifo(UartState *s, uint32_t *c)
         return;
     }
 
-    s->r[R_SR] &= ~UART_SR_INTR_RFUL;
-
     if (s->rx_count) {
         uint32_t rx_rpos =
                 (RX_FIFO_SIZE + s->rx_wpos - s->rx_count) % RX_FIFO_SIZE;
         *c = s->rx_fifo[rx_rpos];
         s->rx_count--;
 
-        if (!s->rx_count) {
-            s->r[R_SR] |= UART_SR_INTR_REMPTY;
-        }
         qemu_chr_accept_input(s->chr);
     } else {
         *c = 0;
-        s->r[R_SR] |= UART_SR_INTR_REMPTY;
     }
 
-    if (s->rx_count < s->r[R_RTRIG]) {
-        s->r[R_SR] &= ~UART_SR_INTR_RTRIG;
-    }
     uart_update_status(s);
 }
 
@@ -447,6 +429,7 @@ static void cadence_uart_reset(DeviceState *dev)
 
     s->rx_count = 0;
     s->rx_wpos = 0;
+    uart_update_status(s);
 }
 
 static int cadence_uart_init(SysBusDevice *dev)
-- 
1.8.5.1

[Qemu-devel] [PATCH v1 06/11] char/cadence_uart: Define Missing SR/ISR fields

[Peter Crosthwaite]

Some (interrupt) status register bits relating to the TxFIFO path were
not defined. Define them. This prepares support for proper Tx data path
flow control.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/char/cadence_uart.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index 1d006c5..a9700f7 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -34,6 +34,9 @@
 #define UART_SR_INTR_RFUL      0x00000004
 #define UART_SR_INTR_TEMPTY    0x00000008
 #define UART_SR_INTR_TFUL      0x00000010
+/* somewhat awkwardly, TTRIG is misaligned between SR and ISR */
+#define UART_SR_TTRIG          0x00002000
+#define UART_INTR_TTRIG        0x00000400
 /* bits fields in CSR that correlate to CISR. If any of these bits are set in
  * SR, then the same bit in CISR is set high too */
 #define UART_SR_TO_CISR_MASK   0x0000001F
@@ -43,6 +46,7 @@
 #define UART_INTR_PARE         0x00000080
 #define UART_INTR_TIMEOUT      0x00000100
 #define UART_INTR_DMSI         0x00000200
+#define UART_INTR_TOVR         0x00001000
 
 #define UART_SR_RACTIVE    0x00000400
 #define UART_SR_TACTIVE    0x00000800
-- 
1.8.5.1

[Qemu-devel] [PATCH v1 07/11] char/cadence_uart: Remove TX timer & add TX FIFO state

[Peter Crosthwaite]

This tx timer implementation is flawed. Despite the controller
attempting to time the guest visable assertion of the TX-empty status
bit (and corresponding interrupt) the controller is still transmitting
characters instantaneously. There is also no sense of multiple character
delay.

The only side effect of this timer is assertion of tx-empty status. So
just remove the timer completely and hold tx-empty as permanently
asserted (its reset status). This matches the actual behaviour of
instantaneous transmission.

While we are VMSD version bumping, add the tx_fifo as device state to
prepare for upcomming TxFIFO flow control. Implement the interrupt
generation logic for the TxFIFO occupancy.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/char/cadence_uart.c | 44 +++++++++++++-------------------------------
 1 file changed, 13 insertions(+), 31 deletions(-)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index a9700f7..f28e503 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -121,13 +121,14 @@ typedef struct {
     MemoryRegion iomem;
     uint32_t r[R_MAX];
     uint8_t rx_fifo[RX_FIFO_SIZE];
+    uint8_t tx_fifo[TX_FIFO_SIZE];
     uint32_t rx_wpos;
     uint32_t rx_count;
+    uint32_t tx_count;
     uint64_t char_tx_time;
     CharDriverState *chr;
     qemu_irq irq;
     QEMUTimer *fifo_trigger_handle;
-    QEMUTimer *tx_time_handle;
 } UartState;
 
 static void uart_update_status(UartState *s)
@@ -138,8 +139,12 @@ static void uart_update_status(UartState *s)
     s->r[R_SR] |= !s->rx_count ? UART_SR_INTR_REMPTY : 0;
     s->r[R_SR] |= s->rx_count >= s->r[R_RTRIG] ? UART_SR_INTR_RTRIG : 0;
 
-    s->r[R_SR] |= UART_SR_INTR_TEMPTY;
+    s->r[R_SR] |= s->tx_count == TX_FIFO_SIZE ? UART_SR_INTR_TFUL : 0;
+    s->r[R_SR] |= !s->tx_count ? UART_SR_INTR_TEMPTY : 0;
+    s->r[R_SR] |= s->tx_count >= s->r[R_TTRIG] ? UART_SR_TTRIG : 0;
+
     s->r[R_CISR] |= s->r[R_SR] & UART_SR_TO_CISR_MASK;
+    s->r[R_CISR] |= s->r[R_SR] & UART_SR_TTRIG ? UART_INTR_TTRIG : 0;
     qemu_set_irq(s->irq, !!(s->r[R_IMR] & s->r[R_CISR]));
 }
 
@@ -152,24 +157,6 @@ static void fifo_trigger_update(void *opaque)
     uart_update_status(s);
 }
 
-static void uart_tx_redo(UartState *s)
-{
-    uint64_t new_tx_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-
-    timer_mod(s->tx_time_handle, new_tx_time + s->char_tx_time);
-
-    s->r[R_SR] |= UART_SR_INTR_TEMPTY;
-
-    uart_update_status(s);
-}
-
-static void uart_tx_write(void *opaque)
-{
-    UartState *s = (UartState *)opaque;
-
-    uart_tx_redo(s);
-}
-
 static void uart_rx_reset(UartState *s)
 {
     s->rx_wpos = 0;
@@ -181,6 +168,7 @@ static void uart_rx_reset(UartState *s)
 
 static void uart_tx_reset(UartState *s)
 {
+    s->tx_count = 0;
 }
 
 static void uart_send_breaks(UartState *s)
@@ -261,10 +249,6 @@ static void uart_ctrl_update(UartState *s)
 
     s->r[R_CR] &= ~(UART_CR_TXRST | UART_CR_RXRST);
 
-    if ((s->r[R_CR] & UART_CR_TX_EN) && !(s->r[R_CR] & UART_CR_TX_DIS)) {
-            uart_tx_redo(s);
-    }
-
     if (s->r[R_CR] & UART_CR_STARTBRK && !(s->r[R_CR] & UART_CR_STOPBRK)) {
         uart_send_breaks(s);
     }
@@ -447,9 +431,6 @@ static int cadence_uart_init(SysBusDevice *dev)
     s->fifo_trigger_handle = timer_new_ns(QEMU_CLOCK_VIRTUAL,
             (QEMUTimerCB *)fifo_trigger_update, s);
 
-    s->tx_time_handle = timer_new_ns(QEMU_CLOCK_VIRTUAL,
-            (QEMUTimerCB *)uart_tx_write, s);
-
     s->char_tx_time = (get_ticks_per_sec() / 9600) * 10;
 
     s->chr = qemu_char_get_next_serial();
@@ -473,17 +454,18 @@ static int cadence_uart_post_load(void *opaque, int version_id)
 
 static const VMStateDescription vmstate_cadence_uart = {
     .name = "cadence_uart",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .minimum_version_id_old = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .minimum_version_id_old = 2,
     .post_load = cadence_uart_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32_ARRAY(r, UartState, R_MAX),
         VMSTATE_UINT8_ARRAY(rx_fifo, UartState, RX_FIFO_SIZE),
+        VMSTATE_UINT8_ARRAY(tx_fifo, UartState, RX_FIFO_SIZE),
         VMSTATE_UINT32(rx_count, UartState),
+        VMSTATE_UINT32(tx_count, UartState),
         VMSTATE_UINT32(rx_wpos, UartState),
         VMSTATE_TIMER(fifo_trigger_handle, UartState),
-        VMSTATE_TIMER(tx_time_handle, UartState),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.8.5.1

[Qemu-devel] [PATCH v1 08/11] char/cadence_uart: Fix can_receive logic

[Peter Crosthwaite]

The can_receive logic was only taking into account the RxFIFO
occupancy. RxFIFO population is only used for the echo and normal modes
however. Improve the logic to correctly return the true number of
receivable characters based on the current mode:

Normal mode: RxFIFO vacancy.
Remote loopback: TxFIFO vacancy.
Echo mode: The min of the TxFIFO and RxFIFO vacancies.
Local Loopback: Return non-zero (to implement droppage)

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/char/cadence_uart.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index f28e503..85e9a00 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -233,8 +233,16 @@ static void uart_parameters_setup(UartState *s)
 static int uart_can_receive(void *opaque)
 {
     UartState *s = (UartState *)opaque;
+    int ret = MAX(RX_FIFO_SIZE, TX_FIFO_SIZE);
+    uint32_t ch_mode = s->r[R_MR] & UART_MR_CHMODE;
 
-    return RX_FIFO_SIZE - s->rx_count;
+    if (ch_mode == NORMAL_MODE || ch_mode == ECHO_MODE) {
+        ret = MIN(ret, RX_FIFO_SIZE - s->rx_count);
+    }
+    if (ch_mode == REMOTE_LOOPBACK || ch_mode == ECHO_MODE) {
+        ret = MIN(ret, TX_FIFO_SIZE - s->tx_count);
+    }
+    return ret;
 }
 
 static void uart_ctrl_update(UartState *s)
-- 
1.8.5.1

[Qemu-devel] [PATCH v1 09/11] char/cadence_uart: Use the TX fifo for transmission

[Peter Crosthwaite]

Populate the TxFIFO with the Tx data before sending. Prepares
support for proper Tx flow control implementation.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/char/cadence_uart.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index 85e9a00..cffb4c3 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -292,7 +292,22 @@ static void uart_write_tx_fifo(UartState *s, const uint8_t *buf, int size)
         return;
     }
 
-    qemu_chr_fe_write_all(s->chr, buf, size);
+    if (size > TX_FIFO_SIZE - s->tx_count) {
+        size = TX_FIFO_SIZE - s->tx_count;
+        /*
+         * This can only be a guest error via a bad tx fifo register push,
+         * as can_receive() should stop remote loop and echo modes ever getting
+         * us to here.
+         */
+        qemu_log_mask(LOG_GUEST_ERROR, "cadence_uart: TxFIFO overflow");
+        s->r[R_CISR] |= UART_INTR_ROVR;
+    }
+
+    memcpy(s->tx_fifo + s->tx_count, buf, size);
+    s->tx_count += size;
+
+    qemu_chr_fe_write_all(s->chr, s->tx_fifo, s->tx_count);
+    s->tx_count = 0;
 }
 
 static void uart_receive(void *opaque, const uint8_t *buf, int size)
-- 
1.8.5.1

[Qemu-devel] [PATCH v1 10/11] char/cadence_uart: Delete redundant rx rst logic

[Peter Crosthwaite]

uart_rx_reset() called immediately above already does this. Remove.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/char/cadence_uart.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index cffb4c3..3be110c 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -438,8 +438,6 @@ static void cadence_uart_reset(DeviceState *dev)
     uart_rx_reset(s);
     uart_tx_reset(s);
 
-    s->rx_count = 0;
-    s->rx_wpos = 0;
     uart_update_status(s);
 }
 
-- 
1.8.5.1

[Qemu-devel] [PATCH v1 11/11] char/cadence_uart: Implement Tx flow control

[Peter Crosthwaite]

If the UART back-end blocks, buffer in the Tx FIFO to try again later.
This stops the IO-thread busy waiting on char back-ends (which causes
all sorts of performance problems).

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/char/cadence_uart.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index 3be110c..6516a3e 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -286,6 +286,34 @@ static void uart_write_rx_fifo(void *opaque, const uint8_t *buf, int size)
     uart_update_status(s);
 }
 
+static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond,
+                                  void *opaque)
+{
+    UartState *s = opaque;
+    int ret;
+
+    /* instant drain the fifo when there's no back-end */
+    if (!s->chr) {
+        s->tx_count = 0;
+    }
+
+    if (!s->tx_count) {
+        return FALSE;
+    }
+
+    ret = qemu_chr_fe_write(s->chr, s->tx_fifo, s->tx_count);
+    s->tx_count -= ret;
+    memmove(s->tx_fifo, s->tx_fifo + ret, s->tx_count);
+
+    if (s->tx_count) {
+        int r = qemu_chr_fe_add_watch(s->chr, G_IO_OUT, cadence_uart_xmit, s);
+        assert(r);
+    }
+
+    uart_update_status(s);
+    return FALSE;
+}
+
 static void uart_write_tx_fifo(UartState *s, const uint8_t *buf, int size)
 {
     if ((s->r[R_CR] & UART_CR_TX_DIS) || !(s->r[R_CR] & UART_CR_TX_EN)) {
@@ -306,8 +334,7 @@ static void uart_write_tx_fifo(UartState *s, const uint8_t *buf, int size)
     memcpy(s->tx_fifo + s->tx_count, buf, size);
     s->tx_count += size;
 
-    qemu_chr_fe_write_all(s->chr, s->tx_fifo, s->tx_count);
-    s->tx_count = 0;
+    cadence_uart_xmit(NULL, G_IO_OUT, s);
 }
 
 static void uart_receive(void *opaque, const uint8_t *buf, int size)
-- 
1.8.5.1

Re: [Qemu-devel] [PATCH v1 01/11] char/cadence_uart: Mark struct fields as public/private

[Andreas Färber]

Am 17.12.2013 02:40, schrieb Peter Crosthwaite:
> As per current QOM conventions.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> 
>  hw/char/cadence_uart.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index f18db53..2f19a53 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -110,7 +110,9 @@
>  #define CADENCE_UART(obj) OBJECT_CHECK(UartState, (obj), TYPE_CADENCE_UART)
>  
>  typedef struct {
> +    /* <private> */
>      SysBusDevice parent_obj;
> +    /* <public> */

Note that it's /*< ... >*/ if I'm not mistaken.

However since this is outside include/ it won't get parsed by the
patches flying around. This being Cadence IP that is likely to be reused
on other SoCs, I do guess we'll end up with an
include/hw/char/cadence_uart.h at some point.

Cheers,
Andreas

>  
>      MemoryRegion iomem;
>      uint32_t r[R_MAX];

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v1 01/11] char/cadence_uart: Mark struct fields as public/private

[Peter Crosthwaite]

On Thu, Dec 19, 2013 at 9:15 AM, Andreas Färber <afaerber@suse.de> wrote:
> Am 17.12.2013 02:40, schrieb Peter Crosthwaite:
>> As per current QOM conventions.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/char/cadence_uart.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
>> index f18db53..2f19a53 100644
>> --- a/hw/char/cadence_uart.c
>> +++ b/hw/char/cadence_uart.c
>> @@ -110,7 +110,9 @@
>>  #define CADENCE_UART(obj) OBJECT_CHECK(UartState, (obj), TYPE_CADENCE_UART)
>>
>>  typedef struct {
>> +    /* <private> */
>>      SysBusDevice parent_obj;
>> +    /* <public> */
>
> Note that it's /*< ... >*/ if I'm not mistaken.

Thanks, will fix v2.

>
> However since this is outside include/ it won't get parsed by the
> patches flying around. This being Cadence IP that is likely to be reused
> on other SoCs, I do guess we'll end up with an
> include/hw/char/cadence_uart.h at some point.
>

Well ideally all peripherals are headerified for the sake of
consistency. I dont think we should be making judgement calls on
coding style based on target usages.

I'm patching old style files as I notice issue just in general
browsing/developing hopefully to one day bring them into line.

Regards,
Peter

> Cheers,
> Andreas
>
>>
>>      MemoryRegion iomem;
>>      uint32_t r[R_MAX];
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>