Re: [PATCH 06/21] hw/arm/fsl-imx8mp: Implement clock tree

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 20 Jan 2025 at 20:38, Bernhard Beschow <shentey@gmail.com> wrote:
>
> Fixes quite a few stack traces during the Linux boot process. Also provides the
> clocks for devices added later, e.g. enet1.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  MAINTAINERS                    |   2 +
>  docs/system/arm/imx8mp-evk.rst |   1 +
>  include/hw/arm/fsl-imx8mp.h    |   3 +
>  include/hw/misc/imx8mp_ccm.h   |  97 ++++++++++
>  hw/arm/fsl-imx8mp.c            |  20 +++
>  hw/misc/imx8mp_ccm.c           | 315 +++++++++++++++++++++++++++++++++
>  hw/misc/meson.build            |   1 +
>  7 files changed, 439 insertions(+)
>  create mode 100644 include/hw/misc/imx8mp_ccm.h
>  create mode 100644 hw/misc/imx8mp_ccm.c
> diff --git a/include/hw/misc/imx8mp_ccm.h b/include/hw/misc/imx8mp_ccm.h
> new file mode 100644
> index 0000000000..2378c157de
> --- /dev/null
> +++ b/include/hw/misc/imx8mp_ccm.h
> @@ -0,0 +1,97 @@
> +/*
> + * Copyright (c) 2025 Bernhard Beschow <shentey@gmail.com>
> + *
> + * i.MX8MP CCM, ANALOG IP blocks emulation code
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef IMX8MP_CCM_H
> +#define IMX8MP_CCM_H
> +
> +#include "hw/misc/imx_ccm.h"
> +#include "qom/object.h"
> +
> +enum IMX8MPAnalogRegisters {
> +    ANALOG_AUDIO_PLL1_GEN_CTRL = 0x000 / 4,
> +    ANALOG_AUDIO_PLL1_FDIV_CTL0 = 0x004 / 4,
> +    ANALOG_AUDIO_PLL1_FDIV_CTL1 = 0x008 / 4,
> +    ANALOG_AUDIO_PLL1_SSCG_CTRL = 0x00c / 4,
> +    ANALOG_AUDIO_PLL1_MNIT_CTRL = 0x010 / 4,
> +    ANALOG_AUDIO_PLL2_GEN_CTRL = 0x014 / 4,
> +    ANALOG_AUDIO_PLL2_FDIV_CTL0 = 0x018 / 4,
> +    ANALOG_AUDIO_PLL2_FDIV_CTL1 = 0x01c / 4,
> +    ANALOG_AUDIO_PLL2_SSCG_CTRL = 0x020 / 4,
> +    ANALOG_AUDIO_PLL2_MNIT_CTRL = 0x024 / 4,
> +    ANALOG_VIDEO_PLL1_GEN_CTRL = 0x028 / 4,
> +    ANALOG_VIDEO_PLL1_FDIV_CTL0 = 0x02c / 4,
> +    ANALOG_VIDEO_PLL1_FDIV_CTL1 = 0x030 / 4,
> +    ANALOG_VIDEO_PLL1_SSCG_CTRL = 0x034 / 4,
> +    ANALOG_VIDEO_PLL1_MNIT_CTRL = 0x038 / 4,
> +    ANALOG_DRAM_PLL_GEN_CTRL = 0x050 / 4,
> +    ANALOG_DRAM_PLL_FDIV_CTL0 = 0x054 / 4,
> +    ANALOG_DRAM_PLL_FDIV_CTL1 = 0x058 / 4,
> +    ANALOG_DRAM_PLL_SSCG_CTRL = 0x05c / 4,
> +    ANALOG_DRAM_PLL_MNIT_CTRL = 0x060 / 4,
> +    ANALOG_GPU_PLL_GEN_CTRL = 0x064 / 4,
> +    ANALOG_GPU_PLL_FDIV_CTL0 = 0x068 / 4,
> +    ANALOG_GPU_PLL_LOCKD_CTRL = 0x06c / 4,
> +    ANALOG_GPU_PLL_MNIT_CTRL = 0x070 / 4,
> +    ANALOG_VPU_PLL_GEN_CTRL = 0x074 / 4,
> +    ANALOG_VPU_PLL_FDIV_CTL0 = 0x078 / 4,
> +    ANALOG_VPU_PLL_LOCKD_CTRL = 0x07c / 4,
> +    ANALOG_VPU_PLL_MNIT_CTRL = 0x080 / 4,
> +    ANALOG_ARM_PLL_GEN_CTRL = 0x084 / 4,
> +    ANALOG_ARM_PLL_FDIV_CTL0 = 0x088 / 4,
> +    ANALOG_ARM_PLL_LOCKD_CTRL = 0x08c / 4,
> +    ANALOG_ARM_PLL_MNIT_CTRL = 0x090 / 4,
> +    ANALOG_SYS_PLL1_GEN_CTRL = 0x094 / 4,
> +    ANALOG_SYS_PLL1_FDIV_CTL0 = 0x098 / 4,
> +    ANALOG_SYS_PLL1_LOCKD_CTRL = 0x09c / 4,
> +    ANALOG_SYS_PLL1_MNIT_CTRL = 0x100 / 4,
> +    ANALOG_SYS_PLL2_GEN_CTRL = 0x104 / 4,
> +    ANALOG_SYS_PLL2_FDIV_CTL0 = 0x108 / 4,
> +    ANALOG_SYS_PLL2_LOCKD_CTRL = 0x10c / 4,
> +    ANALOG_SYS_PLL2_MNIT_CTRL = 0x110 / 4,
> +    ANALOG_SYS_PLL3_GEN_CTRL = 0x114 / 4,
> +    ANALOG_SYS_PLL3_FDIV_CTL0 = 0x118 / 4,
> +    ANALOG_SYS_PLL3_LOCKD_CTRL = 0x11c / 4,
> +    ANALOG_SYS_PLL3_MNIT_CTRL = 0x120 / 4,
> +    ANALOG_OSC_MISC_CFG = 0x124 / 4,
> +    ANALOG_ANAMIX_PLL_MNIT_CTL = 0x128 / 4,
> +
> +    ANALOG_DIGPROG = 0x800 / 4,
> +    ANALOG_MAX,
> +};
> +
> +enum IMX8MPCCMRegisters {
> +    CCM_MAX = 0xc6fc / sizeof(uint32_t) + 1,
> +};
> +
> +#define TYPE_IMX8MP_CCM "imx8mp.ccm"
> +OBJECT_DECLARE_SIMPLE_TYPE(IMX8MPCCMState, IMX8MP_CCM)
> +
> +struct IMX8MPCCMState {
> +    IMXCCMState parent_obj;
> +
> +    MemoryRegion iomem;
> +
> +    uint32_t ccm[CCM_MAX];
> +};
> +
> +
> +#define TYPE_IMX8MP_ANALOG "imx8mp.analog"
> +OBJECT_DECLARE_SIMPLE_TYPE(IMX8MPAnalogState, IMX8MP_ANALOG)
> +
> +struct IMX8MPAnalogState {
> +    IMXCCMState parent_obj;
> +
> +    struct {
> +        MemoryRegion container;
> +        MemoryRegion analog;
> +    } mmio;
> +
> +    uint32_t analog[ANALOG_MAX];
> +};
> +
> +#endif /* IMX8MP_CCM_H */

This seems to be implementing two separate devices in a single
source file. Generally we prefer one device per file. Is
there a reason they can't be split?

thanks
-- PMM