On Thu, 4 Dec 2025 at 09:35, Corvin Köhne <[email protected]> wrote:
>
> From: YannickV <[email protected]>
>
> A dummy DDR controller for ZYNQ has been added. While all registers are
> present,
> not all are functional. Read and write access is validated, and the user mode
> can be set. This provides a basic DDR controller initialization, preventing
> system hangs due to endless polling or similar issues.
>
> Signed-off-by: YannickV <[email protected]>
I have a few minor review comments below, but this otherwise
looks good to me.
> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> index fccd735c24..3de37c9e1d 100644
> --- a/hw/misc/Kconfig
> +++ b/hw/misc/Kconfig
> @@ -240,4 +240,7 @@ config IOSB
> config XLNX_VERSAL_TRNG
> bool
>
> +config XLNX_ZYNQ_DDRC
> + bool
> +
> source macio/Kconfig
You also need to have the "config ZYNQ" section in hw/arm/Kconfig
have a line "select XLNX_ZYNQ_DDRC", to tell kconfig that the
Zynq SoC needs this device.
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index b1d8d8e5d2..ffbcca9796 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -95,6 +95,7 @@ system_ss.add(when: 'CONFIG_RASPI', if_true: files(
> ))
> system_ss.add(when: 'CONFIG_SLAVIO', if_true: files('slavio_misc.c'))
> system_ss.add(when: 'CONFIG_ZYNQ', if_true: files('zynq_slcr.c'))
> +system_ss.add(when: 'CONFIG_ZYNQ', if_true: files('xlnx-zynq-ddrc.c'))
This should be "when: 'CONFIG_XLNX_ZYNQ_DDRC'", so we build the
source file exactly when some board or SoC says it needs the device.
(This mistake is what is hiding the effects of missing the
"select XLNX_ZYNQ_DDRC".)
> system_ss.add(when: 'CONFIG_XLNX_ZYNQMP_ARM', if_true:
> files('xlnx-zynqmp-crf.c'))
> system_ss.add(when: 'CONFIG_XLNX_ZYNQMP_ARM', if_true:
> files('xlnx-zynqmp-apu-ctrl.c'))
> system_ss.add(when: 'CONFIG_XLNX_VERSAL', if_true: files(
> +static const MemoryRegionOps ddrctrl_ops = {
> + .read = register_read_memory,
> + .write = register_write_memory,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> +};
It's better to explicitly set the access size controls
.valid.min_access_size
.valid.max_access_size
.impl.min_access_size
.impl.max_access_size
explicitly, even if they happen to match the defaults. This tells
the reader you've thought about what the device actually permits
and what your implementation code can deal with.
(The default is "accept anything, never generate external abort"
and "assume the read and write functions handle all widths".
This is usually not what actual devices do or what the code
people write is intended to accept. Devices on Arm systems are
often "only 32-bit accesses are permitted".)
> +#define TYPE_DDRCTRL "zynq.ddr-ctlr"
> +#define DDRCTRL(obj) \
> + OBJECT_CHECK(DDRCTRLState, (obj), TYPE_DDRCTRL)
Current QEMU style prefers using one of the helper
macros to generate the cast macros etc. For this kind of
"simple leaf class" OBJECT_DECLARE_SIMPLE_TYPE is probably
what you want. See for some more info:
https://www.qemu.org/docs/master/devel/qom.html#standard-type-declaration-and-definition-macros
There is also OBJECT_DEFINE_SIMPLE_TYPE which provides
the boilerplate TypeInfo and type registration in the .c
file for you, but that one is more "optional if you
prefer it".
thanks
-- PMM
