[PATCH 0/5] aspeed: extensions and fixes

[PATCH 0/5] aspeed: extensions and fixes

[Cédric Le Goater]

Hi,

Here is a short series adding :

 - a new eMMC controller model for the AST2600 SoC (Andrew)
 - accessors to control the led state of the pca9552 device (Joel)
 - a 'execute-in-place' property to boot directly from CE0

Thanks,

C.

Andrew Jeffery (2):
  hw/sd: Configure number of slots exposed by the ASPEED SDHCI model
  hw/arm: ast2600: Wire up the eMMC controller

Cédric Le Goater (2):
  ftgmac100: check RX and TX buffer alignment
  hw/arm/aspeed: add a 'execute-in-place' property to boot directly from
    CE0

Joel Stanley (1):
  misc/pca9552: Add qom set and get

 include/hw/arm/aspeed.h      |  2 +
 include/hw/arm/aspeed_soc.h  |  2 +
 include/hw/sd/aspeed_sdhci.h |  1 +
 hw/arm/aspeed.c              | 71 ++++++++++++++++++++++------
 hw/arm/aspeed_ast2600.c      | 23 +++++++++
 hw/arm/aspeed_soc.c          |  2 +
 hw/misc/pca9552.c            | 90 ++++++++++++++++++++++++++++++++++++
 hw/net/ftgmac100.c           | 13 ++++++
 hw/sd/aspeed_sdhci.c         | 11 ++++-
 9 files changed, 198 insertions(+), 17 deletions(-)

-- 
2.21.1

[PATCH 1/5] hw/sd: Configure number of slots exposed by the ASPEED SDHCI model

[Cédric Le Goater]

From: Andrew Jeffery <andrew@aj.id.au>

The AST2600 includes a second cut-down version of the SD/MMC controller
found in the AST2500, named the eMMC controller. It's cut down in the
sense that it only supports one slot rather than two, but it brings the
total number of slots supported by the AST2600 to three.

The existing code assumed that the SD controller always provided two
slots. Rework the SDHCI object to expose the number of slots as a
property to be set by the SoC configuration.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/sd/aspeed_sdhci.h |  1 +
 hw/arm/aspeed.c              |  2 +-
 hw/arm/aspeed_ast2600.c      |  2 ++
 hw/arm/aspeed_soc.c          |  2 ++
 hw/sd/aspeed_sdhci.c         | 11 +++++++++--
 5 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/include/hw/sd/aspeed_sdhci.h b/include/hw/sd/aspeed_sdhci.h
index dfdab4379021..dffbb46946b9 100644
--- a/include/hw/sd/aspeed_sdhci.h
+++ b/include/hw/sd/aspeed_sdhci.h
@@ -24,6 +24,7 @@ typedef struct AspeedSDHCIState {
     SysBusDevice parent;
 
     SDHCIState slots[ASPEED_SDHCI_NUM_SLOTS];
+    uint8_t num_slots;
 
     MemoryRegion iomem;
     qemu_irq irq;
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index cc06af4fbb3e..4174e313cae5 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -263,7 +263,7 @@ static void aspeed_machine_init(MachineState *machine)
         amc->i2c_init(bmc);
     }
 
