On Tue, Mar 3, 2015 at 5:40 AM, Peter Crosthwaite
<peter.crosthwa...@xilinx.com> wrote:
> On Mon, Feb 23, 2015 at 6:24 PM, Alistair Francis
> <alistair.fran...@xilinx.com> wrote:
>> On Tue, Feb 24, 2015 at 9:04 AM, Peter Crosthwaite
>> <peter.crosthwa...@xilinx.com> wrote:
>>> Zynq MPSoC supports external DDR RAM. Add a RAM at 0 to the model.
>>>
>>> Signed-off-by: Peter Crosthwaite <peter.crosthwa...@xilinx.com>
>>> ---
>>> hw/arm/xlnx-zynq-mp-generic.c | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/hw/arm/xlnx-zynq-mp-generic.c b/hw/arm/xlnx-zynq-mp-generic.c
>>> index ff69b07..7394e82 100644
>>> --- a/hw/arm/xlnx-zynq-mp-generic.c
>>> +++ b/hw/arm/xlnx-zynq-mp-generic.c
>>> @@ -18,9 +18,11 @@
>>> #include "hw/arm/xlnx-zynq-mp.h"
>>> #include "hw/boards.h"
>>> #include "qemu/error-report.h"
>>> +#include "exec/address-spaces.h"
>>>
>>> typedef struct XlnxZynqMPGeneric {
>>> XlnxZynqMPState soc;
>>> + MemoryRegion ddr_ram;
>>> } XlnxZynqMPGeneric;
>>>
>>> static void xlnx_zynq_mp_generic_init(MachineState *machine)
>>> @@ -36,6 +38,11 @@ static void xlnx_zynq_mp_generic_init(MachineState
>>> *machine)
>>> error_report("%s", error_get_pretty(err));
>>> exit(1);
>>> }
>>> +
>>> + memory_region_init_ram(&s->ddr_ram, NULL, "ddr-ram", machine->ram_size,
>>> + &error_abort);
>>
>> Shouldn't there be a default size if none is specified? This looks
>> like it will cause user
>> issues if they don't understand that they must specify the memory size.
>>
>
> So vl.c provides a core default of 128MB if -m is unspecified. It
> should boot with that successfully. The error case is probably when
> the user overrides -m to a low value such that the boot cant work
> anymore, but that would be a error caught by the boot loader I think.
I still think that this could cause issues. If a user doesn't realise that
they have to specify -m and the boot still works, they will just assume
that the correct memory size is in the machine model. When in fact all
that is happening is they are getting a small amount of memory that is
just lucky enough to work. 128MB is very small for ZynqMP.
Maybe instead of an error or a default just have a warning if the memory
is below a certain size. That way it will still boot, but the user will know
that they should be specifying a size.
Thanks,
Alistair
>
> Regards,
> Peter
>
>> At least return an error if none is specified.
>>
>> Thanks,
>>
>> Alistair
>>
>>> + vmstate_register_ram_global(&s->ddr_ram);
>>> + memory_region_add_subregion(get_system_memory(), 0, &s->ddr_ram);
>>> }
>>>
>>> static QEMUMachine xlnx_zynq_mp_generic_machine = {
>>> --
>>> 2.3.0.1.g27a12f1
>>>
>>>
>>
>
