From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60673) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USWZD-0000Sm-AQ for qemu-devel@nongnu.org; Wed, 17 Apr 2013 13:54:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1USWZB-0007lt-A4 for qemu-devel@nongnu.org; Wed, 17 Apr 2013 13:54:47 -0400 Received: from cantor2.suse.de ([195.135.220.15]:41190 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1USWZB-0007Tk-0R for qemu-devel@nongnu.org; Wed, 17 Apr 2013 13:54:45 -0400 Message-ID: <516EE1BA.7010301@suse.de> Date: Wed, 17 Apr 2013 19:54:02 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1366183380-24333-1-git-send-email-lig.fnst@cn.fujitsu.com> <1366183380-24333-2-git-send-email-lig.fnst@cn.fujitsu.com> In-Reply-To: <1366183380-24333-2-git-send-email-lig.fnst@cn.fujitsu.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC][PATCH 2/2] hw: add Embedded Controller chip emulation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: liguang Cc: Paolo Bonzini , seabios@seabios.org, qemu-devel@nongnu.org, Anthony Liguori Am 17.04.2013 09:23, schrieb liguang: > this work implemented Embedded Controller chip emulation > which was defined at ACPI SEPC v5 chapter 12: > "ACPI Embedded Controller Interface Specification" >=20 > commonly Embedded Controller will emulate keyboard, > mouse, handle ACPI defined operations and some > low-speed devices like SMbus. >=20 > Signed-off-by: liguang > --- > hw/ec.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++= ++++++++ hw/misc/ec.c? hw/i386/ec.c? Not directly in hw/ anymore. > hw/ec.h | 20 +++++++++++ > 2 files changed, 133 insertions(+), 0 deletions(-) > create mode 100644 hw/ec.c > create mode 100644 hw/ec.h >=20 > diff --git a/hw/ec.c b/hw/ec.c > new file mode 100644 > index 0000000..69c92cf > --- /dev/null > +++ b/hw/ec.c > @@ -0,0 +1,113 @@ > +#include "ec.h" > +#include "hw/hw.h" > +#include "hw/isa/isa.h" > +#include "sysemu/sysemu.h" > + > +#define TYPE_EC_DEV Missing string value. > +#define EC_DEV(obj) \ > + OBJECT_CHECK(ECState, (obj), TYPE_EC_DEV) > + > +static char ec_acpi_space[EC_ACPI_SPACE_SIZE] =3D {0}; > + > +typedef struct ECState { > + ISADevice dev; parent_obj and white line please. > + char cmd; > + char status; > + char data; > + char irq; > + char buf; char seems a bit unsafe here, since it might be signed or unsigned. Suggest uint8_t. > + MemoryRegion io; > +} ECState; > + > + > +static void io62_write(void *opaque, hwaddr addr, uint64_t val, unsign= ed size) > +{ > + ECState *s =3D opaque; > + char tmp =3D val & 0xff; > + > + if (s->status & EC_ACPI_CMD) { > + s->buf =3D tmp; > + s->status &=3D ~EC_ACPI_CMD; > + } else { > + if (tmp < EC_ACPI_SPACE_SIZE) { > + ec_acpi_space[s->buf] =3D tmp; > + } > + } > +} > + > +static uint64_t io62_read(void *opaque, hwaddr addr, unsigned size) > +{ > + return s->data; > +} > + > +static void io66_write(void *opaque, hwaddr addr, uint64_t val, unsign= ed size) > +{ > + ECState *s =3D opaque; > + > + s->status =3D EC_ACPI_CMD | EC_ACPI_IBF; > + > + switch (val & 0xff) { > + case EC_ACPI_CMD_READ: > + case EC_ACPI_CMD_WRITE: > + case EC_ACPI_CMD_BURST_EN: > + s->statu |=3D EC_ACPI_BST; s->status > + case EC_ACPI_CMD_BURST_DN: > + s->statu &=3D ~EC_ACPI_BST; s->status > + case EC_ACPI_CMD_QUERY: > + s->cmd =3D val & 0xff; > + case default: > + break; > + } > +} > + > +static uint64_t io66_read(void *opaque, hwaddr addr, unsigned size) > +{ > + ECState *s =3D opaque; > + > + return s->status; > +} > + > +static const MemoryRegionOps io62_io_ops =3D { > + .write =3D io62_write, > + .read =3D io62_read, > + .endianness =3D DEVICE_NATIVE_ENDIAN, > + .impl =3D { > + .min_access_size =3D 1, > + .max_access_size =3D 1, > + }, > +}; > + > +static const MemoryRegionOps io66_io_ops =3D { > + .write =3D io66_write, > + .read =3D io66_read, > + .endianness =3D DEVICE_NATIVE_ENDIAN, > + .impl =3D { > + .min_access_size =3D 1, > + .max_access_size =3D 1, > + }, > +}; > + > +static void ec_realizefn(DeviceState *dev, Error **err) > +{ > + ISADevice *isadev =3D ISA_DEVICE(dev); > + ECState *s =3D EC_DEV(dev); > + > + isa_init_irq(isadev, &s->irq, 0xb); > + > + memory_region_init_io(&s->io, &io62_io_ops, NULL, "ec-acpi-data", = 1); > + isa_register_ioport(isadev, &s->io, 0x62); > + > + memory_region_init_io(&s->io, &io66_io_ops, NULL, "ec-acpi-cmd", 1= ); > + isa_register_ioport(isadev, &s->io, 0x66); Since you are not defining any properties, all of this could go into a .instance_init function. But I am glad to see a realize function being used. :) > + > + s->status =3D 0; > + s->data =3D 0; This is probably not necessary here, since all fields get zero-initialized. It should rather go into a reset function. > +} > + > +static void ec_class_initfn(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc =3D DEVICE_CLASS(klass); > + > + dc->realize =3D ec_realizefn; > + dc->no_user =3D 1; > +} This file ends with an unused static function, it's missing type registration. I figure this device should rather be a specific (real) model, reflected in a particular name, similar to how we have different watchdog devices rather than a "watchdog" device. > diff --git a/hw/ec.h b/hw/ec.h > new file mode 100644 > index 0000000..110ce04 > --- /dev/null > +++ b/hw/ec.h > @@ -0,0 +1,20 @@ > +#inndef __EC_H #ifndef and no leading underscores, please. > +#define __EC_H > + > +#define EC_ACPI_SPACE_SIZE 0x80 > + > +#define EC_ACPI_CMD_PORT 0x66 > +#define EC_ACPI_DATA_PORT 0x62 > + > +#define EC_ACPI_OBF 0x1 > +#define EC_ACPI_IBF 0x2 > +#define EC_ACPI_CMD 0x8 > +#define EC_ACPI_BST 0x10 > + > +#define EC_ACPI_CMD_READ 0x80 > +#define EC_ACPI_CMD_WRITE 0x81 > +#define EC_ACPI_BURST_EN 0x82 > +#define EC_ACPI_BURST_DN 0x83 > +#define EC_ACPI_CMD_QUERY 0x84 > + > +#endif Regards, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg