Re: [Qemu-devel] [RFC v2] arm: Add NRF51 SOC non-volatile memory controller

[Stefan Hajnoczi]

On Tue, Jun 26, 2018 at 11:32:04AM +0200, Steffen Görtz wrote:
> Changes since V1:
> - Code style changes

Please put the changelog below '---'.

> diff --git a/hw/nvram/nrf51_nvmc.c b/hw/nvram/nrf51_nvmc.c
> new file mode 100644
> index 0000000000..5dde3088a8
> --- /dev/null
> +++ b/hw/nvram/nrf51_nvmc.c
> @@ -0,0 +1,168 @@
> +/*
> + * nrf51_nvmc.c
> + *
> + * Copyright 2018 Steffen Görtz <address@hidden>
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + */

Please mention the full name of the peripheral so its purpose is clear
and include a link to the datasheet.

It's convenient to have this information in the .c file, since that's
where the majority of code changes happen.

> diff --git a/include/hw/nvram/nrf51_nvmc.h b/include/hw/nvram/nrf51_nvmc.h
> new file mode 100644
> index 0000000000..3a63b7e5ad
> --- /dev/null
> +++ b/include/hw/nvram/nrf51_nvmc.h
> @@ -0,0 +1,51 @@
> +/*
> + * nrf51_nvmc.h
> + *
> + * Copyright 2018 Steffen Görtz <address@hidden>
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + *
> + * See Nrf51 reference manual 6 Non-Volatile Memory Controller (NVMC)
> + * See Nrf51 product sheet 8.22 NVMC specifications
> + *
> + * QEMU interface:
> + * + sysbus MMIO regions 0: Memory Region with registers
> + *   to be mapped to the peripherals instance address by the SOC.
> + * + page_size property to set the page size in bytes.
> + * + code_size property to set the code size in number of pages.
> + *
> + * Accuracy of the peripheral model:
> + * + The NVMC is always ready, all requested erase operations succeed
> + *   immediately.
> + * + CONFIG.WEN and CONFIG.EEN flags can be written and read back
> + *   but are not evaluated to check whether a requested write/erase operation
> + *   is legal.

Checking CONFIG.EEN would be easy, please do it.

Peter: CONFIG.WEN determines whether stores to the flash memory region
are allowed.  What is the best way to implement this protection?

> +    MemoryRegion *mr;
> +    AddressSpace as;

I remember you said you tried
address_space_read/write(get_system_memory(), ...) but it didn't work.
Did you figure out why?

> +    struct {
> +        uint32_t config:2;
> +    } state;

I noticed you used bit-fields in recent patches.  They are rarely-used
in QEMU.

The representation of bit-fields is (compiler) implementation-defined,
so they cannot be used for live-migration where values are serialized
(marshaled).

For boolean flags just 'bool' is preferred because 'true'/'false' is
clearer in code than '0'/'1' (the reader doesn't know whether it's a
bool or used as an integer at some point unless they study all the
code).

For this field I would use just uint32_t config.

Stefan

signature.asc
Description: PGP signature