[PATCH 0/2] hw/arm: Add support for STM32 H405 and fix STM32F405 memory layout

[PATCH 0/2] hw/arm: Add support for STM32 H405 and fix STM32F405 memory layout

[Felipe Balbi]

Hi,

The following patches pass checkpatch.pl and have been tested against today's
HEAD (55745005e90a).

Felipe Balbi (2):
  hw/arm/stm32f405: correctly describe the memory layout
  hw/arm: Add Olimex H405

 MAINTAINERS                             |  6 +++
 configs/devices/arm-softmmu/default.mak |  1 +
 hw/arm/Kconfig                          |  4 ++
 hw/arm/meson.build                      |  1 +
 hw/arm/olimex-stm32-h405.c              | 65 +++++++++++++++++++++++++
 hw/arm/stm32f405_soc.c                  |  8 +++
 include/hw/arm/stm32f405_soc.h          |  5 +-
 7 files changed, 89 insertions(+), 1 deletion(-)
 create mode 100644 hw/arm/olimex-stm32-h405.c

-- 
2.38.1

[PATCH 1/2] hw/arm/stm32f405: correctly describe the memory layout

[Felipe Balbi]

STM32F405 has 128K of SRAM and another 64K of CCM (Core-coupled
Memory) at a different base address. Correctly describe the memory
layout to give existing FW images have a chance to run unmodified.

Signed-off-by: Felipe Balbi <balbi@kernel.org>
---
 hw/arm/stm32f405_soc.c         | 8 ++++++++
 include/hw/arm/stm32f405_soc.h | 5 ++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/arm/stm32f405_soc.c b/hw/arm/stm32f405_soc.c
index c07947d9f8b1..cef23d7ee41a 100644
--- a/hw/arm/stm32f405_soc.c
+++ b/hw/arm/stm32f405_soc.c
@@ -139,6 +139,14 @@ static void stm32f405_soc_realize(DeviceState *dev_soc, Error **errp)
     }
     memory_region_add_subregion(system_memory, SRAM_BASE_ADDRESS, &s->sram);
 
+    memory_region_init_ram(&s->ccm, NULL, "STM32F405.ccm", CCM_SIZE,
+                           &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
+    }
+    memory_region_add_subregion(system_memory, CCM_BASE_ADDRESS, &s->ccm);
+
     armv7m = DEVICE(&s->armv7m);
     qdev_prop_set_uint32(armv7m, "num-irq", 96);
     qdev_prop_set_string(armv7m, "cpu-type", s->cpu_type);
diff --git a/include/hw/arm/stm32f405_soc.h b/include/hw/arm/stm32f405_soc.h
index 5bb0c8d56979..249ab5434ec7 100644
--- a/include/hw/arm/stm32f405_soc.h
+++ b/include/hw/arm/stm32f405_soc.h
@@ -46,7 +46,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(STM32F405State, STM32F405_SOC)
 #define FLASH_BASE_ADDRESS 0x08000000
 #define FLASH_SIZE (1024 * 1024)
 #define SRAM_BASE_ADDRESS 0x20000000
-#define SRAM_SIZE (192 * 1024)
+#define SRAM_SIZE (128 * 1024)
+#define CCM_BASE_ADDRESS 0x10000000
+#define CCM_SIZE (64 * 1024)
 
 struct STM32F405State {
     /*< private >*/
@@ -65,6 +67,7 @@ struct STM32F405State {
     STM32F2XXADCState adc[STM_NUM_ADCS];
     STM32F2XXSPIState spi[STM_NUM_SPIS];
 
+    MemoryRegion ccm;
     MemoryRegion sram;
     MemoryRegion flash;
     MemoryRegion flash_alias;
-- 
2.38.1

[PATCH 2/2] hw/arm: Add Olimex H405

[Felipe Balbi]

Olimex makes a series of low-cost STM32 boards. This commit introduces
the minimum setup to support SMT32-H405. See [1] for details

[1] https://www.olimex.com/Products/ARM/ST/STM32-H405/

Signed-off-by: Felipe Balbi <balbi@kernel.org>
---
 MAINTAINERS                             |  6 +++
 configs/devices/arm-softmmu/default.mak |  1 +
 hw/arm/Kconfig                          |  4 ++
 hw/arm/meson.build                      |  1 +
 hw/arm/olimex-stm32-h405.c              | 65 +++++++++++++++++++++++++
 5 files changed, 77 insertions(+)
 create mode 100644 hw/arm/olimex-stm32-h405.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3bd433b65a55..e37846df0071 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1026,6 +1026,12 @@ L: qemu-arm@nongnu.org
 S: Maintained
 F: hw/arm/netduinoplus2.c
 
+Olimex STM32 H405
+M: Felipe Balbi <balbi@kernel.org>
+L: qemu-arm@nongnu.org
+S: Maintained
+F: hw/arm/olimex-stm32-h405.c
+
 SmartFusion2
 M: Subbaraya Sundeep <sundeep.lkml@gmail.com>
 M: Peter Maydell <peter.maydell@linaro.org>
diff --git a/configs/devices/arm-softmmu/default.mak b/configs/devices/arm-softmmu/default.mak
index 6985a25377a0..1b49a7830c7e 100644
--- a/configs/devices/arm-softmmu/default.mak
+++ b/configs/devices/arm-softmmu/default.mak
@@ -30,6 +30,7 @@ CONFIG_COLLIE=y
 CONFIG_ASPEED_SOC=y
 CONFIG_NETDUINO2=y
 CONFIG_NETDUINOPLUS2=y
+CONFIG_OLIMEX_STM32_H405=y
 CONFIG_MPS2=y
 CONFIG_RASPI=y
 CONFIG_DIGIC=y
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 17fcde8e1ccc..9143533ef792 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -119,6 +119,10 @@ config NETDUINOPLUS2
     bool
     select STM32F405_SOC
 
+config OLIMEX_STM32_H405
+    bool
+    select STM32F405_SOC
+
 config NSERIES
     bool
     select OMAP
diff --git a/hw/arm/meson.build b/hw/arm/meson.build
index 92f9f6e000ea..76d4d650e42e 100644
--- a/hw/arm/meson.build
+++ b/hw/arm/meson.build
@@ -12,6 +12,7 @@ arm_ss.add(when: 'CONFIG_MICROBIT', if_true: files('microbit.c'))
 arm_ss.add(when: 'CONFIG_MUSICPAL', if_true: files('musicpal.c'))
 arm_ss.add(when: 'CONFIG_NETDUINO2', if_true: files('netduino2.c'))
 arm_ss.add(when: 'CONFIG_NETDUINOPLUS2', if_true: files('netduinoplus2.c'))
+arm_ss.add(when: 'CONFIG_OLIMEX_STM32_H405', if_true: files('olimex-stm32-h405.c'))
 arm_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx.c', 'npcm7xx_boards.c'))
 arm_ss.add(when: 'CONFIG_NSERIES', if_true: files('nseries.c'))
 arm_ss.add(when: 'CONFIG_SX1', if_true: files('omap_sx1.c'))
diff --git a/hw/arm/olimex-stm32-h405.c b/hw/arm/olimex-stm32-h405.c
new file mode 100644
index 000000000000..5171a66074bb
--- /dev/null
+++ b/hw/arm/olimex-stm32-h405.c
@@ -0,0 +1,65 @@
+/*
+ * ST STM32VLDISCOVERY machine
+ * Olimex STM32-H405 machine
+ *
+ * Copyright (c) 2022 Felipe Balbi <balbi@kernel.org>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/boards.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-clock.h"
+#include "qemu/error-report.h"
+#include "hw/arm/stm32f405_soc.h"
+#include "hw/arm/boot.h"
+
+/* olimex-stm32-h405 implementation is derived from netduinoplus2 */
+
+/* Main SYSCLK frequency in Hz (168MHz) */
+#define SYSCLK_FRQ 168000000ULL
+
+static void olimex_stm32_h405_init(MachineState *machine)
+{
+    DeviceState *dev;
+    Clock *sysclk;
+
+    /* This clock doesn't need migration because it is fixed-frequency */
+    sysclk = clock_new(OBJECT(machine), "SYSCLK");
+    clock_set_hz(sysclk, SYSCLK_FRQ);
+
+    dev = qdev_new(TYPE_STM32F405_SOC);
+    qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m4"));
+    qdev_connect_clock_in(dev, "sysclk", sysclk);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+
+    armv7m_load_kernel(ARM_CPU(first_cpu),
+                       machine->kernel_filename,
+                       0, FLASH_SIZE);
+}
+
+static void olimex_stm32_h405_machine_init(MachineClass *mc)
+{
+    mc->desc = "Olimex STM32-H405 (Cortex-M4)";
+    mc->init = olimex_stm32_h405_init;
+}
+
+DEFINE_MACHINE("olimex-stm32-h405", olimex_stm32_h405_machine_init)
-- 
2.38.1

Re: [PATCH 1/2] hw/arm/stm32f405: correctly describe the memory layout

[Alistair Francis]

On Mon, Dec 19, 2022 at 1:24 AM Felipe Balbi <balbi@kernel.org> wrote:
>
> STM32F405 has 128K of SRAM and another 64K of CCM (Core-coupled
> Memory) at a different base address. Correctly describe the memory
> layout to give existing FW images have a chance to run unmodified.
>
> Signed-off-by: Felipe Balbi <balbi@kernel.org>

Reviewed-by: Alistair Francis <alistair@alistair23.me>

Alistair

> ---
>  hw/arm/stm32f405_soc.c         | 8 ++++++++
>  include/hw/arm/stm32f405_soc.h | 5 ++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/stm32f405_soc.c b/hw/arm/stm32f405_soc.c
> index c07947d9f8b1..cef23d7ee41a 100644
> --- a/hw/arm/stm32f405_soc.c
> +++ b/hw/arm/stm32f405_soc.c
> @@ -139,6 +139,14 @@ static void stm32f405_soc_realize(DeviceState *dev_soc, Error **errp)
>      }
>      memory_region_add_subregion(system_memory, SRAM_BASE_ADDRESS, &s->sram);
>
> +    memory_region_init_ram(&s->ccm, NULL, "STM32F405.ccm", CCM_SIZE,
> +                           &err);
> +    if (err != NULL) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    memory_region_add_subregion(system_memory, CCM_BASE_ADDRESS, &s->ccm);
> +
>      armv7m = DEVICE(&s->armv7m);
>      qdev_prop_set_uint32(armv7m, "num-irq", 96);
>      qdev_prop_set_string(armv7m, "cpu-type", s->cpu_type);
> diff --git a/include/hw/arm/stm32f405_soc.h b/include/hw/arm/stm32f405_soc.h
> index 5bb0c8d56979..249ab5434ec7 100644
> --- a/include/hw/arm/stm32f405_soc.h
> +++ b/include/hw/arm/stm32f405_soc.h
> @@ -46,7 +46,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(STM32F405State, STM32F405_SOC)
>  #define FLASH_BASE_ADDRESS 0x08000000
>  #define FLASH_SIZE (1024 * 1024)
>  #define SRAM_BASE_ADDRESS 0x20000000
> -#define SRAM_SIZE (192 * 1024)
> +#define SRAM_SIZE (128 * 1024)
> +#define CCM_BASE_ADDRESS 0x10000000
> +#define CCM_SIZE (64 * 1024)
>
>  struct STM32F405State {
>      /*< private >*/
> @@ -65,6 +67,7 @@ struct STM32F405State {
>      STM32F2XXADCState adc[STM_NUM_ADCS];
>      STM32F2XXSPIState spi[STM_NUM_SPIS];
>
> +    MemoryRegion ccm;
>      MemoryRegion sram;
>      MemoryRegion flash;
>      MemoryRegion flash_alias;
> --
> 2.38.1
>
>

Re: [PATCH 1/2] hw/arm/stm32f405: correctly describe the memory layout

[Philippe Mathieu-Daudé]

Hi Felipe,

On 18/12/22 08:12, Felipe Balbi wrote:
> STM32F405 has 128K of SRAM and another 64K of CCM (Core-coupled
> Memory) at a different base address. Correctly describe the memory
> layout to give existing FW images have a chance to run unmodified.
> 
> Signed-off-by: Felipe Balbi <balbi@kernel.org>
> ---
>   hw/arm/stm32f405_soc.c         | 8 ++++++++
>   include/hw/arm/stm32f405_soc.h | 5 ++++-
>   2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/stm32f405_soc.c b/hw/arm/stm32f405_soc.c
> index c07947d9f8b1..cef23d7ee41a 100644
> --- a/hw/arm/stm32f405_soc.c
> +++ b/hw/arm/stm32f405_soc.c
> @@ -139,6 +139,14 @@ static void stm32f405_soc_realize(DeviceState *dev_soc, Error **errp)
>       }
>       memory_region_add_subregion(system_memory, SRAM_BASE_ADDRESS, &s->sram);
>   
> +    memory_region_init_ram(&s->ccm, NULL, "STM32F405.ccm", CCM_SIZE,

Including the machine name in the memory description seems a bad
habit from old days. What do you think about renaming as
'core-coupled-memory'?

> +                           &err);
> +    if (err != NULL) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    memory_region_add_subregion(system_memory, CCM_BASE_ADDRESS, &s->ccm);
> +
>       armv7m = DEVICE(&s->armv7m);
>       qdev_prop_set_uint32(armv7m, "num-irq", 96);
>       qdev_prop_set_string(armv7m, "cpu-type", s->cpu_type);
> diff --git a/include/hw/arm/stm32f405_soc.h b/include/hw/arm/stm32f405_soc.h
> index 5bb0c8d56979..249ab5434ec7 100644
> --- a/include/hw/arm/stm32f405_soc.h
> +++ b/include/hw/arm/stm32f405_soc.h
> @@ -46,7 +46,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(STM32F405State, STM32F405_SOC)
>   #define FLASH_BASE_ADDRESS 0x08000000
>   #define FLASH_SIZE (1024 * 1024)
>   #define SRAM_BASE_ADDRESS 0x20000000
> -#define SRAM_SIZE (192 * 1024)
> +#define SRAM_SIZE (128 * 1024)
> +#define CCM_BASE_ADDRESS 0x10000000
> +#define CCM_SIZE (64 * 1024)

Since the CCM_SIZE won't be used elsewhere, we can simply use '64 * KiB'
in the memory_region_init_ram() in the source file. Up to the maintainer
:)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/2] hw/arm: Add Olimex H405

[Philippe Mathieu-Daudé]

On 18/12/22 08:12, Felipe Balbi wrote:
> Olimex makes a series of low-cost STM32 boards. This commit introduces
> the minimum setup to support SMT32-H405. See [1] for details
> 
> [1] https://www.olimex.com/Products/ARM/ST/STM32-H405/
> 
> Signed-off-by: Felipe Balbi <balbi@kernel.org>
> ---
>   MAINTAINERS                             |  6 +++
>   configs/devices/arm-softmmu/default.mak |  1 +
>   hw/arm/Kconfig                          |  4 ++
>   hw/arm/meson.build                      |  1 +
>   hw/arm/olimex-stm32-h405.c              | 65 +++++++++++++++++++++++++
>   5 files changed, 77 insertions(+)
>   create mode 100644 hw/arm/olimex-stm32-h405.c

See also docs/system/arm/stm32.rst

> diff --git a/hw/arm/meson.build b/hw/arm/meson.build
> index 92f9f6e000ea..76d4d650e42e 100644
> --- a/hw/arm/meson.build
> +++ b/hw/arm/meson.build
> @@ -12,6 +12,7 @@ arm_ss.add(when: 'CONFIG_MICROBIT', if_true: files('microbit.c'))
>   arm_ss.add(when: 'CONFIG_MUSICPAL', if_true: files('musicpal.c'))
>   arm_ss.add(when: 'CONFIG_NETDUINO2', if_true: files('netduino2.c'))
>   arm_ss.add(when: 'CONFIG_NETDUINOPLUS2', if_true: files('netduinoplus2.c'))
> +arm_ss.add(when: 'CONFIG_OLIMEX_STM32_H405', if_true: files('olimex-stm32-h405.c'))
>   arm_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx.c', 'npcm7xx_boards.c'))
>   arm_ss.add(when: 'CONFIG_NSERIES', if_true: files('nseries.c'))
>   arm_ss.add(when: 'CONFIG_SX1', if_true: files('omap_sx1.c'))
> diff --git a/hw/arm/olimex-stm32-h405.c b/hw/arm/olimex-stm32-h405.c
> new file mode 100644
> index 000000000000..5171a66074bb
> --- /dev/null
> +++ b/hw/arm/olimex-stm32-h405.c
> @@ -0,0 +1,65 @@
> +/*
> + * ST STM32VLDISCOVERY machine
> + * Olimex STM32-H405 machine
> + *
> + * Copyright (c) 2022 Felipe Balbi <balbi@kernel.org>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "hw/boards.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-clock.h"
> +#include "qemu/error-report.h"
> +#include "hw/arm/stm32f405_soc.h"
> +#include "hw/arm/boot.h"
> +
> +/* olimex-stm32-h405 implementation is derived from netduinoplus2 */
> +
> +/* Main SYSCLK frequency in Hz (168MHz) */
> +#define SYSCLK_FRQ 168000000ULL

No need to comment if named MAIN_SYSCLK_FREQ_HZ.

> +static void olimex_stm32_h405_init(MachineState *machine)
> +{
> +    DeviceState *dev;
> +    Clock *sysclk;

I like the simplicity, but wonder if we shouldn't add checks such
the MPS2 machines, in case one use different -cpu / -m values:

        if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) {
            error_report("This board can only be used with CPU %s",
                         mc->default_cpu_type);
            exit(1);
        }

        if (machine->ram_size != mc->default_ram_size) {
            g_autofree char *sz = size_to_str(mc->default_ram_size);
            error_report("Invalid RAM size, should be %s", sz);
            exit(1);
        }

Or maybe better would be to refactor that since this pattern is common
to SoC/SoM.

> +    /* This clock doesn't need migration because it is fixed-frequency */
> +    sysclk = clock_new(OBJECT(machine), "SYSCLK");
> +    clock_set_hz(sysclk, SYSCLK_FRQ);
> +
> +    dev = qdev_new(TYPE_STM32F405_SOC);
> +    qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m4"));
> +    qdev_connect_clock_in(dev, "sysclk", sysclk);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +
> +    armv7m_load_kernel(ARM_CPU(first_cpu),
> +                       machine->kernel_filename,
> +                       0, FLASH_SIZE);
> +}
> +
> +static void olimex_stm32_h405_machine_init(MachineClass *mc)
> +{
> +    mc->desc = "Olimex STM32-H405 (Cortex-M4)";
> +    mc->init = olimex_stm32_h405_init;

Per previous comment:

        mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m4");
        mc->default_ram_size = 0; /* SRAM already allocated in SoC */

> +}
> +
> +DEFINE_MACHINE("olimex-stm32-h405", olimex_stm32_h405_machine_init)

