[PATCH 0/4] aspeed: LPC peripheral controller devices

[PATCH 0/4] aspeed: LPC peripheral controller devices

[Andrew Jeffery]

Hello,

This series adds support for some of the LPC[1] peripherals found in Aspeed BMC
SoCs. BMCs typically provide a number of features to their host via LPC that
include but are not limited to:

1. Mapping LPC firmware cycles to BMC-controlled flash devices
2. UART(s) for system console routing
3. POST code routing
4. Keyboard-Controller-Style (KCS) IPMI devices
5. Block Transfer (BT) IPMI devices
6. A SuperIO controller for management of LPC devices and miscellaneous
   functionality

[1] https://www.intel.com/content/dam/www/program/design/us/en/documents/low-pin-count-interface-specification.pdf

Specifically, this series adds basic support for functions 1 and 4 above,
handling the BMC firmware configuring the bridge mapping LPC firmware cycles
onto its AHB as well as support for four KCS devices.

Aspeed's LPC controller is not a straight-forward device by any stretch. It
contains at least the capabilities outlined above, in the sense that it's not
possible to cleanly separate the different functions into distinct MMIO
sub-regions: Registers for the various bits of functionality have the feel of
arbitrary placement with a nod to feature-creep and backwards compatibility.
Further, the conceptually coherent pieces of functionality often come with the
ability to issue interrupts, though for the AST2400 and AST2500 there is one
shared VIC IRQ for all LPC "subdevices". By contrast the AST2600 gives each
subdevice a distinct IRQ via the GIC.

All this combined leads to some complexity regarding the interrupts and handling
the MMIO accesses (in terms of mapping the access back to the function it's
affecting).

Finally, as a point of clarity, Aspeed BMCs also contain an LPC Host Controller
to master the LPC bus. This series does not concern itself with the LPC Host
Controller function, only with a subset of the peripheral devices the BMC
presents to the host.

I've tested the series using a combination of the ast2600-evb, witherspoon-bmc
and romulus-bmc machines along with a set of recently-posted patches for
Linux[2].

Please review!

Andrew

[2] https://lore.kernel.org/openbmc/20210219142523.3464540-1-andrew@aj.id.au/T/#m1e2029e7aa2be3056320e8d46b3b5b1539a776b4

Andrew Jeffery (3):
  arm: ast2600: Force a multiple of 32 of IRQs for the GIC
  arm: ast2600: Fix iBT IRQ ID
  hw/misc: Model KCS devices in the Aspeed LPC controller

Cédric Le Goater (1):
  hw/misc: Add a basic Aspeed LPC controller model

 docs/system/arm/aspeed.rst   |   2 +-
 hw/arm/aspeed_ast2600.c      |  44 +++-
 hw/arm/aspeed_soc.c          |  34 ++-
 hw/misc/aspeed_lpc.c         | 485 +++++++++++++++++++++++++++++++++++
 hw/misc/meson.build          |   7 +-
 include/hw/arm/aspeed_soc.h  |   3 +
 include/hw/misc/aspeed_lpc.h |  47 ++++
 7 files changed, 615 insertions(+), 7 deletions(-)
 create mode 100644 hw/misc/aspeed_lpc.c
 create mode 100644 include/hw/misc/aspeed_lpc.h


base-commit: 51db2d7cf26d05a961ec0ee0eb773594b32cc4a1
-- 
2.27.0

[PATCH 1/4] arm: ast2600: Force a multiple of 32 of IRQs for the GIC

[Andrew Jeffery]

This appears to be a requirement of the GIC model. The AST2600 allocates
197 GIC IRQs, which we will adjust shortly.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/arm/aspeed_ast2600.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index bf31ca351feb..bc0eeb058b24 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -65,7 +65,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
 
 #define ASPEED_A7MPCORE_ADDR 0x40460000
 
-#define ASPEED_SOC_AST2600_MAX_IRQ 128
+#define AST2600_MAX_IRQ 128
 
 /* Shared Peripheral Interrupt values below are offset by -32 from datasheet */
 static const int aspeed_soc_ast2600_irqmap[] = {
@@ -267,7 +267,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     object_property_set_int(OBJECT(&s->a7mpcore), "num-cpu", sc->num_cpus,
                             &error_abort);
     object_property_set_int(OBJECT(&s->a7mpcore), "num-irq",
-                            ASPEED_SOC_AST2600_MAX_IRQ + GIC_INTERNAL,
+                            ROUND_UP(AST2600_MAX_IRQ + GIC_INTERNAL, 32),
                             &error_abort);
 
     sysbus_realize(SYS_BUS_DEVICE(&s->a7mpcore), &error_abort);
-- 
2.27.0

Re: [PATCH 1/4] arm: ast2600: Force a multiple of 32 of IRQs for the GIC

[Philippe Mathieu-Daudé]

On 2/26/21 7:57 AM, Andrew Jeffery wrote:
> This appears to be a requirement of the GIC model.

If so this should be adjusted in the GIC or a15mp_priv_realize(),
not in each caller, isn't it?

> The AST2600 allocates
> 197 GIC IRQs, which we will adjust shortly.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  hw/arm/aspeed_ast2600.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Re: [PATCH 1/4] arm: ast2600: Force a multiple of 32 of IRQs for the GIC

[Andrew Jeffery]

On Fri, 26 Feb 2021, at 19:26, Philippe Mathieu-Daudé wrote:
> On 2/26/21 7:57 AM, Andrew Jeffery wrote:
> > This appears to be a requirement of the GIC model.
> 
> If so this should be adjusted in the GIC or a15mp_priv_realize(),
> not in each caller, isn't it?
> 

Maybe, let me look into it. I'll clean it up in v2 if it makes sense.

Cheers,

Andrew

Re: [PATCH 1/4] arm: ast2600: Force a multiple of 32 of IRQs for the GIC

[Andrew Jeffery]

On Mon, 1 Mar 2021, at 09:37, Andrew Jeffery wrote:
> 
> 
> On Fri, 26 Feb 2021, at 19:26, Philippe Mathieu-Daudé wrote:
> > On 2/26/21 7:57 AM, Andrew Jeffery wrote:
> > > This appears to be a requirement of the GIC model.
> > 
> > If so this should be adjusted in the GIC or a15mp_priv_realize(),
> > not in each caller, isn't it?
> > 
> 
> Maybe, let me look into it. I'll clean it up in v2 if it makes sense.

So the current behaviour has been around since 2009, originating in 
41c1e2f54e6f ("arm: make sure that number of irqs can be represented in 
GICD_TYPER."). The GIC architecture specification says:

"The GICD_TYPER.ITLinesNumber field identifies the number of 
implemented GICD_ISENABLERns, and therefore the maximum number of SPIs 
that might be supported."

While the code says:

    /* ITLinesNumber is represented as (N / 32) - 1 (see
     * gic_dist_readb) so this is an implementation imposed
     * restriction, not an architectural one:
     */
    if (s->num_irq < 32 || (s->num_irq % 32)) {
        error_setg(errp,
                   "%d interrupt lines unsupported: not divisible by 
32",
                   num_irq);
        return;
    }

My feeling is that it's better to be explicit in the models that are 
affected (i.e. leave the ROUND_UP() as I have it in this patch). This 
way if the implementation restriction is ever lifted, we know which 
models we can clean up. I won't be reworking the GIC to remove the 
restriction in this series, so unless you have a particularly strong 
preference/justification for the implicit ROUND_UP(), I plan to leave 
it as is.

Cheers,

Andrew

[PATCH 2/4] arm: ast2600: Fix iBT IRQ ID

[Andrew Jeffery]

The AST2600 allocates individual GIC IRQ lines for the LPC sub-devices.
This is a contrast to the AST2400 and AST2500 which use one shared VIC
IRQ line for the LPC sub-devices. Switch the iBT device to use the
GIC IRQ ID documented in the datasheet.

While we're here, set the number of IRQs to the allocated number of IRQs
in the datasheet.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/arm/aspeed_ast2600.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index bc0eeb058b24..2125a96ef317 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -65,7 +65,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
 
 #define ASPEED_A7MPCORE_ADDR 0x40460000
 
-#define AST2600_MAX_IRQ 128
+#define AST2600_MAX_IRQ 197
 
 /* Shared Peripheral Interrupt values below are offset by -32 from datasheet */
 static const int aspeed_soc_ast2600_irqmap[] = {
@@ -98,7 +98,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
     [ASPEED_DEV_WDT]       = 24,
     [ASPEED_DEV_PWM]       = 44,
     [ASPEED_DEV_LPC]       = 35,
-    [ASPEED_DEV_IBT]       = 35,    /* LPC */
+    [ASPEED_DEV_IBT]       = 143,
     [ASPEED_DEV_I2C]       = 110,   /* 110 -> 125 */
     [ASPEED_DEV_ETH1]      = 2,
     [ASPEED_DEV_ETH2]      = 3,
-- 
2.27.0

Re: [PATCH 2/4] arm: ast2600: Fix iBT IRQ ID

[Philippe Mathieu-Daudé]

On 2/26/21 7:57 AM, Andrew Jeffery wrote:
> The AST2600 allocates individual GIC IRQ lines for the LPC sub-devices.
> This is a contrast to the AST2400 and AST2500 which use one shared VIC
> IRQ line for the LPC sub-devices. Switch the iBT device to use the
> GIC IRQ ID documented in the datasheet.

[*]

> 
> While we're here, set the number of IRQs to the allocated number of IRQs
> in the datasheet.

Please do one change per patch. This would be the first change,
and [*] is the second.

> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  hw/arm/aspeed_ast2600.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Re: [PATCH 2/4] arm: ast2600: Fix iBT IRQ ID

[Andrew Jeffery]

On Fri, 26 Feb 2021, at 19:28, Philippe Mathieu-Daudé wrote:
> On 2/26/21 7:57 AM, Andrew Jeffery wrote:
> > The AST2600 allocates individual GIC IRQ lines for the LPC sub-devices.
> > This is a contrast to the AST2400 and AST2500 which use one shared VIC
> > IRQ line for the LPC sub-devices. Switch the iBT device to use the
> > GIC IRQ ID documented in the datasheet.
> 
> [*]
> 
> > 
> > While we're here, set the number of IRQs to the allocated number of IRQs
> > in the datasheet.
> 
> Please do one change per patch. This would be the first change,
> and [*] is the second.

Given that I had to change the current value to support the iBT device 
I figured it would be fine in the same patch, but sure, I can split 
this out in v2.

Andrew

[PATCH 3/4] hw/misc: Add a basic Aspeed LPC controller model

[Andrew Jeffery]

From: Cédric Le Goater <clg@kaod.org>

This is a very minimal framework to access registers which are used to
configure the AHB memory mapping of the flash chips on the LPC HC
Firmware address space.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 docs/system/arm/aspeed.rst   |   2 +-
 hw/arm/aspeed_ast2600.c      |  10 +++
 hw/arm/aspeed_soc.c          |  10 +++
 hw/misc/aspeed_lpc.c         | 131 +++++++++++++++++++++++++++++++++++
 hw/misc/meson.build          |   7 +-
 include/hw/arm/aspeed_soc.h  |   2 +
 include/hw/misc/aspeed_lpc.h |  32 +++++++++
 7 files changed, 192 insertions(+), 2 deletions(-)
 create mode 100644 hw/misc/aspeed_lpc.c
 create mode 100644 include/hw/misc/aspeed_lpc.h

diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
index 690bada7842b..2f6fa8938d02 100644
--- a/docs/system/arm/aspeed.rst
+++ b/docs/system/arm/aspeed.rst
@@ -48,6 +48,7 @@ Supported devices
  * UART
  * Ethernet controllers
  * Front LEDs (PCA9552 on I2C bus)
+ * LPC Peripheral Controller (a subset of subdevices are supported)
 
 
 Missing devices
@@ -56,7 +57,6 @@ Missing devices
  * Coprocessor support
  * ADC (out of tree implementation)
  * PWM and Fan Controller
- * LPC Bus Controller
  * Slave GPIO Controller
  * Super I/O Controller
  * Hash/Crypto Engine
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 2125a96ef317..60152de001e6 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -211,6 +211,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
 
     object_initialize_child(obj, "emmc-controller.sdhci", &s->emmc.slots[0],
                             TYPE_SYSBUS_SDHCI);
+
+    object_initialize_child(obj, "lpc", &s->lpc, TYPE_ASPEED_LPC);
 }
 
 /*
@@ -469,6 +471,14 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->emmc), 0, sc->memmap[ASPEED_DEV_EMMC]);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->emmc), 0,
                        aspeed_soc_get_irq(s, ASPEED_DEV_EMMC));
+
+    /* LPC */
+    if (!sysbus_realize(SYS_BUS_DEVICE(&s->lpc), errp)) {
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->emmc), 0,
+                       aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
 }
 
 static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 7eefd54ac07a..4f098da437ac 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -211,6 +211,8 @@ static void aspeed_soc_init(Object *obj)
         object_initialize_child(obj, "sdhci[*]", &s->sdhci.slots[i],
                                 TYPE_SYSBUS_SDHCI);
     }
