Re: [PATCH] hw/misc/imx6_ccm: add the i.MX6SLL variant

On Mon, 3 Aug 2026 at 20:36, Pablo Mazzini <[email protected]> wrote:
>
> The i.MX6SLL shares the i.MX6Q clock tree, but reports its own chip id
> in the ANATOP DIGPROG register and implements the PMU and XTALOSC24M
> registers that the i.MX6Q does not.
>
> Add TYPE_IMX6SLL_CCM as a subtype of TYPE_IMX6_CCM.  QOM runs the
> parent's instance_init first, so the subtype only has to override
> DIGPROG and widen the decoded ANALOG window to cover the extra
> registers; the i.MX6Q window is unchanged.

There's no user of this new type. We prefer patches adding new
devices to come in a patchset that includes support for whatever
their user is.

>
> DIGPROG is a hardwired chip identifier rather than a reset value, so
> move its default out of imx6_ccm_reset() and into imx6_ccm_init().
>
> The analog register array grows to cover the new registers.
>
> Signed-off-by: Pablo Mazzini <[email protected]>
> ---
>  hw/misc/imx6_ccm.c         | 39 ++++++++++++++++++++++++++++++++++++--
>  include/hw/misc/imx6_ccm.h | 12 +++++++++++-
>  2 files changed, 48 insertions(+), 3 deletions(-)
>
> diff --git a/hw/misc/imx6_ccm.c b/hw/misc/imx6_ccm.c
> index 45fdd0d5a8..6f48de5954 100644
> --- a/hw/misc/imx6_ccm.c
> +++ b/hw/misc/imx6_ccm.c
> @@ -223,6 +223,14 @@ static const char *imx6_analog_reg_name(uint32_t reg)
>          return "PMU_MISC1_TOG";
>      case USB_ANALOG_DIGPROG:
>          return "USB_ANALOG_DIGPROG";
> +    case PMU_LOW_PWR_CTRL:
> +        return "PMU_LOW_PWR_CTRL";
> +    case XTALOSC24M_OSC_CONFIG0:
> +        return "XTALOSC24M_OSC_CONFIG0";
> +    case XTALOSC24M_OSC_CONFIG1:
> +        return "XTALOSC24M_OSC_CONFIG1";
> +    case XTALOSC24M_OSC_CONFIG2:
> +        return "XTALOSC24M_OSC_CONFIG2";
>      default:
>          snprintf(unknown, sizeof(unknown), "%u ?", reg);
>          return unknown;
> @@ -237,7 +245,7 @@ static const VMStateDescription vmstate_imx6_ccm = {
>      .minimum_version_id = 1,
>      .fields = (const VMStateField[]) {
>          VMSTATE_UINT32_ARRAY(ccm, IMX6CCMState, CCM_MAX),
> -        VMSTATE_UINT32_ARRAY(analog, IMX6CCMState, CCM_ANALOG_MAX),
> +        VMSTATE_UINT32_ARRAY(analog, IMX6CCMState, CCM_ANALOG_REGS),
>          VMSTATE_END_OF_LIST()
>      },
>  };

Adding new elements to a vmstate is a migration bump, and at
minimum needs the version ID numbers to be incremented.

In this case I would favour having the new register state
in a vmstate subsection so that we only migrate it for the new
device type, and the existing device's migration info doesn't change.

> @@ -474,7 +482,6 @@ static void imx6_ccm_reset(DeviceState *dev)
>      s->analog[USB_ANALOG_USB2_VBUS_DETECT] = 0x00000004;
>      s->analog[USB_ANALOG_USB2_CHRG_DETECT] = 0x00000000;
>      s->analog[USB_ANALOG_USB2_MISC] = 0x00000002;
> -    s->analog[USB_ANALOG_DIGPROG] = 0x00630000;
>
>      /* all PLLs need to be locked */
>      s->analog[CCM_ANALOG_PLL_ARM]   |= CCM_ANALOG_PLL_LOCK;
> @@ -763,6 +770,12 @@ static void imx6_ccm_init(Object *obj)
>      memory_region_add_subregion(&s->container, 0x4000, &s->ioanalog);
>
>      sysbus_init_mmio(sd, &s->container);
> +
> +    /*
> +     * DIGPROG is a hardwired chip identifier: it is not affected by reset.
> +     * Default to i.MX6Q; variants sharing this CCM override it below.
> +     */
> +    s->analog[USB_ANALOG_DIGPROG] = 0x00630000;
>  }
>
>  static void imx6_ccm_class_init(ObjectClass *klass, const void *data)
> @@ -777,6 +790,21 @@ static void imx6_ccm_class_init(ObjectClass *klass, const void *data)
>      ccm->get_clock_frequency = imx6_ccm_get_clock_frequency;
>  }
>
> +static void imx6sll_ccm_init(Object *obj)
> +{
> +    IMX6CCMState *s = IMX6_CCM(obj);
> +
> +    /*
> +     * Runs after imx6_ccm_init(), which installs the i.MX6Q default.  The
> +     * clock tree is the same; only the chip identifier differs:
> +     * MXC_CPU_IMX6SLL (0x67), revision 1.0.
> +     */
> +    s->analog[USB_ANALOG_DIGPROG] = 0x00670000;
> +
> +    /* Unlike the i.MX6Q, this one implements the PMU/XTALOSC24M registers. */
> +    memory_region_set_size(&s->ioanalog, CCM_ANALOG_REGS * sizeof(uint32_t));

memory_region_set_size() is really for MRs that dynamically change size,
not for fixing up a size that we could have got right in the first place.

I recommend that you add a class struct to the TYPE_IMX6_CCM, and give
that struct suitable fields that define how the subclasses differ (DIGPROG
reset value, whether XTALOSC24M registers are present, etc). In the class_init
function for each subclass you set those fields in the class struct. Then in
instance_init you can look at the class struct to see how big you should
make the MemoryRegion, and in reset you can look to see what reset value to
use, and so on.

hw/misc/npcm_gcr.c is an example of this approach.

thanks
-- PMM