On Sun, Jul 5, 2015 at 5:04 PM, Jean-Christophe Dubois
<address@hidden> wrote:
> Move constructor to DeviceClass methods
> * imx_serial_init
> * imx_serial_realize
>
> imx32_serial_properties is renamed to imx_serial_properties.
>
> The Qdev construction helper is moved to an include file as an
> inline function. This function is going to be removed soon.
>
> 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:
> * not present on v7
>
> Changes since v8:
> * Remove Qdev construction helper
>
> Changes since v9:
> * Qdev construction helper is reintegrated and moved to a header file
> as an inline function.
>
> hw/char/imx_serial.c | 70 +++++++++++++++-------------------------------------
> include/hw/arm/imx.h | 30 +++++++++++++++++++++-
> 2 files changed, 49 insertions(+), 51 deletions(-)
>
> diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
> index 1dcb325..ef5af05 100644
> --- a/hw/char/imx_serial.c
> +++ b/hw/char/imx_serial.c
> @@ -21,7 +21,6 @@
> #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
> @@ -38,13 +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
>
> static const VMStateDescription vmstate_imx_serial = {
> - .name = "imx-serial",
> + .name = TYPE_IMX_SERIAL,
> .version_id = 1,
> .minimum_version_id = 1,
> .fields = (VMStateField[]) {
> @@ -299,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,
> @@ -323,45 +318,20 @@ 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;
> }
>
> -void imx_serial_create(int uart, const hwaddr addr, qemu_irq irq)
> +static void imx_serial_init(Object *obj)
> {
> - 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);
I'm confused as to why you needed to replace the qdev_prop_set_chr
logic with qemu_get_char_get_next_serial(). Can you give some details
on why we want to do this?
I am just following the general ARM template of other serial drivers. I am not proficient enough in Qemu wizardry to proprose my own thing in this regard.
So far no other ARM plateform (except strongarm?) seems to be using "chardev" and all seems to be using qemu_get_char_get_next_serial() even if it is supposed to be deprecated some day (even the cadence serial driver).
> - 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);
> + 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);
> }
>
> -
> -static Property imx32_serial_properties[] = {
> +static Property imx_serial_properties[] = {
> DEFINE_PROP_CHR("chardev", IMXSerialState, chr),
> DEFINE_PROP_END_OF_LIST(),
> };
> @@ -369,21 +339,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;
> }
>
> 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/arm/imx.h b/include/hw/arm/imx.h
> index ea9e093..b861e80 100644
> --- a/include/hw/arm/imx.h
> +++ b/include/hw/arm/imx.h
> @@ -11,7 +11,34 @@
> #ifndef IMX_H
> #define IMX_H
>
> -void imx_serial_create(int uart, const hwaddr addr, qemu_irq irq);
> +#include "hw/qdev-core.h"
> +#include "hw/sysbus.h"
> +#include "hw/char/imx_serial.h"
> +
> +/***
> + * This Qdev construction helper is going to be removed soon
> + */
> +static inline void imx_serial_create(int uart, const hwaddr addr, qemu_irq irq)
> +{
> + /* uart parameter is not used anymore */
> + DeviceState *dev;
> +
> + dev = qdev_create(NULL, TYPE_IMX_SERIAL);
> +
> + if (dev) {
> + SysBusDevice *bus;
> +
> + 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);
> + }
> +}
Can you leave the implementation in imx_serial.c? I don't see the need
for moving it out. There is no reason the c file cannot have all of
_init _realize and _create.
I sure could, but I wanted to move these Qdev functions to a single place where I could nuke them together (removing a single file) when I don't need them anymore (when porting KZM to SOC logic).
Regards,
Peter
>
> typedef enum {
> NOCLK,
> @@ -31,4 +58,5 @@ void imx_timerg_create(const hwaddr addr,
> DeviceState *ccm);
>
>
> +
> #endif /* IMX_H */
> --
> 2.1.4
>
>