[Qemu-devel] [PATCH] hw/sd/aspeed_sdhci: New device

Eddie James posted 1 patch 4 years, 9 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
hw/arm/aspeed.c              |   1 -
hw/arm/aspeed_soc.c          |  67 ++++++++++++++++++
hw/sd/Makefile.objs          |   1 +
hw/sd/aspeed_sdhci.c         | 163 +++++++++++++++++++++++++++++++++++++++++++
hw/sd/sdhci.c                |   8 ++-
include/hw/arm/aspeed_soc.h  |   3 +
include/hw/sd/aspeed_sdhci.h |  39 +++++++++++
include/hw/sd/sdhci.h        |   1 +
8 files changed, 281 insertions(+), 2 deletions(-)
create mode 100644 hw/sd/aspeed_sdhci.c
create mode 100644 include/hw/sd/aspeed_sdhci.h
[Qemu-devel] [PATCH] hw/sd/aspeed_sdhci: New device
Posted by Eddie James 4 years, 9 months ago
The Aspeed SOCs have two SD/MMC controllers. Add a device that
encapsulates both of these controllers and models the Aspeed-specific
registers and behavior.

Both controllers use a single HW interrupt. In order to trigger that
interrupt, a function pointer was added to the generic SDHCI structure.
This function (if the pointer is set) is called when the SDHCI model
changes it's interrupt status, allowing the user (the Aspeed SDHCI
model in this case) to set it's own interrupt.

This goes on top of Cedric's set of Aspeed changes.

Tested, booted, and read from /dev/mmcblk0 and /dev/mmcblk1:
./arm-softmmu/qemu-system-arm -M ast2500-evb -nographic \
 -drive file=flash-romulus,format=raw,if=mtd \
 -drive file=<some file>,format=raw,if=sd \
 -drive file=<some file>,format=raw,if=sd \
 -kernel zImage \
 -dtb aspeed-ast2500-evb.dtb \
 -initrd romulus.cpio.lzma \
 -d trace:sdhci* -no-reboot

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 hw/arm/aspeed.c              |   1 -
 hw/arm/aspeed_soc.c          |  67 ++++++++++++++++++
 hw/sd/Makefile.objs          |   1 +
 hw/sd/aspeed_sdhci.c         | 163 +++++++++++++++++++++++++++++++++++++++++++
 hw/sd/sdhci.c                |   8 ++-
 include/hw/arm/aspeed_soc.h  |   3 +
 include/hw/sd/aspeed_sdhci.h |  39 +++++++++++
 include/hw/sd/sdhci.h        |   1 +
 8 files changed, 281 insertions(+), 2 deletions(-)
 create mode 100644 hw/sd/aspeed_sdhci.c
 create mode 100644 include/hw/sd/aspeed_sdhci.h

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 5fee2f5..fbec946 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -422,7 +422,6 @@ static void aspeed_machine_class_init(ObjectClass *oc, void *data)
     mc->desc = board->desc;
     mc->init = aspeed_machine_init;
     mc->max_cpus = ASPEED_CPUS_NUM;
-    mc->no_sdcard = 1;
     mc->no_floppy = 1;
     mc->no_cdrom = 1;
     mc->no_parallel = 1;
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 1bbbdae..1378498 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -22,6 +22,7 @@
 #include "qemu/error-report.h"
 #include "hw/i2c/aspeed_i2c.h"
 #include "net/net.h"
+#include "sysemu/blockdev.h"
 
 #define ASPEED_SOC_IOMEM_SIZE       0x00200000
 
@@ -62,6 +63,7 @@ static const hwaddr aspeed_soc_ast2500_memmap[] = {
     [ASPEED_XDMA]   = 0x1E6E7000,
     [ASPEED_ADC]    = 0x1E6E9000,
     [ASPEED_SRAM]   = 0x1E720000,
+    [ASPEED_SDHCI]  = 0x1E740000,
     [ASPEED_GPIO]   = 0x1E780000,
     [ASPEED_RTC]    = 0x1E781000,
     [ASPEED_TIMER1] = 0x1E782000,
@@ -142,6 +144,7 @@ static const int aspeed_soc_ast2400_irqmap[] = {
     [ASPEED_ETH1]   = 2,
     [ASPEED_ETH2]   = 3,
     [ASPEED_XDMA]   = 6,
+    [ASPEED_SDHCI]  = 26,
 };
 
 #define aspeed_soc_ast2500_irqmap aspeed_soc_ast2400_irqmap
@@ -378,6 +381,16 @@ static void aspeed_soc_init(Object *obj)
         sysbus_init_child_obj(obj, "fsi[*]", OBJECT(&s->fsi[0]),
                               sizeof(s->fsi[0]), TYPE_ASPEED_FSI);
     }
+
+    sysbus_init_child_obj(obj, "sdhci", OBJECT(&s->sdhci), sizeof(s->sdhci),
+                          TYPE_ASPEED_SDHCI);
+
+    for (i = 0; i < ASPEED_SDHCI_NUM_SLOTS; ++i) {
+        sysbus_init_child_obj(obj, "sdhci_slot[*]",
+                              OBJECT(&s->sdhci.slots[i].sdhci),
+                              sizeof(s->sdhci.slots[i].sdhci),
+                              TYPE_SYSBUS_SDHCI);
+    }
 }
 
 static void aspeed_soc_realize(DeviceState *dev, Error **errp)
@@ -699,6 +712,60 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->fsi[0]), 0,
                            aspeed_soc_get_irq(s, ASPEED_FSI1));
     }
+
+    /* SD/SDIO */
+    for (i = 0; i < ASPEED_SDHCI_NUM_SLOTS; i++) {
+        hwaddr hci_addr = sc->info->memmap[ASPEED_SDHCI] + (0x100 * (i + 1));
+        DriveInfo *di;
+        BlockBackend *blk;
+        DeviceState *card;
+
+        /*
+         * Compatible with:
+         * - SD Host Controller Specification Version 2.0
+         * - SDIO Specification Version 2.0
+         * - MMC Specification Version 4.3
+         */
+        object_property_set_int(OBJECT(&s->sdhci.slots[i].sdhci), 2,
+                                "sd-spec-version", &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
+
+        object_property_set_uint(OBJECT(&s->sdhci.slots[i].sdhci),
+                                 ASPEED_SDHCI_CAPABILITIES, "capareg", &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
+
+        object_property_set_bool(OBJECT(&s->sdhci.slots[i].sdhci), true,
+                                 "realized", &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
+
+        sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdhci.slots[i].sdhci), 0, hci_addr);
+
+        di = drive_get_next(IF_SD);
+        blk = di ? blk_by_legacy_dinfo(di) : NULL;
+        card = qdev_create(qdev_get_child_bus(DEVICE(&s->sdhci.slots[i].sdhci),
+                                              "sd-bus"), TYPE_SD_CARD);
+        qdev_prop_set_drive(card, "drive", blk, &error_fatal);
+        object_property_set_bool(OBJECT(card), true, "realized", &error_fatal);
+    }
+
+    object_property_set_bool(OBJECT(&s->sdhci), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdhci), 0,
+                    sc->info->memmap[ASPEED_SDHCI]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci), 0,
+                       aspeed_soc_get_irq(s, ASPEED_SDHCI));
 }
 static Property aspeed_soc_properties[] = {
     DEFINE_PROP_UINT32("num-cpus", AspeedSoCState, num_cpus, 0),
diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
index 0665727..a884c23 100644
--- a/hw/sd/Makefile.objs
+++ b/hw/sd/Makefile.objs
@@ -8,3 +8,4 @@ obj-$(CONFIG_MILKYMIST) += milkymist-memcard.o
 obj-$(CONFIG_OMAP) += omap_mmc.o
 obj-$(CONFIG_PXA2XX) += pxa2xx_mmci.o
 obj-$(CONFIG_RASPI) += bcm2835_sdhost.o
+obj-$(CONFIG_ASPEED_SOC) += aspeed_sdhci.o
diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c
new file mode 100644
index 0000000..f684b12
--- /dev/null
+++ b/hw/sd/aspeed_sdhci.c
@@ -0,0 +1,163 @@
+/*
+ * Aspeed SD Host Controller
+ * Eddie James <eajames@linux.ibm.com>
+ *
+ * Copyright (C) 2019 IBM Corp
+ * SPDX-License-Identifer: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/error-report.h"
+#include "hw/sd/aspeed_sdhci.h"
+#include "qapi/error.h"
+
+#define ASPEED_SDHCI_INFO            0x00
+#define  ASPEED_SDHCI_INFO_RESET     0x00030000
+#define ASPEED_SDHCI_DEBOUNCE        0x04
+#define  ASPEED_SDHCI_DEBOUNCE_RESET 0x00000005
+#define ASPEED_SDHCI_BUS             0x08
+#define ASPEED_SDHCI_SDIO_140        0x10
+#define ASPEED_SDHCI_SDIO_148        0x18
+#define ASPEED_SDHCI_SDIO_240        0x20
+#define ASPEED_SDHCI_SDIO_248        0x28
+#define ASPEED_SDHCI_WP_POL          0xec
+#define ASPEED_SDHCI_CARD_DET        0xf0
+#define ASPEED_SDHCI_IRQ_STAT        0xfc
+
+#define TO_REG(addr) ((addr) / sizeof(uint32_t))
+
+static uint64_t aspeed_sdhci_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    uint32_t val = 0;
+    AspeedSDHCIState *sdhci = opaque;
+
+    switch (addr) {
+    case ASPEED_SDHCI_SDIO_140:
+        val = (uint32_t)sdhci->slots[0].sdhci.capareg;
+        break;
+    case ASPEED_SDHCI_SDIO_148:
+        val = (uint32_t)sdhci->slots[0].sdhci.maxcurr;
+        break;
+    case ASPEED_SDHCI_SDIO_240:
+        val = (uint32_t)sdhci->slots[1].sdhci.capareg;
+        break;
+    case ASPEED_SDHCI_SDIO_248:
+        val = (uint32_t)sdhci->slots[1].sdhci.maxcurr;
+        break;
+    default:
+        if (addr < ASPEED_SDHCI_REG_SIZE) {
+            val = sdhci->regs[TO_REG(addr)];
+        }
+    }
+
+    return (uint64_t)val;
+}
+
+static void aspeed_sdhci_write(void *opaque, hwaddr addr, uint64_t val,
+                               unsigned int size)
+{
+    AspeedSDHCIState *sdhci = opaque;
+
+    switch (addr) {
+    case ASPEED_SDHCI_SDIO_140:
+        sdhci->slots[0].sdhci.capareg = (uint64_t)(uint32_t)val;
+        break;
+    case ASPEED_SDHCI_SDIO_148:
+        sdhci->slots[0].sdhci.maxcurr = (uint64_t)(uint32_t)val;
+        break;
+    case ASPEED_SDHCI_SDIO_240:
+        sdhci->slots[1].sdhci.capareg = (uint64_t)(uint32_t)val;
+        break;
+    case ASPEED_SDHCI_SDIO_248:
+        sdhci->slots[1].sdhci.maxcurr = (uint64_t)(uint32_t)val;
+        break;
+    default:
+        if (addr < ASPEED_SDHCI_REG_SIZE) {
+            sdhci->regs[TO_REG(addr)] = (uint32_t)val;
+        }
+    }
+}
+
+static const MemoryRegionOps aspeed_sdhci_ops = {
+    .read = aspeed_sdhci_read,
+    .write = aspeed_sdhci_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid.min_access_size = 4,
+    .valid.max_access_size = 4,
+};
+
+static void aspeed_sdhci_irq_notify(SDHCIState *s, int level)
+{
+    AspeedSDHCISlotState *as = container_of(s, AspeedSDHCISlotState, sdhci);
+    AspeedSDHCIState *sdhci = container_of(as, AspeedSDHCIState,
+                                           slots[as->slot]);
+
+    if (level) {
+        sdhci->regs[TO_REG(ASPEED_SDHCI_IRQ_STAT)] |= BIT(as->slot);
+
+        qemu_irq_raise(sdhci->irq);
+    } else {
+        sdhci->regs[TO_REG(ASPEED_SDHCI_IRQ_STAT)] &= ~BIT(as->slot);
+
+        qemu_irq_lower(sdhci->irq);
+    }
+}
+
+static void aspeed_sdhci_realize(DeviceState *dev, Error **errp)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
+
+    sysbus_init_irq(sbd, &sdhci->irq);
+    memory_region_init_io(&sdhci->iomem, OBJECT(sdhci), &aspeed_sdhci_ops,
+                          sdhci, TYPE_ASPEED_SDHCI, ASPEED_SDHCI_REG_SIZE);
+    sysbus_init_mmio(sbd, &sdhci->iomem);
+
+    sdhci->slots[0].slot = 0;
+    sdhci->slots[0].sdhci.irq_notify = aspeed_sdhci_irq_notify;
+
+    sdhci->slots[1].slot = 1;
+    sdhci->slots[1].sdhci.irq_notify = aspeed_sdhci_irq_notify;
+}
+
+static void aspeed_sdhci_reset(DeviceState *dev)
+{
+    AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
+
+    memset(sdhci->regs, 0, ASPEED_SDHCI_REG_SIZE);
+    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] = ASPEED_SDHCI_INFO_RESET;
+    sdhci->regs[TO_REG(ASPEED_SDHCI_DEBOUNCE)] = ASPEED_SDHCI_DEBOUNCE_RESET;
+}
+
+static const VMStateDescription vmstate_aspeed_sdhci = {
+    .name = TYPE_ASPEED_SDHCI,
+    .version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(regs, AspeedSDHCIState, ASPEED_SDHCI_NUM_REGS),
+        VMSTATE_END_OF_LIST(),
+    },
+};
+
+static void aspeed_sdhci_class_init(ObjectClass *classp, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(classp);
+
+    dc->realize = aspeed_sdhci_realize;
+    dc->reset = aspeed_sdhci_reset;
+    dc->vmsd = &vmstate_aspeed_sdhci;
+}
+
+static TypeInfo aspeed_sdhci_info = {
+    .name          = TYPE_ASPEED_SDHCI,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AspeedSDHCIState),
+    .class_init    = aspeed_sdhci_class_init,
+};
+
+static void aspeed_sdhci_register_types(void)
+{
+    type_register_static(&aspeed_sdhci_info);
+}
+
+type_init(aspeed_sdhci_register_types)
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 7b80b1d..51a733b 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -213,7 +213,13 @@ static uint8_t sdhci_slotint(SDHCIState *s)
 
 static inline void sdhci_update_irq(SDHCIState *s)
 {
-    qemu_set_irq(s->irq, sdhci_slotint(s));
+    int level = sdhci_slotint(s);
+
+    qemu_set_irq(s->irq, level);
+
+    if (s->irq_notify) {
+        s->irq_notify(s, level);
+    }
 }
 
 static void sdhci_raise_insertion_irq(void *opaque)
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 7abb394..ba0f72f 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -29,6 +29,7 @@
 #include "hw/misc/aspeed_pwm.h"
 #include "hw/misc/aspeed_lpc.h"
 #include "hw/misc/aspeed_fsi.h"
+#include "hw/sd/aspeed_sdhci.h"
 
 #define ASPEED_SPIS_NUM  2
 #define ASPEED_WDTS_NUM  4
@@ -62,6 +63,7 @@ typedef struct AspeedSoCState {
     AspeedPWMState pwm;
     AspeedLPCState lpc;
     AspeedFsiState fsi[2];
+    AspeedSDHCIState sdhci;
 } AspeedSoCState;
 
 #define TYPE_ASPEED_SOC "aspeed-soc"
@@ -108,6 +110,7 @@ enum {
     ASPEED_SCU,
     ASPEED_ADC,
     ASPEED_SRAM,
+    ASPEED_SDHCI,
     ASPEED_GPIO,
     ASPEED_RTC,
     ASPEED_TIMER1,
diff --git a/include/hw/sd/aspeed_sdhci.h b/include/hw/sd/aspeed_sdhci.h
new file mode 100644
index 0000000..67fb7ef
--- /dev/null
+++ b/include/hw/sd/aspeed_sdhci.h
@@ -0,0 +1,39 @@
+/*
+ * Aspeed SD Host Controller
+ * Eddie James <eajames@linux.ibm.com>
+ *
+ * Copyright (C) 2019 IBM Corp
+ * SPDX-License-Identifer: GPL-2.0-or-later
+ */
+
+#ifndef ASPEED_SDHCI_H
+#define ASPEED_SDHCI_H
+
+#include "hw/sd/sdhci.h"
+
+#define TYPE_ASPEED_SDHCI "aspeed.sdhci"
+#define ASPEED_SDHCI(obj) OBJECT_CHECK(AspeedSDHCIState, (obj), \
+                                       TYPE_ASPEED_SDHCI)
+
+#define ASPEED_SDHCI_CAPABILITIES 0x01E80080
+#define ASPEED_SDHCI_NUM_SLOTS    2
+#define ASPEED_SDHCI_NUM_REGS     (ASPEED_SDHCI_REG_SIZE / sizeof(uint32_t))
+#define ASPEED_SDHCI_REG_SIZE     0x100
+
+typedef struct AspeedSDHCISlotState {
+    SDHCIState sdhci;
+    int slot;
+} AspeedSDHCISlotState;
+
+typedef struct AspeedSDHCIState {
+    SysBusDevice parent;
+
+    AspeedSDHCISlotState slots[ASPEED_SDHCI_NUM_SLOTS];
+
+    MemoryRegion iomem;
+    qemu_irq irq;
+
+    uint32_t regs[ASPEED_SDHCI_NUM_REGS];
+} AspeedSDHCIState;
+
+#endif /* ASPEED_SDHCI_H */
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index cbf415e..90dbf7b 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -48,6 +48,7 @@ typedef struct SDHCIState {
     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
     QEMUTimer *transfer_timer;
     qemu_irq irq;
+    void (*irq_notify)(struct SDHCIState *s, int level);
 
     /* Registers cleared on reset */
     uint32_t sdmasysad;    /* SDMA System Address register */
-- 
1.8.3.1


Re: [Qemu-devel] [PATCH] hw/sd/aspeed_sdhci: New device
Posted by Peter Maydell 4 years, 8 months ago
On Wed, 26 Jun 2019 at 19:43, Eddie James <eajames@linux.ibm.com> wrote:
>
> The Aspeed SOCs have two SD/MMC controllers. Add a device that
> encapsulates both of these controllers and models the Aspeed-specific
> registers and behavior.
>
> Both controllers use a single HW interrupt. In order to trigger that
> interrupt, a function pointer was added to the generic SDHCI structure.
> This function (if the pointer is set) is called when the SDHCI model
> changes it's interrupt status, allowing the user (the Aspeed SDHCI
> model in this case) to set it's own interrupt.
>
> This goes on top of Cedric's set of Aspeed changes.
>
> Tested, booted, and read from /dev/mmcblk0 and /dev/mmcblk1:
> ./arm-softmmu/qemu-system-arm -M ast2500-evb -nographic \
>  -drive file=flash-romulus,format=raw,if=mtd \
>  -drive file=<some file>,format=raw,if=sd \
>  -drive file=<some file>,format=raw,if=sd \
>  -kernel zImage \
>  -dtb aspeed-ast2500-evb.dtb \
>  -initrd romulus.cpio.lzma \
>  -d trace:sdhci* -no-reboot
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>

