Re: [Qemu-devel] [PATCH v5 2/3] arm: Add Nordic Semiconductor nRF51 SoC

[Joel Stanley]

Hi Peter,

On Thu, 16 Aug 2018 at 07:24, Peter Maydell <address@hidden> wrote:

> > +static Property nrf51_soc_properties[] = {
> > +    DEFINE_PROP_LINK("memory", NRF51State, board_memory, 
> > TYPE_MEMORY_REGION,
> > +                     MemoryRegion *),
> > +    DEFINE_PROP_UINT32("sram-size", NRF51State, sram_size, 
> > NRF51822_SRAM_SIZE),
> > +    DEFINE_PROP_UINT32("flash-size", NRF51State, flash_size, 
> > NRF51822_FLASH_SIZE),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
>
> Ah, I was wondering how flash_size was handled when I looked at patch 3.
>
> Instead of your patch 3 board model code reaching into the internals
> of the NRF51State struct to pull out the flash_size field, the board
> code should set this property on the object it creates, and then it
> knows how big the flash it's asked for is and can pass that value also
> to the armv7m_load_kernel() function.

Thanks for the review. I've sorted out the other comments relating to
this patch, but I wanted to discuss this one.

I agree that it would be neater to do this. I didn't as the flash is
part of the NRF51822 SoC, opposed to some external flash that is on
the microbit board and connected to the SoC. This is mentioned in the
comment at the start of the file:

 /*
  * The size and base is for the NRF51822 part. If other parts
  * are supported in the future, add a sub-class of NRF51SoC for
  * the specific variants
  */

What would you prefer we do here?

Cheers,

Joel