Re: [RFC PATCH 06/10] hw/avr: Add ATmega microcontrollers

[Igor Mammedov]

On Fri, 20 Dec 2019 13:58:29 +0100
Philippe Mathieu-Daudé <address@hidden> wrote:

> Hi Igor,
> 
> On 12/20/19 11:09 AM, Igor Mammedov wrote:
> > On Thu, 28 Nov 2019 02:50:26 +0100
> > Philippe Mathieu-Daudé <address@hidden> wrote:
> >   
> >> Add famous ATmega MCUs:
> >>
> >> - middle range: ATmega168 and ATmega328
> >> - high range: ATmega1280 and ATmega2560
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> >> ---
[...]
> >> +static void atmega_realize(DeviceState *dev, Error **errp)
> >> +{
> >> +    AtmegaState *s = ATMEGA(dev);
> >> +    AtmegaClass *bc = ATMEGA_GET_CLASS(dev);
> >> +    const AtmegaInfo *info = bc->info;
> >> +    DeviceState *cpudev;
> >> +    SysBusDevice *sbd;
> >> +    Error *err = NULL;
> >> +    char *devname;
> >> +    size_t i;
> >> +
> >> +    if (!s->xtal_freq_hz) {
> >> +        error_setg(errp, "\"xtal-frequency-hz\" property must be 
> >> provided.");
> >> +        return;
> >> +    }
> >> +
> >> +    /* CPU */
> >> +    object_initialize_child(OBJECT(dev), "cpu", &s->cpu, sizeof(s->cpu),
> >> +                            info->cpu_type, &err, NULL);
> >> +    if (err) {
> >> +        error_propagate(errp, err);
> >> +        return;
> >> +    }
> >> +    object_property_set_bool(OBJECT(&s->cpu), true, "realized", 
> >> &error_abort);
> >> +    cpudev = DEVICE(&s->cpu);
> >> +
> >> +    /* SRAM */
> >> +    memory_region_allocate_system_memory(&s->sram, OBJECT(dev),
> >> +                                         "sram", info->sram_size);  
> > with main RAM conversion to hostmem backend, this API will go away
> > and RAM memory region will be allocated by generic machine code
> > and shall be treated as backend. Users would be able to access it
> > via MachineState::ram memory region.
> > 
> > Meanwhile I'd suggest to move this line to arduino_machine_init()
> > and pass it to MCU as a link property.  
> 
> Thanks for reviewing this patch.
> 
> I think this MCU and few ARM SoC are good case for your RAM conversion.
> 
> These chipsets provide onboard RAM (SRAM). This amount of SRAM is enough 
> to run u-boot, FreeRTOS, ... Some ARM boards add DRAM, usually larger 
> than the SRAM amount.
> 
> The previous recomendation was to use 
> memory_region_allocate_system_memory() only once, but on the biggest 
> chunk of memory, for performance reasons.
that makes sense only if flexibility is necessary (mem-path, binding to numa 
nodes, prealloc, ...)
Do we really  care about it in non virt usecases.
 
> In the previous cases, the RAM is not added by the board/machine, but is 
> present in the MCU/SoC. This looks incorrect to me to allocate the RAM 
> in the board/machine and pass it to the MCU/SoC.
If it's not user specified value but a fixed memory embedded in SoC,
I'd for simplicity use memory_region_init_ram() directly
(which some boards do for built-in sram).
 
> You are saying the machine/board code will have to ask its children how 
> many ram they need, then allocate, then pass it to each?
machine code will use -m value (and that defaults to default_ram_size,
which board can override) or user provided memdev.

On board side, one could check if user provided backend is suitable and
refuse to start asking to correct CLI error.

So we are still talking about single backend RAM blob, which boards
could use as is or partition with aliases (x86) or map to some other
front-end devices. (point is not to mix device model with backend in this case)


> > Also use MachineState::ram_size and add check that it matches 
> > mc->default_ram_size.
> > Ex: 
> > https://github.com/imammedo/qemu/commit/241c65d506ccba1e0239a2bc0632d7dc9c3517c1
> >   
> >> +    memory_region_add_subregion(get_system_memory(),
> >> +                                OFFSET_DATA + 0x200, &s->sram);
> >> +
> >> +    /* Flash */
> >> +    memory_region_init_rom(&s->flash, OBJECT(dev),
> >> +                           "flash", info->flash_size, &error_fatal);
> >> +    memory_region_add_subregion(get_system_memory(), OFFSET_CODE, 
> >> &s->flash);
> >> +
[...]