Re: [PATCH 3/3] Add i.MX7 SRC device implementation

[Peter Maydell]

On Wed, 26 Jul 2023 at 16:54, Jean-Christophe Dubois
<jcd@tribudubois.net> wrote:
>
> From: jcdubois <jcd@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 <jcd@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