Re: [Qemu-devel] [PATCH 5/5] arm: SoC model for Calxeda Highbank

[Peter Maydell]

On 5 January 2012 20:02, Mark Langsdorf <address@hidden> wrote:
> From: Rob Herring <address@hidden>
>
> Adds support for Calxeda's Highbank SoC.

Is there a test kernel image/etc we can use to confirm that this all works?

> --- /dev/null
> +++ b/hw/highbank.c
> @@ -0,0 +1,227 @@
> +/*
> + * Calxeda Highbank SoC emulation

Is it worth splitting the SoC emulation out from the board emulation, or
is the expectation that the SoC will be used in this board and only this
board?

> + *
> + * Copyright (c) 2010-2012 Calxeda
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.

2-or-later, please.

> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "sysbus.h"
> +#include "arm-misc.h"
> +#include "primecell.h"
> +#include "devices.h"
> +#include "loader.h"
> +#include "elf.h"
> +#include "net.h"
> +#include "sysemu.h"
> +#include "boards.h"
> +#include "sysbus.h"
> +#include "blockdev.h"
> +#include "exec-memory.h"

This looks like a lot of includes -- are they all required? (eg why elf.h?)

> +
> +#define SMP_BOOT_ADDR 0
> +#define NIRQ_GIC      160
> +
> +/* Board init.  */
> +static void highbank_cpu_reset(void *opaque)
> +{
> +    CPUState *env = opaque;
> +
> +    cpu_reset(env);
> +    env->cp15.c15_config_base_address = 0xfff10000;
> +
> +    /* Set entry point for secondary CPUs.  This assumes we're using
> +       the init code from arm_boot.c.  Real hardware resets all CPUs
> +       the same.  */
> +    env->regs[15] = 0;
> +}

The env->regs[] bit at least shouldn't be needed -- see commit
6ed221b637 which consolidated it into arm_boot.c. Ditto the
cpu_reset().

I think your attempt to set c15_config_base_address here may
be being defeated by the cpu_reset() call in
hw/arm_boot.c:do_cpu_reset(), but I haven't checked that.

> +static void hb_regs_write(void *opaque, target_phys_addr_t offset,
> +                          uint64_t value, unsigned size)
> +{
> +    uint32_t *regs = opaque;
> +
> +    if (offset == 0xf00) {
> +        if (value == 1 || value == 2) {
> +            qemu_system_reset_request();
> +        } else if (value == 3) {
> +            qemu_system_shutdown_request();
> +        }
> +    }
> +
> +    regs[offset/4] = value;
> +}

Please make this a proper qdev device (it can stay in this
file).

> +    /* Override default RAM size */
> +    if (ram_size == 0x8000000) {
> +        if (sizeof(long) == 8) {
> +            ram_size = 0xff900000;
> +        } else {
> +            ram_size = 0x80000000;
> +        }

Yuck. Model behaviour shouldn't depend on properties of
the host system like sizeof(long).

> +    }
> +    sysmem = get_system_memory();
> +    dram = g_new(MemoryRegion, 1);
> +    memory_region_init_ram(dram, "highbank.dram", ram_size);
> +    /* SDRAM at address zero.  */
> +    memory_region_add_subregion(sysmem, 0, dram);
> +
> +    sysram = g_new(MemoryRegion, 1);
> +    memory_region_init_ram(sysram, "highbank.sysram", 0x8000);
> +    memory_region_add_subregion(sysmem, 0xfff88000, sysram);
> +    if (load_image_targphys("sysram.bin", 0xfff88000, 0x8000) < 0) {
> +            fprintf(stderr, "Unable to load sysram.bin\n");
> +    }

Is this for some sort of BIOS-image equivalent?

> +    dev = qdev_create(NULL, "sp804");
> +    qdev_prop_set_uint32(dev, "freq0", 150000000);
> +    qdev_prop_set_uint32(dev, "freq1", 150000000);

So, er, we just committed a patch saying the timer frequency
could go up to 1MHz, and this is rather more than that :-)

> +    qemu_check_nic_model(&nd_table[0], "xgmac");
> +    dev = qdev_create(NULL, "xgmac");
> +    qdev_set_nic_properties(dev, &nd_table[0]);
> +    qdev_init_nofail(dev);

You need to guard the nic creation and wiring with
  if (nd_table[0].vlan) {
     ....
  }

to catch the case where the user requested no NIC at all
(ie the nd_table[] entry is unused).
(A command line with "-net user" and no other -net options
will provoke this.)

> +static QEMUMachine highbank_machine = {
> +    .name = "highbank",
> +    .desc = "highbank",

Can we have an actually descriptive description? :-)

-- PMM