Re: [Qemu-devel] [PATCH 17/17] Implement support for i.MX7 Sabre board

[Andrey Smirnov]

On Fri, Oct 6, 2017 at 7:42 AM, Peter Maydell <address@hidden> wrote:
> On 18 September 2017 at 20:51, Andrey Smirnov <address@hidden> wrote:
>> Cc: Peter Maydell <address@hidden>
>> Cc: address@hidden
>> Cc: address@hidden
>> Cc: address@hidden
>> Signed-off-by: Andrey Smirnov <address@hidden>
>> ---
>>  hw/arm/Makefile.objs   |   2 +-
>>  hw/arm/mcimx7d-sabre.c | 100 
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 101 insertions(+), 1 deletion(-)
>>  create mode 100644 hw/arm/mcimx7d-sabre.c
>>
>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> index 33f6051ae3..fc4a963de8 100644
>> --- a/hw/arm/Makefile.objs
>> +++ b/hw/arm/Makefile.objs
>> @@ -19,5 +19,5 @@ obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
>>  obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
>>  obj-$(CONFIG_ASPEED_SOC) += aspeed_soc.o aspeed.o
>>  obj-$(CONFIG_MPS2) += mps2.o
>> -obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o
>> +obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o mcimx7d-sabre.o
>>
>> diff --git a/hw/arm/mcimx7d-sabre.c b/hw/arm/mcimx7d-sabre.c
>> new file mode 100644
>> index 0000000000..34e3933db8
>> --- /dev/null
>> +++ b/hw/arm/mcimx7d-sabre.c
>> @@ -0,0 +1,100 @@
>> +/*
>> + * Copyright (c) 2017, Impinj, Inc.
>> + *
>> + * MCIMX7D_SABRE Board System emulation.
>> + *
>> + * Author: Andrey Smirnov <address@hidden>
>> + *
>> + * This code is licensed under the GPL, version 2 or later.
>> + * See the file `COPYING' in the top level directory.
>> + *
>> + * It (partially) emulates a mcimx7d_sabre board, with a Freescale
>> + * i.MX7 SoC
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu-common.h"
>> +#include "hw/arm/fsl-imx7.h"
>> +#include "hw/boards.h"
>> +#include "sysemu/sysemu.h"
>> +#include "sysemu/device_tree.h"
>> +#include "qemu/error-report.h"
>> +#include "sysemu/qtest.h"
>> +#include "net/net.h"
>> +
>> +typedef struct {
>> +    FslIMX7State soc;
>> +    MemoryRegion ram;
>> +} MCIMX7Sabre;
>> +
>> +static void mcimx7d_add_psci_node(const struct arm_boot_info *boot_info, 
>> void *fdt)
>> +{
>> +    const char comp[] = "arm,psci-0.2\0arm,psci";
>> +
>> +    qemu_fdt_add_subnode(fdt, "/psci");
>> +    qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
>> +    qemu_fdt_setprop_string(fdt, "/psci", "method", "smc");
>> +}
>
> You shouldn't need this -- we should just be able to work with whatever
> the real hardware's device tree is. ("virt" is a special case because
> we create our own device tree there.)
>

Upstream kernel for i.MX7 relies on PSCI to support SMP and this is a
kind of DT fixup that would normally be done by a PSCI-compatible
bootloader. I can remove this code, but it would effectively disable
SMP on vanilla DT/kernel combos.

>> +
>> +static void mcimx7d_sabre_init(MachineState *machine)
>> +{
>> +    static struct arm_boot_info boot_info;
>> +    MCIMX7Sabre *s = g_new0(MCIMX7Sabre, 1);
>> +    Object *soc;
>> +    int i;
>> +
>> +    if (machine->ram_size > FSL_IMX7_MMDC_SIZE) {
>> +        error_report("RAM size " RAM_ADDR_FMT " above max supported (%08x)",
>> +                     machine->ram_size, FSL_IMX7_MMDC_SIZE);
>> +        exit(1);
>> +    }
>> +
>> +    boot_info = (struct arm_boot_info) {
>> +        .loader_start = FSL_IMX7_MMDC_ADDR,
>> +        .board_id = -1,
>> +        .ram_size = machine->ram_size,
>> +        .kernel_filename = machine->kernel_filename,
>> +        .kernel_cmdline = machine->kernel_cmdline,
>> +        .initrd_filename = machine->initrd_filename,
>> +        .nb_cpus = smp_cpus,
>> +        .modify_dtb = mcimx7d_add_psci_node,
>> +    };
>> +
>> +    object_initialize(&s->soc, sizeof(s->soc), TYPE_FSL_IMX7);
>> +    soc = OBJECT(&s->soc);
>> +    object_property_add_child(OBJECT(machine), "soc", soc, &error_fatal);
>> +    object_property_set_bool(soc, true, "realized", &error_fatal);
>> +
>> +    memory_region_allocate_system_memory(&s->ram, NULL, "mcimx7d-sabre.ram",
>> +                                         machine->ram_size);
>> +    memory_region_add_subregion(get_system_memory(),
>> +                                FSL_IMX7_MMDC_ADDR, &s->ram);
>> +
>> +    for (i = 0; i < FSL_IMX7_NUM_USDHCS; i++) {
>> +        BusState *bus;
>> +        DeviceState *carddev;
>> +        DriveInfo *di;
>> +        BlockBackend *blk;
>> +
>> +        di = drive_get_next(IF_SD);
>> +        blk = di ? blk_by_legacy_dinfo(di) : NULL;
>> +        bus = qdev_get_child_bus(DEVICE(&s->soc.usdhc[i]), "sd-bus");
>> +        carddev = qdev_create(bus, TYPE_SD_CARD);
>> +        qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
>> +        object_property_set_bool(OBJECT(carddev), true, "realized", 
>> &error_fatal);
>> +    }
>> +
>> +    if (!qtest_enabled()) {
>> +        arm_load_kernel(&s->soc.cpu[0], &boot_info);
>> +    }
>> +}
>> +
>> +static void mcimx7d_sabre_machine_init(MachineClass *mc)
>> +{
>> +    mc->desc = "Freescale i.MX7 DUAL SABRE (Cortex A7)";
>> +    mc->init = mcimx7d_sabre_init;
>> +    mc->max_cpus = FSL_IMX7_NUM_CPUS;
>> +    mc->ignore_memory_transaction_failures = true;
>
> Please don't set this flag on new board models -- it is only
> for legacy existing board models (where we don't know what
> running guest code might be broken by turning bad accesses
> into guest aborts. For a new board model, you should leave
> the flag unset and instead stub out any devices that you
> need to using create_unimplemented_device() so that your
> guest code boots.

That flag didn't exist when I started porting this board and I only
discovered it a couple of hours before I prepared v1 of the patchset,
since I didn't want to submit the code that wouldn't work but didn't
have time to implements all of the missing blocks. I'll update v2, to
get rid of it.

Thanks,
Andrey Smirnov