> On Jun 23, 2022, at 1:21 PM, Cédric Le Goater <c...@kaod.org> wrote:
>
> Currently, the Aspeed machines allocate a ram container region in
> which the machine ram region is mapped. See commit ad1a9782186d
> ("aspeed: add a RAM memory region container"). An extra region is
> mapped after ram in the ram container to catch invalid access done by
> FW. That's how FW determines the size of ram. See commit ebe31c0a8ef7
> ("aspeed: add a max_ram_size property to the memory controller").
>
> Let's move all the logic under the SoC where it should be. It will
> also ease the work on multi SoC support.
Looks good!
Reviewed-by: Peter Delevoryas <p...@fb.com>
>
> Signed-off-by: Cédric Le Goater <c...@kaod.org>
> ---
>
> Changes in v2:
>
> - handle errors
>
> include/hw/arm/aspeed_soc.h | 2 ++
> hw/arm/aspeed.c | 39 ++---------------------------------
> hw/arm/aspeed_ast2600.c | 6 ++++--
> hw/arm/aspeed_soc.c | 41 +++++++++++++++++++++++++++++++++++--
> 4 files changed, 47 insertions(+), 41 deletions(-)
>
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index 02a5a9ffcbd3..e8a104823d35 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -50,6 +50,7 @@ struct AspeedSoCState {
> A15MPPrivState a7mpcore;
> ARMv7MState armv7m;
> MemoryRegion *dram_mr;
> + MemoryRegion dram_container;
> MemoryRegion sram;
> AspeedVICState vic;
> AspeedRtcState rtc;
> @@ -165,5 +166,6 @@ enum {
>
> qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int dev);
> void aspeed_soc_uart_init(AspeedSoCState *s);
> +bool aspeed_soc_dram_init(AspeedSoCState *s, Error **errp);
>
> #endif /* ASPEED_SOC_H */
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index a06f7c1b62a9..dc09773b0ba5 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -174,27 +174,6 @@ struct AspeedMachineState {
> #define BLETCHLEY_BMC_HW_STRAP1 AST2600_EVB_HW_STRAP1
> #define BLETCHLEY_BMC_HW_STRAP2 AST2600_EVB_HW_STRAP2
>
> -/*
> - * The max ram region is for firmwares that scan the address space
> - * with load/store to guess how much RAM the SoC has.
> - */
> -static uint64_t max_ram_read(void *opaque, hwaddr offset, unsigned size)
> -{
> - return 0;
> -}
> -
> -static void max_ram_write(void *opaque, hwaddr offset, uint64_t value,
> - unsigned size)
> -{
> - /* Discard writes */
> -}
> -
> -static const MemoryRegionOps max_ram_ops = {
> - .read = max_ram_read,
> - .write = max_ram_write,
> - .endianness = DEVICE_NATIVE_ENDIAN,
> -};
> -
> #define AST_SMP_MAILBOX_BASE 0x1e6e2180
> #define AST_SMP_MBOX_FIELD_ENTRY (AST_SMP_MAILBOX_BASE + 0x0)
> #define AST_SMP_MBOX_FIELD_GOSIGN (AST_SMP_MAILBOX_BASE + 0x4)
> @@ -324,20 +303,16 @@ static void aspeed_machine_init(MachineState *machine)
> AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine);
> AspeedSoCClass *sc;
> DriveInfo *drive0 = drive_get(IF_MTD, 0, 0);
> - ram_addr_t max_ram_size;
> int i;
> NICInfo *nd = &nd_table[0];
>
> - memory_region_init(&bmc->ram_container, NULL, "aspeed-ram-container",
> - 4 * GiB);
> - memory_region_add_subregion(&bmc->ram_container, 0, machine->ram);
> -
> object_initialize_child(OBJECT(machine), "soc", &bmc->soc, amc->soc_name);
>
> sc = ASPEED_SOC_GET_CLASS(&bmc->soc);
>
> /*
> - * This will error out if isize is not supported by memory controller.
> + * This will error out if the RAM size is not supported by the
> + * memory controller of the SoC.
> */
> object_property_set_uint(OBJECT(&bmc->soc), "ram-size", machine->ram_size,
> &error_fatal);
> @@ -369,16 +344,6 @@ static void aspeed_machine_init(MachineState *machine)
> amc->uart_default);
> qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
>
> - memory_region_add_subregion(get_system_memory(),
> - sc->memmap[ASPEED_DEV_SDRAM],
> - &bmc->ram_container);
> -
> - max_ram_size = object_property_get_uint(OBJECT(&bmc->soc),
> "max-ram-size",
> - &error_abort);
> - memory_region_init_io(&bmc->max_ram, NULL, &max_ram_ops, NULL,
> - "max_ram", max_ram_size - machine->ram_size);
> - memory_region_add_subregion(&bmc->ram_container, machine->ram_size,
> &bmc->max_ram);
> -
> aspeed_board_init_flashes(&bmc->soc.fmc,
> bmc->fmc_model ? bmc->fmc_model :
> amc->fmc_model,
> amc->num_cs, 0);
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index b0a4199b6960..f70b17d3f9cf 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -197,8 +197,6 @@ static void aspeed_soc_ast2600_init(Object *obj)
> object_initialize_child(obj, "sdmc", &s->sdmc, typename);
> object_property_add_alias(obj, "ram-size", OBJECT(&s->sdmc),
> "ram-size");
> - object_property_add_alias(obj, "max-ram-size", OBJECT(&s->sdmc),
> - "max-ram-size");
>
> for (i = 0; i < sc->wdts_num; i++) {
> snprintf(typename, sizeof(typename), "aspeed.wdt-%s", socname);
> @@ -271,6 +269,10 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev,
> Error **errp)
> /* IO space */
> create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_DEV_IOMEM],
> ASPEED_SOC_IOMEM_SIZE);
> + /* RAM */
> + if (!aspeed_soc_dram_init(s, errp)) {
> + return;
> + }
>
> /* Video engine stub */
> create_unimplemented_device("aspeed.video", sc->memmap[ASPEED_DEV_VIDEO],
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 30574d4276ab..f5300288745b 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -11,6 +11,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/units.h"
> #include "qapi/error.h"
> #include "hw/misc/unimp.h"
> #include "hw/arm/aspeed_soc.h"
> @@ -191,8 +192,6 @@ static void aspeed_soc_init(Object *obj)
> object_initialize_child(obj, "sdmc", &s->sdmc, typename);
> object_property_add_alias(obj, "ram-size", OBJECT(&s->sdmc),
> "ram-size");
> - object_property_add_alias(obj, "max-ram-size", OBJECT(&s->sdmc),
> - "max-ram-size");
>
> for (i = 0; i < sc->wdts_num; i++) {
> snprintf(typename, sizeof(typename), "aspeed.wdt-%s", socname);
> @@ -237,6 +236,11 @@ static void aspeed_soc_realize(DeviceState *dev, Error
> **errp)
> create_unimplemented_device("aspeed_soc.io", sc->memmap[ASPEED_DEV_IOMEM],
> ASPEED_SOC_IOMEM_SIZE);
>
> + /* RAM */
> + if (!aspeed_soc_dram_init(s, errp)) {
> + return;
> + }
> +
> /* Video engine stub */
> create_unimplemented_device("aspeed.video", sc->memmap[ASPEED_DEV_VIDEO],
> 0x1000);
> @@ -561,3 +565,36 @@ void aspeed_soc_uart_init(AspeedSoCState *s)
> serial_hd(i), DEVICE_LITTLE_ENDIAN);
> }
> }
> +
> +bool aspeed_soc_dram_init(AspeedSoCState *s, Error **errp)
> +{
> + AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> + ram_addr_t ram_size, max_ram_size;
> + DeviceState *dev;
> +
> + memory_region_init(&s->dram_container, OBJECT(s), "ram-container", 4 *
> GiB);
> + memory_region_add_subregion(&s->dram_container, 0, s->dram_mr);
> +
> + /*
> + * Add a memory region beyond the RAM region to let firmwares scan
> + * the address space with load/store and guess how much RAM the
> + * SoC has.
> + */
> + ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size",
> + &error_abort);
> + max_ram_size = object_property_get_uint(OBJECT(&s->sdmc), "max-ram-size",
> + &error_abort);
> +
> + dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
> + qdev_prop_set_string(dev, "name", "ram-empty");
> + qdev_prop_set_uint64(dev, "size", max_ram_size - ram_size);
> + if (!sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp)) {
> + return false;
> + }
> + memory_region_add_subregion_overlap(&s->dram_container, ram_size,
> + sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0), -1000);
> +
> + memory_region_add_subregion(get_system_memory(),
> + sc->memmap[ASPEED_DEV_SDRAM], &s->dram_container);
> + return true;
> +}
> --
> 2.35.3
>
