Re: [Qemu-devel] [RFC 1/3] hw/arm/nrf51_soc: Fix compilation and memory regions

[Peter Maydell]

On 29 May 2018 at 23:03, Julia Suvorova <address@hidden> wrote:
> nRF51 SoC implementation is intended for the BBC Micro:bit board,
> which has 256 KB flash and 16 KB RAM.
> Added FICR defines.
>
> Signed-off-by: Julia Suvorova <address@hidden>
> ---
>  hw/arm/nrf51_soc.c         | 12 +++++++-----
>  include/hw/arm/nrf51_soc.h |  1 +
>  2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index e59ba7079f..6fe06dcfd2 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -26,15 +26,17 @@
>  #define IOMEM_SIZE      0x20000000
>
>  #define FLASH_BASE      0x00000000
> -#define FLASH_SIZE      (144 * 1024)
> +#define FLASH_SIZE      (256 * 1024)
> +
> +#define FICR_BASE       0x10000000
> +#define FICR_SIZE       0x100
>
>  #define SRAM_BASE       0x20000000
> -#define SRAM_SIZE       (6 * 1024)
> +#define SRAM_SIZE       (16 * 1024)
>
>  static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>  {
>      NRF51State *s = NRF51_SOC(dev_soc);
> -    DeviceState *nvic;
>      Error *err = NULL;
>
>      /* IO space */
> @@ -69,8 +71,8 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error 
> **errp)
>      memory_region_add_subregion(system_memory, SRAM_BASE, sram);
>
>      /* TODO: implement a cortex m0 and update this */
> -    nvic = armv7m_init(get_system_memory(), FLASH_SIZE, 96,
> -            s->kernel_filename, ARM_CPU_TYPE_NAME("cortex-m3"));
> +    s->nvic = armv7m_init(get_system_memory(), FLASH_SIZE, 96,
> +               s->kernel_filename, ARM_CPU_TYPE_NAME("cortex-m3"));
>  }
>
>  static Property nrf51_soc_properties[] = {
> diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
> index 5431d200f8..a6bbe9f108 100644
> --- a/include/hw/arm/nrf51_soc.h
> +++ b/include/hw/arm/nrf51_soc.h
> @@ -23,6 +23,7 @@ typedef struct NRF51State {
>
>      /*< public >*/
>      char *kernel_filename;
> +    DeviceState *nvic;
>
>      MemoryRegion iomem;
>  } NRF51State;

The better approach here (which I think I suggested in review
of Joel's patches) is to have the armv7m object embedded in the
NRF51State. Then you can initialize and realize it in-place,
and the board code calls armv7m_load_kernel() rather than
having to pass the kernel filename to the SoC. (Nothing then
needs to call armv7m_init() at all.)

thanks
-- PMM