Re: [Qemu-devel] [PATCH 01/14] ARM: s5pc210: Basic support of s5pc210 boards

[Peter Maydell]

If you split this patch up so it is:
 1 cmu
 2 uart
 3 initial basic board

this will be easier to review than a 2000 line patch.

> +DeviceState *s5pc210_uart_create(target_phys_addr_t addr,
> +                                 int fifo_size,
> +                                 int channel,
> +                                 CharDriverState *chr,
> +                                 qemu_irq irq)
> +{
> +    DeviceState  *dev;
> +    SysBusDevice *bus;
> +    S5pc210UartState *state;
> +
> +    dev = qdev_create(NULL, "s5pc210.uart");
> +
> +    if (!chr) {
> +        if (channel >= MAX_SERIAL_PORTS) {
> +            hw_error("Only %d serial ports are supported by QEMU.\n",
> +                     MAX_SERIAL_PORTS);
> +        }
> +        chr = serial_hds[channel];
> +        if (!chr) {
> +            chr = qemu_chr_new("s5pc210.uart", "null", NULL);
> +            if (!(chr)) {
> +                hw_error("Can't assign serial port to UART%d.\n", channel);
> +            }
> +        }
> +    }
> +
> +    qdev_prop_set_chr(dev, "chardev", chr);
> +    qdev_prop_set_uint32(dev, "channel", channel);
> +
> +    bus = sysbus_from_qdev(dev);
> +    qdev_init_nofail(dev);
> +    if (addr != (target_phys_addr_t)-1) {
> +        sysbus_mmio_map(bus, 0, addr);
> +    }
> +    sysbus_connect_irq(bus, 0, irq);
> +
> +    state = FROM_SYSBUS(S5pc210UartState, bus);
> +
> +    state->rx.size = fifo_size;
> +    state->tx.size = fifo_size;

This is wrong. Code at the "qdev_create(something)" level should
not then reach in and mess with the underlying state structure
of the device it's just created. fifo_size should probably be passed
to the device as a qdev property.

> +static int s5pc210_uart_init(SysBusDevice *dev)
> +{
> +    S5pc210UartState *s = FROM_SYSBUS(S5pc210UartState, dev);
> +
> +    /* memory mapping */
> +    memory_region_init_io(&s->iomem, &s5pc210_uart_ops, s, "s5pc210.uart",
> +                          S5PC210_UART_REGS_MEM_SIZE);
> +    sysbus_init_mmio_region(dev, &s->iomem);
> +
> +    sysbus_init_irq(dev, &s->irq);
> +
> +    qemu_chr_add_handlers(s->chr, s5pc210_uart_can_receive,
> +                          s5pc210_uart_receive, s5pc210_uart_event, s);
> +
> +    vmstate_register(&dev->qdev, -1, &vmstate_s5pc210_uart, s);
> +
> +    qemu_register_reset(s5pc210_uart_reset, s);

You can set these up using .qdev.reset and .qdev.vmsd fields
in your SysBusDeviceInfo struct, which is cleaner than calling
functions to register them.

-- PMM