+
+    object_initialize_child(obj, "lpc", &s->lpc, TYPE_ASPEED_LPC);
 }
 
 static void aspeed_soc_realize(DeviceState *dev, Error **errp)
@@ -393,6 +395,14 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
                     sc->memmap[ASPEED_DEV_SDHCI]);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci), 0,
                        aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI));
+
+    /* LPC */
+    if (!sysbus_realize(SYS_BUS_DEVICE(&s->lpc), errp)) {
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 0,
+                       aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
 }
 static Property aspeed_soc_properties[] = {
     DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
diff --git a/hw/misc/aspeed_lpc.c b/hw/misc/aspeed_lpc.c
new file mode 100644
index 000000000000..e668e985ff04
--- /dev/null
+++ b/hw/misc/aspeed_lpc.c
@@ -0,0 +1,131 @@
+/*
+ *  ASPEED LPC Controller
+ *
+ *  Copyright (C) 2017-2018 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/error-report.h"
+#include "hw/misc/aspeed_lpc.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+
+#define TO_REG(offset) ((offset) >> 2)
+
+#define HICR0                TO_REG(0x00)
+#define HICR1                TO_REG(0x04)
+#define HICR2                TO_REG(0x08)
+#define HICR3                TO_REG(0x0C)
+#define HICR4                TO_REG(0x10)
+#define HICR5                TO_REG(0x80)
+#define HICR6                TO_REG(0x84)
+#define HICR7                TO_REG(0x88)
+#define HICR8                TO_REG(0x8C)
+
+static uint64_t aspeed_lpc_read(void *opaque, hwaddr offset, unsigned size)
+{
+    AspeedLPCState *s = ASPEED_LPC(opaque);
+    int reg = TO_REG(offset);
+
+    if (reg >= ARRAY_SIZE(s->regs)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
+                      __func__, offset);
+        return 0;
+    }
+
+    return s->regs[reg];
+}
+
+static void aspeed_lpc_write(void *opaque, hwaddr offset, uint64_t data,
+                             unsigned int size)
+{
+    AspeedLPCState *s = ASPEED_LPC(opaque);
+    int reg = TO_REG(offset);
+
+    if (reg >= ARRAY_SIZE(s->regs)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx "\n",
+                      __func__, offset);
+        return;
+    }
+
+    s->regs[reg] = data;
+}
+
+static const MemoryRegionOps aspeed_lpc_ops = {
+    .read = aspeed_lpc_read,
+    .write = aspeed_lpc_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 4,
+    },
+};
+
+static void aspeed_lpc_reset(DeviceState *dev)
+{
+    struct AspeedLPCState *s = ASPEED_LPC(dev);
+
+    memset(s->regs, 0, sizeof(s->regs));
+
+    s->regs[HICR7] = s->hicr7;
+}
+
+static void aspeed_lpc_realize(DeviceState *dev, Error **errp)
+{
+    AspeedLPCState *s = ASPEED_LPC(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+    sysbus_init_irq(sbd, &s->irq);
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_lpc_ops, s,
+            TYPE_ASPEED_LPC, 0x1000);
+
+    sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static const VMStateDescription vmstate_aspeed_lpc = {
+    .name = TYPE_ASPEED_LPC,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(regs, AspeedLPCState, ASPEED_LPC_NR_REGS),
+        VMSTATE_END_OF_LIST(),
+    }
+};
+
+static Property aspeed_lpc_properties[] = {
+    DEFINE_PROP_UINT32("hicr7", AspeedLPCState, hicr7, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void aspeed_lpc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = aspeed_lpc_realize;
+    dc->reset = aspeed_lpc_reset;
+    dc->desc = "Aspeed LPC Controller",
+    dc->vmsd = &vmstate_aspeed_lpc;
+    device_class_set_props(dc, aspeed_lpc_properties);
+}
+
+static const TypeInfo aspeed_lpc_info = {
+    .name = TYPE_ASPEED_LPC,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AspeedLPCState),
+    .class_init = aspeed_lpc_class_init,
+};
+
+static void aspeed_lpc_register_types(void)
+{
+    type_register_static(&aspeed_lpc_info);
+}
+
+type_init(aspeed_lpc_register_types);
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 629283957fcc..e3263383cd59 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -102,7 +102,12 @@ softmmu_ss.add(when: 'CONFIG_ARMSSE_MHU', if_true: files('armsse-mhu.c'))
 softmmu_ss.add(when: 'CONFIG_PVPANIC_ISA', if_true: files('pvpanic-isa.c'))
 softmmu_ss.add(when: 'CONFIG_PVPANIC_PCI', if_true: files('pvpanic-pci.c'))
 softmmu_ss.add(when: 'CONFIG_AUX', if_true: files('auxbus.c'))
-softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_scu.c', 'aspeed_sdmc.c', 'aspeed_xdma.c'))
+softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files(
+  'aspeed_lpc.c',
+  'aspeed_scu.c',
+  'aspeed_sdmc.c',
+  'aspeed_xdma.c'))
+
 softmmu_ss.add(when: 'CONFIG_MSF2', if_true: files('msf2-sysreg.c'))
 softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
 
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 11cfe6e3585b..42c64bd28ba2 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -28,6 +28,7 @@
 #include "hw/sd/aspeed_sdhci.h"
 #include "hw/usb/hcd-ehci.h"
 #include "qom/object.h"
+#include "hw/misc/aspeed_lpc.h"
 
 #define ASPEED_SPIS_NUM  2
 #define ASPEED_EHCIS_NUM 2
@@ -61,6 +62,7 @@ struct AspeedSoCState {
     AspeedGPIOState gpio_1_8v;
     AspeedSDHCIState sdhci;
     AspeedSDHCIState emmc;
+    AspeedLPCState lpc;
 };
 
 #define TYPE_ASPEED_SOC "aspeed-soc"
diff --git a/include/hw/misc/aspeed_lpc.h b/include/hw/misc/aspeed_lpc.h
new file mode 100644
index 000000000000..0fbb7f68bed2
--- /dev/null
+++ b/include/hw/misc/aspeed_lpc.h
@@ -0,0 +1,32 @@
+/*
+ *  ASPEED LPC Controller
+ *
+ *  Copyright (C) 2017-2018 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef ASPEED_LPC_H
+#define ASPEED_LPC_H
+
+#include "hw/sysbus.h"
+
+#define TYPE_ASPEED_LPC "aspeed.lpc"
+#define ASPEED_LPC(obj) OBJECT_CHECK(AspeedLPCState, (obj), TYPE_ASPEED_LPC)
+
+#define ASPEED_LPC_NR_REGS (0x260 >> 2)
+
+typedef struct AspeedLPCState {
+    /* <private> */
+    SysBusDevice parent;
+
+    /*< public >*/
+    MemoryRegion iomem;
+    qemu_irq irq;
+
+    uint32_t regs[ASPEED_LPC_NR_REGS];
+    uint32_t hicr7;
+} AspeedLPCState;
+
+#endif /* _ASPEED_LPC_H_ */
-- 
2.27.0

[PATCH 4/4] hw/misc: Model KCS devices in the Aspeed LPC controller

[Andrew Jeffery]

Keyboard-Controller-Style devices for IPMI purposes are exposed via LPC
IO cycles from the BMC to the host.

Expose support on the BMC side by implementing the usual MMIO
behaviours, and expose the ability to inspect the KCS registers in
"host" style by accessing QOM properties associated with each register.

The model caters to the IRQ style of both the AST2600 and the earlier
SoCs (AST2400 and AST2500). The AST2600 allocates an IRQ for each LPC
sub-device, while there is a single IRQ shared across all subdevices on
the AST2400 and AST2500.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/arm/aspeed_ast2600.c      |  28 ++-
 hw/arm/aspeed_soc.c          |  24 ++-
 hw/misc/aspeed_lpc.c         | 354 +++++++++++++++++++++++++++++++++++
 include/hw/arm/aspeed_soc.h  |   1 +
 include/hw/misc/aspeed_lpc.h |  17 +-
 5 files changed, 421 insertions(+), 3 deletions(-)

diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 60152de001e6..fd463775d281 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -104,7 +104,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
     [ASPEED_DEV_ETH2]      = 3,
     [ASPEED_DEV_ETH3]      = 32,
     [ASPEED_DEV_ETH4]      = 33,
-
+    [ASPEED_DEV_KCS]       = 138,   /* 138 -> 142 */
 };
 
 static qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int ctrl)
@@ -477,8 +477,34 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
         return;
     }
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
+
+    /* Connect the LPC IRQ to the GIC. It is otherwise unused. */
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->emmc), 0,
                        aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
