On Wed, 26 Jul 2023 at 16:54, Jean-Christophe Dubois
<j...@tribudubois.net> wrote:
>
> From: jcdubois <j...@tribudubois.net>
>
> The SRC device is normaly used to start the secondary CPU.
>
> When running Linux directly, Qemu is emulating a PSCI interface that UBOOT
> is installing at boot time and therefore the fact that the SRC device is
> unimplemented is hidden as Qemu respond directly to PSCI requets without
> using the SRC device.
>
> But if you try to run a more bare metal application (maybe uboot itself),
> then it is not possible to start the secondary CPU as the SRC is an
> unimplemented device.
>
> This patch adds the ability to start the secondary CPU through the SRC
> device so that you can use this feature in bare metal application.
>
> Signed-off-by: jcdubois <j...@tribudubois.net>
I have a few style-type comments below, but overall this looks good.
> ---
> hw/arm/fsl-imx7.c | 9 +-
> hw/misc/imx7_src.c | 289 +++++++++++++++++++++++++++++++++++++
> hw/misc/meson.build | 1 +
> include/hw/arm/fsl-imx7.h | 2 +
> include/hw/misc/imx7_src.h | 68 +++++++++
> 5 files changed, 368 insertions(+), 1 deletion(-)
> create mode 100644 hw/misc/imx7_src.c
> create mode 100644 include/hw/misc/imx7_src.h
>
> diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
> index 05cdd4831e..db103069e1 100644
> --- a/hw/arm/fsl-imx7.c
> +++ b/hw/arm/fsl-imx7.c
> @@ -82,6 +82,11 @@ static void fsl_imx7_init(Object *obj)
> */
> object_initialize_child(obj, "gpcv2", &s->gpcv2, TYPE_IMX_GPCV2);
>
> + /*
> + * SRC
> + */
> + object_initialize_child(obj, "src", &s->src, TYPE_IMX7_SRC);
> +
> /*
> * ECSPIs
> */
> @@ -90,6 +95,7 @@ static void fsl_imx7_init(Object *obj)
> object_initialize_child(obj, name, &s->spi[i], TYPE_IMX_SPI);
> }
>
> +
> /*
> * I2Cs
> */
Stray whitespace change.
> @@ -490,7 +496,8 @@ static void fsl_imx7_realize(DeviceState *dev, Error
> **errp)
> /*
> * SRC
> */
> - create_unimplemented_device("src", FSL_IMX7_SRC_ADDR, FSL_IMX7_SRC_SIZE);
> + sysbus_realize(SYS_BUS_DEVICE(&s->src), &error_abort);
> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->src), 0, FSL_IMX7_SRC_ADDR);
>
> /*
> * Watchdogs
> +#define DPRINTF(fmt, args...) \
> + do { \
> + if (DEBUG_IMX7_SRC) { \
> + fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX7_SRC, \
> + __func__, ##args); \
> + } \
> + } while (0)
Please don't add DEBUG/DPRINTF macros in new code. Use
tracepoints instead.
> +#define EXTRACT(value, name) extract32(value, name##_SHIFT, name##_LENGTH)
Please don't define new wrappers around extract32() and
friends. If you want to have something name based, use
FIELD_EX32() from hw/registerfields.h.
thanks
-- PMM
