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 <cont...@steffen-goertz.de>
> + *
> + * 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 <cont...@steffen-goertz.de>
> + *
> + * 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