+
+    /*
+     * On the AST2600 LPC subdevice IRQs are connected straight to the GIC.
+     *
+     * LPC subdevice IRQ sources are offset from 1 because the LPC model caters
+     * to the AST2400 and AST2500. SoCs before the AST2600 have one LPC IRQ
+     * shared across the subdevices, and the shared IRQ output to the VIC is at
+     * offset 0.
+     */
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_1,
+                       qdev_get_gpio_in(DEVICE(&s->a7mpcore),
+                                sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_1));
+
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_2,
+                       qdev_get_gpio_in(DEVICE(&s->a7mpcore),
+                                sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_2));
+
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_3,
+                       qdev_get_gpio_in(DEVICE(&s->a7mpcore),
+                                sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_3));
+
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_4,
+                       qdev_get_gpio_in(DEVICE(&s->a7mpcore),
+                                sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_4));
 }
 
 static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 4f098da437ac..057d053c8478 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -112,7 +112,6 @@ static const int aspeed_soc_ast2400_irqmap[] = {
     [ASPEED_DEV_WDT]    = 27,
     [ASPEED_DEV_PWM]    = 28,
     [ASPEED_DEV_LPC]    = 8,
-    [ASPEED_DEV_IBT]    = 8, /* LPC */
     [ASPEED_DEV_I2C]    = 12,
     [ASPEED_DEV_ETH1]   = 2,
     [ASPEED_DEV_ETH2]   = 3,
@@ -401,8 +400,31 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
         return;
     }
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
+
+    /* Connect the LPC IRQ to the VIC */
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 0,
                        aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
+
+    /*
+     * On the AST2400 and AST2500 the one LPC IRQ is shared between all of the
+     * subdevices. Connect the LPC subdevice IRQs to the LPC controller IRQ (by
+     * contrast, on the AST2600, the subdevice IRQs are connected straight to
+     * the GIC).
+     *
+     * LPC subdevice IRQ sources are offset from 1 because the shared IRQ output
+     * to the VIC is at offset 0.
+     */
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_1,
+                       qdev_get_gpio_in(DEVICE(&s->lpc), aspeed_lpc_kcs_1));
+
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_2,
+                       qdev_get_gpio_in(DEVICE(&s->lpc), aspeed_lpc_kcs_2));
+
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_3,
+                       qdev_get_gpio_in(DEVICE(&s->lpc), aspeed_lpc_kcs_3));
+
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_4,
+                       qdev_get_gpio_in(DEVICE(&s->lpc), aspeed_lpc_kcs_4));
 }
 static Property aspeed_soc_properties[] = {
     DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
diff --git a/hw/misc/aspeed_lpc.c b/hw/misc/aspeed_lpc.c
index e668e985ff04..672131209dfa 100644
--- a/hw/misc/aspeed_lpc.c
+++ b/hw/misc/aspeed_lpc.c
@@ -12,20 +12,301 @@
 #include "qemu/error-report.h"
 #include "hw/misc/aspeed_lpc.h"
 #include "qapi/error.h"
+#include "qapi/visitor.h"
+#include "hw/irq.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 
 #define TO_REG(offset) ((offset) >> 2)
 
 #define HICR0                TO_REG(0x00)
+#define   HICR0_LPC3E        BIT(7)
+#define   HICR0_LPC2E        BIT(6)
+#define   HICR0_LPC1E        BIT(5)
 #define HICR1                TO_REG(0x04)
 #define HICR2                TO_REG(0x08)
+#define   HICR2_IBFIE3       BIT(3)
+#define   HICR2_IBFIE2       BIT(2)
+#define   HICR2_IBFIE1       BIT(1)
 #define HICR3                TO_REG(0x0C)
 #define HICR4                TO_REG(0x10)
+#define   HICR4_KCSENBL      BIT(2)
+#define IDR1                 TO_REG(0x24)
+#define IDR2                 TO_REG(0x28)
+#define IDR3                 TO_REG(0x2C)
+#define ODR1                 TO_REG(0x30)
+#define ODR2                 TO_REG(0x34)
+#define ODR3                 TO_REG(0x38)
+#define STR1                 TO_REG(0x3C)
+#define   STR_OBF            BIT(0)
+#define   STR_IBF            BIT(1)
+#define   STR_CMD_DATA       BIT(3)
+#define STR2                 TO_REG(0x40)
+#define STR3                 TO_REG(0x44)
 #define HICR5                TO_REG(0x80)
 #define HICR6                TO_REG(0x84)
 #define HICR7                TO_REG(0x88)
 #define HICR8                TO_REG(0x8C)
+#define HICRB                TO_REG(0x100)
+#define   HICRB_IBFIE4       BIT(1)
+#define   HICRB_LPC4E        BIT(0)
+#define IDR4                 TO_REG(0x114)
+#define ODR4                 TO_REG(0x118)
+#define STR4                 TO_REG(0x11C)
+
+enum aspeed_kcs_channel_id {
+    kcs_channel_1 = 0,
+    kcs_channel_2,
+    kcs_channel_3,
+    kcs_channel_4,
+};
+
+static const enum aspeed_lpc_subdevice aspeed_kcs_subdevice_map[] = {
+    [kcs_channel_1] = aspeed_lpc_kcs_1,
+    [kcs_channel_2] = aspeed_lpc_kcs_2,
+    [kcs_channel_3] = aspeed_lpc_kcs_3,
+    [kcs_channel_4] = aspeed_lpc_kcs_4,
+};
+
+struct aspeed_kcs_channel {
+    enum aspeed_kcs_channel_id id;
+
+    int idr;
+    int odr;
+    int str;
+};
+
+static const struct aspeed_kcs_channel aspeed_kcs_channel_map[] = {
+    [kcs_channel_1] = {
+        .id = kcs_channel_1,
+        .idr = IDR1,
+        .odr = ODR1,
+        .str = STR1
+    },
+
+    [kcs_channel_2] = {
+        .id = kcs_channel_2,
+        .idr = IDR2,
+        .odr = ODR2,
+        .str = STR2
+    },
+
+    [kcs_channel_3] = {
+        .id = kcs_channel_3,
+        .idr = IDR3,
+        .odr = ODR3,
+        .str = STR3
+    },
+
+    [kcs_channel_4] = {
+        .id = kcs_channel_4,
+        .idr = IDR4,
+        .odr = ODR4,
+        .str = STR4
+    },
+};
+
+struct aspeed_kcs_register_data {
+    const char *name;
+    int reg;
+    const struct aspeed_kcs_channel *chan;
+};
+
+static const struct aspeed_kcs_register_data aspeed_kcs_registers[] = {
+    {
+        .name = "idr1",
+        .reg = IDR1,
+        .chan = &aspeed_kcs_channel_map[kcs_channel_1],
+    },
+    {
+        .name = "odr1",
+        .reg = ODR1,
+        .chan = &aspeed_kcs_channel_map[kcs_channel_1],
+    },
+    {
+        .name = "str1",
+        .reg = STR1,
+        .chan = &aspeed_kcs_channel_map[kcs_channel_1],
+    },
+    {
+        .name = "idr2",
+        .reg = IDR2,
+        .chan = &aspeed_kcs_channel_map[kcs_channel_2],
+    },
+    {
+        .name = "odr2",
+        .reg = ODR2,
+        .chan = &aspeed_kcs_channel_map[kcs_channel_2],
+    },
+    {
+        .name = "str2",
+        .reg = STR2,
+        .chan = &aspeed_kcs_channel_map[kcs_channel_2],
+    },
+    {
+        .name = "idr3",
+        .reg = IDR3,
+        .chan = &aspeed_kcs_channel_map[kcs_channel_3],
+    },
+    {
+        .name = "odr3",
+        .reg = ODR3,
+        .chan = &aspeed_kcs_channel_map[kcs_channel_3],
+    },
+    {
+        .name = "str3",
+        .reg = STR3,
+        .chan = &aspeed_kcs_channel_map[kcs_channel_3],
+    },
+    {
+        .name = "idr4",
+        .reg = IDR4,
+        .chan = &aspeed_kcs_channel_map[kcs_channel_4],
+    },
+    {
+        .name = "odr4",
+        .reg = ODR4,
+        .chan = &aspeed_kcs_channel_map[kcs_channel_4],
+    },
+    {
+        .name = "str4",
+        .reg = STR4,
+        .chan = &aspeed_kcs_channel_map[kcs_channel_4],
+    },
+    { },
+};
+
+static const struct aspeed_kcs_register_data *
+aspeed_kcs_get_register_data_by_name(const char *name)
+{
+    const struct aspeed_kcs_register_data *pos = aspeed_kcs_registers;
+
+    while (pos->name) {
+        if (!strcmp(pos->name, name)) {
+            return pos;
+        }
+        pos++;
+    }
+
+    return NULL;
+}
+
+static const struct aspeed_kcs_channel *
+aspeed_kcs_get_channel_by_register(int reg)
+{
+    const struct aspeed_kcs_register_data *pos = aspeed_kcs_registers;
+
+    while (pos->name) {
+        if (pos->reg == reg) {
+            return pos->chan;
+        }
+        pos++;
+    }
+
+    return NULL;
+}
+
+static void aspeed_kcs_get_register_property(Object *obj,
+                                             Visitor *v,
+                                             const char *name,
+                                             void *opaque,
+                                             Error **errp)
+{
+    const struct aspeed_kcs_register_data *data;
+    AspeedLPCState *s = ASPEED_LPC(obj);
+    uint32_t val;
+
+    data = aspeed_kcs_get_register_data_by_name(name);
+    if (!data) {
+        return;
+    }
+
+    if (!strncmp("odr", name, 3)) {
+        s->regs[data->chan->str] &= ~STR_OBF;
+    }
+
+    val = s->regs[data->reg];
+
+    visit_type_uint32(v, name, &val, errp);
+}
+
+static bool aspeed_kcs_channel_enabled(AspeedLPCState *s,
+                                       const struct aspeed_kcs_channel *channel)
+{
+    switch (channel->id) {
+    case kcs_channel_1: return s->regs[HICR0] & HICR0_LPC1E;
+    case kcs_channel_2: return s->regs[HICR0] & HICR0_LPC2E;
+    case kcs_channel_3:
+        return (s->regs[HICR0] & HICR0_LPC3E) &&
+                    (s->regs[HICR4] & HICR4_KCSENBL);
+    case kcs_channel_4: return s->regs[HICRB] & HICRB_LPC4E;
+    default: return false;
+    }
+}
+
+static bool
+aspeed_kcs_channel_ibf_irq_enabled(AspeedLPCState *s,
+                                   const struct aspeed_kcs_channel *channel)
+{
+    if (!aspeed_kcs_channel_enabled(s, channel)) {
+            return false;
+    }
+
+    switch (channel->id) {
+    case kcs_channel_1: return s->regs[HICR2] & HICR2_IBFIE1;
+    case kcs_channel_2: return s->regs[HICR2] & HICR2_IBFIE2;
+    case kcs_channel_3: return s->regs[HICR2] & HICR2_IBFIE3;
+    case kcs_channel_4: return s->regs[HICRB] & HICRB_IBFIE4;
+    default: return false;
+    }
+}
+
+static void aspeed_kcs_set_register_property(Object *obj,
+                                             Visitor *v,
+                                             const char *name,
+                                             void *opaque,
+                                             Error **errp)
+{
+    const struct aspeed_kcs_register_data *data;
+    AspeedLPCState *s = ASPEED_LPC(obj);
+    uint32_t val;
+
+    data = aspeed_kcs_get_register_data_by_name(name);
+    if (!data) {
+        return;
+    }
+
+    if (!visit_type_uint32(v, name, &val, errp)) {
+        return;
+    }
+
+    if (strncmp("str", name, 3)) {
+        s->regs[data->reg] = val;
+    }
+
+    if (!strncmp("idr", name, 3)) {
+        s->regs[data->chan->str] |= STR_IBF;
+        if (aspeed_kcs_channel_ibf_irq_enabled(s, data->chan)) {
+            enum aspeed_lpc_subdevice subdev;
+
+            subdev = aspeed_kcs_subdevice_map[data->chan->id];
+            qemu_irq_raise(s->subdevice_irqs[subdev]);
+        }
+    }
+}
+
+static void aspeed_lpc_set_irq(void *opaque, int irq, int level)
+{
+    AspeedLPCState *s = (AspeedLPCState *)opaque;
+
+    if (level) {
+        s->subdevice_irqs_pending |= BIT(irq);
+    } else {
+        s->subdevice_irqs_pending &= ~BIT(irq);
+    }
+
+    qemu_set_irq(s->irq, !!s->subdevice_irqs_pending);
+}
 
 static uint64_t aspeed_lpc_read(void *opaque, hwaddr offset, unsigned size)
 {
@@ -39,6 +320,29 @@ static uint64_t aspeed_lpc_read(void *opaque, hwaddr offset, unsigned size)
         return 0;
     }
 
+    switch (reg) {
+    case IDR1:
+    case IDR2:
+    case IDR3:
+    case IDR4:
+    {
+        const struct aspeed_kcs_channel *channel;
+
+        channel = aspeed_kcs_get_channel_by_register(reg);
+        if (s->regs[channel->str] & STR_IBF) {
+            enum aspeed_lpc_subdevice subdev;
+
+            subdev = aspeed_kcs_subdevice_map[channel->id];
+            qemu_irq_lower(s->subdevice_irqs[subdev]);
+        }
+
+        s->regs[channel->str] &= ~STR_IBF;
+        break;
+    }
+    default:
+        break;
+    }
+
     return s->regs[reg];
 }
 
