On Mon, Jul 6, 2015 at 1:01 AM, <j...@tribudubois.net> wrote: > > > ----- Le 6 Juil 15, à 8:40, Peter Crosthwaite <peter.crosthwa...@xilinx.com> > a écrit : > > On Sun, Jul 5, 2015 at 5:04 PM, Jean-Christophe Dubois > <j...@tribudubois.net> 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 <j...@tribudubois.net> >> --- >> >> 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). >
I wonder if this one is right, and everyone else is wrong. Propertyified chr is better than the devs using qemu_chr_get_next_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); >> + 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). > Yes. I made a comment on the KZM patch when I figured it all out. Regards, Peter > > Regards, > Peter > >> >> typedef enum { >> NOCLK, >> @@ -31,4 +58,5 @@ void imx_timerg_create(const hwaddr addr, >> DeviceState *ccm); >> >> >> + >> #endif /* IMX_H */ >> -- >> 2.1.4 >> >>