-    for (i = 0; i < ARRAY_SIZE(bmc->soc.sdhci.slots); i++) {
+    for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
         SDHCIState *sdhci = &bmc->soc.sdhci.slots[i];
         DriveInfo *dinfo = drive_get_next(IF_SD);
         BlockBackend *blk;
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 89e4b0095041..fb73c4043ea3 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -199,6 +199,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
     sysbus_init_child_obj(obj, "sdc", OBJECT(&s->sdhci), sizeof(s->sdhci),
                           TYPE_ASPEED_SDHCI);
 
+    object_property_set_int(OBJECT(&s->sdhci), 2, "num-slots", &error_abort);
+
     /* Init sd card slot class here so that they're under the correct parent */
     for (i = 0; i < ASPEED_SDHCI_NUM_SLOTS; ++i) {
         sysbus_init_child_obj(obj, "sdhci[*]", OBJECT(&s->sdhci.slots[i]),
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index a6237e594017..c15ceb683950 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -209,6 +209,8 @@ static void aspeed_soc_init(Object *obj)
     sysbus_init_child_obj(obj, "sdc", OBJECT(&s->sdhci), sizeof(s->sdhci),
                           TYPE_ASPEED_SDHCI);
 
+    object_property_set_int(OBJECT(&s->sdhci), 2, "num-slots", &error_abort);
+
     /* Init sd card slot class here so that they're under the correct parent */
     for (i = 0; i < ASPEED_SDHCI_NUM_SLOTS; ++i) {
         sysbus_init_child_obj(obj, "sdhci[*]", OBJECT(&s->sdhci.slots[i]),
diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c
index cff3eb7dd21e..939d1510dedb 100644
--- a/hw/sd/aspeed_sdhci.c
+++ b/hw/sd/aspeed_sdhci.c
@@ -13,6 +13,7 @@
 #include "qapi/error.h"
 #include "hw/irq.h"
 #include "migration/vmstate.h"
+#include "hw/qdev-properties.h"
 
 #define ASPEED_SDHCI_INFO            0x00
 #define  ASPEED_SDHCI_INFO_RESET     0x00030000
@@ -120,14 +121,14 @@ static void aspeed_sdhci_realize(DeviceState *dev, Error **errp)
 
     /* Create input irqs for the slots */
     qdev_init_gpio_in_named_with_opaque(DEVICE(sbd), aspeed_sdhci_set_irq,
-                                        sdhci, NULL, ASPEED_SDHCI_NUM_SLOTS);
+                                        sdhci, NULL, sdhci->num_slots);
 
     sysbus_init_irq(sbd, &sdhci->irq);
     memory_region_init_io(&sdhci->iomem, OBJECT(sdhci), &aspeed_sdhci_ops,
                           sdhci, TYPE_ASPEED_SDHCI, 0x1000);
     sysbus_init_mmio(sbd, &sdhci->iomem);
 
-    for (int i = 0; i < ASPEED_SDHCI_NUM_SLOTS; ++i) {
+    for (int i = 0; i < sdhci->num_slots; ++i) {
         Object *sdhci_slot = OBJECT(&sdhci->slots[i]);
         SysBusDevice *sbd_slot = SYS_BUS_DEVICE(&sdhci->slots[i]);
 
@@ -174,6 +175,11 @@ static const VMStateDescription vmstate_aspeed_sdhci = {
     },
 };
 
+static Property aspeed_sdhci_properties[] = {
+    DEFINE_PROP_UINT8("num-slots", AspeedSDHCIState, num_slots, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void aspeed_sdhci_class_init(ObjectClass *classp, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(classp);
@@ -181,6 +187,7 @@ static void aspeed_sdhci_class_init(ObjectClass *classp, void *data)
     dc->realize = aspeed_sdhci_realize;
     dc->reset = aspeed_sdhci_reset;
     dc->vmsd = &vmstate_aspeed_sdhci;
+    dc->props = aspeed_sdhci_properties;
 }
 
 static TypeInfo aspeed_sdhci_info = {
-- 
2.21.1

[PATCH 2/5] hw/arm: ast2600: Wire up the eMMC controller

[Cédric Le Goater]

From: Andrew Jeffery <andrew@aj.id.au>

Initialise another SDHCI model instance for the AST2600's eMMC
controller and use the SDHCI's num_slots value introduced previously to
determine whether we should create an SD card instance for the new slot.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/arm/aspeed_soc.h |  2 ++
 hw/arm/aspeed.c             | 25 ++++++++++++++++---------
 hw/arm/aspeed_ast2600.c     | 21 +++++++++++++++++++++
 3 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index e84380984f7b..90ac7f7ffa3b 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -57,6 +57,7 @@ typedef struct AspeedSoCState {
     AspeedGPIOState gpio;
     AspeedGPIOState gpio_1_8v;
     AspeedSDHCIState sdhci;
+    AspeedSDHCIState emmc;
 } AspeedSoCState;
 
 #define TYPE_ASPEED_SOC "aspeed-soc"
@@ -126,6 +127,7 @@ enum {
     ASPEED_MII4,
     ASPEED_SDRAM,
     ASPEED_XDMA,
+    ASPEED_EMMC,
 };
 
 #endif /* ASPEED_SOC_H */
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 4174e313cae5..0a7dfd29868b 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -171,6 +171,18 @@ static void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
     }
 }
 
+static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo)
+{
+        BlockBackend *blk;
+        DeviceState *card;
+
+        blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
+        card = qdev_create(qdev_get_child_bus(DEVICE(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);
+}
+
 static void aspeed_machine_init(MachineState *machine)
 {
     AspeedBoardState *bmc;
@@ -264,16 +276,11 @@ static void aspeed_machine_init(MachineState *machine)
     }
 
     for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
-        SDHCIState *sdhci = &bmc->soc.sdhci.slots[i];
-        DriveInfo *dinfo = drive_get_next(IF_SD);
-        BlockBackend *blk;
-        DeviceState *card;
+        sdhci_attach_drive(&bmc->soc.sdhci.slots[i], drive_get_next(IF_SD));
+    }
 
-        blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
-        card = qdev_create(qdev_get_child_bus(DEVICE(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);
+    if (bmc->soc.emmc.num_slots) {
+        sdhci_attach_drive(&bmc->soc.emmc.slots[0], drive_get_next(IF_SD));
     }
 
     arm_load_kernel(ARM_CPU(first_cpu), machine, &aspeed_board_binfo);
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index fb73c4043ea3..e59cdd291925 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -46,6 +46,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
     [ASPEED_ADC]       = 0x1E6E9000,
     [ASPEED_VIDEO]     = 0x1E700000,
     [ASPEED_SDHCI]     = 0x1E740000,
+    [ASPEED_EMMC]      = 0x1E750000,
     [ASPEED_GPIO]      = 0x1E780000,
     [ASPEED_GPIO_1_8V] = 0x1E780800,
     [ASPEED_RTC]       = 0x1E781000,
@@ -64,6 +65,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
 
 #define ASPEED_SOC_AST2600_MAX_IRQ 128
 
+/* Shared Peripheral Interrupt values below are offset by -32 from datasheet */
 static const int aspeed_soc_ast2600_irqmap[] = {
     [ASPEED_UART1]     = 47,
     [ASPEED_UART2]     = 48,
@@ -77,6 +79,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
     [ASPEED_ADC]       = 78,
     [ASPEED_XDMA]      = 6,
     [ASPEED_SDHCI]     = 43,
+    [ASPEED_EMMC]      = 15,
     [ASPEED_GPIO]      = 40,
     [ASPEED_GPIO_1_8V] = 11,
     [ASPEED_RTC]       = 13,
@@ -206,6 +209,14 @@ static void aspeed_soc_ast2600_init(Object *obj)
         sysbus_init_child_obj(obj, "sdhci[*]", OBJECT(&s->sdhci.slots[i]),
                               sizeof(s->sdhci.slots[i]), TYPE_SYSBUS_SDHCI);
     }
+
+    sysbus_init_child_obj(obj, "emmc", OBJECT(&s->emmc), sizeof(s->emmc),
+                          TYPE_ASPEED_SDHCI);
+
+    object_property_set_int(OBJECT(&s->emmc), 1, "num-slots", &error_abort);
+
+    sysbus_init_child_obj(obj, "emmc[*]", OBJECT(&s->emmc.slots[0]),
+            sizeof(s->emmc.slots[0]), TYPE_SYSBUS_SDHCI);
 }
 
 /*
@@ -497,6 +508,16 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
                     sc->memmap[ASPEED_SDHCI]);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci), 0,
                        aspeed_soc_get_irq(s, ASPEED_SDHCI));
+
+    /* eMMC */
+    object_property_set_bool(OBJECT(&s->emmc), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->emmc), 0, sc->memmap[ASPEED_EMMC]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->emmc), 0,
+                       aspeed_soc_get_irq(s, ASPEED_EMMC));
 }
 
 static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
-- 
2.21.1

[PATCH 3/5] ftgmac100: check RX and TX buffer alignment

[Cédric Le Goater]

These buffers should be aligned on 16 bytes.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/net/ftgmac100.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 86ac25894a89..051f7b7af2d6 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -198,6 +198,8 @@ typedef struct {
     uint32_t        des3;
 } FTGMAC100Desc;
 
+#define FTGMAC100_DESC_ALIGNMENT 16
+
 /*
  * Specific RTL8211E MII Registers
  */
@@ -722,6 +724,12 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
         s->itc = value;
         break;
     case FTGMAC100_RXR_BADR: /* Ring buffer address */
+        if (!QEMU_IS_ALIGNED(value, FTGMAC100_DESC_ALIGNMENT)) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad RX buffer alignment 0x%"
+                          HWADDR_PRIx "\n", __func__, value);
+            return;
+        }
+
         s->rx_ring = value;
         s->rx_descriptor = s->rx_ring;
         break;
@@ -731,6 +739,11 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
         break;
 
     case FTGMAC100_NPTXR_BADR: /* Transmit buffer address */
+        if (!QEMU_IS_ALIGNED(value, FTGMAC100_DESC_ALIGNMENT)) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad TX buffer alignment 0x%"
+                          HWADDR_PRIx "\n", __func__, value);
+            return;
+        }
         s->tx_ring = value;
         s->tx_descriptor = s->tx_ring;
         break;
