[Qemu-devel] [RFC][SeaBIOS][PATCH 1/2] acpi: add ASL for Embedded Controller

[Qemu-devel] [RFC][SeaBIOS][PATCH 1/2] acpi: add ASL for Embedded Controller

[liguang]

defined at ACPI SPEC v5 chapter 12:
"ACPI Embedded Controller Interface Specification"

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 src/acpi-dsdt-ec.dsl |  106 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 106 insertions(+), 0 deletions(-)
 create mode 100644 src/acpi-dsdt-ec.dsl

diff --git a/src/acpi-dsdt-ec.dsl b/src/acpi-dsdt-ec.dsl
new file mode 100644
index 0000000..6bd8edd
--- /dev/null
+++ b/src/acpi-dsdt-ec.dsl
@@ -0,0 +1,106 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+
+Device (EC0)
+{
+	Name (_HID, EISAID ("PNP0C09"))
+	Name(_UID, 1)
+	
+	Method(_CRS, 0)
+  	{
+    	Name(BFFR, ResourceTemplate()
+    	{
+      		IO(Decode16, 0x62, 0x62, 0, 1)      // ACPI DATA IN/OUT
+      		IO(Decode16, 0x66, 0x66, 0, 1)      // CMD/STS
+      		IRQ(Edge, ActiveHigh, Exclusive) {0x0B}
+    	})
+    Return(BFFR)
+  	}
+  	
+  	OperationRegion(ECF0, EmbeddedControl, 0, 0xFF)
+    Field(ECF2, ByteAcc, Lock, Preserve)
+  	{
+		Offset(1),
+		STMP,   8,      // 1,      Sensor Temperature
+		ACPW,   8,      // 2,      AC Power (AC Present = 1, else 0)
+		LIDS,   8,      // 3,      Lid State (Lid Open = 1, else 0)
+		PBNS,   8,      // 4,      Power Button State (Pressed = 1, else 0)
+		BTST,   8,      // 5,      Battery Status
+		BTCR,   8,      // 6,      Battery Current Rate
+		BTCC,   8,      // 7,      Battery Current Capacity
+		BTVT,   8,      // 8,      Battery Voltage
+		offset(0x10),
+		SPTR,   8,      // 5,      SMBus Protocol Register
+		SSTS,   8,      // 5,      SMBus Status Register
+		SADR,   8,      // 6,      SMBus Address Register
+		SCMD,   8,      // 7,      SMBus Command Register
+		SBFR,   256,    // 8,      SMBus Block Buffer
+		SCNT,   8,      // 40,     SMBus Block Count
+  	}
+  	
+	Method(_REG, 2)
+	{
+	}
+	
+	/* AC status: present */
+	Method(_Q01, 0, NotSerialized)
+	{
+		Notify (AC, 0x80)
+	}
+
+	/* AC status: dispear*/
+	Method(_Q02, 0, NotSerialized)
+	{
+		Notify (AC, 0x80)
+	}
+
+    Method(_Q04, 0, NotSerialized)
+    {
+       Notify(LID, 0x80)
+    }
+	
+    Method(_Q04, 0, NotSerialized)
+    {
+       Notify(LID, 0x80)
+    }	
+	
+	Device(AC)
+	{
+		Name(_HID, "ACPI0003")
+		Name(_UID, 0x00)
+		Name(_PCL, Package() { \_SB } )
+
+		Method(_PSR, 0, NotSerialized)
+		{
+			return (ACPW)
+		}
+
+		Method(_STA, 0, NotSerialized)
+		{
+			Return (0x0f)
+		}
+	}
+	
+	Device(LID)
+	{
+		Name(_HID, "PNP0C0D")
+
+		Method(_LID, 0, NotSerialized)
+		{
+			return (LIDS)
+		}
+	}
+}
-- 
1.7.2.5

[Qemu-devel] [RFC][PATCH 2/2] hw: add Embedded Controller chip emulation

[liguang]

this work implemented Embedded Controller chip emulation
which was defined at ACPI SEPC v5 chapter 12:
"ACPI Embedded Controller Interface Specification"