@@ -55,6 +359,18 @@ static void aspeed_lpc_write(void *opaque, hwaddr offset, uint64_t data,
         return;
     }
 
+
+    switch (reg) {
+    case ODR1:
+    case ODR2:
+    case ODR3:
+    case ODR4:
+        s->regs[aspeed_kcs_get_channel_by_register(reg)->str] |= STR_OBF;
+        break;
+    default:
+        break;
+    }
+
     s->regs[reg] = data;
 }
 
@@ -72,6 +388,8 @@ static void aspeed_lpc_reset(DeviceState *dev)
 {
     struct AspeedLPCState *s = ASPEED_LPC(dev);
 
+    s->subdevice_irqs_pending = 0;
+
     memset(s->regs, 0, sizeof(s->regs));
 
     s->regs[HICR7] = s->hicr7;
@@ -83,11 +401,46 @@ static void aspeed_lpc_realize(DeviceState *dev, Error **errp)
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
     sysbus_init_irq(sbd, &s->irq);
+    sysbus_init_irq(sbd, &s->subdevice_irqs[aspeed_lpc_kcs_1]);
+    sysbus_init_irq(sbd, &s->subdevice_irqs[aspeed_lpc_kcs_2]);
+    sysbus_init_irq(sbd, &s->subdevice_irqs[aspeed_lpc_kcs_3]);
+    sysbus_init_irq(sbd, &s->subdevice_irqs[aspeed_lpc_kcs_4]);
+    sysbus_init_irq(sbd, &s->subdevice_irqs[aspeed_lpc_ibt]);
 
     memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_lpc_ops, s,
             TYPE_ASPEED_LPC, 0x1000);
 
     sysbus_init_mmio(sbd, &s->iomem);
