[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 01/11] i.MX: Split the i.MX serial driver int
From: |
Peter Crosthwaite |
Subject: |
Re: [Qemu-devel] [PATCH v8 01/11] i.MX: Split the i.MX serial driver into a header file and a source file |
Date: |
Tue, 30 Jun 2015 00:04:17 -0700 |
On Mon, Jun 29, 2015 at 1:11 PM, Jean-Christophe Dubois
<address@hidden> wrote:
> Also adding a "realise" callback.
>
> This is to prepare to accomodate the SOC requirements.
>
You have also done a significant code style sweep. I would suggest
doing a git add -p and split this into 3 patches:
1: Style cleanups
2: Header movement
3: realize
The hunks look pretty self contained so it should be churn free.
Perhaps do the style cleanup last so there are no "you missed a bit"
for style issues in header-movement/realize changes.
Regards,
Peter
> Signed-off-by: Jean-Christophe Dubois <address@hidden>
> ---
>
> Changes since v1:
> * not present on v1
>
> Changes since v2:
> * not present on v2
>
> Changes since v3:
> * not present on v3
>
> Changes since v4:
> * not present on v4
>
> Changes since v5:
> * not present on v5
>
> Changes since v6:
> * not present on v6
>
> Changes since v7:
> * Splited the i.MX serial emulator into a header file and a source file
>
> hw/char/imx_serial.c | 176
> +++++++++++--------------------------------
> include/hw/char/imx_serial.h | 104 +++++++++++++++++++++++++
> 2 files changed, 149 insertions(+), 131 deletions(-)
> create mode 100644 include/hw/char/imx_serial.h
>
> diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
> index f3fbc77..8c2d071 100644
> --- a/hw/char/imx_serial.c
> +++ b/hw/char/imx_serial.c
> @@ -4,6 +4,7 @@
> * Copyright (c) 2008 OKL
> * Originally Written by Hans Jiang
> * Copyright (c) 2011 NICTA Pty Ltd.
> + * Updated by Jean-Christophe Dubois <address@hidden>
> *
> * This work is licensed under the terms of the GNU GPL, version 2 or later.
> * See the COPYING file in the top-level directory.
> @@ -17,16 +18,14 @@
> * is a real serial device.
> */
>
> -#include "hw/hw.h"
> -#include "hw/sysbus.h"
> +#include "hw/char/imx_serial.h"
> #include "sysemu/sysemu.h"
> #include "sysemu/char.h"
> -#include "hw/arm/imx.h"
>
> //#define DEBUG_SERIAL 1
> #ifdef DEBUG_SERIAL
> #define DPRINTF(fmt, args...) \
> -do { printf("imx_serial: " fmt , ##args); } while (0)
> +do { printf("%s: " fmt , TYPE_IMX_SERIAL, ##args); } while (0)
> #else
> #define DPRINTF(fmt, args...) do {} while (0)
> #endif
> @@ -38,42 +37,13 @@ do { printf("imx_serial: " fmt , ##args); } while (0)
> //#define DEBUG_IMPLEMENTATION 1
> #ifdef DEBUG_IMPLEMENTATION
> # define IPRINTF(fmt, args...) \
> - do { fprintf(stderr, "imx_serial: " fmt, ##args); } while (0)
> + do { fprintf(stderr, "%s: " fmt, TYPE_IMX_SERIAL, ##args); } while (0)
> #else
> # define IPRINTF(fmt, args...) do {} while (0)
> #endif
>
> -#define TYPE_IMX_SERIAL "imx-serial"
> -#define IMX_SERIAL(obj) OBJECT_CHECK(IMXSerialState, (obj), TYPE_IMX_SERIAL)
> -
> -typedef struct IMXSerialState {
> - SysBusDevice parent_obj;
> -
> - MemoryRegion iomem;
> - int32_t readbuff;
> -
> - uint32_t usr1;
> - uint32_t usr2;
> - uint32_t ucr1;
> - uint32_t ucr2;
> - uint32_t uts1;
> -
> - /*
> - * The registers below are implemented just so that the
> - * guest OS sees what it has written
> - */
> - uint32_t onems;
> - uint32_t ufcr;
> - uint32_t ubmr;
> - uint32_t ubrc;
> - uint32_t ucr3;
> -
> - qemu_irq irq;
> - CharDriverState *chr;
> -} IMXSerialState;
> -
> static const VMStateDescription vmstate_imx_serial = {
> - .name = "imx-serial",
> + .name = TYPE_IMX_SERIAL,
> .version_id = 1,
> .minimum_version_id = 1,
> .fields = (VMStateField[]) {
> @@ -91,55 +61,6 @@ static const VMStateDescription vmstate_imx_serial = {
> },
> };
>
> -
> -#define URXD_CHARRDY (1<<15) /* character read is valid */
> -#define URXD_ERR (1<<14) /* Character has error */
> -#define URXD_BRK (1<<11) /* Break received */
> -
> -#define USR1_PARTYER (1<<15) /* Parity Error */
> -#define USR1_RTSS (1<<14) /* RTS pin status */
> -#define USR1_TRDY (1<<13) /* Tx ready */
> -#define USR1_RTSD (1<<12) /* RTS delta: pin changed state */
> -#define USR1_ESCF (1<<11) /* Escape sequence interrupt */
> -#define USR1_FRAMERR (1<<10) /* Framing error */
> -#define USR1_RRDY (1<<9) /* receiver ready */
> -#define USR1_AGTIM (1<<8) /* Aging timer interrupt */
> -#define USR1_DTRD (1<<7) /* DTR changed */
> -#define USR1_RXDS (1<<6) /* Receiver is idle */
> -#define USR1_AIRINT (1<<5) /* Aysnch IR interrupt */
> -#define USR1_AWAKE (1<<4) /* Falling edge detected on RXd pin */
> -
> -#define USR2_ADET (1<<15) /* Autobaud complete */
> -#define USR2_TXFE (1<<14) /* Transmit FIFO empty */
> -#define USR2_DTRF (1<<13) /* DTR/DSR transition */
> -#define USR2_IDLE (1<<12) /* UART has been idle for too long */
> -#define USR2_ACST (1<<11) /* Autobaud counter stopped */
> -#define USR2_RIDELT (1<<10) /* Ring Indicator delta */
> -#define USR2_RIIN (1<<9) /* Ring Indicator Input */
> -#define USR2_IRINT (1<<8) /* Serial Infrared Interrupt */
> -#define USR2_WAKE (1<<7) /* Start bit detected */
> -#define USR2_DCDDELT (1<<6) /* Data Carrier Detect delta */
> -#define USR2_DCDIN (1<<5) /* Data Carrier Detect Input */
> -#define USR2_RTSF (1<<4) /* RTS transition */
> -#define USR2_TXDC (1<<3) /* Transmission complete */
> -#define USR2_BRCD (1<<2) /* Break condition detected */
> -#define USR2_ORE (1<<1) /* Overrun error */
> -#define USR2_RDR (1<<0) /* Receive data ready */
> -
> -#define UCR1_TRDYEN (1<<13) /* Tx Ready Interrupt Enable */
> -#define UCR1_RRDYEN (1<<9) /* Rx Ready Interrupt Enable */
> -#define UCR1_TXMPTYEN (1<<6) /* Tx Empty Interrupt Enable */
> -#define UCR1_UARTEN (1<<0) /* UART Enable */
> -
> -#define UCR2_TXEN (1<<2) /* Transmitter enable */
> -#define UCR2_RXEN (1<<1) /* Receiver enable */
> -#define UCR2_SRST (1<<0) /* Reset complete */
> -
> -#define UTS1_TXEMPTY (1<<6)
> -#define UTS1_RXEMPTY (1<<5)
> -#define UTS1_TXFULL (1<<4)
> -#define UTS1_RXFULL (1<<3)
> -
> static void imx_update(IMXSerialState *s)
> {
> uint32_t flags;
> @@ -242,13 +163,13 @@ static uint64_t imx_serial_read(void *opaque, hwaddr
> offset,
> return 0x0; /* TODO */
>
> default:
> - IPRINTF("imx_serial_read: bad offset: 0x%x\n", (int)offset);
> + IPRINTF("%s: bad offset: 0x%x\n", __func__, (int)offset);
> return 0;
> }
> }
>
> static void imx_serial_write(void *opaque, hwaddr offset,
> - uint64_t value, unsigned size)
> + uint64_t value, unsigned size)
> {
> IMXSerialState *s = (IMXSerialState *)opaque;
> unsigned char ch;
> @@ -298,25 +219,25 @@ static void imx_serial_write(void *opaque, hwaddr
> offset,
>
> case 0x25: /* USR1 */
> value &= USR1_AWAKE | USR1_AIRINT | USR1_DTRD | USR1_AGTIM |
> - USR1_FRAMERR | USR1_ESCF | USR1_RTSD | USR1_PARTYER;
> + USR1_FRAMERR | USR1_ESCF | USR1_RTSD | USR1_PARTYER;
> s->usr1 &= ~value;
> break;
>
> case 0x26: /* USR2 */
> - /*
> - * Writing 1 to some bits clears them; all other
> - * values are ignored
> - */
> + /*
> + * Writing 1 to some bits clears them; all other
> + * values are ignored
> + */
> value &= USR2_ADET | USR2_DTRF | USR2_IDLE | USR2_ACST |
> - USR2_RIDELT | USR2_IRINT | USR2_WAKE |
> - USR2_DCDDELT | USR2_RTSF | USR2_BRCD | USR2_ORE;
> + USR2_RIDELT | USR2_IRINT | USR2_WAKE |
> + USR2_DCDDELT | USR2_RTSF | USR2_BRCD | USR2_ORE;
> s->usr2 &= ~value;
> break;
>
> - /*
> - * Linux expects to see what it writes to these registers
> - * We don't currently alter the baud rate
> - */
> + /*
> + * Linux expects to see what it writes to these registers
> + * We don't currently alter the baud rate
> + */
> case 0x29: /* UBIR */
> s->ubrc = value & 0xffff;
> break;
> @@ -344,7 +265,7 @@ static void imx_serial_write(void *opaque, hwaddr offset,
> break;
>
> default:
> - IPRINTF("imx_serial_write: Bad offset 0x%x\n", (int)offset);
> + IPRINTF("%s: Bad offset 0x%x\n", __func__, (int)offset);
> }
> }
>
> @@ -377,22 +298,18 @@ static void imx_event(void *opaque, int event)
> }
> }
>
> -
> static const struct MemoryRegionOps imx_serial_ops = {
> .read = imx_serial_read,
> .write = imx_serial_write,
> .endianness = DEVICE_NATIVE_ENDIAN,
> };
>
> -static int imx_serial_init(SysBusDevice *dev)
> +static void imx_serial_realize(DeviceState *dev, Error **errp)
> {
> IMXSerialState *s = IMX_SERIAL(dev);
>
> -
> - memory_region_init_io(&s->iomem, OBJECT(s), &imx_serial_ops, s,
> - "imx-serial", 0x1000);
> - sysbus_init_mmio(dev, &s->iomem);
> - sysbus_init_irq(dev, &s->irq);
> + /* FIXME use a qdev chardev prop instead of qemu_char_get_next_serial()
> */
> + s->chr = qemu_char_get_next_serial();
>
> if (s->chr) {
> qemu_chr_add_handlers(s->chr, imx_can_receive, imx_receive,
> @@ -401,45 +318,42 @@ static int imx_serial_init(SysBusDevice *dev)
> DPRINTF("No char dev for uart at 0x%lx\n",
> (unsigned long)s->iomem.ram_addr);
> }
> +}
>
> - return 0;
> +static void imx_serial_init(Object *obj)
> +{
> + SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> + IMXSerialState *s = IMX_SERIAL(obj);
> +
> + memory_region_init_io(&s->iomem, obj, &imx_serial_ops, s,
> + TYPE_IMX_SERIAL, 0x1000);
> + sysbus_init_mmio(sbd, &s->iomem);
> + sysbus_init_irq(sbd, &s->irq);
> }
>
> void imx_serial_create(int uart, const hwaddr addr, qemu_irq irq)
> {
> DeviceState *dev;
> SysBusDevice *bus;
> - CharDriverState *chr;
> - const char chr_name[] = "serial";
> - char label[ARRAY_SIZE(chr_name) + 1];
> -
> - dev = qdev_create(NULL, TYPE_IMX_SERIAL);
>
> if (uart >= MAX_SERIAL_PORTS) {
> hw_error("Cannot assign uart %d: QEMU supports only %d ports\n",
> uart, MAX_SERIAL_PORTS);
> }
> - chr = serial_hds[uart];
> - if (!chr) {
> - snprintf(label, ARRAY_SIZE(label), "%s%d", chr_name, uart);
> - chr = qemu_chr_new(label, "null", NULL);
> - if (!(chr)) {
> - hw_error("Can't assign serial port to imx-uart%d.\n", uart);
> - }
> - }
>
> - qdev_prop_set_chr(dev, "chardev", chr);
> + dev = qdev_create(NULL, TYPE_IMX_SERIAL);
> +
> bus = SYS_BUS_DEVICE(dev);
> qdev_init_nofail(dev);
> +
> if (addr != (hwaddr)-1) {
> sysbus_mmio_map(bus, 0, addr);
> }
> - sysbus_connect_irq(bus, 0, irq);
>
> + sysbus_connect_irq(bus, 0, irq);
> }
>
> -
> -static Property imx32_serial_properties[] = {
> +static Property imx_serial_properties[] = {
> DEFINE_PROP_CHR("chardev", IMXSerialState, chr),
> DEFINE_PROP_END_OF_LIST(),
> };
> @@ -447,21 +361,21 @@ static Property imx32_serial_properties[] = {
> static void imx_serial_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> - SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>
> - k->init = imx_serial_init;
> + dc->realize = imx_serial_realize;
> dc->vmsd = &vmstate_imx_serial;
> dc->reset = imx_serial_reset_at_boot;
> - set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
> dc->desc = "i.MX series UART";
> - dc->props = imx32_serial_properties;
> + dc->props = imx_serial_properties;
> + set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
> }
>
> static const TypeInfo imx_serial_info = {
> - .name = TYPE_IMX_SERIAL,
> - .parent = TYPE_SYS_BUS_DEVICE,
> - .instance_size = sizeof(IMXSerialState),
> - .class_init = imx_serial_class_init,
> + .name = TYPE_IMX_SERIAL,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(IMXSerialState),
> + .instance_init = imx_serial_init,
> + .class_init = imx_serial_class_init,
> };
>
> static void imx_serial_register_types(void)
> diff --git a/include/hw/char/imx_serial.h b/include/hw/char/imx_serial.h
> new file mode 100644
> index 0000000..dba0cbb
> --- /dev/null
> +++ b/include/hw/char/imx_serial.h
> @@ -0,0 +1,104 @@
> +/*
> + * Device model for i.MX UART
> + *
> + * Copyright (c) 2008 OKL
> + * Originally Written by Hans Jiang
> + * Copyright (c) 2011 NICTA Pty Ltd.
> + * Updated by Jean-Christophe Dubois <address@hidden>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef IMX_SERIAL_H
> +#define IMX_SERIAL_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_IMX_SERIAL "imx.serial"
> +#define IMX_SERIAL(obj) OBJECT_CHECK(IMXSerialState, (obj), TYPE_IMX_SERIAL)
> +
> +#define URXD_CHARRDY (1<<15) /* character read is valid */
> +#define URXD_ERR (1<<14) /* Character has error */
> +#define URXD_BRK (1<<11) /* Break received */
> +
> +#define USR1_PARTYER (1<<15) /* Parity Error */
> +#define USR1_RTSS (1<<14) /* RTS pin status */
> +#define USR1_TRDY (1<<13) /* Tx ready */
> +#define USR1_RTSD (1<<12) /* RTS delta: pin changed state */
> +#define USR1_ESCF (1<<11) /* Escape sequence interrupt */
> +#define USR1_FRAMERR (1<<10) /* Framing error */
> +#define USR1_RRDY (1<<9) /* receiver ready */
> +#define USR1_AGTIM (1<<8) /* Aging timer interrupt */
> +#define USR1_DTRD (1<<7) /* DTR changed */
> +#define USR1_RXDS (1<<6) /* Receiver is idle */
> +#define USR1_AIRINT (1<<5) /* Aysnch IR interrupt */
> +#define USR1_AWAKE (1<<4) /* Falling edge detected on RXd pin */
> +
> +#define USR2_ADET (1<<15) /* Autobaud complete */
> +#define USR2_TXFE (1<<14) /* Transmit FIFO empty */
> +#define USR2_DTRF (1<<13) /* DTR/DSR transition */
> +#define USR2_IDLE (1<<12) /* UART has been idle for too long */
> +#define USR2_ACST (1<<11) /* Autobaud counter stopped */
> +#define USR2_RIDELT (1<<10) /* Ring Indicator delta */
> +#define USR2_RIIN (1<<9) /* Ring Indicator Input */
> +#define USR2_IRINT (1<<8) /* Serial Infrared Interrupt */
> +#define USR2_WAKE (1<<7) /* Start bit detected */
> +#define USR2_DCDDELT (1<<6) /* Data Carrier Detect delta */
> +#define USR2_DCDIN (1<<5) /* Data Carrier Detect Input */
> +#define USR2_RTSF (1<<4) /* RTS transition */
> +#define USR2_TXDC (1<<3) /* Transmission complete */
> +#define USR2_BRCD (1<<2) /* Break condition detected */
> +#define USR2_ORE (1<<1) /* Overrun error */
> +#define USR2_RDR (1<<0) /* Receive data ready */
> +
> +#define UCR1_TRDYEN (1<<13) /* Tx Ready Interrupt Enable */
> +#define UCR1_RRDYEN (1<<9) /* Rx Ready Interrupt Enable */
> +#define UCR1_TXMPTYEN (1<<6) /* Tx Empty Interrupt Enable */
> +#define UCR1_UARTEN (1<<0) /* UART Enable */
> +
> +#define UCR2_TXEN (1<<2) /* Transmitter enable */
> +#define UCR2_RXEN (1<<1) /* Receiver enable */
> +#define UCR2_SRST (1<<0) /* Reset complete */
> +
> +#define UTS1_TXEMPTY (1<<6)
> +#define UTS1_RXEMPTY (1<<5)
> +#define UTS1_TXFULL (1<<4)
> +#define UTS1_RXFULL (1<<3)
> +
> +typedef struct IMXSerialState {
> + /* Private */
> + SysBusDevice parent_obj;
> +
> + /* Public */
> + MemoryRegion iomem;
> + int32_t readbuff;
> +
> + uint32_t usr1;
> + uint32_t usr2;
> + uint32_t ucr1;
> + uint32_t ucr2;
> + uint32_t uts1;
> +
> + /*
> + * The registers below are implemented just so that the
> + * guest OS sees what it has written
> + */
> + uint32_t onems;
> + uint32_t ufcr;
> + uint32_t ubmr;
> + uint32_t ubrc;
> + uint32_t ucr3;
> +
> + qemu_irq irq;
> + CharDriverState *chr;
> +} IMXSerialState;
> +
> +void imx_serial_create(int uart, const hwaddr addr, qemu_irq irq);
> +
> +#endif
> --
> 2.1.4
>
>
- [Qemu-devel] [PATCH v8 00/11] i.MX: Add i.MX25 support through the 3DS evaluation board., Jean-Christophe Dubois, 2015/06/29
- [Qemu-devel] [PATCH v8 01/11] i.MX: Split the i.MX serial driver into a header file and a source file, Jean-Christophe Dubois, 2015/06/29
- Re: [Qemu-devel] [PATCH v8 01/11] i.MX: Split the i.MX serial driver into a header file and a source file,
Peter Crosthwaite <=
- [Qemu-devel] [PATCH v8 02/11] i.MX: Split the i.MX AVIC emulator into a header file and a source file, Jean-Christophe Dubois, 2015/06/29
- [Qemu-devel] [PATCH v8 03/11] i.MX: Split the i.MX CCM emulator into a header file and a source file., Jean-Christophe Dubois, 2015/06/29
- [Qemu-devel] [PATCH v8 04/11] i.MX: Split the i.MX EPIT emulator into a header file and a source file., Jean-Christophe Dubois, 2015/06/29
- [Qemu-devel] [PATCH v8 06/11] kzm: Use modified i.MX emulators., Jean-Christophe Dubois, 2015/06/29
- [Qemu-devel] [PATCH v8 05/11] i.MX: Split the i.MX GPT emulator into a header file and a source file., Jean-Christophe Dubois, 2015/06/29
- [Qemu-devel] [PATCH v8 07/11] i.MX: Add FEC Ethernet Emulator, Jean-Christophe Dubois, 2015/06/29
- [Qemu-devel] [PATCH v8 09/11] i.MX25: Add the i.MX25 SOC support, Jean-Christophe Dubois, 2015/06/29