On Mon, Mar 2, 2015 at 2:38 PM, Alistair Francis
<alistair.fran...@xilinx.com> wrote:
> 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.
>
So there is precedent for upper clamps on this but not lower clamps. I
think we may actually want the same as zynq with an upper clamp:
158 /* max 2GB ram */
159 if (ram_size > 0x80000000) {
160 ram_size = 0x80000000;
161 }
> 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.
>
Adding the warning for <= 128MB. Anything more than that then the user
must have manually specified -m so not too worried about that.
Regards,
Peter
> 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
>>>>
>>>>
>>>
>>
>