(Apologies for the rather delayed review -- combination of the
4.1 release work, and hoping one of the other Aspeed developers
would get to it.)


>  #define aspeed_soc_ast2500_irqmap aspeed_soc_ast2400_irqmap
> @@ -378,6 +381,16 @@ static void aspeed_soc_init(Object *obj)
>          sysbus_init_child_obj(obj, "fsi[*]", OBJECT(&s->fsi[0]),
>                                sizeof(s->fsi[0]), TYPE_ASPEED_FSI);
>      }
> +
> +    sysbus_init_child_obj(obj, "sdhci", OBJECT(&s->sdhci), sizeof(s->sdhci),
> +                          TYPE_ASPEED_SDHCI);
> +
> +    for (i = 0; i < ASPEED_SDHCI_NUM_SLOTS; ++i) {
> +        sysbus_init_child_obj(obj, "sdhci_slot[*]",
> +                              OBJECT(&s->sdhci.slots[i].sdhci),
> +                              sizeof(s->sdhci.slots[i].sdhci),
> +                              TYPE_SYSBUS_SDHCI);
> +    }

This seems a bit odd. Why does the SoC init the slots[i].sdhci
objects, rather than letting the TYPE_ASPEED_SDHCI object itself
take care of initializing and realizing its own child objects?

