diff mbox

[V2,2/4] grlib-apbuart: Add support of various flags

Message ID 1361290933-4748-3-git-send-email-chouteau@adacore.com
State New
Headers show

Commit Message

Fabien Chouteau Feb. 19, 2013, 4:22 p.m. UTC 
From: Ronald Hecht <ronald.hecht@gmx.de>

 - enable/disable Rx and Tx
 - Rx and Tx interrupt
 - Tx FIFO empty and Tx SHIFT empty

Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
 hw/grlib_apbuart.c |   36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

Comments

Andreas Färber Feb. 23, 2013, 5:13 p.m. UTC | #1 
Fabien,

Am 19.02.2013 17:22, schrieb Fabien Chouteau:
> From: Ronald Hecht <ronald.hecht@gmx.de>
> 
>  - enable/disable Rx and Tx
>  - Rx and Tx interrupt
>  - Tx FIFO empty and Tx SHIFT empty
> 
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> ---
>  hw/grlib_apbuart.c |   36 +++++++++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/grlib_apbuart.c b/hw/grlib_apbuart.c
> index 3a61788..ba1685a 100644
> --- a/hw/grlib_apbuart.c
> +++ b/hw/grlib_apbuart.c

> @@ -242,6 +250,19 @@ static int grlib_apbuart_init(SysBusDevice *dev)
>      return 0;
>  }
>  
> +static void grlib_apbuart_reset(DeviceState *d)
> +{
> +    UART *uart = container_of(d, UART, busdev.qdev);

This is still wrong, please introduce a QOM cast macro for this device,
e.g., GRLIB_APB_UART(d) due to the unfortunate use of UART rather than
UARTState for the struct.

http://wiki.qemu.org/QOMConventions

Also if you have some cycles, all SysBus init functions such as the one
visible above should be replaced by instance_init and realize functions.

Regards,
Andreas
Fabien Chouteau Feb. 26, 2013, 9:59 a.m. UTC | #2 
On 02/23/2013 06:13 PM, Andreas Färber wrote:
> Am 19.02.2013 17:22, schrieb Fabien Chouteau:
>>
>> +static void grlib_apbuart_reset(DeviceState *d)
>> +{
>> +    UART *uart = container_of(d, UART, busdev.qdev);
>
> This is still wrong, please introduce a QOM cast macro for this device,
> e.g., GRLIB_APB_UART(d) due to the unfortunate use of UART rather than
> UARTState for the struct.

Hello,

Changing UART to UARTState is not difficult, but I don't understand why
the current name is a problem.

>
> http://wiki.qemu.org/QOMConventions
>

I didn't know this page. I think it would be great to have an example
for each point. Or maybe a complete "dummy" device that shows the best
practices.

> Also if you have some cycles, all SysBus init functions such as the one
> visible above should be replaced by instance_init and realize functions.
>

You mean dc->init instead of k->init?
diff mbox

Patch

diff --git a/hw/grlib_apbuart.c b/hw/grlib_apbuart.c
index 3a61788..ba1685a 100644
--- a/hw/grlib_apbuart.c
+++ b/hw/grlib_apbuart.c
@@ -75,7 +75,6 @@  typedef struct UART {
     CharDriverState *chr;
 
     /* registers */
-    uint32_t receive;
     uint32_t status;
     uint32_t control;
 
@@ -136,12 +135,14 @@  static void grlib_apbuart_receive(void *opaque, const uint8_t *buf, int size)
 {
     UART *uart = opaque;
 
-    uart_add_to_fifo(uart, buf, size);
+    if (uart->control & UART_RECEIVE_ENABLE) {
+        uart_add_to_fifo(uart, buf, size);
 
-    uart->status |= UART_DATA_READY;
+        uart->status |= UART_DATA_READY;
 
-    if (uart->control & UART_RECEIVE_INTERRUPT) {
-        qemu_irq_pulse(uart->irq);
+        if (uart->control & UART_RECEIVE_INTERRUPT) {
+            qemu_irq_pulse(uart->irq);
+        }
     }
 }
 
@@ -193,8 +194,15 @@  static void grlib_apbuart_write(void *opaque, hwaddr addr,
     switch (addr) {
     case DATA_OFFSET:
     case DATA_OFFSET + 3:       /* When only one byte write */
-        c = value & 0xFF;
-        qemu_chr_fe_write(uart->chr, &c, 1);
+        /* Transmit when character device available and transmitter enabled */
+        if ((uart->chr) && (uart->control & UART_TRANSMIT_ENABLE)) {
+            c = value & 0xFF;
+            qemu_chr_fe_write(uart->chr, &c, 1);
+            /* Generate interrupt */
+            if (uart->control & UART_TRANSMIT_INTERRUPT) {
+                qemu_irq_pulse(uart->irq);
+            }
+        }
         return;
 
     case STATUS_OFFSET:
@@ -242,6 +250,19 @@  static int grlib_apbuart_init(SysBusDevice *dev)
     return 0;
 }
 
+static void grlib_apbuart_reset(DeviceState *d)
+{
+    UART *uart = container_of(d, UART, busdev.qdev);
+
+    /* Transmitter FIFO and shift registers are always empty in QEMU */
+    uart->status =  UART_TRANSMIT_FIFO_EMPTY | UART_TRANSMIT_SHIFT_EMPTY;
+    /* Everything is off */
+    uart->control = 0;
+    /* Flush receive FIFO */
+    uart->len = 0;
+    uart->current = 0;
+}
+
 static Property grlib_apbuart_properties[] = {
     DEFINE_PROP_CHR("chrdev", UART, chr),
     DEFINE_PROP_END_OF_LIST(),
@@ -253,6 +274,7 @@  static void grlib_apbuart_class_init(ObjectClass *klass, void *data)
     SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
     k->init = grlib_apbuart_init;
+    dc->reset = grlib_apbuart_reset;
     dc->props = grlib_apbuart_properties;
 }