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

[Bernhard Beschow]

Am 28. Januar 2025 14:35:14 UTC schrieb 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?

I took inspiration from i.mx7 where these two are also implemented in one file, 
presumably because both share some code. This isn't the case here so I'm more 
than happy to split.

Best regards,
Bernhard

>
>thanks
>-- PMM