-- 
2.21.1

[PATCH 4/5] hw/arm/aspeed: add a 'execute-in-place' property to boot directly from CE0

[Cédric Le Goater]

The overhead for the OpenBMC firmware images using the a custom U-Boot
is around 2 seconds, which is fine, but with a U-Boot from mainline,
it takes an extra 50 seconds or so to reach Linux. A quick survey on
the number of reads performed on the flash memory region gives the
following figures :

  OpenBMC U-Boot      922478 (~ 3.5 MBytes)
  Mainline U-Boot   20569977 (~ 80  MBytes)

QEMU must be trashing the TCG TBs and reloading text very often. Some
addresses are read more than 250.000 times. Until we find a solution
to improve boot time, execution from MMIO is not activated by default.

Setting this option also breaks migration compatibility.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/aspeed.h |  2 ++
 hw/arm/aspeed.c         | 44 ++++++++++++++++++++++++++++++++++++-----
 2 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
index 4423cd0cda71..18521484b90e 100644
--- a/include/hw/arm/aspeed.h
+++ b/include/hw/arm/aspeed.h
@@ -19,6 +19,8 @@ typedef struct AspeedBoardState AspeedBoardState;
 
 typedef struct AspeedMachine {
     MachineState parent_obj;
+
+    bool mmio_exec;
 } AspeedMachine;
 
 #define ASPEED_MACHINE_CLASS(klass) \
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 0a7dfd29868b..bf23579fa53d 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -260,11 +260,18 @@ static void aspeed_machine_init(MachineState *machine)
          * SoC and 128MB for the AST2500 SoC, which is twice as big as
          * needed by the flash modules of the Aspeed machines.
          */
-        memory_region_init_rom(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
-                               fl->size, &error_abort);
-        memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
-                                    boot_rom);
-        write_boot_rom(drive0, FIRMWARE_ADDR, fl->size, &error_abort);
+        if (ASPEED_MACHINE(machine)->mmio_exec) {
+            memory_region_init_alias(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
+                                     &fl->mmio, 0, fl->size);
+            memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
+                                        boot_rom);
+        } else {
+            memory_region_init_rom(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
+                                   fl->size, &error_abort);
+            memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
+                                        boot_rom);
+            write_boot_rom(drive0, FIRMWARE_ADDR, fl->size, &error_abort);
+        }
     }
 
     aspeed_board_binfo.ram_size = ram_size;
@@ -398,6 +405,30 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
     /* Bus 11: TODO ucd90160@64 */
 }
 
+static bool aspeed_get_mmio_exec(Object *obj, Error **errp)
+{
+    return ASPEED_MACHINE(obj)->mmio_exec;
+}
+
+static void aspeed_set_mmio_exec(Object *obj, bool value, Error **errp)
+{
+    ASPEED_MACHINE(obj)->mmio_exec = value;
+}
+
+static void aspeed_machine_instance_init(Object *obj)
+{
+    ASPEED_MACHINE(obj)->mmio_exec = false;
+}
+
+static void aspeed_machine_class_props_init(ObjectClass *oc)
+{
+    object_class_property_add_bool(oc, "execute-in-place",
+                                   aspeed_get_mmio_exec,
+                                   aspeed_set_mmio_exec, &error_abort);
+    object_class_property_set_description(oc, "execute-in-place",
+                           "boot directly from CE0 flash device", &error_abort);
+}
+
 static void aspeed_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -407,6 +438,8 @@ static void aspeed_machine_class_init(ObjectClass *oc, void *data)
     mc->no_floppy = 1;
     mc->no_cdrom = 1;
     mc->no_parallel = 1;
+
+    aspeed_machine_class_props_init(oc);
 }
 
 static void aspeed_machine_palmetto_class_init(ObjectClass *oc, void *data)