+
+    qdev_init_gpio_in(dev, aspeed_lpc_set_irq, ASPEED_LPC_NR_SUBDEVS);
+}
+
+static void aspeed_lpc_init(Object *obj)
+{
+    object_property_add(obj, "idr1", "uint32", aspeed_kcs_get_register_property,
+                        aspeed_kcs_set_register_property, NULL, NULL);
+    object_property_add(obj, "odr1", "uint32", aspeed_kcs_get_register_property,
+                        aspeed_kcs_set_register_property, NULL, NULL);
+    object_property_add(obj, "str1", "uint32", aspeed_kcs_get_register_property,
+                        aspeed_kcs_set_register_property, NULL, NULL);
+    object_property_add(obj, "idr2", "uint32", aspeed_kcs_get_register_property,
+                        aspeed_kcs_set_register_property, NULL, NULL);
+    object_property_add(obj, "odr2", "uint32", aspeed_kcs_get_register_property,
+                        aspeed_kcs_set_register_property, NULL, NULL);
+    object_property_add(obj, "str2", "uint32", aspeed_kcs_get_register_property,
+                        aspeed_kcs_set_register_property, NULL, NULL);
+    object_property_add(obj, "idr3", "uint32", aspeed_kcs_get_register_property,
+                        aspeed_kcs_set_register_property, NULL, NULL);
+    object_property_add(obj, "odr3", "uint32", aspeed_kcs_get_register_property,
+                        aspeed_kcs_set_register_property, NULL, NULL);
+    object_property_add(obj, "str3", "uint32", aspeed_kcs_get_register_property,
+                        aspeed_kcs_set_register_property, NULL, NULL);
+    object_property_add(obj, "idr4", "uint32", aspeed_kcs_get_register_property,
+                        aspeed_kcs_set_register_property, NULL, NULL);
+    object_property_add(obj, "odr4", "uint32", aspeed_kcs_get_register_property,
+                        aspeed_kcs_set_register_property, NULL, NULL);
+    object_property_add(obj, "str4", "uint32", aspeed_kcs_get_register_property,
+                        aspeed_kcs_set_register_property, NULL, NULL);
 }
 
 static const VMStateDescription vmstate_aspeed_lpc = {
@@ -121,6 +474,7 @@ static const TypeInfo aspeed_lpc_info = {
     .parent = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(AspeedLPCState),
     .class_init = aspeed_lpc_class_init,
+    .instance_init = aspeed_lpc_init,
 };
 
 static void aspeed_lpc_register_types(void)
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 42c64bd28ba2..9359d6da336d 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -132,6 +132,7 @@ enum {
     ASPEED_DEV_SDRAM,
     ASPEED_DEV_XDMA,
     ASPEED_DEV_EMMC,
+    ASPEED_DEV_KCS,
 };
 
 #endif /* ASPEED_SOC_H */
diff --git a/include/hw/misc/aspeed_lpc.h b/include/hw/misc/aspeed_lpc.h
index 0fbb7f68bed2..df418cfcd36c 100644
--- a/include/hw/misc/aspeed_lpc.h
+++ b/include/hw/misc/aspeed_lpc.h
@@ -12,10 +12,22 @@
 
 #include "hw/sysbus.h"
 
+#include <stdint.h>
+
 #define TYPE_ASPEED_LPC "aspeed.lpc"
 #define ASPEED_LPC(obj) OBJECT_CHECK(AspeedLPCState, (obj), TYPE_ASPEED_LPC)
 
-#define ASPEED_LPC_NR_REGS (0x260 >> 2)
+#define ASPEED_LPC_NR_REGS      (0x260 >> 2)
+
+enum aspeed_lpc_subdevice {
+    aspeed_lpc_kcs_1 = 0,
+    aspeed_lpc_kcs_2,
+    aspeed_lpc_kcs_3,
+    aspeed_lpc_kcs_4,
+    aspeed_lpc_ibt,
+};
+
+#define ASPEED_LPC_NR_SUBDEVS   5
 
 typedef struct AspeedLPCState {
     /* <private> */
@@ -25,6 +37,9 @@ typedef struct AspeedLPCState {
     MemoryRegion iomem;
     qemu_irq irq;
 
+    qemu_irq subdevice_irqs[ASPEED_LPC_NR_SUBDEVS];
+    uint32_t subdevice_irqs_pending;
+
     uint32_t regs[ASPEED_LPC_NR_REGS];
     uint32_t hicr7;
 } AspeedLPCState;
-- 
2.27.0

Re: [PATCH 4/4] hw/misc: Model KCS devices in the Aspeed LPC controller

[Cédric Le Goater]

On 2/26/21 7:57 AM, Andrew Jeffery wrote:
> Keyboard-Controller-Style devices for IPMI purposes are exposed via LPC
> IO cycles from the BMC to the host.
> 
> Expose support on the BMC side by implementing the usual MMIO
> behaviours, and expose the ability to inspect the KCS registers in
> "host" style by accessing QOM properties associated with each register.
> 
> The model caters to the IRQ style of both the AST2600 and the earlier
> SoCs (AST2400 and AST2500). The AST2600 allocates an IRQ for each LPC
> sub-device, while there is a single IRQ shared across all subdevices on
> the AST2400 and AST2500.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  hw/arm/aspeed_ast2600.c      |  28 ++-
>  hw/arm/aspeed_soc.c          |  24 ++-
>  hw/misc/aspeed_lpc.c         | 354 +++++++++++++++++++++++++++++++++++
>  include/hw/arm/aspeed_soc.h  |   1 +
>  include/hw/misc/aspeed_lpc.h |  17 +-
>  5 files changed, 421 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 60152de001e6..fd463775d281 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -104,7 +104,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
>      [ASPEED_DEV_ETH2]      = 3,
>      [ASPEED_DEV_ETH3]      = 32,
>      [ASPEED_DEV_ETH4]      = 33,
> -
> +    [ASPEED_DEV_KCS]       = 138,   /* 138 -> 142 */
>  };
>  
>  static qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int ctrl)
> @@ -477,8 +477,34 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
> +
> +    /* Connect the LPC IRQ to the GIC. It is otherwise unused. */
>      sysbus_connect_irq(SYS_BUS_DEVICE(&s->emmc), 0,
>                         aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
> +
> +    /*
> +     * On the AST2600 LPC subdevice IRQs are connected straight to the GIC.
> +     *
> +     * LPC subdevice IRQ sources are offset from 1 because the LPC model caters
> +     * to the AST2400 and AST2500. SoCs before the AST2600 have one LPC IRQ
> +     * shared across the subdevices, and the shared IRQ output to the VIC is at
> +     * offset 0.
> +     */
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_1,
> +                       qdev_get_gpio_in(DEVICE(&s->a7mpcore),
> +                                sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_1));
> +
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_2,
> +                       qdev_get_gpio_in(DEVICE(&s->a7mpcore),
> +                                sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_2));
> +
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_3,
> +                       qdev_get_gpio_in(DEVICE(&s->a7mpcore),
> +                                sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_3));
> +
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_4,
> +                       qdev_get_gpio_in(DEVICE(&s->a7mpcore),
> +                                sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_4));
>  }
>  
>  static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 4f098da437ac..057d053c8478 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -112,7 +112,6 @@ static const int aspeed_soc_ast2400_irqmap[] = {
>      [ASPEED_DEV_WDT]    = 27,
>      [ASPEED_DEV_PWM]    = 28,
>      [ASPEED_DEV_LPC]    = 8,
> -    [ASPEED_DEV_IBT]    = 8, /* LPC */
>      [ASPEED_DEV_I2C]    = 12,
>      [ASPEED_DEV_ETH1]   = 2,
>      [ASPEED_DEV_ETH2]   = 3,
> @@ -401,8 +400,31 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
> +
> +    /* Connect the LPC IRQ to the VIC */
>      sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 0,
>                         aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
> +
> +    /*
> +     * On the AST2400 and AST2500 the one LPC IRQ is shared between all of the
> +     * subdevices. Connect the LPC subdevice IRQs to the LPC controller IRQ (by
> +     * contrast, on the AST2600, the subdevice IRQs are connected straight to
> +     * the GIC).
> +     *
> +     * LPC subdevice IRQ sources are offset from 1 because the shared IRQ output
> +     * to the VIC is at offset 0.
> +     */
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_1,
> +                       qdev_get_gpio_in(DEVICE(&s->lpc), aspeed_lpc_kcs_1));
> +
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_2,
> +                       qdev_get_gpio_in(DEVICE(&s->lpc), aspeed_lpc_kcs_2));
> +
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_3,
> +                       qdev_get_gpio_in(DEVICE(&s->lpc), aspeed_lpc_kcs_3));
> +
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_4,
> +                       qdev_get_gpio_in(DEVICE(&s->lpc), aspeed_lpc_kcs_4));
>  }
>  static Property aspeed_soc_properties[] = {
>      DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
> diff --git a/hw/misc/aspeed_lpc.c b/hw/misc/aspeed_lpc.c
> index e668e985ff04..672131209dfa 100644
> --- a/hw/misc/aspeed_lpc.c
> +++ b/hw/misc/aspeed_lpc.c
> @@ -12,20 +12,301 @@
>  #include "qemu/error-report.h"
>  #include "hw/misc/aspeed_lpc.h"
>  #include "qapi/error.h"
> +#include "qapi/visitor.h"
> +#include "hw/irq.h"
>  #include "hw/qdev-properties.h"
>  #include "migration/vmstate.h"
>  
>  #define TO_REG(offset) ((offset) >> 2)
>  
>  #define HICR0                TO_REG(0x00)
> +#define   HICR0_LPC3E        BIT(7)
> +#define   HICR0_LPC2E        BIT(6)
> +#define   HICR0_LPC1E        BIT(5)
>  #define HICR1                TO_REG(0x04)
>  #define HICR2                TO_REG(0x08)
> +#define   HICR2_IBFIE3       BIT(3)
> +#define   HICR2_IBFIE2       BIT(2)
> +#define   HICR2_IBFIE1       BIT(1)
>  #define HICR3                TO_REG(0x0C)
>  #define HICR4                TO_REG(0x10)
> +#define   HICR4_KCSENBL      BIT(2)
> +#define IDR1                 TO_REG(0x24)
> +#define IDR2                 TO_REG(0x28)
> +#define IDR3                 TO_REG(0x2C)
> +#define ODR1                 TO_REG(0x30)
> +#define ODR2                 TO_REG(0x34)
> +#define ODR3                 TO_REG(0x38)
> +#define STR1                 TO_REG(0x3C)
> +#define   STR_OBF            BIT(0)
> +#define   STR_IBF            BIT(1)
> +#define   STR_CMD_DATA       BIT(3)
> +#define STR2                 TO_REG(0x40)
> +#define STR3                 TO_REG(0x44)
>  #define HICR5                TO_REG(0x80)
>  #define HICR6                TO_REG(0x84)
>  #define HICR7                TO_REG(0x88)
>  #define HICR8                TO_REG(0x8C)
> +#define HICRB                TO_REG(0x100)
> +#define   HICRB_IBFIE4       BIT(1)
> +#define   HICRB_LPC4E        BIT(0)
> +#define IDR4                 TO_REG(0x114)
> +#define ODR4                 TO_REG(0x118)
> +#define STR4                 TO_REG(0x11C)
> +
> +enum aspeed_kcs_channel_id {
> +    kcs_channel_1 = 0,
> +    kcs_channel_2,
> +    kcs_channel_3,
> +    kcs_channel_4,
> +};
> +
> +static const enum aspeed_lpc_subdevice aspeed_kcs_subdevice_map[] = {
> +    [kcs_channel_1] = aspeed_lpc_kcs_1,
> +    [kcs_channel_2] = aspeed_lpc_kcs_2,
> +    [kcs_channel_3] = aspeed_lpc_kcs_3,
> +    [kcs_channel_4] = aspeed_lpc_kcs_4,
> +};
> +
> +struct aspeed_kcs_channel {
> +    enum aspeed_kcs_channel_id id;
> +
> +    int idr;
> +    int odr;
> +    int str;
> +};
> +
> +static const struct aspeed_kcs_channel aspeed_kcs_channel_map[] = {
> +    [kcs_channel_1] = {
> +        .id = kcs_channel_1,
> +        .idr = IDR1,
> +        .odr = ODR1,
> +        .str = STR1
> +    },
> +
> +    [kcs_channel_2] = {
> +        .id = kcs_channel_2,
> +        .idr = IDR2,
> +        .odr = ODR2,
> +        .str = STR2
> +    },
> +
> +    [kcs_channel_3] = {
> +        .id = kcs_channel_3,
> +        .idr = IDR3,
> +        .odr = ODR3,
> +        .str = STR3
> +    },
> +
> +    [kcs_channel_4] = {
> +        .id = kcs_channel_4,
> +        .idr = IDR4,
> +        .odr = ODR4,
> +        .str = STR4
> +    },
> +};
> +
> +struct aspeed_kcs_register_data {
> +    const char *name;
> +    int reg;
> +    const struct aspeed_kcs_channel *chan;
> +};
> +
> +static const struct aspeed_kcs_register_data aspeed_kcs_registers[] = {
> +    {
> +        .name = "idr1",
> +        .reg = IDR1,
> +        .chan = &aspeed_kcs_channel_map[kcs_channel_1],
> +    },
> +    {
> +        .name = "odr1",
> +        .reg = ODR1,
> +        .chan = &aspeed_kcs_channel_map[kcs_channel_1],
> +    },
> +    {
> +        .name = "str1",
> +        .reg = STR1,
> +        .chan = &aspeed_kcs_channel_map[kcs_channel_1],
> +    },
> +    {
> +        .name = "idr2",
> +        .reg = IDR2,
> +        .chan = &aspeed_kcs_channel_map[kcs_channel_2],
> +    },
> +    {
> +        .name = "odr2",
> +        .reg = ODR2,
> +        .chan = &aspeed_kcs_channel_map[kcs_channel_2],
> +    },
> +    {
> +        .name = "str2",
> +        .reg = STR2,
> +        .chan = &aspeed_kcs_channel_map[kcs_channel_2],
> +    },
> +    {
> +        .name = "idr3",
> +        .reg = IDR3,
> +        .chan = &aspeed_kcs_channel_map[kcs_channel_3],
> +    },
> +    {
> +        .name = "odr3",
> +        .reg = ODR3,
> +        .chan = &aspeed_kcs_channel_map[kcs_channel_3],
> +    },
> +    {
> +        .name = "str3",
> +        .reg = STR3,
> +        .chan = &aspeed_kcs_channel_map[kcs_channel_3],
> +    },
> +    {
> +        .name = "idr4",
> +        .reg = IDR4,
> +        .chan = &aspeed_kcs_channel_map[kcs_channel_4],
> +    },
> +    {
> +        .name = "odr4",
> +        .reg = ODR4,
> +        .chan = &aspeed_kcs_channel_map[kcs_channel_4],
> +    },
> +    {
> +        .name = "str4",
> +        .reg = STR4,
> +        .chan = &aspeed_kcs_channel_map[kcs_channel_4],
> +    },
> +    { },
> +};
> +
> +static const struct aspeed_kcs_register_data *
> +aspeed_kcs_get_register_data_by_name(const char *name)
> +{
> +    const struct aspeed_kcs_register_data *pos = aspeed_kcs_registers;
> +
> +    while (pos->name) {
> +        if (!strcmp(pos->name, name)) {
> +            return pos;
> +        }
> +        pos++;
> +    }
> +
> +    return NULL;
> +}
> +
> +static const struct aspeed_kcs_channel *
> +aspeed_kcs_get_channel_by_register(int reg)
> +{
> +    const struct aspeed_kcs_register_data *pos = aspeed_kcs_registers;
> +
> +    while (pos->name) {
> +        if (pos->reg == reg) {
> +            return pos->chan;
> +        }
> +        pos++;
> +    }
> +
> +    return NULL;
> +}
> +
> +static void aspeed_kcs_get_register_property(Object *obj,
> +                                             Visitor *v,
> +                                             const char *name,
> +                                             void *opaque,
> +                                             Error **errp)
> +{
> +    const struct aspeed_kcs_register_data *data;
> +    AspeedLPCState *s = ASPEED_LPC(obj);
> +    uint32_t val;
> +
> +    data = aspeed_kcs_get_register_data_by_name(name);
> +    if (!data) {
> +        return;
> +    }
> +
> +    if (!strncmp("odr", name, 3)) {
> +        s->regs[data->chan->str] &= ~STR_OBF;
> +    }
> +
> +    val = s->regs[data->reg];
> +
> +    visit_type_uint32(v, name, &val, errp);
> +}
> +
> +static bool aspeed_kcs_channel_enabled(AspeedLPCState *s,
> +                                       const struct aspeed_kcs_channel *channel)
> +{
> +    switch (channel->id) {
> +    case kcs_channel_1: return s->regs[HICR0] & HICR0_LPC1E;
> +    case kcs_channel_2: return s->regs[HICR0] & HICR0_LPC2E;
> +    case kcs_channel_3:
> +        return (s->regs[HICR0] & HICR0_LPC3E) &&
> +                    (s->regs[HICR4] & HICR4_KCSENBL);
> +    case kcs_channel_4: return s->regs[HICRB] & HICRB_LPC4E;
> +    default: return false;
> +    }
> +}
> +
> +static bool
> +aspeed_kcs_channel_ibf_irq_enabled(AspeedLPCState *s,
> +                                   const struct aspeed_kcs_channel *channel)
> +{
> +    if (!aspeed_kcs_channel_enabled(s, channel)) {
> +            return false;
> +    }
> +
> +    switch (channel->id) {
> +    case kcs_channel_1: return s->regs[HICR2] & HICR2_IBFIE1;
> +    case kcs_channel_2: return s->regs[HICR2] & HICR2_IBFIE2;
> +    case kcs_channel_3: return s->regs[HICR2] & HICR2_IBFIE3;
> +    case kcs_channel_4: return s->regs[HICRB] & HICRB_IBFIE4;
> +    default: return false;
> +    }
> +}
> +
> +static void aspeed_kcs_set_register_property(Object *obj,
> +                                             Visitor *v,
> +                                             const char *name,
> +                                             void *opaque,
> +                                             Error **errp)
> +{
> +    const struct aspeed_kcs_register_data *data;
> +    AspeedLPCState *s = ASPEED_LPC(obj);
> +    uint32_t val;
> +
> +    data = aspeed_kcs_get_register_data_by_name(name);
> +    if (!data) {
> +        return;
> +    }
> +
> +    if (!visit_type_uint32(v, name, &val, errp)) {
> +        return;
> +    }
> +
> +    if (strncmp("str", name, 3)) {
> +        s->regs[data->reg] = val;
> +    }
> +
> +    if (!strncmp("idr", name, 3)) {
> +        s->regs[data->chan->str] |= STR_IBF;
> +        if (aspeed_kcs_channel_ibf_irq_enabled(s, data->chan)) {
> +            enum aspeed_lpc_subdevice subdev;
> +
> +            subdev = aspeed_kcs_subdevice_map[data->chan->id];
> +            qemu_irq_raise(s->subdevice_irqs[subdev]);
> +        }
> +    }
> +}
> +
> +static void aspeed_lpc_set_irq(void *opaque, int irq, int level)
> +{
> +    AspeedLPCState *s = (AspeedLPCState *)opaque;
> +
> +    if (level) {
> +        s->subdevice_irqs_pending |= BIT(irq);
> +    } else {
> +        s->subdevice_irqs_pending &= ~BIT(irq);
> +    }
> +
> +    qemu_set_irq(s->irq, !!s->subdevice_irqs_pending);
> +}


Nice ! I have adapted the iBT model ant it works fine.

 
>  
>  static uint64_t aspeed_lpc_read(void *opaque, hwaddr offset, unsigned size)
>  {
> @@ -39,6 +320,29 @@ static uint64_t aspeed_lpc_read(void *opaque, hwaddr offset, unsigned size)
>          return 0;
>      }
>  
> +    switch (reg) {
> +    case IDR1:
> +    case IDR2:
> +    case IDR3:
> +    case IDR4:
> +    {
> +        const struct aspeed_kcs_channel *channel;
> +
> +        channel = aspeed_kcs_get_channel_by_register(reg);
> +        if (s->regs[channel->str] & STR_IBF) {
> +            enum aspeed_lpc_subdevice subdev;
> +
> +            subdev = aspeed_kcs_subdevice_map[channel->id];
> +            qemu_irq_lower(s->subdevice_irqs[subdev]);
> +        }
> +
> +        s->regs[channel->str] &= ~STR_IBF;
> +        break;
> +    }
> +    default:
> +        break;
> +    }
> +
>      return s->regs[reg];
>  }
>  
> @@ -55,6 +359,18 @@ static void aspeed_lpc_write(void *opaque, hwaddr offset, uint64_t data,
>          return;
>      }
>  
> +
> +    switch (reg) {
> +    case ODR1:
> +    case ODR2:
> +    case ODR3:
> +    case ODR4:
> +        s->regs[aspeed_kcs_get_channel_by_register(reg)->str] |= STR_OBF;
> +        break;
> +    default:
> +        break;
> +    }
> +
>      s->regs[reg] = data;
>  }
>  
> @@ -72,6 +388,8 @@ static void aspeed_lpc_reset(DeviceState *dev)
>  {
>      struct AspeedLPCState *s = ASPEED_LPC(dev);
>  
> +    s->subdevice_irqs_pending = 0;
> +
>      memset(s->regs, 0, sizeof(s->regs));
>  
>      s->regs[HICR7] = s->hicr7;
> @@ -83,11 +401,46 @@ static void aspeed_lpc_realize(DeviceState *dev, Error **errp)
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
>      sysbus_init_irq(sbd, &s->irq);
> +    sysbus_init_irq(sbd, &s->subdevice_irqs[aspeed_lpc_kcs_1]);
> +    sysbus_init_irq(sbd, &s->subdevice_irqs[aspeed_lpc_kcs_2]);
> +    sysbus_init_irq(sbd, &s->subdevice_irqs[aspeed_lpc_kcs_3]);
> +    sysbus_init_irq(sbd, &s->subdevice_irqs[aspeed_lpc_kcs_4]);
> +    sysbus_init_irq(sbd, &s->subdevice_irqs[aspeed_lpc_ibt]);
>  
>      memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_lpc_ops, s,
>              TYPE_ASPEED_LPC, 0x1000);
>  
>      sysbus_init_mmio(sbd, &s->iomem);
> +
> +    qdev_init_gpio_in(dev, aspeed_lpc_set_irq, ASPEED_LPC_NR_SUBDEVS);
> +}
> +
> +static void aspeed_lpc_init(Object *obj)
> +{
> +    object_property_add(obj, "idr1", "uint32", aspeed_kcs_get_register_property,
> +                        aspeed_kcs_set_register_property, NULL, NULL);
> +    object_property_add(obj, "odr1", "uint32", aspeed_kcs_get_register_property,
> +                        aspeed_kcs_set_register_property, NULL, NULL);
> +    object_property_add(obj, "str1", "uint32", aspeed_kcs_get_register_property,
> +                        aspeed_kcs_set_register_property, NULL, NULL);
> +    object_property_add(obj, "idr2", "uint32", aspeed_kcs_get_register_property,
> +                        aspeed_kcs_set_register_property, NULL, NULL);
> +    object_property_add(obj, "odr2", "uint32", aspeed_kcs_get_register_property,
> +                        aspeed_kcs_set_register_property, NULL, NULL);
> +    object_property_add(obj, "str2", "uint32", aspeed_kcs_get_register_property,
> +                        aspeed_kcs_set_register_property, NULL, NULL);
> +    object_property_add(obj, "idr3", "uint32", aspeed_kcs_get_register_property,
> +                        aspeed_kcs_set_register_property, NULL, NULL);
> +    object_property_add(obj, "odr3", "uint32", aspeed_kcs_get_register_property,
> +                        aspeed_kcs_set_register_property, NULL, NULL);
> +    object_property_add(obj, "str3", "uint32", aspeed_kcs_get_register_property,
> +                        aspeed_kcs_set_register_property, NULL, NULL);
> +    object_property_add(obj, "idr4", "uint32", aspeed_kcs_get_register_property,
> +                        aspeed_kcs_set_register_property, NULL, NULL);
> +    object_property_add(obj, "odr4", "uint32", aspeed_kcs_get_register_property,
> +                        aspeed_kcs_set_register_property, NULL, NULL);
> +    object_property_add(obj, "str4", "uint32", aspeed_kcs_get_register_property,
> +                        aspeed_kcs_set_register_property, NULL, NULL);
>  }
>  
>  static const VMStateDescription vmstate_aspeed_lpc = {
> @@ -121,6 +474,7 @@ static const TypeInfo aspeed_lpc_info = {
>      .parent = TYPE_SYS_BUS_DEVICE,
>      .instance_size = sizeof(AspeedLPCState),
>      .class_init = aspeed_lpc_class_init,
> +    .instance_init = aspeed_lpc_init,
>  };
>  
>  static void aspeed_lpc_register_types(void)
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index 42c64bd28ba2..9359d6da336d 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -132,6 +132,7 @@ enum {
>      ASPEED_DEV_SDRAM,
>      ASPEED_DEV_XDMA,
>      ASPEED_DEV_EMMC,
> +    ASPEED_DEV_KCS,
>  };
>  
>  #endif /* ASPEED_SOC_H */
> diff --git a/include/hw/misc/aspeed_lpc.h b/include/hw/misc/aspeed_lpc.h
> index 0fbb7f68bed2..df418cfcd36c 100644
> --- a/include/hw/misc/aspeed_lpc.h
> +++ b/include/hw/misc/aspeed_lpc.h
> @@ -12,10 +12,22 @@
>  
>  #include "hw/sysbus.h"
>  
> +#include <stdint.h>
> +
>  #define TYPE_ASPEED_LPC "aspeed.lpc"
>  #define ASPEED_LPC(obj) OBJECT_CHECK(AspeedLPCState, (obj), TYPE_ASPEED_LPC)
>  
> -#define ASPEED_LPC_NR_REGS (0x260 >> 2)
> +#define ASPEED_LPC_NR_REGS      (0x260 >> 2)
> +
> +enum aspeed_lpc_subdevice {
> +    aspeed_lpc_kcs_1 = 0,
> +    aspeed_lpc_kcs_2,
> +    aspeed_lpc_kcs_3,
> +    aspeed_lpc_kcs_4,
> +    aspeed_lpc_ibt,
> +};
> +
> +#define ASPEED_LPC_NR_SUBDEVS   5
>  
>  typedef struct AspeedLPCState {
>      /* <private> */
> @@ -25,6 +37,9 @@ typedef struct AspeedLPCState {
>      MemoryRegion iomem;
>      qemu_irq irq;
>  
> +    qemu_irq subdevice_irqs[ASPEED_LPC_NR_SUBDEVS];
> +    uint32_t subdevice_irqs_pending;


This field should be added to the vmstate.

C.

> +
>      uint32_t regs[ASPEED_LPC_NR_REGS];
>      uint32_t hicr7;
>  } AspeedLPCState;
> 

Re: [PATCH 4/4] hw/misc: Model KCS devices in the Aspeed LPC controller

[Andrew Jeffery]

On Fri, 26 Feb 2021, at 20:21, Cédric Le Goater wrote:
> On 2/26/21 7:57 AM, Andrew Jeffery wrote:
> > Keyboard-Controller-Style devices for IPMI purposes are exposed via LPC
> > IO cycles from the BMC to the host.
> > 
> > Expose support on the BMC side by implementing the usual MMIO
> > behaviours, and expose the ability to inspect the KCS registers in
> > "host" style by accessing QOM properties associated with each register.
> > 
> > The model caters to the IRQ style of both the AST2600 and the earlier
> > SoCs (AST2400 and AST2500). The AST2600 allocates an IRQ for each LPC
> > sub-device, while there is a single IRQ shared across all subdevices on
> > the AST2400 and AST2500.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  hw/arm/aspeed_ast2600.c      |  28 ++-
> >  hw/arm/aspeed_soc.c          |  24 ++-
> >  hw/misc/aspeed_lpc.c         | 354 +++++++++++++++++++++++++++++++++++
> >  include/hw/arm/aspeed_soc.h  |   1 +
> >  include/hw/misc/aspeed_lpc.h |  17 +-
> >  5 files changed, 421 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> > index 60152de001e6..fd463775d281 100644
> > --- a/hw/arm/aspeed_ast2600.c
> > +++ b/hw/arm/aspeed_ast2600.c
> > @@ -104,7 +104,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
> >      [ASPEED_DEV_ETH2]      = 3,
> >      [ASPEED_DEV_ETH3]      = 32,
> >      [ASPEED_DEV_ETH4]      = 33,
> > -
> > +    [ASPEED_DEV_KCS]       = 138,   /* 138 -> 142 */
> >  };
> >  
> >  static qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int ctrl)
> > @@ -477,8 +477,34 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
> >          return;
> >      }
> >      sysbus_mmio_map(SYS_BUS_DEVICE(&s->lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
> > +
> > +    /* Connect the LPC IRQ to the GIC. It is otherwise unused. */
> >      sysbus_connect_irq(SYS_BUS_DEVICE(&s->emmc), 0,
> >                         aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
> > +
> > +    /*
> > +     * On the AST2600 LPC subdevice IRQs are connected straight to the GIC.
> > +     *
> > +     * LPC subdevice IRQ sources are offset from 1 because the LPC model caters
> > +     * to the AST2400 and AST2500. SoCs before the AST2600 have one LPC IRQ
> > +     * shared across the subdevices, and the shared IRQ output to the VIC is at
> > +     * offset 0.
> > +     */
> > +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_1,
> > +                       qdev_get_gpio_in(DEVICE(&s->a7mpcore),
> > +                                sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_1));
> > +
> > +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_2,
> > +                       qdev_get_gpio_in(DEVICE(&s->a7mpcore),
> > +                                sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_2));
> > +
> > +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_3,
> > +                       qdev_get_gpio_in(DEVICE(&s->a7mpcore),
> > +                                sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_3));
> > +
> > +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_4,
> > +                       qdev_get_gpio_in(DEVICE(&s->a7mpcore),
> > +                                sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_4));
> >  }
> >  
> >  static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
> > diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> > index 4f098da437ac..057d053c8478 100644
> > --- a/hw/arm/aspeed_soc.c
> > +++ b/hw/arm/aspeed_soc.c
> > @@ -112,7 +112,6 @@ static const int aspeed_soc_ast2400_irqmap[] = {
> >      [ASPEED_DEV_WDT]    = 27,
> >      [ASPEED_DEV_PWM]    = 28,
> >      [ASPEED_DEV_LPC]    = 8,
> > -    [ASPEED_DEV_IBT]    = 8, /* LPC */
> >      [ASPEED_DEV_I2C]    = 12,
> >      [ASPEED_DEV_ETH1]   = 2,
> >      [ASPEED_DEV_ETH2]   = 3,
> > @@ -401,8 +400,31 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> >          return;
> >      }
> >      sysbus_mmio_map(SYS_BUS_DEVICE(&s->lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
> > +
> > +    /* Connect the LPC IRQ to the VIC */
> >      sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 0,
> >                         aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
> > +
> > +    /*
> > +     * On the AST2400 and AST2500 the one LPC IRQ is shared between all of the
> > +     * subdevices. Connect the LPC subdevice IRQs to the LPC controller IRQ (by
> > +     * contrast, on the AST2600, the subdevice IRQs are connected straight to
> > +     * the GIC).
> > +     *
> > +     * LPC subdevice IRQ sources are offset from 1 because the shared IRQ output
> > +     * to the VIC is at offset 0.
> > +     */
> > +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_1,
> > +                       qdev_get_gpio_in(DEVICE(&s->lpc), aspeed_lpc_kcs_1));
> > +
> > +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_2,
> > +                       qdev_get_gpio_in(DEVICE(&s->lpc), aspeed_lpc_kcs_2));
> > +
> > +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_3,
> > +                       qdev_get_gpio_in(DEVICE(&s->lpc), aspeed_lpc_kcs_3));
> > +
> > +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->lpc), 1 + aspeed_lpc_kcs_4,
> > +                       qdev_get_gpio_in(DEVICE(&s->lpc), aspeed_lpc_kcs_4));
> >  }
> >  static Property aspeed_soc_properties[] = {
> >      DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
> > diff --git a/hw/misc/aspeed_lpc.c b/hw/misc/aspeed_lpc.c
> > index e668e985ff04..672131209dfa 100644
> > --- a/hw/misc/aspeed_lpc.c
> > +++ b/hw/misc/aspeed_lpc.c
> > @@ -12,20 +12,301 @@
> >  #include "qemu/error-report.h"
> >  #include "hw/misc/aspeed_lpc.h"
> >  #include "qapi/error.h"
> > +#include "qapi/visitor.h"
> > +#include "hw/irq.h"
> >  #include "hw/qdev-properties.h"
> >  #include "migration/vmstate.h"
> >  
> >  #define TO_REG(offset) ((offset) >> 2)
> >  
> >  #define HICR0                TO_REG(0x00)
> > +#define   HICR0_LPC3E        BIT(7)
> > +#define   HICR0_LPC2E        BIT(6)
> > +#define   HICR0_LPC1E        BIT(5)
> >  #define HICR1                TO_REG(0x04)
> >  #define HICR2                TO_REG(0x08)
> > +#define   HICR2_IBFIE3       BIT(3)
> > +#define   HICR2_IBFIE2       BIT(2)
> > +#define   HICR2_IBFIE1       BIT(1)
> >  #define HICR3                TO_REG(0x0C)
> >  #define HICR4                TO_REG(0x10)
> > +#define   HICR4_KCSENBL      BIT(2)
> > +#define IDR1                 TO_REG(0x24)
> > +#define IDR2                 TO_REG(0x28)
> > +#define IDR3                 TO_REG(0x2C)
> > +#define ODR1                 TO_REG(0x30)
> > +#define ODR2                 TO_REG(0x34)
> > +#define ODR3                 TO_REG(0x38)
> > +#define STR1                 TO_REG(0x3C)
> > +#define   STR_OBF            BIT(0)
> > +#define   STR_IBF            BIT(1)
> > +#define   STR_CMD_DATA       BIT(3)
> > +#define STR2                 TO_REG(0x40)
> > +#define STR3                 TO_REG(0x44)
> >  #define HICR5                TO_REG(0x80)
> >  #define HICR6                TO_REG(0x84)
> >  #define HICR7                TO_REG(0x88)
> >  #define HICR8                TO_REG(0x8C)
> > +#define HICRB                TO_REG(0x100)
> > +#define   HICRB_IBFIE4       BIT(1)
> > +#define   HICRB_LPC4E        BIT(0)
> > +#define IDR4                 TO_REG(0x114)
> > +#define ODR4                 TO_REG(0x118)
> > +#define STR4                 TO_REG(0x11C)
> > +
> > +enum aspeed_kcs_channel_id {
> > +    kcs_channel_1 = 0,
> > +    kcs_channel_2,
> > +    kcs_channel_3,
> > +    kcs_channel_4,
> > +};
> > +
> > +static const enum aspeed_lpc_subdevice aspeed_kcs_subdevice_map[] = {
> > +    [kcs_channel_1] = aspeed_lpc_kcs_1,
> > +    [kcs_channel_2] = aspeed_lpc_kcs_2,
> > +    [kcs_channel_3] = aspeed_lpc_kcs_3,
> > +    [kcs_channel_4] = aspeed_lpc_kcs_4,
> > +};
> > +
> > +struct aspeed_kcs_channel {
> > +    enum aspeed_kcs_channel_id id;
> > +
> > +    int idr;
> > +    int odr;
> > +    int str;
> > +};
> > +
> > +static const struct aspeed_kcs_channel aspeed_kcs_channel_map[] = {
> > +    [kcs_channel_1] = {
> > +        .id = kcs_channel_1,
> > +        .idr = IDR1,
> > +        .odr = ODR1,
> > +        .str = STR1
> > +    },
> > +
> > +    [kcs_channel_2] = {
> > +        .id = kcs_channel_2,
> > +        .idr = IDR2,
> > +        .odr = ODR2,
> > +        .str = STR2
> > +    },
> > +
> > +    [kcs_channel_3] = {
> > +        .id = kcs_channel_3,
> > +        .idr = IDR3,
> > +        .odr = ODR3,
> > +        .str = STR3
> > +    },
> > +
> > +    [kcs_channel_4] = {
> > +        .id = kcs_channel_4,
> > +        .idr = IDR4,
> > +        .odr = ODR4,
> > +        .str = STR4
> > +    },
> > +};
> > +
> > +struct aspeed_kcs_register_data {
> > +    const char *name;
> > +    int reg;
> > +    const struct aspeed_kcs_channel *chan;
> > +};
> > +
> > +static const struct aspeed_kcs_register_data aspeed_kcs_registers[] = {
> > +    {
> > +        .name = "idr1",
> > +        .reg = IDR1,
> > +        .chan = &aspeed_kcs_channel_map[kcs_channel_1],
> > +    },
> > +    {
> > +        .name = "odr1",
> > +        .reg = ODR1,
> > +        .chan = &aspeed_kcs_channel_map[kcs_channel_1],
> > +    },
> > +    {
> > +        .name = "str1",
> > +        .reg = STR1,
> > +        .chan = &aspeed_kcs_channel_map[kcs_channel_1],
> > +    },
> > +    {
> > +        .name = "idr2",
> > +        .reg = IDR2,
> > +        .chan = &aspeed_kcs_channel_map[kcs_channel_2],
> > +    },
> > +    {
> > +        .name = "odr2",
> > +        .reg = ODR2,
> > +        .chan = &aspeed_kcs_channel_map[kcs_channel_2],
> > +    },
> > +    {
> > +        .name = "str2",
> > +        .reg = STR2,
> > +        .chan = &aspeed_kcs_channel_map[kcs_channel_2],
> > +    },
> > +    {
> > +        .name = "idr3",
> > +        .reg = IDR3,
> > +        .chan = &aspeed_kcs_channel_map[kcs_channel_3],
> > +    },
> > +    {
> > +        .name = "odr3",
> > +        .reg = ODR3,
> > +        .chan = &aspeed_kcs_channel_map[kcs_channel_3],
> > +    },
> > +    {
> > +        .name = "str3",
> > +        .reg = STR3,
> > +        .chan = &aspeed_kcs_channel_map[kcs_channel_3],
> > +    },
> > +    {
> > +        .name = "idr4",
> > +        .reg = IDR4,
> > +        .chan = &aspeed_kcs_channel_map[kcs_channel_4],
> > +    },
> > +    {
> > +        .name = "odr4",
> > +        .reg = ODR4,
> > +        .chan = &aspeed_kcs_channel_map[kcs_channel_4],
> > +    },
> > +    {
> > +        .name = "str4",
> > +        .reg = STR4,
> > +        .chan = &aspeed_kcs_channel_map[kcs_channel_4],
> > +    },
> > +    { },
> > +};
> > +
> > +static const struct aspeed_kcs_register_data *
> > +aspeed_kcs_get_register_data_by_name(const char *name)
> > +{
> > +    const struct aspeed_kcs_register_data *pos = aspeed_kcs_registers;
> > +
> > +    while (pos->name) {
> > +        if (!strcmp(pos->name, name)) {
> > +            return pos;
> > +        }
> > +        pos++;
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> > +static const struct aspeed_kcs_channel *
> > +aspeed_kcs_get_channel_by_register(int reg)
> > +{
> > +    const struct aspeed_kcs_register_data *pos = aspeed_kcs_registers;
> > +
> > +    while (pos->name) {
> > +        if (pos->reg == reg) {
> > +            return pos->chan;
> > +        }
> > +        pos++;
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> > +static void aspeed_kcs_get_register_property(Object *obj,
> > +                                             Visitor *v,
> > +                                             const char *name,
> > +                                             void *opaque,
> > +                                             Error **errp)
> > +{
> > +    const struct aspeed_kcs_register_data *data;
> > +    AspeedLPCState *s = ASPEED_LPC(obj);
> > +    uint32_t val;
> > +
> > +    data = aspeed_kcs_get_register_data_by_name(name);
> > +    if (!data) {
> > +        return;
> > +    }
> > +
> > +    if (!strncmp("odr", name, 3)) {
> > +        s->regs[data->chan->str] &= ~STR_OBF;
> > +    }
> > +
> > +    val = s->regs[data->reg];
> > +
> > +    visit_type_uint32(v, name, &val, errp);
> > +}
> > +
> > +static bool aspeed_kcs_channel_enabled(AspeedLPCState *s,
> > +                                       const struct aspeed_kcs_channel *channel)
> > +{
> > +    switch (channel->id) {
> > +    case kcs_channel_1: return s->regs[HICR0] & HICR0_LPC1E;
> > +    case kcs_channel_2: return s->regs[HICR0] & HICR0_LPC2E;
> > +    case kcs_channel_3:
> > +        return (s->regs[HICR0] & HICR0_LPC3E) &&
> > +                    (s->regs[HICR4] & HICR4_KCSENBL);
> > +    case kcs_channel_4: return s->regs[HICRB] & HICRB_LPC4E;
> > +    default: return false;
> > +    }
> > +}
> > +
> > +static bool
> > +aspeed_kcs_channel_ibf_irq_enabled(AspeedLPCState *s,
> > +                                   const struct aspeed_kcs_channel *channel)
> > +{
> > +    if (!aspeed_kcs_channel_enabled(s, channel)) {
> > +            return false;
> > +    }
> > +
> > +    switch (channel->id) {
> > +    case kcs_channel_1: return s->regs[HICR2] & HICR2_IBFIE1;
> > +    case kcs_channel_2: return s->regs[HICR2] & HICR2_IBFIE2;
> > +    case kcs_channel_3: return s->regs[HICR2] & HICR2_IBFIE3;
> > +    case kcs_channel_4: return s->regs[HICRB] & HICRB_IBFIE4;
> > +    default: return false;
> > +    }
> > +}
> > +
> > +static void aspeed_kcs_set_register_property(Object *obj,
> > +                                             Visitor *v,
> > +                                             const char *name,
> > +                                             void *opaque,
> > +                                             Error **errp)
> > +{
> > +    const struct aspeed_kcs_register_data *data;
> > +    AspeedLPCState *s = ASPEED_LPC(obj);
> > +    uint32_t val;
> > +
> > +    data = aspeed_kcs_get_register_data_by_name(name);
> > +    if (!data) {
> > +        return;
> > +    }
> > +
> > +    if (!visit_type_uint32(v, name, &val, errp)) {
> > +        return;
> > +    }
> > +
> > +    if (strncmp("str", name, 3)) {
> > +        s->regs[data->reg] = val;
> > +    }
> > +
> > +    if (!strncmp("idr", name, 3)) {
> > +        s->regs[data->chan->str] |= STR_IBF;
> > +        if (aspeed_kcs_channel_ibf_irq_enabled(s, data->chan)) {
> > +            enum aspeed_lpc_subdevice subdev;
> > +
> > +            subdev = aspeed_kcs_subdevice_map[data->chan->id];
> > +            qemu_irq_raise(s->subdevice_irqs[subdev]);
> > +        }
> > +    }
> > +}
> > +
> > +static void aspeed_lpc_set_irq(void *opaque, int irq, int level)
> > +{
> > +    AspeedLPCState *s = (AspeedLPCState *)opaque;
> > +
> > +    if (level) {
> > +        s->subdevice_irqs_pending |= BIT(irq);
> > +    } else {
> > +        s->subdevice_irqs_pending &= ~BIT(irq);
> > +    }
> > +
> > +    qemu_set_irq(s->irq, !!s->subdevice_irqs_pending);
> > +}
> 
> 
> Nice !

Yeah, I figured this was kind of neat. All the alternatives I tried 
were worse :)

> I have adapted the iBT model ant it works fine.

I should have mentioned that I had pushed a patch for the BT model to 
my tree on github, just hadn't included it in the series I've sent to 
the list. Anyway, good to hear it worked for you too.

> 
>  
> >  
> >  static uint64_t aspeed_lpc_read(void *opaque, hwaddr offset, unsigned size)
> >  {
> > @@ -39,6 +320,29 @@ static uint64_t aspeed_lpc_read(void *opaque, hwaddr offset, unsigned size)
> >          return 0;
> >      }
> >  
> > +    switch (reg) {
> > +    case IDR1:
> > +    case IDR2:
> > +    case IDR3:
> > +    case IDR4:
> > +    {
> > +        const struct aspeed_kcs_channel *channel;
> > +
> > +        channel = aspeed_kcs_get_channel_by_register(reg);
> > +        if (s->regs[channel->str] & STR_IBF) {
> > +            enum aspeed_lpc_subdevice subdev;
> > +
> > +            subdev = aspeed_kcs_subdevice_map[channel->id];
> > +            qemu_irq_lower(s->subdevice_irqs[subdev]);
> > +        }
> > +
> > +        s->regs[channel->str] &= ~STR_IBF;
> > +        break;
> > +    }
> > +    default:
> > +        break;
> > +    }
> > +
> >      return s->regs[reg];
> >  }
> >  
> > @@ -55,6 +359,18 @@ static void aspeed_lpc_write(void *opaque, hwaddr offset, uint64_t data,
> >          return;
> >      }
> >  
> > +
> > +    switch (reg) {
> > +    case ODR1:
> > +    case ODR2:
> > +    case ODR3:
> > +    case ODR4:
> > +        s->regs[aspeed_kcs_get_channel_by_register(reg)->str] |= STR_OBF;
> > +        break;
> > +    default:
> > +        break;
> > +    }
> > +
> >      s->regs[reg] = data;
> >  }
> >  
> > @@ -72,6 +388,8 @@ static void aspeed_lpc_reset(DeviceState *dev)
> >  {
> >      struct AspeedLPCState *s = ASPEED_LPC(dev);
> >  
> > +    s->subdevice_irqs_pending = 0;
> > +
> >      memset(s->regs, 0, sizeof(s->regs));
> >  
> >      s->regs[HICR7] = s->hicr7;
> > @@ -83,11 +401,46 @@ static void aspeed_lpc_realize(DeviceState *dev, Error **errp)
> >      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> >  
> >      sysbus_init_irq(sbd, &s->irq);
> > +    sysbus_init_irq(sbd, &s->subdevice_irqs[aspeed_lpc_kcs_1]);
> > +    sysbus_init_irq(sbd, &s->subdevice_irqs[aspeed_lpc_kcs_2]);
> > +    sysbus_init_irq(sbd, &s->subdevice_irqs[aspeed_lpc_kcs_3]);
> > +    sysbus_init_irq(sbd, &s->subdevice_irqs[aspeed_lpc_kcs_4]);
> > +    sysbus_init_irq(sbd, &s->subdevice_irqs[aspeed_lpc_ibt]);
> >  
> >      memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_lpc_ops, s,
> >              TYPE_ASPEED_LPC, 0x1000);
> >  
> >      sysbus_init_mmio(sbd, &s->iomem);
> > +
> > +    qdev_init_gpio_in(dev, aspeed_lpc_set_irq, ASPEED_LPC_NR_SUBDEVS);
> > +}
> > +
> > +static void aspeed_lpc_init(Object *obj)
> > +{
> > +    object_property_add(obj, "idr1", "uint32", aspeed_kcs_get_register_property,
> > +                        aspeed_kcs_set_register_property, NULL, NULL);
> > +    object_property_add(obj, "odr1", "uint32", aspeed_kcs_get_register_property,
> > +                        aspeed_kcs_set_register_property, NULL, NULL);
> > +    object_property_add(obj, "str1", "uint32", aspeed_kcs_get_register_property,
> > +                        aspeed_kcs_set_register_property, NULL, NULL);
> > +    object_property_add(obj, "idr2", "uint32", aspeed_kcs_get_register_property,
> > +                        aspeed_kcs_set_register_property, NULL, NULL);
> > +    object_property_add(obj, "odr2", "uint32", aspeed_kcs_get_register_property,
> > +                        aspeed_kcs_set_register_property, NULL, NULL);
> > +    object_property_add(obj, "str2", "uint32", aspeed_kcs_get_register_property,
> > +                        aspeed_kcs_set_register_property, NULL, NULL);
> > +    object_property_add(obj, "idr3", "uint32", aspeed_kcs_get_register_property,
> > +                        aspeed_kcs_set_register_property, NULL, NULL);
> > +    object_property_add(obj, "odr3", "uint32", aspeed_kcs_get_register_property,
> > +                        aspeed_kcs_set_register_property, NULL, NULL);
> > +    object_property_add(obj, "str3", "uint32", aspeed_kcs_get_register_property,
> > +                        aspeed_kcs_set_register_property, NULL, NULL);
> > +    object_property_add(obj, "idr4", "uint32", aspeed_kcs_get_register_property,
> > +                        aspeed_kcs_set_register_property, NULL, NULL);
> > +    object_property_add(obj, "odr4", "uint32", aspeed_kcs_get_register_property,
> > +                        aspeed_kcs_set_register_property, NULL, NULL);
> > +    object_property_add(obj, "str4", "uint32", aspeed_kcs_get_register_property,
> > +                        aspeed_kcs_set_register_property, NULL, NULL);
> >  }
> >  
> >  static const VMStateDescription vmstate_aspeed_lpc = {
> > @@ -121,6 +474,7 @@ static const TypeInfo aspeed_lpc_info = {
> >      .parent = TYPE_SYS_BUS_DEVICE,
> >      .instance_size = sizeof(AspeedLPCState),
> >      .class_init = aspeed_lpc_class_init,
> > +    .instance_init = aspeed_lpc_init,
> >  };
> >  
> >  static void aspeed_lpc_register_types(void)
> > diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> > index 42c64bd28ba2..9359d6da336d 100644
> > --- a/include/hw/arm/aspeed_soc.h
> > +++ b/include/hw/arm/aspeed_soc.h
> > @@ -132,6 +132,7 @@ enum {
> >      ASPEED_DEV_SDRAM,
> >      ASPEED_DEV_XDMA,
> >      ASPEED_DEV_EMMC,
> > +    ASPEED_DEV_KCS,
> >  };
> >  
> >  #endif /* ASPEED_SOC_H */
> > diff --git a/include/hw/misc/aspeed_lpc.h b/include/hw/misc/aspeed_lpc.h
> > index 0fbb7f68bed2..df418cfcd36c 100644
> > --- a/include/hw/misc/aspeed_lpc.h
> > +++ b/include/hw/misc/aspeed_lpc.h
> > @@ -12,10 +12,22 @@
> >  
> >  #include "hw/sysbus.h"
> >  
> > +#include <stdint.h>
> > +
> >  #define TYPE_ASPEED_LPC "aspeed.lpc"
> >  #define ASPEED_LPC(obj) OBJECT_CHECK(AspeedLPCState, (obj), TYPE_ASPEED_LPC)
> >  
> > -#define ASPEED_LPC_NR_REGS (0x260 >> 2)
> > +#define ASPEED_LPC_NR_REGS      (0x260 >> 2)
> > +
> > +enum aspeed_lpc_subdevice {
> > +    aspeed_lpc_kcs_1 = 0,
> > +    aspeed_lpc_kcs_2,
> > +    aspeed_lpc_kcs_3,
> > +    aspeed_lpc_kcs_4,
> > +    aspeed_lpc_ibt,
> > +};
> > +
> > +#define ASPEED_LPC_NR_SUBDEVS   5
> >  
> >  typedef struct AspeedLPCState {
> >      /* <private> */
> > @@ -25,6 +37,9 @@ typedef struct AspeedLPCState {
> >      MemoryRegion iomem;
> >      qemu_irq irq;
> >  
> > +    qemu_irq subdevice_irqs[ASPEED_LPC_NR_SUBDEVS];
> > +    uint32_t subdevice_irqs_pending;
> 
> 
> This field should be added to the vmstate.

Ah yes, good catch. Let me do a v2 to fix this.

Andrew