On Sat, 10 May 2025 at 07:58, <jc...@duck.com> wrote:
>
> This patch implements the Instruction Cache Controller for the MAX78000 SOC
>
> Signed-off-by: Jackson Donaldson <jc...@duck.com>
> ---
> hw/arm/Kconfig | 1 +
> hw/arm/max78000_soc.c | 20 ++++++--
> hw/misc/Kconfig | 3 ++
> hw/misc/max78000_icc.c | 89 ++++++++++++++++++++++++++++++++++
> hw/misc/meson.build | 1 +
> include/hw/arm/max78000_soc.h | 6 +++
> include/hw/misc/max78000_icc.h | 34 +++++++++++++
> 7 files changed, 150 insertions(+), 4 deletions(-)
> create mode 100644 hw/misc/max78000_icc.c
> create mode 100644 include/hw/misc/max78000_icc.h
We generally prefer to split into two patches,
one for "just add the new device" and then a separate
one for "instantiate the new device in the SoC".
(This device is quite small, but for larger ones it
helps to keep the patchsize down.)
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 1c365d1115..3f23af3244 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -367,6 +367,7 @@ config ALLWINNER_R40
> config MAX78000_SOC
> bool
> select ARM_V7M
> + select MAX78000_ICC
>
> config RASPI
> bool
> diff --git a/hw/arm/max78000_soc.c b/hw/arm/max78000_soc.c
> index 64578438bd..4d598bddd4 100644
> --- a/hw/arm/max78000_soc.c
> +++ b/hw/arm/max78000_soc.c
> @@ -16,12 +16,19 @@
> #include "hw/qdev-clock.h"
> #include "hw/misc/unimp.h"
>
> +static const uint32_t max78000_icc_addr[] = {0x4002a000, 0x4002a800};
> +
> static void max78000_soc_initfn(Object *obj)
> {
> MAX78000State *s = MAX78000_SOC(obj);
> + int i;
>
> object_initialize_child(obj, "armv7m", &s->armv7m, TYPE_ARMV7M);
>
> + for (i = 0; i < MAX78000_NUM_ICC; i++) {
> + object_initialize_child(obj, "icc[*]", &s->icc[i],
> TYPE_MAX78000_ICC);
> + }
> +
> s->sysclk = qdev_init_clock_in(DEVICE(s), "sysclk", NULL, NULL, 0);
> s->refclk = qdev_init_clock_in(DEVICE(s), "refclk", NULL, NULL, 0);
> }
> @@ -30,8 +37,9 @@ static void max78000_soc_realize(DeviceState *dev_soc,
> Error **errp)
> {
> MAX78000State *s = MAX78000_SOC(dev_soc);
> MemoryRegion *system_memory = get_system_memory();
> - DeviceState *armv7m;
> + DeviceState *dev, *armv7m;
> Error *err = NULL;
> + int i;
>
> /*
> * We use s->refclk internally and only define it with
> qdev_init_clock_in()
> @@ -87,6 +95,13 @@ static void max78000_soc_realize(DeviceState *dev_soc,
> Error **errp)
> return;
> }
>
> + for (i = 0; i < MAX78000_NUM_ICC; i++) {
> + dev = DEVICE(&(s->icc[i]));
> + sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);
You want just sysbus_realize() here. There are two patterns:
- when creating a device via qdev_new() or something similar,
realize it with a *_realize_and_unref() function
- when initializing an object in place with object_initialize_child(),
realize it with a *_realize() function
(This is because object_initialize_child() has already arranged
for the only reference to the child device to be held by the
parent via the child<> property, so we don't want to deref it
again.)
> + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, max78000_icc_addr[i]);
> + }
> +
> +
> create_unimplemented_device("globalControl", 0x40000000, 0x400);
> create_unimplemented_device("systemInterface", 0x40000400, 0x400);
> create_unimplemented_device("functionControl", 0x40000800, 0x3400);
> @@ -120,9 +135,6 @@ static void max78000_soc_realize(DeviceState *dev_soc,
> Error **errp)
> create_unimplemented_device("standardDMA", 0x40028000, 0x1000);
> create_unimplemented_device("flashController0", 0x40029000, 0x400);
>
> - create_unimplemented_device("icc0", 0x4002a000, 0x800);
> - create_unimplemented_device("icc1", 0x4002a800, 0x800);
> -
> create_unimplemented_device("adc", 0x40034000, 0x1000);
> create_unimplemented_device("pulseTrainEngine", 0x4003c000, 0xa0);
> create_unimplemented_device("oneWireMaster", 0x4003d000, 0x1000);
> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> index ec0fa5aa9f..781bcf74cc 100644
> --- a/hw/misc/Kconfig
> +++ b/hw/misc/Kconfig
> @@ -47,6 +47,9 @@ config A9SCU
> config ARM11SCU
> bool
>
> +config MAX78000_ICC
> + bool
> +
> config MOS6522
> bool
>
> diff --git a/hw/misc/max78000_icc.c b/hw/misc/max78000_icc.c
> new file mode 100644
> index 0000000000..3eacf6bd1b
> --- /dev/null
> +++ b/hw/misc/max78000_icc.c
> @@ -0,0 +1,89 @@
> +/*
> + * MAX78000 Instruction Cache
> + *
> + * Copyright (c) 2025 Jackson Donaldson <jc...@duck.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "trace.h"
> +#include "hw/irq.h"
> +#include "migration/vmstate.h"
> +#include "hw/misc/max78000_icc.h"
> +
> +
> +static uint64_t max78000_icc_read(void *opaque, hwaddr addr,
> + unsigned int size)
> +{
> + Max78000IccState *s = opaque;
> + switch (addr) {
> + case ICC_INFO:{
> + return s->info;
> + }
Your switch case indentation seems a bit odd. Our usual
style puts the "case" at the same indent level as "switch".
Also, you don't need braces on cases unless they are
required to define the scope of a local variable.
So none of these need them.
> + case ICC_SZ:{
> + return s->sz;
> + }
> + case ICC_CTRL:{
> + return s->ctrl;
> + }
> + case ICC_INVALIDATE:{
> + return s->invalidate;
> + }
ICCn_INVALIDATE is write-only; it doesn't need a state
field in the device state struct. It doesn't need a
case in the read function, we can let that drop into
the default case.
> + default:{
> + return 0;
For "there is no register at this offset", use:
default:
qemu_log_mask(LOG_GUEST_ERROR,
"%s: bad offset 0x%" HWADDR_PRIx "\n",
__func__, addr);
return 0;
(Similarly for write.)
> + }
> + }
> +}
> +
> +static void max78000_icc_write(void *opaque, hwaddr addr,
> + uint64_t val64, unsigned int size)
> +{
> + Max78000IccState *s = opaque;
> +
> + switch (addr) {
> + case ICC_CTRL:{
> + s->ctrl = 0x10000 + (val64 & 1);
I prefer "|" over "+" for this kind of thing, as we are
assembling bit fields, not doing arithmetic.
> + break;
> + }
> + case ICC_INVALIDATE:{
> + s->ctrl = s->ctrl | 0x80;
What is this doing? The datasheet says bit 7 in ICCn_CTRL
is reserved.
> + }
A default case logging the guest error would be good.
> + }
> +}
> +
> +static const MemoryRegionOps stm32f4xx_exti_ops = {
> + .read = max78000_icc_read,
> + .write = max78000_icc_write,
I think we should also specify
.valid.min_access_size = 4,
.valid.max_access_size = 4,
which will mean the guest gets an exception if it tries
to do a byte read to these 32-bit registers. Otherwise
we would have to put in extra complexity to handle the
byte and halfword access cases. (QEMU can try to
synthesize these for you but its default is not
necessarily what you need for all devices.)
> + .endianness = DEVICE_NATIVE_ENDIAN,
We're trying to reduce the use of DEVICE_NATIVE_ENDIAN,
so use DEVICE_LITTLE_ENDIAN here, please. (For an Arm
machine type, the two are the same thing.)
> +};
> +
> +static void max78000_icc_init(Object *obj)
> +{
> + Max78000IccState *s = MAX78000_ICC(obj);
> + s->info = 0;
> + s->sz = 0x10000010;
> + s->ctrl = 0x10000;
> + s->invalidate = 0;
These look like reset values. Reset of registers needs to
be done in a reset method, not in init.
> +
> +
> + memory_region_init_io(&s->mmio, obj, &stm32f4xx_exti_ops, s,
> + TYPE_MAX78000_ICC, 0x800);
> + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
> +
> +}
You also need to support migration, which means having
a VMStateDescription that defines what the guest-visible
state of your device is, and setting dc->vmsd to point to it
in your device's class init. For more info see
https://www.qemu.org/docs/master/devel/migration/main.html
and look at other devices in the source tree for examples.
thanks
-- PMM