>  }
>
>  static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> @@ -699,6 +712,60 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->fsi[0]), 0,
>                             aspeed_soc_get_irq(s, ASPEED_FSI1));
>      }
> +
> +    /* SD/SDIO */
> +    for (i = 0; i < ASPEED_SDHCI_NUM_SLOTS; i++) {
> +        hwaddr hci_addr = sc->info->memmap[ASPEED_SDHCI] + (0x100 * (i + 1));
> +        DriveInfo *di;
> +        BlockBackend *blk;
> +        DeviceState *card;
> +
> +        /*
> +         * Compatible with:
> +         * - SD Host Controller Specification Version 2.0
> +         * - SDIO Specification Version 2.0
> +         * - MMC Specification Version 4.3
> +         */
> +        object_property_set_int(OBJECT(&s->sdhci.slots[i].sdhci), 2,
> +                                "sd-spec-version", &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +
> +        object_property_set_uint(OBJECT(&s->sdhci.slots[i].sdhci),
> +                                 ASPEED_SDHCI_CAPABILITIES, "capareg", &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +
> +        object_property_set_bool(OBJECT(&s->sdhci.slots[i].sdhci), true,
> +                                 "realized", &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdhci.slots[i].sdhci), 0, hci_addr);
> +
> +        di = drive_get_next(IF_SD);
> +        blk = di ? blk_by_legacy_dinfo(di) : NULL;
> +        card = qdev_create(qdev_get_child_bus(DEVICE(&s->sdhci.slots[i].sdhci),
> +                                              "sd-bus"), TYPE_SD_CARD);
> +        qdev_prop_set_drive(card, "drive", blk, &error_fatal);
> +        object_property_set_bool(OBJECT(card), true, "realized", &error_fatal);

Code at the SoC level shouldn't be calling drive_get_next() or
creating SD card objects directly itself -- it should leave this
up to either the board code or to the command line.

> +    }
> +
> +    object_property_set_bool(OBJECT(&s->sdhci), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdhci), 0,
> +                    sc->info->memmap[ASPEED_SDHCI]);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci), 0,
> +                       aspeed_soc_get_irq(s, ASPEED_SDHCI));
>  }
>  static Property aspeed_soc_properties[] = {
>      DEFINE_PROP_UINT32("num-cpus", AspeedSoCState, num_cpus, 0),
> diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
> index 0665727..a884c23 100644
> --- a/hw/sd/Makefile.objs
> +++ b/hw/sd/Makefile.objs
> @@ -8,3 +8,4 @@ obj-$(CONFIG_MILKYMIST) += milkymist-memcard.o
>  obj-$(CONFIG_OMAP) += omap_mmc.o
>  obj-$(CONFIG_PXA2XX) += pxa2xx_mmci.o
>  obj-$(CONFIG_RASPI) += bcm2835_sdhost.o
> +obj-$(CONFIG_ASPEED_SOC) += aspeed_sdhci.o
> diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c
> new file mode 100644
> index 0000000..f684b12
> --- /dev/null
> +++ b/hw/sd/aspeed_sdhci.c
> @@ -0,0 +1,163 @@
> +/*
> + * Aspeed SD Host Controller
> + * Eddie James <eajames@linux.ibm.com>
> + *
> + * Copyright (C) 2019 IBM Corp
> + * SPDX-License-Identifer: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/error-report.h"
> +#include "hw/sd/aspeed_sdhci.h"
> +#include "qapi/error.h"
> +
> +#define ASPEED_SDHCI_INFO            0x00
> +#define  ASPEED_SDHCI_INFO_RESET     0x00030000
> +#define ASPEED_SDHCI_DEBOUNCE        0x04
> +#define  ASPEED_SDHCI_DEBOUNCE_RESET 0x00000005
> +#define ASPEED_SDHCI_BUS             0x08
> +#define ASPEED_SDHCI_SDIO_140        0x10
> +#define ASPEED_SDHCI_SDIO_148        0x18
> +#define ASPEED_SDHCI_SDIO_240        0x20
> +#define ASPEED_SDHCI_SDIO_248        0x28
> +#define ASPEED_SDHCI_WP_POL          0xec
> +#define ASPEED_SDHCI_CARD_DET        0xf0
> +#define ASPEED_SDHCI_IRQ_STAT        0xfc
> +
> +#define TO_REG(addr) ((addr) / sizeof(uint32_t))
> +
> +static uint64_t aspeed_sdhci_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    uint32_t val = 0;
> +    AspeedSDHCIState *sdhci = opaque;
> +
> +    switch (addr) {
> +    case ASPEED_SDHCI_SDIO_140:
> +        val = (uint32_t)sdhci->slots[0].sdhci.capareg;
> +        break;
> +    case ASPEED_SDHCI_SDIO_148:
> +        val = (uint32_t)sdhci->slots[0].sdhci.maxcurr;
> +        break;
> +    case ASPEED_SDHCI_SDIO_240:
> +        val = (uint32_t)sdhci->slots[1].sdhci.capareg;
> +        break;
> +    case ASPEED_SDHCI_SDIO_248:
> +        val = (uint32_t)sdhci->slots[1].sdhci.maxcurr;
> +        break;
> +    default:
> +        if (addr < ASPEED_SDHCI_REG_SIZE) {
> +            val = sdhci->regs[TO_REG(addr)];
> +        }
> +    }
> +
> +    return (uint64_t)val;
> +}
> +
> +static void aspeed_sdhci_write(void *opaque, hwaddr addr, uint64_t val,
> +                               unsigned int size)
> +{
> +    AspeedSDHCIState *sdhci = opaque;
> +
> +    switch (addr) {
> +    case ASPEED_SDHCI_SDIO_140:
> +        sdhci->slots[0].sdhci.capareg = (uint64_t)(uint32_t)val;
> +        break;
> +    case ASPEED_SDHCI_SDIO_148:
> +        sdhci->slots[0].sdhci.maxcurr = (uint64_t)(uint32_t)val;
> +        break;
> +    case ASPEED_SDHCI_SDIO_240:
> +        sdhci->slots[1].sdhci.capareg = (uint64_t)(uint32_t)val;
> +        break;
> +    case ASPEED_SDHCI_SDIO_248:
> +        sdhci->slots[1].sdhci.maxcurr = (uint64_t)(uint32_t)val;
> +        break;
> +    default:
> +        if (addr < ASPEED_SDHCI_REG_SIZE) {
> +            sdhci->regs[TO_REG(addr)] = (uint32_t)val;
> +        }
> +    }
> +}
> +
> +static const MemoryRegionOps aspeed_sdhci_ops = {
> +    .read = aspeed_sdhci_read,
> +    .write = aspeed_sdhci_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid.min_access_size = 4,
> +    .valid.max_access_size = 4,
> +};
> +
> +static void aspeed_sdhci_irq_notify(SDHCIState *s, int level)
> +{
> +    AspeedSDHCISlotState *as = container_of(s, AspeedSDHCISlotState, sdhci);
> +    AspeedSDHCIState *sdhci = container_of(as, AspeedSDHCIState,
> +                                           slots[as->slot]);
> +
> +    if (level) {
> +        sdhci->regs[TO_REG(ASPEED_SDHCI_IRQ_STAT)] |= BIT(as->slot);
> +
> +        qemu_irq_raise(sdhci->irq);
> +    } else {
> +        sdhci->regs[TO_REG(ASPEED_SDHCI_IRQ_STAT)] &= ~BIT(as->slot);
> +
> +        qemu_irq_lower(sdhci->irq);
> +    }
> +}
> +
> +static void aspeed_sdhci_realize(DeviceState *dev, Error **errp)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
> +
> +    sysbus_init_irq(sbd, &sdhci->irq);
> +    memory_region_init_io(&sdhci->iomem, OBJECT(sdhci), &aspeed_sdhci_ops,
> +                          sdhci, TYPE_ASPEED_SDHCI, ASPEED_SDHCI_REG_SIZE);
> +    sysbus_init_mmio(sbd, &sdhci->iomem);
> +
> +    sdhci->slots[0].slot = 0;
> +    sdhci->slots[0].sdhci.irq_notify = aspeed_sdhci_irq_notify;
> +
> +    sdhci->slots[1].slot = 1;
> +    sdhci->slots[1].sdhci.irq_notify = aspeed_sdhci_irq_notify;
> +}
> +
> +static void aspeed_sdhci_reset(DeviceState *dev)
> +{
> +    AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
> +
> +    memset(sdhci->regs, 0, ASPEED_SDHCI_REG_SIZE);
> +    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] = ASPEED_SDHCI_INFO_RESET;
> +    sdhci->regs[TO_REG(ASPEED_SDHCI_DEBOUNCE)] = ASPEED_SDHCI_DEBOUNCE_RESET;
> +}
> +
> +static const VMStateDescription vmstate_aspeed_sdhci = {
> +    .name = TYPE_ASPEED_SDHCI,
> +    .version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(regs, AspeedSDHCIState, ASPEED_SDHCI_NUM_REGS),
> +        VMSTATE_END_OF_LIST(),
> +    },
> +};
> +
> +static void aspeed_sdhci_class_init(ObjectClass *classp, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(classp);
> +
> +    dc->realize = aspeed_sdhci_realize;
> +    dc->reset = aspeed_sdhci_reset;
> +    dc->vmsd = &vmstate_aspeed_sdhci;
> +}
> +
> +static TypeInfo aspeed_sdhci_info = {
> +    .name          = TYPE_ASPEED_SDHCI,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(AspeedSDHCIState),
> +    .class_init    = aspeed_sdhci_class_init,
> +};
> +
> +static void aspeed_sdhci_register_types(void)
> +{
> +    type_register_static(&aspeed_sdhci_info);
> +}
> +
> +type_init(aspeed_sdhci_register_types)
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 7b80b1d..51a733b 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -213,7 +213,13 @@ static uint8_t sdhci_slotint(SDHCIState *s)
>
>  static inline void sdhci_update_irq(SDHCIState *s)
>  {
> -    qemu_set_irq(s->irq, sdhci_slotint(s));
> +    int level = sdhci_slotint(s);
> +
> +    qemu_set_irq(s->irq, level);
> +
> +    if (s->irq_notify) {
> +        s->irq_notify(s, level);
> +    }

Rather than doing this, just wire the irq lines from
the SDHCIState device up to your AspeedSDHCIState device,
and then have that update the register state and assert
its own irq lines outbound.

>  }

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/sd/aspeed_sdhci: New device
Posted by Eddie James 4 years, 8 months ago
On 8/5/19 9:31 AM, Peter Maydell wrote:
> On Wed, 26 Jun 2019 at 19:43, Eddie James <eajames@linux.ibm.com> wrote:
>> The Aspeed SOCs have two SD/MMC controllers. Add a device that
>> encapsulates both of these controllers and models the Aspeed-specific
>> registers and behavior.
>>
>> Both controllers use a single HW interrupt. In order to trigger that
>> interrupt, a function pointer was added to the generic SDHCI structure.
>> This function (if the pointer is set) is called when the SDHCI model
>> changes it's interrupt status, allowing the user (the Aspeed SDHCI
>> model in this case) to set it's own interrupt.
>>
>> This goes on top of Cedric's set of Aspeed changes.
>>
>> Tested, booted, and read from /dev/mmcblk0 and /dev/mmcblk1:
>> ./arm-softmmu/qemu-system-arm -M ast2500-evb -nographic \
>>   -drive file=flash-romulus,format=raw,if=mtd \
>>   -drive file=<some file>,format=raw,if=sd \
>>   -drive file=<some file>,format=raw,if=sd \
>>   -kernel zImage \
>>   -dtb aspeed-ast2500-evb.dtb \
>>   -initrd romulus.cpio.lzma \
>>   -d trace:sdhci* -no-reboot
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> (Apologies for the rather delayed review -- combination of the
> 4.1 release work, and hoping one of the other Aspeed developers
> would get to it.)


No problem, thanks for the review!


>
>
>>   #define aspeed_soc_ast2500_irqmap aspeed_soc_ast2400_irqmap
>> @@ -378,6 +381,16 @@ static void aspeed_soc_init(Object *obj)
>>           sysbus_init_child_obj(obj, "fsi[*]", OBJECT(&s->fsi[0]),
>>                                 sizeof(s->fsi[0]), TYPE_ASPEED_FSI);
>>       }
>> +
>> +    sysbus_init_child_obj(obj, "sdhci", OBJECT(&s->sdhci), sizeof(s->sdhci),
>> +                          TYPE_ASPEED_SDHCI);
>> +
>> +    for (i = 0; i < ASPEED_SDHCI_NUM_SLOTS; ++i) {
>> +        sysbus_init_child_obj(obj, "sdhci_slot[*]",
>> +                              OBJECT(&s->sdhci.slots[i].sdhci),
>> +                              sizeof(s->sdhci.slots[i].sdhci),
>> +                              TYPE_SYSBUS_SDHCI);
>> +    }
> This seems a bit odd. Why does the SoC init the slots[i].sdhci
> objects, rather than letting the TYPE_ASPEED_SDHCI object itself
> take care of initializing and realizing its own child objects?


I couldn't get it to work with the actual sdhci objects underneath the 
Aspeed object. Qemu wouldn't start with an sd card drive in the command 
line. I'm thinking it can't find the actual sdhci objects if they're not 
on that parent bus?


>
>>   }
>>
>>   static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>> @@ -699,6 +712,60 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>>           sysbus_connect_irq(SYS_BUS_DEVICE(&s->fsi[0]), 0,
>>                              aspeed_soc_get_irq(s, ASPEED_FSI1));
>>       }
>> +
>> +    /* SD/SDIO */
>> +    for (i = 0; i < ASPEED_SDHCI_NUM_SLOTS; i++) {
>> +        hwaddr hci_addr = sc->info->memmap[ASPEED_SDHCI] + (0x100 * (i + 1));
>> +        DriveInfo *di;
>> +        BlockBackend *blk;
>> +        DeviceState *card;
>> +
>> +        /*
>> +         * Compatible with:
>> +         * - SD Host Controller Specification Version 2.0
>> +         * - SDIO Specification Version 2.0
>> +         * - MMC Specification Version 4.3
>> +         */
>> +        object_property_set_int(OBJECT(&s->sdhci.slots[i].sdhci), 2,
>> +                                "sd-spec-version", &err);
>> +        if (err) {
>> +            error_propagate(errp, err);
>> +            return;
>> +        }
>> +
>> +        object_property_set_uint(OBJECT(&s->sdhci.slots[i].sdhci),
>> +                                 ASPEED_SDHCI_CAPABILITIES, "capareg", &err);
>> +        if (err) {
>> +            error_propagate(errp, err);
>> +            return;
>> +        }
>> +
>> +        object_property_set_bool(OBJECT(&s->sdhci.slots[i].sdhci), true,
>> +                                 "realized", &err);
>> +        if (err) {
>> +            error_propagate(errp, err);
>> +            return;
>> +        }
>> +
>> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdhci.slots[i].sdhci), 0, hci_addr);
>> +
>> +        di = drive_get_next(IF_SD);
>> +        blk = di ? blk_by_legacy_dinfo(di) : NULL;
>> +        card = qdev_create(qdev_get_child_bus(DEVICE(&s->sdhci.slots[i].sdhci),
>> +                                              "sd-bus"), TYPE_SD_CARD);
>> +        qdev_prop_set_drive(card, "drive", blk, &error_fatal);
>> +        object_property_set_bool(OBJECT(card), true, "realized", &error_fatal);
> Code at the SoC level shouldn't be calling drive_get_next() or
> creating SD card objects directly itself -- it should leave this
> up to either the board code or to the command line.


