Re: [Qemu-devel] [PATCH v1 4/5] hw/arm: Add the STM32F4xx SoC

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 2 May 2019 at 06:41, Alistair Francis <alistair@alistair23.me> wrote:
>
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> ---
>  MAINTAINERS                     |   8 +
>  default-configs/arm-softmmu.mak |   1 +
>  hw/arm/Kconfig                  |   3 +
>  hw/arm/Makefile.objs            |   1 +
>  hw/arm/stm32f405_soc.c          | 306 ++++++++++++++++++++++++++++++++
>  include/hw/arm/stm32f405_soc.h  |  74 ++++++++
>  6 files changed, 393 insertions(+)
>  create mode 100644 hw/arm/stm32f405_soc.c
>  create mode 100644 include/hw/arm/stm32f405_soc.h
>

> +static void stm32f405_soc_realize(DeviceState *dev_soc, Error **errp)
> +{
> +    STM32F405State *s = STM32F405_SOC(dev_soc);
> +    DeviceState *dev, *armv7m;
> +    SysBusDevice *busdev;
> +    Error *err = NULL;
> +    int i;
> +
> +    s->system_memory = get_system_memory();
> +    s->sram = g_new(MemoryRegion, 1);
> +    s->flash = g_new(MemoryRegion, 1);
> +    s->flash_alias = g_new(MemoryRegion, 1);

What I meant by my comment on v1 was that rather than doing
g_new() here you can just have the STM32F405State struct
have
 MemoryRegion sram;
 MemoryRegion flash;
etc

and then instead of
  memory_region_init_ram(s->flash, ...)
you use
  memory_region_init_ram(&s->flash, ...)
etc

which avoids doing separate memory allocations (which would
need to be freed if you then do an error-exit from the
realize function, I think).

And you don't need to have an s->system_memory -- that
can just be a local variable, because it's just caching
the pointer to the global system memory MemoryRegion.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM