Re: [Qemu-devel] [PATCH 3/7] arm: Add chip variant property to nRF51 SoC
Hi Peter, object_property_set_bool(soc, true, "realized", &error_abort);>> >> -armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, >> -NRF51_SOC(soc)->flash_size); >> +armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, 0x00); > > Hmm. Using 0x00 here doesn't seem ideal. i agree. The actual sizes for RAM/ROM are calculated in the SOC at the moment. I do not really know what impact max_sz has here. I could set the it to the maximum code size defined in the memory map of the NRF51 series. Another way would be to lift the calculation of the ROM size from the SOC to the Board. What would you propose? >> +#define PAGE_SIZE 1024 > > Page size isn't really an obvious concept for M profile. Flash pages are referred to here, not MMU pages. This constant is moved and renamed to NRF51_PAGE_SIZE in the upcoming revision. Would you suggest another name or can with stick with this? > Why have these lines been changed? (Rule of thumb: don't include code > reformatting or style cleanups in the same patch as substantive changes.) These lines where > 80 chars before. I should rather suggest a change of Joel's patch series. Thank you for your remarks. Steffen
Re: [Qemu-devel] [PATCH 3/7] arm: Add chip variant property to nRF51 SoC
On 21 August 2018 at 11:03, Steffen Görtz wrote: > Hi Peter, > > object_property_set_bool(soc, true, "realized", &error_abort);>> >>> -armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, >>> -NRF51_SOC(soc)->flash_size); >>> +armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, 0x00); >> >> Hmm. Using 0x00 here doesn't seem ideal. > > i agree. The actual sizes for RAM/ROM are calculated in the SOC at the moment. > I do not really know what impact max_sz has here. I could set the it to the > maximum code size defined in the memory map of the NRF51 series. Another way > would be > to lift the calculation of the ROM size from the SOC to the Board. What would > you propose? I think we need to specify a value somehow, otherwise we won't be able to load raw images (which I think are the only ones where we enforce the limit value). You could just use the largest possible size, I suppose. >>> +#define PAGE_SIZE 1024 >> >> Page size isn't really an obvious concept for M profile. > > Flash pages are referred to here, not MMU pages. This constant is moved and > renamed to NRF51_PAGE_SIZE in the upcoming revision. Would you suggest > another name or can with stick with this? I guess NRF51_FLASH_PAGE_SIZE would be a little clearer ? thanks -- PMM
Re: [Qemu-devel] [PATCH 3/7] arm: Add chip variant property to nRF51 SoC
On 11 August 2018 at 10:08, Steffen Görtz wrote:
> Nordic Semiconductor nRF51 SOCs are available
> in different "variants" which differ in available memory.
> This patchs adds a "variant" attribute to the SOC
> so that the user can choose between different variants and
> thus memory sizes.
>
> See product specification
> http://infocenter.nordicsemi.com/pdf/nRF51822_PS_v3.1.pdf
> section 10.6
>
> Signed-off-by: Steffen Görtz
> ---
> hw/arm/microbit.c | 4 ++--
> hw/arm/nrf51_soc.c | 40 +++---
> include/hw/arm/nrf51_soc.h | 15 +++---
> 3 files changed, 43 insertions(+), 16 deletions(-)
>
> diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
> index 8f3c446f52..d6776dea0a 100644
> --- a/hw/arm/microbit.c
> +++ b/hw/arm/microbit.c
> @@ -27,11 +27,11 @@ static void microbit_init(MachineState *machine)
> object_property_add_child(OBJECT(machine), "nrf51", soc, &error_fatal);
> object_property_set_link(soc, OBJECT(system_memory),
> "memory", &error_abort);
> +qdev_prop_set_uint32(DEVICE(soc), "variant", NRF51_VARIANT_AA);
>
> object_property_set_bool(soc, true, "realized", &error_abort);
>
> -armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
> -NRF51_SOC(soc)->flash_size);
> +armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, 0x00);
Hmm. Using 0x00 here doesn't seem ideal.
> }
>
>
> diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index 441d05e1ef..85bce2c1e0 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -32,15 +32,21 @@
> #define FLASH_BASE 0x
> #define SRAM_BASE 0x2000
>
> -/* 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 */
> -#define NRF51822_FLASH_SIZE (256 * 1024)
> -#define NRF51822_SRAM_SIZE (16 * 1024)
> +#define PAGE_SIZE 1024
Page size isn't really an obvious concept for M profile.
You might find the KiB macro in qemu/units.h useful ?
>
> /* IRQ lines can be derived from peripheral base addresses */
> #define BASE_TO_IRQ(base) (((base) >> 12) & 0x1F)
>
> +/* RAM and CODE size in number of pages for different NRF51Variants variants
> */
> +struct {
> + hwaddr ram_size;
> + hwaddr flash_size;
> +} NRF51VariantAttributes[] = {
> +[NRF51_VARIANT_AA] = {.ram_size = 16, .flash_size = 256 },
> +[NRF51_VARIANT_AB] = {.ram_size = 16, .flash_size = 128 },
> +[NRF51_VARIANT_AC] = {.ram_size = 32, .flash_size = 256 },
> +};
> +
> static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
> {
> NRF51State *s = NRF51_SOC(dev_soc);
> @@ -51,13 +57,21 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error
> **errp)
> return;
> }
>
> +if (!(s->part_variant > NRF51_VARIANT_INVALID
> +&& s->part_variant < NRF51_VARIANT_MAX)) {
> +error_setg(errp, "VARIANT not set or invalid");
> +return;
> +}
> +
> object_property_set_link(OBJECT(&s->cpu), OBJECT(&s->container),
> "memory",
> &err);
> object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
>
> memory_region_add_subregion_overlap(&s->container, 0, s->board_memory,
> -1);
>
> -memory_region_init_ram(&s->flash, OBJECT(s), "nrf51.flash",
> s->flash_size,
> +/* FLASH */
> +memory_region_init_ram(&s->flash, NULL, "nrf51_soc.flash",
> +NRF51VariantAttributes[s->part_variant].flash_size * PAGE_SIZE,
> &err);
> if (err) {
> error_propagate(errp, err);
> @@ -66,7 +80,9 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error
> **errp)
> memory_region_set_readonly(&s->flash, true);
> memory_region_add_subregion(&s->container, FLASH_BASE, &s->flash);
>
> -memory_region_init_ram(&s->sram, NULL, "nrf51.sram", s->sram_size, &err);
> +/* SRAM */
> +memory_region_init_ram(&s->sram, NULL, "nrf51_soc.sram",
> +NRF51VariantAttributes[s->part_variant].ram_size * PAGE_SIZE,
> &err);
> if (err) {
> error_propagate(errp, err);
> return;
> @@ -85,17 +101,19 @@ static void nrf51_soc_init(Object *obj)
> memory_region_init(&s->container, obj, "nrf51-container", UINT64_MAX);
>
> object_initialize(&s->cpu, sizeof(s->cpu), TYPE_ARMV7M);
> -object_property_add_child(OBJECT(s), "armv6m", OBJECT(&s->cpu),
> &error_abort);
> +object_property_add_child(OBJECT(s), "armv6m", OBJECT(&s->cpu),
> + &error_abort);
> qdev_set_parent_bus(DEVICE(&s->cpu), sysbus_get_default());
> -qdev_prop_set_string(DEVICE(&s->cpu), "cpu-type",
> ARM_CPU_TYPE_NAME("cortex-m0"));
> +qdev_prop_set_string(DEVICE(&s->cpu), "cpu-type",
> + ARM_CPU_TYPE_NAME("cortex-m0"));
Why have these lines been changed? (Rule of thumb: don't include code
reformatting or style
[Qemu-devel] [PATCH 3/7] arm: Add chip variant property to nRF51 SoC
Nordic Semiconductor nRF51 SOCs are available
in different "variants" which differ in available memory.
This patchs adds a "variant" attribute to the SOC
so that the user can choose between different variants and
thus memory sizes.
See product specification
http://infocenter.nordicsemi.com/pdf/nRF51822_PS_v3.1.pdf
section 10.6
Signed-off-by: Steffen Görtz
---
hw/arm/microbit.c | 4 ++--
hw/arm/nrf51_soc.c | 40 +++---
include/hw/arm/nrf51_soc.h | 15 +++---
3 files changed, 43 insertions(+), 16 deletions(-)
diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
index 8f3c446f52..d6776dea0a 100644
--- a/hw/arm/microbit.c
+++ b/hw/arm/microbit.c
@@ -27,11 +27,11 @@ static void microbit_init(MachineState *machine)
object_property_add_child(OBJECT(machine), "nrf51", soc, &error_fatal);
object_property_set_link(soc, OBJECT(system_memory),
"memory", &error_abort);
+qdev_prop_set_uint32(DEVICE(soc), "variant", NRF51_VARIANT_AA);
object_property_set_bool(soc, true, "realized", &error_abort);
-armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
-NRF51_SOC(soc)->flash_size);
+armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename, 0x00);
}
diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 441d05e1ef..85bce2c1e0 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -32,15 +32,21 @@
#define FLASH_BASE 0x
#define SRAM_BASE 0x2000
-/* 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 */
-#define NRF51822_FLASH_SIZE (256 * 1024)
-#define NRF51822_SRAM_SIZE (16 * 1024)
+#define PAGE_SIZE 1024
/* IRQ lines can be derived from peripheral base addresses */
#define BASE_TO_IRQ(base) (((base) >> 12) & 0x1F)
+/* RAM and CODE size in number of pages for different NRF51Variants variants */
+struct {
+ hwaddr ram_size;
+ hwaddr flash_size;
+} NRF51VariantAttributes[] = {
+[NRF51_VARIANT_AA] = {.ram_size = 16, .flash_size = 256 },
+[NRF51_VARIANT_AB] = {.ram_size = 16, .flash_size = 128 },
+[NRF51_VARIANT_AC] = {.ram_size = 32, .flash_size = 256 },
+};
+
static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
{
NRF51State *s = NRF51_SOC(dev_soc);
@@ -51,13 +57,21 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error
**errp)
return;
}
+if (!(s->part_variant > NRF51_VARIANT_INVALID
+&& s->part_variant < NRF51_VARIANT_MAX)) {
+error_setg(errp, "VARIANT not set or invalid");
+return;
+}
+
object_property_set_link(OBJECT(&s->cpu), OBJECT(&s->container), "memory",
&err);
object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
memory_region_add_subregion_overlap(&s->container, 0, s->board_memory, -1);
-memory_region_init_ram(&s->flash, OBJECT(s), "nrf51.flash", s->flash_size,
+/* FLASH */
+memory_region_init_ram(&s->flash, NULL, "nrf51_soc.flash",
+NRF51VariantAttributes[s->part_variant].flash_size * PAGE_SIZE,
&err);
if (err) {
error_propagate(errp, err);
@@ -66,7 +80,9 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error
**errp)
memory_region_set_readonly(&s->flash, true);
memory_region_add_subregion(&s->container, FLASH_BASE, &s->flash);
-memory_region_init_ram(&s->sram, NULL, "nrf51.sram", s->sram_size, &err);
+/* SRAM */
+memory_region_init_ram(&s->sram, NULL, "nrf51_soc.sram",
+NRF51VariantAttributes[s->part_variant].ram_size * PAGE_SIZE,
&err);
if (err) {
error_propagate(errp, err);
return;
@@ -85,17 +101,19 @@ static void nrf51_soc_init(Object *obj)
memory_region_init(&s->container, obj, "nrf51-container", UINT64_MAX);
object_initialize(&s->cpu, sizeof(s->cpu), TYPE_ARMV7M);
-object_property_add_child(OBJECT(s), "armv6m", OBJECT(&s->cpu),
&error_abort);
+object_property_add_child(OBJECT(s), "armv6m", OBJECT(&s->cpu),
+ &error_abort);
qdev_set_parent_bus(DEVICE(&s->cpu), sysbus_get_default());
-qdev_prop_set_string(DEVICE(&s->cpu), "cpu-type",
ARM_CPU_TYPE_NAME("cortex-m0"));
+qdev_prop_set_string(DEVICE(&s->cpu), "cpu-type",
+ ARM_CPU_TYPE_NAME("cortex-m0"));
qdev_prop_set_uint32(DEVICE(&s->cpu), "num-irq", 32);
}
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_INT32("variant", NRF51State, part_variant,
+ NRF51_VARIANT_INVALID),
DEFINE_PROP_EN