Re: [PATCH 1/2] hw/arm/stm32f405: correctly describe the memory layout

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2465 bytes --]


Hi,

Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> On 18/12/22 08:12, Felipe Balbi wrote:
>> STM32F405 has 128K of SRAM and another 64K of CCM (Core-coupled
>> Memory) at a different base address. Correctly describe the memory
>> layout to give existing FW images have a chance to run unmodified.
>> 
>> Signed-off-by: Felipe Balbi <balbi@kernel.org>
>> ---
>>   hw/arm/stm32f405_soc.c         | 8 ++++++++
>>   include/hw/arm/stm32f405_soc.h | 5 ++++-
>>   2 files changed, 12 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/arm/stm32f405_soc.c b/hw/arm/stm32f405_soc.c
>> index c07947d9f8b1..cef23d7ee41a 100644
>> --- a/hw/arm/stm32f405_soc.c
>> +++ b/hw/arm/stm32f405_soc.c
>> @@ -139,6 +139,14 @@ static void stm32f405_soc_realize(DeviceState *dev_soc, Error **errp)
>>       }
>>       memory_region_add_subregion(system_memory, SRAM_BASE_ADDRESS, &s->sram);
>>   
>> +    memory_region_init_ram(&s->ccm, NULL, "STM32F405.ccm", CCM_SIZE,
>
> Including the machine name in the memory description seems a bad
> habit from old days. What do you think about renaming as
> 'core-coupled-memory'?

I don't oppose it, but I was merely following the model from
`netduino2plus'.

>> +                           &err);
>> +    if (err != NULL) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +    memory_region_add_subregion(system_memory, CCM_BASE_ADDRESS, &s->ccm);
>> +
>>       armv7m = DEVICE(&s->armv7m);
>>       qdev_prop_set_uint32(armv7m, "num-irq", 96);
>>       qdev_prop_set_string(armv7m, "cpu-type", s->cpu_type);
>> diff --git a/include/hw/arm/stm32f405_soc.h b/include/hw/arm/stm32f405_soc.h
>> index 5bb0c8d56979..249ab5434ec7 100644
>> --- a/include/hw/arm/stm32f405_soc.h
>> +++ b/include/hw/arm/stm32f405_soc.h
>> @@ -46,7 +46,9 @@ OBJECT_DECLARE_SIMPLE_TYPE(STM32F405State, STM32F405_SOC)
>>   #define FLASH_BASE_ADDRESS 0x08000000
>>   #define FLASH_SIZE (1024 * 1024)
>>   #define SRAM_BASE_ADDRESS 0x20000000
>> -#define SRAM_SIZE (192 * 1024)
>> +#define SRAM_SIZE (128 * 1024)
>> +#define CCM_BASE_ADDRESS 0x10000000
>> +#define CCM_SIZE (64 * 1024)
>
> Since the CCM_SIZE won't be used elsewhere, we can simply use '64 * KiB'
> in the memory_region_init_ram() in the source file. Up to the maintainer
> :)

Also not opposed. I'll wait a couple days before respinning the patch to
give the rest of the community time to reply.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH 2/2] hw/arm: Add Olimex H405

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 5841 bytes --]


Hi,

Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> On 18/12/22 08:12, Felipe Balbi wrote:
>> Olimex makes a series of low-cost STM32 boards. This commit introduces
>> the minimum setup to support SMT32-H405. See [1] for details
>> 
>> [1] https://www.olimex.com/Products/ARM/ST/STM32-H405/
>> 
>> Signed-off-by: Felipe Balbi <balbi@kernel.org>
>> ---
>>   MAINTAINERS                             |  6 +++
>>   configs/devices/arm-softmmu/default.mak |  1 +
>>   hw/arm/Kconfig                          |  4 ++
>>   hw/arm/meson.build                      |  1 +
>>   hw/arm/olimex-stm32-h405.c              | 65 +++++++++++++++++++++++++
>>   5 files changed, 77 insertions(+)
>>   create mode 100644 hw/arm/olimex-stm32-h405.c
>
> See also docs/system/arm/stm32.rst

Added an entry, thanks

>> diff --git a/hw/arm/meson.build b/hw/arm/meson.build
>> index 92f9f6e000ea..76d4d650e42e 100644
>> --- a/hw/arm/meson.build
>> +++ b/hw/arm/meson.build
>> @@ -12,6 +12,7 @@ arm_ss.add(when: 'CONFIG_MICROBIT', if_true: files('microbit.c'))
>>   arm_ss.add(when: 'CONFIG_MUSICPAL', if_true: files('musicpal.c'))
>>   arm_ss.add(when: 'CONFIG_NETDUINO2', if_true: files('netduino2.c'))
>>   arm_ss.add(when: 'CONFIG_NETDUINOPLUS2', if_true: files('netduinoplus2.c'))
>> +arm_ss.add(when: 'CONFIG_OLIMEX_STM32_H405', if_true: files('olimex-stm32-h405.c'))
>>   arm_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx.c', 'npcm7xx_boards.c'))
>>   arm_ss.add(when: 'CONFIG_NSERIES', if_true: files('nseries.c'))
>>   arm_ss.add(when: 'CONFIG_SX1', if_true: files('omap_sx1.c'))
>> diff --git a/hw/arm/olimex-stm32-h405.c b/hw/arm/olimex-stm32-h405.c
>> new file mode 100644
>> index 000000000000..5171a66074bb
>> --- /dev/null
>> +++ b/hw/arm/olimex-stm32-h405.c
>> @@ -0,0 +1,65 @@
>> +/*
>> + * ST STM32VLDISCOVERY machine
>> + * Olimex STM32-H405 machine
>> + *
>> + * Copyright (c) 2022 Felipe Balbi <balbi@kernel.org>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "hw/boards.h"
>> +#include "hw/qdev-properties.h"
>> +#include "hw/qdev-clock.h"
>> +#include "qemu/error-report.h"
>> +#include "hw/arm/stm32f405_soc.h"
>> +#include "hw/arm/boot.h"
>> +
>> +/* olimex-stm32-h405 implementation is derived from netduinoplus2 */
>> +
>> +/* Main SYSCLK frequency in Hz (168MHz) */
>> +#define SYSCLK_FRQ 168000000ULL
>
> No need to comment if named MAIN_SYSCLK_FREQ_HZ.

Heh, came verbatim from netduino2plus.

>> +static void olimex_stm32_h405_init(MachineState *machine)
>> +{
>> +    DeviceState *dev;
>> +    Clock *sysclk;
>
> I like the simplicity, but wonder if we shouldn't add checks such
> the MPS2 machines, in case one use different -cpu / -m values:
>
>         if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) {
>             error_report("This board can only be used with CPU %s",
>                          mc->default_cpu_type);
>             exit(1);
>         }
>
>         if (machine->ram_size != mc->default_ram_size) {
>             g_autofree char *sz = size_to_str(mc->default_ram_size);
>             error_report("Invalid RAM size, should be %s", sz);
>             exit(1);
>         }

yeah, I see where you're coming from, but leaving those flexible might
be useful in case e.g. Olimex wants to respin the board with a slightly
different SoC and would like to very, before building HW, that their SW
is still valid.

I don't have a strong opinion either way, though.

> Or maybe better would be to refactor that since this pattern is common
> to SoC/SoM.

refactoring should be done as a separate patchset, IMHO.

>> +    /* This clock doesn't need migration because it is fixed-frequency */
>> +    sysclk = clock_new(OBJECT(machine), "SYSCLK");
>> +    clock_set_hz(sysclk, SYSCLK_FRQ);
>> +
>> +    dev = qdev_new(TYPE_STM32F405_SOC);
>> +    qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m4"));
>> +    qdev_connect_clock_in(dev, "sysclk", sysclk);
>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> +
>> +    armv7m_load_kernel(ARM_CPU(first_cpu),
>> +                       machine->kernel_filename,
>> +                       0, FLASH_SIZE);
>> +}
>> +
>> +static void olimex_stm32_h405_machine_init(MachineClass *mc)
>> +{
>> +    mc->desc = "Olimex STM32-H405 (Cortex-M4)";
>> +    mc->init = olimex_stm32_h405_init;
>
> Per previous comment:
>
>         mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m4");
>         mc->default_ram_size = 0; /* SRAM already allocated in SoC */

I can add these, no problem.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]