Peter Maydell <peter.mayd...@linaro.org> writes:
> On 25 May 2016 at 11:58, xiaoqiang zhao <zxq_yx_...@163.com> wrote:
>> * drop qemu_char_get_next_serial and use chardev prop
>> * add pl011_create wrapper function to create pl011 uart device
>> * change affected board code to use the new way
>>
>> Signed-off-by: xiaoqiang zhao <zxq_yx_...@163.com>
>> ---
>> hw/arm/bcm2835_peripherals.c | 16 +++-----------
>> hw/arm/highbank.c | 3 ++-
>> hw/arm/integratorcp.c | 5 +++--
>> hw/arm/realview.c | 9 ++++----
>> hw/arm/stellaris.c | 6 +++--
>> hw/arm/versatilepb.c | 9 ++++----
>> hw/arm/vexpress.c | 9 ++++----
>> hw/arm/virt.c | 1 +
>> hw/char/pl011.c | 11 +++++-----
>> include/hw/char/pl011.h | 52
>> ++++++++++++++++++++++++++++++++++++++++++++
>> 10 files changed, 86 insertions(+), 35 deletions(-)
>> create mode 100644 include/hw/char/pl011.h
>>
>> diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
>> index 234d518..46c320f 100644
>> --- a/hw/arm/bcm2835_peripherals.c
>> +++ b/hw/arm/bcm2835_peripherals.c
>> @@ -14,6 +14,7 @@
>> #include "hw/misc/bcm2835_mbox_defs.h"
>> #include "hw/arm/raspi_platform.h"
>> #include "sysemu/char.h"
>> +#include "sysemu/sysemu.h"
>>
>> /* Peripheral base address on the VC (GPU) system bus */
>> #define BCM2835_VC_PERI_BASE 0x7e000000
>> @@ -106,7 +107,6 @@ static void bcm2835_peripherals_realize(DeviceState
>> *dev, Error **errp)
>> MemoryRegion *ram;
>> Error *err = NULL;
>> uint32_t ram_size, vcram_size;
>> - CharDriverState *chr;
>> int n;
>>
>> obj = object_property_get_link(OBJECT(dev), "ram", &err);
>> @@ -147,6 +147,7 @@ static void bcm2835_peripherals_realize(DeviceState
>> *dev, Error **errp)
>> sysbus_pass_irq(SYS_BUS_DEVICE(s), SYS_BUS_DEVICE(&s->ic));
>>
>> /* UART0 */
>> + qdev_prop_set_chr(DEVICE(&s->uart0), "chardev", serial_hds[0]);
>> object_property_set_bool(OBJECT(s->uart0), true, "realized", &err);
>> if (err) {
>> error_propagate(errp, err);
>> @@ -158,17 +159,8 @@ static void bcm2835_peripherals_realize(DeviceState
>> *dev, Error **errp)
>> sysbus_connect_irq(s->uart0, 0,
>> qdev_get_gpio_in_named(DEVICE(&s->ic), BCM2835_IC_GPU_IRQ,
>> INTERRUPT_UART));
>> -
>> /* AUX / UART1 */
>> - /* TODO: don't call qemu_char_get_next_serial() here, instead set
>> - * chardev properties for each uart at the board level, once pl011
>> - * (uart0) has been updated to avoid qemu_char_get_next_serial()
>> - */
>
> This comment says this should be fixed by having board-level
> properties; you've removed it but this patch isn't adding
> the properties to this (SoC-level) device. I think the board
> level should be looking at serial_hds[], not this code.
Device models should not fish backends out of global state. Whether
they fish directly or via some helper like qemu_char_get_next_serial()
doesn't matter. The ones that still do need to set
cannot_instantiate_with_device_add_yet with a suitable comment.
>> @@ -310,8 +312,7 @@ static void pl011_class_init(ObjectClass *oc, void *data)
>>
>> dc->realize = pl011_realize;
>> dc->vmsd = &vmstate_pl011;
>> - /* Reason: realize() method uses qemu_char_get_next_serial() */
>> - dc->cannot_instantiate_with_device_add_yet = true;
>
> Why does instantiating with device_add work now? There's
> still no way to wire up interrupt lines or map mmio regions.
> (This has never made much sense to me -- Markus?)
Uh, which part does "this" refer to?
I systematically reviewed devices for my "Clean up and fix no_user"
series (commit f976b09..7ea5e78), and wrote down my findings in
"Reason:" comments next to cannot_instantiate_with_device_add_yet
assignments. Any such assignment must have such a comment.
Testing can catch cases where we missed *all* reasons. Example: my "Fix
device introspection regressions" series (commit b37686f..33fe968). It
can't catch cases where we missed *some* reasons.
Note that cannot_instantiate_with_device_add_yet can get set by
(possibly abstract) parent devices as well. If a parent device sets it,
its children should nevertheless set it *again* if they have additional
reasons. I believe this is such a case. I'm not 100% sure, because I
haven't been 100% sure about anything related to sysbus devices ever
since Alex's commit 33cd52b "sysbus: Make devices spawnable via -device"
dropped cannot_instantiate_with_device_add_yet from
sysbus_device_class_init(), quoted below. As you see, that assignment
covered "no way to wire up interrupt lines or map mmio regions."
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 19437e6..7bfe381 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -283,13 +283,6 @@ static void sysbus_device_class_init(ObjectClass *klass, vo
id *data)
DeviceClass *k = DEVICE_CLASS(klass);
k->init = sysbus_device_init;
k->bus_type = TYPE_SYSTEM_BUS;
- /*
- * device_add plugs devices into suitable bus. For "real" buses,
- * that actually connects the device. For sysbus, the connections
- * need to be made separately, and device_add can't do that. The
- * device would be left unconnected, and could not possibly work.
- */
- k->cannot_instantiate_with_device_add_yet = true;
}