@@ -549,6 +582,7 @@ static const TypeInfo aspeed_machine_types[] = {
         .name          = TYPE_ASPEED_MACHINE,
         .parent        = TYPE_MACHINE,
         .instance_size = sizeof(AspeedMachine),
+        .instance_init = aspeed_machine_instance_init,
         .class_size    = sizeof(AspeedMachineClass),
         .class_init    = aspeed_machine_class_init,
         .abstract      = true,
-- 
2.21.1

[PATCH 5/5] misc/pca9552: Add qom set and get

[Cédric Le Goater]

From: Joel Stanley <joel@jms.id.au>

Following the pattern of the work recently done with the ASPEED GPIO
model, this adds support for inspecting and modifying the PCA9552 LEDs
from the monitor.

 (qemu) qom-set  /machine/unattached/device[17] led0 on
 (qemu) qom-set  /machine/unattached/device[17] led0 off
 (qemu) qom-set  /machine/unattached/device[17] led0 pwm0
 (qemu) qom-set  /machine/unattached/device[17] led0 pwm1

Signed-off-by: Joel Stanley <joel@jms.id.au>
[clg: - removed the "qom-get" examples from the commit log
      - merged memory leak fixes from Joel ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/misc/pca9552.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 73be28d9369c..efd961e04148 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -15,12 +15,16 @@
 #include "hw/misc/pca9552.h"
 #include "hw/misc/pca9552_regs.h"
 #include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
 
 #define PCA9552_LED_ON   0x0
 #define PCA9552_LED_OFF  0x1
 #define PCA9552_LED_PWM0 0x2
 #define PCA9552_LED_PWM1 0x3
 
+static const char *led_state[] = {"on", "off", "pwm0", "pwm1"};
+
 static uint8_t pca9552_pin_get_config(PCA9552State *s, int pin)
 {
     uint8_t reg   = PCA9552_LS0 + (pin / 4);
@@ -169,6 +173,82 @@ static int pca9552_event(I2CSlave *i2c, enum i2c_event event)
     return 0;
 }
 
+static void pca9552_get_led(Object *obj, Visitor *v, const char *name,
+                            void *opaque, Error **errp)
+{
+    PCA9552State *s = PCA9552(obj);
+    int led, rc, reg;
+    uint8_t state;
+
+    rc = sscanf(name, "led%2d", &led);
+    if (rc != 1) {
+        error_setg(errp, "%s: error reading %s", __func__, name);
+        return;
+    }
+    if (led < 0 || led > s->nr_leds) {
+        error_setg(errp, "%s invalid led %s", __func__, name);
+        return;
+    }
+    /*
+     * Get the LSx register as the qom interface should expose the device
+     * state, not the modeled 'input line' behaviour which would come from
+     * reading the INPUTx reg
+     */
+    reg = PCA9552_LS0 + led / 4;
+    state = (pca9552_read(s, reg) >> (led % 8)) & 0x3;
+    visit_type_str(v, name, (char **)&led_state[state], errp);
+}
+
+/*
+ * Return an LED selector register value based on an existing one, with
+ * the appropriate 2-bit state value set for the given LED number (0-3).
+ */
+static inline uint8_t pca955x_ledsel(uint8_t oldval, int led_num, int state)
+{
+        return (oldval & (~(0x3 << (led_num << 1)))) |
+                ((state & 0x3) << (led_num << 1));
+}
+
+static void pca9552_set_led(Object *obj, Visitor *v, const char *name,
+                            void *opaque, Error **errp)
+{
+    PCA9552State *s = PCA9552(obj);
+    Error *local_err = NULL;
+    int led, rc, reg, val;
+    uint8_t state;
+    char *state_str;
+
+    visit_type_str(v, name, &state_str, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    rc = sscanf(name, "led%2d", &led);
+    if (rc != 1) {
+        error_setg(errp, "%s: error reading %s", __func__, name);
+        return;
+    }
+    if (led < 0 || led > s->nr_leds) {
+        error_setg(errp, "%s invalid led %s", __func__, name);
+        return;
+    }
+
+    for (state = 0; state < ARRAY_SIZE(led_state); state++) {
+        if (!strcmp(state_str, led_state[state])) {
+            break;
+        }
+    }
+    if (state >= ARRAY_SIZE(led_state)) {
+        error_setg(errp, "%s invalid led state %s", __func__, state_str);
+        return;
+    }
+
+    reg = PCA9552_LS0 + led / 4;
+    val = pca9552_read(s, reg);
+    val = pca955x_ledsel(val, led % 4, state);
+    pca9552_write(s, reg, val);
+}
+
 static const VMStateDescription pca9552_vmstate = {
     .name = "PCA9552",
     .version_id = 0,
@@ -204,6 +284,7 @@ static void pca9552_reset(DeviceState *dev)
 static void pca9552_initfn(Object *obj)
 {
     PCA9552State *s = PCA9552(obj);
+    int led;
 
     /* If support for the other PCA955X devices are implemented, these
      * constant values might be part of class structure describing the
@@ -211,6 +292,15 @@ static void pca9552_initfn(Object *obj)
      */
     s->max_reg = PCA9552_LS3;
     s->nr_leds = 16;
+
+    for (led = 0; led < s->nr_leds; led++) {
+        char *name;
+
+        name = g_strdup_printf("led%d", led);
+        object_property_add(obj, name, "bool", pca9552_get_led, pca9552_set_led,
+                            NULL, NULL, NULL);
+        g_free(name);
+    }
 }
 
 static void pca9552_class_init(ObjectClass *klass, void *data)
-- 
2.21.1

Re: [PATCH 2/5] hw/arm: ast2600: Wire up the eMMC controller

[Philippe Mathieu-Daudé]

On 1/7/20 8:34 AM, Cédric Le Goater wrote:
> From: Andrew Jeffery <andrew@aj.id.au>
> 
> Initialise another SDHCI model instance for the AST2600's eMMC
> controller and use the SDHCI's num_slots value introduced previously to
> determine whether we should create an SD card instance for the new slot.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   include/hw/arm/aspeed_soc.h |  2 ++
>   hw/arm/aspeed.c             | 25 ++++++++++++++++---------
>   hw/arm/aspeed_ast2600.c     | 21 +++++++++++++++++++++
>   3 files changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index e84380984f7b..90ac7f7ffa3b 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -57,6 +57,7 @@ typedef struct AspeedSoCState {
>       AspeedGPIOState gpio;
>       AspeedGPIOState gpio_1_8v;
>       AspeedSDHCIState sdhci;
> +    AspeedSDHCIState emmc;
>   } AspeedSoCState;
>   
>   #define TYPE_ASPEED_SOC "aspeed-soc"
> @@ -126,6 +127,7 @@ enum {
>       ASPEED_MII4,
>       ASPEED_SDRAM,
>       ASPEED_XDMA,
> +    ASPEED_EMMC,
>   };
>   
>   #endif /* ASPEED_SOC_H */
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 4174e313cae5..0a7dfd29868b 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -171,6 +171,18 @@ static void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
>       }
>   }
>   
> +static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo)
> +{
> +        BlockBackend *blk;
> +        DeviceState *card;
> +
> +        blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
> +        card = qdev_create(qdev_get_child_bus(DEVICE(sdhci), "sd-bus"),
> +                           TYPE_SD_CARD);
> +        qdev_prop_set_drive(card, "drive", blk, &error_fatal);

I find this form easier to review:

            if (dinfo) {
                 qdev_prop_set_drive(card, "drive",
                                     blk_by_legacy_dinfo(dinfo),
                                     &error_fatal);
            }

> +        object_property_set_bool(OBJECT(card), true, "realized", &error_fatal);
> +}
> +
>   static void aspeed_machine_init(MachineState *machine)
>   {
>       AspeedBoardState *bmc;
> @@ -264,16 +276,11 @@ static void aspeed_machine_init(MachineState *machine)
>       }
>   
>       for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
> -        SDHCIState *sdhci = &bmc->soc.sdhci.slots[i];
> -        DriveInfo *dinfo = drive_get_next(IF_SD);
> -        BlockBackend *blk;
> -        DeviceState *card;
> +        sdhci_attach_drive(&bmc->soc.sdhci.slots[i], drive_get_next(IF_SD));
> +    }
>   
> -        blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
> -        card = qdev_create(qdev_get_child_bus(DEVICE(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);
> +    if (bmc->soc.emmc.num_slots) {
> +        sdhci_attach_drive(&bmc->soc.emmc.slots[0], drive_get_next(IF_SD));
>       }
>   
>       arm_load_kernel(ARM_CPU(first_cpu), machine, &aspeed_board_binfo);
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index fb73c4043ea3..e59cdd291925 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -46,6 +46,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
>       [ASPEED_ADC]       = 0x1E6E9000,
>       [ASPEED_VIDEO]     = 0x1E700000,
>       [ASPEED_SDHCI]     = 0x1E740000,
> +    [ASPEED_EMMC]      = 0x1E750000,
>       [ASPEED_GPIO]      = 0x1E780000,
>       [ASPEED_GPIO_1_8V] = 0x1E780800,
>       [ASPEED_RTC]       = 0x1E781000,
> @@ -64,6 +65,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
>   
>   #define ASPEED_SOC_AST2600_MAX_IRQ 128
>   
> +/* Shared Peripheral Interrupt values below are offset by -32 from datasheet */
>   static const int aspeed_soc_ast2600_irqmap[] = {
>       [ASPEED_UART1]     = 47,
>       [ASPEED_UART2]     = 48,
> @@ -77,6 +79,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
>       [ASPEED_ADC]       = 78,
>       [ASPEED_XDMA]      = 6,
>       [ASPEED_SDHCI]     = 43,
> +    [ASPEED_EMMC]      = 15,
>       [ASPEED_GPIO]      = 40,
>       [ASPEED_GPIO_1_8V] = 11,
>       [ASPEED_RTC]       = 13,
> @@ -206,6 +209,14 @@ static void aspeed_soc_ast2600_init(Object *obj)
>           sysbus_init_child_obj(obj, "sdhci[*]", OBJECT(&s->sdhci.slots[i]),
>                                 sizeof(s->sdhci.slots[i]), TYPE_SYSBUS_SDHCI);
>       }
> +
> +    sysbus_init_child_obj(obj, "emmc", OBJECT(&s->emmc), sizeof(s->emmc),
> +                          TYPE_ASPEED_SDHCI);
> +
> +    object_property_set_int(OBJECT(&s->emmc), 1, "num-slots", &error_abort);
> +
> +    sysbus_init_child_obj(obj, "emmc[*]", OBJECT(&s->emmc.slots[0]),

Single block, so use "emmc" instead.

Using "emmc":
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +            sizeof(s->emmc.slots[0]), TYPE_SYSBUS_SDHCI);
>   }
>   
>   /*
> @@ -497,6 +508,16 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>                       sc->memmap[ASPEED_SDHCI]);
>       sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci), 0,
>                          aspeed_soc_get_irq(s, ASPEED_SDHCI));
> +
> +    /* eMMC */
> +    object_property_set_bool(OBJECT(&s->emmc), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->emmc), 0, sc->memmap[ASPEED_EMMC]);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->emmc), 0,
> +                       aspeed_soc_get_irq(s, ASPEED_EMMC));
>   }
>   
>   static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
> 

