From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55150) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S9hIK-0001pb-GR for qemu-devel@nongnu.org; Mon, 19 Mar 2012 14:27:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S9hII-0005Uy-Eq for qemu-devel@nongnu.org; Mon, 19 Mar 2012 14:27:00 -0400 Message-ID: <4F677A64.9010302@reactos.org> Date: Mon, 19 Mar 2012 19:26:44 +0100 From: =?UTF-8?B?SGVydsOpIFBvdXNzaW5lYXU=?= MIME-Version: 1.0 References: <1331995186-18507-1-git-send-email-hpoussin@reactos.org> <1331995186-18507-6-git-send-email-hpoussin@reactos.org> <4F67318C.8050302@suse.de> In-Reply-To: <4F67318C.8050302@suse.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 5/6] prep: add pc87312 Super I/O emulation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org Andreas F=C3=A4rber a =C3=A9crit : > Am 17.03.2012 15:39, schrieb Herv=C3=A9 Poussineau: >> This provides floppy and IDE controllers as well as serial and paralle= l ports. >> However, dynamic configuration of devices is not yet supported. >> >> Cc: Andreas F=C3=A4rber >> Signed-off-by: Herv=C3=A9 Poussineau >> --- >> Makefile.objs | 1 + >> hw/pc87312.c | 425 ++++++++++++++++++++++++++++++++++++++++++++++++= +++++++++ >> 2 files changed, 426 insertions(+), 0 deletions(-) >> create mode 100644 hw/pc87312.c >=20 >> diff --git a/hw/pc87312.c b/hw/pc87312.c >> new file mode 100644 >> index 0000000..1e28dbd >> --- /dev/null >> +++ b/hw/pc87312.c >> @@ -0,0 +1,425 @@ >> +/* >> + * QEMU National Semiconductor PC87312 (Super I/O) >> + * >> + * Copyright (c) 2010-2012 Herve Poussineau >=20 > FWIW mind to add >=20 > Copyright (c) 2011 Andreas F=C3=A4rber >=20 > ? Yes, of course. Sorry about that. >> + >> + chr =3D s->parallel.chr; >> + if (s->parallel.chr !=3D NULL && is_parallel_enabled(s)) { >=20 > This logic still seems to be flawed: it should not depend on > s->parallel.chr. If that is NULL we need to create a null char device t= o > match the device that's present in hardware according to > is_parallel_enabled(s). Ok, will remove the 'chr !=3D NULL' check. >=20 >> + qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL); /* HACK *= / >=20 > HACK alarm in [PATCH]: What for? The problem is composition. Main board contains a pc87312, which=20 contains a isa-parallel. Main board doesn't know the isa-parallel device. isa-parallel device must have a chardev (set by a property). pc87312=20 creates the isa-parallel device, so it must know the chardev. I did it=20 also with a chardev property. Unfortunately, a chardev can only be used once, and I have not found a=20 better way to give chardev from main board to superI/O to parallel. Do you have any better idea? >> + for (i =3D 0; i < 2; i++) { >> + chr =3D s->uart[i].chr; >=20 >> + if (chr !=3D NULL && is_uart_enabled(s, i)) { >> + qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL); /* HA= CK */ >=20 > 2x ditto. Same answer :) >> + >> +static void pc87312_class_initfn(ObjectClass *klass, void *data) >=20 > I always thought initfn was used for instances... Ok, will change to pc87312_class_init >> + >> +type_init(pc87312_register_types) >> + >=20 > Trailing empty line. Ok, will remove. >=20 > So what about the ugly ISA hot-plug issue that originally stalled our > two series? I'm missing a Change Log about that. You changed the initia= l > configuration to the one used by 40P firmware IIRC and stopped > supporting the chipset's reconfiguration? Either way any limitation of > this implementation should be prominently documented please. Yes, chipset reconfiguration required ISA hot-plug issue, so I didn't=20 want to wait for this serie. In commit message, I wrote "However,=20 dynamic configuration of devices is not yet supported.", and you get an=20 error message at each reconfiguration: +static void reconfigure_devices(PC87312State *s) +{ + error_report("pc87312: unsupported device reconfiguration (%02x=20 %02x %02x)", + s->FER, s->FAR, s->PTR); +} What else should I add? Anyway, if/when dynamic reconfiguration will be implemented, only this=20 method will be changed. About the initial configuration, it is controlled by "config" property. In patch 6/6, when I use the Super I/O chip in 'prep' machine, I set=20 this property to 13, which means fdc + serial0 + serial1 + parallel0. For IBM 40p, the initial configuration should be 15 (fdc + ide + serial0=20 + parallel0). So I don't think the Super I/O is exclusively tied to IBM 40p. >=20 > Thanks for your work on this, Thanks Herv=C3=A9