OK.


>
>> +    }
>> +
>> +    object_property_set_bool(OBJECT(&s->sdhci), true, "realized", &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->sdhci), 0,
>> +                    sc->info->memmap[ASPEED_SDHCI]);
>> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci), 0,
>> +                       aspeed_soc_get_irq(s, ASPEED_SDHCI));
>>   }
>>   static Property aspeed_soc_properties[] = {
>>       DEFINE_PROP_UINT32("num-cpus", AspeedSoCState, num_cpus, 0),
>> diff --git a/hw/sd/Makefile.objs b/hw/sd/Makefile.objs
>> index 0665727..a884c23 100644
>> --- a/hw/sd/Makefile.objs
>> +++ b/hw/sd/Makefile.objs
>> @@ -8,3 +8,4 @@ obj-$(CONFIG_MILKYMIST) += milkymist-memcard.o
>>   obj-$(CONFIG_OMAP) += omap_mmc.o
>>   obj-$(CONFIG_PXA2XX) += pxa2xx_mmci.o
>>   obj-$(CONFIG_RASPI) += bcm2835_sdhost.o
>> +obj-$(CONFIG_ASPEED_SOC) += aspeed_sdhci.o
>> diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c
>> new file mode 100644
>> index 0000000..f684b12
>> --- /dev/null
>> +++ b/hw/sd/aspeed_sdhci.c
>> @@ -0,0 +1,163 @@
>> +/*
>> + * Aspeed SD Host Controller
>> + * Eddie James <eajames@linux.ibm.com>
>> + *
>> + * Copyright (C) 2019 IBM Corp
>> + * SPDX-License-Identifer: GPL-2.0-or-later
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "qemu/error-report.h"
>> +#include "hw/sd/aspeed_sdhci.h"
>> +#include "qapi/error.h"
>> +
>> +#define ASPEED_SDHCI_INFO            0x00
>> +#define  ASPEED_SDHCI_INFO_RESET     0x00030000
>> +#define ASPEED_SDHCI_DEBOUNCE        0x04
>> +#define  ASPEED_SDHCI_DEBOUNCE_RESET 0x00000005
>> +#define ASPEED_SDHCI_BUS             0x08
>> +#define ASPEED_SDHCI_SDIO_140        0x10
>> +#define ASPEED_SDHCI_SDIO_148        0x18
>> +#define ASPEED_SDHCI_SDIO_240        0x20
>> +#define ASPEED_SDHCI_SDIO_248        0x28
>> +#define ASPEED_SDHCI_WP_POL          0xec
>> +#define ASPEED_SDHCI_CARD_DET        0xf0
>> +#define ASPEED_SDHCI_IRQ_STAT        0xfc
>> +
>> +#define TO_REG(addr) ((addr) / sizeof(uint32_t))
>> +
>> +static uint64_t aspeed_sdhci_read(void *opaque, hwaddr addr, unsigned int size)
>> +{
>> +    uint32_t val = 0;
>> +    AspeedSDHCIState *sdhci = opaque;
>> +
>> +    switch (addr) {
>> +    case ASPEED_SDHCI_SDIO_140:
>> +        val = (uint32_t)sdhci->slots[0].sdhci.capareg;
>> +        break;
>> +    case ASPEED_SDHCI_SDIO_148:
>> +        val = (uint32_t)sdhci->slots[0].sdhci.maxcurr;
>> +        break;
>> +    case ASPEED_SDHCI_SDIO_240:
>> +        val = (uint32_t)sdhci->slots[1].sdhci.capareg;
>> +        break;
>> +    case ASPEED_SDHCI_SDIO_248:
>> +        val = (uint32_t)sdhci->slots[1].sdhci.maxcurr;
>> +        break;
>> +    default:
>> +        if (addr < ASPEED_SDHCI_REG_SIZE) {
>> +            val = sdhci->regs[TO_REG(addr)];
>> +        }
>> +    }
>> +
>> +    return (uint64_t)val;
>> +}
>> +
>> +static void aspeed_sdhci_write(void *opaque, hwaddr addr, uint64_t val,
>> +                               unsigned int size)
>> +{
>> +    AspeedSDHCIState *sdhci = opaque;
>> +
>> +    switch (addr) {
>> +    case ASPEED_SDHCI_SDIO_140:
>> +        sdhci->slots[0].sdhci.capareg = (uint64_t)(uint32_t)val;
>> +        break;
>> +    case ASPEED_SDHCI_SDIO_148:
>> +        sdhci->slots[0].sdhci.maxcurr = (uint64_t)(uint32_t)val;
>> +        break;
>> +    case ASPEED_SDHCI_SDIO_240:
>> +        sdhci->slots[1].sdhci.capareg = (uint64_t)(uint32_t)val;
>> +        break;
>> +    case ASPEED_SDHCI_SDIO_248:
>> +        sdhci->slots[1].sdhci.maxcurr = (uint64_t)(uint32_t)val;
>> +        break;
>> +    default:
>> +        if (addr < ASPEED_SDHCI_REG_SIZE) {
>> +            sdhci->regs[TO_REG(addr)] = (uint32_t)val;
>> +        }
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps aspeed_sdhci_ops = {
>> +    .read = aspeed_sdhci_read,
>> +    .write = aspeed_sdhci_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +    .valid.min_access_size = 4,
>> +    .valid.max_access_size = 4,
>> +};
>> +
>> +static void aspeed_sdhci_irq_notify(SDHCIState *s, int level)
>> +{
>> +    AspeedSDHCISlotState *as = container_of(s, AspeedSDHCISlotState, sdhci);
>> +    AspeedSDHCIState *sdhci = container_of(as, AspeedSDHCIState,
>> +                                           slots[as->slot]);
>> +
>> +    if (level) {
>> +        sdhci->regs[TO_REG(ASPEED_SDHCI_IRQ_STAT)] |= BIT(as->slot);
>> +
>> +        qemu_irq_raise(sdhci->irq);
>> +    } else {
>> +        sdhci->regs[TO_REG(ASPEED_SDHCI_IRQ_STAT)] &= ~BIT(as->slot);
>> +
>> +        qemu_irq_lower(sdhci->irq);
>> +    }
>> +}
>> +
>> +static void aspeed_sdhci_realize(DeviceState *dev, Error **errp)
>> +{
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> +    AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
>> +
>> +    sysbus_init_irq(sbd, &sdhci->irq);
>> +    memory_region_init_io(&sdhci->iomem, OBJECT(sdhci), &aspeed_sdhci_ops,
>> +                          sdhci, TYPE_ASPEED_SDHCI, ASPEED_SDHCI_REG_SIZE);
>> +    sysbus_init_mmio(sbd, &sdhci->iomem);
>> +
>> +    sdhci->slots[0].slot = 0;
>> +    sdhci->slots[0].sdhci.irq_notify = aspeed_sdhci_irq_notify;
>> +
>> +    sdhci->slots[1].slot = 1;
>> +    sdhci->slots[1].sdhci.irq_notify = aspeed_sdhci_irq_notify;
>> +}
>> +
>> +static void aspeed_sdhci_reset(DeviceState *dev)
>> +{
>> +    AspeedSDHCIState *sdhci = ASPEED_SDHCI(dev);
>> +
>> +    memset(sdhci->regs, 0, ASPEED_SDHCI_REG_SIZE);
>> +    sdhci->regs[TO_REG(ASPEED_SDHCI_INFO)] = ASPEED_SDHCI_INFO_RESET;
>> +    sdhci->regs[TO_REG(ASPEED_SDHCI_DEBOUNCE)] = ASPEED_SDHCI_DEBOUNCE_RESET;
>> +}
>> +
>> +static const VMStateDescription vmstate_aspeed_sdhci = {
>> +    .name = TYPE_ASPEED_SDHCI,
>> +    .version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32_ARRAY(regs, AspeedSDHCIState, ASPEED_SDHCI_NUM_REGS),
>> +        VMSTATE_END_OF_LIST(),
>> +    },
>> +};
>> +
>> +static void aspeed_sdhci_class_init(ObjectClass *classp, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(classp);
>> +
>> +    dc->realize = aspeed_sdhci_realize;
>> +    dc->reset = aspeed_sdhci_reset;
>> +    dc->vmsd = &vmstate_aspeed_sdhci;
>> +}
>> +
>> +static TypeInfo aspeed_sdhci_info = {
>> +    .name          = TYPE_ASPEED_SDHCI,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(AspeedSDHCIState),
>> +    .class_init    = aspeed_sdhci_class_init,
>> +};
>> +
>> +static void aspeed_sdhci_register_types(void)
>> +{
>> +    type_register_static(&aspeed_sdhci_info);
>> +}
>> +
>> +type_init(aspeed_sdhci_register_types)
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index 7b80b1d..51a733b 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -213,7 +213,13 @@ static uint8_t sdhci_slotint(SDHCIState *s)
>>
>>   static inline void sdhci_update_irq(SDHCIState *s)
>>   {
>> -    qemu_set_irq(s->irq, sdhci_slotint(s));
>> +    int level = sdhci_slotint(s);
>> +
>> +    qemu_set_irq(s->irq, level);
>> +
>> +    if (s->irq_notify) {
>> +        s->irq_notify(s, level);
>> +    }
> Rather than doing this, just wire the irq lines from
> the SDHCIState device up to your AspeedSDHCIState device,
> and then have that update the register state and assert
> its own irq lines outbound.


Sorry I don't follow you at all. Isn't that what I'm doing here? Surely 
I need to take action when sdhci_update_irq is called. I don't see any 
way other way to have my code called at this time.


Thanks,

Eddie


>
>>   }
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH] hw/sd/aspeed_sdhci: New device
Posted by Peter Maydell 4 years, 8 months ago
On Tue, 6 Aug 2019 at 23:12, Eddie James <eajames@linux.ibm.com> wrote:
>
>
> On 8/5/19 9:31 AM, Peter Maydell wrote:
> > On Wed, 26 Jun 2019 at 19:43, Eddie James <eajames@linux.ibm.com> wrote:

> >> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> >> index 7b80b1d..51a733b 100644
> >> --- a/hw/sd/sdhci.c
> >> +++ b/hw/sd/sdhci.c
> >> @@ -213,7 +213,13 @@ static uint8_t sdhci_slotint(SDHCIState *s)
> >>
> >>   static inline void sdhci_update_irq(SDHCIState *s)
> >>   {
> >> -    qemu_set_irq(s->irq, sdhci_slotint(s));
> >> +    int level = sdhci_slotint(s);
> >> +
> >> +    qemu_set_irq(s->irq, level);
> >> +
> >> +    if (s->irq_notify) {
> >> +        s->irq_notify(s, level);
> >> +    }
> > Rather than doing this, just wire the irq lines from
> > the SDHCIState device up to your AspeedSDHCIState device,
> > and then have that update the register state and assert
> > its own irq lines outbound.
>
>
> Sorry I don't follow you at all. Isn't that what I'm doing here? Surely
> I need to take action when sdhci_update_irq is called. I don't see any
> way other way to have my code called at this time.

The way to 'take action when sdhci_update_irq is called'
is to be the function on the other end of the qemu_set_irq()
call, ie be the device at the other end of the qemu_irq line.
You don't need to modify this code in sdhci.c at all.

thanks
-- PMM