Re: [PATCH 3/5] ftgmac100: check RX and TX buffer alignment

[Philippe Mathieu-Daudé]

On 1/7/20 8:34 AM, Cédric Le Goater wrote:
> These buffers should be aligned on 16 bytes.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   hw/net/ftgmac100.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 86ac25894a89..051f7b7af2d6 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -198,6 +198,8 @@ typedef struct {
>       uint32_t        des3;
>   } FTGMAC100Desc;
>   
> +#define FTGMAC100_DESC_ALIGNMENT 16
> +
>   /*
>    * Specific RTL8211E MII Registers
>    */
> @@ -722,6 +724,12 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
>           s->itc = value;
>           break;
>       case FTGMAC100_RXR_BADR: /* Ring buffer address */
> +        if (!QEMU_IS_ALIGNED(value, FTGMAC100_DESC_ALIGNMENT)) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad RX buffer alignment 0x%"
> +                          HWADDR_PRIx "\n", __func__, value);
> +            return;

What is the hardware behavior?

> +        }
> +
>           s->rx_ring = value;
>           s->rx_descriptor = s->rx_ring;
>           break;
> @@ -731,6 +739,11 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
>           break;
>   
>       case FTGMAC100_NPTXR_BADR: /* Transmit buffer address */
> +        if (!QEMU_IS_ALIGNED(value, FTGMAC100_DESC_ALIGNMENT)) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad TX buffer alignment 0x%"
> +                          HWADDR_PRIx "\n", __func__, value);
> +            return;
> +        }
>           s->tx_ring = value;
>           s->tx_descriptor = s->tx_ring;
>           break;
> 

Re: [PATCH 4/5] hw/arm/aspeed: add a 'execute-in-place' property to boot directly from CE0

[Philippe Mathieu-Daudé]

On 1/7/20 8:34 AM, Cédric Le Goater wrote:
> The overhead for the OpenBMC firmware images using the a custom U-Boot
> is around 2 seconds, which is fine, but with a U-Boot from mainline,
> it takes an extra 50 seconds or so to reach Linux. A quick survey on
> the number of reads performed on the flash memory region gives the
> following figures :
> 
>    OpenBMC U-Boot      922478 (~ 3.5 MBytes)
>    Mainline U-Boot   20569977 (~ 80  MBytes)
> 
> QEMU must be trashing the TCG TBs and reloading text very often. Some
> addresses are read more than 250.000 times. Until we find a solution
> to improve boot time, execution from MMIO is not activated by default.
> 
> Setting this option also breaks migration compatibility.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/arm/aspeed.h |  2 ++
>   hw/arm/aspeed.c         | 44 ++++++++++++++++++++++++++++++++++++-----
>   2 files changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
> index 4423cd0cda71..18521484b90e 100644
> --- a/include/hw/arm/aspeed.h
> +++ b/include/hw/arm/aspeed.h
> @@ -19,6 +19,8 @@ typedef struct AspeedBoardState AspeedBoardState;
>   
>   typedef struct AspeedMachine {
>       MachineState parent_obj;
> +
> +    bool mmio_exec;
>   } AspeedMachine;
>   
>   #define ASPEED_MACHINE_CLASS(klass) \
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 0a7dfd29868b..bf23579fa53d 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -260,11 +260,18 @@ static void aspeed_machine_init(MachineState *machine)
>            * SoC and 128MB for the AST2500 SoC, which is twice as big as
>            * needed by the flash modules of the Aspeed machines.
>            */
> -        memory_region_init_rom(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
> -                               fl->size, &error_abort);
> -        memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
> -                                    boot_rom);
> -        write_boot_rom(drive0, FIRMWARE_ADDR, fl->size, &error_abort);
> +        if (ASPEED_MACHINE(machine)->mmio_exec) {
> +            memory_region_init_alias(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
> +                                     &fl->mmio, 0, fl->size);
> +            memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
> +                                        boot_rom);
> +        } else {
> +            memory_region_init_rom(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
> +                                   fl->size, &error_abort);
> +            memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
> +                                        boot_rom);
> +            write_boot_rom(drive0, FIRMWARE_ADDR, fl->size, &error_abort);
> +        }