commonly Embedded Controller will emulate keyboard,
mouse, handle ACPI defined operations and some
low-speed devices like SMbus.

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 hw/ec.c |  113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/ec.h |   20 +++++++++++
 2 files changed, 133 insertions(+), 0 deletions(-)
 create mode 100644 hw/ec.c
 create mode 100644 hw/ec.h

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
+#define EC_DEV(obj) \
+    OBJECT_CHECK(ECState, (obj), TYPE_EC_DEV)
+
+static char ec_acpi_space[EC_ACPI_SPACE_SIZE] = {0};
+
+typedef struct ECState {
+    ISADevice dev;
+    char cmd;
+    char status;
+    char data;
+    char irq;
+    char buf;
+    MemoryRegion io;
+} ECState;
+
+
+static void io62_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
+{
+    ECState *s = opaque;
+    char tmp = val & 0xff;
+
+    if (s->status & EC_ACPI_CMD) {
+        s->buf = tmp;
+        s->status &= ~EC_ACPI_CMD;
+    } else {
+        if (tmp < EC_ACPI_SPACE_SIZE) {
+            ec_acpi_space[s->buf] = 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, unsigned size)
+{
+    ECState *s = opaque;
+
+    s->status = 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 |= EC_ACPI_BST;
+    case EC_ACPI_CMD_BURST_DN:
+        s->statu &= ~EC_ACPI_BST;
+    case EC_ACPI_CMD_QUERY:
+        s->cmd = val & 0xff;
+    case default:
+        break;
+    }
+}
+
+static uint64_t io66_read(void *opaque, hwaddr addr, unsigned size)
+{
+    ECState *s = opaque;
+
+    return s->status;
+}
+
+static const MemoryRegionOps io62_io_ops = {
+    .write = io62_write,
+    .read = io62_read,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+static const MemoryRegionOps io66_io_ops = {
+    .write = io66_write,
+    .read = io66_read,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+static void ec_realizefn(DeviceState *dev, Error **err)
+{
+    ISADevice *isadev = ISA_DEVICE(dev);
+    ECState *s = 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);
+
+    s->status = 0;
+    s->data = 0;
+}
+
+static void ec_class_initfn(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = ec_realizefn;
+    dc->no_user = 1;
+}
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
+#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
-- 
1.7.2.5

Re: [Qemu-devel] [SeaBIOS] [RFC][PATCH 2/2] hw: add Embedded Controller chip emulation

[Mark Marshall]

[-- Attachment #1: Type: text/plain, Size: 4442 bytes --]

Hi.

At least one major bug (noted below), have you tested all of this yet?

MM


On 17 April 2013 09:23, liguang <lig.fnst@cn.fujitsu.com> wrote:

> this work implemented Embedded Controller chip emulation
> which was defined at ACPI SEPC v5 chapter 12:
> "ACPI Embedded Controller Interface Specification"
>
> commonly Embedded Controller will emulate keyboard,
> mouse, handle ACPI defined operations and some
> low-speed devices like SMbus.
>
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  hw/ec.c |  113
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/ec.h |   20 +++++++++++
>  2 files changed, 133 insertions(+), 0 deletions(-)
>  create mode 100644 hw/ec.c
>  create mode 100644 hw/ec.h
>
> 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
> +#define EC_DEV(obj) \
> +    OBJECT_CHECK(ECState, (obj), TYPE_EC_DEV)
> +
> +static char ec_acpi_space[EC_ACPI_SPACE_SIZE] = {0};
> +
> +typedef struct ECState {
> +    ISADevice dev;
> +    char cmd;
> +    char status;
> +    char data;
> +    char irq;
> +    char buf;
> +    MemoryRegion io;
> +} ECState;
> +
> +
> +static void io62_write(void *opaque, hwaddr addr, uint64_t val, unsigned
> size)
> +{
> +    ECState *s = opaque;
> +    char tmp = val & 0xff;
> +
> +    if (s->status & EC_ACPI_CMD) {
> +        s->buf = tmp;
> +        s->status &= ~EC_ACPI_CMD;
> +    } else {
> +        if (tmp < EC_ACPI_SPACE_SIZE) {
> +            ec_acpi_space[s->buf] = 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, unsigned
> size)
> +{
> +    ECState *s = opaque;
> +
> +    s->status = 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 |= EC_ACPI_BST;
> +    case EC_ACPI_CMD_BURST_DN:
> +        s->statu &= ~EC_ACPI_BST;
> +    case EC_ACPI_CMD_QUERY:
> +        s->cmd = val & 0xff;
> +    case default:
> +        break;
> +    }
> +}
>

You've missed 'break' here a few times.


> +
> +static uint64_t io66_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    ECState *s = opaque;
> +
> +    return s->status;
> +}
> +
> +static const MemoryRegionOps io62_io_ops = {
> +    .write = io62_write,
> +    .read = io62_read,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +static const MemoryRegionOps io66_io_ops = {
> +    .write = io66_write,
> +    .read = io66_read,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +static void ec_realizefn(DeviceState *dev, Error **err)
> +{
> +    ISADevice *isadev = ISA_DEVICE(dev);
> +    ECState *s = 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);
> +
> +    s->status = 0;
> +    s->data = 0;
> +}
> +
> +static void ec_class_initfn(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = ec_realizefn;
> +    dc->no_user = 1;
> +}
> 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
> +#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
> --
> 1.7.2.5
>
>
> _______________________________________________
> SeaBIOS mailing list
> SeaBIOS@seabios.org
> http://www.seabios.org/mailman/listinfo/seabios
>

[-- Attachment #2: Type: text/html, Size: 5874 bytes --]

Re: [Qemu-devel] [SeaBIOS] [RFC][PATCH 2/2] hw: add Embedded Controller chip emulation

[li guang]

No, sorry.
As this is RFC patch,
I sent it only for demo my thought.

在 2013-04-17三的 12:41 +0200，Mark Marshall写道：
> Hi.
> 
> 
> At least one major bug (noted below), have you tested all of this yet?
> 
> 
> MM
> 
> 
> On 17 April 2013 09:23, liguang <lig.fnst@cn.fujitsu.com> wrote:
>         this work implemented Embedded Controller chip emulation
>         which was defined at ACPI SEPC v5 chapter 12:
>         "ACPI Embedded Controller Interface Specification"
>         
>         commonly Embedded Controller will emulate keyboard,
>         mouse, handle ACPI defined operations and some
>         low-speed devices like SMbus.
>         
>         Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
>         ---
>          hw/ec.c |  113
>         +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>          hw/ec.h |   20 +++++++++++
>          2 files changed, 133 insertions(+), 0 deletions(-)
>          create mode 100644 hw/ec.c
>          create mode 100644 hw/ec.h
>         
>         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
>         +#define EC_DEV(obj) \
>         +    OBJECT_CHECK(ECState, (obj), TYPE_EC_DEV)
>         +
>         +static char ec_acpi_space[EC_ACPI_SPACE_SIZE] = {0};
>         +
>         +typedef struct ECState {
>         +    ISADevice dev;
>         +    char cmd;
>         +    char status;
>         +    char data;
>         +    char irq;
>         +    char buf;
>         +    MemoryRegion io;
>         +} ECState;
>         +
>         +
>         +static void io62_write(void *opaque, hwaddr addr, uint64_t
>         val, unsigned size)
>         +{
>         +    ECState *s = opaque;
>         +    char tmp = val & 0xff;
>         +
>         +    if (s->status & EC_ACPI_CMD) {
>         +        s->buf = tmp;
>         +        s->status &= ~EC_ACPI_CMD;
>         +    } else {
>         +        if (tmp < EC_ACPI_SPACE_SIZE) {
>         +            ec_acpi_space[s->buf] = 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, unsigned size)
>         +{
>         +    ECState *s = opaque;
>         +
>         +    s->status = 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 |= EC_ACPI_BST;
>         +    case EC_ACPI_CMD_BURST_DN:
>         +        s->statu &= ~EC_ACPI_BST;
>         +    case EC_ACPI_CMD_QUERY:
>         +        s->cmd = val & 0xff;
>         +    case default:
>         +        break;
>         +    }
>         +}
> 
> You've missed 'break' here a few times.
>  
> 
>         +
>         +static uint64_t io66_read(void *opaque, hwaddr addr, unsigned
>         size)
>         +{
>         +    ECState *s = opaque;
>         +
>         +    return s->status;
>         +}
>         +
>         +static const MemoryRegionOps io62_io_ops = {
>         +    .write = io62_write,
>         +    .read = io62_read,
>         +    .endianness = DEVICE_NATIVE_ENDIAN,
>         +    .impl = {
>         +        .min_access_size = 1,
>         +        .max_access_size = 1,
>         +    },
>         +};
>         +
>         +static const MemoryRegionOps io66_io_ops = {
>         +    .write = io66_write,
>         +    .read = io66_read,
>         +    .endianness = DEVICE_NATIVE_ENDIAN,
>         +    .impl = {
>         +        .min_access_size = 1,
>         +        .max_access_size = 1,
>         +    },
>         +};
>         +
>         +static void ec_realizefn(DeviceState *dev, Error **err)
>         +{
>         +    ISADevice *isadev = ISA_DEVICE(dev);
>         +    ECState *s = 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);
>         +
>         +    s->status = 0;
>         +    s->data = 0;
>         +}
>         +
>         +static void ec_class_initfn(ObjectClass *klass, void *data)
>         +{
>         +    DeviceClass *dc = DEVICE_CLASS(klass);
>         +
>         +    dc->realize = ec_realizefn;
>         +    dc->no_user = 1;
>         +}
>         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
>         +#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
>         --
>         1.7.2.5
>         
>         
>         _______________________________________________
>         SeaBIOS mailing list
>         SeaBIOS@seabios.org
>         http://www.seabios.org/mailman/listinfo/seabios
> 
> 

Re: [Qemu-devel] [RFC][PATCH 2/2] hw: add Embedded Controller chip emulation

[Andreas Färber]

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"
> 
> commonly Embedded Controller will emulate keyboard,
> mouse, handle ACPI defined operations and some
> low-speed devices like SMbus.
> 
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  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
> 
> 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] = {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, unsigned size)
> +{
> +    ECState *s = opaque;
> +    char tmp = val & 0xff;
> +
> +    if (s->status & EC_ACPI_CMD) {
> +        s->buf = tmp;
> +        s->status &= ~EC_ACPI_CMD;
> +    } else {
> +        if (tmp < EC_ACPI_SPACE_SIZE) {
> +            ec_acpi_space[s->buf] = 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, unsigned size)
> +{
> +    ECState *s = opaque;
> +
> +    s->status = 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 |= EC_ACPI_BST;

s->status

> +    case EC_ACPI_CMD_BURST_DN:
> +        s->statu &= ~EC_ACPI_BST;

s->status

> +    case EC_ACPI_CMD_QUERY:
> +        s->cmd = val & 0xff;
> +    case default:
> +        break;
> +    }
> +}
> +
> +static uint64_t io66_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    ECState *s = opaque;
> +
> +    return s->status;
> +}
> +
> +static const MemoryRegionOps io62_io_ops = {
> +    .write = io62_write,
> +    .read = io62_read,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +static const MemoryRegionOps io66_io_ops = {
> +    .write = io66_write,
> +    .read = io66_read,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +static void ec_realizefn(DeviceState *dev, Error **err)
> +{
> +    ISADevice *isadev = ISA_DEVICE(dev);
> +    ECState *s = 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 = 0;
> +    s->data = 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 = DEVICE_CLASS(klass);
> +
> +    dc->realize = ec_realizefn;
> +    dc->no_user = 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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [RFC][PATCH 2/2] hw: add Embedded Controller chip emulation

[li guang]

在 2013-04-17三的 19:54 +0200，Andreas Färber写道：
> 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"
> > 
> > commonly Embedded Controller will emulate keyboard,
> > mouse, handle ACPI defined operations and some
> > low-speed devices like SMbus.
> > 
> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> > ---
> >  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
> > 
> > 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] = {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, unsigned size)
> > +{
> > +    ECState *s = opaque;
> > +    char tmp = val & 0xff;
> > +
> > +    if (s->status & EC_ACPI_CMD) {
> > +        s->buf = tmp;
> > +        s->status &= ~EC_ACPI_CMD;
> > +    } else {
> > +        if (tmp < EC_ACPI_SPACE_SIZE) {
> > +            ec_acpi_space[s->buf] = 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, unsigned size)
> > +{
> > +    ECState *s = opaque;
> > +
> > +    s->status = 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 |= EC_ACPI_BST;
> 
> s->status
> 
> > +    case EC_ACPI_CMD_BURST_DN:
> > +        s->statu &= ~EC_ACPI_BST;
> 
> s->status
> 
> > +    case EC_ACPI_CMD_QUERY:
> > +        s->cmd = val & 0xff;
> > +    case default:
> > +        break;
> > +    }
> > +}
> > +
> > +static uint64_t io66_read(void *opaque, hwaddr addr, unsigned size)
> > +{
> > +    ECState *s = opaque;
> > +
> > +    return s->status;
> > +}
> > +
> > +static const MemoryRegionOps io62_io_ops = {
> > +    .write = io62_write,
> > +    .read = io62_read,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > +    .impl = {
> > +        .min_access_size = 1,
> > +        .max_access_size = 1,
> > +    },
> > +};
> > +
> > +static const MemoryRegionOps io66_io_ops = {
> > +    .write = io66_write,
> > +    .read = io66_read,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > +    .impl = {
> > +        .min_access_size = 1,
> > +        .max_access_size = 1,
> > +    },
> > +};
> > +
> > +static void ec_realizefn(DeviceState *dev, Error **err)
> > +{
> > +    ISADevice *isadev = ISA_DEVICE(dev);
> > +    ECState *s = 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 = 0;
> > +    s->data = 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 = DEVICE_CLASS(klass);
> > +
> > +    dc->realize = ec_realizefn;
> > +    dc->no_user = 1;
> > +}
> 
> This file ends with an unused static function, it's missing type
> registration.

OK, will fix all,

Thanks you so much!

> 
> 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.

does it has to be specific model?
my reason to be generic like "generic-sdhci"(SD host controller) device
is mostly all EC devices play a similar role, and we do not have to
know the difference between EC chip from different vendors

> 
> > 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
> 

Re: [Qemu-devel] [SeaBIOS] [RFC][PATCH 1/2] acpi: add ASL for Embedded Controller

[Marc Jones]

[-- Attachment #1: Type: text/plain, Size: 4384 bytes --]

What EC is this for? The device and interface is defined by the spec, but
the offsets and methods are custom for every EC.


Marc


On Wed, Apr 17, 2013 at 1:22 AM, liguang <lig.fnst@cn.fujitsu.com> wrote:

> defined at ACPI SPEC v5 chapter 12:
> "ACPI Embedded Controller Interface Specification"
>
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  src/acpi-dsdt-ec.dsl |  106
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 106 insertions(+), 0 deletions(-)
>  create mode 100644 src/acpi-dsdt-ec.dsl
>
> diff --git a/src/acpi-dsdt-ec.dsl b/src/acpi-dsdt-ec.dsl
> new file mode 100644
> index 0000000..6bd8edd
> --- /dev/null
> +++ b/src/acpi-dsdt-ec.dsl
> @@ -0,0 +1,106 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301
> USA
> + */
> +
> +
> +Device (EC0)
> +{
> +       Name (_HID, EISAID ("PNP0C09"))
> +       Name(_UID, 1)
> +
> +       Method(_CRS, 0)
> +       {
> +       Name(BFFR, ResourceTemplate()
> +       {
> +               IO(Decode16, 0x62, 0x62, 0, 1)      // ACPI DATA IN/OUT
> +               IO(Decode16, 0x66, 0x66, 0, 1)      // CMD/STS
> +               IRQ(Edge, ActiveHigh, Exclusive) {0x0B}
> +       })
> +    Return(BFFR)
> +       }
> +
> +       OperationRegion(ECF0, EmbeddedControl, 0, 0xFF)
> +    Field(ECF2, ByteAcc, Lock, Preserve)
> +       {
> +               Offset(1),
> +               STMP,   8,      // 1,      Sensor Temperature
> +               ACPW,   8,      // 2,      AC Power (AC Present = 1, else
> 0)
> +               LIDS,   8,      // 3,      Lid State (Lid Open = 1, else 0)
> +               PBNS,   8,      // 4,      Power Button State (Pressed =
> 1, else 0)
> +               BTST,   8,      // 5,      Battery Status
> +               BTCR,   8,      // 6,      Battery Current Rate
> +               BTCC,   8,      // 7,      Battery Current Capacity
> +               BTVT,   8,      // 8,      Battery Voltage
> +               offset(0x10),
> +               SPTR,   8,      // 5,      SMBus Protocol Register
> +               SSTS,   8,      // 5,      SMBus Status Register
> +               SADR,   8,      // 6,      SMBus Address Register
> +               SCMD,   8,      // 7,      SMBus Command Register
> +               SBFR,   256,    // 8,      SMBus Block Buffer
> +               SCNT,   8,      // 40,     SMBus Block Count
> +       }
> +
> +       Method(_REG, 2)
> +       {
> +       }
> +
> +       /* AC status: present */
> +       Method(_Q01, 0, NotSerialized)
> +       {
> +               Notify (AC, 0x80)
> +       }
> +
> +       /* AC status: dispear*/
> +       Method(_Q02, 0, NotSerialized)
> +       {
> +               Notify (AC, 0x80)
> +       }
> +
> +    Method(_Q04, 0, NotSerialized)
> +    {
> +       Notify(LID, 0x80)
> +    }
> +
> +    Method(_Q04, 0, NotSerialized)
> +    {
> +       Notify(LID, 0x80)
> +    }
> +
> +       Device(AC)
> +       {
> +               Name(_HID, "ACPI0003")
> +               Name(_UID, 0x00)
> +               Name(_PCL, Package() { \_SB } )
> +
> +               Method(_PSR, 0, NotSerialized)
> +               {
> +                       return (ACPW)
> +               }
> +
> +               Method(_STA, 0, NotSerialized)
> +               {
> +                       Return (0x0f)
> +               }
> +       }
> +
> +       Device(LID)
> +       {
> +               Name(_HID, "PNP0C0D")
> +
> +               Method(_LID, 0, NotSerialized)
> +               {
> +                       return (LIDS)
> +               }
> +       }
> +}
> --
> 1.7.2.5
>
>
> _______________________________________________
> SeaBIOS mailing list
> SeaBIOS@seabios.org
> http://www.seabios.org/mailman/listinfo/seabios
>



-- 
http://se-eng.com

[-- Attachment #2: Type: text/html, Size: 5449 bytes --]

Re: [Qemu-devel] [SeaBIOS] [RFC][PATCH 1/2] acpi: add ASL for Embedded Controller

[li guang]

在 2013-04-17三的 07:46 -0600，Marc Jones写道：
> What EC is this for? The device and interface is defined by the spec,
> but the offsets and methods are custom for every EC.
> 
> 

you're mostly right,
the specific content ACPI space will vary 
for different boards, this one just for
a common case

> 
> 
> Marc
> 
> 
> On Wed, Apr 17, 2013 at 1:22 AM, liguang <lig.fnst@cn.fujitsu.com>
> wrote:
>         defined at ACPI SPEC v5 chapter 12:
>         "ACPI Embedded Controller Interface Specification"
>         
>         Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
>         ---
>          src/acpi-dsdt-ec.dsl |  106
>         ++++++++++++++++++++++++++++++++++++++++++++++++++
>          1 files changed, 106 insertions(+), 0 deletions(-)
>          create mode 100644 src/acpi-dsdt-ec.dsl
>         
>         diff --git a/src/acpi-dsdt-ec.dsl b/src/acpi-dsdt-ec.dsl
>         new file mode 100644
>         index 0000000..6bd8edd
>         --- /dev/null
>         +++ b/src/acpi-dsdt-ec.dsl
>         @@ -0,0 +1,106 @@
>         +/*
>         + * This program is free software; you can redistribute it
>         and/or modify
>         + * it under the terms of the GNU General Public License as
>         published by
>         + * the Free Software Foundation; version 2 of the License.
>         + *
>         + * This program is distributed in the hope that it will be
>         useful,
>         + * but WITHOUT ANY WARRANTY; without even the implied
>         warranty of
>         + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
>         the
>         + * GNU General Public License for more details.
>         + *
>         + * You should have received a copy of the GNU General Public
>         License
>         + * along with this program; if not, write to the Free
>         Software
>         + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
>         02110-1301 USA
>         + */
>         +
>         +
>         +Device (EC0)
>         +{
>         +       Name (_HID, EISAID ("PNP0C09"))
>         +       Name(_UID, 1)
>         +
>         +       Method(_CRS, 0)
>         +       {
>         +       Name(BFFR, ResourceTemplate()
>         +       {
>         +               IO(Decode16, 0x62, 0x62, 0, 1)      // ACPI
>         DATA IN/OUT
>         +               IO(Decode16, 0x66, 0x66, 0, 1)      // CMD/STS
>         +               IRQ(Edge, ActiveHigh, Exclusive) {0x0B}
>         +       })
>         +    Return(BFFR)
>         +       }
>         +
>         +       OperationRegion(ECF0, EmbeddedControl, 0, 0xFF)
>         +    Field(ECF2, ByteAcc, Lock, Preserve)
>         +       {
>         +               Offset(1),
>         +               STMP,   8,      // 1,      Sensor Temperature
>         +               ACPW,   8,      // 2,      AC Power (AC
>         Present = 1, else 0)
>         +               LIDS,   8,      // 3,      Lid State (Lid Open
>         = 1, else 0)
>         +               PBNS,   8,      // 4,      Power Button State
>         (Pressed = 1, else 0)
>         +               BTST,   8,      // 5,      Battery Status
>         +               BTCR,   8,      // 6,      Battery Current
>         Rate
>         +               BTCC,   8,      // 7,      Battery Current
>         Capacity
>         +               BTVT,   8,      // 8,      Battery Voltage
>         +               offset(0x10),
>         +               SPTR,   8,      // 5,      SMBus Protocol
>         Register
>         +               SSTS,   8,      // 5,      SMBus Status
>         Register
>         +               SADR,   8,      // 6,      SMBus Address
>         Register
>         +               SCMD,   8,      // 7,      SMBus Command
>         Register
>         +               SBFR,   256,    // 8,      SMBus Block Buffer
>         +               SCNT,   8,      // 40,     SMBus Block Count
>         +       }
>         +
>         +       Method(_REG, 2)
>         +       {
>         +       }
>         +
>         +       /* AC status: present */
>         +       Method(_Q01, 0, NotSerialized)
>         +       {
>         +               Notify (AC, 0x80)
>         +       }
>         +
>         +       /* AC status: dispear*/
>         +       Method(_Q02, 0, NotSerialized)
>         +       {
>         +               Notify (AC, 0x80)
>         +       }
>         +
>         +    Method(_Q04, 0, NotSerialized)
>         +    {
>         +       Notify(LID, 0x80)
>         +    }
>         +
>         +    Method(_Q04, 0, NotSerialized)
>         +    {
>         +       Notify(LID, 0x80)
>         +    }
>         +
>         +       Device(AC)
>         +       {
>         +               Name(_HID, "ACPI0003")
>         +               Name(_UID, 0x00)
>         +               Name(_PCL, Package() { \_SB } )
>         +
>         +               Method(_PSR, 0, NotSerialized)
>         +               {
>         +                       return (ACPW)
>         +               }
>         +
>         +               Method(_STA, 0, NotSerialized)
>         +               {
>         +                       Return (0x0f)
>         +               }
>         +       }
>         +
>         +       Device(LID)
>         +       {
>         +               Name(_HID, "PNP0C0D")
>         +
>         +               Method(_LID, 0, NotSerialized)
>         +               {
>         +                       return (LIDS)
>         +               }
>         +       }
>         +}
>         --
>         1.7.2.5
>         
>         
>         _______________________________________________
>         SeaBIOS mailing list
>         SeaBIOS@seabios.org
>         http://www.seabios.org/mailman/listinfo/seabios
> 
> 
> 
> 
> -- 
> http://se-eng.com