From patchwork Mon May 30 11:35:09 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Markus Armbruster X-Patchwork-Id: 9140935 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id AF14F60777 for ; Mon, 30 May 2016 11:35:37 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A01DF28185 for ; Mon, 30 May 2016 11:35:37 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 92CF5281C2; Mon, 30 May 2016 11:35:37 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 4C57D28185 for ; Mon, 30 May 2016 11:35:36 +0000 (UTC) Received: from localhost ([::1]:59472 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b7LTr-0007GW-E7 for patchwork-qemu-devel@patchwork.kernel.org; Mon, 30 May 2016 07:35:35 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43397) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b7LTb-0007BW-VM for qemu-devel@nongnu.org; Mon, 30 May 2016 07:35:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b7LTa-0002dB-N4 for qemu-devel@nongnu.org; Mon, 30 May 2016 07:35:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46456) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b7LTU-0002c2-M3; Mon, 30 May 2016 07:35:12 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C6A117DCD8; Mon, 30 May 2016 11:35:11 +0000 (UTC) Received: from blackfin.pond.sub.org (ovpn-116-46.ams2.redhat.com [10.36.116.46]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u4UBZ96s028933 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Mon, 30 May 2016 07:35:10 -0400 Received: by blackfin.pond.sub.org (Postfix, from userid 1000) id 0E7781138645; Mon, 30 May 2016 13:35:09 +0200 (CEST) From: Markus Armbruster To: Peter Maydell References: <1464173896-4088-1-git-send-email-zxq_yx_007@163.com> <1464173896-4088-2-git-send-email-zxq_yx_007@163.com> Date: Mon, 30 May 2016 13:35:09 +0200 In-Reply-To: (Peter Maydell's message of "Wed, 25 May 2016 14:08:08 +0100") Message-ID: <87fuszeq36.fsf@dusky.pond.sub.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Mon, 30 May 2016 11:35:12 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: Re: [Qemu-devel] [PATCH 1/6] hw/char: QOM'ify pl011 model X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Rob Herring , Peter Crosthwaite , xiaoqiang zhao , QEMU Developers , Alistair Francis , qemu-arm , =?utf-8?B?0JDQvdGC0L7QvSDQn9Cw0LLQu9C+?= =?utf-8?B?0LI=?= , Paolo Bonzini , "Edgar E. Iglesias" Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP Peter Maydell writes: > On 25 May 2016 at 11:58, xiaoqiang zhao 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 >> --- >> 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; }