Nitpick: I'd use rom_add_file_mr() in write_boot_rom(), and keep the 
memory_region_add_subregion() call after the if/else statement.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>       }
>   
>       aspeed_board_binfo.ram_size = ram_size;
> @@ -398,6 +405,30 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
>       /* Bus 11: TODO ucd90160@64 */
>   }
>   
> +static bool aspeed_get_mmio_exec(Object *obj, Error **errp)
> +{
> +    return ASPEED_MACHINE(obj)->mmio_exec;
> +}
> +
> +static void aspeed_set_mmio_exec(Object *obj, bool value, Error **errp)
> +{
> +    ASPEED_MACHINE(obj)->mmio_exec = value;
> +}
> +
> +static void aspeed_machine_instance_init(Object *obj)
> +{
> +    ASPEED_MACHINE(obj)->mmio_exec = false;
> +}
> +
> +static void aspeed_machine_class_props_init(ObjectClass *oc)
> +{
> +    object_class_property_add_bool(oc, "execute-in-place",
> +                                   aspeed_get_mmio_exec,
> +                                   aspeed_set_mmio_exec, &error_abort);
> +    object_class_property_set_description(oc, "execute-in-place",
> +                           "boot directly from CE0 flash device", &error_abort);
> +}
> +
>   static void aspeed_machine_class_init(ObjectClass *oc, void *data)
>   {
>       MachineClass *mc = MACHINE_CLASS(oc);
> @@ -407,6 +438,8 @@ static void aspeed_machine_class_init(ObjectClass *oc, void *data)
>       mc->no_floppy = 1;
>       mc->no_cdrom = 1;
>       mc->no_parallel = 1;
> +
> +    aspeed_machine_class_props_init(oc);
>   }
>   
>   static void aspeed_machine_palmetto_class_init(ObjectClass *oc, void *data)
> @@ -549,6 +582,7 @@ static const TypeInfo aspeed_machine_types[] = {
>           .name          = TYPE_ASPEED_MACHINE,
>           .parent        = TYPE_MACHINE,
>           .instance_size = sizeof(AspeedMachine),
> +        .instance_init = aspeed_machine_instance_init,
>           .class_size    = sizeof(AspeedMachineClass),
>           .class_init    = aspeed_machine_class_init,
>           .abstract      = true,
> 

Re: [PATCH 3/5] ftgmac100: check RX and TX buffer alignment

[Cédric Le Goater]

On 1/7/20 9:27 AM, Philippe Mathieu-Daudé wrote:
> On 1/7/20 8:34 AM, Cédric Le Goater wrote:
>> These buffers should be aligned on 16 bytes.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   hw/net/ftgmac100.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
>> index 86ac25894a89..051f7b7af2d6 100644
>> --- a/hw/net/ftgmac100.c
>> +++ b/hw/net/ftgmac100.c
>> @@ -198,6 +198,8 @@ typedef struct {
>>       uint32_t        des3;
>>   } FTGMAC100Desc;
>>   +#define FTGMAC100_DESC_ALIGNMENT 16
>> +
>>   /*
>>    * Specific RTL8211E MII Registers
>>    */
>> @@ -722,6 +724,12 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
>>           s->itc = value;
>>           break;
>>       case FTGMAC100_RXR_BADR: /* Ring buffer address */
>> +        if (!QEMU_IS_ALIGNED(value, FTGMAC100_DESC_ALIGNMENT)) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad RX buffer alignment 0x%"
>> +                          HWADDR_PRIx "\n", __func__, value);
>> +            return;
> 
> What is the hardware behavior?

This is not documented :/

C.

> 
>> +        }
>> +
>>           s->rx_ring = value;
>>           s->rx_descriptor = s->rx_ring;
>>           break;
>> @@ -731,6 +739,11 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
>>           break;
>>         case FTGMAC100_NPTXR_BADR: /* Transmit buffer address */
>> +        if (!QEMU_IS_ALIGNED(value, FTGMAC100_DESC_ALIGNMENT)) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad TX buffer alignment 0x%"
>> +                          HWADDR_PRIx "\n", __func__, value);
>> +            return;
>> +        }
>>           s->tx_ring = value;
>>           s->tx_descriptor = s->tx_ring;
>>           break;
>>
> 

Re: [PATCH 2/5] hw/arm: ast2600: Wire up the eMMC controller

[Cédric Le Goater]

