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

[Mark Langsdorf <mark.langsdorf@calxeda.com>]

On 01/06/2012 10:29 AM, Peter Maydell wrote:
> On 5 January 2012 20:02, Mark Langsdorf <mark.langsdorf@calxeda.com> wrote:
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> Adds support for Calxeda's Highbank SoC.
> 
> Is there a test kernel image/etc we can use to confirm that this all works?

The 3.2 kernel should have all the necessary support for Highbank.

>> --- /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?

Rob and I expect that any board differences will be non-existent from
QEMU's perspective.

>> +
>> +#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;
>> +}
>
> 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.

It works on our test boot.

>> +    /* 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).

The board is populated with 4G of DRAM, which we'd like
to support if the host can. Is there a better way to do
that?

>> +    }
>> +    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?

Yes, uboot and the like. It isn't necessary to boot the system,
but it models the actual board a bit better.

>> +    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 :-)

You'd think I would have noticed.

The set frequencies patch doesn't check the maximum frequency,
so the logic is good. I can send a small patch to update the
documentation.

>> +    .name = "highbank",
>> +    .desc = "highbank",
> 
> Can we have an actually descriptive description? :-)

Sure. I'll update it and fix the other issues.

--Mark Langsdorf
Calxeda, Inc.