On 1/7/20 9:25 AM, Philippe Mathieu-Daudé wrote:
> On 1/7/20 8:34 AM, Cédric Le Goater wrote:
>> From: Andrew Jeffery <andrew@aj.id.au>
>>
>> Initialise another SDHCI model instance for the AST2600's eMMC
>> controller and use the SDHCI's num_slots value introduced previously to
>> determine whether we should create an SD card instance for the new slot.
>>
>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   include/hw/arm/aspeed_soc.h |  2 ++
>>   hw/arm/aspeed.c             | 25 ++++++++++++++++---------
>>   hw/arm/aspeed_ast2600.c     | 21 +++++++++++++++++++++
>>   3 files changed, 39 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
>> index e84380984f7b..90ac7f7ffa3b 100644
>> --- a/include/hw/arm/aspeed_soc.h
>> +++ b/include/hw/arm/aspeed_soc.h
>> @@ -57,6 +57,7 @@ typedef struct AspeedSoCState {
>>       AspeedGPIOState gpio;
>>       AspeedGPIOState gpio_1_8v;
>>       AspeedSDHCIState sdhci;
>> +    AspeedSDHCIState emmc;
>>   } AspeedSoCState;
>>     #define TYPE_ASPEED_SOC "aspeed-soc"
>> @@ -126,6 +127,7 @@ enum {
>>       ASPEED_MII4,
>>       ASPEED_SDRAM,
>>       ASPEED_XDMA,
>> +    ASPEED_EMMC,
>>   };
>>     #endif /* ASPEED_SOC_H */
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 4174e313cae5..0a7dfd29868b 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -171,6 +171,18 @@ static void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
>>       }
>>   }
>>   +static void sdhci_attach_drive(SDHCIState *sdhci, DriveInfo *dinfo)
>> +{
>> +        BlockBackend *blk;
>> +        DeviceState *card;
>> +
>> +        blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
>> +        card = qdev_create(qdev_get_child_bus(DEVICE(sdhci), "sd-bus"),
>> +                           TYPE_SD_CARD);
>> +        qdev_prop_set_drive(card, "drive", blk, &error_fatal);
> 
> I find this form easier to review:
> 
>            if (dinfo) {
>                 qdev_prop_set_drive(card, "drive",
>                                     blk_by_legacy_dinfo(dinfo),
>                                     &error_fatal);
>            }

yes.

>> +        object_property_set_bool(OBJECT(card), true, "realized", &error_fatal);
>> +}
>> +
>>   static void aspeed_machine_init(MachineState *machine)
>>   {
>>       AspeedBoardState *bmc;
>> @@ -264,16 +276,11 @@ static void aspeed_machine_init(MachineState *machine)
>>       }
>>         for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
>> -        SDHCIState *sdhci = &bmc->soc.sdhci.slots[i];
>> -        DriveInfo *dinfo = drive_get_next(IF_SD);
>> -        BlockBackend *blk;
>> -        DeviceState *card;
>> +        sdhci_attach_drive(&bmc->soc.sdhci.slots[i], drive_get_next(IF_SD));
>> +    }
>>   -        blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
>> -        card = qdev_create(qdev_get_child_bus(DEVICE(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);
>> +    if (bmc->soc.emmc.num_slots) {
>> +        sdhci_attach_drive(&bmc->soc.emmc.slots[0], drive_get_next(IF_SD));
>>       }
>>         arm_load_kernel(ARM_CPU(first_cpu), machine, &aspeed_board_binfo);
>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
>> index fb73c4043ea3..e59cdd291925 100644
>> --- a/hw/arm/aspeed_ast2600.c
>> +++ b/hw/arm/aspeed_ast2600.c
>> @@ -46,6 +46,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
>>       [ASPEED_ADC]       = 0x1E6E9000,
>>       [ASPEED_VIDEO]     = 0x1E700000,
>>       [ASPEED_SDHCI]     = 0x1E740000,
>> +    [ASPEED_EMMC]      = 0x1E750000,
>>       [ASPEED_GPIO]      = 0x1E780000,
>>       [ASPEED_GPIO_1_8V] = 0x1E780800,
>>       [ASPEED_RTC]       = 0x1E781000,
>> @@ -64,6 +65,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
>>     #define ASPEED_SOC_AST2600_MAX_IRQ 128
>>   +/* Shared Peripheral Interrupt values below are offset by -32 from datasheet */
>>   static const int aspeed_soc_ast2600_irqmap[] = {
>>       [ASPEED_UART1]     = 47,
>>       [ASPEED_UART2]     = 48,
>> @@ -77,6 +79,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
>>       [ASPEED_ADC]       = 78,
>>       [ASPEED_XDMA]      = 6,
>>       [ASPEED_SDHCI]     = 43,
>> +    [ASPEED_EMMC]      = 15,
>>       [ASPEED_GPIO]      = 40,
>>       [ASPEED_GPIO_1_8V] = 11,
>>       [ASPEED_RTC]       = 13,
>> @@ -206,6 +209,14 @@ static void aspeed_soc_ast2600_init(Object *obj)
>>           sysbus_init_child_obj(obj, "sdhci[*]", OBJECT(&s->sdhci.slots[i]),
>>                                 sizeof(s->sdhci.slots[i]), TYPE_SYSBUS_SDHCI);
>>       }
>> +
>> +    sysbus_init_child_obj(obj, "emmc", OBJECT(&s->emmc), sizeof(s->emmc),
>> +                          TYPE_ASPEED_SDHCI);
>> +
>> +    object_property_set_int(OBJECT(&s->emmc), 1, "num-slots", &error_abort);
>> +
>> +    sysbus_init_child_obj(obj, "emmc[*]", OBJECT(&s->emmc.slots[0]),
> 
> Single block, so use "emmc" instead.
> 
> Using "emmc":
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

I will do the fixes and resend.

Thanks,

C. 


> 
>> +            sizeof(s->emmc.slots[0]), TYPE_SYSBUS_SDHCI);
>>   }
>>     /*
>> @@ -497,6 +508,16 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>>                       sc->memmap[ASPEED_SDHCI]);
>>       sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci), 0,
>>                          aspeed_soc_get_irq(s, ASPEED_SDHCI));
>> +
>> +    /* eMMC */
>> +    object_property_set_bool(OBJECT(&s->emmc), true, "realized", &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->emmc), 0, sc->memmap[ASPEED_EMMC]);
>> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->emmc), 0,
>> +                       aspeed_soc_get_irq(s, ASPEED_EMMC));
>>   }
>>     static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
>>
> 

Re: [PATCH 4/5] hw/arm/aspeed: add a 'execute-in-place' property to boot directly from CE0

[Cédric Le Goater]

On 1/7/20 9:34 AM, Philippe Mathieu-Daudé wrote:
> On 1/7/20 8:34 AM, Cédric Le Goater wrote:
>> The overhead for the OpenBMC firmware images using the a custom U-Boot
>> is around 2 seconds, which is fine, but with a U-Boot from mainline,
>> it takes an extra 50 seconds or so to reach Linux. A quick survey on
>> the number of reads performed on the flash memory region gives the
>> following figures :
>>
>>    OpenBMC U-Boot      922478 (~ 3.5 MBytes)
>>    Mainline U-Boot   20569977 (~ 80  MBytes)
>>
>> QEMU must be trashing the TCG TBs and reloading text very often. Some
>> addresses are read more than 250.000 times. Until we find a solution
>> to improve boot time, execution from MMIO is not activated by default.
>>
>> Setting this option also breaks migration compatibility.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   include/hw/arm/aspeed.h |  2 ++
>>   hw/arm/aspeed.c         | 44 ++++++++++++++++++++++++++++++++++++-----
>>   2 files changed, 41 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
>> index 4423cd0cda71..18521484b90e 100644
>> --- a/include/hw/arm/aspeed.h
>> +++ b/include/hw/arm/aspeed.h
>> @@ -19,6 +19,8 @@ typedef struct AspeedBoardState AspeedBoardState;
>>     typedef struct AspeedMachine {
>>       MachineState parent_obj;
>> +
>> +    bool mmio_exec;
>>   } AspeedMachine;
>>     #define ASPEED_MACHINE_CLASS(klass) \
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 0a7dfd29868b..bf23579fa53d 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -260,11 +260,18 @@ static void aspeed_machine_init(MachineState *machine)
>>            * SoC and 128MB for the AST2500 SoC, which is twice as big as
>>            * needed by the flash modules of the Aspeed machines.
>>            */
>> -        memory_region_init_rom(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
>> -                               fl->size, &error_abort);
>> -        memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
>> -                                    boot_rom);
>> -        write_boot_rom(drive0, FIRMWARE_ADDR, fl->size, &error_abort);
>> +        if (ASPEED_MACHINE(machine)->mmio_exec) {
>> +            memory_region_init_alias(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
>> +                                     &fl->mmio, 0, fl->size);
>> +            memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
>> +                                        boot_rom);
>> +        } else {
>> +            memory_region_init_rom(boot_rom, OBJECT(bmc), "aspeed.boot_rom",
>> +                                   fl->size, &error_abort);
>> +            memory_region_add_subregion(get_system_memory(), FIRMWARE_ADDR,
>> +                                        boot_rom);
>> +            write_boot_rom(drive0, FIRMWARE_ADDR, fl->size, &error_abort);
>> +        }
> 
> Nitpick: I'd use rom_add_file_mr() in write_boot_rom(), and keep the memory_region_add_subregion() call after the if/else statement.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

I would rather address changes in follow-up  patches.

Thanks for the hint. 

C. 

> 
>>       }
>>         aspeed_board_binfo.ram_size = ram_size;
>> @@ -398,6 +405,30 @@ static void witherspoon_bmc_i2c_init(AspeedBoardState *bmc)
>>       /* Bus 11: TODO ucd90160@64 */
>>   }
>>   +static bool aspeed_get_mmio_exec(Object *obj, Error **errp)
>> +{
>> +    return ASPEED_MACHINE(obj)->mmio_exec;
>> +}
>> +
>> +static void aspeed_set_mmio_exec(Object *obj, bool value, Error **errp)
>> +{
>> +    ASPEED_MACHINE(obj)->mmio_exec = value;
>> +}
>> +
>> +static void aspeed_machine_instance_init(Object *obj)
>> +{
>> +    ASPEED_MACHINE(obj)->mmio_exec = false;
>> +}
>> +
>> +static void aspeed_machine_class_props_init(ObjectClass *oc)
>> +{
>> +    object_class_property_add_bool(oc, "execute-in-place",
>> +                                   aspeed_get_mmio_exec,
>> +                                   aspeed_set_mmio_exec, &error_abort);
>> +    object_class_property_set_description(oc, "execute-in-place",
>> +                           "boot directly from CE0 flash device", &error_abort);
>> +}
>> +
>>   static void aspeed_machine_class_init(ObjectClass *oc, void *data)
>>   {
>>       MachineClass *mc = MACHINE_CLASS(oc);
>> @@ -407,6 +438,8 @@ static void aspeed_machine_class_init(ObjectClass *oc, void *data)
>>       mc->no_floppy = 1;
>>       mc->no_cdrom = 1;
>>       mc->no_parallel = 1;
>> +
>> +    aspeed_machine_class_props_init(oc);
>>   }
>>     static void aspeed_machine_palmetto_class_init(ObjectClass *oc, void *data)
>> @@ -549,6 +582,7 @@ static const TypeInfo aspeed_machine_types[] = {
>>           .name          = TYPE_ASPEED_MACHINE,
>>           .parent        = TYPE_MACHINE,
>>           .instance_size = sizeof(AspeedMachine),
>> +        .instance_init = aspeed_machine_instance_init,
>>           .class_size    = sizeof(AspeedMachineClass),
>>           .class_init    = aspeed_machine_class_init,
>>           .abstract      = true,
>>
> 

Re: [PATCH 3/5] ftgmac100: check RX and TX buffer alignment

[Philippe Mathieu-Daudé]

On 1/7/20 9:36 AM, Cédric Le Goater wrote:
> On 1/7/20 9:27 AM, Philippe Mathieu-Daudé wrote:
>> On 1/7/20 8:34 AM, Cédric Le Goater wrote:
>>> These buffers should be aligned on 16 bytes.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>    hw/net/ftgmac100.c | 13 +++++++++++++
>>>    1 file changed, 13 insertions(+)
>>>
>>> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
>>> index 86ac25894a89..051f7b7af2d6 100644
>>> --- a/hw/net/ftgmac100.c
>>> +++ b/hw/net/ftgmac100.c
>>> @@ -198,6 +198,8 @@ typedef struct {
>>>        uint32_t        des3;
>>>    } FTGMAC100Desc;
>>>    +#define FTGMAC100_DESC_ALIGNMENT 16
>>> +
>>>    /*
>>>     * Specific RTL8211E MII Registers
>>>     */
>>> @@ -722,6 +724,12 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
>>>            s->itc = value;
>>>            break;
>>>        case FTGMAC100_RXR_BADR: /* Ring buffer address */
>>> +        if (!QEMU_IS_ALIGNED(value, FTGMAC100_DESC_ALIGNMENT)) {
>>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad RX buffer alignment 0x%"
>>> +                          HWADDR_PRIx "\n", __func__, value);
>>> +            return;
>>
>> What is the hardware behavior?
> 
> This is not documented :/

What happens if we don't return?

>>
>>> +        }
>>> +
>>>            s->rx_ring = value;
>>>            s->rx_descriptor = s->rx_ring;
>>>            break;
>>> @@ -731,6 +739,11 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
>>>            break;
>>>          case FTGMAC100_NPTXR_BADR: /* Transmit buffer address */
>>> +        if (!QEMU_IS_ALIGNED(value, FTGMAC100_DESC_ALIGNMENT)) {
>>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad TX buffer alignment 0x%"
>>> +                          HWADDR_PRIx "\n", __func__, value);
>>> +            return;
>>> +        }
>>>            s->tx_ring = value;
>>>            s->tx_descriptor = s->tx_ring;
>>>            break;
>>>
>>
> 

Re: [PATCH 2/5] hw/arm: ast2600: Wire up the eMMC controller

[Cédric Le Goater]

>> +
>> +    sysbus_init_child_obj(obj, "emmc", OBJECT(&s->emmc), sizeof(s->emmc),
>> +                          TYPE_ASPEED_SDHCI);
>> +
>> +    object_property_set_int(OBJECT(&s->emmc), 1, "num-slots", &error_abort);
>> +
>> +    sysbus_init_child_obj(obj, "emmc[*]", OBJECT(&s->emmc.slots[0]),
> 
> Single block, so use "emmc" instead.

Andrew, how should we call the objects in the slots ? "sdhci" ? 

Thanks,

C. 

Re: [PATCH 2/5] hw/arm: ast2600: Wire up the eMMC controller

[Andrew Jeffery]

On Fri, 10 Jan 2020, at 22:26, Cédric Le Goater wrote:
> >> +
> >> +    sysbus_init_child_obj(obj, "emmc", OBJECT(&s->emmc), sizeof(s->emmc),
> >> +                          TYPE_ASPEED_SDHCI);
> >> +
> >> +    object_property_set_int(OBJECT(&s->emmc), 1, "num-slots", &error_abort);
> >> +
> >> +    sysbus_init_child_obj(obj, "emmc[*]", OBJECT(&s->emmc.slots[0]),
> > 
> > Single block, so use "emmc" instead.
> 
> Andrew, how should we call the objects in the slots ? "sdhci" ? 

I think that's the right way to go, but maybe we need to rethink the naming at the
controller level.

Andrew

Re: [PATCH 0/5] aspeed: extensions and fixes

[Cédric Le Goater]

On 1/7/20 8:34 AM, Cédric Le Goater wrote:
> Hi,
> 
> Here is a short series adding :
> 
>  - a new eMMC controller model for the AST2600 SoC (Andrew)
>  - accessors to control the led state of the pca9552 device (Joel)
>  - a 'execute-in-place' property to boot directly from CE0

There is a naming issue with the eMMC model. I will let Andrew resend.
We can drop this patchset for now.

Thanks,

C.